Pre-Launch Security Review

API contract review — branch feat/orders-v2 vs main

| # | Change | Endpoint | Severity | Rule |
|---|---|---|---|---|
| 1 | `customer_id` removed from response | GET /api/v1/orders/{id} | BREAKING | removed response field (R2) |
| 2 | `status` enum gains `refunded` | GET /api/v1/orders | ADDITIVE, risky | new enum value; strict clients may throw (A4) |
| 3 | `limit` max lowered 100 → 50 | GET /api/v1/orders | BREAKING | tightened validation (R5) |
| 4 | `X-Request-Id` header documented | all | COSMETIC | docs only |

Verdict: 2 breaking changes. Mobile 2.3.x reads `customer_id` on the order screen and will render an empty name.

Recommendation: keep `customer_id` as a deprecated alias for two releases, raise `limit` back to 100 with a server-side cap, and ship `refunded` behind the existing `status` field with a changelog note. No new API version needed if both are done.

Migration note for client teams: handle unknown `status` values as `other` before 2026-12-01.

Project: fresh `npm init -y` + `npm i -D vitest typescript` (Vitest 5.1.0, TypeScript strict). Every runner output below is pasted from the actual run.

Behaviour list (simplest first): 1. returns empty for blank input · 2. returns the address trimmed and lowercased when valid · 3. returns malformed when there is no @ · 4. returns malformed when there is more than one @ · 5. returns malformed when the domain has no dot · 6. returns too_long when the address is over 254 characters

Cycle 1: returns empty for blank input

```
[TDD] cycle 1/6  step RED  behaviour: returns empty for blank input
test:    src/validateEmail.test.ts > validateEmail > returns empty for blank input
ran:     npx vitest run src/validateEmail.test.ts -t "returns empty for blank input"
result:  FAIL  Error: Cannot find module './validateEmail' imported from <project>/src/validateEmail.test.ts
reason:  right (the unit under test does not exist yet: missing function, not a typo in the test)
changed: src/validateEmail.test.ts (+8, new file)
next:    minimum code in src/validateEmail.ts
```

```
[TDD] cycle 1/6  step GREEN  behaviour: returns empty for blank input
ran:     npx vitest run src/validateEmail.test.ts -t "returns empty for blank input" --reporter=verbose; npx vitest run src/validateEmail.test.ts
result:  PASS  Tests 1 passed (1)
changed: src/validateEmail.ts (+9, new file: result types + hard-coded `{ ok: false, error: 'empty' }`)
next:    refactor
```

Refactor: no refactor (hard-coded return is forced into the general version by cycle 2).

[Cycles 2–6 follow the same shape: each RED shows a real assertion mismatch such as `Tests 1 failed | 1 skipped (2)`, each GREEN the passing count, with refactors in cycles 5 and 6.]

Done

```
[TDD] DONE  cycles 6  tests added 6  suite PASS
ran:     npm test   (vitest run)   and   npx tsc -p .
result:  Test Files  1 passed (1) | Tests  6 passed (6); tsc exit 0
pre-existing failures: none (no tests existed)
found, not fixed: the 254 boundary itself (exactly 254 chars accepted) has no test; it passes today, so a test would be green on first run. 'a@.com' and 'a@b.' are accepted because the spec only asks for a dot in the domain.
```

No-fake-green check before every GREEN: no `.skip`/`.todo`, no assertion weakened, no test-only branch, nothing mocked.

> Assumption: signed-in users read their own orders and create their own orders as `pending`; every status change after that is made by your server with the `service_role` key, never by the browser.

Audit: `public.orders` (about 1.8M rows)

Rule files applied: `security-rls-basics`, `security-rls-performance`, `security-privileges`, `schema-foreign-key-indexes`, `query-missing-indexes`, `query-composite-indexes`, `schema-data-types`, `schema-constraints`, `lock-short-transactions`, `monitor-explain-analyze`.

Findings, by rule (first 6 of 12)

