mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-13 23:11:40 +00:00
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 1667b8f740)
This commit is contained in:
parent
5113f5d53d
commit
9947a2fe6a
4 changed files with 287 additions and 2 deletions
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue