feat: reconcile per-user MCP credentials on VK and MCP client changes

[Pratham-Mishra04]

Summary

When a Virtual Key's MCP allowlist changes (via dashboard edit, AP propagation, or SCIM auto-assign) or when an MCP client's access configuration changes (VK configs diff or AllowOnAllVirtualKeys toggle), per-user credentials that are no longer valid were previously left stale. This PR introduces a reconciliation layer that orphans vk-keyed OAuth tokens and header credentials whose MCP grant was revoked, reactivates them if the grant returns, and hard-deletes any in-flight pending flow rows that can no longer complete.

Changes

• Added DeleteVirtualKey now also hard-deletes TableMCPPerUserHeaderFlow rows tied to the deleted VK, consistent with how other credential rows are cleaned up on VK deletion.
• Added vkEffectiveMCPClientIDs helper that computes the union of a VK's explicit per-VK MCP allowlist and MCPs with AllowOnAllVirtualKeys=true, mirroring the runtime grant check.
• Added reconcileVKDirectTokensDB and reconcileVKDirectHeaderRowsDB — transactional DB helpers that orphan active credentials outside the effective allowlist, reactivate orphaned credentials that regained access, and hard-delete pending flow rows for lost grants.
• Added readVKsHoldingOauthCredsForMCP and readVKsHoldingHeaderCredsForMCP to identify which VKs need re-evaluation when a change originates on the MCP side.
• Exposed four new ConfigStore interface methods: ReconcileOauthAfterVKChange, ReconcileMCPHeadersAfterVKChange, ReconcileOauthAfterMCPChange, and ReconcileMCPHeadersAfterMCPChange.
• Called the VK-side reconcile methods in updateVirtualKey when MCPConfigs is present in the request.
• Called the MCP-side reconcile methods in updateMCPClient when VKConfigs changed or AllowOnAllVirtualKeys was toggled.
• Added no-op stubs for all four new interface methods to MockConfigStore in tests.
• Reconciliation errors are logged but non-fatal so the primary update response is not blocked.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

1. Create a VK with an explicit MCP allowlist and establish an active OAuth token and a header credential for one of the allowed MCPs.
2. Edit the VK to remove that MCP from its allowlist — verify the token and credential rows transition to status='orphaned' and any pending flow rows for that MCP are deleted.
3. Re-add the MCP to the VK's allowlist — verify the orphaned rows return to status='active'.
4. Toggle AllowOnAllVirtualKeys on an MCP client — verify all VKs holding credentials for that MCP are re-evaluated accordingly.
5. Delete a VK — verify mcp_per_user_header_flows rows for that VK are removed alongside other credential rows.

go test ./framework/configstore/...
go test ./transports/bifrost-http/...

Breaking changes

• Yes
• No

Related issues

Security considerations

Orphaned credentials are invisible to runtime grant checks (status='active' filter), so a user whose VK loses MCP access can no longer use stale credentials to reach that MCP. Pending in-flight flows for revoked grants are hard-deleted immediately, preventing completion of an auth flow that would produce an unusable credential.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[coderabbitai]

Warning

Review limit reached

@Pratham-Mishra04, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 15 minutes. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b895685c-f3aa-4f15-b55b-3c17f4c4f5ed

📥 Commits

Reviewing files that changed from the base of the PR and between 78a9988 and dd1652a.

📒 Files selected for processing (10)

• core/mcp/credstore/per_user_headers.go
• core/mcp/utils/utils.go
• framework/configstore/rdb.go
• framework/configstore/store.go
• transports/bifrost-http/handlers/governance.go
• transports/bifrost-http/handlers/mcp.go
• transports/bifrost-http/handlers/mcp_per_user_headers.go
• transports/bifrost-http/handlers/mcp_sessions.go
• transports/bifrost-http/lib/config.go
• transports/bifrost-http/lib/config_test.go

📝 Walkthrough

Walkthrough

This PR adds per-user credential reconciliation and header-key canonicalization. It introduces helper utilities to normalize header keys (lowercase+trim), canonicalizes keys at request and storage boundaries, defines a four-method ConfigStore reconciliation interface, implements RDB transactional reconciliation that orphans and reactivates per-user OAuth tokens and MCP header credentials based on VK-MCP grant changes, wires reconciliation into MCP and governance handlers with error logging, and improves error handling in session operations.

Changes

Per-user Credential Reconciliation and Header Normalization