| # | Severity | Rule | Finding |
|---|---|---|---|
| 1 | CRITICAL | `security-rls-basics` | `orders_insert ... with check (true)` lets anyone who can insert write an order for any `user_id`. Combined with finding 2, an anonymous visitor holding only your public anon key can create orders on behalf of any user. |
| 2 | CRITICAL | `security-privileges` | `grant select, insert, update ... to anon`. Orders belong to signed-in users, so `anon` needs no privilege at all. None of the three policies has a `to` clause, so each applies to every role, including `anon`. |
| 3 | HIGH | `security-privileges` | `authenticated` can `update` every column of its own rows: the owner can still run `update orders set status = 'paid', total_cents = 1` from the browser. |
| 4 | HIGH | `security-rls-basics` | The insert path also trusts `status`: a client can insert a row that is already `status = 'paid'`. |
| 5 | HIGH | `security-rls-performance` | All policies call `auth.uid()` bare, so it is evaluated per row. Wrap it as `(select auth.uid())` so it is evaluated once per statement. |
| 6 | HIGH | `schema-foreign-key-indexes`, `query-missing-indexes` | No index on `user_id`. It is the foreign key, the RLS filter column and the column every "my orders" query filters on. Each RLS-filtered select over 1.8M rows is a sequential scan. |

Fixing migration (file 1 of 2, excerpt)

```sql
set local lock_timeout = '5s';

revoke all on public.orders from anon;
revoke update, delete, truncate, references, trigger on public.orders from authenticated;
grant select, insert on public.orders to authenticated;

drop policy if exists orders_select on public.orders;
drop policy if exists orders_insert on public.orders;
drop policy if exists orders_update on public.orders;

create policy orders_select on public.orders
  for select to authenticated
  using ((select auth.uid()) = user_id);

create policy orders_insert on public.orders
  for insert to authenticated
  with check ((select auth.uid()) = user_id and status = 'pending');
-- No update or delete policy: denied for every role except service_role.
```

File 2 holds only `create index concurrently if not exists orders_user_id_created_at_idx on public.orders (user_id, created_at desc);` because `concurrently` cannot run inside the transaction the Supabase CLI wraps around each migration.

[The full answer continues with findings 7–12, the lock plan for 1.8M rows, the status and total_cents check constraints added `not valid` then validated, an `explain analyze` to run before and after, and one RLS test per role.]

Assumption: no n8n-MCP server is connected in this session, so I took the skill's fallback path: I wrote the workflow JSON directly from the bundled rules and checked it by review (a rule-check script plus a dry run of every expression). The real validator (`validate_workflow`) was not run, and nothing was deployed.

How the skill routed this

| Step | Sub-skill used | What it decided |
|---|---|---|
| Shape | `n8n-workflow-patterns` | Webhook processing pattern: Webhook, then Validate, then Action, then Respond |
| Node parameters | `n8n-node-configuration` | Slack post needs a channel and text. `responseMode: "responseNode"`. `options.responseCode` set on every Respond node. UUID node ids, no placeholder credentials |
| Expressions | `n8n-expression-syntax` | Webhook data lives under `.body`. Use `$('Webhook').item.json.body.*` once past the IF node. `=` prefix on every expression field |
| Error paths | `n8n-error-handling` | Slack gets `onError: "continueErrorOutput"` and its `main[1]` is wired to the 500 responder. `retryOnFail` is on. Bad input is caught upstream and gets a 400 |
| Validation | `n8n-validation-expert` | Runtime-profile checks done by review. Security warnings get fixed (header auth on the webhook) |

```
Webhook ──> Validate Input ──true──> Post to #signups ──success (main[0])──> Respond 200 OK
                        └──false──> Respond 400 Invalid Input
                                     Post to #signups ──error (main[1])────> Respond 500 Slack Failed
```

Validation result (excerpt)

