fix(auth): handle NaN clockDrift to fix token auto-refresh#14689
Open
pgbezerra wants to merge 2 commits intoaws-amplify:mainfrom
Open
fix(auth): handle NaN clockDrift to fix token auto-refresh#14689pgbezerra wants to merge 2 commits intoaws-amplify:mainfrom
pgbezerra wants to merge 2 commits intoaws-amplify:mainfrom
Conversation
pgbezerra
requested review from
a team,
avi-karthik,
pranavosu and
sarayev
as code owners
This was referenced Jan 26, 2026
Author
…y#14618) Add test cases for the getTokens() method in TokenOrchestrator to verify token refresh behavior when tokens expire. These tests prove that: - Expired access tokens trigger automatic refresh - Expired ID tokens trigger automatic refresh - forceRefresh option works correctly with valid tokens - signInDetails are preserved after token refresh - NotAuthorizedException returns null and clears tokens - Network errors are thrown (not swallowed) - clientMetadata is passed to the token refresher - New tokens are stored after successful refresh All 12 new tests pass, confirming the core token refresh logic works as expected. This suggests issues reported in aws-amplify#14618 may be related to specific user configurations rather than the refresh mechanism itself. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…fy#14618) Fix automatic token refresh failing silently when clockDrift is NaN. This addresses the issue where fetchAuthSession() does not auto-refresh expired tokens for CUSTOM_WITHOUT_SRP auth flow. ## Root Cause When clockDrift is NaN, the token expiration check always returns false: ```javascript // isTokenExpired.ts (before) return currentTime + clockDrift + tolerance > expiresAt; // currentTime + NaN + tolerance = NaN // NaN > expiresAt = false (always) ``` This causes expired tokens to never be detected as expired, preventing automatic refresh. ## How clockDrift becomes NaN In TokenStore.loadTokens(), clockDrift is parsed from storage: ```javascript const clockDriftString = (await storage.getItem(key)) ?? '0'; const clockDrift = Number.parseInt(clockDriftString); ``` The nullish coalescing operator (??) only handles null/undefined, not empty strings. If clockDrift is stored as "" (empty string): | Stored Value | ?? '0' result | parseInt() result | |--------------|---------------|-------------------| | null | '0' | 0 ✓ | | '123' | '123' | 123 ✓ | | '' | '' (falsy!) | NaN ✗ | This can happen with custom KeyValueStorage implementations or when certain auth flows don't properly initialize the clockDrift value. ## Fix Added three-layer defense against NaN clockDrift: 1. **TokenStore.ts**: Sanitize at load time ```javascript const parsedClockDrift = Number.parseInt(clockDriftString); const clockDrift = Number.isNaN(parsedClockDrift) ? 0 : parsedClockDrift; ``` 2. **TokenOrchestrator.ts**: Use || instead of ?? for fallback ```javascript clockDrift: tokens.clockDrift || 0 // NaN || 0 = 0 // vs clockDrift: tokens.clockDrift ?? 0 // NaN ?? 0 = NaN ``` 3. **isTokenExpired.ts**: Final safety net ```javascript const safeClockDrift = Number.isNaN(clockDrift) ? 0 : clockDrift; ``` ## Test Coverage Added comprehensive tests for clockDrift edge cases: - NaN clockDrift with expired tokens triggers refresh - NaN clockDrift with valid tokens does not trigger refresh - undefined clockDrift with expired tokens triggers refresh - Positive/negative/zero clockDrift handled correctly Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
pgbezerra
force-pushed
the
fix-custom_without_srp-token-storage
branch
from
af45f26 to
45db6c3
Compare
|
Author
|
@pranavosu @ahmedhamouda78, I rebased the branch and noticed that there is a changeset-bot, but I can't add it since I'm not a maintainer. Could you take a look at this PR? It's been more than a month now. |
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
3 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.
Tip
Review each commit separately
NaNclockDrift valuesfetchAuthSession()does not auto-refresh expired tokens forCUSTOM_WITHOUT_SRPauth flowProblem
If
clockDriftwere to becomeNaN, the token expiration check would silently fail:This would cause expired tokens to never be detected as expired, preventing automatic refresh.
How clockDrift could become NaN
In
TokenStore.loadTokens(), clockDrift is parsed from storage:The nullish coalescing operator (
??) only handlesnull/undefined, not empty strings. IfclockDriftwere stored as""(empty string):?? '0'resultparseInt()resultnull'0'0✓'123''123'123✓''''(falsy!)NaN✗This could happen with custom
KeyValueStorageimplementations or if certain auth flows don't properly initialize theclockDriftvalue.Fix
Added defensive handling for
NaNclockDrift at multiple layers:||instead of??for fallback (NaN || 0 = 0vsNaN ?? 0 = NaN)NaNcheckRelated
🤖 Generated with Claude Code