Layer / File(s)	Summary
Header canonicalization contract and utilities core/mcp/credstore/per_user_headers.go, core/mcp/utils/utils.go	Header-key canonicalization documented as lowercase+trim per CanonicalizeHeaderKey; new exported helpers normalize keys, key slices, and maps while preserving nil inputs; callback URL base trimmed of trailing /.
Canonicalization at request and persistence boundaries transports/bifrost-http/handlers/mcp.go, transports/bifrost-http/handlers/mcp_per_user_headers.go, transports/bifrost-http/lib/config.go	MCP handler canonicalizes request and admin-provided header keys during creation and updates; canonicalized keys persisted to storage; per-user-headers handler canonicalizes submitted keys; mcputils imported in both handlers.
Reconciliation interface contract and mocks framework/configstore/store.go, transports/bifrost-http/lib/config_test.go	ConfigStore interface adds four reconciliation methods for OAuth/header credentials after VK or MCP changes; MockConfigStore stubs added for test isolation.
RDB reconciliation implementation framework/configstore/rdb.go	Computes VK effective MCP allowlist from per-VK entries and allow_on_all_virtual_keys clients; orphans/reactivates VK-keyed OAuth tokens and header credentials; hard-deletes pending flows outside allowlist; four exported transactional reconciliation methods reconcile single VKs or all VKs affected by MCP changes in deterministic order; also updates config persistence and query documentation.
MCP handler snapshot and post-update flows transports/bifrost-http/handlers/mcp.go	Snapshots AllowOnAllVirtualKeys and PerUserHeaderKeys before in-memory update; moves credential mark-needs-update after runtime update, triggering only when schema adds keys; calls OAuth and header reconciliation when grants or allowlist change.
Governance handler post-VK-update reconciliation transports/bifrost-http/handlers/governance.go	Invokes OAuth and header reconciliation for updated VK when MCPConfigs change and configStore available; logs failures without failing update response.
Session and revoke safety improvements transports/bifrost-http/handlers/mcp_sessions.go, transports/bifrost-http/handlers/mcp_per_user_headers.go	Session handlers treat store lookup errors as 500 instead of misses; revoke deletes pending per-user header flows matching credential's auth-mode binding before credential deletion to prevent post-revoke recreation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• maximhq/bifrost#3565: Both PRs touch the per-user OAuth persistence layer; this PR introduces VK/MCP grant reconciliation methods that change per-user OAuth token/session statuses and delete pending flows, while PR #3565 refactors the underlying per-user OAuth flow/token/session APIs and data model to be mode/identity scoped—the reconciliation logic directly depends on those refactored OAuth store behaviors.

Suggested reviewers

• danpiths
• roroghost17
• akshaydeo

Poem

🐰 Headers now stand tall and true,
With canonicalization through and through,
VKs and MCPs sync their way,
Credentials dance—orphaned stay!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main feature: reconciling per-user MCP credentials when VK and MCP client configurations change.
Description check	✅ Passed	The description follows the template structure with all major sections completed: Summary, Changes, Type of change, Affected areas, How to test, Breaking changes, Security considerations, and Checklist. Content is comprehensive and addresses the reconciliation feature thoroughly.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 05-23-fix_mcp_auth_session_reconciliation_fixes

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Pratham-Mishra04]

• docs: add mcp sessions page and refactor per-user oauth to lazy-auth model #3706 : 2 dependent PRs (#3707 , #3708 )
• feat: reconcile per-user MCP credentials on VK and MCP client changes #3705 👈 (View in Graphite)
• feat: add mcp per-user headers auth flow ui wiring #3704
• feat: add mcp per-user headers auth type with credential storage and submission flow #3703
• refactor: centralize mcp connection lifecycle into AcquireClientConn and decouple credentials from plugin gate #3702
• refactor: introduce MCPCredentialStore abstraction in MCP credential resolution #3656
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[stepsecurity-app]

Security Policy Alert: Secret Policy Violation

This workflow run has been blocked by StepSecurity's secrets policy because it accesses secrets and the workflow file differs from the default branch.

To approve this workflow, please add the workflows-approved label to this PR.

Note: The label must be added by someone other than the PR author (Pratham-Mishra04) or automation bots to ensure proper security review.

After the label is added, you can re-run the blocked workflow to proceed.

This workflow will be automatically approved once merged into the default branch.

For more information, see StepSecurity's Secret Exfiltration Policy documentation.

[greptile-apps]

Confidence Score: 5/5

The reconciliation logic is correct and idempotent; the two remaining findings are non-blocking quality concerns that do not affect security or credential grant enforcement.

The core correctness goals are implemented correctly. The pending-only flow delete and the pre-update snapshots address the most critical issues from the prior review. The two open findings are non-fatal and do not produce wrong grant state.

framework/configstore/rdb.go (readVKsHoldingOauthCredsForMCP status filter) and transports/bifrost-http/handlers/mcp_per_user_headers.go (mergeExistingPerUserHeaders error handling)

Important Files Changed

Filename	Overview
framework/configstore/rdb.go	Adds per-user credential reconciliation helpers (orphan/reactivate/delete-pending). The pending-only filter for flow deletion is correctly implemented. Minor: readVKsHoldingOauthCredsForMCP and its header counterpart include VKs with only expired/failed rows, causing wasted reconcile iterations.
transports/bifrost-http/handlers/mcp.go	Pre-update snapshots of existingAllowOnAllVirtualKeys and existingPerUserHeaderKeys address prior review concerns. Reconciliation fires after UpdateMCPClient succeeds. Early return on VKConfigs failure still skips reconciliation when AllowOnAllVirtualKeys was already committed.
transports/bifrost-http/handlers/mcp_per_user_headers.go	Adds mergeExistingPerUserHeaders for partial header updates and headersFlowIdentity helper. Pending-flow deletion before credential revoke is correct. DB errors in the merge path are silently swallowed, producing a confusing 400 under storage outages.
transports/bifrost-http/handlers/governance.go	Calls reconcile on both OAuth and header surfaces after a VK MCPConfigs update; errors are logged but non-fatal so the primary update response is unblocked.
framework/configstore/store.go	Adds four new ConfigStore interface methods for per-user credential reconciliation (VK-side and MCP-side, OAuth and headers).
transports/bifrost-http/handlers/mcp_sessions.go	Properly surfaces store errors as 500s instead of silently falling through to the OAuth path on DB failure.
core/mcp/utils/utils.go	Adds CanonicalizeHeaderKey, CanonicalizeHeaderKeys, and CanonicalizeHeaderMap helpers; adds trailing-slash trim to BuildMCPCallbackBaseURL.
core/mcp/credstore/per_user_headers.go	Comment-only changes documenting the canonical-form invariant for header keys. No logic changes.
transports/bifrost-http/lib/config.go	Applies CanonicalizeHeaderKeys in mcpClientConfigToTable to normalize keys on the config-file load path.
transports/bifrost-http/lib/config_test.go	Adds no-op stubs for all four new ConfigStore reconciliation methods on MockConfigStore.

_{Reviews (14): Last reviewed commit: "fix: mcp auth session reconciliation fix..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

transports/bifrost-http/handlers/mcp.go (1)

1115-1125: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reconcile after partial-success updates too.

If allow_on_all_virtual_keys changes, the MCP config update has already committed before the vk_configs transaction runs. When that later transaction fails and the handler returns at Line 1124, the block at Lines 1193-1203 never executes, so revoked per-user OAuth/header credentials stay in their old state until another update.

💡 Minimal fix

 		}); err != nil {
+			if req.AllowOnAllVirtualKeys != existingAllowOnAllVirtualKeys {
+				if recErr := h.store.ConfigStore.ReconcileOauthAfterMCPChange(ctx, id); recErr != nil {
+					logger.Error(fmt.Sprintf("reconcile OAuth credentials after MCP %s update failed: %v", id, recErr))
+				}
+				if recErr := h.store.ConfigStore.ReconcileMCPHeadersAfterMCPChange(ctx, id); recErr != nil {
+					logger.Error(fmt.Sprintf("reconcile per-user-headers credentials after MCP %s update failed: %v", id, recErr))
+				}
+			}
 			// NOTE: Partial success — the MCP client config was already updated in DB and memory above.

Also applies to: 1183-1203

🤖 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 `@framework/configstore/rdb.go`:
- Around line 5707-5717: The vkIDs slice returned by
readVKsHoldingOauthCredsForMCP is unordered and can cause deadlocks when two
MCP-triggered reconciliations lock VKs in different orders; to fix, sort the
vkIDs slice (e.g., using Go's sort.Slice or sort.Strings/int conversion)
immediately after vkIDs, before iterating and calling
reconcileVKDirectTokensDB(tx, vkID) inside the transaction, and apply the same
change to the other similar transaction block (the one mirrored at lines
~5727-5737) so both reconciliation loops iterate VKs in a deterministic,
consistent order.
- Around line 5570-5599: The orphaning/reactivation and flows delete currently
only filter on virtual_key_id (orphanQ, flowsQ), so they can touch rows from
other modes; update the queries on TableOauthUserToken (orphanQ and the
reactivation query) to also filter by auth_mode = "virtual_key" and update the
TableOauthUserSession delete (flowsQ) to also filter by flow_mode =
"virtual_key" (or the exact sentinel your codebase uses for VK-mode) and
preserve the existing allowedClientIDs conditions; this ensures only VK-mode
rows (vkID-bound tokens and sessions) are orphaned/reactivated or deleted.

In `@framework/configstore/store.go`:
- Around line 434-437: Update the comment for
ListAllPendingMCPPerUserHeaderFlows to reflect that it does not return rows
regardless of caller identity but is subject to query-scope narrowing via
ScopedDB(ctx); mention that the implementation in framework/configstore/rdb.go
uses ScopedDB(ctx) so results are limited to rows visible to the caller
(row-visibility scoping) rather than global pending rows.

🪄 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: 4997c318-9da5-41cd-a2a2-20056cd15f2b

📥 Commits

Reviewing files that changed from the base of the PR and between 70c0449 and 5919a07.

📒 Files selected for processing (5)

• framework/configstore/rdb.go
• framework/configstore/store.go
• transports/bifrost-http/handlers/governance.go
• transports/bifrost-http/handlers/mcp.go
• transports/bifrost-http/lib/config_test.go

[Pratham-Mishra04]

Merge activity

• May 27, 10:30 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 27, 10:42 AM UTC: Graphite rebased this pull request as part of a merge.
• May 27, 10:43 AM UTC: @Pratham-Mishra04 merged this pull request with Graphite.

The base branch was changed.