Skip to content

Commit 174896e

Browse files
suryaiyer95claude
suryaiyer95
and
claude
committed
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 <[email protected]>
1 parent eb2a2d9 commit 174896e

3 files changed

Lines changed: 104 additions & 13 deletions

File tree

‎packages/opencode/src/mcp/discover.ts‎

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -89,16 +89,47 @@ function transform(
8989
})
9090
// altimate_change end
9191
}
92-
// altimate_change start — preserve bearer-auth fields, mirroring config.ts
93-
// `normalizeMcpConfig`. Without these passes a discovered server's
94-
// `headersCommand` / `oauth` would be dropped before reaching the runtime,
95-
// silently connecting with no auth (and the loss could be persisted back to
96-
// disk via mcp-discover). Copied as-is — argv values are executed via
97-
// execFile at connect time, so no env-var resolution applies here, and
98-
// malformed shapes are rejected downstream by the McpRemote schema with an
99-
// actionable error. See #791 / #792.
100-
if (entry.headersCommand && typeof entry.headersCommand === "object") result.headersCommand = entry.headersCommand
101-
if (entry.oauth !== undefined) result.oauth = entry.oauth
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+
}
102133
// altimate_change end
103134
if (typeof entry.timeout === "number") result.timeout = entry.timeout
104135
if (typeof entry.enabled === "boolean") result.enabled = entry.enabled

‎packages/opencode/test/mcp/discover.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,45 @@ describe("discoverExternalMcp", () => {
227227
oauth: false,
228228
})
229229
})
230+
231+
test("remote: foreign oauth/headersCommand dialects are dropped, server kept", async () => {
232+
// Discovered configs are foreign files: shapes our schema rejects (e.g.
233+
// Gemini CLI's `oauth: { enabled: true }`) must be dropped, not passed
234+
// through — `mcp-discover add` persists entries to opencode.json without
235+
// re-validation, and an invalid shape there fails every config load.
236+
await mkdir(path.join(tempDir, ".gemini"), { recursive: true })
237+
await writeFile(
238+
path.join(tempDir, ".gemini/settings.json"),
239+
JSON.stringify({
240+
mcpServers: {
241+
"gemini-oauth": {
242+
url: "https://example.com/mcp",
243+
oauth: { enabled: true },
244+
},
245+
"bool-oauth": {
246+
url: "https://example.com/mcp",
247+
oauth: true,
248+
},
249+
"bad-headers-command": {
250+
url: "https://example.com/mcp",
251+
headersCommand: { Authorization: "not-an-argv-array" },
252+
},
253+
"valid-oauth": {
254+
url: "https://example.com/mcp",
255+
oauth: { clientId: "client-xyz" },
256+
},
257+
},
258+
}),
259+
)
260+
261+
const { servers: result } = await discoverExternalMcp(tempDir)
262+
expect(result["gemini-oauth"]).toMatchObject({ type: "remote", url: "https://example.com/mcp" })
263+
expect(result["gemini-oauth"]).not.toHaveProperty("oauth")
264+
expect(result["bool-oauth"]).not.toHaveProperty("oauth")
265+
expect(result["bad-headers-command"]).toMatchObject({ type: "remote", url: "https://example.com/mcp" })
266+
expect(result["bad-headers-command"]).not.toHaveProperty("headersCommand")
267+
expect(result["valid-oauth"]).toMatchObject({ oauth: { clientId: "client-xyz" } })
268+
})
230269
// altimate_change end
231270

232271
test("env → environment rename", async () => {

‎packages/opencode/test/mcp/mcp-bearer-auth.test.ts‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,29 @@ describe("lenient tools/list schema", () => {
8989
// ---------------------------------------------------------------------------
9090
describe("listToolsLenient retry against SDK $ZodError (#792)", () => {
9191
test("retries with lenient schema when strict listTools() rejects Fabric-style nulls", async () => {
92-
const { ListToolsResultSchema } = await import("@modelcontextprotocol/sdk/types.js")
93-
const { safeParse } = await import("@modelcontextprotocol/sdk/server/zod-compat.js")
92+
// Deliberately NOT imported from @modelcontextprotocol/sdk: mcp.test.ts
93+
// replaces sdk/types.js via mock.module (process-global in bun), so a
94+
// full-suite run would hand this test a mock without ListToolsResultSchema.
95+
// Instead, replicate the SDK's strict annotation typing (boolean, null
96+
// rejected — SDK 1.26.0/1.29.0 ToolAnnotationsSchema) with zod/v4-mini,
97+
// the same zod build the SDK validates with, so safeParse produces the
98+
// identical `$ZodError` the real listTools() rejects with.
99+
const zm = await import("zod/v4-mini")
100+
const strictAnnotations = zm.object({
101+
readOnlyHint: zm.optional(zm.boolean()),
102+
destructiveHint: zm.optional(zm.boolean()),
103+
idempotentHint: zm.optional(zm.boolean()),
104+
openWorldHint: zm.optional(zm.boolean()),
105+
})
106+
const strictListToolsResult = zm.object({
107+
tools: zm.array(
108+
zm.object({
109+
name: zm.string(),
110+
inputSchema: zm.any(),
111+
annotations: zm.optional(strictAnnotations),
112+
}),
113+
),
114+
})
94115
// Real payload shape from Microsoft Fabric Core MCP: null annotation hints.
95116
const fabricPayload = {
96117
tools: [
@@ -102,7 +123,7 @@ describe("listToolsLenient retry against SDK $ZodError (#792)", () => {
102123
],
103124
}
104125
// Produce the exact error the SDK would reject with (a `$ZodError`).
105-
const strict: any = safeParse(ListToolsResultSchema as any, fabricPayload)
126+
const strict: any = strictListToolsResult.safeParse(fabricPayload)
106127
expect(strict.success).toBe(false)
107128
expect(strict.error?.name).toBe("$ZodError")
108129

0 commit comments

Comments
 (0)