```
PASS  Webhook responseMode is responseNode (Respond nodes control the reply)
PASS  Every branch (success and each error) ends at a Respond to Webhook node: no hanging branch
PASS  'Post to #signups': onError=continueErrorOutput AND main[1] wired
PASS  'Post to #signups': retryOnFail with maxTries <= 5 and waitBetweenTries <= 5000
PASS  Every {{ }} field starts with = (no 'Expression requires = prefix' error)
PASS  Webhook fields read from .body, never $json.name / $json.email / $json.plan
…
43 passed, 0 failed
```

Expression dry run against sample webhook items:

```
valid           IF -> true  (Slack)  Slack text: New signup: Ana Lima (ana@example.com) on pro
missing_plan    IF -> false (400)
bad_email       IF -> false (400)
no_body         IF -> false (400)
500 body:   {"error":"upstream_error","message":"The signup was received but could not be posted to Slack. Please retry later."}
```

Not verified without the server: the node typeVersions (Webhook 2.2, IF 2.3, Slack 2.3, Respond 1.5) and the Slack v2 parameter names; confirm by opening the nodes after import.

[The full answer also holds the six-node list, the importable workflow JSON and the expressions table with the reason behind each one.]

2. Tool design

| Tool | Does | Inputs (constraints) | Returns | Read-only / destructive | Errors the model can fix |
|---|---|---|---|---|---|
| `find_customers` | Customers whose name or email contains the text, case-insensitive | `query` string 1–100 (trimmed), `limit` int 1–50 = 20, `cursor` optional | id, name, email; `next_cursor` | read-only, idempotent | no match: try a shorter part of the name or the email domain |
| `list_customer_orders` | One customer's orders, newest first | `customer_id` int ≥ 1, `status` enum optional, `limit`, `cursor` | customer + orders; `next_cursor` | read-only, idempotent | unknown customer: call `find_customers` |
| `get_order` | One order with the customer's name and email | `order_id` int ≥ 1 | order + customer (structured) | read-only, idempotent | not found: check the number or list the customer's orders |
| `refund_order` | Sets a paid order to refunded | `order_id` int ≥ 1, `confirm` int optional, must equal `order_id` | updated order, `previous_status: "paid"` | destructive, idempotent | no `confirm`: preview, ask the user; order pending or already refunded; not found |

Checklist notes: one rule for empty results (an `isError` result with a next step), applied to every tool. All SQL is parameterized. Read tools use a read-only SQLite handle; only `refund_order` opens a writable one, and its `UPDATE ... WHERE id = ? AND status = 'paid'` is a single statement. The guard on the destructive tool is server-side (the preview/confirm step), not just the `destructiveHint` annotation.

3. Scaffold

From `templates/typescript-server.md`. Installed: `@modelcontextprotocol/server` 2.4.0, `@modelcontextprotocol/client` 2.4.0, `zod` 4.6.7; Node 24.6.0.

```
build-an-mcp-server/
  data/seed.sql, data/shop.db     3 customers, 5 orders
  src/index.ts                    stdio entry point
  src/server.ts                   createServer(): the four tools
  src/shop.ts                     SQLite client module (the system being wrapped)
  test/smoke.ts                   spawns the server over stdio, calls every tool
```

5. Test (Inspector CLI, eval loop layer 2)

```
$ npx @modelcontextprotocol/inspector --cli npx tsx src/index.ts --method tools/call --tool-name get_order --tool-arg order_id=1001
{
  "content": [{ "type": "text", "text": "Order #1001 for Ada Lovelace <ada@example.com>: $49.99, paid, created 2026-10-02T09:15:00Z" }],
  "structuredContent": { "order": { "id": 1001, "customer_id": 1, "total_cents": 4999, "status": "paid", "created_at": "2026-10-02T09:15:00Z", "customer_name": "Ada Lovelace", "customer_email": "ada@example.com" } }
}
```

Built, tested, not tested

