From a8c6d916c4479e84ba693f33e3871242e0a244ed Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Wed, 30 Sep 2026 17:46:29 -0700 Subject: [PATCH 1/8] Scope secret-store entries by a per-state-file namespace Two OAuth state files (profiles) that authorize against the same server previously shared one secret-store entry, so each profile's save silently overwrote the other's tokens (#2549). Each state file now carries a top-level secretsNamespace UUID, minted and stamped on its first write and baked into every secret-store id the file's entries use (oauth++). The namespace charset excludes the '+' delimiter and ':' so ids cannot prefix-collide and the keyring's first-colon account parse is unaffected. A pre-namespace file is adopted transparently on its first save: its legacy un-namespaced entries are copied under the new namespace, the file is stamped (the commit point), and the legacy originals are deleted best-effort. A copy or stamp failure rolls the copies back and leaves the file legacy, so the next write retries. Reads never adopt; removeOAuthStore purges only the file's own ids. Closes #2549 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- clients/cli/__tests__/stored-auth.test.ts | 4 +- .../test/core/auth/oauth-persist-file.test.ts | 7 +- .../src/test/core/auth/oauth-secrets.test.ts | 39 ++ .../test/integration/storage/adapters.test.ts | 26 +- .../storage/oauth-secret-split.test.ts | 72 ++-- .../storage/oauth-secrets-namespace.test.ts | 356 ++++++++++++++++++ .../storage/oauth-write-convergence.test.ts | 67 ++-- core/auth/node/oauth-persist-file.ts | 233 +++++++++++- core/auth/node/oauth-secrets.ts | 53 ++- docs/cli-smoke-testing.md | 6 + docs/environment-variables.md | 2 +- docs/secret-storage.md | 2 +- 12 files changed, 767 insertions(+), 100 deletions(-) create mode 100644 clients/web/src/test/integration/storage/oauth-secrets-namespace.test.ts diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index 0965b093ef..210cac6786 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 eab3ea482d..b7eedecc5c 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 @@ -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 b47ae2d89f..cdbb1539e1 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/storage/adapters.test.ts b/clients/web/src/test/integration/storage/adapters.test.ts index 2cb50c25db..ce51500e31 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"), + ); + 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 24afda57d1..0857a572a1 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-secrets-namespace.test.ts b/clients/web/src/test/integration/storage/oauth-secrets-namespace.test.ts new file mode 100644 index 0000000000..cd0d30f220 --- /dev/null +++ b/clients/web/src/test/integration/storage/oauth-secrets-namespace.test.ts @@ -0,0 +1,356 @@ +/** + * 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, +})); + +import { + writeOAuthSections, + readOAuthStore, + 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 { + 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("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); + }); +}); + +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("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(); + }); +}); 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 22b7985c09..a52c48bc6e 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(() => { @@ -160,11 +166,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 +198,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 +222,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 +236,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 +284,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 +343,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 +364,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 +400,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 +417,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 +449,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 +463,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 +479,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 +493,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/oauth-persist-file.ts b/core/auth/node/oauth-persist-file.ts index 8c707bf257..2bc65034b3 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, @@ -241,6 +251,158 @@ async function readDiskForMutation( return parsed; } +/** + * 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. + */ +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}). The scoped copies are in use; the leftovers are harmless duplicates but 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, so a failure here leaves + * harmless-but-unindexed duplicates and a warning, never a lost token + * ({@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. + */ +async function adoptSecretsNamespace( + filePath: string, + secretStore: SecretStore, +): Promise { + const raw = await readStoreFile(filePath); + const existing = parseSecretsNamespace(raw); + if (existing !== undefined) return existing; + const snapshot = parseOAuthPersistBlob(raw); + if (raw !== null && snapshot === null) { + throw new OAuthStateFileUnrecognizedError(filePath, "save"); + } + 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], + })), + ]; + // 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; + } + try { + for (const { legacyId } of moves) { + await secretStore.deleteAllForServer(legacyId); + } + } catch (error) { + warnNamespaceCleanupFailure(error); + } + return namespace; +} + /** * Persist one entry's secrets: set every post-split value that *differs* * from its snapshotted store value, delete every candidate field the split @@ -390,6 +552,10 @@ export async function writeOAuthSections( const policy = getPersistTokensPolicy(); const durable = await secretStoreIsDurable(secretStore); await withOAuthStateLock(filePath, "save", async () => { + // The namespace scoping every store id below; minted (and legacy + // entries moved) on this file's first namespaced write. Resolved once + // per save, inside the lock: an attempt retry must not re-adopt. + const namespace = await adoptSecretsNamespace(filePath, secretStore); // 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 @@ -569,7 +735,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 +827,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 +890,7 @@ export async function writeOAuthSections( }); } - written = serializeOAuthPersistBlob(merged); + written = serializeOAuthFileBlob(merged, namespace); await writeStoreFile(filePath, written); unconfirmed = { written, writes: attemptWrites }; } catch (error) { @@ -765,17 +931,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 +953,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 +969,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 +1020,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 +1052,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 +1071,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 +1107,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 +1117,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 +1151,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 bf2c001f48..e62e991204 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 62039c82c3..4d833a388a 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 0acc315c16..a8b3f2a60c 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 6c03505b46..8e296f3774 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 nobody is logged out. 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 From 82343a7ee354dd3eda1f02f805b21ef92ee5c1ed Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Wed, 30 Sep 2026 17:57:58 -0700 Subject: [PATCH 2/8] Clarify stale-leftover risk in cleanup warning and adoption docs Review follow-up (#2556): the cleanup-failure warning called the surviving legacy entries "harmless duplicates" while the adoption docblock notes a pre-namespace profile can still read them as stale credentials; say that explicitly in both. Narrow the docs' "nobody is logged out" claim to the adopting file, with the one-time re-auth for other pre-namespace profiles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- core/auth/node/oauth-persist-file.ts | 8 +++++--- docs/secret-storage.md | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/core/auth/node/oauth-persist-file.ts b/core/auth/node/oauth-persist-file.ts index 2bc65034b3..af299390f6 100644 --- a/core/auth/node/oauth-persist-file.ts +++ b/core/auth/node/oauth-persist-file.ts @@ -311,7 +311,7 @@ function warnNamespaceCleanupFailure(error: unknown): void { 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}). The scoped copies are in use; the leftovers are harmless duplicates but will not be cleaned up automatically.`, + `[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.`, ); } @@ -336,8 +336,10 @@ function warnNamespaceCleanupFailure(error: unknown): void { * 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, so a failure here leaves - * harmless-but-unindexed duplicates and a warning, never a lost token + * 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 diff --git a/docs/secret-storage.md b/docs/secret-storage.md index 8e296f3774..a8ad05925e 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. 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 nobody is logged out. 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. 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. 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 From cfd93f57fd788722c73300d9b65b36fbda052f75 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Wed, 30 Sep 2026 21:39:52 -0700 Subject: [PATCH 3/8] Address review round 2: namespace convergence, per-id cleanup, removal test - Make the secrets namespace participate in write convergence: each save attempt re-reads the raw blob and, when a concurrent adopter's different valid namespace is observed on disk (degraded unlocked locking), rolls this call's earlier store writes back to baseline and re-keys to the namespace on disk instead of re-stamping its own mint. - Catch per legacy id in the adoption cleanup loop so one failed purge no longer abandons the remaining best-effort deletions. - Remove readDiskForMutation (its one remaining caller now reads raw). - Tests: namespace-convergence race in oauth-write-convergence.test.ts; removal isolation and per-id cleanup cases in oauth-secrets-namespace.test.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- .../storage/oauth-secrets-namespace.test.ts | 62 ++++++++++++++++++ .../storage/oauth-write-convergence.test.ts | 50 ++++++++++++++ core/auth/node/oauth-persist-file.ts | 65 +++++++++++-------- 3 files changed, 151 insertions(+), 26 deletions(-) diff --git a/clients/web/src/test/integration/storage/oauth-secrets-namespace.test.ts b/clients/web/src/test/integration/storage/oauth-secrets-namespace.test.ts index cd0d30f220..a66c7119cf 100644 --- a/clients/web/src/test/integration/storage/oauth-secrets-namespace.test.ts +++ b/clients/web/src/test/integration/storage/oauth-secrets-namespace.test.ts @@ -53,6 +53,7 @@ vi.mock("@napi-rs/keyring", () => ({ import { writeOAuthSections, readOAuthStore, + removeOAuthStore, resetOAuthSecretStoreWarnings, SECRETS_NAMESPACE_KEY, } from "@inspector/core/auth/node/oauth-persist-file.js"; @@ -180,6 +181,21 @@ describe("secrets namespace isolation (#2549)", () => { 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)", () => { @@ -283,6 +299,52 @@ describe("legacy adoption (#2549)", () => { ).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( 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 a52c48bc6e..6ce15f6879 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 @@ -136,6 +136,56 @@ 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("gives up with a typed, retryable error when the file keeps changing", async () => { hook.afterWrite = (path) => writeFileSync(path, onlyA); diff --git a/core/auth/node/oauth-persist-file.ts b/core/auth/node/oauth-persist-file.ts index af299390f6..f79e6bc500 100644 --- a/core/auth/node/oauth-persist-file.ts +++ b/core/auth/node/oauth-persist-file.ts @@ -234,23 +234,6 @@ 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}). - */ -async function readDiskForMutation( - filePath: string, - action: "save" | "remove", -): Promise { - const raw = await readStoreFile(filePath); - const parsed = parseOAuthPersistBlob(raw); - if (raw !== null && parsed === null) { - throw new OAuthStateFileUnrecognizedError(filePath, action); - } - return parsed; -} - /** * 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 @@ -395,12 +378,14 @@ async function adoptSecretsNamespace( await restoreSecretFields(secretStore, touched, warnRestoreFailure); throw error; } - try { - for (const { legacyId } of moves) { + // 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); } - } catch (error) { - warnNamespaceCleanupFailure(error); } return namespace; } @@ -556,8 +541,12 @@ export async function writeOAuthSections( await withOAuthStateLock(filePath, "save", async () => { // The namespace scoping every store id below; minted (and legacy // entries moved) on this file's first namespaced write. Resolved once - // per save, inside the lock: an attempt retry must not re-adopt. - const namespace = await adoptSecretsNamespace(filePath, secretStore); + // per save, inside the lock: an attempt retry never re-ADOPTS — but it + // may re-KEY. Under degraded (unlocked) locking a concurrent first + // writer can adopt 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); // 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 @@ -704,10 +693,34 @@ 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) { + await restoreToBaseline(); + 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 From 49dcf9973da1d66efc1820d218ce1e7f538db52d Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Wed, 30 Sep 2026 23:34:07 -0700 Subject: [PATCH 4/8] Gate legacy secret-namespace migration on the real file lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 3 (#2556): two adopters of the same legacy state file under degraded locking could race the migration — one copies and deletes the legacy entries while the other strict-reads null, stamps its own namespace, and strands the first adopter's copies. withSecretFileLock now tells its callback whether the lock is actually held, withOAuthStateLock threads that through, and adoptSecretsNamespace refuses a legacy migration (snapshot present, lock not held) with a retryable SecretStoreUnavailableError before touching the file or the store. Locked adopters serialize; mint-only adoption of a fresh file and saves against an already-stamped file stay allowed unlocked, where the write-convergence re-key already handles namespace divergence. Also moves oauth-secrets-namespace.test.ts to integration/auth/node/ to mirror the source path (review round 3, previously-missed finding). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- .../test/core/auth/oauth-persist-file.test.ts | 6 +- .../integration/auth/node/file-lock.test.ts | 16 +- .../auth/node/oauth-adoption-locking.test.ts | 155 ++++++++++++++++++ .../node}/oauth-secrets-namespace.test.ts | 0 core/auth/node/file-lock.ts | 10 +- core/auth/node/oauth-persist-file.ts | 40 +++-- docs/secret-storage.md | 2 +- 7 files changed, 208 insertions(+), 21 deletions(-) create mode 100644 clients/web/src/test/integration/auth/node/oauth-adoption-locking.test.ts rename clients/web/src/test/integration/{storage => auth/node}/oauth-secrets-namespace.test.ts (100%) 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 b7eedecc5c..2f160335e8 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, ); }); 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 5dccf460eb..c74b646c7d 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 0000000000..fd156e1b8f --- /dev/null +++ b/clients/web/src/test/integration/auth/node/oauth-adoption-locking.test.ts @@ -0,0 +1,155 @@ +/** + * 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 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/storage/oauth-secrets-namespace.test.ts b/clients/web/src/test/integration/auth/node/oauth-secrets-namespace.test.ts similarity index 100% rename from clients/web/src/test/integration/storage/oauth-secrets-namespace.test.ts rename to clients/web/src/test/integration/auth/node/oauth-secrets-namespace.test.ts diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index b1eede68b8..ce7a8011b4 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -401,6 +401,10 @@ export async function isFileLockHeld(filePath: string): Promise { * * 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). */ /** * Take the lock and hand back its release, or `null` when locking is @@ -515,12 +519,12 @@ export async function openSecretFileLock( 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 f79e6bc500..0feb675b28 100644 --- a/core/auth/node/oauth-persist-file.ts +++ b/core/auth/node/oauth-persist-file.ts @@ -197,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); @@ -333,10 +333,22 @@ function warnNamespaceCleanupFailure(error: unknown): void { * * 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 + * 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, secretStore: SecretStore, + locked: boolean, ): Promise { const raw = await readStoreFile(filePath); const existing = parseSecretsNamespace(raw); @@ -347,6 +359,11 @@ async function adoptSecretsNamespace( } const namespace = newSecretsNamespace(); if (snapshot === null) 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.`, + ); + } const moves = [ ...Object.entries(snapshot.servers).map(([url, state]) => ({ @@ -538,15 +555,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) on this file's first namespaced write. Resolved once - // per save, inside the lock: an attempt retry never re-ADOPTS — but it - // may re-KEY. Under degraded (unlocked) locking a concurrent first - // writer can adopt 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); + // 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 diff --git a/docs/secret-storage.md b/docs/secret-storage.md index a8ad05925e..4034d98ccb 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. 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. 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. 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 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. 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 From d6da6d15d32dadf7c327bd27d2e75d8dc77b5ad6 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Wed, 30 Sep 2026 23:50:36 -0700 Subject: [PATCH 5/8] Allow mint-only adoption for entry-less legacy files; pin removal semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 4 (#2556): - An entry-less legacy file ({servers:{},idpSessions:{}}) indexes no store ids, so no destructive migration race exists — the degraded-lock gate now sits after the moves are computed and an empty move list mints like a fresh file, so lock-hostile filesystems can still save. - Pin the deliberate legacy-removal semantics with a test and a docs clause: removing a still-pre-namespace profile purges the shared legacy ids, since the file being deleted is the store's only index of them — skipping the purge would strand credentials. - Two-profile isolation now also runs against the real FileSecretStore (nested locking, serialized whole-file mutations). - Cover the commit-point failure: a stamp write that rejects after the scoped copies landed rolls the copies back and leaves the legacy file and ids authoritative. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- .../auth/node/oauth-adoption-locking.test.ts | 20 +++++ .../auth/node/oauth-secrets-namespace.test.ts | 87 +++++++++++++++++++ core/auth/node/oauth-persist-file.ts | 20 +++-- docs/secret-storage.md | 2 +- 4 files changed, 121 insertions(+), 8 deletions(-) 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 index fd156e1b8f..4d15a7d7aa 100644 --- 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 @@ -134,6 +134,26 @@ describe("legacy adoption under a degraded (unlocked) file lock", () => { ).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); 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 index a66c7119cf..bb7452846a 100644 --- 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 @@ -50,6 +50,19 @@ vi.mock("@napi-rs/keyring", () => ({ 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, @@ -62,6 +75,7 @@ import { 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, @@ -165,6 +179,15 @@ describe("secrets namespace isolation (#2549)", () => { ).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); @@ -415,4 +438,68 @@ describe("legacy adoption (#2549)", () => { >; 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/core/auth/node/oauth-persist-file.ts b/core/auth/node/oauth-persist-file.ts index 0feb675b28..d456946fca 100644 --- a/core/auth/node/oauth-persist-file.ts +++ b/core/auth/node/oauth-persist-file.ts @@ -341,8 +341,9 @@ function warnNamespaceCleanupFailure(error: unknown): void { * 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 - * stays allowed unlocked — there is nothing to move, and a concurrent + * 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( @@ -359,11 +360,6 @@ async function adoptSecretsNamespace( } const namespace = newSecretsNamespace(); if (snapshot === null) 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.`, - ); - } const moves = [ ...Object.entries(snapshot.servers).map(([url, state]) => ({ @@ -377,6 +373,16 @@ async function adoptSecretsNamespace( 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.`, + ); + } + // 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". diff --git a/docs/secret-storage.md b/docs/secret-storage.md index 4034d98ccb..d128ccef55 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. 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 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. 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 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 From d211178923ede33f4b58a05fbbdfda83c90a582a Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Thu, 1 Oct 2026 00:03:20 -0700 Subject: [PATCH 6/8] Type the parsed namespace read in adapters.test.ts Review round 5 (#2556): JSON.parse returns any, so the destructured secretsNamespace bypassed type checking before reaching the id builder. Cast to the expected shape like the PR's other namespace reads. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- clients/web/src/test/integration/storage/adapters.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/web/src/test/integration/storage/adapters.test.ts b/clients/web/src/test/integration/storage/adapters.test.ts index ce51500e31..9005f4697e 100644 --- a/clients/web/src/test/integration/storage/adapters.test.ts +++ b/clients/web/src/test/integration/storage/adapters.test.ts @@ -640,7 +640,7 @@ describe("OAuth persistence", () => { // 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(); From 3e4b4ec38fbf66c7966f9830829d569d1632b3c7 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Thu, 1 Oct 2026 00:17:06 -0700 Subject: [PATCH 7/8] Abort the namespace re-key when the baseline restore fails Review round 6 (#2556): the re-key's restoreToBaseline was best-effort, so a failed restore still cleared the baseline, switched namespaces, and could let the save report success with earlier attempts' writes stranded under the abandoned namespace, unindexed by any file. Unlike the failure exits there is no original error to preserve here, so the re-key restore is now strict: the first restore failure aborts the attempt, keeping the baseline for the rethrow's reconciliation, and a retried save converges cleanly. New convergence test refuses the unwind once and asserts the save fails loudly with nothing stranded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- .../storage/oauth-write-convergence.test.ts | 44 +++++++++++++++++++ core/auth/node/oauth-persist-file.ts | 17 ++++++- 2 files changed, 60 insertions(+), 1 deletion(-) 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 6ce15f6879..7d4562b900 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 @@ -186,6 +186,50 @@ describe("writeOAuthSections convergence verification", () => { 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); diff --git a/core/auth/node/oauth-persist-file.ts b/core/auth/node/oauth-persist-file.ts index d456946fca..ce3ef33402 100644 --- a/core/auth/node/oauth-persist-file.ts +++ b/core/auth/node/oauth-persist-file.ts @@ -737,7 +737,22 @@ export async function writeOAuthSections( // writes (all keyed under the abandoned namespace) back to baseline. const diskNamespace = parseSecretsNamespace(rawDisk); if (diskNamespace !== undefined && diskNamespace !== namespace) { - await restoreToBaseline(); + // 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 From 04b14c1773107f2a9cf6ceb3511c64129ae25dc7 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Fri, 2 Oct 2026 00:30:59 -0700 Subject: [PATCH 8/8] Address human review (docs, error-message escape, JSDoc placement) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The degraded-lock adoption refusal now names the escape that works on boxes where the lock directory cannot be made writable: clear the file's stored OAuth state and re-authorize — clearing does not migrate, and a fresh state file mints its namespace unlocked. - docs/secret-storage.md documents that escape and the mixed-version hazard: downgrading after adoption logs you out, and alternating versions on one state file re-adopts under a fresh namespace each time, orphaning the earlier namespace's entries. - file-lock.ts: moved withSecretFileLock's JSDoc (including the new locked-parameter contract) down to the function it documents — it sat above openSecretFileLock's own JSDoc, so it never showed on hover. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- core/auth/node/file-lock.ts | 30 ++++++++++++++-------------- core/auth/node/oauth-persist-file.ts | 2 +- docs/secret-storage.md | 2 +- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index ce7a8011b4..295bb401b9 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -391,21 +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. - * `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). - */ /** * Take the lock and hand back its release, or `null` when locking is * unavailable here and the caller should proceed unprotected. @@ -517,6 +502,21 @@ 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: (locked: boolean) => Promise, diff --git a/core/auth/node/oauth-persist-file.ts b/core/auth/node/oauth-persist-file.ts index ce3ef33402..8a91b4c01a 100644 --- a/core/auth/node/oauth-persist-file.ts +++ b/core/auth/node/oauth-persist-file.ts @@ -379,7 +379,7 @@ async function adoptSecretsNamespace( 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.`, + `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).`, ); } diff --git a/docs/secret-storage.md b/docs/secret-storage.md index d128ccef55..4b371e9df4 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. 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 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)). +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