refactor(core): split session strategies into function modules - #243
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe PR splits stateful and stateless session strategies into dedicated typed handlers. It creates new context types in config, moves strategy-specific types from session.ts to config.ts, adds modular lifecycle and OAuth handlers for both strategies, adds shared device and expiration utilities, introduces an OAuth account mismatch error, and updates tests to use account-ID based lookups. ChangesSession strategy modularization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (6)
packages/core/src/@types/config.ts (1)
540-542: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider making
InternalContextForStatefulgeneric instead of fixingany, any.The
any, anyarguments erase the identity and sign-up schema types. Every consumer then loses type safety onctx.identity.schemaRegistry, andstateful.tsmust passctx as anyat each handler construction (lines 270-279). A generic alias preserves the caller types.♻️ Proposed refactor
-export type InternalContextForStateful = Omit<InternalContext<any, any>, "sessionConfig"> & { - sessionConfig: StatefulStrategyConfig -} +export type InternalContextForStateful< + Identity extends Identities = Identities, + SignUpSchema extends SchemaTypes = SchemaTypes, +> = Omit<InternalContext<Identity, SignUpSchema>, "sessionConfig"> & { + sessionConfig: StatefulStrategyConfig +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/`@types/config.ts around lines 540 - 542, Make InternalContextForStateful generic over the identity and sign-up schema types, and pass those type parameters through to InternalContext instead of fixing them to any. Update stateful.ts handler construction and related consumers to use the generic alias with the existing context types, removing the ctx as any casts while preserving the StatefulStrategyConfig sessionConfig override.packages/core/src/@types/session.ts (1)
387-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRename
cookieConfigto avoid a collision withInternalContext.cookieConfig.
InternalContextalready declarescookieConfig: { secure, standard }(packages/core/src/@types/config.tslines 534-537). HerecookieConfigholds the cookie manager returned bycreateCookieManager. Handlers destructure both shapes, so the same name means two different things in one scope. Rename this field tocookieManager.♻️ Proposed refactor
export interface InternalStatefulContext { ctx: InternalContextForStateful - cookieConfig: CookieManager + cookieManager: CookieManager }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/`@types/session.ts around lines 387 - 390, Rename the InternalStatefulContext field cookieConfig to cookieManager, preserving its CookieManager type and value from createCookieManager. Update all consumers and handler destructuring of InternalStatefulContext to use cookieManager, while leaving InternalContext.cookieConfig unchanged for the secure/standard configuration object.packages/core/src/session/stateful/createSession.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
@/alias for theutils.tsimport.Every other import in this file and in the sibling handlers uses the
@/session/...alias. This relative specifier is the only exception.♻️ Proposed refactor
-import { createDevice as __createDevice } from "./utils.ts" +import { createDevice as __createDevice } from "`@/session/stateful/utils.ts`"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/createSession.ts` at line 3, Update the import of createDevice in createSession.ts to use the established `@/session/`... alias for utils.ts instead of the relative "./utils.ts" specifier, preserving the existing __createDevice binding.packages/core/src/session/stateful/oauthCallback.ts (2)
240-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn a redirect response instead of a JSON body with status 302.
A 302 response with a JSON payload mixes two contracts. Browsers follow
Locationand discard the body. Returnnew Response(null, { status: 302, headers })so the intent is explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/oauthCallback.ts` at line 240, Update the OAuth callback return statement to create an empty redirect response with status 302 instead of using Response.json. Preserve the existing headersBuilder.toHeaders() result, including the Location header, and pass it to the new Response constructor.
82-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the cast with a declared error code.
"DATABASE_TOKEN_HASH_NOT_FOUND" as anybypasses the error-code union, and the code does not describe a missing PKCE verifier. Add a dedicated code, for exampleOAUTH_CODE_VERIFIER_MISSING, and remove the cast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/oauthCallback.ts` around lines 82 - 84, Add a dedicated declared error code for a missing OAuth PKCE verifier, such as OAUTH_CODE_VERIFIER_MISSING, to the relevant error-code union or definitions. Update the transaction.codeVerifier check in the OAuth callback to throw AuraAuthError with that code and remove the as any cast.packages/core/src/session/stateful/signIn.ts (1)
12-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant non-null assertions.
The guard at Line 14 already narrows
providerto a defined value. Theprovider!assertions at Line 26 add no safety and hide future narrowing regressions.♻️ Proposed cleanup
- const resolvedProvider = isOIDC ? await resolveOpenIDProvider(provider!) : provider! + const resolvedProvider = isOIDC ? await resolveOpenIDProvider(provider) : provider🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/signIn.ts` around lines 12 - 26, Remove the redundant non-null assertions from both uses of provider in the sign-in callback after the !provider guard, including the resolveOpenIDProvider call and the resolvedProvider assignment, while preserving the existing provider narrowing and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/session/stateful/createSession.ts`:
- Around line 87-111: Remove the user.email fields from the structuredData
payloads in the STATEFUL_USER_CREATED and STATEFUL_USER_UPDATED logger calls
within the session user creation/update flow. Retain only non-sensitive
identifiers such as user_id, or replace email with a boolean presence indicator
if that context is required; do not log email addresses.
In `@packages/core/src/session/stateful/getProviderTokens.ts`:
- Around line 117-126: Update the unconfigured-provider branch in
getProviderTokens to avoid returning success with unchecked stored tokens. Align
it with revokeToken by throwing UNSUPPORTED_OAUTH_CONFIGURATION, or return the
established failure result while preserving the existing provider-not-found
logging.
- Around line 20-61: Extract the duplicated session-cookie, token lookup, user,
active-status, expiry, and expired-session revocation logic into one shared
helper that returns either the validated session or a discriminated failure.
Update packages/core/src/session/stateful/getProviderTokens.ts lines 20-61 to
map helper failures through handleApiError,
packages/core/src/session/stateful/revokeToken.ts lines 22-53 to throw
AuraAuthError with SESSION_NOT_FOUND, and
packages/core/src/session/stateful/isProviderConnected.ts lines 16-47 to return
false; preserve each handler’s existing success and failure shapes.
- Around line 81-86: Update the missing OAuth-account branch in
getProviderTokens to construct AuraAuthError with OAUTH_UNLINKED_ACCOUNT_ERROR
instead of COOKIE_INVALID_VALUE, matching the account-related code used by
revokeToken.ts while preserving the existing handleApiError response flow.
- Around line 157-168: Update the updateOAuthTokens call in the token refresh
flow to pass getAccount?.id as its first argument instead of oauthId, while
preserving the existing refreshed token payload.
- Around line 96-107: Update the token object in getProviderTokens to expose the
OAuth access-token expiry as accessTokenExpiresAt while retaining expiresAt as
the existing alias, and remove the missing or redundant field as directed.
Eliminate the tokens as any casts at the referenced call sites, preserving the
OAuthTokenPayload typing through shouldRefresh and refreshProviderToken.
In `@packages/core/src/session/stateful/getSession.ts`:
- Around line 76-87: Update the inactive-session branch in getSession,
specifically the session.status !== "active" handling, to clear the session
cookie before returning. Match the cookie-clearing behavior used by the expired
and error branches via cookieConfig.clear(), while preserving the existing
logging and null session response.
In `@packages/core/src/session/stateful/isProviderConnected.ts`:
- Around line 78-87: Update the catch block in isProviderConnected so it returns
false only for expected missing-session or missing-account conditions; re-throw
adapter, database, and other unexpected errors (or propagate an equivalent
discriminated failure result) instead of treating them as disconnected
providers, while preserving the existing OAUTH_ACCESS_TOKEN_ERROR logging.
- Around line 38-40: Replace the invalid "expired" revoke reason with an
accepted RevokeReason for each expiry-driven call:
packages/core/src/session/stateful/isProviderConnected.ts lines 38-40,
packages/core/src/session/stateful/revokeToken.ts lines 44-46, and
packages/core/src/session/stateful/getProviderTokens.ts lines 52-54. Update the
revokeSession calls in these flows to use a valid reason such as
"max_sessions_exceeded" or "admin_action".
In `@packages/core/src/session/stateful/oauthCallback.ts`:
- Around line 169-208: In the account reuse branch around getAccountByProvider,
compare account.userId with the resolved userId before updateOAuthTokens; reject
the OAuth callback when they differ, and only update tokens when ownership
matches. Preserve the existing new-account creation flow for missing accounts.
- Around line 137-167: Update the user-resolution flow around getUserByEmail,
updateUser, and createUser so email-based account matching and linking occurs
only when userInfo indicates the provider-asserted email is verified; otherwise
require the existing explicit-link path or create a separate user without
attaching it to an existing account. Set emailVerifiedAt only for newly created
users whose provider email is verified, leaving it unset otherwise. Also
destructure and omit sub from userInfo attributes in the createUser branch,
matching the existing updateUser behavior.
- Around line 210-240: Update the OAuth callback flow around createSession and
the sessionToken cookie to send the raw sessionToken to the client while
continuing to persist tokenHash in the session record. Set the session
expiration using the existing sessionConfig lifetime when provided, otherwise
preserve the 15-day default, and keep the token lookup compatible with
getSession and SessionsAdapter.getSessionByToken.
- Around line 23-78: Replace the separate getOAuthTransactionByState and later
consumeOAuthTransaction flow with a single atomic conditional-consume operation
that returns the transaction only when the state exists, is unexpired, matches
oauthId, and has not already been consumed. Update the surrounding OAuth
callback logic to validate the returned result and proceed with code exchange
and session creation only after successful consumption, preserving the existing
protocol responses for invalid, expired, or mismatched transactions.
In `@packages/core/src/session/stateful/refreshSession.ts`:
- Around line 181-230: The refresh flow in __refreshSession updates the server
expiration but does not renew the client cookie. Before returning the refreshed
session, re-set the session cookie using the existing cookie configuration and
the refreshed session token so its maxAge is extended alongside newExpiresAt;
preserve the existing secureApiHeaders and response structure.
In `@packages/core/src/session/stateful/revokeToken.ts`:
- Around line 84-96: Update the revokeToken flow around revokeProviderToken and
updateAccountStatus so disconnect semantics are consistent: when disconnect is
true, revoke the provider token and mark the account as unlinked; when false,
skip both operations. Gate both actions on the same disconnect condition and
preserve the existing logging within the revocation path.
- Around line 62-71: Update the OAuth account lookup in the revoke-token flow to
call getAccountsByUserId(sessionByToken.userId), select the active account
matching oauthId, and verify its userId matches sessionByToken.userId before
revoking or unlinking. Preserve the existing OAUTH_UNLINKED_ACCOUNT_ERROR
behavior when no valid account is found, and pass the validated account id to
subsequent revoke/unlink operations.
In `@packages/core/src/session/stateful/signIn.ts`:
- Around line 56-59: Update the sign-in flow around createDevice to avoid
persisting x-device-fingerprint and x-device-id as trusted device identity;
retain them only as explicitly untrusted metadata or omit them, and ensure
device consumers resolve identity from server-derived fingerprints. Replace the
hardcoded 10-minute expiresAt calculation with a configurable
transaction-lifetime value from sessionConfig.
In `@packages/core/src/session/stateful/stateful.ts`:
- Around line 8-20: Remove the __refreshUserInfo import from the stateful
session import list, leaving the other session symbols unchanged; do not add a
refreshUserInfo module unless the stateful implementation actually requires that
method.
- Around line 31-266: Remove the local refreshSession implementation and update
refreshUserInfo to delegate to the returned __refreshSession handler, preserving
its headers, user payload, and skipCSRFCheck arguments so both refresh paths
share the same CSRF cookie handling and session logic.
---
Nitpick comments:
In `@packages/core/src/`@types/config.ts:
- Around line 540-542: Make InternalContextForStateful generic over the identity
and sign-up schema types, and pass those type parameters through to
InternalContext instead of fixing them to any. Update stateful.ts handler
construction and related consumers to use the generic alias with the existing
context types, removing the ctx as any casts while preserving the
StatefulStrategyConfig sessionConfig override.
In `@packages/core/src/`@types/session.ts:
- Around line 387-390: Rename the InternalStatefulContext field cookieConfig to
cookieManager, preserving its CookieManager type and value from
createCookieManager. Update all consumers and handler destructuring of
InternalStatefulContext to use cookieManager, while leaving
InternalContext.cookieConfig unchanged for the secure/standard configuration
object.
In `@packages/core/src/session/stateful/createSession.ts`:
- Line 3: Update the import of createDevice in createSession.ts to use the
established `@/session/`... alias for utils.ts instead of the relative
"./utils.ts" specifier, preserving the existing __createDevice binding.
In `@packages/core/src/session/stateful/oauthCallback.ts`:
- Line 240: Update the OAuth callback return statement to create an empty
redirect response with status 302 instead of using Response.json. Preserve the
existing headersBuilder.toHeaders() result, including the Location header, and
pass it to the new Response constructor.
- Around line 82-84: Add a dedicated declared error code for a missing OAuth
PKCE verifier, such as OAUTH_CODE_VERIFIER_MISSING, to the relevant error-code
union or definitions. Update the transaction.codeVerifier check in the OAuth
callback to throw AuraAuthError with that code and remove the as any cast.
In `@packages/core/src/session/stateful/signIn.ts`:
- Around line 12-26: Remove the redundant non-null assertions from both uses of
provider in the sign-in callback after the !provider guard, including the
resolveOpenIDProvider call and the resolvedProvider assignment, while preserving
the existing provider narrowing and behavior.
🪄 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 Plus
Run ID: a9cf30e9-1786-4b46-9086-77da037dc868
📒 Files selected for processing (17)
packages/core/src/@types/config.tspackages/core/src/@types/session.tspackages/core/src/session/stateful.tspackages/core/src/session/stateful/createSession.tspackages/core/src/session/stateful/destroySession.tspackages/core/src/session/stateful/getProviderTokens.tspackages/core/src/session/stateful/getSession.tspackages/core/src/session/stateful/index.tspackages/core/src/session/stateful/isProviderConnected.tspackages/core/src/session/stateful/oauthCallback.tspackages/core/src/session/stateful/refreshSession.tspackages/core/src/session/stateful/revokeSession.tspackages/core/src/session/stateful/revokeToken.tspackages/core/src/session/stateful/signIn.tspackages/core/src/session/stateful/stateful.tspackages/core/src/session/stateful/utils.tspackages/core/src/session/strategy.ts
💤 Files with no reviewable changes (1)
- packages/core/src/session/stateful.ts
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/session/stateful/refreshSession.ts (1)
190-198: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winKeep the persisted session token and cookie token consistent.
At Lines 190-198, the session row keeps the old
sessionByToken.tokenHash. At Lines 230-232, the response sets a newly generatedtokenHashin the cookie but never persists it. The next request sends a token that does not resolve to this session and the user is logged out after a successful refresh. The current creation flow also stores a hash while returning the raw secret, so do not place the hash in the bearer cookie. (raw.githubusercontent.com)If refresh only extends expiry, set the existing
sessionTokenin the cookie. If rotation is required, persist the new hash in the same update and return the corresponding raw token.🔐 Proposed fix when rotation is not required
- const secretValue = createSecretValue(64) - const tokenHash = await createHash(secretValue) - return { session: updatedSession, headers: cookieManager.setCookie({ sessionToken: tokenHash }) } + return { session: updatedSession, headers: cookieManager.setCookie({ sessionToken }) }Also applies to: 230-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/refreshSession.ts` around lines 190 - 198, Update the refresh flow around sessionConfig.adapter.updateSession and the response cookie assignment so the persisted session token and cookie token remain consistent: when refresh only extends expiry, preserve sessionByToken.tokenHash and set the existing raw sessionToken in the cookie instead of generating or returning a new hash. If rotation is intentionally retained, persist the new hash in the same update and return its corresponding raw token.packages/core/src/session/stateful/destroySession.ts (1)
4-21: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the request-scoped cookie store for CSRF verification.
__destroySessioncapturesctx.cookieswhen the strategy is constructed. The router selects the secure or standard store per request, whileInternalStatefulContext.cookies()andcookieManageruse the current store. On secure requests, this call can read the standard CSRF cookie and reject a valid logout. (github.com)Destructure
cookiesfromInternalStatefulContextand passcookies()toverifyCSRFToken.🔧 Proposed fix
-export const __destroySession = ({ ctx, cookieManager }: InternalStatefulContext) => { - const { logger, sessionConfig, cookies, jose } = ctx +export const __destroySession = ({ ctx, cookies, cookieManager }: InternalStatefulContext) => { + const { logger, sessionConfig, jose } = ctx ... - cookies: cookies, + cookies: cookies(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/destroySession.ts` around lines 4 - 21, Update __destroySession to use the request-scoped cookie store for CSRF verification: destructure the cookies accessor from InternalStatefulContext and pass cookies() to verifyCSRFToken instead of the construction-time ctx.cookies value, while preserving the existing verification flow.
🧹 Nitpick comments (2)
packages/core/src/session/stateful/index.ts (1)
22-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWire handler contexts with the session types.
The handler factories accept
DatabaseStrategyOptions/JWTStrategyOptions, but the handler constructors requireInternalStatefulContextorInternalStatelessContext.ctx as anyhides that specialization mismatch. Pass a single typed handler context to each handler factory instead.
packages/core/src/session/stateful/index.ts#L22-L32packages/core/src/session/stateless/index.ts#L24-L34🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/index.ts` around lines 22 - 32, Replace the repeated ctx as any arguments in the handler factory calls with one explicitly typed handler context, then pass that context to all factories in packages/core/src/session/stateful/index.ts lines 22-32 and packages/core/src/session/stateless/index.ts lines 24-34. Use the appropriate InternalStatefulContext and InternalStatelessContext types so refreshUserInfo, getSession, createSession, refreshSession, revokeSession, revokeToken, destroySession, getProviderTokens, isProviderConnected, signIn, and oauthCallback receive the specialized context without unsafe casts.packages/core/src/session/stateless/isProviderConnected.ts (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a log event that describes the missing cookie.
OAUTH_ACCESS_TOKEN_REQUEST_INITIATEDmarks the start of a token request.revokeToken.tsline 25 uses it for that purpose. Reusing it here for an absent cookie makes the two cases indistinguishable in logs. Emit a dedicated event, for example a "no access token cookie" event, asgetProviderTokens.tsline 49 does.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateless/isProviderConnected.ts` around lines 13 - 18, The catch branch in isProviderConnected currently logs OAUTH_ACCESS_TOKEN_REQUEST_INITIATED for a missing cookie, conflating it with token-request initiation. Replace that event in the catch block with the existing dedicated no-access-token-cookie event used by getProviderTokens, preserving the provider and hasCookie structured data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/session/stateful/refreshSession.ts`:
- Around line 233-240: Remove the console.error call from the catch block in the
refreshSession flow and rely solely on
logger?.log("STATEFUL_REFRESH_SESSION_ERROR", ...). Preserve the existing
sanitized error_type and error_message fields in the structured logger.
In `@packages/core/src/session/stateless/getProviderTokens.ts`:
- Around line 113-117: Stop propagating inbound request headers into stateless
response headers. In packages/core/src/session/stateless/getProviderTokens.ts at
lines 113-117, build headers only from secureApiHeaders and the updated cookie,
and apply the same change to return paths at lines 36, 61, 145, 159, and 171
that return request.headers directly; in
packages/core/src/session/stateless/getSession.ts at lines 49-51, return
newHeaders; and in packages/core/src/session/stateless/refreshUserInfo.ts at
lines 15-17, initialize HeadersBuilder from empty Headers or secureApiHeaders
instead of headers.
In `@packages/core/src/session/stateless/getSession.ts`:
- Around line 4-23: The stateless session handler duplicates the shared
updateExpires logic, risking inconsistent expiration behavior. Remove the local
updateExpires definition and import the existing helper from
"`@/shared/utils/session-strategy.ts`", preserving the current session expiration
flow and calls.
- Around line 38-41: Update the userClaims assignment in the session handler to
honor identity.skipValidation, bypassing
identity.schemaRegistry.parse(defaultPayload) when validation is disabled and
preserving the existing parse behavior otherwise. Align this logic with the
guarded call in refreshSession.ts.
In `@packages/core/src/session/stateless/oauthCallback.ts`:
- Around line 18-19: Update the OAuth callback flow around oauthConfig and
isOIDCProvider to detect when providers[oauthId] is undefined before passing it
to any provider logic. Throw the existing UNSUPPORTED_OAUTH_CONFIGURATION error,
matching the guard and behavior used in signIn.ts, and leave valid provider
handling unchanged.
In `@packages/core/src/session/stateless/refreshSession.ts`:
- Around line 38-53: Update the refresh flow around parsedClaims and
updatedSession.user to remove the reserved JWT claims exp, iat, and mexp before
constructing the user payload, matching the sanitization used by getSession.ts.
Preserve sub and all non-reserved claims while ensuring those reserved fields
cannot flow into the returned Session.user.
In `@packages/core/src/session/stateless/refreshUserInfo.ts`:
- Around line 8-26: Update __refreshUserInfo to delegate session renewal to
__refreshSession instead of calling __createSession directly, passing the
current context, cookies, cookieManager, and partial user information so
existing claims are merged and verifyCSRFToken runs before issuing the
replacement token. Preserve the subsequent header construction and
getStandardSession flow using the token returned by __refreshSession.
In `@packages/core/src/session/stateless/revokeToken.ts`:
- Around line 12-22: Update the returned revoke-token handler around getCookie
and the provider lookup to catch a missing access-token cookie, continue to
cookie clearing, and skip provider revocation when no token is available.
Preserve the existing unsupported-provider validation and token verification
behavior when a cookie exists.
In `@packages/core/src/shared/utils.ts`:
- Line 173: Remove the console.error call in verifyCSRFToken that logs headers
and cookies, leaving the existing structured logger call intact.
In `@packages/core/src/shared/utils/session-strategy.ts`:
- Around line 40-53: Update the session expiration logic in the strategy switch
to preserve an existing expiration when no rollover is required: return new
Date(exp * 1000) for fixed and absolute strategies and for sliding sessions
outside the refresh threshold, while retaining null only for missing exp. Add
coverage for fixed, absolute, and sliding sessions above the threshold.
---
Outside diff comments:
In `@packages/core/src/session/stateful/destroySession.ts`:
- Around line 4-21: Update __destroySession to use the request-scoped cookie
store for CSRF verification: destructure the cookies accessor from
InternalStatefulContext and pass cookies() to verifyCSRFToken instead of the
construction-time ctx.cookies value, while preserving the existing verification
flow.
In `@packages/core/src/session/stateful/refreshSession.ts`:
- Around line 190-198: Update the refresh flow around
sessionConfig.adapter.updateSession and the response cookie assignment so the
persisted session token and cookie token remain consistent: when refresh only
extends expiry, preserve sessionByToken.tokenHash and set the existing raw
sessionToken in the cookie instead of generating or returning a new hash. If
rotation is intentionally retained, persist the new hash in the same update and
return its corresponding raw token.
---
Nitpick comments:
In `@packages/core/src/session/stateful/index.ts`:
- Around line 22-32: Replace the repeated ctx as any arguments in the handler
factory calls with one explicitly typed handler context, then pass that context
to all factories in packages/core/src/session/stateful/index.ts lines 22-32 and
packages/core/src/session/stateless/index.ts lines 24-34. Use the appropriate
InternalStatefulContext and InternalStatelessContext types so refreshUserInfo,
getSession, createSession, refreshSession, revokeSession, revokeToken,
destroySession, getProviderTokens, isProviderConnected, signIn, and
oauthCallback receive the specialized context without unsafe casts.
In `@packages/core/src/session/stateless/isProviderConnected.ts`:
- Around line 13-18: The catch branch in isProviderConnected currently logs
OAUTH_ACCESS_TOKEN_REQUEST_INITIATED for a missing cookie, conflating it with
token-request initiation. Replace that event in the catch block with the
existing dedicated no-access-token-cookie event used by getProviderTokens,
preserving the provider and hasCookie structured data.
🪄 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 Plus
Run ID: 23107345-8cfb-4c16-8c34-bf45dab96e21
📒 Files selected for processing (39)
packages/core/src/@types/config.tspackages/core/src/@types/session.tspackages/core/src/session/stateful/createSession.tspackages/core/src/session/stateful/destroySession.tspackages/core/src/session/stateful/getProviderTokens.tspackages/core/src/session/stateful/getSession.tspackages/core/src/session/stateful/index.tspackages/core/src/session/stateful/isProviderConnected.tspackages/core/src/session/stateful/oauthCallback.tspackages/core/src/session/stateful/refreshSession.tspackages/core/src/session/stateful/refreshUserInfo.tspackages/core/src/session/stateful/revokeToken.tspackages/core/src/session/stateful/signIn.tspackages/core/src/session/stateless.tspackages/core/src/session/stateless/createSession.tspackages/core/src/session/stateless/destroySession.tspackages/core/src/session/stateless/getProviderTokens.tspackages/core/src/session/stateless/getSession.tspackages/core/src/session/stateless/index.tspackages/core/src/session/stateless/isProviderConnected.tspackages/core/src/session/stateless/oauthCallback.tspackages/core/src/session/stateless/refreshSession.tspackages/core/src/session/stateless/refreshUserInfo.tspackages/core/src/session/stateless/revokeToken.tspackages/core/src/session/stateless/signIn.tspackages/core/src/session/strategy.tspackages/core/src/shared/errors.tspackages/core/src/shared/logger.tspackages/core/src/shared/utils.tspackages/core/src/shared/utils/session-strategy.tspackages/core/test/actions/providers/tokens/revoke/stateful.test.tspackages/core/test/actions/providers/tokens/tokens/stateful.test.tspackages/core/test/actions/providers/user/refresh/stateful.test.tspackages/core/test/api/stateful/getAccessToken.test.tspackages/core/test/api/stateful/getProviderTokens.test.tspackages/core/test/api/stateful/refreshUserInfo.test.tspackages/core/test/api/stateful/revokeToken.test.tspackages/core/test/api/stateful/updateSession.test.tspackages/core/test/presets.ts
💤 Files with no reviewable changes (1)
- packages/core/src/session/stateless.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/core/src/session/strategy.ts
- packages/core/src/session/stateful/isProviderConnected.ts
- packages/core/src/session/stateful/getSession.ts
- packages/core/src/session/stateful/oauthCallback.ts
- packages/core/src/@types/config.ts
- packages/core/src/session/stateful/getProviderTokens.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/core/src/session/stateful/oauthCallback.ts (1)
82-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an OAuth-specific error for a missing PKCE verifier.
DATABASE_TOKEN_HASH_NOT_FOUND as anyreports an unrelated failure and hides a type mismatch. Use a typed OAuth protocol or PKCE error instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/oauthCallback.ts` around lines 82 - 83, Update the missing-verifier branch in the OAuth callback flow to throw the existing typed OAuth protocol or PKCE-specific error instead of AuraAuthError with DATABASE_TOKEN_HASH_NOT_FOUND and an any cast; preserve the current behavior of rejecting transactions without transaction.codeVerifier.packages/core/src/session/stateful/refreshSession.ts (3)
180-180: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftUse the configured session expiration strategy.
This line resets every session to
now + 15 days. It bypasses the shared fixed, absolute, rolling, and sliding expiration rules and can extend an absolute session indefinitely. Use the shared expiration utility and derive cookie renewal from the same result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/refreshSession.ts` at line 180, Update the session refresh logic around newExpiresAt to use the shared session expiration utility and configured fixed, absolute, rolling, or sliding strategy instead of hardcoding 15 days from now. Derive the cookie renewal expiration from that same computed result so absolute sessions cannot be extended indefinitely.
148-171: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject immutable identity fields in session updates.
sessionPayloadis spread overparsedCurrentUser, but onlysubis restored. A caller can provideid, causingvalidatedUser.idto differ fromsub.updateUserthen receivesidinuserUpdateFields. Remove immutable identity fields before merging and persisting the patch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/refreshSession.ts` around lines 148 - 171, Update the session update flow around parsedCurrentUser and sessionPayload to remove immutable identity fields, including id and sub, from the incoming patch before merging. Preserve the existing parsedCurrentUser identity values during validation, and ensure the userUpdateFields passed to sessionConfig.adapter.updateUser cannot contain those immutable fields.
189-199: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftProtect the session refresh update from stale writes.
refreshSessionreads the session before parsing users, so a concurrent admin/API revoke can run unconditionally.updateSessionreplaces the fields unconditionally, which can restorestatus: "active"after a newerrevokedAtupdate. Use a conditional update/optimistic check and do not write stale fields such as status,mfaState, ortokenHash.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/refreshSession.ts` around lines 189 - 199, Update the session refresh persistence in refreshSession to use a conditional/optimistic update based on the session state read before refresh, preventing writes when the stored session has changed concurrently. Only persist the intended expiration refresh (and safe identifying fields), and exclude stale mutable fields such as status, mfaState, and tokenHash so a newer revoke or administrative update cannot be overwritten.packages/core/src/session/stateful/getSession.ts (1)
136-146: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSeparate invalid-session handling from infrastructure failures.
Both handlers clear the session cookie and return a null session for every exception. Preserve the cookie for transient adapter or validation failures, and clear it only for confirmed invalid or expired sessions.
packages/core/src/session/stateful/getSession.ts#L136-L146: classify errors before clearing the cookie.packages/core/src/session/stateful/refreshSession.ts#L230-L237: apply the same classification before clearing the cookie.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateful/getSession.ts` around lines 136 - 146, Classify caught errors in getSession and refreshSession before clearing cookies: clear the cookie and return a null session only for confirmed invalid or expired-session errors; preserve the existing cookie for transient adapter, validation, or infrastructure failures while retaining error logging and appropriate failure handling. Apply this in packages/core/src/session/stateful/getSession.ts lines 136-146 and packages/core/src/session/stateful/refreshSession.ts lines 230-237, using each method’s existing error classification symbols.
🧹 Nitpick comments (3)
packages/core/src/session/stateless/refreshUserInfo.ts (1)
5-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the
DefaultUserreturn type.
SessionStrategy.refreshUserInforeturnssession: Session<DefaultUser> | null. Instantiate__refreshSessionwithDefaultUserand return its typed result so this stateless implementation does not suppress type checking withvalue as any.Proposed fix
- const refreshSession = __refreshSession({ ctx, cookies, cookieManager }) + const refreshSession = __refreshSession<DefaultUser>({ ctx, cookies, cookieManager }) return async (userInfo: Partial<DefaultUser>, headers: Headers, skipCSRFCheck?: boolean) => { - const value = await refreshSession(headers, { user: userInfo }, skipCSRFCheck) - return value as any + return refreshSession(headers, { user: userInfo }, skipCSRFCheck) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateless/refreshUserInfo.ts` around lines 5 - 9, Update refreshUserInfo to instantiate __refreshSession with the DefaultUser generic, allowing refreshSession to preserve the Session<DefaultUser> | null result type. Return the awaited value directly and remove the value as any cast.packages/core/src/@types/config.ts (1)
546-568: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce duplication between
InternalStatefulContextandInternalStatelessContext.Both interfaces declare the same
cookiesandcookieManagerfields. Only thectxfield type differs. Extract a shared generic interface and derive both types from it.♻️ Proposed refactor
-export interface InternalStatefulContext { - ctx: InternalContextForStateful - cookies: () => InternalCookieStoreConfig - cookieManager: CookieManager -} - -export interface InternalStatelessContext { - ctx: InternalContextForStateless - cookies: () => InternalCookieStoreConfig - cookieManager: CookieManager -} +export interface InternalSessionContext<Ctx> { + ctx: Ctx + cookies: () => InternalCookieStoreConfig + cookieManager: CookieManager +} + +export type InternalStatefulContext = InternalSessionContext<InternalContextForStateful> +export type InternalStatelessContext = InternalSessionContext<InternalContextForStateless>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/`@types/config.ts around lines 546 - 568, Extract the shared cookies and cookieManager fields from InternalStatefulContext and InternalStatelessContext into a generic context interface, parameterized by the ctx type. Define both existing interfaces by extending or otherwise deriving from that shared interface with InternalContextForStateful and InternalContextForStateless respectively, preserving their public shapes.packages/core/src/session/stateless/index.ts (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging the no-op
revokeSessioncall for observability.
revokeSessionsilently returns for the JWT strategy, since there is no server-side session record to revoke. Every other handler in this module logs a structured event. Add a log call here so operators can distinguish an intentional no-op from a missing feature when debugging.🔧 Proposed addition
- const revokeSession = async (_sessionId: string): Promise<void> => {} + const revokeSession = async (_sessionId: string): Promise<void> => { + ctx.logger?.log("STATELESS_REVOKE_SESSION_NOOP", { + structuredData: { strategy: "stateless", reason: "no_server_side_session_record" }, + }) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/session/stateless/index.ts` at line 16, Update the stateless `revokeSession` handler to emit the module’s existing structured log event when invoked, while preserving its no-op behavior and Promise<void> contract. Match the logging style and context fields used by the other handlers in this module so operators can identify intentional JWT revocation no-ops.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/session/strategy.ts`:
- Around line 15-27: Update createSessionStrategy to validate the selected
sessionConfig shape before casting ctx to InternalStatefulContext or
InternalStatelessContext: require the database strategy to have a valid adapter,
and verify the stateless strategy configuration before calling
createStatelessStrategy. Reject invalid configurations through the existing
error path rather than relying on the strategy discriminator alone.
---
Outside diff comments:
In `@packages/core/src/session/stateful/getSession.ts`:
- Around line 136-146: Classify caught errors in getSession and refreshSession
before clearing cookies: clear the cookie and return a null session only for
confirmed invalid or expired-session errors; preserve the existing cookie for
transient adapter, validation, or infrastructure failures while retaining error
logging and appropriate failure handling. Apply this in
packages/core/src/session/stateful/getSession.ts lines 136-146 and
packages/core/src/session/stateful/refreshSession.ts lines 230-237, using each
method’s existing error classification symbols.
In `@packages/core/src/session/stateful/oauthCallback.ts`:
- Around line 82-83: Update the missing-verifier branch in the OAuth callback
flow to throw the existing typed OAuth protocol or PKCE-specific error instead
of AuraAuthError with DATABASE_TOKEN_HASH_NOT_FOUND and an any cast; preserve
the current behavior of rejecting transactions without transaction.codeVerifier.
In `@packages/core/src/session/stateful/refreshSession.ts`:
- Line 180: Update the session refresh logic around newExpiresAt to use the
shared session expiration utility and configured fixed, absolute, rolling, or
sliding strategy instead of hardcoding 15 days from now. Derive the cookie
renewal expiration from that same computed result so absolute sessions cannot be
extended indefinitely.
- Around line 148-171: Update the session update flow around parsedCurrentUser
and sessionPayload to remove immutable identity fields, including id and sub,
from the incoming patch before merging. Preserve the existing parsedCurrentUser
identity values during validation, and ensure the userUpdateFields passed to
sessionConfig.adapter.updateUser cannot contain those immutable fields.
- Around line 189-199: Update the session refresh persistence in refreshSession
to use a conditional/optimistic update based on the session state read before
refresh, preventing writes when the stored session has changed concurrently.
Only persist the intended expiration refresh (and safe identifying fields), and
exclude stale mutable fields such as status, mfaState, and tokenHash so a newer
revoke or administrative update cannot be overwritten.
---
Nitpick comments:
In `@packages/core/src/`@types/config.ts:
- Around line 546-568: Extract the shared cookies and cookieManager fields from
InternalStatefulContext and InternalStatelessContext into a generic context
interface, parameterized by the ctx type. Define both existing interfaces by
extending or otherwise deriving from that shared interface with
InternalContextForStateful and InternalContextForStateless respectively,
preserving their public shapes.
In `@packages/core/src/session/stateless/index.ts`:
- Line 16: Update the stateless `revokeSession` handler to emit the module’s
existing structured log event when invoked, while preserving its no-op behavior
and Promise<void> contract. Match the logging style and context fields used by
the other handlers in this module so operators can identify intentional JWT
revocation no-ops.
In `@packages/core/src/session/stateless/refreshUserInfo.ts`:
- Around line 5-9: Update refreshUserInfo to instantiate __refreshSession with
the DefaultUser generic, allowing refreshSession to preserve the
Session<DefaultUser> | null result type. Return the awaited value directly and
remove the value as any cast.
🪄 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 Plus
Run ID: c2217755-81a6-4585-9aa1-c1d81ca4b63d
📒 Files selected for processing (29)
packages/core/src/@types/config.tspackages/core/src/@types/session.tspackages/core/src/router/context.tspackages/core/src/session/stateful/createSession.tspackages/core/src/session/stateful/destroySession.tspackages/core/src/session/stateful/getProviderTokens.tspackages/core/src/session/stateful/getSession.tspackages/core/src/session/stateful/index.tspackages/core/src/session/stateful/isProviderConnected.tspackages/core/src/session/stateful/oauthCallback.tspackages/core/src/session/stateful/refreshSession.tspackages/core/src/session/stateful/refreshUserInfo.tspackages/core/src/session/stateful/revokeSession.tspackages/core/src/session/stateful/revokeToken.tspackages/core/src/session/stateful/signIn.tspackages/core/src/session/stateless/createSession.tspackages/core/src/session/stateless/destroySession.tspackages/core/src/session/stateless/getProviderTokens.tspackages/core/src/session/stateless/getSession.tspackages/core/src/session/stateless/index.tspackages/core/src/session/stateless/isProviderConnected.tspackages/core/src/session/stateless/oauthCallback.tspackages/core/src/session/stateless/refreshSession.tspackages/core/src/session/stateless/refreshUserInfo.tspackages/core/src/session/stateless/revokeToken.tspackages/core/src/session/stateless/signIn.tspackages/core/src/session/strategy.tspackages/core/src/shared/utils.tspackages/core/src/shared/utils/session-strategy.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/core/src/shared/utils/session-strategy.ts
- packages/core/src/session/stateless/refreshSession.ts
- packages/core/src/session/stateful/revokeSession.ts
- packages/core/src/session/stateless/getProviderTokens.ts
- packages/core/src/session/stateless/getSession.ts
- packages/core/src/session/stateful/destroySession.ts
- packages/core/src/session/stateful/isProviderConnected.ts
- packages/core/src/session/stateful/createSession.ts
- packages/core/src/session/stateful/getProviderTokens.ts
- packages/core/src/session/stateless/isProviderConnected.ts
This was referenced Aug 1, 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
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.
Description
This pull request refactors and reorganizes the session strategy implementation by splitting the Stateless (JWT) and Stateful (Database) strategies into dedicated function modules.
Previously, much of the Stateful implementation resided in a single
stateful.tsfile containing more than 1,400 lines of code, making it difficult to navigate, maintain, and extend. As support for additional authentication flows and features continues to grow, this structure became increasingly challenging to work with.To improve the architecture, the session strategy code has been split into dedicated modules organized under the
/statelessand/statefuldirectories. Each authentication operation is now implemented in its own file, resulting in a more modular and maintainable codebase.In addition to the restructuring, related code has been cleaned up and standardized to improve readability and consistency.
Key Changes
/statelessand/statefuldirectories.stateful.tsimplementation.Note
This PR is purely a refactoring effort. It does not introduce any behavioral changes or new functionality. Its purpose is to improve the internal organization and maintainability of the codebase while preserving the existing behavior.
@coderabbitai ignore