`lawyer-directory-builder/claude-review.md`

39 lines

# Claude review of codex patches — lawyer-directory-builder

## Verdict
MIXED — the 13 body-guard patches are correctly applied and the data_market.ts miss was caught in pass 2, but the P0 Stripe webhook ordering bug remains live and codex's pass-2 sign-off on it was silent.

## Where codex got it right
- All 9 body guards (`!req.body || typeof req.body !== 'object'`) are present and correctly placed before any destructuring. Non-object/null bodies are handled.
- `data_market.ts:321` miss caught and fixed in CHANGES2 — good self-correction.
- `site_audit.ts:268`, `click_through.ts:30`, `probe_calbar.ts:14` — `try/finally` browser cleanup confirmed in source.
- `create_admin.ts:18` — env-var password lookup confirmed; CLI arg path removed.
- `.env.example` created with placeholder values only.

## Where codex got it wrong or missed (file:line)

**Critical — Stripe webhook is still broken.**
`src/server/index.ts:21` runs `app.use(express.json())` globally before any router. `upgrade.ts:245` adds `express.raw()` as route-level middleware on `/webhooks/stripe`, but by the time it runs the body is already parsed and consumed by the global JSON middleware. Express's body-parser only fires when `req.body` is still unset, so `express.raw()` at the route level is a no-op here. `stripe.webhooks.constructEvent` receives a JavaScript object, not raw bytes, and will throw a signature verification error on every real webhook. Codex's rereview simply listed this as "deferred" without flagging that the existing code is actively broken (not just missing a hardening), and CHANGES2 confirms it is still unaddressed.

Fix: In `src/server/index.ts`, mount the raw parser for the webhook path before `express.json()`:
```
app.use('/webhooks/stripe', express.raw({ type: 'application/json' }));
app.use(express.json());
```
Then remove the duplicate `express.raw()` from `upgrade.ts:245`.

**Still live — host-header redirect injection.**
`upgrade.ts:176-178` still reads `x-forwarded-host` directly to build Stripe `success_url`/`cancel_url`. An attacker can redirect post-payment to an arbitrary domain. No `PUBLIC_BASE_URL` env variable has been introduced. Codex CHANGES2 makes no mention of this having been addressed.

## Operational concerns codex missed
- The `.env` file with live Stripe keys and DB credentials is still present in the project tree (`CHANGES.md:17` confirms it was left untouched). At :9701 this is a running production process — the live secrets are one `cat` away from any shell user on the host.
- `upgrade.ts:25` adds `express.urlencoded` at the router level. Combined with the global `express.json()`, both parsers run on every upgrade route. Not harmful but redundant and worth noting during cleanup.

## Final action items (priorities)

1. **P0 — Fix Stripe webhook parser ordering** (`src/server/index.ts:21`, `src/server/upgrade.ts:245`). Webhooks are currently failing signature verification silently.
2. **P0 — Remove `.env` from project tree and rotate live keys.** Requires Steve.
3. **P0 — Replace forwarded-host base URL** (`upgrade.ts:176-178`) with `process.env.PUBLIC_BASE_URL`.
4. **P1 — Add login rate limiting** (`portal.ts:391`) — admin email is known; brute-force window is open.
5. **P1 — Add spam controls to public lead/inquiry endpoints** (`leads.ts:186`, `directory.ts:468`).