fix: validate authorizationParams off disk; note the EMA exception (#2018)

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 <cliff@futurescale.com>

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(
+      <ServerSettingsForm
+        {...baseHandlers}
+        settings={{ ...emptySettings, enterpriseManaged: true }}
+        expandedSections={["oauth"]}
+      />,
+    );
+    expect(
+      screen.getByText(/Not sent while Enterprise-managed authorization is on/),
+    ).toBeInTheDocument();
+
+    rerender(
+      <ServerSettingsForm
+        {...baseHandlers}
+        settings={{ ...emptySettings, enterpriseManaged: false }}
+        expandedSections={["oauth"]}
+      />,
+    );
+    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(
       <ServerSettingsForm

clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx

@@ -835,6 +835,9 @@ export function ServerSettingsForm({
                   only — never the token request. Use them for provider-specific
                   hints such as kc_idp_hint, login_hint, prompt, acr_values, or
                   audience.
+                  {settings.enterpriseManaged
+                    ? " Not sent while Enterprise-managed authorization is on: that flow authorizes against the enterprise IdP, a different authorization server."
+                    : ""}
                 </FieldDescription>
                 {authorizationParams.length === 0 ? (
                   <EmptyHint>No additional parameters configured</EmptyHint>

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<string, string>`, 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<typeof vi.spyOn>;
+
+  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<string, string>),
+    ).toBeUndefined();
+    expect(warnSpy).toHaveBeenCalledTimes(1);
+  });
+
+  it("rejects an array with a warning", () => {
+    expect(
+      cleanAuthorizationParams(["a"] as unknown as Record<string, string>),
+    ).toBeUndefined();
+    expect(warnSpy.mock.calls[0]?.[1]).toBe("array");
+  });
+
+  it("rejects null with a warning", () => {
+    expect(
+      cleanAuthorizationParams(null as unknown as Record<string, string>),
+    ).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<string, string>),
+    ).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" },
+    ]);
+  });
+});

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;
       }

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<string, string>`, 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<string, string> | undefined,
+): Record<string, string> | 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<string, string> = {};
+  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)
+  // write side then omits it — keeping a byte-stable round-trip. Sanitized
-  if (
+  // rather than trusted: only the web client's `/api/servers` route validates
-    stored.oauth?.authorizationParams &&
+  // this field, while the CLI/TUI read `mcp.json` straight off disk. (#2018)
-    Object.keys(stored.oauth.authorizationParams).length > 0
+  const authorizationParams = cleanAuthorizationParams(
-  ) {
+    stored.oauth?.authorizationParams,
-    settings.oauthAuthorizationParams = envRecordToPairs(
+  );
-      stored.oauth.authorizationParams,
+  if (authorizationParams) {
-    );
+    settings.oauthAuthorizationParams = envRecordToPairs(authorizationParams);
   }
   if (stored.oauth?.onInsufficientScope) {
     settings.oauthOnInsufficientScope = stored.oauth.onInsufficientScope;