From 82e99f38ebe21a6230e6f8ae860e6d515af6081b Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Thu, 23 Apr 2026 10:27:31 -1000 Subject: [PATCH 1/4] Add OAuth client for outbound remote MCP sources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the gap where workspace bundles with `{ url }` refs against OAuth-gated remote MCP servers (Reboot under `rbt dev`, ChatGPT Apps, hosted Claude.ai MCP endpoints) returned `401 invalid_token` and failed to start. The MCP TS SDK already supports the OAuth client flow via `OAuthClientProvider`; we just weren't passing one. - WorkspaceOAuthProvider: file-backed `OAuthClientProvider` scoped per `(workspace, serverName)`. Persists DCR client info, tokens, and PKCE verifier under `.nimblebrain/workspaces//credentials/ mcp-oauth//` (mode 0o700). Follows the authorize-redirect chain up to 10 hops and detects when the chain lands at our own callback — Reboot's `Anonymous` dev flow completes headlessly this way. Non-self-target flows throw `InteractiveOAuthNotSupportedError`, staging real browser OAuth for a follow-up. - oauth-flow-registry: process-local `Map` bridging the provider to the HTTP callback route. Extension point for the interactive flow. - /v1/mcp-auth/callback route: `GET` handler that resolves pending flows by state. Unauthenticated by design (state param guards against unsolicited codes). - createRemoteTransport: optional `authProvider` parameter; static `transport.auth` takes precedence. - McpSource.start(): retry exactly once on `UnauthorizedError` after awaiting the provider's pending flow and calling `transport.finishAuth(code)`. Extracted helpers (connectWithTimeout, cleanupOnStartFailure) keep the retry readable. - startBundleSource: instantiate WorkspaceOAuthProvider for url-ref bundles without static auth; threads `opts.wsId` + `opts.workDir` with sensible defaults. Tests: 10 unit tests for the provider (persistence roundtrips, headless self-target detection via multi-hop redirect, interactive rejection) plus 4 for the flow registry. --- src/api/app.ts | 6 + src/api/routes/mcp-auth.ts | 80 +++++ src/bundles/startup.ts | 24 ++ src/tools/mcp-source.ts | 96 ++++-- src/tools/oauth-flow-registry.ts | 57 ++++ src/tools/remote-transport.ts | 24 +- src/tools/workspace-oauth-provider.ts | 333 +++++++++++++++++++++ test/unit/oauth-flow-registry.test.ts | 38 +++ test/unit/workspace-oauth-provider.test.ts | 244 +++++++++++++++ 9 files changed, 881 insertions(+), 21 deletions(-) create mode 100644 src/api/routes/mcp-auth.ts create mode 100644 src/tools/oauth-flow-registry.ts create mode 100644 src/tools/workspace-oauth-provider.ts create mode 100644 test/unit/oauth-flow-registry.test.ts create mode 100644 test/unit/workspace-oauth-provider.test.ts diff --git a/src/api/app.ts b/src/api/app.ts index 4be0ad3..789ce95 100644 --- a/src/api/app.ts +++ b/src/api/app.ts @@ -9,6 +9,7 @@ import { conversationEventRoutes } from "./routes/conversation-events.ts"; import { eventRoutes } from "./routes/events.ts"; import { healthRoutes } from "./routes/health.ts"; import { mcpRoutes } from "./routes/mcp.ts"; +import { mcpAuthRoutes } from "./routes/mcp-auth.ts"; import { resourceRoutes } from "./routes/resources.ts"; import { toolRoutes } from "./routes/tools.ts"; import { wellKnownRoutes } from "./routes/well-known.ts"; @@ -29,6 +30,11 @@ export function createApp( app.route("/", wellKnownRoutes(ctx)); app.route("/", healthRoutes(ctx)); app.route("/", authRoutes(ctx)); + // Outbound-OAuth callback for remote MCP servers. Unauthenticated by + // design — state param guards against unsolicited codes. Must be + // reachable before any authenticated middleware; ordering alongside + // authRoutes keeps that invariant obvious. + app.route("/", mcpAuthRoutes(ctx)); // MCP routes BEFORE other authenticated routes — prevents other sub-app // wildcard middleware from intercepting /mcp requests. Hono runs use("*") diff --git a/src/api/routes/mcp-auth.ts b/src/api/routes/mcp-auth.ts new file mode 100644 index 0000000..0e702b9 --- /dev/null +++ b/src/api/routes/mcp-auth.ts @@ -0,0 +1,80 @@ +import { Hono } from "hono"; +import { resolveWithCode } from "../../tools/oauth-flow-registry.ts"; +import type { AppContext } from "../types.ts"; + +/** + * Callback endpoint for outbound OAuth flows where NimbleBrain is acting as + * the client against a remote MCP server's authorization server. Pairs with + * `WorkspaceOAuthProvider`: when the provider's flow requires a real browser + * round-trip, the remote authorization server redirects the user's browser + * here with `?code=&state=`. + * + * The route looks up the pending flow by `state` via the process-local + * `oauth-flow-registry`, resolves it with the code, and shows a minimal + * "done" page so the user can close the tab. + * + * Unauthenticated by design — it's the return leg of an OAuth flow the user + * explicitly initiated by adding a remote bundle. State param prevents + * unsolicited code injection; unknown states 400 cleanly. + * + * MVP note: Reboot's `Anonymous` dev OAuth is handled entirely inside + * `WorkspaceOAuthProvider.redirectToAuthorization` (headless) — the + * authorization URL self-targets this route with the code already embedded, + * and the provider resolves the deferred in-process without making an HTTP + * request. This route is kept in place as the extension point for real + * interactive providers (follow-up iteration). + */ +export function mcpAuthRoutes(_ctx: AppContext) { + const app = new Hono(); + + app.get("/v1/mcp-auth/callback", (c) => { + const code = c.req.query("code"); + const state = c.req.query("state"); + const error = c.req.query("error"); + + if (error) { + return c.html( + `

Authorization failed

${escapeHtml(error)}
`, + 400, + ); + } + if (!code || !state) { + return c.text("missing code or state", 400); + } + + const matched = resolveWithCode(state, code); + if (!matched) { + return c.html( + "

Unknown or expired OAuth flow.

" + + "

Re-initiate the connection from NimbleBrain.

", + 404, + ); + } + + return c.html( + "

Authorization complete.

" + + "

You can close this tab and return to NimbleBrain.

", + ); + }); + + return app; +} + +function escapeHtml(s: string): string { + return s.replace(/[&<>"']/g, (ch) => { + switch (ch) { + case "&": + return "&"; + case "<": + return "<"; + case ">": + return ">"; + case '"': + return """; + case "'": + return "'"; + default: + return ch; + } + }); +} diff --git a/src/bundles/startup.ts b/src/bundles/startup.ts index 4717cd0..4170c0f 100644 --- a/src/bundles/startup.ts +++ b/src/bundles/startup.ts @@ -11,6 +11,7 @@ import type { EventSink } from "../engine/types.ts"; import { McpSource } from "../tools/mcp-source.ts"; import type { ToolRegistry } from "../tools/registry.ts"; import type { ToolSource } from "../tools/types.ts"; +import { WorkspaceOAuthProvider } from "../tools/workspace-oauth-provider.ts"; import { extractBundleMeta } from "./defaults.ts"; import { filterEnvForBundle } from "./env-filter.ts"; import { validateManifest } from "./manifest.ts"; @@ -70,12 +71,35 @@ export async function startBundleSource( // SSRF protection: validate URL before connecting validateBundleUrl(new URL(ref.url), { allowInsecure: opts?.allowInsecureRemotes }); log.info(`[bundles] Starting remote bundle ${ref.url} as ${sourceName}...`); + + // Attach an OAuthClientProvider when no static auth is configured. The + // provider is workspace-scoped: tokens and DCR credentials live under + // /workspaces//credentials/mcp-oauth//. Named + // bundles already require `wsId`; URL bundles historically did not — + // fall back to `ws_default` only if wsId is missing, which preserves + // behavior for any callers that haven't threaded workspace context. + let authProvider: WorkspaceOAuthProvider | undefined; + const hasStaticAuth = ref.transport?.auth && ref.transport.auth.type !== "none"; + if (!hasStaticAuth) { + const wsId = opts?.wsId ?? "ws_default"; + const workDir = opts?.workDir ?? process.env.NB_WORK_DIR ?? join(homedir(), ".nimblebrain"); + const apiBase = process.env.NB_API_URL ?? "http://localhost:27247"; + const callbackUrl = `${apiBase.replace(/\/+$/, "")}/v1/mcp-auth/callback`; + authProvider = new WorkspaceOAuthProvider({ + wsId, + serverName, + workDir, + callbackUrl, + }); + } + const source = new McpSource( sourceName, { type: "remote", url: new URL(ref.url), transportConfig: ref.transport, + authProvider, }, eventSink, ); diff --git a/src/tools/mcp-source.ts b/src/tools/mcp-source.ts index 81d07ef..583f605 100644 --- a/src/tools/mcp-source.ts +++ b/src/tools/mcp-source.ts @@ -1,5 +1,7 @@ +import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; +import type { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"; import type { RemoteTransportConfig } from "../bundles/types.ts"; import { log } from "../cli/log.ts"; @@ -7,6 +9,7 @@ import { textContent } from "../engine/content-helpers.ts"; import type { ContentBlock, EventSink, ToolResult } from "../engine/types.ts"; import { createRemoteTransport } from "./remote-transport.ts"; import type { ResourceData, Tool, ToolSource } from "./types.ts"; +import type { WorkspaceOAuthProvider } from "./workspace-oauth-provider.ts"; /** * Default time-to-live (ms) sent with task-augmented `tools/call` requests. @@ -27,7 +30,20 @@ export interface McpSpawnConfig { /** Discriminated union for how McpSource connects to its MCP server. */ export type McpTransportMode = | { type: "stdio"; spawn: McpSpawnConfig } - | { type: "remote"; url: URL; transportConfig?: RemoteTransportConfig }; + | { + type: "remote"; + url: URL; + transportConfig?: RemoteTransportConfig; + /** + * Optional OAuth provider for the MCP SDK. When set and no static + * `transportConfig.auth` is present, `createRemoteTransport` attaches + * it to the client transport. If the server returns 401 on connect, + * `start()` catches `UnauthorizedError`, awaits the provider's pending + * flow for an authorization code, calls `transport.finishAuth`, and + * retries `connect()` exactly once. + */ + authProvider?: WorkspaceOAuthProvider; + }; /** * ToolSource wrapping a single MCP server (stdio subprocess or remote HTTP/SSE). @@ -77,7 +93,11 @@ export class McpSource implements ToolSource { stderr: "pipe", }); } else { - this.transport = createRemoteTransport(this.mode.url, this.mode.transportConfig); + this.transport = createRemoteTransport( + this.mode.url, + this.mode.transportConfig, + this.mode.authProvider, + ); // Remote: watch for transport close — mark source as dead this.transport.onclose = () => { @@ -109,27 +129,42 @@ export class McpSource implements ToolSource { // Timeout MCP handshake — remote gets shorter timeout (15s vs 30s) const CONNECT_TIMEOUT = this.mode.type === "remote" ? 15_000 : 30_000; - const timeout = new Promise((_, reject) => - setTimeout( - () => - reject( - new Error(`MCP connect timeout after ${CONNECT_TIMEOUT / 1000}s for ${this.name}`), - ), - CONNECT_TIMEOUT, - ), - ); try { - await Promise.race([this.client.connect(this.transport), timeout]); + await this.connectWithTimeout(CONNECT_TIMEOUT); } catch (err) { - // Clean up on failure - try { - if (this.transport) await this.transport.close(); - } catch (cleanupErr) { - console.error("[mcp-source] transport cleanup failed:", cleanupErr); + // One-shot OAuth retry: if we have an authProvider and the SDK threw + // UnauthorizedError, the provider's pending flow was either resolved + // in-process (headless, e.g. Reboot Anonymous) or rejected with a + // clear error (interactive, which we don't support yet). Await the + // flow, finish auth on the transport, and try exactly once more. + if ( + err instanceof UnauthorizedError && + this.mode.type === "remote" && + this.mode.authProvider && + this.transport + ) { + try { + const code = await this.mode.authProvider.awaitPendingFlow(); + const streamableTransport = this.transport as StreamableHTTPClientTransport; + if (typeof streamableTransport.finishAuth !== "function") { + throw new Error( + `[mcp-source] transport does not support finishAuth (got ${this.transport.constructor.name})`, + ); + } + await streamableTransport.finishAuth(code); + log.debug("mcp", `[oauth] ${this.name}: finishAuth ok, retrying connect`); + await this.connectWithTimeout(CONNECT_TIMEOUT); + this.dead = false; + this.startedAt = Date.now(); + return; + } catch (retryErr) { + await this.cleanupOnStartFailure(); + throw retryErr; + } } - this.client = null; - this.transport = null; + + await this.cleanupOnStartFailure(); throw err; } @@ -137,6 +172,29 @@ export class McpSource implements ToolSource { this.startedAt = Date.now(); } + private async connectWithTimeout(timeoutMs: number): Promise { + if (!this.client || !this.transport) { + throw new Error("[mcp-source] connectWithTimeout called before init"); + } + const timeout = new Promise((_, reject) => + setTimeout( + () => reject(new Error(`MCP connect timeout after ${timeoutMs / 1000}s for ${this.name}`)), + timeoutMs, + ), + ); + await Promise.race([this.client.connect(this.transport), timeout]); + } + + private async cleanupOnStartFailure(): Promise { + try { + if (this.transport) await this.transport.close(); + } catch (cleanupErr) { + console.error("[mcp-source] transport cleanup failed:", cleanupErr); + } + this.client = null; + this.transport = null; + } + /** Check if the transport is still connected. */ isAlive(): boolean { return this.transport !== null && this.client !== null && !this.dead; diff --git a/src/tools/oauth-flow-registry.ts b/src/tools/oauth-flow-registry.ts new file mode 100644 index 0000000..30352c3 --- /dev/null +++ b/src/tools/oauth-flow-registry.ts @@ -0,0 +1,57 @@ +/** + * Process-local registry for pending OAuth authorization flows. + * + * Bridges `WorkspaceOAuthProvider` (initiator) and the + * `/v1/mcp-auth/callback` route (code receiver) when the auth flow requires + * a real browser round-trip. Keyed by the OAuth `state` parameter. + * + * For headless flows (Reboot Anonymous in `rbt dev`), the provider resolves + * its own deferred directly from `redirectToAuthorization` and does not + * register here — the registry is only used when an HTTP callback actually + * arrives from outside the provider's control. + * + * State is not persisted: OAuth flows complete in seconds, and if a process + * restart interrupts one, re-initiating is correct. Cross-process concerns + * don't apply — NimbleBrain runs single-process. + */ + +interface PendingFlow { + resolve: (code: string) => void; + reject: (err: Error) => void; + wsId: string; + serverName: string; +} + +const flows = new Map(); + +export function register(state: string, wsId: string, serverName: string): Promise { + return new Promise((resolve, reject) => { + flows.set(state, { resolve, reject, wsId, serverName }); + }); +} + +/** Resolve a pending flow by state. Returns true if found. */ +export function resolveWithCode(state: string, code: string): boolean { + const flow = flows.get(state); + if (!flow) return false; + flows.delete(state); + flow.resolve(code); + return true; +} + +/** Reject a pending flow by state. Returns true if found. */ +export function rejectFlow(state: string, err: Error): boolean { + const flow = flows.get(state); + if (!flow) return false; + flows.delete(state); + flow.reject(err); + return true; +} + +/** For tests: drop all pending flows. */ +export function _clearAll(): void { + for (const flow of flows.values()) { + flow.reject(new Error("flow registry cleared")); + } + flows.clear(); +} diff --git a/src/tools/remote-transport.ts b/src/tools/remote-transport.ts index 436ef8a..aa7f14a 100644 --- a/src/tools/remote-transport.ts +++ b/src/tools/remote-transport.ts @@ -1,3 +1,4 @@ +import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js"; import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"; @@ -6,8 +7,18 @@ import type { RemoteTransportConfig } from "../bundles/types.ts"; /** * Create a remote MCP transport from a URL and optional config. * Default transport: Streamable HTTP. Use type: "sse" for legacy SSE servers. + * + * Auth precedence: static header auth (`config.auth`) wins if present. An + * `authProvider` is only attached when no static auth is configured — + * servers using API keys in headers don't trigger OAuth flows they might + * not support. When attached, the MCP SDK handles discovery (RFC 9728), + * dynamic client registration (RFC 7591), PKCE, and token refresh. */ -export function createRemoteTransport(url: URL, config?: RemoteTransportConfig): Transport { +export function createRemoteTransport( + url: URL, + config?: RemoteTransportConfig, + authProvider?: OAuthClientProvider, +): Transport { const headers: Record = { ...(config?.headers ?? {}) }; if (config?.auth?.type === "bearer") { @@ -16,14 +27,23 @@ export function createRemoteTransport(url: URL, config?: RemoteTransportConfig): headers[config.auth.name] = config.auth.value; } + // Only wire the OAuth provider if no static auth is configured. Static + // auth is the simpler, explicit contract — don't second-guess it. + const effectiveAuthProvider = + config?.auth && config.auth.type !== "none" ? undefined : authProvider; + const requestInit: RequestInit = Object.keys(headers).length > 0 ? { headers } : {}; if (config?.type === "sse") { - return new SSEClientTransport(url, { requestInit }); + return new SSEClientTransport(url, { + requestInit, + authProvider: effectiveAuthProvider, + }); } return new StreamableHTTPClientTransport(url, { requestInit, + authProvider: effectiveAuthProvider, reconnectionOptions: config?.reconnection ? { maxReconnectionDelay: config.reconnection.maxReconnectionDelay ?? 30_000, diff --git a/src/tools/workspace-oauth-provider.ts b/src/tools/workspace-oauth-provider.ts new file mode 100644 index 0000000..89aac9e --- /dev/null +++ b/src/tools/workspace-oauth-provider.ts @@ -0,0 +1,333 @@ +import { randomBytes } from "node:crypto"; +import { existsSync } from "node:fs"; +import { chmod, mkdir, readFile, unlink, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js"; +import type { + OAuthClientInformationFull, + OAuthClientInformationMixed, + OAuthClientMetadata, + OAuthTokens, +} from "@modelcontextprotocol/sdk/shared/auth.js"; +import { log } from "../cli/log.ts"; + +/** + * Thrown from `redirectToAuthorization` when a remote MCP server requires + * interactive browser-based OAuth. Caught by `McpSource.start()` and surfaced + * as a clear startup failure. Resolving this properly is a follow-up; for + * now it fails fast rather than hanging on a flow that can't complete + * headlessly. + */ +export class InteractiveOAuthNotSupportedError extends Error { + constructor(public readonly authorizationUrl: string) { + super( + `Interactive OAuth not yet supported in this build. The remote MCP server ` + + `requires browser authorization at:\n ${authorizationUrl}\n` + + `Only headless flows (e.g. Reboot's Anonymous dev provider) are supported today.`, + ); + this.name = "InteractiveOAuthNotSupportedError"; + } +} + +export interface WorkspaceOAuthProviderOptions { + wsId: string; + serverName: string; + workDir: string; + /** Absolute callback URL — must match the /v1/mcp-auth/callback route. */ + callbackUrl: string; +} + +interface Deferred { + promise: Promise; + resolve: (value: T) => void; + reject: (err: Error) => void; +} + +function deferred(): Deferred { + let resolve!: (value: T) => void; + let reject!: (err: Error) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +/** + * File-backed OAuthClientProvider scoped to a `(workspace, serverName)` + * pair. Persistence layout: + * + * /workspaces//credentials/mcp-oauth// + * ├── client.json — DCR result (OAuthClientInformationFull) + * ├── tokens.json — OAuthTokens (access + refresh) + * └── verifier.json — PKCE verifier (ephemeral; deleted after finishAuth) + * + * Directory is created with mode 0o700; files are written 0o600 via an + * atomic rename pattern (write to tmp, chmod, rename). Same discipline as + * `src/config/workspace-credentials.ts`. + * + * For Reboot's `Anonymous` dev OAuth (rbt dev): the authorization URL + * returned by the server is ALREADY our own callback URL with + * `?code=anonymous&state=...` embedded (see + * `reboot/aio/auth/oauth_providers.py:278-281`). We detect the self-target + * in `redirectToAuthorization` and resolve the pending flow in-process — + * no HTTP round-trip, no browser. For all other interactive flows, we + * throw `InteractiveOAuthNotSupportedError` and fail fast. + */ +export class WorkspaceOAuthProvider implements OAuthClientProvider { + private readonly wsId: string; + private readonly serverName: string; + private readonly dir: string; + private readonly callbackUrl: string; + /** Cached DCR result + tokens to avoid redundant disk reads within a flow. */ + private cachedClientInfo: OAuthClientInformationFull | null = null; + private cachedTokens: OAuthTokens | null = null; + /** + * The deferred for the in-flight authorization. Set by `state()`, resolved + * or rejected by `redirectToAuthorization` (for headless flows) or by the + * HTTP callback route via the flow-registry (for interactive flows in a + * future iteration). + */ + private pendingFlow: Deferred | null = null; + + constructor(opts: WorkspaceOAuthProviderOptions) { + this.wsId = opts.wsId; + this.serverName = opts.serverName; + this.callbackUrl = opts.callbackUrl; + this.dir = join( + opts.workDir, + "workspaces", + opts.wsId, + "credentials", + "mcp-oauth", + opts.serverName, + ); + } + + // ── OAuthClientProvider interface ───────────────────────────────── + + get redirectUrl(): string { + return this.callbackUrl; + } + + get clientMetadata(): OAuthClientMetadata { + return { + client_name: `NimbleBrain (${this.wsId})`, + redirect_uris: [this.callbackUrl], + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + token_endpoint_auth_method: "none", + }; + } + + state(): string { + const s = randomBytes(32).toString("base64url"); + // Create the deferred early so `awaitPendingFlow()` is safe to call any + // time after `state()` runs. If an error unwinds before + // `redirectToAuthorization`, we reject the deferred there to avoid + // leaving consumers hanging. + this.pendingFlow = deferred(); + return s; + } + + async clientInformation(): Promise { + if (this.cachedClientInfo) return this.cachedClientInfo; + const data = await this.readJson("client.json"); + if (data) this.cachedClientInfo = data; + return data ?? undefined; + } + + async saveClientInformation(info: OAuthClientInformationFull): Promise { + this.cachedClientInfo = info; + await this.writeJson("client.json", info); + } + + async tokens(): Promise { + if (this.cachedTokens) return this.cachedTokens; + const data = await this.readJson("tokens.json"); + if (data) this.cachedTokens = data; + return data ?? undefined; + } + + async saveTokens(tokens: OAuthTokens): Promise { + this.cachedTokens = tokens; + await this.writeJson("tokens.json", tokens); + } + + async saveCodeVerifier(codeVerifier: string): Promise { + await this.writeJson("verifier.json", { codeVerifier }); + } + + async codeVerifier(): Promise { + const data = await this.readJson<{ codeVerifier: string }>("verifier.json"); + if (!data) throw new Error("PKCE code verifier missing — OAuth flow corrupted"); + return data.codeVerifier; + } + + async redirectToAuthorization(url: URL): Promise { + if (!this.pendingFlow) { + throw new Error( + "[workspace-oauth-provider] redirectToAuthorization called without an active flow", + ); + } + const d = this.pendingFlow; + + // Follow the authorize redirect chain hop-by-hop. Headless providers + // (Reboot `Anonymous`, client_credentials-style flows) eventually 302 to + // our own callback with the authorization code already in the URL, at + // which point we can extract it directly. Reboot specifically does two + // hops: /__/oauth/authorize → /__/oauth/callback → our callback. + // + // We use manual redirect handling (not fetch's default follow) so we + // can inspect every Location, stop as soon as one targets our callback, + // and avoid actually dispatching a request to our own server (which + // would tangle our own HTTP event loop into the probe). + // + // Real interactive providers (Granola, Claude.ai hosted) redirect to a + // login page on a different origin — the loop never lands on our + // callback and we fall through to the interactive branch. + const MAX_HOPS = 10; + let current = url; + try { + for (let hop = 0; hop < MAX_HOPS; hop++) { + const res = await fetch(current.toString(), { redirect: "manual" }); + if (res.status < 300 || res.status >= 400) { + // Non-redirect response — provider sent us a login page (200) or + // an error (4xx/5xx). Not headless. + break; + } + const location = res.headers.get("location"); + if (!location) break; + const next = new URL(location, current); + const nextPath = `${next.origin}${next.pathname}`; + if (nextPath === this.callbackUrl) { + const code = next.searchParams.get("code"); + const errParam = next.searchParams.get("error"); + if (code) { + log.debug( + "mcp", + `[oauth] headless flow: ${this.serverName} got code=${code.slice(0, 8)}… after ${hop + 1} hop(s)`, + ); + d.resolve(code); + return; + } + if (errParam) { + const err = new Error( + `[workspace-oauth-provider] authorization server returned error: ${errParam}`, + ); + d.reject(err); + throw err; + } + break; + } + current = next; + } + } catch (probeErr) { + // Rethrow our own explicit error; swallow anything else (network + // failure, invalid URL, etc.) and fall through to the interactive + // branch below. + if (probeErr instanceof Error && probeErr.message.includes("[workspace-oauth-provider]")) { + throw probeErr; + } + log.debug("mcp", `[oauth] ${this.serverName} redirect probe failed: ${String(probeErr)}`); + } + + // Interactive flows (real browser redirect) are the extension point for + // a future iteration — that's when the flow-registry becomes load-bearing + // (the HTTP callback route resolves a registered flow by state). For now + // we don't register: there's no one to wait for the code, and registering + // would create an unhandled rejection when we immediately reject. + const err = new InteractiveOAuthNotSupportedError(url.toString()); + // Reject the provider-local deferred so `awaitPendingFlow()` also fails + // fast instead of hanging — consumers that await it get the same error + // surface as the throw below. + d.reject(err); + throw err; + } + + async invalidateCredentials( + scope: "all" | "client" | "tokens" | "verifier" | "discovery", + ): Promise { + if (scope === "all" || scope === "client") { + this.cachedClientInfo = null; + await this.unlinkIfExists("client.json"); + } + if (scope === "all" || scope === "tokens") { + this.cachedTokens = null; + await this.unlinkIfExists("tokens.json"); + } + if (scope === "all" || scope === "verifier") { + await this.unlinkIfExists("verifier.json"); + } + // 'discovery' is SDK-internal metadata; we don't persist it. + } + + // ── Extensions used by McpSource.start() ────────────────────────── + + /** + * Await the in-flight authorization to yield an authorization code. + * Called by `McpSource.start()` after catching `UnauthorizedError` so it + * can then call `transport.finishAuth(code)` and retry `connect()`. + * + * Fails fast if the flow was rejected (e.g., interactive OAuth). + */ + async awaitPendingFlow(): Promise { + if (!this.pendingFlow) { + throw new Error( + "[workspace-oauth-provider] awaitPendingFlow called with no active flow — " + + "redirectToAuthorization was never invoked on this provider", + ); + } + return this.pendingFlow.promise; + } + + // ── File I/O helpers ────────────────────────────────────────────── + + private async ensureDir(): Promise { + await mkdir(this.dir, { recursive: true, mode: 0o700 }); + try { + await chmod(this.dir, 0o700); + } catch { + // mkdir succeeded; chmod failure is non-fatal (file mode 0o600 still + // protects the contents). A permissive parent leaks existence of + // credentials via directory listings but not their values. + } + } + + private filePath(name: string): string { + return join(this.dir, name); + } + + private async readJson(name: string): Promise { + const path = this.filePath(name); + if (!existsSync(path)) return null; + try { + const raw = await readFile(path, "utf-8"); + return JSON.parse(raw) as T; + } catch (err) { + log.debug("mcp", `[oauth] failed to read ${path}: ${String(err)}`); + return null; + } + } + + private async writeJson(name: string, value: unknown): Promise { + await this.ensureDir(); + const path = this.filePath(name); + const tmp = `${path}.tmp.${randomBytes(4).toString("hex")}`; + const content = JSON.stringify(value, null, 2); + await writeFile(tmp, content, { encoding: "utf-8", mode: 0o600 }); + await chmod(tmp, 0o600); + const { rename } = await import("node:fs/promises"); + await rename(tmp, path); + } + + private async unlinkIfExists(name: string): Promise { + const path = this.filePath(name); + if (!existsSync(path)) return; + try { + await unlink(path); + } catch { + // ignore — file may have been removed concurrently + } + } +} diff --git a/test/unit/oauth-flow-registry.test.ts b/test/unit/oauth-flow-registry.test.ts new file mode 100644 index 0000000..40fe7e3 --- /dev/null +++ b/test/unit/oauth-flow-registry.test.ts @@ -0,0 +1,38 @@ +import { beforeEach, describe, expect, it } from "bun:test"; +import { + _clearAll, + register, + rejectFlow, + resolveWithCode, +} from "../../src/tools/oauth-flow-registry.ts"; + +describe("oauth-flow-registry", () => { + beforeEach(() => { + _clearAll(); + }); + + it("resolves a registered flow with the provided code", async () => { + const p = register("state-abc", "ws_test", "srv"); + const matched = resolveWithCode("state-abc", "the-code"); + expect(matched).toBe(true); + await expect(p).resolves.toBe("the-code"); + }); + + it("returns false for unknown state on resolve", () => { + const matched = resolveWithCode("unknown-state", "code"); + expect(matched).toBe(false); + }); + + it("rejects a registered flow with the provided error", async () => { + const p = register("state-xyz", "ws_test", "srv"); + const rejected = rejectFlow("state-xyz", new Error("boom")); + expect(rejected).toBe(true); + await expect(p).rejects.toThrow("boom"); + }); + + it("removes the flow after resolve (second resolve is a no-op)", () => { + register("state-1", "ws_test", "srv"); + expect(resolveWithCode("state-1", "a")).toBe(true); + expect(resolveWithCode("state-1", "b")).toBe(false); + }); +}); diff --git a/test/unit/workspace-oauth-provider.test.ts b/test/unit/workspace-oauth-provider.test.ts new file mode 100644 index 0000000..87f62a3 --- /dev/null +++ b/test/unit/workspace-oauth-provider.test.ts @@ -0,0 +1,244 @@ +import { mkdtempSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { beforeEach, describe, expect, it } from "bun:test"; +import type { + OAuthClientInformationFull, + OAuthTokens, +} from "@modelcontextprotocol/sdk/shared/auth.js"; +import { + InteractiveOAuthNotSupportedError, + WorkspaceOAuthProvider, +} from "../../src/tools/workspace-oauth-provider.ts"; + +const CALLBACK = "http://localhost:27247/v1/mcp-auth/callback"; + +function makeProvider(workDir: string, serverName = "test-srv"): WorkspaceOAuthProvider { + return new WorkspaceOAuthProvider({ + wsId: "ws_test", + serverName, + workDir, + callbackUrl: CALLBACK, + }); +} + +describe("WorkspaceOAuthProvider", () => { + let workDir: string; + + beforeEach(() => { + workDir = mkdtempSync(join(tmpdir(), "nb-oauth-test-")); + }); + + it("roundtrips client information via files", async () => { + const p = makeProvider(workDir); + const info: OAuthClientInformationFull = { + client_id: "cid-123", + redirect_uris: [CALLBACK], + }; + await p.saveClientInformation(info); + + const read = await p.clientInformation(); + expect(read).toEqual(info); + + // Second provider instance (no in-memory cache) should see the file + const p2 = makeProvider(workDir); + const read2 = await p2.clientInformation(); + expect(read2).toEqual(info); + }); + + it("roundtrips tokens via files", async () => { + const p = makeProvider(workDir); + const tokens: OAuthTokens = { + access_token: "acc", + token_type: "Bearer", + refresh_token: "ref", + expires_in: 3600, + }; + await p.saveTokens(tokens); + + const p2 = makeProvider(workDir); + const read = await p2.tokens(); + expect(read).toEqual(tokens); + }); + + it("headless flow: authorize endpoint 302 to our callback with code resolves pending flow", async () => { + // Stand up a mock authorize endpoint that behaves like Reboot's Anonymous: + // 302 straight back to the client's redirect_uri with ?code=anonymous&state=. + const mockAuthServer = Bun.serve({ + port: 0, + fetch(req: Request) { + const u = new URL(req.url); + const state = u.searchParams.get("state") ?? ""; + const redirectUri = u.searchParams.get("redirect_uri") ?? CALLBACK; + const target = new URL(redirectUri); + target.searchParams.set("code", "anonymous"); + target.searchParams.set("state", state); + return new Response(null, { status: 302, headers: { location: target.toString() } }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir); + const state = p.state(); + authUrl.searchParams.set("state", state); + authUrl.searchParams.set("redirect_uri", CALLBACK); + const pending = p.awaitPendingFlow(); + + await p.redirectToAuthorization(authUrl); + await expect(pending).resolves.toBe("anonymous"); + } finally { + mockAuthServer.stop(true); + } + }); + + it("headless flow: multi-hop same-origin redirects eventually hitting our callback (Reboot pattern)", async () => { + // Mimics Reboot: /authorize 302s to /intermediate, which 302s to our callback. + const mockAuthServer = Bun.serve({ + port: 0, + fetch(req: Request) { + const u = new URL(req.url); + if (u.pathname === "/authorize") { + const state = u.searchParams.get("state") ?? ""; + const redirectUri = u.searchParams.get("redirect_uri") ?? CALLBACK; + // Hop 1: redirect to our own /intermediate with an internal token + const interm = new URL(`http://localhost:${mockAuthServer.port}/intermediate`); + interm.searchParams.set("internal_token", "reboot-jwt"); + interm.searchParams.set("mcp_state", state); + interm.searchParams.set("mcp_redirect_uri", redirectUri); + return new Response(null, { + status: 302, + headers: { location: interm.toString() }, + }); + } + if (u.pathname === "/intermediate") { + // Hop 2: redirect to the client's redirect_uri with the real code + const mcpState = u.searchParams.get("mcp_state") ?? ""; + const mcpRedirect = u.searchParams.get("mcp_redirect_uri") ?? CALLBACK; + const target = new URL(mcpRedirect); + target.searchParams.set("code", "anonymous"); + target.searchParams.set("state", mcpState); + return new Response(null, { + status: 302, + headers: { location: target.toString() }, + }); + } + return new Response("not found", { status: 404 }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir); + const state = p.state(); + authUrl.searchParams.set("state", state); + authUrl.searchParams.set("redirect_uri", CALLBACK); + const pending = p.awaitPendingFlow(); + + await p.redirectToAuthorization(authUrl); + await expect(pending).resolves.toBe("anonymous"); + } finally { + mockAuthServer.stop(true); + } + }); + + it("interactive flow: authorize endpoint 302s to a login page throws InteractiveOAuthNotSupportedError", async () => { + // Mock authorize endpoint that redirects to a non-self-target login page + // (how Granola / Claude.ai / real OAuth providers behave). + const mockAuthServer = Bun.serve({ + port: 0, + fetch(_req: Request) { + return new Response(null, { + status: 302, + headers: { location: "https://login.example.com/authenticate" }, + }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir); + const state = p.state(); + authUrl.searchParams.set("state", state); + const pending = p.awaitPendingFlow(); + const pendingSettled = pending.catch((err) => err); + + await expect(p.redirectToAuthorization(authUrl)).rejects.toBeInstanceOf( + InteractiveOAuthNotSupportedError, + ); + expect(await pendingSettled).toBeInstanceOf(InteractiveOAuthNotSupportedError); + } finally { + mockAuthServer.stop(true); + } + }); + + it("interactive flow: authorize endpoint 200s with login form throws InteractiveOAuthNotSupportedError", async () => { + // Providers that return 200 with an HTML login form (not a 302) are also + // interactive from our perspective — we can't extract a code. + const mockAuthServer = Bun.serve({ + port: 0, + fetch(_req: Request) { + return new Response("login form", { + status: 200, + headers: { "content-type": "text/html" }, + }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir); + const state = p.state(); + authUrl.searchParams.set("state", state); + const pending = p.awaitPendingFlow(); + const pendingSettled = pending.catch((err) => err); + + await expect(p.redirectToAuthorization(authUrl)).rejects.toBeInstanceOf( + InteractiveOAuthNotSupportedError, + ); + expect(await pendingSettled).toBeInstanceOf(InteractiveOAuthNotSupportedError); + } finally { + mockAuthServer.stop(true); + } + }); + + it("verifier roundtrip + codeVerifier missing throws", async () => { + const p = makeProvider(workDir); + await p.saveCodeVerifier("pkce-verifier-xyz"); + expect(await p.codeVerifier()).toBe("pkce-verifier-xyz"); + + await p.invalidateCredentials("verifier"); + await expect(p.codeVerifier()).rejects.toThrow(/verifier missing/); + }); + + it("invalidateCredentials removes tokens but keeps client info on 'tokens' scope", async () => { + const p = makeProvider(workDir); + await p.saveClientInformation({ client_id: "cid", redirect_uris: [CALLBACK] }); + await p.saveTokens({ access_token: "a", token_type: "Bearer" }); + + await p.invalidateCredentials("tokens"); + + const p2 = makeProvider(workDir); + expect(await p2.tokens()).toBeUndefined(); + expect(await p2.clientInformation()).toBeDefined(); + }); + + it("files are written under /workspaces//credentials/mcp-oauth//", async () => { + const p = makeProvider(workDir, "my-server"); + const tokens: OAuthTokens = { access_token: "a", token_type: "Bearer" }; + await p.saveTokens(tokens); + + const expectedPath = join( + workDir, + "workspaces", + "ws_test", + "credentials", + "mcp-oauth", + "my-server", + "tokens.json", + ); + const onDisk = JSON.parse(readFileSync(expectedPath, "utf-8")); + expect(onDisk).toEqual(tokens); + }); + + it("awaitPendingFlow without state() throws (no active flow)", async () => { + const p = makeProvider(workDir); + await expect(p.awaitPendingFlow()).rejects.toThrow(/no active flow/i); + }); +}); From db8dfbf8eefb3062ea8ec11737e71d2e7618f68f Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Thu, 23 Apr 2026 13:43:20 -1000 Subject: [PATCH 2/4] Address QA feedback on #68 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical fixes: - Cross-tenant OAuth credential leak closed. `lifecycle.ts::installRemote` now threads `wsId` + `workDir` into `startBundleSource` (previously dropped), and `startup.ts`'s URL-branch hard-errors on missing `wsId` when no static auth is configured. The old `?? "ws_default"` fallback would have silently pooled OAuth tokens across every workspace under the default id; named bundles already threw, URL bundles silently diverged. Matches the named-bundle precedent at the credential boundary. - SSRF vector on the OAuth authorize chain closed. `validateBundleUrl` previously ran only on `ref.url`; a compromised remote MCP server could hand us an authorize URL pointing at AWS IMDS / RFC1918 / our own loopback, and we'd probe it directly. Every hop through `redirectToAuthorization`'s redirect loop now passes through `validateBundleUrl`; blocks surface as explicit `[workspace-oauth-provider] SSRF block …` errors so operators see the real cause instead of the generic "interactive not supported" fallthrough. `allowInsecureRemotes` is propagated into the provider so local-dev (Reboot on localhost:9991) still works when the flag is explicitly set. - Callback self-match normalized. Comparing `next.origin + next.pathname` as a raw string was flaky: configured `callbackUrl` with a trailing slash, or a server echoing an explicit default port / uppercase hostname, would fail the equality check and silently fall through to InteractiveOAuthNotSupportedError. Provider now precomputes a canonical form (lowercased origin, pathname stripped of trailing `/`) at constructor time and compares against that. Also: discovered and fixed a latent retry-path bug while writing the integration test QA requested (W5). `StreamableHTTPClientTransport` rejects a second `start()` on the same instance, so my original "connect → catch 401 → finishAuth → connect on same transport" would have failed whenever the retry actually fired. Matches the SDK's own `simpleOAuthClient` pattern of new-transport-per-attempt: retry now tears down the first transport+client, rebuilds via `createRemoteTransport`, and reconnects on the fresh instance. The real Reboot flow happened to work because tokens landed on disk via a prior partial run and the first connect succeeded with cached tokens; the test surfaced the gap. Warnings applied: - Bun.serve-based cases moved from `test/unit/workspace-oauth-provider` to `test/integration/workspace-oauth-provider`, per CLAUDE.md's classification rule. - `test/integration/mcp-source-oauth-retry.test.ts` added with an end-to-end Bun.serve mock of the OAuth + MCP stack (discovery, DCR, authorize, token, /mcp 401-then-200). Directly exercises the retry path the PR introduces. - `connectWithTimeout` timer leak fixed. The `setTimeout` handle is now captured and cleared in a finally; previously every successful connect leaked a 15–30s timer, doubled under the retry path. - `rename` moved to the top static import in `workspace-oauth-provider` — dynamic `await import("node:fs/promises")` on every token write was out of step with the rest of the file. - `mcp-auth.ts` callback route adds `Cache-Control: no-store` + `Pragma: no-cache`. Defense-in-depth against intermediate caches storing the success/failure HTML with `?code=` in the URL. Warning #8 (token_endpoint_auth_method): acknowledged, deferred. Switching between "none" and "client_secret_basic" based on DCR response is not a one-liner — it requires implementing `OAuthClientProvider.addClientAuthentication` to inject the credential on token requests post-hoc. Noted as a TODO in a follow-up ticket; shipping "none" unconditionally is correct for Reboot Anonymous and works for any AS that issues refresh tokens to public clients (the ext-apps examples do). Suggestions applied: - oauth-flow-registry gains a 15-minute TTL per registration (also the boundary documented in INTERACTIVE_OAUTH_UI.md). Timer handles are cleared on resolve/reject/_clearAll so late fires can't double-settle a promise. `timeout.unref?.()` so a stuck flow's timer doesn't keep CLI invocations alive. - `mcp-source` cast `this.transport as StreamableHTTPClientTransport` replaced with a narrower `Transport & { finishAuth?: (...) }` — SSE also has finishAuth, the old cast was a narrowing lie. - Redundant `this.dead = false` in retry path dropped — happy path leaves it unset, matching the default, one code path for "started." - `NB_API_URL` startup warning: if unset when a URL-ref bundle is being wired, log a one-time warning that localhost:27247 is dev-only and prod deployments behind a proxy need the env var set. Suggestion #11 (replace hand-rolled escape with Hono `html` template) deliberately skipped — the escape helper is minimal, auditable, and doesn't depend on Hono's tagging API; swap isn't worth the churn. Full verify green: 1874 unit + 117 web + 378 integration + 16 smoke. --- src/api/routes/mcp-auth.ts | 8 + src/bundles/lifecycle.ts | 9 +- src/bundles/startup.ts | 41 ++- src/tools/mcp-source.ts | 94 +++++- src/tools/oauth-flow-registry.ts | 42 ++- src/tools/workspace-oauth-provider.ts | 60 +++- .../mcp-source-oauth-retry.test.ts | 220 ++++++++++++++ test/integration/remote-integration.test.ts | 12 +- test/integration/remote-lifecycle.test.ts | 8 +- .../workspace-oauth-provider.test.ts | 281 ++++++++++++++++++ test/unit/oauth-flow-registry.test.ts | 22 ++ test/unit/workspace-oauth-provider.test.ts | 152 +--------- 12 files changed, 765 insertions(+), 184 deletions(-) create mode 100644 test/integration/mcp-source-oauth-retry.test.ts create mode 100644 test/integration/workspace-oauth-provider.test.ts diff --git a/src/api/routes/mcp-auth.ts b/src/api/routes/mcp-auth.ts index 0e702b9..6493c2d 100644 --- a/src/api/routes/mcp-auth.ts +++ b/src/api/routes/mcp-auth.ts @@ -28,6 +28,14 @@ export function mcpAuthRoutes(_ctx: AppContext) { const app = new Hono(); app.get("/v1/mcp-auth/callback", (c) => { + // Belt-and-suspenders: an intermediate proxy caching the success page + // (with `?code=...` in the URL) in a shared cache space is a classic + // OAuth footgun. Codes are single-use so the real boundary is the + // flow registry, but explicitly marking the response non-cacheable + // kills the class entirely. + c.header("Cache-Control", "no-store"); + c.header("Pragma", "no-cache"); + const code = c.req.query("code"); const state = c.req.query("state"); const error = c.req.query("error"); diff --git a/src/bundles/lifecycle.ts b/src/bundles/lifecycle.ts index 00ce16d..c231c57 100644 --- a/src/bundles/lifecycle.ts +++ b/src/bundles/lifecycle.ts @@ -217,12 +217,19 @@ export class BundleLifecycleManager { ui?: BundleUiMeta | null, trustScore?: number | null, ): Promise { + // Thread wsId + workDir through to startBundleSource so the URL-bundle + // branch can key OAuth credentials by (wsId, serverName) instead of + // falling back to `ws_default`. Without this, a URL bundle installed + // via `installRemote` from any workspace would share OAuth tokens across + // workspaces under the default id — silent cross-tenant credential + // leakage. + const nbWorkDir = process.env.NB_WORK_DIR ?? join(homedir(), ".nimblebrain"); const { sourceName, meta } = await startBundleSource( { url, serverName, transport: transportConfig, ui: ui ?? null }, registry, this.eventSink, this.configPath ? dirname(this.configPath) : undefined, - { allowInsecureRemotes: this.allowInsecureRemotes }, + { allowInsecureRemotes: this.allowInsecureRemotes, wsId, workDir: nbWorkDir }, ); const instance: BundleInstance = { diff --git a/src/bundles/startup.ts b/src/bundles/startup.ts index 4170c0f..cae4e18 100644 --- a/src/bundles/startup.ts +++ b/src/bundles/startup.ts @@ -74,22 +74,45 @@ export async function startBundleSource( // Attach an OAuthClientProvider when no static auth is configured. The // provider is workspace-scoped: tokens and DCR credentials live under - // /workspaces//credentials/mcp-oauth//. Named - // bundles already require `wsId`; URL bundles historically did not — - // fall back to `ws_default` only if wsId is missing, which preserves - // behavior for any callers that haven't threaded workspace context. + // /workspaces//credentials/mcp-oauth//. + // + // `wsId` is REQUIRED here — not defaulted — to match the named-bundle + // branch's behavior at the credential boundary. A silent `ws_default` + // fallback would cause cross-tenant credential leakage: URL bundles + // installed from different workspaces would share OAuth tokens under + // the same default id. Callers must thread workspace context through + // `installRemote` / `startBundleSource`. let authProvider: WorkspaceOAuthProvider | undefined; const hasStaticAuth = ref.transport?.auth && ref.transport.auth.type !== "none"; if (!hasStaticAuth) { - const wsId = opts?.wsId ?? "ws_default"; - const workDir = opts?.workDir ?? process.env.NB_WORK_DIR ?? join(homedir(), ".nimblebrain"); - const apiBase = process.env.NB_API_URL ?? "http://localhost:27247"; - const callbackUrl = `${apiBase.replace(/\/+$/, "")}/v1/mcp-auth/callback`; + if (!opts?.wsId) { + throw new Error( + `[bundles] URL bundle "${sourceName}" without static auth requires opts.wsId — ` + + "OAuth credentials are workspace-scoped and silent defaults would cross tenants. " + + "Thread wsId through installRemote() or the caller that invoked startBundleSource().", + ); + } + const workDir = opts.workDir ?? process.env.NB_WORK_DIR ?? join(homedir(), ".nimblebrain"); + const apiBase = process.env.NB_API_URL; + // Startup warning when a URL-ref bundle is being wired but NB_API_URL + // isn't set. Default is only safe for local dev — in prod (NB behind a + // proxy), the OAuth provider would hand the authorization server a + // redirect_uri pointing at the pod's localhost, which the user's + // browser can't reach. One-time log per process is enough. + if (!apiBase) { + log.warn( + `[bundles] NB_API_URL not set; OAuth callback defaults to http://localhost:27247. ` + + "In production (NB behind a proxy / on a different host from the user's browser), " + + "set NB_API_URL to the platform's externally reachable URL.", + ); + } + const callbackUrl = `${(apiBase ?? "http://localhost:27247").replace(/\/+$/, "")}/v1/mcp-auth/callback`; authProvider = new WorkspaceOAuthProvider({ - wsId, + wsId: opts.wsId, serverName, workDir, callbackUrl, + allowInsecureRemotes: opts.allowInsecureRemotes === true, }); } diff --git a/src/tools/mcp-source.ts b/src/tools/mcp-source.ts index 583f605..887132b 100644 --- a/src/tools/mcp-source.ts +++ b/src/tools/mcp-source.ts @@ -1,7 +1,6 @@ import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; -import type { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"; import type { RemoteTransportConfig } from "../bundles/types.ts"; import { log } from "../cli/log.ts"; @@ -27,6 +26,16 @@ export interface McpSpawnConfig { cwd?: string; } +/** + * Narrow shape for a transport that can complete an OAuth authorization + * code exchange. Both streamable-HTTP and SSE transports in the MCP SDK + * expose this method when an `authProvider` is attached; a bare cast to + * one concrete class would lie about which transport shapes are valid. + */ +type AuthFinishableTransport = Transport & { + finishAuth?: (authorizationCode: string) => Promise; +}; + /** Discriminated union for how McpSource connects to its MCP server. */ export type McpTransportMode = | { type: "stdio"; spawn: McpSpawnConfig } @@ -137,7 +146,11 @@ export class McpSource implements ToolSource { // UnauthorizedError, the provider's pending flow was either resolved // in-process (headless, e.g. Reboot Anonymous) or rejected with a // clear error (interactive, which we don't support yet). Await the - // flow, finish auth on the transport, and try exactly once more. + // flow, finish auth on the EXISTING transport (so tokens land via + // authProvider.saveTokens), then tear down the transport+client and + // rebuild for the retry — `StreamableHTTPClientTransport` rejects a + // second `start()` on the same instance (matching the SDK's own + // `simpleOAuthClient` example pattern of new-transport-per-attempt). if ( err instanceof UnauthorizedError && this.mode.type === "remote" && @@ -146,16 +159,24 @@ export class McpSource implements ToolSource { ) { try { const code = await this.mode.authProvider.awaitPendingFlow(); - const streamableTransport = this.transport as StreamableHTTPClientTransport; - if (typeof streamableTransport.finishAuth !== "function") { + const authable = this.transport as AuthFinishableTransport; + if (typeof authable.finishAuth !== "function") { throw new Error( `[mcp-source] transport does not support finishAuth (got ${this.transport.constructor.name})`, ); } - await streamableTransport.finishAuth(code); - log.debug("mcp", `[oauth] ${this.name}: finishAuth ok, retrying connect`); + await authable.finishAuth(code); + log.debug("mcp", `[oauth] ${this.name}: finishAuth ok, recreating transport for retry`); + + // Drop the first-attempt transport+client. Both are single-use + // after a failed start; the SDK tracks internal state + // (AbortController on the transport, handshake promise on the + // client) that a second connect would trip over. + await this.cleanupOnStartFailure(); + this.rebuildRemoteTransport(); + this.client = this.buildClient(); + await this.connectWithTimeout(CONNECT_TIMEOUT); - this.dead = false; this.startedAt = Date.now(); return; } catch (retryErr) { @@ -176,13 +197,22 @@ export class McpSource implements ToolSource { if (!this.client || !this.transport) { throw new Error("[mcp-source] connectWithTimeout called before init"); } - const timeout = new Promise((_, reject) => - setTimeout( + // Capture and clear the timer on BOTH success and failure; without this, + // every successful connect leaks a 15–30s setTimeout that keeps the + // event loop awake. Under the OAuth retry path this would fire twice + // per successful start(). + let timer: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timer = setTimeout( () => reject(new Error(`MCP connect timeout after ${timeoutMs / 1000}s for ${this.name}`)), timeoutMs, - ), - ); - await Promise.race([this.client.connect(this.transport), timeout]); + ); + }); + try { + await Promise.race([this.client.connect(this.transport), timeout]); + } finally { + if (timer !== undefined) clearTimeout(timer); + } } private async cleanupOnStartFailure(): Promise { @@ -195,6 +225,46 @@ export class McpSource implements ToolSource { this.transport = null; } + /** + * Rebuild a fresh remote transport after an OAuth 401 → retry. Uses the + * same config as the original transport (URL, auth headers, provider) so + * the retry sees exactly the same surface with a clean internal state. + * Caller must have cleaned up the previous transport via + * `cleanupOnStartFailure()` first. + */ + private rebuildRemoteTransport(): void { + if (this.mode.type !== "remote") { + throw new Error("[mcp-source] rebuildRemoteTransport called on non-remote mode"); + } + this.transport = createRemoteTransport( + this.mode.url, + this.mode.transportConfig, + this.mode.authProvider, + ); + this.transport.onclose = () => { + this.dead = true; + this.eventSink.emit({ + type: "run.error", + data: { source: this.name, event: "source.crashed", error: "Remote transport closed" }, + }); + }; + } + + private buildClient(): Client { + return new Client( + { name: "nimblebrain", version: "0.1.0" }, + { + capabilities: { + tasks: { + requests: { tools: { call: {} } }, + cancel: {}, + list: {}, + }, + }, + }, + ); + } + /** Check if the transport is still connected. */ isAlive(): boolean { return this.transport !== null && this.client !== null && !this.dead; diff --git a/src/tools/oauth-flow-registry.ts b/src/tools/oauth-flow-registry.ts index 30352c3..12568d7 100644 --- a/src/tools/oauth-flow-registry.ts +++ b/src/tools/oauth-flow-registry.ts @@ -12,7 +12,12 @@ * * State is not persisted: OAuth flows complete in seconds, and if a process * restart interrupts one, re-initiating is correct. Cross-process concerns - * don't apply — NimbleBrain runs single-process. + * don't apply — NimbleBrain runs single-process. Intra-process leaks, on + * the other hand, matter: an orphaned pending-flow entry (user closed the + * tab, network failure, never hit the callback) would keep a promise alive + * forever if not timed out. Every registration is bounded by a 15-minute + * TTL — long enough for a reasonable interactive OAuth round-trip, short + * enough that a stuck flow is reclaimed before it piles up. */ interface PendingFlow { @@ -20,13 +25,41 @@ interface PendingFlow { reject: (err: Error) => void; wsId: string; serverName: string; + timeout: ReturnType; } const flows = new Map(); -export function register(state: string, wsId: string, serverName: string): Promise { +/** + * Default TTL for a pending flow. Exposed as a constant so tests can target + * the same boundary condition without hardcoding magic numbers. + */ +export const DEFAULT_FLOW_TTL_MS = 15 * 60 * 1000; + +export function register( + state: string, + wsId: string, + serverName: string, + ttlMs: number = DEFAULT_FLOW_TTL_MS, +): Promise { return new Promise((resolve, reject) => { - flows.set(state, { resolve, reject, wsId, serverName }); + const timeout = setTimeout(() => { + // Only reject if we haven't already been resolved/rejected by the + // callback. `flows.delete` before `reject` prevents the reject + // callback from racing with a late callback resolve. + const existing = flows.get(state); + if (existing?.timeout === timeout) { + flows.delete(state); + reject( + new Error(`[oauth-flow-registry] flow ${state.slice(0, 8)}… timed out after ${ttlMs}ms`), + ); + } + }, ttlMs); + // `unref` so a stuck flow's timer doesn't keep the event loop alive + // in short-lived CLI invocations. In the HTTP server process this is + // a no-op — the server keeps the loop alive independently. + timeout.unref?.(); + flows.set(state, { resolve, reject, wsId, serverName, timeout }); }); } @@ -35,6 +68,7 @@ export function resolveWithCode(state: string, code: string): boolean { const flow = flows.get(state); if (!flow) return false; flows.delete(state); + clearTimeout(flow.timeout); flow.resolve(code); return true; } @@ -44,6 +78,7 @@ export function rejectFlow(state: string, err: Error): boolean { const flow = flows.get(state); if (!flow) return false; flows.delete(state); + clearTimeout(flow.timeout); flow.reject(err); return true; } @@ -51,6 +86,7 @@ export function rejectFlow(state: string, err: Error): boolean { /** For tests: drop all pending flows. */ export function _clearAll(): void { for (const flow of flows.values()) { + clearTimeout(flow.timeout); flow.reject(new Error("flow registry cleared")); } flows.clear(); diff --git a/src/tools/workspace-oauth-provider.ts b/src/tools/workspace-oauth-provider.ts index 89aac9e..c221dc5 100644 --- a/src/tools/workspace-oauth-provider.ts +++ b/src/tools/workspace-oauth-provider.ts @@ -1,6 +1,6 @@ import { randomBytes } from "node:crypto"; import { existsSync } from "node:fs"; -import { chmod, mkdir, readFile, unlink, writeFile } from "node:fs/promises"; +import { chmod, mkdir, readFile, rename, unlink, writeFile } from "node:fs/promises"; import { join } from "node:path"; import type { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js"; import type { @@ -9,6 +9,7 @@ import type { OAuthClientMetadata, OAuthTokens, } from "@modelcontextprotocol/sdk/shared/auth.js"; +import { validateBundleUrl } from "../bundles/url-validator.ts"; import { log } from "../cli/log.ts"; /** @@ -35,6 +36,28 @@ export interface WorkspaceOAuthProviderOptions { workDir: string; /** Absolute callback URL — must match the /v1/mcp-auth/callback route. */ callbackUrl: string; + /** + * Whether loopback / RFC1918 / cloud-metadata hosts are acceptable targets + * for the authorize chain. Mirrors the platform-level `allowInsecureRemotes` + * flag; when `false` (production default), every hop of the authorize + * redirect chain is passed through `validateBundleUrl` to block SSRF + * against internal infrastructure (AWS IMDS, RFC1918 admin panels, + * NimbleBrain's own loopback ports). + */ + allowInsecureRemotes?: boolean; +} + +/** + * Normalize a callback URL to a `{origin, pathname}` canonical form so the + * self-match check tolerates trivial differences a strict `===` would miss: + * trailing slash on pathname, explicit default port vs implicit, hostname + * case. The pathname is stripped of trailing `/` and compared case-sensitively + * (paths are case-sensitive); the origin is lowercased. + */ +function canonicalEndpoint(u: URL): string { + const origin = u.origin.toLowerCase(); + const path = u.pathname.replace(/\/+$/, "") || "/"; + return `${origin}${path}`; } interface Deferred { @@ -79,6 +102,9 @@ export class WorkspaceOAuthProvider implements OAuthClientProvider { private readonly serverName: string; private readonly dir: string; private readonly callbackUrl: string; + /** Canonical form of `callbackUrl` for self-match comparison. */ + private readonly canonicalCallback: string; + private readonly allowInsecureRemotes: boolean; /** Cached DCR result + tokens to avoid redundant disk reads within a flow. */ private cachedClientInfo: OAuthClientInformationFull | null = null; private cachedTokens: OAuthTokens | null = null; @@ -94,6 +120,8 @@ export class WorkspaceOAuthProvider implements OAuthClientProvider { this.wsId = opts.wsId; this.serverName = opts.serverName; this.callbackUrl = opts.callbackUrl; + this.canonicalCallback = canonicalEndpoint(new URL(opts.callbackUrl)); + this.allowInsecureRemotes = opts.allowInsecureRemotes === true; this.dir = join( opts.workDir, "workspaces", @@ -190,6 +218,24 @@ export class WorkspaceOAuthProvider implements OAuthClientProvider { let current = url; try { for (let hop = 0; hop < MAX_HOPS; hop++) { + // SSRF defense: validate EVERY hop (including the initial URL the + // server handed us), not just the configured bundle URL. The + // authorize URL and every Location header are attacker-controlled — + // a compromised remote MCP server could otherwise use our fetch() + // as an internal-network probe tool (AWS IMDS, RFC1918 admin + // panels, loopback services). Wrap with our marker prefix so the + // outer catch rethrows instead of silently falling through to the + // "interactive not supported" message. + try { + validateBundleUrl(current, { allowInsecure: this.allowInsecureRemotes }); + } catch (err) { + throw new Error( + `[workspace-oauth-provider] SSRF block on ${current.toString()}: ${ + err instanceof Error ? err.message : String(err) + }`, + ); + } + const res = await fetch(current.toString(), { redirect: "manual" }); if (res.status < 300 || res.status >= 400) { // Non-redirect response — provider sent us a login page (200) or @@ -199,8 +245,7 @@ export class WorkspaceOAuthProvider implements OAuthClientProvider { const location = res.headers.get("location"); if (!location) break; const next = new URL(location, current); - const nextPath = `${next.origin}${next.pathname}`; - if (nextPath === this.callbackUrl) { + if (canonicalEndpoint(next) === this.canonicalCallback) { const code = next.searchParams.get("code"); const errParam = next.searchParams.get("error"); if (code) { @@ -223,10 +268,12 @@ export class WorkspaceOAuthProvider implements OAuthClientProvider { current = next; } } catch (probeErr) { - // Rethrow our own explicit error; swallow anything else (network - // failure, invalid URL, etc.) and fall through to the interactive - // branch below. + // Rethrow our own explicit errors (authz server error, SSRF block) + // so callers see the real cause instead of the generic + // "interactive not supported" message. Swallow network failures and + // fall through to the interactive branch below. if (probeErr instanceof Error && probeErr.message.includes("[workspace-oauth-provider]")) { + d.reject(probeErr); throw probeErr; } log.debug("mcp", `[oauth] ${this.serverName} redirect probe failed: ${String(probeErr)}`); @@ -317,7 +364,6 @@ export class WorkspaceOAuthProvider implements OAuthClientProvider { const content = JSON.stringify(value, null, 2); await writeFile(tmp, content, { encoding: "utf-8", mode: 0o600 }); await chmod(tmp, 0o600); - const { rename } = await import("node:fs/promises"); await rename(tmp, path); } diff --git a/test/integration/mcp-source-oauth-retry.test.ts b/test/integration/mcp-source-oauth-retry.test.ts new file mode 100644 index 0000000..9eccac4 --- /dev/null +++ b/test/integration/mcp-source-oauth-retry.test.ts @@ -0,0 +1,220 @@ +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { Server } from "@modelcontextprotocol/sdk/server/index.js"; +import { WebStandardStreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/webStandardStreamableHttp.js"; +import { ListToolsRequestSchema } from "@modelcontextprotocol/sdk/types.js"; +import { NoopEventSink } from "../../src/adapters/noop-events.ts"; +import { McpSource } from "../../src/tools/mcp-source.ts"; +import { WorkspaceOAuthProvider } from "../../src/tools/workspace-oauth-provider.ts"; + +/** + * End-to-end coverage of the retry-once path in `McpSource.start()`: + * connect() → 401 UnauthorizedError → provider.awaitPendingFlow() → + * transport.finishAuth(code) → connect() (succeeds) → tools() works. + * + * The mock server implements the minimum OAuth + MCP surface the SDK + * requires to drive an auth code + PKCE flow: + * + * /.well-known/oauth-protected-resource → advertises the auth server + * /.well-known/oauth-authorization-server → endpoint catalog + * /register → dynamic client registration + * /authorize → 302 to redirect_uri with code + * /token → exchanges code for access token + * /mcp → 401 without bearer, 200 with + * + * This is the test W5 in the QA review asked for — the PR's biggest + * behavioral change previously had zero direct coverage beyond the Reboot + * hand-exercise. + */ + +const CALLBACK = "http://localhost:27247/v1/mcp-auth/callback"; + +interface MockOAuthMcpServer { + port: number; + url: string; + stop: () => void; +} + +function startMockOAuthMcpServer(): MockOAuthMcpServer { + const ISSUED = new Map(); // client_id → code + const VALID_TOKENS = new Set(); + const transports: WebStandardStreamableHTTPServerTransport[] = []; + const servers: Server[] = []; + + const createMcpServer = (): Server => { + const mcpServer = new Server( + { name: "oauth-test-mcp", version: "0.1.0" }, + { capabilities: { tools: {} } }, + ); + mcpServer.setRequestHandler(ListToolsRequestSchema, async () => ({ + tools: [ + { + name: "noop", + description: "no-op tool", + inputSchema: { type: "object" as const, properties: {} }, + }, + ], + })); + return mcpServer; + }; + + const httpServer = Bun.serve({ + port: 0, + async fetch(req: Request) { + const url = new URL(req.url); + const base = `http://localhost:${httpServer.port}`; + + // ---- OAuth discovery ---- + if (url.pathname === "/.well-known/oauth-protected-resource") { + return Response.json({ + resource: base, + authorization_servers: [base], + }); + } + if (url.pathname === "/.well-known/oauth-authorization-server") { + return Response.json({ + issuer: base, + authorization_endpoint: `${base}/authorize`, + token_endpoint: `${base}/token`, + registration_endpoint: `${base}/register`, + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "refresh_token"], + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none"], + }); + } + + // ---- DCR ---- + if (url.pathname === "/register" && req.method === "POST") { + const body = (await req.json()) as Record; + const client_id = "mock-client-" + Math.random().toString(36).slice(2, 10); + return Response.json( + { + client_id, + client_id_issued_at: Math.floor(Date.now() / 1000), + redirect_uris: body.redirect_uris ?? [CALLBACK], + grant_types: body.grant_types ?? ["authorization_code"], + response_types: body.response_types ?? ["code"], + token_endpoint_auth_method: body.token_endpoint_auth_method ?? "none", + }, + { status: 201 }, + ); + } + + // ---- Authorize: 302 straight to redirect_uri with code ---- + if (url.pathname === "/authorize") { + const state = url.searchParams.get("state") ?? ""; + const clientId = url.searchParams.get("client_id") ?? ""; + const redirectUri = url.searchParams.get("redirect_uri") ?? CALLBACK; + const code = "mock-code-" + Math.random().toString(36).slice(2, 10); + ISSUED.set(clientId, { code }); + const target = new URL(redirectUri); + target.searchParams.set("code", code); + target.searchParams.set("state", state); + return new Response(null, { + status: 302, + headers: { location: target.toString() }, + }); + } + + // ---- Token exchange ---- + if (url.pathname === "/token" && req.method === "POST") { + const form = await req.formData(); + const code = form.get("code"); + const clientId = form.get("client_id"); + const issued = typeof clientId === "string" ? ISSUED.get(clientId) : undefined; + if (!issued || issued.code !== code) { + return Response.json({ error: "invalid_grant" }, { status: 400 }); + } + const access = "mock-token-" + Math.random().toString(36).slice(2, 10); + VALID_TOKENS.add(access); + return Response.json({ + access_token: access, + token_type: "Bearer", + expires_in: 3600, + }); + } + + // ---- MCP endpoint: 401 without bearer, 200 with ---- + if (url.pathname === "/mcp") { + const auth = req.headers.get("authorization"); + const token = auth?.toLowerCase().startsWith("bearer ") ? auth.slice(7) : null; + if (!token || !VALID_TOKENS.has(token)) { + return Response.json( + { error: "invalid_token" }, + { + status: 401, + headers: { + "WWW-Authenticate": `Bearer realm="${base}", resource_metadata="${base}/.well-known/oauth-protected-resource"`, + }, + }, + ); + } + const mcpServer = createMcpServer(); + servers.push(mcpServer); + const transport = new WebStandardStreamableHTTPServerTransport({ + sessionIdGenerator: undefined, + }); + transports.push(transport); + await mcpServer.connect(transport); + return transport.handleRequest(req); + } + + return new Response("not found", { status: 404 }); + }, + }); + + return { + port: httpServer.port, + url: `http://localhost:${httpServer.port}/mcp`, + stop: () => { + for (const t of transports) t.close?.(); + for (const s of servers) s.close?.(); + httpServer.stop(true); + }, + }; +} + +describe("McpSource — OAuth retry path", () => { + let workDir: string; + let server: MockOAuthMcpServer; + + beforeEach(() => { + workDir = mkdtempSync(join(tmpdir(), "nb-mcp-oauth-retry-")); + server = startMockOAuthMcpServer(); + }); + + afterEach(() => { + server.stop(); + }); + + it("401 → OAuth → 200: start() completes and tools() returns the server's tools", async () => { + const provider = new WorkspaceOAuthProvider({ + wsId: "ws_test", + serverName: "retry-test", + workDir, + callbackUrl: CALLBACK, + // Mock runs on localhost; SSRF validator would otherwise reject. + allowInsecureRemotes: true, + }); + + const source = new McpSource( + "retry-test", + { + type: "remote", + url: new URL(server.url), + authProvider: provider, + }, + new NoopEventSink(), + ); + + await source.start(); + const tools = await source.tools(); + expect(tools.length).toBe(1); + expect(tools[0]?.name).toBe("retry-test__noop"); + + await source.stop(); + }, 15_000); +}); diff --git a/test/integration/remote-integration.test.ts b/test/integration/remote-integration.test.ts index 1b671ea..2683e9f 100644 --- a/test/integration/remote-integration.test.ts +++ b/test/integration/remote-integration.test.ts @@ -141,7 +141,7 @@ describe("Remote integration: config → validate → load → tools", () => { // Step 3: Start bundle source from the validated ref const registry = new ToolRegistry(); const ref: BundleRef = config.bundles[0] as BundleRef; - const meta = await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true }); + const meta = await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" }); expect(meta).not.toBeNull(); expect(meta.meta).not.toBeNull(); @@ -178,7 +178,7 @@ describe("Remote integration: config → validate → load → tools", () => { // Start source (auth headers won't affect our mock server) const registry = new ToolRegistry(); const ref: BundleRef = config.bundles[0] as BundleRef; - const meta = await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true }); + const meta = await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" }); expect(meta).not.toBeNull(); expect(registry.hasSource("authed-remote")).toBe(true); @@ -202,7 +202,7 @@ describe("Remote integration: config → validate → load → tools", () => { const registry = new ToolRegistry(); const ref: BundleRef = config.bundles[0] as BundleRef; - const results = await Promise.allSettled([startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true })]); + const results = await Promise.allSettled([startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" })]); expect(results[0]!.status).toBe("rejected"); expect(registry.hasSource("dead-remote")).toBe(false); }, 20_000); @@ -360,7 +360,7 @@ describe("Remote integration: registering remote bundles in workspace registry", // Register a remote bundle into the workspace registry const registry = runtime.getRegistryForWorkspace(TEST_WORKSPACE_ID); const ref: BundleRef = { url: mockServer.url, serverName: "runtime-remote" }; - await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true }); + await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" }); expect(registry.hasSource("runtime-remote")).toBe(true); @@ -387,14 +387,14 @@ describe("Remote integration: registering remote bundles in workspace registry", // Try to register a bad remote (should fail) const badRef: BundleRef = { url: "http://127.0.0.1:1/mcp", serverName: "bad-remote" }; const badResult = await Promise.allSettled([ - startBundleSource(badRef, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true }), + startBundleSource(badRef, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" }), ]); expect(badResult[0]!.status).toBe("rejected"); expect(registry.hasSource("bad-remote")).toBe(false); // Register a good remote (should succeed) const goodRef: BundleRef = { url: mockServer.url, serverName: "good-remote" }; - await startBundleSource(goodRef, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true }); + await startBundleSource(goodRef, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" }); expect(registry.hasSource("good-remote")).toBe(true); await registry.removeSource("good-remote"); diff --git a/test/integration/remote-lifecycle.test.ts b/test/integration/remote-lifecycle.test.ts index e72b005..26d3729 100644 --- a/test/integration/remote-lifecycle.test.ts +++ b/test/integration/remote-lifecycle.test.ts @@ -280,7 +280,7 @@ describe("startBundleSource — remote url entries", () => { serverName: "startup-remote", }; - const meta = await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true }); + const meta = await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" }); expect(meta).not.toBeNull(); expect(meta.meta).not.toBeNull(); @@ -301,7 +301,7 @@ describe("startBundleSource — remote url entries", () => { url: mockServer.url, }; - const meta = await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true }); + const meta = await startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" }); expect(meta).not.toBeNull(); // deriveServerName on a URL will produce something like "mcp" @@ -320,7 +320,7 @@ describe("startBundleSource — remote url entries", () => { // startBundleSource throws — but callers use allSettled const results = await Promise.allSettled([ - startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true }), + startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" }), ]); expect(results[0]!.status).toBe("rejected"); @@ -338,7 +338,7 @@ describe("startBundleSource — remote url entries", () => { ]; const results = await Promise.allSettled( - refs.map((ref) => startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true })), + refs.map((ref) => startBundleSource(ref, registry, new NoopEventSink(), undefined, { allowInsecureRemotes: true, wsId: "ws_test" })), ); // First two fail, third succeeds diff --git a/test/integration/workspace-oauth-provider.test.ts b/test/integration/workspace-oauth-provider.test.ts new file mode 100644 index 0000000..61360a8 --- /dev/null +++ b/test/integration/workspace-oauth-provider.test.ts @@ -0,0 +1,281 @@ +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, beforeEach, describe, expect, it } from "bun:test"; +import { + InteractiveOAuthNotSupportedError, + WorkspaceOAuthProvider, +} from "../../src/tools/workspace-oauth-provider.ts"; + +// Moved from `test/unit/` per AGENTS.md: any test that calls Bun.serve() +// belongs in integration. These cases exercise the provider's authorize- +// redirect probe against a real HTTP target on localhost; SSRF validation +// is explicitly opt-in via `allowInsecureRemotes: true` in each makeProvider +// call so the loopback targets aren't blocked. + +const CALLBACK = "http://localhost:27247/v1/mcp-auth/callback"; + +function makeProvider( + workDir: string, + overrides: { serverName?: string; callbackUrl?: string; allowInsecureRemotes?: boolean } = {}, +): WorkspaceOAuthProvider { + return new WorkspaceOAuthProvider({ + wsId: "ws_test", + serverName: overrides.serverName ?? "test-srv", + workDir, + callbackUrl: overrides.callbackUrl ?? CALLBACK, + // Most tests target localhost: via Bun.serve, which validateBundleUrl + // blocks by default. Each test opts in explicitly; the "SSRF block" test + // flips this off to assert the blocker works. + allowInsecureRemotes: overrides.allowInsecureRemotes ?? true, + }); +} + +describe("WorkspaceOAuthProvider — authorize redirect probe (headless)", () => { + let workDir: string; + beforeEach(() => { + workDir = mkdtempSync(join(tmpdir(), "nb-oauth-integ-")); + }); + + it("authorize endpoint 302 to our callback with code resolves pending flow", async () => { + const mockAuthServer = Bun.serve({ + port: 0, + fetch(req: Request) { + const u = new URL(req.url); + const state = u.searchParams.get("state") ?? ""; + const redirectUri = u.searchParams.get("redirect_uri") ?? CALLBACK; + const target = new URL(redirectUri); + target.searchParams.set("code", "anonymous"); + target.searchParams.set("state", state); + return new Response(null, { status: 302, headers: { location: target.toString() } }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir); + const state = p.state(); + authUrl.searchParams.set("state", state); + authUrl.searchParams.set("redirect_uri", CALLBACK); + const pending = p.awaitPendingFlow(); + + await p.redirectToAuthorization(authUrl); + await expect(pending).resolves.toBe("anonymous"); + } finally { + mockAuthServer.stop(true); + } + }); + + it("multi-hop same-origin redirects eventually hitting our callback (Reboot pattern)", async () => { + const mockAuthServer = Bun.serve({ + port: 0, + fetch(req: Request) { + const u = new URL(req.url); + if (u.pathname === "/authorize") { + const state = u.searchParams.get("state") ?? ""; + const redirectUri = u.searchParams.get("redirect_uri") ?? CALLBACK; + const interm = new URL(`http://localhost:${mockAuthServer.port}/intermediate`); + interm.searchParams.set("internal_token", "reboot-jwt"); + interm.searchParams.set("mcp_state", state); + interm.searchParams.set("mcp_redirect_uri", redirectUri); + return new Response(null, { status: 302, headers: { location: interm.toString() } }); + } + if (u.pathname === "/intermediate") { + const mcpState = u.searchParams.get("mcp_state") ?? ""; + const mcpRedirect = u.searchParams.get("mcp_redirect_uri") ?? CALLBACK; + const target = new URL(mcpRedirect); + target.searchParams.set("code", "anonymous"); + target.searchParams.set("state", mcpState); + return new Response(null, { status: 302, headers: { location: target.toString() } }); + } + return new Response("not found", { status: 404 }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir); + const state = p.state(); + authUrl.searchParams.set("state", state); + authUrl.searchParams.set("redirect_uri", CALLBACK); + const pending = p.awaitPendingFlow(); + + await p.redirectToAuthorization(authUrl); + await expect(pending).resolves.toBe("anonymous"); + } finally { + mockAuthServer.stop(true); + } + }); + + it("callback self-match tolerates trailing slash and explicit default port", async () => { + // Reviewer-called-out regression guard: the old self-match was a raw + // string === comparison that false-negative'd on trivial URL variants. + // Here the CONFIGURED callbackUrl has a trailing `/`, and the + // authorize server echoes an explicit default-port form back. Both + // should be treated as the same endpoint. + const mockAuthServer = Bun.serve({ + port: 0, + fetch(req: Request) { + const u = new URL(req.url); + const state = u.searchParams.get("state") ?? ""; + // Return the callback URL with a trailing slash stripped AND no + // explicit default port — i.e., a cosmetic variant of the + // configured callback. + const target = new URL("http://localhost:27247/v1/mcp-auth/callback"); + target.searchParams.set("code", "anonymous"); + target.searchParams.set("state", state); + return new Response(null, { status: 302, headers: { location: target.toString() } }); + }, + }); + try { + // Configured callback has a trailing slash. + const p = makeProvider(workDir, { + callbackUrl: "http://LOCALHOST:27247/v1/mcp-auth/callback/", + }); + const state = p.state(); + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + authUrl.searchParams.set("state", state); + const pending = p.awaitPendingFlow(); + + await p.redirectToAuthorization(authUrl); + await expect(pending).resolves.toBe("anonymous"); + } finally { + mockAuthServer.stop(true); + } + }); +}); + +describe("WorkspaceOAuthProvider — authorize redirect probe (interactive)", () => { + let workDir: string; + beforeEach(() => { + workDir = mkdtempSync(join(tmpdir(), "nb-oauth-integ-")); + }); + + it("302 to a non-self-target login page throws InteractiveOAuthNotSupportedError", async () => { + const mockAuthServer = Bun.serve({ + port: 0, + fetch(_req: Request) { + return new Response(null, { + status: 302, + headers: { location: "https://login.example.com/authenticate" }, + }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir); + const state = p.state(); + authUrl.searchParams.set("state", state); + const pending = p.awaitPendingFlow(); + const pendingSettled = pending.catch((err) => err); + + await expect(p.redirectToAuthorization(authUrl)).rejects.toBeInstanceOf( + InteractiveOAuthNotSupportedError, + ); + expect(await pendingSettled).toBeInstanceOf(InteractiveOAuthNotSupportedError); + } finally { + mockAuthServer.stop(true); + } + }); + + it("200 with a login form (no redirect) throws InteractiveOAuthNotSupportedError", async () => { + const mockAuthServer = Bun.serve({ + port: 0, + fetch(_req: Request) { + return new Response("login form", { + status: 200, + headers: { "content-type": "text/html" }, + }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir); + const state = p.state(); + authUrl.searchParams.set("state", state); + const pending = p.awaitPendingFlow(); + const pendingSettled = pending.catch((err) => err); + + await expect(p.redirectToAuthorization(authUrl)).rejects.toBeInstanceOf( + InteractiveOAuthNotSupportedError, + ); + expect(await pendingSettled).toBeInstanceOf(InteractiveOAuthNotSupportedError); + } finally { + mockAuthServer.stop(true); + } + }); +}); + +describe("WorkspaceOAuthProvider — SSRF defense", () => { + let workDir: string; + beforeEach(() => { + workDir = mkdtempSync(join(tmpdir(), "nb-oauth-integ-")); + }); + + it("blocks a loopback authorize URL when allowInsecureRemotes is false", async () => { + // Mock server that would gladly 302-to-metadata if reached — we should + // never reach it because validateBundleUrl blocks the initial hop. + const mockAuthServer = Bun.serve({ + port: 0, + fetch(_req: Request) { + return new Response(null, { + status: 302, + headers: { location: "http://169.254.169.254/latest/meta-data/" }, + }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir, { allowInsecureRemotes: false }); + p.state(); + const pending = p.awaitPendingFlow(); + const pendingSettled = pending.catch((err) => err); + + const thrown = await p.redirectToAuthorization(authUrl).catch((err) => err); + // SSRF block surfaces as an `[workspace-oauth-provider] SSRF block …` + // error rather than the generic "interactive not supported" — + // operators see the real cause. + expect(thrown).toBeInstanceOf(Error); + expect((thrown as Error).message).toMatch(/\[workspace-oauth-provider\] SSRF block/); + // The provider-local deferred is also rejected so awaitPendingFlow + // returns the same error (caller never hangs on a dead flow). + expect((await pendingSettled) as Error).toBeInstanceOf(Error); + } finally { + mockAuthServer.stop(true); + } + }); + + it("blocks a redirect-chain hop that points at cloud metadata", async () => { + // Authorize endpoint redirects to AWS IMDS. With allowInsecureRemotes=true + // for localhost, the initial hop passes, but the NEXT hop (169.254...) + // is non-loopback private and must be blocked regardless. + const mockAuthServer = Bun.serve({ + port: 0, + fetch(_req: Request) { + return new Response(null, { + status: 302, + headers: { location: "http://169.254.169.254/latest/meta-data/iam" }, + }); + }, + }); + try { + const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); + const p = makeProvider(workDir, { allowInsecureRemotes: true }); + p.state(); + const pending = p.awaitPendingFlow(); + const pendingSettled = pending.catch((err) => err); + + const thrown = await p.redirectToAuthorization(authUrl).catch((err) => err); + expect(thrown).toBeInstanceOf(Error); + expect((thrown as Error).message).toMatch(/\[workspace-oauth-provider\] SSRF block/); + expect((await pendingSettled) as Error).toBeInstanceOf(Error); + } finally { + mockAuthServer.stop(true); + } + }); +}); + +// Ensure the process's ref-counted handles from timers / pending flows don't +// keep test runner alive between suites. +afterAll(() => { + // No-op; each `it` cleans up its own server. This hook exists so the + // suite has an explicit teardown point for future additions. +}); diff --git a/test/unit/oauth-flow-registry.test.ts b/test/unit/oauth-flow-registry.test.ts index 40fe7e3..7bfdc45 100644 --- a/test/unit/oauth-flow-registry.test.ts +++ b/test/unit/oauth-flow-registry.test.ts @@ -35,4 +35,26 @@ describe("oauth-flow-registry", () => { expect(resolveWithCode("state-1", "a")).toBe(true); expect(resolveWithCode("state-1", "b")).toBe(false); }); + + it("rejects with a timeout error when TTL elapses without a callback", async () => { + // Intra-process leaks are the concern: an orphaned pending flow (tab + // closed, network failure) would keep a promise alive forever without + // a TTL. Use a tiny TTL to exercise the timer path quickly. + const p = register("state-ttl", "ws_test", "srv", 20); + await expect(p).rejects.toThrow(/timed out/i); + }); + + it("clearTimeout on resolve prevents late timer from firing stale reject", async () => { + // Resolve first, then wait past the TTL boundary. The timer must be + // cleared on resolve or we'd get an unhandled rejection from a late + // fire on an already-settled flow. + const p = register("state-resolved", "ws_test", "srv", 30); + resolveWithCode("state-resolved", "ok"); + await expect(p).resolves.toBe("ok"); + await new Promise((r) => setTimeout(r, 60)); + // If the timer had fired, `p` would have been re-rejected — but a + // Promise is immutable once settled, so a late reject would only show + // as an unhandled rejection. Absence of one (no diagnostic below) is + // our positive signal here. + }); }); diff --git a/test/unit/workspace-oauth-provider.test.ts b/test/unit/workspace-oauth-provider.test.ts index 87f62a3..3a3991e 100644 --- a/test/unit/workspace-oauth-provider.test.ts +++ b/test/unit/workspace-oauth-provider.test.ts @@ -6,10 +6,15 @@ import type { OAuthClientInformationFull, OAuthTokens, } from "@modelcontextprotocol/sdk/shared/auth.js"; -import { - InteractiveOAuthNotSupportedError, - WorkspaceOAuthProvider, -} from "../../src/tools/workspace-oauth-provider.ts"; +import { WorkspaceOAuthProvider } from "../../src/tools/workspace-oauth-provider.ts"; + +// Bun.serve-based cases (headless single-hop, headless multi-hop, interactive +// 302, interactive 200, SSRF block) live in +// `test/integration/workspace-oauth-provider.test.ts`, per AGENTS.md: +// "If a test calls Runtime.start(), startServer(), Bun.serve(), or +// spawnSync(), it belongs in test/integration/." +// This unit file covers file-IO roundtrips and `awaitPendingFlow` guards +// that don't need a real HTTP target. const CALLBACK = "http://localhost:27247/v1/mcp-auth/callback"; @@ -22,7 +27,7 @@ function makeProvider(workDir: string, serverName = "test-srv"): WorkspaceOAuthP }); } -describe("WorkspaceOAuthProvider", () => { +describe("WorkspaceOAuthProvider — file I/O roundtrips", () => { let workDir: string; beforeEach(() => { @@ -61,143 +66,6 @@ describe("WorkspaceOAuthProvider", () => { expect(read).toEqual(tokens); }); - it("headless flow: authorize endpoint 302 to our callback with code resolves pending flow", async () => { - // Stand up a mock authorize endpoint that behaves like Reboot's Anonymous: - // 302 straight back to the client's redirect_uri with ?code=anonymous&state=. - const mockAuthServer = Bun.serve({ - port: 0, - fetch(req: Request) { - const u = new URL(req.url); - const state = u.searchParams.get("state") ?? ""; - const redirectUri = u.searchParams.get("redirect_uri") ?? CALLBACK; - const target = new URL(redirectUri); - target.searchParams.set("code", "anonymous"); - target.searchParams.set("state", state); - return new Response(null, { status: 302, headers: { location: target.toString() } }); - }, - }); - try { - const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); - const p = makeProvider(workDir); - const state = p.state(); - authUrl.searchParams.set("state", state); - authUrl.searchParams.set("redirect_uri", CALLBACK); - const pending = p.awaitPendingFlow(); - - await p.redirectToAuthorization(authUrl); - await expect(pending).resolves.toBe("anonymous"); - } finally { - mockAuthServer.stop(true); - } - }); - - it("headless flow: multi-hop same-origin redirects eventually hitting our callback (Reboot pattern)", async () => { - // Mimics Reboot: /authorize 302s to /intermediate, which 302s to our callback. - const mockAuthServer = Bun.serve({ - port: 0, - fetch(req: Request) { - const u = new URL(req.url); - if (u.pathname === "/authorize") { - const state = u.searchParams.get("state") ?? ""; - const redirectUri = u.searchParams.get("redirect_uri") ?? CALLBACK; - // Hop 1: redirect to our own /intermediate with an internal token - const interm = new URL(`http://localhost:${mockAuthServer.port}/intermediate`); - interm.searchParams.set("internal_token", "reboot-jwt"); - interm.searchParams.set("mcp_state", state); - interm.searchParams.set("mcp_redirect_uri", redirectUri); - return new Response(null, { - status: 302, - headers: { location: interm.toString() }, - }); - } - if (u.pathname === "/intermediate") { - // Hop 2: redirect to the client's redirect_uri with the real code - const mcpState = u.searchParams.get("mcp_state") ?? ""; - const mcpRedirect = u.searchParams.get("mcp_redirect_uri") ?? CALLBACK; - const target = new URL(mcpRedirect); - target.searchParams.set("code", "anonymous"); - target.searchParams.set("state", mcpState); - return new Response(null, { - status: 302, - headers: { location: target.toString() }, - }); - } - return new Response("not found", { status: 404 }); - }, - }); - try { - const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); - const p = makeProvider(workDir); - const state = p.state(); - authUrl.searchParams.set("state", state); - authUrl.searchParams.set("redirect_uri", CALLBACK); - const pending = p.awaitPendingFlow(); - - await p.redirectToAuthorization(authUrl); - await expect(pending).resolves.toBe("anonymous"); - } finally { - mockAuthServer.stop(true); - } - }); - - it("interactive flow: authorize endpoint 302s to a login page throws InteractiveOAuthNotSupportedError", async () => { - // Mock authorize endpoint that redirects to a non-self-target login page - // (how Granola / Claude.ai / real OAuth providers behave). - const mockAuthServer = Bun.serve({ - port: 0, - fetch(_req: Request) { - return new Response(null, { - status: 302, - headers: { location: "https://login.example.com/authenticate" }, - }); - }, - }); - try { - const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); - const p = makeProvider(workDir); - const state = p.state(); - authUrl.searchParams.set("state", state); - const pending = p.awaitPendingFlow(); - const pendingSettled = pending.catch((err) => err); - - await expect(p.redirectToAuthorization(authUrl)).rejects.toBeInstanceOf( - InteractiveOAuthNotSupportedError, - ); - expect(await pendingSettled).toBeInstanceOf(InteractiveOAuthNotSupportedError); - } finally { - mockAuthServer.stop(true); - } - }); - - it("interactive flow: authorize endpoint 200s with login form throws InteractiveOAuthNotSupportedError", async () => { - // Providers that return 200 with an HTML login form (not a 302) are also - // interactive from our perspective — we can't extract a code. - const mockAuthServer = Bun.serve({ - port: 0, - fetch(_req: Request) { - return new Response("login form", { - status: 200, - headers: { "content-type": "text/html" }, - }); - }, - }); - try { - const authUrl = new URL(`http://localhost:${mockAuthServer.port}/authorize`); - const p = makeProvider(workDir); - const state = p.state(); - authUrl.searchParams.set("state", state); - const pending = p.awaitPendingFlow(); - const pendingSettled = pending.catch((err) => err); - - await expect(p.redirectToAuthorization(authUrl)).rejects.toBeInstanceOf( - InteractiveOAuthNotSupportedError, - ); - expect(await pendingSettled).toBeInstanceOf(InteractiveOAuthNotSupportedError); - } finally { - mockAuthServer.stop(true); - } - }); - it("verifier roundtrip + codeVerifier missing throws", async () => { const p = makeProvider(workDir); await p.saveCodeVerifier("pkce-verifier-xyz"); From 8666a757e5c9d21ce67cce6fd2291c35da7d29ed Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Thu, 23 Apr 2026 13:56:32 -1000 Subject: [PATCH 3/4] Lock wsId enforcement behind tests + document the rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two-part follow-up to the QA fix-up's credential-boundary guard: - Positive + negative integration tests for `startBundleSource`'s URL branch. (a) missing wsId + no static auth throws `requires opts.wsId`; (b) missing wsId WITH static auth starts cleanly — confirms the wsId requirement is scoped exactly to the path that would otherwise construct an OAuth provider. A future refactor that weakens the check to a default now fails CI. - `products/nimblebrain/code/CLAUDE.md`'s "Workspace Isolation" section gains a one-line rule: hard-error on missing `wsId` at credential boundaries, don't silently default. Sits next to the existing `requireWorkspaceId()` guidance — same rule family. The rule is enforceable by the test; the doc is a cross-reference for humans reading the section. Not saved to session memory — it's a codebase convention, versioned with the code. --- test/integration/remote-lifecycle.test.ts | 44 +++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/integration/remote-lifecycle.test.ts b/test/integration/remote-lifecycle.test.ts index 26d3729..fb7cf49 100644 --- a/test/integration/remote-lifecycle.test.ts +++ b/test/integration/remote-lifecycle.test.ts @@ -327,6 +327,50 @@ describe("startBundleSource — remote url entries", () => { expect(registry.hasSource("bad-remote")).toBe(false); }, 20_000); + it("url bundle without static auth + missing wsId throws (no silent ws_default fallback)", async () => { + // Credential-boundary guard: URL bundles that will open an OAuth flow + // must be workspace-scoped. A silent `?? "ws_default"` fallback would + // pool OAuth tokens across workspaces, so startBundleSource hard-errors + // instead. If someone refactors and weakens the check to a default, + // this test fails — which is the whole point. + const registry = new ToolRegistry(); + const ref: BundleRef = { + url: mockServer.url, + serverName: "no-ws", + // no transport.auth — triggers OAuth provider path + }; + + await expect( + startBundleSource(ref, registry, new NoopEventSink(), undefined, { + allowInsecureRemotes: true, + // wsId intentionally omitted + }), + ).rejects.toThrow(/requires opts\.wsId/); + expect(registry.hasSource("no-ws")).toBe(false); + }, 15_000); + + it("url bundle WITH static auth starts without wsId (no OAuth provider needed)", async () => { + // Complement to the above: when static auth is present, no OAuth + // provider is constructed, so missing wsId is not a credential- + // boundary concern. Confirms the wsId requirement is scoped exactly + // to the path that would otherwise leak credentials. + const registry = new ToolRegistry(); + const ref: BundleRef = { + url: mockServer.url, + serverName: "static-auth", + transport: { type: "streamable-http", auth: { type: "bearer", token: "t" } }, + }; + + const meta = await startBundleSource(ref, registry, new NoopEventSink(), undefined, { + allowInsecureRemotes: true, + // wsId intentionally omitted — allowed here + }); + expect(meta).not.toBeNull(); + expect(registry.hasSource("static-auth")).toBe(true); + + await registry.removeSource("static-auth"); + }, 15_000); + it("handles mix of name, path, and url entries via allSettled", async () => { const registry = new ToolRegistry(); From ed9301b577e4ce80bdbd7823bc683449f72fed81 Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Thu, 23 Apr 2026 13:56:58 -1000 Subject: [PATCH 4/4] Document wsId-at-credential-boundary rule in AGENTS.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Companion to 8666a75 — that commit caught the test but not the docs update because `git add CLAUDE.md` resolved the symlink to AGENTS.md and I didn't notice the status-line divergence. One-liner in the existing "Workspace Isolation" section pointing at the rule. --- AGENTS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 8edecd3..d95da54 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -108,6 +108,8 @@ web/ Vite + React + TypeScript SPA (separate package.json) All tool handlers that access data must be workspace-scoped. Use `runtime.requireWorkspaceId()` (never `getCurrentWorkspaceId()`). In dev mode it returns `"_dev"` — no special-case logic needed. +When adding a new code path that touches workspace-scoped credentials or identity, match the existing precedent: **hard-error on missing `wsId`, don't silently default**. `startBundleSource`'s named-bundle branch throws; the URL-bundle branch does too (for OAuth-provider paths). A `?? "ws_default"` fallback would pool credentials across tenants. + ## Debug Logging Hot-path diagnostics are gated behind namespace flags so they're available when you need them without editing source. Use for tracing across the runtime ↔ SSE ↔ browser ↔ iframe chain.