diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index 0965b093e..210cac678 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/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)); }); diff --git a/clients/web/src/test/core/auth/oauth-persist-file.test.ts b/clients/web/src/test/core/auth/oauth-persist-file.test.ts index eab3ea482..2f160335e 100644 --- a/clients/web/src/test/core/auth/oauth-persist-file.test.ts +++ b/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, + async (_path, fn) => fn(true) as Promise, ); 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, + async (_path, fn) => fn(true) as Promise, ); 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, + async (_path, fn) => fn(true) as Promise, ); }); @@ -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)) diff --git a/clients/web/src/test/core/auth/oauth-secrets.test.ts b/clients/web/src/test/core/auth/oauth-secrets.test.ts index b47ae2d89..cdbb1539e 100644 --- a/clients/web/src/test/core/auth/oauth-secrets.test.ts +++ b/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", () => { diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index 5dccf460e..c74b646c7 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/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. diff --git a/clients/web/src/test/integration/auth/node/oauth-adoption-locking.test.ts b/clients/web/src/test/integration/auth/node/oauth-adoption-locking.test.ts new file mode 100644 index 000000000..4d15a7d7a --- /dev/null +++ b/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 ( + _filePath: string, + fn: (locked: boolean) => Promise, + ): Promise => 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" }); + }); +}); diff --git a/clients/web/src/test/integration/auth/node/oauth-secrets-namespace.test.ts b/clients/web/src/test/integration/auth/node/oauth-secrets-namespace.test.ts new file mode 100644 index 000000000..bb7452846 --- /dev/null +++ b/clients/web/src/test/integration/auth/node/oauth-secrets-namespace.test.ts @@ -0,0 +1,505 @@ +/** + * Integration tests for the per-state-file secrets namespace (#2549): + * isolation between two state files sharing a server, adoption of a legacy + * (un-namespaced) file's store entries, fresh-file minting, invalid + * namespaces, adoption rollback, and keyring-backend isolation (the issue's + * acceptance criterion — exercised via the mocked `@napi-rs/keyring` + * bindings, the same pattern as secret-store.test.ts, since the real + * keychain is unreachable in CI). + */ + +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"; + +// Hoisted above the secret-store import so KeyringSecretStore binds to the +// in-memory fake instead of the native module (absent in CI). +const keyringMocks = vi.hoisted(() => { + const password = new Map(); + class AsyncEntry { + private readonly key: string; + constructor(_service: string, username: string) { + this.key = username; + } + async getPassword(): Promise { + const v = password.get(this.key); + return v === undefined || v === null ? undefined : v; + } + async setPassword(value: string): Promise { + password.set(this.key, value); + } + async deleteCredential(): Promise { + return password.delete(this.key); + } + } + const findCredentialsAsync = async (): Promise< + Array<{ account: string; password: string }> + > => { + const out: Array<{ account: string; password: string }> = []; + for (const [k, v] of password.entries()) { + if (v !== null) out.push({ account: k, password: v }); + } + return out; + }; + return { AsyncEntry, findCredentialsAsync, password }; +}); + +vi.mock("@napi-rs/keyring", () => ({ + AsyncEntry: keyringMocks.AsyncEntry, + findCredentialsAsync: keyringMocks.findCredentialsAsync, +})); + +// Passthrough mock so one test can make adoption's stamp write fail at the +// commit point; every other call runs the real implementation. +vi.mock("@inspector/core/storage/store-io.js", async (importOriginal) => { + const actual = + await importOriginal< + typeof import("@inspector/core/storage/store-io.js") + >(); + return { + ...actual, + writeStoreFile: vi.fn(actual.writeStoreFile), + }; +}); + +import { + writeOAuthSections, + readOAuthStore, + removeOAuthStore, + resetOAuthSecretStoreWarnings, + SECRETS_NAMESPACE_KEY, +} from "@inspector/core/auth/node/oauth-persist-file.js"; +import { + InMemorySecretStore, + KeyringSecretStore, + type SecretStore, +} from "@inspector/core/auth/node/secret-store.js"; +import { FileSecretStore } from "@inspector/core/auth/node/file-secret-store.js"; +import { + PERSIST_TOKENS_ENV, + oauthSecretServerId, + oauthIdpSecretServerId, + isValidSecretsNamespace, + LEGACY_TOKENS_FIELD, + IDP_SESSION_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 ISSUER = "https://as.example"; + +function tokensFor(tag: string) { + return { + access_token: `at-${tag}`, + token_type: "Bearer", + refresh_token: `rt-${tag}`, + }; +} + +function snapshotFor(tag: string): OAuthPersistSnapshot { + return { + servers: { [SERVER]: { scope: "read", tokens: tokensFor(tag) } }, + idpSessions: {}, + }; +} + +function namespaceOf(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 fileA: string; +let fileB: string; +let savedPolicy: string | undefined; + +beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-oauth-ns-")); + fileA = join(tempDir, "profile-a.json"); + fileB = join(tempDir, "profile-b.json"); + savedPolicy = process.env[PERSIST_TOKENS_ENV]; + delete process.env[PERSIST_TOKENS_ENV]; + keyringMocks.password.clear(); +}); + +afterEach(() => { + if (savedPolicy === undefined) delete process.env[PERSIST_TOKENS_ENV]; + else process.env[PERSIST_TOKENS_ENV] = savedPolicy; + resetPersistTokensPolicyWarnings(); + resetOAuthSecretStoreWarnings(); + vi.restoreAllMocks(); + rmSync(tempDir, { recursive: true, force: true }); +}); + +async function flushBoth(): Promise { + await flushStoreFileWrites(fileA); + await flushStoreFileWrites(fileB); +} + +/** The issue's core scenario, parameterized over the shared store backend. */ +async function assertTwoProfileIsolation(store: SecretStore): Promise { + await writeOAuthSections(fileA, snapshotFor("a"), undefined, store); + await writeOAuthSections(fileB, snapshotFor("b"), undefined, store); + await flushBoth(); + + const nsA = namespaceOf(fileA); + const nsB = namespaceOf(fileB); + expect(nsA).not.toBe(nsB); + expect(oauthSecretServerId(SERVER, nsA)).not.toBe( + oauthSecretServerId(SERVER, nsB), + ); + + // B's save must not have clobbered A's entry for the same server. + const readA = await readOAuthStore(fileA, store); + const readB = await readOAuthStore(fileB, store); + expect(readA?.servers[SERVER]?.tokens).toEqual(tokensFor("a")); + expect(readB?.servers[SERVER]?.tokens).toEqual(tokensFor("b")); +} + +describe("secrets namespace isolation (#2549)", () => { + it("keeps two state files' tokens for the same server apart in one shared store", async () => { + await assertTwoProfileIsolation(new InMemorySecretStore()); + }); + + it("keeps them apart on the keyring backend too (acceptance criterion)", async () => { + await assertTwoProfileIsolation(new KeyringSecretStore()); + // Both namespaced accounts coexist in the shared keychain. + const accounts = [...keyringMocks.password.keys()]; + expect( + accounts.filter((a) => a.includes(encodeURIComponent(SERVER))), + ).toHaveLength(2); + }); + + it("keeps them apart on the file backend too (acceptance criterion)", async () => { + // The real FileSecretStore: nested secrets-file locking and serialized + // whole-file mutations are backend-specific and not represented by the + // in-memory double. + await assertTwoProfileIsolation( + new FileSecretStore({ filePath: join(tempDir, "secrets.json") }), + ); + }); + + it("mints a valid namespace on a fresh file's first write and keeps it on later saves", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections(fileA, snapshotFor("a"), undefined, store); + await flushStoreFileWrites(fileA); + const ns = namespaceOf(fileA); + expect(isValidSecretsNamespace(ns)).toBe(true); + + await writeOAuthSections( + fileA, + { servers: { [SERVER]: { scope: "write" } }, idpSessions: {} }, + { servers: [SERVER] }, + store, + ); + await flushStoreFileWrites(fileA); + expect(namespaceOf(fileA)).toBe(ns); + }); + + it("removing one profile's state purges only its own entries, not the other's", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections(fileA, snapshotFor("a"), undefined, store); + await writeOAuthSections(fileB, snapshotFor("b"), undefined, store); + await flushBoth(); + const idB = oauthSecretServerId(SERVER, namespaceOf(fileB)); + + await removeOAuthStore(fileA, store); + + // B's scoped entry survives A's removal, and B still reads back whole. + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).not.toBeNull(); + const readB = await readOAuthStore(fileB, store); + expect(readB?.servers[SERVER]?.tokens).toEqual(tokensFor("b")); + }); +}); + +describe("legacy adoption (#2549)", () => { + /** A pre-namespace file plus its legacy unscoped store entries. */ + async function seedLegacy(store: SecretStore): Promise { + await writeStoreFile( + fileA, + JSON.stringify({ + servers: { [SERVER]: { scope: "read" } }, + idpSessions: { [ISSUER]: { clientInformation: { client_id: "idp" } } }, + }), + ); + await flushStoreFileWrites(fileA); + await store.set( + oauthSecretServerId(SERVER), + LEGACY_TOKENS_FIELD, + JSON.stringify(tokensFor("legacy")), + ); + await store.set( + oauthIdpSecretServerId(ISSUER), + IDP_SESSION_FIELD, + JSON.stringify({ tokens: tokensFor("idp") }), + ); + } + + it("first write stamps a namespace, moves legacy entries under it, and deletes the originals", async () => { + const store = new InMemorySecretStore(); + await seedLegacy(store); + + // A sectioned save touching an unrelated server triggers adoption. + await writeOAuthSections( + fileA, + { + servers: { "https://other.example": { scope: "x" } }, + idpSessions: {}, + }, + { servers: ["https://other.example"] }, + store, + ); + await flushStoreFileWrites(fileA); + + const ns = namespaceOf(fileA); + expect(isValidSecretsNamespace(ns)).toBe(true); + expect( + await store.get(oauthSecretServerId(SERVER, ns), LEGACY_TOKENS_FIELD), + ).toBe(JSON.stringify(tokensFor("legacy"))); + expect( + await store.get(oauthIdpSecretServerId(ISSUER, ns), IDP_SESSION_FIELD), + ).toBe(JSON.stringify({ tokens: tokensFor("idp") })); + // The shared legacy slots are retired. + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD), + ).toBeNull(); + expect( + await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), + ).toBeNull(); + + // Joined read-back still sees the moved tokens. + const read = await readOAuthStore(fileA, store); + expect(read?.servers[SERVER]?.tokens).toEqual(tokensFor("legacy")); + }); + + it("re-adopts under a fresh namespace when the stamp is stripped", async () => { + const store = new InMemorySecretStore(); + await seedLegacy(store); + // Simulate a file whose namespace stamp was lost (hand-edit, partial + // restore): earlier scoped entries exist, the file reads as legacy, and + // the next save re-adopts under a brand-new UUID. + await writeOAuthSections(fileA, snapshotFor("scoped"), undefined, store); + await flushStoreFileWrites(fileA); + const ns = namespaceOf(fileA); + const parsed = JSON.parse(readFileSync(fileA, "utf8")) as Record< + string, + unknown + >; + delete parsed[SECRETS_NAMESPACE_KEY]; + await writeStoreFile(fileA, JSON.stringify(parsed)); + await flushStoreFileWrites(fileA); + await store.set( + oauthSecretServerId(SERVER), + LEGACY_TOKENS_FIELD, + JSON.stringify(tokensFor("stale-legacy")), + ); + + await writeOAuthSections( + fileA, + { + servers: { "https://other.example": { scope: "x" } }, + idpSessions: {}, + }, + { servers: ["https://other.example"] }, + store, + ); + await flushStoreFileWrites(fileA); + + // The re-adoption minted a new UUID, so read it back from the file. + const ns2 = namespaceOf(fileA); + expect(ns2).not.toBe(ns); + expect( + await store.get(oauthSecretServerId(SERVER, ns2), LEGACY_TOKENS_FIELD), + ).toBe(JSON.stringify(tokensFor("stale-legacy"))); + }); + + it("a failed legacy purge still attempts every remaining legacy id", async () => { + const store = new InMemorySecretStore(); + await seedLegacy(store); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + // The first purge (the server's legacy id) fails; the idp session's + // must still be attempted rather than abandoned (best-effort per id). + const realDelete = store.deleteAllForServer.bind(store); + vi.spyOn(store, "deleteAllForServer").mockImplementation(async (id) => { + if (id === oauthSecretServerId(SERVER)) { + throw new Error("purge refused"); + } + return realDelete(id); + }); + + await writeOAuthSections( + fileA, + { + servers: { "https://other.example": { scope: "x" } }, + idpSessions: {}, + }, + { servers: ["https://other.example"] }, + store, + ); + await flushStoreFileWrites(fileA); + + const ns = namespaceOf(fileA); + // Both moves landed, and the idp legacy original was purged despite the + // earlier server purge failing. + expect( + await store.get(oauthSecretServerId(SERVER, ns), LEGACY_TOKENS_FIELD), + ).toBe(JSON.stringify(tokensFor("legacy"))); + expect( + await store.get(oauthIdpSecretServerId(ISSUER, ns), IDP_SESSION_FIELD), + ).toBe(JSON.stringify({ tokens: tokensFor("idp") })); + expect( + await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), + ).toBeNull(); + // The failed purge left its legacy original behind, warned not thrown. + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD), + ).toBe(JSON.stringify(tokensFor("legacy"))); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("legacy un-namespaced secret-store entries"), + ); + }); + + it("an invalid secretsNamespace is ignored on read (legacy ids) and replaced on save", async () => { + const store = new InMemorySecretStore(); + await writeStoreFile( + fileA, + JSON.stringify({ + [SECRETS_NAMESPACE_KEY]: "bad+delimiter", + servers: { [SERVER]: { scope: "read" } }, + idpSessions: {}, + }), + ); + await flushStoreFileWrites(fileA); + await store.set( + oauthSecretServerId(SERVER), + LEGACY_TOKENS_FIELD, + JSON.stringify(tokensFor("legacy")), + ); + + // Read resolves through legacy ids, never the invalid value. + const read = await readOAuthStore(fileA, store); + expect(read?.servers[SERVER]?.tokens).toEqual(tokensFor("legacy")); + + // A save (touching an unrelated server) treats the file as legacy: + // mints a fresh valid namespace and moves the legacy entry under it. + await writeOAuthSections( + fileA, + { + servers: { "https://other.example": { scope: "x" } }, + idpSessions: {}, + }, + { servers: ["https://other.example"] }, + store, + ); + await flushStoreFileWrites(fileA); + const ns = namespaceOf(fileA); + expect(isValidSecretsNamespace(ns)).toBe(true); + expect( + await store.get(oauthSecretServerId(SERVER, ns), LEGACY_TOKENS_FIELD), + ).toBe(JSON.stringify(tokensFor("legacy"))); + }); + + it("rolls the copied entries back when adoption cannot complete", async () => { + const store = new InMemorySecretStore(); + await seedLegacy(store); + // First scoped copy lands, second throws → the first must be restored + // and the legacy entries left untouched for the retry. + const realSet = store.set.bind(store); + let sets = 0; + vi.spyOn(store, "set").mockImplementation(async (id, field, value) => { + sets += 1; + if (sets === 2) throw new Error("keychain write refused"); + await realSet(id, field, value); + }); + + await expect( + writeOAuthSections(fileA, snapshotFor("new"), undefined, store), + ).rejects.toThrow("keychain write refused"); + + // Legacy entries are intact; no stamped namespace reached the file. + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD), + ).toBe(JSON.stringify(tokensFor("legacy"))); + expect( + await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), + ).toBe(JSON.stringify({ tokens: tokensFor("idp") })); + const parsed = JSON.parse(readFileSync(fileA, "utf8")) as Record< + string, + unknown + >; + expect(parsed[SECRETS_NAMESPACE_KEY]).toBeUndefined(); + }); + + it("rolls the scoped copies back when the commit-point stamp write fails", async () => { + const store = new InMemorySecretStore(); + await seedLegacy(store); + // Copies land, then the state-file stamp — the migration's commit + // point — rejects. The copies must be removed and the legacy file and + // ids left authoritative for the retry. + vi.mocked(writeStoreFile).mockImplementationOnce(async () => { + throw new Error("disk full during stamp"); + }); + + await expect( + writeOAuthSections(fileA, snapshotFor("new"), undefined, store), + ).rejects.toThrow("disk full during stamp"); + + // The namespace the failed stamp would have committed (from the blob + // handed to the rejected write) holds no copies. + const attempted = vi.mocked(writeStoreFile).mock.calls.at(-1)?.[1]; + const ns = (JSON.parse(attempted as string) as Record)[ + SECRETS_NAMESPACE_KEY + ] as string; + expect(isValidSecretsNamespace(ns)).toBe(true); + expect( + await store.get(oauthSecretServerId(SERVER, ns), LEGACY_TOKENS_FIELD), + ).toBeNull(); + expect( + await store.get(oauthIdpSecretServerId(ISSUER, ns), IDP_SESSION_FIELD), + ).toBeNull(); + // Legacy entries are intact and the file is still un-stamped. + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD), + ).toBe(JSON.stringify(tokensFor("legacy"))); + expect( + await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), + ).toBe(JSON.stringify({ tokens: tokensFor("idp") })); + const parsed = JSON.parse(readFileSync(fileA, "utf8")) as Record< + string, + unknown + >; + expect(parsed[SECRETS_NAMESPACE_KEY]).toBeUndefined(); + }); + + it("removing a still-legacy profile purges the shared legacy ids — deliberately", async () => { + // A pre-namespace file's live index IS the shared legacy ids, and the + // file being deleted is the store's only index of them: skipping the + // purge would strand credentials in the shared store with nothing left + // able to find or clear them. So removal keeps the pre-namespace + // world's semantics — another still-legacy profile sharing the server + // re-authorizes once, the same cost it pays when a sibling adopts. + // Isolation on removal is a property of *stamped* files (covered + // above), not a retroactive one. + const store = new InMemorySecretStore(); + await seedLegacy(store); + + await removeOAuthStore(fileA, store); + + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD), + ).toBeNull(); + expect( + await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), + ).toBeNull(); + expect(() => readFileSync(fileA, "utf8")).toThrow(); + }); +}); diff --git a/clients/web/src/test/integration/storage/adapters.test.ts b/clients/web/src/test/integration/storage/adapters.test.ts index 2cb50c25d..9005f4697 100644 --- a/clients/web/src/test/integration/storage/adapters.test.ts +++ b/clients/web/src/test/integration/storage/adapters.test.ts @@ -97,7 +97,7 @@ describe("OAuth persistence", () => { expect( JSON.parse( (await secretStore.get( - oauthSecretServerId("https://example.com"), + oauthSecretServerId("https://example.com", parsed.secretsNamespace), LEGACY_TOKENS_FIELD, ))!, ), @@ -319,7 +319,9 @@ describe("OAuth persistence", () => { ); await flushStoreFileWrites(filePath); const parsed = JSON.parse(readFileSync(filePath, "utf-8")); + // The first write also stamps the file's secrets namespace (#2549). expect(parsed).toEqual({ + secretsNamespace: expect.any(String), servers: { "https://mine.example": { scope: "mine" } }, idpSessions: {}, }); @@ -569,7 +571,7 @@ describe("OAuth persistence", () => { expect(raw.servers["https://example.com"].tokens).toBeUndefined(); expect( await secretStore.get( - oauthSecretServerId("https://example.com"), + oauthSecretServerId("https://example.com", raw.secretsNamespace), LEGACY_TOKENS_FIELD, ), ).not.toBeNull(); @@ -634,12 +636,13 @@ describe("OAuth persistence", () => { idpSessions: {}, }), }); - expect( - await secretStore.get( - oauthSecretServerId("https://example.com"), - LEGACY_TOKENS_FIELD, - ), - ).not.toBeNull(); + // The id is scoped by the file's adopted namespace; capture it while + // the file still exists (#2549). + const { secretsNamespace } = JSON.parse( + readFileSync(join(tempDir, "oauth.json"), "utf-8"), + ) as { secretsNamespace: string }; + const id = oauthSecretServerId("https://example.com", secretsNamespace); + expect(await secretStore.get(id, LEGACY_TOKENS_FIELD)).not.toBeNull(); const del = await fetch(`${baseUrl}/api/storage/oauth`, { method: "DELETE", @@ -647,12 +650,7 @@ describe("OAuth persistence", () => { }); expect(del.status).toBe(200); expect(existsSync(join(tempDir, "oauth.json"))).toBe(false); - expect( - await secretStore.get( - oauthSecretServerId("https://example.com"), - LEGACY_TOKENS_FIELD, - ), - ).toBeNull(); + expect(await secretStore.get(id, LEGACY_TOKENS_FIELD)).toBeNull(); }); }); }); diff --git a/clients/web/src/test/integration/storage/oauth-secret-split.test.ts b/clients/web/src/test/integration/storage/oauth-secret-split.test.ts index 24afda57d..0857a572a 100644 --- a/clients/web/src/test/integration/storage/oauth-secret-split.test.ts +++ b/clients/web/src/test/integration/storage/oauth-secret-split.test.ts @@ -95,6 +95,24 @@ function readRawFile(): OAuthPersistSnapshot { return JSON.parse(readFileSync(filePath, "utf8")) as OAuthPersistSnapshot; } +/** The secrets namespace the file's first write adopted (#2549). */ +function fileNamespace(): string { + const { secretsNamespace } = JSON.parse(readFileSync(filePath, "utf8")) as { + secretsNamespace: string; + }; + return secretsNamespace; +} + +/** Store id for a server under the file's adopted namespace. */ +function idOf(url: string): string { + return oauthSecretServerId(url, fileNamespace()); +} + +/** Store id for an IdP issuer under the file's adopted namespace. */ +function idpIdOf(issuer: string): string { + return oauthIdpSecretServerId(issuer, fileNamespace()); +} + describe("writeOAuthSections secret split", () => { it("writes only residue to the file and secrets to the store", async () => { const store = new InMemorySecretStore(); @@ -107,7 +125,7 @@ describe("writeOAuthSections secret split", () => { expect(raw.servers[SERVER]!.clientInformation).toEqual({ client_id: "cid", }); - const id = oauthSecretServerId(SERVER); + const id = idOf(SERVER); expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( TOKENS, ); @@ -134,9 +152,7 @@ describe("writeOAuthSections secret split", () => { const raw = readRawFile(); expect(raw.idpSessions[ISSUER]).toEqual({ idTokenExpiresAt: 9 }); - expect( - await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), - ).not.toBeNull(); + expect(await store.get(idpIdOf(ISSUER), IDP_SESSION_FIELD)).not.toBeNull(); const joined = await readOAuthStore(filePath, store); expect(joined?.idpSessions[ISSUER]).toEqual({ @@ -157,7 +173,7 @@ describe("writeOAuthSections secret split", () => { ); await flushStoreFileWrites(filePath); - const id = oauthSecretServerId(SERVER); + const id = idOf(SERVER); expect(await store.get(id, LEGACY_TOKENS_FIELD)).toBeNull(); expect(await store.get(id, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); expect(readRawFile().servers[SERVER]).toBeUndefined(); @@ -183,7 +199,7 @@ describe("writeOAuthSections secret split", () => { { servers: [SERVER] }, store, ); - const id = oauthSecretServerId(SERVER); + const id = idOf(SERVER); expect(await store.get(id, issuerTokensField(ISSUER))).not.toBeNull(); await writeOAuthSections( @@ -198,10 +214,10 @@ describe("writeOAuthSections secret split", () => { it("enforces the persist-tokens policy and self-cleans on downgrade", async () => { const store = new InMemorySecretStore(); - const id = oauthSecretServerId(SERVER); process.env[PERSIST_TOKENS_ENV] = "access"; await writeOAuthSections(filePath, snapshotWith(), undefined, store); + const id = idOf(SERVER); expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual({ access_token: "at", token_type: "Bearer", @@ -222,7 +238,7 @@ describe("writeOAuthSections secret split", () => { undefined, store, ); - const id = oauthIdpSecretServerId(ISSUER); + const id = idpIdOf(ISSUER); expect(await store.get(id, IDP_SESSION_FIELD)).not.toBeNull(); // Sections naming only idpSessions also exercises the servers-omitted @@ -395,7 +411,7 @@ describe("writeOAuthSections secret split", () => { // The store holds the *old* secrets again, matching the old residue // still on disk — no cid/cs2 mismatch on the next joined read. - const id = oauthSecretServerId(SERVER); + const id = idOf(SERVER); expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( TOKENS, ); @@ -434,9 +450,9 @@ describe("writeOAuthSections secret split", () => { chmodSync(tempDir, 0o755); } - expect( - await store.get(oauthSecretServerId(SERVER), LEGACY_CLIENT_SECRET_FIELD), - ).toBe("cs"); + expect(await store.get(idOf(SERVER), LEGACY_CLIENT_SECRET_FIELD)).toBe( + "cs", + ); }); it("aborts the write when a store delete fails, keeping the old residue", async () => { @@ -1016,15 +1032,13 @@ describe("removeOAuthStore", () => { store, ); await flushStoreFileWrites(filePath); + const id = idOf(SERVER); + const idpId = idpIdOf(ISSUER); await removeOAuthStore(filePath, store); expect(existsSync(filePath)).toBe(false); - expect( - await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD), - ).toBeNull(); - expect( - await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), - ).toBeNull(); + expect(await store.get(id, LEGACY_TOKENS_FIELD)).toBeNull(); + expect(await store.get(idpId, IDP_SESSION_FIELD)).toBeNull(); }); it("propagates a failed purge and leaves the file as the index", async () => { @@ -1078,13 +1092,9 @@ describe("removeOAuthStore", () => { // The first target's purged secrets were restored — a retry of the // removal (or a plain read) still finds everything the file indexes. expect( - JSON.parse( - (await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD))!, - ), + JSON.parse((await store.get(idOf(SERVER), LEGACY_TOKENS_FIELD))!), ).toEqual(TOKENS); - expect( - await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), - ).not.toBeNull(); + expect(await store.get(idpIdOf(ISSUER), IDP_SESSION_FIELD)).not.toBeNull(); }); it("restores purged secrets when the file delete fails", async () => { @@ -1104,13 +1114,11 @@ describe("removeOAuthStore", () => { // The file survives as the index and the store matches it again. expect(existsSync(filePath)).toBe(true); expect( - JSON.parse( - (await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD))!, - ), + JSON.parse((await store.get(idOf(SERVER), LEGACY_TOKENS_FIELD))!), ).toEqual(TOKENS); - expect( - await store.get(oauthSecretServerId(SERVER), LEGACY_CLIENT_SECRET_FIELD), - ).toBe("cs"); + expect(await store.get(idOf(SERVER), LEGACY_CLIENT_SECRET_FIELD)).toBe( + "cs", + ); }); it("is a no-op purge for a missing file", async () => { @@ -1268,12 +1276,12 @@ describe("partial token payloads round-trip through the store", () => { it("a save moves a partial token payload to the store and serves it back", async () => { const store = new InMemorySecretStore(); - const id = oauthSecretServerId(SERVER); const snapshot = snapshotWith(); snapshot.servers[SERVER]!.tokens = { ...PARTIAL } as never; await writeOAuthSections(filePath, snapshot, { servers: [SERVER] }, store); await flushStoreFileWrites(filePath); + const id = idOf(SERVER); // The bearer-grade refresh token is in the store, not the file. expect(readRawFile().servers[SERVER]!.tokens).toBeUndefined(); @@ -1386,7 +1394,7 @@ describe("saves only touch changed store fields", () => { expect(raw.servers[SERVER]!.tokens).toBeUndefined(); // The store still holds the unchanged credentials, untouched. - const id = oauthSecretServerId(SERVER); + const id = idOf(SERVER); expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( TOKENS, ); diff --git a/clients/web/src/test/integration/storage/oauth-write-convergence.test.ts b/clients/web/src/test/integration/storage/oauth-write-convergence.test.ts index 22b7985c0..7d4562b90 100644 --- a/clients/web/src/test/integration/storage/oauth-write-convergence.test.ts +++ b/clients/web/src/test/integration/storage/oauth-write-convergence.test.ts @@ -82,6 +82,8 @@ let filePath: string; let store: InMemorySecretStore; /** File bytes holding only server A, as the racing writer would leave them. */ let onlyA: string; +/** Store id for a url under the seed write's adopted secrets namespace. */ +let idOf: (url: string) => string; beforeEach(async () => { tempDir = mkdtempSync(join(tmpdir(), "inspector-oauth-converge-")); @@ -98,6 +100,10 @@ beforeEach(async () => { store, ); onlyA = readFileSync(filePath, "utf-8"); + const { secretsNamespace } = JSON.parse(onlyA) as { + secretsNamespace: string; + }; + idOf = (url) => oauthSecretServerId(url, secretsNamespace); }); afterEach(() => { @@ -130,6 +136,100 @@ describe("writeOAuthSections convergence verification", () => { expect(read?.servers[SERVER_B]?.tokens?.access_token).toBe("at-b"); }); + it("converges onto a concurrent adopter's namespace instead of re-stamping its own", async () => { + // The degraded-lock first-write race: another writer adopted a different + // namespace and won the file between our write and read-back. The retry + // must re-key to the namespace observed on disk — re-stamping our own + // mint would ping-pong and strand the other writer's secrets. + const racingNs = "99999999-9999-4999-8999-999999999999"; + const racing = JSON.parse(onlyA) as Record; + racing.secretsNamespace = racingNs; + // The racing writer's own store entries live under its namespace. + await store.set( + oauthSecretServerId(SERVER_A, racingNs), + LEGACY_TOKENS_FIELD, + JSON.stringify({ access_token: "at-racing", token_type: "Bearer" }), + ); + let clobbers = 0; + hook.afterWrite = (path) => { + if (clobbers++ === 0) writeFileSync(path, JSON.stringify(racing)); + }; + + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ); + + // Seed + clobbered attempt + converging retry. + expect(vi.mocked(writeStoreFile)).toHaveBeenCalledTimes(3); + const final = JSON.parse(readFileSync(filePath, "utf-8")) as { + secretsNamespace: string; + }; + expect(final.secretsNamespace).toBe(racingNs); + // B's secrets were written under the adopted namespace… + expect( + await store.get( + oauthSecretServerId(SERVER_B, racingNs), + LEGACY_TOKENS_FIELD, + ), + ).not.toBeNull(); + // …and the abandoned attempt's writes under our own mint were unwound. + expect(await store.get(idOf(SERVER_B), LEGACY_TOKENS_FIELD)).toBeNull(); + expect( + await store.get(idOf(SERVER_B), LEGACY_CLIENT_SECRET_FIELD), + ).toBeNull(); + // Joined read-back sees both writers' entries under the one namespace. + const read = await readOAuthStore(filePath, store); + expect(read?.servers[SERVER_B]?.tokens?.access_token).toBe("at-b"); + expect(read?.servers[SERVER_A]?.tokens?.access_token).toBe("at-racing"); + }); + + it("aborts the namespace re-key when the baseline restore fails, instead of reporting success", async () => { + // Same race as above, but unwinding the abandoned attempt's store + // writes fails. Carrying on would clear the rollback baseline and let + // the save report success with those writes stranded under a namespace + // no file references — the re-key must abort instead, leaving a loud + // failure a retried save can converge from. + const racingNs = "99999999-9999-4999-8999-999999999999"; + const racing = JSON.parse(onlyA) as Record; + racing.secretsNamespace = racingNs; + let clobbers = 0; + hook.afterWrite = (path) => { + if (clobbers++ === 0) writeFileSync(path, JSON.stringify(racing)); + }; + // The re-key restore deletes the abandoned attempt's new-entry writes + // (their baseline is "absent"). Refuse the first such delete once; the + // failure-path rollback that follows retries it and succeeds. + const realDelete = store.delete.bind(store); + let refused = false; + vi.spyOn(store, "delete").mockImplementation(async (serverId, field) => { + if (!refused && serverId === idOf(SERVER_B)) { + refused = true; + throw new Error("keychain delete refused"); + } + await realDelete(serverId, field); + }); + + await expect( + writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ), + ).rejects.toThrow("keychain delete refused"); + + // The failure-path rollback unwound the abandoned writes after all — + // nothing is stranded under our mint, and the racing file stands. + expect(await store.get(idOf(SERVER_B), LEGACY_TOKENS_FIELD)).toBeNull(); + const final = JSON.parse(readFileSync(filePath, "utf-8")) as { + secretsNamespace: string; + }; + expect(final.secretsNamespace).toBe(racingNs); + }); + it("gives up with a typed, retryable error when the file keeps changing", async () => { hook.afterWrite = (path) => writeFileSync(path, onlyA); @@ -160,11 +260,11 @@ describe("writeOAuthSections convergence verification", () => { // Server B never made it into the file, so its secrets must not linger // in the store (they would have no index for removeOAuthStore to find). - const idB = oauthSecretServerId(SERVER_B); + const idB = idOf(SERVER_B); expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBeNull(); expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); // Server A's stored secrets are untouched. - const idA = oauthSecretServerId(SERVER_A); + const idA = idOf(SERVER_A); expect(await store.get(idA, LEGACY_TOKENS_FIELD)).not.toBeNull(); expect(await store.get(idA, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-a"); }); @@ -192,10 +292,10 @@ describe("writeOAuthSections convergence verification", () => { ), ).rejects.toThrow(/disk full/); - const idB = oauthSecretServerId(SERVER_B); + const idB = idOf(SERVER_B); expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBeNull(); expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); - const idA = oauthSecretServerId(SERVER_A); + const idA = idOf(SERVER_A); expect(await store.get(idA, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-a"); expect(readFileSync(filePath, "utf-8")).toBe(onlyA); }); @@ -216,10 +316,10 @@ describe("writeOAuthSections convergence verification", () => { ), ).rejects.toThrow(/refusing/i); - const idB = oauthSecretServerId(SERVER_B); + const idB = idOf(SERVER_B); expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBeNull(); expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); - const idA = oauthSecretServerId(SERVER_A); + const idA = idOf(SERVER_A); expect(await store.get(idA, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-a"); }); @@ -230,7 +330,7 @@ describe("writeOAuthSections convergence verification", () => { // writer. The rollback baseline folds each attempt's priors, telling our // own earlier attempt's writes (equal to what this call writes — they // are constant across attempts) apart from foreign values. - const idB = oauthSecretServerId(SERVER_B); + const idB = idOf(SERVER_B); await writeOAuthSections( filePath, snapshotOf({ [SERVER_B]: serverState("b") }), @@ -278,7 +378,7 @@ describe("writeOAuthSections convergence verification", () => { // pairs with — so the failure escalates into the reconciling exit, which // finds the file changed and restores the pre-operation secrets. const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); - const idB = oauthSecretServerId(SERVER_B); + const idB = idOf(SERVER_B); await writeOAuthSections( filePath, snapshotOf({ [SERVER_B]: serverState("b") }), @@ -337,7 +437,7 @@ describe("writeOAuthSections convergence verification", () => { // so attempt 2's store writes must be rolled back to the baseline, not // left in place under the old residue. const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); - const idB = oauthSecretServerId(SERVER_B); + const idB = idOf(SERVER_B); await writeOAuthSections( filePath, snapshotOf({ [SERVER_B]: serverState("b") }), @@ -358,11 +458,12 @@ describe("writeOAuthSections convergence verification", () => { let reads = 0; hook.beforeRead = () => { reads += 1; - // Read 1: attempt 1's disk read. Read 2: its failing read-back. - // Read 3: attempt 2's disk read — the store has recovered by now. - // Read 4: the reconciling exit's confirmation read. - if (reads === 2) throw new Error("EIO: read failed"); - if (reads === 3) failNewSets = false; + // Read 1: the save's namespace-adoption read. Read 2: attempt 1's + // disk read. Read 3: its failing read-back. Read 4: attempt 2's disk + // read — the store has recovered by now. Read 5: the reconciling + // exit's confirmation read. + if (reads === 3) throw new Error("EIO: read failed"); + if (reads === 4) failNewSets = false; }; let writes = 0; hook.beforeWrite = () => { @@ -393,7 +494,7 @@ describe("writeOAuthSections convergence verification", () => { // reconciling exit. The file is confirmed to still hold attempt 1's // write, so the save is committed: the store is re-pointed at attempt // 1's values and the call reports success. - const idB = oauthSecretServerId(SERVER_B); + const idB = idOf(SERVER_B); await writeOAuthSections( filePath, snapshotOf({ [SERVER_B]: serverState("b") }), @@ -410,12 +511,13 @@ describe("writeOAuthSections convergence verification", () => { let reads = 0; hook.beforeRead = () => { reads += 1; - // Read 1: attempt 1's disk read. Read 2: its failing read-back. - // Read 3: attempt 2's disk read — the store starts flaking here. - // Read 4: the confirmation read — the flake has passed. - if (reads === 2) throw new Error("EIO: read failed"); - if (reads === 3) failSets = true; - if (reads === 4) failSets = false; + // Read 1: the save's namespace-adoption read. Read 2: attempt 1's + // disk read. Read 3: its failing read-back. Read 4: attempt 2's disk + // read — the store starts flaking here. Read 5: the confirmation + // read — the flake has passed. + if (reads === 3) throw new Error("EIO: read failed"); + if (reads === 4) failSets = true; + if (reads === 5) failSets = false; }; await writeOAuthSections( @@ -441,10 +543,11 @@ describe("writeOAuthSections convergence verification", () => { let reads = 0; hook.beforeRead = () => { reads += 1; - // Read 1: attempt 1's disk read. Reads 2-3: attempt 1's verifying - // read-back and attempt 2's disk read, both failing. Read 4: the - // reconciling exit's confirmation read, which succeeds. - if (reads === 2 || reads === 3) throw new Error("EIO: read failed"); + // Read 1: the save's namespace-adoption read. Read 2: attempt 1's + // disk read. Reads 3-4: attempt 1's verifying read-back and attempt + // 2's disk read, both failing. Read 5: the reconciling exit's + // confirmation read, which succeeds. + if (reads === 3 || reads === 4) throw new Error("EIO: read failed"); }; await writeOAuthSections( @@ -454,7 +557,7 @@ describe("writeOAuthSections convergence verification", () => { store, ); - const idB = oauthSecretServerId(SERVER_B); + const idB = idOf(SERVER_B); expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toContain("at-b"); expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-b"); const read = await readOAuthStore(filePath, store); @@ -470,7 +573,9 @@ describe("writeOAuthSections convergence verification", () => { let reads = 0; hook.beforeRead = () => { reads += 1; - if (reads >= 2) throw new Error("EIO: read failed"); + // Read 1 is the save's namespace-adoption read, read 2 attempt 1's + // disk read; everything after fails. + if (reads >= 3) throw new Error("EIO: read failed"); }; await expect( @@ -482,7 +587,7 @@ describe("writeOAuthSections convergence verification", () => { ), ).rejects.toThrow(/EIO/); - const idB = oauthSecretServerId(SERVER_B); + const idB = idOf(SERVER_B); expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBeNull(); expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); expect( diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index b1eede68b..295bb401b 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -391,17 +391,6 @@ export async function isFileLockHeld(filePath: string): Promise { } } -/** - * Run `fn` holding an exclusive cross-process lock on `filePath`. - * - * The lock is `.lock`, a directory beside the secrets file rather - * than inside it — `proper-lockfile` never opens or truncates the file it - * guards, so a lock that outlives its holder can only ever block a write, - * never damage one. - * - * Returns whatever `fn` returns. `fn` runs exactly once either way — the - * lock's absence changes the guarantee, never whether the work happens. - */ /** * Take the lock and hand back its release, or `null` when locking is * unavailable here and the caller should proceed unprotected. @@ -513,14 +502,29 @@ export async function openSecretFileLock( }; } +/** + * Run `fn` holding an exclusive cross-process lock on `filePath`. + * + * The lock is `.lock`, a directory beside the secrets file rather + * than inside it — `proper-lockfile` never opens or truncates the file it + * guards, so a lock that outlives its holder can only ever block a write, + * never damage one. + * + * Returns whatever `fn` returns. `fn` runs exactly once either way — the + * lock's absence changes the guarantee, never whether the work happens. + * `fn` receives whether the lock is actually held (`false` = degraded, + * unlocked run), so a caller whose work is only safe under real exclusion + * — legacy secret-entry migration, which deletes its sources — can refuse + * instead of racing (#2556 review). + */ export async function withSecretFileLock( filePath: string, - fn: () => Promise, + fn: (locked: boolean) => Promise, ): Promise { const release = await openSecretFileLock(filePath); - if (release === null) return fn(); + if (release === null) return fn(false); try { - return await fn(); + return await fn(true); } finally { await release(); } diff --git a/core/auth/node/oauth-persist-file.ts b/core/auth/node/oauth-persist-file.ts index 8c707bf25..8a91b4c01 100644 --- a/core/auth/node/oauth-persist-file.ts +++ b/core/auth/node/oauth-persist-file.ts @@ -21,6 +21,13 @@ * store is durable, the same guard the mcp.json/client.json migrations * use. A store write failure degrades those tokens to memory-only with a * loud warning; it never falls back to writing them into the file. + * 3. **Secret-store namespace** (#2549): each state file carries a + * `secretsNamespace` UUID, baked into every secret-store id its entries + * use, so two state files (profiles) naming the same server hold + * separate store entries instead of overwriting one shared slot. The + * namespace is minted — and a legacy file's unscoped entries moved under + * it — on the file's first write (`adoptSecretsNamespace`); reads and + * removes honor whatever the file says and never adopt. */ import { @@ -28,6 +35,7 @@ import { writeStoreFile, deleteStoreFile, } from "../../storage/store-io.js"; +import { serializeStore } from "../../storage/store-serialize.js"; import { setOwnEntry, getOwnEntry } from "../../storage/own-entry.js"; import { mergeOAuthSections, @@ -58,8 +66,10 @@ import { IDP_SESSION_FIELD, getPersistTokensPolicy, isUsableStoredSecret, + isValidSecretsNamespace, joinIdpSession, joinServerOAuthState, + newSecretsNamespace, oauthIdpSecretServerId, oauthSecretServerId, serverSecretFields, @@ -187,13 +197,13 @@ function rethrowLockError( async function withOAuthStateLock( filePath: string, action: "save" | "read" | "remove", - body: () => Promise, + body: (locked: boolean) => Promise, ): Promise { let entered = false; try { - return await withSecretFileLock(filePath, async () => { + return await withSecretFileLock(filePath, async (locked) => { entered = true; - return body(); + return body(locked); }); } catch (error) { if (!entered) rethrowLockError(filePath, error, action); @@ -225,20 +235,182 @@ export class OAuthStateFileUnrecognizedError extends Error { } /** - * Locked-read helper for the mutation paths: parse the OAuth state file, - * distinguishing "absent" (null) from "present but unrecognized" (refuse — - * see {@link OAuthStateFileUnrecognizedError}). + * Top-level state-file key holding the file's secrets namespace (#2549): a + * UUID minted per state file and baked into every secret-store id the file's + * entries use (see `oauthSecretServerId`). It is what keeps two state files + * (profiles) that connect to the same server from sharing — and silently + * overwriting — one store entry. Node-file-backend-only: the browser and + * sessionStorage backends never see it ({@link parseOAuthPersistBlob} + * ignores unknown keys), and every writer re-reads it from disk under the + * file lock, so a snapshot round-tripped through the API cannot strip it. */ -async function readDiskForMutation( +export const SECRETS_NAMESPACE_KEY = "secretsNamespace"; + +/** + * Extract the secrets namespace from a raw state-file blob. Tolerant like + * the read path: an unparseable file or an invalid value reads as "no + * namespace" (legacy unscoped ids) rather than failing — an invalid value + * must not reach store ids, where it could forge the `+` delimiter or break + * the keyring's colon parse (see `isValidSecretsNamespace`). + */ +function parseSecretsNamespace(raw: string | null): string | undefined { + if (raw === null) return undefined; + try { + const parsed: unknown = JSON.parse(raw); + if (typeof parsed !== "object" || parsed === null) return undefined; + const value = (parsed as Record)[SECRETS_NAMESPACE_KEY]; + return isValidSecretsNamespace(value) ? value : undefined; + } catch { + return undefined; + } +} + +/** + * Serialize a snapshot for the state file, stamping the secrets namespace + * first so it survives every rewrite (sectioned saves, the plaintext-secret + * migration, adoption itself). Key order is fixed — namespace, then the + * snapshot — because the write path compares raw file strings to detect + * concurrent writers. + */ +function serializeOAuthFileBlob( + snapshot: OAuthPersistSnapshot, + namespace: string | undefined, +): string { + if (namespace === undefined) return serializeOAuthPersistBlob(snapshot); + return serializeStore({ [SECRETS_NAMESPACE_KEY]: namespace, ...snapshot }); +} + +/** + * Cleanup-failure variant of {@link warnStoreWriteFailure}: namespace + * adoption copied the legacy unscoped entries to their namespaced ids and + * stamped the file, but deleting the legacy originals failed. Nothing is + * lost — the namespaced ids are authoritative from here on — but the + * leftovers sit in the store un-indexed (no state file will purge them) and + * could serve stale credentials to a profile that has not adopted yet. + */ +function warnNamespaceCleanupFailure(error: unknown): void { + const reason = error instanceof Error ? error.message : String(error); + const key = `namespace-cleanup:${reason}`; + if (warnedStoreFailures.has(key)) return; + warnedStoreFailures.add(key); + console.warn( + `[mcp-inspector] Could not remove legacy un-namespaced secret-store entries after scoping them to this state file (${reason}). This file uses the scoped copies, but another pre-namespace state file could still read the stale leftovers until it adopts or re-authorizes — and they will not be cleaned up automatically.`, + ); +} + +/** + * Ensure the state file has a secrets namespace, minting and stamping one — + * and moving its legacy unscoped store entries under it — when it does not + * (#2549). Runs under the caller's file lock, on the mutation path only: + * reads keep working against whatever the file currently says, so a + * pre-adoption profile loses nothing until it first writes. + * + * For a legacy file (recognized, entries, no namespace) the move is + * copy → stamp → delete, in that order, because each step's failure mode + * differs: + * + * - **Copy** (legacy id → namespaced id) lands on vacant scoped ids — the + * namespace is freshly minted, so nothing can already live under it. A + * failure deletes the copies it made and rethrows — the file is + * unstamped, so everything still resolves through the legacy ids and + * the next write retries under a new UUID. + * - **Stamp** (rewrite the file with the namespace) is the commit point: a + * failure triggers the same restore, because a successful copy with no + * stamp would be re-run under a *different* UUID next time, stranding + * this one's copies forever. + * - **Delete** (the legacy originals) is best-effort after the commit: the + * namespaced ids are already authoritative for this file, so a failure + * here never loses a token — but the leftovers are not harmless to + * everyone: they are unindexed here, and a pre-namespace profile could + * still read them as stale credentials until it adopts or re-authorizes + * ({@link warnNamespaceCleanupFailure}). Deleting is deliberate, not + * cautious copying: the legacy entry is exactly the shared slot this + * change exists to retire, and `removeOAuthStore` purges by the file's + * own ids, so a leftover would otherwise be orphaned forever. Another + * profile still reading the legacy ids re-authorizes once — its copy of + * those tokens was already being overwritten by every other profile, + * which is the bug. + * + * A fresh file (absent, or no entries on disk) just mints: the namespace + * reaches disk with the write that follows, and there is nothing to move. + * + * Migration requires the real file lock (`locked`). The move deletes its + * sources, so two unlocked adopters racing on one legacy file can each + * observe the other's half-finished move — one copies and deletes, the + * other strict-reads nothing, stamps an empty namespace of its own, and + * can win the file, stranding the first's copies under an abandoned + * namespace. Under the lock adopters serialize (the second sees the + * first's stamp and returns it); degraded, this refuses the one-time + * migration loudly rather than risking that loss. Mint-only adoption — + * a file that is absent, or recognized but indexing no entries — stays + * allowed unlocked: there is nothing to move, and a concurrent + * mint converges via the namespace re-key in `writeOAuthSections`. + */ +async function adoptSecretsNamespace( filePath: string, - action: "save" | "remove", -): Promise { + secretStore: SecretStore, + locked: boolean, +): Promise { const raw = await readStoreFile(filePath); - const parsed = parseOAuthPersistBlob(raw); - if (raw !== null && parsed === null) { - throw new OAuthStateFileUnrecognizedError(filePath, action); + const existing = parseSecretsNamespace(raw); + if (existing !== undefined) return existing; + const snapshot = parseOAuthPersistBlob(raw); + if (raw !== null && snapshot === null) { + throw new OAuthStateFileUnrecognizedError(filePath, "save"); } - return parsed; + const namespace = newSecretsNamespace(); + if (snapshot === null) return namespace; + + const moves = [ + ...Object.entries(snapshot.servers).map(([url, state]) => ({ + legacyId: oauthSecretServerId(url), + scopedId: oauthSecretServerId(url, namespace), + fields: serverSecretFields(state), + })), + ...Object.keys(snapshot.idpSessions).map((issuer) => ({ + legacyId: oauthIdpSecretServerId(issuer), + scopedId: oauthIdpSecretServerId(issuer, namespace), + fields: [IDP_SESSION_FIELD], + })), + ]; + // A recognized but entry-less legacy file indexes no store ids, so no + // destructive race exists — it mints like a fresh file, locked or not + // (the save that follows writes the namespace to disk). + if (moves.length === 0) return namespace; + if (!locked) { + throw new SecretStoreUnavailableError( + `Could not save OAuth state: ${filePath} predates per-state-file secret namespaces, and migrating its secret-store entries needs the file lock, which is unavailable here (see the lock warning above). Migrating without it could lose credentials if two processes migrate at once. Nothing was changed; make the lock directory writable and retry — or, if you cannot, clear this file's stored OAuth state and re-authorize (clearing does not migrate, and the fresh state file mints its namespace without the lock).`, + ); + } + + // Rollback baseline: the scoped ids are vacant before this call — the + // namespace is a UUID minted moments ago, so nothing can already live + // under it — which makes "restore" simply "delete what we copied". + const touched: SecretFieldSnapshot[] = []; + try { + for (const { legacyId, scopedId, fields } of moves) { + for (const field of fields) { + const value = await secretStoreGetStrict(secretStore, legacyId, field); + if (value === null) continue; + touched.push({ serverId: scopedId, field, value: null }); + await secretStore.set(scopedId, field, value); + } + } + await writeStoreFile(filePath, serializeOAuthFileBlob(snapshot, namespace)); + } catch (error) { + await restoreSecretFields(secretStore, touched, warnRestoreFailure); + throw error; + } + // Per-id catch: one failed purge must not abandon the remaining legacy + // ids — each gets its own best-effort attempt. + for (const { legacyId } of moves) { + try { + await secretStore.deleteAllForServer(legacyId); + } catch (error) { + warnNamespaceCleanupFailure(error); + } + } + return namespace; } /** @@ -389,7 +561,16 @@ export async function writeOAuthSections( ): Promise { const policy = getPersistTokensPolicy(); const durable = await secretStoreIsDurable(secretStore); - await withOAuthStateLock(filePath, "save", async () => { + await withOAuthStateLock(filePath, "save", async (locked) => { + // The namespace scoping every store id below; minted (and legacy + // entries moved — under the real lock only, see adoptSecretsNamespace) + // on this file's first namespaced write. Resolved once per save: an + // attempt retry never re-ADOPTS — but it may re-KEY. Under degraded + // (unlocked) locking a concurrent first writer can mint a different + // namespace and win the file between this read and an attempt's write, + // so each attempt below re-checks the namespace observed on disk and + // converges onto it (`let`, not `const`). + let namespace = await adoptSecretsNamespace(filePath, secretStore, locked); // Restore baseline for every failure exit below. For each touched // (server, field) it holds the latest store value NOT written by this // call: the pre-operation value, superseded by a concurrent writer's @@ -536,10 +717,49 @@ export async function writeOAuthSections( // The disk read sits inside the try too: on a retry the store already // holds an earlier attempt's writes, and a concurrent writer replacing - // the file with something unrecognized would otherwise make - // `readDiskForMutation` throw past the loop without any rollback. + // the file with something unrecognized would otherwise throw past the + // loop without any rollback. Read raw, not just parsed: the namespace + // check below needs the unparsed blob. try { - const disk = await readDiskForMutation(filePath, "save"); + const rawDisk = await readStoreFile(filePath); + const disk = parseOAuthPersistBlob(rawDisk); + if (rawDisk !== null && disk === null) { + throw new OAuthStateFileUnrecognizedError(filePath, "save"); + } + // Namespace convergence: under degraded (unlocked) locking a + // concurrent first writer can adopt a different namespace and win + // the file after our adoption read. The merge below already + // converges the *data* onto what they left; the namespace must + // converge the same way, or every retry re-stamps our own mint and + // the two writers ping-pong, stranding the loser's secrets under a + // namespace the final file no longer references. Re-key to the + // namespace observed on disk, first rolling earlier attempts' store + // writes (all keyed under the abandoned namespace) back to baseline. + const diskNamespace = parseSecretsNamespace(rawDisk); + if (diskNamespace !== undefined && diskNamespace !== namespace) { + // Strict, unlike the failure exits' best-effort restores: this is + // normal control flow with no original error to preserve, and + // carrying on past a failed restore would clear the baseline and + // let the save report success with earlier attempts' writes + // stranded under the abandoned namespace, unindexed by any file. + // Aborting keeps the baseline for the rethrow's reconciliation, + // and a retried save converges cleanly. + let restoreFailure: unknown; + await restoreSecretFields( + secretStore, + [...restoreBaseline.values()], + (error) => { + restoreFailure ??= error; + }, + ); + if (restoreFailure !== undefined) throw restoreFailure; + restoreBaseline.clear(); + ourWrites.clear(); + // Our unconfirmed write carried the abandoned namespace, and the + // read above proves the file no longer holds it. + unconfirmed = null; + namespace = diskNamespace; + } // Deduplicated: caller-passed sections may repeat a URL/issuer, and a // second pass over the same entry would snapshot the value the first // pass just wrote — a rollback would then "restore" that intermediate @@ -569,7 +789,7 @@ export async function writeOAuthSections( const merged = mergeOAuthSections(disk, snapshot, effective); for (const url of effective.servers ?? []) { - const serverId = oauthSecretServerId(url); + const serverId = oauthSecretServerId(url, namespace); // Own-property reads: with a `__proto__` key a plain lookup on a // map that lacks it returns the inherited prototype, so a clear // would read as an update and skip the purge below. @@ -661,7 +881,7 @@ export async function writeOAuthSections( } for (const issuer of effective.idpSessions ?? []) { - const serverId = oauthIdpSecretServerId(issuer); + const serverId = oauthIdpSecretServerId(issuer, namespace); const next = getOwnEntry(snapshot.idpSessions, issuer); const entryPrior = await snapshotSecretFields(secretStore, serverId, [ IDP_SESSION_FIELD, @@ -724,7 +944,7 @@ export async function writeOAuthSections( }); } - written = serializeOAuthPersistBlob(merged); + written = serializeOAuthFileBlob(merged, namespace); await writeStoreFile(filePath, written); unconfirmed = { written, writes: attemptWrites }; } catch (error) { @@ -765,17 +985,18 @@ export async function writeOAuthSections( /** Build the bulk-read request list for everything a snapshot could hold. */ function secretRequestsFor( snapshot: OAuthPersistSnapshot, + namespace: string | undefined, ): SecretBulkRequest[] { const requests: SecretBulkRequest[] = []; for (const [url, state] of Object.entries(snapshot.servers)) { requests.push({ - serverId: oauthSecretServerId(url), + serverId: oauthSecretServerId(url, namespace), fields: serverSecretFields(state), }); } for (const issuer of Object.keys(snapshot.idpSessions)) { requests.push({ - serverId: oauthIdpSecretServerId(issuer), + serverId: oauthIdpSecretServerId(issuer, namespace), fields: [IDP_SESSION_FIELD], }); } @@ -786,8 +1007,9 @@ function secretRequestsFor( async function joinSnapshot( snapshot: OAuthPersistSnapshot, secretStore: SecretStore, + namespace: string | undefined, ): Promise { - const requests = secretRequestsFor(snapshot); + const requests = secretRequestsFor(snapshot, namespace); // Strict: this read hydrates the memory state that later sectioned writes // diff against, so a tolerant read during a store outage would present // every credential as absent — and the next save would *delete* them from @@ -801,13 +1023,19 @@ async function joinSnapshot( servers: Object.fromEntries( Object.entries(snapshot.servers).map(([url, state]) => [ url, - joinServerOAuthState(state, values[oauthSecretServerId(url)] ?? {}), + joinServerOAuthState( + state, + values[oauthSecretServerId(url, namespace)] ?? {}, + ), ]), ), idpSessions: Object.fromEntries( Object.entries(snapshot.idpSessions).map(([issuer, session]) => [ issuer, - joinIdpSession(session, values[oauthIdpSecretServerId(issuer)] ?? {}), + joinIdpSession( + session, + values[oauthIdpSecretServerId(issuer, namespace)] ?? {}, + ), ]), ), }; @@ -846,6 +1074,11 @@ async function migratePlaintextSecrets( const raw = await readStoreFile(filePath); const fresh = parseOAuthPersistBlob(raw); if (!fresh || !snapshotHasPlaintextSecrets(fresh)) return; + // Migration honors — and preserves — the file's own namespace; it never + // mints one. Scoping is a write-path decision (`adoptSecretsNamespace`), + // and a read-triggered strip that also re-keyed the entries would be the + // adoption without its legacy-entry move. + const namespace = parseSecretsNamespace(raw); const migrateEntrySecrets = async ( serverId: string, secrets: OAuthSecretValues, @@ -873,12 +1106,18 @@ async function migratePlaintextSecrets( for (const [url, state] of Object.entries(fresh.servers)) { const split = splitServerOAuthState(state, "all"); setOwnEntry(residue.servers, url, split.residue); - await migrateEntrySecrets(oauthSecretServerId(url), split.secrets); + await migrateEntrySecrets( + oauthSecretServerId(url, namespace), + split.secrets, + ); } for (const [issuer, session] of Object.entries(fresh.idpSessions)) { const split = splitIdpSession(session, "all"); setOwnEntry(residue.idpSessions, issuer, split.residue); - await migrateEntrySecrets(oauthIdpSecretServerId(issuer), split.secrets); + await migrateEntrySecrets( + oauthIdpSecretServerId(issuer, namespace), + split.secrets, + ); } // The split leaves only type-corrupt token payloads in the residue (see // `splitTokens`), so a hand-edited junk entry stays plaintext and @@ -886,7 +1125,7 @@ async function migratePlaintextSecrets( // reject. Such a file re-enters migration on every read; skip the rewrite // when nothing would change so a steady-state file is not re-written (and // a concurrent writer not clobbered) per read. - const stripped = serializeOAuthPersistBlob(residue); + const stripped = serializeOAuthFileBlob(residue, namespace); if (stripped !== raw) { await writeStoreFile(filePath, stripped); } @@ -922,7 +1161,8 @@ export async function readOAuthStore( secretStore: SecretStore = defaultSecretStore(), ): Promise { return withOAuthStateLock(filePath, "read", async () => { - let snapshot = parseOAuthPersistBlob(await readStoreFile(filePath)); + let raw = await readStoreFile(filePath); + let snapshot = parseOAuthPersistBlob(raw); if (snapshot === null) return null; if ( @@ -931,14 +1171,16 @@ export async function readOAuthStore( ) { try { await migratePlaintextSecrets(filePath, secretStore); - snapshot = - parseOAuthPersistBlob(await readStoreFile(filePath)) ?? snapshot; + raw = await readStoreFile(filePath); + snapshot = parseOAuthPersistBlob(raw) ?? snapshot; } catch (error) { warnMigrationFailure(error); } } - return joinSnapshot(snapshot, secretStore); + // Reads never adopt: a legacy file's entries stay at their legacy ids + // until a write mints the namespace and moves them (#2549). + return joinSnapshot(snapshot, secretStore, parseSecretsNamespace(raw)); }); } @@ -963,15 +1205,24 @@ export async function removeOAuthStore( secretStore: SecretStore = defaultSecretStore(), ): Promise { await withOAuthStateLock(filePath, "remove", async () => { - const snapshot = await readDiskForMutation(filePath, "remove"); + const rawBlob = await readStoreFile(filePath); + const snapshot = parseOAuthPersistBlob(rawBlob); + if (rawBlob !== null && snapshot === null) { + throw new OAuthStateFileUnrecognizedError(filePath, "remove"); + } if (snapshot) { + // Purge by the ids this file's entries actually use. A legacy + // (un-namespaced) file purges the legacy ids; a namespaced one must + // purge ONLY its own namespaced ids — the legacy ids may still be the + // live index of another, not-yet-adopted state file (#2549). + const namespace = parseSecretsNamespace(rawBlob); const targets = [ ...Object.entries(snapshot.servers).map(([url, state]) => ({ - id: oauthSecretServerId(url), + id: oauthSecretServerId(url, namespace), fields: serverSecretFields(state), })), ...Object.keys(snapshot.idpSessions).map((issuer) => ({ - id: oauthIdpSecretServerId(issuer), + id: oauthIdpSecretServerId(issuer, namespace), fields: [IDP_SESSION_FIELD], })), ]; diff --git a/core/auth/node/oauth-secrets.ts b/core/auth/node/oauth-secrets.ts index bf2c001f4..e62e99120 100644 --- a/core/auth/node/oauth-secrets.ts +++ b/core/auth/node/oauth-secrets.ts @@ -20,6 +20,7 @@ * route); the browser round-trips full snapshots over the authed local API. */ +import { randomUUID } from "node:crypto"; import { OAuthTokensSchema } from "@modelcontextprotocol/core"; import type { OAuthTokens } from "@modelcontextprotocol/client"; import { setOwnEntry } from "../../storage/own-entry.js"; @@ -74,11 +75,55 @@ export function resetPersistTokensPolicyWarnings(): void { * prefix of another's (`https://a` vs `https://a:8080`), letting * prefix-matching stores delete the wrong server's secrets. Encoding turns * `:` and `/` into `%3A`/`%2F`, which no other id can collide with. + * + * Ids are additionally scoped by the state file's secrets namespace + * (#2549): without it, two state files (profiles) that connect to the same + * server share one store entry, so profile B's login overwrites profile + * A's tokens and A silently acts as B. The namespace is a UUID stored in + * the state file itself (see `adoptSecretsNamespace` in + * `oauth-persist-file.ts`), inserted between the prefix and the encoded + * URL: `oauth++`. The delimiter stays unambiguous + * because `encodeURIComponent` escapes `+` (to `%2B`) and + * {@link isValidSecretsNamespace} rejects `+` (and `:`, keeping the id + * colon-free for the keyring account parse) — so a namespaced id can never + * equal a legacy unscoped one, and no (namespace, url) pair can produce + * another pair's id. `namespace === undefined` yields the legacy unscoped + * shape, still used to read (and migrate away from) pre-#2549 entries. */ -export const oauthSecretServerId = (serverUrl: string): string => - `oauth+${encodeURIComponent(serverUrl)}`; -export const oauthIdpSecretServerId = (issuer: string): string => - `oauth-idp+${encodeURIComponent(issuer)}`; +export const oauthSecretServerId = ( + serverUrl: string, + namespace?: string, +): string => + namespace === undefined + ? `oauth+${encodeURIComponent(serverUrl)}` + : `oauth+${namespace}+${encodeURIComponent(serverUrl)}`; +export const oauthIdpSecretServerId = ( + issuer: string, + namespace?: string, +): string => + namespace === undefined + ? `oauth-idp+${encodeURIComponent(issuer)}` + : `oauth-idp+${namespace}+${encodeURIComponent(issuer)}`; + +/** + * A usable secrets namespace: what `randomUUID()` produces, plus room for a + * hand-chosen value. The charset is what carries the id guarantees above — + * no `:` (keyring accounts parse at the first colon), no `+` (the id + * delimiter), and nothing `encodeURIComponent` leaves unescaped in a way + * that could forge a delimiter. Anything else in the file is ignored as if + * absent rather than propagated into store ids. + */ +export function isValidSecretsNamespace(value: unknown): value is string { + return ( + typeof value === "string" && + /^[A-Za-z0-9][A-Za-z0-9._-]{0,127}$/.test(value) + ); +} + +/** Mint a fresh secrets namespace for a state file that has none. */ +export function newSecretsNamespace(): string { + return randomUUID(); +} /** Field for one issuer's acquired tokens (JSON-serialized `OAuthTokens`). */ export const issuerTokensField = (issuer: string): string => `tokens:${issuer}`; diff --git a/docs/cli-smoke-testing.md b/docs/cli-smoke-testing.md index 62039c82c..4d833a388 100644 --- a/docs/cli-smoke-testing.md +++ b/docs/cli-smoke-testing.md @@ -357,6 +357,12 @@ npx @modelcontextprotocol/inspector --cli --server-url "$SERVER_URL" --list-stor # → {"oauthStatePath":"/tmp/tmp.XXXX/oauth.json","storedServerUrls":[]} ``` +The secret store needs no equivalent isolation: each state file's store +entries are scoped by a namespace stamped into the file itself, so a smoke +run's tokens and the developer's real keychain entries for the same server +URL never share a slot (see [Where secrets are +stored](./secret-storage.md)). + For a server that genuinely needs a credential in CI, prefer a static header over OAuth entirely — `--header 'Authorization: Bearer '`, with the token from your CI secret store. And **do not** put a credential in the URL: the CLI diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 0acc315c1..a8b3f2a60 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -57,7 +57,7 @@ Both are **refused** — ignored with a warning, keeping the bind-derived addres | Variable | Read by | Default | Effect | | -------------------------------- | ------------- | -------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `MCP_STORAGE_DIR` | web, CLI, TUI | `~/.mcp-inspector/storage` | Storage directory. Relocates the OAuth state file (`oauth.json`) and the secrets file (`secrets.json`) for every client. For the **web** backend it also relocates `client.json`; the CLI and TUI find `client.json` through `MCP_CLIENT_CONFIG_PATH` instead. | -| `MCP_INSPECTOR_OAUTH_STATE_PATH` | CLI, TUI | `~/.mcp-inspector/storage/oauth.json` | Names the OAuth state file outright. Lookup order: this variable, then `/oauth.json`, then `~/.mcp-inspector/storage/oauth.json`. ⚠️ Setting `MCP_STORAGE_DIR` alone does not isolate a CLI or TUI run if this variable is also exported. **The web backend does not read it** — it always uses `/oauth.json`. | +| `MCP_INSPECTOR_OAUTH_STATE_PATH` | CLI, TUI | `~/.mcp-inspector/storage/oauth.json` | Names the OAuth state file outright. Lookup order: this variable, then `/oauth.json`, then `~/.mcp-inspector/storage/oauth.json`. ⚠️ Setting `MCP_STORAGE_DIR` alone does not isolate a CLI or TUI run if this variable is also exported. **The web backend does not read it** — it always uses `/oauth.json`. Each state file keeps its own secret-store entries — they are scoped by a namespace stamped into the file — so per-profile state paths stay isolated even on a shared keychain (see [Where secrets are stored](./secret-storage.md)). | | `MCP_CLIENT_CONFIG_PATH` | CLI, TUI | `~/.mcp-inspector/storage/client.json` | Install-level client config (CIMD, enterprise IdP). `--client-config` takes precedence. | ### Home directory diff --git a/docs/secret-storage.md b/docs/secret-storage.md index 6c03505b4..4b371e9df 100644 --- a/docs/secret-storage.md +++ b/docs/secret-storage.md @@ -17,7 +17,7 @@ These values are stored as secrets: They are kept out of `mcp.json` so that sharing, committing or syncing the file does not leak credentials (#1356). When the Inspector saves an entry to a durable store, it leaves each `env` key in `mcp.json` with an empty value and omits the client secret; the real values live in the store. `headers` are **not** moved: they are saved in `mcp.json` exactly as written, so a header that carries a credential stays in the file. [MCP server configuration](./mcp-server-configuration.md) describes what that means for other tools reading the same file. -Acquired tokens follow the same rule for the OAuth state file: `oauth.json` keeps only non-secret state (flow bookkeeping, discovered metadata, public client ids), and the tokens and client secrets it used to hold live in the secret store. A pre-existing `oauth.json` that still carries plaintext tokens is migrated on first read — the tokens move into the store and the file is rewritten without them — but only when the store is durable; under the `memory` store the file is left as-is, since it is still the only durable copy. A token entry so malformed it cannot be a credential (for example, a hand-edited value of the wrong type) is left in the file rather than migrated, so it stays visible and clearable. `MCP_INSPECTOR_PERSIST_TOKENS=all|access|none` controls which acquired tokens are persisted at all (see [Environment variables](./environment-variables.md#secret-store)). +Acquired tokens follow the same rule for the OAuth state file: `oauth.json` keeps only non-secret state (flow bookkeeping, discovered metadata, public client ids), and the tokens and client secrets it used to hold live in the secret store. Each state file's store entries are scoped by a `secretsNamespace` UUID stamped into the file on its first write, so two state files (for example, per-profile `MCP_INSPECTOR_OAUTH_STATE_PATH` values) that authorize against the same server keep separate entries even in a shared store such as the OS keychain. A pre-namespace file is adopted transparently on its first save — its existing un-namespaced entries move under the new namespace, so the adopting file keeps its credentials. That migration deletes its sources, so it runs only under the real cross-process file lock; on a box where the lock cannot be taken, the save fails with a retryable error instead of racing a concurrent adopter — if you cannot make the lock directory writable there, the escape is to clear the file's stored OAuth state and re-authorize, since clearing does not migrate and a fresh state file mints its namespace without the lock. The namespace stamp is also one-way across versions: an Inspector older than the namespace (≤ 2.9.x) resolves only un-namespaced entries, so downgrading after adoption logs you out, and its next save strips the stamp — alternating old and new versions on one state file therefore re-adopts under a fresh namespace each time, leaving the earlier namespace's entries orphaned in the store where nothing will find or clear them. If more than one pre-namespace state file was sharing a server's un-namespaced entry, the first one to adopt takes it with it, and each remaining pre-namespace profile re-authorizes that server once — its copy was already being overwritten by every other profile's saves, which is the bug the namespace fixes. Removing a profile that is still pre-namespace likewise purges the shared un-namespaced entries, as removal always has: the file being deleted is the store's only index of them, so leaving them would strand credentials nothing could find or clear again. A pre-existing `oauth.json` that still carries plaintext tokens is migrated on first read — the tokens move into the store and the file is rewritten without them — but only when the store is durable; under the `memory` store the file is left as-is, since it is still the only durable copy. A token entry so malformed it cannot be a credential (for example, a hand-edited value of the wrong type) is left in the file rather than migrated, so it stays visible and clearable. `MCP_INSPECTOR_PERSIST_TOKENS=all|access|none` controls which acquired tokens are persisted at all (see [Environment variables](./environment-variables.md#secret-store)). ## How the store is chosen