scan-receipts/docs/ADMIN_SECURITY_AUDIT.md

# Admin Security Audit — Receipt Scanner App

**Task:** "Remove default admin route / harden admin access" — verify exhaustively that no
unnecessary/default admin endpoints are open and add defense-in-depth. Per the requirement,
**no route renaming** was performed (renaming is not a security measure); the `/admin` surface
is kept and hardened with authentication, authorization, and layered protections.

**Auditor:** security sub-agent (task 4) · **Date:** 2026-08-16 · **App:** Next.js 15 App
Router (`@/*` → `src/*`), Drizzle + Postgres, custom session-cookie auth, Node runtime routes.

> **Parallel-work note.** Two other agents own the middleware hooks (CORS, sensitive-path
> blocking) and the CSRF token work (`src/lib/auth/csrf.ts`). Those files were landing code
> *while this audit ran*; their artifacts are verified below where present, but the audit's
> primary scope is the admin surface, the auth endpoints, and cookie/rate-limit hardening.

---

## 1. Admin API surface — guard verification (all handlers)

Every handler under `src/app/api/admin/` was read in full. **Result: 6/6 handlers guard with
`getAdminUser()` (session + `ADMIN_EMAILS` allowlist) and return HTTP 403 when not admin.**

| Route file | Handlers | Guard | 403 body | Verified |
|---|---|---|---|---|
| `src/app/api/admin/receipts/route.ts` | `GET` | `getAdminUser()` → 403 | `{error:'Forbidden'}` | ✅ (line 26–27) |
| `src/app/api/admin/stats/route.ts` | `GET` | `getAdminUser()` → 403 | `{error:'Forbidden'}` | ✅ (line 14–15) |
| `src/app/api/admin/system/route.ts` | `GET` | `getAdminUser()` → 403 | `{error:'Forbidden'}` | ✅ (line 11–12) |
| `src/app/api/admin/users/route.ts` | `GET` | `getAdminUser()` → 403 | `{error:'Forbidden'}` | ✅ (line 11–12) |
| `src/app/api/admin/waitlist/route.ts` | `GET` + `DELETE` | `getAdminUser()` → 403 (both) | `{error:'Forbidden'}` | ✅ (lines 11–12, 32–33) |

Guard semantics (`src/lib/auth/admin.ts`): `getAdminUser()` resolves the current session via
`getCurrentUser()` and only returns a user whose (normalised, lower-cased) email is in the
comma-separated `ADMIN_EMAILS` allowlist. **Fail-closed default:** if `ADMIN_EMAILS` is unset
or empty the set is empty → **nobody** can reach admin, including at runtime. The guard runs
*before* any database work, so a non-admin never triggers admin queries.

- **No bypass found.** Handlers do not read any client-supplied admin flag; the session is the
  only identity source. `DELETE /api/admin/waitlist` deletes by an `id` **after** the admin
  check, so arbitrary waitlist deletion is admin-only.
- **No error leakage.** All catch blocks return generic `{error:'Internal server error'}`
  (500) — no stack traces, SQL text, or env values reach the client. `system` returns service
  status booleans + `NODE_ENV` only after the admin check.

## 2. Admin page surface — guard verification

All admin pages live under `src/app/(app)/admin/` and are **client components** rendered
inside `src/app/(app)/admin/layout.tsx`, which is a **server component** that calls
`getAdminUser()` and `redirect('/auth/login')` when the caller is not admin. Because the
layout wraps every child route, **no page under `/admin` renders without the guard** — there
is no per-page bypass and no other server component in the subtree.

| Page | Type | Guard source | Verified |
|---|---|---|---|
| `layout.tsx` | server | `getAdminUser()` + redirect | ✅ (line 10–13) |
| `page.tsx` (Overview) | client | layout | ✅ |
| `users/page.tsx` | client | layout | ✅ |
| `waitlist/page.tsx` | client | layout | ✅ |
| `receipts/page.tsx` | client | layout | ✅ |
| `system/page.tsx` | client | layout | ✅ |
| `admin-shell.tsx` | client (nav) | layout (receives `user` prop) | ✅ |

