Fix/token refresh loop - #275
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved refresh/logout race, interceptor queue, retry backoff, and session-expiration handling issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves frontend and backend token renewal, throttling, clock-drift handling, retry behavior, and localized session-expiration messaging.
Changes:
- Unifies refresh-endpoint throttling and adds security tests.
- Tracks token lifetime and acquisition time for resilient renewal.
- Updates refresh backoff, interceptor coordination, and expiration translations.
File summaries
| File | Reviewed changes |
|---|---|
project/urls.py |
Routes body refreshes through the custom throttled view. |
project/settings.py |
Configures refresh throttling and exposes Retry-After. |
ngen/views/auth.py |
Applies the shared refresh throttle scope. |
ngen/tests/api/test_login_security.py |
Tests refresh throttling behavior. |
frontend/src/views/auth/sso/SsoCallback.jsx |
Uses shared session payload creation. |
frontend/src/store/accountReducer.js |
Persists token lifetime metadata. |
frontend/src/hooks/useTokenManager.jsx |
Adds renewal timing and backoff logic. |
frontend/src/api/setupInterceptors.jsx |
Coordinates refresh subscribers and failures. |
frontend/src/api/services/auth.jsx |
Centralizes session metadata and expiration helpers. |
frontend/public/locales/es/translation.json |
Adds Spanish expiration messaging. |
frontend/public/locales/en/translation.json |
Adds English expiration messaging. |
Review details
Suppressed comments (3)
frontend/src/api/setupInterceptors.jsx:79
- The queue is drained before
isRefreshingis reset in the laterfinallyreaction. Another 401 response handled between these promise callbacks can seeisRefreshing === true, subscribe aftertakeSubscribers()has emptied the queue, and remain pending forever because this refresh has already completed. Reset the flag atomically with draining the subscribers, or otherwise handle subscribers that arrive during completion.
.catch((refreshError) => {
onRefreshFailed(refreshError);
frontend/src/hooks/useTokenManager.jsx:61
- This branch logs out solely from
Date.now() - obtainedAt, so a forward browser-clock adjustment or a transient outage lasting beyond the configured lifetime can discard a still-valid session without consulting the refresh endpoint. That contradicts the newtoken_not_valid-only expiration policy; remove this client-side logout and let the refresh response determine whether the session has ended.
if (refreshLifetime.current && age > refreshLifetime.current) {
logout(true);
frontend/src/hooks/useTokenManager.jsx:82
- When the activity-driven refresh detects an invalid/expired refresh token, this branch logs the user out silently. The new
ngen.auth.session_expiredmessage is only shown by the interceptor path, so a session that expires while the user is active but before another API call redirects to login without feedback. Show the same translated expiration alert before logging out here.
if (isSessionExpired(error)) {
logout(true);
return;
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The login and the refresh shared the scope "login", 20/min, so every browser of the deployment spent the budget meant to make guessing a password cost something. And it can only be spent per address: simplejwt leaves TokenViewBase with authentication_classes = (), so the bucket is never keyed by user and a whole organization behind one nat shares it. Renewing is not a place to guess anything, it asks for the refresh cookie of a session that already exists, so it gets a scope of its own, configurable with NGEN_THROTTLE_TOKEN_REFRESH and higher than the login. It keeps a ceiling because the endpoint answers without credentials and costs a signature check and a write. The refresh that takes the token in the body had no limit at all, and now it is counted the same. Retry-After is exposed through cors: a browser hides every answer header that is not on its short list, so with the api on another origin the frontend could not read how long it was asked to wait and had to guess.
…he server iat and exp are written with the clock of the server and were compared against Date.now(), the clock of the browser. When the two disagree by more than a quarter of the life of a token, every token is born already due, so a renewal was asked for on every mouse move: one request per second, the api refusing after twenty, and the session thrown away. With an access token of five minutes it took seventy five seconds of drift to start, thirty seconds to end up on the login screen, and no message. Only a duration survives the trip between two clocks, so the lifetime is read from what the server signed and the age from the moment the token arrived here, measured with the clock that is asking. sessionPayload stamps both and every place that stores a session goes through it, the login, the renewal and the sso callback, so none of them can forget it. Renewing now happens once three quarters of the life of the token is gone, which is what the name of the window said it meant and what leaves a margin, instead of once a quarter of it was gone. How often a renewal can be asked for at all is decided in the service and not in each caller, because the retry of a request that was answered that its token is not valid asks for the same thing and the two have to wait together. No renewal starts less than fifteen seconds after the last one, below the shortest interval any deployment can ask for, and after a failure it waits what the api asked for but never under that floor: Retry-After is what is left of the window over the requests that fit in it, so it is one second often enough to become a flood of its own. An answer that arrives after the user logged out is dropped instead of stored. It was verified that clicking logout while a renewal was travelling brought the session back up, logged in and with a live token. A failure that is not the session ending no longer ends the session: only the api answering that the refresh token is not valid anymore does. Being refused for asking too often, a backend that is restarting or a network that dropped are moments that pass. The session still ends once nothing is left to renew it with, and either way it says so instead of leaving the user on the login screen with no explanation. refreshToken asks for a token and nothing else: it used to log out on its own and the caller logged out again, so every failure closed the session twice over.
… waits The interceptor answered a 401 with token_not_valid by renewing the session, and left out only the urls with "refresh" in them. The logout also asks for a token, so with the access token already gone it answered 401 with that same code, which asked for a renewal, which failed and logged out again: one action of the user turned into three renewals and four logouts. Every endpoint that hands out, renews or closes a session is left out now. When the renewal failed the list of waiting requests was emptied without answering any of them, so each one stayed pending forever and the screens behind them loading forever. They are rejected now. The failure follows the same rule as the renewal on activity: only the api saying that the refresh token is gone closes the session, and closing it goes through logout() so the token is given back to the api and the url is kept for after the next login, instead of dispatching LOGOUT on its own. Its message was the only string in the file written in spanish by hand.
mateodurante
force-pushed
the
fix/token_refresh_loop
branch
from
September 15, 2026 16:12
2e90558 to
e913026
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Frontend refresh concurrency and session-termination handling still have unresolved issues.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
frontend/src/api/services/auth.jsx:168
- The generation guard is advanced only in
_doLogout, which runs fromlogout(...).finally()after the network request. A refresh already in flight can therefore resolve and dispatch a new token after logout has been requested and before local state is cleared, so this guard does not prevent the resurrection it is meant to block. Invalidate the generation synchronously when logout starts, while keeping local cleanup infinally.
// Whatever renewal is on its way belongs to the session that is ending here
sessionGeneration += 1;
frontend/src/api/setupInterceptors.jsx:89
- This logout is also asynchronous, while
isRefreshingis reset infinallyimmediately after the failed refresh. Other requests that receive 401s beforelogout(true)finishes can therefore start another refresh against the already-invalid cookie and schedule another logout. Set a shared session-ending flag (or clear the session synchronously) before allowing any further refresh attempts.
if (isSessionExpired(refreshError)) {
setAlert(i18next.t("ngen.auth.session_expired"), "error");
logout(true);
}
frontend/src/api/setupInterceptors.jsx:39
isSessionEndpointomits the body-token refresh route (api/token/refresh/, registered astoken-refresh). A 401 withcode: token_not_validfrom that endpoint therefore enters this branch, triggers the cookie refresh, and can log out a client that has a valid body refresh token but no cookie. Keep the body and cookie refresh URLs excluded from the interceptor; the previousurl.includes("refresh")check covered both.
const isSessionEndpoint = (url = "") =>
[COMPONENT_URL.login, COMPONENT_URL.logout, COMPONENT_URL.refreshCookieToken].some((path) => url.includes(path));
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
…velling Coming back to a tab wakes the renewal on activity and the requests of the page at the same moment, so with a token that is already stale both ask for a new one at once. The second one found the wait that the first had just set and was answered that it has to wait, which for the retry of a request means rejecting it: the screens showed errors even though the renewal the other caller started was about to work. They share the answer that is already on its way now, and the wait only applies when there is nothing travelling. A session that is over can be found out by both of them at the same time too, and closing it takes a request, so until it answers the token is still in the store. Each one said it and closed it on its own, and the activity of the user could start yet another one in between. There is one way to end a session now, it says so once, and it is over from the moment it is called and not from the moment the api answers. The interceptor lets go of its own state before telling anyone: whoever subscribes from there on subscribes to the next renewal and not to a list that nobody is going to read. It took a network answer inside a microtask for that to bite, so it was never reachable, but the order it is written in now is the one that does not need the argument.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Refresh attempts are not suppressed while session closure is pending, allowing the renewal loop to recur.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
Closing a session takes a request, and until the api answers the token is still in the store. The wait between renewals is fifteen seconds and the answer that ends a session on purpose does not make it longer, so a logout that took longer than that left the activity of the user asking for a new token once every wait, for a session that was already on its way out. Measured with a logout held for forty seconds: three renewals during it, one now. The mark is set by logout() and not by the one caller that found out the session was over, because the button of the interface and the timer of inactivity leave the same token in the store while their own request travels.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Refresh/logout races can retry requests or restore token state after logout begins.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/src/api/setupInterceptors.jsx:71
- A request sent with the old access token can return this 401 after another request has already completed renewal. At that point
isRefreshingis false, butrefreshToken()rejects withrenewalPostponedbecause of the 15-second minimum interval, so this otherwise retryable request fails. Detect that the store now contains a newer token and retry with it, while ensuring the retry is still tied to the same login/session generation.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
The mark that tells a renewal on its way that it belongs to a session that is gone was set when the api answered the logout, not when the logout was asked for. A renewal started before the click and answered during that window passed the check, so the token it brought was stored, the browser was logged in again and whatever was waiting for that token was sent. It is the likely order of the two, because the renewal left first: measured with a logout held for ten seconds, the stored token changed. Verified both ways now, with the renewal answering before the logout and after it: the token that comes back is dropped and the session stays closed.
…new one A request that left with a token that was replaced while it was travelling comes back answered that its token is not valid. There is nothing to renew there, the session already has a newer one, but this asked for a renewal anyway and the wait between renewals refused it, so a request that only had to go again failed and its screen showed an error. It goes again with the token that is in the store, which is what it was missing.
…appen What this branch fixes are races, and a build does not catch a race: the four of them were found by hand, driving a browser with its clock moved and answers held back, which is not something anyone can run again. The libraries to write a test were already in package.json, with no runner to use them and not a single test in the repo. This adds vitest and jsdom, the script, and thirteen cases over the part of a session that depends on timing: two callers sharing the renewal that is travelling, the wait between renewals and the floor under what the api asks for, a token that comes back after the logout was asked for, nothing renewing while a logout travels, a session closing once however many callers find out, and telling a session that is over from a moment that passes. They run in a second, with the clock in hand and no browser. Undoing any of the four fixes makes exactly one of them fail, which is what they are worth. CI runs them next to the build it already runs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A late unauthorized response can refresh and recreate a session after logout has completed.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (1)
- frontend/package-lock.json: Generated file
- Files reviewed: 15/16 changed files
- Comments generated: 1
- Review effort level: Balanced
The mark that says a logout is in progress is dropped once the api answers it, and from there on nothing told a store with no session apart from one with a live session. A request that was travelling can come back answered that its token is not valid after that, and the refresh cookie outlives the logout, so the renewal it would set off hands a token back and stands a session up that nobody is in: logged in, with no user behind it. It could not be produced from the interface -the requests that follow a logout came back answered- but the cookie really does stay valid, so the only thing in the way was that no request happened to come back at the wrong moment. A store with no session has nothing to renew, which is the same shape as the rest of the guards around it.
…circulation Logging out revoked nothing. The cookie was written with the path of the endpoint that renews it, so the browser never sent it to the one that closes the session, and simplejwt mints a brand new token when it is handed none instead of failing: the view blacklisted that one, which belongs to nobody, and answered 205. Following the jti of the user through it: after a logout the token was not blacklisted and its cookie still answered 200 for the rest of its hour. The count of blacklisted tokens went up on every logout, which is what made it look like it worked. The cookie now reaches the endpoints of the session and no further, the view refuses to work with an empty value, and the answer takes the cookie out of the browser, which only happens when the path is named. It also stops asking for an access token. A browser that was left alone has one that expired minutes ago, and that is exactly when somebody closes the session: it answered 401 before reaching the view, so the refresh token stayed alive. The cookie is the proof of which session is being closed, the same way it is when it asks for a new access token, and a logout that finds nothing left to revoke is not an error. Verified in a browser: the cookie travels to the logout, the session answers 205 with the access token already expired, the cookie is gone afterwards and the refresh token answers 401 from outside.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Legacy cookie migration and refresh/logout races can leave valid refresh tokens active after logout.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (1)
- frontend/package-lock.json: Generated file
- Files reviewed: 15/16 changed files
- Comments generated: 2
- Review effort level: Balanced
Renewing rotates the cookie, and the browser writes the one that comes back whether anything is listening or not. Closing while a renewal was travelling sent the token that was being replaced, so the api revoked that one and the replacement stayed behind: valid, in a browser nobody is logged into. The generation counter keeps the new access token out of the store, but it cannot keep the cookie out of the browser. The logout waits for the renewal to settle first, so what it presents is the token that is going to be there afterwards. It waits for the answer, never for its success, and never longer than three seconds: a session that does not close because something else never answered is worse than closing it with the token at hand. This is the one tab. Another tab renewing at the same moment rotates the cookie under this one, and that needs the renewal to be taken once per browser instead of once per tab, which is a change of its own.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Logout currently has a late-refresh race and lacks CSRF protection for its cookie-based operation.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (1)
- frontend/package-lock.json: Generated file
- Files reviewed: 15/16 changed files
- Comments generated: 2
- Review effort level: Balanced
Waiting for it meant picking a number, and an answer slower than that number landed after the session was revoked: rotation hands back a new cookie and the browser writes it whether anything is listening or not, so what was left behind was a valid token in a browser nobody is logged into. The request is aborted instead, which keeps the browser from ever reading that answer, and the logout goes out right away with the token that is there. The three second wait is gone with it. There was no number that was both short enough not to leave the user looking at a session that does not close and long enough to be sure.
Closing a session stopped asking for an access token so that a browser that was left alone could still do it, and that left the cookie as the only credential. A browser attaches it to any post of the same site, and same site is the whole domain: a form served from another subdomain was enough to close somebody's session, which the access token in a header never allowed. The request has to carry a header now. A form cannot add one, and anything that can add one is asked for permission before the request is sent, which is the cors allowlist. Verified: the same post without it is refused and the token it was aiming at stays valid.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Logout cancellation and stalled requests can produce incorrect or indefinitely pending session state, and test dependencies conflict with the declared Node range.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (1)
- frontend/package-lock.json: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
frontend/package.json:109
- This direct test dependency no longer supports the package's declared
node >=22range: the lockfile records jsdom 30.0.1 as requiring^22.22.2 || ^24.15.0 || >=26.0.0(and Vitest 5 also starts at Node 22.12). Developers on valid Node 22 releases below those floors can therefore fail to install or run the new tests. Either select compatible test versions or tighten the rootengines.noderange (and pin CI accordingly).
- Files reviewed: 15/16 changed files
- Comments generated: 2
- Review effort level: Balanced
The local side of the logout ran in the finally of the request that revokes it, and the shared instance has no timeout of its own: a stalled connection left the browser sitting on a session it was told to close, for as long as the connection took to give up, with the token still in the store. The comment above it said best effort while everything waited for it. The store is emptied first and the revocation goes out after, which is what best effort means. It can be that way now because the endpoint stopped asking for the access token: what it needs is the cookie, and the browser attaches that on its own. The flag that said a logout was in progress goes away with it. What it was for -not renewing, not announcing twice, not closing twice- is the store being empty, which is now true from the first line instead of from the answer. A flag that has to be reset is one more thing that can be wrong; the store cannot disagree with itself.
Closing a session aborts the renewal that is travelling, and an aborted request has no answer, which is the same shape the interceptor reads as the network being down. So logging out while a renewal was in the air opened a red alert saying the connection to the server failed, on the way to the login screen, loud enough to bury the message that belonged there. Verified both ways: with the check the screen is clean, without it the alert is there.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new test dependencies conflict with part of the package’s declared supported Node range.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (1)
- frontend/package-lock.json: Generated file
- Files reviewed: 15/16 changed files
- Comments generated: 1
- Review effort level: Balanced
The package said it runs on node 22 and up, and the runner that came with the tests does not: jsdom asks for ^22.22.2 || ^24.15.0 || >=26 and vitest for ^22.12.0 || ^24.0.0 || >=26. So node 22.0, 23, 24.0 or 25 installed with a warning and could not run npm test, which is a worse way to find out than being told at install time. What is declared now is the intersection, which is the strictest of the two. The versions it leaves out are old patches of the lines that are supported and the odd numbered lines, which are not long term releases and which both of those projects leave out on purpose. The image runs node 24.21, so nothing changes for the build or for CI: verified by building the image again from scratch, with no EBADENGINE and with the tests and the build green.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A delayed logout response can erase the refresh cookie created by a subsequent login.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (1)
- frontend/package-lock.json: Generated file
- Files reviewed: 15/16 changed files
- Comments generated: 1
- Review effort level: Balanced
…xt session The answer to a logout takes the cookie out of the browser, and a cookie is only its name, its domain and its path: an answer that arrives late takes out whichever one is there by then. Since the store is now emptied before the request is sent, the login screen is there to be used while it travels, so somebody can log in and have the answer to the previous logout delete the cookie they just got. Measured with a logout held for fifteen seconds: the new session loses its cookie and cannot renew, which sends it back to the login screen five minutes later with no explanation. A login drops whatever logout is still travelling, the same way closing a session drops the renewal that is travelling. What it revokes was the token of the session that ended, and that one is already gone from the browser. The sso login needs nothing: it comes back from a redirect, and leaving the page takes any request of the old one with it.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Authentication lifecycle and concurrency changes span multiple trust boundaries and warrant final human end-to-end validation.
Review details
Files not reviewed (1)
- frontend/package-lock.json: Generated file
- Files reviewed: 15/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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
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.
This pull request introduces significant improvements to session and token management in both the frontend and backend. The main focus is on making token renewal more robust and user-friendly, handling browser/server clock mismatches, improving error handling, and ensuring that session expiration is managed consistently. Additionally, backend changes ensure that token refresh endpoints are properly rate-limited, and frontend UI is updated to reflect session expiration in multiple languages.
Frontend improvements to session/token management:
sessionPayloadfunction, which records when a token was received (obtainedAt) and its lifetime, allowing token expiry and renewal to be calculated based on local time rather than relying on potentially mismatched server/browser clocks. This prevents unnecessary logouts due to clock drift. (frontend/src/api/services/auth.jsx[1] [2] [3] [4] [5] [6]useTokenManagerto use exponential backoff and respect server-providedRetry-Afterheaders. Only true session expiration (as indicated by the backend) will log the user out, making the system more resilient to transient network or backend errors. (frontend/src/hooks/useTokenManager.jsx[1] [2]frontend/src/api/setupInterceptors.jsx[1] [2] [3]frontend/public/locales/en/translation.json[1]frontend/public/locales/es/translation.json[2]Backend improvements to token refresh and throttling:
CustomTokenRefreshViewto ensure that both body-based and cookie-based token refresh endpoints share the same throttle scope (token_refresh). This prevents abuse and ensures consistent rate limiting. (ngen/views/auth.pyngen/views/auth.pyR163-R171)Retry-Afterheader when rate limiting is triggered. (ngen/tests/api/test_login_security.pyngen/tests/api/test_login_security.pyR205-R289)Summary of most important changes:
Frontend: Session and Token Management
sessionPayloadto consistently track token lifetime and acquisition time, fixing issues with browser/server clock drift and improving session renewal accuracy. [1] [2] [3] [4] [5] [6]Retry-Afterheaders, ensuring users are only logged out when the backend explicitly ends the session. [1] [2] [3] [4] [5]Frontend: User Experience
Backend: Token Refresh and Throttling
Retry-Afterheaders in rate-limited responses.