Worktree fix mcp byok oauth - #27892
Conversation
[Infra] Promote Internal Staging to main
[Infra] Promote Internal Staging to main
[Infra] Promote internal staging to main
Authenticated clients could supply CustomPricingLiteLLMParams fields (input_cost_per_token, output_cost_per_token, etc.) in the request body. These were forwarded to register_model() in main.py, permanently mutating the shared global litellm.model_cost dict for all users on the instance. Adds all CustomPricingLiteLLMParams fields to _BANNED_REQUEST_BODY_PARAMS so is_request_body_safe() rejects them before they reach completion(). New pricing fields added to CustomPricingLiteLLMParams are auto-covered. Admin opt-in via allow_client_side_credentials or configurable_clientside_auth_params still works as before. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
aws_sts_endpoint, aws_web_identity_token, and aws_bedrock_runtime_endpoint in ingest_options.vector_store were passed directly to the Bedrock ingestion class, which reads them into boto3 STS client construction. Any authenticated caller could redirect AssumeRole calls to an attacker-controlled server, leaking the proxy's instance profile credentials. Calls is_request_body_safe() on ingest_options["vector_store"] before forwarding to litellm.aingest(). Same banned-params list and admin opt-in escape hatch (allow_client_side_credentials) as the /chat/completions path. ValueError from the safety check is caught and re-raised as HTTP 400. Co-Authored-By: Claude Sonnet 4.6 (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 adds two security hardening measures: it blocks pricing-field injection into the global
Confidence Score: 4/5Both changes are targeted, security-positive fixes with dedicated tests. The main concerns are the incomplete scope of the SSRF guard and a weak happy-path test assertion. The core fixes in auth_utils.py and endpoints.py are correct and well-tested. The SSRF guard covers only the vector_store sub-dict, leaving top-level ingest_options keys unchecked, and the happy-path test asserts != 400 so a 500 from an unrelated failure would still pass. litellm/proxy/rag_endpoints/endpoints.py and tests/test_litellm/proxy/rag_endpoints/test_rag_endpoints.py
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/auth_utils.py | Adds all CustomPricingLiteLLMParams fields to _BANNED_REQUEST_BODY_PARAMS at import time, blocking clients from poisoning the global model cost registry via request body injection. Change is minimal and correct. |
| litellm/proxy/rag_endpoints/endpoints.py | Adds is_request_body_safe guard to rag_ingest against SSRF via vector_store config; the check only covers ingest_options vector_store and not the top-level ingest_options dict, leaving a potential gap if banned params are passed at the top level. |
| tests/test_litellm/proxy/auth/test_auth_utils.py | New TestPricingInjectionBlocked class with parametrized per-field rejection tests, a coverage completeness check, and an admin opt-in test. All assertions are specific and meaningful. |
| tests/test_litellm/proxy/rag_endpoints/test_rag_endpoints.py | New TestRagIngestSSRFBlocked class tests SSRF field rejection in the RAG ingest endpoint. The happy-path test uses a weak != 400 assertion that would pass on 500 errors, reducing its value as a regression guard. |
Reviews (1): Last reviewed commit: "fix: block SSRF fields in RAG ingest vec..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assert response.status_code != 400, ( | ||
| f"Clean Bedrock ingest_options should not be rejected: {response.json()}" | ||
| ) |
There was a problem hiding this comment.
response.status_code != 400 is too weak — it passes if the endpoint returns a 500 (e.g., missing DB config, unrelated server error). The test is meant to prove that clean Bedrock options reach the handler successfully, but a crash masked as a 5xx would still satisfy this condition. Use a positive status assertion instead.
| assert response.status_code != 400, ( | |
| f"Clean Bedrock ingest_options should not be rejected: {response.json()}" | |
| ) | |
| assert response.status_code == 200, ( | |
| f"Clean Bedrock ingest_options should be accepted (200), got {response.status_code}: {response.json()}" | |
| ) |
| is_request_body_safe( | ||
| request_body=ingest_options.get("vector_store", {}), | ||
| general_settings=general_settings, | ||
| llm_router=llm_router, | ||
| model="", | ||
| ) | ||
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail={"error": str(e)}) |
There was a problem hiding this comment.
vector_store sub-dict is validated
is_request_body_safe is called on ingest_options.get("vector_store", {}), which means banned fields placed directly in ingest_options (e.g. {"aws_sts_endpoint": "...", "vector_store": {...}}) are not checked. If litellm.aingest propagates top-level ingest_options keys into outbound requests, those fields would bypass this guard. Consider passing ingest_options itself as request_body and letting is_request_body_safe's nested-key descent handle vector_store as a nested config key, or additionally calling _check_banned_params on the top-level ingest_options.
fix: backport #27892 to litellm_1.84.0rc2
…auth Worktree fix mcp byok oauth
4 participants
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes