refactor(proxy/auth): cherry-pick #29343 into patch/v1.87.0rc1 - #29362
Conversation
…9343) * refactor(proxy/auth): normalize Bearer prefix in safe-hash helper UserAPIKeyAuth._safe_hash_litellm_api_key now strips a leading "Bearer "/"bearer " prefix before its existing sk-/JWT classification, so the helper produces the same hashed output regardless of whether the caller stripped the Authorization header prefix or passed the header value through unchanged. * refactor(proxy/auth): make Bearer-prefix strip case-insensitive Per RFC 7235 the HTTP authorization scheme token is case-insensitive. Replace the two-prefix loop with a single case-insensitive check so the helper normalizes "Bearer ", "bearer ", "BEARER ", and any mixed-case variant before classifying the remainder as sk- or JWT. The contract test gains coverage of "BEARER " and "BeArEr ". * test(mcp): align auth-handler test expectations with safe-hash helper The two MCP auth tests asserted that UserAPIKeyAuth(api_key="Bearer ...") retained the raw header bytes on the api_key field. _safe_hash_litellm_api_key now normalizes that input — stripping the Bearer prefix and hashing the resulting sk- key — so the expectations move to the normalized form: the bare token in the parametrize case, and hash_token("sk-...") in the backward-compat assertion. This matches what the real auth flow produces (the builder strips Bearer and the DB stores the hashed token), so the mocks now line up with production rather than with the un-normalized validator output.
Greptile SummaryThis cherry-pick fixes a key-leakage bug where
Confidence Score: 4/5Safe to merge — the change is a focused, no-conflict cherry-pick that closes a real observability key-leak with no regressions on the changed path. The core fix in _safe_hash_litellm_api_key is correct and the test updates reflect genuinely improved assertions (checking for a proper hash rather than a raw Bearer string). The only minor concern is that the test imports hash_token from litellm.proxy.utils while the production code calls a separate but identical copy in _types.py; if either copy diverges the test would silently pass against the wrong function. No other issues found across the three changed files. No files require special attention; all three changed files are straightforward and the logic is easy to follow.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_types.py | Adds Bearer-prefix stripping in _safe_hash_litellm_api_key before hashing; fix is minimal, correct, and handles case-insensitive variants. |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Two assertion updates: one now expects the Bearer prefix to be stripped, the other now verifies the key is properly hashed (stricter assertion than before). |
| tests/test_litellm/proxy/test_proxy_types.py | New contract test covering all four capitalisation variants of the Bearer prefix; verifies both api_key and token fields are normalised correctly. |
Reviews (1): Last reviewed commit: "refactor(proxy/auth): normalize Bearer p..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Should succeed with the LiteLLM key from Authorization header | ||
| assert auth_result.api_key == "Bearer sk-litellm-valid-key" | ||
| from litellm.proxy.utils import hash_token | ||
|
|
||
| assert auth_result.api_key == hash_token("sk-litellm-valid-key") |
There was a problem hiding this comment.
hash_token import inside the assertion block pulls from litellm.proxy.utils, while _safe_hash_litellm_api_key (in _types.py) calls its own local hash_token defined at line 218. Both are identical SHA-256 implementations today, so the assertion is valid — but if either copy is changed independently the test would silently diverge. Importing from _types keeps the test coupled to the same symbol the production path uses.
| # Should succeed with the LiteLLM key from Authorization header | |
| assert auth_result.api_key == "Bearer sk-litellm-valid-key" | |
| from litellm.proxy.utils import hash_token | |
| assert auth_result.api_key == hash_token("sk-litellm-valid-key") | |
| # Should succeed with the LiteLLM key from Authorization header | |
| from litellm.proxy._types import hash_token | |
| assert auth_result.api_key == hash_token("sk-litellm-valid-key") |
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!
| return api_key | ||
| if JWTHandler.is_jwt(token=normalized): | ||
| return f"hashed-jwt-{hash_token(token=normalized)}" | ||
| return normalized |
There was a problem hiding this comment.
return normalized returns the prefix-stripped token for keys that are neither sk-* nor JWT. For an opaque Bearer <token> value that matches neither case, the old code returned Bearer <token> verbatim (unhashed), while the new code returns <token> (also unhashed). Both are technically correct — the pre-existing behaviour was already not hashing this class of input — but a clarifying comment here would make the intentional non-hashing explicit for future readers.
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!
1 participant
Relevant issues
Cherry-pick of #29343 (merge commit
94a043efb2) onto thepatch/v1.87.0rc1branch.Linear ticket
n/a
Pre-Submission checklist
make test-unit(asserted in refactor(proxy/auth): normalize Bearer prefix in safe-hash helper #29343; this PR is a no-conflict cherry-pick of that exact commit)CI (LiteLLM team)
Bug verification on v1.87.0-rc.1
Confirmed the bug is present at the v1.87.0-rc.1 tag (head of
patch/v1.87.0rc1) before this cherry-pick lands._safe_hash_litellm_api_keyonly branches onsk-prefix and JWT; any other input falls through toreturn api_keyunchanged, so passing the rawAuthorizationheader value (Bearer sk-...) returns the literal string.Effect on the v1.87.0-rc.1 build: any code path that hands the raw header value to this helper (e.g. observability labels such as Prometheus
litellm_proxy_failed_requests_metric_total{hashed_api_key=...}) ends up emitting the unhashedBearer sk-...string as a metric label, leaking the key.Screenshots / Proof of Fix
End-to-end proof-of-fix (curl-driven Prometheus scrape showing the same metric row going from
hashed_api_key="Bearer sk-..."to a proper sha256 hash) is captured in the original PR body at #29343 and was run against a live proxy with prometheus enabled. The cherry-pick is verbatim, so the same harness output applies.Type
Refactoring + Test
Changes
Verbatim cherry-pick of #29343's merge commit
94a043efb226c5ccdbfc028fbb930ce45fb965ebontopatch/v1.87.0rc1. Auto-merged cleanly (no conflicts). 3 files, +30/-7 -- matches the sum of the three original commits exactly (+8/-5 in_types.py, +4/-2 in the MCP auth test, +18/-0 for the new contract test). Full rationale lives in the #29343 PR body.