diff --git a/clients/web/src/test/core/auth/oauth-namespace-ledger.test.ts b/clients/web/src/test/core/auth/oauth-namespace-ledger.test.ts index 0ae22d7c6..cf0150be7 100644 --- a/clients/web/src/test/core/auth/oauth-namespace-ledger.test.ts +++ b/clients/web/src/test/core/auth/oauth-namespace-ledger.test.ts @@ -2,7 +2,8 @@ * Unit tests for the secrets-namespace ledger (#2560): its tolerant parse, * record-only-when-new writes, per-namespace purge bookkeeping, and the * pending carry-over a record an adoption could not read owes its - * successor (#2672), and the best-effort failure handling that keeps a ledger problem from ever + * successor (#2672) — carried only where the store's owner stamp vouches + * for the value (#2734) — and the best-effort failure handling that keeps a ledger problem from ever * failing the OAuth save or removal it rides on. The end-to-end scenarios * (a stripped stamp re-adopted, a stripped file removed) live in the * integration suite's `oauth-secrets-namespace.test.ts`. @@ -65,6 +66,8 @@ class FakeKeyring extends KeyringSecretStore { import { LEGACY_TOKENS_FIELD, IDP_SESSION_FIELD, + clientCredentialStamp, + clientCredentialStampField, issuerClientSecretField, oauthIdpSecretServerId, oauthSecretServerId, @@ -514,9 +517,31 @@ describe("pending carry-over (#2672)", () => { const SECRET = issuerClientSecretField(ISSUER); const owners = (clientId: string) => new Map([[SERVER, new Map([[SECRET, clientId]])]]); + const STAMP = clientCredentialStampField(SECRET); // Another run's store: marking is about the records it could not read. const elsewhere = new KeyringSecretStore(); + /** The keychain, as far as `instanceof` is concerned, over a map. */ + class MapKeyring extends KeyringSecretStore { + readonly inner = new InMemorySecretStore(); + get = this.inner.get.bind(this.inner); + getStrict = this.inner.get.bind(this.inner); + set = this.inner.set.bind(this.inner); + deleteAllForServer = this.inner.deleteAllForServer.bind(this.inner); + } + + /** Store `secret` under `namespace`, stamped as `clientId`'s, as a save would. */ + async function stamped( + target: { set: InMemorySecretStore["set"] }, + namespace: string, + secret: string, + clientId: string, + ) { + const id = oauthSecretServerId(SERVER, namespace); + await target.set(id, SECRET, secret); + await target.set(id, STAMP, clientCredentialStamp(clientId, secret)); + } + /** NS1's record in this run's store, owed to NS2 by a run elsewhere. */ async function owed(clientId = "c", secret: string | null = "s3cret") { await recordNamespaceKeys( @@ -527,9 +552,7 @@ describe("pending carry-over (#2672)", () => { [], owners(clientId), ); - if (secret !== null) { - await store.set(oauthSecretServerId(SERVER, NS1), SECRET, secret); - } + if (secret !== null) await stamped(store, NS1, secret, clientId); await markUncarriedNamespaces(stateFile, elsewhere, NS2); } @@ -675,11 +698,11 @@ describe("pending carry-over (#2672)", () => { }), ); const carries = await pendingCarries(stateFile, store, NS2); - expect(carries.map((c) => [c.namespace, [...c.owners.keys()]])).toEqual([ + expect(carries.map((c) => [c.namespace, [...c.fields.keys()]])).toEqual([ ["44444444-4444-4444-8444-444444444444", []], [NS3, [SERVER]], ]); - expect([...carries[1].owners.get(SERVER)!]).toEqual([["ok", "c"]]); + expect([...carries[1].fields.get(SERVER)!]).toEqual(["ok"]); }); }); @@ -691,6 +714,10 @@ describe("pending carry-over (#2672)", () => { await owed(); await purgeSupersededNamespaces(stateFile, store, NS2, owners("c")); expect(await store.get(to, SECRET)).toBe("s3cret"); + // With its stamp, so it still vouches for the value there. + expect(await store.get(to, STAMP)).toBe( + clientCredentialStamp("c", "s3cret"), + ); expect(await store.get(from, SECRET)).toBeNull(); // The carried key and its owner are recorded under the current // namespace, like every other write the ledger tracks. @@ -729,12 +756,32 @@ describe("pending carry-over (#2672)", () => { [], owners("c"), ); - await store.set(oauthSecretServerId(SERVER, NS3), SECRET, "newer"); + await stamped(store, NS3, "newer", "c"); await markUncarriedNamespaces(stateFile, elsewhere, NS2); await purgeSupersededNamespaces(stateFile, store, NS2, owners("c")); expect(await store.get(to, SECRET)).toBe("newer"); }); + it.each([ + ["no stamp", null], + ["a malformed stamp", "{not json"], + ["a stamp that is not an object", "7"], + ["a stamp naming no client", JSON.stringify({ mac: "x" })], + // The hand-off keeps and copies per account: a kept value can meet a + // copied stamp, and an older Inspector can rewrite a value alone. + ["another value's stamp", clientCredentialStamp("c", "other")], + ])( + "does not carry a value with %s, and still purges", + async (_l, stamp) => { + await owed(); + if (stamp === null) await store.delete(from, STAMP); + else await store.set(from, STAMP, stamp); + await purgeSupersededNamespaces(stateFile, store, NS2, owners("c")); + expect(await store.get(to, SECRET)).toBeNull(); + expect(await store.get(from, SECRET)).toBeNull(); + }, + ); + it.each([ ["a different client now", owners("other")], ["no client for that server now", new Map()], @@ -768,22 +815,35 @@ describe("pending carry-over (#2672)", () => { expect(await store.get(from, SECRET)).toBeNull(); }); - it("keeps the record unpurged, and warns, when the carry fails", async () => { - await owed(); - const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); - vi.spyOn(store, "set").mockRejectedValueOnce( - new Error("keychain locked"), - ); - await purgeSupersededNamespaces(stateFile, store, NS2, owners("c")); - expect(await store.get(from, SECRET)).toBe("s3cret"); - expect(readLedger()[NS1].memory.owedTo).toBe(NS2); - expect(warn.mock.calls[0][0]).toContain("keychain locked"); - - // The next locked save retries it. - await purgeSupersededNamespaces(stateFile, store, NS2, owners("c")); - expect(await store.get(to, SECRET)).toBe("s3cret"); - expect(await store.get(from, SECRET)).toBeNull(); - }); + it.each([ + ["the stamp", 0], + ["the value", 1], + ])( + "keeps the record unpurged, and warns, when writing %s fails, and the retry carries both", + async (_label, failing) => { + await owed(); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const set = store.set.bind(store); + let calls = 0; + vi.spyOn(store, "set").mockImplementation(async (...args) => { + if (calls++ === failing) throw new Error("keychain locked"); + await set(...args); + }); + await purgeSupersededNamespaces(stateFile, store, NS2, owners("c")); + expect(await store.get(from, SECRET)).toBe("s3cret"); + expect(await store.get(to, SECRET)).toBeNull(); + expect(readLedger()[NS1].memory.owedTo).toBe(NS2); + expect(warn.mock.calls[0][0]).toContain("keychain locked"); + + // The next locked save retries it, value and stamp together. + await purgeSupersededNamespaces(stateFile, store, NS2, owners("c")); + expect(await store.get(to, SECRET)).toBe("s3cret"); + expect(await store.get(to, STAMP)).toBe( + clientCredentialStamp("c", "s3cret"), + ); + expect(await store.get(from, SECRET)).toBeNull(); + }, + ); it("ends the pass at a failed carry, so an older record cannot take the newer one's field", async () => { await owed("c", "older"); @@ -795,7 +855,7 @@ describe("pending carry-over (#2672)", () => { [], owners("c"), ); - await store.set(oauthSecretServerId(SERVER, NS3), SECRET, "newer"); + await stamped(store, NS3, "newer", "c"); await markUncarriedNamespaces(stateFile, elsewhere, NS2); const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); vi.spyOn(store, "set").mockRejectedValueOnce(new Error("busy")); @@ -808,14 +868,13 @@ describe("pending carry-over (#2672)", () => { expect(await store.get(to, SECRET)).toBe("newer"); }); - it("in a keychain run, trusts the keychain's owner over a secrets file's", async () => { - // The hand-off keeps a value the keychain already holds, so where - // both record one, the keychain's value is its own record's client's. + /** A `secrets.json` store at `name`, for its ledger location only. */ + const secretsFile = (name = "secrets.json") => + new FileSecretStore({ filePath: join(tempDir, name), passphrase: "" }); + + it("in a keychain run, looks in every field a covered record wrote, keeping the keychain's own owner", async () => { const keyring = new FakeKeyring(); - const fileStore = new FileSecretStore({ - filePath: join(tempDir, "secrets.json"), - passphrase: "", - }); + const fileStore = secretsFile(); await recordNamespaceKeys( stateFile, keyring, @@ -834,9 +893,9 @@ describe("pending carry-over (#2672)", () => { ); await markUncarriedNamespaces(stateFile, store, NS2); const [carry] = await pendingCarries(stateFile, keyring, NS2); - expect([...carry.owners.get(SERVER)!]).toEqual([[SECRET, "a"]]); + expect([...carry.fields.get(SERVER)!]).toEqual([SECRET]); - // And the file run's hand-off of its record keeps the keychain's. + // The file run's hand-off of its record keeps the keychain's owner. vi.spyOn(fileStore, "deleteAllForServer").mockResolvedValue(undefined); await purgeSupersededNamespaces(stateFile, fileStore, NS2, owners("b")); expect(readLedger()[NS1].keyring.owners).toEqual({ @@ -844,56 +903,103 @@ describe("pending carry-over (#2672)", () => { }); }); - it("in a keychain run, carries nothing a secrets file names an owner for when the keychain record's own is unknown", async () => { - // Written to the keychain before owners were kept (or for a client - // with no secret): the hand-off may have kept that value or filled - // the field from the file, and nothing says which. - const keyring = new FakeKeyring(); - const fileStore = new FileSecretStore({ - filePath: join(tempDir, "secrets.json"), - passphrase: "", - }); - await recordNamespaceKeys(stateFile, keyring, NS1, [SERVER], []); + it("in a keychain run, carries a value absorbed from a re-registered secrets file only for the client it was written for (#2734)", async () => { + // A's secret reached the keychain in one hand-off; the file then + // re-registered B, and the next hand-off kept A's value. The file's + // record says B, but the value is A's. + const keyring = new MapKeyring(); await recordNamespaceKeys( stateFile, - fileStore, + secretsFile(), NS1, [SERVER], [], owners("b"), ); + await stamped(keyring, NS1, "a-secret", "a"); await markUncarriedNamespaces(stateFile, store, NS2); - const [carry] = await pendingCarries(stateFile, keyring, NS2); - expect(carry.owners.size).toBe(0); - // The file run's hand-off does not label the keychain's field either. - vi.spyOn(fileStore, "deleteAllForServer").mockResolvedValue(undefined); - await purgeSupersededNamespaces(stateFile, fileStore, NS2, owners("b")); - expect(readLedger()[NS1].keyring.owners).toBeUndefined(); + await purgeSupersededNamespaces(stateFile, keyring, NS2, owners("b")); + expect(await keyring.get(to, SECRET)).toBeNull(); + expect(await keyring.get(from, SECRET)).toBeNull(); }); - it("in a keychain run, carries before purging a covered file record listed ahead of the owed one", async () => { - // An unowed `file:` record ahead of the owed keychain record: its - // purge also deletes the server's ids from the keychain, so the - // carry has to run first. - class MapKeyring extends KeyringSecretStore { - readonly inner = new InMemorySecretStore(); - get = this.inner.get.bind(this.inner); - getStrict = this.inner.get.bind(this.inner); - set = this.inner.set.bind(this.inner); - deleteAllForServer = this.inner.deleteAllForServer.bind(this.inner); - } + it("in a keychain run, still carries for the client the stamp names when a file record says otherwise", async () => { const keyring = new MapKeyring(); await recordNamespaceKeys( stateFile, - new FileSecretStore({ - filePath: join(tempDir, "secrets.json"), - passphrase: "", - }), + secretsFile(), + NS1, + [SERVER], + [], + owners("b"), + ); + await stamped(keyring, NS1, "a-secret", "a"); + await markUncarriedNamespaces(stateFile, store, NS2); + + await purgeSupersededNamespaces(stateFile, keyring, NS2, owners("a")); + expect(await keyring.get(to, SECRET)).toBe("a-secret"); + }); + + it("in a keychain run, carries an absorbed value its own record lists the server for but names no owner of (#2734)", async () => { + // An unrelated keychain save lists every merged server in its record + // but owners only for what it writes; that is no keychain write here. + const keyring = new MapKeyring(); + await recordNamespaceKeys(stateFile, keyring, NS1, [SERVER], []); + await recordNamespaceKeys( + stateFile, + secretsFile(), NS1, [SERVER], [], + owners("c"), ); + await stamped(keyring, NS1, "s3cret", "c"); + await markUncarriedNamespaces(stateFile, store, NS2); + + await purgeSupersededNamespaces(stateFile, keyring, NS2, owners("c")); + expect(await keyring.get(to, SECRET)).toBe("s3cret"); + }); + + it.each([ + ["the client whose file was absorbed", "a", "a-secret"], + ["the other file's client", "b", null], + ])( + "in a keychain run with two secrets files, carries only for %s (#2734)", + async (_label, now, carried) => { + // F1 holds A's secret and F2 B's under one namespace; the keychain + // absorbed F1's. Records are merged in no order that says which. + const keyring = new MapKeyring(); + await recordNamespaceKeys( + stateFile, + secretsFile("f1.json"), + NS1, + [SERVER], + [], + owners("a"), + ); + await recordNamespaceKeys( + stateFile, + secretsFile("f2.json"), + NS1, + [SERVER], + [], + owners("b"), + ); + await stamped(keyring, NS1, "a-secret", "a"); + await markUncarriedNamespaces(stateFile, store, NS2); + + await purgeSupersededNamespaces(stateFile, keyring, NS2, owners(now)); + expect(await keyring.get(to, SECRET)).toBe(carried); + }, + ); + + it("in a keychain run, carries before purging a covered file record listed ahead of the owed one", async () => { + // An unowed `file:` record ahead of the owed keychain record: its + // purge also deletes the server's ids from the keychain, so the + // carry has to run first. + const keyring = new MapKeyring(); + await recordNamespaceKeys(stateFile, secretsFile(), NS1, [SERVER], []); await recordNamespaceKeys( stateFile, keyring, @@ -902,7 +1008,7 @@ describe("pending carry-over (#2672)", () => { [], owners("c"), ); - await keyring.set(from, SECRET, "s3cret"); + await stamped(keyring, NS1, "s3cret", "c"); await markUncarriedNamespaces(stateFile, store, NS2); await purgeSupersededNamespaces(stateFile, keyring, NS2, owners("c")); @@ -913,10 +1019,7 @@ describe("pending carry-over (#2672)", () => { it("hands a secrets file's owed record to the keychain, mark and owners included", async () => { // Its entries may have been absorbed into the keychain before this // file run, which then finds nothing to carry. - const fileStore = new FileSecretStore({ - filePath: join(tempDir, "secrets.json"), - passphrase: "", - }); + const fileStore = secretsFile(); await recordNamespaceKeys( stateFile, fileStore, 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 cdbb1539e..ef629162a 100644 --- a/clients/web/src/test/core/auth/oauth-secrets.test.ts +++ b/clients/web/src/test/core/auth/oauth-secrets.test.ts @@ -27,6 +27,10 @@ import { joinIdpSession, serverSecretFields, snapshotHasPlaintextSecrets, + clientCredentialStamp, + clientCredentialStampField, + restampClientCredentials, + stampedOwner, } from "@inspector/core/auth/node/oauth-secrets.js"; import type { ServerOAuthState } from "@inspector/core/auth/store.js"; import type { OAuthTokens } from "@modelcontextprotocol/client"; @@ -396,20 +400,133 @@ describe("serverSecretFields", () => { [ LEGACY_TOKENS_FIELD, LEGACY_CLIENT_SECRET_FIELD, + clientCredentialStampField(LEGACY_CLIENT_SECRET_FIELD), LEGACY_REGISTRATION_TOKEN_FIELD, + clientCredentialStampField(LEGACY_REGISTRATION_TOKEN_FIELD), PREREG_CLIENT_SECRET_FIELD, PREREG_REGISTRATION_TOKEN_FIELD, ].sort(), ); const issuer = "https://as.example"; - expect(serverSecretFields({ byIssuer: { [issuer]: {} } })).toContain( + const fields = serverSecretFields({ byIssuer: { [issuer]: {} } }); + for (const field of [ issuerTokensField(issuer), - ); - expect(serverSecretFields({ byIssuer: { [issuer]: {} } })).toContain( issuerClientSecretField(issuer), - ); - expect(serverSecretFields({ byIssuer: { [issuer]: {} } })).toContain( + clientCredentialStampField(issuerClientSecretField(issuer)), issuerRegistrationTokenField(issuer), + clientCredentialStampField(issuerRegistrationTokenField(issuer)), + ]) { + expect(fields).toContain(field); + } + }); +}); + +describe("client-credential owner stamps (#2734)", () => { + const issuer = "https://as.example"; + const SECRET = issuerClientSecretField(issuer); + const STAMP = clientCredentialStampField(SECRET); + const dcr = (clientId: string, secret?: string): ServerOAuthState => ({ + byIssuer: { + [issuer]: { + clientInformation: { + client_id: clientId, + ...(secret !== undefined && { client_secret: secret }), + }, + }, + }, + }); + + it("stamps each DCR client credential with its client, and never a pre-registered one", () => { + const { secrets } = splitServerOAuthState( + { + ...dcr("cid", "ics"), + clientInformation: { + client_id: "legacy", + client_secret: "cs", + registration_access_token: "rat", + } as ServerOAuthState["clientInformation"], + preregisteredClientInformation: { + client_id: "pre", + client_secret: "pre-cs", + }, + }, + "all", + ); + expect(stampedOwner(secrets[STAMP], "ics")).toBe("cid"); + expect( + stampedOwner( + secrets[clientCredentialStampField(LEGACY_CLIENT_SECRET_FIELD)], + "cs", + ), + ).toBe("legacy"); + expect( + stampedOwner( + secrets[clientCredentialStampField(LEGACY_REGISTRATION_TOKEN_FIELD)], + "rat", + ), + ).toBe("legacy"); + expect( + secrets[clientCredentialStampField(PREREG_CLIENT_SECRET_FIELD)], + ).toBeUndefined(); + // The join serves the values and ignores the stamps. + expect( + joinServerOAuthState( + { byIssuer: { [issuer]: { clientInformation: { client_id: "cid" } } } }, + secrets, + ).byIssuer![issuer]!.clientInformation, + ).toEqual({ client_id: "cid", client_secret: "ics" }); + }); + + it("stamps nothing for a client with no client_id", () => { + const { secrets } = splitServerOAuthState( + { + clientInformation: { + client_secret: "cs", + } as ServerOAuthState["clientInformation"], + }, + "all", + ); + expect(secrets).toEqual({ [LEGACY_CLIENT_SECRET_FIELD]: "cs" }); + }); + + it.each([ + ["no stamp", undefined], + ["a malformed stamp", "{"], + ["a non-object stamp", "null"], + ["a non-string client_id", JSON.stringify({ client_id: 7, mac: "" })], + ["another value's stamp", clientCredentialStamp("cid", "other")], + [ + "a stamp whose client was swapped", + JSON.stringify({ + ...(JSON.parse(clientCredentialStamp("cid", "ics")) as object), + client_id: "other", + }), + ], + ])("names no owner for %s", (_label, stamp) => { + expect(stampedOwner(stamp, "ics")).toBeUndefined(); + }); + + it("restamps only what no stamp vouches for, keeping a valid one whatever client it names", () => { + const valid = clientCredentialStamp("earlier", "ics"); + expect( + restampClientCredentials(dcr("cid"), { [SECRET]: "ics", [STAMP]: valid }), + ).toEqual({ [SECRET]: "ics", [STAMP]: valid }); + expect( + restampClientCredentials(dcr("cid"), { + [SECRET]: "ics", + [STAMP]: clientCredentialStamp("cid", "other"), + }), + ).toEqual({ + [SECRET]: "ics", + [STAMP]: clientCredentialStamp("cid", "ics"), + }); + expect(restampClientCredentials(dcr("cid"), { [SECRET]: "ics" })).toEqual({ + [SECRET]: "ics", + [STAMP]: clientCredentialStamp("cid", "ics"), + }); + // A stamp with no value beside it vouches for nothing. + expect(restampClientCredentials(dcr("cid"), { [STAMP]: valid })).toEqual( + {}, ); }); }); 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 45af9ce5b..505321bd5 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 @@ -94,6 +94,8 @@ import { IDP_SESSION_FIELD, issuerClientSecretField, resetPersistTokensPolicyWarnings, + clientCredentialStampField, + stampedOwner, } from "@inspector/core/auth/node/oauth-secrets.js"; import { writeStoreFile, @@ -1303,6 +1305,161 @@ describe("a stripped stamp re-adopted on another backend keeps what only the old ); }); + /** + * The issue's (#2734) steps 1–4 and the strip of step 5: client A saved + * to `secrets.json` and absorbed, client B registered in the file under + * the same stamp, absorbed again — the keychain keeps A's value — then + * the stamp stripped and the file re-adopted on a memory backend. + */ + async function repeatedHandOff( + keyring: SecretStore, + reregister: boolean, + ): Promise<{ ns1: string; ns2: string }> { + const secretsFile = join(tempDir, "secrets.json"); + const fileStore = () => + new FileSecretStore({ filePath: secretsFile, passphrase: "" }); + const savedFileEnv = process.env[SECRET_FILE_ENV]; + process.env[SECRET_FILE_ENV] = secretsFile; + vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + await writeOAuthSections( + fileA, + confidentialClient("client-a", "secret-a"), + undefined, + fileStore(), + ); + await flushStoreFileWrites(fileA); + await absorbFileSecretsIntoKeyring(keyring); + await writeOAuthSections( + fileA, + reregister + ? confidentialClient("client-b", "secret-b") + : { servers: { [OTHER]: { scope: "x" } }, idpSessions: {} }, + { servers: [reregister ? SERVER : OTHER] }, + fileStore(), + ); + await flushStoreFileWrites(fileA); + await absorbFileSecretsIntoKeyring(keyring); + expect(existsSync(secretsFile)).toBe(false); + } finally { + if (savedFileEnv === undefined) delete process.env[SECRET_FILE_ENV]; + else process.env[SECRET_FILE_ENV] = savedFileEnv; + } + const ns1 = namespaceOf(fileA); + await stripStamp(fileA); + await writeOAuthSections( + fileA, + { servers: { [OTHER]: { scope: "y" } }, idpSessions: {} }, + { servers: [OTHER] }, + new InMemorySecretStore(), + ); + await flushStoreFileWrites(fileA); + return { ns1, ns2: namespaceOf(fileA) }; + } + + it("does not pair a secret a repeated hand-off kept with the client the file registered since (#2734)", async () => { + const keyring = new KeyringSecretStore(); + const { ns1, ns2 } = await repeatedHandOff(keyring, true); + // The keychain kept A's value; the file, and its record, say B. + expect( + await keyring.get(oauthSecretServerId(SERVER, ns1), SECRET_FIELD), + ).toBe("secret-a"); + expect(clientOf(await readOAuthStore(fileA, keyring))).toEqual({ + client_id: "client-b", + }); + + await writeOAuthSections( + fileA, + { servers: { [OTHER]: { scope: "z" } }, idpSessions: {} }, + { servers: [OTHER] }, + keyring, + ); + await flushStoreFileWrites(fileA); + expect( + await keyring.get(oauthSecretServerId(SERVER, ns2), SECRET_FIELD), + ).toBeNull(); + expect( + await keyring.get(oauthSecretServerId(SERVER, ns1), SECRET_FIELD), + ).toBeNull(); + }); + + it("still carries a secret a repeated hand-off kept for the client it was written for (#2734)", async () => { + const keyring = new KeyringSecretStore(); + const { ns1, ns2 } = await repeatedHandOff(keyring, false); + expect(clientOf(await readOAuthStore(fileA, keyring))).toEqual({ + client_id: "client-a", + client_secret: "secret-a", + }); + + await writeOAuthSections( + fileA, + { servers: { [OTHER]: { scope: "z" } }, idpSessions: {} }, + { servers: [OTHER] }, + keyring, + ); + await flushStoreFileWrites(fileA); + expect( + await keyring.get(oauthSecretServerId(SERVER, ns2), SECRET_FIELD), + ).toBe("secret-a"); + expect( + await keyring.get(oauthSecretServerId(SERVER, ns1), SECRET_FIELD), + ).toBeNull(); + }); + + it("stamps a legacy secret adoption moves, so a later re-adoption elsewhere can still carry it (#2734)", async () => { + // A pre-#2549 file: no stamp, and its client secret at the shared + // legacy id with no owner stamp beside it. + const keyring = new KeyringSecretStore(); + await writeStoreFile( + fileA, + JSON.stringify({ + servers: { + [SERVER]: { + activeIssuer: ISSUER, + byIssuer: { + [ISSUER]: { + clientInformation: { client_id: "dcr-client" }, + clientRegistrationKind: "dcr", + }, + }, + }, + }, + idpSessions: {}, + }), + ); + await flushStoreFileWrites(fileA); + await keyring.set(oauthSecretServerId(SERVER), SECRET_FIELD, "s3cret"); + await writeOAuthSections( + fileA, + { servers: { [OTHER]: { scope: "x" } }, idpSessions: {} }, + { servers: [OTHER] }, + keyring, + ); + await flushStoreFileWrites(fileA); + const ns1 = namespaceOf(fileA); + expect( + stampedOwner( + (await keyring.get( + oauthSecretServerId(SERVER, ns1), + clientCredentialStampField(SECRET_FIELD), + )) ?? undefined, + "s3cret", + ), + ).toBe("dcr-client"); + + await stripStamp(fileA); + await writeOAuthSections( + fileA, + { servers: { [OTHER]: { scope: "y" } }, idpSessions: {} }, + { servers: [OTHER] }, + new InMemorySecretStore(), + ); + await flushStoreFileWrites(fileA); + expect(clientOf(await readOAuthStore(fileA, keyring))?.client_secret).toBe( + "s3cret", + ); + }); + it("does not resurrect the tokens, only the client's credentials", async () => { const keyring = new KeyringSecretStore(); const { ns2 } = await readoptElsewhere(keyring, new InMemorySecretStore()); diff --git a/core/auth/node/oauth-namespace-ledger.ts b/core/auth/node/oauth-namespace-ledger.ts index 1d92b2f23..cfa6c3383 100644 --- a/core/auth/node/oauth-namespace-ledger.ts +++ b/core/auth/node/oauth-namespace-ledger.ts @@ -38,17 +38,25 @@ * the successor's stamp means no read falls back to it again. Purging it * from its own location later would delete what may be the only copy of a * confidential client's secret. So every record also notes, as each save - * writes it, the `client_id` each client-credential field belongs to, and - * adoption marks every record it could not read as owed to the namespace - * that superseded it. A run in that location then reads, and finally - * copies, those fields into the successor before the purge — but only - * where the state file still holds a client with that same `client_id`. A - * field the successor lacks is otherwise indistinguishable from one - * deliberately cleared since, and a different `client_id` means the secret - * would be paired with the wrong client. The owner is the one recorded at - * write time, never inferred from the file being adopted: an older - * Inspector may have re-registered in between. A record written before - * owners were recorded has none, so nothing is carried from it. + * writes it, which client-credential fields it wrote, and adoption marks + * every record it could not read as owed to the namespace that superseded + * it. A run in that location then reads, and finally copies, those fields + * into the successor before the purge — but only where the state file + * still holds the client the value was written for. A field the successor + * lacks is otherwise indistinguishable from one deliberately cleared + * since, and a different client means the secret would be paired with the + * wrong one. + * + * Which client a value was written for is read from the owner stamp stored + * beside it (#2734, `clientCredentialStampField`), never from the ledger. + * The ledger records each field's `client_id` too, but only as the writer + * saw it in its own location, and a value reaches the keychain without the + * ledger seeing it: a file-to-keychain hand-off copies a file's value only + * where the keychain has none, keeps an older one otherwise, and may be + * absorbing any of several `secrets.json` files. No combination of + * per-location records says which value it kept. The stamp moves with the + * value instead, so a value with no stamp that vouches for it — written + * before stamps existed, or separated from its stamp — is not carried. * * Everything else here is best-effort and never throws: the ledger is a cleanup * aid, not part of the credential path, so a failure warns once and the @@ -73,9 +81,11 @@ import { import { FileSecretStore } from "./file-secret-store.js"; import { resolveConcreteSecretStore } from "./secret-store-selection.js"; import { + clientCredentialStampField, isValidSecretsNamespace, oauthIdpSecretServerId, oauthSecretServerId, + stampedOwner, } from "./oauth-secrets.js"; /** The ledger's path, derived from the state file it belongs to. */ @@ -139,10 +149,15 @@ function coveredLocations( */ export type ClientCredentialOwners = Map>; +/** Per server URL, the client-credential fields a carry may copy (#2672). */ +export type CarryFields = Map>; + /** * The keys one namespace has had store entries written under, the client - * each client credential written there belongs to, and — for a record an - * adoption could not read (#2672) — the namespace it is owed to. + * each client credential written there was written for (which fields a + * carry should look in — the owner itself is read from the store's stamp, + * see the header), and — for a record an adoption could not read (#2672) — + * the namespace it is owed to. */ interface LedgerKeys { servers: Set; @@ -158,43 +173,40 @@ const emptyKeys = (): LedgerKeys => ({ }); /** - * Fold `other`'s owners into `own`'s, for a server `own` has never been - * written for only (#2672). Where `own` has, the value its store holds - * for a field may be its own write's or one a hand-off filled in, and its - * owners alone say which: an owner names the client of a value it wrote, - * and a missing one — a vacant field, or a record from before owners were - * kept — is ambiguous, so nothing is carried rather than guessed. + * The client-credential fields a run in `location` should look in for one + * namespace (#2672): every field any record it covers wrote, its own and a + * covered secrets file's alike, since a hand-off may have brought either + * here. Which of them holds a value worth carrying, and for which client, + * is the store's stamp to say (see the header), so a field listed here + * that was never written costs one empty read. */ -function foldOwners( - own: LedgerKeys | undefined, - other: ClientCredentialOwners, -): ClientCredentialOwners { - const owners: ClientCredentialOwners = new Map(); - for (const [url, fields] of other) { - if (!own?.servers.has(url)) owners.set(url, new Map(fields)); +function candidateFields( + location: string, + byLocation: Map, +): CarryFields { + const fields: CarryFields = new Map(); + for (const { keys } of coveredLocations(location, byLocation)) { + for (const [url, owned] of keys.owners) { + const kept = fields.get(url) ?? new Set(); + for (const field of owned.keys()) kept.add(field); + fields.set(url, kept); + } } - mergeOwners(owners, own?.owners ?? new Map()); - return owners; + return fields; } -/** - * The owners a run in `location` should trust for one namespace's - * credentials (#2672): its own record's, plus a covered secrets file's - * for servers its own record was never written for — a value the - * file-to-keychain hand-off can only have brought from that file. - */ -function effectiveOwners( - location: string, - byLocation: Map, -): ClientCredentialOwners { - const fromFiles: ClientCredentialOwners = new Map(); - for (const { location: recorded, keys } of coveredLocations( - location, - byLocation, - )) { - if (recorded !== location) mergeOwners(fromFiles, keys.owners); +/** Add to `target` each of `source`'s owners for a field it does not name. */ +function addMissingOwners( + target: ClientCredentialOwners, + source: ClientCredentialOwners, +): void { + for (const [url, fields] of source) { + const kept = target.get(url) ?? new Map(); + for (const [field, clientId] of fields) { + if (!kept.has(field)) kept.set(field, clientId); + } + if (kept.size > 0) target.set(url, kept); } - return foldOwners(byLocation.get(location), fromFiles); } /** Merge `source` into `target`, the later owner winning; reports a change. */ @@ -422,11 +434,10 @@ export async function markUncarriedNamespaces( /** * The records a run in `secretStore`'s location still owes `current` a * carry-over from (#2672), newest first: each one's namespace and, per - * server, the `client_id` each client credential it holds belongs to. A - * stamped file's reads fall back to these — for those fields only, and - * only where the file still holds a client with that `client_id` — so the - * state a save starts from does not drop a secret the purge is about to - * carry. + * server, the client-credential fields it may hold. A stamped file's reads + * fall back to these — for those fields only, and only where the value's + * stamp names a client the file still holds — so the state a save starts + * from does not drop a secret the purge is about to carry. * * Throws on an unreadable ledger, like the strict store reads beside it: * reading as empty would present an owed secret as absent, and the save @@ -436,13 +447,12 @@ export async function pendingCarries( stateFilePath: string, secretStore: SecretStore, current: string, -): Promise> { +): Promise> { const ledger = parseLedger( await readStoreFile(namespaceLedgerPath(stateFilePath)), ); const location = await secretStoreLocation(secretStore); - const carries: Array<{ namespace: string; owners: ClientCredentialOwners }> = - []; + const carries: Array<{ namespace: string; fields: CarryFields }> = []; for (const [namespace, byLocation] of ledger) { const owed = coveredLocations(location, byLocation).some( ({ keys }) => keys.owedTo === current, @@ -450,7 +460,7 @@ export async function pendingCarries( if (owed) { carries.push({ namespace, - owners: effectiveOwners(location, byLocation), + fields: candidateFields(location, byLocation), }); } } @@ -460,8 +470,9 @@ export async function pendingCarries( /** * Copy what an owed record holds into `current` before it is purged * (#2672): each client credential it recorded, into the same server's id - * under `current`, only where `current` holds nothing for it and the state - * file (`now`) still holds a client with the `client_id` it belongs to. + * under `current`, only where `current` holds nothing for it and the + * value's stamp names the client the state file (`now`) holds for that + * field (#2734) — the stamp is copied with it, so it still vouches there. * Records the key and its owner under `current` first, like every other * write the ledger tracks. Returns whether every carry succeeded; a * failure leaves the record for the next locked save. @@ -473,18 +484,24 @@ async function carryOwed( secretStore: SecretStore, namespace: string, current: string, - recorded: ClientCredentialOwners, + recorded: CarryFields, now: ClientCredentialOwners, ): Promise { let carried = true; for (const [url, fields] of recorded) { - for (const [field, clientId] of fields) { - if (now.get(url)?.get(field) !== clientId) continue; + for (const field of fields) { + const clientId = now.get(url)?.get(field); + if (clientId === undefined) continue; try { const from = oauthSecretServerId(url, namespace); const to = oauthSecretServerId(url, current); + const stampField = clientCredentialStampField(field); const value = await secretStoreGetStrict(secretStore, from, field); if (value === null) continue; + const stamp = await secretStoreGetStrict(secretStore, from, stampField); + if (stamp === null || stampedOwner(stamp, value) !== clientId) { + continue; + } if ((await secretStoreGetStrict(secretStore, to, field)) !== null) { continue; } @@ -501,6 +518,11 @@ async function carryOwed( if (mergeOwners(target.owners, owner) || isNew) { await writeLedger(stateFilePath, ledger); } + // Stamp first: a stamp left alone by a failed value write vouches + // for nothing, and the retry finds the field vacant and redoes + // both. The other order could leave an unstamped value the retry + // skips as already held, and no later carry would trust. + await secretStore.set(to, stampField, stamp); await secretStore.set(to, field, value); } catch (error) { carried = false; @@ -604,7 +626,7 @@ export async function purgeSupersededNamespaces( secretStore, namespace, current, - effectiveOwners(location, byLocation), + candidateFields(location, byLocation), owners, )) ) { @@ -637,14 +659,15 @@ export async function purgeSupersededNamespaces( if (!purged || recorded !== location) continue; byLocation.delete(recorded); // A file's entries may also have been copied into the keychain by a - // hand-off this ledger never saw; keep tracking them there — owners - // and any carry still owed included, since a copy absorbed before - // this run is one this run's carry could not find (#2672). + // hand-off this ledger never saw; keep tracking them there — the + // fields a carry should look in and any carry still owed included, + // since a copy absorbed before this run is one this run's carry could + // not find (#2672). Where both name a field the keychain's owner is + // kept: neither decides a carry (the stamp does), but it is the one + // its own location wrote. if (recorded.startsWith(FILE_LOCATION_PREFIX)) { const keyring = byLocation.get(KEYRING_LOCATION) ?? emptyKeys(); - // Before the servers merge, which would make every one look like - // the keychain's own. - keyring.owners = foldOwners(keyring, keys.owners); + addMissingOwners(keyring.owners, keys.owners); for (const url of keys.servers) keyring.servers.add(url); for (const issuer of keys.idpSessions) keyring.idpSessions.add(issuer); keyring.owedTo ??= keys.owedTo; diff --git a/core/auth/node/oauth-persist-file.ts b/core/auth/node/oauth-persist-file.ts index 090261a0d..032487ef1 100644 --- a/core/auth/node/oauth-persist-file.ts +++ b/core/auth/node/oauth-persist-file.ts @@ -84,6 +84,7 @@ import { MAX_WRITE_ATTEMPTS } from "./file-secret-store.js"; import { IDP_SESSION_FIELD, clientCredentialOwners, + clientCredentialStampField, getPersistTokensPolicy, isUsableStoredSecret, isValidSecretsNamespace, @@ -92,10 +93,12 @@ import { newSecretsNamespace, oauthIdpSecretServerId, oauthSecretServerId, + restampClientCredentials, serverSecretFields, snapshotHasPlaintextSecrets, splitIdpSession, splitServerOAuthState, + stampedOwner, type OAuthSecretValues, } from "./oauth-secrets.js"; @@ -468,9 +471,14 @@ async function adoptSecretsNamespace( const touched: SecretFieldSnapshot[] = []; try { const copies = [ + // Restamped: a value read from the legacy ids predates stamps, and a + // fallback's stamp may belong to a value read from elsewhere (#2734). ...[...values.servers].map(([url, secrets]) => ({ scopedId: oauthSecretServerId(url, namespace), - secrets, + secrets: restampClientCredentials( + getOwnEntry(snapshot.servers, url) ?? {}, + secrets, + ), })), ...[...values.idpSessions].map(([issuer, secrets]) => ({ scopedId: oauthIdpSecretServerId(issuer, namespace), @@ -1298,8 +1306,10 @@ type PendingCarry = Awaited>[number]; * * `carries` is non-empty only for a stamped file with records in this * store an adoption elsewhere could not read (#2672, `pendingCarries`): - * each supplies only the client-credential fields it recorded an owner - * for, and only for a client the file still holds under that `client_id`. + * each supplies only the client-credential fields it recorded, and only + * where the value's owner stamp (#2734) names the client the file holds + * for that field now — the stamp, not the ledger, because a hand-off can + * have kept or copied a value no ledger record describes. * * Strict: this read hydrates the memory state that later sectioned writes * diff against, so a tolerant read during a store outage would present @@ -1335,29 +1345,42 @@ async function readEntrySecrets( } } const owners = snapshotClientOwners(snapshot); - for (const { namespace: from, owners: recorded } of carries) { + for (const { namespace: from, fields: recorded } of carries) { const requests: SecretBulkRequest[] = []; for (const [url, fields] of recorded) { const target = servers.get(url); if (!target) continue; - const wanted = [...fields] - .filter( - ([field, clientId]) => - owners.get(url)?.get(field) === clientId && - !Object.hasOwn(target, field), - ) - .map(([field]) => field); + const wanted = [...fields].filter( + (field) => + owners.get(url)?.has(field) === true && !Object.hasOwn(target, field), + ); if (wanted.length > 0) { requests.push({ serverId: oauthSecretServerId(url, from), - fields: wanted, + fields: wanted.flatMap((field) => [ + field, + clientCredentialStampField(field), + ]), }); } } if (requests.length === 0) continue; const values = await secretStoreGetManyStrict(secretStore, requests); for (const [url, target] of servers) { - fillMissingSecrets(target, values[oauthSecretServerId(url, from)]); + const found = values[oauthSecretServerId(url, from)] ?? {}; + for (const [field, clientId] of owners.get(url) ?? []) { + const value = Object.hasOwn(found, field) ? found[field] : undefined; + const stampField = clientCredentialStampField(field); + if ( + value === undefined || + Object.hasOwn(target, field) || + stampedOwner(found[stampField], value) !== clientId + ) { + continue; + } + setOwnEntry(target, field, value); + setOwnEntry(target, stampField, found[stampField]); + } } } return { servers, idpSessions }; diff --git a/core/auth/node/oauth-secrets.ts b/core/auth/node/oauth-secrets.ts index f77a154b5..b728ca155 100644 --- a/core/auth/node/oauth-secrets.ts +++ b/core/auth/node/oauth-secrets.ts @@ -20,7 +20,7 @@ * route); the browser round-trips full snapshots over the authed local API. */ -import { randomUUID } from "node:crypto"; +import { createHmac, randomUUID } from "node:crypto"; import { OAuthTokensSchema } from "@modelcontextprotocol/core"; import type { OAuthTokens } from "@modelcontextprotocol/client"; import { setOwnEntry } from "../../storage/own-entry.js"; @@ -144,6 +144,72 @@ export const PREREG_REGISTRATION_TOKEN_FIELD = "prereg-registration-token"; /** One IdP issuer's session tokens (JSON `{ idToken?, refreshToken? }`). */ export const IDP_SESSION_FIELD = "idp-session"; +/** + * Field for the owner stamp stored beside a DCR client credential (#2734): + * which `client_id` the value in `field` was written for. The store does + * not otherwise record that pairing, and a carry-over from a superseded + * namespace needs it — a secret paired with the wrong client is useless. + * + * It lives in the store, beside the value, rather than in the namespace + * ledger because the value moves without the ledger seeing it: the + * file-to-keychain hand-off copies each account it finds unless the + * keychain already holds one, and knows nothing of state files. A stamp in + * the store is copied or kept exactly as its value is, so it names the + * client of the value actually there, however many hand-offs ran. + */ +export const clientCredentialStampField = (field: string): string => + `${field}#owner`; + +/** + * A MAC over `clientId` keyed by the credential itself: only a writer + * holding `value` can produce it, and it matches only that value and that + * client. Keyed rather than a plain digest of the value, so the credential + * is never the hashed input — a fast unkeyed hash of a secret is the shape + * of an offline-guessable password hash, even though this one sits in the + * same store as the value and so reveals nothing that store does not. + */ +const credentialMac = (clientId: string, value: string): string => + createHmac("sha256", value).update(clientId).digest("hex"); + +/** + * The stamp naming `clientId` as the owner of `value`. It carries a MAC + * bound to the value so a stamp separated from its value — the hand-off + * keeps and copies per account, so a kept value can meet a copied stamp, + * and an older Inspector can rewrite a value without its stamp — never + * vouches for another value. + */ +export function clientCredentialStamp(clientId: string, value: string): string { + return JSON.stringify({ + client_id: clientId, + mac: credentialMac(clientId, value), + }); +} + +/** + * The `client_id` a stamp names as `value`'s owner, or `undefined` when + * there is no stamp, it is malformed, or it was written for a different + * value — all of which make the owner unknown, and an unknown owner is + * never guessed. + */ +export function stampedOwner( + stamp: string | undefined, + value: string, +): string | undefined { + if (stamp === undefined) return undefined; + let parsed: unknown; + try { + parsed = JSON.parse(stamp); + } catch { + return undefined; + } + if (typeof parsed !== "object" || parsed === null) return undefined; + const { client_id: clientId, mac } = parsed as Record; + if (typeof clientId !== "string" || mac !== credentialMac(clientId, value)) { + return undefined; + } + return clientId; +} + /** Secret values extracted from one entry, keyed by store field. */ export type OAuthSecretValues = Record; @@ -239,14 +305,20 @@ function splitTokens( * `StoredOAuthClientInformation` in `store.ts`). */ type ClientInfoSecretKeys = { + client_id?: unknown; client_secret?: string; registration_access_token?: string; }; -/** The pair of store fields one `clientInformation` slot splits into. */ +/** + * The pair of store fields one `clientInformation` slot splits into, and + * whether each value is stamped with its owner (DCR slots only — see + * {@link clientCredentialOwners} for why pre-registered ones are not). + */ interface ClientInfoFields { clientSecret: string; registrationToken: string; + stamped: boolean; } /** @@ -254,7 +326,8 @@ interface ClientInfoFields { * `secrets`, returning the public residue. Both keys are handled in one * place so a slot with a `registration_access_token` but no `client_secret` * still splits — key-by-key extraction is how the token used to slip - * through to plaintext. + * through to plaintext. A DCR slot's values are stamped with the client + * they belong to (#2734). */ function splitClientInformation( info: T, @@ -265,12 +338,18 @@ function splitClientInformation( if (client_secret === undefined && registration_access_token === undefined) { return info; } - if (client_secret !== undefined) { - secrets[fields.clientSecret] = client_secret; - } - if (registration_access_token !== undefined) { - secrets[fields.registrationToken] = registration_access_token; - } + const put = (field: string, value: string | undefined): void => { + if (value === undefined) return; + secrets[field] = value; + if (fields.stamped && typeof info.client_id === "string") { + secrets[clientCredentialStampField(field)] = clientCredentialStamp( + info.client_id, + value, + ); + } + }; + put(fields.clientSecret, client_secret); + put(fields.registrationToken, registration_access_token); return publicInfo as T; } @@ -301,14 +380,17 @@ function joinClientInformation( const issuerClientInfoFields = (issuer: string): ClientInfoFields => ({ clientSecret: issuerClientSecretField(issuer), registrationToken: issuerRegistrationTokenField(issuer), + stamped: true, }); const LEGACY_CLIENT_INFO_FIELDS: ClientInfoFields = { clientSecret: LEGACY_CLIENT_SECRET_FIELD, registrationToken: LEGACY_REGISTRATION_TOKEN_FIELD, + stamped: true, }; const PREREG_CLIENT_INFO_FIELDS: ClientInfoFields = { clientSecret: PREREG_CLIENT_SECRET_FIELD, registrationToken: PREREG_REGISTRATION_TOKEN_FIELD, + stamped: false, }; /** @@ -559,17 +641,25 @@ export function joinIdpSession( export function serverSecretFields( state: ServerOAuthState | undefined, ): string[] { + const stamped = (field: string): string[] => [ + field, + clientCredentialStampField(field), + ]; const fields = new Set([ LEGACY_TOKENS_FIELD, - LEGACY_CLIENT_SECRET_FIELD, - LEGACY_REGISTRATION_TOKEN_FIELD, + ...stamped(LEGACY_CLIENT_SECRET_FIELD), + ...stamped(LEGACY_REGISTRATION_TOKEN_FIELD), PREREG_CLIENT_SECRET_FIELD, PREREG_REGISTRATION_TOKEN_FIELD, ]); for (const issuer of Object.keys(state?.byIssuer ?? {})) { fields.add(issuerTokensField(issuer)); - fields.add(issuerClientSecretField(issuer)); - fields.add(issuerRegistrationTokenField(issuer)); + for (const field of stamped(issuerClientSecretField(issuer))) { + fields.add(field); + } + for (const field of stamped(issuerRegistrationTokenField(issuer))) { + fields.add(field); + } } return [...fields]; } @@ -603,6 +693,32 @@ export function clientCredentialOwners( return owners; } +/** + * `secrets` with each DCR client credential's stamp made to vouch for the + * value beside it (#2734), for a write that copies values it read rather + * than splitting them from state (adoption). A stamp that already vouches + * for its value is kept, whichever client it names — it records where that + * value came from. Any other value is stamped for the client `state` pairs + * it with, which is the pairing the copy itself asserts; a stamp whose + * value is absent is dropped. + */ +export function restampClientCredentials( + state: ServerOAuthState, + secrets: OAuthSecretValues, +): OAuthSecretValues { + const out: OAuthSecretValues = { ...secrets }; + for (const [field, clientId] of clientCredentialOwners(state)) { + const stampField = clientCredentialStampField(field); + const value = Object.hasOwn(out, field) ? out[field] : undefined; + if (value === undefined) { + delete out[stampField]; + } else if (stampedOwner(out[stampField], value) === undefined) { + setOwnEntry(out, stampField, clientCredentialStamp(clientId, value)); + } + } + return out; +} + /** Does this `clientInformation` object still carry a bearer-grade key? */ function clientInfoHasPlaintext( info: ClientInfoSecretKeys | undefined, diff --git a/docs/secret-storage.md b/docs/secret-storage.md index 4d74ba33a..be20aa202 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 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. The earlier namespace's entries are not stranded by this: each state file keeps a sidecar ledger, `.namespaces.json`, recording every namespace it has used and the servers and IdP issuers given store entries under each, and the next re-adoption (or removal of the state file) purges every recorded namespace the file no longer carries. Before re-adoption purges anything, it carries over what the older version could not see: for every server and IdP issuer the file still names, any secret the un-namespaced entries lack is copied from the namespaces the file used to carry, newest first, so a confidential client's `client_secret` (which the older version saved the `client_id` for, but could not reach) survives the round trip; reads of an unstamped file fall back the same way, so the state a save starts from already holds it. Only what nothing references any longer is purged — on every store backend, the OS keychain included, since the ledger names each entry and nothing has to be enumerated. The same purge cleans up after a state file deleted by hand, at the next save to that path. Entries are recorded per store backend (the keychain, or a particular `secrets.json`), and only a run using that backend purges them — so a run that falls back to a different store leaves the keychain's record for the next keychain run rather than "purging" an empty store and forgetting it. A keychain run also purges what was recorded against a `secrets.json`, since moving that file into the keychain once one is available copies its entries across; and when a `secrets.json` run purges its own record, the record passes to the keychain rather than being forgotten, so those copies are still cleaned up the next time the keychain is in use. Re-adoption's carry-over can only read the backend it runs on, so when it lands on a different one than the entries it supersedes (a locked keychain falling back to `secrets.json`, say), it marks those entries in the ledger as still owed to the new namespace (and fails the save if it cannot, rather than commit without the mark). Each save also records in the ledger the `client_id` that every dynamically registered client's `client_secret` and registration token it writes belongs to. The next run on the owed entries' backend reads them, and its first locked save copies them into the new namespace before purging the old one — but only where the new namespace holds nothing for that field and the state file still holds a client with the `client_id` recorded when the secret was written, since a re-registered client's secret is not the old one's, and anything else could resurrect a value cleared since. Entries written before this was recorded carry no owner, so they are purged as before — as is a keychain value whose owner the ledger cannot tell apart from one a `secrets.json` hand-off filled in, rather than guessing. Tokens and pre-registered clients' secrets are not carried this way: either could have been cleared deliberately in the meantime with nothing to tell, and a token is recovered by re-authorizing. Like adoption's migration, the purge runs only under the real cross-process file lock; a namespace whose purge fails stays recorded and is retried on the next save, with a warning. One limit remains: if the ledger is deleted along with its state file, the entries it recorded cannot be found on the keyring backend, which cannot list its entries; on the `file` backend they remain visible in `secrets.json` (ids beginning `oauth++`) and can be removed by hand. 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. The earlier namespace's entries are not stranded by this: each state file keeps a sidecar ledger, `.namespaces.json`, recording every namespace it has used and the servers and IdP issuers given store entries under each, and the next re-adoption (or removal of the state file) purges every recorded namespace the file no longer carries. Before re-adoption purges anything, it carries over what the older version could not see: for every server and IdP issuer the file still names, any secret the un-namespaced entries lack is copied from the namespaces the file used to carry, newest first, so a confidential client's `client_secret` (which the older version saved the `client_id` for, but could not reach) survives the round trip; reads of an unstamped file fall back the same way, so the state a save starts from already holds it. Only what nothing references any longer is purged — on every store backend, the OS keychain included, since the ledger names each entry and nothing has to be enumerated. The same purge cleans up after a state file deleted by hand, at the next save to that path. Entries are recorded per store backend (the keychain, or a particular `secrets.json`), and only a run using that backend purges them — so a run that falls back to a different store leaves the keychain's record for the next keychain run rather than "purging" an empty store and forgetting it. A keychain run also purges what was recorded against a `secrets.json`, since moving that file into the keychain once one is available copies its entries across; and when a `secrets.json` run purges its own record, the record passes to the keychain rather than being forgotten, so those copies are still cleaned up the next time the keychain is in use. Re-adoption's carry-over can only read the backend it runs on, so when it lands on a different one than the entries it supersedes (a locked keychain falling back to `secrets.json`, say), it marks those entries in the ledger as still owed to the new namespace (and fails the save if it cannot, rather than commit without the mark). Each save also stores, beside every dynamically registered client's `client_secret` and registration token, an owner stamp naming the `client_id` it was written for, with a MAC keyed by the value so the stamp vouches for that value only (an extra `…#owner` entry per credential in the store). The stamp lives in the store rather than the ledger because moving `secrets.json` into the keychain copies or keeps each entry as it finds it, without consulting any ledger, so the stamp follows the value that was actually kept. The next run on the owed entries' backend reads them, and its first locked save copies them, stamp included, into the new namespace before purging the old one — but only where the new namespace holds nothing for that field and the value's stamp names the client the state file holds now, since a re-registered client's secret is not the old one's, and anything else could resurrect a value cleared since. A value with no stamp that vouches for it (written before stamps existed, or left behind beside another value's stamp) is purged as before rather than guessed at. Tokens and pre-registered clients' secrets are not carried this way: either could have been cleared deliberately in the meantime with nothing to tell, and a token is recovered by re-authorizing. Like adoption's migration, the purge runs only under the real cross-process file lock; a namespace whose purge fails stays recorded and is retried on the next save, with a warning. One limit remains: if the ledger is deleted along with its state file, the entries it recorded cannot be found on the keyring backend, which cannot list its entries; on the `file` backend they remain visible in `secrets.json` (ids beginning `oauth++`) and can be removed by hand. 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