fix: gate verify-headers replay guard on pre-request pending_verification state, project needs_reauth over unstable, and block premature shared-OAuth completion - #5973
Merged
Pratham-Mishra04 merged 1 commit intoAug 8, 2026
Conversation
|
|
This was referenced Aug 8, 2026
Merged
Collaborator
Author
18 tasks
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe MCP handler now distinguishes concurrent header-verification replays from credential repairs, blocks premature OAuth completion, and projects stale admin credentials as ChangesMCP verification lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Aug 8, 2026
Collaborator
Author
Merge activity
|
![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: 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/mcp.go`:
- Around line 497-504: Serialize initial header verification in the handler
before upstream discovery by atomically claiming the pending-verification
bootstrap state, so only one request can proceed to runtime activation and
credential retention. Ensure failed discovery or bootstrap releases the claim,
while successful verification persists the established tools and retained
headers; update the replay guard around isConcurrentVerifyReplay accordingly.
Add a concurrent handler test that makes both requests reload before the first
persistence and verifies the second cannot activate or retain credentials.
🪄 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: a46b5b5a-bfa6-4818-a5b6-9d8b5800d7ca
📒 Files selected for processing (4)
transports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcp_oauth_premature_completion_test.gotransports/bifrost-http/handlers/mcp_peruser_state_projection_test.gotransports/bifrost-http/handlers/mcp_verifyheaders_replay_test.go
Comment on lines
+497
to
+504
| // | ||
| // Gated on isConcurrentVerifyReplay (not a bare DiscoveredTools != nil | ||
| // check): a client that wasn't pending_verification when this request | ||
| // started isn't racing another request's bootstrap, it's a legitimate | ||
| // repair of an already-verified client — that case must fall through | ||
| // and run to completion, or the admin credential (retained further | ||
| // below) could never be repaired once a client has been verified once. | ||
| if isConcurrentVerifyReplay(wasPendingVerificationBeforeThisRequest, clientConfig.DiscoveredTools != nil) { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize initial header verification.
If two requests start in pending_verification, both capture true. If both upstream discovery calls finish before either request persists DiscoveredTools, both reload rows without tools and bypass this guard. Both requests can then activate and persist different tool maps and retained admin headers. The later request wins without replay detection.
Atomically claim the bootstrap verification before discovery, and release the claim on failure. Do not let a second bootstrap request reach runtime activation or credential retention. Add a concurrent handler test that forces both requests to reload before the first persistence.
🤖 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 `@transports/bifrost-http/handlers/mcp.go` around lines 497 - 504, Serialize
initial header verification in the handler before upstream discovery by
atomically claiming the pending-verification bootstrap state, so only one
request can proceed to runtime activation and credential retention. Ensure
failed discovery or bootstrap releases the claim, while successful verification
persists the established tools and retained headers; update the replay guard
around isConcurrentVerifyReplay accordingly. Add a concurrent handler test that
makes both requests reload before the first persistence and verifies the second
cannot activate or retain credentials.
Pratham-Mishra04
changed the base branch from
08-08-fix_persist_discovered_tools_before_triggering_cluster_propagation_in_verify-headers_verify-exchange_to_close_a_stale-read_race
to
graphite-base/5973
August 8, 2026 11:42
Pratham-Mishra04
force-pushed
the
08-08-fix_correct_per-user_mcp_state-projection_and_reauthorize_completion_bugs
branch
from
August 8, 2026 11:45
6d002c3 to
2e96e7f
Compare
Pratham-Mishra04
deleted the
08-08-fix_correct_per-user_mcp_state-projection_and_reauthorize_completion_bugs
branch
August 8, 2026 11:47
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
2 participants
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
Fixes two bugs in the MCP client verification and OAuth reauthorization flows:
Verify-headers repair was permanently broken for already-verified per-user-headers clients. The concurrent-replay guard used a bare
DiscoveredTools != nilcheck, which caused every legitimate admin-credential repair attempt to short-circuit before reaching theUpsertCredentialstep — leaving the credential permanently stuck atneeds_updateonce a client had been verified once.Shared-OAuth reauthorization could complete prematurely. When an upstream/mock authorization server rejected a reauth request with a dead-end error page (instead of redirecting back via the OAuth callback),
complete-oauthcould still succeed using the stale credential already on file. The existing per-user-oauth freshness check (shared token presence) doesn't apply to shared-oauth clients, which always have an active token in flight during reauth.Changes
isConcurrentVerifyReplay: Replaces the bareDiscoveredTools != nilguard with a two-condition check: a replay is only detected when the client was genuinely inpending_verificationstate at the start of the request AND the reloaded row now has tools. A client that was already verified (any other state) falls through to completion regardless of whether tools are present, ensuring the admin-credential retention step always runs during a repair.isPrematureOAuthCompletion: Adds a freshness check for shared-OAuth reauthorization using the OAuth flow row rather than token status.TableOauthConfig.Statusis write-once and never regresses once"authorized", so it can't distinguish a stale credential from a freshly-rotated one during a failed reauth. A flow row still in"pending"and not yet expired means the callback never ran, and completion is rejected with a409 Conflict.getMCPClientRuntimeState: Extracts a helper to read the liveMCPConnectionStatefrom the running manager for a single client by ID, sinceGetMCPClientConfigByIDdoes not populate state (it is not a DB column).projectPerUserAdminCredentialState: Extends theneeds_reauthprojection to also apply overUnstableruntime states, not justHealthy. A dead admin credential never self-heals, soneeds_reauthis a more actionable signal thanUnstableand must win.DisabledandPendingVerificationstill pass through unchanged.Adds unit tests for
isConcurrentVerifyReplay,isPrematureOAuthCompletion, and the updatedprojectPerUserAdminCredentialStateprojection cases.Type of change
Affected areas
How to test
go test ./transports/bifrost-http/handlers/...TestIsConcurrentVerifyReplay: verifies the three cases — genuine concurrent race, legitimate repair on an already-verified client, and first-time verification with no race.TestIsPrematureOAuthCompletion: verifies nil flow, pending+unexpired (premature), pending+expired (abandoned), and failed flow rows.TestProjectPerUserAdminCredentialState: verifies thatUnstablewith a dead admin credential projects toneeds_reauth,Unstablewith an active credential staysUnstable, andPendingVerification/Disabledpass through unchanged.Breaking changes
Security considerations
The premature-OAuth-completion guard prevents a reauthorization from silently succeeding with a stale or broken credential when the upstream authorization server rejects the flow before the callback runs. Without it, an admin could believe reauth succeeded while the client continues operating on an invalid credential.
Checklist