Stale `is_admin` flag grants persistent admin access after demotion

[dale053]

Description

A demoted admin retains full access to every /admin page and /api/admin endpoint for the entire lifetime of their existing session — up to the session's maxAge — after their is_admin flag is cleared in the database.

The root cause is an ordering bug introduced by the partial fix in PR #117 (issue #116). PR #117 added a live DB re-check for the status field to prevent banned users from using a stale cookie. It did not add the equivalent re-check for is_admin. The admin-route guard at middleware.ts:36 reads session.is_admin directly from the signed JWT cookie, which is frozen at login time and never refreshed. The DB lookup (getUserById) happens at line 46 — after the admin guard has already returned — so the live is_admin value from the database is never consulted when protecting admin routes.

This is a distinct vulnerability from #116 (which concerns the status / ban flow), but it shares the same architectural root: privileged fields baked into the session cookie are never re-validated against the database.

Steps to Reproduce

1. Log in as an admin. Session cookie is issued with is_admin: true in the JWT payload (src/lib/session-token.ts → setSessionCookieFor).
2. A second admin visits Admin → Users and clicks Demote on the first admin's account.
3. POST /api/admin/users/:id/demote calls demoteUser() in src/lib/auth.ts:303, which sets is_admin = 0 in the database.
4. The demoted admin does not log out. Their browser still holds the original cookie.
5. The demoted admin navigates to /admin or calls any /api/admin/* endpoint.
6. middleware.ts:36 evaluates !session.is_admin → false (cookie still says is_admin: true) → admin guard passes.
7. The DB lookup at middleware.ts:46 reads the live user.status only — is_admin is never checked against the database.
8. The demoted admin has full admin access.

Expected Behavior

After demoteUser() sets is_admin = 0 in the database, the affected user should be denied access to all admin routes on their very next request, regardless of the value stored in their session cookie.

Actual Behavior

The demoted admin continues to pass the admin route guard at middleware.ts:36 because the guard reads session.is_admin (stale cookie value, always true) instead of the live database value. Access persists for the full session lifetime.

Affected Files

File	Location	Role in the bug
src/middleware.ts	lines 34–43	Admin route guard reads stale session.is_admin; DB re-check at line 46 is never reached for admin routes
src/middleware.ts	lines 45–47	getUserById result only used for status — is_admin is ignored
src/lib/session-token.ts	SessionPayload.is_admin	is_admin is encoded into the JWT at login and never rotated
src/lib/auth.ts	setSessionCookieFor	Cookie issued with is_admin: !!user.is_admin; no re-issue on demotion
src/app/api/admin/users/[id]/demote/route.ts	full file	Calls demoteUser() but does not invalidate or rotate the target user's session cookie

Root Cause Analysis

Current control flow in middleware.ts:

L22  session = verifySessionToken(cookie)   // JWT decoded; is_admin frozen at login
L36  if (isAdminRoute && !session.is_admin) // GUARD — stale cookie value, no DB hit
       return 403 / redirect
L46  user = getUserById(session.uid)         // DB re-check — happens AFTER guard return
L47  liveStatus = user?.status              // only status is used; is_admin discarded

The getUserById call that could provide a live is_admin value is sequenced after the guard that needs it. This ordering makes the DB value unreachable for admin authorization decisions.

Required Structural Changes

This is not a one-line patch. Fixing it correctly requires a structural reorder of middleware.ts and a decision about whether is_admin should remain in the session payload at all.

1. Move the DB lookup before the admin route guard

getUserById(session.uid) must be called before line 36 so the live row is available to both the admin guard and the status check.

// After session verification at L22:
const user = getUserById(session.uid);

// Unified authorization block using live DB values:
if (!user || user.status === 'rejected') {
  // clear cookie and redirect (same logic as current lines 49–62)
}

const isAdminRoute = pathname.startsWith('/admin') || pathname.startsWith('/api/admin');
if (isAdminRoute && !user.is_admin) {          // live DB value, not session cookie
  // 403 / redirect (same logic as current lines 37–43)
}

This collapses two separate DB-aware blocks into one, eliminates the ordering bug, and cuts the number of conditional branches in the hot path.

2. Handle the deleted-user case

If getUserById returns null (user row deleted after the session was issued), the current code falls through. With the reorder, a null result must be treated as unauthenticated and result in a session-cookie deletion and redirect to /sign-in.

3. Consider removing is_admin from SessionPayload

Since is_admin must always be re-read from the database to be trustworthy, storing it in the JWT provides no benefit and is the direct source of this class of vulnerability. Removing it from SessionPayload in src/lib/session-token.ts would:

• Make future stale-flag bugs impossible at the type level.
• Require updating any code that reads session.is_admin (currently only middleware.ts:36).
• Reduce the surface of the signed token.

If is_admin is retained in the token (e.g., for client-side UI decisions), the middleware must be documented to never trust the token value for server-side authorization.

4. Audit setSessionCookieFor call sites

demoteUser() does not call setSessionCookieFor to re-issue a corrected cookie after demotion. Even after the middleware fix, the stale value persists in the cookie until expiry. If the token is also read client-side for UI gating, the demoted admin may still see admin UI elements. Either re-issue the cookie in demoteUser(), or strip is_admin from the token entirely (see point 3).

Expected Behavior After Fix

• Immediately after demoteUser() executes, the target user's next request to any admin route returns 403 / redirects to /.
• No session invalidation is required (though it remains best practice).
• The fix must not introduce an additional DB query per request on non-admin routes; the single getUserById call should serve both the status check and the admin check.

Environment

• Server-side (Next.js middleware, Node.js runtime)
• Affects all authenticated sessions where the user's is_admin flag has changed since the session was issued

Additional Context

• PR Avoid apt npm dependency conflicts in setup_env.sh (Ubuntu 24.04) #117 fixed the equivalent staleness bug for the status field but explicitly did not address is_admin.
• Issue more columns for downstream visibility #116 documents the architectural pattern (frozen cookie fields) but scopes its fix to the ban flow.
• better-sqlite3 is synchronous; getUserById does not add async overhead to the middleware path.
• Session maxAge determines the maximum window of exposure. If sessions are long-lived (e.g., 30 days), a demoted admin retains access for up to 30 days.