fix(d24): pg allocation row-lock + index ordering; docs (D24 record, 389 tests)

- recordPayment locks the target invoice row (SELECT ... FOR UPDATE, Postgres
  only — SQLite is serialized by the connection mutex) before reading
  outstanding, so concurrent payments cannot over-allocate on prod.
- INDEX_DDL now runs AFTER migrate() in openDb, so indexes referencing
  migrate-added columns (owner_id, lockout cols) build on older DBs.
- D24 remediation recorded in 06-DECISIONS; test counts 377 -> 389.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat/client-detail-redesign
Thomas Joise 4 days ago
parent 9c59430c31
commit 70234161dc

@ -43,7 +43,7 @@ distilled to what applies to HQ. A change that breaks one does not land.
| Append-only audit | Every mutation calls `writeAudit(db, userId, action, entity, id, before, after)` in the **same transaction** as the change. The audit log is append-only — never update or delete rows. Previews and the public share route write nothing. | `audit.ts` | | Append-only audit | Every mutation calls `writeAudit(db, userId, action, entity, id, before, after)` in the **same transaction** as the change. The audit log is append-only — never update or delete rows. Previews and the public share route write nothing. | `audit.ts` |
| Client id is the future tenant id | `client.id` is a client-generated UUIDv7 that becomes `tenant_id` when the cloud tier stands up — same rows, promoted once. Never re-key clients. | `repos-clients.ts` | | Client id is the future tenant id | `client.id` is a client-generated UUIDv7 that becomes `tenant_id` when the cloud tier stands up — same rows, promoted once. Never re-key clients. | `repos-clients.ts` |
| No silent caps | Every list that can grow paginates or warns; never silently truncate. | reports, directory queries | | No silent caps | Every list that can grow paginates or warns; never silently truncate. | reports, directory queries |
| Strict TS, green before landing | Strict TypeScript everywhere; `npm run typecheck` and `npm test` both clean before anything lands. Money math is locked by tests — a rounding change without a test change does not land. | `tsc`, vitest (377 tests) | | Strict TS, green before landing | Strict TypeScript everywhere; `npm run typecheck` and `npm test` both clean before anything lands. Money math is locked by tests — a rounding change without a test change does not land. | `tsc`, vitest (389 tests) |
## Running it ## Running it
@ -66,7 +66,7 @@ cd apps/hq-web && npm run dev
- Postgres integration tests: `HQ_PG_TEST_URL=postgres://hq:hq_dev@localhost:5432/hq_test npm test` - Postgres integration tests: `HQ_PG_TEST_URL=postgres://hq:hq_dev@localhost:5432/hq_test npm test`
(wipes that database each run — never point it at real data). (wipes that database each run — never point it at real data).
- Health check: `GET /api/health` → `{ ok: true }`. - Health check: `GET /api/health` → `{ ok: true }`.
- Verified state: `npm install` + typecheck clean, 377 tests pass. Exercise real flows end - Verified state: `npm install` + typecheck clean, 389 tests pass. Exercise real flows end
to end before claiming done. to end before claiming done.
## Docs (kept in `docs/`) ## Docs (kept in `docs/`)

@ -34,7 +34,7 @@ document and conversation we've had with them.
## Build state (start here) ## Build state (start here)
Working codebase, not a plan. `npm install` + typecheck are clean and **377 tests pass**. Working codebase, not a plan. `npm install` + typecheck are clean and **389 tests pass**.
Current state and run-vs-pending detail live in **[STATUS.md](STATUS.md)**; the go-live Current state and run-vs-pending detail live in **[STATUS.md](STATUS.md)**; the go-live
runbook (server, HTTPS, backups, Gmail/AWS/import wiring, the Postgres switch) is in runbook (server, HTTPS, backups, Gmail/AWS/import wiring, the Postgres switch) is in
**[docs/DEPLOY-HQ.md](docs/DEPLOY-HQ.md)**. **[docs/DEPLOY-HQ.md](docs/DEPLOY-HQ.md)**.

