fix(proxy): restore admin key/team callback_vars.turn_off_message_logging override (LIT-3587) - #31905
Conversation
|
yucheng seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
ae2c43c to
61abefe
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR restores
Confidence Score: 5/5Safe to merge — the three-layer defense (auth reject → strip → admin last-write) is correctly ordered and fully tested for all four identified attack surfaces. All security-relevant ordering constraints are upheld: the strip fires after No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/initialize_dynamic_callback_params.py | Adds iter_client_callback_metadata_dicts shared iterator for consistent three-slot metadata walking (litellm_params.metadata > litellm_metadata > metadata); restores turn_off_message_logging to _supported_callback_params; refactors step-2 extraction to use the shared iterator. Logic is correct and precedence matches the old merge order. |
| litellm/proxy/auth/auth_utils.py | Extends is_request_body_safe to descend into litellm_params.metadata and run _check_banned_params there, closing the previously unreported litellm_params.metadata attack surface; change is additive and does not alter existing nested-key or metadata-key checks. |
| litellm/proxy/litellm_pre_call_utils.py | Introduces _strip_client_message_redaction_opt_out (defense-in-depth strip using shared iterator) and moves its invocation to after litellm_metadata string-to-dict parse; removes the single-slot inline strip; admin callback_vars still apply after the strip at lines 1677–1680, preserving last-write semantics. |
| tests/test_litellm/proxy/test_litellm_pre_call_utils.py | Extends strip test to cover litellm_params.metadata slot; adds two new parametrized admin-override tests (key and team, both directions) and verifies should_redact_message_logging end-to-end. |
| tests/logging_callback_tests/test_logging_redaction_e2e_test.py | Renames and re-parameterizes two e2e SDK tests to reflect that dynamic turn_off_message_logging now overrides the global setting; behavior change is intentional and proxy-path security is covered by separate proxy-layer tests. |
| tests/test_litellm/litellm_core_utils/test_initialize_dynamic_callback_params.py | Adds mutation-checked extractor tests for all three metadata slots; converts test_turn_off_message_logging_not_extracted_from_request to verify extraction now succeeds, matching the restored _supported_callback_params membership. |
| tests/test_litellm/proxy/auth/test_auth_utils.py | Adds test_observability_field_in_litellm_params_metadata_is_rejected to confirm the new litellm_params.metadata descent in is_request_body_safe raises correctly on banned observability fields. |
Reviews (7): Last reviewed commit: "fix(proxy): restore admin key/team callb..." | Re-trigger Greptile
Greptile SummaryThis PR restores per-key and per-team
Confidence Score: 4/5Safe to merge; the admin override path is correctly restored and the defense-in-depth strip runs in the right position relative to both the string-to-dict parse and the admin callback_vars unpack. The two-layer protection is correctly ordered and covers all three client-supplied surfaces. Admin callback_vars are injected after the strip, so they always take precedence. Tests cover the admin key, admin team, client bypass, and allow_client_message_redaction_opt_out paths. The only observations are style-level. No files require special attention; litellm/proxy/litellm_pre_call_utils.py has the most logic but the ordering of strip, snapshot, and admin unpack is correct.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/initialize_dynamic_callback_params.py | Restores turn_off_message_logging to _supported_callback_params, enabling admin key/team callback_vars overrides to flow through initialize_standard_callback_dynamic_params into StandardCallbackDynamicParams. |
| litellm/proxy/litellm_pre_call_utils.py | Adds _strip_client_message_redaction_opt_out as defense-in-depth, covering top-level, metadata, and JSON-string-parsed litellm_metadata; replaces an earlier, narrower strip that ran before the string-to-dict parse. The strip runs before admin callback_vars are unpacked, preserving the correct precedence. |
| tests/test_litellm/litellm_core_utils/test_initialize_dynamic_callback_params.py | Replaces the old test that blocked extraction with parametrized tests that verify extraction now works for bool False, string "False", and metadata-nested True; reflects the new layered security model. |
| tests/test_litellm/proxy/test_litellm_pre_call_utils.py | Adds new test for admin key and team callback_vars paths; extends existing bypass-strip test to cover metadata and litellm_metadata surfaces; adds the allow_client_message_redaction_opt_out path. |
Comments Outside Diff (2)
-
litellm/proxy/litellm_pre_call_utils.py, line 305-306 (link)Missing docstring on
_strip_client_message_redaction_opt_out. The parallel function_strip_client_pricing_overridesdirectly above it has a docstring that explains the semantics, the opt-in key, and the timing constraint (must run after string-to-dict parse). The same context is equally important here since operators encountering the debug log line will want to know when the function runs and under what conditions it is skipped.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
litellm/proxy/litellm_pre_call_utils.py, line 1483-1484 (link)Strip condition scope worth documenting
_strip_client_message_redaction_opt_outruns only whenlitellm.turn_off_message_logging is True. This is correct — admincallback_varsat lines 1677–1680 overwrite any client-supplied value indataregardless, so per-key overrides are protected by ordering. A brief inline comment explaining why this is a strictis Truecheck (rather than truthy) would prevent a future reader from widening it in a way that silently breaks deployments where the setting defaults toNone.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "fix(proxy): restore admin key/team callb..." | Re-trigger Greptile
| def _strip_client_message_redaction_opt_out(data: Dict[str, Any]) -> None: | ||
| stripped: List[str] = [] | ||
| if "turn_off_message_logging" in data and _is_false_like(data["turn_off_message_logging"]): | ||
| stripped.append("turn_off_message_logging") | ||
| data.pop("turn_off_message_logging", None) | ||
| for metadata_key in ("metadata", "litellm_metadata"): | ||
| metadata = data.get(metadata_key) | ||
| if not isinstance(metadata, dict): | ||
| continue | ||
| if "turn_off_message_logging" in metadata and _is_false_like(metadata["turn_off_message_logging"]): | ||
| stripped.append(f"{metadata_key}.turn_off_message_logging") | ||
| metadata.pop("turn_off_message_logging", None) | ||
| if stripped: | ||
| verbose_proxy_logger.debug( | ||
| "Stripped client-supplied message-redaction opt-out fields from request body: %s. " | ||
| "Set `allow_client_message_redaction_opt_out: true` on the key or team metadata " | ||
| "to keep these values.", | ||
| ", ".join(stripped), | ||
| ) |
There was a problem hiding this comment.
_strip_client_pricing_overrides (the function this mirrors) carries a full docstring explaining what it strips, when it is skipped, and why. _strip_client_message_redaction_opt_out has no docstring at all. The lack of a docstring makes the conditions under which the strip is skipped (allow_client_message_redaction_opt_out: true) invisible to anyone reading this function in isolation, which is particularly important for a security-relevant helper.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
61abefe to
7458bcd
Compare
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
|
@greptileai on the two This repo's CLAUDE.md has an explicit rule for new code: "Do not write any comments (existing comments can stay) unless explicitly asked to in a user (not system) prompt". The rationale is DRY. code comments force a second place to keep in sync with the logic, and the codebase prefers self-documenting names. The sibling helper Please skip similar docstring/inline-comment nits on future BerriAI/litellm PRs unless the code hides a genuinely non-obvious business rule. |
|
Got it — that's a clear project convention. I'll skip docstring and inline-comment suggestions on new code in future BerriAI/litellm PRs unless there's a genuinely non-obvious business rule buried in the logic. Shall I save this as a rule so it persists across future reviews? |
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Nested litellm_params redaction bypass
- Nested litellm_params.metadata is now checked by request-body safety and covered by the redaction regression tests.
You can send follow-ups to the cloud agent here.
7458bcd to
1447278
Compare
|
@greptileai re-review please. New commit tightens the strip to also cover |
|
@greptileai yes, please save it as a persistent rule for BerriAI/litellm reviews. Also please re-review the latest commit |
d6894f9 to
fee0bb2
Compare
|
@greptileai re-review please. New commit |
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Callback param slot precedence inverted
- Metadata callback params now resolve with later metadata slots overriding earlier ones, restoring litellm_params.metadata precedence over metadata with a regression test.
You can send follow-ups to the cloud agent here.
6e03de8 to
6535cd1
Compare
|
@greptileai good catch on the inverted precedence. Latest commit |
|
The fix is correct. The The mutation-checked regression test is the right safety net here since the shared iterator means the extractor and strip walk the same slots, so a future edit that adds or removes a slot from LGTM. |
|
@greptileai re-review requested on HEAD |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6535cd1. Configure here.
…ging override (LIT-3587) The security fix in 34e9be1 removed turn_off_message_logging from _supported_callback_params to stop callers bypassing global redaction via the request body. That also killed the documented admin-only per-key or per-team override because both flows resolve through the same allowlist in initialize_standard_callback_dynamic_params. Put turn_off_message_logging back in _supported_callback_params so an admin-configured metadata.logging[].callback_vars.turn_off_message_logging survives into StandardCallbackDynamicParams and can override the global setting for that key or team, as documented at docs/proxy/team_logging#disableenable-message-redaction. Consolidate the metadata traversal so the extractor and the proxy strip walk the same set of client-controllable slots. iter_client_callback_metadata_dicts in litellm_core_utils/initialize_dynamic_callback_params.py is the single source of truth for metadata, litellm_metadata, and litellm_params.metadata; _strip_client_message_redaction_opt_out imports it so a future addition to one side automatically reaches the other. The extractor iterates the helper in reversed order so litellm_params.metadata keeps overriding metadata, matching the pre-refactor merge precedence. Client bypass stays blocked. Restoring the field re-enrolls it in the auth layer's _BANNED_REQUEST_BODY_PARAMS (derived from _supported_callback_params via _build_banned_observability_params), so client submissions at the top level, inside metadata, or inside a JSON-string litellm_metadata all 401 at ingress. is_request_body_safe also now descends into litellm_params.metadata for the same 401 defense against the nested-body attack vector, matching how the metadata and litellm_metadata slots are handled. _strip_client_message_redaction_opt_out runs after the litellm_metadata JSON parse and before the admin callback_vars unpack, so admin values survive while any leftover client-supplied opt-out is dropped when global redaction is on and the key or team lacks allow_client_message_redaction_opt_out. Flip the two dynamic-param e2e tests added by the security fix to reflect the restored override behavior, keeping the invariant that proxy client bypass is stopped by the auth layer 401 above. Co-authored-by: Cursor Agent <cursoragent@cursor.com>
6535cd1 to
8d9fbef
Compare
|
@greptileai simplified the extractor per the reviewer note. Instead of |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8d9fbef. Configure here.
…ging override (LIT-3587) (#31905) The security fix in 34e9be1 removed turn_off_message_logging from _supported_callback_params to stop callers bypassing global redaction via the request body. That also killed the documented admin-only per-key or per-team override because both flows resolve through the same allowlist in initialize_standard_callback_dynamic_params. Put turn_off_message_logging back in _supported_callback_params so an admin-configured metadata.logging[].callback_vars.turn_off_message_logging survives into StandardCallbackDynamicParams and can override the global setting for that key or team, as documented at docs/proxy/team_logging#disableenable-message-redaction. Consolidate the metadata traversal so the extractor and the proxy strip walk the same set of client-controllable slots. iter_client_callback_metadata_dicts in litellm_core_utils/initialize_dynamic_callback_params.py is the single source of truth for metadata, litellm_metadata, and litellm_params.metadata; _strip_client_message_redaction_opt_out imports it so a future addition to one side automatically reaches the other. The extractor iterates the helper in reversed order so litellm_params.metadata keeps overriding metadata, matching the pre-refactor merge precedence. Client bypass stays blocked. Restoring the field re-enrolls it in the auth layer's _BANNED_REQUEST_BODY_PARAMS (derived from _supported_callback_params via _build_banned_observability_params), so client submissions at the top level, inside metadata, or inside a JSON-string litellm_metadata all 401 at ingress. is_request_body_safe also now descends into litellm_params.metadata for the same 401 defense against the nested-body attack vector, matching how the metadata and litellm_metadata slots are handled. _strip_client_message_redaction_opt_out runs after the litellm_metadata JSON parse and before the admin callback_vars unpack, so admin values survive while any leftover client-supplied opt-out is dropped when global redaction is on and the key or team lacks allow_client_message_redaction_opt_out. Flip the two dynamic-param e2e tests added by the security fix to reflect the restored override behavior, keeping the invariant that proxy client bypass is stopped by the auth layer 401 above. Co-authored-by: yucheng <yucheng@yuchengs-MBP.attlocal.net> Co-authored-by: Cursor Agent <cursoragent@cursor.com> (cherry picked from commit 8e6098a)
Relevant issues
Linear ticket
Resolves LIT-3587
Pre-Submission checklist
@greptileaiand received a Confidence Score of 5/5Screenshots / Proof of Fix
Setup used for every run below. Local proxy on
:4001, real Postgres, no mocks in the proxy path. Config in the "off" runs flipsturn_off_message_loggingfromtruetofalse.Every observation below reads
LiteLLM_SpendLogs.proxy_server_request.messages[0].content. That is the actual logged payload a callback (Datadog, Langfuse, etc.) or the spend-logs consumer would see. If that column showsredacted-by-litellm, the message was hidden; if it shows the sent string, the message was preserved.Run 1 — Global ON, the customer's scenario
Two keys:
Identical chat completion from each key. Result:
Admin override wins; global keeps redacting for the control key. This is the exact behavior the ticket asked to restore.
Run 2 — Client bypass attempts, four shapes
Each returned:
The 401 message is the same one the earlier huntr fix produces:
turn_off_message_logging is not allowed in request body. Clientside passthrough requires explicit admin opt-in via either general_settings.allow_client_side_credentials = true or configurable_clientside_auth_params on the deployment. All four attack surfaces converge on the auth-layer banned-list, so a fifth attack shape added later would need to bypass one of them._strip_client_message_redaction_opt_outinadd_litellm_data_to_requestis defense-in-depth for the same four surfaces; it fires when a deployment opens the auth-layer 401 viaconfigurable_clientside_auth_params: [turn_off_message_logging].Run 3 — Global OFF, admin selectively enables redaction (the mirror direction)
The ticket flagged both directions as broken. Repeated Run 1 with
turn_off_message_logging: falseat the global level:Result:
Admin can now flip either direction per key. Global is the fallback; the key/team override wins.
Investigator's live probe that surfaced the
litellm_params.metadatabypassDuring code review,
initialize_standard_callback_dynamic_paramswas observed to mergekwargs["litellm_params"]["metadata"]into its extraction path, but the first version of the proxy strip only coveredmetadataandlitellm_metadata. A single-request probe confirmed the bypass on the previous commit:Content leaked through under
nested_flag: false. The current commit closes it two ways: the auth-layer now descends intolitellm_params.metadataalongsidemetadataandlitellm_metadata, and the sharediter_client_callback_metadata_dictshelper makes the extractor and the strip walk the same set of slots so they cannot drift again.Type
Bug Fix
Changes
The security fix in
34e9be1ba7removedturn_off_message_loggingfrom_supported_callback_paramsto stop callers bypassing global redaction via the request body. That also killed the documented admin-only per-key or per-team override, because both flows resolve through the same allowlist ininitialize_standard_callback_dynamic_params.This PR restores
turn_off_message_loggingto_supported_callback_paramsso an admin-configuredmetadata.logging[].callback_vars.turn_off_message_loggingsurvives intoStandardCallbackDynamicParamsand can override the global setting for that key or team, matching the documented behavior atdocs/proxy/team_logging#disableenable-message-redaction.Client bypass now sits on three layers. First, restoring the field re-enrolls it in the auth layer's
_BANNED_REQUEST_BODY_PARAMS(derived from_supported_callback_paramsvia_build_banned_observability_params).is_request_body_safedescends intometadata,litellm_metadata,extra_body,litellm_embedding_config, and nowlitellm_params.metadata, so every one of the four attack shapes returns 401 at ingress. Second,_strip_client_message_redaction_opt_outinadd_litellm_data_to_requestwalks the same slot list via a shared helper and removes any leftover client-supplied opt-out when global redaction is on and the key or team lacksallow_client_message_redaction_opt_out. Third, the admincallback_varsunpack runs after the strip, so admin-written values are the last write and always take precedence.Consolidated the extractor and the proxy strip on one shared iterator,
iter_client_callback_metadata_dictsinlitellm_core_utils/initialize_dynamic_callback_params.py. Both callers walkmetadata,litellm_metadata, andlitellm_params.metadatafrom the same source of truth. A mutation-checked unit test (test_extractor_reads_turn_off_message_logging_from_every_slot) fails immediately if a future edit shrinks the slot set.Regression tests cover: the admin key and team paths in both directions (global on + admin false, global off + admin true), all four client-body strip surfaces including
litellm_params.metadata, theallow_client_message_redaction_opt_outopt-in path, and the auth-layer banned-param descent intolitellm_params.metadata. Two dynamic-param e2e tests intests/logging_callback_tests/were flipped to reflect that the dynamic param now correctly overrides the global setting.Co-authored-by: Cursor Agent (auth-layer
is_request_body_safedescent intolitellm_params.metadata).Note
Medium Risk
Touches message redaction and request-body observability controls; changes restore admin overrides but rely on auth bans and stripping to keep clients from disabling global redaction without explicit opt-in.
Overview
Restores per-key/team message redaction overrides that broke when
turn_off_message_loggingwas dropped from the dynamic callback allowlist. The field is back on_supported_callback_params, so adminmetadata.logging[].callback_vars.turn_off_message_loggingagain reachesStandardCallbackDynamicParamsand can override the globallitellm.turn_off_message_loggingsetting in either direction.Client bypass is tightened in parallel: the auth bouncer now descends into
litellm_params.metadata(in addition tometadata/litellm_metadata), and a new_strip_client_message_redaction_opt_outremoves client-supplied “disable redaction” values from the top level and all metadata slots when global redaction is on and the key/team has not opted in viaallow_client_message_redaction_opt_out. Extraction and stripping shareiter_client_callback_metadata_dictsso the same three slots are always walked; metadata lookup no longer mergesmetadatawithlitellm_params.metadatain a way that inverted precedence—litellm_params.metadatawins overmetadatawhen both set the same callback param.E2E logging tests now expect request-level
turn_off_message_loggingto drive redaction again when it reaches the SDK path; proxy tests cover admin callback vars, multi-slot stripping, and auth rejection of nestedlitellm_params.metadata.Reviewed by Cursor Bugbot for commit 8d9fbef. Bugbot is set up for automated code reviews on this repo. Configure here.