Repository navigation
fix(auth): read a DCR secret stamped for another client as absent (#2768) - #2792
Merged
Merged
Conversation
) 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 <[email protected]> Signed-off-by: cliffhall <[email protected]>
There was a problem hiding this comment.
🟢 Approval recommended
The focused ownership check matches the issue’s requirements and has targeted regression coverage, with no blocking findings.
No open issues 🎉
What changed in this PR
Fixes #2768 in the shared OAuth storage layer by treating DCR credentials as absent when a valid owner stamp names a different client.
Changes:
- Checks client secrets and registration tokens independently before restoring them from storage.
- Preserves existing behavior for unstamped or invalidly stamped values, pre-registered clients, and slots without a client ID.
- Adds unit coverage and repeated-handoff integration regressions.
| File | Description |
|---|---|
| core/auth/node/oauth-secrets.ts | Filters stored DCR credentials with a verified owner mismatch. |
| clients/web/src/test/integration/auth/node/oauth-secrets-namespace.test.ts | Shares handoff setup and tests ordinary reads with and without re-registration. |
| clients/web/src/test/core/auth/oauth-secrets.test.ts | Covers field-level filtering and compatibility cases. |
Review effort: Balanced
Member
Author
|
Copilot review loop closed: round 1 was clean (approval recommended, no inline or suppressed comments), so no further round was requested. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2768
Problem
#2739 stamps each DCR
client_secret/ registration token with its owner (…#owner: aclient_idplus a MAC keyed by the value). Only carries consulted that stamp. The ordinary read,joinServerOAuthState, paired whatever value the store held with the state file'sclient_id. After a repeated file-to-keychain hand-off with a re-registration in between, the keychain keeps client A's secret, and a read returned:{"client_id":"client-b","client_secret":"secret-a"}That client then fails with
invalid_clientat the token endpoint.Fix
joinClientInformation(core/auth/node/oauth-secrets.ts) now treats a DCR value as absent when its stamp vouches for it (MAC valid) but names a differentclient_idthan the slot holds. The read then behaves as it would for a client with no stored secret.What keeps today's behavior:
client_idhas no owner to compare against and is served as before.The secret and the registration token are checked field by field, so only the mismatched one is dropped.
Tests
oauth-secrets-namespace.test.tsstages the issue's repeated hand-off with no strip and asserts the read returns onlyclient_id: client-b. Without the fix it fails with exactly the issue's output ("client_secret": "secret-a"). A companion test confirms the secret is still served when no re-registration happened. The hand-off steps are pulled out of the existingrepeatedHandOffhelper intohandOffTwiceso both tests can share them.oauth-secrets.test.tscovers a mismatched secret plus registration token, a mismatch on one field only, the unstamped and invalid-stamp cases, the legacy unkeyed slot versus a pre-registered slot, and a slot with noclient_id.npm run local:gatepasses.🤖 Generated with Claude Code