From 9396102e0754ceaf91b94f7f473546aebaeb177a Mon Sep 17 00:00:00 2001 From: suryaiyer95 Date: Tue, 5 May 2026 11:28:05 -0700 Subject: [PATCH 1/4] fix: [#791][#792] support bearer-auth remote MCP servers (Microsoft Fabric, etc.) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three coupled fixes that together let altimate-code connect to MCP servers gated by short-lived bearer tokens — most prominently Microsoft Fabric Core MCP — without a local proxy or per-hour config edits. Issues addressed: - #791 (dynamic auth for remote MCP servers): static `headers` only. - #792 (external MCP servers not auto-activated at session start): mostly caused by OAuth dynamic-client-registration probes pre-empting valid bearer headers and ending the connect in a non-`connected` status. - A third defect surfaced during this work: the strict `ListToolsResultSchema` from `@modelcontextprotocol/sdk` rejects the `null` annotation hints that Fabric returns, causing `listTools()` to throw and the connection to be marked failed even after a successful initialize. None of these are altimate-specific — they all reproduce on upstream `opencode`. Changes: 1. `McpRemote.headersCommand: Record` — header values produced by running an argv command (executed via `execFile`, not a shell, so values aren't subject to shell injection). Resolved on every connect so expiring tokens refresh automatically. Example: "headersCommand": { "Authorization": ["sh", "-c", "printf 'Bearer %s' \"$(az account get-access-token ... -o tsv)\""] } 2. OAuth provider is no longer attached when the user supplies an explicit `Authorization` header (statically or via `headersCommand`) and `oauth` is not specified. Prevents Entra ID's DCR rejection from short-circuiting a bearer-authenticated connection. Behavior is unchanged when `oauth` is explicitly configured. 3. `listTools()` is wrapped to retry with a permissive Zod schema on schema-validation errors. The fast path is unchanged for compliant servers; non-compliant servers (Fabric returns `null` for `annotations.{readOnlyHint,destructiveHint,idempotentHint,openWorldHint}`) no longer fail the connection. 4. `normalizeMcpConfig` was silently dropping `oauth` and (would have dropped) `headersCommand` when reconstructing remote entries. Both are now passed through. This was a pre-existing latent bug that made `oauth: false` configurations no-op at runtime. Tests: - 22 new tests across `test/mcp/mcp-bearer-auth.test.ts` (lenient schema, `headersCommand` resolution + execFile-not-shell semantics, Authorization detection, schema round-trip) and `test/mcp/headers.test.ts` (OAuth auto-disable on static bearer, on `headersCommand`, and explicit-OAuth override). All pass. - Zero regressions vs `origin/main` baseline on existing 185 MCP tests. - Live-verified end-to-end against `https://api.fabric.microsoft.com/v1/mcp/core` using `headersCommand` + `az account get-access-token`: server connected, 29 tools registered, no proxy required. Co-Authored-By: Claude Opus 4.7 --- packages/opencode/src/config/config.ts | 26 +- packages/opencode/src/mcp/index.ts | 172 +++++++++++- packages/opencode/test/mcp/headers.test.ts | 92 +++++- .../opencode/test/mcp/mcp-bearer-auth.test.ts | 263 ++++++++++++++++++ 4 files changed, 537 insertions(+), 16 deletions(-) create mode 100644 packages/opencode/test/mcp/mcp-bearer-auth.test.ts diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 05243196e..ccaca86bb 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -652,11 +652,26 @@ export namespace Config { url: z.string().describe("URL of the remote MCP server"), enabled: z.boolean().optional().describe("Enable or disable the MCP server on startup"), headers: z.record(z.string(), z.string()).optional().describe("Headers to send with the request"), + // altimate_change start — dynamic header values produced by a shell command, + // resolved on each (re)connect so callers can refresh expiring bearer tokens + // without restarting the session (e.g. `az account get-access-token`). + headersCommand: z + .record(z.string(), z.array(z.string()).nonempty()) + .optional() + .describe( + "Headers whose values are produced by running a command (argv form: [cmd, ...args]). " + + "stdout is trimmed and used as the header value. Resolved on every connect so tokens " + + "with short TTLs (e.g. Microsoft Entra ID bearer tokens for Fabric) refresh automatically. " + + "Values from headersCommand override matching keys in `headers`.", + ), + // altimate_change end oauth: z .union([McpOAuth, z.literal(false)]) .optional() .describe( - "OAuth authentication configuration for the MCP server. Set to false to disable OAuth auto-detection.", + "OAuth authentication configuration for the MCP server. Set to false to disable OAuth auto-detection. " + + "When `headers.Authorization` or `headersCommand.Authorization` is set and `oauth` is not specified, " + + "OAuth is disabled automatically so a static bearer token isn't overridden by a competing OAuth flow.", ), timeout: z .number() @@ -1452,6 +1467,15 @@ export namespace Config { } else if (entry.url && typeof entry.url === "string") { const transformed: Record = { type: "remote", url: entry.url } if (entry.headers && typeof entry.headers === "object") transformed.headers = entry.headers + // altimate_change start — preserve fields that the original normalizer dropped + // silently. Without these passes, a user-supplied `oauth: false` or + // `headersCommand` would be reconstructed-away, leaving the runtime + // believing the config was bare. See #791 / #792. + if (entry.headersCommand && typeof entry.headersCommand === "object") { + transformed.headersCommand = entry.headersCommand + } + if (entry.oauth !== undefined) transformed.oauth = entry.oauth + // altimate_change end if (typeof entry.timeout === "number") transformed.timeout = entry.timeout if (typeof entry.enabled === "boolean") transformed.enabled = entry.enabled if (typeof entry.updatedAt === "string") transformed.updatedAt = entry.updatedAt diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 1e4d72f37..dc41c4028 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -13,6 +13,12 @@ import { Config } from "../config/config" import { Log } from "../util/log" import { NamedError } from "@opencode-ai/util/error" import z from "zod/v4" +// altimate_change start — needed to resolve `headersCommand` for remote MCP +// servers that require bearer tokens with short TTLs (Microsoft Fabric, etc.) +import { execFile } from "node:child_process" +import { promisify } from "node:util" +const execFileAsync = promisify(execFile) +// altimate_change end import { Instance } from "../project/instance" import { Installation } from "../installation" // altimate_change start — persist enabled flag @@ -125,6 +131,118 @@ export namespace MCP { const toolListCache = new Map() // altimate_change end + // altimate_change start — Microsoft Fabric Core MCP returns `null` for + // `tool.annotations.{readOnlyHint,destructiveHint,idempotentHint,openWorldHint}`, + // which the SDK's `ListToolsResultSchema` (z.boolean().optional()) rejects via + // Zod, blocking listTools() entirely. We accept `null` as "hint absent" by + // calling `client.request()` with a permissive schema in place of the SDK's + // strict one. See https://github.com/AltimateAI/altimate-code/issues/792. + const LenientToolAnnotationsSchema = z + .object({ + title: z.string().optional(), + readOnlyHint: z.boolean().nullable().optional(), + destructiveHint: z.boolean().nullable().optional(), + idempotentHint: z.boolean().nullable().optional(), + openWorldHint: z.boolean().nullable().optional(), + }) + .loose() + + const LenientToolSchema = z + .object({ + name: z.string(), + title: z.string().optional(), + description: z.string().optional(), + inputSchema: z.any(), + outputSchema: z.any().optional(), + annotations: LenientToolAnnotationsSchema.optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + .loose() + + const LenientListToolsResultSchema = z + .object({ + tools: z.array(LenientToolSchema), + nextCursor: z.string().optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + .loose() + + function isSchemaError(err: unknown): boolean { + if (!err) return false + if (err instanceof Error && (err.name === "ZodError" || err.constructor?.name === "$ZodError")) return true + if (typeof err === "object" && err !== null && "issues" in err) return true + return false + } + + /** + * Calls the SDK's strict `listTools()` first; on a Zod schema-validation + * failure (e.g. server emits non-spec values like `null` annotation hints), + * retries via `client.request()` with a permissive schema. This keeps the + * fast path unchanged for compliant servers while letting non-compliant + * ones (Microsoft Fabric, etc.) still register their tools. + */ + async function listToolsLenient(client: MCPClient): Promise<{ tools: MCPToolDef[] }> { + try { + return await client.listTools() + } catch (err) { + if (!isSchemaError(err)) throw err + log.info("listTools strict schema rejected response, retrying with lenient schema", { + error: err instanceof Error ? err.message.slice(0, 200) : String(err).slice(0, 200), + }) + const result = await client.request( + { method: "tools/list", params: {} }, + LenientListToolsResultSchema as any, + ) + return result as { tools: MCPToolDef[] } + } + } + + /** @internal — exported only for unit tests. Prefer using `tools()` in production code. */ + export const _testing = { + LenientListToolsResultSchema, + isSchemaError, + resolveHeadersCommand: (spec: Record | undefined, key = "test") => + resolveHeadersCommand(spec, key), + hasAuthorizationHeader, + } + // altimate_change end + + // altimate_change start — resolve dynamic header values produced by shell + // commands (e.g. `az account get-access-token`). Runs via execFile (not a + // shell) so values aren't subject to shell injection. Re-runs on every + // connect so expiring bearer tokens refresh without manual config edits. + // See https://github.com/AltimateAI/altimate-code/issues/791. + async function resolveHeadersCommand( + spec: Record | undefined, + serverKey: string, + ): Promise> { + if (!spec) return {} + const out: Record = {} + for (const [name, argv] of Object.entries(spec)) { + if (!Array.isArray(argv) || argv.length === 0) { + throw new Error(`headersCommand[${name}] must be a non-empty argv array`) + } + const [cmd, ...args] = argv + const { stdout } = await execFileAsync(cmd, args, { + encoding: "utf-8", + maxBuffer: 1024 * 1024, + timeout: 30_000, + }) + const value = stdout.trim() + if (!value) { + throw new Error(`headersCommand[${name}] produced empty output`) + } + log.info("resolved dynamic header", { server: serverKey, header: name }) + out[name] = value + } + return out + } + + function hasAuthorizationHeader(headers: Record): boolean { + return Object.keys(headers).some((k) => k.toLowerCase() === "authorization") + } + // altimate_change end + // Register notification handlers for MCP client function registerNotificationHandlers(client: MCPClient, serverName: string) { client.setNotificationHandler(ToolListChangedNotificationSchema, async () => { @@ -368,8 +486,36 @@ export namespace MCP { let connectedTransport: "stdio" | "sse" | "streamable-http" | undefined = undefined if (mcp.type === "remote") { - // OAuth is enabled by default for remote servers unless explicitly disabled with oauth: false - const oauthDisabled = mcp.oauth === false + // altimate_change start — resolve dynamic headers (e.g. bearer tokens + // produced by `az account get-access-token`) before constructing + // transports. Failure to resolve aborts the connect attempt with a + // clear error so the user sees `failed: headersCommand[...] failed` + // in `mcp list` rather than a generic transport error. + let dynamicHeaders: Record = {} + try { + dynamicHeaders = await resolveHeadersCommand(mcp.headersCommand, key) + } catch (err) { + const message = err instanceof Error ? err.message : String(err) + log.error("headersCommand resolution failed", { key, error: message }) + return { + mcpClient: undefined, + status: { status: "failed" as const, error: `headersCommand failed: ${message}` }, + } + } + const mergedHeaders: Record = { ...(mcp.headers ?? {}), ...dynamicHeaders } + // altimate_change end + + // altimate_change start — OAuth is enabled by default for remote servers, + // BUT if the user provided an explicit Authorization header (statically or + // via headersCommand) and didn't ask for OAuth, skip OAuth so the bearer + // header isn't pre-empted by an OAuth flow that fails (e.g. Microsoft + // Entra ID rejects RFC 7591 dynamic client registration). See #792. + const oauthExplicitlyDisabled = mcp.oauth === false + const oauthExplicitlyConfigured = typeof mcp.oauth === "object" + const oauthDisabled = + oauthExplicitlyDisabled || + (!oauthExplicitlyConfigured && hasAuthorizationHeader(mergedHeaders)) + // altimate_change end const oauthConfig = typeof mcp.oauth === "object" ? mcp.oauth : undefined let authProvider: McpOAuthProvider | undefined @@ -391,22 +537,25 @@ export namespace MCP { ) } + // altimate_change start — pass merged (static + dynamic) headers to transports + const requestInit = Object.keys(mergedHeaders).length > 0 ? { headers: mergedHeaders } : undefined const transports: Array<{ name: string; transport: TransportWithAuth }> = [ { name: "StreamableHTTP", transport: new StreamableHTTPClientTransport(new URL(mcp.url), { authProvider, - requestInit: mcp.headers ? { headers: mcp.headers } : undefined, + requestInit, }), }, { name: "SSE", transport: new SSEClientTransport(new URL(mcp.url), { authProvider, - requestInit: mcp.headers ? { headers: mcp.headers } : undefined, + requestInit, }), }, ] + // altimate_change end let lastError: Error | undefined const connectTimeout = mcp.timeout ?? DEFAULT_TIMEOUT @@ -433,8 +582,10 @@ export namespace MCP { duration_ms: Date.now() - connectStart, }) // altimate_change start — bridge merge: prefetch tool list synchronously - // for cache so MCP.tools() doesn't re-call listTools. - const toolsList = await client.listTools().catch(() => undefined) + // for cache so MCP.tools() doesn't re-call listTools. Use lenient + // schema so servers that emit `null` annotation hints (e.g. Fabric) + // don't trip Zod validation. See #792. + const toolsList = await listToolsLenient(client).catch(() => undefined) if (toolsList) toolListCache.set(key, toolsList.tools) // altimate_change end // Census: collect resource counts (fire-and-forget, never block connect) @@ -571,8 +722,9 @@ export namespace MCP { duration_ms: Date.now() - localConnectStart, }) // altimate_change start — bridge merge: prefetch tool list synchronously - // for cache so MCP.tools() doesn't re-call listTools. - const toolsListSync = await client.listTools().catch(() => undefined) + // for cache so MCP.tools() doesn't re-call listTools. Use lenient schema + // so non-compliant annotation hints (`null`) don't fail validation. + const toolsListSync = await listToolsLenient(client).catch(() => undefined) if (toolsListSync) toolListCache.set(key, toolsListSync.tools) // altimate_change end // Census: collect resource counts (fire-and-forget, never block connect) @@ -639,7 +791,7 @@ export namespace MCP { const cachedTools = toolListCache.get(key) const result = cachedTools ? { tools: cachedTools } - : await withTimeout(mcpClient.listTools(), mcp.timeout ?? DEFAULT_TIMEOUT).catch((err) => { + : await withTimeout(listToolsLenient(mcpClient), mcp.timeout ?? DEFAULT_TIMEOUT).catch((err) => { log.error("failed to get tools from client", { key, error: err }) return undefined }) @@ -836,7 +988,7 @@ export namespace MCP { if (cached) { return { clientName, client, toolsResult: { tools: cached } } } - const toolsResult = await client.listTools().catch((e) => { + const toolsResult = await listToolsLenient(client).catch((e) => { log.error("failed to get tools", { clientName, error: e.message }) const failedStatus = { status: "failed" as const, diff --git a/packages/opencode/test/mcp/headers.test.ts b/packages/opencode/test/mcp/headers.test.ts index 8c488d4c4..d4fb3f55b 100644 --- a/packages/opencode/test/mcp/headers.test.ts +++ b/packages/opencode/test/mcp/headers.test.ts @@ -47,7 +47,7 @@ const { MCP } = await import("../../src/mcp/index") const { Instance } = await import("../../src/project/instance") const { tmpdir } = await import("../fixture/fixture") -test("headers are passed to transports when oauth is enabled (default)", async () => { +test("headers are passed to transports when oauth is enabled (default, no Authorization header)", async () => { await using tmp = await tmpdir({ init: async (dir) => { await Bun.write( @@ -59,8 +59,8 @@ test("headers are passed to transports when oauth is enabled (default)", async ( type: "remote", url: "https://example.com/mcp", headers: { - Authorization: "Bearer test-token", "X-Custom-Header": "custom-value", + "X-Trace-Id": "trace-1", }, }, }, @@ -77,8 +77,8 @@ test("headers are passed to transports when oauth is enabled (default)", async ( type: "remote", url: "https://example.com/mcp", headers: { - Authorization: "Bearer test-token", "X-Custom-Header": "custom-value", + "X-Trace-Id": "trace-1", }, }).catch(() => {}) @@ -88,15 +88,97 @@ test("headers are passed to transports when oauth is enabled (default)", async ( for (const call of transportCalls) { expect(call.options.requestInit).toBeDefined() expect(call.options.requestInit?.headers).toEqual({ - Authorization: "Bearer test-token", "X-Custom-Header": "custom-value", + "X-Trace-Id": "trace-1", }) - // OAuth should be enabled by default, so authProvider should exist + // OAuth should be enabled by default when no Authorization header is provided. + expect(call.options.authProvider).toBeDefined() + } + }, + }) +}) + +// altimate_change start — covers the OAuth auto-disable behavior added for +// https://github.com/AltimateAI/altimate-code/issues/792. When the user +// supplies an explicit Authorization header (statically or via headersCommand), +// the OAuth provider is not attached, so a failing OAuth flow (e.g. Microsoft +// Entra ID rejecting RFC 7591 dynamic client registration) cannot pre-empt the +// bearer token. +test("OAuth is auto-disabled when an explicit Authorization header is present", async () => { + await using tmp = await tmpdir() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + transportCalls.length = 0 + await MCP.add("auto-disable-server", { + type: "remote", + url: "https://example.com/mcp", + headers: { + Authorization: "Bearer static-token", + "X-Custom-Header": "x", + }, + }).catch(() => {}) + + expect(transportCalls.length).toBeGreaterThanOrEqual(1) + for (const call of transportCalls) { + expect(call.options.requestInit?.headers).toMatchObject({ + Authorization: "Bearer static-token", + }) + // No authProvider — OAuth was auto-disabled because user provided bearer. + expect(call.options.authProvider).toBeUndefined() + } + }, + }) +}) + +test("OAuth is auto-disabled when Authorization is supplied via headersCommand", async () => { + await using tmp = await tmpdir() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + transportCalls.length = 0 + await MCP.add("auto-disable-cmd-server", { + type: "remote", + url: "https://example.com/mcp", + headersCommand: { + Authorization: ["printf", "Bearer dynamic-token"], + }, + } as any).catch(() => {}) + + expect(transportCalls.length).toBeGreaterThanOrEqual(1) + for (const call of transportCalls) { + expect(call.options.requestInit?.headers).toMatchObject({ + Authorization: "Bearer dynamic-token", + }) + expect(call.options.authProvider).toBeUndefined() + } + }, + }) +}) + +test("OAuth still attaches when Authorization header is present but oauth is explicitly configured", async () => { + await using tmp = await tmpdir() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + transportCalls.length = 0 + await MCP.add("explicit-oauth-server", { + type: "remote", + url: "https://example.com/mcp", + headers: { Authorization: "Bearer fallback" }, + oauth: { clientId: "client-xyz" }, + }).catch(() => {}) + + expect(transportCalls.length).toBeGreaterThanOrEqual(1) + for (const call of transportCalls) { + // User explicitly opted in to OAuth, so provider is attached even + // though a static Authorization header is also present. expect(call.options.authProvider).toBeDefined() } }, }) }) +// altimate_change end test("headers are passed to transports when oauth is explicitly disabled", async () => { await using tmp = await tmpdir() diff --git a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts new file mode 100644 index 000000000..7da2d7019 --- /dev/null +++ b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts @@ -0,0 +1,263 @@ +import { describe, test, expect } from "bun:test" +import z from "zod/v4" +import { Config } from "../../src/config/config" + +// We replicate the lenient ListTools schema used in mcp/index.ts here so the +// test does not depend on the MCP module's heavy import surface (Instance, +// Telemetry, Bus, Plugin, etc.). The schema is byte-for-byte identical to +// the one in mcp/index.ts; if it drifts, the integration test in mcp.test.ts +// will catch it. +const LenientToolAnnotationsSchema = z + .object({ + title: z.string().optional(), + readOnlyHint: z.boolean().nullable().optional(), + destructiveHint: z.boolean().nullable().optional(), + idempotentHint: z.boolean().nullable().optional(), + openWorldHint: z.boolean().nullable().optional(), + }) + .loose() + +const LenientToolSchema = z + .object({ + name: z.string(), + title: z.string().optional(), + description: z.string().optional(), + inputSchema: z.any(), + outputSchema: z.any().optional(), + annotations: LenientToolAnnotationsSchema.optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + .loose() + +const LenientListToolsResultSchema = z + .object({ + tools: z.array(LenientToolSchema), + nextCursor: z.string().optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + .loose() + +// --------------------------------------------------------------------------- +// 1. Lenient tools/list schema accepts what real-world servers emit. +// --------------------------------------------------------------------------- +describe("lenient tools/list schema", () => { + test("accepts null annotation hints (Microsoft Fabric Core MCP behavior)", () => { + // Real payload shape we observed from https://api.fabric.microsoft.com/v1/mcp/core + const fabricStyleResponse = { + tools: [ + { + name: "list_workspaces", + description: "Lists all Microsoft fabric workspaces user has access to.", + inputSchema: { type: "object", properties: {} }, + annotations: { + title: "List Workspaces", + readOnlyHint: true, + destructiveHint: null, + idempotentHint: null, + openWorldHint: null, + }, + }, + ], + } + const result = LenientListToolsResultSchema.safeParse(fabricStyleResponse) + expect(result.success).toBe(true) + if (result.success) { + expect(result.data.tools).toHaveLength(1) + expect(result.data.tools[0].name).toBe("list_workspaces") + } + }) + + test("accepts proper boolean annotation hints (compliant servers)", () => { + const compliantResponse = { + tools: [ + { + name: "delete_workspace", + inputSchema: { type: "object" }, + annotations: { + readOnlyHint: false, + destructiveHint: true, + idempotentHint: true, + openWorldHint: false, + }, + }, + ], + } + const result = LenientListToolsResultSchema.safeParse(compliantResponse) + expect(result.success).toBe(true) + }) + + test("accepts tools without annotations at all", () => { + const result = LenientListToolsResultSchema.safeParse({ + tools: [{ name: "minimal", inputSchema: {} }], + }) + expect(result.success).toBe(true) + }) + + test("rejects malformed top-level (missing tools array)", () => { + expect(LenientListToolsResultSchema.safeParse({ tools: "not-an-array" }).success).toBe(false) + expect(LenientListToolsResultSchema.safeParse({}).success).toBe(false) + }) + + test("preserves unknown fields via .loose() (forward compatibility)", () => { + const future = { + tools: [{ name: "x", inputSchema: {}, futureField: { nested: 1 } }], + futureTopLevel: "ok", + } + const result = LenientListToolsResultSchema.safeParse(future) + expect(result.success).toBe(true) + }) +}) + +// --------------------------------------------------------------------------- +// 2. McpRemote schema accepts new headersCommand field (issue #791). +// --------------------------------------------------------------------------- +describe("McpRemote.headersCommand schema (#791)", () => { + test("accepts headersCommand as record of header → argv", () => { + const config = { + type: "remote" as const, + url: "https://example.com/mcp", + headersCommand: { + Authorization: ["az", "account", "get-access-token", "--query", "accessToken", "-o", "tsv"], + }, + } + const result = Config.McpRemote.safeParse(config) + expect(result.success).toBe(true) + }) + + test("rejects headersCommand with empty argv (would silently no-op at runtime)", () => { + const result = Config.McpRemote.safeParse({ + type: "remote", + url: "https://example.com/mcp", + headersCommand: { Authorization: [] }, + }) + expect(result.success).toBe(false) + }) + + test("allows static headers and headersCommand to coexist", () => { + const config = { + type: "remote" as const, + url: "https://example.com/mcp", + headers: { "X-Trace-Id": "abc" }, + headersCommand: { Authorization: ["echo", "Bearer xyz"] }, + } + const result = Config.McpRemote.safeParse(config) + expect(result.success).toBe(true) + }) + + test("headersCommand is optional (existing configs still validate)", () => { + const result = Config.McpRemote.safeParse({ + type: "remote", + url: "https://example.com/mcp", + headers: { Authorization: "Bearer static" }, + }) + expect(result.success).toBe(true) + }) +}) + +// --------------------------------------------------------------------------- +// 3. headersCommand resolution behavior (#791). +// Tests the actual helper from the MCP module. +// --------------------------------------------------------------------------- +describe("resolveHeadersCommand helper", () => { + test("returns empty object when spec is undefined", async () => { + const { MCP } = await import("../../src/mcp") + const result = await MCP._testing.resolveHeadersCommand(undefined) + expect(result).toEqual({}) + }) + + test("runs argv via execFile and uses trimmed stdout as header value", async () => { + const { MCP } = await import("../../src/mcp") + const result = await MCP._testing.resolveHeadersCommand({ + Authorization: ["printf", "Bearer hello-world"], + "X-Trace": ["printf", "trace-123\n"], + }) + expect(result.Authorization).toBe("Bearer hello-world") + expect(result["X-Trace"]).toBe("trace-123") + }) + + test("throws when command emits empty output", async () => { + const { MCP } = await import("../../src/mcp") + await expect(MCP._testing.resolveHeadersCommand({ Authorization: ["true"] })).rejects.toThrow( + /produced empty output/, + ) + }) + + test("throws when command does not exist", async () => { + const { MCP } = await import("../../src/mcp") + await expect( + MCP._testing.resolveHeadersCommand({ Authorization: ["this-binary-does-not-exist-xyz"] }), + ).rejects.toThrow() + }) + + test("does not invoke a shell (argv is passed directly to execFile)", async () => { + // If a shell were used, the metacharacters below would be interpreted. + // execFile passes argv directly, so the literal string is echoed back. + const { MCP } = await import("../../src/mcp") + const result = await MCP._testing.resolveHeadersCommand({ + X: ["printf", "%s", "$(whoami); rm -rf /"], + }) + expect(result.X).toBe("$(whoami); rm -rf /") + }) +}) + +// --------------------------------------------------------------------------- +// 4. Authorization-header detection used to auto-disable OAuth (#792). +// --------------------------------------------------------------------------- +describe("hasAuthorizationHeader helper (#792)", () => { + test("matches case-insensitively", async () => { + const { MCP } = await import("../../src/mcp") + expect(MCP._testing.hasAuthorizationHeader({ Authorization: "Bearer x" })).toBe(true) + expect(MCP._testing.hasAuthorizationHeader({ authorization: "Bearer x" })).toBe(true) + expect(MCP._testing.hasAuthorizationHeader({ AUTHORIZATION: "Bearer x" })).toBe(true) + }) + + test("returns false when no auth header is present", async () => { + const { MCP } = await import("../../src/mcp") + expect(MCP._testing.hasAuthorizationHeader({})).toBe(false) + expect(MCP._testing.hasAuthorizationHeader({ "X-Trace": "abc" })).toBe(false) + }) + + test("does not match prefixes that merely contain 'authorization'", async () => { + const { MCP } = await import("../../src/mcp") + expect(MCP._testing.hasAuthorizationHeader({ "X-Authorization-Type": "Bearer" })).toBe(false) + expect(MCP._testing.hasAuthorizationHeader({ "Pre-Authorization": "x" })).toBe(false) + }) +}) + +// --------------------------------------------------------------------------- +// 5. normalizeMcpConfig preserves headersCommand and oauth (round-trip). +// +// Without this, the field-stripping normalizer drops user-supplied values +// silently, leaving the runtime to behave as if the user hadn't configured +// them. See #791 / #792. +// --------------------------------------------------------------------------- +describe("config normalize round-trip", () => { + test("McpRemote with headersCommand survives Mcp parse", () => { + // Simulates the post-normalize entry: with our fix, the load path + // forwards `headersCommand` through into the typed shape. + const entry = { + type: "remote", + url: "https://example.com/mcp", + headersCommand: { Authorization: ["echo", "Bearer x"] }, + } + const result = Config.Mcp.safeParse(entry) + expect(result.success).toBe(true) + if (result.success && result.data.type === "remote") { + expect(result.data.headersCommand).toEqual({ Authorization: ["echo", "Bearer x"] }) + } + }) + + test("McpRemote with oauth=false survives Mcp parse", () => { + const entry = { + type: "remote", + url: "https://example.com/mcp", + headers: { Authorization: "Bearer x" }, + oauth: false, + } + const result = Config.Mcp.safeParse(entry) + expect(result.success).toBe(true) + if (result.success && result.data.type === "remote") { + expect(result.data.oauth).toBe(false) + } + }) +}) From 3157a5ad8fc047860d1358f1dfbe31fa095b79c9 Mon Sep 17 00:00:00 2001 From: suryaiyer95 Date: Tue, 23 Jun 2026 10:30:10 -0700 Subject: [PATCH 2/4] fix: [#791][#792] address review feedback on bearer-auth remote MCP Follow-up to f6b2e2dbc resolving all bot review comments: - `isSchemaError`: narrow to `name === ZodError|$ZodError` AND an `issues` array, so unrelated errors carrying an `issues` property no longer trigger the lenient retry (Copilot). - `resolveHeadersCommand`: wrap `execFile` failures with the header key (`headersCommand[] failed: ...`) and append trimmed `stderr` so `mcp list` points to the exact failing command (Copilot). - Drop the now-redundant `headersCommand failed:` prefix at the call site since the thrown message already names the header. - Use Zod v4 `z.looseObject(...)` instead of deprecated `.loose()` for the lenient `tools/list` schemas (CodeRabbit). - `config.ts`: reword `headersCommand` comment to clarify it is argv via `execFile` (no shell interpolation); document why `normalizeMcpConfig` passes malformed array shapes through (schema rejects them with an actionable `invalid_type` rather than silently dropping) (Copilot). - Tests: remove unnecessary `as any` cast; assert against the production `LenientListToolsResultSchema` via `MCP._testing` instead of a duplicated copy; add an end-to-end `listToolsLenient` retry test using a real SDK `$ZodError`, and a test that array-shaped headers are rejected loudly. Co-Authored-By: Claude Opus 4.8 --- packages/opencode/src/config/config.ts | 14 ++- packages/opencode/src/mcp/index.ts | 112 ++++++++++------- packages/opencode/test/mcp/headers.test.ts | 2 +- .../opencode/test/mcp/mcp-bearer-auth.test.ts | 118 ++++++++++++------ 4 files changed, 159 insertions(+), 87 deletions(-) diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index ccaca86bb..529c28b2b 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -652,9 +652,11 @@ export namespace Config { url: z.string().describe("URL of the remote MCP server"), enabled: z.boolean().optional().describe("Enable or disable the MCP server on startup"), headers: z.record(z.string(), z.string()).optional().describe("Headers to send with the request"), - // altimate_change start — dynamic header values produced by a shell command, - // resolved on each (re)connect so callers can refresh expiring bearer tokens - // without restarting the session (e.g. `az account get-access-token`). + // altimate_change start — dynamic header values produced by an argv command + // (run via execFile, not a shell — no shell interpolation unless the user + // explicitly invokes one like `sh -c`), resolved on each (re)connect so + // callers can refresh expiring bearer tokens without restarting the session + // (e.g. `az account get-access-token`). headersCommand: z .record(z.string(), z.array(z.string()).nonempty()) .optional() @@ -1466,6 +1468,12 @@ export namespace Config { servers[name] = transformed } else if (entry.url && typeof entry.url === "string") { const transformed: Record = { type: "remote", url: entry.url } + // Copy `headers` / `headersCommand` through as-is — including malformed + // array shapes. The downstream `Info.safeParse` validates `mcp` against + // the McpRemote schema, and `z.record(...)` rejects an array with an + // actionable `invalid_type` error. Stripping arrays here would instead + // drop the field silently and connect a header-less server with no + // feedback to the user. See #791 / #792. if (entry.headers && typeof entry.headers === "object") transformed.headers = entry.headers // altimate_change start — preserve fields that the original normalizer dropped // silently. Without these passes, a user-supplied `oauth: false` or diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index dc41c4028..27babe262 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -137,41 +137,42 @@ export namespace MCP { // Zod, blocking listTools() entirely. We accept `null` as "hint absent" by // calling `client.request()` with a permissive schema in place of the SDK's // strict one. See https://github.com/AltimateAI/altimate-code/issues/792. - const LenientToolAnnotationsSchema = z - .object({ - title: z.string().optional(), - readOnlyHint: z.boolean().nullable().optional(), - destructiveHint: z.boolean().nullable().optional(), - idempotentHint: z.boolean().nullable().optional(), - openWorldHint: z.boolean().nullable().optional(), - }) - .loose() - - const LenientToolSchema = z - .object({ - name: z.string(), - title: z.string().optional(), - description: z.string().optional(), - inputSchema: z.any(), - outputSchema: z.any().optional(), - annotations: LenientToolAnnotationsSchema.optional(), - _meta: z.record(z.string(), z.unknown()).optional(), - }) - .loose() - - const LenientListToolsResultSchema = z - .object({ - tools: z.array(LenientToolSchema), - nextCursor: z.string().optional(), - _meta: z.record(z.string(), z.unknown()).optional(), - }) - .loose() + const LenientToolAnnotationsSchema = z.looseObject({ + title: z.string().optional(), + readOnlyHint: z.boolean().nullable().optional(), + destructiveHint: z.boolean().nullable().optional(), + idempotentHint: z.boolean().nullable().optional(), + openWorldHint: z.boolean().nullable().optional(), + }) + + const LenientToolSchema = z.looseObject({ + name: z.string(), + title: z.string().optional(), + description: z.string().optional(), + inputSchema: z.any(), + outputSchema: z.any().optional(), + annotations: LenientToolAnnotationsSchema.optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + + const LenientListToolsResultSchema = z.looseObject({ + tools: z.array(LenientToolSchema), + nextCursor: z.string().optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) function isSchemaError(err: unknown): boolean { - if (!err) return false - if (err instanceof Error && (err.name === "ZodError" || err.constructor?.name === "$ZodError")) return true - if (typeof err === "object" && err !== null && "issues" in err) return true - return false + // Narrowly match Zod validation errors only. We key off the error *name* + // (not `instanceof z.ZodError`) because the MCP SDK may throw a ZodError + // from a different zod copy (it depends on `zod@^3.25 || ^4.0`), which + // would fail a cross-realm `instanceof`. In practice the SDK's z4-mini path + // names the error `$ZodError` (the primary case); the public ZodError + // subclass names it `ZodError`. We require an `issues` *array* (rather than + // `"issues" in err`) to avoid a throwing getter and to reject unrelated + // errors that merely carry an `issues` property of some other type. + if (typeof err !== "object" || err === null) return false + const name = (err as { name?: string }).name ?? (err as { constructor?: { name?: string } }).constructor?.name + return (name === "ZodError" || name === "$ZodError") && Array.isArray((err as { issues?: unknown }).issues) } /** @@ -201,16 +202,20 @@ export namespace MCP { export const _testing = { LenientListToolsResultSchema, isSchemaError, + listToolsLenient: (client: { + listTools: () => Promise<{ tools: MCPToolDef[] }> + request: (...args: any[]) => Promise + }) => listToolsLenient(client as unknown as MCPClient), resolveHeadersCommand: (spec: Record | undefined, key = "test") => resolveHeadersCommand(spec, key), hasAuthorizationHeader, } // altimate_change end - // altimate_change start — resolve dynamic header values produced by shell - // commands (e.g. `az account get-access-token`). Runs via execFile (not a - // shell) so values aren't subject to shell injection. Re-runs on every - // connect so expiring bearer tokens refresh without manual config edits. + // altimate_change start — resolve dynamic header values from argv commands + // (e.g. `az account get-access-token`). Each value is an argv array run via + // execFile (no shell) so values aren't subject to shell injection. Re-runs on + // every connect so expiring bearer tokens refresh without manual config edits. // See https://github.com/AltimateAI/altimate-code/issues/791. async function resolveHeadersCommand( spec: Record | undefined, @@ -223,11 +228,26 @@ export namespace MCP { throw new Error(`headersCommand[${name}] must be a non-empty argv array`) } const [cmd, ...args] = argv - const { stdout } = await execFileAsync(cmd, args, { - encoding: "utf-8", - maxBuffer: 1024 * 1024, - timeout: 30_000, - }) + let stdout = "" + try { + stdout = ( + await execFileAsync(cmd, args, { + encoding: "utf-8", + maxBuffer: 1024 * 1024, + timeout: 30_000, + }) + ).stdout + } catch (err) { + // Wrap with the header key so `mcp list` points to the exact failing + // command (ENOENT, timeout, non-zero exit) rather than a bare error. + // On a non-zero exit the actionable reason lives on `err.stderr` (e.g. + // `az`'s "run 'az login'"), which `err.message` omits — append it. + const e = err as { message?: string; stderr?: string } + const stderr = typeof e.stderr === "string" ? e.stderr.trim().slice(0, 500) : "" + const base = err instanceof Error ? err.message : String(err) + const message = stderr ? `${base}: ${stderr}` : base + throw new Error(`headersCommand[${name}] failed: ${message}`) + } const value = stdout.trim() if (!value) { throw new Error(`headersCommand[${name}] produced empty output`) @@ -488,9 +508,9 @@ export namespace MCP { if (mcp.type === "remote") { // altimate_change start — resolve dynamic headers (e.g. bearer tokens // produced by `az account get-access-token`) before constructing - // transports. Failure to resolve aborts the connect attempt with a - // clear error so the user sees `failed: headersCommand[...] failed` - // in `mcp list` rather than a generic transport error. + // transports. Failure to resolve aborts the connect attempt. The thrown + // message already names the failing header (`headersCommand[] failed: + // ...`), so the user sees exactly which command broke in `mcp list`. let dynamicHeaders: Record = {} try { dynamicHeaders = await resolveHeadersCommand(mcp.headersCommand, key) @@ -499,7 +519,7 @@ export namespace MCP { log.error("headersCommand resolution failed", { key, error: message }) return { mcpClient: undefined, - status: { status: "failed" as const, error: `headersCommand failed: ${message}` }, + status: { status: "failed" as const, error: message }, } } const mergedHeaders: Record = { ...(mcp.headers ?? {}), ...dynamicHeaders } diff --git a/packages/opencode/test/mcp/headers.test.ts b/packages/opencode/test/mcp/headers.test.ts index d4fb3f55b..8ce78dd2b 100644 --- a/packages/opencode/test/mcp/headers.test.ts +++ b/packages/opencode/test/mcp/headers.test.ts @@ -143,7 +143,7 @@ test("OAuth is auto-disabled when Authorization is supplied via headersCommand", headersCommand: { Authorization: ["printf", "Bearer dynamic-token"], }, - } as any).catch(() => {}) + }).catch(() => {}) expect(transportCalls.length).toBeGreaterThanOrEqual(1) for (const call of transportCalls) { diff --git a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts index 7da2d7019..71779cd24 100644 --- a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts +++ b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts @@ -1,41 +1,12 @@ import { describe, test, expect } from "bun:test" -import z from "zod/v4" import { Config } from "../../src/config/config" -// We replicate the lenient ListTools schema used in mcp/index.ts here so the -// test does not depend on the MCP module's heavy import surface (Instance, -// Telemetry, Bus, Plugin, etc.). The schema is byte-for-byte identical to -// the one in mcp/index.ts; if it drifts, the integration test in mcp.test.ts -// will catch it. -const LenientToolAnnotationsSchema = z - .object({ - title: z.string().optional(), - readOnlyHint: z.boolean().nullable().optional(), - destructiveHint: z.boolean().nullable().optional(), - idempotentHint: z.boolean().nullable().optional(), - openWorldHint: z.boolean().nullable().optional(), - }) - .loose() - -const LenientToolSchema = z - .object({ - name: z.string(), - title: z.string().optional(), - description: z.string().optional(), - inputSchema: z.any(), - outputSchema: z.any().optional(), - annotations: LenientToolAnnotationsSchema.optional(), - _meta: z.record(z.string(), z.unknown()).optional(), - }) - .loose() - -const LenientListToolsResultSchema = z - .object({ - tools: z.array(LenientToolSchema), - nextCursor: z.string().optional(), - _meta: z.record(z.string(), z.unknown()).optional(), - }) - .loose() +// Assert against the *production* lenient schema directly (exported via +// MCP._testing) so this test can never pass against a stale duplicate. The MCP +// module is already imported dynamically by the resolveHeadersCommand tests +// below, so pulling it in here adds no new import surface. +const { MCP } = await import("../../src/mcp") +const { LenientListToolsResultSchema } = MCP._testing // --------------------------------------------------------------------------- // 1. Lenient tools/list schema accepts what real-world servers emit. @@ -108,6 +79,67 @@ describe("lenient tools/list schema", () => { }) }) +// --------------------------------------------------------------------------- +// 1b. End-to-end listToolsLenient retry (#792). Locks in the load-bearing +// contract: when the SDK's strict listTools() rejects a Fabric-style payload, +// the rejected error is named `$ZodError` (zod v4-mini), isSchemaError catches +// it, and the lenient client.request() retry returns the tools. A future +// re-narrowing of isSchemaError would break this without tripping the +// schema-only tests above. +// --------------------------------------------------------------------------- +describe("listToolsLenient retry against SDK $ZodError (#792)", () => { + test("retries with lenient schema when strict listTools() rejects Fabric-style nulls", async () => { + const { ListToolsResultSchema } = await import("@modelcontextprotocol/sdk/types.js") + const { safeParse } = await import("@modelcontextprotocol/sdk/server/zod-compat.js") + // Real payload shape from Microsoft Fabric Core MCP: null annotation hints. + const fabricPayload = { + tools: [ + { + name: "list_workspaces", + inputSchema: { type: "object", properties: {} }, + annotations: { readOnlyHint: true, destructiveHint: null, idempotentHint: null, openWorldHint: null }, + }, + ], + } + // Produce the exact error the SDK would reject with (a `$ZodError`). + const strict: any = safeParse(ListToolsResultSchema as any, fabricPayload) + expect(strict.success).toBe(false) + expect(strict.error?.name).toBe("$ZodError") + + let requestCalled = false + const fakeClient = { + listTools: async () => { + throw strict.error + }, + request: async () => { + requestCalled = true + return fabricPayload + }, + } + + const result = await MCP._testing.listToolsLenient(fakeClient) + expect(requestCalled).toBe(true) + expect(result.tools).toHaveLength(1) + expect(result.tools[0].name).toBe("list_workspaces") + }) + + test("does NOT retry (rethrows) when the error is not a schema error", async () => { + const transportError = new Error("ECONNREFUSED") + let requestCalled = false + const fakeClient = { + listTools: async () => { + throw transportError + }, + request: async () => { + requestCalled = true + return { tools: [] } + }, + } + await expect(MCP._testing.listToolsLenient(fakeClient)).rejects.toThrow(/ECONNREFUSED/) + expect(requestCalled).toBe(false) + }) +}) + // --------------------------------------------------------------------------- // 2. McpRemote schema accepts new headersCommand field (issue #791). // --------------------------------------------------------------------------- @@ -133,6 +165,16 @@ describe("McpRemote.headersCommand schema (#791)", () => { expect(result.success).toBe(false) }) + test("rejects array-shaped headers/headersCommand with an actionable invalid_type error", () => { + // normalizeMcpConfig passes these malformed shapes through unchanged so the + // schema rejects them loudly instead of the normalizer silently dropping + // them (which would connect a header-less server with no feedback). + const headersArr = Config.McpRemote.safeParse({ type: "remote", url: "https://x/mcp", headers: ["a", "b"] }) + expect(headersArr.success).toBe(false) + const cmdArr = Config.McpRemote.safeParse({ type: "remote", url: "https://x/mcp", headersCommand: [["x"]] }) + expect(cmdArr.success).toBe(false) + }) + test("allows static headers and headersCommand to coexist", () => { const config = { type: "remote" as const, @@ -182,11 +224,13 @@ describe("resolveHeadersCommand helper", () => { ) }) - test("throws when command does not exist", async () => { + test("throws when command does not exist, naming the failing header", async () => { const { MCP } = await import("../../src/mcp") + // The error must name the specific header so `mcp list` points to the + // exact failing command rather than a bare ENOENT. await expect( MCP._testing.resolveHeadersCommand({ Authorization: ["this-binary-does-not-exist-xyz"] }), - ).rejects.toThrow() + ).rejects.toThrow(/headersCommand\[Authorization\] failed:/) }) test("does not invoke a shell (argv is passed directly to execFile)", async () => { From eb2a2d9cfd546b441ff8660f5eadcda05fe71e65 Mon Sep 17 00:00:00 2001 From: suryaiyer95 Date: Tue, 7 Jul 2026 12:49:22 -0700 Subject: [PATCH 3/4] fix: [#791][#792] address second-round review on bearer-auth MCP support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `discover.ts`: preserve `headersCommand` / `oauth` in the discovery normalizer (mirrors `normalizeMcpConfig`) so auto-discovered servers from `.mcp.json` / `.gemini/settings.json` keep their bearer-token config instead of silently connecting with no auth - `supportsOAuth()`: mirror `create()`'s OAuth auto-disable — return `false` when an `Authorization` header is present (statically or via `headersCommand`) and `oauth` is not explicitly configured, so the auth routes no longer start OAuth flows the connection ignores - `mergeHeaders()`: merge static + dynamic headers case-insensitively (HTTP header names are case-insensitive) so `headersCommand` overrides a static header differing only in casing instead of sending duplicates - `resolveHeadersCommand()`: run the composed failure message through `Telemetry.maskString()` before it reaches logs / `status.error`, so a token echoed in argv or printed to stderr by a verbose auth CLI is redacted - tests: discovery round-trip for `headersCommand`/`oauth`, `supportsOAuth` matrix, case-insensitive merge (unit + transport-level), stderr masking Co-Authored-By: Claude Fable 5 --- packages/opencode/src/mcp/discover.ts | 11 +++ packages/opencode/src/mcp/index.ts | 46 ++++++++- packages/opencode/test/mcp/discover.test.ts | 38 ++++++++ packages/opencode/test/mcp/headers.test.ts | 95 +++++++++++++++++++ .../opencode/test/mcp/mcp-bearer-auth.test.ts | 20 ++++ 5 files changed, 206 insertions(+), 4 deletions(-) diff --git a/packages/opencode/src/mcp/discover.ts b/packages/opencode/src/mcp/discover.ts index 18affeb58..9bf08d0d5 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -89,6 +89,17 @@ function transform( }) // altimate_change end } + // altimate_change start — preserve bearer-auth fields, mirroring config.ts + // `normalizeMcpConfig`. Without these passes a discovered server's + // `headersCommand` / `oauth` would be dropped before reaching the runtime, + // silently connecting with no auth (and the loss could be persisted back to + // disk via mcp-discover). Copied as-is — argv values are executed via + // execFile at connect time, so no env-var resolution applies here, and + // malformed shapes are rejected downstream by the McpRemote schema with an + // actionable error. See #791 / #792. + if (entry.headersCommand && typeof entry.headersCommand === "object") result.headersCommand = entry.headersCommand + if (entry.oauth !== undefined) result.oauth = entry.oauth + // altimate_change end if (typeof entry.timeout === "number") result.timeout = entry.timeout if (typeof entry.enabled === "boolean") result.enabled = entry.enabled return result as Config.Mcp diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 27babe262..6e06ef2c2 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -209,6 +209,7 @@ export namespace MCP { resolveHeadersCommand: (spec: Record | undefined, key = "test") => resolveHeadersCommand(spec, key), hasAuthorizationHeader, + mergeHeaders, } // altimate_change end @@ -242,10 +243,14 @@ export namespace MCP { // command (ENOENT, timeout, non-zero exit) rather than a bare error. // On a non-zero exit the actionable reason lives on `err.stderr` (e.g. // `az`'s "run 'az login'"), which `err.message` omits — append it. + // The composed message is masked before it escapes: execFile's message + // echoes the full argv and an auth CLI run with --verbose can print the + // token to stderr, and this string reaches logs and the status API. + // Over-masking (quoted spans become `?`) is the correct failure mode. const e = err as { message?: string; stderr?: string } const stderr = typeof e.stderr === "string" ? e.stderr.trim().slice(0, 500) : "" const base = err instanceof Error ? err.message : String(err) - const message = stderr ? `${base}: ${stderr}` : base + const message = Telemetry.maskString(stderr ? `${base}: ${stderr}` : base) throw new Error(`headersCommand[${name}] failed: ${message}`) } const value = stdout.trim() @@ -258,9 +263,30 @@ export namespace MCP { return out } - function hasAuthorizationHeader(headers: Record): boolean { + // Accepts any header-shaped record (static `headers` values are strings, + // `headersCommand` values are argv arrays) — only key names are inspected. + function hasAuthorizationHeader(headers: Record): boolean { return Object.keys(headers).some((k) => k.toLowerCase() === "authorization") } + + // HTTP header names are case-insensitive, so a dynamic header must replace a + // static one that differs only in casing (`headers.Authorization` + + // `headersCommand.authorization` would otherwise both be sent — duplicate + // credentials some servers reject). Dynamic values win under the documented + // contract: "Values from headersCommand override matching keys in `headers`." + function mergeHeaders( + staticHeaders: Record, + dynamicHeaders: Record, + ): Record { + const merged: Record = { ...staticHeaders } + for (const [name, value] of Object.entries(dynamicHeaders)) { + for (const existing of Object.keys(merged)) { + if (existing.toLowerCase() === name.toLowerCase()) delete merged[existing] + } + merged[name] = value + } + return merged + } // altimate_change end // Register notification handlers for MCP client @@ -522,7 +548,7 @@ export namespace MCP { status: { status: "failed" as const, error: message }, } } - const mergedHeaders: Record = { ...(mcp.headers ?? {}), ...dynamicHeaders } + const mergedHeaders: Record = mergeHeaders(mcp.headers ?? {}, dynamicHeaders) // altimate_change end // altimate_change start — OAuth is enabled by default for remote servers, @@ -1348,7 +1374,19 @@ export namespace MCP { const mcpConfig = cfg.mcp?.[mcpName] if (!mcpConfig) return false if (!isMcpConfigured(mcpConfig)) return false - return mcpConfig.type === "remote" && mcpConfig.oauth !== false + if (mcpConfig.type !== "remote") return false + if (mcpConfig.oauth === false) return false + if (typeof mcpConfig.oauth === "object") return true + // altimate_change start — mirror `create()`'s auto-disable: when the user + // provided an Authorization header (statically or via `headersCommand`) and + // didn't explicitly configure OAuth, connect-time skips OAuth — so the auth + // API surface must not advertise it, or `POST /:name/auth` would start an + // OAuth flow whose tokens the connection never uses. `headersCommand` is + // not resolved here (that would execute the command); the presence of an + // Authorization key in its spec is enough to know connect-time disables + // OAuth. See #792. + return !hasAuthorizationHeader({ ...(mcpConfig.headers ?? {}), ...(mcpConfig.headersCommand ?? {}) }) + // altimate_change end } /** diff --git a/packages/opencode/test/mcp/discover.test.ts b/packages/opencode/test/mcp/discover.test.ts index cce9cde10..f2e9a4a2a 100644 --- a/packages/opencode/test/mcp/discover.test.ts +++ b/packages/opencode/test/mcp/discover.test.ts @@ -191,6 +191,44 @@ describe("discoverExternalMcp", () => { }) }) + // altimate_change start — discovery must preserve bearer-auth fields + // (headersCommand / oauth) the same way config.ts `normalizeMcpConfig` does, + // or auto-discovered servers silently connect with no auth. See #791 / #792. + test("remote: headersCommand and oauth are preserved", async () => { + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + fabric: { + url: "https://api.fabric.microsoft.com/v1/mcp/core", + headersCommand: { + Authorization: ["az", "account", "get-access-token"], + }, + }, + "no-oauth": { + url: "https://example.com/mcp", + headers: { Authorization: "Bearer token" }, + oauth: false, + }, + }, + }), + ) + + const { servers: result } = await discoverExternalMcp(tempDir) + expect(result["fabric"]).toMatchObject({ + type: "remote", + url: "https://api.fabric.microsoft.com/v1/mcp/core", + headersCommand: { Authorization: ["az", "account", "get-access-token"] }, + }) + expect(result["no-oauth"]).toMatchObject({ + type: "remote", + url: "https://example.com/mcp", + headers: { Authorization: "Bearer token" }, + oauth: false, + }) + }) + // altimate_change end + test("env → environment rename", async () => { await writeFile( path.join(tempDir, ".mcp.json"), diff --git a/packages/opencode/test/mcp/headers.test.ts b/packages/opencode/test/mcp/headers.test.ts index 8ce78dd2b..238c3b9b9 100644 --- a/packages/opencode/test/mcp/headers.test.ts +++ b/packages/opencode/test/mcp/headers.test.ts @@ -178,6 +178,101 @@ test("OAuth still attaches when Authorization header is present but oauth is exp }, }) }) +test("headersCommand overrides a static header that differs only in casing", async () => { + await using tmp = await tmpdir() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + transportCalls.length = 0 + await MCP.add("case-merge-server", { + type: "remote", + url: "https://example.com/mcp", + headers: { authorization: "Bearer stale-static", "X-Other": "keep" }, + headersCommand: { + Authorization: ["printf", "Bearer fresh-dynamic"], + }, + }).catch(() => {}) + + expect(transportCalls.length).toBeGreaterThanOrEqual(1) + for (const call of transportCalls) { + // HTTP header names are case-insensitive: only the dynamic value may + // survive, or two Authorization headers would be sent on the wire. + expect(call.options.requestInit?.headers).toEqual({ + Authorization: "Bearer fresh-dynamic", + "X-Other": "keep", + }) + expect(call.options.authProvider).toBeUndefined() + } + }, + }) +}) + +test("mergeHeaders: dynamic value wins over static key differing only in casing", () => { + expect( + MCP._testing.mergeHeaders({ authorization: "Bearer stale", "X-Other": "keep" }, { Authorization: "Bearer fresh" }), + ).toEqual({ + Authorization: "Bearer fresh", + "X-Other": "keep", + }) +}) + +// Covers the auth API surface: `supportsOAuth()` must agree with `create()`'s +// auto-disable, or `POST /:name/auth` would start an OAuth flow whose tokens +// the bearer connection never uses. +test("supportsOAuth mirrors the OAuth auto-disable for bearer-auth servers", async () => { + await using tmp = await tmpdir({ + init: async (dir) => { + await Bun.write( + `${dir}/opencode.json`, + JSON.stringify({ + $schema: "https://altimate.ai/config.json", + mcp: { + "bearer-static": { + type: "remote", + url: "https://example.com/mcp", + headers: { Authorization: "Bearer static-token" }, + }, + "bearer-cmd": { + type: "remote", + url: "https://example.com/mcp", + headersCommand: { authorization: ["printf", "Bearer dynamic-token"] }, + }, + "explicit-oauth": { + type: "remote", + url: "https://example.com/mcp", + headers: { Authorization: "Bearer fallback" }, + oauth: { clientId: "client-xyz" }, + }, + "plain-remote": { + type: "remote", + url: "https://example.com/mcp", + }, + "oauth-off": { + type: "remote", + url: "https://example.com/mcp", + oauth: false, + }, + }, + }), + ) + }, + }) + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + // Bearer present (statically or via headersCommand), oauth unspecified — + // connect-time auto-disables OAuth, so the API must not advertise it. + expect(await MCP.supportsOAuth("bearer-static")).toBe(false) + expect(await MCP.supportsOAuth("bearer-cmd")).toBe(false) + // Explicit opt-in wins even with a bearer header present. + expect(await MCP.supportsOAuth("explicit-oauth")).toBe(true) + // Defaults unchanged. + expect(await MCP.supportsOAuth("plain-remote")).toBe(true) + expect(await MCP.supportsOAuth("oauth-off")).toBe(false) + }, + }) +}) // altimate_change end test("headers are passed to transports when oauth is explicitly disabled", async () => { diff --git a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts index 71779cd24..05f6d736f 100644 --- a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts +++ b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts @@ -242,6 +242,26 @@ describe("resolveHeadersCommand helper", () => { }) expect(result.X).toBe("$(whoami); rm -rf /") }) + + test("masks bearer tokens leaked to stderr in the failure message", async () => { + // An auth CLI run with --verbose/--debug can print the token to stderr, + // and the failure message reaches logs and the status API. The composed + // message must redact token-shaped values (via Telemetry.maskString). + const { MCP } = await import("../../src/mcp") + const token = "sTLeakedTokenValue0123456789abcdef" + let message = "" + try { + await MCP._testing.resolveHeadersCommand({ + Authorization: ["sh", "-c", `echo DEBUG: authorization: Bearer ${token} >&2; exit 1`], + }) + throw new Error("expected resolveHeadersCommand to reject") + } catch (err) { + message = err instanceof Error ? err.message : String(err) + } + expect(message).toMatch(/headersCommand\[Authorization\] failed:/) + expect(message).toContain("Bearer ***") + expect(message).not.toContain(token) + }) }) // --------------------------------------------------------------------------- From 174896eccf148fb211691eb5d37eefb98ed69091 Mon Sep 17 00:00:00 2001 From: suryaiyer95 Date: Tue, 7 Jul 2026 13:05:31 -0700 Subject: [PATCH 4/4] fix: validate foreign `oauth`/`headersCommand` shapes at MCP discovery ingestion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression-review follow-up to 03f82f2fb. The discovery normalizer passed `oauth` / `headersCommand` through unvalidated from foreign config files. Gemini CLI's `settings.json` legitimately uses `oauth: { enabled: true }`, which our strict `McpOAuth` schema rejects — harmless at runtime, but `mcp-discover add` persists discovered entries to `opencode.json` without re-validation, so the poisoned entry would fail `Info.safeParse` on every subsequent config load and break the session. - `discover.ts`: preserve `headersCommand` only when it is a record of non-empty string argv arrays, and `oauth` only when it is `false` or a strict object of optional string `clientId`/`clientSecret`/`scope`; drop foreign dialects with a debug log (matching pre-PR behavior). Validators mirror `McpRemote` — schemas can't be imported as values here because config.ts dynamically imports this module and the Config import is type-only to avoid a static cycle. - `mcp-bearer-auth.test.ts`: stop importing `sdk/types.js` in the `$ZodError` retry test — `mcp.test.ts` replaces that module via `mock.module` (process-global in bun), leaving `ListToolsResultSchema` undefined in full-suite runs. Replicate the SDK's strict annotation typing with `zod/v4-mini` (the same zod build the SDK validates with) so the test produces the identical `$ZodError` deterministically. - `discover.test.ts`: new test — foreign `oauth`/`headersCommand` dialects are dropped while the server itself is still discovered, and schema-valid shapes are preserved. Full `test/mcp` suite (220 tests): failure set is byte-identical to `origin/main`'s 29 pre-existing environment failures; all 31 tests added by this PR pass. Co-Authored-By: Claude Fable 5 --- packages/opencode/src/mcp/discover.ts | 51 +++++++++++++++---- packages/opencode/test/mcp/discover.test.ts | 39 ++++++++++++++ .../opencode/test/mcp/mcp-bearer-auth.test.ts | 27 ++++++++-- 3 files changed, 104 insertions(+), 13 deletions(-) diff --git a/packages/opencode/src/mcp/discover.ts b/packages/opencode/src/mcp/discover.ts index 9bf08d0d5..d903825fb 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -89,16 +89,47 @@ function transform( }) // altimate_change end } - // altimate_change start — preserve bearer-auth fields, mirroring config.ts - // `normalizeMcpConfig`. Without these passes a discovered server's - // `headersCommand` / `oauth` would be dropped before reaching the runtime, - // silently connecting with no auth (and the loss could be persisted back to - // disk via mcp-discover). Copied as-is — argv values are executed via - // execFile at connect time, so no env-var resolution applies here, and - // malformed shapes are rejected downstream by the McpRemote schema with an - // actionable error. See #791 / #792. - if (entry.headersCommand && typeof entry.headersCommand === "object") result.headersCommand = entry.headersCommand - if (entry.oauth !== undefined) result.oauth = entry.oauth + // altimate_change start — preserve bearer-auth fields so a discovered + // server's `headersCommand` / `oauth` isn't dropped before reaching the + // runtime, silently connecting with no auth. Unlike config.ts + // `normalizeMcpConfig` (which passes malformed shapes through so the user's + // own file fails `Info.safeParse` with an actionable error), discovery + // ingests FOREIGN config files: only shapes our McpRemote schema accepts + // are preserved, and foreign dialects (e.g. Gemini CLI's + // `oauth: { enabled: true }`) are dropped as before. Discovered entries are + // merged into the runtime config after validation and can be persisted to + // opencode.json via `mcp-discover add`, so an unvalidated pass-through + // would poison the config file and fail every subsequent load. + // Validators mirror the McpRemote schema: `headersCommand` is a record of + // non-empty string argv arrays; `oauth` is `false` or a strict object of + // optional string fields clientId/clientSecret/scope. (Schemas can't be + // imported as values here — config.ts dynamically imports this module, and + // the Config import above is type-only to avoid a static cycle.) + // See #791 / #792. + const headersCommand = entry.headersCommand + if (headersCommand !== undefined) { + const valid = + headersCommand !== null && + typeof headersCommand === "object" && + !Array.isArray(headersCommand) && + Object.values(headersCommand).every( + (argv) => Array.isArray(argv) && argv.length > 0 && argv.every((part: unknown) => typeof part === "string"), + ) + if (valid) result.headersCommand = headersCommand + else log.debug("dropping unrecognized headersCommand from discovered server", context) + } + const oauth = entry.oauth + if (oauth !== undefined) { + const oauthStringFields = ["clientId", "clientSecret", "scope"] + const valid = + oauth === false || + (oauth !== null && + typeof oauth === "object" && + !Array.isArray(oauth) && + Object.entries(oauth).every(([k, v]) => oauthStringFields.includes(k) && typeof v === "string")) + if (valid) result.oauth = oauth + else log.debug("dropping unrecognized oauth config from discovered server", context) + } // altimate_change end if (typeof entry.timeout === "number") result.timeout = entry.timeout if (typeof entry.enabled === "boolean") result.enabled = entry.enabled diff --git a/packages/opencode/test/mcp/discover.test.ts b/packages/opencode/test/mcp/discover.test.ts index f2e9a4a2a..71366d969 100644 --- a/packages/opencode/test/mcp/discover.test.ts +++ b/packages/opencode/test/mcp/discover.test.ts @@ -227,6 +227,45 @@ describe("discoverExternalMcp", () => { oauth: false, }) }) + + test("remote: foreign oauth/headersCommand dialects are dropped, server kept", async () => { + // Discovered configs are foreign files: shapes our schema rejects (e.g. + // Gemini CLI's `oauth: { enabled: true }`) must be dropped, not passed + // through — `mcp-discover add` persists entries to opencode.json without + // re-validation, and an invalid shape there fails every config load. + await mkdir(path.join(tempDir, ".gemini"), { recursive: true }) + await writeFile( + path.join(tempDir, ".gemini/settings.json"), + JSON.stringify({ + mcpServers: { + "gemini-oauth": { + url: "https://example.com/mcp", + oauth: { enabled: true }, + }, + "bool-oauth": { + url: "https://example.com/mcp", + oauth: true, + }, + "bad-headers-command": { + url: "https://example.com/mcp", + headersCommand: { Authorization: "not-an-argv-array" }, + }, + "valid-oauth": { + url: "https://example.com/mcp", + oauth: { clientId: "client-xyz" }, + }, + }, + }), + ) + + const { servers: result } = await discoverExternalMcp(tempDir) + expect(result["gemini-oauth"]).toMatchObject({ type: "remote", url: "https://example.com/mcp" }) + expect(result["gemini-oauth"]).not.toHaveProperty("oauth") + expect(result["bool-oauth"]).not.toHaveProperty("oauth") + expect(result["bad-headers-command"]).toMatchObject({ type: "remote", url: "https://example.com/mcp" }) + expect(result["bad-headers-command"]).not.toHaveProperty("headersCommand") + expect(result["valid-oauth"]).toMatchObject({ oauth: { clientId: "client-xyz" } }) + }) // altimate_change end test("env → environment rename", async () => { diff --git a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts index 05f6d736f..d26101582 100644 --- a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts +++ b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts @@ -89,8 +89,29 @@ describe("lenient tools/list schema", () => { // --------------------------------------------------------------------------- describe("listToolsLenient retry against SDK $ZodError (#792)", () => { test("retries with lenient schema when strict listTools() rejects Fabric-style nulls", async () => { - const { ListToolsResultSchema } = await import("@modelcontextprotocol/sdk/types.js") - const { safeParse } = await import("@modelcontextprotocol/sdk/server/zod-compat.js") + // Deliberately NOT imported from @modelcontextprotocol/sdk: mcp.test.ts + // replaces sdk/types.js via mock.module (process-global in bun), so a + // full-suite run would hand this test a mock without ListToolsResultSchema. + // Instead, replicate the SDK's strict annotation typing (boolean, null + // rejected — SDK 1.26.0/1.29.0 ToolAnnotationsSchema) with zod/v4-mini, + // the same zod build the SDK validates with, so safeParse produces the + // identical `$ZodError` the real listTools() rejects with. + const zm = await import("zod/v4-mini") + const strictAnnotations = zm.object({ + readOnlyHint: zm.optional(zm.boolean()), + destructiveHint: zm.optional(zm.boolean()), + idempotentHint: zm.optional(zm.boolean()), + openWorldHint: zm.optional(zm.boolean()), + }) + const strictListToolsResult = zm.object({ + tools: zm.array( + zm.object({ + name: zm.string(), + inputSchema: zm.any(), + annotations: zm.optional(strictAnnotations), + }), + ), + }) // Real payload shape from Microsoft Fabric Core MCP: null annotation hints. const fabricPayload = { tools: [ @@ -102,7 +123,7 @@ describe("listToolsLenient retry against SDK $ZodError (#792)", () => { ], } // Produce the exact error the SDK would reject with (a `$ZodError`). - const strict: any = safeParse(ListToolsResultSchema as any, fabricPayload) + const strict: any = strictListToolsResult.safeParse(fabricPayload) expect(strict.success).toBe(false) expect(strict.error?.name).toBe("$ZodError")