forge/claude_artifacts/security-20260827-180635.md

# security — RBAC + token-management + user-management audit

**Date:** 2026-08-27 · From: [engineer-20260827-180000](engineer-20260827-180000.md), [principal-20260827-175448](principal-20260827-175448.md)
**Scope:** the new RBAC / API-token / user-management surface on the public domain. Read-only audit (no edits).
**Out of scope (per task):** DB-superuser-in-`.env` (tracked separately), analytics math.

## Method / evidence
- Read `server/auth.ts`, `server/tokens.ts`, `server/db.ts`, `index.ts`, client `App.tsx`/services.
- Full route inventory via grep (`app.get|post|put|delete|use`) — 15 API routes, all accounted for below.
- Grepped for the removed `ADMIN_KEY`/`x-admin-key` gate → **no matches** (fully removed). No `dangerouslySetInnerHTML` in `client/src`.
- Live DB (role `forge_app`, `31.133.115.50:5440/forge`): `current_user=forge_app`; `SELECT count(*) FROM app_users WHERE role='admin'` → **1**; the sole account `admin` is role `admin` (bootstrap migrated correctly). No destructive SQL run.

## Verified correct (no action)
- **Route gating matches the spec matrix.** viewer: `/api/tickets`,`/api/tickets/:number`,`/api/stats`,`/api/config` (`requireAuth`). pm: `/api/analytics/overview|sla`,`/api/insights` (`requireRole('pm')`). lead: `GET/POST /api/users`,`DELETE /api/users/:username`. admin: `GET/POST /api/tokens`,`POST /api/tokens/:id/revoke`. `/api/sync` token-only.
- **401 vs 403 correct** (`auth.ts:46-61`): unauth→401, under-privileged→403.
- **Role cannot be spoofed** — read from the server-side PG session store (`req.session.user`, `auth.ts:14`), never from client input.
- **Admin-creation guard present** (`index.ts:249`): non-admin creating `role==='admin'` → 403.
- **`isRole` validation** (`db.ts:191`): arbitrary/invalid role string → 400 `invalid_role`.
- **Self-delete guard present** (`index.ts:262`).
- **Tokens:** admin-only; raw `fg_…` returned once (`tokens.ts:81-89`), only `sha256(secret)` persisted; `listTokens` selects id/token_id/label/dates/revoked — **never `token_hash`** (`tokens.ts:97-106`); revoke honored (`resolveToken` filters `revoked=false AND not expired`, `tokens.ts:62`); constant-time `timingSafeEqual` compare.
- **Cookie hygiene:** httpOnly, sameSite=lax, secure-in-prod, `SESSION_SECRET` required in prod (`process.exit(1)`), login rate-limited (20/15min), bcrypt dummy-hash compare for unknown users (timing-uniform), bcrypt cost 12.
- **No secret leak:** `/api/users`→id/username/role/dates only (no `password_hash`); `/api/me`→`{username,role}` only.
- **Client gating is defense-in-depth only** (`App.tsx` `Navigate` redirects); the server is the true gate.

---

## Ranked findings

### HIGH-1 — A `lead` can delete `admin` accounts (no target-rank check)
**Where:** `index.ts:260` (`DELETE /api/users/:username` gated only at `requireRole('lead')`) + `server/db.ts:227` (`deleteUser` = unconditional `DELETE FROM app_users WHERE username=$1`).
**Exploit:** The create path forbids a non-admin from making an admin (`index.ts:249`), but the delete path has **no symmetric rank check** — a `lead` (or a compromised lead session) can delete any user, admins included. A lead can therefore remove every admin even though it can never create one.
**Impact:** Privilege-boundary violation. Combined with "only an admin creates an admin," a lead that deletes all admins permanently strips the org of token-management / admin-create ability until a server reboot re-seeds `AUTH_USER`.
**Fix:** Enforce actor-outranks-target in the delete path — refuse deleting a user whose role rank ≥ the actor's (a lead may not delete admins/leads), or raise `DELETE /api/users/:username` to admin-only. Hand to **engineer** (route + `deleteUser`).

### HIGH-2 — Last-admin lockout (no guard on deleting/removing the final admin)
**Where:** `server/db.ts:227` `deleteUser`; no count check anywhere. There is **no demote endpoint** (confirmed — only `seedUser` ever writes `role`, `db.ts:179`), so deletion is the only removal path.
**Exploit:** Live state is a **single** admin (`admin`). Deleting it (reachable today by any lead per HIGH-1, or by a second admin) leaves zero admins. `/api/tokens*` then 403s for everyone; no one can create a new admin.
**Impact:** Availability/integrity loss of the admin tier. Recovery requires shell/redeploy access — `seedUser` re-inserts `AUTH_USER` as admin only when its row is absent (`db.ts:176-185`), and only that one account.
**Fix:** Guard against removing the last remaining admin (`SELECT count(*) … role='admin'` before delete; refuse if target is the only admin). Same for any future demote. Hand to **engineer/dba**.

