fix: [Bug]: Health checks use max_completion_tokens=1, causing failures for GPT-5 models (#23836) - #24893
Closed
hannahmadison wants to merge 553 commits into
Closed
fix: [Bug]: Health checks use max_completion_tokens=1, causing failures for GPT-5 models (#23836)#24893hannahmadison wants to merge 553 commits into
hannahmadison wants to merge 553 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Contributor
Author
Contributor
Contributor
Greptile SummaryThis PR tightens the wildcard-model assertion in the health check max-tokens test, changing from a weak disjunctive check to a strict key-absence assertion ( Confidence Score: 4/5Safe to merge after adding monkeypatch isolation to the wildcard test. The core assertion tightening is correct and aligns with actual _resolve_health_check_max_tokens behaviour. The one concern is that the wildcard test now asserts strict key absence without isolating the module-level constant, making it fragile in environments where the env var is set. The parallel default-token test already shows the right pattern. tests/test_litellm/proxy/test_health_check_max_tokens.py — missing monkeypatch in the wildcard test.
|
| Filename | Overview |
|---|---|
| tests/test_litellm/proxy/test_health_check_max_tokens.py | Tightens the wildcard-model assertion from a weak disjunctive check to a strict key-absent assertion; the wildcard test is missing monkeypatch isolation for the module-level constant. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_resolve_health_check_max_tokens] --> B{model_info has explicit health_check_max_tokens?}
B -- Yes --> C[return that value]
B -- No --> D{is wildcard model?}
D -- No --> E{reasoning/non-reasoning model_info keys set?}
E -- Yes --> F[return matching key value]
E -- No --> G{REASONING env var set AND model is reasoning?}
G -- Yes --> H[return reasoning env value]
G -- No --> I{global max-tokens env var set?}
I -- Yes --> J[return global env value]
I -- No --> K[return default 5]
D -- Yes --> L{global max-tokens env var set?}
L -- Yes --> M[return global env value]
L -- No --> N[return None — no max_tokens injected]
style N fill:#f9f,stroke:#333
style K fill:#9f9,stroke:#333
Reviews (4): Last reviewed commit: "Merge branch 'main' into git-genie/issue..." | Re-trigger Greptile
hannahmadison
marked this pull request as ready for review
April 1, 2026 03:38
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.
Two litellm-level flags wired through litellm_settings YAML: - user_url_validation (bool, default True): master switch. When False, safe_get/async_safe_get bypass validation and call client.get directly. - user_url_allowed_hosts (List[str], default []): per-host allowlist. Entries are 'host' (matches any port) or 'host:port' (port-specific). Matched hosts skip the blocked-networks check but still resolve DNS and still rewrite HTTP to the validated IP, preserving rebinding protection within the permitted name. Also fix an existing Host header bug: IPv6 literals (e.g. 2001:db8::1) were emitted unbracketed, producing ambiguous values like '2001:db8::1:8080' per RFC 7230 5.4. Bracket them consistently in _format_host_header.
…ssertion Drop test_bedrock_invoke_messages_injects_thinking_for_clear_thinking_context_management. Its assertion 'interleaved-thinking-2025-05-14' in betas cannot hold because anthropic_beta_headers_config.json maps that header to null for the bedrock provider, so filter_and_transform_beta_headers drops it from the auto-added beta set before anthropic_beta is written to the request. The adjacent test_bedrock_invoke_messages_skips_thinking_injection_when_already_enabled already covers the inverse behavior for the same model, so no coverage is lost.
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.
…icast and Azure Wire Server
Replace the hand-maintained _BLOCKED_NETWORKS CIDR list with a
default-deny check based on ipaddress.is_global (RFC 6890 semantics,
implemented by Python's stdlib). Also reject multicast explicitly —
is_global returns True for public multicast allocations, which are
not legitimate HTTP targets.
Only globally-routable cloud-fabric IPs need explicit exceptions; the
canonical list contains one entry today: Azure Wire Server
(168.63.129.16), an in-fabric service reachable from any Azure VM.
Coverage delta picked up automatically via is_global:
- Alibaba Cloud metadata (100.100.100.200, CGNAT)
- Legacy Oracle metadata (192.0.0.192, IETF Protocol Assignments)
- IPv4 documentation ranges (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24)
- IPv4 reserved/future-use (240.0.0.0/4) and broadcast
- IPv6 documentation (2001:db8::/32)
Also fix two issues Greptile flagged:
- HTTP relative-redirect hops lost the original hostname because
_extract_redirect_url joined the Location against the rewritten
(IP-based) URL. Join against the pre-rewrite URL so the next hop's
Host header keeps the original hostname.
- Two unit tests performed real socket.getaddrinfo('localhost')
calls. Monkeypatch them.
Add coverage tests for every cloud-metadata IP from the canonical
SSRF dictionary (AWS/GCP/Azure/Alibaba/Oracle/DO/OpenStack) plus the
new multicast/reserved/documentation/broadcast ranges, and a
regression test for redirect-hostname preservation.
) * Add announcement bar for Trivy compromise resolution notice Add a Docusaurus announcement bar to the top of the docs site informing users that the Trivy supply-chain compromise has been mitigated and resolved. The banner: - States all affected packages have been deleted and releases are safe - Links to the Security Townhall blog post for details - Links to the CI/CD v2 blog post for improvements made - Uses a green background with closeable dismiss button Co-authored-by: Krrish Dholakia <krrish-berri-2@users.noreply.github.com> * Use :::note admonition instead of announcement bar Replace the Docusaurus announcementBar with a :::note admonition on the docs index page. The note appears below the hero image with the title 'Security Update' and links to the Security Townhall and CI/CD v2 blog posts. Co-authored-by: Krrish Dholakia <krrish-berri-2@users.noreply.github.com> * Update security notice wording to 'contained' Co-authored-by: Krrish Dholakia <krrish-berri-2@users.noreply.github.com> * Move note above hero image and add to root page - Move the security notice above the product screenshot on /docs - Add the same notice to the root page (src/pages/index.md) Co-authored-by: Krrish Dholakia <krrish-berri-2@users.noreply.github.com> * Update security notice wording Co-authored-by: Krrish Dholakia <krrish-berri-2@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Krrish Dholakia <krrish-berri-2@users.noreply.github.com>
…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.
…ThinkingBetaTest [Test] Remove dead Bedrock clear_thinking interleaved-thinking-beta assertion
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.
…headers and add tests - Move protected-headers set to module level as a frozenset - Add x-api-key, x-goog-api-key to protected set (provider credential headers) - Block x-amz- prefix to cover AWS SigV4 signing headers - Normalize forwarded header names to lowercase on write - Log at debug level when a protected header is skipped - Add unit test covering protected-header drop and non-protected forwarding
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.
…striction [Fix] Restrict x-pass- header forwarding for credential and protocol headers
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.
…striction [Fix] Tighten api_key value check in credential validation
Three tests inherited by TestBedrockMoonshotInvoke from BaseLLMChatTest make live AWS Bedrock completion calls: test_developer_role_translation, test_message_with_name, and test_completion_cost. These have been crashing llm_translation_testing CI workers (reported as "failed on setup with worker 'gwN' crashed"). Replace each with a mocked override that intercepts the outgoing request via HTTPHandler.post / AsyncHTTPHandler.post patching: - test_developer_role_translation asserts the outgoing body maps the developer role to system (LiteLLM's translation for non-OpenAI providers). - test_message_with_name asserts the outgoing body preserves the user message. - test_completion_cost returns a canned moonshot-shaped response body with usage and asserts response_cost > 0 against the local model cost map. Follows the existing HTTPHandler + patch.object(client, "post") pattern used in test_bedrock_gpt_oss.py and test_bedrock_completion.py. No network traffic; the three tests now complete in ~0.3s.
…itellm_/amazing-almeida # Conflicts: # tests/test_litellm/llms/bedrock/messages/invoke_transformations/test_anthropic_claude3_transformation.py
Existing tests pinned exact kwargs on `PrismaManager.setup_database`, but the opt-in v2 resolver added `use_v2_resolver=False` to every call. Update the three assertions to reflect the new signature. Fixes: - TestHealthAppFactory::test_use_prisma_db_push_flag_behavior - TestHealthAppFactory::test_startup_fails_when_db_setup_fails
…span (BerriAI#26133) * add litellm_call_id field to StandardLoggingPayload * populate litellm_call_id in get_standard_logging_object_payload * emit litellm.call_id span attribute in OTel integration * test: litellm_call_id is present in StandardLoggingPayload * test: litellm.call_id emitted as OTel span attribute * test: allow litellm. prefix attributes in redacted span validator
- Open the psycopg connection in `_warn_if_db_ahead_of_head` with autocommit=True. Without it, psycopg3's `with conn` calls COMMIT on clean exit, which fails after the `UndefinedTable` (fresh-DB) branch left the transaction in an aborted state — crashing first-run startups. - Wrap the v2 `prisma db push` path in try/except and raise RuntimeError on CalledProcessError/TimeoutExpired. Otherwise these propagate past proxy_cli.py's `except RuntimeError` as unhandled tracebacks. - Reword the loop-exhaustion error to cover the non-timeout exit path (repeated P3005/P3009/P3018 idempotent-recovery `continue`s), not just persistent timeouts. Adds a unit test for the db_push error wrapping.
… Preview) (BerriAI#26196) * add anthropic.claude-mythos-preview to model_prices_and_context_window.json * add mantle route to bedrock common_utils: route detection, chat config, messages config dispatch * add AmazonMantleConfig for bedrock/mantle /chat/completions endpoint * add AmazonMantleMessagesConfig for bedrock/mantle /messages endpoint * register AmazonMantleMessagesConfig in __init__.py and lazy imports registry * add unit tests for bedrock mantle route and config dispatch * add e2e tests for bedrock mantle: URL, body, SigV4 header, region routing
…lures Addresses two further Greptile findings: - `_warn_if_db_ahead_of_head` only caught `psycopg.OperationalError`. Non-connection DB errors (e.g. `InsufficientPrivilege` / 42501 if the runtime DB user lacks SELECT on `_prisma_migrations`) would propagate uncaught and crash startup — contradicting the docstring's "informational only, never blocks" guarantee. Widen the catch to `psycopg.DatabaseError` so all DB-layer errors are swallowed. - In the P3009 and P3018 idempotent-recovery paths, the call to `_resolve_specific_migration(name)` was not wrapped in its own try/except. Being inside an active `except CalledProcessError` handler, a new `CalledProcessError` from the resolve call would NOT re-enter the same handler — it would propagate out as `CalledProcessError`, past `proxy_cli.py`'s `except RuntimeError`, crashing startup with an unhandled traceback instead of the intended clean `sys.exit(2)`. Wrap both call sites to convert to RuntimeError. Adds unit tests for both behaviors.
[Infra] Merge dev branch
…hrashing [Feature] Proxy: opt-in v2 migration resolver
…treaming fix(bedrock_guardrails): use Bedrock OUTPUT source for apply_guardrail when scanning model responses
…he team default Previously, members added to a team without an explicit per-member budget were all linked to the same `litellm_budgettable` row referenced by the team's `metadata.team_member_budget_id`. Updating one member's budget via `/team/member_update` mutated the shared row and silently changed every other member's budget too. Now both write paths produce a private, per-member budget: - `add_new_member` clones the team's default budget into a fresh row when a member is added without `max_budget_in_team`/`allowed_models`. If no team default exists, the membership is created with no budget. - `_upsert_budget_and_membership` detects when an existing membership still points at the team's default budget id and clones-on-write, relinking the membership to the new private budget before applying the update. - `team_member_update` reads `team_member_budget_id` from team metadata and passes it through so the helper can make this distinction. Adds unit tests for clone-on-write, in-place update of a private budget, and the no-default-no-budget add path. Made-with: Cursor
[Infra] Bump version
…-member-budgets Litellm individual team member budgets
[Infra] Promote internal staging to main
Contributor
Author
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
|
Too many files changed for review. ( |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 29203053 | Triggered | Generic Password | c62a1e2 | .circleci/config.yml | View secret |
| 29203065 | Triggered | JSON Web Token | e8461b5 | tests/test_litellm/proxy/test_litellm_pre_call_utils.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
4 tasks
Contributor
Author
|
Closing this PR in favor of #26217, which contains the same fix on a clean branch based off This PR inadvertently picked up ~250 upstream commits via a merge from Thank you for the review feedback — the tightened wildcard assertion from this PR has been carried over to the new one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
19 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relevant issues
Fixes #23836
Pre-Submission checklist
tests/test_litellm/directorymake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewType
🐛 Bug Fix
Changes
Problem
Health checks were setting
max_tokens=1for non-wildcard models, which causes failures with newer models like GPT-5 that have minimum token requirements. A single token is often insufficient for these models to generate a valid response.Solution
Increased the default
max_tokensvalue from 1 to 16 for health check requests. This provides enough tokens for models to generate a meaningful response while still keeping the health check lightweight and fast.Changes Made
litellm/proxy/health_check.py: Changedmax_tokensdefault from 1 to 16tests/test_litellm/proxy/test_health_check_max_tokens.pyto expect the new default valueThe change is minimal and focused on fixing the token limit issue without affecting other health check behavior.