diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 05243196e..529c28b2b 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -652,11 +652,28 @@ 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 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() + .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() @@ -1451,7 +1468,22 @@ 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 + // `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/discover.ts b/packages/opencode/src/mcp/discover.ts index 18affeb58..d903825fb 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -89,6 +89,48 @@ function transform( }) // altimate_change end } + // 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 return result as Config.Mcp diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 1e4d72f37..6e06ef2c2 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,164 @@ 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.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 { + // 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) + } + + /** + * 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, + 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, + mergeHeaders, + } + // altimate_change end + + // 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, + 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 + 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. + // 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 = Telemetry.maskString(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`) + } + log.info("resolved dynamic header", { server: serverKey, header: name }) + out[name] = value + } + return out + } + + // 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 function registerNotificationHandlers(client: MCPClient, serverName: string) { client.setNotificationHandler(ToolListChangedNotificationSchema, async () => { @@ -368,8 +532,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. 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) + } 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: message }, + } + } + const mergedHeaders: Record = mergeHeaders(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 +583,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 +628,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 +768,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 +837,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 +1034,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, @@ -1176,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..71366d969 100644 --- a/packages/opencode/test/mcp/discover.test.ts +++ b/packages/opencode/test/mcp/discover.test.ts @@ -191,6 +191,83 @@ 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, + }) + }) + + 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 () => { 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 8c488d4c4..238c3b9b9 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,192 @@ 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 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"], + }, + }).catch(() => {}) + + expect(transportCalls.length).toBeGreaterThanOrEqual(1) + for (const call of transportCalls) { + expect(call.options.requestInit?.headers).toMatchObject({ + Authorization: "Bearer dynamic-token", }) - // OAuth should be enabled by default, so authProvider should exist + 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() } }, }) }) +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 () => { 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..d26101582 --- /dev/null +++ b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts @@ -0,0 +1,348 @@ +import { describe, test, expect } from "bun:test" +import { Config } from "../../src/config/config" + +// 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. +// --------------------------------------------------------------------------- +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) + }) +}) + +// --------------------------------------------------------------------------- +// 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 () => { + // 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: [ + { + 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 = strictListToolsResult.safeParse(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). +// --------------------------------------------------------------------------- +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("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, + 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, 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(/headersCommand\[Authorization\] failed:/) + }) + + 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 /") + }) + + 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) + }) +}) + +// --------------------------------------------------------------------------- +// 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) + } + }) +})