# Audit & Architecture Plan — hackerspace-management (2026-08)

Branch: `audit/cleanup-architecture`. Method: 14-agent adversarial audit (each security/correctness
finding independently verified against RLS policies, code authz gates, and DB constraints before it
counts). **All 16 risk findings survived verification; 0 were refuted** — the finders were disciplined,
not noise. Every finding below has file:line evidence and a concrete failure scenario.

## Baseline health (verified today, main @ 9ac0c24)

| Check | State |
|---|---|
| `tsc --noEmit` | **586 errors**, all hidden by `next.config.mjs typescript.ignoreBuildErrors:true` |
| `eslint .` | **fails** — no flat config present (ESLint 9 needs `eslint.config.js`) |
| `vitest run` (unit) | **639 tests / 32 files, all green** (~11s) |
| integration / e2e | present but self-skip without a DB / are largely vacuous (see Tests) |
| `build` | passes *because* type errors are ignored |
| Package manager | **pnpm** (not npm); there is **no `typecheck` script** |

**Headline:** this is a well-built codebase. The server-action pattern (`parseInput`(Zod) →
`requireMemberWithRole` → `.eq('space_id', self.space_id)` → `logActivity`) is uniform and correct, RLS
is enabled on all 50 tables, and the layering the brief asks for is ~70% already present. The real work
is (a) a handful of concrete bugs, (b) turning type-checking back on, (c) making tests actually test,
and (d) subdividing giant files along seams that already exist — **evolve, don't rewrite.**

---

## 1. Correctness & security findings (ranked, all verified)

### HIGH

**H1 — Invite `max_uses` is a check-then-increment TOCTOU** · `lib/auth-actions.ts:210`
Two users redeem a `max_uses=1` invite simultaneously: both read `uses_count=0`, both pass the JS cap
check, both insert a membership, and the non-atomic `uses_count = invite.uses_count + 1` write leaves
the counter at 1 instead of 2. Single-use invites become multi-use; counter corrupts.
*Fix:* one atomic guarded statement — `UPDATE space_invites SET uses_count = uses_count + 1 WHERE id = :id
AND (max_uses IS NULL OR uses_count < max_uses) RETURNING id`; only proceed if a row came back.

### MEDIUM

**M1 — Board can re-arm an existing admin-granting invite** · `lib/actions/invites.ts:89` (privilege escalation)
`updateInvite` runs the `canAssignInviteRole` guard **only when `role` is in the payload**. A board member
(who cannot *mint* an admin invite) can call `updateInvite(id, { is_enabled:true, max_uses:5 })` on an
exhausted/disabled admin invite — no `role` field — and re-arm it. RLS `invites_update` only checks
admin/board, so nothing stops it. Board thereby distributes admin access it can't create directly.
*Fix:* load the existing invite's role and require `canAssignInviteRole(member.role, existingRole)`
whenever any privilege-affecting field (`is_enabled→true`, `max_uses`, `expires_at`) changes.

**M2 — `appealIncident` role gate is broader than the RLS write; silent partial failure** · `lib/actions/incidents.ts:247`
Gated by `requireMember` (any member — the docstring intends reporters to appeal), but the incident
`UPDATE` is restricted to admin/board by `incidents_update` RLS. A reporter's appeal **inserts a draft
proposal, silently updates 0 incident rows, and returns success** — orphaned proposal, incident never
linked. *Fix:* do the incident update via admin client after re-authorizing caller as reporter-or-admin,
**or** add a reporter clause to `incidents_update` RLS; and assert rows-affected before creating the proposal.

**M3 — SSRF filter misses hex IPv4-mapped IPv6** · `lib/door-logic.ts:72`
`isBlockedDoorIp` blocks the dotted form `::ffff:169.254.169.254` but not the hex form `::ffff:a9fe:a9fe`
(= 169.254.169.254 IMDS) or `::ffff:7f00:1` (= 127.0.0.1). Defense-in-depth gap on the door egress /
metadata-endpoint boundary. Node resolvers currently emit the dotted form, so not exploitable *today*,
but it's a latent SSRF bypass. *Fix:* decode `::ffff:HHHH:HHHH` to its 32-bit IPv4 and run the same checks.

