Scope secret-store entries by a per-state-file namespace

clients/cli/__tests__/stored-auth.test.ts

@@ -82,7 +82,9 @@ describe("refreshStoredAuthToken", () => {
   // Persisted writes split tokens into the process-wide (in-memory, per
   // vitest.config.ts) secret store, and joined reads prefer the store over
   // file plaintext — so purge the entry between tests or one test's rotated
-  // tokens would leak into the next test's fixture.
+  // tokens would leak into the next test's fixture. Post-#2549 each fixture
+  // file mints its own secrets namespace, so namespaced entries can't collide
+  // across tests; this purge covers the legacy (un-namespaced) id.
   afterEach(async () => {
     await defaultSecretStore().deleteAllForServer(oauthSecretServerId(SERVER));
   });

clients/web/src/test/core/auth/oauth-persist-file.test.ts

@@ -85,7 +85,7 @@ describe("writeOAuthSections lock failures", () => {
     // the wrong file. Only acquisition failures (the mocks above, which
     // reject before the callback runs) get the OAuth wording.
     vi.mocked(withSecretFileLock).mockImplementation(
-      async (_path, fn) => fn() as Promise<never>,
+      async (_path, fn) => fn(true) as Promise<never>,
     );
     const original = new SecretFileLockHeldError(
       "Could not lock the secrets file at /home/u/.mcp-inspector/secrets.json",
@@ -142,7 +142,7 @@ describe("readOAuthStore locking", () => {
     // residue with its already-committed new secrets. The whole read must
     // execute inside the same lock the writers hold.
     vi.mocked(withSecretFileLock).mockImplementation(
-      async (_path, fn) => fn() as Promise<never>,
+      async (_path, fn) => fn(true) as Promise<never>,
     );
 
     const result = await readOAuthStore(
@@ -190,7 +190,7 @@ describe("persistEntrySecrets partial-commit compensation", () => {
   beforeEach(() => {
     vi.mocked(withSecretFileLock).mockReset();
     vi.mocked(withSecretFileLock).mockImplementation(
-      async (_path, fn) => fn() as Promise<never>,
+      async (_path, fn) => fn(true) as Promise<never>,
     );
   });
 
@@ -218,13 +218,18 @@ describe("persistEntrySecrets partial-commit compensation", () => {
     failWhen: (field: string, value: string) => boolean,
   ) => {
     const store = new InMemorySecretStore();
-    const serverId = oauthSecretServerId(url);
     await writeOAuthSections(
       file,
       { servers: { [url]: SEED_STATE }, idpSessions: {} },
       { servers: [url] },
       store,
     );
+    // The seed write adopted a secrets namespace (#2549); the entry's store
+    // id is scoped by it, so read it back from the written file.
+    const { secretsNamespace } = JSON.parse(await readFile(file, "utf8")) as {
+      secretsNamespace: string;
+    };
+    const serverId = oauthSecretServerId(url, secretsNamespace);
     const realSet = store.set.bind(store);
     store.set = async (sid: string, field: string, value: string) => {
       if (failWhen(field, value))

clients/web/src/test/core/auth/oauth-secrets.test.ts

@@ -10,6 +10,8 @@ import {
   resetPersistTokensPolicyWarnings,
   oauthSecretServerId,
   oauthIdpSecretServerId,
+  isValidSecretsNamespace,
+  newSecretsNamespace,
   issuerTokensField,
   issuerClientSecretField,
   issuerRegistrationTokenField,
@@ -103,6 +105,43 @@ describe("id and field schemes", () => {
     const withPort = `${oauthSecretServerId("https://a.example:8080")}:tokens`;
     expect(withPort.startsWith(plain)).toBe(false);
   });
+
+  it("scopes ids by secrets namespace with an unforgeable delimiter", () => {
+    const ns = "9a3c2e1f-0b4d-4c5e-8f6a-7b8c9d0e1f2a";
+    expect(oauthSecretServerId("https://s.example/mcp", ns)).toBe(
+      `oauth+${ns}+https%3A%2F%2Fs.example%2Fmcp`,
+    );
+    expect(oauthIdpSecretServerId("https://idp.example", ns)).toBe(
+      `oauth-idp+${ns}+https%3A%2F%2Fidp.example`,
+    );
+    // Namespaced ids stay colon-free (same keyring purge constraint).
+    expect(oauthSecretServerId("https://s.example:8443/mcp", ns)).not.toContain(
+      ":",
+    );
+    // encodeURIComponent escapes `+`, so a URL cannot forge the delimiter:
+    // a legacy id over a `+`-bearing URL never collides with a namespaced id.
+    expect(oauthSecretServerId(`${ns}+https://s.example/mcp`)).not.toBe(
+      oauthSecretServerId("https://s.example/mcp", ns),
+    );
+  });
+
+  it("validates and mints namespaces", () => {
+    expect(isValidSecretsNamespace(newSecretsNamespace())).toBe(true);
+    expect(isValidSecretsNamespace("abc-123.DEF_456")).toBe(true);
+    for (const bad of [
+      undefined,
+      null,
+      42,
+      "",
+      "-leading-separator",
+      "has:colon",
+      "has+plus",
+      "has space",
+      "a".repeat(129),
+    ]) {
+      expect(isValidSecretsNamespace(bad)).toBe(false);
+    }
+  });
 });
 
 describe("splitServerOAuthState", () => {

clients/web/src/test/integration/auth/node/file-lock.test.ts

@@ -184,11 +184,15 @@ describe("withSecretFileLock across processes", () => {
     await expect(fs.stat(target)).rejects.toThrow();
 
     let ran = false;
-    await withSecretFileLock(target, async () => {
+    let sawLocked: boolean | undefined;
+    await withSecretFileLock(target, async (locked) => {
       ran = true;
+      sawLocked = locked;
     });
 
     expect(ran).toBe(true);
+    // The body is told it holds a real lock (adoption gates on this).
+    expect(sawLocked).toBe(true);
     expect(warnings()).toBe("");
   });
 
@@ -271,14 +275,20 @@ describe("withSecretFileLock degrades rather than failing", () => {
     const target = path.join(tmpDir, "not-a-dir", "secrets.json");
 
     let ran = 0;
-    await withSecretFileLock(target, async () => {
+    const sawLocked: boolean[] = [];
+    await withSecretFileLock(target, async (locked) => {
       ran += 1;
+      sawLocked.push(locked);
     });
-    await withSecretFileLock(target, async () => {
+    await withSecretFileLock(target, async (locked) => {
       ran += 1;
+      sawLocked.push(locked);
     });
 
     expect(ran).toBe(2);
+    // The body is told the run is unlocked, so work that is only safe
+    // under real exclusion (legacy adoption) can refuse instead of racing.
+    expect(sawLocked).toEqual([false, false]);
     expect(warnings()).toContain("Could not take a lock on the secrets file");
     // Once per reason per process — a warning on every save would be noise
     // on precisely the deployment that cannot act on it.

clients/web/src/test/integration/auth/node/oauth-adoption-locking.test.ts

@@ -0,0 +1,175 @@
+/**
+ * Degraded-lock gate on legacy namespace adoption (#2549): migrating a
+ * legacy file's secret-store entries deletes its sources, so it is only
+ * safe under the real cross-process file lock — two unlocked adopters can
+ * each observe the other's half-finished move and strand credentials.
+ * These tests mock `withSecretFileLock` to simulate the degraded
+ * (unlocked) run and assert the save refuses the migration before
+ * touching anything, while mint-only adoption (fresh file) and
+ * already-stamped files keep saving unlocked.
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
+import { mkdtempSync, readFileSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+
+// Every lock in this suite degrades: the body runs, told it is unlocked.
+vi.mock("@inspector/core/auth/node/file-lock.js", async (importOriginal) => {
+  const actual =
+    await importOriginal<
+      typeof import("@inspector/core/auth/node/file-lock.js")
+    >();
+  return {
+    ...actual,
+    withSecretFileLock: async <T>(
+      _filePath: string,
+      fn: (locked: boolean) => Promise<T>,
+    ): Promise<T> => fn(false),
+  };
+});
+
+import {
+  writeOAuthSections,
+  SECRETS_NAMESPACE_KEY,
+} from "@inspector/core/auth/node/oauth-persist-file.js";
+import {
+  InMemorySecretStore,
+  SecretStoreUnavailableError,
+} from "@inspector/core/auth/node/secret-store.js";
+import {
+  PERSIST_TOKENS_ENV,
+  oauthSecretServerId,
+  isValidSecretsNamespace,
+  LEGACY_TOKENS_FIELD,
+  resetPersistTokensPolicyWarnings,
+} from "@inspector/core/auth/node/oauth-secrets.js";
+import {
+  writeStoreFile,
+  flushStoreFileWrites,
+} from "@inspector/core/storage/store-io.js";
+import type { OAuthPersistSnapshot } from "@inspector/core/auth/oauth-persist.js";
+
+const SERVER = "https://api.example/mcp";
+const TOKENS = {
+  access_token: "at-legacy",
+  token_type: "Bearer",
+  refresh_token: "rt-legacy",
+};
+
+function snapshotFor(tag: string): OAuthPersistSnapshot {
+  return {
+    servers: {
+      [SERVER]: {
+        scope: "read",
+        tokens: { access_token: `at-${tag}`, token_type: "Bearer" },
+      },
+    },
+    idpSessions: {},
+  };
+}
+
+function fileNamespace(filePath: string): string {
+  const parsed = JSON.parse(readFileSync(filePath, "utf8")) as Record<
+    string,
+    unknown
+  >;
+  return parsed[SECRETS_NAMESPACE_KEY] as string;
+}
+
+let tempDir: string;
+let filePath: string;
+let store: InMemorySecretStore;
+let savedPolicy: string | undefined;
+
+beforeEach(() => {
+  tempDir = mkdtempSync(join(tmpdir(), "inspector-oauth-adopt-lock-"));
+  filePath = join(tempDir, "oauth.json");
+  store = new InMemorySecretStore();
+  savedPolicy = process.env[PERSIST_TOKENS_ENV];
+  delete process.env[PERSIST_TOKENS_ENV];
+});
+
+afterEach(() => {
+  if (savedPolicy === undefined) delete process.env[PERSIST_TOKENS_ENV];
+  else process.env[PERSIST_TOKENS_ENV] = savedPolicy;
+  resetPersistTokensPolicyWarnings();
+  rmSync(tempDir, { recursive: true, force: true });
+});
+
+describe("legacy adoption under a degraded (unlocked) file lock", () => {
+  it("refuses the migration and leaves the file and legacy entries untouched", async () => {
+    const legacyBlob = JSON.stringify({
+      servers: { [SERVER]: { scope: "read" } },
+      idpSessions: {},
+    });
+    await writeStoreFile(filePath, legacyBlob);
+    await flushStoreFileWrites(filePath);
+    await store.set(
+      oauthSecretServerId(SERVER),
+      LEGACY_TOKENS_FIELD,
+      JSON.stringify(TOKENS),
+    );
+
+    await expect(
+      writeOAuthSections(filePath, snapshotFor("new"), undefined, store),
+    ).rejects.toThrow(SecretStoreUnavailableError);
+
+    // Nothing moved, nothing stamped: the legacy entry still resolves and
+    // the file carries no namespace, so a locked retry migrates cleanly.
+    expect(readFileSync(filePath, "utf8")).toBe(legacyBlob);
+    expect(
+      await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD),
+    ).toBe(JSON.stringify(TOKENS));
+  });
+
+  it("still mints for a fresh file — nothing to migrate, nothing to lose", async () => {
+    await writeOAuthSections(filePath, snapshotFor("fresh"), undefined, store);
+    await flushStoreFileWrites(filePath);
+
+    const ns = fileNamespace(filePath);
+    expect(isValidSecretsNamespace(ns)).toBe(true);
+    expect(
+      await store.get(oauthSecretServerId(SERVER, ns), LEGACY_TOKENS_FIELD),
+    ).not.toBeNull();
+  });
+
+  it("still mints for a recognized but entry-less legacy file", async () => {
+    // `{ servers: {}, idpSessions: {} }` indexes no store ids, so there is
+    // no destructive race to guard — refusing it would leave users on
+    // lock-hostile filesystems unable to save forever.
+    await writeStoreFile(
+      filePath,
+      JSON.stringify({ servers: {}, idpSessions: {} }),
+    );
+    await flushStoreFileWrites(filePath);
+
+    await writeOAuthSections(filePath, snapshotFor("empty"), undefined, store);
+    await flushStoreFileWrites(filePath);
+
+    const ns = fileNamespace(filePath);
+    expect(isValidSecretsNamespace(ns)).toBe(true);
+    expect(
+      await store.get(oauthSecretServerId(SERVER, ns), LEGACY_TOKENS_FIELD),
+    ).not.toBeNull();
+  });
+
+  it("still saves against an already-stamped file under its namespace", async () => {
+    await writeOAuthSections(filePath, snapshotFor("first"), undefined, store);
+    await flushStoreFileWrites(filePath);
+    const ns = fileNamespace(filePath);
+
+    await writeOAuthSections(filePath, snapshotFor("second"), undefined, store);
+    await flushStoreFileWrites(filePath);
+
+    expect(fileNamespace(filePath)).toBe(ns);
+    expect(
+      JSON.parse(
+        (await store.get(
+          oauthSecretServerId(SERVER, ns),
+          LEGACY_TOKENS_FIELD,
+        ))!,
+      ),
+    ).toMatchObject({ access_token: "at-second" });
+  });
+});