fix(auth): address security issues and bugs in authentication flow#70
Merged
Conversation
…loses #69) - Remove removeRefreshToken() from loginStart() to prevent losing the session when a network error occurs during token refresh; call it explicitly only at the start of a fresh login (not during refresh) - Swap token storage order in loginSuccessful(): persist the new refresh token before updating state so a page reload mid-swap never loses it - Guard store.refresh() in the interceptor with a loginLoading() check so concurrent 401 responses only trigger one refresh round-trip - Replace '|| store.expired()' with only status === 401 in the interceptor catchError; non-401 errors no longer spuriously kick off a token refresh when the client-side clock says the token is expired - Add per-IP fixed-window rate limiting (10 req/min) to all /api/account/* endpoints via AddRateLimiter to mitigate brute force - Configure BearerTokenOptions explicitly: access 1 h, refresh 7 d - Allow localhost origins in CORS during development - Remove hardcoded test access token and credentials from Api.http - Replace pre-filled test@test.com/SuperSecret1! defaults in the login form with empty strings - Add/update tests covering the corrected interceptor and store behaviour Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://green-water-08792290f-70.eastus2.2.azurestaticapps.net |
|
Note Coverage shown for affected projects only. Per-file thresholds (80%) are enforced by vitest.
|
chrisjwalk
approved these changes
Mar 16, 2026
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
2 participants
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 #69
What changed
Bug fixes
loginStart()was removing the refresh token prematurelyThe old code called
removeRefreshToken()insideloginStart(), which is invoked at the start of both fresh logins and silent refreshes. If a refresh request failed due to a network error, the refresh token was already gone and the user would be silently logged out on their next visit. NowremoveRefreshToken()is called only at the explicit start of a fresh login (inside thelogin()rxMethodtap), never during a background refresh.Token swap order in
loginSuccessful()The new refresh token is now persisted to
localStoragebefore the state is patched. This ensures a page reload that lands between the two operations always recovers the latest token.Concurrent 401 race condition in the HTTP interceptor
Multiple in-flight requests failing with 401 simultaneously would each call
store.refresh()independently, firing multiple refresh round-trips and causing unpredictable state transitions. The interceptor now checksstore.loginLoading()before callingstore.refresh(); subsequent 401 handlers just subscribe tologinStatus$and wait for the first refresh to complete.Spurious refresh triggered by non-401 errors
The old
catchErrorcondition waserror.status === 401 || store.expired(). A 400, 500, or any other error would trigger a refresh attempt whenever the client-side clock said the access token was expired. The condition is nowerror.status === 401only.Security improvements
Rate limiting on
/api/account/*Added
AddRateLimiterwith a per-IP fixed-window policy (10 requests per minute) applied to the entire/api/accountendpoint group. This covers login, register, and password-reset against brute force.Explicit bearer token lifetimes
BearerTokenOptionsis now configured explicitly: access token 1 h (unchanged from default), refresh token 7 d (down from the 14 d default). The shorter refresh window reduces the blast radius of a stolen token and makes the security posture auditable at a glance.localhostadded to CORS allowlist in developmentThe CORS policy now also permits
localhostorigins when running in theDevelopmentenvironment.Hardcoded credentials removed from
Api.httpPre-filled test credentials removed from the login form
defaultLoginRequestinlogin.store.tswas initialising the login form with the same test credentials, shipping them to every end user in the compiled bundle. Replaced with empty strings.Tests
should not trigger refresh for non-401 errors even when the token is expiredshould not start a duplicate refresh when one is already in progress