chore(proxy): backport #27898 + #27801 to 1.84.0rc2#27902
Conversation
Greptile SummaryThis backport brings two security-hardening changes from
Confidence Score: 4/5Safe to merge; both findings only manifest under adversarial PROXY_ADMIN input and do not affect normal operation. The gate and scrub logic correctly close the documented attack vectors and all config-load paths are updated consistently. The two gaps found — double-scrub of v2 guardrail litellm_params (harmless) and unguarded target=None in pass_through route registration — are minor and do not affect normal operation. litellm/proxy/proxy_server.py (guardrail double-scrub, pass_through target=None handling) and litellm/proxy/pass_through_endpoints/pass_through_endpoints.py (no null guard for target=None in create_pass_through_route)
|
| Filename | Overview |
|---|---|
| litellm/proxy/types_utils/utils.py | Adds a runtime gate that rejects s3:// / gcs:// remote-module loads when config_file_path is None; clean and well-tested. |
| litellm/proxy/auth/auth_utils.py | Extends banned-params descent to extra_body with _coerce_metadata_to_dict coercion and adds azure_ad_token to the ban list; symmetric with existing metadata key handling. |
| litellm/proxy/proxy_server.py | Adds _scrub_db_overlay_remote_module_loads and threads config_file_path through the load chain; v2 guardrail litellm_params is scrubbed twice (harmless), and scrubbed target=None in pass_through_endpoints is not guarded downstream. |
| litellm/proxy/_types.py | Pops config_file_path from kwargs in LiteLLM_JWTAuth.init before unknown-keys check, then threads it to get_instance_fn for custom_validate; clean change. |
| litellm/proxy/pass_through_endpoints/pass_through_endpoints.py | Propagates config_file_path through create_pass_through_route and initialize_pass_through_endpoints; no null guard for target=None after scrubbing. |
| litellm/proxy/_experimental/mcp_server/tool_registry.py | Adds config_file_path parameter to load_tools_from_config and passes it to get_instance_fn; straightforward threading change. |
| tests/test_litellm/proxy/auth/test_banned_params_extra_body.py | New tests covering extra_body banned-param descent including stringified-JSON form and the allow_client_side_credentials escape; comprehensive. |
| tests/test_litellm/proxy/types_utils/test_db_overlay_remote_module_scrub.py | New tests for _scrub_db_overlay_remote_module_loads covering all field shapes; mutation-safety and passthrough cases covered. |
| tests/test_litellm/proxy/types_utils/test_get_instance_fn_runtime_gate.py | New tests verifying the get_instance_fn gate for both schemes, the config-file-path allow path, and threading through pass_through and MCP registry. |
| tests/proxy_unit_tests/test_custom_logger_s3_gcs.py | Existing tests updated to pass config_file_path for s3:// / gcs:// URLs, preserving intent without weakening coverage. |
Comments Outside Diff (2)
-
litellm/proxy/proxy_server.py, line 3353-3366 (link)Redundant double-scrub of v2 guardrail
litellm_paramsFor v2-shaped guardrail entries (
{"guardrail_name": "...", "litellm_params": {...}}),_scrub_guardrail_inneris called twice on the samelitellm_paramsdict — once in the genericfor inner in entry.values()loop (becauselitellm_paramsvalue is a dict) and once via the explicitlp = entry.get("litellm_params")path below. The second call is a no-op (both calls mutate the same deep-copied object), but the intent is unclear at a glance. Excludinglitellm_paramsfrom the generic loop would make the v1/v2 distinction explicit. -
litellm/proxy/proxy_server.py, line 3383-3398 (link)Scrubbed
target: Noneis not guarded against in downstream route registrationWhen a DB-overlay
pass_through_endpointsentry has its remote-URLtargetscrubbed toNone, the entry remains in the list. The test comment notes it "can still be skipped explicitly downstream", butcreate_pass_through_routehas no null guard: it callsget_instance_fn(value=None, ...)which hitsNone.startswith("s3://")and raisesAttributeError. If the surroundingtryblock does not catchAttributeError, the config-reload cycle would fail. An explicitif target is None: returnguard in_register_pass_through_endpointorcreate_pass_through_routewould make the intended skip behavior concrete.
Reviews (1): Last reviewed commit: "Merge pull request #27801 from stuxf/cho..." | Re-trigger Greptile
Backports two request-body / module-load hardening PRs onto
litellm_1.84.0rc2.Backports
chore(proxy): cover extra_body + azure_ad_token in banned-params checkchore(proxy): refuse remote-URL instance-fn loads outside config-file pathBoth are squash-merged on
litellm_internal_staging; this branch cherry-picks each merge commit as a single backport commit (-m 1 -x).Conflict resolution
rc2 is 969 commits behind staging from the merge-base, but git auto-merged the three overlapping files cleanly with no manual conflict resolution:
litellm/proxy/proxy_server.py— line offsets only (rc2 has fewer lines above the patch site)litellm/proxy/_types.py— line offsets onlylitellm/proxy/_experimental/mcp_server/tool_registry.py— line offsets onlyVerified by comparing per-file diffs
git diff <pr_merge>^1..<pr_merge>(the PR's own hunk) againstgit diff origin/litellm_1.84.0rc2..HEAD(what we landed): byte-identical content, only line numbers differ. No staging-only features leaked in (e.g.TEAM_KEY_BULK_UPDATE,compliance_check_routes,unregister_tools_with_prefix,delegate_auth_to_upstreamare correctly absent on this backport since they belong to other PRs).Test plan
pytest tests/test_litellm/proxy/auth/test_banned_params_extra_body.py tests/test_litellm/proxy/types_utils/test_get_instance_fn_runtime_gate.py tests/test_litellm/proxy/types_utils/test_db_overlay_remote_module_scrub.py— 38 passedpytest tests/proxy_unit_tests/test_custom_logger_s3_gcs.py— 11 passedpytest tests/test_litellm/proxy/auth/— 808 passed