Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Comments Outside DiffThese findings could not be posted inline.
|
|
@greptileai please re-review the latest commit, which addresses both P1 findings from your previous review on this PR |
|
@greptileai please re-review the latest commit, which tightens the tests to assert whole payloads and removes docstrings that repeated the code |
…ery path Guardrails read user_api_key_alias, user_api_key_team_id, the key hash and the request route from request metadata, and the generic guardrail API forwards them to the vendor. The chat path strips caller copies of those fields and writes the real ones, but three other paths did not, so a caller could claim another key, team or request route (which call-type lookups key on) /guardrails/apply_guardrail passed the body metadata straight to the guardrail. It now drops the fields the chat path treats as untrusted, plus the bare user_api_key and the caller's headers (a slightly wider strip than the chat path, on purpose). Then it adds the authenticated identity and the proxy's real request headers Pass-through handed the raw body to pre_call_hook. It now runs the chat path's strip on both metadata buckets first, which also stops a body from switching off global guardrails, and drops the body's headers and proxy_server_request so it cannot pick the inbound headers a vendor sees. Those keys were already popped before the upstream send, so the forwarded body does not change. The pass-through guardrail text no longer includes litellm_metadata, which carried the key's identity dump into the scanned payload The unified guardrail only filled litellm_metadata when it was missing, so a caller-supplied bucket won. It now overwrites the identity fields of an existing bucket and drops any user_api_key_token there, since the proxy never writes one. It leaves user_api_key_auth_metadata alone because on litellm_metadata routes the proxy has already merged team metadata into it transform_user_api_key_dict_to_metadata used to dump every UserAPIKeyAuth field, including the raw token of non-sk keys, JWT claims, team membership, proxy config and org and project metadata with callback secrets. It now returns only the chat path's identity fields plus user_api_key_key_alias for existing readers, so MCP, pass-through, output-side handlers and realtime transcript guardrails all get that same identity allowlist and see the real user_api_key_alias. The generic guardrail uses user_api_key_token only from litellm_metadata and only when user_api_key_hash is missing, so a CLI session key's raw per-login token never reaches the vendor The strip and the identity fields live in one place in litellm_pre_call_utils, and the chat path calls them too
Moves the imports this PR added inside functions to the top of their modules: UserAPIKeyAuth and BaseTranslation in realtime_streaming, BaseTranslation, StreamingScanKey and LiteLLMProxyRequestSetup in unified_guardrail (so its annotations no longer need quotes), and the test-only imports in the unified guardrail, guardrail endpoint, pass-through and realtime tests. None of them creates a cycle, and import litellm still does not load the proxy request layer or fastapi One import stays inside its function: base_translation's LiteLLMProxyRequestSetup. import litellm loads base_translation before litellm.Router exists, and litellm_pre_call_utils imports Router, so hoisting it makes import litellm fail with "cannot import name 'Router' from 'litellm'". It carries a one-line comment saying so
Pass-through guardrails lost their inbound headers once the caller's body copies were stripped, so an operator's extra_headers allowlist forwarded nothing to the vendor. Both the pre-call and the post-call guardrail hooks now get the real request headers in proxy_server_request, built once the way the chat path builds them: clean_headers drops the proxy's auth header and a custom litellm_key_header_name, MCP upstream credential headers are dropped, and redact_credential_headers masks cookies and other credentials On the success path the headers are attached after the logging object is created, so they do not land in the logged request. When a pre-call guardrail blocks, the failure log now receives these cleaned and redacted headers, which matches what the chat path logs. The pass-through guardrail leaves proxy_server_request out of the text it scans, and the upstream body is unchanged because proxy_server_request is popped with the other litellm params before the send
…ed-request log The apply_guardrail tests compare the whole request_data again, with the expected identity built from get_authenticated_identity_metadata and the key hash pinned, so an extra leaked key or a wrong hash fails them. A new pass-through test checks that a request blocked by a pre-call guardrail logs the cleaned and redacted inbound headers. The realtime Gray Swan test now injects an HTTP transport instead of overriding a private method, so the real request path runs _ensure_litellm_metadata is renamed to _apply_authenticated_identity_to_litellm_metadata, and docstrings that only repeated the code are gone. The apply_guardrail strip no longer lists user_api_key, because the authenticated identity always overwrites it. The identity helpers now return Mapping, since no caller mutates what they return
Main passes endpoint_type to pre_call_hook and reads request.scope before parsing a body, so the hook fake takes endpoint_type and the mocked request gets a path
4b63b53 to
af6c230
Compare
|
@greptileai please review af6c230, rebased onto current main with two pass-through test fakes adapted to main's hook signature |
| def get_authenticated_identity_metadata(user_api_key_dict: UserAPIKeyAuth) -> Mapping[str, object]: | ||
| return { | ||
| **LiteLLMProxyRequestSetup.get_sanitized_user_information_from_key(user_api_key_dict), | ||
| "user_api_key": LiteLLMProxyRequestSetup.get_logged_api_key(user_api_key_dict), |
There was a problem hiding this comment.
Low: Raw custom-auth credentials sent to guardrail vendors
When a custom auth callback returns UserAPIKeyAuth(api_key="my-custom-auth-credential-abc123"), the constructor leaves this non-sk-, non-JWT credential unchanged, and this helper copies it into both user_api_key_hash and user_api_key. The new /apply_guardrail and realtime paths forward these fields to Generic/GraySwan services, allowing the recipient to reuse the caller's bearer credential; hash or omit opaque credentials in both fields before constructing vendor-facing identity metadata.
PR overviewThe PR updates guardrail identity propagation so guardrails receive the authenticated caller’s identity across request paths, including One issue remains open: custom-auth credentials that are neither Open issues (1)
Fixed/addressed: 0 · PR risk: 6/10 |
TLDR
Problem this solves:
How it solves it:
Intentional product change: guardrail vendors now always receive the authenticated identity in request metadata, even when the body sent none, and pass-through guardrails no longer see any identity or headers a request body claims
User Flow
Before: a key holder can make the guardrail vendor believe the request came from another key or team
metadatabatch-workeron both routes, plus teamteam-exempton apply_guardrail, and applies that policyAfter: the vendor always receives the caller's real identity
metadatateam-prod, and the upstream provider gets an unchanged bodyRelevant issues
Split out of #37055, as requested in its review
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/unit/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or moremake checkpasses locally withBASE_REF=upstream/main, and CI on af6c230 is runningmisc / Run testsfails the same 4 interactions OpenAPI tests on main,rust-testwas cancelled at its time limit, andbudget-ratchetflags a basedpyright limit that fix(cost_calculator): bill ultrafast prompts above 272k at the ultrafast long-context rates #43764 raised on main, in a file this PR does not touch@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review)
Screenshots / Proof of Fix
All runs are against a live proxy on
localhost:4000started withpython litellm/proxy/proxy_cli.py --config proof_config.yaml --detailed_debug --use_v2_migration_resolver, backed by a real Postgres, and every completion is a real, billed call to Bedrockus.anthropic.claude-haiku-4-5-20251001-v1:0. The guardrail vendor is a small FastAPI inspector app on127.0.0.1:8787that implementsPOST /beta/litellm_basic_guardrail_api, answers{"action": "NONE"}, and appends every payload it receives toinspector.jsonl. Nothing else is stubbed/my-llmis a user-defined pass-through route whose upstream is an OpenAI-compatible endpoint: the proxy's own/v1/chat/completions, which calls Bedrock. That is what puts a real model call behind the pass-through casesThe keys come from the real management API, once, and both runs use the same database.
/guardrails/apply_guardrailis admin-only by default, so the apply case uses a key granted that routeBefore (02f61c9)
Case 1: /guardrails/apply_guardrail with a forged identity in metadata
Case 2: pass-through route with a forged identity in the body, real Bedrock call upstream
Case 3: pass-through route with a real x-tenant header and a forged one in the body (extra_headers: [x-tenant])
After (af6c230)
Case 1: /guardrails/apply_guardrail with a forged identity in metadata
Case 2: pass-through route with a forged identity in the body, real Bedrock call upstream
Case 3: pass-through route with a real x-tenant header and a forged one in the body (extra_headers: [x-tenant])
Type
🐛 Bug Fix
Caveats (if any)
Medium
Low
guardrailskey/utils/test_policies_and_guardrailsstill passes caller-built request data to guardrailsui-token) now send no key hashcli-session-...token needs the SSO CLI login flowFinal Attestation