* test: drop the cwd-relative sys.path.insert calls from the test suite
TQ003 stands at 1,077 across 1,058 files, and 1,015 of them are the same shape:
sys.path.insert(0, os.path.abspath("../..")) and its deeper siblings. The
argument resolves against the working directory rather than the file, so from
the repo root, where every job runs pytest, it inserts the directory two levels
above the checkout. It has never pointed at litellm. The package is installed
into the environment anyway, which is what actually makes the import work, and
what the rule's message has said all along.
Removing them leaves 1,634 imports of sys and os with no remaining reference,
and those go too, except where another test module imports the name back out of
the file. The rest of TQ003 is 62 call sites that resolve against __file__ or a
variable, which are a different question and are left alone.
Collection is identical either way: 45,871 tests and the same 51 pre-existing
collection errors before and after, and ruff reports no new undefined name.
* test: drop the duplicate imports the sys.path sweep exposed to F811
* test(pre-call-utils): restore the os import the new bedrock tests need
* fix(mcp): resolve admin OAuth sessions to the same server set the connect page shows
* fix(mcp): bind admitted admin rows through the entitlement ceiling, not the credential scope clause
* fix(mcp): strip root_path before matching the per-server MCP route spelling
The 401 challenge for a gateway-managed oauth2 MCP server advertises the
protected-resource metadata URL in the spelling the client connected on, so a
strict RFC 9728 section 3 client lands on a document whose `resource` equals the
URL it actually called. That spelling test compared `_original_path` against the
root-relative `/{server}/mcp` shape, but `_original_path` and `scope["path"]`
are raw request-line paths that still carry the deployment's `root_path`
On a SERVER_ROOT_PATH deployment the prefix therefore made the legacy test fail
and every request fell through to the standard `/mcp/{server}` branch. A client
connecting on `/litellm/github/mcp` was pointed at the standard-pattern
document, which serves `resource = {base}/litellm/mcp/github`; that is not the
URL the client called, so a strict client aborts discovery before the MCP
request fires
Route the path through `get_route_relative_request_path` first, which removes
`root_path` on a segment boundary the same way
`litellm.proxy.auth.auth_utils.get_request_route` already does for the rest of
the MCP auth path, so `/litellmfoo` is not truncated under `root_path=/litellm`
* fix(mcp): make the gateway-managed 401 challenge root-path aware
The gateway-managed authorization_code challenge in process_mcp_request
built its AS-metadata URL from two root-path-unaware pieces:
- it matched the caller's spelling against `scope["_original_path"]`, a
raw request-line path that still carries the deployment prefix, so on a
SERVER_ROOT_PATH deployment the `/mcp/{server}` branch never matched and
every request fell through to the legacy one-segment form
- it hardcoded `/.well-known/oauth-authorization-server` without the
root-path segment the discovery route decorators bake in, so the URL
404'd under a sub-path deployment regardless of which branch was taken
Route the spelling match through get_route_relative_request_path and the
well-known root through well_known_root_suffix, the same two helpers the
discovery route registrations derive their paths from, so the advertised
URL cannot drift from the route that serves it.
Root-mounted deployments are unaffected: both helpers are no-ops when
SERVER_ROOT_PATH is unset.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
A test that asserts on the error inside its own except block passes when the
call stops raising, because nothing runs the handler. That is the exact case
the test exists to catch, so the regression lands green.
Rewrites all 111 such blocks into pytest.raises, which fails when the call
succeeds, and selects PT017 in ruff-tests.toml so no new one lands.
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
tests/litellm/ was a second mirror beside tests/test_litellm/ that no workflow,
Makefile target, or CircleCI job ever named. Its other 33 files were reconciled
during August 2026; this one stayed behind under a ci-coverage-allowlist entry
asking a later pass to decide which of its five orphan behaviours still hold.
They no longer hold as written: 25 of its 32 cases fail against today's code,
because the file froze on the day it stopped being collected and the endpoints
kept moving. Three of the five are already covered by the live twin, and better.
test_get_request_base_url_xff_trust_gate parametrizes the trust gate in both
directions, including the exact untrusted-caller case the orphan asserted, and
the standard and legacy protected-resource shapes are both exercised through
use_standard_pattern.
The other two were the only tests anywhere for validate_trusted_redirect_uri
under that same gate, so they are ported rather than dropped, rebuilt on the
live file's request-mock conventions. Both directions are load-bearing: forcing
is_request_from_trusted_proxy to True fails the untrusted case, forcing it to
False fails the trusted one.
313 tests pass in the live file, up from 311. Dropping the dead file clears one
zero-assert TQ001 violation, so its ceiling ratchets down with it.
`pytest.raises(Exception)` with no `match=` passes on any error that broad. A
TypeError from a refactor, a botched fixture, an import that moved: all of them
read as the rejection the test claims to police, so the test goes green for the
wrong reason and stays green after the behaviour it guards is gone.
PT011 closes that gap for the 317 sites B017 could not reach, because B017 only
fires on a single-statement body with no `as e` binding. Each pattern here is the
message the code actually raised, recorded by running the sites under a plugin
that logged the concrete type and text per call site, so the assertions describe
observed behaviour rather than a guess. Where a site raises more than one message
across its parametrize cases, the pattern is an alternation of what was seen;
where the exception carries an empty `str()` and puts the text on `.message`, the
site keeps a narrow `noqa` with the reason.
PT014 removes four parametrize cases that were listed twice. The duplicate re-runs
an assertion that already passed, and it usually marks a case someone meant to
vary and forgot to edit.
* test(lint): ban blind pytest.raises(Exception) with ruff B017
A bare pytest.raises(Exception) accepts whatever the body throws. The TypeError
a refactor introduces satisfies it exactly as well as the rejection the test was
written for, so the crash reads as a pass and the test never goes red.
All 111 existing sites are narrowed here. A runtime probe recorded the concrete
exception each one actually catches, and each site now names that type. Where
the code under test genuinely raises a bare Exception, the site pins a stable
slice of the message with match= instead.
Two sites tell on themselves. The shared responses-API cancel test raises
"custom_llm_provider is required but passed as None" rather than talking to a
provider at all, because cancel_responses takes a provider, not a model. And
test_bedrock_guardrails_with_streaming was the only test in its file still
passing without AWS credentials, because the NoCredentialsError boto3 raised
long before the guardrail ran satisfied the blind raises.
* fix(test): widen the openai batch-dispatch assertion to OpenAIError
The narrowed NotFoundError only holds where OPENAI_API_KEY is set. Without one
the SDK raises OpenAIError while building the client, long before any 404, so CI
went red. OpenAIError covers both and still rejects a TypeError from a refactor.
store_user_oauth_credential refused to overwrite any existing row that did not
decode as an OAuth2 payload, which conflated two states: a live BYOK secret that
reads back as plaintext, and ciphertext written under a LITELLM_SALT_KEY the proxy
no longer holds. The second is unrecoverable by any caller, so refusing preserved
nothing and instead wedged the user out of the OAuth flow permanently, since
re-authorizing is their only recovery.
The guard now raises only when the existing value is genuinely readable. An
undecryptable row is logged and replaced by the newly authorized token.
Both read paths were equally silent: get_user_oauth_credential and
list_user_oauth_credentials (which backs the bulk prefetch) each dropped an
undecryptable row indistinguishably from "user never authorized", so an operator
saw an upstream 401 and no hint that a credential had failed to decrypt. Both now
warn with the user and server ids, never the stored value.
ruff.toml excludes tests/* from `ruff check`, so nothing has ever checked the
test tree for names that do not exist. That matters more in tests than in
product code: a NameError inside a test whose body is wrapped in
`except Exception: pass` is swallowed, and the test reports green forever.
Adds ruff-tests.toml selecting F821 alone, wired into the lint workflow and
`make lint-ruff`, and clears every existing violation:
- 4 tests interpolated an unbound `e` into a `pytest.fail` message reached only
on the failure path, so the NameError, not the assertion, is what ran.
test_llm_guard_error_raising is the worst: it passes today with content
safety disabled entirely. It now asserts the 400 and its detail body.
- 5 sites construct BaseExceptionGroup, a 3.11 builtin, in a tree that still
supports 3.10. Guarded behind the exceptiongroup backport that anyio already
pulls in below 3.11.
- 9 missing imports (json, openai, Any, Final, HTTPException), including one in
a helper that catches HTTPException by a name it never imported, so the
challenge path it exists to detect raises NameError instead.
- 5 annotations naming types imported inside the function body, hoisted to
module scope or TYPE_CHECKING.
- 2 blocks of dead code: everything after a pytest.fail in
test_claude_agent_sdk, and an unused helper in test_end_users calling a
function defined in a different module.
- 1 error-path f-string in the router-settings doc test that masked the real
FileNotFoundError behind a NameError.
Only F821 for now. Widening the select list means ratcheting thousands of
pre-existing findings, so rules go in one at a time with their violations
already fixed.
revoke_refresh_token discarded the single-use claim result, so a revocation that arrived while Redis was unreachable answered 200 and left the refresh token live. The token endpoint reported the same outage as invalid_grant "already used". The guard now reports first, replayed, or unavailable, and both endpoints answer 503 temporarily_unavailable for an outage (RFC 7009 section 2.2.1, RFC 6749 section 5.2), which the CLI surfaces as a one-line warning while keeping the key it has
The consent page offers the team picker, but a form posted without a team
sealed a teamless grant and the token endpoint minted an unscoped
credential for a team member, escaping the team attribution classic lite
login always applies. The minter now refuses such a grant on redemption
and refresh alike; memberships whose team rows are gone still count as no
team so they cannot lock a user out
The proxy's OAuth authorization server (dynamic registration, PKCE S256,
loopback redirects, single-use codes, refresh rotation) gains a proxy-API
audience: /authorize?resource=<proxy origin> renders a consent page with
team selection and /token mints the same per-user credential lite login
mints, so a native CLI can sign a user in through the system browser and
call /v1/* with user and team attribution. Adds GET /.well-known/litellm-cli-auth
as the versioned discovery contract for non-Python clients, POST /revoke
(RFC 7009) for logout, and lite login --pkce, lite logout, and
lite auth print-token on the CLI side. Proxy-API grants only ever redirect
to a loopback address and the server never picks a team on the user's behalf.
Fixes#37332
An OpenAPI-backed MCP tool whose upstream answered 401 came back as a
successful tool result carrying the upstream's rejection as its content, so a
caller saw {"error":"invalid_token"} presented as data and the gateway recorded
the request in its own spend log as call_mcp_tool | success.
Three layers each erased the outcome. The request function returned
response.text whatever the status, _handle_local_mcp_tool caught every exception
and returned it as ordinary TextContent, and both dispatch sites then stamped
isError=False unconditionally. Fixing only the first, which is the obvious fix,
changes nothing, because the two above it still map failure onto the
success-shaped value.
The status is now classified where the response is held: a 401 becomes
MCPUpstreamAuthError so the caller is told to re-authenticate, and every other
non-2xx becomes MCPOpenApiUpstreamError, which carries the status and drops the
upstream body rather than serving it as tool content. _handle_local_mcp_tool no
longer swallows, and the call_tool arm keeps the auth error's type. Nothing new
renders these: call_mcp_tool and call_tool_rest_api already turn them into an
isError result naming the status and into a real 401 with WWW-Authenticate, and
the OpenAPI path simply never reached them.
The result is now byte-identical to the regular MCP path for the same failure.
Both OpenAPI dispatch arms sourced the upstream credential only from the
deprecated global / BYOK mcp_auth_header and never from mcp_server_auth_headers,
so x-mcp-{alias}-authorization was silently dropped on spec_path servers and the
upstream API received no Authorization at all. The managed path already resolves
it through lookup_mcp_server_auth_in_headers, so the two had drifted.
_resolve_openapi_tool_auth now owns that resolution for both arms. A per-server
value is already a complete header value and is forwarded verbatim, while a BYOK
credential keeps its auth-type prefix, so the two are never conflated into
"Bearer Bearer <token>". The resolved credential is also handed to
resolve_openapi_upstream_auth, whose passthrough arm reads it through
_passthrough_token_from_mcp_auth_header and outranks the ContextVar.
server.py loses its inlined copy of the forwarded-header logic along with its
mcp_server is None guards, which are unreachable after the 503 raised above them.
Credit to the earlier analysis and approach in #33349, which this supersedes
against the current v2 credential resolver.
true_passthrough and oauth_delegate forward the caller's own bearer and mint
nothing, so their sessions consume no discovered OAuth endpoint. The discovery
completeness gate still failed them closed with a 503 raised before the upstream
was ever contacted, which the tools/list fan-out swallowed into HTTP 200 with an
empty tool list. Any upstream that publishes no RFC 9728 metadata, including
every OpenAPI-backed internal API, was permanently unusable.
A failed discovery is now fatal only to servers whose OAuth flow the gateway
runs itself. Discovery still runs for the forwarding modes, so /authorize,
/token and /register keep resolving their endpoints, and each keeps its own 400
when they are missing.
MCPServer.is_client_forwarded_token now owns the mode pair that five call sites
spelled inline, one of which had already named it is_client_forwarded_mode.
* fix(mcp): scope authorization server issuer
Generated with AI
Co-Authored-By: Claude Code <noreply@anthropic.com>
* fix(mcp): keep the bare-origin issuer when no server was named
The scoped issuer must key off whether the request actually carried a server
name. _build_oauth_authorization_server_response rebinds mcp_server_name when
root discovery resolves the single configured OAuth2 server, so gating on the
rebound value also scoped /.well-known/openid-configuration, whose document is
served from the bare origin and whose issuer must stay the bare origin
Adds the named-server regression test for the reported mismatch, restores the
bare-origin assertion, and covers the OIDC document
* test(mcp): type the delegate_auth_to_upstream helper parameter
* refactor(mcp): bind the discovery issuer to a local before building the response
---------
Co-authored-by: Irosh <15094153+irosh-colombage-ZocDoc2@users.noreply.github.com>
Co-authored-by: Claude Code <noreply@anthropic.com>
Registry-swap reconciliation used bool(server.url) while registration uses _requires_oauth_discovery, dropping slots for issuer-anchored servers without a url. The preemptive 401 loop awaited discovery before the stamped client_credentials continue, so a deferred discovery failure could 503 requests whose challenge decision never reads metadata
* fix(mcp): drop caller host and configured upstream headers from logged metadata
The synthetic request that carries MCP client headers into
add_litellm_data_to_request forwarded the caller's Host header, and
Request.url is built from it, so a caller chose the proxy_server_request
url and the metadata endpoint that every logging callback records.
_upstream_credential_headers also only knew the configured client side
auth header and the x-mcp- prefix family, so a header name declared in
mcp_servers.<name>.extra_headers reached logging metadata in cleartext.
Those names are admin chosen, so no prefix rule can recognize them; read
them off the server registry instead. The header is still forwarded
upstream, which is what extra_headers is for. authorization is left out
because clean_headers already strips it and claiming it here would move
authenticated_with_header on the oauth passthrough config.
The Responses bridge tests stub the server manager, so their fakes gain
the registry accessor the sanitizer now reads.
* fix(mcp): drop caller host from the sanitized header mapping too
The synthetic request stopped forwarding host, but the parallel sanitizer
did not, so a forged hostname still reached the guardrail payload and the
list_tools spend row. Drop it there as well.
Exempt the configured identity headers from the upstream credential set.
get_user_from_headers resolves end user attribution off the same request
this module reconstructs, and it only fills end_user_id when auth left it
unset, so claiming user_header_name or a user_header_mappings name would
lose attribution on the MCP paths that authenticate upstream.
Drop the isinstance guard on extra_headers entries: the field is typed
list[str], so the check is dead and basedpyright scores it.
* fix(mcp): accept a bare user_header_mappings entry when exempting identity headers
get_internal_user_header_from_mapping and get_customer_user_header_from_mapping
both normalize a single mapping to a one element list, and config_settings.md
documents the key as a dict. Iterating the bare form yields its keys instead,
so the exemption silently matched nothing and an identity header also named in
an MCP server's extra_headers was dropped after all.
MCP tool calls run their guardrails against a throwaway LLM-shaped dict
built by `ProxyLogging._convert_mcp_to_llm_format`, not against the dict
the tool call is logged from. `@log_guardrail_information` therefore
appended `standard_logging_guardrail_information` to that throwaway
dict's metadata bucket, where `get_standard_logging_object_payload`
never saw it, so the Guardrails Monitor reported zero evaluations and
zero blocks for all MCP traffic.
Thread the request's `litellm_logging_obj` into `pre_call_tool_check`
and `_create_during_hook_task` and bridge the guardrail records onto it:
- Seed `data["litellm_logging_obj"]`, which unified guardrails read and
pass into `apply_guardrail`.
- Call `_sync_guardrail_info_to_logging_obj` in a `finally`, which is
what native guardrails need and what makes the block path work: a
blocked call raises straight out of `pre_call_tool_check`, so the
record has to be attached before the exception leaves the frame.
Only the guardrail evaluation records are copied. The synthetic
request's messages and tool arguments are deliberately left behind --
they can carry end-user data and nothing in the monitor needs them.
In `call_mcp_tool`, flush the failure handlers before
`post_call_failure_hook` so the `status="failure"` standard logging
object exists when `_ProxyDBLogger.async_post_call_failure_hook` writes
the spend-log row the monitor's "Total Blocked" counts. Both handlers
gate on `should_run_logging("sync_failure")` / `("async_failure")` and
then mark it, so the `@client` wrapper's own post-raise logging is a
no-op and nothing is double-counted -- the same pattern
`_fire_mcp_tool_call_logging` already uses for `isError=True`.
Threaded through every MCP tool entry point: the managed-server path,
the local-OpenAPI registry path, the legacy registry fallback, and the
Responses API's `_execute_tool_calls`.
* fix(mcp): expose client HTTP headers to logging callbacks and hooks
MCP protocol tool calls built a synthetic Request with only content-type, so metadata.headers reaching logging callbacks and guardrails was empty while /mcp-rest/tools/call exposed the full set. Rebuild the synthetic request from the connection's raw headers (shared with the sampling path), and pass sanitized headers to the pre-call hook, the MCP to LLM guardrail bridge and the Responses API MCP bridge. Credential headers stay masked and proxy key headers stripped.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
* fix(mcp): strip custom proxy key and upstream MCP credential headers from logging copies
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
* refactor(mcp): make client side auth header name accessor public
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
* fix(mcp): strip custom proxy key and client redaction opt-out from mcp headers
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
* fix(mcp): drop custom proxy key header in the synthetic request builder
Strips general_settings.litellm_key_header_name in build_synthetic_mcp_request so every caller, including sampling, is covered, and reverts passing general_settings into add_litellm_data_to_request on the tool call path since that also switches on enforced_params.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
---------
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: shivam <shivam@berri.ai>
An upstream that ends its response stream without a JSON-RPC reply leaves the
request pending forever. Tool discovery then only ended when an outer cancel
scope killed it, which logged a cancelled list_tools, ignored the timeout the
operator configured, and reported no tools to the client. Prompts and resources
had no outer guard at all.
Give the client session a read timeout so every request it sends is bounded,
including initialize. The SDK reports its own elapsed timeout as an McpError
carrying an HTTP status code in the field that otherwise holds JSON-RPC error
codes, and it relays an upstream's JSON-RPC error through that same class and
field, so the code alone cannot separate the two: an upstream answering with
application code 408 would be blamed on the gateway as a 504. Translate the
SDK's timeout into a TimeoutError in the module that configures the timeout,
matching on the elapsed timeout in the exception's context chain rather than on
the number, so the listing taxonomy never has to read a JSON-RPC code as an HTTP
status and every caller gets the same signal.
The bare cancellation warning is replaced by a line naming the server and the
budget that elapsed, and quiet_on_error does not demote it.
Three groups, all verified by running the suite rather than by inspection.
18 files whose every test function carries an unconditional @pytest.mark.skip,
39 test functions in total. They are collected on every CI run and always skip,
so they advertise coverage the suite does not have. Reasons on the marks include
"AWS Suspended Account", "lakera deprecated their v1 endpoint" and "moved to
using 'otel' for logging"; 26 of the marks predate 2025.
30 test functions with a byte-identical body and identical decorators to a
sibling in the same file and class, differing only in name. Deleting one of each
pair removes no coverage. Four further candidates were excluded because they
override an inherited test, where deleting the override un-shadows the base
class implementation instead of removing a duplicate.
9 test functions that a later definition of the same name shadows, so Python
never binds them and pytest cannot collect them.
One file that is a demo script rather than a test; its own docstring says to run
it with python.
Verification: collecting the 26 edited files gives 2,492 node IDs before and
2,462 after. The 30 duplicate deletions account for exactly 30 removals, the 9
shadowed deletions account for 0 (confirming at runtime that they were never
collectable), nothing unexplained disappeared, and nothing new appeared. No
other test or module imports any deleted symbol.
Register remote MCP servers without awaiting OAuth metadata, warm discovery in the background, and share bounded request-time retries with per-server cooldowns.
Preserve the existing discovered-tool boundary for explicit server calls.
Co-Authored-By: Codex
A keyless internal user signing in to the Admin UI was redirected off the
post-login landing to /ui/connect, which renders nothing but the MCP apps panel,
so a plain gateway sign-in ended on an MCP OAuth surface the user never asked
for. The landing now renders the keys dashboard for every role. The key lookup
that existed only to make that routing decision goes with it, along with the
useKeys enabled flag it was the sole caller of and the role-hydration hold that
guarded its one-frame dashboard flash
The gateway DCR consent flow moves the other way. Its /authorize handed the
browser to /ui/chat/integrations, whose layout hard-blocks when enable_chat_ui
is off, which is the default, and client-side redirects to /ui/ without the
query string; that destroys the connect_flow handle and strands the MCP client
until the 600s flow cookie expires. It now lands on /ui/connect, which reads
connect_flow and connect_client, mounts the consent banner and puts the apps
panel in connect mode. /ui/chat/integrations keeps its connect-mode handling
this release so flows sealed before the deploy still finish
Resolves LIT-5104
Resolves LIT-4911
Tool-level MCP entitlements are enforced in one place,
check_tool_permission_for_key_team, reached from pre_call_tool_check. Two
dispatch paths reached a tool handler without passing through it.
execute_mcp_tool's legacy fallback dispatched into the local tool registry
after retrying the unprefixed name, with no allowed/banned-tool check, no
key/team/org tool permissions and no parameter validation. It now runs the same
gate, and only when something can actually dispatch: when the unprefixed name is
absent from the local registry too, the existing 404 stands rather than becoming
a misleading "server unavailable".
The server the tool-level checks need is available even though the tool name is
not in the tool -> server mapping: a non-empty prefix has already been compared
against the caller's allowed_mcp_servers by exact name, so the named server is
in that list. It is resolved from allowed_mcp_servers rather than from the
manager's registry, because the registry can return a server the caller holds no
grant for, and matching on anything other than name would accept a server the
server-level check never validated. The remaining case is a prefix segment that
is empty, which the server-level check skips entirely because it is gated on a
non-empty server name; that now fails closed with 503 instead of dispatching for
a caller holding no server grant at all.
An entitled caller's legacy call therefore still dispatches, so a configuration
that worked before keeps working; only the unentitled call is refused, now with
the entitlement gate's own 403.
call_tool ran pre_call_tool_check inside `if proxy_logging_obj:`, so an absent
logging object would have skipped authorization silently. This half is defensive
with no live hole: all four call sites source the module-level ProxyLogging
singleton from proxy_server.py, which is never None. The shape was still wrong.
pre_call_tool_check now runs its three authorization checks unconditionally and
only the guardrail hooks, which are dispatched through the logger, depend on one
being present.
A third reported path, where allow_all_keys, BYOM-submitted and
upstream-delegated servers are unioned in after the resolver's ceilings, was
investigated and found not to be a defect. The widening is real, but a server's
tool surface is already boundable for every caller at registration through
MCPServer.allowed_tools / disallowed_tools, enforced by
check_allowed_or_banned_tools ahead of the entitlement check, and per-caller
narrowing plus the org tool ceiling remain available. Nothing here changes that
path.
Resolves LIT-4956
The ID-JAG egress arm could only assert a caller that presented its own IdP
identity token on the request, so an agent holding a brokered LiteLLM
credential got a 412 and never reached the upstream. The assertion captured at
SSO login was already persisted per user for exactly this purpose, but nothing
read it back.
The arm now falls back to that stored assertion, keyed on the authenticated
principal's user_id. The identity is always taken from the credential the
gateway authenticated, never from a caller-supplied field, so no caller can
select whose identity is asserted upstream. A missing, expired, or
unidentified subject stays a 412; ID-JAG exists to assert a specific user and a
missing subject has no safe substitute. A store outage is the one exception: it
is surfaced as a typed AssertionStoreUnavailable and mapped to 503, so a
database blip cannot 500 the egress or the upstream-401 retry, and does not
tell the user to sign in again over something they cannot fix.
Sourcing a subject from the store rather than the request changed what
invalidation can rely on, so the exchanged-token cache changed with it. The
entry is now addressed by a slot key derived from the principal, plus the
caller's own token when it presented one, with a fingerprint of the subject
token and config stored beside the bearer and compared on every read. A
mismatch reads as a miss and re-mints, so a rotated assertion or an edited
server config cannot be served a bearer authorized under the old inputs, and
two callers cannot receive each other's. Invalidation is a single delete of a
key it can always compute, needing no store lookup on the recovery path.
The upstream-401 invalidate-and-retry path was also gated on a truthy inbound
subject token, which skipped recovery entirely for store-sourced calls. The
gate is now mode-aware: token_exchange still requires an inbound token because
it has nothing else to mint from, id_jag does not.
oauth2_id_jag is also now selectable in the admin dashboard with its own field
set, instead of being reachable only from config.yaml or the REST API. The
auth-type selects drop antd list virtualization: at eleven options the last one
no longer mounts, which is a scroll in a browser but makes the option
unreachable to anything reading the rendered list.
Co-authored-by: Yassin Kortam <yassin.kortam@gmail.com>
An MCP permission level answers which servers and tools it permits, and a
level that answers nothing places no restriction. Key auth was reading a
lookup FAULT as that same answer, so the end user, agent and org ceilings
quietly disappeared for as long as one lasted, while the keyless
gateway-admitted path failed closed on the very same fault.
Those levels now separate the two fault classes the user level already
did. A principal row that NAMES an object_permission_id whose contents
cannot be read is a known entitlement with unknown contents, so it denies.
A lookup that fails before we can tell whether the principal is entitled
at all still places no ceiling, that being the state which existed before
the level did; denying there would refuse MCP to the majority of callers,
who have no such entitlement configured. The keyless path is unchanged.
Resolves LIT-4960
* fix(mcp): annotate connected-app reachability on the gateway connect page
The MCP connect page resolved its server grid through the dashboard identity
(admin shortcut or view_all returns the whole registry) while the gateway DCR
session it sets up resolves servers as an admitted subject through grant
sources only, so the page showed servers and tool counts the session is never
served. GET /v1/mcp/server now accepts connected_app_view=true and stamps each
returned server with connected_app_reachable, computed by the same
_reload_admitted_user + get_allowed_mcp_servers pair the live session uses.
The connect page requests the flag in connect mode and renders unreachable
servers dimmed with a label, excluded from the Connected count and tool-count
fetches. Failure to build the admitted set marks everything unreachable, which
matches what such a session would actually be served. Default behavior without
the param is unchanged for every existing consumer.
* fix(mcp): block connecting unavailable servers from the connect-mode detail view
A server the connect page marks unavailable could still be added through its
detail view Connect action, so the selection could contain servers the
connected-app session is never served. The unavailability decision now lives in
one predicate, connectUnavailabilityLabel, consumed by the card indicator, the
detail view action area, the toggle-on path, the oauth auto-select effect, and
the Connected count, so no interaction path can disagree with the label. This
also closes the same pre-existing hole for servers marked not supported on this
connection, whose detail view likewise offered Connect, and removes a
grandfathered nested ternary, ratcheting the eslint suppressions baseline down
* fix(mcp): hide unreachable servers on the connect page instead of dimming them
Product decision: the connect page should only show what a connected-app
session will actually be served, so annotated-unreachable servers are now
filtered out of the connect-mode list at fetch time rather than rendered
dimmed. Unsupported auth types keep their existing dimmed label since they are
a property of the server, not the caller. A user with zero reachable servers
gets an explanatory empty state pointing at grants. The list filter is the
single source: counts, tabs, auto-select, detail view, and tool-count fetches
all derive from the already-filtered state
* fix(mcp): guarantee the connect view lists every session-reachable server
The connect view's membership came from the dashboard resolver with the
admitted-subject answer only annotated on top, so a server reachable by the
session but missing from the dashboard list would be invisible on the page; an
under-report, the mirror of the bug this PR fixes. The connect view now unions
in any session-reachable server the dashboard resolver did not list, built
from the registry and redacted through the same ladder, so page membership
equals the admitted set by construction in both directions
* fix(mcp): honor connected_app_view only for the dashboard UI session credential
The reachability view resolves through the owning user's admitted identity, so
a caller-passed virtual key could use the param to enumerate servers beyond
its own scope (ids, names, descriptions of the owner's wider grants). The view
is now gated on is_ui_session_credential, a predicate factored out of
resolve_ui_session_team_ids so the two user-identity widening sites share one
trust boundary: the SSO-minted dashboard session token acting as its user. Any
other credential gets the param as a no-op and the admitted resolver is never
consulted for it
* fix(mcp): resolve UI sessions with the admitted-user context everywhere, not per endpoint
The list endpoint unioned in session-reachable servers itself while tool
counts, Connect actions, and credential endpoints still authorized through
build_effective_auth_contexts, whose contexts carry team grants but never the
user row's own object permission; a user-granted server could render on the
connect page while every interaction on it failed. The admitted-user context
(the same auth a gateway session resolves with) is now appended inside
build_effective_auth_contexts for UI session credentials, so the page list and
every per-server action endpoint answer identically, and the list endpoint's
one-off union is deleted. Caller-passed keys are still never widened
(is_ui_session_credential gate inside the context builder) and a reload
failure falls back to team contexts only
* fix(mcp): resolve non-admin dashboard sessions as the admitted subject on tool routes
Server reachability on the REST tool routes came from the widened context
union while tool permission checks ran on the bare session key, which carries
no object permission, so a dashboard user could invoke tools their user-level
grant excludes. Rather than bookkeeping which context granted which server,
the routes now choose one principal at the boundary: acting_user_auth swaps a
non-admin UI session for the admitted-subject auth, the same identity a
gateway session resolves with, so reachability, per-source fail-closed tool
ceilings, rate limits, and billing attribution all bind through the admitted
arms that already exist downstream. Admin sessions keep their operator view
and caller-passed credentials are never widened. One swap point per route,
no per-server principal picking, no parallel permission logic
* fix(mcp): derive the connect page's detail view from the reachable server list
The detail view held its own copy of the server object, so it outlived the list it came
from. When a refetch dropped that server as unreachable, the open detail view kept
rendering it and its Connect action still ran: the guard looked the server back up by id
or name in the current list, found nothing, and fell through, because a missing target
read as "nothing to block" rather than "no longer connectable"
Store the selected server's id and derive the row from the list instead. A server the
list no longer carries cannot be the detail view's subject, so the stale render, the
stale tools query and the guard bypass stop being reachable states rather than being
blocked one at a time. handleToggle now takes the server it is toggling, which deletes
the lookup that could miss at all
* refactor(mcp): one owner for the identity a dashboard session acts as
Three call sites reloaded the admitted subject independently, and the management
endpoint carried its own copy of the reload, the HTTPException swallow and the logging.
admitted_user_context is now the only place that answers "what user identity does this
dashboard session act as", and the connected-app reachability helper reads it, which
also drops its dead empty-user_id branch
That owner now carries the request's tracing span onto the admitted principal.
_reload_admitted_user builds a fresh auth from the user row and has no span of its own,
so swapping it in on the REST tool routes silently detached every downstream lookup and
the tool-call logging from the request's trace
Toolset scoping and the acting-as-user swap are mutually exclusive, so they now share
one owner on the tools list route. The admitted subject resolves per grant source and a
team source deliberately carries none of the caller's object_permission, so a toolset
narrowing layered on top would evaporate on every team-granted server: the request would
be admitted through the toolset grant and then served tools from servers the toolset
never named. A request carrying a toolset name stays on the caller's own credential,
exactly as it did before the swap
* fix(mcp): commit every async connect-page write against the list as it stands
Three continuations in the panel decided against state captured before their await and
committed after it, so a reachability refetch landing in between could not be seen
handleToggle validated the server at click time and then, once listMCPTools resolved,
wrote its name into the selection whatever the list had since become; a server the
refresh had dropped was selected anyway. It now re-asks connectableNow at the commit,
and that predicate resolves the id against the current list, so absence fails closed
instead of reading as nothing to block
The load pipeline was worse, because its cancel flag was shared across runs: the
successor's effect body reset it to false before the predecessor's fetch resolved, so a
superseded load could still run setServers and put the dropped server back on the page
outright. The flag is now a per-effect local that only that run's cleanup can clear,
which is also what makes unmount stop the chunked tool-count loop again. The load
passes its own liveness check down to the tool-count and oauth-status writes rather
than having them consult a flag they share with every other run
* fix(mcp): write the connect-page server list to its ref as it is committed
connectableNow resolves a server id against serversRef, but that ref was a mirror kept
in step by a passive effect, so it lagged the state it mirrored by however long React
took to render and flush. A continuation resolving inside that window read the previous
list: the commit-time reachability check would find a server the refetch had already
dropped, call it connectable, and select it, which is the mismatch the check exists to
prevent
The lag was the whole defect, so the mirror is gone. commitServers writes the ref and
the state together, at the one point the list is ever replaced, and the ref is now
never older than the last committed list. Readers that want the newest answer
(connectableNow, the oauth auto-select effect) get it; rendering still derives from
state, so what is on screen is unchanged
Pinned by a test that resolves the refetch and the in-flight Connect in the same tick,
with no render flushed between them, which is the interleaving the earlier regression
could not reach. The two prop mirrors are deliberately untouched: their staleness is
inherent to appending to a parent-owned list from an async callback rather than caused
by the mirror, and no reachability decision reads them
Greptile found that the OpenAPI fallback added a commit ago collapsed operation
IDs that registration keeps apart: foo/bar and foo.bar register as two tools but
sanitize_openapi_tool_name rewrites both to foo_bar, so a policy naming either
also decided the other.
The cause was two owners for one map, and picking the wrong one. Registration
names an operationId inline at _register_openapi_tools with
operation_id.replace(" ", "_").lower(), which keeps / and . ; the separate
sanitize_openapi_tool_name replaces every character outside [a-zA-Z0-9_-] and
belongs to register_tools_from_openapi, which has no production caller. Nothing
made the matcher use the one that actually registers, so it used the lookalike.
That inline expression is now openapi_tool_name in utils, and both registration
and the matcher call it. Replaying the registering function is the whole safety
argument, and it is structural rather than a claim: two operationIds that
register as two tools normalize to two names here by construction, because this
is the map that registered them. A coarser lookalike cannot be substituted
without a test failing.
The matcher also loses its exact-then-fallback split. The transform is identity
on native servers and idempotent on already-registered names, so normalizing
both sides is exact matching where no OpenAPI spec is involved. Executable lines
drop by three this round; the branch is +5 over the merge-base for four shared
owners that removed duplication at six call sites.
Greptile flagged that match_known_tool_name case-folded both the configured
entry and the derived spellings. Routing keeps two tools whose names differ
only in case as two tools, so folding merged identities the dispatcher
separates: on a server exposing getPet and getpet, an allowlist naming getPet
also granted getpet, and a blocklist naming getPet also denied getpet. That is
unauthorized execution on one arm and the wrong tool denied on the other.
Matching is now exact, which is what identity means here. The case leniency it
replaces was never typo tolerance; _register_openapi_tools rewrites every
operationId through sanitize_openapi_tool_name, so an allowed_tools entry
holding the spec's own spelling never equals the registered name. That link is
recovered by replaying the same rewrite, and only on servers that carry a
spec_path, which is how the rest of the manager already recognizes an OpenAPI
server. Every name that rewrite produces is lowercased, so no two tools on such
a server can differ only in case and the fold cannot merge anything.
Native servers get no folding at all. test_case_folding_applies_to_openapi_
servers_and_not_to_native_ones pins both halves, and two tests pin that a
policy naming one tool leaves its case-variant sibling alone. Dropping the
spec_path guard, dropping the fold, and forcing the fold path are all killed.
The two pre-existing case-insensitivity tests describe OpenAPI servers in their
own docstrings but built fixtures without a spec_path, a shape production never
produces for one; they now set it.
Bugbot flagged REST listing advertising key/team grants that tools/call then
refuses. The listing side was fixed by routing through
filter_tools_by_key_team_permissions, but the two paths still answered the
question with separate implementations that only happened to agree: listing
stripped the known prefix and compared bare, dispatch compared whatever name it
was handed, and each carried its own reading of None and of an empty list.
Changing either side silently diverges from the other, which is how this defect
appeared in the first place.
MCPRequestHandler.tool_is_granted owns the whole decision, and both
is_tool_allowed_for_server and filter_tools_by_key_team_permissions read it.
None still means no tool-level restriction and an empty list still grants
nothing, now stated once. Grants are stored bare by every writer, so matching
stays exact against the bare name, deliberately unlike the server-level lists,
which honor every spelling routing accepts.
test_key_team_listing_and_dispatch_agree drives both production paths over one
matrix. It asserts the expected verdict as well as the agreement, because two
paths reading one predicate makes equality alone tautological: a wrong
predicate keeps them consistent and the agreement assertion alone survived two
mutants that the verdict assertion kills.
The allow list, the deny list, allowed_params and the discovery filter all ask
the same question, "which configured entry names this tool on this server", and
each answered it in its own idiom: any() over a spelling tuple, all() over the
same tuple negated, a next() that pulled a value out of a dict, and a
lowercased set membership. Two review findings on this PR were symptoms of that
duplication. Deriving the operands differently at one site produced the
over-strip; needing a value rather than a boolean at another produced a
truthiness test that read an explicitly empty allowed_params list as "nothing
configured" and allowed every parameter.
match_known_tool_name returns the matching entry or None, and all four sites
read it, so no site can test a container's values to decide membership and the
empty-list fail-open is no longer representable. Matching is case-insensitive
everywhere, which closes the last divergence between discovery and dispatch: a
case-variant disallowed_tools entry used to hide a tool from tools/list while
tools/call still executed it.
Executable lines over the merge-base drop from +9 to +4, all of it the new
owner; mcp_server_manager.py loses 12 lines and the discovery filter loses 17.
The gateway publishes a tool as `<server prefix><separator><tool name>` and has to
recover that boundary on the way back in, to compare a called name against a toolset
or allow/deny list and to rebuild the native name sent upstream. Several sites
recovered it by cutting at the FIRST separator and others reconstructed it by hand
from `MCPServer.name` with a literal `-`, so both disagreed with the prefix the
server actually publishes
`get_server_prefix` publishes short_prefix, then alias, then server_name, then
server_id; it never reads `name`. A server with no alias therefore publishes its
hyphen-filled UUID `server_id` as the prefix, and cutting at the first separator
leaves most of the UUID glued to the tool name. Every comparison against the stored
`(server_id, tool_name)` toolset row then misses: an allowlist denies a tool the list
endpoint just advertised, and a disallowed entry stops blocking, which fails open
Recover the boundary in one place instead. `match_known_server_prefix` matches a name
against the server's registered prefixes, longest first so a prefix that itself
contains the separator beats a shorter prefix that is merely its leading segment, and
returns None when the name carries none of them. `strip_known_server_prefix` and
`is_tool_name_prefixed` both delegate to it, and the sites that receive a wire name
call the owner rather than re-deriving the boundary. `split_server_prefix_from_name`
stays for the routing pair it was written for, with a docstring saying so
The server-level permission checks are the other half. They run after the boundary is
already resolved, so their input is bare and the correction there is to derive the
wire form rather than strip it back out; stripping a stored entry a second time cuts a
boundary the caller already consumed, which breaks a native name that itself opens
with the server prefix. Deriving from `get_server_prefix` alone is not enough either,
because routing resolves an inbound name against every prefix from
`iter_known_server_prefixes`, so enforcement keyed to the published spelling answers
for fewer names than are reachable. Turning `LITELLM_USE_SHORT_MCP_TOOL_PREFIX` on
republishes every tool under the short ID while an entry stored under the alias stays
routable and silently stops being enforced, which is a fail-open on a config nobody
edited. `iter_known_tool_name_spellings` yields the bare name plus the wire form under
each accepted prefix, and the allow list, the deny list, `allowed_params` and the
routing map that `_create_prefixed_tools` builds now all key off that one function, so
the set of names enforcement honors and the set routing accepts cannot drift apart
`_tool_name_matches` takes the server as a required argument, so a future caller
cannot silently fall back to guessing, and it matches against that same spelling set,
so `tools/list` hides exactly what dispatch refuses. Answering for fewer spellings in
the filter than enforcement honors leaves a blocked tool advertised, which is how the
alias-form entry above stayed listed even once the call was refused. The OpenAPI
registry lookup builds its key the same way registration does, via `add_server_prefix_to_name` and `get_server_prefix`,
because registration used exactly one key; a server whose `name` differs from its
published prefix stops missing its own tools
