fix: clear invalid session cookie on login render#2454
Merged
Conversation
When a user visits /login with a stale or rotated-secret cookie the useUserLocals query errors. Previously the stale cookie stayed in the browser, causing the server-side checkUser to redirect back to /upload on the next hard navigation (seeing a cookie → assumes logged in). LoginPage now removes the token cookie in a useEffect whenever the userLocals fetch errors and a cookie is present. This breaks the loop regardless of why the cookie is invalid (expiry, SECRET rotation, etc.). Fixes #2352. Part of wave 2/4 of the first-deck-success activation wave (umbrella PR #2446). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Deploy Preview for notion2anki ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
The LoginPage.tsx fix (removeCookie on isError) required useUserLocals to surface a genuine error state. The api.ts get() function was returning undefined on 401 (after a no-op redirectToLogin on /login), which kept isError false and prevented cookie clearance. Fix api.ts to throw on 401 so TanStack Query sets isError: true after retries exhaust. Extend mock-server to return 401 when token=stale-jwt-token. Add Playwright test that sets the stale cookie, navigates to /login, and asserts the form renders and the cookie is cleared. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
When a user visits
/loginwith a stale or invalid session cookie,LoginPagenow clears that cookie on render. Previously the stale cookie persisted in the browser, causing the server-sidecheckUserto see a cookie and redirect the user back to/uploadon the next hard navigation — creating a loop.Why
Fixes #2352. Part of wave 2 of 4 in the first-deck-success activation wave (umbrella PR #2446).
The root cause:
checkUserinUsersController.tscallsgetUserFrom(req.cookies.token). With a valid token it redirects to/upload(or wherever). With no cookie it servesindex.html(the login form). With a stale cookie (expired JWT, rotated SECRET),getUserFromthrows and Express 5 returns a 500. The stale cookie persists across browser sessions until it expires or the user manually clears it — but during that window the user can land in a redirect loop.The fix is symptom-level and intentional per the spec: it prevents the loop regardless of why the cookie is invalid. Root causes (SECRET rotation, OAuth client rotation) remain separate issues.
How
LoginPage.tsxadds auseEffectthat watches thetokencookie and theisErrorflag fromuseUserLocals. When both are true (cookie present, fetch errored), it removes the cookie viauseCookies. The fix does not affect the login flow, theuseHandleLoginSubmithook, or server-side code.The cookie was set without
HttpOnly(plainres.cookie('token', token)inUsersControllers.ts), so it is readable and deletable from JavaScript.Measuring success
After this ships: a user with a stale cookie who navigates to
/loginwill see the login form on first render without a redirect loop. Measurable via Sentry (no more 500 cascade on/loginfrom stale cookies) and by direct testing with an expired JWT.Testing
web/src/pages/LoginPage/LoginPage.test.tsx— 4 tests covering:isErrorfromuseUserLocals→ cookie removedisError→ no change (nothing to remove)Changelog
Deferred to the wave's final PR per the first-deck-success spec (umbrella PR #2446). A single combined entry will land there.
Risks
useEffectfires on every render where cookie+isError are true, but sinceremoveCookieis stable and React deduplicates effects, this is safe.useUserLocalsusesretry: 3— the effect fires after the third failure settles (isErrorbecomes true), not on each retry attempt.checkUserorgetUserFrombehavior. The root loop causes (SECRET rotation, etc.) are tracked separately.Goal alignment
Removes a friction point that makes the site look broken to returning users with stale cookies — directly addresses onboarding/re-activation conversion. Moves toward the 300K-user goal by not losing users at the login door.
Sonar
sonar-scannernot run locally —SONAR_TOKENnot configured in this environment. Changes are small (1useEffect, 1 test file). No new HTTP calls, no user-input reflection, no zip extraction, no taint paths.Need help on this PR? Tag
@codesmithwith what you need.