Skip to content

fix(mcp): deny the interactive dcr_bridge authorize for a user without server access - #37865

Merged
tin-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_dcr_bridge_v2_resolver
Aug 21, 2026
Merged

fix(mcp): deny the interactive dcr_bridge authorize for a user without server access#37865
tin-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_dcr_bridge_v2_resolver

Conversation

@tin-berri

@tin-berri tin-berri commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

How it solves it:

  • The interactive bridge authorize now checks server access for the signed-in user
  • Same reload and same allowed-servers resolver MCP egress uses, nothing mirrored
  • Denied users get an RFC 6749 access_denied redirect at connect time

User Flow

Before: an operator adds their gateway's dcr_bridge MCP server as a claude.ai custom connector and ends up connected with no tools and no clue why

  1. They add https://litellm-domain/mcp/example_bridge as a custom connector and click connect
  2. The browser walks register, sign-in, upstream OAuth, and callback; every leg returns 200
  3. claude.ai shows the connector as Connected
  4. The tools list is empty ("This connector has no tools available") and the upstream MCP server's own logs show it never received a request

After: the same operator finds out at connect time, with an error naming the fix

  1. They add the same connector and click connect
  2. After sign-in, the flow immediately returns to claude.ai with error=access_denied and the description "the signed-in user has no access to MCP server 'example_bridge' on this gateway; grant it through a team or user object permission, or mark the server allow_all_keys"
  3. claude.ai shows the authorization as failed instead of pretending to be connected
  4. Once the operator grants the user the server (or sets allow_all_keys: true), the same connect flow completes and tools list and run

Relevant issues

Addresses the silent failure behind #36358. The issue's traced mechanism (the v2 resolver discarding the admission-injected credential in _create_mcp_client) does not reproduce: on the reporter's own version (v1.97.0-rc.1) and on current staging, a user who does have a grant gets the unsealed upstream credential delivered correctly through the v2 PassthroughConfig path, proven live below. The observed symptom (connected, zero tools, upstream session count 0) comes from the allowed-servers scope filter failing closed for a bridge-admitted user with no litellm-side grant, silently

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • The handful of test files covering my change pass locally, e.g. uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*, make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more
  • My PR passes all required CI/CD checks (e.g., lint, schema.d.ts sync check, etc.)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Screenshots / Proof of Fix

Shared setup: proxy on localhost:4610 backed by Postgres, a stub upstream MCP server on :8933 that 401s unless it receives Authorization: Bearer real_upstream_token_12345 and counts every authed request, and a stub IdP on :8934 whose /token returns that bearer as access_token. Three DB-defined MCP servers, all auth_type: oauth_delegate, dcr_bridge: true, pointed at the stub upstream: bridge_gated (signed-in user has no grant), example_bridge (user granted via object permission), bridge_open (allow_all_keys: true). The walk simulates an external DCR client: POST /login, POST /{server}/register, GET /{server}/authorize following redirects, POST /{server}/token, then MCP initialize and tools/list against /mcp/{server} with the minted envelope bearer and no cookies

Before (40b8300)

no grant: every leg 200, connected, silently zero tools

  1. GET /example_bridge/authorize?... returns 307 to the stub IdP, the callback returns 302 with a gateway code, POST /example_bridge/token returns 200 with {"access_token": "llm_env_...", "token_type": "Bearer"}
  2. MCP initialize with that bearer returns 200, tools/list returns {"jsonrpc":"2.0","id":2,"result":{"tools":[]}}
  3. Stub upstream stats: {"authed_requests": 0, "unauthed_requests": 0, "last_auth_header": null}, the upstream was never contacted

with grant: works, proving the credential path was never the bug

  1. Same walk after POST /user/update with {"object_permission": {"mcp_servers": ["example_bridge"]}}
  2. tools/list returns the example_bridge-echo tool
  3. Stub upstream stats: {"authed_requests": 3, ..., "last_auth_header": "Bearer real_upstream_token_12345"}

