fix: add client-side auth guard to tenant pages (defense-in-depth)#1977
fix: add client-side auth guard to tenant pages (defense-in-depth)#1977amikofalvy wants to merge 1 commit intomainfrom
Conversation
Adds a `useRequireAuth` hook that monitors the client-side session state and redirects to /login if the session becomes invalid. This handles the case where a user's session expires while they are already on a page (the server-side layout cookie check only runs on initial render). Applied to all client-component pages under /[tenantId] that previously had no auth protection: - Work Apps (overview, Slack, GitHub, GitHub installation detail) - Stats (overview, AI calls, tool calls) - Settings 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
|
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
🔴❗ Critical (0) ❗🔴
None
🟠⚠️ Major (0) 🟠⚠️
None
🟡 Minor (0) 🟡
None
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
use-require-auth.ts:20-24Align redirect condition withisAuthenticatedinstead of!userfor consistency
✅ APPROVE
Summary: Clean, well-designed implementation of a defense-in-depth authentication guard. The hook correctly waits for loading to complete before checking auth status, uses router.replace() to avoid polluting browser history, and leverages existing utilities with proper open redirect protection. The security architecture is sound—server-side layout check blocks unauthenticated initial access, while this client-side hook handles the session expiry scenario during active page usage. Ship it! 🚀
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
use-require-auth.ts:20 |
Flash of protected content before redirect | Defense-in-depth design: server-side check blocks unauthenticated users on initial load; this hook handles session expiry. API calls would fail with 401 anyway and FullPageError handles that reactively. UX polish rather than security issue. |
multi-file |
Other tenant pages don't use the hook | Intentional partial rollout per PRD-6033 scope. PR explicitly targets Work Apps, Stats, and Settings. Server-side layout check protects all pages. |
work-apps/page.tsx:9 |
Returned values from hook not used | Intentional flexible design—hook can be used fire-and-forget or with return values. No correctness issue. |
use-require-auth.ts:8-15 |
Existing FullPageError pattern for 401 handling | Informational only. The JSDoc already documents the defense-in-depth purpose. |
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 |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-frontend |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-consistency |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 6 | 0 | 0 | 0 | 1 | 0 | 5 |
Note: Frontend and Consistency reviewers flagged overlapping issues (isAuthenticated vs user) which were merged. Consistency's "incomplete application" finding was discarded as out of scope per PRD-6033.
| useEffect(() => { | ||
| if (!isLoading && !user) { | ||
| router.replace(buildLoginUrlWithCurrentPath()); | ||
| } | ||
| }, [isLoading, user, router]); |
There was a problem hiding this comment.
💭 Consider: Align redirect condition with return value
Issue: The redirect condition checks !user but the hook returns isAuthenticated (which is !!session). These could theoretically diverge if session exists but session.user is null.
Why: While unlikely in practice (Better Auth's contract should guarantee both), using consistent conditions makes the code clearer and avoids potential edge cases.
Fix: Either use !isAuthenticated for the redirect check, or document why !user is preferred:
| useEffect(() => { | |
| if (!isLoading && !user) { | |
| router.replace(buildLoginUrlWithCurrentPath()); | |
| } | |
| }, [isLoading, user, router]); | |
| useEffect(() => { | |
| if (!isLoading && !isAuthenticated) { | |
| router.replace(buildLoginUrlWithCurrentPath()); | |
| } | |
| }, [isLoading, isAuthenticated, router]); |
Refs:
Summary
useRequireAuth()hook that monitors the client-side session and redirects to/loginif the session is absent or expires/[tenantId]that previously had no auth protection: Work Apps (overview, Slack, GitHub, GitHub installation detail), Stats (overview, AI calls, tool calls), and SettingsWhy Both Layers?
useRequireAuth(this PR)/login/loginChanges
New file:
agents-manage-ui/src/hooks/use-require-auth.tsuseAuthSession()hook to monitor session statebuildLoginUrlWithCurrentPath()to preserve return URLrouter.replace()when session is absent after loading completesModified pages (added
useRequireAuth()call):[tenantId]/work-apps/page.tsx[tenantId]/work-apps/slack/page.tsx[tenantId]/work-apps/github/page.tsx[tenantId]/work-apps/github/[installationId]/page.tsx[tenantId]/stats/page.tsx[tenantId]/stats/ai-calls/page.tsx[tenantId]/stats/tool-calls/page.tsx[tenantId]/settings/page.tsxTest 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/loginbetter-auth.session_tokencookie manually → should redirect to/loginResolves PRD-6033
Made with Cursor