**M4 — `logCashPayment` stamps `last_paid_at` from a caller-supplied (backdatable) date** · `lib/actions/payments.ts:56`
**M5 — `linkPaymentToMember` overwrites `last_paid_at` with the linked payment's date** · `lib/actions/payments.ts:104`
Both unconditionally set `last_paid_at` and force `payment_status='current'` with no `GREATEST` guard.
Linking/logging a backdated (e.g. CSV-imported) payment **regresses** a member's dues state — a
currently-paid member can be pushed backward or a lapsed member wrongly shown current. `flexibleDateTime`
has no lower bound, so historical dates are real inputs. *Fix:* advance-only (`GREATEST(last_paid_at, txn)`),
derive `payment_status` from the resulting effective date.

**M6 — Null deref crashes the members list on search** · `app/(app)/members/members-client.tsx:110`
`m.display_name.toLowerCase()` / `m.email.toLowerCase()` — both columns are nullable (schema + types;
the `WHERE email IS NOT NULL` partial index proves null-email rows are by design, e.g. imported members).
Typing any character into the search box throws `TypeError` and blanks the whole members page.
*Fix:* optional-chain like the adjacent `handle` already does.

### LOW (confirmed)

- **L1** `reserveEquipment` doesn't verify the manager-supplied `memberId` is in the caller's space (its
  sibling `signUpForClass` does) — mis-attributed reservation **within** the space; no cross-tenant leak. `equipment.ts:249`
- **L2** Unverified members (pending approval in `require_approval` spaces) can create/open proposals and
  cast counted votes before an admin vets them. Arguable-by-design; RLS also permits it. `proposals.ts:150`
- **L3** `markOnboardingStepDone` read-modify-writes the `onboarding_progress` JSON in JS → concurrent
  step completions can be lost (last-write-wins). `onboarding.ts:142`
- **L4** Email dates render in server-local TZ (no `timeZone` option) → off-by-one-day in renewal/booking
  emails depending on the droplet's `TZ`. `notifications-logic.ts:114`
- **L5** `parseHeatSyncLog`: (a) a stale `H:M:E` time-of-day bleeds into later events (fabricated audit
  timestamp); (b) one high-half token can be reused for two adjacent grants (wrong card numbers). `door-log-logic.ts:78,91`
- **L6** Null deref in the payment link-picker initials (`m.display_name.split(' ')`). `payments-client.tsx:377`
- **L7** Unparseable CSV `transaction_date` throws mid-import → nothing imports, no toast, button stuck
  on "Importing…" forever (no `try/finally`). `import-client.tsx:215`

### Public / webhook / cron surface — CLEAN

