fix(proxy): extend banned-params + admin-clear lists (VERIA-493) - #31742
Conversation
Merging this PR will improve performance by 23.24%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_completion_with_tools |
4.2 ms | 3.2 ms | +31.39% |
| ⚡ | test_completion_simple_message |
4.7 ms | 4 ms | +15.59% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing litellm_veria_493_nvidia_riva_nvcf (019afa8) with litellm_internal_staging (a0b26d2)
Greptile SummaryThis PR adds two NVIDIA Riva-specific fields (
Confidence Score: 5/5The change is additive and consistent with the existing provider-field pattern; both admin opt-in escape hatches are preserved and well-tested. Two fields are appended to two existing lists using the same pattern as the OCI, Bedrock, and SageMaker entries already there. All new tests are mock-only and cover both changed files including the per-deployment opt-in path. No existing tests are weakened. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/auth_utils.py | Adds nvcf_function_id and use_ssl to _BANNED_REQUEST_BODY_PARAMS, consistent with the existing provider-specific endpoint fields in the same list |
| litellm/router_utils/clientside_credential_handler.py | Adds the same two fields to kwargs_only_fields so admin-pinned values are cleared when a caller redirects api_base, parallel to the existing OCI entries |
| tests/test_litellm/proxy/auth/test_auth_utils.py | Adds two new test classes covering root-level rejection, api_key co-presence, proxy-wide opt-in, per-deployment opt-in (monkeypatched), nested smuggling, and api_base-override clearing for both new fields; all tests are mock-only |
Reviews (6): Last reviewed commit: "fix(proxy): extend banned-params + admin..." | Re-trigger Greptile
Greptile SummaryThis PR adds
Confidence Score: 4/5Safe to merge; the changes are additive and narrowly scoped to two enforcement lists with no modifications to surrounding logic. Both changes correctly follow the established pattern for banned params and admin-config clearing. The only gap is the absence of a test for the per-deployment The test file would benefit from a per-deployment opt-in test to match what the error message documents; no production files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/auth_utils.py | Adds nvcf_function_id to _BANNED_REQUEST_BODY_PARAMS; follows the same pattern as aws_bedrock_project_id and other provider-specific endpoint fields. No logic changes to the enforcement code itself. |
| litellm/router_utils/clientside_credential_handler.py | Adds nvcf_function_id to the kwargs_only_fields list in _admin_config_fields_to_clear_on_base_override(), consistent with the OCI field entries immediately above it. Correctly dropped when a caller redirects api_base. |
| tests/test_litellm/proxy/auth/test_auth_utils.py | Adds four new tests covering root-level rejection, rejection when api_key is co-present, proxy-wide opt-in, and clearing on base override. Missing a test for the per-deployment configurable_clientside_auth_params opt-in path, which the error message documents as a valid opt-in. |
Reviews (1): Last reviewed commit: "fix(proxy): block client-supplied nvcf_f..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
b9d01a6 to
83a94ab
Compare
|
Addressed manager-review feedback. @greptileai please re-review the new head. |
|
Addressed Greptile coverage feedback as a test-only follow-up commit. @greptileai please take another pass. |
|
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 88a8e63. Configure here.
|
Addressed Greptile feedback on test coverage and docstring terseness. @greptileai please re-review. |
|
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 29e55ae. Configure here.
|
Final CI: green. One known-flaky live-provider job at the time of comment, resolved on re-run; this PR does not touch that code path. |
mateo-berri
left a comment
There was a problem hiding this comment.
LGTM; thanks!
non-blocking nit: is Greptile's docstring nit here legit?:
the single non-critical note is a style-level docstring cleanup in tests/test_litellm/proxy/auth/test_auth_utils.py
…(VERIA-493) Two NVIDIA-Riva-specific fields consumed by the audio-transcription handler via the provider's `optional_params` passthrough were not covered by the proxy's existing banned-request-body list or the admin-config clearing list applied on `api_base` BYOK override: * `nvcf_function_id` * `use_ssl` Add both to `_BANNED_REQUEST_BODY_PARAMS` in `litellm/proxy/auth/auth_utils.py` and to the kwargs-only list in `_admin_config_fields_to_clear_on_base_override()` in `litellm/router_utils/clientside_credential_handler.py`, next to the analogous provider-specific entries already there (`aws_bedrock_*`, OCI provider fields, etc.). Same admin opt-ins as every other entry on those lists (`general_settings.allow_client_side_credentials` proxy-wide, or `configurable_clientside_auth_params` per deployment). Regression tests in `tests/test_litellm/proxy/auth/test_auth_utils.py` cover root-level rejection, the historical `api_key` bypass, both admin opt-in paths (proxy-wide and per-deployment), nested-container smuggling via the existing recursive walk, and clearing on `api_base` override. Mutation check verified. Resolves VERIA-493
29e55ae to
019afa8
Compare
|
Force-pushed: squashed into a single commit with a redacted message and trimmed inline comments. Same diff content. |
…(VERIA-493) (BerriAI#31742) Two NVIDIA-Riva-specific fields consumed by the audio-transcription handler via the provider's `optional_params` passthrough were not covered by the proxy's existing banned-request-body list or the admin-config clearing list applied on `api_base` BYOK override: * `nvcf_function_id` * `use_ssl` Add both to `_BANNED_REQUEST_BODY_PARAMS` in `litellm/proxy/auth/auth_utils.py` and to the kwargs-only list in `_admin_config_fields_to_clear_on_base_override()` in `litellm/router_utils/clientside_credential_handler.py`, next to the analogous provider-specific entries already there (`aws_bedrock_*`, OCI provider fields, etc.). Same admin opt-ins as every other entry on those lists (`general_settings.allow_client_side_credentials` proxy-wide, or `configurable_clientside_auth_params` per deployment). Regression tests in `tests/test_litellm/proxy/auth/test_auth_utils.py` cover root-level rejection, the historical `api_key` bypass, both admin opt-in paths (proxy-wide and per-deployment), nested-container smuggling via the existing recursive walk, and clearing on `api_base` override. Mutation check verified. Resolves VERIA-493
Relevant issues
Linear ticket
Resolves VERIA-493
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
Extends the proxy's existing banned-request-body list and admin-config clearing list, matching how analogous provider fields are already handled. Verified against a live local proxy
Type
🐛 Bug Fix
Changes
Two new entries in each of the two existing lists, next to the existing provider-specific entries. Regression tests in the existing mapped test file under
tests/test_litellm/proxy/auth/. Greptile 5/5