Skip to content

refactor: remove deferred-fill user-mode OAuth flow support - #3839

Merged
akshaydeo merged 1 commit into
devfrom
05-28-fix_removes_deferred_user_mode_in_mcp_sessions
May 28, 2026
Merged

refactor: remove deferred-fill user-mode OAuth flow support#3839
akshaydeo merged 1 commit into
devfrom
05-28-fix_removes_deferred_user_mode_in_mcp_sessions

Conversation

@Pratham-Mishra04

Copy link
Copy Markdown
Collaborator

Summary

Removes the "deferred-fill" user-mode OAuth flow concept, where a user identity could be left nil at flow initiation and stamped in later at completion time. User-mode flows now require a valid user identity to be present in context at initiation, simplifying the flow lifecycle and eliminating ambiguous states.

Changes

  • InitiateUserOAuthFlow now returns an error immediately if a user-mode flow is started without a user identity in context, rather than allowing a nil user_id to be stored and resolved later.
  • CompleteUserOAuthFlow no longer attempts to pull a user identity from the completer's context as a fallback — the identity must already be on the session row.
  • The UserID comment on TableOauthUserSession has been updated to reflect that it is only populated for user-mode rows and is nil for vk/session-mode rows.
  • The list handler no longer skips deferred-fill user-mode rows (the concept no longer exists).
  • The flowDetail handler no longer falls back to the signed-in caller's user identity when flow.UserID is nil.
  • The loadAuthorizedFlow comment no longer references deferred-fill visibility semantics.
  • The lookupID guard in InitiateUserOAuthFlow has been removed since lookupID is now always populated before the lookup.

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

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

Attempt to initiate a user-mode OAuth flow without a user identity in context and confirm an error is returned. Initiate a user-mode flow with a valid user identity and confirm the flow proceeds and completes correctly with the identity already present on the session row.

Breaking changes

  • Yes
  • No

User-mode OAuth flows that previously relied on deferred identity stamping (initiating without a user identity and resolving it at completion) will now fail at initiation. Callers must ensure a user identity is present in context before calling InitiateUserOAuthFlow for user-mode flows.

Security considerations

This change tightens the identity model for user-mode OAuth flows. Previously, a flow row could exist with no associated user identity, which could allow any authenticated caller to claim the flow URL. Requiring identity at initiation ensures each flow is bound to a specific user from the start, removing the ambiguous ownership window.

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

coderabbitai Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (3)
  • framework/configstore/tables/oauth.go is excluded by none and included by none
  • framework/oauth2/main.go is excluded by none and included by none
  • transports/bifrost-http/handlers/mcp_sessions.go is excluded by none and included by none

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8ceea431-2058-4a81-947b-ab8c2372aee4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 05-28-fix_removes_deferred_user_mode_in_mcp_sessions

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

Pratham-Mishra04 commented May 28, 2026

Copy link
Copy Markdown
Collaborator Author

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
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.

@greptile-apps

greptile-apps Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

Safe to merge; the refactor is internally consistent across all three files and correctly enforces the new identity-at-initiation invariant.

The logic changes are coherent and correctly remove the deferred-fill path end-to-end. The lookupID guard removal is safe because every mode now guarantees a non-empty value before reaching the DB lookup. The removed in-memory audit stamp was never subsequently persisted in the completion path, so its removal has no observable effect.

framework/oauth2/main.go deserves a second look only to confirm that pre-existing null-UserID session rows in production databases are handled acceptably at the flowDetail and list endpoints post-deploy.

Important Files Changed

