Repository navigation
fix!: re-check budget on router fallback targets - #41379
Conversation
7718d42 to
039a4d7
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
039a4d7 to
88c0d51
Compare
|
The Docs PR: BerriAI/litellm-docs#1493 — adds the These two jobs will stay red until that merges. Verified locally with the docs branch checked out at
|
88c0d51 to
2e0aa52
Compare
Greptile SummaryThis PR adds opt-in key and user budget validation before cross-model fallback attempts and skips fallback targets whose caller is over budget
Confidence Score: 3/5This PR is not yet safe to merge because fallback key-budget enforcement can accept stale-low spend state, and explicit repository requirements remain unsatisfied The key fallback lookup does not supply its budget to Files Needing Attention: litellm/proxy/auth/fallback_budget.py, tests/test_litellm/proxy/test_proxy_server.py Security ReviewA blocking authentication-layer correctness gap remains because the key fallback decision can use stale-low spend state without authoritative floor verification Important Files Changed
Reviews (1): Last reviewed commit: "fix: re-check budget on router fallback ..." | Re-trigger Greptile |
| key_spend: Final = await _counter_spend( | ||
| counter_key=f"spend:key:{valid_token.token}", | ||
| fallback_spend=valid_token.spend or 0.0, | ||
| ) |
There was a problem hiding this comment.
Omitting max_budget lets stale-low counters allow paid fallbacks.
How this was verified: Existing key checks pass max_budget for authoritative verification.
Rule Used: What: Fail any PR which may contains a security incident on litellm's authentication layer Why: Do not cause security incidents Bad: ```python # Check cache first cache_key = ( f"oidc_userinfo_{token[:20]}" # Use fi... (source)
|
|
||
| return general_settings.get("apply_user_budget_to_team_keys") is True | ||
|
|
There was a problem hiding this comment.
Each paid target can call database-backed get_current_spend twice, violating the requirement to avoid critical-path database requests and use budget object helpers.
Rule Used: What: Avoid creating new database requests or Router objects in the critical request path. Why: Creating these objects on every request causes performance degradation and unnecessary resource consumption. (source)
|
|
||
| router, _, _ = await ProxyConfig().load_config(router=None, config_file_path=str(config_file)) | ||
|
|
||
| assert router.fallback_budget_check is router_fallback_budget_check |
There was a problem hiding this comment.
This identity assertion tests callback wiring, not fallback behavior, violating the requirement that tests verify function rather than code structure before merging.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if _is_model_cost_zero(model=model, llm_router=llm_router): | ||
| return True | ||
|
|
||
| key_budget: Final = valid_token.max_budget |
There was a problem hiding this comment.
Low: Non-key budget scopes are not enforced
This predicate checks only key and personal-user limits. A caller whose team, team-member, end-user, organization, global, or per-model budget is exhausted can request a free group and still consume a paid fallback because auth skipped all those checks for the original free model. The fallback evaluation needs read-only checks for every budget scope that common_checks bypasses.
| if valid_token is None: | ||
| return True | ||
| try: | ||
| return await is_token_within_budget_for_model(model=model, valid_token=valid_token, llm_router=llm_router) |
There was a problem hiding this comment.
Low: Fallback admission does not reserve budget
This performs a read-only spend check after auth skipped budget reservation for the free model. An attacker can submit many concurrent requests while the counter is just below its limit; each observes the same available budget and proceeds to the paid fallback before cost callbacks update the counter. Atomically reserve the fallback's estimated cost against the applicable counters before attempting it, and reconcile or release that reservation through the existing completion callbacks.
| if not self.is_enforced(): | ||
| return True | ||
| valid_token: Final = _user_api_key_auth_from_request(request_kwargs) | ||
| if valid_token is None: |
There was a problem hiding this comment.
Low: Missing auth metadata fails open
Authenticated proxy requests do not all carry user_api_key_auth. In particular, /queue/chat/completions manually adds scalar user_api_key_* metadata and then calls schedule_acompletion, so an over-budget caller can trigger a paid fallback through that endpoint and this branch allows it. Ensure every authenticated router call carries the trusted auth object, or distinguish known internal calls and fail closed for proxy requests where it is absent.
PR overviewThis pull request adds budget re-checking when the router moves a request from a free model group to a paid fallback target. The changes focus on fallback-budget handling in the proxy authentication flow. One issue has been addressed, but paid fallbacks can still proceed despite exhausted budgets in several cases, including non-key scopes, rolling key windows, and requests missing trusted authentication metadata. Concurrent requests can also pass read-only checks before spend counters update, allowing aggregate usage beyond configured limits. These gaps permit authenticated callers to generate unintended paid usage, though the impact is limited to budget enforcement and excess spend. Open issues (4)
Fixed/addressed: 1 · PR risk: 6/10 |
2e0aa52 to
de7716b
Compare
|
Thanks both — pushed FixedStale spend can pass (Greptile P1, Test checks wiring only (Greptile P2, Documented as known limitationsNo reservation on fallback admission (veria-ai) — accurate. This reads the counter rather than reserving against it, so concurrent requests all observe the same pre-billing figure and a cap can be crossed by roughly the number of in-flight fallbacks times their cost. Auth-time enforcement avoids this via Missing auth metadata fails open (veria-ai) — verified, and correct: only Answered, no changeEnforcement disabled by default (veria-ai) — deliberate, and the open question I raised in #41344. Defaulting to Non-key budget scopes not enforced (veria-ai) — deliberate and disclosed in the PR description and #41344. The auth-path functions for team, team-member, end-user, org, global and per-model budgets raise rather than return a verdict, and reusing them here would fire threshold alerts and take spend reservations for a target that is then skipped. Full coverage wants read-only evaluation paths per dimension; happy to follow up. Fallbacks add database reads (Greptile P2) — worth noting this pulls directly against the P1 fix: passing CI note: |
|
Thanks — the stale-counter finding is addressed: both counter reads now pass The database-path finding remains unresolved under the repository’s request-path rule. The default-off behavior, limited budget scopes, missing metadata behavior, and lack of reservation are documented design limitations rather than additional actionable findings for this scoped change. I would clear the stale-spend and test-wiring findings, but keep the confidence at 3/5 until the request-path database work is resolved or explicitly accepted. Tip: You can customize Greptile's behavior for this repo with |
| if _is_model_cost_zero(model=model, llm_router=llm_router): | ||
| return True | ||
|
|
||
| key_budget: Final = valid_token.max_budget |
There was a problem hiding this comment.
Low: Rolling key budgets are not enforced
A key that has exhausted a budget_limits window can request a free model group and still trigger its paid fallback because this path checks only max_budget. Evaluate every configured window against its spend:key:{token}:window:{budget_duration} counter using the same window parameters as _virtual_key_multi_budget_check before admitting the fallback.
Budget is enforced once during auth, against the requested model group. `_is_model_cost_zero` waives every budget check for a zero-cost group, and the router then picks a fallback target afterwards, inside `run_async_fallback`, where nothing re-checks budget. A free model with a paid fallback therefore bills with no budget gate at all. Add `fallback_budget_check`, the budget sibling of the existing `fallback_access_check`: a predicate awaited per fallback target that skips targets the caller cannot pay for. The primary attempt is untouched, so a zero-cost model is never blocked by budget and only the paid fallback is refused. Counter reads pass `max_budget` so `get_current_spend` verifies against authoritative recorded spend, matching the auth-time key and user checks; a counter restored from an older snapshot reads as a hit rather than a clean miss, so without it a stale-low value would keep admitting paid fallbacks. A zero-cost fallback target is always allowed, and a team key does not inherit the key owner's personal budget unless `apply_user_budget_to_team_keys` is set, matching `_PROXY_MaxBudgetLimiter`. Scope is key and user budgets. Team, team-member, end-user, org, global and per-model budgets are not covered yet: those auth-path functions enforce rather than report, so reusing them would fire threshold alerts and take spend reservations for a target that is then skipped. Two limitations of that scope are documented in the module docstring: the check reads the spend counter rather than reserving against it, so concurrent fallbacks can cross a cap together; and a request reaching the router without `metadata["user_api_key_auth"]` is not restricted. Both are shared with `fallback_model_access.py`. Opt-in via `general_settings.enforce_fallback_budget`. Relates to BerriAI#41344 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
de7716b to
4a70bc3
Compare
|
@ryan-crabbe-berri — tagging you since this is the direct sibling of This is out of draft and the Proof of Fix section now has a full e2e run against a real proxy and Postgres, with the leak reproduced at the merge base and closed at the PR tip. Two things need a decision from a maintainer rather than more work from me. 1. A Greptile rule that conflicts with a Veria findingGreptile is holding at 3/5 on one finding and offers two exits: "resolved or explicitly accepted." It objects that The conflict is that Veria's findings on the same PR ask for more budget dimensions — specifically the key's rolling Greptile's own P1 on this PR also asked me to pass What I'd like from you, whichever you prefer:
I did not want to trade away a capability to satisfy a bot without you weighing in. 2. The two red checks are not fixable from this repo
BerriAI/litellm-docs#1493 adds the row, plus the Also worth your attentionThe e2e surfaced something relevant to #29912 and #38515. My first run used a personal |
A budget bypass that ships off by default stays open for every deployment that does not know to look for the flag, so `enforce_fallback_budget` now defaults to true and `general_settings.enforce_fallback_budget: false` is the opt-out for anyone who wants the old unguarded behaviour back. BREAKING CHANGE: a paid fallback target is now refused for callers who are over their key or user `max_budget`. Deployments relying on fallbacks to keep serving over-budget callers must set enforce_fallback_budget: false.
* docs: document fallback_budget_check and enforce_fallback_budget Router gained `fallback_budget_check` (BerriAI/litellm#41379), the budget sibling of `fallback_access_check`: budget is enforced once during auth against the requested model group, while the fallback target is chosen afterwards inside the router, so a zero-cost model with a paid fallback bills with no budget gate. Adds the `router_settings` reference row, the `general_settings` reference row and yaml entry for `enforce_fallback_budget`, and an "Enforce Budget on Fallbacks" section mirroring "Enforce Key Model Access on Fallbacks". Relates to BerriAI/litellm#41344 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: enforce_fallback_budget is on by default --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: ryan-crabbe-berri <ryan@berri.ai>
TLDR
Problem this solves:
How it solves it:
fallback_access_check, on by defaultUser Flow
Before: a developer on a free model with a paid fallback keeps being billed after their budget is gone
After: the free model still works, and only the paid fallback is refused once they are over budget
x-litellm-attempted-fallbacksheaderenforce_fallback_budget: falseto get the old behavior backRelevant issues
Relates to #41344
Related, and compatible with, #29912 and #38515: both ask for the zero-cost bypass to be extended to
_PROXY_MaxBudgetLimiter. That hook is currently the only thing bounding fallback spend for a zero-cost group, so this change is what makes extending it safe. See the note under Proof of Fix, which shows that hook masking the leak on the personal-budget path today.Affected release
This is a regression, not a long-standing gap. #20249 (v1.81.16) added the zero-cost bypass that waives every auth-time budget check, and that is what lets an over-budget key reach the router at all. Before it, the key's
max_budgetcheck refused the request whatever model it named, so the fallback never ran.Linear ticket
Pre-Submission checklist
documentationandcode-qualitycannot pass from this repo; they need docs: document fallback_budget_check and enforce_fallback_budget litellm-docs#1493 merged (details below)Screenshots / Proof of Fix
Shared setup. Real proxy, real Postgres, real router fallback walk, real spend pipeline, and a real paid provider: the fallback target is OpenAI
gpt-5.5on a live API key, so every fallback that gets taken costs real money and lands in the spend table. The zero-cost primary points at a closed port so it always fails. Nothing is mocked.Two keys, both created through
POST /key/generate:max_budget: 0.0005, driven past its cap with two directpaid-modelcalls, then confirmed at$0.00065through/key/infomax_budget: 100.0Cases 1 and 2 run against the config above. Case 3 adds
enforce_fallback_budget: falseundergeneral_settingsand nothing else.Before (930ec96)
1. Over-budget key requests the zero-cost model
GET /key/info->spend: 0.0013,max_budget: 0.0005POST /v1/chat/completions {"model": "free-model"}GET /key/info->spend: 0.00162. The paid fallback billed while the key was already over its cap2. Under-budget key requests the zero-cost model
POST /v1/chat/completions {"model": "free-model"}->HTTP/1.1 200 OK,x-litellm-attempted-fallbacks: 13. Over-budget key with
enforce_fallback_budget: falseGET /key/info->spend: 0.00162,max_budget: 0.0005POST /v1/chat/completions {"model": "free-model"}->HTTP/1.1 200 OK,x-litellm-attempted-fallbacks: 1GET /key/info->spend: 0.00178. The setting is in the config and has no effect, because this build does not know itAfter (17844cf)
1. Over-budget key requests the zero-cost model
GET /key/info->spend: 0.00178,max_budget: 0.0005POST /v1/chat/completions {"model": "free-model"}x-litellm-attempted-fallbacksheader. The paid target was skipped, not attemptedGET /key/info->spend: 0.00178, unchanged. The caller getsfree-model's own error instead of a billed answerThis is the whole point of the change: there is no
enforce_fallback_budgetanywhere in the config for this run. An unconfigured proxy now refuses the paid fallback.2. Under-budget key requests the zero-cost model
POST /v1/chat/completions {"model": "free-model"}->HTTP/1.1 200 OK,x-litellm-attempted-fallbacks: 1The paid fallback is still taken. Budget state is the only difference between this and case 1, so the 500 above is the skip, not a dead upstream.
3. Over-budget key with
enforce_fallback_budget: falsePOST /v1/chat/completions {"model": "free-model"}->HTTP/1.1 200 OK,x-litellm-attempted-fallbacks: 1GET /key/info->spend: 0.00178then0.00194. The opt-out puts the old unguarded behavior back, exactly as the Before run shows itNote for #29912 / #38515
An earlier run of this used a personal
max_budgetand returned429 Max budget limit reachedat auth, never reaching the fallback._PROXY_MaxBudgetLimiterstill blocks zero-cost models when the user's personal budget is exhausted, the exact behavior those two issues ask to remove, so it incidentally masks this leak on that one path. It readsuser_max_budgetonly, so key budgets (used above) are already unguarded today. Removing that hook's blocking without a fallback-time check would open the personal-budget path too.Implementation notes
The mechanism is the budget sibling of
fallback_access_check, whose module docstring describes this same class of problem: auth-time checks run against the requested group, the router picks a different one afterwards.Router(fallback_access_check=…)Router(fallback_budget_check=…)RouterFallbackAccessCheckRouterFallbackBudgetCheckcan_key_call_resolved_model(...)max_budget, evaluated against the targetgeneral_settings.enforce_fallback_model_access(opt-in)general_settings.enforce_fallback_budget(on, opt out withfalse)Three deliberate behaviours:
apply_user_budget_to_team_keysis set, matching_PROXY_MaxBudgetLimiter.fallback_model_access.py. The tradeoff is that a transient spend-counter failure denies the paid fallback to someone under budget.Scope and known limitations are stated in the module docstring: this covers the key's and the user's
max_budget. Team, team-member, end-user, org, global, per-model and the key's rollingbudget_limitswindows are not covered. It also reads the spend counter rather than reserving against it, so concurrent fallbacks can cross a cap together; and a request reaching the router withoutmetadata["user_api_key_auth"]is not restricted. The last two are shared withfallback_model_access.py.Type
🐛 Bug Fix
Caveats (if any)
Severe
enforce_fallback_budget: falseMedium
max_budget; team, org, per-model, windows uncoveredLow
fallback_access_checkmetadata["user_api_key_auth"]stay unrestrictedFinal Attestation