feat(mcp): dcr_bridge authorize and token relay redirect handling with mandatory S256 - #32747
Conversation
f937dd6 to
306a687
Compare
Greptile SummaryThis PR adds the DCR-bridge authorize and token relay logic: for servers with
Confidence Score: 5/5Safe to merge; the relay-arm and short-circuit-arm branching is correctly implemented and well-tested. The relay arm correctly avoids gateway custody of authorization codes, S256 PKCE enforcement is well-tested across all four downgrade paths, and the short-circuit arm's existing trust gate is untouched. All 11 tests are new and additive with no weakening of prior coverage. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py | Adds three helper functions and branches both authorize_with_server and exchange_token_with_server on the dcr_bridge relay arm vs short-circuit arm; the security properties are correctly implemented. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py | Adds 11 new tests covering verbatim passthrough, PKCE downgrade rejections, short-circuit redirect trust, pre-bridge regression pin, and token relay fields; all new, no existing tests modified. |
Reviews (4): Last reviewed commit: "docs(mcp): note S256 enforcement fires o..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
On the third finding (bridge_challenge/bridge_method unused on the short-circuit arm): that is the deliberate shape of the narrowing helper. _require_s256_pkce both enforces and narrows; the relay arm consumes the narrowed pair, while the short-circuit arm calls it purely for enforcement and continues into the /callback flow with the original Optional values, which the downstream encode-state path accepts. Splitting enforcement from narrowing would mean either two helpers with one call site each or a cast, which the type-discipline gate rejects. The double _dcr_bridge_relays_client_registration call and the detail-shape inconsistency are fixed in the follow-up commit |
733cf1e to
c79dcef
Compare
|
bugbot run |
|
@greptileai rereview |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c79dcef. Configure here.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 195f145. Configure here.
Relevant issues
Linear ticket
Part of LIT-4337 (PR 2 of the stack, based on #32745 so the review diff is only this change; GitHub retargets to litellm_internal_staging when #32745 merges). The discovery facade and register relay that make these paths discoverable land in PR 3
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Live proxy from this branch on localhost:4010 (backed by real Postgres, with the dcr_bridge column from #32745 now on staging), a true_passthrough server with dcr_bridge on pointing at the real Linear MCP upstream. These curls drive the authorize and token relays this PR changes; the redirect target is Linear's real authorization server
The complete interactive flow (MCP Inspector and Claude Desktop finishing DCR sign-in through the gateway) exercises these same endpoints and is proven end to end in the discovery-facade PR that makes them discoverable. Unit coverage: 11 bridge-specific tests plus the full discoverable-endpoints file at 123 passing, with the flag-off relay behavior pinned byte-identical
Type
🆕 New Feature
Changes
This is the isolated security diff of the DCR-bridge stack: how the gateway's authorize and token relays handle the client redirect target for
dcr_bridgeservers. Keeping it to one reviewable change is deliberate because the redirect target is exactly the VERIA-57 code-theft surfaceThe design splits by whether the gateway holds an upstream client registration. In the relay arm (the server has a
registration_urland no admin-configuredclient_id, so client registration is relayed to the upstream AS), the authorize relay passes the client'sclient_id,redirect_uri,state, and PKCE parameters through verbatim and sets no state cookie; the authorization code returns from the upstream directly to the client's own redirect URI and never transits the gateway, and the upstream AS enforces the redirect binding it recorded at registration. The token relay correspondingly posts the client'sredirect_uri(required, matching the authorize leg) instead of the gateway callback. Because the gateway never has custody of the code in this arm, no gateway-side redirect trust decision exists to make, which is a strictly smaller attack surface than validating and forwarding codes to third-party redirect targetsIn the short-circuit arm (an admin-configured or persisted OAuth client, so the upstream only knows the gateway's own callback), the existing
/callbackencrypted-state relay andvalidate_trusted_redirect_urigate (same-origin, loopback,MCP_TRUSTED_REDIRECT_ORIGINSops allowlist) apply unchanged; ops opt public clients in through the existing allowlist rather than any new mechanismOn both arms,
dcr_bridgeservers require PKCE withcode_challenge_method=S256at the gateway (RFC 7636 defaults a missing method to plain, so the method must be explicit); these servers serve unauthenticated public clients, so the downgrade paths are rejected here rather than trusting upstream enforcement. The dashboard's browser Authorize already sends S256 so it is unaffected, and servers withoutdcr_bridgekeep the pre-bridge contract byte for byte (pinned by tests)Tests cover the verbatim passthrough contract for both client-forwarded auth types, the four PKCE downgrade rejections, the short-circuit arm keeping callback plus redirect trust, the no-flag regression pins for authorize and token, and the token relay's redirect_uri requirement and upstream form fields
Note
High Risk
Changes OAuth authorize/token redirect handling on a security-sensitive surface (VERIA-57 code-theft); relay arm bypasses gateway redirect trust by design, so correctness of arm detection and PKCE enforcement matters.
Overview
For MCP servers with
dcr_bridge, authorize and token handling now split into two paths instead of always forcing the gateway/callbackrelay.Relay arm (upstream DCR via
registration_url, no adminclient_id):authorize_with_serverredirects to the upstream IdP with the client’sclient_id,redirect_uri,state, and PKCE unchanged—no OAuth state cookie and no gateway callback.exchange_token_with_serverposts the client’sredirect_uri(required) to the upstream token endpoint instead of the proxy callback.Short-circuit arm (admin or persisted OAuth client): existing
validate_trusted_redirect_uri, encrypted state, and/callbackbehavior is unchanged after mandatory S256 PKCE.All
dcr_bridgeservers must sendcode_challengewithcode_challenge_method=S256at the gateway; missing or non-S256 PKCE returns 400. Servers withoutdcr_bridgekeep prior behavior (covered by regression tests).Eleven new unit tests pin relay passthrough, PKCE rejections, short-circuit redirect trust, and token form fields.
Reviewed by Cursor Bugbot for commit 195f145. Bugbot is set up for automated code reviews on this repo. Configure here.