Filename Overview
framework/configstore/tables/oauth.go Comment-only change: updates the UserID field description to drop deferred-fill language, accurately reflecting the new invariant.
framework/oauth2/main.go Core logic: InitiateUserOAuthFlow now errors on missing user identity; the lookupID guard is safely removed since all modes guarantee a non-empty lookupID; CompleteUserOAuthFlow drops deferred-fill context fallback and the in-memory audit stamp. The removed stamp-back was never persisted post-initiation, so its removal is safe.
transports/bifrost-http/handlers/mcp_sessions.go Handler updates are consistent with the new model: list no longer skips nil-UserID rows (they won't be created anymore), flowDetail drops the caller-identity fallback, and comments are updated to remove deferred-fill references.

Reviews (1): Last reviewed commit: "fix: removes deferred user mode in mcp s..." | Re-trigger Greptile

akshaydeo commented May 28, 2026

Copy link
Copy Markdown
Contributor

Merge activity

  • May 28, 12:21 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 28, 12:22 PM UTC: @akshaydeo merged this pull request with Graphite.

@akshaydeo
akshaydeo merged commit 76632ff into dev May 28, 2026
13 of 14 checks passed
@akshaydeo
akshaydeo deleted the 05-28-fix_removes_deferred_user_mode_in_mcp_sessions branch May 28, 2026 12:22
akshaydeo pushed a commit that referenced this pull request May 29, 2026
## Summary

Removes the "deferred-fill" user-mode OAuth flow concept, where a user identity could be left nil at flow initiation and stamped in later at completion time. User-mode flows now require a valid user identity to be present in context at initiation, simplifying the flow lifecycle and eliminating ambiguous states.

## Changes

- `InitiateUserOAuthFlow` now returns an error immediately if a user-mode flow is started without a user identity in context, rather than allowing a nil `user_id` to be stored and resolved later.
- `CompleteUserOAuthFlow` no longer attempts to pull a user identity from the completer's context as a fallback — the identity must already be on the session row.
- The `UserID` comment on `TableOauthUserSession` has been updated to reflect that it is only populated for user-mode rows and is nil for vk/session-mode rows.
- The `list` handler no longer skips deferred-fill user-mode rows (the concept no longer exists).
- The `flowDetail` handler no longer falls back to the signed-in caller's user identity when `flow.UserID` is nil.
- The `loadAuthorizedFlow` comment no longer references deferred-fill visibility semantics.
- The `lookupID` guard in `InitiateUserOAuthFlow` has been removed since `lookupID` is now always populated before the lookup.

## Type of change

- [ ] Bug fix
- [ ] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go test ./framework/oauth2/...
go test ./transports/bifrost-http/...
```

Attempt to initiate a user-mode OAuth flow without a user identity in context and confirm an error is returned. Initiate a user-mode flow with a valid user identity and confirm the flow proceeds and completes correctly with the identity already present on the session row.

## Breaking changes

- [x] Yes
- [ ] No

User-mode OAuth flows that previously relied on deferred identity stamping (initiating without a user identity and resolving it at completion) will now fail at initiation. Callers must ensure a user identity is present in context before calling `InitiateUserOAuthFlow` for user-mode flows.

## Security considerations

This change tightens the identity model for user-mode OAuth flows. Previously, a flow row could exist with no associated user identity, which could allow any authenticated caller to claim the flow URL. Requiring identity at initiation ensures each flow is bound to a specific user from the start, removing the ambiguous ownership window.

## 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
@akshaydeo akshaydeo mentioned this pull request May 29, 2026
18 tasks
akshaydeo added a commit that referenced this pull request May 29, 2026
## Summary

This PR releases **core v1.5.14**, **framework v1.3.14**, **transports v1.5.6**, and bumps all dependent plugins to their respective `.14` patch versions. It delivers a broad set of new capabilities across MCP authentication, key rotation, OTel metrics, Bedrock/Anthropic compatibility, and UI improvements, alongside a number of targeted bug fixes and refactors.

## Changes

- **Direct API Key Header** — Providers can now receive an API key passed directly via a request header (#3817)
- **MCP Per-User Auth** — Introduced `MCPCredentialStore` abstraction, per-user MCP credential reconciliation, and a new per-user header auth type with lazy-auth submission flow (#3656, #3702, #3703, #3704, #3705)
- **MCP TLS Configuration** — Added configurable TLS (`insecureSkipVerify`, `caCertPem`) for HTTP/SSE MCP client connections (#3779, #3783)
- **MCP Sessions Management** — Filter, search, and pagination on the MCP sessions list API and table, plus a `can_reauth` identity gate (#3823, #3824, #3825)
- **Key Rotation** — Keys now rotate on 401/402/403 responses; returns `502 upstream_credentials_exhausted` when all keys are permanently exhausted. Added `triggered_rotation` to `KeyAttemptRecord` and tightened `bifrost_key_rotation_events_total` semantics (#3430, #3491)
- **OTel Metrics** — Added OTel spec-compatible metrics (backward compatible) with provider cache and semantic cache attributes in metrics export (#3865, #3816)
- **Opus 4.8 Support** — System message handling and general compatibility for Opus 4.8 (#3868, #3878)
- **Dimension Rankings** — New `GetDimensionRankings` API and dashboard tabs for team, customer, BU, and user rankings (#3766)
- **Model Pricing Attributes** — `additional_attributes` field on model pricing rows with management API and UI editor (#3829)
- **Prompt Cache Retention** — Added prompt cache retention parameter on responses requests (#3810)
- **Tool Call Execution UI** — Inline tool-call execution, stop streaming, bulk execute/submit, and a redesigned tool-call UI (#3837, #3843)
- **Sheet Navigation** — Prev/next keyboard navigation and URL state across virtual key, MCP client, and routing rule sheets (#3739, #3740, #3744, #3745)
- **Bedrock Tool Name Truncation** — Truncate Bedrock function/tool names to the provider length limit
- **Bedrock Guardrails** — Set guardrail config in Bedrock requests built from responses (#3862)
- **Anthropic Tool Use** — Default `tool_use` input to `{}` when arguments are absent (#3880)
- **Responses Streaming** — Fixed responses stream events (#3838)
- **Compat Flow** — Fixed missing parameter parsing on the compat flow (#3881)
- **Passthrough API Version** — Set a default API version in passthrough requests as a fallback (#3853)
- **Virtual Key Updates** — Avoid overriding optional fields during virtual key update (#3855)
- **User-Mode Flows** — Gate user-mode flows on caller `user_id`, skip temp token mint, and unify flow/credential kind filtering for pending flows (#3841, #3859)
- **Partial Tool Calls** — Handle partial tool call execution failures and return successful results (#3849)
- **URL Query Escaping** — Support escaped characters in URL query parameters (#3826)
- **MCP Auth Errors** — Inline banner and retry support for MCP auth-required errors (#3856)
- **Renamed Resolvers** — `staticHeadersResolver`/`serverOAuthResolver` renamed to `sharedHeadersResolver`/`sharedOAuthResolver` (#3840)
- **Starlark Nested Tool Calls** — Exposed `RunWithPluginPipeline` on `ClientManager` and routed Starlark nested tool calls through the canonical plugin gate (#3794)
- **Deferred-Fill OAuth Removed** — Removed deferred-fill user-mode OAuth flow support (#3839)
- **Go 1.26.3** — Upgraded toolchain to Go 1.26.3 (#3782)

## Type of change

- [x] Bug fix
- [x] Feature
- [x] Refactor
- [ ] Documentation
- [x] Chore/CI

## Affected areas

- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [x] UI (React)
- [ ] Docs

## How to test

```sh
# Core/Transports
go version  # should report go1.26.3
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```

- Validate MCP per-user auth by configuring a per-user header auth type and confirming credentials are stored and reconciled on virtual key and MCP client changes.
- Validate key rotation by triggering a 401/402/403 from an upstream provider and confirming rotation occurs; exhaust all keys and confirm a `502 upstream_credentials_exhausted` is returned.
- Validate OTel metrics output includes `provider_cache` and `semantic_cache` attributes.
- Validate Bedrock requests with tool names exceeding the provider limit are truncated correctly.
- Validate Opus 4.8 system message handling by sending a request with a system message to an Opus 4.8 endpoint.

## Breaking changes

- [x] Yes
- [ ] No

The deferred-fill user-mode OAuth flow has been removed (#3839). Any integrations relying on that flow must migrate to the new per-user credential store approach. The `staticHeadersResolver` and `serverOAuthResolver` identifiers have been renamed to `sharedHeadersResolver` and `sharedOAuthResolver` respectively (#3840); any direct references must be updated.

## Related issues

#3817, #3656, #3702, #3703, #3704, #3705, #3779, #3783, #3823, #3824, #3825, #3430, #3491, #3865, #3816, #3868, #3878, #3766, #3829, #3810, #3837, #3843, #3739, #3740, #3744, #3745, #3862, #3880, #3838, #3881, #3853, #3855, #3841, #3859, #3849, #3826, #3856, #3840, #3794, #3839, #3782, #3724, #3814, #3836, #3869, #3886

## Security considerations

- MCP per-user credentials are stored via the new `MCPCredentialStore` abstraction; ensure the backing store is appropriately access-controlled and that credential values are encrypted at rest.
- The direct API key header feature passes provider secrets via HTTP headers; ensure TLS is enforced on all ingress paths and that headers are not logged in plaintext.
- User-mode flows are now gated on `caller user_id` and temp token minting is skipped where appropriate, reducing the surface for privilege escalation.
- TLS configuration for MCP HTTP/SSE connections supports `insecureSkipVerify`; this should only be enabled in controlled environments.

## Checklist

- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants