security(api): 36 read routes returned customer data to any signed-in user

[goetchstone]

Chasing one reported gap — the warehouse returns queue — turned up the class. 83 Pages Router route files called getServerSession, confirmed somebody was signed in, and proceeded. No role, no permission.

Several sit behind pages that are role-gated, which is what made it invisible: the UI looked restricted while the data was not.

Triaged, not blanket-gated

6 mutate	The NextAuth handler and 5 crons carrying a Bearer AUTO_IMPORT_API_KEY — already covered by the existing mutating-route tripwire and its reasoned allowlist.
36 read	The exposure. Customer names, phone numbers, pickup addresses, delivery notes, vendor cost positions — and in two cases the entire customer book as a CSV, no date filter, no pagination.

Each route takes an existing permission copied from a sibling route doing the same job, never an invented mapping. Where no API sibling existed, the page's own requirePage() permission was used (service/dashboard.ts). Where neither existed, the route was skipped.

Two failure modes, both caught by the adversarial pass

Widening. Three routes already had inline role !== "MANAGER" && … checks that my initial grep didn't recognise as gates, so they were mis-triaged as unguarded. On leads/needs-attention.ts the replacement permission sales.lead is one DESIGNER holds — the "fix" would have loosened a working control. That file and mailchimp/backfill-customer-links.ts are reverted; neither was ever unguarded.

Over-gating. Three inventory reports were gated on inventory.count (ADMIN/MANAGER/SUPER_ADMIN/WAREHOUSE) when the pages behind them are reachable by anyone the Inventory nav admits — inventory.read, which adds DESIGNER and REGISTER. That would have 403'd staff mid-workflow, a worse outcome for the business than the leak. Corrected to inventory.read; scan-history and unidentified-scans keep inventory.count because they're called from the count workflow itself.

Deliberately not changed: reports/detailed-sales/export.ts takes reporting.export while its page takes reporting.read, so DESIGNER and MARKETING can view but not export. That split is documented at navPermissions.ts:71 — intent, not an over-gate.

The tripwire, and why its first draft was worthless

__tests__/apiReadRouteAuthorization.test.ts is a companion to the existing apiRouteAuthorization.test.ts — that one covers mutating routes, this one reads. Two files because the rules differ: a mutating route needs a gate full stop; a read route needs one matching its page's audience.

My first draft examined only routes calling getServerSession. The both-directions check killed it immediately: adding requirePermission replaces that call, so every route this sweep fixed left the population the moment it was fixed. It could watch the debt shrink and never notice a gate being torn out again — the exact regression it exists to catch.

It now covers all 448 routes, and removing a gate fails it by name. The inline-role-check pattern also matches both role !== "X" guards and role === "X" || … allows; a first draft caught only the negation and misread a route that was already protected.

The remaining 64 unguarded routes are pinned in a fixture as declared debt rather than silently excluded, so they're counted and shrinking them is visible work.

Still open

The pages in front of these endpoints are mostly bare requirePage() with no roles — the nav hides them, but the URL works for anyone signed in. Gating the APIs closes the data leak; the pages are a separate pass.

Verification

• next build — exit 0
• tsc --noEmit — 0
• jest --selectProjects unit — 213 suites, 3,302 tests
• Both tripwires green; both-directions verified on two separate routes

🤖 Generated with Claude Code