fix(transports,framework): tighten mcp oauth2 server validation (#7851) - #7875
Merged
Merged
Conversation
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (15)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughMCP OAuth configuration now requires an issuer in OAuth-enabled modes. Issuance endpoints are gated by auth mode and validate input sizes. Session consent now requires a signed-in identity under the documented conditions. Tests and documentation cover these rules. ChangesMCP OAuth behavior
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant ConsentFlow
participant IdentityCheck
Browser->>ConsentFlow: Request available consent modes
ConsentFlow->>IdentityCheck: Check request identity
IdentityCheck-->>ConsentFlow: Return identity status
ConsentFlow-->>Browser: Return available modes
Browser->>ConsentFlow: Submit session consent
ConsentFlow->>IdentityCheck: Verify consenting identity
IdentityCheck-->>ConsentFlow: Return identity status
ConsentFlow-->>Browser: Return 401 if identity is absent
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue was established in the reviewed changes; the PR is ready for normal checks before merging through the stack. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description contains only the repository template and placeholder text. It does not explain the changes, identify affected areas, provide test results, or address the breaking change and security considerations. Resolution Replace the placeholder text with a completed description. Summarize the OAuth2 validation and authorization changes, select the applicable change types and affected areas, provide actual test steps and results, describe the issuer_url migration and breaking change, address security considerations, and complete the checklist. Mark non-applicable sections as not applicable. Full details: Docstring CoverageExplanation Docstring coverage is 65.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 8 files. (6 skipped: 6 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This was referenced Oct 3, 2026
Contributor
Author
This was referenced Oct 3, 2026
fix(core,transports): bound request decompression and harden realtime request handling (#7856)
#7880
Merged
akshaydeo
marked this pull request as ready for review
October 3, 2026 04:28
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
akshaydeo
force-pushed
the
backport/mcp-oauth2-server
branch
from
October 3, 2026 06:29
8211deb to
bf39499
Compare
akshaydeo
force-pushed
the
backport/provider-endpoint-updates
branch
from
October 3, 2026 06:29
46be3ee to
1b176c5
Compare
Contributor
Author
Merge activity
|
akshaydeo
changed the base branch from
backport/provider-endpoint-updates
to
graphite-base/7875
October 3, 2026 08:52
akshaydeo
dismissed
coderabbitai[bot]’s stale review
October 3, 2026 08:54
The base branch was changed.
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Hardens the Bifrost MCP OAuth2 authorization server against a Host-header poisoning attack: when discovery is enabled, every issuer reference (discovery documents, authorize redirect, JWT `iss`/`aud`) was previously derived from the unauthenticated, per-request `Host` header when `issuer_url` was not explicitly set. An attacker who could reach the always-public `/.well-known/` endpoints could spoof the `issuer`, `token_endpoint`, and `jwks_uri` that MCP clients trust. `issuer_url` is now required whenever `mcp_server_auth_mode` is `oauth` or `both`, enforced at config load time, at `PUT /api/config`, and in the JSON schema. Additionally, the anonymous "session" consent identity is now gated on the consenting party being authenticated (verified dashboard session or SSO/IdP user), and the issuance endpoints (`/oauth2/register`, `/oauth2/authorize`, `/oauth2/token`) now return `404` in `headers` mode, matching the discovery endpoints. Input bounds are enforced on free-text DCR and authorize fields before any state is written, and all discovery responses carry `Cache-Control: no-store`. - **`issuer_url` is now required** when `mcp_server_auth_mode` is `oauth` or `both`. `validateClientConfig` rejects configs missing it at load time; `updateConfig` rejects the write at the API layer. The JSON schema enforces this with an `allOf`/`if`/`then` constraint. The fallback to `BuildBaseURL(request)` is retained only in `headers` mode, where no OAuth issuer identity is served. - **`oauth2IssuerURL` never derives the issuer from the request `Host` header while discovery is enabled.** If the invariant is bypassed, it returns `""` (yielding relative URLs) rather than an attacker-chosen absolute one. - **Discovery responses are sent with `Cache-Control: no-store`** so a CDN or reverse proxy in front of Bifrost never serves a cached copy. - **Issuance endpoints 404 in `headers` mode.** `/oauth2/register`, `/oauth2/authorize`, and `/oauth2/token` now check `issuanceEnabled()` (same predicate as the discovery handler) and return `404` when MCP OAuth is off, so disabling discovery disables the whole authorization server. - **Session consent requires an authenticated consenting user.** `flowSubmit` returns `401` if `mode=session` is submitted without a verified dashboard session or IdP identity. `flowDetail` omits `session` from `available_modes` for unauthenticated callers. The auth-disabled bypass (`BifrostContextKeyAuthBypassed`) explicitly does not count as being signed in. - **Input bounds on DCR and authorize parameters** are enforced before any row is written: `client_name` ≤ 2048 bytes, `scope` ≤ 4096 bytes, at most 32 `redirect_uris` of ≤ 2048 bytes each, `state` ≤ 8192 bytes, `code_challenge` ≤ 128 bytes. Oversize requests are refused with `400` before touching the store. - **Documentation updated** to reflect that `issuer_url` is required (not optional), that `headers` mode 404s the issuance endpoints, that session mode requires a signed-in consenter, and to add troubleshooting entries for the new `401` and the `issuer_url must be set` rejection. - **E2E and unit tests added** covering: oversize `client_name` refusal, anonymous session-mode `401`, auth-bypass not counting as identity, VK consent completing normally, discovery document pinning the configured issuer regardless of `Host`, `Cache-Control: no-store` on all three discovery documents, issuance endpoints 404ing in `headers` mode, all DCR/authorize oversize-field cases, and `issuer_url`-required validation at both load time and the config API. - [x] Bug fix - [x] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ```sh go test ./transports/bifrost-http/handlers/... ./transports/bifrost-http/lib/... ``` Key scenarios to validate manually: 1. Start Bifrost with `mcp_server_auth_mode: oauth` and no `issuer_url` — boot must fail with `issuer_url must be set`. 2. `PUT /api/config` switching to `oauth` or `both` without `issuer_url` must return `400`. 3. In `headers` mode, `GET /.well-known/oauth-authorization-server`, `POST /oauth2/register`, `GET /oauth2/authorize`, and `POST /oauth2/token` must all return `404`. 4. In `both`/`oauth` mode with a spoofed `Host: evil.example` header, the discovery document's `issuer`, `token_endpoint`, and `jwks_uri` must reflect the configured `issuer_url`, not `evil.example`. The response must include `Cache-Control: no-store`. 5. Open a consent flow without a dashboard session — `available_modes` must not include `session`. Submitting `mode=session` must return `401` with `authenticated consenting user` in the body. 6. Open a consent flow with a valid dashboard session — `session` appears in `available_modes` and submitting it succeeds. 7. `POST /oauth2/register` with a 2049-char `client_name` must return `400 invalid_client_metadata`. - [x] Yes - [ ] No `issuer_url` is now required whenever `mcp_server_auth_mode` is `oauth` or `both`. Existing deployments using either mode without a pinned `issuer_url` will fail to start after this change. **Migration:** add `oauth2_server_config.issuer_url` set to Bifrost's stable public URL to `config.json` (or via `PUT /api/config`) before upgrading. This PR directly addresses a Host-header injection vulnerability in the OAuth2 discovery surface. The `/.well-known/` endpoints are always public (pre-auth), so an attacker reachable to those endpoints could previously poison the `issuer`, `token_endpoint`, and `jwks_uri` values that MCP clients cache and trust, enabling token substitution or redirect attacks. The fix makes `issuer_url` a hard requirement and removes the per-request fallback entirely while OAuth is enabled. The `Cache-Control: no-store` addition prevents a CDN from amplifying a poisoned response. The session-mode identity gate prevents an unauthenticated party who receives a consent link from minting themselves a valid `/mcp` token with no identity attached. - [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
akshaydeo
force-pushed
the
backport/mcp-oauth2-server
branch
from
October 3, 2026 08:55
bf39499 to
2a7aea3
Compare
This branch was successfully deployed
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
1 participant
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
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines