Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/bundles/lifecycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
2 changes: 1 addition & 1 deletion src/registries/projection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down
6 changes: 2 additions & 4 deletions src/tools/connector-tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
44 changes: 1 addition & 43 deletions src/tools/workspace-oauth-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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<string, string> | 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:
Expand Down
52 changes: 52 additions & 0 deletions src/util/oauth-params.ts
Original file line number Diff line number Diff line change
@@ -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<string, string> | 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(", ")}`,
);
}
}
41 changes: 41 additions & 0 deletions test/unit/oauth-params.test.ts
Original file line number Diff line number Diff line change
@@ -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/);
});
});