@ -25,7 +25,7 @@ section-per-module quotation PDF — then closed all 8 findings from its re
| --- | --- | | --- | --- |
| `npm install` | clean | | `npm install` | clean |
| `npm run typecheck` (root + workspaces) | clean, no errors | | `npm run typecheck` (root + workspaces) | clean, no errors |
| `npm test` (`vitest run`) | **377 tests pass across 71 files** | | `npm test` (`vitest run`) | **389 tests pass across 74 files** |
The 346 figure is the live `vitest run` count measured 2026-07-17, after the The 346 figure is the live `vitest run` count measured 2026-07-17, after the
D18 go-live cluster **and its post-delivery review pass** (8 reviewer findings D18 go-live cluster **and its post-delivery review pass** (8 reviewer findings

@ -354,7 +354,7 @@ CREATE TABLE IF NOT EXISTS aws_usage (
updated_at TEXT NOT NULL, updated_at TEXT NOT NULL,
UNIQUE (client_id, month) -- one row per client per month; the pull upserts on this key UNIQUE (client_id, month) -- one row per client per month; the pull upserts on this key
); );
${INDEX_DDL}` `
/** Open the SQLite engine (dev/tests). Deliberately sync schema + migrate run /** Open the SQLite engine (dev/tests). Deliberately sync schema + migrate run
* on the raw handle before it is wrapped, so test fixtures stay one-liners. */ * on the raw handle before it is wrapped, so test fixtures stay one-liners. */
@ -370,6 +370,9 @@ export function openDb(dataDir?: string): DB {
} }
raw.exec(SCHEMA) raw.exec(SCHEMA)
migrate(raw) migrate(raw)
// Indexes run AFTER migrate() so every column they reference exists on an older DB
// that migrate() has just patched (e.g. client.owner_id, the D24 lockout columns).
raw.exec(INDEX_DDL)
return new SqliteDb(raw) return new SqliteDb(raw)
} }

