From 9947a2fe6a75199a2b847d615c3e2849a8e62fb5 Mon Sep 17 00:00:00 2001 From: yucheng-berri Date: Mon, 22 Jun 2026 11:34:20 -0700 Subject: [PATCH] fix: reject model_list in proxy body and gate advisor client credentials (#30585) * fix: validate proxy request body and nested fields Ensure caller-supplied request fields cannot override server-side deployment configuration, and apply request-body validation consistently to nested structures. Adjusts router kwarg handling and client-side credential handling for base-url overrides * test: cover router strip ordering and advisor clientside credential gate * fix: clear deployment credentials on client base-url override When a request overrides api_base/base_url, recompute the deployment's litellm_params (clearing the deployment's own api_key) and drop the cached client built for the original endpoint, so the deployment credential is not reused for the client-supplied endpoint. Adds regression tests that assert the credentials actually forwarded to litellm.completion/acompletion. * fix(proxy): require api_key alongside api_base override A request that overrides api_base/base_url but supplies no api_key still left the proxy carrying a server credential: once the override clears the banned-param opt-in, the provider re-resolves a key from the environment (api_key or get_secret("OPENAI_API_KEY") and ~30 sibling chains in main.py) and forwards it to the caller-controlled URL. Popping the deployment api_key only changed which server key leaked. Gate is_request_body_safe so a permitted api_base/base_url override must also carry a non-empty caller api_key; reject otherwise. The env resolution in main.py is left as the provider boundary. * fix(proxy): extend request-body banlist with five additional credential and session targeting fields Yuneng's review found five deployment-owned request-body params still missing from the denylist and the router strip set. Each lets a caller reach the operator's provider credentials or retarget the outbound request: aws_profile_name selects a local AWS profile, oci_compartment_id and oci_region retarget the OCI request, litellm_credential_name selects any server-loaded credential by name with no ownership check, and runtimeSessionId resumes a Bedrock AgentCore runtime session (AWS does not enforce session-to-user mapping, so this is a cross-tenant session-resume vector). Add all five to _BANNED_REQUEST_BODY_PARAMS in auth_utils.py and to _DEPLOYMENT_OWNED_CREDENTIAL_KWARGS in router.py. Deployment litellm_params and SDK direct calls are unaffected: the banlist gates the request body only, and the router strip drops caller kwargs, never deployment["litellm_params"]. * test: rename arbitrary canary values in security tests to neutral placeholders * fix(proxy): apply api_key co-presence to nested base override and warn on Router credential strip P1-A: is_request_body_safe descended into _NESTED_CONFIG_KEYS (litellm_embedding_config, extra_body) for the banned-param check but not for the api_key co-presence check, so a base override smuggled into one of those nested dicts cleared the client-side-credentials opt-in without a paired api_key and let the provider re-resolve a server credential from the environment. Run _check_base_override_has_api_key on each nested config dict too, so the requirement applies wherever a base override is permitted. P1-B: the deployment-owned credential strip in the Router runs unconditionally on every _completion/_acompletion, which is security-correct but silently drops per-call api_version/vertex_project/etc. for SDK Router callers. Emit a single warning (key names only, never values) when the strip removes a non-empty value, so the backwards-incompatible behavior is visible without gating the strip on a context flag that does not exist. * fix(proxy): apply api_key co-presence to tool-entry base override is_request_body_safe scans three surfaces (root, _NESTED_CONFIG_KEYS, and tools[]); the previous commit extended the api_key co-presence rule to root and nested config dicts but not to tool entries. With allow_client_side_credentials enabled, a tool entry carrying api_base/base_url and no paired api_key cleared the gate, letting a provider interceptor fall back to a server-side credential for a caller-controlled URL. Add the same _check_base_override_has_api_key call to each tool dict and its nested function dict, mirroring the symmetry already applied to the nested config keys. The rule is unchanged: api_key must live in the same dict as the base override it accompanies. * test(proxy/auth): require paired api_key under extra_body opt-in * fix(router): gate deployment-owned kwarg strip on litellm.proxy_is_running * fix(advisor): narrow proxy-import guard to ImportError-family * fix(router): gate api_key clear on base override behind litellm.proxy_is_running * test(proxy/auth): scope proxy_is_running flag to dynamic-params class with autouse fixture * style: use built-in generics in PR-added type annotations * revert: drop proxy_is_running flag and router-level credential strip; rely on proxy gate * revert: scope PR to LIT-3828 + LIT-3834 only; drop LIT-3830/LIT-3833 changes * style: black-format advisor orchestration test (cherry picked from commit 1667b8f740485a15b0e37834ab9dcef08ea872f6) --- .../messages/interceptors/advisor.py | 24 +- litellm/proxy/auth/auth_utils.py | 6 + .../messages/test_advisor_orchestration.py | 214 ++++++++++++++++++ .../proxy/auth/test_auth_utils.py | 45 ++++ 4 files changed, 287 insertions(+), 2 deletions(-) diff --git a/litellm/llms/anthropic/experimental_pass_through/messages/interceptors/advisor.py b/litellm/llms/anthropic/experimental_pass_through/messages/interceptors/advisor.py index c7c110ff3e3..8714939f025 100644 --- a/litellm/llms/anthropic/experimental_pass_through/messages/interceptors/advisor.py +++ b/litellm/llms/anthropic/experimental_pass_through/messages/interceptors/advisor.py @@ -84,8 +84,14 @@ class AdvisorOrchestrationHandler(MessagesInterceptor): ) # Optional routing overrides for the advisor sub-call (e.g. proxy routing). # If not set in the tool definition, litellm resolves from env vars. - advisor_api_key: Optional[str] = advisor_tool.get("api_key") - advisor_api_base: Optional[str] = advisor_tool.get("api_base") + # The advisor tool is caller-controlled; only honor a client-supplied + # api_base/api_key when the proxy has enabled clientside credentials, + # otherwise let litellm resolve from server config. + advisor_api_key: Optional[str] = None + advisor_api_base: Optional[str] = None + if _allow_client_side_advisor_credentials(): + advisor_api_key = advisor_tool.get("api_key") + advisor_api_base = advisor_tool.get("api_base") # Build the synthetic tool definition the provider will receive. synthetic_advisor_tool = _make_synthetic_advisor_tool() @@ -181,6 +187,20 @@ class AdvisorOrchestrationHandler(MessagesInterceptor): # --------------------------------------------------------------------------- +def _allow_client_side_advisor_credentials() -> bool: + """Whether a caller-supplied advisor api_base/api_key may be honored. + + Gated on the proxy's ``allow_client_side_credentials`` opt-in. When the + interceptor runs outside the proxy (SDK use), there is no admin boundary + to protect, so client-supplied routing is allowed. + """ + try: + from litellm.proxy.proxy_server import general_settings + except (ImportError, ModuleNotFoundError): + return True + return general_settings.get("allow_client_side_credentials") is True + + def _make_synthetic_advisor_tool() -> Dict: """Build a regular tool definition the executor provider can understand.""" return { diff --git a/litellm/proxy/auth/auth_utils.py b/litellm/proxy/auth/auth_utils.py index 94b2ed84f20..3a2f2221ee3 100644 --- a/litellm/proxy/auth/auth_utils.py +++ b/litellm/proxy/auth/auth_utils.py @@ -285,6 +285,8 @@ _BANNED_REQUEST_BODY_PARAMS: Tuple[str, ...] = ( "s3_endpoint_url", "sagemaker_base_url", "deployment_url", + # SDK-only field; also rejected outright in is_request_body_safe. + "model_list", # Observability credentials, hosts, and project identifiers: derived # from the canonical ``_supported_callback_params`` allowlist so new # integrations are covered automatically. Sorted for stable iteration @@ -365,6 +367,10 @@ def is_request_body_safe( ``litellm_embedding_config.api_base`` (VERIA-6) without exposing a recursion-depth DoS surface. """ + if "model_list" in request_body: + raise ValueError( + "Rejected Request: model_list is not allowed in the request body." + ) _check_banned_params(request_body, general_settings, llm_router, model) for nested_key in _NESTED_CONFIG_KEYS: nested = _coerce_metadata_to_dict(request_body.get(nested_key)) diff --git a/tests/test_litellm/llms/anthropic/messages/test_advisor_orchestration.py b/tests/test_litellm/llms/anthropic/messages/test_advisor_orchestration.py index 2cb7b4db3d4..31047d30970 100644 --- a/tests/test_litellm/llms/anthropic/messages/test_advisor_orchestration.py +++ b/tests/test_litellm/llms/anthropic/messages/test_advisor_orchestration.py @@ -516,3 +516,217 @@ async def test_max_uses_none_falls_back_to_default(): ) assert str(_c.ADVISOR_MAX_USES) in str(exc_info.value) + + +# --------------------------------------------------------------------------- +# 12. Defense-in-depth: client-supplied advisor api_base/api_key are dropped +# unless the proxy admin opted into clientside credentials +# --------------------------------------------------------------------------- + + +ADVISOR_TOOL_WITH_CREDS = { + "type": "advisor_20260301", + "name": "advisor", + "model": "claude-opus-4-6", + "api_base": "https://other.example", + "api_key": "sk-other", +} + + +async def _run_advisor_and_capture_subcall_kwargs(): + """Run one advisor turn and return the kwargs of the advisor sub-call.""" + from litellm.llms.anthropic.experimental_pass_through.messages.interceptors.advisor import ( + AdvisorOrchestrationHandler, + ) + + advisor_tool_use_resp = _make_advisor_tool_use_response(tool_id="toolu_01") + advisor_advice_resp = _make_text_response("advice", model="claude-opus-4-6") + final_resp = _make_text_response("final answer") + + captured = {} + call_count = 0 + + async def mock_call(model, messages, tools, stream, max_tokens, **kwargs): + nonlocal call_count + call_count += 1 + if call_count == 1: + return advisor_tool_use_resp + if call_count == 2: + # The advisor sub-call — capture its routing kwargs. + captured["api_key"] = kwargs.get("api_key") + captured["api_base"] = kwargs.get("api_base") + return advisor_advice_resp + return final_resp + + with patch( + "litellm.llms.anthropic.experimental_pass_through.messages.interceptors.advisor._call_messages_handler", + side_effect=mock_call, + ): + h = AdvisorOrchestrationHandler() + await h.handle( + model="openai/gpt-4o-mini", + messages=MESSAGES, + tools=[ADVISOR_TOOL_WITH_CREDS], + stream=False, + max_tokens=512, + custom_llm_provider="openai", + ) + return captured + + +@pytest.mark.asyncio +async def test_advisor_creds_dropped_when_proxy_opt_in_disabled(): + """On the proxy without opt-in, the caller's advisor api_base/api_key must + NOT reach the sub-call (would redirect it / leak the server key).""" + with patch( + "litellm.llms.anthropic.experimental_pass_through.messages.interceptors.advisor._allow_client_side_advisor_credentials", + return_value=False, + ): + captured = await _run_advisor_and_capture_subcall_kwargs() + assert captured["api_key"] is None + assert captured["api_base"] is None + + +@pytest.mark.asyncio +async def test_advisor_creds_honored_when_proxy_opt_in_enabled(): + """With the admin opt-in, the documented clientside routing still works.""" + with patch( + "litellm.llms.anthropic.experimental_pass_through.messages.interceptors.advisor._allow_client_side_advisor_credentials", + return_value=True, + ): + captured = await _run_advisor_and_capture_subcall_kwargs() + assert captured["api_key"] == "sk-other" + assert captured["api_base"] == "https://other.example" + + +# --------------------------------------------------------------------------- +# 13. The proxy gate itself: _allow_client_side_advisor_credentials() and the +# full handle() driven by the real proxy general_settings flag. +# --------------------------------------------------------------------------- + + +def _fake_proxy_server(general_settings: Dict): + """A stand-in litellm.proxy.proxy_server module exposing general_settings. + + The real proxy_server pulls in heavy optional deps that may be absent in a + unit-test environment, so the gate's + ``from litellm.proxy.proxy_server import general_settings`` is satisfied by + injecting this lightweight module into sys.modules. + """ + import types + + module = types.ModuleType("litellm.proxy.proxy_server") + module.general_settings = general_settings # type: ignore[attr-defined] + return module + + +def test_allow_client_side_advisor_credentials_reads_proxy_flag(): + """The gate mirrors the proxy's allow_client_side_credentials opt-in.""" + import sys + + from litellm.llms.anthropic.experimental_pass_through.messages.interceptors.advisor import ( + _allow_client_side_advisor_credentials, + ) + + cases = ( + ({"allow_client_side_credentials": True}, True), + ({"allow_client_side_credentials": False}, False), + # Flag absent entirely -> default deny on the proxy. + ({}, False), + ) + for settings, expected in cases: + with patch.dict( + sys.modules, + {"litellm.proxy.proxy_server": _fake_proxy_server(settings)}, + ): + assert _allow_client_side_advisor_credentials() is expected + + +def test_allow_client_side_advisor_credentials_defaults_true_outside_proxy(): + """Outside the proxy (proxy_server import unavailable), there is no admin + boundary, so the gate permits client-supplied routing.""" + import builtins + import sys + + from litellm.llms.anthropic.experimental_pass_through.messages.interceptors.advisor import ( + _allow_client_side_advisor_credentials, + ) + + real_import = builtins.__import__ + + def _blocked_import(name, *args, **kwargs): + if name == "litellm.proxy.proxy_server": + raise ImportError("proxy server unavailable") + return real_import(name, *args, **kwargs) + + with patch.dict(sys.modules): + sys.modules.pop("litellm.proxy.proxy_server", None) + with patch.object(builtins, "__import__", _blocked_import): + assert _allow_client_side_advisor_credentials() is True + + +def test_advisor_gate_propagates_non_import_errors(): + """Non-ImportError failures during the proxy module probe must not + default permissive. If the proxy is partially loaded and raises + RuntimeError, the gate should surface that rather than silently + returning True.""" + import sys + + from litellm.llms.anthropic.experimental_pass_through.messages.interceptors import ( + advisor, + ) + + original = sys.modules.get("litellm.proxy.proxy_server") + + class _Broken: + def __getattr__(self, _name): + raise RuntimeError("partial proxy boot") + + sys.modules["litellm.proxy.proxy_server"] = _Broken() + try: + with pytest.raises(RuntimeError, match="partial proxy boot"): + advisor._allow_client_side_advisor_credentials() + finally: + if original is None: + sys.modules.pop("litellm.proxy.proxy_server", None) + else: + sys.modules["litellm.proxy.proxy_server"] = original + + +@pytest.mark.asyncio +async def test_advisor_ignores_tool_credentials_when_clientside_disabled(): + """Driven by the real proxy flag (not a patched gate): with + allow_client_side_credentials False, the tool-supplied api_base/api_key must + not reach the advisor sub-call.""" + import sys + + with patch.dict( + sys.modules, + { + "litellm.proxy.proxy_server": _fake_proxy_server( + {"allow_client_side_credentials": False} + ) + }, + ): + captured = await _run_advisor_and_capture_subcall_kwargs() + assert captured["api_key"] is None + assert captured["api_base"] is None + + +@pytest.mark.asyncio +async def test_advisor_uses_tool_credentials_when_clientside_enabled(): + """Driven by the real proxy flag: with allow_client_side_credentials True, + the tool-supplied api_base/api_key flow through to the advisor sub-call.""" + import sys + + with patch.dict( + sys.modules, + { + "litellm.proxy.proxy_server": _fake_proxy_server( + {"allow_client_side_credentials": True} + ) + }, + ): + captured = await _run_advisor_and_capture_subcall_kwargs() + assert captured["api_key"] == "sk-other" + assert captured["api_base"] == "https://other.example" diff --git a/tests/test_litellm/proxy/auth/test_auth_utils.py b/tests/test_litellm/proxy/auth/test_auth_utils.py index e652c109987..cd8cf10d037 100644 --- a/tests/test_litellm/proxy/auth/test_auth_utils.py +++ b/tests/test_litellm/proxy/auth/test_auth_utils.py @@ -2160,3 +2160,48 @@ class TestGetRequestRouteTemplate: lambda self: (_ for _ in ()).throw(RuntimeError("boom")) ) assert get_request_route_template(req) is None + + +class TestIsRequestBodySafeBlocksModelList: + """model_list is an SDK-only field with no proxy API meaning; it must + be rejected from the request body regardless of any opt-in.""" + + def test_model_list_rejected_with_no_opt_in(self): + with pytest.raises(ValueError, match="model_list is not allowed"): + is_request_body_safe( + request_body={ + "model": "gpt-4", + "messages": [{"role": "user", "content": "hi"}], + "model_list": [{"model_name": "x", "litellm_params": {}}], + }, + general_settings={}, + llm_router=None, + model="gpt-4", + ) + + def test_model_list_rejected_even_with_proxy_wide_opt_in(self): + with pytest.raises(ValueError, match="model_list is not allowed"): + is_request_body_safe( + request_body={ + "model": "gpt-4", + "messages": [{"role": "user", "content": "hi"}], + "model_list": [], + }, + general_settings={"allow_client_side_credentials": True}, + llm_router=None, + model="gpt-4", + ) + + def test_normal_body_still_passes(self): + assert ( + is_request_body_safe( + request_body={ + "model": "gpt-4", + "messages": [{"role": "user", "content": "hi"}], + }, + general_settings={}, + llm_router=None, + model="gpt-4", + ) + is True + )