Allow DPoP-bound OAuth access tokens#1395
Merged
brendan-kellam merged 7 commits intoJun 29, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds DPoP sender-constrained OAuth token support across storage, authorization, token issuance, discovery metadata, and request authentication. It also adds request-scoped context handling for API handlers and updates tests and mocks for the new DPoP fields and flows. ChangesDPoP OAuth Token Binding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…cess-tokens-ex-dpop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/web/src/ee/features/oauth/dpop.test.ts (1)
34-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for request and key binding.
The suite verifies
athand replay failures, but not wronghtu/htmorexpectedJktmismatch. Those are core sender-constrained checks and cheap regression tests.Suggested tests
+ test('rejects a proof for a different request URI', async () => { + const keyPair = await generateKeyPair(); + const request = new Request('http://internal.test/api/ee/oauth/token', { method: 'POST' }); + const proof = await signDpopProof({ + ...keyPair, + htm: 'POST', + htu: 'https://sourcebot.test/api/ee/oauth/other', + }); + + await expect(verifyDpopProof({ request, proof })).resolves.toMatchObject({ + ok: false, + error: 'invalid_dpop_proof', + }); + }); + + test('rejects a proof signed by a different key than the expected binding', async () => { + const keyPair = await generateKeyPair(); + const otherKeyPair = await generateKeyPair(); + const request = new Request('http://internal.test/api/ee/oauth/token', { method: 'POST' }); + const proof = await signDpopProof({ + ...keyPair, + htm: 'POST', + htu: 'https://sourcebot.test/api/ee/oauth/token', + }); + + await expect(verifyDpopProof({ + request, + proof, + expectedJkt: calculateDpopJkt(otherKeyPair.publicJwk), + })).resolves.toMatchObject({ + ok: false, + error: 'invalid_dpop_proof', + }); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/ee/features/oauth/dpop.test.ts` around lines 34 - 120, Add negative coverage to verifyDpopProof for sender-constrained checks that are currently missing. Extend the dpop.test.ts suite with tests around verifyDpopProof that reject a proof when the request method/URL binding is wrong (mismatched htm and htu) and when the key binding is wrong (expectedJkt does not match the proof’s public key). Reuse the existing generateKeyPair, signDpopProof, calculateDpopJkt, and verifyDpopProof helpers to keep the tests focused and consistent with the current cases.packages/web/src/ee/features/oauth/dpop.ts (1)
49-49: 🔒 Security & Privacy | 🔵 TrivialVerify replay protection across production instances.
seenProofJtisis process-local, so replay prevention will not hold across multiple Node workers/serverless instances; the unboundedMapplus full cleanup scan can also become an availability risk. Please confirm single-process deployment or move this to a bounded shared TTL store.Also applies to: 241-255
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/ee/features/oauth/dpop.ts` at line 49, The replay cache in dpop.ts is only process-local, so DPoP replay protection will fail across multiple workers or instances and the unbounded seenProofJtis Map plus cleanup scan can hurt availability. Update the DPoP proof handling around seenProofJtis and the related replay-check logic in the verifier to use a bounded shared TTL-backed store, or explicitly guard it so it only runs in a single-process deployment; keep the unique jti tracking semantics but remove reliance on in-memory process state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/web/src/app/oauth/authorize/page.tsx`:
- Around line 30-37: The OAuth authorize page currently accepts repeated or
empty dpop_jkt values by collapsing arrays to the first entry and only
validating when a truthy value exists. Update the validation in
authorize/page.tsx so the handling around params destructuring and the consent
gate rejects repeated dpop_jkt values as invalid, and also validates when
dpop_jkt is present even if empty. Use the existing authorize-page flow and the
dpopJkt/resource parsing logic to ensure any supplied dpop_jkt parameter must be
a single non-empty value before consent is shown.
In `@packages/web/src/ee/features/oauth/dpop.ts`:
- Around line 155-176: The DPoP replay cache in the validation flow is expiring
`jti` entries based on the current time instead of the proof’s accepted `iat`
boundary, so update the `recordProofJti()` call site in
`validateDpopProof`/`invalidDpopProof` flow to compute expiration from
`payload.iat` plus `DPOP_PROOF_IAT_WINDOW_SECONDS`. Also move
`recordProofJti(jkt, payload.jti)` to after the `ath` checks so a proof is only
recorded once it has fully passed validation.
- Around line 102-113: The DPoP parsing path in dpop.ts needs to verify the
decoded JWT values are plain objects before reading properties. In the block
that assigns header and payload, keep the JSON.parse handling but add an
object-shape guard for both values before using header.typ in validateDpopProof,
so malformed JSON like null, arrays, or primitives returns invalidDpopProof
instead of throwing. Use the existing DpopHeader and DpopPayload parsing flow
and the invalidDpopProof helper to keep the failure path consistent.
---
Nitpick comments:
In `@packages/web/src/ee/features/oauth/dpop.test.ts`:
- Around line 34-120: Add negative coverage to verifyDpopProof for
sender-constrained checks that are currently missing. Extend the dpop.test.ts
suite with tests around verifyDpopProof that reject a proof when the request
method/URL binding is wrong (mismatched htm and htu) and when the key binding is
wrong (expectedJkt does not match the proof’s public key). Reuse the existing
generateKeyPair, signDpopProof, calculateDpopJkt, and verifyDpopProof helpers to
keep the tests focused and consistent with the current cases.
In `@packages/web/src/ee/features/oauth/dpop.ts`:
- Line 49: The replay cache in dpop.ts is only process-local, so DPoP replay
protection will fail across multiple workers or instances and the unbounded
seenProofJtis Map plus cleanup scan can hurt availability. Update the DPoP proof
handling around seenProofJtis and the related replay-check logic in the verifier
to use a bounded shared TTL-backed store, or explicitly guard it so it only runs
in a single-process deployment; keep the unique jti tracking semantics but
remove reliance on in-memory process state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efedc7d9-2516-4068-9ac1-044fefee9bc8
📒 Files selected for processing (17)
CHANGELOG.mdpackages/db/prisma/migrations/20260629193000_add_oauth_dpop_binding/migration.sqlpackages/db/prisma/schema.prismapackages/web/src/__mocks__/prisma.tspackages/web/src/app/api/(server)/ee/.well-known/oauth-authorization-server/route.tspackages/web/src/app/api/(server)/ee/.well-known/oauth-protected-resource/[...path]/route.tspackages/web/src/app/api/(server)/ee/mcp/route.tspackages/web/src/app/api/(server)/ee/oauth/token/route.tspackages/web/src/app/oauth/authorize/components/consentScreen.tsxpackages/web/src/app/oauth/authorize/page.tsxpackages/web/src/ee/features/oauth/actions.tspackages/web/src/ee/features/oauth/dpop.test.tspackages/web/src/ee/features/oauth/dpop.tspackages/web/src/ee/features/oauth/server.test.tspackages/web/src/ee/features/oauth/server.tspackages/web/src/middleware/withAuth.test.tspackages/web/src/middleware/withAuth.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/app/api/(server)/ee/mcp/route.ts (1)
23-32: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the
WWW-Authenticateauth-param with the MCP discovery parser.
extractResourceMetadataUrl()inpackages/web/src/ee/features/chat/mcp/dcrDiscovery.ts:93-113only matchesresource_metadata="...". Advertisingresource_metadata_uri="..."here means those 401 challenges are ignored, so the MCP client never learns the protected-resource metadata URL from this response. Update both sides to use the same parameter name in this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/src/app/api/`(server)/ee/mcp/route.ts around lines 23 - 32, The `WWW-Authenticate` challenges in the MCP route use `resource_metadata_uri`, but `extractResourceMetadataUrl()` in `dcrDiscovery.ts` only parses `resource_metadata`, so the 401 response is ignored by the client. Update the auth-param name in the route handler that appends the `Bearer` and `DPoP` headers, and make sure the MCP discovery parser expects the same parameter name so both sides match consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/web/src/app/api/`(server)/ee/mcp/route.ts:
- Around line 23-32: The `WWW-Authenticate` challenges in the MCP route use
`resource_metadata_uri`, but `extractResourceMetadataUrl()` in `dcrDiscovery.ts`
only parses `resource_metadata`, so the 401 response is ignored by the client.
Update the auth-param name in the route handler that appends the `Bearer` and
`DPoP` headers, and make sure the MCP discovery parser expects the same
parameter name so both sides match consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbe9430f-e9ba-4294-a5e6-bd2c52b6d832
📒 Files selected for processing (6)
packages/web/src/app/api/(server)/ee/mcp/route.tspackages/web/src/lib/apiHandler.test.tspackages/web/src/lib/apiHandler.tspackages/web/src/lib/requestContext.tspackages/web/src/middleware/withAuth.test.tspackages/web/src/middleware/withAuth.ts
brendan-kellam
deleted the
brendan/sou-947-allow-sender-constraining-access-tokens-ex-dpop
branch
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
1 participant
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.
Summary
Testing
Summary by CodeRabbit
dpop_jktcapture/validation on the authorization page and propagation through token exchanges; token responses now return the appropriate token type when bound.