fix(auth): secure refresh-token handling and rebuild client refresh logic - #92
Merged
Conversation
Logout previously required a valid access token, so an expired session could not log out (the exact moment logout matters). Reuse the validateRefreshToken middleware, which already populates req.user for the controller to revoke the stored token.
The refresh endpoint set an httpOnly accessToken cookie that no client ever reads; the access token already travels in the response body.
SameSite=None + Secure is only valid for cross-site HTTPS deployments; in dev the client and API are same-site over plain http, where Secure cookies get dropped and None requires TLS. Use Lax/insecure outside production, None/Secure in production.
Add a response interceptor that refreshes the access token once and replays the failed request. Concurrent 401s share one in-flight refresh; a request whose token was already rotated by a concurrent request replays directly instead of triggering a second round-trip. When the refresh itself fails, the stored token is cleared and listeners are notified so the auth provider can reset state. Also add a node-env vitest unit project so shared client lib code has a test runner (the existing vitest setup only ran Storybook tests).
Bootstrap now restores the session via the shared single-flight refresh and simply marks the user signed out when it fails — it no longer calls the authenticated logout endpoint (which always 401'd and threw an unhandled rejection). Remove the wasLoggedOut localStorage flag that kept logging users out after a logout->login cycle, swallow logout errors when the session is already dead server-side, and reset auth state when the interceptor reports an unrecoverable session expiry. Add e2e coverage for logout->re-login->reload session restore and for not calling logout during an unauthenticated bootstrap; serialize auth specs since real logins overwrite the user's single stored refresh token.
clearCookie must mirror the cookie's Secure/SameSite/Path/HttpOnly attributes, otherwise browsers retain the prod SameSite=None; Secure cookie after logout and subsequent refresh still succeeds.
Verifies that logout clears both refreshToken and legacy accessToken cookies with the same Path/HttpOnly/SameSite attributes used on set (otherwise prod SameSite=None; Secure cookies survive logout and refresh still succeeds), and that replaying the old cookie after logout is rejected. Would have failed before 4f8cdf5.
Concurrent 401s sharing one refreshPromise notified listeners per waiter (3×) and could replay with Bearer null after clearAccessToken. Move clear+notify into refreshPromise rejection (once) and guard the current !== _tokenUsed branch to throw when current is null instead of asserting non-null.
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.
Closes #22
Summary
The refresh token was stored as an
httpOnlycookie withSameSite=None; Secureunconditionally, which browsers drop in non-TLS dev environments, and the client's refresh logic was broken in several ways. This PR keeps the httpOnly-cookie transport (best practice, XSS-safe) per the issue discussion and fixes the surrounding behaviour.Server
validateRefreshTokenmiddleware already populatesreq.user).accessTokencookie set on refresh; nothing ever read it.SameSite=None; Secureonly in production (cross-site HTTPS),LaxwithoutSecurein dev/test where client and API are same-site over plain HTTP.Client
AuthProvider): restores the session through the same shared refresh flight and marks the user signed out on failure — it no longer calls the authenticated/auth/logoutendpoint on boot (guaranteed 401 + unhandled rejection).wasLoggedOutlocalStorage hack, which permanently logged users out after any logout → re-login → reload cycle.Tests
--project unit, 5 tests) covering refresh-once-and-replay, single-flight under concurrency, failure notification, and no-refresh-when-signed-out.Notes
e2e/collaboration.spec.ts:103fails on unmodifieddevelopwhenE2E_API_URLisn't set to the local server port (it defaults to the CI port 5001); unrelated to this change.accessTokencookie;logoutcontinues clearing it for migration safety.