fix(proxy): read guardrail config from admin metadata, fix tag routing consistency - #25905
fix(proxy): read guardrail config from admin metadata, fix tag routing consistency#25905yuneng-berri merged 15 commits into
Conversation
…g consistency Read guardrail control flags (disable_global_guardrails, opted_out_global_guardrails) from admin-configured key metadata instead of the request body. This ensures callers cannot override admin security policies. Fix tag-based routing to enforce strict tag checks regardless of whether the request includes tags. Fix budget limiter to use the same dynamic metadata key resolution as the tag router for consistent tag extraction.
…olution Extract _get_admin_metadata() in CustomGuardrail to deduplicate metadata lookup. Hoist tag resolution above the deployment loop in budget limiter. Update stale comment in tag routing.
…ging from dynamic params Include user_api_key_team_metadata alongside user_api_key_metadata in _get_admin_metadata() so team-level guardrail settings are respected. Key-level settings take precedence over team-level. Remove turn_off_message_logging from _supported_callback_params so it cannot be set via request metadata. Admin controls logging globally or via key/team configuration. Update tests to verify user-injected guardrail flags are ignored while admin-configured flags are respected.
…g removal Verify turn_off_message_logging is no longer extracted from request kwargs since it is now admin-only.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR hardens the guardrail and tag-routing security surface: Confidence Score: 5/5Safe to merge — security fixes are sound, well-tested, and the remaining findings are minor style issues only. All P0/P1 concerns from prior threads are addressed in the implementation. The two inline litellm/proxy/auth/auth_utils.py and litellm/proxy/common_utils/http_parsing_utils.py — minor inline import style cleanup only.
|
| Filename | Overview |
|---|---|
| litellm/integrations/custom_guardrail.py | Adds _get_admin_metadata helper that reads user_api_key_metadata/user_api_key_team_metadata from both metadata keys; updates get_disable_global_guardrail and get_opted_out_global_guardrails_from_metadata to only trust admin-injected fields. |
| litellm/proxy/auth/auth_checks.py | Expands _guardrail_modification_check to cover litellm_metadata, root-body bypass keys, and JSON-string-encoded payloads; correctly adds safe_json_loads at module level. |
| litellm/proxy/auth/auth_utils.py | Adds _as_dict helper to coerce JSON-string metadata for end-user attribution; contains an inline import of safe_json_loads inside the nested function instead of at module level. |
| litellm/proxy/common_utils/http_parsing_utils.py | Fixes get_tags_from_request_body to coerce JSON-string metadata; contains an inline import of safe_json_loads inside the function body instead of at module level. |
| litellm/proxy/litellm_pre_call_utils.py | Major restructuring: strips user_api_key_* and _pipeline_managed_guardrails from both metadata dicts, enforces allow_client_tags gate for header/body tags, defers the proxy_server_request.body snapshot to post-strip, and correctly orders requester_metadata capture and litellm_metadata merge. |
| litellm/router_strategy/tag_based_routing.py | Removes bool(request_tags) from strict-tag guard so regex is blocked even when the request carries no tags, fixing the policy bypass. |
| litellm/router_strategy/budget_limiter.py | Passes metadata_variable_name from get_metadata_variable_name_from_kwargs at all three _get_tags_from_request_kwargs call sites and hoists tag resolution outside the deployment loop. |
| litellm/litellm_core_utils/initialize_dynamic_callback_params.py | Removes turn_off_message_logging from _supported_callback_params so callers can no longer suppress message logging via request body. |
| litellm/proxy/proxy_server.py | Guards metadata initialization with isinstance(..., dict) check to prevent TypeError when metadata arrives as a JSON string in multipart/extra_body requests. |
| tests/test_litellm/proxy/test_litellm_pre_call_utils.py | Extensive new test coverage: admin injection slot stripping, user_api_key_* prefix family stripping, string metadata survival, and tag allow-listing behavior. |
| tests/test_litellm/proxy/auth/test_auth_checks.py | Adds TestGuardrailModificationCheck class covering all new bypass vectors (litellm_metadata key, root body, JSON-string encoding) with appropriate 403 assertions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request] --> B[proxy_server.py\nguard: ensure metadata is dict]
B --> C[auth layer\n_guardrail_modification_check\nchecks metadata + litellm_metadata\n+ root body + JSON strings]
C -->|team lacks permission| D[403 Forbidden]
C -->|permitted| E[add_litellm_data_to_request]
E --> F[Parse JSON-string metadata/litellm_metadata to dict]
F --> G[Strip user_api_key_* keys\nand _pipeline_managed_guardrails\nfrom BOTH metadata dicts]
G --> H{allow_client_tags\nin key/team metadata?}
H -->|No| I[Strip tags from metadata\nlitellm_metadata and root body\nBlock header tags too]
H -->|Yes| J[Preserve client tags]
I --> K[Snapshot proxy_server_request.body\nand requester_metadata POST-strip]
J --> K
K --> L[Proxy writes user_api_key_*\nfields into _metadata_variable_name]
L --> M[Merge admin team/key tags]
M --> N[add_request_tag_to_metadata\nreads headers + root body]
N -->|allow_client_tags=True| O[Set tags into metadata]
N -->|allow_client_tags=False| P[Log warning, discard header tags]
O --> Q[CustomGuardrail.should_run_guardrail\n_get_admin_metadata reads only\nuser_api_key_* sub-fields]
P --> Q
Q -->|disable_global_guardrails in admin meta| R[Skip guardrail]
Q -->|guardrail permitted| S[Run guardrail]
Reviews (8): Last reviewed commit: "fix(proxy): strip root-level data['tags'..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile P2: _get_admin_metadata used 'litellm_metadata or metadata', meaning a caller sending a non-empty litellm_metadata would shadow admin config the proxy had injected into data['metadata']. Admin exemptions would be silently ignored. Check both keys and prefer whichever contains admin fields. Add regression test covering the shadowing scenario.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
There was a problem hiding this comment.
High: Guardrail bypass via user-supplied litellm_metadata
This PR correctly moves guardrail config reads to admin-only metadata (user_api_key_metadata / user_api_key_team_metadata), but _get_admin_metadata reads from both data["metadata"] and data["litellm_metadata"], taking the last truthy value. The proxy only overwrites admin metadata in one of these two dicts (whichever _metadata_variable_name resolves to). A caller can inject forged admin metadata into the other dict to disable guardrails.
- high: guardrail bypass via user-injected
user_api_key_metadatainlitellm_metadata— litellm/integrations/custom_guardrail.py - low:
user_api_key_metadata/user_api_key_team_metadatanot stripped from user input alongside_pipeline_managed_guardrails— litellm/proxy/litellm_pre_call_utils.py
Expand the pre-call metadata strip to also remove user_api_key_metadata and user_api_key_team_metadata. The proxy writes these fields into data[_metadata_variable_name] with admin-authoritative values, but only into that one metadata key; the caller's value in the OTHER metadata key (metadata vs litellm_metadata) would otherwise persist and be picked up by _get_admin_metadata, letting a caller supply their own 'admin' config to disable guardrails, opt out of global policies, etc. VERIA-28 (High): Security Policy and Guardrail Bypass via Unsanitized Request Metadata. Add regression test at the proxy boundary verifying the strip, and extend the guardrail test to cover the post-strip admin-config path.
There was a problem hiding this comment.
High: Guardrail bypass via string-encoded metadata
This PR hardens guardrail config to only read from admin-injected metadata and strips attacker-supplied user_api_key_metadata / user_api_key_team_metadata from both metadata dicts. However, the stripping runs before the string-to-dict parsing that happens ~90 lines later for multipart/form-data and extra_body payloads. An attacker can send litellm_metadata (or metadata) as a JSON string containing user_api_key_metadata, which bypasses the isinstance(dict) guard and survives into the parsed dict where _get_admin_metadata reads it.
…ent_tags VERIA-28 (High) follow-up: tag-based routing and tag budget enforcement read metadata.tags directly from the request, letting an attacker reach restricted tag-routed deployments or misattribute spend to a victim team's tag. Strip metadata.tags (and litellm_metadata.tags) at the pre-call boundary unless the caller's key or team metadata opts in with allow_client_tags=True. Default-deny: existing clients that need to pass routing tags must have the flag set explicitly on their key or team. Preserves the tag-routing feature for admins who trust their callers; closes the injection path for everyone else.
Two pre-existing tests codified the pre-fix behavior where any caller- supplied metadata.tags would flow through to spend logs and routing: - test_add_key_or_team_level_spend_logs_metadata_to_request exercised the request/key/team tag merge. Set allow_client_tags=True on the key metadata so the merge path is still tested under the new regime. - test_create_file_with_nested_litellm_metadata asserted that litellm_metadata[tags] form-data propagated to the handler. Drop the tag field; the test still proves nested form-parser correctness via spend_logs_metadata and environment.
Silent strip is the worst debug UX: admin's client sends routing tags, they disappear, admin can't figure out why. Emit a warning naming the metadata key the tags came from and telling the admin exactly which flag to set if this is intentional.
There was a problem hiding this comment.
High: String-encoded metadata bypasses new sanitization
This PR correctly adds stripping of user_api_key_metadata, user_api_key_team_metadata, _pipeline_managed_guardrails, and tags from user-supplied metadata. However, the stripping block (lines 985-1012) runs before the string-to-dict parsing (lines 1098-1124). An attacker can send metadata or litellm_metadata as a JSON string (possible via multipart/form-data or extra_body), causing the isinstance(_, dict) guard to return False and skip all stripping. The payload is then parsed into a dict and merged into the authoritative metadata, restoring the injection vectors this PR aims to close.
There was a problem hiding this comment.
High: String-encoded metadata still bypasses admin-field stripping
This PR hardens guardrail configuration by reading only from admin-injected metadata and stripping user-supplied user_api_key_metadata / user_api_key_team_metadata / _pipeline_managed_guardrails / tags from both metadata dicts. However, the stripping block runs before the string-to-dict JSON parsing at lines ~1109-1128. An attacker can send metadata or litellm_metadata as a JSON string (valid via extra_body or multipart/form-data), bypass the isinstance(dict) guard, and have the payload parsed and merged into the request after stripping has already run.
- high: string-encoded metadata bypasses admin-field stripping — litellm/proxy/litellm_pre_call_utils.py
test_add_litellm_data_to_request_duplicate_tags tests the request/key tag merge when tags overlap. The merge requires caller-supplied tags to flow through — set allow_client_tags=True on the key so the merge path stays testable under the new default-deny regime.
Veria AI caught a bypass: metadata can arrive as a JSON string via multipart/form-data or extra_body, and the existing strip block ran before the string-to-dict parse. The isinstance(_user_meta, dict) guard returned False on the string, the strip was skipped, and then the parse turned the string into a dict — leaving user_api_key_metadata / user_api_key_team_metadata / _pipeline_managed_guardrails / tags intact in the parsed dict. Move the strip to run AFTER the parse and BEFORE the merge of litellm_metadata into data[_metadata_variable_name], closing the bypass for both raw-dict and string-encoded payloads. Regression test: test_add_litellm_data_to_request_strips_string_encoded_admin_injection.
…keys Per VERIA-28's secondary recommendation. The existing check only gated metadata.guardrails. User-supplied values for disable_global_guardrails (plural and the original singular typo variant) and opted_out_global_guardrails are already silently ignored by _get_admin_metadata at read time, but the silent-ignore makes diagnosis confusing and relies on one specific read site catching them. Reject at auth time with a 403 when any of: - guardrails list (existing) - disable_global_guardrails (new) - disable_global_guardrail (new — historical singular-key variant) - opted_out_global_guardrails (new) are present in metadata, litellm_metadata, or at the request root, and the caller's team lacks can_modify_guardrails. Defense in depth: the strip at the pre-call layer still runs; this check fails loudly one layer earlier so operators see an explicit 403 rather than a silent-ignore.
Close three variant bypasses adjacent to VERIA-28 found during post-fix
variant audit:
1. _guardrail_modification_check had the same isinstance(dict) bypass
Veria-AI just flagged on the pre-call strip. A caller sending
`{"metadata": "{…}"}` as a JSON-encoded string (multipart/form-data
or extra_body) skipped the guard, got parsed to dict downstream, and
reached guardrail logic with bypass flags intact. Coerce strings via
safe_json_loads before evaluating.
2. The allow_client_tags strip only covered body metadata.tags and
litellm_metadata.tags — caller-supplied tags arriving via the
x-litellm-tags header or root-level data["tags"] bypassed it. Gate
add_request_tag_to_metadata's result on the same flag.
3. requester_metadata was deepcopied BEFORE the strip, so attacker
injections (user_api_key_metadata shadows, disallowed tags,
_pipeline_managed_guardrails) persisted in the snapshot. The PANW
guardrail (and any future consumer) trusting requester_metadata
would see forged values. Move the deepcopy to after the strip.
Regression tests added for each.
Post-merge audit found 6 adjacent variants of the VERIA-28 class. All
fixed here with regression tests:
1. Strip widened from 3 named keys to the full user_api_key_* prefix.
The proxy writes a dozen user_api_key_* fields (user_id, alias,
spend, team_id, request_route, end_user_id, …) into
data[_metadata_variable_name]; the 3-key strip left the rest
exploitable for identity/spend forgery in audit logs and guardrails.
2. proxy_server_request['body'] snapshot moved to AFTER the strip.
Was captured at line ~990 before the strip ran, so
standard_logging_object, lago, and spend_tracking readers saw the
attacker-forged payload even though the live data dict was clean.
3. get_tags_from_request_body (auth-time) now coerces JSON-string
metadata via safe_json_loads. Previously crashed with
AttributeError on string metadata (DoS; potential RBAC bypass if
a caller swallowed the exception).
4. get_end_user_id_from_request_body coerces JSON-string
metadata/litellm_metadata. Previously isinstance(dict) guard
caused end-user budget attribution to be silently skipped when
the caller sent metadata as a JSON string.
5. Four hand-rolled 'if data.get("metadata") is None: data["metadata"] = {}'
blocks in proxy_server.py (7160, 7341, 7590, 11375) now guard on
isinstance(dict). They crashed with TypeError when metadata was a
JSON string (DoS).
6. _get_admin_metadata defensively guards with isinstance(dict);
previously AttributeError'd on any leaked string metadata.
Also hoists the inline safe_json_loads import in _guardrail_modification_check
to module level per CLAUDE.md style.
Greptile P2. The admin-inject gate only removed tags from data['metadata'] and data['litellm_metadata']; and the policy engine read directly, so a caller without allow_client_tags could still drive tag-based policy decisions by moving tags to the body root. Also strip the root key in the same branch.
32714a4
into
BerriAI:litellm_yj_apr17
…trols fix(proxy): read guardrail config from admin metadata, fix tag routing consistency
2 participants
Relevant issues
Fixes inconsistencies in guardrail configuration resolution and tag-based routing/budget enforcement.
Reopens #25832 (original base
litellm_yj_apr15was deleted); rebased ontolitellm_internal_staging. No content changes.Pre-Submission checklist
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 reviewType
🐛 Bug Fix
🧹 Refactoring
Changes
1. Read guardrail control flags from admin metadata
get_disable_global_guardrail()andget_opted_out_global_guardrails_from_metadata()inCustomGuardrailnow read fromuser_api_key_metadata(admin-configured key/team metadata populated by the proxy) instead of scanning the top-level request body and user-supplied metadata. Extracted_get_admin_metadata()helper to deduplicate the lookup pattern.Also strips
_pipeline_managed_guardrailsfrom user-supplied metadata at the proxy boundary since it is internal pipeline state.2. Fix tag routing strict check
_match_deployment()intag_based_routing.pypreviously requiredbool(request_tags)for the strict tag check to fire. This meant requests with no tags could bypass the strict policy and fall through to regex matching. Removed thebool(request_tags)condition so strict tag enforcement applies regardless of whether the request includes tags.3. Fix tag budget metadata key resolution
budget_limiter.pyused a hardcoded default"metadata"key when calling_get_tags_from_request_kwargs(), while the tag router dynamically resolves between"metadata"and"litellm_metadata". This inconsistency meant tags could be extracted differently by routing vs budget enforcement. Now passesmetadata_variable_namefromget_metadata_variable_name_from_kwargs()at all three call sites. Also hoisted tag resolution above the deployment loop since it's loop-invariant.