Repository navigation
fix(proxy): treat omitted auth on config pass-through routes as enforced at registration - #44253
devin-ai-integration[bot] wants to merge 3 commits into
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
bugbot run |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1de04ce. Configure here.
|
@veria-ai run |
TLDR
Problem this solves:
authstopped writing failure spend rowsauthwas registered as unauthenticatedHow it solves it:
endpoint_data.get("auth", True), matching request-time authinclude_subpathwildcard land inopenai_routesagainUser Flow
Before: an admin's config pass-through route with no
authkey hides upstream failures from spend logs/audit-pttogeneral_settings.pass_through_endpointswithinclude_subpath: trueand noauthAfter: the same failed request shows up in spend logs as a failure row
authkeyfailurerow for that request id with error code 403Relevant issues
Regression from #43962
Affected release
Regression since v1.105.0-dev.2 (commit 2eb2bf1, also on rc/1.105.0). Cherry-pick targets: stable/1.103.x, rc/1.104.0, rc/1.105.0
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 more@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
DB-backed proxy. Measured on a live proxy (2 workers, real Postgres, scripted upstream that always answers 403
{"error": {"message": "max budget reached for this deployment"}}) at each revision with the same config:/audit-ptwithinclude_subpath: trueand noauth,/audit-pt-offwithauth: false, plus DB endpoints created through POST /config/pass_through_endpoint (/audit-dbdefault auth,/audit-db-offwithauth: false). Keys: master key, a non-admin internal-user key with noallowed_passthrough_routes, and a key on a blocked team (to see whether common_checks runs)/audit-ptinopenai_routes/audit-pt/*inopenai_routesis_auth_enforced_pass_through_routeexact / subpathallowed_passthrough_routes, /audit-ptallowed_passthrough_routesallowed_passthrough_routesauth: false/audit-pt-off: no key,openai_routes, spend row/audit-db:openai_routesexact/wildcard, no key, valid keyallowed_passthrough_routesallowed_passthrough_routes/audit-db-off(auth: false):openai_routes, no keyallowed_routes: ["llm_api_routes"], /audit-ptallowed_passthrough_routesallowed_passthrough_routesmax_budget: 0), /audit-pt and /audit-pt/subbudget_exceededbudget_exceededDB-less proxy (no
database_url). Without a prisma client the parent never re-registered config entries throughPassThroughGenericEndpoint, so omittedauthstayed unenforced there and this PR does not match 2eb2bf1^ on DB-less proxies. Measured with one worker, the same scripted upstream,/audit-ptwithinclude_subpath: trueand noauth,enforce_user_param: trueandreject_clientside_metadata_tags: true, master key on every request/audit-ptand/audit-pt/*inopenai_routesuser'user' param not passed inmetadata.tagsClient-side 'metadata.tags' not allowedThe common_checks skip on the exact path is the pre-existing
endpoint.get("auth") is not Truecheck in user_api_key_auth.py, unchanged here because #39017 and #43250 own that policyShared setup for the curl proof below:
KEYis a fresh virtual key from POST /key/generate withkey_alias: audit-proof,MKis the master keyBefore (ac59ec6)
After (58fa9b2)
Admin UI Logs page (http://localhost:/ui/?page=logs, logged in as admin) right after the same request: main shows "No requests yet", fix shows the Failure row for c4f43c72 with key alias audit-proof
The head after review, 1de04ce, only wraps long test lines and rewords one suppression reason in the unit test, so the DB-backed rows measured on 58fa9b2 carry over. Terminal audit on head 58fa9b2: the same integration node, run twice back to back through the integration rig against the PR head, passed both times (1 passed in 28.77s, 1 passed in 25.74s). Post-review live risk: no code changed after the bot loop (Greptile 5/5 and Bugbot clean on 58fa9b2), so the A/B matrix above still describes the head
The existing integration node
tests/integration/observability/test_passthrough_upstream_error_visibility.py::test_config_pass_through_route_logs_body_and_strips_queryis unchanged and passes at 2eb2bf1^, fails on main (State did not converge: []waiting for the spend row) and passes at this PR's headType
🐛 Bug Fix
✅ Test
Caveats (if any)
Severe
authregain parent behavior, so non-admin keys needallowed_passthrough_routeson subpaths againauth, unlike 2eb2bf1^enforce_user_param: true, a master-key POST to a subpath withoutusernow gets 401reject_clientside_metadata_tags: true, clientmetadata.tagson a subpath now gets 400allowed_passthrough_routesauth: falseon the entryallowed_routes: ["llm_api_routes"]now get 403 on the exact path too, and over-budget keys get 422 on both pathsLow
authis left as is, owned by fix(proxy): enforce auth defaults for raw pass-through config #39017 and fix(proxy): run policy checks on config pass-through entries that omit auth #43250Final Attestation
Link to Devin session: https://app.devin.ai/sessions/90bf11d0f3a449eda38ad50de59af074
Open in Devin Desktop: https://app.devin.ai/desktop/session/90bf11d0f3a449eda38ad50de59af074?variant=devin
Note
Medium Risk
Changes proxy auth registration for config pass-throughs (including DB-less deployments) and restores stricter key/route checks; operators who relied on omitted
authmust setauth: falseexplicitly.Overview
Config pass-through routes with no
authkey are registered as authenticated again, matching request-time behavior inuser_api_key_auth(get("auth", True)).Registration now uses
endpoint_data.get("auth", True)instead of bareget("auth"), so omittedauthenablesuser_api_key_authon the route, adds exact and wildcard paths toLiteLLMRoutes.openai_routes, and restores spend/failure logging and route gates that regressed when omittedauthwas treated as unenforced.Explicit
auth: falseis unchanged. New parametrized unit tests cover config dicts and DBPassThroughGenericEndpointregistration for exact and subpaths.Reviewed by Cursor Bugbot for commit 1de04ce. Bugbot is set up for automated code reviews on this repo. Configure here.