Moheeb ZaraandClaude Opus 4.8
1766780bca
The installed supabase-js (2.99, 5 generics) is incompatible with
@supabase/ssr@0.6.1 (calls it with 3), collapsing the server client's
schema to `never` — the root cause of ~450 of the 586 hidden type errors
(next.config typescript.ignoreBuildErrors masks them all).
- lib/supabase/server.ts: annotate the factory as SupabaseClient<Database>
and bridge the ssr/js impedance mismatch at the one documented boundary;
type the setAll cookie params.
- lib/supabase/proxy.ts, proxy.ts: type the setAll cookie params.
- lib/auth-helpers.ts: parseInput now infers its result from the schema via
z.infer (was a fragile ZodSchema<T> that widened outputs to {}).
- lib/validations.ts: annotate flexibleDateTime's output type.
- types/database.ts: add class_signup_tx / class_cancel_tx RPC signatures
(called but absent; pending full regeneration).
- tsconfig.json: target ES2020 (door-log-logic uses BigInt literals).
- vitest.setup.ts: import beforeAll/afterAll instead of relying on globals.
No runtime behavior change; 639 tests still green. ignoreBuildErrors stays
on until the remaining errors reach zero.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
231 lines
16 KiB
Markdown
231 lines
16 KiB
Markdown
# 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.**
|