### HIGH-3 — Deleting a user does not invalidate their active session
**Where:** `auth.ts:46-61` (`requireAuth`/`requireRole` read identity+role from `req.session.user` set once at login, `index.ts:47`); session `maxAge` 7 days (`auth.ts:40`); session rows persist in `user_sessions`. `deleteUser` (`db.ts:227`) removes only the `app_users` row.
**Exploit:** An off-boarded/removed user keeps their cookie working at their cached role for up to 7 days. A deleted **lead** still passes `requireRole('lead')` and can keep deleting users (feeding HIGH-1) until session expiry. Role is never re-validated against the DB per request.
**Impact:** On a public domain, account removal is not effective access revocation; no mechanism to force-logout a user.
**Fix:** On delete (and any future role change), purge that user's sessions — e.g. `DELETE FROM user_sessions WHERE (sess->'user'->>'username') = $1` — or re-check user existence/role from `app_users` per request (short-cached). Hand to **engineer/dba**.

### MEDIUM-1 — No session-fixation protection on login
**Where:** `index.ts:43-49` assigns `req.session.user = ok` on the pre-existing session id; no `req.session.regenerate()` at the privilege transition.
**Exploit:** Classic session fixation — an attacker who can plant a session cookie (e.g. via a sibling/adjacent context) rides the same id after the victim authenticates and it becomes privileged. Mitigated but not eliminated by httpOnly + sameSite=lax.
**Fix:** Regenerate the session id on successful login before writing `user`. Hand to **engineer**.

### MEDIUM-2 — Weak password policy on a public login
**Where:** `index.ts:246` — minimum 6 chars, no complexity/denylist. Throttle is per-IP only (`loginLimiter` 20/15min, `index.ts:41`).
**Exploit:** Created accounts can carry trivially guessable passwords; per-IP throttling doesn't stop distributed/slow guessing against a public domain.
**Fix:** Raise minimum (~12 chars) and/or add a zxcvbn/denylist check; consider per-account lockout in addition to per-IP. Hand to **engineer**.

### LOW-1 — No HTTP security headers (helmet absent)
**Where:** app has no `helmet`/CSP (grep empty); serves SPA + API on a public domain.
**Impact:** No `X-Frame-Options`/`frame-ancestors` (clickjacking of the authenticated UI), no HSTS, no `X-Content-Type-Options`. Adjacent to the auth perimeter, not RBAC-specific.
**Fix:** Add `helmet` with a same-origin CSP + HSTS-in-prod. Hand to **engineer/devops**.

### LOW-2 — Silent no-op on delete/revoke of a non-existent id
**Where:** `db.ts:227` `deleteUser`, `tokens.ts:108` `revokeToken` — no affected-row check. `DELETE /api/users/:missing` and revoke of an unknown token id both return `{success:true}`.
**Impact:** Cosmetic/idempotency only (no enumeration leak — always success). Consider returning 404 when 0 rows affected so the admin UI reflects reality. Hand to **engineer**.

### LOW-3b (informational) — CSRF on state-changing POSTs rests on `sameSite=lax`
State-changing routes (`/login`, `/api/users`, `/api/tokens*`, `DELETE /api/users/:username`) carry no CSRF token; cross-site protection is provided by the `sameSite=lax` cookie (`auth.ts:38`). Defensible today. If any of these routes ever move to `sameSite=none` (e.g. cross-origin embedding), add an explicit CSRF token. No fix required now.

### LOW-3 (informational) — Leads self-propagate the user-management tier
**Where:** `index.ts:248-249` blocks only `role==='admin'` for non-admins; a lead may create unlimited additional `lead`s. This is per the capability spec ("leadership tops out at lead"), so it is by design — but note it compounds HIGH-1/HIGH-3: a lead can mint a co-lead, and either can delete the sole admin. No fix required; revisit if leads should not be able to grant lead.

## Next
engineer/dba — fix HIGH-1 (target-rank check on delete), HIGH-2 (last-admin guard), HIGH-3 (session purge on delete). engineer — MEDIUM-1 (session regenerate), MEDIUM-2 (password policy), LOW-1/2. Then re-audit the delete path.