Add replay protection to private_key_jwt client assertions - #4673
Conversation
📝 WalkthroughWalkthroughOAuth client assertion validation now uses shared JTI storage and JWT leeway. OAuth initialization passes these values through PAR, CIBA, token, introspection, and revocation route setup. ChangesOAuth JTI replay protection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/internal/oauth/oauth2/clientauth/clientauth_test.go (1)
1516-1521: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the JTI retention deadline.
These expectations accept any expiry value. Assert
time.Unix(9999999999+testLeeway, 0)in oneRecordJTIexpectation. Otherwise, removing leeway fromrecordAssertionJTIkeeps the replay tests passing.🤖 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 `@backend/internal/oauth/oauth2/clientauth/clientauth_test.go` around lines 1516 - 1521, Update the two RecordJTI expectations in the replay test to assert the retention deadline explicitly: use time.Unix(9999999999+testLeeway, 0) for the expiry argument in one expectation instead of mock.Anything, while preserving the existing return values and call ordering.
🤖 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 `@backend/internal/oauth/oauth2/clientauth/clientauth.go`:
- Around line 312-335: Update the relevant OAuth client-authentication guide
under docs/content/guides/ and the request semantics in docs/content/apis.mdx to
document private_key_jwt requirements: jti is mandatory, each assertion is
single-use across client-authenticated OAuth endpoints, replay attempts fail,
and replay retention lasts until exp plus the configured JWT leeway. Ensure the
documentation matches recordAssertionJTI behavior.
---
Nitpick comments:
In `@backend/internal/oauth/oauth2/clientauth/clientauth_test.go`:
- Around line 1516-1521: Update the two RecordJTI expectations in the replay
test to assert the retention deadline explicitly: use
time.Unix(9999999999+testLeeway, 0) for the expiry argument in one expectation
instead of mock.Anything, while preserving the existing return values and call
ordering.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 64180c34-8fa4-4c98-850e-88d468d7508e
📒 Files selected for processing (12)
backend/internal/oauth/init.gobackend/internal/oauth/oauth2/ciba/init.gobackend/internal/oauth/oauth2/clientauth/clientauth.gobackend/internal/oauth/oauth2/clientauth/clientauth_test.gobackend/internal/oauth/oauth2/clientauth/middleware.gobackend/internal/oauth/oauth2/clientauth/middleware_test.gobackend/internal/oauth/oauth2/introspect/init.gobackend/internal/oauth/oauth2/introspect/init_test.gobackend/internal/oauth/oauth2/par/init.gobackend/internal/oauth/oauth2/revocation/init.gobackend/internal/oauth/oauth2/revocation/init_test.gobackend/internal/oauth/oauth2/token/init.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Purpose
private_key_jwtclient assertions had no replay protection:jtiwas never tracked,unlike DPoP proofs and ID-JAG assertions which both use the shared
oauth2/jtideny-list.A captured, still-valid
client_assertioncould be replayed to mint tokens as that clientwithout its private key. Affects all endpoints behind
ClientAuthMiddleware. (CWE-294.)Approach
Wire the existing
jtideny-list into theprivate_key_jwtpath, mirroring DPoP/ID-JAG:after verification, record the assertion's
jti(now required) and reject replays. Theentry is bounded by the assertion's
expplus clock-skew leeway. Threaded the sharedjtiStoreand leeway throughClientAuthMiddlewareinto all five consuming endpoints.Summary by CodeRabbit