- Built: four tools over `data/shop.db`, server-side preview/confirm guard on `refund_order`, smoke test, configs for Claude Code, Codex and Cursor, README.
- Tested: starter smoke (pass), full smoke test 14 checks (pass), strict typecheck (pass), Inspector CLI call (pass), seed database unchanged after tests.
- Not tested: the Inspector web UI by hand and the model check (running the five real requests through a live Claude Code session and recording which tool it picks).

[The full answer also holds `src/shop.ts`, `src/server.ts`, `test/smoke.ts` and the three client config snippets.]

Phase 1: Root cause investigation
Full suite: FAIL test_expired_coupon_rejected_twice (tests/test_checkout.py:16) AssertionError: Coupon(code='OLD50', …expires_at=2020-01-01…) is not None.
In isolation the same test fails at a different line (17): AssertionError: 50.0 != 100.0. So the first lookup in a process is rejected and every later lookup is accepted; in the full suite the first lookup was done by test_expired_coupon_rejected. Cross-test pollution through module-level state.
Cache probe at the boundary: cache before [] → 1st None → cache after ['OLD50'] → 2nd Coupon(OLD50). Docstring says the cache was added in 3f2a1c9 "to cut DB reads": that is the regression point.
Trace (root-cause-tracing.md): apply_coupon → get_coupon → `if code in _CACHE: return _CACHE[code]`. get_coupon writes the coupon to _CACHE before the expiry check, and the cache-hit branch has no expiry check at all.

Phase 2: Pattern analysis
Working path: cache miss → _load → expiry check. Broken path: cache hit. Expiry depends on `now`, passed per call, so it cannot be a cached property; "don't cache expired coupons" alone would still serve a coupon cached while valid after it expires.

Phase 3: Hypothesis
"get_coupon returns cached coupons without re-checking expires_at against the caller's now; the cache was added around the lookup but the validity check stayed only on the miss path." Confirmed by the probe above.

Phase 4: Implementation
Failing test first: test_cached_coupon_rechecked_for_expiry (look up while valid, then after expiry) plus a setUp that clears the module cache. Before the fix: 5 tests, 2 failures, both for the expected reason.
Fix at the source: the cache stores the raw record; the expiry check runs on every lookup, cached or not.

    c = _CACHE.get(code)
    if c is None:
        c = _load(code)
        if c is None:
            return None
        _CACHE[code] = c
    if c.expires_at < now:
        return None
    return c

Verification: python3 -m unittest -v → Ran 5 tests … OK. The originally failing test in isolation, 3 runs: OK, OK, OK. Fix attempts: 1.

Stage 1, brainstorm. Echo: accountants want one month of invoices as CSV. listInvoices({month}) already filters by YYYY-MM; the seed data has "Acme, Inc." (comma) and Bo "Bob" Ltd (quotes), so RFC 4180 quoting is required. One question: should drafts be included? Accountants usually book only issued invoices; I'd exclude them. (User: exclude.)

Approaches
A. A hand-written toCsv plus exportMonthCsv in a new src/invoices/csv.js: zero dependencies, testable. Recommended.
B. A CSV library: needs npm install, overkill for 8 columns.
C. Put the logic in store.js: mixes formatting with storage.
Small change, so a short design doc and no builder check. Written and approved: docs/specs/2026-11-07-invoice-csv-export-design.md (goals, non-goals, decisions, my calls: CRLF line endings, invalid month throws RangeError).

Stage 2, plan: docs/plans/2026-11-07-invoice-csv-export.md
Global Constraints: no dependencies; columns number, issued_at, client, net, tax, total, currency, status; CRLF and RFC 4180 quoting; drafts excluded.
Review Focus: a comma in the client name; quotes in the client name; an empty month returns the header only; a malformed month throws RangeError; a field containing a newline.
Task 1: CSV serializer. Create src/invoices/csv.js, tests/csv.test.js. Produces toCsv(rows, columns) and INVOICE_COLUMNS. Red, green, commit.
Task 2: Month export. Produces exportMonthCsv(month). Tests: October gives 3 lines and contains "Acme, Inc."; November gives the header only (INV-003 is a draft); '2026-1' and undefined throw RangeError.
User approved and chose inline execution.

