From e6e4600a731b4a74d92901f1c19a15cfc5400850 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 22:10:36 -0400 Subject: [PATCH] fix: validate authorizationParams off disk; note the EMA exception (#2018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 5 — three of four findings implemented. - `cleanAuthorizationParams` validates the field as it comes off disk. Only the web client's /api/servers route checked the shape; the CLI and TUI read mcp.json directly, so `authorizationParams: "oops"` enumerated as the character-indexed pairs 0=o, 1=o, … and `{ audience: 5 }` reached URLSearchParams.set, which stringifies it — either way the Inspector sent query parameters the user never wrote. Now validated in shared code so all three clients enforce the documented string→string contract, warning on each rejection. Follows `cleanRoots`, which guards the same class for `roots`. - The form's description now states that these parameters are not sent while Enterprise-managed authorization is on. The EMA leg authorizes against the enterprise IdP — a different authorization server — and deliberately sends none of them, so the copy was presenting a configured-but-inert value as active. - The /api/servers malformed-`oauth` warning listed an expected shape omitting `onInsufficientScope`, which the validator accepts and the docs describe, so it told users a valid config was malformed. Ten new cases, all verified to fail against the pre-fix sources. Declined: recording the authorize URL in the Network tab (repeat of round 2) — that URL is reached by a top-level browser navigation rather than a tracked fetch, so it has never appeared there for any authorization request; adding a synthetic entry changes every OAuth flow and is a product decision for a maintainer, not a change to make inside this PR. Signed-off-by: cliffhall --- .../ServerSettingsForm.test.tsx | 30 ++++++ .../ServerSettingsForm/ServerSettingsForm.tsx | 3 + .../web/src/test/core/mcp/serverList.test.ts | 92 +++++++++++++++++++ core/mcp/remote/node/server.ts | 2 +- core/mcp/serverList.ts | 55 +++++++++-- 5 files changed, 173 insertions(+), 9 deletions(-) diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index a9ff4f5d..d238228a 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -903,6 +903,36 @@ describe("ServerSettingsForm", () => { ); }); + // #2018 — the EMA leg authorizes against the enterprise IdP, a different + // authorization server, and deliberately sends none of these parameters. The + // description has to say so, or the form shows a configured value as active + // when it will not be sent. + it("warns that authorization parameters are unused under enterprise-managed auth", () => { + const { rerender } = renderWithMantine( + , + ); + expect( + screen.getByText(/Not sent while Enterprise-managed authorization is on/), + ).toBeInTheDocument(); + + rerender( + , + ); + expect( + screen.queryByText( + /Not sent while Enterprise-managed authorization is on/, + ), + ).not.toBeInTheDocument(); + }); + it("does not flag a blank authorization-parameter row as reserved", () => { renderWithMantine( {authorizationParams.length === 0 ? ( No additional parameters configured diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index c6dc9436..6641e55b 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { + cleanAuthorizationParams, cleanRoots, DEFAULT_SEED_CONFIG, envPairsToRecord, @@ -1415,3 +1416,94 @@ describe("stdio env / cwd mirroring", () => { }); }); }); + +// #2018 — `StoredMCPServer.oauth.authorizationParams` is typed +// `Record`, but only the web client's `/api/servers` route +// validates it; the CLI and TUI read `mcp.json` off disk. These cases drive the +// shapes a hand-edited file can hold, which the type says are impossible. +describe("cleanAuthorizationParams", () => { + let warnSpy: ReturnType; + + beforeEach(() => { + warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + + afterEach(() => { + warnSpy.mockRestore(); + }); + + it("passes a well-formed record through", () => { + expect( + cleanAuthorizationParams({ kc_idp_hint: "corp", prompt: "login" }), + ).toEqual({ kc_idp_hint: "corp", prompt: "login" }); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it("returns undefined for an absent or empty record", () => { + expect(cleanAuthorizationParams(undefined)).toBeUndefined(); + expect(cleanAuthorizationParams({})).toBeUndefined(); + }); + + // A bare string enumerates as character-indexed pairs (0=o, 1=o, …), so + // without this guard the Inspector would send four invented parameters. + it("rejects a string with a warning instead of enumerating its characters", () => { + expect( + cleanAuthorizationParams("oops" as unknown as Record), + ).toBeUndefined(); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); + + it("rejects an array with a warning", () => { + expect( + cleanAuthorizationParams(["a"] as unknown as Record), + ).toBeUndefined(); + expect(warnSpy.mock.calls[0]?.[1]).toBe("array"); + }); + + it("rejects null with a warning", () => { + expect( + cleanAuthorizationParams(null as unknown as Record), + ).toBeUndefined(); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); + + // `URLSearchParams.set` stringifies whatever it is given, so a number would + // otherwise reach the authorize URL as "5". + it("drops a non-string value with a warning and keeps the rest", () => { + expect( + cleanAuthorizationParams({ + audience: 5, + kc_idp_hint: "corp", + } as unknown as Record), + ).toEqual({ kc_idp_hint: "corp" }); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); + + it("returns undefined when every value was dropped", () => { + expect( + cleanAuthorizationParams({ audience: 5 } as unknown as Record< + string, + string + >), + ).toBeUndefined(); + }); + + it("skips a blank key", () => { + expect(cleanAuthorizationParams({ " ": "x", ok: "y" })).toEqual({ + ok: "y", + }); + }); + + it("is applied when lifting stored fields into settings", () => { + const settings = storedFieldsToInspectorSettings({ + type: "streamable-http", + url: "https://x.test", + oauth: { + authorizationParams: { audience: 5, kc_idp_hint: "corp" }, + }, + } as unknown as StoredMCPServer); + expect(settings?.oauthAuthorizationParams).toEqual([ + { key: "kc_idp_hint", value: "corp" }, + ]); + }); +}); diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 2489a521..d0ee6ea1 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1255,7 +1255,7 @@ export function createRemoteApp( if ("oauth" in valObj && !isOauthObject(valObj.oauth)) { logWarn( { route: "/api/servers", id, droppedKey: "oauth" }, - "Dropping malformed `oauth` field — expected `{ clientId?, clientSecret?, scopes?, authorizationParams?, enterpriseManaged? }`.", + "Dropping malformed `oauth` field — expected `{ clientId?, clientSecret?, scopes?, authorizationParams?, enterpriseManaged?, onInsufficientScope? }`.", ); delete valObj.oauth; } diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index c6e5e507..1f34cab0 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -194,6 +194,45 @@ export function envPairsToRecord( return out; } +/** + * Validate a server's `oauth.authorizationParams` as it comes off disk, or + * `undefined` when there is nothing usable. (#2018) + * + * `StoredMCPServer` types this as `Record`, but that is a + * compile-time promise a hand-edited `mcp.json` does not keep — and only the web + * client's `/api/servers` route checks it, while the CLI and TUI read the file + * directly. Without this, `authorizationParams: "oops"` enumerates as the + * character-indexed pairs `0=o, 1=o, …` and `{ audience: 5 }` reaches + * `URLSearchParams.set`, which stringifies it — either way the Inspector sends + * query parameters the user never wrote. Each rejection warns, following + * `cleanRoots` above, which guards the same class for `roots`. + */ +export function cleanAuthorizationParams( + params: Record | undefined, +): Record | undefined { + if (params === undefined) return undefined; + // Keep: unreachable per the parameter type, reachable from disk. + if (typeof params !== "object" || params === null || Array.isArray(params)) { + console.warn( + "Ignoring `oauth.authorizationParams`: expected an object of string values, got", + Array.isArray(params) ? "array" : typeof params, + ); + return undefined; + } + const out: Record = {}; + for (const [key, value] of Object.entries(params)) { + if (typeof value !== "string") { + console.warn( + `Dropping \`oauth.authorizationParams.${key}\`: expected a string value, got ${typeof value}.`, + ); + continue; + } + if (key.trim() === "") continue; + out[key] = value; + } + return Object.keys(out).length > 0 ? out : undefined; +} + /** * Collapse a server's custom authorization-parameter rows into the record the * `InspectorClientOptions.oauth.authorizationParams` option takes, or @@ -309,14 +348,14 @@ export function storedFieldsToInspectorSettings( if (stored.oauth?.scopes) settings.oauthScopes = stored.oauth.scopes; // Optional record with no default; carried through only when the file has a // non-empty object, so an absent/empty field reads back as unset and the - // write side then omits it — keeping a byte-stable round-trip. (#2018) - if ( - stored.oauth?.authorizationParams && - Object.keys(stored.oauth.authorizationParams).length > 0 - ) { - settings.oauthAuthorizationParams = envRecordToPairs( - stored.oauth.authorizationParams, - ); + // write side then omits it — keeping a byte-stable round-trip. Sanitized + // rather than trusted: only the web client's `/api/servers` route validates + // this field, while the CLI/TUI read `mcp.json` straight off disk. (#2018) + const authorizationParams = cleanAuthorizationParams( + stored.oauth?.authorizationParams, + ); + if (authorizationParams) { + settings.oauthAuthorizationParams = envRecordToPairs(authorizationParams); } if (stored.oauth?.onInsufficientScope) { settings.oauthOnInsufficientScope = stored.oauth.onInsufficientScope;