feat: proxy VTEX refresh token and tighten ValidateSession refresh logic#3261
Open
renatomaurovtex wants to merge 1 commit intodevfrom
Open
feat: proxy VTEX refresh token and tighten ValidateSession refresh logic#3261renatomaurovtex wants to merge 1 commit intodevfrom
renatomaurovtex wants to merge 1 commit intodevfrom
Conversation
- Add POST /api/fs/refresh-token to forward cookies to VTEX Identity and return Set-Cookie headers to the browser - Route refreshTokenRequest through the same-origin proxy with retries, failure logging, and single in-flight deduplication - Extract ValidateSession shouldRefreshToken rules (60s grace, first-login iat window, jwt-present expired path) and log server-side reasons - Track consecutive refresh failures in sessionStorage; after 3 failures reset session and redirect to /login (403 flow and validateSession) - Add unit tests for refresh policy, getCookie isExpired, and refreshTokenRequest Made-with: Cursor
renatomaurovtex
requested review from
ommeirelles and
renatamottam
and removed request for
a team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Contributor
WalkthroughThis PR introduces a new proxy endpoint for VTEX refresh-token requests, refactors session refresh logic into reusable decision utilities, implements retry tracking with failure limits to prevent infinite loops, and adds comprehensive test coverage for the new flows. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Proxy as /api/fs/refresh-token
participant VTEX as VTEX API
participant SessionStore
participant SessionStorage as sessionStorage<br/>(Retry Counter)
Client->>Proxy: POST (Cookie included)
alt Feature Disabled
Proxy-->>Client: 404
else Invalid Request
Proxy-->>Client: 401/400
else Proxy Session Active
Proxy->>VTEX: POST /api/vtexid/refreshtoken/webstore<br/>(forward Cookie, Host)
VTEX-->>Proxy: Response with set-cookie
Note over Proxy: Normalize Domain based on<br/>request hostname & allowlist
Proxy-->>Client: 200 with normalized cookies
else VTEX Request Fails
alt Max Retries Exceeded
Proxy->>SessionStorage: increment failure counter
SessionStorage-->>SessionStore: trigger reset on max
SessionStore-->>Client: redirect to /login
else Retry Available
Proxy->>SessionStorage: increment failure counter
SessionStorage-->>Proxy: return new count
Proxy-->>Client: 500
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
packages/core/test/sdk/account/refreshToken.test.ts (1)
32-36: Consider adding a test for disabled feature flag.The
refreshTokenRequestfunction returnsundefinedwhendiscoveryConfig.experimental?.refreshTokenis falsy. Adding a test for this case would ensure the feature gate works correctly.Suggested test case
it('returns undefined when refreshToken feature is disabled', async () => { // You'd need to reset the mock or use jest.isolateModules jest.resetModules() jest.doMock('discovery.config', () => ({ __esModule: true, default: { experimental: { refreshToken: false } }, })) const { refreshTokenRequest } = await import('../../../src/sdk/account/refreshToken') const result = await refreshTokenRequest() expect(result).toBeUndefined() expect(mockedUnfetch).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/sdk/account/refreshToken.test.ts` around lines 32 - 36, Add a unit test that verifies refreshTokenRequest returns undefined and does not call mockedUnfetch when the feature flag is disabled: reset module state (jest.resetModules or jest.isolateModules), mock the discovery config module to export { experimental: { refreshToken: false } }, import refreshTokenRequest from '../../../src/sdk/account/refreshToken', call refreshTokenRequest(), and assert the result is undefined and mockedUnfetch was not called; reference the refreshTokenRequest function and mockedUnfetch in the test to locate where to add this case.packages/core/src/sdk/account/refreshToken.ts (1)
28-32: Consider limiting logged response body content.Logging
lastBody.slice(0, 500)could inadvertently expose sensitive information from error responses. Consider logging only the status code and a generic message, or sanitizing the body content.Also applies to: 39-42, 49-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sdk/account/refreshToken.ts` around lines 28 - 32, In refreshTokenRequest, avoid logging raw response bodies (lastBody) to prevent leaking sensitive data; update the console.error calls (the ones referencing lastStatus and lastBody) to only log the status and a sanitized or generic message (e.g., "non-200 response" plus lastStatus) or a redacted/truncated fingerprint (like length or hash) instead of lastBody.slice(0,500); apply the same change to the other error logging sites in this file (the similar console.error blocks around the other occurrences) so only non-sensitive metadata is logged.packages/core/test/utils/sessionValidateRefreshDecision.test.ts (1)
20-40: Consider adding boundary test at exactly the grace threshold.The tests check
+1and-1around the grace boundary but not the exact boundary (nowSec + SESSION_REFRESH_EXPIRY_GRACE_SEC). Adding this edge case would verify the>=vs>comparison behavior.Suggested addition
it('returns true when exp is exactly at the grace boundary', () => { expect( isExpiredForSessionRefresh(nowSec + SESSION_REFRESH_EXPIRY_GRACE_SEC) ).toBe(true) // or false, depending on intended behavior })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/test/utils/sessionValidateRefreshDecision.test.ts` around lines 20 - 40, Add a boundary test for isExpiredForSessionRefresh to cover the exact grace threshold by asserting the result for exp = nowSec + SESSION_REFRESH_EXPIRY_GRACE_SEC; update the test suite around isExpiredForSessionRefresh to include a new it case that calls isExpiredForSessionRefresh(nowSec + SESSION_REFRESH_EXPIRY_GRACE_SEC) and expects the behavior consistent with intended comparison (>= vs >) so the implementation's boundary behavior is verified.packages/core/src/sdk/account/useRefreshToken.ts (1)
46-52: Duplicated retry-failure handling logic.The retry counting and redirect logic at lines 46-52 is identical to lines 64-70. Extract this into a helper to reduce duplication and ensure consistent behavior.
Suggested extraction
+const handleRefreshFailure = (currentSession: Session) => { + const failures = incrementRefreshFailureCount() + if (failures >= MAX_REFRESH_RETRIES) { + clearRefreshFailureCount() + sessionStore.set(sessionStore.readInitial()) + window.location.href = '/login' + return true // handled with redirect + } + sessionStore.set({ + ...currentSession, + refreshAfter: String(Math.floor(Date.now() / 1000) + 1 * 60 * 60), + }) + return false +} // Then in both branches: } else { - const failures = incrementRefreshFailureCount() - if (failures >= MAX_REFRESH_RETRIES) { - clearRefreshFailureCount() - sessionStore.set(sessionStore.readInitial()) - window.location.href = '/login' - return - } - sessionStore.set({ - ...currentSession, - refreshAfter: String(Math.floor(Date.now() / 1000) + 1 * 60 * 60), - }) + if (handleRefreshFailure(currentSession)) return setShouldShow403(true) }Also applies to: 64-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sdk/account/useRefreshToken.ts` around lines 46 - 52, The retry/failure handling logic using incrementRefreshFailureCount(), MAX_REFRESH_RETRIES, clearRefreshFailureCount(), sessionStore.set(sessionStore.readInitial()) and window.location.href = '/login' is duplicated; extract it into a single helper (e.g., handleRefreshFailure or checkAndHandleRefreshFailure) that increments the failure count, checks against MAX_REFRESH_RETRIES, clears the count, resets the session via sessionStore.set(sessionStore.readInitial()), redirects to '/login' and returns a boolean/void as needed, then replace both duplicated blocks in useRefreshToken.ts with calls to that helper to ensure consistent behavior.packages/core/src/utils/normalizeSetCookieForRequest.ts (1)
3-3: Consider adding a dot prefix tolocalhostfor consistent suffix matching.The suffix
'localhost'(without a leading dot) will match any hostname ending inlocalhost, includingmaliciouslocalhost.com. The.vtex.appand.localhostentries correctly require a subdomain separator, but plainlocalhostis permissive.Suggested safer allowlist
-const ALLOWED_HOST_SUFFIXES = ['localhost', '.vtex.app', '.localhost'] +const ALLOWED_HOST_SUFFIXES = ['.localhost', '.vtex.app'] +const ALLOWED_EXACT_HOSTS = ['localhost']Then update
isAllowedHostto check exact matches as well:const isAllowedHost = ({ host, allowList }: { host: string; allowList: string[] }) => { const normalizedHost = host.toLowerCase() return ( ALLOWED_EXACT_HOSTS.includes(normalizedHost) || allowList.some((suffix) => normalizedHost.endsWith(suffix)) ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/normalizeSetCookieForRequest.ts` at line 3, Change the permissive 'localhost' entry in ALLOWED_HOST_SUFFIXES to require a leading dot (use '.localhost') and add an ALLOWED_EXACT_HOSTS array for exact host matches; then update isAllowedHost to first check if the normalized host is in ALLOWED_EXACT_HOSTS and fall back to allowList.some(suffix => normalizedHost.endsWith(suffix)). Reference ALLOWED_HOST_SUFFIXES and isAllowedHost when making these updates so suffix checks require a dot and exact matches like 'localhost' are handled safely.packages/core/src/sdk/session/index.ts (1)
134-155: SSR-safe redirect handling, but duplicates the retry pattern fromuseRefreshToken.The
typeof window !== 'undefined'check on line 145 is necessary sincevalidateSessioncan run server-side during SSR. However, this retry-failure handling block duplicates logic fromuseRefreshToken. Consider extracting to a shared helper that both can use.Also, after
window.location.href = '/login'at line 148, the code returnsnullbut navigation is asynchronous—any code running aftervalidateSessionreturns could still execute before the redirect completes. This is likely fine given thereturn null, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/sdk/session/index.ts` around lines 134 - 155, The retry-failure + SSR-safe redirect block in validateSession duplicates logic in useRefreshToken; extract that behavior into a shared helper (e.g., handleRefreshFailure) that uses incrementRefreshFailureCount, clearRefreshFailureCount and MAX_REFRESH_RETRIES, performs the typeof window !== 'undefined' guard, resets sessionStore via sessionStore.set(sessionStore.readInitial()) and triggers window.location.href = '/login' when threshold reached, then call this helper from validateSession and useRefreshToken and remove the duplicated block; ensure the helper returns a boolean or null marker so callers can early-return (as validateSession currently returns null after redirect).packages/core/src/utils/sessionValidateRefreshDecision.ts (4)
80-82: Redundant optional chaining on line 81.
jwt?.expuses optional chaining, butjwtis already verified truthy in the outerBoolean(jwt && ...)condition. Minor inconsistency with line 46 which usesjwt.exp.🧹 Consistency fix
const tokenExpired = Boolean( - jwt && isExpiredForSessionRefresh(Number(jwt?.exp)) + jwt && isExpiredForSessionRefresh(Number(jwt.exp)) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/sessionValidateRefreshDecision.ts` around lines 80 - 82, Remove the redundant optional chaining in the token expiration check: where tokenExpired is computed using Boolean(jwt && isExpiredForSessionRefresh(Number(jwt?.exp))), change jwt?.exp to jwt.exp so the call to isExpiredForSessionRefresh uses jwt.exp (jwt is already checked truthy); update the expression in the tokenExpired declaration to mirror the style used elsewhere (e.g., line 46) and keep the call to isExpiredForSessionRefresh(Number(jwt.exp)) intact.
55-64:tokenExpiredWithJwtPresentduplicates check already intokenExpired.
tokenExpiredis defined asBoolean(jwt && isExpiredForSessionRefresh(...)), sojwtpresence is already baked in. Line 58'sBoolean(jwt && tokenExpired)is equivalent to justtokenExpired.🧹 Simplify
- const tokenExpiredWithJwtPresent = Boolean(jwt && tokenExpired) - return ( tokenExistAndIsFirstRefreshTokenRequest || tokenNotExistAndRefreshAfterExistAndIsExpired || - tokenExpiredWithJwtPresent + tokenExpired )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/sessionValidateRefreshDecision.ts` around lines 55 - 64, The variable tokenExpiredWithJwtPresent is redundant because tokenExpired already includes the jwt presence check; remove or replace tokenExpiredWithJwtPresent and use tokenExpired directly in the returned boolean expression (update the return that currently references tokenExpiredWithJwtPresent and eliminate the tokenExpiredWithJwtPresent declaration) so the logic uses tokenExistAndIsFirstRefreshTokenRequest, tokenNotExistAndRefreshAfterExistAndIsExpired, and tokenExpired only.
67-96:describeShouldRefreshTokenReasonduplicates logic fromcomputeShouldRefreshToken.Both functions compute
tokenExpired,refreshAfterExpired, and callisJwtEligibleForFirstRefreshTokenRequest. Consider refactoring to compute once and return both the boolean and reason together, or havedescribeShouldRefreshTokenReasoncall a shared internal helper.This is a minor DRY concern—acceptable for now given the code is well-tested, but worth addressing if this logic grows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/sessionValidateRefreshDecision.ts` around lines 67 - 96, describeShouldRefreshTokenReason duplicates computeShouldRefreshToken logic; factor out the shared computation into a small internal helper and have both functions use it. Create a helper (e.g., computeRefreshDecision or similar) that accepts the same params and returns the boolean shouldRefresh plus the derived values (refreshAfterExist, tokenExpired, refreshAfterExpired, and jwt), then update computeShouldRefreshToken to call that helper and return shouldRefresh, and update describeShouldRefreshTokenReason to call the helper and use its derived flags and isJwtEligibleForFirstRefreshTokenRequest to pick and return the correct reason string.
45-50:Number()on potentially undefinedexpyieldsNaN.If
jwt.expisundefined,Number(undefined)returnsNaN, andisExpiredForSessionRefresh(NaN)will returntrue(sincenow + grace > NaNisfalse, but wait—actuallyNaNcomparisons always returnfalse). This means a JWT withoutexpwould settokenExpired = false, which may be the intended behavior but is implicit and could mask issues.Consider explicit handling:
🛡️ Explicit undefined handling
const tokenExpired = Boolean( - jwt && isExpiredForSessionRefresh(Number(jwt.exp)) + jwt && jwt.exp != null && isExpiredForSessionRefresh(jwt.exp) ) const refreshAfterExpired = - refreshAfterExist && isExpiredForSessionRefresh(Number(refreshAfter)) + refreshAfterExist && isExpiredForSessionRefresh(Number(refreshAfter))For
refreshAfter, since it's a string,Number()is appropriate, but consider validating it's a valid numeric string if the source is untrusted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/sessionValidateRefreshDecision.ts` around lines 45 - 50, The code currently calls Number(jwt.exp) which yields NaN when jwt.exp is undefined; change the logic in tokenExpired to explicitly handle missing exp (e.g., set tokenExpired = false when jwt?.exp is undefined) instead of passing NaN into isExpiredForSessionRefresh, and for refreshAfter use Number only after validating refreshAfterExist and that Number(refreshAfter) is a finite number (use Number.isFinite or !Number.isNaN) before calling isExpiredForSessionRefresh; update the expressions that reference tokenExpired, jwt.exp, isExpiredForSessionRefresh, refreshAfterExist, and refreshAfter accordingly.packages/core/src/pages/api/fs/refresh-token.ts (4)
75-78: Nullish coalescing ondatais redundant.At line 77,
datais guaranteed to be defined (either parsed JSON or{}from line 55). The?? { status: 'Error' }fallback will never execute.🧹 Minor cleanup
response .status(vtexRes.status === 401 ? 401 : 500) - .json(data ?? { status: 'Error' }) + .json(data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pages/api/fs/refresh-token.ts` around lines 75 - 78, The JSON response uses a redundant nullish fallback for the variable `data` in the response path (inside the handler that constructs `vtexRes`/`data` and calls `response.status(...).json(...)`); remove the `?? { status: 'Error' }` and simply call `.json(data)` since `data` is always defined (parsed JSON or `{}` from earlier), leaving the status logic (`vtexRes.status === 401 ? 401 : 500`) unchanged.
82-85: Catch block should distinguish abort errors if timeout is added.Currently the catch logs all errors uniformly. If you add the
AbortControllertimeout, consider checking forAbortErrorto log a more specific message (e.g., "VTEX request timed out").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pages/api/fs/refresh-token.ts` around lines 82 - 85, Update the catch in the API handler in refresh-token.ts to detect abort/timeouts: when catching errors from the proxied VTEX call (the catch for err in the handler), check whether the error is an AbortError (e.g., err.name === 'AbortError' or using AbortError type) and log a specific message like "VTEX request timed out" and return an appropriate status (e.g., 504) instead of the generic 500; otherwise keep the existing console.error and response.status(500).end() behavior. Ensure this logic is applied where AbortController is used for the timeout around the request.
41-50: Consider adding a request timeout.The
fetchcall has no timeout. If VTEX Identity is slow or unresponsive, this request will hang until the serverless function times out (typically 10-30s depending on deployment config). Adding anAbortControllerwith a reasonable timeout (e.g., 5-10s) would improve resilience.⏱️ Suggested timeout implementation
+const VTEX_TIMEOUT_MS = 10_000 + const handler: NextApiHandler = async (request, response) => { // ... existing guards ... const url = `${discoveryConfig.storeUrl}${VTEX_REFRESH_PATH}` try { + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), VTEX_TIMEOUT_MS) + const vtexRes = await fetch(url, { method: 'POST', headers: { 'content-type': 'application/json', cookie: cookieHeader, Host: `${sanitizeHost(discoveryConfig.storeUrl)}`, }, body: JSON.stringify({}), + signal: controller.signal, }) + + clearTimeout(timeoutId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pages/api/fs/refresh-token.ts` around lines 41 - 50, The fetch call that assigns vtexRes lacks a timeout; wrap the request in an AbortController (create controller, pass controller.signal into the fetch used in refresh-token.ts) and start a timer (e.g., 5–10s) that calls controller.abort() if elapsed, then clear the timer after fetch completes; update the try/catch around the fetch to specifically handle an abort/timeout error and surface a clear message while still using existing symbols like vtexRes, cookieHeader, sanitizeHost and discoveryConfig.storeUrl; ensure the controller timer is always cleared to avoid leaks.
54-60: Type assertion on JSON.parse result.
JSON.parse(text) as Record<string, unknown>is a type assertion. While acceptable here since the VTEX API contract is known, consider adding a minimal runtime check if the response structure matters for downstream logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pages/api/fs/refresh-token.ts` around lines 54 - 60, The code currently asserts the result of JSON.parse into data without verifying its shape; after parsing (JSON.parse(text)) validate that the result is a non-null object (and if downstream logic expects specific keys, check those keys, e.g., presence of "refresh_token" or other required fields) and if the check fails log a clear error and return the 500 response (use the same response.status(...).json(...) flow). Update the block around JSON.parse(text) and the data variable to perform this minimal runtime validation before proceeding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/sdk/account/refreshToken.ts`:
- Around line 27-34: The retry loop in refreshTokenRequest currently treats any
non-200 response the same and continues retrying; change behavior so that if
res.status === 404 you do not retry but immediately fail fast (log or throw an
error) to avoid unnecessary delays when experimental.refreshToken is disabled.
Locate the refreshTokenRequest function and adjust the non-200 branch that
checks res.status to handle 404 as a non-retryable case (use the res.status
value) instead of executing the continue path, ensuring other status codes still
follow the existing retry logic.
In `@packages/core/src/sdk/account/useRefreshToken.ts`:
- Around line 46-52: The failure counter is being incremented in both
useRefreshToken and validateSession causing double-counts; modify the retry
logic so only one site updates the counter (preferably centralize into
incrementRefreshFailureCount) or make incrementRefreshFailureCount idempotent by
adding short-window deduplication: store a lastFailureTimestamp and only
increment if now - lastFailureTimestamp > DEDUP_WINDOW_MS, and ensure
clearRefreshFailureCount/reset timestamps are called (clearRefreshFailureCount)
when a refresh succeeds; update references in useRefreshToken and
validateSession to rely on the centralized increment (or the timestamp-guarded
increment) and keep MAX_REFRESH_RETRIES checks using the single source of truth.
---
Nitpick comments:
In `@packages/core/src/pages/api/fs/refresh-token.ts`:
- Around line 75-78: The JSON response uses a redundant nullish fallback for the
variable `data` in the response path (inside the handler that constructs
`vtexRes`/`data` and calls `response.status(...).json(...)`); remove the `?? {
status: 'Error' }` and simply call `.json(data)` since `data` is always defined
(parsed JSON or `{}` from earlier), leaving the status logic (`vtexRes.status
=== 401 ? 401 : 500`) unchanged.
- Around line 82-85: Update the catch in the API handler in refresh-token.ts to
detect abort/timeouts: when catching errors from the proxied VTEX call (the
catch for err in the handler), check whether the error is an AbortError (e.g.,
err.name === 'AbortError' or using AbortError type) and log a specific message
like "VTEX request timed out" and return an appropriate status (e.g., 504)
instead of the generic 500; otherwise keep the existing console.error and
response.status(500).end() behavior. Ensure this logic is applied where
AbortController is used for the timeout around the request.
- Around line 41-50: The fetch call that assigns vtexRes lacks a timeout; wrap
the request in an AbortController (create controller, pass controller.signal
into the fetch used in refresh-token.ts) and start a timer (e.g., 5–10s) that
calls controller.abort() if elapsed, then clear the timer after fetch completes;
update the try/catch around the fetch to specifically handle an abort/timeout
error and surface a clear message while still using existing symbols like
vtexRes, cookieHeader, sanitizeHost and discoveryConfig.storeUrl; ensure the
controller timer is always cleared to avoid leaks.
- Around line 54-60: The code currently asserts the result of JSON.parse into
data without verifying its shape; after parsing (JSON.parse(text)) validate that
the result is a non-null object (and if downstream logic expects specific keys,
check those keys, e.g., presence of "refresh_token" or other required fields)
and if the check fails log a clear error and return the 500 response (use the
same response.status(...).json(...) flow). Update the block around
JSON.parse(text) and the data variable to perform this minimal runtime
validation before proceeding.
In `@packages/core/src/sdk/account/refreshToken.ts`:
- Around line 28-32: In refreshTokenRequest, avoid logging raw response bodies
(lastBody) to prevent leaking sensitive data; update the console.error calls
(the ones referencing lastStatus and lastBody) to only log the status and a
sanitized or generic message (e.g., "non-200 response" plus lastStatus) or a
redacted/truncated fingerprint (like length or hash) instead of
lastBody.slice(0,500); apply the same change to the other error logging sites in
this file (the similar console.error blocks around the other occurrences) so
only non-sensitive metadata is logged.
In `@packages/core/src/sdk/account/useRefreshToken.ts`:
- Around line 46-52: The retry/failure handling logic using
incrementRefreshFailureCount(), MAX_REFRESH_RETRIES, clearRefreshFailureCount(),
sessionStore.set(sessionStore.readInitial()) and window.location.href = '/login'
is duplicated; extract it into a single helper (e.g., handleRefreshFailure or
checkAndHandleRefreshFailure) that increments the failure count, checks against
MAX_REFRESH_RETRIES, clears the count, resets the session via
sessionStore.set(sessionStore.readInitial()), redirects to '/login' and returns
a boolean/void as needed, then replace both duplicated blocks in
useRefreshToken.ts with calls to that helper to ensure consistent behavior.
In `@packages/core/src/sdk/session/index.ts`:
- Around line 134-155: The retry-failure + SSR-safe redirect block in
validateSession duplicates logic in useRefreshToken; extract that behavior into
a shared helper (e.g., handleRefreshFailure) that uses
incrementRefreshFailureCount, clearRefreshFailureCount and MAX_REFRESH_RETRIES,
performs the typeof window !== 'undefined' guard, resets sessionStore via
sessionStore.set(sessionStore.readInitial()) and triggers window.location.href =
'/login' when threshold reached, then call this helper from validateSession and
useRefreshToken and remove the duplicated block; ensure the helper returns a
boolean or null marker so callers can early-return (as validateSession currently
returns null after redirect).
In `@packages/core/src/utils/normalizeSetCookieForRequest.ts`:
- Line 3: Change the permissive 'localhost' entry in ALLOWED_HOST_SUFFIXES to
require a leading dot (use '.localhost') and add an ALLOWED_EXACT_HOSTS array
for exact host matches; then update isAllowedHost to first check if the
normalized host is in ALLOWED_EXACT_HOSTS and fall back to allowList.some(suffix
=> normalizedHost.endsWith(suffix)). Reference ALLOWED_HOST_SUFFIXES and
isAllowedHost when making these updates so suffix checks require a dot and exact
matches like 'localhost' are handled safely.
In `@packages/core/src/utils/sessionValidateRefreshDecision.ts`:
- Around line 80-82: Remove the redundant optional chaining in the token
expiration check: where tokenExpired is computed using Boolean(jwt &&
isExpiredForSessionRefresh(Number(jwt?.exp))), change jwt?.exp to jwt.exp so the
call to isExpiredForSessionRefresh uses jwt.exp (jwt is already checked truthy);
update the expression in the tokenExpired declaration to mirror the style used
elsewhere (e.g., line 46) and keep the call to
isExpiredForSessionRefresh(Number(jwt.exp)) intact.
- Around line 55-64: The variable tokenExpiredWithJwtPresent is redundant
because tokenExpired already includes the jwt presence check; remove or replace
tokenExpiredWithJwtPresent and use tokenExpired directly in the returned boolean
expression (update the return that currently references
tokenExpiredWithJwtPresent and eliminate the tokenExpiredWithJwtPresent
declaration) so the logic uses tokenExistAndIsFirstRefreshTokenRequest,
tokenNotExistAndRefreshAfterExistAndIsExpired, and tokenExpired only.
- Around line 67-96: describeShouldRefreshTokenReason duplicates
computeShouldRefreshToken logic; factor out the shared computation into a small
internal helper and have both functions use it. Create a helper (e.g.,
computeRefreshDecision or similar) that accepts the same params and returns the
boolean shouldRefresh plus the derived values (refreshAfterExist, tokenExpired,
refreshAfterExpired, and jwt), then update computeShouldRefreshToken to call
that helper and return shouldRefresh, and update
describeShouldRefreshTokenReason to call the helper and use its derived flags
and isJwtEligibleForFirstRefreshTokenRequest to pick and return the correct
reason string.
- Around line 45-50: The code currently calls Number(jwt.exp) which yields NaN
when jwt.exp is undefined; change the logic in tokenExpired to explicitly handle
missing exp (e.g., set tokenExpired = false when jwt?.exp is undefined) instead
of passing NaN into isExpiredForSessionRefresh, and for refreshAfter use Number
only after validating refreshAfterExist and that Number(refreshAfter) is a
finite number (use Number.isFinite or !Number.isNaN) before calling
isExpiredForSessionRefresh; update the expressions that reference tokenExpired,
jwt.exp, isExpiredForSessionRefresh, refreshAfterExist, and refreshAfter
accordingly.
In `@packages/core/test/sdk/account/refreshToken.test.ts`:
- Around line 32-36: Add a unit test that verifies refreshTokenRequest returns
undefined and does not call mockedUnfetch when the feature flag is disabled:
reset module state (jest.resetModules or jest.isolateModules), mock the
discovery config module to export { experimental: { refreshToken: false } },
import refreshTokenRequest from '../../../src/sdk/account/refreshToken', call
refreshTokenRequest(), and assert the result is undefined and mockedUnfetch was
not called; reference the refreshTokenRequest function and mockedUnfetch in the
test to locate where to add this case.
In `@packages/core/test/utils/sessionValidateRefreshDecision.test.ts`:
- Around line 20-40: Add a boundary test for isExpiredForSessionRefresh to cover
the exact grace threshold by asserting the result for exp = nowSec +
SESSION_REFRESH_EXPIRY_GRACE_SEC; update the test suite around
isExpiredForSessionRefresh to include a new it case that calls
isExpiredForSessionRefresh(nowSec + SESSION_REFRESH_EXPIRY_GRACE_SEC) and
expects the behavior consistent with intended comparison (>= vs >) so the
implementation's boundary behavior is verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 915dc805-3edf-47ac-9aa0-33681b076799
⛔ Files ignored due to path filters (1)
specs/session-refresh-token-24h-expiration.mdis excluded by none and included by none
📒 Files selected for processing (11)
packages/core/src/pages/api/fs/refresh-token.tspackages/core/src/pages/api/graphql.tspackages/core/src/sdk/account/refreshToken.tspackages/core/src/sdk/account/useRefreshToken.tspackages/core/src/sdk/session/index.tspackages/core/src/utils/normalizeSetCookieForRequest.tspackages/core/src/utils/refreshTokenRetry.tspackages/core/src/utils/sessionValidateRefreshDecision.tspackages/core/test/sdk/account/refreshToken.test.tspackages/core/test/utils/getCookie.test.tspackages/core/test/utils/sessionValidateRefreshDecision.test.ts
Comment on lines
+27
to
+34
| if (res.status !== 200) { | ||
| console.error( | ||
| '[refreshTokenRequest] non-200 response', | ||
| lastStatus, | ||
| lastBody.slice(0, 500) | ||
| ) | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Retrying on 404 may cause unnecessary delays when feature is disabled server-side.
If the server has experimental.refreshToken disabled (returns 404), the client will retry 3 times before giving up. Consider treating 404 as a non-retryable status to fail fast when there's a client/server config mismatch.
Suggested fix
if (res.status !== 200) {
+ // 404 means feature disabled server-side - don't retry
+ if (res.status === 404) {
+ console.warn('[refreshTokenRequest] refresh-token endpoint not enabled')
+ return undefined
+ }
console.error(
'[refreshTokenRequest] non-200 response',
lastStatus,
lastBody.slice(0, 500)
)
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (res.status !== 200) { | |
| console.error( | |
| '[refreshTokenRequest] non-200 response', | |
| lastStatus, | |
| lastBody.slice(0, 500) | |
| ) | |
| continue | |
| } | |
| if (res.status !== 200) { | |
| // 404 means feature disabled server-side - don't retry | |
| if (res.status === 404) { | |
| console.warn('[refreshTokenRequest] refresh-token endpoint not enabled') | |
| return undefined | |
| } | |
| console.error( | |
| '[refreshTokenRequest] non-200 response', | |
| lastStatus, | |
| lastBody.slice(0, 500) | |
| ) | |
| continue | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/sdk/account/refreshToken.ts` around lines 27 - 34, The
retry loop in refreshTokenRequest currently treats any non-200 response the same
and continues retrying; change behavior so that if res.status === 404 you do not
retry but immediately fail fast (log or throw an error) to avoid unnecessary
delays when experimental.refreshToken is disabled. Locate the
refreshTokenRequest function and adjust the non-200 branch that checks
res.status to handle 404 as a non-retryable case (use the res.status value)
instead of executing the continue path, ensuring other status codes still follow
the existing retry logic.
Comment on lines
+46
to
+52
| const failures = incrementRefreshFailureCount() | ||
| if (failures >= MAX_REFRESH_RETRIES) { | ||
| clearRefreshFailureCount() | ||
| sessionStore.set(sessionStore.readInitial()) | ||
| window.location.href = '/login' | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all call sites of validateSession and how they might overlap with useRefreshToken
echo "=== validateSession call sites ==="
rg -n "validateSession\(" --type=ts -g '!*.test.ts' -g '!*.d.ts' -C2
echo ""
echo "=== useRefreshToken usage ==="
rg -n "useRefreshToken\(" --type=ts -g '!*.test.ts' -g '!*.d.ts' -C2Repository: vtex/faststore
Length of output: 5456
🏁 Script executed:
cat -n packages/core/src/sdk/account/useRefreshToken.tsRepository: vtex/faststore
Length of output: 3589
🏁 Script executed:
cat -n packages/core/src/sdk/session/index.tsRepository: vtex/faststore
Length of output: 10416
Coordinate refresh token retry counting to prevent double-increments.
Both useRefreshToken (lines 46, 64) and validateSession (line 144 in session/index.ts) independently increment the same failure counter when handling 401 errors. If a single auth failure triggers both code paths—for example, when validateSession executes from useGeolocation or useRegion while the 403 page is simultaneously running useRefreshToken—the counter increments twice, causing premature logout.
Consider centralizing the retry logic to a single call site or adding timestamp-based deduplication to prevent double-counting within a short window.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/sdk/account/useRefreshToken.ts` around lines 46 - 52, The
failure counter is being incremented in both useRefreshToken and validateSession
causing double-counts; modify the retry logic so only one site updates the
counter (preferably centralize into incrementRefreshFailureCount) or make
incrementRefreshFailureCount idempotent by adding short-window deduplication:
store a lastFailureTimestamp and only increment if now - lastFailureTimestamp >
DEDUP_WINDOW_MS, and ensure clearRefreshFailureCount/reset timestamps are called
(clearRefreshFailureCount) when a refresh succeeds; update references in
useRefreshToken and validateSession to rely on the centralized increment (or the
timestamp-guarded increment) and keep MAX_REFRESH_RETRIES checks using the
single source of truth.
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
1 participant
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.
Made-with: Cursor
What's the purpose of this pull request?
How it works?
How to test it?
Starters Deploy Preview
References
Checklist
You may erase this after checking them all 😉
PR Title and Commit Messages
feat,fix,chore,docs,style,refactor,ciandtestPR Description
breaking change,bug,contributing,performance,documentation..Dependencies
pnpm-lock.yamlfile when there were changes to the packagesDocumentation
@Mariana-Caetanoto review and update (Or submit a doc request)Summary by CodeRabbit
Release Notes
New Features
Improvements