[Gap-Audit] ProtectedRoute auth-race: latent stale closure on user inside admin/layout.tsx async

[TortoiseWolfe]

Summary

src/app/admin/layout.tsx:20-38 runs an async admin-check inside useEffect. It captures user via closure, then awaits an async service call, then calls router.push('/') based on the result. This is the same shape that was reverted three times before in ProtectedRoute (6b4c13a, 2c97e67, 259b38d).

The current implementation includes a cancelled flag that prevents the post-unmount redirect — that's a partial mitigation. The latent risk is that user may change mid-async (token refresh, sign-out, sign-in-as-different-user) and the closure still holds the stale reference, sending the wrong user to / or skipping the redirect entirely.

Code under review

useEffect(() => {
  if (authLoading) return;
  if (!user) return; // ProtectedRoute handles unauthenticated redirect
  let cancelled = false;
  (async () => {
    const service = new AdminAuthService(supabase);
    const admin = await service.checkIsAdmin(user.id);  // ← stale-closure risk
    if (cancelled) return;
    setIsAdmin(admin);
    if (!admin && !wasAdmin.current) router.push('/');  // ← acts on stale user
  })();
  return () => { cancelled = true; };
}, [user, authLoading, router]);

Repro paths (manual; need automated regression cover)

1. Mount admin page as admin → token refresh fires → user ref is replaced → in-flight checkIsAdmin resolves against old user.id → setIsAdmin writes correct value (the value that goes with the OLD user). On unrelated re-renders this can also stash a wrong wasAdmin.current.
2. Sign-out during the async call: cancelled prevents the redirect, but the "if (!user) return" guard ran before cancelled could fire, and setIsAdmin may still race the unmount.

Plan

1. Add a regression test first. Mount <AdminLayout> with a controllable AuthContext, assert correct redirect under: (a) sign-out mid-async, (b) user-id change mid-async, (c) authLoading flipping back to true mid-async. Vitest + React Testing Library. Lives at tests/integration/auth/admin-layout.test.tsx.
2. Pin the user reference for the duration of the effect: capture const targetUserId = user.id before the async, then assert at the resolution point that userRef.current === targetUserId before calling setIsAdmin or router.push. The userRef is a useRef synced via a separate effect.
3. Centralize the pattern. This is the same shape as ProtectedRoute. Both should call into a shared useAuthGate({onAdmin, onNotAdmin}) hook so the next regression has one place to land. Live in src/hooks/useAuthGate.ts.
4. Sweep the codebase for other useEffect blocks that await on a captured user and then router.push — likely candidates: any other layout.tsx files under src/app/, plus MessagingProvider, OnTheAccount page.

Acceptance

• Regression test from step 1 passes
• grep -rn 'router\.push.*user\.' src/app/ returns zero direct matches in async closures (use useAuthGate instead)
• No new revert of this shape for 30 days

Reference

• STATUS.md "Stability hotspots" table
• Prior reverts: 6b4c13a, 2c97e67, 259b38d
• Related: [Gap-Audit] 042 Payment RLS Policies: un-skip 25 E2E test stubs and verify policies #44 (closed) — verifying RLS surfaced this still hadn't been hardened on the admin path

[TortoiseWolfe]

Tracked in docs/STABILITY-TRACKING.md — see Family A (for #51, #52) or Family B (for #53) for ordering and rationale. Branch chore/post-044-tracking, commit 1c47c84.

[TortoiseWolfe]

Scope correction

Reading the code carefully, the issue body's claim that this is a 'latent stale-closure on user' overstated the problem. The combination of wasAdmin.current ref + cancelled cleanup flag + [user, authLoading, router] dep array already handles the three repro paths I listed. The actual gap was that none of those defenses had test coverage — the same gap that let three prior auth-race regressions ship in ProtectedRoute (6b4c13a, 2c97e67, 259b38d).

PR #56 lands a regression test that pins each defense as a contract:

• Loading spinner before authLoading resolves
• Loading spinner while admin check is in flight
• Children render on admin=true
• Redirect on admin=false first mount
• wasAdmin.current debounce: no redirect on transient flip
• Dep-array integrity: user.id change re-runs checkIsAdmin
• cancelled flag: no setState after unmount mid-flight
• Renders nothing (not the nav) when never-admin
• No admin check while authLoading is true
• No admin check when user is null

10/10 pass against current code. If a future refactor strips any of these defenses, the test now catches it before merge.

Out of scope (deliberately, with a contract that lets us revisit)

The original plan had four steps. Step 1 (regression test) ships in PR #56. Steps 2–4 (pin user.id via a parallel userRef, hoist into a shared useAuthGate hook, sweep the codebase) are removed from this issue's scope:

• Step 2 (userRef pin): the failure mode it defends against is already caught by the cancelled flag. YAGNI unless a regression case empirically fails.
• Step 3 (useAuthGate hook): real DRY opportunity (ProtectedRoute and AdminGate have parallel wasX.current patterns) but deserves its own focused refactor PR, not 'while we're here'.
• Step 4 (codebase sweep): should run on a clean baseline — i.e., after PR test(admin): extract AdminGate, pin safety properties via regression test #56 lands.

Closing this issue when PR #56 merges. If steps 3 or 4 turn out to be worth doing later, file them as separate, more narrowly-scoped issues with concrete justification at that time.

[TortoiseWolfe]

Closed by PR #56 — AdminGate extracted, 10 regression cases pin the safety properties. Steps 2-4 of the original plan removed from scope; refile if a regression case ever empirically demands them.