From 0a4ac04aa7f4375af1aaaa389cf67e3c4d8ced56 Mon Sep 17 00:00:00 2001 From: credence-the-bot Date: Mon, 11 May 2026 03:45:24 +0000 Subject: [PATCH] fix(oauth): hoist manual-token + requiresClientSecret + app-not-found validations to apply uniformly Three follow-up fixes from review of #30251: - CLI: provider serializer emits `authUrl` (camelCase) but `connect.ts` was reading `auth_url`, making the manual-token guard dead code for BYO-mode providers like slack_channel and telegram. - Daemon: `handleOAuthConnectStart` previously ran the manual-token check, app-not-found error, and `requiresClientSecret` check only inside `if (!clientId)`. Callers passing `--client-id` bypassed all three. Hoisted them above the branch split so they apply uniformly, and added the missing app-not-found `NotFoundError` to the explicit-clientId branch. - Cleanup: removed dead `PlatformConnectionEntry` export from `cli/commands/oauth/shared.ts` (zero importers in the repo). Adds 7 new tests in `oauth-connect-routes.test.ts` covering every validation path the refactor touches. Existing tests now mock `oauth-store` so they no longer depend on an unseeded DB returning null. --- .../__tests__/oauth-connect-routes.test.ts | 185 ++++++++++++++++-- assistant/src/cli/commands/oauth/connect.ts | 2 +- assistant/src/cli/commands/oauth/shared.ts | 11 -- .../runtime/routes/oauth-connect-routes.ts | 59 +++--- 4 files changed, 208 insertions(+), 49 deletions(-) diff --git a/assistant/src/__tests__/oauth-connect-routes.test.ts b/assistant/src/__tests__/oauth-connect-routes.test.ts index 89dd8bcd2ac..eb0bd56493b 100644 --- a/assistant/src/__tests__/oauth-connect-routes.test.ts +++ b/assistant/src/__tests__/oauth-connect-routes.test.ts @@ -41,6 +41,49 @@ mock.module("../util/logger.js", () => ({ }), })); +// ── Mock oauth-store so handleOAuthConnectStart can run without a seeded DB ── +type MockProviderRow = { + authorizeUrl?: string; + requiresClientSecret?: boolean; + [key: string]: unknown; +}; + +const DEFAULT_PROVIDER: MockProviderRow = { + authorizeUrl: "https://accounts.google.com/o/oauth2/auth", + requiresClientSecret: false, +}; + +let mockGetProvider: (service: string) => MockProviderRow | undefined = () => + DEFAULT_PROVIDER; +let mockGetAppByProviderAndClientId: ( + service: string, + clientId: string, +) => Record | undefined = (_service, clientId) => ({ + id: "test-app-id", + provider: "google", + clientId, +}); +let mockGetMostRecentAppByProvider: ( + service: string, +) => Record | undefined = () => ({ + id: "test-app-id", + provider: "google", + clientId: "default-client-id", +}); +let mockGetAppClientSecret: ( + row: Record, +) => Promise = async () => undefined; + +mock.module("../oauth/oauth-store.js", () => ({ + getProvider: (service: string) => mockGetProvider(service), + getAppByProviderAndClientId: (service: string, clientId: string) => + mockGetAppByProviderAndClientId(service, clientId), + getMostRecentAppByProvider: (service: string) => + mockGetMostRecentAppByProvider(service), + getAppClientSecret: (row: Record) => + mockGetAppClientSecret(row), +})); + // NOTE: Do NOT mock oauth-connect-state — use the real module so we can // verify state transitions via getOAuthConnectState. @@ -65,6 +108,23 @@ function findRoute(operationId: string) { // ── Tests ────────────────────────────────────────────────────────────────────── describe("oauth-connect-routes", () => { + beforeEach(() => { + // Reset oauth-store mocks to permissive defaults so every test starts + // from a clean baseline regardless of what the previous test set. + mockGetProvider = () => DEFAULT_PROVIDER; + mockGetAppByProviderAndClientId = (_service, clientId) => ({ + id: "test-app-id", + provider: "google", + clientId, + }); + mockGetMostRecentAppByProvider = () => ({ + id: "test-app-id", + provider: "google", + clientId: "default-client-id", + }); + mockGetAppClientSecret = async () => undefined; + }); + describe("POST internal/oauth/connect/start", () => { beforeEach(() => { capturedOnDeferredComplete = undefined; @@ -109,17 +169,6 @@ describe("oauth-connect-routes", () => { ).rejects.toBeInstanceOf(BadRequestError); }); - test("missing clientId throws BadRequestError", async () => { - await expect( - findRoute("internal_oauth_connect_start").handler({ - body: { - service: "google", - callbackTransport: "gateway", - }, - }), - ).rejects.toBeInstanceOf(BadRequestError); - }); - test("missing service throws BadRequestError", async () => { await expect( findRoute("internal_oauth_connect_start").handler({ @@ -183,6 +232,120 @@ describe("oauth-connect-routes", () => { }), ).rejects.toBeInstanceOf(InternalError); }); + + test("unknown provider throws NotFoundError", async () => { + mockGetProvider = () => undefined; + await expect( + findRoute("internal_oauth_connect_start").handler({ + body: { + service: "made-up-service", + clientId: "my-client-id", + callbackTransport: "gateway", + }, + }), + ).rejects.toBeInstanceOf(NotFoundError); + }); + + test("manual-token provider rejected even when clientId is explicitly supplied", async () => { + // Codex/Devin finding on PR #30251: the manual-token check used to live + // inside `if (!clientId)`, so callers passing `--client-id` for a + // manual-token provider (e.g. slack_channel, telegram) bypassed it. + mockGetProvider = () => ({ + authorizeUrl: "urn:manual-token", + requiresClientSecret: false, + }); + await expect( + findRoute("internal_oauth_connect_start").handler({ + body: { + service: "slack_channel", + clientId: "any-client-id", + callbackTransport: "gateway", + }, + }), + ).rejects.toBeInstanceOf(BadRequestError); + }); + + test("manual-token provider rejected when no clientId is supplied", async () => { + mockGetProvider = () => ({ + authorizeUrl: "urn:manual-token", + requiresClientSecret: false, + }); + await expect( + findRoute("internal_oauth_connect_start").handler({ + body: { + service: "slack_channel", + callbackTransport: "gateway", + }, + }), + ).rejects.toBeInstanceOf(BadRequestError); + }); + + test("explicit clientId with no registered app throws NotFoundError", async () => { + // Devin finding on PR #30251: the explicit-clientId branch used to + // silently continue when getAppByProviderAndClientId returned null, + // letting orchestrateOAuthConnect fail later with a less helpful error. + mockGetAppByProviderAndClientId = () => undefined; + await expect( + findRoute("internal_oauth_connect_start").handler({ + body: { + service: "google", + clientId: "unregistered-client-id", + callbackTransport: "gateway", + }, + }), + ).rejects.toBeInstanceOf(NotFoundError); + }); + + test("no clientId and no registered app throws BadRequestError", async () => { + mockGetMostRecentAppByProvider = () => undefined; + await expect( + findRoute("internal_oauth_connect_start").handler({ + body: { + service: "google", + callbackTransport: "gateway", + }, + }), + ).rejects.toBeInstanceOf(BadRequestError); + }); + + test("missing client_secret when provider requiresClientSecret throws BadRequestError", async () => { + // Devin finding on PR #30251: requiresClientSecret was only checked + // inside the `!clientId` branch. Hoisting it ensures explicit-clientId + // callers also fail fast with an actionable error. + mockGetProvider = () => ({ + authorizeUrl: "https://accounts.example.com/o/oauth2/auth", + requiresClientSecret: true, + }); + mockGetAppClientSecret = async () => undefined; + await expect( + findRoute("internal_oauth_connect_start").handler({ + body: { + service: "github", + clientId: "explicit-client-id", + callbackTransport: "gateway", + }, + }), + ).rejects.toBeInstanceOf(BadRequestError); + }); + + test("explicit clientId + stored secret succeeds without caller-supplied secret", async () => { + mockGetProvider = () => ({ + authorizeUrl: "https://accounts.example.com/o/oauth2/auth", + requiresClientSecret: true, + }); + mockGetAppClientSecret = async () => "stored-secret-from-keyring"; + const result = await findRoute("internal_oauth_connect_start").handler({ + body: { + service: "github", + clientId: "explicit-client-id", + callbackTransport: "gateway", + }, + }); + expect(result).toMatchObject({ + auth_url: "https://accounts.google.com/o/oauth2/auth?client_id=test", + state: "test-state-uuid-abc123", + }); + }); }); describe("GET internal/oauth/connect/status/:state", () => { diff --git a/assistant/src/cli/commands/oauth/connect.ts b/assistant/src/cli/commands/oauth/connect.ts index 01de20cb760..121234e4caf 100644 --- a/assistant/src/cli/commands/oauth/connect.ts +++ b/assistant/src/cli/commands/oauth/connect.ts @@ -144,7 +144,7 @@ Examples: const providerRow = providerCheck.result?.provider as | Record | undefined; - const authorizeUrl = providerRow?.auth_url as string | undefined; + const authorizeUrl = providerRow?.authUrl as string | undefined; // --------------------------------------------------------------- // 2. Detect mode via IPC diff --git a/assistant/src/cli/commands/oauth/shared.ts b/assistant/src/cli/commands/oauth/shared.ts index b8eba3317f6..ee596d304a8 100644 --- a/assistant/src/cli/commands/oauth/shared.ts +++ b/assistant/src/cli/commands/oauth/shared.ts @@ -10,17 +10,6 @@ import type { Command } from "commander"; import { VellumPlatformClient } from "../../../platform/client.js"; import { writeOutput } from "../../output.js"; -// --------------------------------------------------------------------------- -// Shared types -// --------------------------------------------------------------------------- - -export interface PlatformConnectionEntry { - id: string; - account_label?: string; - scopes_granted?: string[]; - status?: string; -} - // --------------------------------------------------------------------------- // Platform connection helpers // --------------------------------------------------------------------------- diff --git a/assistant/src/runtime/routes/oauth-connect-routes.ts b/assistant/src/runtime/routes/oauth-connect-routes.ts index 53f420e1e99..3a3d1bc4c68 100644 --- a/assistant/src/runtime/routes/oauth-connect-routes.ts +++ b/assistant/src/runtime/routes/oauth-connect-routes.ts @@ -47,26 +47,28 @@ async function handleOAuthConnectStart({ if (!service) throw new BadRequestError("service is required"); - // When clientId is not provided, resolve from the DB + // Provider row drives validation that applies regardless of whether the + // caller supplied an explicit clientId: existence, manual-token rejection, + // and the requiresClientSecret check below. + const providerRow = getProvider(service); + if (!providerRow) { + throw new NotFoundError( + `Unknown provider "${service}". Run 'assistant oauth providers list' to see available providers.`, + ); + } + + // Manual-token providers don't use OAuth2 browser flows. + if (providerRow.authorizeUrl === "urn:manual-token") { + throw new BadRequestError( + `"${service}" uses manual token configuration, not an OAuth browser flow. ` + + `Set the token with: assistant credentials set --service ${service} --field `, + ); + } + let clientId = rawClientId; let clientSecret = rawClientSecret; if (!clientId) { - const providerRow = getProvider(service); - if (!providerRow) { - throw new NotFoundError( - `Unknown provider "${service}". Run 'assistant oauth providers list' to see available providers.`, - ); - } - - // Manual-token providers don't use OAuth2 browser flows - if (providerRow.authorizeUrl === "urn:manual-token") { - throw new BadRequestError( - `"${service}" uses manual token configuration, not an OAuth browser flow. ` + - `Set the token with: assistant credentials set --service ${service} --field `, - ); - } - const dbApp = getMostRecentAppByProvider(service); if (!dbApp) { throw new BadRequestError( @@ -79,23 +81,28 @@ async function handleOAuthConnectStart({ const storedSecret = await getAppClientSecret(dbApp); if (storedSecret) clientSecret = storedSecret; } - - // Check if client_secret is required but missing - if (clientSecret === undefined && providerRow.requiresClientSecret) { - throw new BadRequestError( - `client_secret is required for ${service} but not found. ` + - `Store it with 'assistant oauth apps upsert --client-secret'.`, + } else { + // clientId was explicitly provided — resolve its app. + const dbApp = getAppByProviderAndClientId(service, clientId); + if (!dbApp) { + throw new NotFoundError( + `No registered app found for "${service}" with client_id "${clientId}". ` + + `Register one with 'assistant oauth apps upsert'.`, ); } - } else if (clientId) { - // clientId was explicitly provided — resolve its app - const dbApp = getAppByProviderAndClientId(service, clientId); - if (dbApp && clientSecret === undefined) { + if (clientSecret === undefined) { const storedSecret = await getAppClientSecret(dbApp); if (storedSecret) clientSecret = storedSecret; } } + if (clientSecret === undefined && providerRow.requiresClientSecret) { + throw new BadRequestError( + `client_secret is required for ${service} but not found. ` + + `Store it with 'assistant oauth apps upsert --client-secret'.`, + ); + } + if (callbackTransport !== "loopback" && callbackTransport !== "gateway") { throw new BadRequestError( 'callbackTransport must be "loopback" or "gateway"',