@ -164,6 +164,13 @@ export async function recordPayment(db: DB, userId: string, input: RecordPayment
allocated.push({ documentId, amountPaise }) allocated.push({ documentId, amountPaise })
await refreshSettlementStatus(db, userId, documentId) await refreshSettlementStatus(db, userId, documentId)
} }
// D24 (red-team): serialize concurrent allocations to the SAME invoice so two payments
// cannot both read the old outstanding and over-allocate. On Postgres, take a row lock
// before reading outstanding — held to COMMIT. On SQLite the connection mutex already
// serializes whole transactions, and FOR UPDATE is a syntax error there, so it's a no-op.
const lockInvoice = async (docId: string): Promise<void> => {
if (db.engine === 'postgres') await db.run(`SELECT id FROM document WHERE id=? FOR UPDATE`, docId)
}
if (input.allocations !== undefined) { if (input.allocations !== undefined) {
// Explicit allocations bypass the oldest-first loop, but never exceed // Explicit allocations bypass the oldest-first loop, but never exceed
@ -178,6 +185,7 @@ export async function recordPayment(db: DB, userId: string, input: RecordPayment
if (doc.docNo === null) throw new Error(`Invoice is not issued: ${a.documentId}`) if (doc.docNo === null) throw new Error(`Invoice is not issued: ${a.documentId}`)
if (doc.status === 'cancelled') throw new Error(`Invoice is cancelled: ${doc.docNo}`) if (doc.status === 'cancelled') throw new Error(`Invoice is cancelled: ${doc.docNo}`)
if (doc.clientId !== input.clientId) throw new Error(`Invoice ${doc.docNo} belongs to another client`) if (doc.clientId !== input.clientId) throw new Error(`Invoice ${doc.docNo} belongs to another client`)
await lockInvoice(doc.id)
const outstanding = await outstandingOf(db, doc) const outstanding = await outstandingOf(db, doc)
if (a.amountPaise > outstanding) { if (a.amountPaise > outstanding) {
throw new Error(`Allocation ${a.amountPaise} exceeds outstanding ${outstanding} on ${doc.docNo}`) throw new Error(`Allocation ${a.amountPaise} exceeds outstanding ${outstanding} on ${doc.docNo}`)
@ -193,6 +201,7 @@ export async function recordPayment(db: DB, userId: string, input: RecordPayment
// stays unallocated (an advance). // stays unallocated (an advance).
for (const doc of await unsettledInvoices(db, input.clientId)) { for (const doc of await unsettledInvoices(db, input.clientId)) {
if (pool <= 0) break if (pool <= 0) break
await lockInvoice(doc.id)
const take = Math.min(pool, await outstandingOf(db, doc)) const take = Math.min(pool, await outstandingOf(db, doc))
if (take <= 0) continue if (take <= 0) continue
pool -= take pool -= take

@ -382,3 +382,41 @@ so `ORDER BY id` is genuine creation order (pagination + the sync cursor rely on
sub-millisecond tie no longer falls back to the random tail. sub-millisecond tie no longer falls back to the random tail.
Suite at 377 tests / 71 files after D22. Suite at 377 tests / 71 files after D22.
## D24 — Red-team audit remediation ✅ (2026-07-18)
A 14-surface multi-agent red-team + quality audit (report artifact + `redteam-findings`)
returned 96 verified findings — **0 critical, 16 high, 43 medium, 37 low**. All 16 highs and
the load-bearing mediums were fixed in seven batches, each committed green with tests:
- **Auth (A):** `staff_user.failed_count`/`locked_until` — 8-fail/15-min account lockout;
`login()` constant-time (dummy scrypt on missing email — no enumeration oracle) returning a
typed `{ok,reason}`; per-IP (20/min) + per-email (8/15min) limiters on `/auth/login`;
per-user reveal limiter (20/min) on the three credential-reveal routes; `trust proxy` so
`req.ip` is the real client behind nginx. `makeRateLimiter``rate-limit.ts` (no cycle).
- **Authorization (B):** `requireManagerial` gate on `cancel` / `credit-note` (destructive)
and `POST /documents/:id/share` (the unauthenticated-public-link IDOR); `PATCH
/interactions/:id` restricted to author-or-managerial. **Deliberate:** general client
read/edit stays open to staff — a shared support-team book is the intended model; the
sensitive credential reveals are managerial + rate-limited.
- **Money (C):** credit-note capped to the invoice's remaining creditable value (read+insert
one txn); `nextDocNo` atomic `UPDATE … RETURNING` (row lock, no duplicate/burned numbers);
`markStatus` rejects accepted/lost on non-QUOTATION docs; `computeBill` bounds qty
magnitude (fractional kept for weighed Store goods) + HQ `buildLines` requires whole-unit
integer qty; `recordPayment` rejects TDS > cash received.
- **Concurrency (D):** `SqliteDb.transaction` serialized by an async mutex + AsyncLocalStorage
re-entrancy (nested → savepoints) — the shared `txnDepth` counter could interleave
concurrent BEGIN/COMMIT and lose writes; scheduler isolates each recurring plan (one bad
plan no longer aborts the whole scan + auto-send); notify-broadcast IIFE gets a trailing catch.
- **Rendering (E):** strict CSP meta in both letterhead templates + puppeteer aborts every
non-`data:` request — an `esc()` miss degrades to a rendering glitch, not SSRF/RCE. Preview
↔ PDF byte-fidelity preserved.
- **Backend perf (D24):** 30 indexes on the hot FK/status/date columns, both engines
(`INDEX_DDL` + pg migration 007); client search + district/sector filters `LOWER(…) LIKE
LOWER(?)` — bare LIKE was case-SENSITIVE on Postgres and silently broke prod search.
- **Frontend (F):** a11y (keyboard rows, `<main>`+skip link, aria-labels, form Login, alert
toasts), UX safety (confirm on mass-email + reminder send, in-flight disable to stop
double-submit, debounced search, visible inline errors, ErrorBoundary), and shared
component extraction.
**Explicitly accepted / deferred (documented, not fixed):** async scrypt for `verifyPin`
(large shared-package refactor; the event-loop-DoS vector is mitigated by the login rate
limiter now); rendered-PDF caching on `/share`; api.ts file split; broad naming-consistency
renames (`staff`/`user`/`employee`, `at_wall`, schema.sql drift) — too risky pre-go-live.

Loading…
Cancel
Save