diff --git a/src/bundles/lifecycle.ts b/src/bundles/lifecycle.ts index f9990d35..7a00ea0d 100644 --- a/src/bundles/lifecycle.ts +++ b/src/bundles/lifecycle.ts @@ -9,10 +9,8 @@ import { FileCredentialStore } from "../tools/credential-store.ts"; import { McpSource } from "../tools/mcp-source.ts"; import type { ToolRegistry } from "../tools/registry.ts"; import { UserPoolSource } from "../tools/user-pool-source.ts"; -import { - validateAdditionalAuthorizationParams, - WorkspaceOAuthProvider, -} from "../tools/workspace-oauth-provider.ts"; +import { WorkspaceOAuthProvider } from "../tools/workspace-oauth-provider.ts"; +import { validateAdditionalAuthorizationParams } from "../util/oauth-params.ts"; import { WorkspaceContext } from "../workspace/context.ts"; import type { AutomationDomainContext } from "./automations/src/domain.ts"; import { createAutomation, deleteAutomation } from "./automations/src/domain.ts"; diff --git a/src/registries/projection.ts b/src/registries/projection.ts index 9c457316..54e8c1b6 100644 --- a/src/registries/projection.ts +++ b/src/registries/projection.ts @@ -17,7 +17,7 @@ */ import { getNimbleBrainConnectorMeta, type ServerDetail } from "../connectors/server-detail.ts"; -import { validateAdditionalAuthorizationParams } from "../tools/workspace-oauth-provider.ts"; +import { validateAdditionalAuthorizationParams } from "../util/oauth-params.ts"; import { isHttpUrl } from "../util/url.ts"; import type { DirectoryEntry, RegistryType } from "./types.ts"; diff --git a/src/tools/connector-tools.ts b/src/tools/connector-tools.ts index ea396ebe..bb826298 100644 --- a/src/tools/connector-tools.ts +++ b/src/tools/connector-tools.ts @@ -15,13 +15,11 @@ import type { UserIdentity } from "../identity/provider.ts"; import type { ConnectorCatalogEntry } from "../registries/projection.ts"; import type { DirectoryEntry } from "../registries/types.ts"; import type { Runtime } from "../runtime/runtime.ts"; +import { validateAdditionalAuthorizationParams } from "../util/oauth-params.ts"; import { isHttpUrl } from "../util/url.ts"; import { FileCredentialStore } from "./credential-store.ts"; import type { InProcessTool } from "./in-process-app.ts"; -import { - validateAdditionalAuthorizationParams, - WorkspaceOAuthProvider, -} from "./workspace-oauth-provider.ts"; +import { WorkspaceOAuthProvider } from "./workspace-oauth-provider.ts"; /** * `manage_connectors` tool — single surface for the Connectors UI diff --git a/src/tools/workspace-oauth-provider.ts b/src/tools/workspace-oauth-provider.ts index 20f4f191..50196229 100644 --- a/src/tools/workspace-oauth-provider.ts +++ b/src/tools/workspace-oauth-provider.ts @@ -14,6 +14,7 @@ import type { } from "@modelcontextprotocol/sdk/shared/auth.js"; import { validateBundleUrl } from "../bundles/url-validator.ts"; import { log } from "../cli/log.ts"; +import { validateAdditionalAuthorizationParams } from "../util/oauth-params.ts"; import type { WorkspaceContext } from "../workspace/context.ts"; import { register as registerInteractiveFlow } from "./oauth-flow-registry.ts"; @@ -167,49 +168,6 @@ export interface WorkspaceOAuthProviderOptions { abortSignal?: AbortSignal; } -/** - * Reserved authorize-URL params that the OAuth flow controls itself. - * Operator-supplied `additionalAuthorizationParams` from - * `workspace.json` MUST NOT include these — overriding any of them - * would let a misconfigured catalog entry break PKCE binding or steal - * the redirect target. Validated at config load (see - * `validateAdditionalAuthorizationParams`). - */ -export const RESERVED_AUTHORIZE_PARAMS = [ - "client_id", - "redirect_uri", - "response_type", - "state", - "code_challenge", - "code_challenge_method", - "scope", - // OIDC-style hijack vectors: `request` / `request_uri` smuggle a - // signed/unsigned JWT request object that can override every other - // parameter; `response_mode` can change response delivery (form_post, - // fragment) in ways that break our callback assumptions. - "request", - "request_uri", - "response_mode", -] as const; - -/** - * Throw if any reserved key appears in the params map. Called at the - * boundary where `workspace.json` is parsed — bundle install / - * `seedInstance` — so a bad config fails loud rather than at OAuth- - * flow time. - */ -export function validateAdditionalAuthorizationParams( - params: Record | undefined, -): void { - if (!params) return; - const reserved = RESERVED_AUTHORIZE_PARAMS.filter((k) => k in params); - if (reserved.length > 0) { - throw new Error( - `[workspace-oauth-provider] additionalAuthorizationParams cannot include reserved keys: ${reserved.join(", ")}`, - ); - } -} - /** * Normalize a callback URL to a `{origin, pathname}` canonical form so the * self-match check tolerates trivial differences a strict `===` would miss: diff --git a/src/util/oauth-params.ts b/src/util/oauth-params.ts new file mode 100644 index 00000000..74325b4e --- /dev/null +++ b/src/util/oauth-params.ts @@ -0,0 +1,52 @@ +/** + * Pure validation for operator-supplied `additionalAuthorizationParams`. + * + * Kept free of the `WorkspaceOAuthProvider` module's dependency surface + * (MCP SDK auth, the OAuth flow registry, `validateBundleUrl`, crypto/fs) + * so the registry / config-load layers can run the reserved-key check + * without dragging the full provider in. Same rationale as `url.ts` — + * pure helpers live in `src/util/`. + */ + +/** + * Reserved authorize-URL params that the OAuth flow controls itself. + * Operator-supplied `additionalAuthorizationParams` from + * `workspace.json` MUST NOT include these — overriding any of them + * would let a misconfigured catalog entry break PKCE binding or steal + * the redirect target. Validated at config load (see + * `validateAdditionalAuthorizationParams`). + */ +export const RESERVED_AUTHORIZE_PARAMS = [ + "client_id", + "redirect_uri", + "response_type", + "state", + "code_challenge", + "code_challenge_method", + "scope", + // OIDC-style hijack vectors: `request` / `request_uri` smuggle a + // signed/unsigned JWT request object that can override every other + // parameter; `response_mode` can change response delivery (form_post, + // fragment) in ways that break our callback assumptions. + "request", + "request_uri", + "response_mode", +] as const; + +/** + * Throw if any reserved key appears in the params map. Called at the + * boundary where `workspace.json` is parsed — bundle install / + * `seedInstance` — so a bad config fails loud rather than at OAuth- + * flow time. + */ +export function validateAdditionalAuthorizationParams( + params: Record | undefined, +): void { + if (!params) return; + const reserved = RESERVED_AUTHORIZE_PARAMS.filter((k) => k in params); + if (reserved.length > 0) { + throw new Error( + `[oauth-params] additionalAuthorizationParams cannot include reserved keys: ${reserved.join(", ")}`, + ); + } +} diff --git a/test/unit/oauth-params.test.ts b/test/unit/oauth-params.test.ts new file mode 100644 index 00000000..a5148ff8 --- /dev/null +++ b/test/unit/oauth-params.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, test } from "bun:test"; +import { + RESERVED_AUTHORIZE_PARAMS, + validateAdditionalAuthorizationParams, +} from "../../src/util/oauth-params.ts"; + +describe("validateAdditionalAuthorizationParams", () => { + test("no-op for undefined", () => { + expect(() => validateAdditionalAuthorizationParams(undefined)).not.toThrow(); + }); + + test("no-op for empty map", () => { + expect(() => validateAdditionalAuthorizationParams({})).not.toThrow(); + }); + + test("allows non-reserved keys", () => { + expect(() => + validateAdditionalAuthorizationParams({ access_type: "offline", prompt: "consent" }), + ).not.toThrow(); + }); + + test("throws on each reserved key", () => { + for (const key of RESERVED_AUTHORIZE_PARAMS) { + expect(() => validateAdditionalAuthorizationParams({ [key]: "x" })).toThrow( + /reserved keys/, + ); + } + }); + + test("throws on OIDC hijack vectors", () => { + for (const key of ["request", "request_uri", "response_mode"]) { + expect(() => validateAdditionalAuthorizationParams({ [key]: "x" })).toThrow(); + } + }); + + test("error message names every offending key", () => { + expect(() => + validateAdditionalAuthorizationParams({ client_id: "a", state: "b", access_type: "c" }), + ).toThrow(/client_id, state/); + }); +});