AUT-1496 Re-authenticate instead of refresh-looping on session-capped token#149
Conversation
… token Keycloak caps a refreshed token's refreshExpiresIn to whatever remains of the parent SSO session. SmartlingAuthApi.refreshToken() only fell back to authenticate() when the refresh call threw, never when it succeeded but returned an already near-expiry session-capped token - same bug class fixed in authentication-api-sdk (AUT-1473) and java-api-sdk (AUT-1474). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
maescomua
left a comment
maescomua
left a comment
There was a problem hiding this comment.
Review-loop pass (auth-reliability + test-quality personas). Fix is correct and safe to merge — leaving two non-blocking suggestions inline.
| isSessionCapped(response: AccessTokenDto): boolean { | ||
| return response.refreshExpiresIn < this.ttlCorrectionSec; | ||
| } | ||
|
|
There was a problem hiding this comment.
Suggestion (non-blocking): isSessionCapped fails open if refreshExpiresIn is ever missing or non-numeric on a successful refresh response — undefined < this.ttlCorrectionSec is false, so a malformed response is treated as "not capped" and returned as-is.
In practice this self-heals: the same missing field makes tokenCanBeRenewed()'s arithmetic evaluate to NaN, so the very next refreshToken() call is forced into a full authenticate() anyway. And there's no runtime DTO validation anywhere else in this file (makeRequest returns any from a bare JSON.parse), so adding a guard here would be the only validated field in an otherwise-unvalidated contract.
Flagging as a judgment call rather than a fix: is the one-cycle exposure worth a typeof/Number.isFinite guard, or is matching this file's existing no-validation convention preferable? Either answer seems defensible.
There was a problem hiding this comment.
refreshExpiresIn - I learned yesterday that it can be 0 for offline token type. So probably we should address this case too (because MCP server will use offline tokens).
There was a problem hiding this comment.
Fixed by combining with dimitrystd's offline-token point below: refreshExpiresIn > 0 && refreshExpiresIn < this.ttlCorrectionSec. The > 0 guard also makes undefined/NaN safely non-capped (same as before), so no separate typeof/Number.isFinite check needed. In a405df8.
There was a problem hiding this comment.
Confirmed - Keycloak sets refresh_expires_in: 0 for offline-type tokens as a sentinel (governed by Offline Session Idle/Max, not the normal refresh TTL), not "already expired". Fixed with a refreshExpiresIn > 0 guard in isSessionCapped, covered by a new test (offline-type refresh returns as non-capped). In a405df8.
Note: tokenCanBeRenewed() has the same refreshExpiresIn-arithmetic issue for the currently held token (computes requestTimestamp + 0 - ttlCorrectionSec, always in the past) - meaning offline-token clients today never even reach the refresh branch, they always full-authenticate(). That predates this PR and isn't in AUT-1496's scope, but worth a follow-up ticket if the MCP server work depends on offline tokens actually being refreshed rather than always re-authenticated.
| ); | ||
|
|
||
| if (!this.isSessionCapped(refreshedToken)) { | ||
| return refreshedToken; |
There was a problem hiding this comment.
Suggestion (non-blocking): this debug line still reads Can't refresh, doing re-auth with: on the new session-capped fallthrough path — but on that path the refresh call succeeded; it was discarded for being session-capped, not because refresh failed. During an incident, seeing Refreshed token has a session-capped lifetime (5s); re-authenticating. immediately followed by Can't refresh, doing re-auth with: {...} is contradictory and could send an on-call engineer down the wrong path (backend outage vs. expected Keycloak session-cap behavior).
| return refreshedToken; | |
| this.logger.debug(`Falling back to re-auth with: ${JSON.stringify(this.response, SmartlingBaseApi.sensitiveReplacer)}`); |
(Wording is just a suggestion — the point is decoupling this log from the specific "can't refresh" claim so it reads correctly on both the throw-based and session-capped fallback paths.)
There was a problem hiding this comment.
Fixed - changed to "Falling back to re-auth with:" so it reads correctly on both the throw-based and session-capped fallback paths. In a405df8.
isSessionCapped() treated refreshExpiresIn: 0 (Keycloak's sentinel for offline-type tokens, which don't expire on the normal refresh TTL) as session-capped, forcing an unnecessary full re-authenticate. Guard with refreshExpiresIn > 0, which also keeps missing/non-numeric values correctly non-capped without a separate validation guard. Also decouple the re-auth fallback log line from "Can't refresh" wording since it's shared with the session-capped path, where the refresh call actually succeeded. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Summary
SmartlingAuthApi.refreshToken()only fell back toauthenticate()when the refresh call threw, never when it succeeded but returned a session-capped token (Keycloak caps a refreshed token's lifetime to whatever remains of the parent SSO session as it nears its 12h max, so refreshed tokens returned right before that cutoff can be seconds long).isSessionCapped(), checked after a successful refresh: if the refreshed token'srefreshExpiresInis under the existing freshness buffer (ttlCorrectionSec, 10s), discard it and fall through to a fullauthenticate()instead of returning an unusable token.Test plan
isSessionCappedboundary behavior viarefreshToken()(capped refresh falls through toauthenticate(); normal refresh returned as-is)npm run pretest)Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com