fix(client): implement refresh token handling for seamless session renewal #3102
Conversation
djclarkson
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Auth State Management client/src/Features/Auth/authSlice.js |
Add refreshToken to initial state and populate it from auth responses; reset it in clearAuthState; add and export setAuthToken reducer to update the access token. |
Network Token Refresh Logic client/src/Utils/NetworkService.js |
Add conditional x-refresh-token header, introduce isRefreshing flag and failedQueue, add processQueue helper, implement 403-driven refresh flow that calls /auth/refresh, dispatches setAuthToken, retries queued requests, and handles failures by clearing auth and navigating to login. |
Sequence Diagram
sequenceDiagram
actor Client
participant NS as NetworkService
participant API as Server (/auth/..., resources)
participant Redux
Client->>NS: Send API request
NS->>NS: Attach Authorization header
NS->>NS: Attach x-refresh-token for refresh endpoint case
NS->>API: Forward request
API-->>NS: 403 "refresh required"
alt isRefreshing = false
NS->>NS: isRefreshing = true
NS->>API: POST /auth/refresh (with refresh token)
API-->>NS: 200 { token: newToken } or error
alt refresh success
NS->>Redux: dispatch(setAuthToken(newToken))
NS->>NS: processQueue(resolve) - retry queued requests with newToken
NS->>API: Retry original request with newToken
API-->>NS: 200 Success
NS->>Client: Return response
else refresh failed
NS->>Redux: dispatch(clearAuthState)
NS->>Client: Reject / navigate to login
NS->>NS: processQueue(reject)
end
NS->>NS: isRefreshing = false
else isRefreshing = true
NS->>NS: enqueue original request in failedQueue
NS->>NS: await queue processing
NS->>API: Retry queued request after refresh
API-->>NS: 200 Success
NS->>Client: Return response
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
I nibble tokens, soft and spry, 🥕🐇
I stash the refresh beneath the sky,
When access wanes, I hop and fetch,
Queue the requests, no tangled mess,
Sessions stay warm — a cozy stretch.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the main change: implementing refresh token handling for seamless session renewal, which directly addresses the core objective. |
| Description check | ✅ Passed | The PR description comprehensively covers changes, includes the issue number (#3101), explains the refresh flow, and provides a test plan, though the required checklist items are not explicitly marked as complete. |
| Linked Issues check | ✅ Passed | The code changes fully implement all requirements from issue #3101: storing refreshToken, sending x-refresh-token headers, calling /auth/refresh on 403, retrying with new token, queueing concurrent requests, and handling refresh failures. |
| Out of Scope Changes check | ✅ Passed | All changes are directly aligned with issue #3101 requirements; no out-of-scope modifications are present in the authSlice or NetworkService files. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @client/src/Utils/NetworkService.js:
- Around line 29-36: The code is currently adding the refreshToken from
store.getState().auth into config.headers for every request; change this to only
add the "x-refresh-token" header when the request is for the refresh endpoint
(e.g., when config.url or config.url.endsWith('/auth/refresh') and optionally
config.method === 'post'). In practice, remove the unconditional
...(refreshToken && { "x-refresh-token": refreshToken }) spread and instead,
after building config.headers (with Authorization and Accept-Language using
currentLanguage and authToken), conditionally set
config.headers['x-refresh-token'] = refreshToken only when the request matches
the refresh endpoint; keep the rest of the header spread order as-is so other
headers in config.headers are preserved.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/Features/Auth/authSlice.jsclient/src/Utils/NetworkService.js
🔇 Additional comments (5)
client/src/Utils/NetworkService.js (2)
44-57: LGTM!The refresh state management with
isRefreshingflag and request queuing viafailedQueueis a solid pattern for preventing race conditions during token refresh.
114-122: LGTM!The 401 handler correctly clears auth state and redirects to login. The error is properly rejected at the end.
client/src/Features/Auth/authSlice.js (3)
5-12: LGTM!The
refreshTokenfield is correctly added to the initial state.
137-144: LGTM!The
refreshTokenis correctly extracted from the authentication response with a safe fallback.
191-201: LGTM!The
clearAuthStatecorrectly resets therefreshToken, andsetAuthTokenappropriately updates only the access token (since the refresh flow returns a new access token while the refresh token remains valid).
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR implements refresh token handling to prevent unnecessary logouts but introduces a critical security gap in the refresh mechanism and a breaking change in global error handling.
📄 Documentation Diagram
This diagram documents the new automatic token refresh workflow implemented to prevent unnecessary user logouts.
sequenceDiagram
participant U as User
participant C as Client (NetworkService)
participant S as Server
participant A as Auth Store
U->>C: Login Request
C->>S: POST /auth/login
S->>C: Return access_token and refresh_token
C->>A: Store tokens
Note over C: PR #35;3102 introduces automatic refresh on 403
U->>C: API Request
C->>S: Request with access_token
S->>C: 403 (Token Expired)
C->>S: POST /auth/refresh with x-refresh-token
S->>C: New access_token
C->>A: Update access_token
C->>S: Retry Original Request
S->>C: Success Response
C->>U: Return Data
🌟 Strengths
- Addresses the core issue of session expiration without user action.
- Implements intelligent request queuing to prevent duplicate refresh calls.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P0 | client/src/Utils/NetworkService.js | Security | Missing fallback for absent refresh token risks silent auth failures. | |
| P1 | client/src/Utils/NetworkService.js | Architecture | Race condition in queuing masks errors and causes leaks. | |
| P1 | client/src/Utils/NetworkService.js | Architecture | Global 401 handling conflicts with existing localized clearances. | path:client/src/Pages/v1/Account/components/ProfilePanel.jsx |
| P2 | client/src/Features/Auth/authSlice.js | Maintainability | Silent fallback to empty string hides server token bugs. | |
| P2 | client/src/Utils/NetworkService.js | Architecture | Refresh state not reset on logout, causing cross-session pollution. | path:client/src/Utils/NetworkServiceProvider.jsx |
🔍 Notable Themes
- Error Propagation Issues: Inconsistencies in how errors are handled and propagated during the refresh flow.
- Cross-Session State Management: Risks of state pollution across different user authentication sessions.
📈 Risk Diagram
This diagram illustrates the security risk when the refresh token is missing during the refresh process, leading to potential silent authentication failures.
sequenceDiagram
participant C as Client
participant S as Server
participant A as Auth Store
C->>S: API Request (expired token)
S->>C: 403 Error
C->>A: Get refresh_token
alt refresh_token present
C->>S: POST /auth/refresh
S->>C: New access_token
C->>A: Update token
C->>S: Retry Request
S->>C: Success
else refresh_token missing
Note over C,A: R1(P0): No fallback, leads to silent logout
C->>A: Clear auth state
C->>C: Redirect to login
end
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| // Call refresh endpoint | ||
| const response = await this.axiosInstance.post("/auth/refresh"); | ||
| const newToken = response.data.data.token; |
There was a problem hiding this comment.
P0 | Confidence: High
The refresh token is not being sent to the /auth/refresh endpoint. The POST /auth/refresh call uses the same axiosInstance, which automatically adds the x-refresh-token header via the request interceptor. However, this creates a circular dependency: the interceptor triggers on 403, and within the refresh logic, we make another request that will also go through the interceptor. While the code intends to reuse the stored refresh token, there is a critical security gap: if the refresh token itself is invalid/expired, this request will fail, but more importantly, the refresh endpoint expects the refresh token to be presented for validation. The current implementation correctly sends it via the header due to the interceptor, but there is a risk: if the refresh token is cleared from the state mid-process, this call will fail. More critically, the server's refresh endpoint likely requires the refresh token to be presented (often in the request body or a header like Authorization: Bearer <refresh_token>). The PR description states the server expects it in the header x-refresh-token, which is being set globally. However, the code does not explicitly show the token being passed to the refresh call, relying on the interceptor. This is acceptable but should be verified. The critical issue is the lack of a fallback if the refresh token is missing when the 403 occurs. The code will attempt the refresh call without a token, leading to a 401, which will then trigger logout. This is correct behavior but may be a user experience issue if the token is present but the interceptor fails to attach it.
Code Suggestion:
const { refreshToken } = store.getState().auth;
if (!refreshToken) {
// No refresh token available, cannot refresh
dispatch(clearAuthState());
navigate("/login");
return Promise.reject(error);
}
// Call refresh endpoint with the refresh token explicitly
const response = await this.axiosInstance.post("/auth/refresh", {}, {
headers: { "x-refresh-token": refreshToken }
});
const newToken = response.data.data.token;
client/src/Utils/NetworkService.js
Outdated
| const originalRequest = error.config; | ||
|
|
||
| // Handle 403 - server is telling us to request a new access token | ||
| if (error.response && error.response.status === 403 && !originalRequest._retry) { |
There was a problem hiding this comment.
P1 | Confidence: High
The queuing mechanism for concurrent requests during token refresh has a race condition and error handling flaw. The processQueue function is defined inside the constructor, creating a closure over this.failedQueue. When multiple requests fail with 403 simultaneously, the first will set this.isRefreshing = true and start the refresh. Subsequent requests will be queued. However, if the refresh fails (e.g., refresh token invalid), processQueue is called with an error, which rejects all queued promises. The problem: the original failing requests (which triggered the 403) will have their promises rejected with the refresh error, not the original 403 error. This masks the root cause and may lead to incorrect error handling in the calling code. The intended behavior is likely that if refresh fails, all queued requests should fail and trigger logout. However, the error propagated should be the original 403 or a dedicated session expiry error, not the internal refresh error. Additionally, there's a memory leak risk: the failedQueue is never cleared if the component unmounts or the service is recreated (though NetworkService is a singleton).
Impact: Components making API calls may receive a cryptic "Refresh failed" error instead of a meaningful "Session expired" message, breaking error handling logic.
Code Suggestion:
const processQueue = (error, token = null) => {
this.failedQueue.forEach((prom) => {
if (error) {
// Reject with a consistent session expiry error
prom.reject(new Error('Session expired. Please log in again.'));
} else {
prom.resolve(token);
}
});
this.failedQueue = [];
};| } | ||
| } | ||
|
|
||
| // Handle 401 - authentication failed (invalid/expired refresh token or no token) |
There was a problem hiding this comment.
P1 | Confidence: High
The global 401 handling now conflicts with existing, localized clearAuthState dispatches. The related_context shows clearAuthState() is dispatched in ProfilePanel.jsx (line 166) and NewPasswordConfirmed.jsx (line 19) for specific user actions (e.g., account deletion, password reset). With the new interceptor, any 401 response (including those from these actions) will trigger the global clearAuthState() and navigate to /login, potentially interfering with the intended UX flow. For example, after a password reset, the user might be immediately logged out and redirected, losing any success messages. This is a breaking change to the existing authentication error handling architecture.
Impact: Components that intentionally trigger 401s (like logout or post-deletion calls) will have their state management disrupted by the global interceptor, leading to double dispatches and unexpected navigation.
Code Suggestion:
// Skip global 401 handling for specific endpoints that intentionally clear auth
const intentionalAuthClearEndpoints = ['/auth/logout', '/user/delete']; // Example
if (error.response && error.response.status === 401 &&
!intentionalAuthClearEndpoints.some(ep => error.config.url.includes(ep))) {
dispatch(clearAuthState());
navigate("/login");
}Evidence: path:client/src/Pages/v1/Account/components/ProfilePanel.jsx, path:client/src/Pages/v1/Auth/NewPasswordConfirmed.jsx
| state.success = action.payload.success; | ||
| state.msg = action.payload.msg; | ||
| state.authToken = action.payload.data.token; | ||
| state.refreshToken = action.payload.data.refreshToken || ""; |
There was a problem hiding this comment.
P2 | Confidence: High
The fallback to empty string (|| "") assumes the refreshToken might be null or undefined in the response. This is defensive but could hide a server-side bug where the refreshToken is missing. If the server is supposed to always return a refreshToken on successful login/register, a missing token should be treated as an error. Silently defaulting to an empty string will cause the refresh mechanism to fail later (when the token is needed), leading to a logout. This is a data consistency issue.
Impact: Users might experience unexpected logouts if the server has a regression and stops sending refreshToken. The client will not surface a clear error.
Code Suggestion:
const refreshToken = action.payload.data.refreshToken;
if (!refreshToken) {
console.error('Server did not return a refresh token. Authentication may be impaired.');
// Optionally dispatch an error action
}
state.refreshToken = refreshToken;| // Track if we're currently refreshing to prevent multiple refresh calls | ||
| this.isRefreshing = false; | ||
| this.failedQueue = []; |
There was a problem hiding this comment.
P2 | Confidence: Medium
The NetworkService now has internal mutable state (isRefreshing, failedQueue) that is not reset when the user logs out. The related_context shows NetworkService is instantiated once in NetworkServiceProvider.jsx and likely persists for the entire application lifetime. When a user logs out (clearAuthState), these flags are not reset. If a new user logs in, the isRefreshing flag could still be true from the previous session, blocking all subsequent refresh attempts for the new user. This is a state pollution issue across authentication sessions.
Impact: A user logs out, another user logs in, and the new user's requests that trigger a 403 might be incorrectly queued or skipped because the service thinks a refresh is already in progress.
Code Suggestion:
// In the NetworkService constructor, subscribe to store changes
this.unsubscribeAuth = store.subscribe(() => {
const { authToken } = store.getState().auth;
if (!authToken && this.isRefreshing) {
// Auth token cleared while refreshing? Reset state.
this.isRefreshing = false;
this.failedQueue = [];
}
});
// Remember to call this.unsubscribeAuth() in a cleanup method if needed.Evidence: path:client/src/Utils/NetworkServiceProvider.jsx
…newal The server already supports refresh tokens but the client was not using them. This caused users to be logged out after 2 hours (default TOKEN_TTL) even though the refresh token is valid for 7 days. Changes: - authSlice.js: Store refreshToken from login/register responses - authSlice.js: Add setAuthToken action for updating token after refresh - authSlice.js: Clear refreshToken on logout - NetworkService.js: Send x-refresh-token header with all requests - NetworkService.js: On 403 response, call /auth/refresh to get new token - NetworkService.js: Retry failed requests with new token - NetworkService.js: Queue concurrent requests during refresh to prevent races The refresh flow: 1. Access token expires, server returns 403 "Request new access token" 2. Client calls POST /auth/refresh with refresh token in header 3. Server returns new access token 4. Client updates store and retries original request 5. If refresh fails (expired/invalid), redirect to login
- Only send x-refresh-token header to /auth/refresh endpoint (security) - Skip refresh logic for refresh endpoint to prevent circular dependency - Check if refresh token exists before attempting refresh - Redirect to login if no refresh token available
djclarkson
force-pushed
the
fix/client-refresh-token
branch
from
fe6e1f3 to
05d0e95
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @client/src/Utils/NetworkService.js:
- Around line 108-121: Implement a server-side POST /auth/refresh handler in
your auth routes (e.g., add a controller and route in
server/src/routes/v1/authRoute.js) that reads the refresh token from request
headers, validates it, issues a new access token, and responds with JSON { data:
{ token: "<newToken>" } } on success; ensure it returns proper 401/400 on
invalid/expired refresh tokens and that any client-side logic using
axiosInstance.post("/auth/refresh"), dispatch(setAuthToken), processQueue, and
originalRequest can receive the new token so isRefreshing can be cleared and
queued requests retried.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/Features/Auth/authSlice.jsclient/src/Utils/NetworkService.js
🔇 Additional comments (5)
client/src/Features/Auth/authSlice.js (2)
5-12: LGTM!The
refreshTokenis correctly added to the initial state alongside the existingauthToken, enabling the client to persist and manage the refresh token through Redux.
191-201: LGTM!The
clearAuthStatereducer correctly resetsrefreshTokenon logout, and the newsetAuthTokenaction provides a clean way for the NetworkService to update the access token after a successful refresh.client/src/Utils/NetworkService.js (3)
37-40: LGTM!The refresh token is now correctly scoped to only the
/auth/refreshendpoint, addressing the security concern of exposing the long-lived token on every request.
76-91: LGTM!The guard conditions correctly:
- Exclude the refresh endpoint from the 403 retry logic to prevent circular dependency
- Check for the presence of a refresh token before attempting refresh
- Clear auth and redirect when no refresh token is available
93-103: LGTM!The queuing mechanism correctly defers concurrent requests while a refresh is in progress, then retries them with the new token once refresh completes.
| try { | ||
| // Call refresh endpoint | ||
| const response = await this.axiosInstance.post("/auth/refresh"); | ||
| const newToken = response.data.data.token; | ||
|
|
||
| // Update the auth token in store | ||
| dispatch(setAuthToken(newToken)); | ||
|
|
||
| // Process queued requests | ||
| processQueue(null, newToken); | ||
|
|
||
| // Retry original request with new token | ||
| originalRequest.headers["Authorization"] = `Bearer ${newToken}`; | ||
| return this.axiosInstance(originalRequest); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f \( -name "*.js" -o -name "*.ts" -o -name "*.py" \) ! -path "*/client/*" ! -path "*/node_modules/*" ! -path "*/.git/*" | head -20Repository: bluewave-labs/Checkmate
Length of output: 876
🏁 Script executed:
rg -n "auth.*refresh|refresh.*token|/auth/refresh" --type=js --type=ts -g '!client/**' -g '!node_modules/**' -A 10 -B 2Repository: bluewave-labs/Checkmate
Length of output: 49
🏁 Script executed:
cat server/src/controllers/v1/authController.jsRepository: bluewave-labs/Checkmate
Length of output: 15859
🏁 Script executed:
rg -n "refresh" server/src/controllers/v1/authController.js -A 15 -B 2Repository: bluewave-labs/Checkmate
Length of output: 49
🏁 Script executed:
rg -n "refresh" server/src/ -A 10 -B 2 --type=js --type=tsRepository: bluewave-labs/Checkmate
Length of output: 49
🏁 Script executed:
cat server/src/config/routes.jsRepository: bluewave-labs/Checkmate
Length of output: 2702
🏁 Script executed:
rg -n "token|auth" server/src/config/routes.js -A 3 -B 1Repository: bluewave-labs/Checkmate
Length of output: 1018
🏁 Script executed:
cat server/src/routes/v1/authRoute.jsRepository: bluewave-labs/Checkmate
Length of output: 1616
🏁 Script executed:
rg -n "loginUser" server/src/services/ -A 20 -B 2 --type=js --type=tsRepository: bluewave-labs/Checkmate
Length of output: 127
🏁 Script executed:
cat server/src/services/userService.js | head -200Repository: bluewave-labs/Checkmate
Length of output: 133
🏁 Script executed:
find server/src -type f -name "*Service*" -o -name "*service*" | head -20Repository: bluewave-labs/Checkmate
Length of output: 988
🏁 Script executed:
ls -la server/src/Repository: bluewave-labs/Checkmate
Length of output: 997
🏁 Script executed:
rg -n "loginUser" server/src/service/v1/business/userService.js -A 30 -B 2Repository: bluewave-labs/Checkmate
Length of output: 1369
🏁 Script executed:
cat server/src/service/v1/business/userService.js | head -250Repository: bluewave-labs/Checkmate
Length of output: 6965
Critical issue: The /auth/refresh endpoint does not exist on the server.
The client code at lines 110-121 attempts to call POST /auth/refresh to refresh the access token, but this endpoint is not defined in the server routes (server/src/routes/v1/authRoute.js). The auth routes only include: register, login, recovery/*, and user management endpoints. This will cause the token refresh flow to fail with a 404 error, preventing users from refreshing expired tokens and getting locked out of the application. The isRefreshing flag will remain stuck in a true state, blocking all queued requests indefinitely.
This endpoint needs to be implemented on the server side to handle token refresh using the refresh token from the request headers.
🤖 Prompt for AI Agents
In @client/src/Utils/NetworkService.js around lines 108 - 121, Implement a
server-side POST /auth/refresh handler in your auth routes (e.g., add a
controller and route in server/src/routes/v1/authRoute.js) that reads the
refresh token from request headers, validates it, issues a new access token, and
responds with JSON { data: { token: "<newToken>" } } on success; ensure it
returns proper 401/400 on invalid/expired refresh tokens and that any
client-side logic using axiosInstance.post("/auth/refresh"),
dispatch(setAuthToken), processQueue, and originalRequest can receive the new
token so isRefreshing can be cleared and queued requests retried.
1 participant
Summary
Fixes #3101
The server already supports refresh tokens but the client was not using them. This caused users to be logged out after 2 hours (default TOKEN_TTL) even though the refresh token is valid for 7 days.
Changes
client/src/Features/Auth/authSlice.jsrefreshTokenfrom login/register responsessetAuthTokenaction for updating token after refreshrefreshTokenon logoutclient/src/Utils/NetworkService.jsx-refresh-tokenheader with all requests/auth/refreshto get new tokenRefresh Flow
POST /auth/refreshwith refresh token in headerTest Plan
tokenandrefreshTokenare stored in ReduxTOKEN_TTL)Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.