Stripe (per-space signing secret, replay-protected via unique insert), door inbound (per-connection vault
bearer + `timingSafeEqual` behind a length pre-check, uniform 401/404), both crons (`CRON_SECRET` via
`timingSafeEqual`, fail-closed 503), public forms (`getPublicForm` excludes `members` visibility, writes
scoped to the looked-up form's space), `/track` (192-bit reporter token, redacted projection). Only nits:
two **auth-gated** DoS-hardening gaps on door inbound (trusts `Content-Length`; DB lookup before rate-limit).

### RLS cross-check — CLEAN

All 50 tables have RLS. A programmatic check of all 147 anon-client writes against 287 policies found
**zero** writes to a table lacking that operation's policy. The two policy-less tables
(`notification_preferences`, `stripe_webhook_events`) are admin-only by design and correctly scoped. The
sole `WITH CHECK(true)` is the intentional space-creation onboarding path.

---

## 2. Type safety — the linchpin (path to remove `ignoreBuildErrors`)

**Root cause of ~450 of the 586 errors:** version drift. `package.json` pins `supabase-js ^2.49.4` but the
install resolved to **2.99.0**, whose `SupabaseClient` grew from 3 to 5 generics. `@supabase/ssr@0.6.1`'s
`createServerClient` still passes the old 3-generic shape, feeding the *Schema object* into the *SchemaName
string* slot. The constraint violation collapses the client's `Schema` to **`never`**, so every
`.from().select()` row is `never` (→ 396 × TS2339 "property does not exist on never"), every insert value
is `never`, every rpc arg is `undefined`. This is exactly why `lib/actions/*` (admin client, supabase-js
direct) mostly type-check while every `app/**/page.tsx` (ssr client) is a wall of `never`.

**Proven interim fix (verified compiling):** annotate the two factory returns as `SupabaseClient<Database>`
in `lib/supabase/server.ts` + `lib/supabase/proxy.ts` — re-asserting `Database` lets the 5-generic client
recompute `Schema` and restores full row typing, clearing ~450 errors. Proper fix: align versions
(`@supabase/ssr ≥0.7` for supabase-js 2.9x, or pin supabase-js to the 2.5x line) so the lockfile stops drifting.

**Second cause:** `types/database.ts` is partially hand-maintained and **out of sync** with the 54 SQL
migrations — missing RPCs (`class_signup_tx`, `class_cancel_tx` are *called* but absent), missing the
`__InternalSupabase` marker. *Fix:* regenerate wholesale via `supabase gen types typescript`, then delete
the `as string` casts it forced throughout `lib/actions/`.

**Ordered path to drop `ignoreBuildErrors`:** (1) align supabase versions / annotate the two factories →
clears ~450; (2) regenerate `types/database.ts` → clears the RPC/enum union noise; (3) fix ~30 enum-widening
`setState` casts + the 2 null derefs (M6, L6); (4) fix test-file types + `vitest.setup.ts`
`beforeAll/afterAll` imports; (5) remove the flag and add `tsc --noEmit` to CI.

> Note on the brief's "split `types/database.ts` (1958 lines)": **don't physically split it.** It's a
> generated build artifact — the next `supabase gen types` overwrites it wholesale, reverting any hand-split.
> The generated/hand-written seam the brief wants **already runs between `types/database.ts` and `lib/types.ts`.**
> The right move is to keep `database.ts` pristine (add a "GENERATED — do not edit" banner) and split the
> **hand-written** side (`lib/types.ts` → `types/domain/*`). Delivered in the refactor plan below.

---

## 3. Tests — green but under-testing the risk

- **CRITICAL** No test exercises admin-client actions for cross-tenant (IDOR) scoping — the #1 stated risk
  is untested. Nothing would catch a dropped `.eq('space_id', …)`.
- **HIGH** CI runs **no tests at all**; the integration suite self-skips without a DB, so "green" is empty.
- **HIGH** The authz primitives (`requireMember`/`requireMemberWithRole`/`getAuthMember`) have zero tests.
- **HIGH** `__tests__/actions.test.ts` asserts on **string literals that check themselves** (e.g.
  `` const query = `.eq('space_id', …)`; expect(query).toContain('space_id') ``) — never imports an action.
  The describe blocks ("Row Level Security", "Role-Based Access") give false confidence.
- **MEDIUM** e2e specs are unauthenticated/vacuous; `components.test.tsx` tests stub components defined
  inside the test file; concurrency/money only covered at the DB layer, not the action layer.
- `vitest.setup.ts` calls `beforeAll/afterAll` without importing them (works only via `globals:true`, untyped).

*Plan:* add a real action-layer IDOR test (seed spaces A/B, call each id-taking action as an A-admin with a
B id, assert rejection + B untouched); unit-test the authz primitives; replace the tautological
`actions.test.ts` with tests that drive real actions through a mock client and inspect the `.eq` calls;
make CI fail on 0 executed integration tests.

---

## 4. Dead code & duplication

- `lib/security.ts`: 8 of 12 exports unused in production (7 test-only, 1 fully dead);
  `hasSqlInjectionPatterns` is security theater over parameterized queries. Prune to what's used.
- `lib/validations.ts`: 6 exported schemas never referenced (`signInSchema`, `signUpSchema`,
  `createSpaceSchema`, `joinSpaceSchema`, `proposalStatuses`, `updateAreaLeadRoleSchema`) + 1 test-only.
- Orphaned files: `components/task-claim-button.tsx`, `components/theme-provider.tsx`; dead
  `roleDisplayName` in `lib/role-labels.ts`; duplicate `escapeHtml` (dead one in `security.ts`).
- No shared date/number formatter — **68 ad-hoc `toLocale*` call sites** with copy-pasted option objects
  across ~15 files. `classes.ts` repeats a notification fan-out 3×; `door.ts` rebuilds an adapter query 5×.

## 5. Maintainability offenders

- `ops-client.tsx` (956) — God component: 3 modals + 2 row renderers + 4 tab bodies + filtering, **and it
  writes secrets/area-leads directly from the browser** (bypassing the existing `lib/actions/secrets.ts` /
  `area-leads.ts` — skips Zod + activity log). `settings-client.tsx` / `members-client.tsx` (663 each) fuse
  4 tabs / table+toolbar+2 modals.
- **HIGH footgun:** `MemberFormFields` is declared **inside** `members-client.tsx`'s render → remounts and
  **loses input focus on every keystroke**. `members-client.tsx:197`
- `forms.ts submitForm` is a ~200-line monolith with 4 overlapping email-linking functions;
  `import-client.tsx` embeds a CSV parser + auto-mapper that belong in `lib/`.

---

## 6. Architecture & refactor plan (evolve, not rewrite)

The five layers already exist: **validation** (`lib/validations.ts`) · **guard/auth** (`lib/auth-helpers.ts`
+ `lib/*-guard.ts`) · **pure logic** (`lib/*-logic.ts`) · **server actions** (`lib/actions/*.ts`) · **data
access** (`lib/supabase/*`). Feature-folders already exist (`lib/door/`, `lib/stripe/`, `lib/secrets/`) and
UI decomposition already has an exemplar in `app/(app)/customize/` (thin orchestrator + `panels/` + shared
`types.ts`). The plan **promotes these existing patterns** — every giant file splits along seams that are
already marked by section comments or sub-components.

**Target conventions:** `types/database.ts` = only generated file (pristine) · `types/domain/*` =
hand-written domain types (from `lib/types.ts`) · `lib/validations/<feature>.ts` + `primitives.ts` +
`index.ts` barrel (keeps `@/lib/validations` working for all 44 importers) · `lib/<feature>/{logic,guard}.ts`
· `lib/actions/<feature>/*.ts` behind `lib/actions/index.ts` barrel · `app/(app)/<route>/{page,
<route>-client,panels/,components/,types}.tsx`. A new feature = **one file per layer, wired only through
barrels** (full add-a-feature guide goes in `ARCHITECTURE.md`).

**Reviewable slice sequence (each gated on `tsc` + `test` + `build`):**

0. **Enabler** — GENERATED banner on `types/database.ts`; document regen command. (no behavior change)
1. **`lib/validations.ts` → `lib/validations/<feature>.ts` + `primitives.ts` + barrel.** Proves the
   barrel-preserving technique on the most-imported module (44 importers, zero call-site churn).
2. **`lib/types.ts` → `types/domain/{index,governance,reexports}.ts`**; convert governance rows to
   `Tables<'proposals'>` aliases; leave `lib/types.ts` as a re-export shim (11 importers). *This is the
   brief's "split the types file," done correctly.*
3. **`lib/actions/classes.ts` → `classes/{definitions,sessions,signups,instructor}.ts` + `_guard`/`_forms`.**
4. **`lib/actions/door.ts` → `door/{connections,operate,self-entry}.ts` + `_guards`/`_audit`.**
5. **`lib/actions/forms.ts` → `forms/{manage,submit,member}.ts` + `_guard`.**
6. **`ops-client.tsx` → `panels/` + `components/`** *and* replace the direct browser writes with the
   existing server actions (behavior change — ships alone).
7. **`settings-client.tsx` → `panels/`.**
8. **`members-client.tsx` → `components/`** + fix the `MemberFormFields`-remount focus bug.
9. **Cleanup** — delete the `lib/types.ts` shim + dead validations/security exports once importers move.

---

## Proposed execution order (fixes first, then refactor)

- **Phase A — tooling & type safety** (no behavior change): ESLint flat config, `typecheck` script, align
  supabase versions / annotate the two clients, regenerate `types/database.ts`, work toward removing
  `ignoreBuildErrors`. Gate each on `tsc`/`test`/`build`.
- **Phase B — confirmed fixes** (call out the behavior-changing ones): H1, M1–M6, then the LOW batch, each
  with a regression test. This is where the IDOR test + authz-primitive tests land.
- **Phase C — refactor slices 0–9** above, one reviewable commit each.
- **Phase D — `ARCHITECTURE.md`** documenting the resulting structure + add-a-feature guide.

Nothing is committed yet beyond this document. **No pushes to `main` without your explicit approval.**

---

## Progress log

### Phase A — tooling & type safety ✅ (branch `audit/cleanup-architecture`)
- `tsc --noEmit`: **586 → 0**. `next.config` `ignoreBuildErrors` **removed** — builds/deploys now type-check.
- Root cause fixed by annotating the Supabase client factories (`server`/`client`/`proxy`) as
  `SupabaseClient<Database>`; `parseInput` rewritten to infer via `z.infer`; `flexibleDateTime` typed;
  tsconfig target → ES2020; missing RPCs added to the generated types. A 40-agent behavior-preserving
  sweep cleared the long tail (joins bridged with `as unknown as` + `// TODO(types)` to regenerate).
- **ESLint 9 flat config** added (`eslint.config.mjs`) — `pnpm run lint` now passes (0 errors). Added
  `typecheck` script. `next build` green.

### Phase B — confirmed fixes ✅ (one commit per fix, each with a regression test)
- **H1** invite `max_uses` TOCTOU → atomic compare-and-swap claim (`lib/invite-claim.ts`).
- **M1** board re-arming an admin invite → privilege guard on any re-arming field.
- **M2** `appealIncident` → restricted to admin/board; asserts the link UPDATE, rolls back orphan proposal.
- **M3** SSRF filter → blocks hex IPv4-mapped IPv6.
- **M4/M5** payment `last_paid_at` → advance-only (`resolveDuesAdvance`).
- **M6 / L6** members-list & payment-picker null derefs → guarded.
- **member-directory visibility gate** → repaired (was comparing against non-existent enum values).
- **L1** `reserveEquipment` → verifies target member is in the caller's space.
- **L2** unverified members → blocked from proposals/votes (per decision).
- **L5** `parseHeatSyncLog` → stale time-of-day + token-reuse fixed.
- **L7** CSV import → guards unparseable dates, never wedges the button.
- **Tests**: tautological `actions.test` replaced with real authz/IDOR tests; authz primitives covered.
  626 tests green.
- **Deferred — L3** (`markOnboardingStepDone` read-modify-write race): a correct fix needs a DB-side
  atomic append (an RPC/migration) that can't be applied to the live DB from here. Low severity
  (duplicate step-completions only). Follow-up: add a `jsonb` append RPC or a unique constraint.

### CI ✅
- Added `.github/workflows/ci.yml` — a checks-only workflow (typecheck + lint + unit tests) on every PR
  and on `main`, fully separate from `deploy.yml`. Follow-up: a DB-backed integration run that fails on
  0 executed tests.

### Phase C — refactor ✅ (all 10 slices)
- **Slice 0**: `types/database.ts` marked GENERATED.
- **Slice 1**: `lib/validations.ts` (143 exports) → `lib/validations/*` barrel'd folder (symbol-diff
  verified, 40 importers untouched).
- **Slice 2**: `lib/types.ts` → `types/domain/*`.
- **Slices 3–5**: `lib/actions/{classes,door,forms}.ts` (800–908 lines) → per-capability folders with
  `_guard`/`_helpers` (plain modules) + `'use server'` sub-files + `index.ts` barrel. Export parity
  verified (14/14/15); no helper leaked into the public surface.
- **Slices 6–8**: `ops`/`settings`/`members` client components decomposed into thin orchestrators
  (956→230, 663→79, 663→255) + `panels/`+`components/`+`types.ts`. Slice 8 also fixed the
  `MemberFormFields`-in-render focus footgun.
- **Slice 6b (security)**: the ops "Add Secret" modal wrote the value **plaintext** from the browser
  despite its "encrypted at rest" label — routed through `createSecret` (AES-GCM + authz + audit).
- **Slice 9**: deleted orphaned files/exports + the 6 dead validation schemas + `cleanupRateLimits`;
  migrated `@/lib/types` → `@/types/domain` and dropped the shim.
- Each slice: its own commit; gated on tsc 0 + tests + (for UI) `next build`.

### Phase D — ARCHITECTURE.md ✅
- `ARCHITECTURE.md` at repo root: layers, the server-action contract + space-scope invariant, the
  feature-folder conventions, and a step-by-step add-a-feature guide.

### Follow-ups (documented in ARCHITECTURE.md "Known limitations")
- Regenerate `types/database.ts` (drops the `// TODO(types)` join casts + hand-added RPCs; lets the
  governance rows in `types/domain/governance.ts` become `Tables<>`/`Enums<>`).
- Migrate the ops **area-lead** browser writes to server actions (needs reconciling with
  `upsertAreaLead`'s `area_code` interface). RLS-protected today.
- Deferred **L3** onboarding-race RPC; DB-backed CI integration run.
- **UI smoke-test** the decomposed ops/settings/members screens before merging (structural refactor,
  verified by tsc + build + tests, but not by an automated UI test).