fix(pass-through): clean stale routes from auth allowlist - #26082
fix(pass-through): clean stale routes from auth allowlist#26082silencedoctor wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Medium: Stale routes left in authorization allowlist after cleanup
This PR addresses unbounded growth in _registered_pass_through_routes by using dict.pop() instead of remove_endpoint_routes(). However, the old method also cleaned up stale paths from LiteLLMRoutes.openai_routes, which is an authorization allowlist checked during route access control. The new code skips that cleanup, so stale pass-through paths accumulate in the allowlist indefinitely.
In practice, a removed route would 404 at the framework level since the FastAPI handler isn't re-registered, limiting exploitability. But the openai_routes list is consulted for budget enforcement, OAuth2 routing, and admin-viewer access checks, so a growing stale list could have subtle authorization side effects.
Greptile SummaryThis one-line fix replaces a broken Confidence Score: 5/5Safe to merge; the fix is minimal, targeted, and does not affect any other code path. The single changed line correctly resolves the root cause with no side effects. The only finding is P2 (test quality — first two unit tests verify dict built-in behavior rather than the production code path), which does not affect correctness or reliability. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/pass_through_endpoints/pass_through_endpoints.py | Replaces the broken remove_endpoint_routes(endpoint_key) call (which never matched because a composite key was compared against UUID-only values) with a direct O(1) dict.pop(). The fix correctly skips LiteLLMRoutes.openai_routes cleanup since those entries are path-keyed (not UUID-keyed) and remain valid across UUID changes. |
| tests/test_litellm/proxy/pass_through_endpoints/test_pass_through_endpoints.py | Adds TestRemoveStaleEndpointRoute with 3 tests; the core regression test (cycle simulation) is valuable, but the other two tests exercise dict built-in behavior rather than production logic. |
Reviews (1): Last reviewed commit: "fix(pass-through): pop stale route key d..." | Re-trigger Greptile
Congrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a38aeb5 to
4742643
Compare
|
Rebased onto One small conflict in |
4742643 to
8fd5b98
Compare
|
|
Veria Medium / Greptile P2 feedback on BerriAI#26082. The new pop-loop only removed stale keys from the internal registry but left their paths in LiteLLMRoutes.openai_routes.value — the allowlist consulted by RouteChecks.is_llm_api_route for authorization decisions. That allowlist would still grow unboundedly across reload cycles. Extract _remove_stale_pass_through_routes helper that pops the registry entry AND removes the path (plus wildcard for subpath routes) from openai_routes.value, mirroring what remove_endpoint_routes does. Add a direct test that drives the helper and asserts both data structures shrink, addressing Greptile's note that the existing tests exercised dict.pop() semantics rather than the production cleanup logic.
|
Addressed Veria Medium + Greptile P2: extracted |
|
The fix looks good. The
One minor nit: the Otherwise the change is correct and the core regression test ( |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
_await_model_servable used poll_timeout (120s), the spend/log read-back budget. A stuck model reload therefore stalled every suite that creates a deployment for two minutes before failing Give create_model a fixed harness middle ground: model_servable_timeout=40s, polled every 2s, with each /v1/models call capped at 5s and clamped to the remaining deadline so one slow GET cannot overrun the wait. Happy path still returns on the first listing. Not derived from proxy general_settings or env Transport.get accepts an optional per-call timeout for that clamp. Unit tests cover the deadline arithmetic and clamp without a live proxy (cherry picked from commit c082a0e)
create_model returned after the first /v1/models hit that listed the model, so chat could still land on a cold gateway worker (numWorkers>1 / peer pod) and 400 Invalid model name. Require continuous listing for the product default add_deployment interval (30s) after first sight so every worker has synced from the DB; first listing still bounded at 40s (cherry picked from commit 7d1ee2f)
Keep the create_model DB-sync wait in the harness; the pure-function unit file is not needed for this PR (cherry picked from commit 8920465)
When less than one full poll interval remained in the first-listing budget, the pre-sleep check returned NotServable without another /v1/models call. Sleep only min(interval, time left) so a model that becomes listable in the last seconds of the timeout still gets a clamped final poll (cherry picked from commit 8439195)
A poll may start with remaining budget and still return after started+timeout if the transport overruns its clamp. Recheck the first-listing deadline after the response so a late listing does not open the continuous DB-sync phase (cherry picked from commit 7ff2bcb)
…l_servable_timeout test(e2e): bound the post-/model/new servable wait at 40s
Remove authenticated pass-through paths from LiteLLMRoutes.openai_routes when no active authenticated endpoint still owns them Preserve live shared paths, idless reloads, auth transitions, and built-in OpenAI routes while reclaiming departed dynamic paths
fa8d4e4 to
4cdd05a
Compare
TLDR
Problem this solves:
How it solves it:
Relevant issues
Fixes #26081
Linear ticket
Pre-Submission checklist
Delays in PR merge?
If review is delayed, please use the LiteLLM Slack #pr-review channel
Screenshots / Proof of Fix
No end-to-end proof was captured for this internal reload registry change
Type
Bug Fix
Changes
The registry bounding fix from the original PR is already present in the current base
This PR now removes dynamic auth allowlist paths that belonged to authenticated endpoints in the previous reload but are absent from the current configuration
Cleanup preserves paths still used by another endpoint, paths retained by idless reloads, and built-in OpenAI routes. It also handles endpoints whose authentication changes from enabled to disabled
Validation
The pass-through and auth test suites completed with 135 passing tests and 1 skipped test
Final Attestation