Stage 3, ledger (.spec-first/sdd/…/progress.md)
Pre-flight: Ruling: plan Task 1 Step 2 expects FAIL "module not found"; accepted as the expected red for a brand-new module.
Task 1: complete (commits 2b2d09a..bb43c6c, tests: node --test → 2 of 2 passed after red ERR_MODULE_NOT_FOUND)
Task 2: complete (commits bb43c6c..6e3c9a7, tests: node --test → 3 of 3 passed)
Final review: fresh reviewer on the most capable model: "With fixes": 0 Critical, 2 Important, 3 Minor.
Final: fixed CSV formula injection (string fields starting = + - @ TAB CR get a leading '), test red → green, suite 5/5
Final: fixed impossible months 2026-13 / 2026-00 accepted, test red → green, suite 5/5
Final: Ruling: formula guard applies to string values only; negative numeric amounts (credit notes) stay numbers.
Final: minor (deferred): no UTF-8 BOM; Excel on Windows may garble non-ASCII client names.

git log, branch feat/invoice-csv-export
77d1a3b fix: neutralize spreadsheet formulas and reject impossible months in CSV export
6e3c9a7 feat: export a month of invoices as CSV
bb43c6c feat: add RFC 4180 CSV serializer for invoices
2b2d09a docs: invoice CSV export plan
7e753cd docs: invoice CSV export design
Finish options offered: merge, PR, keep, discard. […]

Scope: app/dashboard/page.tsx and the modules it imports (components/charts, UsageCard, ProjectList, RichTextEditor, lib/api.ts).

| # | Rule | File:line | Impact | Finding |
|---|---|---|---|---|
| 1 | async-dependencies | app/dashboard/page.tsx:8-10 | CRITICAL | getProjects(user.id) and getUsage(user.id) depend only on `user` but are awaited one after the other: 3 sequential round trips before any HTML is sent, where 2 would do. Promise.all them. |
| 2 | bundle-barrel-imports | components/ProjectList.tsx:3 | CRITICAL | `import _ from 'lodash'` in a 'use client' file ships all of lodash for one sortBy. Use lodash/sortBy, or toSorted / localeCompare. |
| 3 | async-suspense-boundaries | app/dashboard/page.tsx:7-19 | HIGH | The whole page, including the static shell and RichTextEditor, waits for all three fetches. Move usage and projects into async children in <Suspense> so the shell streams first. |
| 4 | server-serialization | app/dashboard/page.tsx:16 | HIGH | The raw projects array goes to client ProjectList, which reads only id, name and status. Map to {id, name, status} on the server. |
| 5 | rerender-no-inline-components | components/ProjectList.tsx:7 | HIGH | `Row` is defined inside ProjectList, so every keystroke in the filter remounts every <li>. Interaction cost, not first load. Hoist Row. |
| 6 | rerender-derived-state-no-effect | components/UsageCard.tsx:4-5 | MEDIUM | percent is kept in state and set in useEffect: server HTML renders "0% of plan used" and the real value appears only after hydration. Compute it during render. |

Score: 62 / 100 (100 − 2×10 CRITICAL − 3×5 HIGH − 1×3 MEDIUM). This counts rule findings; it is not a measured load time.

Checked and not scored: the local charts barrel (five tiny stubs, tree-shakeable); RichTextEditor is a <textarea> today, so no dynamic import yet. Not a performance rule but it blocks the build: components/charts/index.ts re-exports names the chart files do not export, and page.tsx reads `params.id` synchronously though params is a Promise in Next 15.

What makes the first load slow, in order:
1. The server waterfall (rows 1 and 3). Parallelizing projects and usage removes one full round trip; Suspense lets the header paint after only getUser.
2. Client JS weight (row 2). A one-line import change.
3. Payload and first-paint correctness (rows 4 and 6).
Row 5 is an interaction cost; fix it after the others. Fixes were not requested, so no files were changed.