chore(proxy): close callback-config and observability-credential side channels - #27081
chore(proxy): close callback-config and observability-credential side channels#27081yuneng-berri merged 2 commits into
Conversation
… channels
Two related gaps in the proxy's request bouncer:
1. ``is_request_body_safe`` (auth_utils.py) walked the request-body root
and the ``litellm_embedding_config`` nested dict, but not ``metadata``
or ``litellm_metadata``. The same fields it bans at root — Langfuse /
Langsmith / Arize / PostHog / Braintrust / Phoenix / W&B Weave / GCS /
Humanloop / Lunary credentials and routing — were silently accepted
when the caller put them inside metadata, retargeting observability
callbacks to a caller-controlled host with caller-supplied creds.
Walk both metadata containers (and parse the JSON-string form sent via
multipart / ``extra_body``) through the same banned-params helper, so
the existing ``allow_client_side_credentials`` opt-in covers both
paths consistently.
2. The banned-params list was hand-maintained and lagged the canonical
``_supported_callback_params`` allow-list in
``initialize_dynamic_callback_params``. Derive the observability bans
from that allow-list (minus a small ``_SAFE_CLIENT_CALLBACK_PARAMS``
set for informational fields like ``langfuse_prompt_version`` and
``langsmith_sampling_rate``) so future integrations are covered
automatically; ``_EXTRA_BANNED_OBSERVABILITY_PARAMS`` carries the
handful of fields integrations read but the allow-list hasn't caught
up to. A guard test fails CI if a new entry is added to
``_supported_callback_params`` without an explicit safe-list decision.
Separately in ``litellm_pre_call_utils.py``: add ``callbacks``,
``service_callback``, ``logger_fn``, and ``litellm_disabled_callbacks``
to ``_UNTRUSTED_ROOT_CONTROL_FIELDS``. The first three are appended to
worker-wide ``litellm.{input,success,failure,_async_*,service}_callback``
lists / ``litellm.user_logger_fn`` from inside ``function_setup`` — one
request poisons every subsequent caller in that worker. The last is the
inverse primitive: the legitimate path reads it from key/team metadata,
the request-body version silently disables admin-configured audit /
observability for the call.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR closes two observability side-channel gaps in the proxy's request bouncer: it extends the banned-param check to walk Confidence Score: 5/5Safe to merge — all three gaps are correctly closed, the previously flagged P1 (return→continue) is fixed, and tests cover root/metadata/JSON-string paths plus a guard test for the canonical allow-list. No P0 or P1 issues found. The return→continue regression is corrected, the metadata walk and derived ban list are correctly implemented, circular import is not a risk, and the callback control fields are stripped before any re-addition from trusted DB metadata. Only a minor style observation about hoisting the allow_client_side_credentials check before the loop. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/auth_utils.py | Core security fix: adds metadata path checking, derives observability ban list from canonical allow-list, and fixes the return→continue bug in _check_banned_params so a model-level-allowed param can't shadow subsequent banned params. |
| litellm/proxy/litellm_pre_call_utils.py | Adds callbacks, service_callback, logger_fn, and litellm_disabled_callbacks to _UNTRUSTED_ROOT_CONTROL_FIELDS to prevent worker-wide callback poisoning and unauthorized observability disabling. |
| tests/test_litellm/proxy/auth/test_auth_utils.py | Adds TestObservabilityCallbackBans covering root/metadata/JSON-string paths, allow_client_side_credentials opt-in, safe params passthrough, regression test for the return→continue fix, and guard test ensuring _supported_callback_params is fully covered. |
| tests/test_litellm/proxy/test_litellm_pre_call_utils.py | Parametrized test verifying each of the four new control fields is stripped from both data and the proxy_server_request.body snapshot; no real network calls made. |
Reviews (2): Last reviewed commit: "fix(auth): per-param allow must continue..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _SAFE_CLIENT_CALLBACK_PARAMS: FrozenSet[str] = frozenset( | ||
| { | ||
| "langfuse_prompt_version", | ||
| "langsmith_sampling_rate", | ||
| } |
There was a problem hiding this comment.
_SAFE_CLIENT_CALLBACK_PARAMS may be too narrow for session/trace fields
The safe-list currently covers only langfuse_prompt_version and langsmith_sampling_rate. The PR description explicitly calls out langfuse_session_id, langfuse_tags, trace_id, helicone_meta, langsmith_metadata, span identifiers, lago_api_event_code, and openmeter_event_type as unaffected — these don't choose the destination or credentials. These fields are not in _supported_callback_params today (so they won't be banned), but if any get added to that canonical list later they will be banned by default and break existing callers unless this safe-list is updated simultaneously.
A pre-existing logic bug in ``_check_banned_params``: when the deployment-level ``configurable_clientside_auth_params`` permitted one banned field, the loop ``return``-ed on the first match instead of ``continue``-ing, so any other banned param later in the same body or metadata dict was never checked. This PR's metadata walk multiplies the surface where that bypass matters — a body pairing an allowed ``api_base`` with an observability credential like ``langfuse_host`` would silently pass. Proxy-wide ``allow_client_side_credentials`` keeps ``return`` (it's a global opt-in for every banned param). The per-param branch becomes ``continue`` so only the one explicitly-permitted field is skipped. Adds a regression test that exercises the api_base + langfuse_host pair. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
e4ac46b
into
BerriAI:litellm_internal_staging
chore(proxy): close callback-config and observability-credential side channels
…dy bouncer Two cleanups: * ``LiteLLMSkillsHandler.create_skill`` raised ``HTTPException`` for identity-less callers, importing FastAPI from a ``litellm/llms/`` module — that violates the project rule that FastAPI lives only under ``proxy/``. Switch to ``ValueError`` (the same shape the rest of the handler uses for not-found/forbidden) and update the test. * The proxy-auth body bouncer derived its observability ban list from ``_supported_callback_params`` only, missing ``_request_blocked_callback_params`` (where ``gcs_bucket_name`` and ``gcs_path_service_account`` live). Two recently-merged sibling PRs (BerriAI#27019 added the deny list, BerriAI#27081 added the test asserting these are rejected at the request body root) crossed without folding them together. Union the GCS deny list into the bouncer's derivation so the single source of truth covers both code paths.
2 participants
Type
🐛 Bug Fix
✅ Test
Changes
Two related gaps in the proxy's request bouncer:
is_request_body_safe(auth_utils.py) previously walked the request-body root and thelitellm_embedding_confignested dict, but notmetadata/litellm_metadata. The same fields it bans at the root — Langfuse / Langsmith / Arize / PostHog / Braintrust / Phoenix / W&B Weave / GCS / Humanloop / Lunary credentials and routing — were silently accepted when the caller put them inside metadata, retargeting observability callbacks to a caller-controlled host with caller-supplied credentials. Walk both metadata containers (and parse the JSON-string form sent via multipart /extra_body) through the same banned-params helper, so the existinggeneral_settings.allow_client_side_credentialsopt-in covers both paths consistently.Derived ban list. The banned-params list was hand-maintained and lagged the canonical
_supported_callback_paramsallow-list inlitellm/litellm_core_utils/initialize_dynamic_callback_params.py. Derive the observability bans from that allow-list (minus a small_SAFE_CLIENT_CALLBACK_PARAMSset for informational fields likelangfuse_prompt_versionandlangsmith_sampling_rate) so new integrations are covered automatically;_EXTRA_BANNED_OBSERVABILITY_PARAMScarries the handful of fields integrations read but the allow-list hasn't caught up to (posthog_api_url,phoenix_project_name,wandb_api_key,weave_project_id). A guard test fails CI if a new entry is added to_supported_callback_paramswithout an explicit safe-list decision.Control-field strip in
litellm_pre_call_utils.py. Addcallbacks,service_callback,logger_fn, andlitellm_disabled_callbacksto_UNTRUSTED_ROOT_CONTROL_FIELDS. The first three are appended to worker-widelitellm.{input,success,failure,_async_*,service}_callbacklists /litellm.user_logger_fnfrom insidefunction_setup— one request poisons every subsequent caller in that worker. The last is the inverse primitive: the legitimate path reads it from key/team metadata, the request-body version silently disables admin-configured audit / observability for the call.Compatibility
import litellm; litellm.completion(...)): unaffected. Both changes only run in the proxy ingress path.general_settings.allow_client_side_credentials = trueif they haven't already — the same flag that already gates the root path. Operators using only the metadata path were inadvertently bypassing this gate; this PR closes that gap.langfuse_session_id,langfuse_tags,trace_id,helicone_meta,langsmith_metadata, span identifiers,lago_api_event_code,openmeter_event_type, etc.): unaffected — these don't choose the destination or the credentials.callbacks/service_callback/logger_fn/litellm_disabled_callbacksin request bodies: stripped silently; no documented per-request use, the legitimate equivalent is admin-configured callbacks on the proxy.Test Plan
tests/test_litellm/proxy/auth/test_auth_utils.py— addsTestObservabilityCallbackBans: each new field rejected at root, each rejected frommetadataandlitellm_metadata(dict + JSON-string forms),allow_client_side_credentialsallows the metadata BYO path, informationallangfuse_prompt_version/langsmith_sampling_ratestill pass through. Guard test asserts coverage of_supported_callback_params.tests/test_litellm/proxy/test_litellm_pre_call_utils.py— parametrized test that each of the four new control fields is stripped from bothdataand the post-stripproxy_server_request.bodysnapshot.🤖 Generated with Claude Code