Conversation
mcstepp
requested review from
MichaelUnkey,
chronark,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis set of changes refactors authentication and session management across the dashboard application. Key updates include replacing a global mutex with a per-refresh-token mutex to serialize concurrent refresh token requests, explicit handling and propagation of access and refresh tokens alongside session tokens, and a new React authentication provider for client-side session state management. The session refresh endpoint and supporting libraries now use the refresh token for access token renewal, setting multiple secure cookies with strict same-site policies. Type definitions are updated for clearer distinction between valid and invalid sessions, and additional user role information is incorporated throughout the authentication flow. The layout is wrapped in an authentication provider to enforce user authentication at the UI level. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthProvider (React)
participant API (/api/auth/session)
participant API (/api/auth/refresh)
participant Auth Library
Client->>AuthProvider (React): Mounts or periodic check
AuthProvider (React)->>API (/api/auth/session): GET /api/auth/session
API (/api/auth/session)->>Auth Library: getAuth()
Auth Library-->>API (/api/auth/session): Session info or 401
API (/api/auth/session)-->>AuthProvider (React): 200 OK or 401 Unauthorized
alt Not authenticated and requireAuth
AuthProvider (React)->>Client: Redirect to sign-in
else Authenticated
AuthProvider (React)->>Client: Render children
end
loop Periodic refresh
AuthProvider (React)->>API (/api/auth/refresh): POST /api/auth/refresh
API (/api/auth/refresh)->>Auth Library: refreshAccessToken(refreshToken)
Auth Library-->>API (/api/auth/refresh): New tokens/session
API (/api/auth/refresh)-->>AuthProvider (React): 200 OK with new cookies or 401/500
alt Refresh failed and requireAuth
AuthProvider (React)->>Client: Redirect to sign-in
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/dashboard/lib/auth/workos.ts (1)
768-813: 🛠️ Refactor suggestionDuplicate cookie-setting logic.
You are setting session, access, and refresh tokens in multiple places (e.g.,
completeOrgSelectionandcompleteOAuthSignIn). Consider extracting cookie-setting into a shared helper function to reduce duplication and improve maintainability.
🧹 Nitpick comments (14)
apps/dashboard/lib/auth/cookies.ts (1)
86-86: Comment is slightly inconsistent with function signatureThe comment has been updated to refer to a "Response object", but the function signature still explicitly uses and returns a
NextResponsetype. Consider either:
- Keeping the comment as "NextResponse object" to match the signature, or
- Modifying the function to work with the more general
Responsetype if that's the intentionapps/dashboard/app/(app)/api/auth/session/route.ts (1)
1-18: Good implementation of session verification endpointThis new endpoint provides a clean way to verify user authentication status. It utilizes the enhanced
getAuthfunction with caching and mutex protection for handling concurrent requests efficiently. The error handling is appropriate with proper logging.Consider adding more context to the error log by including the specific error message:
- console.error("Error checking session:", error); + console.error("Error checking session:", error instanceof Error ? error.message : error);apps/dashboard/app/(app)/api/auth/refresh/route.ts (4)
10-12: Consider multi-instance concurrency.Using a single global mutex (
refreshInProgressandrefreshPromise) only works within a single runtime instance. In a distributed environment (multiple replicas or serverless), concurrent refresh attempts might still occur. Consider a more robust approach (e.g., shared cache or database-locked approach) if you anticipate scaling horizontally.
16-18: Check the fallback priority of refresh token retrieval.You are prioritizing the header value
"x-refresh-token"over the cookie value. Ensure this aligns with security best practices (e.g., only trust the cookie if the header is not provided or vice versa) and document the expected usage strategy.
53-54: Clarify the comment for max-age calculation.Though this logic calculates the difference for a 7-day window, the comment references “// 7 days in seconds” which may be confusing if
expiresAtis dynamically determined. Consider clarifying in code or comments thatexpiresAtis set to 7 days from now elsewhere and that you’re computing the number of remaining seconds until that time.
76-80: Refine error handling for invalid refresh attempts.When
refreshAccessTokenfails, you respond with a generic 401. Consider providing more granular messages or codes, especially if the user experience could benefit from knowing whether the refresh token is expired, revoked, or malformed.apps/dashboard/lib/auth/workos.ts (2)
95-129: Refactor or remove the "butt" error log message.At line 126, the error log
"console.error('butt', error);"may be unintentional or unprofessional. Consider replacing it with something more descriptive like"console.error('Refresh error:', error);"for clarity and consistency.- console.error("butt", error); + console.error("Refresh error:", error);
119-123: Consider retrieving user role during refresh.You’re returning
role: nullby default afterauthenticateWithRefreshToken. If user roles are critical to your logic, explore whether the library offers a way to fetch the user’s role or re-check membership/permissions post-refresh to keep the session data accurate.apps/dashboard/lib/auth/get-auth.ts (3)
13-35: Validate session caching approach carefully.
validateSessionCachedmay lead to stale session data if changes occur outside of its lifetime. Consider introducing an eviction or expiration policy in case the session is revoked or updated. Alternatively, incorporate a more robust invalidation mechanism if real-time updates matter.
37-45: Expand concurrency handling approach.Similar to the refresh endpoint, this local in-memory mutex won’t prevent concurrent refreshes across multiple server instances. Evaluate whether you need a distributed lock or a short expiration-based approach in high-availability scenarios.
95-106: Gracefully handle missing tokens in partial scenarios.Currently, if either
sessionTokenorrefreshTokenis missing, you immediately returnnulluser data. If your flow sometimes allows usage without refresh tokens (e.g., ephemeral sessions), consider adjusting logic or adding a fallback/renew step if that scenario is valid.apps/dashboard/providers/AuthProvider.tsx (3)
35-57: Verify error handling for authentication failures.The checkAuth function implements proper error handling and loading state management. However, consider whether logging the full error object to the console is appropriate in a production environment, as it might expose sensitive information.
Consider replacing the console.error with more structured error logging:
- console.error("Auth check failed:", error); + console.error("Auth check failed:", error instanceof Error ? error.message : "Unknown error");
60-82: Consider implementing exponential backoff for failed refresh attempts.The refreshSession function handles errors appropriately, but it doesn't implement any retry mechanism. For network issues, implementing exponential backoff could improve resilience.
Consider adding a simple retry mechanism with backoff:
const refreshSession = useCallback(async () => { + const maxRetries = 3; + let retryCount = 0; + + const attemptRefresh = async (): Promise<boolean> => { try { const response = await fetch("/api/auth/refresh", { method: "POST", credentials: "include", }); setIsAuthenticated(response.ok); if (!response.ok && requireAuth) { router.push(redirectTo); } return response.ok; } catch (error) { - console.error("Session refresh failed:", error); + console.error("Session refresh failed:", error instanceof Error ? error.message : "Unknown error"); setIsAuthenticated(false); + + if (retryCount < maxRetries) { + retryCount++; + const backoffTime = Math.pow(2, retryCount) * 1000; // Exponential backoff + await new Promise(resolve => setTimeout(resolve, backoffTime)); + return attemptRefresh(); + } + if (requireAuth) { router.push(redirectTo); } return false; } + }; + + return attemptRefresh(); }, [requireAuth, router, redirectTo]);
104-114: Consider adding a loading state for the UI.The current implementation doesn't render children during the loading state for routes that require authentication. Consider showing a loading indicator instead of a blank screen.
return ( <AuthContext.Provider value={{ isAuthenticated, isLoading, refreshSession, }} > - {!requireAuth || (requireAuth && isAuthenticated) ? children : null} + {!requireAuth ? children : isLoading ? <LoadingSpinner /> : isAuthenticated ? children : null} </AuthContext.Provider> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
apps/dashboard/app/(app)/api/auth/refresh/route.ts(1 hunks)apps/dashboard/app/(app)/api/auth/session/route.ts(1 hunks)apps/dashboard/app/(app)/layout.tsx(2 hunks)apps/dashboard/app/auth/actions.ts(1 hunks)apps/dashboard/app/auth/sso-callback/[[...sso-callback]]/route.ts(0 hunks)apps/dashboard/lib/auth.ts(3 hunks)apps/dashboard/lib/auth/base-provider.ts(1 hunks)apps/dashboard/lib/auth/cookies.ts(1 hunks)apps/dashboard/lib/auth/get-auth.ts(1 hunks)apps/dashboard/lib/auth/types.ts(2 hunks)apps/dashboard/lib/auth/workos.ts(8 hunks)apps/dashboard/lib/trpc/routers/user/switchOrg.ts(1 hunks)apps/dashboard/package.json(1 hunks)apps/dashboard/providers/AuthProvider.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/app/auth/sso-callback/[[...sso-callback]]/route.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
apps/dashboard/app/auth/actions.ts (1)
apps/dashboard/lib/auth/cookies.ts (1)
SetSessionCookie(103-120)
apps/dashboard/lib/auth/base-provider.ts (1)
apps/dashboard/lib/auth/types.ts (1)
SessionRefreshResult(123-129)
apps/dashboard/app/(app)/api/auth/refresh/route.ts (2)
apps/dashboard/lib/auth/cookies.ts (1)
getCookie(24-27)apps/dashboard/lib/auth/types.ts (4)
UNKEY_REFRESH_TOKEN(7-7)UNKEY_SESSION_COOKIE(4-4)UNKEY_ACCESS_TOKEN(5-5)UNKEY_ACCESS_MAX_AGE(6-6)
apps/dashboard/app/(app)/layout.tsx (4)
apps/dashboard/providers/AuthProvider.tsx (1)
AuthProvider(21-115)apps/dashboard/components/ui/sidebar.tsx (1)
SidebarProvider(600-600)apps/dashboard/components/navigation/sidebar/app-sidebar/index.tsx (1)
AppSidebar(29-130)apps/dashboard/components/navigation/sidebar/sidebar-mobile.tsx (1)
SidebarMobile(9-28)
apps/dashboard/providers/AuthProvider.tsx (4)
apps/dashboard/lib/trpc/context.ts (1)
createContext(7-31)apps/dashboard/lib/trpc/routers/index.ts (1)
router(79-206)apps/dashboard/lib/auth/utils.ts (1)
requireAuth(10-16)apps/dashboard/lib/auth/types.ts (1)
UNKEY_ACCESS_MAX_AGE(6-6)
apps/dashboard/lib/auth/get-auth.ts (3)
apps/dashboard/lib/auth.ts (1)
getAuth(30-37)apps/dashboard/lib/auth/cookies.ts (1)
getCookie(24-27)apps/dashboard/lib/auth/types.ts (2)
UNKEY_SESSION_COOKIE(4-4)UNKEY_REFRESH_TOKEN(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Docs
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (19)
apps/dashboard/package.json (1)
72-72:Details
✅ Verification successful
Version update aligns with authentication improvements
The update to WorkOS node SDK from ^7.35.1 to ^7.46.0 is appropriate as it likely contains improvements related to authentication and token handling that support the refresh token flow being implemented in this PR.
🌐 Web query:
What features or changes were added in @workos-inc/node between versions 7.35.1 and 7.46.0 related to authentication or token management?💡 Result:
Between versions 7.35.1 and 7.46.0 of
@workos-inc/node, several authentication and token management features/changes were introduced:Authentication Enhancements
- Passkey support: Added
Passkeyto theAuthenticationMethodunion type, enabling passwordless authentication flows[1][3].- MFA TOTP authentication: Added support for time-based one-time passwords during MFA challenges, including error handling for pending authentication tokens[8].
- Organization domain verification: Made
verificationTokenoptional onOrganizationDomainto align with API behavior[1][3].Token Management Improvements
- Widget tokens: Introduced server-side token generation for long-lived user management widgets to avoid frontend token expiration issues[2][6].
- Session refresh: Updated
Session#refresh()to return all JWT claims instead of partial data[3][5].- OAuth token handling:
- Added
oauthTokensproperty toAuthenticationResponsefor expanded OAuth data[1][3].- Added PKCE code challenge support with
codeVerifierparameter in authentication flows[4][8].Security Features
- Stable auth tokens: Implemented widget token system allowing 1-hour stable tokens without changing global session durations[2][6].
- Idempotency keys: Added support for
Idempotency-Keyheaders in API requests to prevent duplicate operations[8].- SCIM role assignment: Enabled role updates via identity provider synchronization in AuthKit[5][10].
API Changes
- Added
permissionsfield to organization roles and events APIs[3][5].- Exposed
warrant_tokenattribute from FGA check endpoints[3][8].- Made
scopesfield optional in authentication requests[3][8].These changes enhance authentication flows, provide better token lifecycle management, and improve integration capabilities with enterprise security systems. For implementation details, refer to the WorkOS API Reference[8] and RBAC guide[10].
Citations:
- 1: https://workos.com/docs/sdks/node
- 2: https://www.youtube.com/watch?v=9GXKMPYC5dE
- 3: https://github.com/workos-inc/workos-node/releases
- 4: https://workos.com/blog/the-complete-guide-to-oauth
- 5: https://workos.com/changelog
- 6: https://workos.com/blog/user-management-dashboard-with-node
- 7: https://www.npmjs.com/package/@workos-inc/node
- 8: https://workos.com/docs/reference
- 9: https://workos.com/docs/sdks
- 10: https://workos.com/blog/rbac-with-workos-and-node
Authentication and Token Management Upgrade Confirmed
The update from version ^7.35.1 to ^7.46.0 of the WorkOS Node SDK now brings several significant improvements that benefit our refresh token flow and overall authentication handling:
- Improved OAuth Support: The new version introduces expanded OAuth token handling, including the addition of the
oauthTokensproperty and enhanced PKCE code challenge support.- Enhanced Session Refresh: The updated
Session#refresh()method now returns all JWT claims, providing more comprehensive token details.- Authentication Enhancements: Additional improvements like passkey support and MFA TOTP authentication further strengthen our authentication processes.
Overall, this dependency bump is well-aligned with the PR’s goals of improving authentication and token management.
apps/dashboard/lib/trpc/routers/user/switchOrg.ts (1)
10-10: Improved token naming for better clarity and consistencyRenaming from
newTokentosessionTokenimproves code clarity by explicitly indicating this is a session token. This naming change helps maintain consistency with other authentication-related code in the application.Also applies to: 13-13
apps/dashboard/app/auth/actions.ts (1)
188-190: Improved naming consistency for tokensThe variable renaming from
newTokentosessionTokenimproves clarity and aligns with the token naming convention used throughout the codebase, making the code more consistent and easier to maintain.apps/dashboard/app/(app)/layout.tsx (2)
6-6: Appropriate import for the new AuthProviderThe AuthProvider import is correctly added to support the layout-level authentication requirements.
33-85: Good implementation of layout-level authenticationWrapping the entire layout with AuthProvider and setting
requireAuth={true}properly enforces authentication at the layout level. This is an excellent architectural decision that centralizes authentication logic and ensures all routes within this layout are protected.apps/dashboard/lib/auth.ts (3)
1-1: Improved import naming for clarityRenaming the import from
noCacheGetAuthtobaseGetAuthbetter reflects the function's purpose as the base implementation for the cachedgetAuthfunction.
10-10: Enhanced authentication with role informationAdding the
roleproperty to theGetAuthResulttype properly extends the authentication system to support role-based authorization. This is a forward-thinking change that maintains backward compatibility while enabling more granular access control.
31-31: Consistent use of renamed importThe code correctly uses the renamed
baseGetAuthfunction, maintaining consistency with the import change.apps/dashboard/lib/auth/base-provider.ts (1)
31-31: Improved token handling with OAuth best practicesThe method signature change from
refreshSession(sessionToken)torefreshAccessToken(currentRefreshToken)reflects a shift to a more robust OAuth-style token management approach. This change:
- Separates concerns between access and refresh tokens
- Better aligns with OAuth 2.0 patterns
- Improves the application's ability to handle token expiration gracefully
This is a significant improvement to the authentication system that enhances security and user experience.
apps/dashboard/lib/auth/workos.ts (1)
73-78: Ensure role retrieval is feasible.You’re assigning
role: authResult.role ?? nullinvalidateSession. Confirm that the role can be consistently retrieved for all valid sessions. If some of your flows never assign a role, ensure calling code gracefully handlesnull.apps/dashboard/providers/AuthProvider.tsx (5)
1-5: Well-structured imports and client-side directive.The "use client" directive is correctly placed at the top, and all necessary imports are present. The combination of Next.js navigation and React hooks will support client-side authentication management.
8-19: Clear context definition with appropriate defaults.The AuthContext is well-typed with the necessary properties for authentication state management. Default values are sensibly set with isAuthenticated as false and isLoading as true.
21-33: Good component API design with sensible defaults.The AuthProvider component has a clean API with optional parameters and reasonable defaults. The state initialization is appropriate for the authentication flow.
85-97: Good practice refreshing before token expiration.Setting up a refresh interval that triggers before the token expires (with a 6-second buffer) is a good practice to ensure continuous authentication. The cleanup on component unmount prevents memory leaks.
88-90:Details
❓ Verification inconclusive
Verify the refresh buffer is sufficient.
The current refresh is set to occur 6 seconds before token expiration. Ensure this buffer is sufficient to account for network latency and potential server processing time.
🏁 Script executed:
#!/bin/bash # Check for network latency considerations in other parts of the codebase grep -r "setInterval.*refresh" --include="*.ts" --include="*.tsx" .Length of output: 195
Action: Reassess the 6-Second Refresh Buffer
The token refresh in
apps/dashboard/providers/AuthProvider.tsxis set to trigger 6 seconds before token expiration (UNKEY_ACCESS_MAX_AGE - 6000). While this is the only instance we found in the codebase, please verify that a 6-second window reliably accommodates both expected network latencies and potential server processing delays. If your deployment environment occasionally exhibits higher latency, consider either testing this buffer under realistic conditions or making the value configurable to better adapt to varying operational scenarios.apps/dashboard/lib/auth/types.ts (4)
5-7: Good token naming conventions and appropriate expiration time.The constants are well-named and follow a consistent pattern. Setting the access token expiration to 5 minutes aligns with security best practices by limiting the window of opportunity if a token is compromised.
102-121: Well-structured session validation types.The restructured session validation types provide clearer type safety and better support for type guards. This is a good improvement that makes the code more maintainable and less error-prone.
123-129: Improved token management in SessionRefreshResult.Renaming
newTokentosessionTokenand adding optional fields for access and refresh tokens enhances clarity and aligns with the separation of concerns in token management.
134-134: Good addition of role information for RBAC support.Adding the optional role field to SessionData enables role-based access control, which is an important security feature for controlling user permissions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/api/auth/session/route.ts (2)
6-6: Consider removing or rewording comment.The comment "Use your robust cached and mutex-protected getAuth function" reads like a development note rather than documentation. Consider rewording to something more objective like "Using cached and mutex-protected authentication".
- // Use your robust cached and mutex-protected getAuth function + // Using cached and mutex-protected authentication
7-7: Consider adding type annotation for clarity.Adding a type annotation for the destructured result from
getAuth()would improve code readability and maintainability.- const { userId } = await getAuth(); + const { userId }: { userId: string | null } = await getAuth();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
apps/dashboard/app/(app)/api/auth/session/route.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/dashboard/app/(app)/api/auth/session/route.ts (2)
apps/dashboard/lib/auth/get-auth.ts (1)
getAuth(96-142)apps/dashboard/lib/auth.ts (1)
getAuth(30-37)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/(app)/api/auth/session/route.ts (2)
3-3: Good use of dynamic configuration for authentication endpoint.Setting
dynamic = 'force-dynamic'ensures this authentication endpoint is never cached, which is appropriate for session verification that should always reflect the current state.
4-19: Well-structured session verification endpoint.This implementation correctly uses the updated
getAuthfunction that now handles refresh tokens with mutex protection. The error handling is appropriate, with clear status codes (401 for unauthenticated users and errors, 200 for authenticated users) and error logging.The function aligns well with the PR objective of fixing the refresh access tokens flow by providing a reliable endpoint to verify authentication status.
ogzhanolguncu
left a comment
ogzhanolguncu
left a comment
There was a problem hiding this comment.
Can we get rid of comments if we don't plan to use them any time soon?
|
what comments are you talking about? |
|
Mostly |
|
I tend to remove comments if they are "not" flagged with TODO, INFO or something |
perkinsjr
left a comment
perkinsjr
left a comment
There was a problem hiding this comment.
See comments, ask questions as needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apps/dashboard/lib/auth/base-provider.ts (1)
359-366: 🛠️ Refactor suggestionSign‑out redirect only clears the session cookie
redirectToLogindeletesconfig.cookieNamebut leavesUNKEY_ACCESS_TOKENandUNKEY_REFRESH_TOKENintact, which could:
- Trigger another automatic refresh attempt on the next request, causing a redirect loop.
- Expose stale refresh tokens if the redirect target is a different domain.
Consider deleting all auth‑related cookies, e.g.:
response.cookies.delete(config.cookieName); +response.cookies.delete("unkey-access-token"); +response.cookies.delete("unkey-refresh-token");(or better, reuse the constants from
types.ts).
🧹 Nitpick comments (3)
apps/dashboard/lib/auth/get-auth.ts (1)
59-66: Missing timeout / abort‑signal for internal refresh requestThe POST to
/api/auth/refreshcan in theory hang forever (network hiccup, cold‑start, bug in the route).
That stalls every request waiting on this promise, defeating the mutex and potentially exhausting lambda concurrency.Add an
AbortControllerwith a conservative timeout (e.g. 5 s) and surface a retriable error:+const ctrl = new AbortController(); +const id = setTimeout(() => ctrl.abort(), 5000); const refreshResult = await fetch(`${baseUrl}/api/auth/refresh`, { method: "POST", credentials: "include", headers: { "x-refresh-token": refreshToken }, + signal: ctrl.signal, }); +clearTimeout(id);This keeps the caller responsive and allows graceful degradation.
apps/dashboard/app/(app)/api/auth/refresh/route.ts (1)
50-67: Cookies are set, but response is cacheable by defaultBrowsers / intermediaries might cache
application/jsonresponses when no explicit headers are present.
Because this payload contains sensitive session data, addCache-Control: no-store(and optionallyPragma: no-cache) to be safe:const response = Response.json({ success: true, session }); +response.headers.set("Cache-Control", "no-store");This ensures nothing persists the response inadvertently.
apps/dashboard/lib/auth/base-provider.ts (1)
347-350:startsWithmay over‑match public paths
/publicwill match/publicity.
If you expect exact path prefixes ending with/, document or tighten the check:const isPublic = publicPaths.some((path) => - pathname.startsWith(path) + pathname === path || pathname.startsWith(path.endsWith("/") ? path : `${path}/`) );Minor, but prevents accidental exposure of private routes whose slug begins with a public one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
apps/dashboard/app/(app)/api/auth/refresh/route.ts(1 hunks)apps/dashboard/app/(app)/api/auth/session/route.ts(1 hunks)apps/dashboard/app/(app)/layout.tsx(2 hunks)apps/dashboard/app/auth/actions.ts(1 hunks)apps/dashboard/app/auth/sso-callback/[[...sso-callback]]/route.ts(0 hunks)apps/dashboard/app/new/create-workspace.tsx(1 hunks)apps/dashboard/lib/auth.ts(3 hunks)apps/dashboard/lib/auth/base-provider.ts(3 hunks)apps/dashboard/lib/auth/cookies.ts(3 hunks)apps/dashboard/lib/auth/get-auth.ts(1 hunks)apps/dashboard/lib/auth/types.ts(2 hunks)apps/dashboard/lib/auth/workos.ts(14 hunks)apps/dashboard/lib/trpc/routers/user/switchOrg.ts(1 hunks)apps/dashboard/providers/AuthProvider.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/app/auth/sso-callback/[[...sso-callback]]/route.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/dashboard/app/new/create-workspace.tsx
- apps/dashboard/app/auth/actions.ts
- apps/dashboard/app/(app)/api/auth/session/route.ts
- apps/dashboard/lib/auth.ts
- apps/dashboard/lib/auth/cookies.ts
- apps/dashboard/providers/AuthProvider.tsx
- apps/dashboard/lib/auth/types.ts
- apps/dashboard/app/(app)/layout.tsx
- apps/dashboard/lib/trpc/routers/user/switchOrg.ts
- apps/dashboard/lib/auth/workos.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/dashboard/lib/auth/get-auth.ts (3)
apps/dashboard/lib/auth.ts (1)
getAuth(30-37)apps/dashboard/lib/auth/cookies.ts (1)
getCookie(24-27)apps/dashboard/lib/auth/types.ts (2)
UNKEY_SESSION_COOKIE(4-4)UNKEY_REFRESH_TOKEN(7-7)
apps/dashboard/lib/auth/base-provider.ts (1)
apps/dashboard/lib/auth/types.ts (17)
SessionValidationResult(121-121)SessionRefreshResult(123-129)EmailAuthResult(81-81)VerificationResult(82-85)UserData(177-181)SignInViaOAuthOptions(140-143)OAuthResult(86-90)User(12-21)UpdateOrgParams(161-164)Organization(23-28)MembershipListResponse(98-98)UpdateMembershipParams(166-169)Membership(30-38)OrgInviteParams(171-175)Invitation(146-158)InvitationListResponse(99-99)StateChangeResponse(46-48)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Docs
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/dashboard/app/(app)/api/auth/refresh/route.ts (1)
20-27: Early return leaks no context to caller – consider 400 vs 401When the refresh token is missing, the route returns
401 Unauthorized.
Technically the client is unauthenticated yet also made a malformed request.
Returning400 Bad Requestis often clearer (the client forgot to include mandatory data) and avoids log noise that might be interpreted as auth failures.Double‑check consumer expectations and adjust if appropriate.
|
Closing for a newer branch |
4 participants
What does this PR do?
Appropriately handles access tokens from OAuth and appropriately authenticating with refresh tokens to get new access tokens.
Fixes ENG-1713
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor
Style