fix: standardize auth - single source of truth, getUser() for authz#154
Conversation
…thz, consistent admin rule
|
CodeAnt AI is reviewing your PR. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
codeant-ai
Bot
added
the
size:L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces ChangesCentralized Auth Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. 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 |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| }: { | ||
| children: React.ReactNode; | ||
| }) { | ||
| const { isAdmin } = await getAuthState().catch(() => ({ isAdmin: false, user: null, role: null })); |
There was a problem hiding this comment.
Suggestion: Fetching auth state in the root layout forces global per-request auth work for every page render, including a potential fallback database lookup in getAuthState(), which can significantly degrade response times and disable static optimization site-wide. Move this call out of the root layout into only the routes/components that actually need admin state. [performance]
Severity Level: Critical 🚨
❌ All pages incur Supabase auth lookup per request.
⚠️ Static optimization disabled because root layout uses cookies.Steps of Reproduction ✅
1. Start the application with the App Router so all pages use the root layout in
`app/layout.tsx:109-180` (`RootLayout`).
2. Request any route (e.g., `/interview-experience` or `/blog`); Next.js will execute
`RootLayout` for the render, hitting line 114 where `const { isAdmin } = await
getAuthState().catch(...)` is called.
3. Follow the call into `lib/auth.ts:80-103` where `getAuthState()` obtains a Supabase
server client via `getServerSupabase()` (lines 29-55), then calls `getVerifiedUser()`
(lines 63-72) which invokes `supabase.auth.getUser()` against the Supabase Auth server on
every request.
4. For authenticated non-admin users, observe that `getAuthState()` additionally performs
a database query against the `users` table at `lib/auth.ts:92-99`
(`.from('users').select('user_role').eq('id', user.id).maybeSingle()`), meaning the global
layout performs Supabase auth I/O (and often a DB lookup) for every page render, making
the root layout fully dynamic, disabling static optimization, and adding unnecessary
per-request latency even for pages that do not need admin state.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/layout.tsx
**Line:** 114:114
**Comment:**
*Performance: Fetching auth state in the root layout forces global per-request auth work for every page render, including a potential fallback database lookup in `getAuthState()`, which can significantly degrade response times and disable static optimization site-wide. Move this call out of the root layout into only the routes/components that actually need admin state.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| <MotionProvider> | ||
| <AnnouncementBanner /> | ||
| <SiteHeader /> | ||
| <SiteHeader isAdmin={isAdmin} /> |
There was a problem hiding this comment.
Suggestion: The SiteHeader component does not accept an isAdmin prop, so this value is dropped and never affects rendering. This creates a contract mismatch where server-fetched auth state is computed but not actually used; either add a typed prop to SiteHeader and consume it, or remove this prop and rely on a single auth source inside the header. [api mismatch]
Severity Level: Major ⚠️
⚠️ Admin header never reflects server-side admin status.
⚠️ Unused prop adds confusion around auth wiring.Steps of Reproduction ✅
1. Run the Next.js app so all routes are wrapped by the root layout defined in
`app/layout.tsx:109-180` (`export default async function RootLayout`).
2. Make any request (e.g., the home page `/`) which causes `RootLayout` in
`app/layout.tsx` to execute and compute `{ isAdmin }` via `getAuthState()` at line 114.
3. Observe that `RootLayout` renders the header as `<SiteHeader isAdmin={isAdmin} />` at
`app/layout.tsx:169`, passing the computed admin flag as a prop.
4. Inspect the `SiteHeader` implementation in `components/common/site-header.tsx:22-135`
and see it is declared as `export function SiteHeader()` with no props parameter and no
usage of `props` or `isAdmin`, so the React props object (including `isAdmin`) is passed
at runtime but never read, meaning the server-fetched admin state has no effect on header
rendering.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/layout.tsx
**Line:** 169:169
**Comment:**
*Api Mismatch: The `SiteHeader` component does not accept an `isAdmin` prop, so this value is dropped and never affects rendering. This creates a contract mismatch where server-fetched auth state is computed but not actually used; either add a typed prop to `SiteHeader` and consume it, or remove this prop and rely on a single auth source inside the header.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| response = NextResponse.next({ request: { headers: request.headers } }); | ||
| response.cookies.set({ name, value, ...options }); | ||
| }, | ||
| remove(name: string, options: CookieOptions) { | ||
| request.cookies.set({ | ||
| name, | ||
| value: '', | ||
| ...options, | ||
| }); | ||
| response = NextResponse.next({ | ||
| request: { | ||
| headers: request.headers, | ||
| }, | ||
| }); | ||
| response.cookies.set({ | ||
| name, | ||
| value: '', | ||
| ...options, | ||
| }); | ||
| request.cookies.set({ name, value: '', ...options }); | ||
| response = NextResponse.next({ request: { headers: request.headers } }); | ||
| response.cookies.set({ name, value: '', ...options }); |
There was a problem hiding this comment.
Suggestion: Recreating response inside each cookie set/remove callback overwrites previously accumulated Set-Cookie headers. When Supabase writes multiple cookies/chunks in one refresh cycle, only the last one can survive, which can corrupt or break session refresh. Keep a single response object and append cookies to it instead of reinitializing it per cookie write. [logic error]
Severity Level: Critical 🚨
- ❌ Session refresh may drop access or refresh token cookie.
- ⚠️ Auth on `/add-experience` intermittently fails after refresh.
- ⚠️ Users experience unpredictable logouts while adding experiences.Steps of Reproduction ✅
1. Navigate to the Add Interview Experience page using the `/add-experience` link rendered
in `components/common/profile.tsx:36-42` or via the login CTA that sets `targetPath =
'/add-experience'` in `components/common/login.tsx:19-21`, sending a request to
`/add-experience`.
2. That request is processed by the Next.js middleware defined in `middleware.ts:12-55`,
because the middleware config at `middleware.ts:53-55` sets `matcher:
['/add-experience']`, ensuring this path always goes through the middleware.
3. Inside the middleware, a Supabase server client is created at `middleware.ts:17-37`
with cookie handlers; when `supabase.auth.getUser()` is called at `middleware.ts:41-43` to
verify the user and refresh the session, Supabase's auth library writes multiple cookies
(for access token, refresh token, etc.) by invoking the provided `cookies.set` and
`cookies.remove` callbacks once per cookie name.
4. Each cookie write executes the handlers at `middleware.ts:25-33`, where `response` is
reassigned via `response = NextResponse.next({ request: { headers: request.headers } });`
and then `response.cookies.set(...)` is called; this recreation at lines 27 and 32
overwrites any previously accumulated `Set-Cookie` headers so only the last cookie write
survives when `return response;` runs at `middleware.ts:50`, corrupting or partially
updating the Supabase session and causing subsequent authenticated requests on
`/add-experience` to see inconsistent or broken auth state.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** middleware.ts
**Line:** 27:33
**Comment:**
*Logic Error: Recreating `response` inside each cookie `set`/`remove` callback overwrites previously accumulated `Set-Cookie` headers. When Supabase writes multiple cookies/chunks in one refresh cycle, only the last one can survive, which can corrupt or break session refresh. Keep a single response object and append cookies to it instead of reinitializing it per cookie write.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
| export const config = { | ||
| matcher: ['/dashboard/:path*', '/add-experience'], | ||
| matcher: ['/add-experience'], |
There was a problem hiding this comment.
Suggestion: The matcher is now limited to /add-experience, so middleware-based token refresh no longer runs for other authenticated server-rendered routes (such as admin pages) that rely on middleware to persist refreshed cookies. This can cause valid users to appear logged out when access tokens expire outside that path. Expand the matcher to include all routes that need session refresh, not only a single page. [incomplete implementation]
Severity Level: Critical 🚨
- ❌ Admin users redirected to home when tokens expire.
- ⚠️ Layout misreports admin status across authenticated pages.
- ⚠️ Session refresh inconsistent, causing confusing logged-out state.Steps of Reproduction ✅
1. Inspect the middleware configuration at `middleware.ts:53-55`, where `export const
config = { matcher: ['/add-experience'], };` restricts the middleware to only the
`/add-experience` route, meaning `/admin` and other pages never pass through this
middleware.
2. In `lib/auth.ts:24-28`, note the design comment on `getServerSupabase` stating that
cookie writes from Server Components are no-ops and that middleware is expected to handle
session refresh; the cookie `set` and `remove` implementations at `lib/auth.ts:35-52` wrap
`cookieStore.set(...)` in `try`/`catch`, silently ignoring writes when called from a
Server Component context.
3. Observe that server-rendered components use `getAuthState()` for authorization: the
root layout calls it at `app/layout.tsx:114-25` (to derive `isAdmin` for the UI) and the
admin page calls it at `app/admin/page.tsx:13-19`, redirecting non-admin users to `'/'`
via `redirect('/')` at `app/admin/page.tsx:15-17`.
4. When a logged-in admin user’s Supabase auth tokens expire while they navigate directly
to `/admin` or other pages that rely on the root layout, the middleware does not run
because the matcher excludes `/admin`, so session cookies are never refreshed;
`getAuthState()` in `app/admin/page.tsx:13-16` reads stale cookies without a successful
refresh, returns `isAdmin` as `false`, and the admin page redirects to `'/'`, causing a
valid admin to appear logged out and blocking access to admin functionality until they hit
`/add-experience` or another route that refreshes the session.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** middleware.ts
**Line:** 54:54
**Comment:**
*Incomplete Implementation: The matcher is now limited to `/add-experience`, so middleware-based token refresh no longer runs for other authenticated server-rendered routes (such as admin pages) that rely on middleware to persist refreshed cookies. This can cause valid users to appear logged out when access tokens expire outside that path. Expand the matcher to include all routes that need session refresh, not only a single page.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/layout.tsx (1)
109-114: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid putting Supabase auth resolution in the root layout.
Line 114 makes every page render wait on
getAuthState()and its cookie/Supabase work just to decorate the header. Prefer the new/api/auth/meendpoint or a small dynamic header/admin-link component so public pages don’t inherit per-request auth overhead.🤖 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 `@app/layout.tsx` around lines 109 - 114, The RootLayout component is resolving Supabase auth with getAuthState() during every page render, which adds unnecessary per-request overhead for all pages. Move this auth lookup out of app/layout.tsx and into a smaller dynamic header or admin-link component, or switch the header logic to consume the new /api/auth/me endpoint so only the authenticated UI does the work. Keep the RootLayout focused on static layout composition and reference RootLayout and getAuthState when relocating the auth-dependent logic.
🤖 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 `@app/layout.tsx`:
- Line 169: The SiteHeader usage is passing an unused isAdmin prop even though
SiteHeader is declared with no props, so the JSX will not type-check. Update the
SiteHeader component signature to accept and use a typed admin prop if it
actually needs this state, or remove the isAdmin prop from the SiteHeader render
in app/layout.tsx and also drop the getAuthState() call if the header does not
depend on admin state.
In `@components/common/profile.tsx`:
- Around line 16-18: The profile menu admin check is too narrow because it only
treats user.role === 'admin' as admin, so users with the superadmin role will
miss the Admin Panel link. Update the admin gating in the profile menu component
that uses useAuth and the isAdmin flag to match the same rule as requireAdmin(),
preferably by using the server-provided isAdmin value from /api/auth/me, or by
including both canonical admin roles in the check.
In `@lib/auth.ts`:
- Around line 67-72: The `getUser()` handling in `auth.ts` is collapsing all
failures into `null`, which hides unexpected auth or network errors. Update the
logic around `supabase.auth.getUser()` so the expected no-session case still
returns `null`, but any real `error` is rethrown or surfaced instead of being
swallowed; use the `getUser` call and its `error`/`user` result to distinguish
these paths.
In `@middleware.ts`:
- Around line 53-54: Broaden the middleware matcher in middleware.ts so requests
for Server Components that rely on getAuthState() also pass through the auth
refresh logic. Update the config.matcher currently limited to /add-experience to
include the routes that render app/layout.tsx and app/admin/page.tsx, so
lib/auth.ts can refresh cookies before those Server Component reads; the
/api/auth/me handler does not need changes.
---
Nitpick comments:
In `@app/layout.tsx`:
- Around line 109-114: The RootLayout component is resolving Supabase auth with
getAuthState() during every page render, which adds unnecessary per-request
overhead for all pages. Move this auth lookup out of app/layout.tsx and into a
smaller dynamic header or admin-link component, or switch the header logic to
consume the new /api/auth/me endpoint so only the authenticated UI does the
work. Keep the RootLayout focused on static layout composition and reference
RootLayout and getAuthState when relocating the auth-dependent logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17774e89-bef5-410c-afb7-3b3ed5d69ab8
📒 Files selected for processing (9)
actions/admin.tsapp/admin/page.tsxapp/api/admin/stats/route.tsapp/api/auth/me/route.tsapp/layout.tsxcomponents/common/profile.tsxlib/auth.tsmiddleware.tsutils/supabase/middleware.ts
💤 Files with no reviewable changes (1)
- utils/supabase/middleware.ts
| const { | ||
| data: { user }, | ||
| error, | ||
| } = await supabase.auth.getUser(); | ||
| if (error) return null; | ||
| return user; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In @supabase/supabase-js v2.108.0, what error does auth.getUser() return when no session is present, and how should it be distinguished from network or auth service failures?
💡 Result:
In @supabase/supabase-js v2.108.0, when no session is present, auth.getUser returns an object where the data.user property is null, and the error property is typically null (or specifically indicates no error occurred, as the absence of a session is a valid state) [1][2]. To distinguish between the absence of a session and actual service failures (such as network issues or auth server errors), you should inspect the returned error object: 1. No Session (Expected State): If data.user is null and error is null, this simply means the client has no valid session. This is the expected behavior when a user is not logged in [1][2]. 2. Auth/Network Failures: If the error property is populated, it will contain an AuthError (often an AuthApiError or similar) [3][4]. These errors, such as a 400 status code with a "session_not_found" or other service-level messages, indicate that the attempt to retrieve the user failed due to an issue with the request, the token, or the connection to the Supabase Auth server [4][5]. For robust error handling, you should prioritize checking if the error object is truthy before assuming that a null user signifies a logged-out state [5]. If an error exists, you can inspect its properties (e.g., status or code) to determine if it is a transient network issue or a persistent service-side failure [4]. Note that auth.getUser performs a network request to the Supabase Auth server to verify the authenticity of the session [6][7]. This distinguishes it from auth.getSession, which primarily reads from local storage and should not be used for critical authorization checks on the server [8][7].
Citations:
- 1: fix:
getUserreturns null if there is no session supabase/auth-js#876 - 2: auth.getUser returns an error: Auth Session Missing v2.43.2 supabase/supabase-js#1702
- 3: https://github.com/supabase/supabase-js/releases/tag/v2.108.0
- 4: fix(auth): do not remove session when getUser returns session_not_found supabase/supabase-js#2166
- 5: auth.getUser() returns null user and AuthSessionMissingError in supabase-js@2.42.5 supabase/supabase-js#1024
- 6: https://supabase.com/docs/reference/javascript/auth-getuser
- 7: https://supabase.com/docs/reference/javascript/v1/auth-api-getuser
- 8: https://supabase.com/docs/reference/javascript/auth-getsession
Don't collapse getUser() errors into null. auth.getUser() returns error = null for the expected no-session case; unexpected auth/network failures should be rethrown so protected routes fail with a real server error instead of 401.
🤖 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 `@lib/auth.ts` around lines 67 - 72, The `getUser()` handling in `auth.ts` is
collapsing all failures into `null`, which hides unexpected auth or network
errors. Update the logic around `supabase.auth.getUser()` so the expected
no-session case still returns `null`, but any real `error` is rethrown or
surfaced instead of being swallowed; use the `getUser` call and its
`error`/`user` result to distinguish these paths.
…er, superadmin role in profile, remove layout auth overhead
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:L
|
CodeAnt AI Incremental review completed. |
1 participant
User description
Problem
Auth logic was duplicated and inconsistent across 6 files with different admin rules, causing real bugs (admin stats API silently 403'd because it only checked
app_metadatawhile other files had a fallback to theuserstable). Authorization used insecuregetSession()instead of verifiedgetUser().Changes
New:
lib/auth.ts— single source of truthgetServerSupabase()— the one server-side Supabase clientgetVerifiedUser()— usesgetUser()(JWT verified by auth server), notgetSession()getAuthState()— canonical admin rule:app_metadata.roleORusers.user_role∈ {admin, superadmin}requireAdmin()— throwsAuthError(401/403)for guardsRefactored consumers
app/admin/page.tsxgetAuthState()actions/admin.tsgetSession()checkrequireAdmin()app/api/admin/statsrequireAdmin()app/api/pipelinegetAuthState()app/api/auth/megetAuthState()Cleanup
/dashboardmatcher (no such route), switched togetUser()utils/supabase/middleware.tshandleLogoutfromprofile.tsxVerified
tsc --noEmitcleanMerge order
Merge this before
feat/react-doctor-score-100andfeat/content-pipeline-extension— both depend onlib/auth.ts.Summary by CodeRabbit
New Features
/api/auth/me) that returns admin and role info./feed.xml).Bug Fixes
CodeAnt-AI Description
Standardize sign-in checks and admin access across the site
What Changed
adminandsuperadmin, which fixes cases where valid admins were not treated as adminsImpact
✅ Fewer admin access mismatches✅ Clearer unauthorized and forbidden responses✅ Faster login-state updates in the UI💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.