AUT-1496 Re-authenticate instead of refresh-looping on session-capped token#149
AUT-1496 Re-authenticate instead of refresh-looping on session-capped token#149PavelLoparev wants to merge 1 commit into
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).
| ); | ||
|
|
||
| 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.)
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