fix: add server-side auth gate to tenant layout#1976
Conversation
The [tenantId] layout had no authentication check, allowing unauthenticated users to access Work Apps, Stats, and Settings pages. The Projects page worked by accident because it's a server component whose API call fails with 401, triggering FullPageError's login redirect. This adds a proactive server-side cookie check for `better-auth.session_token` in the tenant layout. If absent, the user is redirected to /login before any page content renders. This protects all current and future routes under /[tenantId]/*. Resolves PRD-6033 Co-authored-by: Cursor <cursoragent@cursor.com>
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR adds a well-implemented server-side authentication gate to the tenant layout, correctly using Next.js patterns and following established codebase conventions.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (2) 💭
💭 1) layout.tsx:15 Extract cookie name to shared constant
Issue: The cookie name 'better-auth.session_token' is hardcoded inline.
Why: The logout/route.ts defines BETTER_AUTH_COOKIES as a named constant array for the same cookie names. Consolidating to a shared constant would improve maintainability and discoverability.
Fix: Consider extracting to a shared constant, though the codebase has mixed patterns (constants in logout, inline strings in api-config.ts), so this is a stylistic preference.
Refs: logout/route.ts:10-15
💭 2) layout.tsx:14-19 Cookie presence vs validity check (acknowledged as intentional)
Issue: The auth gate checks only for cookie presence, not session validity. An expired or revoked token would pass this check.
Why: This is explicitly acknowledged in the PR description as an intentional "lightweight (no API call)" design. The API layer enforces proper session validation, and the worst case is a brief UI flash before API-level redirect. This is a valid defense-in-depth architecture where the layout gate is a UX optimization and the API is the security boundary.
Fix: No change needed — the current approach is appropriate given the documented tradeoffs. If stricter client-side enforcement is ever desired, auth.api.getSession() could be called server-side, but this adds latency to every page load.
Refs: sessionAuth.ts:42-71
✅ APPROVE
Summary: Clean, well-documented security fix that follows established Next.js and codebase patterns. The implementation correctly uses async cookies() and redirect() APIs per Next.js 15+ conventions. The design choice to do a lightweight cookie-presence check (rather than full session validation) is explicitly documented and appropriate — the API layer provides the real security boundary. Ship it! 🚀
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
layout.tsx:14-19 |
Cookie validity check should validate token | Duplicate of Consider #2 — raised by both security-iam and frontend reviewers. Merged into single consideration. |
layout.tsx:10 |
Function signature change from FC to async | Positive observation (INFO level) — correctly follows Next.js App Router patterns seen in peer layouts. |
layout.tsx:10-19 |
Auth check pattern consistency | Positive observation (INFO level) — pattern matches established conventions in logout/route.ts, copilot-token.ts, and api-config.ts. |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
1 | 0 | 1 | 0 | 0 | 0 | 0 |
pr-review-frontend |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-consistency |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
| Total | 6 | 0 | 2 | 0 | 0 | 0 | 4 |
Consolidates all inline 'better-auth.*' cookie name strings into a single `lib/auth/constants.ts` file. Previously the same cookie names were duplicated across the tenant layout, logout route, and API config module. Exports: - BETTER_AUTH_SESSION_TOKEN_COOKIE — used for auth gate checks - BETTER_AUTH_COOKIE_PREFIX — used for cookie filtering - BETTER_AUTH_COOKIES — full list used for logout cleanup Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This delta review covers commit e474a1da which extracts Better Auth cookie name constants to a shared file — directly addressing the "Consider" item from the prior review.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None.
✅ APPROVE
Summary: Clean refactoring that consolidates Better Auth cookie names into a single source of truth. The new constants.ts file follows established naming conventions (SCREAMING_SNAKE_CASE), uses proper TypeScript patterns (as const for tuple typing), and maintains good documentation for these shared constants. All hardcoded cookie strings in agents-manage-ui are now centralized. The prior "Consider" item has been fully addressed. Ship it! 🚢
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
packages/agents-mcp/src/lib/security.ts:244 |
Better Auth cookie name remains hardcoded in MCP packages | These are Speakeasy-generated files (marked "DO NOT EDIT") that cannot import from agents-manage-ui. The split-world is justified by code generation boundaries — no action required. |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-consistency |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 1 | 0 | 0 | 0 | 0 | 0 | 1 |
This reverts commit a9f06e4.
1 participant
Summary
[tenantId]/layout.tsxthat redirects unauthenticated users to/loginbefore any page content renders/[tenantId]/*with a single point of enforcementRoot Cause
The
[tenantId]/layout.tsxhad no authentication check. Client component pages ('use client') like Work Apps, Stats, and Settings rendered immediately without verifying the user's session. Their API calls happened later inuseEffecthooks and errors were swallowed silently or shown as empty states.The Projects page worked by accident because it's a server component that calls
fetchProjects()at render time — when the API returns 401,FullPageErrorcatches it and redirects to/login.How It Works
The layout now checks for the
better-auth.session_tokencookie server-side using Next.jscookies(). If the cookie is absent,redirect('/login')sends a 307 response before any rendering happens. This is lightweight (no API call) and consistent with the existing auth cookie patterns used inapi-config.tsand thelogoutroute.Test Plan
/default/work-appswhile logged out → should redirect to/login/default/statswhile logged out → should redirect to/login/default/settingswhile logged out → should redirect to/login/default/projectswhile logged out → should still redirect to/loginResolves PRD-6033
Made with Cursor