Merge pull request #2259 from modelcontextprotocol/v2/fix/2216-progress-toast-id-and-csprng

fix: type-discriminate progress toast ids, and a CSPRNG fallback for newAttemptId

clients/web/src/lib/oauthResume.test.ts

@@ -26,6 +26,34 @@ import {
   EMPTY_NETWORK_UI,
 } from "../components/screens/screenUiState.js";
 
+/**
+ * Run `body` with one `crypto` member hidden, then restore it.
+ *
+ * `randomUUID` is inherited from `Crypto.prototype`, so there is normally no
+ * OWN descriptor to put back — restoring only when one existed would leave the
+ * `undefined` own property in place and force every later test in this file
+ * onto the fallback path.
+ *
+ * This hides one *member*; the arm that needs the whole `crypto` global gone
+ * stubs `globalThis.crypto` itself instead (see below).
+ */
+function withoutCryptoMember(name: "randomUUID", body: () => void): void {
+  const original = Object.getOwnPropertyDescriptor(globalThis.crypto, name);
+  Object.defineProperty(globalThis.crypto, name, {
+    configurable: true,
+    value: undefined,
+  });
+  try {
+    body();
+  } finally {
+    if (original) {
+      Object.defineProperty(globalThis.crypto, name, original);
+    } else {
+      delete (globalThis.crypto as Partial<Pick<Crypto, "randomUUID">>)[name];
+    }
+  }
+}
+
 describe("oauthResume", () => {
   const storage = new Map<string, string>();
 
@@ -44,6 +72,9 @@ describe("oauthResume", () => {
 
   afterEach(() => {
     vi.unstubAllGlobals();
+    // This project does not set `restoreMocks` globally (see `src/test/setup.ts`),
+    // so the `crypto`/`Math.random` spies below must be reverted by hand.
+    vi.restoreAllMocks();
   });
 
   it("consumeOAuthResumeSnapshot reads once then clears storage", () => {
@@ -456,39 +487,64 @@ describe("oauthResume", () => {
     expect(readOAuthResumeSnapshot()?.attemptId).toBe(token);
   });
 
-  it("writeOAuthResumeSnapshot falls back when randomUUID is unavailable", () => {
+  it("writeOAuthResumeSnapshot falls back to getRandomValues without randomUUID", () => {
     // `crypto.randomUUID` needs a secure context, which a plain-HTTP
-    // non-loopback host is not.
-    const original = Object.getOwnPropertyDescriptor(
-      globalThis.crypto,
-      "randomUUID",
-    );
-    Object.defineProperty(globalThis.crypto, "randomUUID", {
-      configurable: true,
-      value: undefined,
-    });
-    try {
-      const token = writeOAuthResumeSnapshot({
+    // non-loopback host is not. `crypto.getRandomValues` does not, so the
+    // fallback is still a CSPRNG rather than `Math.random`.
+    const randomSpy = vi.spyOn(globalThis.crypto, "getRandomValues");
+    const mathSpy = vi.spyOn(Math, "random");
+    let token: string | undefined;
+    withoutCryptoMember("randomUUID", () => {
+      token = writeOAuthResumeSnapshot({
+        version: 1,
+        serverId: "a",
+        activeTab: "tools",
+        authKind: "reauth",
+        tabUi: {},
+      });
+    });
+    expect(randomSpy).toHaveBeenCalledOnce();
+    expect(mathSpy).not.toHaveBeenCalled();
+    // 16 random bytes, hex-encoded.
+    expect(token).toMatch(/^[0-9a-f]{32}$/);
+    expect(clearOwnOAuthResumeSnapshot(token)).toBe(true);
+    expect(globalThis.crypto.randomUUID).toEqual(expect.any(Function));
+  });
+
+  it("writeOAuthResumeSnapshot falls back to Math.random with no crypto at all", () => {
+    // The whole global goes, not just its two methods — an exotic runtime with
+    // no WebCrypto at all, matching `src/test/core/auth/utils.test.ts`. Hiding
+    // only the methods would leave `globalThis.crypto` truthy and so would not
+    // catch a regression that reads it without the optional guard (Copilot).
+    const mathSpy = vi.spyOn(Math, "random");
+    // `crypto` IS an own property of the global (unlike the `Crypto.prototype`
+    // members above), so there is always a descriptor to restore. Asserted
+    // rather than `!`-ed so a change in that assumption fails loudly here.
+    const original = Object.getOwnPropertyDescriptor(globalThis, "crypto");
+    expect(original).toBeDefined();
+    Object.defineProperty(globalThis, "crypto", {
+      configurable: true,
+      writable: true,
+      value: undefined,
+    });
+    let token: string | undefined;
+    try {
+      token = writeOAuthResumeSnapshot({
         version: 1,
         serverId: "a",
         activeTab: "tools",
         authKind: "reauth",
         tabUi: {},
       });
-      expect(token).toEqual(expect.any(String));
-      expect(clearOwnOAuthResumeSnapshot(token)).toBe(true);
     } finally {
-      // `randomUUID` is inherited from `Crypto.prototype`, so there is
-      // normally no OWN descriptor to put back — restoring only when one
-      // existed would leave the `undefined` own property in place and force
-      // every later test in this file onto the fallback path.
       if (original) {
-        Object.defineProperty(globalThis.crypto, "randomUUID", original);
-      } else {
-        delete (globalThis.crypto as { randomUUID?: unknown }).randomUUID;
+        Object.defineProperty(globalThis, "crypto", original);
       }
     }
-    expect(globalThis.crypto.randomUUID).toEqual(expect.any(Function));
+    expect(mathSpy).toHaveBeenCalled();
+    expect(token).toEqual(expect.any(String));
+    expect(clearOwnOAuthResumeSnapshot(token)).toBe(true);
+    expect(globalThis.crypto.getRandomValues).toEqual(expect.any(Function));
   });
 
   it("clearOAuthResumeSnapshot swallows removeItem failures", () => {

clients/web/src/lib/oauthResume.ts

@@ -227,12 +227,28 @@ export function writeOAuthResumeSnapshot(
  * context, which a `file://` page or a plain-HTTP non-loopback host is not),
  * and otherwise a value that only has to be unique among the handful of
  * redirect attempts one page can have in flight — never a security token.
+ *
+ * The fallback chain matters even so. `crypto.getRandomValues` is *not*
+ * gated on a secure context and exists in every browser that has `crypto` at
+ * all — so precisely the situation `randomUUID` is unavailable in still has a
+ * CSPRNG on hand, and declining to use it would be a gratuitous downgrade
+ * (CodeQL `js/insecure-randomness`, alert 72, flags exactly that). `Math.random`
+ * survives only as the last resort for a `crypto`-less global.
  */
 function newAttemptId(): string {
   const uuid = globalThis.crypto?.randomUUID?.bind(globalThis.crypto);
   if (uuid) {
     return uuid();
   }
+  // Called directly rather than through a bound alias: CodeQL's
+  // `js/insecure-randomness` browser model recognizes a secure RNG by the
+  // literal `crypto.getRandomValues(...)` method call, and an alias does not
+  // match it — so the indirection would leave the `Math.random` last resort
+  // below classified as an unmitigated source (Copilot).
+  if (globalThis.crypto?.getRandomValues) {
+    const bytes = globalThis.crypto.getRandomValues(new Uint8Array(16));
+    return Array.from(bytes, (b) => b.toString(16).padStart(2, "0")).join("");
+  }
   return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}`;
 }
 

clients/web/src/utils/toasts/progressToasts.test.ts

@@ -13,12 +13,37 @@ describe("PROGRESS_TOAST_AUTOCLOSE_MS", () => {
 
 describe("progressToastId", () => {
   it("keys by the progress token", () => {
-    expect(progressToastId("abc")).toBe("progress-abc");
-    expect(progressToastId(7)).toBe("progress-7");
+    expect(progressToastId("abc")).toBe("progress-s:abc");
+    expect(progressToastId(7)).toBe("progress-n:7");
   });
 
   it("shares one id when the server sends no token", () => {
-    expect(progressToastId(undefined)).toBe("progress-default");
+    expect(progressToastId(undefined)).toBe("progress-none");
+  });
+
+  it("does not collide a numeric token with the same-looking string token", () => {
+    expect(progressToastId(7)).not.toBe(progressToastId("7"));
+  });
+
+  it("does not collide the absent token with any token a server can send", () => {
+    const absent = progressToastId(undefined);
+    for (const token of ["default", "none", "", "0"]) {
+      expect(progressToastId(token)).not.toBe(absent);
+    }
+    expect(progressToastId(0)).not.toBe(absent);
+  });
+
+  it("gives each distinct token its own id", () => {
+    const ids = [
+      progressToastId(undefined),
+      progressToastId(0),
+      progressToastId(7),
+      progressToastId("0"),
+      progressToastId("7"),
+      progressToastId("none"),
+      progressToastId("default"),
+    ];
+    expect(new Set(ids).size).toBe(ids.length);
   });
 });
 

clients/web/src/utils/toasts/progressToasts.ts

@@ -11,8 +11,21 @@ export const PROGRESS_TOAST_AUTOCLOSE_MS = 5000;
 // rather than flooding the corner. The injected `progressToken` correlates a
 // stream with the request that triggered it; when absent (the common case —
 // the inspector doesn't expose a caller token), all ticks share one toast.
+//
+// The token's *type* is part of the key. `ProgressToken` is `string | number`,
+// so a bare `String(token)` maps the number 7 and the string "7" — two
+// distinct streams per the spec — onto one id, and the two streams then
+// overwrite each other's toast (id collision means replacement, which is the
+// whole point of the id). The `n:`/`s:` discriminator keeps them apart, and
+// the no-token case gets a prefix of its own rather than the sentinel
+// `"default"`, which a server is free to send as a genuine string token.
 export function progressToastId(token: ProgressToken | undefined): string {
-  return `progress-${String(token ?? "default")}`;
+  if (token === undefined) {
+    return "progress-none";
+  }
+  return typeof token === "number"
+    ? `progress-n:${token}`
+    : `progress-s:${token}`;
 }
 
 // One-line toast body: "<message> — <progress> / <total> (NN%)". The fraction