After (f9ecf18)

no grant: denied loudly at connect time

  1. GET /bridge_gated/authorize?...&state=probe_state_xyz returns 302 straight back to the client: http://127.0.0.1:9777/callback?error=access_denied&error_description=the+signed-in+user+has+no+access+to+MCP+server+%27bridge_gated%27...&state=probe_state_xyz
  2. The upstream IdP is never engaged for this flow and no envelope is minted

with grant: byte-identical success

  1. Same walk against example_bridge completes; tools/list returns example_bridge-echo
  2. Stub upstream stats advanced by exactly the three expected authed requests, last_auth_header: "Bearer real_upstream_token_12345"

allow_all_keys: byte-identical success

  1. Same walk against bridge_open completes; tools/list returns bridge_open-echo
  2. tools/call on bridge_open-echo with {"text": "proof-36358"} returns "echo: proof-36358" through the gateway

Type

🐛 Bug Fix

Caveats (if any)

  • Promotes reload_admitted_user to public per review: it already had a cross-module consumer in ui_session_utils, and the gate adds a second; the freed reportPrivateUsage budget is ratcheted down

  • Key-hash-subject envelopes never pass this authorize leg; key scoping already governs them

  • A grant revoked after mint still fail-closes silently at egress, as any scoped principal does

  • The relay bridge arm (true_passthrough) is deliberately ungated; egress admits it anonymously by design

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

@tin-berri

Copy link
Copy Markdown
Contributor Author

@greptileai can you reply here with a score and why that score?

@tin-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

4/5

The fix is targeted, correct, and well-tested. Here's the breakdown:

