Skip to content

Commit 986641e

Browse files
suryaiyer95claude
andauthored
fix: [#791][#792] support bearer-auth remote MCP servers (Microsoft Fabric, etc.) (#793)
* fix: [#791][#792] support bearer-auth remote MCP servers (Microsoft Fabric, etc.) 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<string, string[]>` — 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 <noreply@anthropic.com> * fix: [#791][#792] address review feedback on bearer-auth remote MCP Follow-up to f6b2e2d 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[<name>] 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 <noreply@anthropic.com> * fix: [#791][#792] address second-round review on bearer-auth MCP support - `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 <noreply@anthropic.com> * fix: validate foreign `oauth`/`headersCommand` shapes at MCP discovery ingestion Regression-review follow-up to 03f82f2. 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent f376fef commit 986641e

6 files changed

Lines changed: 903 additions & 17 deletions

File tree

packages/opencode/src/config/config.ts

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -652,11 +652,28 @@ export namespace Config {
652652
url: z.string().describe("URL of the remote MCP server"),
653653
enabled: z.boolean().optional().describe("Enable or disable the MCP server on startup"),
654654
headers: z.record(z.string(), z.string()).optional().describe("Headers to send with the request"),
655+
// altimate_change start — dynamic header values produced by an argv command
656+
// (run via execFile, not a shell — no shell interpolation unless the user
657+
// explicitly invokes one like `sh -c`), resolved on each (re)connect so
658+
// callers can refresh expiring bearer tokens without restarting the session
659+
// (e.g. `az account get-access-token`).
660+
headersCommand: z
661+
.record(z.string(), z.array(z.string()).nonempty())
662+
.optional()
663+
.describe(
664+
"Headers whose values are produced by running a command (argv form: [cmd, ...args]). " +
665+
"stdout is trimmed and used as the header value. Resolved on every connect so tokens " +
666+
"with short TTLs (e.g. Microsoft Entra ID bearer tokens for Fabric) refresh automatically. " +
667+
"Values from headersCommand override matching keys in `headers`.",
668+
),
669+
// altimate_change end
655670
oauth: z
656671
.union([McpOAuth, z.literal(false)])
657672
.optional()
658673
.describe(
659-
"OAuth authentication configuration for the MCP server. Set to false to disable OAuth auto-detection.",
674+
"OAuth authentication configuration for the MCP server. Set to false to disable OAuth auto-detection. " +
675+
"When `headers.Authorization` or `headersCommand.Authorization` is set and `oauth` is not specified, " +
676+
"OAuth is disabled automatically so a static bearer token isn't overridden by a competing OAuth flow.",
660677
),
661678
timeout: z
662679
.number()
@@ -1451,7 +1468,22 @@ export namespace Config {
14511468
servers[name] = transformed
14521469
} else if (entry.url && typeof entry.url === "string") {
14531470
const transformed: Record<string, any> = { type: "remote", url: entry.url }
1471+
// Copy `headers` / `headersCommand` through as-is — including malformed
1472+
// array shapes. The downstream `Info.safeParse` validates `mcp` against
1473+
// the McpRemote schema, and `z.record(...)` rejects an array with an
1474+
// actionable `invalid_type` error. Stripping arrays here would instead
1475+
// drop the field silently and connect a header-less server with no
1476+
// feedback to the user. See #791 / #792.
14541477
if (entry.headers && typeof entry.headers === "object") transformed.headers = entry.headers
1478+
// altimate_change start — preserve fields that the original normalizer dropped
1479+
// silently. Without these passes, a user-supplied `oauth: false` or
1480+
// `headersCommand` would be reconstructed-away, leaving the runtime
1481+
// believing the config was bare. See #791 / #792.
1482+
if (entry.headersCommand && typeof entry.headersCommand === "object") {
1483+
transformed.headersCommand = entry.headersCommand
1484+
}
1485+
if (entry.oauth !== undefined) transformed.oauth = entry.oauth
1486+
// altimate_change end
14551487
if (typeof entry.timeout === "number") transformed.timeout = entry.timeout
14561488
if (typeof entry.enabled === "boolean") transformed.enabled = entry.enabled
14571489
if (typeof entry.updatedAt === "string") transformed.updatedAt = entry.updatedAt

packages/opencode/src/mcp/discover.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,48 @@ function transform(
8989
})
9090
// altimate_change end
9191
}
92+
// altimate_change start — preserve bearer-auth fields so a discovered
93+
// server's `headersCommand` / `oauth` isn't dropped before reaching the
94+
// runtime, silently connecting with no auth. Unlike config.ts
95+
// `normalizeMcpConfig` (which passes malformed shapes through so the user's
96+
// own file fails `Info.safeParse` with an actionable error), discovery
97+
// ingests FOREIGN config files: only shapes our McpRemote schema accepts
98+
// are preserved, and foreign dialects (e.g. Gemini CLI's
99+
// `oauth: { enabled: true }`) are dropped as before. Discovered entries are
100+
// merged into the runtime config after validation and can be persisted to
101+
// opencode.json via `mcp-discover add`, so an unvalidated pass-through
102+
// would poison the config file and fail every subsequent load.
103+
// Validators mirror the McpRemote schema: `headersCommand` is a record of
104+
// non-empty string argv arrays; `oauth` is `false` or a strict object of
105+
// optional string fields clientId/clientSecret/scope. (Schemas can't be
106+
// imported as values here — config.ts dynamically imports this module, and
107+
// the Config import above is type-only to avoid a static cycle.)
108+
// See #791 / #792.
109+
const headersCommand = entry.headersCommand
110+
if (headersCommand !== undefined) {
111+
const valid =
112+
headersCommand !== null &&
113+
typeof headersCommand === "object" &&
114+
!Array.isArray(headersCommand) &&
115+
Object.values(headersCommand).every(
116+
(argv) => Array.isArray(argv) && argv.length > 0 && argv.every((part: unknown) => typeof part === "string"),
117+
)
118+
if (valid) result.headersCommand = headersCommand
119+
else log.debug("dropping unrecognized headersCommand from discovered server", context)
120+
}
121+
const oauth = entry.oauth
122+
if (oauth !== undefined) {
123+
const oauthStringFields = ["clientId", "clientSecret", "scope"]
124+
const valid =
125+
oauth === false ||
126+
(oauth !== null &&
127+
typeof oauth === "object" &&
128+
!Array.isArray(oauth) &&
129+
Object.entries(oauth).every(([k, v]) => oauthStringFields.includes(k) && typeof v === "string"))
130+
if (valid) result.oauth = oauth
131+
else log.debug("dropping unrecognized oauth config from discovered server", context)
132+
}
133+
// altimate_change end
92134
if (typeof entry.timeout === "number") result.timeout = entry.timeout
93135
if (typeof entry.enabled === "boolean") result.enabled = entry.enabled
94136
return result as Config.Mcp

0 commit comments

Comments
 (0)