fix(e2e): make ui login readiness robust to httpOnly token cookies - #33564
Conversation
The login readiness check waited only on document.cookie including token=, which is empty when the token cookie is httpOnly. If the server ever sets it via a Set-Cookie header, the wait would spin to the 30s timeout and silently reproduce the original hang. Also accept the login form detaching (#username gone after the post-login redirect) so readiness holds regardless of how the cookie is delivered. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Mubashir Osmani seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Greptile SummaryThis PR hardens the
Confidence Score: 4/5Safe to merge; the change is test-only, isolated to the login fixture, and corrects a real gap in the readiness signal. The fix correctly addresses the httpOnly cookie blind spot. The one open question is whether the tests/e2e/management/conftest.py — the
|
| Filename | Overview |
|---|---|
| tests/e2e/management/conftest.py | Login readiness condition widened from cookie-only to cookie-or-form-detach; no post-login state assertion guards against false positives on error pages. |
Reviews (1): Last reviewed commit: "fix(e2e): make ui login readiness robust..." | Re-trigger Greptile
| page.wait_for_function( | ||
| "() => document.cookie.includes('token=') || !document.querySelector('#username')" | ||
| ) |
There was a problem hiding this comment.
No post-login assertion after
#username disappears
The !document.querySelector('#username') branch resolves the wait as soon as the login form unmounts — but it does not verify that the resulting page is the dashboard rather than an error page (e.g., a 500 or a redirect back to /ui/login with a different form structure). If a misconfigured or unreachable backend causes the login to fail with a page that happens not to contain #username, every downstream test in the session will receive a page in a broken state and produce confusing assertion failures instead of a clear "fixture login failed" diagnostic. A lightweight guard after the wait_for_function — checking that the final URL contains /ui/ or that a known dashboard selector is present — would make the failure mode much more actionable.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
* chore(e2e): establish litellm_e2e_staging integration line Long-lived berri branch for e2e suite recovery work (LIT-4479 through LIT-4486) before merge to litellm_internal_staging * test(e2e): remove langfuse_otel logging e2e suite (#33558) * test(e2e): remove langfuse_otel logging e2e suite Removes the LIT-4483 dynamic per-team/key/org langfuse_otel logging e2e tests (tests/e2e/logging/test_langfuse_e2e.py, added in #32857). The shared logging_client harness and the langfuse coverage-registry cells are left in place; only the test module is removed. The otel and prometheus logging e2e suites are unaffected. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(e2e): drop orphaned langfuse coverage-registry cells The three logging.langfuse.*.logs_spend P0 cells were only exercised by the deleted langfuse_otel e2e suite. Remove them so the coverage registry has no orphaned rows. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: yucheng <yucheng@berri.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(e2e): log into the react admin ui in the management browser fixture (#33562) The management ui_page fixture drove the old server-rendered login form: it clicked input[type="submit"] and treated wait_for_url("**/ui/**") as the done signal. /ui/ now serves the react (antd) dashboard whose submit is a <button type="submit">, so the click waited out the full 30s timeout and errored every browser test in the suite. wait_for_url also matched instantly because the login page already lives at /ui/, so on the fast path the fixture navigated before the auth cookie landed and got bounced back to login. Click the antd submit button and wait for the token cookie loginCall sets on document.cookie, the real post-login signal. Co-authored-by: Mubashir Osmani <mubashir@berri.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(e2e): make ui login readiness robust to httpOnly token cookies (#33564) The login readiness check waited only on document.cookie including token=, which is empty when the token cookie is httpOnly. If the server ever sets it via a Set-Cookie header, the wait would spin to the 30s timeout and silently reproduce the original hang. Also accept the login form detaching (#username gone after the post-login redirect) so readiness holds regardless of how the cookie is delivered. Co-authored-by: Mubashir Osmani <mubashir@berri.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: yucheng <yucheng@berri.ai> Co-authored-by: Mubashir Osmani <mubashir@berri.ai>
Relevant issues
Linear ticket
Resolves LIT-4484
Pre-Submission checklist
Screenshots / Proof of Fix
Follow-up hardening on top of #33562 (already merged into
litellm_e2e_staging). Repro against the compose stack intests/e2e/withghcr.io/berriai/litellm:main-latest; the four browser tests need no LLM trafficRan the file 3 times back to back at this branch's HEAD, 4 passed every time:
Type
🐛 Bug Fix
✅ Test
Changes
#33562 fixed the
ui_pagelogin for the react admin UI and used thetokencookie ondocument.cookieas the login-complete signal. That signal only holds whileloginCallsets the cookie from JavaScript;document.cookiereturns empty for anhttpOnlycookie, so if the server ever switches to deliveringtokenvia aSet-Cookieheader thewait_for_functionwould spin to the 30s timeout and silently reproduce the original hang with a different causeThis accepts a second, delivery-agnostic readiness signal: the login form detaching (
#usernamegone once the post-login redirect lands). Readiness now holds whether the cookie is JS-set orhttpOnlyQA runbook
All four cases share the
ui_pagefixture; this PR only changes how the fixture decides login is complete, so the manual check is the login plus each test's dropdown assertion. Prereqs: compose stack up fromtests/e2e/on an image whose bundled UI has #32382;uv sync --inexact --group e2e-dev && uv run playwright install chromiumall-proxy-modelsmodels == ["all-proxy-models"]with no teamall-team-models["all-proxy-models", "gpt-5.5"], open the create modal and select that teamall-proxy-modelspresentmodels == ["all-team-models"]on that team["gpt-5.5"], open it on the api-keys page, Settings tab, Edit Settings["all-proxy-models", "gpt-5.5"], open it, Settings tab, Edit Settingsall-proxy-modelspresentFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/5d5f93d9ba36469b910db0271edb31f2