What works well:

  • The gate is inserted at exactly the right point — after session validation but before the upstream IdP redirect — so no credentials are minted and no external party is contacted for a denied user.
  • Uses the exact same _reload_admitted_user + get_allowed_mcp_servers path that egress uses, so the grant check is guaranteed to be consistent with what happens at tool-list/call time.
  • RFC 6749 access_denied redirect with state propagation is the correct protocol response; error_description names the fix.
  • The 5xx propagation vs. <5xx denial split in _bridge_authorize_access_denial is correct: a DB outage should stay retryable, not silently become an access denial.
  • The three test cases (user_can_reach_server parametrize, reload_status parametrize for 401/500/503, and the existing test's new bypass mock) cover the meaningful cases cleanly.

Why not 5/5:

  1. Private method call with suppressed pyright warning. MCPRequestHandler._reload_admitted_user is called from outside the class with # pyright: ignore[reportPrivateUsage]. The comment explains the coupling is intentional (you want the exact same constructor), but it's still a fragile design — a rename or signature change in _reload_admitted_user won't surface here statically. A thin package-internal helper (e.g., _admit_user_for_server_check) that both egress and this gate call would remove the smell without duplicating logic.

  2. CI is not passing per the pre-submission checklist. Until the required checks are green, the PR isn't mergeable regardless of review score.

Otherwise this is a clean, minimal, well-reasoned fix for the silent failure described in #36358.

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR prevents interactive bridge authorization from completing when the signed-in user lacks access to the requested MCP server

  • Reloads the admitted user and applies the existing allowed-server resolver before redirecting upstream
  • Returns an OAuth access_denied redirect for denied, unknown, or deactivated users while preserving server-side availability errors
  • Makes reload_admitted_user public and updates affected callers, tests, and the private-usage budget

Confidence Score: 5/5

The PR appears safe to merge

No blocking failure remains

Important Files Changed

Filename Overview
litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py Adds the interactive bridge access gate and OAuth denial redirect without leaving an eligible follow-up defect
litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py Promotes the admitted-user reload helper to a public method and updates internal callers
litellm/proxy/_experimental/mcp_server/ui_session_utils.py Updates the dashboard admitted-user context to use the renamed public helper
tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py Covers granted, denied, missing-user, and retryable-failure bridge authorization behavior
basedpyright-code-budget.json Ratchets the private-usage diagnostic budget after making the helper public

Reviews (3): Last reviewed commit: "fix(mcp): deny the interactive dcr_bridg..." | Re-trigger Greptile

Comment thread litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 f072fd2. Configure here.

@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@tin-berri

Copy link
Copy Markdown
Contributor Author

Both P2 threads answered: the gate runs once per interactive connect, and the test idiom matches this file's siblings. The lint red is the pre-existing test-tree PT011/PT012 pair, fix in flight on #37864. @greptileai can you re-score with that context?

@tin-berri
tin-berri force-pushed the litellm_dcr_bridge_v2_resolver branch from f072fd2 to bf4c892 Compare August 21, 2026 18:17
@tin-berri

Copy link
Copy Markdown
Contributor Author

@greptileai can you reply here with a score and why that score?

@tin-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

The PR fixes a real, verified bug with a well-scoped change and solid test coverage. Here's my assessment:

Score: 4/5

What's good:

  • The fix is correctly placed — checking access at authorize time rather than at tool-list time surfaces the failure where users can act on it
  • Uses the same _reload_admitted_user + get_allowed_mcp_servers path that egress uses, so the gate is consistent by construction
  • The 5xx propagation logic (retry-able availability faults don't collapse into an access denial) is correct
  • Test coverage is strong: parametrized happy/sad path, the reload-failure matrix (401→deny, 500/503→propagate), and the regression guard on the pre-existing test via the _bridge_authorize_access_denial mock

What keeps it at 4 rather than 5:

The reportPrivateUsage suppression on:

admitted: Final = await MCPRequestHandler._reload_admitted_user(  # pyright: ignore[reportPrivateUsage]

is a coupling signal. Calling a private class method across module boundaries from non-test code is a design smell — if _reload_admitted_user's signature or semantics shift, this callsite breaks silently. The right fix is to either promote a thin public wrapper (e.g. MCPRequestHandler.reload_admitted_user) or extract the DB-reload logic into a shared helper both can import. Not a blocker, but it's the one rough edge in an otherwise clean PR.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 bf4c892. Configure here.

@tin-berri
tin-berri force-pushed the litellm_dcr_bridge_v2_resolver branch from bf4c892 to bedc287 Compare August 21, 2026 18:32
@tin-berri

Copy link
Copy Markdown
Contributor Author

@greptileai can you reply here with a score and why that score?

@tin-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Score: 4.5/5

Why:

What's good:

  • The fix is surgically placed — the access check runs at the exact right point in authorize_with_server, after the session cookie is resolved and before any upstream redirect or envelope minting.
  • Reuses the same reload_admitted_user + get_allowed_mcp_servers pair that MCP egress uses, so the gate is consistent by construction, not by approximation.
  • The 4xx-denies / 5xx-propagates split in _bridge_authorize_access_denial is correct. A DB outage should not masquerade as an access denial; a deactivated user should fail closed.
  • RFC 6749 §4.1.2.1 access_denied redirect with state echo is exactly the right wire format for an OAuth client to surface the failure.
  • The _reload_admitted_userreload_admitted_user rename (making it non-private) is a clean prerequisite rather than a workaround — the authorize path is outside MCPRequestHandler and needed the public surface.
  • Test coverage is solid: the parametrized happy/sad path test, the 3-status reload-failure test, and the updated monkeypatches throughout.

Why not 5/5 — one thing to verify:

The existing test_interactive_bridge_authorize_with_session_redirects_to_upstream now patches _bridge_authorize_access_denial to always return None. That's fine for the test's intent (it's testing the state-encoding leg), but it means the test no longer exercises the real access check. If the denial function's interface ever changes, this mock will silently diverge. A small comment explaining why the mock is there (e.g. # grant assumed; test focuses on state encoding) would help future readers, but it's a minor documentation gap, not a correctness issue.

Everything else — the caveats about key-hash subjects, the relay-bridge arm being ungated, and the post-mint revocation window — is correctly documented and handled. The implementation is correct.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 bedc287. Configure here.

…t server access

The dcr_bridge oauth_delegate connect flow completed for a signed-in user
with no litellm-side grant to the target server: every leg returned 200,
the DCR client showed connected, and tools/list then fail-closed to an
empty list with the upstream never contacted (#36358). The authorize leg
now admits the user the way MCP egress will (same reload_admitted_user
constructor, same get_allowed_mcp_servers resolver) and refuses with an
RFC 6749 access_denied redirect naming the remedy, before any upstream
OAuth runs or an envelope is minted. Availability faults (5xx) propagate;
unknown or deactivated users deny fail-closed

Promotes MCPRequestHandler reload_admitted_user to public: it already had
a cross-module consumer in ui_session_utils, and this gate adds a second,
so the private name no longer reflected its use. Ratchets the freed
reportPrivateUsage budget headroom down
@tin-berri
tin-berri force-pushed the litellm_dcr_bridge_v2_resolver branch from bedc287 to f9ecf18 Compare August 21, 2026 18:40
@tin-berri

Copy link
Copy Markdown
Contributor Author

@greptileai can you reply here with a score and why that score?

@tin-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Score: 4.5/5

Why:

Strengths:

  • The fix is well-targeted: it addresses the exact silent-failure path (connected with zero tools, no explanation) by adding the access check at the right choke point — the interactive authorize redirect, before any envelope is minted.
  • Reuses the same reload_admitted_user + get_allowed_mcp_servers path that MCP egress already uses, so the gate is consistent with runtime access control rather than a separate policy that could drift.
  • RFC 6749 access_denied redirect is the correct protocol response; it carries state back to the client, which is required.
  • The 5xx-vs-4xx distinction in _bridge_authorize_access_denial (reraise on 5xx, deny on 4xx) is the right fail-safe: DB outages stay retryable, unknown/deactivated users deny.
  • Promoting _reload_admitted_user to reload_admitted_user is a clean correctness fix — it already had two cross-module callers, keeping it private was a fiction.
  • Test coverage is thorough: parametrized grant/no-grant, 401/500/503 reload failures, state passthrough, upstream redirect check.

Minor concerns keeping it from 5/5:

  • _bridge_authorize_access_denial is defined in discoverable_endpoints.py but the deny-redirect logic (_bridge_access_denied_redirect) and the admission check belong conceptually together with the auth module. The split is workable but slightly increases the surface to audit when the access policy changes.
  • The finally: global_mcp_server_manager.registry.clear() in tests relies on direct mutation of a global singleton. If a test fails before the try block and another test runs, it could leave stale state. A fixture or addCleanup would be safer.
  • The **({"state": state} if state else {}) conditional in _bridge_access_denied_redirect is subtly load-bearing — empty string vs None behave differently. A comment or explicit if state is not None would be clearer.

Overall this is a solid, well-reasoned fix with good evidence of correctness and appropriate test coverage. The approach is conservative (reusing existing resolvers, fail-closed on errors) which is exactly right for an auth path.

@tin-berri

Copy link
Copy Markdown
Contributor Author

All three are deliberate: redirect shaping lives with the endpoint while the access decision is delegated; the registry try/finally and the state echo match sibling idioms

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 f9ecf18. Configure here.

@tin-berri

Copy link
Copy Markdown
Contributor Author

@greptileai review the latest commit please

@tin-berri
tin-berri enabled auto-merge (squash) August 21, 2026 19:21
@codspeed-hq

codspeed-hq Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing litellm_dcr_bridge_v2_resolver (f9ecf18) with litellm_internal_staging (0a5fa4f)

Open in CodSpeed

@tin-berri
tin-berri merged commit 6a75bbd into litellm_internal_staging Aug 21, 2026
75 of 76 checks passed
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.

2 participants