From f6b2e2dbc4f3f363f0839865d1036a2893f10086 Mon Sep 17 00:00:00 2001 From: suryaiyer95 Date: Tue, 5 May 2026 11:28:05 -0700 Subject: [PATCH] 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 736f9215f6..8e31b120ae 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -649,11 +649,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() @@ -1440,6 +1455,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 servers[name] = transformed diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index b110467cec..668c07d430 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" import { withTimeout } from "@/util/timeout" @@ -121,6 +127,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 () => { @@ -364,8 +482,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 @@ -387,22 +533,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 @@ -429,8 +578,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) @@ -567,8 +718,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) @@ -635,7 +787,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 }) @@ -784,7 +936,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 8c488d4c4f..d4fb3f55b2 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 0000000000..7da2d70196 --- /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) + } + }) +})