fix(auth): resolve onboarding redirect loop without disabling cookieCache#7
Open
jacksonkasi1 wants to merge 1 commit intoorganization-v2from
Open
fix(auth): resolve onboarding redirect loop without disabling cookieCache#7jacksonkasi1 wants to merge 1 commit intoorganization-v2from
jacksonkasi1 wants to merge 1 commit intoorganization-v2from
Conversation
…ache Re-enable session.cookieCache (maxAge: 60s) and fix the root cause: when onboarding step handlers wrote directly to DB (sessionTable.activeOrganizationId, userTable.shouldOnboard), the signed cookie cache was never refreshed, so RequireOnboarding guards read stale values and looped back to /onboarding. Fix: - hooks.after now intercepts /onboarding/step/* and /onboarding/skip-step/* paths. After the plugin has run adapter.updateOnboardingState (which sets shouldOnboard=false on the completion step), refreshSessionCookie() re-reads the fresh session from DB via internalAdapter.findSession and re-issues the signed cookie — so the very next guard check sees the updated values. - The createOrganization step handler also calls refreshSessionCookie() inline after writing activeOrganizationId to the session row, giving the client an updated cookie mid-flow (before the completion step). - Added packages/auth/src/utils/refresh-session-cookie.ts helper that wraps internalAdapter.findSession + setSessionCookie with error isolation.
Reviewer's GuideRe-enables Better Auth's session cookie cache while ensuring it is refreshed after onboarding-related session mutations to prevent stale session data from causing redirect loops, primarily by adding a reusable refresh helper and wiring it into onboarding middleware and organization creation flows. Sequence diagram for onboarding step completion with session cookie cache refreshsequenceDiagram
actor User
participant WebApp
participant AuthServer
participant OnboardingPlugin
participant SessionDB
participant Cookie
User->>WebApp: Complete onboarding step
WebApp->>AuthServer: POST /onboarding/step/*
activate AuthServer
AuthServer->>OnboardingPlugin: Handle step
OnboardingPlugin->>SessionDB: Update shouldOnboard and currentOnboardingStep
SessionDB-->>OnboardingPlugin: Updated session row
OnboardingPlugin-->>AuthServer: Step handled
Note over AuthServer: hooks.after middleware runs
AuthServer->>AuthServer: Path check /onboarding/step/* or /onboarding/skip-step/*
AuthServer->>AuthServer: refreshSessionCookie(ctx)
AuthServer->>SessionDB: internalAdapter.findSession(sessionToken)
SessionDB-->>AuthServer: Fresh session and user
AuthServer->>Cookie: setSessionCookie(ctx, session, user)
Cookie-->>AuthServer: Signed cookie with updated cache
deactivate AuthServer
User->>WebApp: Navigate to guarded route
WebApp->>AuthServer: GET /get-session
AuthServer->>Cookie: Read signed cookie cache
Cookie-->>AuthServer: Fresh shouldOnboard and activeOrganizationId
AuthServer-->>WebApp: Session with updated onboarding state
WebApp-->>User: Allow access without redirect loop
Sequence diagram for createOrganization flow with immediate cookie cache refreshsequenceDiagram
actor User
participant WebApp
participant AuthServer
participant SessionDB
participant Cookie
User->>WebApp: Submit createOrganization
WebApp->>AuthServer: POST /onboarding/step/createOrganization
activate AuthServer
AuthServer->>SessionDB: Insert organization row
SessionDB-->>AuthServer: Organization created
AuthServer->>SessionDB: Update sessionTable.activeOrganizationId
SessionDB-->>AuthServer: Session row updated
AuthServer->>AuthServer: refreshSessionCookie(ctx)
AuthServer->>SessionDB: internalAdapter.findSession(sessionToken)
SessionDB-->>AuthServer: Fresh session and user
AuthServer->>Cookie: setSessionCookie(ctx, session, user)
Cookie-->>AuthServer: Signed cookie with updated activeOrganizationId
AuthServer-->>WebApp: Response with organizationId and slug
deactivate AuthServer
User->>WebApp: Continue onboarding
WebApp->>AuthServer: Guarded request uses /get-session
AuthServer->>Cookie: Read signed cookie cache
Cookie-->>AuthServer: Session reflecting activeOrganizationId
AuthServer-->>WebApp: Session
WebApp-->>User: Correct step routing without loop
Class diagram for auth configuration and refreshSessionCookie utilityclassDiagram
class AuthConfig {
+configureAuth(env)
}
class SessionConfig {
+bool cookieCacheEnabled
+number cookieCacheMaxAgeSeconds
}
class HooksAfterMiddleware {
+handle(ctx)
}
class RefreshSessionCookieUtil {
+refreshSessionCookie(ctx)
}
class InternalAdapter {
+findSession(sessionToken)
}
class CookieHelper {
+setSessionCookie(ctx, session, user)
}
class Logger {
+info(message)
+error(message)
}
AuthConfig --> SessionConfig : configures
AuthConfig --> HooksAfterMiddleware : registers
HooksAfterMiddleware --> RefreshSessionCookieUtil : calls
HooksAfterMiddleware --> InternalAdapter : uses via ctx.context.internalAdapter
RefreshSessionCookieUtil --> InternalAdapter : reads fresh session
RefreshSessionCookieUtil --> CookieHelper : reissues signed cookie
RefreshSessionCookieUtil --> Logger : logs errors
HooksAfterMiddleware --> Logger : logs session updates
SessionConfig : cookieCacheEnabled = true
SessionConfig : cookieCacheMaxAgeSeconds = 60
HooksAfterMiddleware : +onboardingPathsPrefixStep = /onboarding/step/
HooksAfterMiddleware : +onboardingPathsPrefixSkip = /onboarding/skip-step/
HooksAfterMiddleware : +getSessionPath = /get-session
RefreshSessionCookieUtil : +safeForAnyAuthenticatedEndpoint
RefreshSessionCookieUtil : +noOpWhenNoSessionToken
RefreshSessionCookieUtil : +toleratesCookieRefreshFailures
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
refreshSessionCookie, consider including more contextual metadata in the error log (e.g.ctx.pathand possibly a redacted/session identifier) so that intermittent cookie refresh failures are easier to trace in production. - The
GenericEndpointContextcast aroundctx.context.sessionis fairly loose; if possible, narrow the context type for this helper (e.g. a custom interface withsessionandinternalAdapter) to avoid repeated casting and catch misuse at compile time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `refreshSessionCookie`, consider including more contextual metadata in the error log (e.g. `ctx.path` and possibly a redacted/session identifier) so that intermittent cookie refresh failures are easier to trace in production.
- The `GenericEndpointContext` cast around `ctx.context.session` is fairly loose; if possible, narrow the context type for this helper (e.g. a custom interface with `session` and `internalAdapter`) to avoid repeated casting and catch misuse at compile time.
## Individual Comments
### Comment 1
<location path="packages/auth/src/utils/refresh-session-cookie.ts" line_range="66-74" />
<code_context>
+ session: fresh.session,
+ user: fresh.user,
+ });
+ } catch (error) {
+ // Never let cookie refresh failures break the mutation - log and continue.
+ // Worst case: cookie cache is stale for `cookieCache.maxAge` seconds.
+ logger.error(
+ `Failed to refresh session cookie cache: ${
+ error instanceof Error ? error.message : String(error)
+ }`,
+ );
+ }
+}
</code_context>
<issue_to_address>
**suggestion:** Log the full error object instead of only the message to preserve stack traces and structured data.
Interpolating only `error.message`/`String(error)` discards the stack and any structured fields. If your logger supports it, prefer passing the error object itself, e.g. `logger.error("Failed to refresh session cookie cache", { error })` or `logger.error(error, "Failed to refresh session cookie cache")`, so you retain full diagnostics when issues occur.
```suggestion
} catch (error) {
// Never let cookie refresh failures break the mutation - log and continue.
// Worst case: cookie cache is stale for `cookieCache.maxAge` seconds.
logger.error("Failed to refresh session cookie cache", { error });
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+66
to
+74
| } catch (error) { | ||
| // Never let cookie refresh failures break the mutation - log and continue. | ||
| // Worst case: cookie cache is stale for `cookieCache.maxAge` seconds. | ||
| logger.error( | ||
| `Failed to refresh session cookie cache: ${ | ||
| error instanceof Error ? error.message : String(error) | ||
| }`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
suggestion: Log the full error object instead of only the message to preserve stack traces and structured data.
Interpolating only error.message/String(error) discards the stack and any structured fields. If your logger supports it, prefer passing the error object itself, e.g. logger.error("Failed to refresh session cookie cache", { error }) or logger.error(error, "Failed to refresh session cookie cache"), so you retain full diagnostics when issues occur.
Suggested change
| } catch (error) { | |
| // Never let cookie refresh failures break the mutation - log and continue. | |
| // Worst case: cookie cache is stale for `cookieCache.maxAge` seconds. | |
| logger.error( | |
| `Failed to refresh session cookie cache: ${ | |
| error instanceof Error ? error.message : String(error) | |
| }`, | |
| ); | |
| } | |
| } catch (error) { | |
| // Never let cookie refresh failures break the mutation - log and continue. | |
| // Worst case: cookie cache is stale for `cookieCache.maxAge` seconds. | |
| logger.error("Failed to refresh session cookie cache", { error }); | |
| } |
6 tasks
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.
Problem
After completing onboarding steps,
RequireOnboardingguards on both `web` and `tanstack` apps would redirect users back to `/onboarding` in a loop. The workaround in place was disabling `session.cookieCache` entirely — meaning every `getSession` call hit the database, defeating the cache's purpose.Root Cause
Better Auth's
cookieCachestores a signed copy of the session payload (includingshouldOnboard,activeOrganizationId) in the session cookie. The onboarding step handlers wrote to the database directly (db.update(sessionTable),adapter.updateOnboardingState), but never re-issued the signed cookie. So the cookie cache returned stale pre-onboarding values on subsequent requests, and the guard looped indefinitely until the cache expired.Fix
Re-enable cookieCache with
maxAge: 60seconds, and refresh the signed cookie after every DB mutation that changes session-relevant state:hooks.aftermiddleware — now intercepts/onboarding/step/*and/onboarding/skip-step/*paths. After the onboarding plugin has runadapter.updateOnboardingState(which setsshouldOnboard: falseon the completion step),refreshSessionCookie()re-reads the fresh session viainternalAdapter.findSessionand re-issues the signed cookie. The very next guard check reads the correct values — no loop.createOrganizationhandler — also callsrefreshSessionCookie()inline after writingactiveOrganizationIdto the session row, so the cookie reflects the org even mid-flow (before the completion step fires).New util —
packages/auth/src/utils/refresh-session-cookie.tswrapsinternalAdapter.findSession + setSessionCookiewith error isolation so a cookie refresh failure never breaks the underlying mutation.Files Changed
packages/auth/src/auth.ts— re-enablecookieCache, importrefreshSessionCookie, expandhooks.after, call increateOrganizationhandlerpackages/auth/src/utils/refresh-session-cookie.ts— new helperTest Plan
bun run check-typespasses on all workspacesTrade-offs / Notes
maxAge: 60smeans in the absolute worst case (network failure mid-refresh) a user could see a stale redirect for up to 60 seconds. In practice the cookie is always refreshed before the response is sent.getSession({ fetchOptions: { cache: "no-store" } })guards remain unchanged — they bypass HTTP cache, which is orthogonal to the signed-cookie cache.Summary by Sourcery
Re-enable session cookie caching and ensure session cookies are refreshed after onboarding and organization creation to prevent onboarding redirect loops.
Enhancements: