diff --git a/litellm/proxy/auth/auth_utils.py b/litellm/proxy/auth/auth_utils.py index 51108827f6b..9a6fc95f145 100644 --- a/litellm/proxy/auth/auth_utils.py +++ b/litellm/proxy/auth/auth_utils.py @@ -2,7 +2,7 @@ import os import re import sys from functools import lru_cache -from typing import Any, Dict, List, Mapping, Optional, Tuple, Union +from typing import Any, Dict, FrozenSet, List, Mapping, Optional, Tuple, Union from fastapi import HTTPException, Request, status @@ -173,9 +173,67 @@ def _allow_model_level_clientside_configurable_parameters( # threat shape should be added here. _NESTED_CONFIG_KEYS: Tuple[str, ...] = ("litellm_embedding_config",) -# Banned root-level params. Same list applies to every entry in -# ``_NESTED_CONFIG_KEYS`` because those dicts get spread as ``**kwargs`` -# into the same outbound calls. +# Metadata containers that carry per-request configuration consumed by the +# observability callbacks. The same banned-param list applies — a value +# under ``metadata.langfuse_host`` redirects the same Langfuse client and +# leaks the same credentials as the root-level ``langfuse_host``, but the +# original check only walked the request-body root, so the metadata path +# was an unintentional bypass. +_NESTED_METADATA_KEYS: Tuple[str, ...] = ("metadata", "litellm_metadata") + +# Banned request-body params. The same list applies to every entry in +# ``_NESTED_CONFIG_KEYS`` (dicts spread as ``**kwargs`` into outbound +# calls) and ``_NESTED_METADATA_KEYS`` (dicts read directly by integration +# callbacks), so a single banned name is enforced wherever the field can +# reach the call path from. +# Per-request observability params that are SAFE to accept from clients. +# These describe the request being logged (prompt version, sampling rate) +# without choosing the destination or the credentials, so they don't +# contribute to the data-exfil primitive that the rest of +# ``_supported_callback_params`` does. +_SAFE_CLIENT_CALLBACK_PARAMS: FrozenSet[str] = frozenset( + { + "langfuse_prompt_version", + "langsmith_sampling_rate", + } +) + +# Observability fields that integrations read from the request body or +# metadata but that are not (yet) listed in ``_supported_callback_params``. +# Listed here so the proxy bans them today; the long-term cleanup is to +# fold these into the canonical allowlist so they share one source of +# truth with the rest. +_EXTRA_BANNED_OBSERVABILITY_PARAMS: FrozenSet[str] = frozenset( + { + "posthog_api_url", + "phoenix_project_name", + "wandb_api_key", + "weave_project_id", + } +) + + +def _build_banned_observability_params() -> FrozenSet[str]: + """Derive the observability ban list from the canonical allowlist. + + ``_supported_callback_params`` in + ``litellm/litellm_core_utils/initialize_dynamic_callback_params.py`` is + the single place that enumerates every observability field + integrations resolve from kwargs/metadata. Subtract the small set of + informational fields (``_SAFE_CLIENT_CALLBACK_PARAMS``) and union with + the extras the canonical allowlist hasn't caught up to yet. New + integrations added to the canonical allowlist are banned by default, + which is the safe failure mode. + """ + from litellm.litellm_core_utils.initialize_dynamic_callback_params import ( + _supported_callback_params, + ) + + return ( + frozenset(_supported_callback_params) - _SAFE_CLIENT_CALLBACK_PARAMS + ) | _EXTRA_BANNED_OBSERVABILITY_PARAMS + + _BANNED_REQUEST_BODY_PARAMS: Tuple[str, ...] = ( "api_base", "base_url", @@ -190,11 +248,6 @@ _BANNED_REQUEST_BODY_PARAMS: Tuple[str, ...] = ( # tokens) to the attacker's host, or coerces the proxy into # authenticating against the attacker's host with admin secrets. "aws_bedrock_runtime_endpoint", - "langsmith_base_url", - "langfuse_host", - "posthog_host", - "braintrust_host", - "slack_webhook_url", # Provider-specific endpoint overrides that flow into the outbound # request via ``optional_params``. Same threat as ``api_base``: # ``s3_endpoint_url`` redirects Bedrock file uploads to attacker @@ -203,6 +256,11 @@ _BANNED_REQUEST_BODY_PARAMS: Tuple[str, ...] = ( "s3_endpoint_url", "sagemaker_base_url", "deployment_url", + # Observability credentials, hosts, and project identifiers: derived + # from the canonical ``_supported_callback_params`` allowlist so new + # integrations are covered automatically. Sorted for stable iteration + # order and reviewable diffs. + *sorted(_build_banned_observability_params()), ) @@ -221,6 +279,8 @@ def _check_banned_params( if param not in body: continue if general_settings.get("allow_client_side_credentials") is True: + # Proxy-wide opt-in: every banned param is permitted, exit + # entirely so the rest of the loop doesn't waste work. return if ( _allow_model_level_clientside_configurable_parameters( @@ -231,7 +291,12 @@ def _check_banned_params( ) is True ): - return + # Per-param opt-in: only THIS param is permitted by the + # deployment's ``configurable_clientside_auth_params``. Skip + # to the next banned param so a body that pairs an allowed + # ``api_base`` with an unallowed ``langfuse_host`` is still + # rejected for the second field. + continue raise ValueError( f"Rejected Request: {param} is not allowed in request body. " "Clientside passthrough requires explicit admin opt-in via " @@ -275,9 +340,33 @@ def is_request_body_safe( nested = request_body.get(nested_key) if isinstance(nested, dict): _check_banned_params(nested, general_settings, llm_router, model) + for metadata_key in _NESTED_METADATA_KEYS: + metadata = _coerce_metadata_to_dict(request_body.get(metadata_key)) + if metadata is not None: + _check_banned_params(metadata, general_settings, llm_router, model) return True +def _coerce_metadata_to_dict(value: Any) -> Optional[Dict[str, Any]]: + """Return ``value`` as a dict, parsing it from JSON if delivered as a string. + + Multipart/form-data and ``extra_body`` callers send ``litellm_metadata`` + as a JSON-encoded string; the proxy parses it into a dict later in + ``add_litellm_data_to_request``, but the auth-time bouncer runs first + and would otherwise miss the banned-param check on a still-stringified + metadata blob. + """ + if isinstance(value, dict): + return value + if isinstance(value, str): + from litellm.litellm_core_utils.safe_json_loads import safe_json_loads + + parsed = safe_json_loads(value) + if isinstance(parsed, dict): + return parsed + return None + + async def pre_db_read_auth_checks( request: Request, request_data: dict, diff --git a/litellm/proxy/litellm_pre_call_utils.py b/litellm/proxy/litellm_pre_call_utils.py index b05f69b439d..3b22397559f 100644 --- a/litellm/proxy/litellm_pre_call_utils.py +++ b/litellm/proxy/litellm_pre_call_utils.py @@ -121,6 +121,19 @@ _UNTRUSTED_ROOT_CONTROL_FIELDS = ( "pillar_response_headers", "_guardrail_pipelines", "_pipeline_managed_guardrails", + # Callback-registration fields. ``callbacks``, ``service_callback``, + # and ``logger_fn`` are read by ``litellm.utils.function_setup`` and + # appended to process-wide ``litellm.{input,success,failure,_async_*, + # service}_callback`` lists / ``litellm.user_logger_fn`` — one request + # poisons the worker for every subsequent caller. + # ``litellm_disabled_callbacks`` is the inverse primitive: the + # legitimate path reads it from key/team metadata, the request-body + # version silently turns off admin-configured audit/observability + # for the caller's request. + "callbacks", + "service_callback", + "logger_fn", + "litellm_disabled_callbacks", ) _UNTRUSTED_METADATA_CONTROL_FIELDS = ( diff --git a/tests/test_litellm/proxy/auth/test_auth_utils.py b/tests/test_litellm/proxy/auth/test_auth_utils.py index c146b5ded5b..7c04a4f61fb 100644 --- a/tests/test_litellm/proxy/auth/test_auth_utils.py +++ b/tests/test_litellm/proxy/auth/test_auth_utils.py @@ -1292,3 +1292,219 @@ class TestIsRequestBodySafeNestedConfig: ) is True ) + + +# ── observability-callback ban (root + metadata) ─────────────────────────── + + +class TestObservabilityCallbackBans: + """The proxy must reject observability credentials, hosts, and project + identifiers regardless of whether they arrive at the request body root, + in ``metadata`` / ``litellm_metadata``, or in a JSON-string-encoded + metadata blob (multipart/``extra_body`` path). + + The ban list is derived from + ``litellm.litellm_core_utils.initialize_dynamic_callback_params._supported_callback_params`` + minus a small ``_SAFE_CLIENT_CALLBACK_PARAMS`` allow-list, plus + ``_EXTRA_BANNED_OBSERVABILITY_PARAMS`` for fields integrations read but + that are not yet in the canonical allow-list. The derivation keeps the + proxy in sync as new integrations are added. + """ + + @pytest.fixture(autouse=True) + def _disable_url_validation(self, monkeypatch): + import litellm + + monkeypatch.setattr(litellm, "user_url_validation", False, raising=False) + + @pytest.mark.parametrize( + "field", + [ + "langfuse_public_key", + "langfuse_secret", + "langfuse_secret_key", + "langsmith_api_key", + "langsmith_project", + "langsmith_tenant_id", + "arize_api_key", + "arize_space_key", + "arize_space_id", + "posthog_api_key", + "posthog_api_url", + "braintrust_api_key", + "braintrust_project", + "phoenix_project_name", + "wandb_api_key", + "weave_project_id", + "gcs_bucket_name", + "gcs_path_service_account", + "humanloop_api_key", + "lunary_public_key", + ], + ) + def test_observability_field_in_request_body_root_is_rejected(self, field): + with pytest.raises(ValueError) as exc: + is_request_body_safe( + request_body={"model": "gpt-4", field: "attacker-value"}, + general_settings={}, + llm_router=None, + model="gpt-4", + ) + assert field in str(exc.value) + + @pytest.mark.parametrize( + "metadata_key", + ["metadata", "litellm_metadata"], + ) + @pytest.mark.parametrize( + "field", + [ + "langfuse_host", + "langfuse_secret_key", + "langsmith_api_key", + "posthog_api_url", + "braintrust_project", + "phoenix_project_name", + ], + ) + def test_observability_field_in_metadata_dict_is_rejected( + self, metadata_key, field + ): + # Verifies the metadata walk: a value smuggled inside ``metadata`` + # or ``litellm_metadata`` is just as dangerous as the same field + # at the body root, and must hit the same gate. + with pytest.raises(ValueError) as exc: + is_request_body_safe( + request_body={ + "model": "gpt-4", + metadata_key: {field: "attacker-value"}, + }, + general_settings={}, + llm_router=None, + model="gpt-4", + ) + assert field in str(exc.value) + + @pytest.mark.parametrize( + "metadata_key", + ["metadata", "litellm_metadata"], + ) + def test_observability_field_in_json_string_metadata_is_rejected( + self, metadata_key + ): + # Multipart/form-data and ``extra_body`` callers send metadata as a + # JSON-encoded string. The bouncer parses it before applying the + # banned-params check so the JSON-string path can't smuggle past + # the ``isinstance(dict)`` guard. + import json + + with pytest.raises(ValueError) as exc: + is_request_body_safe( + request_body={ + "model": "gpt-4", + metadata_key: json.dumps( + {"langfuse_host": "https://attacker.example"} + ), + }, + general_settings={}, + llm_router=None, + model="gpt-4", + ) + assert "langfuse_host" in str(exc.value) + + def test_admin_opt_in_allows_metadata_credential_passthrough(self): + # The opt-in gate covers the metadata path the same way it covers + # the root path — operators running BYO observability with + # clientside creds flip a single flag and both paths work. + assert ( + is_request_body_safe( + request_body={ + "model": "gpt-4", + "metadata": { + "langfuse_host": "https://my-langfuse.example", + "langfuse_public_key": "pk-mine", + "langfuse_secret_key": "sk-mine", + }, + }, + general_settings={"allow_client_side_credentials": True}, + llm_router=None, + model="gpt-4", + ) + is True + ) + + def test_safe_per_request_observability_metadata_is_allowed(self): + # Informational fields (sampling rate, prompt version) describe + # the request being logged — they don't choose the destination or + # credentials, so they must remain accepted from clients without + # the opt-in flag. + assert ( + is_request_body_safe( + request_body={ + "model": "gpt-4", + "metadata": { + "langfuse_prompt_version": "v2", + "langsmith_sampling_rate": 0.1, + }, + }, + general_settings={}, + llm_router=None, + model="gpt-4", + ) + is True + ) + + +def test_model_level_allow_does_not_skip_subsequent_banned_params(monkeypatch): + """Greptile P1: ``_check_banned_params`` previously ``return``-ed when a + deployment's ``configurable_clientside_auth_params`` permitted one + banned field, exiting before any later banned field in the same body + was checked. The metadata walk this PR adds multiplies the surface + where that bypass matters: a body pairing a model-level-allowed + ``api_base`` with an observability credential like ``langfuse_host`` + must still reject on the second field, not silently pass.""" + from litellm.proxy.auth import auth_utils + + monkeypatch.setattr( + auth_utils, + "_allow_model_level_clientside_configurable_parameters", + lambda model, param, request_body_value, llm_router: param == "api_base", + ) + + with pytest.raises(ValueError) as exc: + is_request_body_safe( + request_body={ + "model": "gpt-4", + "api_base": "https://allowed-by-deployment.example", + "langfuse_host": "https://attacker.example", + }, + general_settings={}, + llm_router=None, + model="gpt-4", + ) + assert "langfuse_host" in str(exc.value) + + +def test_observability_ban_covers_canonical_supported_callback_params(): + """Guard test: every entry in the canonical + ``_supported_callback_params`` allow-list must end up either banned by + the proxy or explicitly safe-listed. New integrations added to that + list are banned by default (the safe failure mode); flagging them as + safe is an explicit decision recorded in + ``_SAFE_CLIENT_CALLBACK_PARAMS``.""" + from litellm.litellm_core_utils.initialize_dynamic_callback_params import ( + _supported_callback_params, + ) + from litellm.proxy.auth.auth_utils import ( + _BANNED_REQUEST_BODY_PARAMS, + _SAFE_CLIENT_CALLBACK_PARAMS, + ) + + banned = set(_BANNED_REQUEST_BODY_PARAMS) + for param in _supported_callback_params: + assert param in banned or param in _SAFE_CLIENT_CALLBACK_PARAMS, ( + f"{param} is in _supported_callback_params but neither banned nor " + f"safe-listed. Add it to _SAFE_CLIENT_CALLBACK_PARAMS if it is an " + f"informational per-request field; otherwise the derivation will " + f"ban it automatically." + ) diff --git a/tests/test_litellm/proxy/test_litellm_pre_call_utils.py b/tests/test_litellm/proxy/test_litellm_pre_call_utils.py index 4e24d8af653..d2a1468be2a 100644 --- a/tests/test_litellm/proxy/test_litellm_pre_call_utils.py +++ b/tests/test_litellm/proxy/test_litellm_pre_call_utils.py @@ -596,6 +596,59 @@ async def test_add_litellm_data_to_request_strips_user_control_fields(): assert "pillar_response_headers" not in snapshot_body["metadata"] +@pytest.mark.asyncio +@pytest.mark.parametrize( + "control_field", + ["callbacks", "service_callback", "logger_fn", "litellm_disabled_callbacks"], +) +async def test_add_litellm_data_to_request_strips_callback_control_fields( + control_field, +): + """``callbacks`` / ``service_callback`` / ``logger_fn`` get appended to + the worker-wide ``litellm.{input,success,failure,_async_*,service}_callback`` + lists and ``litellm.user_logger_fn`` from inside ``function_setup`` — + one request poisons every subsequent caller in that worker. + ``litellm_disabled_callbacks`` is the inverse: a request-body value + silently disables admin-configured audit/observability for the call. + None has a documented per-request use, so all four are stripped at + the proxy boundary alongside the existing internal-only fields.""" + request_mock = MagicMock(spec=Request) + request_mock.url.path = "/v1/chat/completions" + request_mock.url = MagicMock() + request_mock.url.__str__.return_value = "http://localhost/v1/chat/completions" + request_mock.method = "POST" + request_mock.query_params = {} + request_mock.headers = {"Content-Type": "application/json"} + request_mock.client = MagicMock() + request_mock.client.host = "127.0.0.1" + + sample_value = ( + ["langfuse"] + if control_field + in ("callbacks", "service_callback", "litellm_disabled_callbacks") + else "module.func" + ) + + updated = await add_litellm_data_to_request( + data={ + "model": "gpt-3.5-turbo", + "messages": [{"role": "user", "content": "hi"}], + control_field: sample_value, + }, + request=request_mock, + user_api_key_dict=UserAPIKeyAuth(api_key="hashed-key"), + proxy_config=MagicMock(), + general_settings={}, + version="test-version", + ) + + assert control_field not in updated + # The post-strip body snapshot used by audit/spend logging must also + # not retain the attacker-injected control field. + snapshot_body = updated["proxy_server_request"]["body"] + assert control_field not in snapshot_body + + @pytest.mark.asyncio async def test_add_litellm_data_to_request_allows_client_mock_response_with_admin_opt_in(): request_mock = MagicMock(spec=Request)