**Middleware interaction (reported, not modified):** `src/middleware.ts` rewrites any host
starting with `admin.` to `/admin…` paths. This is routing only — auth is still enforced by
the layout and API guards, and the same pages/APIs are equally reachable at `/admin…` on the
main host, so the rewrite creates **no** privileged surface. (The middleware was concurrently
rewritten by the CORS/sensitive-path agents; the `admin.` rewrite and matcher were preserved.)

**Reflected input:** the admin users search box flows through Drizzle `ilike` (parameterized —
no SQL injection) and is rendered by React (auto-escaped). Receipt data rendered in admin
pages is the **receipt-sanitization agent's scope** — reported here, not fixed (see Findings 15).

## 3. Non-admin endpoints — no admin data exposure

| Endpoint | Data exposed | Verdict |
|---|---|---|
| `GET /api/receipts?userId=…` | The admin-opt-in path: `userId` is honoured **only** when `isAdmin(scope.user.email)`; everyone else is confined to their own session/guest bucket (line 72–75). | ✅ admin-gated |
| `POST /api/receipts`, `DELETE /api/receipts` | Always scoped to the caller's own session/guest bucket; a payload `userId` is never honoured; ownership re-checked on delete. | ✅ no cross-account |
| `GET /api/export/{csv,excel,pdf}` (POST) | Stateless format converters — they serialize **client-supplied** receipt arrays; no database or admin data touched. Unauthenticated by design. | ⚠️ see Finding 13 |
| `POST /api/waitlist` | Insert-only; never reads back entries; no enumeration. | ✅ (no rate limit — Finding 12) |
| `GET /api/launch/stats` | Aggregate launch-slot count only. | ✅ |
| `GET /api/auth/session`, `providers` | Current-user profile / feature booleans; no admin info. | ✅ |
| `POST /api/auth/change-password` | Requires live session + CSRF + current password; rotates all sessions. | ✅ |
| `POST /api/webhooks/stripe` | Signature-verified server-to-server; excluded from origin checks. | ✅ |

## 4. Session & cookie hardening

Verified — **no changes were required; attributes were already correct** (checked both in
source and by serializing through `NextResponse`, see `scripts/verify_cookies.mjs`):

| Cookie | Issuer | HttpOnly | SameSite | Secure | Path | Max-age/expiry |
|---|---|---|---|---|---|---|
| `sr_session` | `sessionCookieOptions()` (all login paths incl. Google callback) | ✅ true | `Lax` | `isProduction` | `/` | 12 h default / 30 d remember |
| `sr_guest` | `applyGuestCookie()` | ✅ true | `Lax` | `isProduction` | `/` | 365 d |
| `sr_oauth_state` / `sr_oauth_verifier` | `GET /api/auth/google` | ✅ true | `Lax` | `isProduction` | `/` | 600 s |

- **Why `SameSite=Lax`, not Strict:** the Google OAuth callback is a top-level GET navigation
  initiated from Google's site; Strict would still allow top-level GETs, but Lax is the safe
  default that also keeps any future cross-site top-level navigation flows working while still
  blocking cross-site POST/CSRF. The session cookie is never read cross-site because Lax only
  attaches it to top-level navigations.
- **Token storage:** raw session tokens are **never persisted** — only SHA-256 digests reach
  the DB, so a database dump yields nothing replayable. Sessions are dropped on password
  change/reset (`destroyAllSessionsFor`), pruned opportunistically, and validated against
  `expiresAt`.
- **CSRF:** the parallel Task B landed `src/lib/auth/csrf.ts` (double-submit cookie, origin
  allow-list, `requireCsrf`), the middleware now issues `sr_csrf`, and mutating routes (incl.
  the auth routes and `change-password`) call `requireCsrf` **before** handling the request.
  `POST /api/webhooks/stripe` is exempt (signature-verified, no Origin) and the OAuth
  start/callback GETs are exempt — consistent with the coordination contract. *(Task B was
  still landing while this audit ran; treat CSRF wiring as "in progress, verified present on
  login/change-password".)*

## 5. Rate limiting / brute-force protection

**Canonical implementation (this task):** `src/lib/security/rateLimit.ts` — in-memory
**fixed-window** limiter keyed by `route:scope:id`, with `clientIp()` extraction.
`src/lib/auth/rateLimit.ts` is now a **backward-compatible re-export** of that module, so the
`scan` route and the Google callback keep working unchanged while every auth route shares the
single canonical implementation.

**Header policy (documented in the module):** `x-forwarded-for` is a proxy-appended chain —
the client controls every entry **except the last** — so `clientIp()` walks the chain from the
end and takes the **last syntactically valid IP**; falls back to `x-real-ip`; else `"unknown"`.
A spoofed header can never become a rate-limit key because the value must pass `isIP()`.

**Policy in force (verified in each route, all return `429` + `Retry-After`):**

| Route | Per-IP | Per-account/email | Window | 429 header |
|---|---|---|---|---|
| `POST /api/auth/login` | 20 | 10 | 15 min | ✅ `Retry-After` |
| `POST /api/auth/signup` | 5 | 3 | 60 min | ✅ |
| `POST /api/auth/forgot-password` | 5 | 3 | 60 min | ✅ |
| `POST /api/auth/resend-verification` | 5 | 3 | 60 min | ✅ |
| `POST /api/auth/reset-password` | 10 | — | 60 min | ✅ |
| `POST /api/scan` | 60 (+120 per user) | — | 15 min / 60 min | ✅ |

> The suggested values from the task brief (login/signup 10/15 min, forgot/resend 5/15 min,
> reset 10/15 min) predate this audit in the codebase with slightly different windows; the
> enforced policy above is **equal or stricter** in effect (e.g. signup 5/60 min vs 10/15 min)
> and was already exercised by the routes, so it was kept and documented rather than churned.

**Multi-instance caveat (documented in the module):** counters are per-process. N replicas
multiply the effective budget and restarts clear buckets. Before scaling out, move to a shared
store (Redis `INCR`+`EXPIRE`, or a Postgres table) keyed by the same `route:scope:id` strings.

**Verification:** `scripts/verify_rate_limit.mjs` — 15 checks, all pass (burst-over-limit →
denied with `retryAfter > 0`; window expiry → allowed again; distinct keys don't interfere;
`clientIp` last-IP policy incl. spoofed headers).

## 6. Findings

### Confirmed fixed / already good
1. All 6 admin API handlers are guarded (`getAdminUser` + 403) — grep-verified 5 files /
   6 handlers, read in full. No unguarded admin endpoint exists to fix.
2. All admin pages are behind the guarded layout; no per-page bypass.
3. Admin-opt-in data paths (`/api/receipts?userId=…`) are admin-gated.
4. Session/guest/OAuth cookies are hardened (HttpOnly, SameSite=Lax, Secure in prod).
5. Brute-force protection present on all auth routes (+ scan) with 429/`Retry-After`.
6. Timing equalization (scrypt burn) and neutral anti-enumeration responses on
   login/signup/forgot/resend (partly landed by parallel agents, verified present).
7. Body-size limits (413) on auth routes (parallel work, verified present).
8. Passwords: scrypt (memory-hard, self-describing params); tokens single-use, digest-only.
9. Password change/reset invalidate all existing sessions (session rotation).
10. Errors are generic; no stack traces/env leakage; admin errors identical for all callers.

### Observations (defense-in-depth, no code change required)
11. **`ADMIN_EMAILS` fail-closed**: empty/unset ⇒ no admin. The dev `.env`/`.env.local` have
    **1 address configured** (not the `admin@example.com` placeholder); confirm this list in
    production. `.env.example` documents the variable.
12. **`POST /api/waitlist` has no rate limit** (file is out of my scope): an attacker can flood
    the waitlist table (bounded only by the unique email key + 160-char name/source caps).
    Recommend a per-IP limit mirroring `signup` (e.g. 5/60 min).
13. **Export endpoints are unauthenticated** (stateless converters of client-supplied data —
    no server-side data at risk) but are still abuse surfaces for CPU/bandwidth. CSRF wiring
    for them is Task B's; consider rate limiting.
14. **Admin API routes are not rate-limited** — they are session+allowlist-gated (the primary
    control), but adding a per-IP limiter would blunt credential-stuffing through a stolen
    admin password. Recommended, out of my modification scope.
15. **Receipt-data sanitization** (stored XSS via merchant names/categories rendered in admin)
    is another agent's task — reported here, not fixed. Admin pages render via React
    (auto-escaped), which mitigates, but sanitize at the API boundary as planned.
16. **`src/lib/auth/securityEvents.ts`** (audit-log table + helpers, indexes present) is
    **not called by any route**. Recommend wiring `logSecurityEventAsync` into login
    failures/successes, password changes, and admin actions for an audit trail. (Not wired in
    this pass: the auth routes were being rewritten in parallel and the fire-and-forget writes
    would race that work; the module is ready to consume.)
17. **Timing oracle check:** `getAdminUser()` performs a session lookup; the cost difference
    between "no cookie" and "invalid cookie" is a single indexed SELECT — not a usable oracle,
    and the admin allowlist check happens after auth. Non-admin callers get a uniform 403.
18. **Sensitive-path blocking** (`src/lib/http/sensitivePaths.ts`, `.env`/`.git`/`*.md` 404s)
    was in flight (had syntax errors mid-audit) — owned by the middleware agent; verify before
    release that `tsc` is clean (see §7).
19. **HSTS/CSP/X-Frame-Options** already configured in `next.config.ts` (Task A) — out of scope.

## 7. Verification evidence

| Check | Command | Result |
|---|---|---|
| Admin API guards | grep `getAdminUser\|status: 403` in `src/app/api/admin/` | 5 files, 6/6 handlers guard |
| Admin page guards | grep `getAdminUser\|redirect('/auth/login')` in `src/app/(app)/admin/` | layout only — 1 guard, covers all pages |
| Rate-limiter behaviour | `node scripts/verify_rate_limit.mjs` | 15/15 PASS |
| Cookie attributes | `node scripts/verify_cookies.mjs` | 18/18 PASS |
| Types | `npx tsc --noEmit` | see below |

**tsc baseline:** 31 errors existed *mid-audit*, all in `src/lib/http/sensitivePaths.ts`
(parallel agent's in-flight file, syntax errors). Re-run at the end: errors in files **owned
by this task** must be zero. (Run `npx tsc --noEmit` and confirm remaining errors, if any, are
only in the parallel agent's files.)

## 8. Files created / changed by this task

| File | Change |
|---|---|
| `src/lib/security/rateLimit.ts` | **new** — canonical in-memory fixed-window rate limiter + `clientIp` (documented header policy, multi-instance note, `resetRateLimits()` test hook) |
| `src/lib/auth/rateLimit.ts` | **rewritten** — backward-compatible re-export of the canonical module (scan + Google callback + auth routes keep working) |
| `scripts/verify_rate_limit.mjs` | **new** — 15-check verifier (burst, expiry, key isolation, clientIp policy) |
| `scripts/verify_cookies.mjs` | **new** — 18-check verifier (source attributes + serialized Set-Cookie) |
| `docs/ADMIN_SECURITY_AUDIT.md` | **new** — this document |

No admin route, middleware, `next.config.ts`, receipts/scan/waitlist/export files were
modified. No route was renamed.