From 4ea20d02e2181dc6ba5b7f1d4860f76c2ee0eeac Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 24 Oct 2026 21:32:29 -0400 Subject: [PATCH] fix(auth): read a DCR secret stamped for another client as absent (#2768) The owner stamp #2739 added beside each DCR client_secret and registration token was consulted only by carries. The ordinary read (joinServerOAuthState) paired whatever value the store held with the state file's client_id, so after a repeated file-to-keychain hand-off with a re-registration in between, a read returned client-b with client-a's secret, failing only at the token endpoint. joinClientInformation now drops a DCR value whose stamp vouches for it (MAC valid) but names a different client_id. Unstamped values keep today's behavior; pre-registered clients are never stamped and are unaffected. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- .../src/test/core/auth/oauth-secrets.test.ts | 87 +++++++++++++++++++ .../auth/node/oauth-secrets-namespace.test.ts | 45 ++++++++-- core/auth/node/oauth-secrets.ts | 27 +++++- 3 files changed, 151 insertions(+), 8 deletions(-) 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 ef629162a..6d7c26493 100644 --- a/clients/web/src/test/core/auth/oauth-secrets.test.ts +++ b/clients/web/src/test/core/auth/oauth-secrets.test.ts @@ -506,6 +506,93 @@ describe("client-credential owner stamps (#2734)", () => { expect(stampedOwner(stamp, "ics")).toBeUndefined(); }); + describe("join reads a value stamped for another client as absent (#2768)", () => { + const REG = issuerRegistrationTokenField(issuer); + const clientOf = ( + residue: ServerOAuthState, + secrets: Record, + ) => + joinServerOAuthState(residue, secrets).byIssuer![issuer]! + .clientInformation; + + it("drops a secret and registration token whose stamps name another client", () => { + expect( + clientOf(dcr("client-b"), { + [SECRET]: "secret-a", + [STAMP]: clientCredentialStamp("client-a", "secret-a"), + [REG]: "rat-a", + [clientCredentialStampField(REG)]: clientCredentialStamp( + "client-a", + "rat-a", + ), + }), + ).toEqual({ client_id: "client-b" }); + }); + + it("drops only the mismatched field", () => { + expect( + clientOf(dcr("client-b"), { + [SECRET]: "secret-a", + [STAMP]: clientCredentialStamp("client-a", "secret-a"), + [REG]: "rat-b", + [clientCredentialStampField(REG)]: clientCredentialStamp( + "client-b", + "rat-b", + ), + }), + ).toEqual({ client_id: "client-b", registration_access_token: "rat-b" }); + }); + + it.each([ + ["no stamp", undefined], + ["a stamp for another value", clientCredentialStamp("client-a", "other")], + ])("still serves a value with %s", (_label, stamp) => { + expect( + clientOf(dcr("client-b"), { + [SECRET]: "secret-a", + ...(stamp !== undefined && { [STAMP]: stamp }), + }), + ).toEqual({ client_id: "client-b", client_secret: "secret-a" }); + }); + + it("applies to the legacy unkeyed slot, but not to a pre-registered client", () => { + const joined = joinServerOAuthState( + { + clientInformation: { client_id: "client-b" }, + preregisteredClientInformation: { client_id: "pre-b" }, + }, + { + [LEGACY_CLIENT_SECRET_FIELD]: "secret-a", + [clientCredentialStampField(LEGACY_CLIENT_SECRET_FIELD)]: + clientCredentialStamp("client-a", "secret-a"), + [PREREG_CLIENT_SECRET_FIELD]: "pre-secret", + [clientCredentialStampField(PREREG_CLIENT_SECRET_FIELD)]: + clientCredentialStamp("pre-a", "pre-secret"), + }, + ); + expect(joined.clientInformation).toEqual({ client_id: "client-b" }); + expect(joined.preregisteredClientInformation).toEqual({ + client_id: "pre-b", + client_secret: "pre-secret", + }); + }); + + it("serves a stamped value to a slot with no client_id", () => { + expect( + joinServerOAuthState( + { + clientInformation: {} as ServerOAuthState["clientInformation"], + }, + { + [LEGACY_CLIENT_SECRET_FIELD]: "secret-a", + [clientCredentialStampField(LEGACY_CLIENT_SECRET_FIELD)]: + clientCredentialStamp("client-a", "secret-a"), + }, + ).clientInformation, + ).toEqual({ client_secret: "secret-a" }); + }); + }); + it("restamps only what no stamp vouches for, keeping a valid one whatever client it names", () => { const valid = clientCredentialStamp("earlier", "ics"); expect( 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 505321bd5..709c7791d 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 @@ -1306,15 +1306,14 @@ 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. + * The issue's (#2734) steps 1–4: client A saved to `secrets.json` and + * absorbed, then (with `reregister`) client B registered in the file + * under the same stamp, and absorbed again — the keychain keeps A's value. */ - async function repeatedHandOff( + async function handOffTwice( keyring: SecretStore, reregister: boolean, - ): Promise<{ ns1: string; ns2: string }> { + ): Promise { const secretsFile = join(tempDir, "secrets.json"); const fileStore = () => new FileSecretStore({ filePath: secretsFile, passphrase: "" }); @@ -1345,6 +1344,17 @@ describe("a stripped stamp re-adopted on another backend keeps what only the old if (savedFileEnv === undefined) delete process.env[SECRET_FILE_ENV]; else process.env[SECRET_FILE_ENV] = savedFileEnv; } + } + + /** + * {@link handOffTwice}, then the strip of the issue's step 5: the stamp + * stripped and the file re-adopted on a memory backend. + */ + async function repeatedHandOff( + keyring: SecretStore, + reregister: boolean, + ): Promise<{ ns1: string; ns2: string }> { + await handOffTwice(keyring, reregister); const ns1 = namespaceOf(fileA); await stripStamp(fileA); await writeOAuthSections( @@ -1357,6 +1367,29 @@ describe("a stripped stamp re-adopted on another backend keeps what only the old return { ns1, ns2: namespaceOf(fileA) }; } + it("an ordinary read does not pair the secret a repeated hand-off kept with the client registered since (#2768)", async () => { + const keyring = new KeyringSecretStore(); + await handOffTwice(keyring, true); + const ns = namespaceOf(fileA); + // The keychain kept A's value under the namespace the file still holds, + // and its stamp correctly names A; the file says B. + expect( + await keyring.get(oauthSecretServerId(SERVER, ns), SECRET_FIELD), + ).toBe("secret-a"); + expect(clientOf(await readOAuthStore(fileA, keyring))).toEqual({ + client_id: "client-b", + }); + }); + + it("an ordinary read still serves the secret a repeated hand-off kept for its own client (#2768)", async () => { + const keyring = new KeyringSecretStore(); + await handOffTwice(keyring, false); + expect(clientOf(await readOAuthStore(fileA, keyring))).toEqual({ + client_id: "client-a", + client_secret: "secret-a", + }); + }); + 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); diff --git a/core/auth/node/oauth-secrets.ts b/core/auth/node/oauth-secrets.ts index b728ca155..7c55d3a4e 100644 --- a/core/auth/node/oauth-secrets.ts +++ b/core/auth/node/oauth-secrets.ts @@ -357,14 +357,37 @@ function splitClientInformation( * Overlay a slot's stored bearer-grade keys back onto its residue * (store-wins). Only called when the residue slot exists — a secret with no * slot was cleared, and rejoining it would resurrect the cleared value. + * + * A DCR value whose stamp vouches for it but names a different client is + * read as absent (#2768): the file-to-keychain hand-off keeps an existing + * keychain value, so after a re-registration the keychain can still hold + * the previous client's secret, and pairing it with the new `client_id` + * fails only at the token endpoint. A value with no valid stamp (legacy, or + * rewritten by an older Inspector) is served as before — its owner is + * unknown, and an unknown owner is never guessed. */ function joinClientInformation( info: T, fields: ClientInfoFields, secrets: OAuthSecretValues, ): T { - const clientSecret = secrets[fields.clientSecret]; - const registrationToken = secrets[fields.registrationToken]; + const owned = (field: string): string | undefined => { + const value = secrets[field]; + if ( + value === undefined || + !fields.stamped || + typeof info.client_id !== "string" + ) { + return value; + } + const owner = stampedOwner( + secrets[clientCredentialStampField(field)], + value, + ); + return owner === undefined || owner === info.client_id ? value : undefined; + }; + const clientSecret = owned(fields.clientSecret); + const registrationToken = owned(fields.registrationToken); if (clientSecret === undefined && registrationToken === undefined) { return info; }