Skip to content

fix(ui): remove insecure ?token= URL handler from LoginPage to close session-fixation - #26924

Merged
yuneng-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_fix_ui_session_fixation_url_token
May 1, 2026
Merged

fix(ui): remove insecure ?token= URL handler from LoginPage to close session-fixation#26924
yuneng-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_fix_ui_session_fixation_url_token

Conversation

@michelligabriele

Copy link
Copy Markdown
Collaborator

Relevant issues

N/A — internal security audit finding (automated scanner). No public GitHub issue.

Linear ticket

N/A

Pre-Submission checklist

  • I have added testing — two new tests live in ui/litellm-dashboard/src/app/login/LoginPage.test.tsx alongside the fix. The change is UI-only (no Python code modified), so I added them to the dashboard test suite rather than tests/test_litellm/. Happy to mirror them somewhere else if there's a convention I missed.
  • make test-unit — not run; this is a frontend-only change with no Python code modified, so the suite shouldn't be affected. Will run if a maintainer would like me to.
  • My PR's scope is as isolated as possible — pure deletion of the legacy ?token= URL handler + regression tests. The related dead-code cleanups and ?code= defense-in-depth I noticed are deliberately kept out (see "Out of scope" below).
  • Greptile review — will request as a comment after the PR is open.

Delays in PR merge?

N/A

CI (LiteLLM team)

  • Branch creation CI run — Link: pending; will fill once CircleCI picks up the branch push
  • CI run for the last commit — Link: pending
  • Merge / cherry-pick CI run — Links: pending

Screenshots / Proof of Fix

Code-deletion fix with no visible UI change, so screenshots don't really apply. Proof is the regression-test output from npx vitest run src/app/login/LoginPage.test.tsx:

✓ LoginPage > URL ?token= legacy path is rejected (security regression test) > must not set a token cookie or redirect to /ui/?login=success when ?token= is in the URL
✓ LoginPage > URL ?token= legacy path is rejected (security regression test) > must not overwrite an existing valid session cookie when ?token= is in the URL
Test Files  1 passed (1)
     Tests  10 passed (10)

Manual reproduction (before the fix):

  1. Visit /ui/login?token=<any_unsigned_JWT_with_future_exp>.
  2. Observe the token cookie is set to the URL-supplied value, the URL bar is rewritten to drop ?token=, and the dashboard loads at /ui/?login=success.

After the fix: the cookie is not set, the URL stays on /ui/login, and the login form renders normally.

Type

🐛 Bug Fix (security)

Changes

What was wrong

LoginPage.tsx accepted a JWT directly from the ?token= URL query parameter, validated it only by base64-decoding the payload to check exp (no signature verification — the proxy signs with LITELLM_MASTER_KEY via HS256 in litellm/proxy/management_endpoints/ui_sso.py:2681-2685, which the browser cannot hold), and wrote it straight to document.cookie. The block ran before the existing-session check, so an attacker URL would also overwrite a logged-in user's cookie unconditionally.

The result is a session-fixation primitive: an attacker delivering /ui/login?token=<their_JWT> to a victim fixates the victim's browser onto the attacker's identity. The dashboard then uses decoded.key as the bearer token (useAuthorized.ts:43-54), so any virtual keys, model entries, or team configuration the victim creates are attributed to the attacker's user_id and harvestable when the attacker logs back into their own account.

The fix

Delete the legacy ?token= block (LoginPage.tsx:69-82, ~14 lines). The block was self-documented as a backwards-compat shim, and from what I could find it has no live callers — every other authentication flow uses either a backend Set-Cookie (/login 303 redirect, /v2/login JSON, /v3/login/exchange JSON, SSO callback 303) or the modern ?code= SSO exchange (single-use, 60s TTL, server-validated). Invitation links use ?invitation_id=<UUID>. After the deletion, authentication relies exclusively on those flows.

Tests added

Two regression tests in ui/litellm-dashboard/src/app/login/LoginPage.test.tsx:

  • ?token=attacker.jwt.value with no active session → cookie is not set, no redirect to /ui/?login=success, login form renders.
  • ?token=attacker.jwt.value with an existing valid session → existing-session redirect to /ui runs, attacker token does not overwrite the legitimate cookie.

The two-test split is intentional: the original misbehavior had two distinct facets (accepting a URL token at all, and overwriting an active session cookie), and testing both makes the suite resistant to a partial-fix regression.

Out of scope (might be good follow-up PRs)

A couple of related things I noticed but kept out of this PR to keep the security diff minimal:

  • Backend cleanup of the dead login_url JSON field at litellm/proxy/proxy_server.py:12109 and the matching OnboardingCredentials.login_url field in useOnboarding.ts:10. The onboarding frontend reads token from the JSON response body and never navigates to login_url (OnboardingForm.tsx:28-45), so it appears to be dead JSON. Not a security issue, just cleanup — might be a good follow-up to drop it now that the URL-token consumer is gone.
  • Defense-in-depth active-session guard on the ?code= SSO exchange path. The same fix-class would apply, but the attack surface is much weaker (60s TTL, single-use, server-validated codes), and the right behavior depends on worker-URL reasoning (a control-plane admin logging into a new worker legitimately needs the worker's JWT to overwrite the existing cookie). Probably worth a separate PR where that nuance can be discussed on its own.

@greptile-apps

greptile-apps Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR removes the legacy query-parameter token handler from LoginPage.tsx that allowed session-fixation: the removed block accepted a token value from the URL and wrote it to a browser cookie without signature verification, before any session check. The change is accompanied by two regression tests covering the no-session and active-session cases.

Confidence Score: 5/5

Safe to merge — security-only deletion with targeted regression tests and no functional regressions in the remaining auth flows.

The change is a pure deletion of clearly identified vulnerable code. All existing auth flows are untouched. The two new tests mirror the exact pre-fix misbehavior and correctly validate the fix. No new code paths or side effects are introduced.

No files require special attention.

Important Files Changed

Filename Overview
ui/litellm-dashboard/src/app/login/LoginPage.tsx Removes the insecure ?token= URL handler (14 lines) that allowed session-fixation; no other logic is changed.
ui/litellm-dashboard/src/app/login/LoginPage.test.tsx Adds two regression tests verifying the ?token= path no longer sets a cookie or redirects to /ui/?login=success, covering both no-session and active-session scenarios.

Reviews (1): Last reviewed commit: "fix(ui): remove insecure ?token= URL han..." | Re-trigger Greptile

Bojun-Vvibe added a commit to Bojun-Vvibe/oss-contributions that referenced this pull request May 1, 2026
Two security-shaped PRs in litellm:
- BerriAI/litellm#26945: merge-as-is, scope stored CLI key to base_url
  via expected_base_url kwarg in get_litellm_gateway_api_key, with
  symmetric rstrip normalization at write+read and fail-closed for
  legacy tokens missing base_url; 4-test contract pin
- BerriAI/litellm#26924: merge-as-is, total deletion of urlToken handler
  at LoginPage.tsx:69-80 closing session-fixation; 2 regression tests
  pin anti-behavior including silent-overwrite arm
@yuneng-berri
yuneng-berri merged commit d1fc398 into litellm_internal_staging May 1, 2026
108 of 115 checks passed
@yuneng-berri
yuneng-berri deleted the litellm_fix_ui_session_fixation_url_token branch May 1, 2026 23:06
fzowl pushed a commit to fzowl/litellm that referenced this pull request Jun 24, 2026
…fixation_url_token

fix(ui): remove insecure ?token= URL handler from LoginPage to close session-fixation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants