Implement token refresh and session management enhancements#467
Merged
DonOmalVindula merged 12 commits intoasgardeo:mainfrom Apr 21, 2026
Merged
Implement token refresh and session management enhancements#467DonOmalVindula merged 12 commits intoasgardeo:mainfrom
DonOmalVindula merged 12 commits intoasgardeo:mainfrom
Conversation
- Introduced `refreshToken` action to refresh access tokens using stored refresh tokens. - Updated `isSignedIn` action to streamline session validation logic. - Enhanced `signInAction` and `switchOrganization` actions to handle refresh tokens and session expiry. - Modified `handleOAuthCallbackAction` to include refresh token handling. - Improved `AsgardeoProvider` to support new actions and session management features. - Added constants for default access token and session expiry durations. - Created `tokenRefreshCore` utility for handling token refresh logic. - Updated middleware to proactively refresh tokens and manage session state effectively. - Added `clearSession` action to delete session cookies without contacting the identity server.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable session-cookie expiry and refresh-token handling across Next.js server middleware and actions; exposes refresh/clear session functions to client context/provider; introduces proactive token-refresh logic, new server actions/utilities, middleware entry for Edge, and Edge build/export entries. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware
participant SessionMgr as SessionManager
participant OAuth as OAuthServer
participant Cookies
Client->>Middleware: Request (with session cookie)
activate Middleware
Middleware->>SessionMgr: verifySessionToken(cookie)
alt Valid & access token near-expiry
Middleware->>SessionMgr: extract refreshToken
Middleware->>OAuth: POST /oauth2/token (grant_type=refresh_token)
activate OAuth
OAuth-->>Middleware: tokenResponse (access_token, expires_in, refresh_token)
deactivate OAuth
Middleware->>SessionMgr: createSessionToken(expires_in, refresh_token)
Middleware->>Cookies: set(updated session cookie with resolved maxAge)
Middleware->>Middleware: attach cookie to response & update forwarded headers
else Token invalid but refreshToken present
Middleware->>SessionMgr: verifySessionTokenForRefresh(cookie)
Middleware->>OAuth: POST /oauth2/token (refresh_token grant)
OAuth-->>Middleware: tokenResponse
Middleware->>SessionMgr: createSessionToken(...)
Middleware->>Cookies: set(new session cookie)
else No valid session / no refreshToken
Middleware->>Cookies: delete stale session cookie
Middleware->>Client: redirect to sign-in (or handler response)
end
Middleware-->>Client: Response (with updated session or redirect)
deactivate Middleware
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx (1)
295-312:⚠️ Potential issue | 🟡 MinorAdd
clearSessionandrefreshTokento the useMemo dependency array.Both
clearSessionandrefreshTokenare component props included incontextValue, but they are missing from the dependency list. Since props can change on parent re-renders, the memoized value should recalculate to avoid stale references.♻️ Minimal fix
- [baseUrl, user, isSignedIn, isLoading, signInUrl, signUpUrl, applicationId, organizationHandle], + [ + applicationId, + baseUrl, + clearSession, + isLoading, + isSignedIn, + organizationHandle, + refreshToken, + signInUrl, + signUpUrl, + user, + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx` around lines 295 - 312, The memoized contextValue created by useMemo is missing clearSession and refreshToken in its dependency array, which can yield stale prop references; update the dependency list for the useMemo call that builds contextValue (the one returning AsgardeoContextProps with applicationId, baseUrl, clearSession, isLoading, isSignedIn, organizationHandle, refreshToken, signIn, signInUrl, signOut, signUp, signUpUrl, user) to include clearSession and refreshToken so the memo recalculates whenever those props change.
🧹 Nitpick comments (6)
packages/javascript/esbuild.config.mjs (1)
46-58: Consider using'process.versions': '({})'as a defensive safeguard.The current approach of replacing
'process.versions'with'undefined'is safe for the guarded access inisNode()(process.versions && process.versions.node). However, using'process.versions': '({})'is a more defensive approach that prevents potential TypeErrors if any bundled code accessesprocess.versions.nodewithout a guard—it would evaluate toundefined(falsy) rather than throwing a TypeError onundefined.node.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/esbuild.config.mjs` around lines 46 - 58, Update the esbuild build define mapping to substitute process.versions with an empty object instead of undefined so guarded and unguarded accesses don't throw; in the esbuild.build call that spreads commonOptions and sets define (the block containing 'process.versions': 'undefined'), change the replacement to 'process.versions': '({})' so code using isNode() (and any direct process.versions.node access) safely yields undefined rather than causing a TypeError.packages/nextjs/src/server/actions/clearSession.ts (1)
21-48: Replacenext/dist/*internal type import with type inference.
next/dist/*is not a stable public API. TheCookieStoretype is not publicly exported by Next.js; use type inference instead:♻️ Proposed cleanup
-import {ReadonlyRequestCookies} from 'next/dist/server/web/spec-extension/adapters/request-cookies'; import {cookies} from 'next/headers'; import logger from '../../utils/logger'; import SessionManager from '../../utils/SessionManager'; @@ const clearSession = async (): Promise<void> => { - const cookieStore: ReadonlyRequestCookies = await cookies(); + const cookieStore = await cookies(); cookieStore.delete(SessionManager.getSessionCookieName());This pattern appears in multiple server actions across the codebase. Consider addressing all instances as part of a broader refactor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/server/actions/clearSession.ts` around lines 21 - 48, Remove the unstable internal type import ReadonlyRequestCookies from next/dist/* and stop annotating cookieStore with it; instead let TypeScript infer the cookies() return type in clearSession (replace "const cookieStore: ReadonlyRequestCookies = await cookies();" with an untyped "const cookieStore = await cookies();" and remove the ReadonlyRequestCookies import). Keep the call to SessionManager.getSessionCookieName() and cookieStore.delete(...) unchanged. Apply the same pattern to other server action files that import next/dist/* types.packages/nextjs/src/server/actions/handleOAuthCallbackAction.ts (1)
152-157: Log the outer catch for parity with other actions.The inner catch at lines 138-143 logs via
logger.error, but the outer catch silently swallows the error.signInAction.tsnow logs before returning; consider doing the same here to avoid losing diagnostic signal when the top-level flow fails.♻️ Proposed change
} catch (error) { + logger.error( + `[handleOAuthCallbackAction] Authentication failed: ${error instanceof Error ? error.message : String(error)}`, + ); return { error: error instanceof Error ? error.message : 'Authentication failed', success: false, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/server/actions/handleOAuthCallbackAction.ts` around lines 152 - 157, The outer catch in handleOAuthCallbackAction currently returns the error without logging; update it to log the caught error via logger.error before returning (mirror signInAction.ts and the inner catch at lines 138-143), ensuring you pass the Error object or message (use error instanceof Error ? error : new Error(String(error))) to logger.error so diagnostics aren’t lost when handleOAuthCallbackAction fails.packages/nextjs/src/utils/tokenRefreshCore.ts (2)
64-65: Drop the?? ''fallback on required credentials.
clientIdandclientSecretare declared as requiredstringinTokenRefreshCoreConfig. The?? ''defaults turn a caller-side misconfiguration (e.g. undefined env var slipping through) into a silent POST with empty credentials, which the IdP returns as an opaque 401. Better to let the type system / an explicit check surface the bug.♻️ Proposed tightening
const body: URLSearchParams = new URLSearchParams({ - client_id: clientId ?? '', - client_secret: clientSecret ?? '', + client_id: clientId, + client_secret: clientSecret, grant_type: 'refresh_token', refresh_token: storedRefreshToken, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/utils/tokenRefreshCore.ts` around lines 64 - 65, The code currently uses fallbacks client_id: clientId ?? '' and client_secret: clientSecret ?? '' which masks missing required credentials; remove the "?? ''" fallbacks and instead pass clientId and clientSecret directly (or add an explicit runtime check) so TokenRefreshCoreConfig's required string properties (clientId, clientSecret) fail fast and surface misconfiguration rather than sending empty credentials; update the block that builds the token POST payload (client_id/client_secret) in tokenRefreshCore.ts accordingly and add an assertion or throw if clientId/clientSecret are falsy.
62-99: Surface token-endpoint error details and validate the access_token.Three small hardenings on the refresh path:
${baseUrl}/oauth2/tokendoubles the slash ifbaseUrlends with/; trim it.- When
response.okis false, the body carries OAutherror/error_description— today they're dropped, makinginvalid_grant(expired/revoked refresh token) indistinguishable frominvalid_client.access_tokenis cast without a presence check; a malformed 200 response silently produces a session JWT wrappingundefined.🛡️ Suggested improvements
- const tokenEndpoint: string = `${baseUrl}/oauth2/token`; + const tokenEndpoint: string = `${baseUrl.replace(/\/+$/, '')}/oauth2/token`; ... if (!response.ok) { - throw new Error(`Token endpoint rejected refresh (HTTP ${response.status}).`); + const errorBody: string = await response.text().catch(() => ''); + throw new Error( + `Token endpoint rejected refresh (HTTP ${response.status}): ${errorBody.slice(0, 500)}`, + ); } ... const newAccessToken: string = tokenData['access_token'] as string; + if (!newAccessToken || typeof newAccessToken !== 'string') { + throw new Error('Token endpoint response missing access_token.'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/utils/tokenRefreshCore.ts` around lines 62 - 99, The refresh logic should avoid double slashes in tokenEndpoint, surface OAuth error details on non-OK responses, and validate presence of access_token before continuing: construct tokenEndpoint by joining baseUrl and '/oauth2/token' without duplicating slashes (adjust how tokenEndpoint is built), when response.ok is false read response.text()/json() and include tokenData.error and tokenData.error_description (or raw body) in the thrown Error to distinguish invalid_grant vs invalid_client, and after parsing tokenData ensure tokenData['access_token'] exists and is a non-empty string before assigning to newAccessToken (throw a clear error if missing) while keeping the rotated refresh token fallback for newRefreshToken.packages/nextjs/src/client/contexts/Asgardeo/AsgardeoContext.ts (1)
40-45: DefaultrefreshTokenreturns an emptyTokenResponse-shaped object.
Promise.resolve({} as TokenResponse)satisfies the type but resolves to an object whose properties are allundefined. Because both props are already declared optional inAsgardeoContextProps, consider omitting them from the default so callers outside the provider must guard with?., instead of silently receiving a "token response" with no fields.♻️ Optional simplification
- clearSession: () => Promise.resolve(), isInitialized: false, isLoading: true, isSignedIn: false, organizationHandle: undefined, - refreshToken: () => Promise.resolve({} as TokenResponse),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/client/contexts/Asgardeo/AsgardeoContext.ts` around lines 40 - 45, The default refreshToken implementation returns Promise.resolve({} as TokenResponse) which produces an object with all undefined fields; instead change the default to return Promise.resolve(undefined) (i.e., () => Promise.resolve(undefined)) so callers outside the provider must guard with optional chaining; update the refreshToken signature where necessary to Promise<TokenResponse | undefined> to keep types consistent and reference AsgardeoContextProps and the refreshToken field when making the change.
🤖 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/nextjs/package.json`:
- Around line 32-37: In the package.json exports, reorder the condition keys
inside the "./middleware" export object so that "types" is the first condition
(move "types": "./dist/types/middleware.d.ts" to the top of that object) to
match the sibling exports and ensure consistent TypeScript resolution; keep the
other keys ("edge-light", "import", "require") unchanged and preserve their
target paths.
In `@packages/nextjs/src/server/actions/isSignedIn.ts`:
- Around line 40-43: The code in isSignedIn.ts returns true whenever
getSessionPayload() returns a payload, but you must also validate payload.type
=== 'session' to ensure only session tokens count; update the isSignedIn logic
to check the SessionTokenPayload.type (or call a helper) after await
getSessionPayload() and only return true when the token exists AND payload.type
=== 'session' (referencing getSessionPayload, SessionTokenPayload, and
SessionManager.verifySessionToken to ensure consistency).
In `@packages/nextjs/src/server/actions/refreshToken.ts`:
- Around line 54-63: The current flow calls SessionManager.verifySessionToken in
refreshToken.ts which treats SessionTokenPayload.exp (the access-token expiry)
as a hard failure, preventing refresh; replace that call with a new
refresh-specific verifier (e.g. SessionManager.verifySessionTokenForRefresh)
that verifies signature and required claims but ignores/does not enforce the
access-token exp, and then pass the returned payload into tokenRefreshCore;
ensure session lifetime is still enforced separately using the configured
session expiry/cookie policy (sessionExpirySeconds) rather than the access-token
exp.
- Around line 80-99: The catch currently unconditionally deletes the session
cookie; change it so only terminal auth failures clear the cookie (e.g., missing
session, invalid session, or invalid_refresh_token), while transient/network/5xx
errors do not. Inspect the thrown error from tokenRefreshCore()/refreshToken
(preserve status codes or error_code like "invalid_grant") and only call
cookies().delete(SessionManager.getSessionCookieName()) and logger.debug when
the error is clearly an auth-terminal error; otherwise rethrow without mutating
cookies. Ensure tokenRefreshCore preserves token endpoint status/error codes if
needed so you can distinguish retryable failures from terminal ones.
- Around line 78-79: The action refreshToken currently returns the full
result.tokenResponse (from tokenRefreshCore) which may include a rotated refresh
token; change the return value so the server-held refresh token is never
returned to client code—either return a new narrowed type/shape (e.g.,
accessToken, expiresIn, tokenType, scope, etc.) or explicitly copy
result.tokenResponse while omitting refreshToken before returning from
refreshToken (ensure tokenRefreshCore's rotated refresh token remains stored
server-side in the HTTP-only session cookie). Update the return signature of
refreshToken (and its callers) to the narrower type if TokenResponse requires
refreshToken.
- Around line 22-23: The code imports Next.js internal type
ReadonlyRequestCookies from 'next/dist/server/...' which is unstable; remove
that import and derive the cookie type from the public cookies() helper instead
(e.g., declare type RequestCookies = ReturnType<typeof cookies> and use that
type wherever ReadonlyRequestCookies was used). Update any function signatures
or variables in refreshToken.ts that reference ReadonlyRequestCookies (for
example in functions handling token cookies) to use the new RequestCookies type,
and ensure you import only cookies from 'next/headers'. Repeat the same
replacement for the other occurrences referenced in the review.
In `@packages/nextjs/src/server/actions/switchOrganization.ts`:
- Around line 56-58: The expiresIn assignment uses parseInt on
tokenResponse.expiresIn which can produce NaN or 0 and silently corrupt session
TTL; update the logic around the expiresIn variable (the parseInt call that
reads tokenResponse.expiresIn) to parse and validate the numeric value (e.g.,
use Number(...) or parseInt then check Number.isFinite(parsed) && parsed > 0)
and fall back to DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS when the parsed value is
not a positive integer, so that SessionManager.createSessionToken always
receives a valid positive expiry.
In `@packages/nextjs/src/server/middleware/asgardeoMiddleware.ts`:
- Around line 180-233: The code currently decodes unverified JWTs via
SessionManager.decodeSessionToken and later passes that untrusted payload
(expiredSession) into tokenRefreshCore to mint a new session token; instead,
change the refresh flow so tokenRefreshCore does not rely on the unverified
decoded payload — either make SessionManager.decodeSessionToken perform
signature verification (allowing expired tokens only) or, preferably, have
tokenRefreshCore return the IdP id_token (or raw id_token from the refresh
response) and derive sub/sessionId/organizationId from that verified id_token
before creating a new session; update the call-site around needsRefresh (where
expiredSession and candidateSession are set and tokenRefreshCore is invoked) to
use the verified id_token claims or to require a verified decode to eliminate
use of attacker-controlled claim values.
In `@packages/nextjs/src/utils/constants.ts`:
- Around line 44-45: Update the documentation to accurately state that the JWT
`exp` and the browser cookie `maxAge` can differ: note that
SessionManager.createSessionToken(...) sets the JWT `exp` using
accessTokenTtlSeconds, while the cookie `maxAge` is derived from the resolved
session lifetime (not accessTokenTtlSeconds), and clarify which value controls
token expiry vs cookie expiry so readers aren’t misled that they always match.
In `@packages/nextjs/src/utils/SessionManager.ts`:
- Around line 116-139: createSessionToken currently sets the JWT exp to
accessTokenTtlSeconds causing session tokens to expire with the access token;
change createSessionToken so the JWT expiration (setExpirationTime) uses the
session lifetime instead and add a separate payload field (e.g.
accessTokenExpiresAt) that records the access token expiry (Date.now()/seconds +
accessTokenTtlSeconds) so callers can validate the access token separately;
update the SessionTokenPayload shape to include accessTokenExpiresAt (and keep
organizationId, refreshToken, scopes, sessionId, type) and ensure jwtVerify
checks the JWT exp for session lifetime while access-token logic reads
accessTokenExpiresAt.
- Around line 127-150: The decodeSessionToken function in SessionManager
currently uses decodeJwt (no signature verification) which allows forged
payloads; change SessionManager.decodeSessionToken to be async, import and use
compactVerify to verify the JWS signature with the existing secret before
decoding, then return the verified payload (preserving ability to inspect
expired tokens safely), and remove direct use of decodeJwt for trust decisions.
Update all call sites (e.g., the asgardeoMiddleware handler that calls
decodeSessionToken) to await the now-async SessionManager.decodeSessionToken and
handle verification failures by rejecting/throwing so refresh logic only runs on
verified tokens.
---
Outside diff comments:
In `@packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx`:
- Around line 295-312: The memoized contextValue created by useMemo is missing
clearSession and refreshToken in its dependency array, which can yield stale
prop references; update the dependency list for the useMemo call that builds
contextValue (the one returning AsgardeoContextProps with applicationId,
baseUrl, clearSession, isLoading, isSignedIn, organizationHandle, refreshToken,
signIn, signInUrl, signOut, signUp, signUpUrl, user) to include clearSession and
refreshToken so the memo recalculates whenever those props change.
---
Nitpick comments:
In `@packages/javascript/esbuild.config.mjs`:
- Around line 46-58: Update the esbuild build define mapping to substitute
process.versions with an empty object instead of undefined so guarded and
unguarded accesses don't throw; in the esbuild.build call that spreads
commonOptions and sets define (the block containing 'process.versions':
'undefined'), change the replacement to 'process.versions': '({})' so code using
isNode() (and any direct process.versions.node access) safely yields undefined
rather than causing a TypeError.
In `@packages/nextjs/src/client/contexts/Asgardeo/AsgardeoContext.ts`:
- Around line 40-45: The default refreshToken implementation returns
Promise.resolve({} as TokenResponse) which produces an object with all undefined
fields; instead change the default to return Promise.resolve(undefined) (i.e.,
() => Promise.resolve(undefined)) so callers outside the provider must guard
with optional chaining; update the refreshToken signature where necessary to
Promise<TokenResponse | undefined> to keep types consistent and reference
AsgardeoContextProps and the refreshToken field when making the change.
In `@packages/nextjs/src/server/actions/clearSession.ts`:
- Around line 21-48: Remove the unstable internal type import
ReadonlyRequestCookies from next/dist/* and stop annotating cookieStore with it;
instead let TypeScript infer the cookies() return type in clearSession (replace
"const cookieStore: ReadonlyRequestCookies = await cookies();" with an untyped
"const cookieStore = await cookies();" and remove the ReadonlyRequestCookies
import). Keep the call to SessionManager.getSessionCookieName() and
cookieStore.delete(...) unchanged. Apply the same pattern to other server action
files that import next/dist/* types.
In `@packages/nextjs/src/server/actions/handleOAuthCallbackAction.ts`:
- Around line 152-157: The outer catch in handleOAuthCallbackAction currently
returns the error without logging; update it to log the caught error via
logger.error before returning (mirror signInAction.ts and the inner catch at
lines 138-143), ensuring you pass the Error object or message (use error
instanceof Error ? error : new Error(String(error))) to logger.error so
diagnostics aren’t lost when handleOAuthCallbackAction fails.
In `@packages/nextjs/src/utils/tokenRefreshCore.ts`:
- Around line 64-65: The code currently uses fallbacks client_id: clientId ?? ''
and client_secret: clientSecret ?? '' which masks missing required credentials;
remove the "?? ''" fallbacks and instead pass clientId and clientSecret directly
(or add an explicit runtime check) so TokenRefreshCoreConfig's required string
properties (clientId, clientSecret) fail fast and surface misconfiguration
rather than sending empty credentials; update the block that builds the token
POST payload (client_id/client_secret) in tokenRefreshCore.ts accordingly and
add an assertion or throw if clientId/clientSecret are falsy.
- Around line 62-99: The refresh logic should avoid double slashes in
tokenEndpoint, surface OAuth error details on non-OK responses, and validate
presence of access_token before continuing: construct tokenEndpoint by joining
baseUrl and '/oauth2/token' without duplicating slashes (adjust how
tokenEndpoint is built), when response.ok is false read response.text()/json()
and include tokenData.error and tokenData.error_description (or raw body) in the
thrown Error to distinguish invalid_grant vs invalid_client, and after parsing
tokenData ensure tokenData['access_token'] exists and is a non-empty string
before assigning to newAccessToken (throw a clear error if missing) while
keeping the rotated refresh token fallback for newRefreshToken.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 102070bb-2c48-4b53-9eee-179f2bff0f05
📒 Files selected for processing (20)
packages/javascript/esbuild.config.mjspackages/javascript/package.jsonpackages/nextjs/esbuild.config.mjspackages/nextjs/package.jsonpackages/nextjs/src/client/contexts/Asgardeo/AsgardeoContext.tspackages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsxpackages/nextjs/src/middleware.tspackages/nextjs/src/models/config.tspackages/nextjs/src/server/AsgardeoProvider.tsxpackages/nextjs/src/server/actions/clearSession.tspackages/nextjs/src/server/actions/handleOAuthCallbackAction.tspackages/nextjs/src/server/actions/isSignedIn.tspackages/nextjs/src/server/actions/refreshToken.tspackages/nextjs/src/server/actions/signInAction.tspackages/nextjs/src/server/actions/switchOrganization.tspackages/nextjs/src/server/index.tspackages/nextjs/src/server/middleware/asgardeoMiddleware.tspackages/nextjs/src/utils/SessionManager.tspackages/nextjs/src/utils/constants.tspackages/nextjs/src/utils/tokenRefreshCore.ts
💤 Files with no reviewable changes (1)
- packages/nextjs/src/server/index.ts
Comment on lines
116
to
139
| static async createSessionToken( | ||
| accessToken: string, | ||
| userId: string, | ||
| sessionId: string, | ||
| scopes: string, | ||
| accessTokenTtlSeconds: number, | ||
| refreshToken: string, | ||
| organizationId?: string, | ||
| expirySeconds: number = this.DEFAULT_EXPIRY_SECONDS, | ||
| ): Promise<string> { | ||
| const secret: Uint8Array = this.getSecret(); | ||
|
|
||
| const jwt: string = await new SignJWT({ | ||
| accessToken, | ||
| organizationId, | ||
| refreshToken, | ||
| scopes, | ||
| sessionId, | ||
| type: 'session', | ||
| } as Omit<SessionTokenPayload, 'sub' | 'iat' | 'exp'>) | ||
| .setProtectedHeader({alg: 'HS256'}) | ||
| .setSubject(userId) | ||
| .setIssuedAt() | ||
| .setExpirationTime(Date.now() / 1000 + expirySeconds) | ||
| .setExpirationTime(Math.floor(Date.now() / 1000) + accessTokenTtlSeconds) | ||
| .sign(secret); |
There was a problem hiding this comment.
Separate session expiry from access-token expiry.
Line 138 makes the session JWT expire after accessTokenTtlSeconds, but cookies are now configured with the resolved session lifetime. That means a 24-hour session cookie can contain a JWT that fails jwtVerify after the access token’s TTL, making isSignedIn() report false unless middleware refreshes first.
Consider using JWT exp for the session lifetime and storing access-token expiry separately, e.g. accessTokenExpiresAt.
🔧 Sketch of the safer shape
static async createSessionToken(
accessToken: string,
userId: string,
sessionId: string,
scopes: string,
accessTokenTtlSeconds: number,
+ sessionTtlSeconds: number,
refreshToken: string,
organizationId?: string,
): Promise<string> {
+ const now: number = Math.floor(Date.now() / 1000);
const secret: Uint8Array = this.getSecret();
const jwt: string = await new SignJWT({
accessToken,
+ accessTokenExpiresAt: now + accessTokenTtlSeconds,
organizationId,
refreshToken,
scopes,
sessionId,
type: 'session',
} as Omit<SessionTokenPayload, 'sub' | 'iat' | 'exp'>)
.setProtectedHeader({alg: 'HS256'})
.setSubject(userId)
.setIssuedAt()
- .setExpirationTime(Math.floor(Date.now() / 1000) + accessTokenTtlSeconds)
+ .setExpirationTime(now + sessionTtlSeconds)
.sign(secret);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nextjs/src/utils/SessionManager.ts` around lines 116 - 139,
createSessionToken currently sets the JWT exp to accessTokenTtlSeconds causing
session tokens to expire with the access token; change createSessionToken so the
JWT expiration (setExpirationTime) uses the session lifetime instead and add a
separate payload field (e.g. accessTokenExpiresAt) that records the access token
expiry (Date.now()/seconds + accessTokenTtlSeconds) so callers can validate the
access token separately; update the SessionTokenPayload shape to include
accessTokenExpiresAt (and keep organizationId, refreshToken, scopes, sessionId,
type) and ensure jwtVerify checks the JWT exp for session lifetime while
access-token logic reads accessTokenExpiresAt.
…n management actions
…sh method and updating middleware to use it for token verification
…ession expiry logic
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nextjs/src/server/middleware/asgardeoMiddleware.ts (1)
56-93: Cookie-header helpers are correct but consider consolidating.
removeCookieFromHeaderandreplaceCookieInHeaderduplicate the same parse-split-trim logic. Safe for JWT values (base64url alphabet contains no;, and only the first=is treated as the name/value separator), and the empty-header case is handled by the append-path inreplaceCookieInHeader. Optional: extract a singleparseCookieParts(header)helper returning{name, rest}tuples to keep both paths in sync as the helper set grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/server/middleware/asgardeoMiddleware.ts` around lines 56 - 93, The two functions removeCookieFromHeader and replaceCookieInHeader duplicate the same split/trim/filter parsing; extract a single helper (e.g., parseCookieParts or getCookieParts) that takes the raw cookie header string and returns an array of objects like { name: string, part: string } or { name: string, value?: string, raw: string }, then update removeCookieFromHeader to filter by part.name and join raw/part strings, and update replaceCookieInHeader to map/update the matching part.name and append when not found; ensure the helper handles empty/falsey headers and only treats the first '=' as the name/value separator so existing behavior for JWTs is preserved.
🤖 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/nextjs/src/server/actions/handleOAuthCallbackAction.ts`:
- Around line 118-120: The expiresIn value from signInResult['expiresIn'] is not
validated and can be 0, negative, or non-numeric; update the code that assigns
expiresIn (currently using the fallback DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS) to
coerce to a Number and ensure it's a positive finite number, falling back to
DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS when it is NaN, <= 0, or not finite (mirror
the validation used in switchOrganization.ts); then proceed to use
SessionManager.resolveSessionExpiry and SessionManager.createSessionToken as
before.
---
Nitpick comments:
In `@packages/nextjs/src/server/middleware/asgardeoMiddleware.ts`:
- Around line 56-93: The two functions removeCookieFromHeader and
replaceCookieInHeader duplicate the same split/trim/filter parsing; extract a
single helper (e.g., parseCookieParts or getCookieParts) that takes the raw
cookie header string and returns an array of objects like { name: string, part:
string } or { name: string, value?: string, raw: string }, then update
removeCookieFromHeader to filter by part.name and join raw/part strings, and
update replaceCookieInHeader to map/update the matching part.name and append
when not found; ensure the helper handles empty/falsey headers and only treats
the first '=' as the name/value separator so existing behavior for JWTs is
preserved.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36b22587-45bc-480a-bb30-0d5a6b5c17f4
📒 Files selected for processing (15)
packages/nextjs/package.jsonpackages/nextjs/src/client/contexts/Asgardeo/AsgardeoContext.tspackages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsxpackages/nextjs/src/server/actions/clearSession.tspackages/nextjs/src/server/actions/getAccessToken.tspackages/nextjs/src/server/actions/getSessionId.tspackages/nextjs/src/server/actions/getSessionPayload.tspackages/nextjs/src/server/actions/handleOAuthCallbackAction.tspackages/nextjs/src/server/actions/refreshToken.tspackages/nextjs/src/server/actions/signInAction.tspackages/nextjs/src/server/actions/signOutAction.tspackages/nextjs/src/server/actions/switchOrganization.tspackages/nextjs/src/server/middleware/asgardeoMiddleware.tspackages/nextjs/src/utils/SessionManager.tspackages/nextjs/src/utils/constants.ts
✅ Files skipped from review due to trivial changes (3)
- packages/nextjs/src/server/actions/getSessionPayload.ts
- packages/nextjs/src/server/actions/getSessionId.ts
- packages/nextjs/src/server/actions/getAccessToken.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx
- packages/nextjs/src/utils/constants.ts
- packages/nextjs/package.json
- packages/nextjs/src/server/actions/clearSession.ts
- packages/nextjs/src/server/actions/signInAction.ts
- packages/nextjs/src/server/actions/refreshToken.ts
Comment on lines
+118
to
+120
| const expiresIn: number = | ||
| (signInResult['expiresIn'] as number | undefined) ?? DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS; | ||
| const sessionExpirySeconds: number = SessionManager.resolveSessionExpiry(config.sessionExpirySeconds); |
There was a problem hiding this comment.
Validate expiresIn is a positive number, mirroring the switchOrganization.ts fix.
?? only rescues null/undefined. If signInResult['expiresIn'] is 0, negative, or a non-numeric value that the cast silently tolerates (e.g. a numeric string, which becomes NaN when arithmetic is applied), the value flows unchecked into SessionManager.createSessionToken and produces a JWT exp of now + 0 (immediately expired) or NaN. switchOrganization.ts already guards against this at lines 57-59; apply the same shape here for consistency.
🛡️ Proposed fix
- const expiresIn: number =
- (signInResult['expiresIn'] as number | undefined) ?? DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS;
+ const rawExpiresIn: unknown = signInResult['expiresIn'];
+ const parsedExpiresIn: number =
+ typeof rawExpiresIn === 'number'
+ ? rawExpiresIn
+ : typeof rawExpiresIn === 'string'
+ ? parseInt(rawExpiresIn, 10)
+ : NaN;
+ const expiresIn: number =
+ Number.isFinite(parsedExpiresIn) && parsedExpiresIn > 0
+ ? parsedExpiresIn
+ : DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS;📝 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
| const expiresIn: number = | |
| (signInResult['expiresIn'] as number | undefined) ?? DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS; | |
| const sessionExpirySeconds: number = SessionManager.resolveSessionExpiry(config.sessionExpirySeconds); | |
| const rawExpiresIn: unknown = signInResult['expiresIn']; | |
| const parsedExpiresIn: number = | |
| typeof rawExpiresIn === 'number' | |
| ? rawExpiresIn | |
| : typeof rawExpiresIn === 'string' | |
| ? parseInt(rawExpiresIn, 10) | |
| : NaN; | |
| const expiresIn: number = | |
| Number.isFinite(parsedExpiresIn) && parsedExpiresIn > 0 | |
| ? parsedExpiresIn | |
| : DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS; | |
| const sessionExpirySeconds: number = SessionManager.resolveSessionExpiry(config.sessionExpirySeconds); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nextjs/src/server/actions/handleOAuthCallbackAction.ts` around lines
118 - 120, The expiresIn value from signInResult['expiresIn'] is not validated
and can be 0, negative, or non-numeric; update the code that assigns expiresIn
(currently using the fallback DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS) to coerce to
a Number and ensure it's a positive finite number, falling back to
DEFAULT_ACCESS_TOKEN_EXPIRY_SECONDS when it is NaN, <= 0, or not finite (mirror
the validation used in switchOrganization.ts); then proceed to use
SessionManager.resolveSessionExpiry and SessionManager.createSessionToken as
before.
… logic - Introduced session cookie expiry time in seconds to AsgardeoNodeConfig and AsgardeoNextConfig. - Implemented handleRefreshToken utility for managing OAuth refresh token logic. - Updated session management actions to utilize session cookie expiry time. - Refactored related actions and middleware to ensure consistent handling of session cookies.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/nextjs/src/server/actions/refreshToken.ts (2)
99-118:⚠️ Potential issue | 🟠 MajorTransient refresh failures still force sign-out (unchanged from prior review).
The catch still
cookieStore.delete(...)unconditionally for every failure path — including network errors, token-endpoint 5xx, and JSON parse errors fromhandleRefreshToken. Only terminal auth failures (e.g.invalid_grant, missing/invalid session) should clear the cookie; transient errors will otherwise turn a blip into a forced sign-out and discard a still-valid refresh token.Consider narrowing the deletion to terminal conditions. This requires
handleRefreshTokento preserve token-endpoint status codes / error codes (currently it throws a genericErroron HTTP failure atpackages/nextjs/src/utils/handleRefreshToken.ts:83) so the caller can distinguish retryable from terminal.Possible direction
- try { - const cookieStore: RequestCookies = await cookies(); - cookieStore.delete(SessionManager.getSessionCookieName()); - logger.debug('[refreshToken] Cleared session cookie after refresh failure.'); - } catch { - // Intentionally swallowed — middleware handles cleanup when mutation is blocked. - } + const isTerminal = + error instanceof AsgardeoAPIError && + (error.statusCode === 400 || error.statusCode === 401); + if (isTerminal) { + try { + const cookieStore: RequestCookies = await cookies(); + cookieStore.delete(SessionManager.getSessionCookieName()); + logger.debug('[refreshToken] Cleared session cookie after terminal refresh failure.'); + } catch { + // Intentionally swallowed — middleware handles cleanup when mutation is blocked. + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/server/actions/refreshToken.ts` around lines 99 - 118, The catch in refreshToken unconditionally deletes the session cookie on any refresh failure; change this so only terminal auth failures remove the cookie by (1) updating handleRefreshToken to throw a structured error that includes HTTP status and token-endpoint error details (e.g., statusCode and errorCode / error_description) instead of a generic Error, and (2) modifying the refreshToken catch to inspect that structured error from handleRefreshToken (or detect missing/invalid session) and only call cookieStore.delete(SessionManager.getSessionCookieName()) when the statusCode or errorCode indicates a terminal condition (e.g., 400/401 or invalid_grant/missing_session); for all other cases (network/5xx/parse errors) rethrow without clearing the cookie so transient failures don’t force sign-out.
21-26:⚠️ Potential issue | 🟡 MinorFix import order (pipeline warning).
ESLint
import/orderflagged that../../utils/handleRefreshTokenshould sort before../../utils/SessionManager.Proposed fix
-import SessionManager, {SessionTokenPayload} from '../../utils/SessionManager'; -import handleRefreshToken, {HandleRefreshTokenResult} from '../../utils/handleRefreshToken'; +import handleRefreshToken, {HandleRefreshTokenResult} from '../../utils/handleRefreshToken'; +import SessionManager, {SessionTokenPayload} from '../../utils/SessionManager';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/server/actions/refreshToken.ts` around lines 21 - 26, ESLint import/order complaint: the relative import for handleRefreshToken should come before SessionManager; reorder the imports so ../../utils/handleRefreshToken (the named export handleRefreshToken and HandleRefreshTokenResult) appears before ../../utils/SessionManager (and its SessionManager and SessionTokenPayload exports) to satisfy the linter and keep imports alphabetically/section-wise sorted.packages/nextjs/src/utils/SessionManager.ts (1)
118-144:⚠️ Potential issue | 🟠 MajorJWT
expis still tied to access-token TTL (unchanged from prior review).
setExpirationTime(Math.floor(Date.now()/1000) + accessTokenTtlSeconds)+ the docstring on line 27 (exp — doubles as the access token expiry) mean a 24-hour session cookie can contain a JWT thatjwtVerifyrejects as soon as the access token expires (typically ~1 hour). That's exactly the trap the PR objective — align session cookie expiry with OIDC token/session validity, consider refresh tokens — is trying to avoid.Concrete consequences visible elsewhere in this PR:
- Any caller of
verifySessionTokenbetween access-token expiry and the next middleware refresh (e.g.isSignedIn()from a Server Component, a Route Handler without middleware matching, RSC rendering invoked via navigation that bypasses middleware for a prefetch) will get a thrownAsgardeoRuntimeErrorand report the user as signed-out.- You already had to introduce
verifySessionTokenForRefreshspecifically to work around this — two verification paths with different semantics is the smell.Prefer a single authoritative session lifetime on JWT
expand carry access-token expiry as a separate claim (accessTokenExpiresAt) thatisSignedIn/ refresh triggers read explicitly. This also collapses the two verify methods back into one.Suggested shape
export interface SessionTokenPayload extends JWTPayload { - /** Expiration timestamp — doubles as the access token expiry (JWT exp == access token exp) */ - exp: number; + /** JWT exp — bounds the session lifetime (matches cookie maxAge). */ + exp: number; + /** Access token expiry (epoch seconds). Trigger refresh when now >= this. */ + accessTokenExpiresAt: number; ... } static async createSessionToken( accessToken: string, userId: string, sessionId: string, scopes: string, accessTokenTtlSeconds: number, + sessionTtlSeconds: number, refreshToken: string, organizationId?: string, ): Promise<string> { + const now = Math.floor(Date.now() / 1000); ... const jwt = await new SignJWT({ accessToken, + accessTokenExpiresAt: now + accessTokenTtlSeconds, organizationId, refreshToken, scopes, sessionId, type: 'session', } as Omit<SessionTokenPayload, 'sub' | 'iat' | 'exp'>) .setProtectedHeader({alg: 'HS256'}) .setSubject(userId) .setIssuedAt() - .setExpirationTime(Math.floor(Date.now() / 1000) + accessTokenTtlSeconds) + .setExpirationTime(now + sessionTtlSeconds) .sign(secret);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/utils/SessionManager.ts` around lines 118 - 144, createSessionToken currently sets JWT exp to accessTokenTtlSeconds which causes the session JWT to expire when the access token does; change createSessionToken to set the JWT exp to the session lifetime (pass in or derive a sessionTtlSeconds) and include a separate claim accessTokenExpiresAt (timestamp) containing the access token expiry; update the SessionTokenPayload shape to include accessTokenExpiresAt, adjust any callers that construct the token to provide sessionTtlSeconds, and modify verifySessionToken to treat exp as the authoritative session validity while reading accessTokenExpiresAt to decide whether a token refresh is needed (and remove/merge verifySessionTokenForRefresh accordingly).
🧹 Nitpick comments (2)
packages/nextjs/src/utils/handleRefreshToken.ts (1)
70-75: No timeout on the token-endpoint fetch.
fetch(tokenEndpoint, ...)has no signal/timeout, so a hung IdP will stall the calling request indefinitely. This matters specifically becausehandleRefreshTokenis invoked fromasgardeoMiddleware(Edge runtime, runs on every matched request) and from a client-invoked server action — either one hanging blocks the user.Proposed fix
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10_000); try { response = await fetch(tokenEndpoint, { body: body.toString(), headers: {'Content-Type': 'application/x-www-form-urlencoded'}, method: 'POST', + signal: controller.signal, }); + } finally { + clearTimeout(timeoutId); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/utils/handleRefreshToken.ts` around lines 70 - 75, handleRefreshToken currently calls fetch(tokenEndpoint, ...) without a timeout which can hang; modify the function (handleRefreshToken) to create an AbortController, set a short configurable timeout (e.g., 5–10s) via setTimeout that calls controller.abort(), pass controller.signal into the fetch options, and ensure you clearTimeout after the fetch completes or errors so you don't leak timers; update any callers or tests if they rely on unlimited waits and handle AbortError cases where response is aborted.packages/nextjs/src/utils/decorateConfigWithNextEnv.ts (1)
47-51: Consider validating the parsed env value to avoidNaNin config.If
ASGARDEO_SESSION_COOKIE_EXPIRY_TIMEis set to a non-numeric string,parseIntreturnsNaNand that flows intoconfig.sessionCookieExpiryTime.SessionManager.resolveSessionCookieExpirydoes guard with> 0and re-reads the env var, so there is no user-visible bug today, but the decorated config becomingNaNis a footgun for any other consumer reading the field directly.Suggested tightening
sessionCookieExpiryTime: sessionCookieExpiryTime || - (process.env['ASGARDEO_SESSION_COOKIE_EXPIRY_TIME'] - ? parseInt(process.env['ASGARDEO_SESSION_COOKIE_EXPIRY_TIME'], 10) - : undefined), + (() => { + const raw: string | undefined = process.env['ASGARDEO_SESSION_COOKIE_EXPIRY_TIME']; + if (!raw) return undefined; + const parsed: number = parseInt(raw, 10); + return Number.isFinite(parsed) && parsed > 0 ? parsed : undefined; + })(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/utils/decorateConfigWithNextEnv.ts` around lines 47 - 51, The config currently assigns sessionCookieExpiryTime from parseInt(process.env['ASGARDEO_SESSION_COOKIE_EXPIRY_TIME'], 10) which can produce NaN; update the logic in decorateConfigWithNextEnv so you parse the env var into a number, validate it (e.g., Number.isFinite or Number.isInteger and > 0) and only set config.sessionCookieExpiryTime when the parsed value is a valid positive integer, otherwise fall back to undefined (or the existing sessionCookieExpiryTime param); reference the sessionCookieExpiryTime field and the ASGARDEO_SESSION_COOKIE_EXPIRY_TIME env lookup when making this change.
🤖 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/nextjs/src/server/actions/signInAction.ts`:
- Around line 122-135: The expiresIn handling needs defensive fallback and
numeric conversion: when extracting expiresIn from signInResult, check both
'expiresIn' and 'expires_in' (mirroring the idToken fallback) and convert to a
number using Number(...) and validate (e.g., fallback to 0 or throw if NaN)
before passing to SessionManager.createSessionToken; replace the current cast
(as number) with this conversion and validation for the symbol expiresIn from
signInResult, and ensure the subsequent SessionManager.createSessionToken calls
receive a proper number. Also wrap the statement that assigns
sessionCookieExpiryTime
(SessionManager.resolveSessionCookieExpiry(config.sessionCookieExpiryTime)) so
it conforms to prettier print width.
In `@packages/nextjs/src/utils/handleRefreshToken.ts`:
- Around line 60-66: The code is silently sending empty client_id/client_secret
due to the `clientId ?? ''` and `clientSecret ?? ''` fallbacks; remove those
fallbacks and instead validate the required inputs (clientId and clientSecret)
up front in the handleRefreshToken logic (e.g., at the start of the function
that builds tokenEndpoint and the URLSearchParams `body`), throwing a clear
error if either is missing or empty so we fail fast before posting to
`/oauth2/token` when constructing `body` (the URLSearchParams used with
tokenEndpoint).
- Around line 68-92: The current token refresh flow in handleRefreshToken.ts
throws plain Errors and discards the token endpoint's response body, preventing
callers (like refreshToken server action and asgardeoMiddleware) from
distinguishing terminal 4xx auth failures from transient errors; change the
error handling so that when fetch fails or response.ok is false you throw an
error object that preserves the HTTP status and the parsed response body (e.g.,
include properties status and body on the thrown error), and when JSON parsing
of the success/failure body occurs ensure tokenData is captured and attached to
the thrown error (use tokenEndpoint, response, tokenData, fetchError as anchors)
so callers can inspect error.status and error.body to decide whether to clear
cookies or retry.
---
Duplicate comments:
In `@packages/nextjs/src/server/actions/refreshToken.ts`:
- Around line 99-118: The catch in refreshToken unconditionally deletes the
session cookie on any refresh failure; change this so only terminal auth
failures remove the cookie by (1) updating handleRefreshToken to throw a
structured error that includes HTTP status and token-endpoint error details
(e.g., statusCode and errorCode / error_description) instead of a generic Error,
and (2) modifying the refreshToken catch to inspect that structured error from
handleRefreshToken (or detect missing/invalid session) and only call
cookieStore.delete(SessionManager.getSessionCookieName()) when the statusCode or
errorCode indicates a terminal condition (e.g., 400/401 or
invalid_grant/missing_session); for all other cases (network/5xx/parse errors)
rethrow without clearing the cookie so transient failures don’t force sign-out.
- Around line 21-26: ESLint import/order complaint: the relative import for
handleRefreshToken should come before SessionManager; reorder the imports so
../../utils/handleRefreshToken (the named export handleRefreshToken and
HandleRefreshTokenResult) appears before ../../utils/SessionManager (and its
SessionManager and SessionTokenPayload exports) to satisfy the linter and keep
imports alphabetically/section-wise sorted.
In `@packages/nextjs/src/utils/SessionManager.ts`:
- Around line 118-144: createSessionToken currently sets JWT exp to
accessTokenTtlSeconds which causes the session JWT to expire when the access
token does; change createSessionToken to set the JWT exp to the session lifetime
(pass in or derive a sessionTtlSeconds) and include a separate claim
accessTokenExpiresAt (timestamp) containing the access token expiry; update the
SessionTokenPayload shape to include accessTokenExpiresAt, adjust any callers
that construct the token to provide sessionTtlSeconds, and modify
verifySessionToken to treat exp as the authoritative session validity while
reading accessTokenExpiresAt to decide whether a token refresh is needed (and
remove/merge verifySessionTokenForRefresh accordingly).
---
Nitpick comments:
In `@packages/nextjs/src/utils/decorateConfigWithNextEnv.ts`:
- Around line 47-51: The config currently assigns sessionCookieExpiryTime from
parseInt(process.env['ASGARDEO_SESSION_COOKIE_EXPIRY_TIME'], 10) which can
produce NaN; update the logic in decorateConfigWithNextEnv so you parse the env
var into a number, validate it (e.g., Number.isFinite or Number.isInteger and >
0) and only set config.sessionCookieExpiryTime when the parsed value is a valid
positive integer, otherwise fall back to undefined (or the existing
sessionCookieExpiryTime param); reference the sessionCookieExpiryTime field and
the ASGARDEO_SESSION_COOKIE_EXPIRY_TIME env lookup when making this change.
In `@packages/nextjs/src/utils/handleRefreshToken.ts`:
- Around line 70-75: handleRefreshToken currently calls fetch(tokenEndpoint,
...) without a timeout which can hang; modify the function (handleRefreshToken)
to create an AbortController, set a short configurable timeout (e.g., 5–10s) via
setTimeout that calls controller.abort(), pass controller.signal into the fetch
options, and ensure you clearTimeout after the fetch completes or errors so you
don't leak timers; update any callers or tests if they rely on unlimited waits
and handle AbortError cases where response is aborted.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 587b6cf4-0e77-453b-a042-6eb4f6673e37
📒 Files selected for processing (11)
packages/nextjs/src/constants/sessionConstants.tspackages/nextjs/src/server/AsgardeoProvider.tsxpackages/nextjs/src/server/actions/handleOAuthCallbackAction.tspackages/nextjs/src/server/actions/refreshToken.tspackages/nextjs/src/server/actions/signInAction.tspackages/nextjs/src/server/actions/switchOrganization.tspackages/nextjs/src/server/middleware/asgardeoMiddleware.tspackages/nextjs/src/utils/SessionManager.tspackages/nextjs/src/utils/decorateConfigWithNextEnv.tspackages/nextjs/src/utils/handleRefreshToken.tspackages/node/src/models/config.ts
✅ Files skipped from review due to trivial changes (2)
- packages/node/src/models/config.ts
- packages/nextjs/src/constants/sessionConstants.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/nextjs/src/server/actions/handleOAuthCallbackAction.ts
- packages/nextjs/src/server/actions/switchOrganization.ts
- packages/nextjs/src/server/AsgardeoProvider.tsx
- packages/nextjs/src/server/middleware/asgardeoMiddleware.ts
Comment on lines
+60
to
+66
| const tokenEndpoint: string = `${baseUrl}/oauth2/token`; | ||
| const body: URLSearchParams = new URLSearchParams({ | ||
| client_id: clientId ?? '', | ||
| client_secret: clientSecret ?? '', | ||
| grant_type: 'refresh_token', | ||
| refresh_token: storedRefreshToken, | ||
| }); |
There was a problem hiding this comment.
Empty-string fallback for clientId/clientSecret will POST invalid credentials.
clientId ?? '' / clientSecret ?? '' silently send empty credentials to /oauth2/token, which the IdP will reject with an opaque invalid_client instead of failing fast at the SDK boundary. Given HandleRefreshTokenConfig types both as required string, the ?? fallback is both unnecessary and masking.
Proposed fix
- const body: URLSearchParams = new URLSearchParams({
- client_id: clientId ?? '',
- client_secret: clientSecret ?? '',
- grant_type: 'refresh_token',
- refresh_token: storedRefreshToken,
- });
+ if (!clientId || !clientSecret || !baseUrl) {
+ throw new Error('handleRefreshToken: baseUrl, clientId and clientSecret are required.');
+ }
+ const body: URLSearchParams = new URLSearchParams({
+ client_id: clientId,
+ client_secret: clientSecret,
+ grant_type: 'refresh_token',
+ refresh_token: storedRefreshToken,
+ });📝 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
| const tokenEndpoint: string = `${baseUrl}/oauth2/token`; | |
| const body: URLSearchParams = new URLSearchParams({ | |
| client_id: clientId ?? '', | |
| client_secret: clientSecret ?? '', | |
| grant_type: 'refresh_token', | |
| refresh_token: storedRefreshToken, | |
| }); | |
| const tokenEndpoint: string = `${baseUrl}/oauth2/token`; | |
| if (!clientId || !clientSecret || !baseUrl) { | |
| throw new Error('handleRefreshToken: baseUrl, clientId and clientSecret are required.'); | |
| } | |
| const body: URLSearchParams = new URLSearchParams({ | |
| client_id: clientId, | |
| client_secret: clientSecret, | |
| grant_type: 'refresh_token', | |
| refresh_token: storedRefreshToken, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nextjs/src/utils/handleRefreshToken.ts` around lines 60 - 66, The
code is silently sending empty client_id/client_secret due to the `clientId ??
''` and `clientSecret ?? ''` fallbacks; remove those fallbacks and instead
validate the required inputs (clientId and clientSecret) up front in the
handleRefreshToken logic (e.g., at the start of the function that builds
tokenEndpoint and the URLSearchParams `body`), throwing a clear error if either
is missing or empty so we fail fast before posting to `/oauth2/token` when
constructing `body` (the URLSearchParams used with tokenEndpoint).
Comment on lines
+68
to
+92
| let response: Response; | ||
|
|
||
| try { | ||
| response = await fetch(tokenEndpoint, { | ||
| body: body.toString(), | ||
| headers: {'Content-Type': 'application/x-www-form-urlencoded'}, | ||
| method: 'POST', | ||
| }); | ||
| } catch (fetchError) { | ||
| throw new Error( | ||
| `Token refresh network error: ${fetchError instanceof Error ? fetchError.message : String(fetchError)}`, | ||
| ); | ||
| } | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Token endpoint rejected refresh (HTTP ${response.status}).`); | ||
| } | ||
|
|
||
| let tokenData: Record<string, unknown>; | ||
|
|
||
| try { | ||
| tokenData = (await response.json()) as Record<string, unknown>; | ||
| } catch { | ||
| throw new Error('Failed to parse token endpoint response as JSON.'); | ||
| } |
There was a problem hiding this comment.
Preserve token-endpoint status and error body on failure.
refreshToken server action and asgardeoMiddleware both want to distinguish terminal auth failures (e.g. invalid_grant → clear cookie, sign out) from transient ones (network / 5xx → keep the session and retry later). As written, both failure paths throw plain Errors carrying only a stringified message, so callers cannot discriminate — this is why packages/nextjs/src/server/actions/refreshToken.ts currently deletes the session cookie on every failure.
Also, when the IdP returns a 4xx with an invalid_grant/invalid_client body, that body is silently discarded, making production debugging harder.
Proposed shape
+ class RefreshTokenHttpError extends Error {
+ constructor(message: string, readonly status: number, readonly oauthError?: string, readonly isTransient: boolean = false) {
+ super(message);
+ }
+ }
...
- } catch (fetchError) {
- throw new Error(
- `Token refresh network error: ${fetchError instanceof Error ? fetchError.message : String(fetchError)}`,
- );
- }
+ } catch (fetchError) {
+ throw new RefreshTokenHttpError(
+ `Token refresh network error: ${fetchError instanceof Error ? fetchError.message : String(fetchError)}`,
+ 0,
+ undefined,
+ true,
+ );
+ }
- if (!response.ok) {
- throw new Error(`Token endpoint rejected refresh (HTTP ${response.status}).`);
- }
+ if (!response.ok) {
+ let oauthError: string | undefined;
+ try {
+ const errBody = (await response.json()) as {error?: string};
+ oauthError = errBody?.error;
+ } catch {
+ /* body not JSON */
+ }
+ throw new RefreshTokenHttpError(
+ `Token endpoint rejected refresh (HTTP ${response.status}${oauthError ? `, ${oauthError}` : ''}).`,
+ response.status,
+ oauthError,
+ response.status >= 500,
+ );
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nextjs/src/utils/handleRefreshToken.ts` around lines 68 - 92, The
current token refresh flow in handleRefreshToken.ts throws plain Errors and
discards the token endpoint's response body, preventing callers (like
refreshToken server action and asgardeoMiddleware) from distinguishing terminal
4xx auth failures from transient errors; change the error handling so that when
fetch fails or response.ok is false you throw an error object that preserves the
HTTP status and the parsed response body (e.g., include properties status and
body on the thrown error), and when JSON parsing of the success/failure body
occurs ensure tokenData is captured and attached to the thrown error (use
tokenEndpoint, response, tokenData, fetchError as anchors) so callers can
inspect error.status and error.body to decide whether to clear cookies or retry.
| const jwt: string = await new SignJWT({ | ||
| accessToken, | ||
| organizationId, | ||
| refreshToken, |
Contributor
There was a problem hiding this comment.
The refresh token is now embedded in the session JWT cookie payload. Is this an intentional design decision? This will make the refresh token travels with every single HTTP request to the server, increasing exposure surface
Contributor
Author
There was a problem hiding this comment.
Yeah, this is intentional because currently there is no dedicated store to keep the refreshToken. When there is support for a dedicated store,we can move refreshToken to there
| sessionId, | ||
| ); | ||
|
|
||
| const config: AsgardeoNextConfig = await (asgardeoClient.getConfiguration() as unknown as Promise<AsgardeoNextConfig>); |
Contributor
There was a problem hiding this comment.
Can we fix the type here? The as unknown as pattern hides real type mismatches. Check in other places as well
Contributor
Author
There was a problem hiding this comment.
Real type issue in javascript layer. That refactoring may need some changes in multiple sdks like react, vue, node. So I removed that unnecessary type cast in nextjs package instead totally fixing the root cause
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nextjs/src/server/actions/signInAction.ts (1)
127-135:⚠️ Potential issue | 🟠 MajorUnresolved:
expiresInstill cast as number, prettier wrap still missing, andrefreshToken/accessTokenlack snake_case fallbacks.The previously agreed-upon fix for
expiresInhas not been applied in this revision:
- Line 133 still uses
signInResult['expiresIn'] as number. PerTokenResponsein@asgardeo/node,expiresInis typed asstring; the cast bypasses conversion and will propagate a string intoSessionManager.createSessionToken(→setExpirationTime(Math.floor(Date.now() / 1000) + "3600")yields"17..."3600", i.e., a garbageexp) and intogetSessionCookieOptions(maxAge)where it becomes the cookiemaxAge. Also missing theexpires_insnake_case fallback that mirrors line 124.- Line 135 still exceeds prettier's print width (previously flagged by the PR Builder pipeline).
- For consistency with the
idToken/id_tokenfallback on line 124,refreshToken(line 128) andaccessToken(line 127) should also tolerate snake_case properties coming back from the underlying SDK response.Proposed fix
- const {accessToken}: {accessToken: string} = signInResult as {accessToken: string}; - const refreshToken: string = (signInResult['refreshToken'] as string | undefined) ?? ''; + const accessToken: string = (signInResult['accessToken'] ?? signInResult['access_token']) as string; + const refreshToken: string = + ((signInResult['refreshToken'] ?? signInResult['refresh_token']) as string | undefined) ?? ''; const scopes: string = signInResult['scope'] as string; const organizationId: string | undefined = (idToken['user_org'] || idToken['organization_id']) as | string | undefined; - const expiresIn: number = signInResult['expiresIn'] as number; + const expiresIn: number = Number(signInResult['expiresIn'] ?? signInResult['expires_in']); + if (!Number.isFinite(expiresIn) || expiresIn <= 0) { + throw new Error('Sign-in response did not include a valid expires_in value.'); + } const config: AsgardeoNextConfig = await client.getConfiguration(); - const sessionCookieExpiryTime: number = SessionManager.resolveSessionCookieExpiry(config.sessionCookieExpiryTime); + const sessionCookieExpiryTime: number = SessionManager.resolveSessionCookieExpiry( + config.sessionCookieExpiryTime, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/server/actions/signInAction.ts` around lines 127 - 135, The code reads tokens and expiry incorrectly from signInResult: update accessToken and refreshToken to fall back to snake_case keys (use signInResult['accessToken'] ?? signInResult['access_token'] and signInResult['refreshToken'] ?? signInResult['refresh_token']), and read expiresIn tolerant of snake_case as well (use signInResult['expiresIn'] ?? signInResult['expires_in']), then convert that value to a number (Number(...) or parseInt(..., 10)) before passing into SessionManager.createSessionToken/getSessionCookieOptions to avoid string propagation; also reflow the long organizationId assignment (idToken['user_org'] || idToken['organization_id']) into multiple lines to satisfy prettier/print-width.
🧹 Nitpick comments (1)
packages/nextjs/src/server/actions/refreshToken.ts (1)
112-117: Status-code propagation is narrow.Only errors that are already
AsgardeoAPIErrorcarry astatusCodeforward. SincehandleRefreshTokenthrows plainErrorfor network failures and non-OK token-endpoint responses, callers of this action always seestatusCode === undefinedfor those cases and cannot distinguish 401/invalid_grantfrom a transient 5xx. This also undermines the terminal-vs-transient gating suggested for the cookie-clearing branch above. Consider upgradinghandleRefreshTokento throwAsgardeoAPIErrorwith the HTTP status and error code from the token endpoint, and forwarding that here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/server/actions/refreshToken.ts` around lines 112 - 117, The thrown AsgardeoAPIError in refreshToken.ts is losing HTTP status and token-endpoint error details because handleRefreshToken currently throws plain Errors; update handleRefreshToken to throw AsgardeoAPIError instances that include the token endpoint HTTP status and error code (e.g., from token response body and response.status) on non-OK responses and network failures, and then forward those fields here when re-throwing (i.e., preserve error instanceof AsgardeoAPIError ? error.statusCode and any errorCode/error_description fields). Locate handleRefreshToken and the catch that currently constructs new AsgardeoAPIError in refreshToken.ts and ensure the original status and token error info are propagated into the new AsgardeoAPIError so callers can distinguish 401/invalid_grant vs transient 5xx.
🤖 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/nextjs/src/server/actions/refreshToken.ts`:
- Around line 94-95: handleRefreshToken currently parses
result.tokenResponse.expiresIn into expiresInSeconds and computes expiresAt,
which can become NaN if expiresIn is missing or "undefined"; update the logic in
handleRefreshToken to validate and default the expiresIn value before computing
expiresAt: check result.tokenResponse.expiresIn (and the parsed number) and if
it's missing or parseInt yields NaN, use a sensible fallback (e.g., 0 or a
configured default TTL) or omit/clear expiresAt so it never becomes NaN; ensure
you update the handling around the expiresInSeconds and expiresAt variables to
reflect the fallback and return a safe, serializable value.
---
Duplicate comments:
In `@packages/nextjs/src/server/actions/signInAction.ts`:
- Around line 127-135: The code reads tokens and expiry incorrectly from
signInResult: update accessToken and refreshToken to fall back to snake_case
keys (use signInResult['accessToken'] ?? signInResult['access_token'] and
signInResult['refreshToken'] ?? signInResult['refresh_token']), and read
expiresIn tolerant of snake_case as well (use signInResult['expiresIn'] ??
signInResult['expires_in']), then convert that value to a number (Number(...) or
parseInt(..., 10)) before passing into
SessionManager.createSessionToken/getSessionCookieOptions to avoid string
propagation; also reflow the long organizationId assignment (idToken['user_org']
|| idToken['organization_id']) into multiple lines to satisfy
prettier/print-width.
---
Nitpick comments:
In `@packages/nextjs/src/server/actions/refreshToken.ts`:
- Around line 112-117: The thrown AsgardeoAPIError in refreshToken.ts is losing
HTTP status and token-endpoint error details because handleRefreshToken
currently throws plain Errors; update handleRefreshToken to throw
AsgardeoAPIError instances that include the token endpoint HTTP status and error
code (e.g., from token response body and response.status) on non-OK responses
and network failures, and then forward those fields here when re-throwing (i.e.,
preserve error instanceof AsgardeoAPIError ? error.statusCode and any
errorCode/error_description fields). Locate handleRefreshToken and the catch
that currently constructs new AsgardeoAPIError in refreshToken.ts and ensure the
original status and token error info are propagated into the new
AsgardeoAPIError so callers can distinguish 401/invalid_grant vs transient 5xx.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72e6151a-cda6-43f5-91a8-a5347832c81b
📒 Files selected for processing (4)
packages/nextjs/src/server/actions/handleOAuthCallbackAction.tspackages/nextjs/src/server/actions/refreshToken.tspackages/nextjs/src/server/actions/signInAction.tspackages/nextjs/src/server/actions/switchOrganization.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nextjs/src/server/actions/switchOrganization.ts
- packages/nextjs/src/server/actions/handleOAuthCallbackAction.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx (2)
296-313:⚠️ Potential issue | 🟡 Minor
useMemodependency list omitsclearSessionandrefreshToken.The memoized
contextValuenow includesclearSessionandrefreshToken, but neither is listed in the dependency array on line 312. If the parent ever passes new function identities for these props (e.g. after a re-render where they aren't stable references), consumers will keep receiving the stale closures. Add them to the deps (or document that they are guaranteed stable server-action references).🛠️ Proposed fix
- [baseUrl, user, isSignedIn, isLoading, signInUrl, signUpUrl, applicationId, organizationHandle], + [ + baseUrl, + user, + isSignedIn, + isLoading, + signInUrl, + signUpUrl, + applicationId, + organizationHandle, + clearSession, + refreshToken, + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx` around lines 296 - 313, The memoized contextValue created by useMemo (returning an AsgardeoContextProps object) includes clearSession and refreshToken but they are missing from the dependency array; update the useMemo dependency list to include clearSession and refreshToken so contextValue updates when those function/value identities change, ensuring consumers don't receive stale closures.
46-82:⚠️ Potential issue | 🟡 MinorCI failing on Prettier formatting.
The PR Builder is failing
prettier/prettieron lines 46–51, 69–82, 163, and 275 (spacing inside type literals / call arguments introduced by this diff). Runpnpm prettier --write(or the repo's lint:fix script) on this file before merge — no logic change needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx` around lines 46 - 82, Prettier formatting errors were introduced in the AsgardeoProvider Types (not logic), e.g. spacing inside the AsgardeoClientProviderProps type literal and in function signatures like handleOAuthCallback and updateProfile; run the repository formatter (pnpm prettier --write or the lint:fix script) on packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx and remove the extra spaces inside type literals/call argument lists so the exported AsgardeoClientProviderProps, handleOAuthCallback, updateProfile, and other signatures match the project's Prettier rules.
♻️ Duplicate comments (1)
packages/nextjs/src/server/actions/signInAction.ts (1)
133-152:⚠️ Potential issue | 🟠 MajorNormalize
expiresInand align the session JWT lifetime with the cookie.Line 133 still trusts
expiresInas a number and does not checkexpires_in. Also, Line 144 uses the access-token TTL as the session JWTexp, while Line 152 keeps the cookie forsessionCookieExpiryTime; with a 1h access token and 24h cookie, the cookie can outlive an unverifiable session JWT, preventing refresh recovery. Split the access-token TTL from the session JWT TTL, or cap the cookie lifetime to the JWT lifetime.Verify the response shape and session lifetime contract:
#!/bin/bash # Description: Inspect token expiry field names/types and how session JWT expiration is computed. # Expectation: # - Token response expiry may require `expiresIn`/`expires_in` fallback plus Number(...) conversion. # - Session JWT expiration should not unintentionally expire before the configured session cookie lifetime. rg -nP --type=ts -C4 'expires[_]?[Ii]n|interface\s+TokenResponse|type\s+TokenResponse' packages/javascript/src packages/node/src packages/nextjs/src sed -n '118,144p' packages/nextjs/src/utils/SessionManager.ts rg -nP --type=ts -C4 'createSessionToken\(|verifySessionToken\(' packages/nextjs/src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nextjs/src/server/actions/signInAction.ts` around lines 133 - 152, Normalize the token expiry by checking both expiresIn and expires_in on signInResult and coercing to a Number (e.g., const tokenTtl = Number(signInResult.expiresIn ?? signInResult.expires_in ?? 0)); then compute a separate session JWT TTL (sessionJwtTtl) that is either derived from tokenTtl or from the configured sessionCookieExpiryTime but ensure session JWT expiration is never shorter than what the cookie will allow—pass sessionJwtTtl to SessionManager.createSessionToken instead of access-token TTL, and when calling cookieStore.set with SessionManager.getSessionCookieOptions(sessionCookieExpiryTime) cap or align sessionCookieExpiryTime to be <= sessionJwtTtl (or vice versa per your policy) so the cookie cannot outlive a verifiable session JWT; update references around signInResult, SessionManager.createSessionToken, SessionManager.getSessionCookieOptions, and SessionManager.resolveSessionCookieExpiry accordingly.
🤖 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/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx`:
- Line 51: The client module AsgardeoProvider.tsx currently imports
RefreshResult with a value import from ../../../server/actions/refreshToken;
change this to a type-only import so the server-action module is not pulled into
the client bundle: replace the import with a type import for RefreshResult
(e.g., use import type { RefreshResult } ...) in AsgardeoProvider.tsx to ensure
the symbol is erased at compile time and preserves the 'use client' boundary.
In `@packages/nextjs/src/server/actions/signInAction.ts`:
- Line 128: The code in signInAction.ts reads only signInResult['refreshToken']
when building the refreshToken variable (const refreshToken) which misses OAuth
responses that use refresh_token; update the extraction to check both keys
(e.g., signInResult['refreshToken'] || signInResult['refresh_token']) and coerce
to string/empty default like the existing accessToken handling; apply the
identical change in handleOAuthCallbackAction.ts where the same refreshToken
extraction occurs so both code paths consistently accept refreshToken or
refresh_token from signInResult.
---
Outside diff comments:
In `@packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx`:
- Around line 296-313: The memoized contextValue created by useMemo (returning
an AsgardeoContextProps object) includes clearSession and refreshToken but they
are missing from the dependency array; update the useMemo dependency list to
include clearSession and refreshToken so contextValue updates when those
function/value identities change, ensuring consumers don't receive stale
closures.
- Around line 46-82: Prettier formatting errors were introduced in the
AsgardeoProvider Types (not logic), e.g. spacing inside the
AsgardeoClientProviderProps type literal and in function signatures like
handleOAuthCallback and updateProfile; run the repository formatter (pnpm
prettier --write or the lint:fix script) on
packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx and remove the
extra spaces inside type literals/call argument lists so the exported
AsgardeoClientProviderProps, handleOAuthCallback, updateProfile, and other
signatures match the project's Prettier rules.
---
Duplicate comments:
In `@packages/nextjs/src/server/actions/signInAction.ts`:
- Around line 133-152: Normalize the token expiry by checking both expiresIn and
expires_in on signInResult and coercing to a Number (e.g., const tokenTtl =
Number(signInResult.expiresIn ?? signInResult.expires_in ?? 0)); then compute a
separate session JWT TTL (sessionJwtTtl) that is either derived from tokenTtl or
from the configured sessionCookieExpiryTime but ensure session JWT expiration is
never shorter than what the cookie will allow—pass sessionJwtTtl to
SessionManager.createSessionToken instead of access-token TTL, and when calling
cookieStore.set with
SessionManager.getSessionCookieOptions(sessionCookieExpiryTime) cap or align
sessionCookieExpiryTime to be <= sessionJwtTtl (or vice versa per your policy)
so the cookie cannot outlive a verifiable session JWT; update references around
signInResult, SessionManager.createSessionToken,
SessionManager.getSessionCookieOptions, and
SessionManager.resolveSessionCookieExpiry accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72022934-08e9-49c6-ad19-8f01e26e7a7b
📒 Files selected for processing (5)
packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsxpackages/nextjs/src/server/actions/handleOAuthCallbackAction.tspackages/nextjs/src/server/actions/refreshToken.tspackages/nextjs/src/server/actions/signInAction.tspackages/nextjs/src/server/middleware/asgardeoMiddleware.ts
✅ Files skipped from review due to trivial changes (2)
- packages/nextjs/src/server/actions/refreshToken.ts
- packages/nextjs/src/server/middleware/asgardeoMiddleware.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nextjs/src/server/actions/handleOAuthCallbackAction.ts
| import { useRouter, useSearchParams } from 'next/navigation'; | ||
| import { FC, PropsWithChildren, RefObject, useEffect, useMemo, useRef, useState } from 'react'; | ||
| import AsgardeoContext, { AsgardeoContextProps } from './AsgardeoContext'; | ||
| import { RefreshResult } from '../../../server/actions/refreshToken'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine its beginning for 'use client' directive
head -60 packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsxRepository: asgardeo/javascript
Length of output: 2158
🏁 Script executed:
# Also check the refreshToken module to see if it's marked 'use server'
head -10 packages/nextjs/src/server/actions/refreshToken.tsRepository: asgardeo/javascript
Length of output: 390
🏁 Script executed:
# Search for all usages of RefreshResult in the AsgardeoProvider file
rg "RefreshResult" packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx -A 2 -B 2Repository: asgardeo/javascript
Length of output: 629
🏁 Script executed:
# Check the refreshToken.ts file to verify it's a server action and understand RefreshResult
cat packages/nextjs/src/server/actions/refreshToken.tsRepository: asgardeo/javascript
Length of output: 5027
Use import type for server-action type import in a client module.
This file is marked 'use client', but RefreshResult is imported with a value import from ../../../server/actions/refreshToken (a 'use server' module). Although only the type is used, a non-type-only import can cause bundlers to pull the server action module (and its server-only dependencies) into the client bundle or trip Next.js' client/server boundary checks. Make it type-only so it's fully erased at compile time.
🛠️ Proposed fix
-import { RefreshResult } from '../../../server/actions/refreshToken';
+import type { RefreshResult } from '../../../server/actions/refreshToken';📝 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
| import { RefreshResult } from '../../../server/actions/refreshToken'; | |
| import type { RefreshResult } from '../../../server/actions/refreshToken'; |
🧰 Tools
🪛 GitHub Actions: 👷 PR Builder
[error] 46-51: ESLint reported Prettier formatting errors (prettier/prettier). Fix spacing in type imports/usages at the listed lines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nextjs/src/client/contexts/Asgardeo/AsgardeoProvider.tsx` at line
51, The client module AsgardeoProvider.tsx currently imports RefreshResult with
a value import from ../../../server/actions/refreshToken; change this to a
type-only import so the server-action module is not pulled into the client
bundle: replace the import with a type import for RefreshResult (e.g., use
import type { RefreshResult } ...) in AsgardeoProvider.tsx to ensure the symbol
is erased at compile time and preserves the 'use client' boundary.
| ); | ||
| const userIdFromToken: string = (idToken['sub'] || signInResult['sub'] || sessionId) as string; | ||
| const {accessToken}: {accessToken: string} = signInResult as {accessToken: string}; | ||
| const refreshToken: string = (signInResult['refreshToken'] as string | undefined) ?? ''; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect refresh-token field naming across token models and Next.js refresh usage.
# Expectation: If raw token responses expose `refresh_token`, signInAction should handle it.
rg -nP --type=ts -C4 '\brefreshToken\b|\brefresh_token\b' packages/javascript/src packages/node/src packages/nextjs/srcRepository: asgardeo/javascript
Length of output: 35553
Handle both refreshToken and refresh_token field names for OAuth consistency.
Line 128 only reads signInResult['refreshToken']. If the OAuth response returns refresh_token instead (as shown in handleRefreshToken.ts line 97 and legacy code), the refresh token becomes an empty string, breaking the newly added session refresh flow.
The same asymmetry exists for accessToken on line 127—it correctly handles both accessToken and access_token, but refreshToken does not.
Proposed fix
- const refreshToken: string = (signInResult['refreshToken'] as string | undefined) ?? '';
+ const refreshToken: string =
+ ((signInResult['refreshToken'] ?? signInResult['refresh_token']) as string | undefined) ?? '';This also applies to handleOAuthCallbackAction.ts line 111, which has the identical issue.
📝 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
| const refreshToken: string = (signInResult['refreshToken'] as string | undefined) ?? ''; | |
| const refreshToken: string = | |
| ((signInResult['refreshToken'] ?? signInResult['refresh_token']) as string | undefined) ?? ''; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nextjs/src/server/actions/signInAction.ts` at line 128, The code in
signInAction.ts reads only signInResult['refreshToken'] when building the
refreshToken variable (const refreshToken) which misses OAuth responses that use
refresh_token; update the extraction to check both keys (e.g.,
signInResult['refreshToken'] || signInResult['refresh_token']) and coerce to
string/empty default like the existing accessToken handling; apply the identical
change in handleOAuthCallbackAction.ts where the same refreshToken extraction
occurs so both code paths consistently accept refreshToken or refresh_token from
signInResult.
kavindadimuthu
force-pushed
the
fix-refresh-issue
branch
from
024588e to
a0c6524
Compare
kavindadimuthu
force-pushed
the
fix-refresh-issue
branch
from
a0c6524 to
0f75633
Compare
…version and error handling
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. |
DonOmalVindula
approved these changes
Apr 21, 2026
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.
Purpose
This pull request introduces significant improvements to the Asgardeo Next.js SDK, focusing on robust session management, enhanced token refresh handling, and improved Edge Runtime compatibility. The most notable changes include the addition of new server actions for clearing sessions and refreshing tokens, updates to session expiry handling, and new build outputs for Edge environments.
Session Management & Token Refresh Enhancements:
clearSession(to immediately remove session cookies for error recovery) andrefreshToken(to securely refresh access tokens and update session cookies). These actions are now exposed via the provider and context, enabling better client and server-side session handling. [1] [2] [3] [4] [5] [6] [7] [8] [9]sessionExpirySeconds), defaulting to 24 hours, and are set appropriately on sign-in and refresh. [1] [2] [3]isSignedInlogic to directly validate JWT expiry and simplify fallback to legacy session checks. [1] [2]Edge Runtime & Build System Updates:
src/middleware.ts) and corresponding ES module build output, ensuring safe usage in Next.js middleware and environments without Node.js APIs. [1] [2] [3] [4] [5]Other Improvements:
clearSession,refreshToken). [1] [2]Summary of Most Important Changes:
Session management and token refresh:
clearSessionandrefreshTokenserver actions for robust session and token lifecycle management, and exposed them through context and provider types. [1] [2] [3] [4] [5] [6] [7] [8] [9]sessionExpirySecondsand ensuring session cookies are set with the correct expiry. [1] [2] [3]isSignedInlogic to directly check JWT validity and only fall back to legacy checks when necessary. [1] [2]Edge Runtime and build outputs:
src/middleware.ts) and build outputs, and updated package exports for Edge environments. [1] [2] [3] [4] [5]Type and documentation improvements:
These changes provide a more reliable authentication experience, better support for modern Next.js middleware, and improved developer ergonomics.
Related Issues
Related PRs
Checklist
Security checks
Summary by CodeRabbit
New Features
Bug Fixes