Greptile flagged two follow-ups on the OpenAPI/local-registry pre-call
check:
1. **P1 runtime crash via None proxy_logging_obj.**
`kwargs.get("proxy_logging_obj")` is `None` on the MCP entry path,
and `pre_call_tool_check` calls `proxy_logging_obj._create_mcp_request_object_from_kwargs`
unconditionally after the security checks, which would have crashed
every legitimate call with `AttributeError`. Source the logging
object from `litellm.proxy.proxy_server` the same way
`_handle_managed_mcp_tool` already does.
2. **P2 authorization-bypass window when mcp_server is None.**
Previously the new check was guarded by `if mcp_server is not None`,
so any local tool whose registry entry had no resolvable server (a
startup-race window before `_initialize_tool_name_to_mcp_server_name_mapping`
completes, or an orphaned registry entry) ran without the security
check. Tools registered via openapi_to_mcp_generator are always tied
to a server, so a missing one is a configuration/timing fault — fail
the call with 503 instead of dispatching unguarded.
Tests: existing two pass with an added assertion that
`proxy_logging_obj` is non-None at the call site, plus a new test that
covers the 503 deny branch when the tool→server mapping is missing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`execute_mcp_tool` dispatches in two ways: managed MCP servers go
through `_handle_managed_mcp_tool`, which calls
`MCPServerManager.pre_call_tool_check` to enforce allowed/banned tool
lists, key/team `object_permission` tool grants, and parameter
validation. OpenAPI-backed tools, however, were resolved via
`global_mcp_tool_registry` and dispatched directly to
`_handle_local_mcp_tool` — entirely skipping `pre_call_tool_check`.
A caller could invoke any registered OpenAPI tool regardless of their
key/team permissions, including administrative or destructive
operations on the upstream API.
Run `pre_call_tool_check` before the local-registry dispatch whenever
the resolved server is set (the same condition used to surface server
context to the managed path). Honor any guardrail-modified arguments
the hook returns. Errors raised by the hook propagate up before
`_handle_local_mcp_tool` runs.
Tests cover both directions: the pre-call hook fires when the local
tool resolves alongside a server, and a hook-raised HTTPException
prevents the local handler from being invoked.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply organization object_permission as a ceiling on allowed MCP servers
and tool permissions, consistent with vector store org checks.
Includes unit tests for org ceiling, intersection, and tool filtering.
Made-with: Cursor
Three follow-ups to the OAuth-discovery SSRF guard:
1. Greptile P1 (redirect bypass): the validated origin could return a
3xx whose ``Location`` points at an internal address, and httpx
would follow without re-checking the new target. Pass
``follow_redirects=False`` to both gated httpx GETs. Spec-compliant
OAuth/OIDC metadata endpoints serve the JSON directly, so this
doesn't affect legitimate providers.
2. Greptile P2 (empty getaddrinfo): POSIX doesn't strictly forbid an
empty success-list from ``getaddrinfo``. Add an explicit
``if not infos: return False`` so the guard fails closed instead of
falling through to ``return True``.
3. Mypy: ``info[4][0]`` is typed ``str | int``; narrow at the
boundary with an ``isinstance`` check (fail-closed if non-str).
Adds two regression tests verifying ``follow_redirects=False`` is
passed at both gated fetch sites, and one verifying the empty-list
case rejects the URL.
The OAuth discovery code in mcp_server_manager followed two
attacker-influenceable URLs without validation: the
``resource_metadata`` URL parsed out of a ``WWW-Authenticate``
challenge, and the ``authorization_servers[0]`` field of the
PRM JSON returned by the resource server. A malicious MCP server
could point those at a cloud-instance-metadata service, an internal
admin panel, or a loopback debug endpoint and the proxy would issue
a blind GET on its behalf.
Add ``_is_safe_metadata_url(url, server_url)`` and gate both follow-
up fetch sites on it. A URL is allowed when:
- it shares scheme + host + port with ``server_url`` (well-known
endpoints constructed from the admin's URL, and PRM published at
the resource server itself per RFC 9728 §3.3), or
- it resolves to publicly-routable IPs only (covers federated
authorization servers — Azure Entra, Google, Okta, GitHub —
hosted cross-origin from the resource server).
URLs that resolve to private / loopback / link-local / cloud-metadata
addresses, or that don't resolve at all, are rejected. ``http`` and
``https`` are the only schemes accepted. The IP block list is
provided by the existing ``_is_blocked_ip`` helper from
``litellm_core_utils.url_utils`` so the policy stays consistent with
the rest of the proxy.
The guard does not protect against active DNS rebinding between
this resolution and the subsequent httpx GET — the same-authority
pin remains the primary mitigation; the IP check is defence in
depth. The surface only triggers on config load / add-server, not
per request, so the synchronous ``getaddrinfo`` is acceptable.
Threads ``server_url`` through ``_fetch_oauth_metadata_from_resource``,
``_fetch_authorization_server_metadata``, and
``_fetch_single_authorization_server_metadata``. Existing tests for
those helpers updated for the new signature; new
``TestOAuthDiscoverySSRFGuard`` covers same-authority allow,
private-IP rejection across IPv4 and IPv6, multi-A-record dual-
stack rejection, unresolvable hosts, non-http schemes, and
end-to-end "no network call when guard denies".
Greptile P1: this PR encrypts LiteLLM_MCPUserCredentials rows under the
salt key, but the /key/regenerate rotation endpoint had no
corresponding step for that table. Rotating the master key would
leave every BYOK and OAuth2 user credential permanently unreadable.
Adds rotate_mcp_user_credentials_master_key, mirroring the existing
rotate_mcp_server_credentials_master_key pattern: read each row with
the current key (via _decode_user_credential, which also handles
unmigrated legacy plaintext rows), re-encrypt under the new master
key, write back. One bad row is logged and skipped instead of
aborting the whole rotation.
Wired into key_management_endpoints.py as step 4b, alongside the
existing server-credentials rotation, with the same try/except shape
so a transient DB error on this table doesn't kill the whole
regenerate-key flow.
Tests cover: round-trip through rotation under a new key, automatic
re-encryption of legacy plaintext rows (rotation also acts as a
migration trigger), and a corrupt row not aborting the rotation.
Greptile P1: deployments that today have ``use_x_forwarded_for: true``
but never configured ``mcp_trusted_proxy_ranges`` would silently see
their MCP OAuth discovery URLs revert to the proxy's literal bind
address after this change, with no log line explaining why.
Emit a one-shot WARNING the first time the gate denies for that
specific reason, telling the operator exactly which setting to add.
The warning is module-scoped (not per-request) so the proxy log
stays quiet after the first hit.
get_request_base_url unconditionally honoured X-Forwarded-Proto / Host /
Port to build OAuth issuer / redirect_uri / authorization_endpoint
values for the MCP discovery endpoints. In a deployment where the
proxy is reachable from a caller that can send those headers (direct
internet exposure, or a reverse proxy that does not strip them), an
attacker could poison the OAuth metadata and steer MCP clients at an
attacker-controlled host.
Apply the same trusted-proxy gate the codebase already uses for
get_mcp_client_ip: only honour the headers when use_x_forwarded_for is
enabled in proxy settings AND the direct connection IP falls inside
mcp_trusted_proxy_ranges. When that's not configured, fall back to
the request's literal base_url, so an untrusted caller cannot poison
the discovery metadata.
The existing X-Forwarded-* parsing test cases now opt into a
trust_xff fixture (the parsing logic itself is unchanged). Adds a
matrix for the new gate covering: XFF disabled, XFF enabled with no
ranges, caller outside ranges, caller inside ranges, and the
loopback-dev-deployment case.
Three minor fixes from Greptile review:
1. _decode_user_credential now also catches TypeError so a null
credential_b64 value returns None instead of propagating, matching
the documented "returns None when neither path yields a valid
string" contract.
2. The OAuth2 BYOK guard error no longer claims the existing row is a
BYOK credential — after a salt-key rotation, an OAuth2 row can fail
to decrypt and reach the same guard. Reword to "could not be
verified as an OAuth2 token", which is accurate for both cases.
3. Drop the no-op sys.path.insert in the new test file (other tests
in the directory don't need it; pytest picks up the package via
the installed editable wheel).
Adds a regression test for the None-input case.
LiteLLM_MCPUserCredentials.credential_b64 stored both BYOK API keys and
OAuth2 access tokens as plain urlsafe-base64 of the raw value. Any DB
read could recover the upstream-provider key.
Run all writes through encrypt_value_helper (nacl SecretBox, the same
helper used for the server-level credentials column) and read back via
a small dual-path helper that tries decryption first, then falls back to
plain base64 so existing rows keep working until they get rewritten.
Folds the three near-identical "decode -> json.loads -> check type ==
oauth2" sites into _decode_oauth_payload, which simplifies the BYOK
guard inside store_user_oauth_credential.
- server.py: drop the redundant server_id append in
_get_filtered_mcp_servers_from_mcp_server_names. iter_known_server_prefixes
already yields server_id unconditionally, so the manual append (and its
misleading comment) was a no-op duplicate.
- utils.py: rewrite the SHORT_MCP_TOOL_PREFIX docstring to accurately
describe the collision behaviour. The previous wording said collisions
were 'cosmetic only', but a natural-hash collision IS a routing-correctness
issue, which is precisely why we already added _assign_unique_short_prefix
to rehash deterministically. The new comment cross-references that path.
- utils.py: restrict the first character of the short prefix to [A-Za-z]
via a 52-char alphabet for position 0 only. The remaining two positions
still use the full base62 alphabet. This keeps prefixes valid identifiers
on every backend and gives 52*62*62 = 199_888 distinct prefixes (still
comfortably more than any realistic deployment).
- tests: add coverage proving the first character of the prefix is always
alphabetic across many server_ids and rehash attempts.
Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
Two MCP servers can natural-hash to the same three-character base62
prefix. With 62**3 = 238_328 slots the birthday bound is ~488 servers
for 50% collision probability, so a single proxy hosting more than
~100 MCP servers has a non-trivial chance of seeing a collision in
practice — and a collision means tool names from two different servers
share a routing key, causing silent mis-routing.
Mitigation:
- compute_short_server_prefix(server_id, attempt=N) folds an attempt
counter into the SHA-256 seed, so rehashes are deterministic and
produce a fresh three-char prefix space per attempt.
- New MCPServer.short_prefix field caches the resolved (post-dedup)
prefix on the model so it stays stable across the process lifetime.
- MCPServerManager._assign_unique_short_prefix walks attempts 0..N
until it finds a prefix not already used by another server in the
combined registry. Logs an INFO line when a rehash happens so
operators have a breadcrumb if it ever does.
- Wired into every registration path: load_servers_from_config,
add_server, update_server, reload_servers_from_database. The
database reload path also carries the previously-resolved prefix
forward so reloads don't churn it.
- get_server_prefix prefers the cached short_prefix when set, so the
resolved value (not the raw natural hash) is used everywhere.
- iter_known_server_prefixes yields the cached short_prefix too, so
reverse-lookup tolerance covers the rehashed form.
No-op when LITELLM_USE_SHORT_MCP_TOOL_PREFIX is disabled — the field
stays None and behaviour is unchanged.
Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
Adds LITELLM_USE_SHORT_MCP_TOOL_PREFIX. When enabled, tool / prompt /
resource / resource-template names emitted from MCP servers are prefixed
with a deterministic three-character base62 ID derived from the server's
server_id (SHA-256 → base62) instead of the (potentially long)
alias / server_name. This keeps namespaced tool names well under the
60-character upper bound enforced by some model APIs while still letting
us distinguish MCP-routed tools from local tools.
Behavioural notes:
- Default off — when the env var is unset, the long-prefix behaviour
is unchanged. The plan is to flip the default in a future release
and remove the gate after a deprecation window.
- Prefix derivation is deterministic, so it is stable across processes,
workers and restarts without any persistence layer.
- Reverse-lookup is tolerant: _create_prefixed_tools registers every
known prefix form (alias / server_name / server_id / short ID) in
the routing map and _get_mcp_server_from_tool_name resolves any of
them. Old clients holding cached long-prefixed names continue to
route correctly even after the flag is enabled.
- _get_allowed_mcp_servers_from_mcp_server_names accepts the short
prefix in /mcp/{server_name}-style URLs.
- The OpenAPI tool-listing path now filters by the active server
prefix instead of server.name so spec-backed servers benefit too.
Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
Greptile review feedback (P2): the two negative `.well-known`-substring
tests fell through to `_target_servers_use_oauth2`, which queries
`global_mcp_server_manager.get_mcp_server_by_name`. Without an explicit
mock the tests passed only because the real registry happens to be empty
in the test process. Mock the manager to return None so the assertion
exercises the fail-closed path explicitly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile flagged a regression introduced in the previous commit's merged
exception handler: ``ProxyException.__init__`` normalizes ``code`` via
``str(code)``, so a ``code=None`` (valid per the type signature) becomes
the string ``"None"``. Coercing that with ``int(...)`` raises
``ValueError``, which propagates uncaught and rewrites the auth error as
an unhandled 500 — degrading security posture compared to the pre-merge
``str(e.code) in ("401", "403")`` shape.
Compare against both int and str forms of the auth-error codes instead
of coercing. Adds a regression test for the ``code=None`` case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related issues in `MCPRequestHandler.process_mcp_request`:
1. Public-route detection used `".well-known" in str(request.url)`, a
substring match against the full URL. Attackers could smuggle the
marker via the query string, hostname, or a deeper path segment to
bypass authentication on any MCP route. Replaced with an exact path
prefix on `request.url.path` (`startswith("/.well-known/")`).
2. The OAuth2 passthrough fallback (added in #20602 to support
`auth_type=oauth2` upstream MCP servers like Atlassian) caught any
401/403 from `user_api_key_auth` and replaced the result with an
anonymous `UserAPIKeyAuth()`. That fallback fired regardless of the
target server's configured `auth_type`, so an attacker presenting a
garbage `Authorization` header could exchange a failed LiteLLM auth
for an anonymous session against any server. The fallback now runs
only when EVERY MCP server the request targets is operator-configured
for `auth_type=oauth2`. For any non-oauth2 server (api_key,
bearer_token, basic, etc.), the auth error propagates as before.
Target resolution prefers the `x-mcp-servers` header when present
(including the explicitly-empty case, which fails closed) and otherwise
parses the standard `/mcp/{server_name}` and `/{server_name}/mcp`
transport URL patterns. Routes that don't match either form fail closed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`_check_byok_credential` previously returned silently when `prisma_client`
was None, bypassing BYOK ownership validation during database-outage
windows. Any proxy-authenticated user could invoke BYOK-protected MCP
tools without a stored credential during the outage window.
Now raises HTTP 503 with a structured error so the flow fails closed.
Regression test asserts 503 is raised when `prisma_client` is None.
Reported by @brodmart in GHSA-6762-2m23-5mxp.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR feedback (greptile P1 / veria high): with the previous change, a team
storing mcp_tool_permissions={"my-alias": ["read_file"]} would pass the
server-access check (because the alias expanded to a concrete id in the
allowed-servers list) but the per-server tool lookup still did
dict.get(server_id) against the raw name-keyed dict — missing, returning
None, which callers treat as "no restrictions" → all tools allowed instead
of only the declared ones.
Add MCPServerManager.expand_tool_permissions() that rewrites the dict so
every key is a concrete server_id where possible (tool lists from keys
pointing at the same server are unioned). Unresolved keys pass through
unchanged so stale id-keyed restrictions still apply when the same string
is used for lookup. Wire the helper into the four dict-lookup sites:
get_allowed_tools_for_server (key + team paths), the agent tool lookup,
and the rest_endpoints.py tool filter.
Also switch expand_permission_list to pass through unresolved entries
(rather than dropping them) so existing test fixtures that use bare string
placeholders continue to work. The downstream access check denies unknown
entries when compared to the concrete request server_id, so security
posture is unchanged.
Sanitize the debug log to use %r formatting so an admin-controlled
identifier with newlines can't forge log entries (CodeQL log-injection
warning).
team.object_permission.mcp_servers (and the per-key equivalent) previously
only accepted server_id strings. For config-loaded MCP servers, the id is
derived from a hash that includes the server URL, so the same logical
server in two regions ends up with two different ids in a shared database.
Permission lists had to enumerate every region's id.
Add a single MCPServerManager.expand_permission_list() helper that resolves
each entry against the current region's config + DB registry union: entries
that match a server_id pass through, entries that match an alias/server_name/
name expand to every matching id, and unresolved entries drop with a debug
log so stale or typo entries are diagnosable. Wire it into the four
_get_allowed_mcp_servers_for_* helpers so direct server entries and
mcp_tool_permissions dict keys are both expanded before the intersection.
Access-check outcomes are unchanged for existing id-based permissions;
name-based entries now resolve instead of being silently denied.
Address codex review P1 + P2 findings:
- BYOK /token now accepts OAuth 2.1 clients that omit redirect_uri
(draft-15 §4.1.3 dropped the requirement). When the client does
submit a value, equality is still enforced vs the /authorize record.
PKCE + client_id binding cover the security role redirect_uri
played under RFC 6749.
- _user_id_from_session_cookie requires the ``exp`` claim on the UI
session JWT (PyJWT options={"require": ["exp"]}) so leaked cookies
have a bounded lifetime.
- validate_loopback_redirect_uri rejects URIs with a fragment
(RFC 6749 §3.1.2) and catches malformed-URI ValueError so
unparseable input surfaces as 400 invalid_request instead of 500.
Address codex review P0 + P1 findings on the discoverable OAuth proxy:
- /callback now re-validates that the decoded base_url is loopback before
302-redirecting to it. State is encrypted but pre-existing states minted
before the /authorize validation was added have no expiry and remain
valid; validating at the sink closes the open-redirect + code-theft
primitive for those stale states too. (VERIA-57 root cause B, P0.)
- /token responses now set Cache-Control: no-store + Pragma: no-cache
per RFC 6749 §5.1 (P1).
- Move TOKEN_NO_CACHE_HEADERS constant from byok_oauth_endpoints into
the shared oauth_utils module so both endpoints use the same value.
Extract the BYOK loopback redirect_uri check into a shared
oauth_utils.validate_loopback_redirect_uri helper. Call it in
discoverable_endpoints.authorize_with_server before the client-supplied
redirect_uri is encrypted into the OAuth state.
Without this check, a non-loopback redirect_uri was encoded into the
state parameter and decoded on /callback to 302 the user back to the
attacker's URL with the authorization code attached — an open-redirect
+ code-theft primitive (VERIA-57 root cause B). The /callback handler
is already safe because state is HMAC-signed via encrypt_value_helper,
so validating at /authorize before encoding is sufficient.
Also updates existing tests to use loopback client redirect_uris and
adds regression tests for non-loopback rejection, IPv4 127.0.0.0/8
range acceptance, and full-form IPv6 loopback acceptance.
* fix(mcp_semantic_tool_filter): match canonical tools that arrive with
a client-side namespace prefix.
`SemanticMCPToolFilter._get_tools_by_names` matched by exact equality
between the canonical name stored in the router
(`<server><MCP_TOOL_PREFIX_SEPARATOR><tool>`) and the name in the
incoming `tools[]` list. MCP clients such as opencode wrap every tool
name with their own additive alias prefix
(`<client_alias>_<canonical>`), so the two never matched, the filter
dropped every tool to zero, and the proxy forwarded `tools: []` with
`tool_choice: auto` — which strict upstream providers reject with a 400.
The fix adds anchored suffix matching with a separator check: the
canonical must form the complete tail of the incoming name and be
preceded by `_` or `-`. Exact matches still win over suffix matches,
incoming tools are returned at most once, and the original tool object
is passed through unchanged so the client-facing name survives for
tool-call round-trips.
Seven unit tests in a new TestGetToolsByNames class cover exact
match, underscore- and dash-prefixed variants, non-separator-anchored
suffixes (which must not match), exact-wins-over-prefixed precedence,
deduplication when two canonicals suffix-match the same incoming tool,
and ordering-follows-router-output.
Fixes#26078
* review: strengthen the suffix-fallback tie-breaker and the
deduplication regression test (Greptile comments on #26117)
- test_same_tool_not_returned_twice now passes two distinct canonicals
("read_file" and "file") that both suffix-match the same incoming
tool, rather than the same canonical twice, so the assertion
actually exercises the used_ids dedup path instead of the
duplicate-input-list path.
- The suffix fallback in _get_tools_by_names now prefers the shortest
incoming name that still qualifies under the separator-anchored
match. In the one-prefix-per-client opencode scenario this is a
no-op, but in multi-namespace configurations the shortest qualifying
name is the least-wrapped one and is the most defensible deterministic
choice, replacing the dict-insertion-order fallback.
- Adds test_suffix_fallback_prefers_shortest_candidate covering the
new tie-breaker directly.
Still 15 tests passing locally (was 14).
* review(#26117): gate suffix-matching on canonical containing MCP_TOOL_PREFIX_SEPARATOR
@krrish-berri-2 flagged a possible collision in the suffix fallback:
a local user function whose name happens to end in a bare canonical
substring (e.g. my_firecrawl_scrape vs canonical firecrawl_scrape)
would be spuriously selected.
Server-registered MCP tools are always emitted as
<server_name><MCP_TOOL_PREFIX_SEPARATOR><tool_name> via
add_server_prefix_to_name, so a canonical without the separator is
not a namespaced MCP tool and does not warrant suffix matching.
Added that guard to _name_matches_canonical with a regression test
(test_does_not_collide_with_local_function_on_unprefixed_canonical)
that reproduces the collision before the fix and is pinned after.
Pre-existing TestGetToolsByNames fixtures that relied on bare
canonicals (get_weather, search, read_file, write/delete/read) were
switched to realistic server-prefixed ones so they continue to
exercise the suffix-fallback path under the new guard. The opencode
scenario (client prefix on already-server-prefixed canonical) is
unchanged.
---------
Co-authored-by: sakenuGOD <sakenuGOD@users.noreply.github.com>
Co-authored-by: Krrish Dholakia <krrish+github@berri.ai>
Address two Greptile P2 findings on the BYOK OAuth endpoints:
- GET /v1/mcp/oauth/authorize now runs _validate_redirect_uri up front
so a non-loopback redirect_uri is rejected before the HTML form is
rendered. Previously the user typed an API key, submitted, and got a
400 with no form state.
- POST /v1/mcp/oauth/token moves the master_key guard ahead of the
code-consumption and credential-store steps. Without this, a proxy
with master_key unset would burn the code and persist the credential
but return an error — leaving the user with no way to retrieve a
session token without restarting the whole flow.
- POST /v1/mcp/oauth/authorize now requires an authenticated UI session
cookie. The authenticated user_id — not the OAuth client_id form
field — is stamped onto the authorization code record (RFC 6749 §2.2:
client_id identifies the client, not the user).
- redirect_uri is restricted to loopback per RFC 8252 §7.3 (localhost
plus any ipaddress.is_loopback IP, covering 127.0.0.0/8 and IPv6
loopback forms).
- POST /v1/mcp/oauth/token enforces exact-match of the redirect_uri and
client_id submitted at /authorize (RFC 6749 §4.1.3).
- /token error responses use the RFC 6749 §5.2 format ({"error":
"<code>"}), and all /token responses set Cache-Control: no-store +
Pragma: no-cache (RFC 6749 §5.1).
Per-user OAuth MCP requests now only skip pre-emptive 401 when a stored token is available, preserving token-reuse behavior while restoring fast PKCE kickoff for first-time or missing-token users.
