chore(sso): bind generic SSO state to a session cookie - #26944
Conversation
The Generic SSO PKCE flow used the URL ``state`` parameter as the
cache key for the PKCE ``code_verifier`` without binding the state
to the caller's browser. An attacker who pre-minted a state and
cached a verifier under it could hand the resulting login link to a
victim; the victim's auth code would then be exchanged with the
attacker's verifier on the callback, producing an access token
under the attacker's control (Login CSRF / token theft).
The non-PKCE branch is unaffected because it delegates to
fastapi-sso's ``verify_and_process``, which performs its own
session-cookie check. The PKCE branch bypasses that helper, which
is exactly the gap this commit closes.
Two-part fix in ``ui_sso.py``:
- ``get_generic_sso_redirect_response`` now sets a
``litellm_oauth_state`` cookie (HttpOnly, SameSite=Lax, 10-min TTL)
carrying the state value used in the redirect URL. The cookie is
set on the redirect response just like the existing
``litellm_cp_return_to`` cookie a few lines earlier in the file.
- ``get_generic_sso_response`` validates ``request.cookies.get(
"litellm_oauth_state")`` against ``request.query_params.get(
"state")`` via ``secrets.compare_digest`` before invoking the
PKCE token exchange. Mismatch (or either being missing) raises a
``ProxyException`` with HTTP 400.
The pre-existing TODO above the redirect logic ("state should be a
random string and added to the user session with cookie") is now
addressed and removed.
Tests cover the redirect-side cookie set, the missing-cookie reject
shape, the URL/cookie-mismatch reject shape, and the matching-cookie
happy path.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR closes a Login-CSRF / token-theft vulnerability in the Generic SSO PKCE flow by binding the OAuth Confidence Score: 5/5Safe to merge — the security fix is correctly implemented with no regressions or new issues. No P0 or P1 issues found. The cookie is scoped strictly to the PKCE branch (both write and read sides), No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/ui_sso.py | Security fix: adds litellm_oauth_state HttpOnly cookie (PKCE flows only) on the redirect side and validates it via secrets.compare_digest on the callback side; correctly scoped to the PKCE branch, defaults Secure=True when scheme is unknown, and threads request through all callers. |
| tests/test_litellm/proxy/management_endpoints/test_ui_sso.py | Adds TestPKCEStateCookieBinding covering cookie presence, HttpOnly/SameSite/Secure attributes, non-PKCE omission, HTTP dev mode (no Secure), missing-cookie rejection, state-mismatch rejection, and happy-path acceptance; all tests use mocks with no real network calls. |
Reviews (2): Last reviewed commit: "fix(sso): tighten oauth_state cookie — S..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Two Greptile review findings addressed: 1. (P1, security) The ``litellm_oauth_state`` cookie is the sole guard against Login-CSRF in the PKCE flow but was set without the ``Secure`` attribute, so a network observer on plain HTTP could read and replay it — bypassing the protection this PR adds. Thread the originating ``Request`` down through ``get_sso_login_redirect`` and ``get_generic_sso_redirect_response`` and set ``Secure`` based on ``request.url.scheme == "https"``. When no request is supplied (programmatic callers / tests) default to ``Secure=True`` — production-safe. Local HTTP dev still works because the request scheme is observed at runtime. 2. (P2) The cookie was set unconditionally, but the callback only validates it inside the PKCE branch. Two concurrent SSO sessions (one PKCE, one plain) could overwrite each other's state cookie and produce spurious 400s for the plain-flow user. Move the ``set_cookie`` call inside the existing ``if code_verifier and "state" in redirect_params`` block so the cookie is only written when PKCE is active and the validation will actually fire. Tests cover both paths: PKCE-on (cookie set with Secure default), PKCE-off (cookie not set), and HTTP dev request (Secure dropped so the browser will actually attach the cookie on the callback hop).
|
@greptileai please re-review — addressed both findings in 2c852ba:
Tests added for all three branches: PKCE-on (cookie set + Secure default), PKCE-off (cookie not set), HTTP dev (Secure dropped). 6/6 |
0ff9d65
into
BerriAI:litellm_internal_staging
chore(sso): bind generic SSO state to a session cookie
2 participants
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
Type
🐛 Bug Fix
Changes
The Generic SSO PKCE flow used the URL `state` parameter as the cache key for the PKCE `code_verifier` without binding that state to the caller's browser. An attacker who pre-minted a state and cached a verifier under it could hand the resulting login link to a victim; the victim's auth code would then be exchanged with the attacker's verifier on the callback, producing an access token under the attacker's control (Login CSRF / token theft).
The non-PKCE branch is unaffected because it delegates to `fastapi-sso`'s `verify_and_process`, which performs its own session-cookie check. The PKCE branch bypasses that helper — exactly the gap this PR closes.
Two-part fix in `litellm/proxy/management_endpoints/ui_sso.py`:
The pre-existing TODO above the redirect logic ("state should be a random string and added to the user session with cookie or a cryptographicly signed state that we can verify stateless") is now addressed and removed.
Tests
`tests/test_litellm/proxy/management_endpoints/test_ui_sso.py` adds a `TestPKCEStateCookieBinding` class covering:
180/180 tests in `test_ui_sso.py` pass.