tests: add OAuth2 issuance, JWT verification, consent, discovery, sessions, and MCP auth unit tests - #4523
Conversation
|
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds OAuth2 and MCP auth test coverage across config-store behavior, handler flows, JWT verification, discovery, consent, issuance, sessions, and server routing, plus a small provider fixture cleanup. ChangesConfigStore OAuth2 and MCP Sessions Tests
HTTP Handler OAuth2 Tests
Provider Test Fixture Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Confidence Score: 5/5Test-only additions with no production code changes; all new tests are self-contained and use in-memory SQLite or mock stores. The three comments are coverage suggestions for edge-case security properties not yet asserted. None represent a defect in the tests already written; the shared infrastructure and real-store issuance tests are well-designed. No files require special attention; coverage gaps noted are optional improvements in mcpoauth2issuance_test.go and mcpoauth2consent_test.go. Important Files Changed
Reviews (23): Last reviewed commit: "feat: adds unit tests for mcp oauth serv..." | Re-trigger Greptile |
5e5e90a to
5ee117b
Compare
91623fa to
045411d
Compare
5ee117b to
ce7bb1b
Compare
045411d to
7cae97e
Compare
ce7bb1b to
e30b088
Compare
736beb4 to
f48f3dd
Compare
e30b088 to
0a481ed
Compare
f48f3dd to
47877f6
Compare
0a481ed to
6e1db93
Compare
47877f6 to
3cbbfba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transports/bifrost-http/handlers/mcpoauth2jwt_test.go`:
- Around line 311-340: The algorithm rejection subtests in verifyMCPJWT are not
isolating the alg check because they omit baseline time claims and can fail
earlier for missing claims. Update the test cases in mcpoauth2jwt_test.go to
start from the same valid claim set used by mintTestToken(), then vary only the
algorithm and signature details for the HS256, RS384, and alg=none cases. This
keeps the other claims valid so the tests specifically verify algorithm
rejection behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 618b8931-087e-4313-9fec-0ae1dabf3d73
📒 Files selected for processing (10)
framework/configstore/rdb_mcp_sessions_identity_test.goframework/configstore/rdb_oauth2_test.gotransports/bifrost-http/handlers/mcpoauth2consent_test.gotransports/bifrost-http/handlers/mcpoauth2discovery_test.gotransports/bifrost-http/handlers/mcpoauth2issuance_test.gotransports/bifrost-http/handlers/mcpoauth2jwt_test.gotransports/bifrost-http/handlers/mcpoauth2sessions_test.gotransports/bifrost-http/handlers/mcpoauth2utils_test.gotransports/bifrost-http/handlers/mcpserver_auth_test.gotransports/bifrost-http/handlers/providers_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- framework/configstore/rdb_mcp_sessions_identity_test.go
- transports/bifrost-http/handlers/mcpoauth2discovery_test.go
- transports/bifrost-http/handlers/providers_test.go
- transports/bifrost-http/handlers/mcpoauth2sessions_test.go
- transports/bifrost-http/handlers/mcpoauth2issuance_test.go
- transports/bifrost-http/handlers/mcpoauth2consent_test.go
- transports/bifrost-http/handlers/mcpoauth2utils_test.go
- framework/configstore/rdb_oauth2_test.go
2dca03f to
fd8fa9f
Compare
074f31b to
68af0f2
Compare
fd8fa9f to
caf8e29
Compare
68af0f2 to
671ccdc
Compare
caf8e29 to
c2b0c9c
Compare
671ccdc to
386bf28
Compare
c2b0c9c to
c1d6601
Compare
386bf28 to
619d270
Compare
Merge activity
|
The base branch was changed.
619d270 to
582cdb3
Compare
…sions, and MCP auth unit tests (#4523) ## Summary Adds a comprehensive test suite for the OAuth2/MCP authentication layer, covering the configstore persistence layer, HTTP handler logic, JWT issuance and verification, consent flows, session management, and discovery endpoints. ## Changes - Added configstore tests validating the OAuth2 signing key auto-generation and stability, authorize request atomic state transitions (pending → consented → code_issued), single-use code enforcement, expired code rejection, refresh token rotation and replay detection, family-wide revocation, token and client sweep/GC behavior, and the identity filter OR-group parenthesization in `ListOauthUserTokens`. - Added handler tests for `getMCPServerForRequest` covering the JWT path (vk/user/session modes, active/inactive VK checks, session validation matching, auth enforcement gating) and the header/anonymous path (header VK acceptance and rejection, anonymous fallback, OAuth strict mode WWW-Authenticate responses). - Added JWT verification tests covering all valid modes, a full matrix of rejection cases (expired, nbf in future, missing claims, issuer/audience mismatch, wrong kid, wrong signing key, non-RS256 algorithms, alg:none, malformed tokens), and infrastructure fault isolation (nil store, unavailable signing key). - Added consent flow tests for `flowDetail` (pending, missing, empty id, expired, already-consented) and `flowSubmit` across VK (active, inactive, unknown, empty, double-submit), session (enforced vs. not enforced), user (no resolver, resolved, no session), and VK-to-user upgrade binding (matching owner, mismatched owner, not signed in). - Added OAuth2 issuance handler tests for DCR registration, the authorize endpoint (happy path, loopback port flexibility, unknown client, unregistered redirect, protocol error redirects), token endpoint authorization code exchange (happy path, PKCE mismatch, single-use, expired, client_id mismatch, missing fields, unsupported grant), and refresh token rotation and replay guard. - Added session management handler tests for listing grants and revoking grants with identity-gated authorization (vk mode unrestricted, user mode caller-match, mismatch, unauthenticated). - Added discovery handler tests verifying that headers mode returns 404 on all discovery endpoints, while oauth and both modes serve PRM, AS metadata, and JWKS with correct field values. - Added utility tests for `matchRedirectURI` (exact match, loopback port flexibility, path/scheme enforcement, malformed input), `oauth2IssuerURL` (configured vs. request-host fallback), and `oauth2ServerCfg` default TTL values. - Introduced shared test infrastructure: `mockOAuth2Store`, `newTestSigningKey`, `newTestOAuth2Config`, `mintTestToken`, `newTestMCPHandler`, and real SQLite-backed store helpers for issuance tests that require actual atomic semantics. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... ./transports/bifrost-http/handlers/... ``` All new test files are self-contained and use in-memory SQLite or mock stores; no external services are required. ## Screenshots/Recordings N/A ## Breaking changes - [x] No ## Related issues N/A ## Security considerations Tests explicitly cover security-critical paths: JWT algorithm confusion (alg:none, HS256, RS384), replay detection for authorization codes and refresh tokens, family-wide token revocation on replay, inactive VK rejection, identity mismatch on session revocation, and WWW-Authenticate header presence on OAuth strict mode rejections. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Adds a comprehensive test suite for the OAuth2/MCP authentication layer, covering the configstore persistence layer, HTTP handler logic, JWT issuance and verification, consent flows, session management, and discovery endpoints.
Changes
ListOauthUserTokens.getMCPServerForRequestcovering the JWT path (vk/user/session modes, active/inactive VK checks, session validation matching, auth enforcement gating) and the header/anonymous path (header VK acceptance and rejection, anonymous fallback, OAuth strict mode WWW-Authenticate responses).flowDetail(pending, missing, empty id, expired, already-consented) andflowSubmitacross VK (active, inactive, unknown, empty, double-submit), session (enforced vs. not enforced), user (no resolver, resolved, no session), and VK-to-user upgrade binding (matching owner, mismatched owner, not signed in).matchRedirectURI(exact match, loopback port flexibility, path/scheme enforcement, malformed input),oauth2IssuerURL(configured vs. request-host fallback), andoauth2ServerCfgdefault TTL values.mockOAuth2Store,newTestSigningKey,newTestOAuth2Config,mintTestToken,newTestMCPHandler, and real SQLite-backed store helpers for issuance tests that require actual atomic semantics.Type of change
Affected areas
How to test
go test ./framework/configstore/... ./transports/bifrost-http/handlers/...All new test files are self-contained and use in-memory SQLite or mock stores; no external services are required.
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
Tests explicitly cover security-critical paths: JWT algorithm confusion (alg:none, HS256, RS384), replay detection for authorization codes and refresh tokens, family-wide token revocation on replay, inactive VK rejection, identity mismatch on session revocation, and WWW-Authenticate header presence on OAuth strict mode rejections.
Checklist
docs/contributing/README.mdand followed the guidelines