`site-factory/claude-review.md`

31 lines
# Claude review of codex patches — site-factory

## Verdict
MIXED — the admin/server.js fixes are solid, but the webhook fix is cosmetic: all three live sites are still running the unguarded stub from stale .next artifacts.

## Where codex got it right
- `admin/server.js:17-19` — fail-fast on missing/weak SESSION_SECRET is correct and clean
- `admin/server.js:139` — binding to 127.0.0.1 is a real improvement
- `ecosystem.config.js:50` — reading SESSION_SECRET from `admin/.session-secret` (97-byte file, mode 0600) is the right pattern; file exists and is properly sized
- Rereview correctly identified that `.next` artifacts are stale and that `next start` serves the compiled bundle, not the TypeScript source
- Rereview correctly listed all three compiled artifacts as the live attack surface

## Where codex got it wrong or missed
- The rereview calls this "FIX BEFORE SHIP" but CHANGES2.md defers all three sites as "forbidden" and ships nothing — the net result is zero remediation on the highest-risk webhook issue; the stale artifacts ARE the production code
- All three compiled bundles still contain `if(!s||!r)return u.NextResponse.json({received:!0,stub:!0})` with NO `NODE_ENV` check: `sites/wholivedthere.com/app/.next/server/app/api/webhook/stripe/route.js:1`, `sites/bubbesblock.com/app/.next/server/app/api/webhook/stripe/route.js:1`, `sites/claimmyaddress.com/app/.next/server/app/api/webhook/stripe/route.js:1` — all three are running in `NODE_ENV=production` (ecosystem.config.js:62,73,84) and will silently accept unsigned Stripe events right now
- Codex deferred the fix as requiring a `next build` and framed it as "forbidden" — a `next build` is a routine operational step, not a code change; this is under-effort framing
- `ecosystem.config.js:49` still sets `SF_ADMIN_DEV: '1'` in production — the original P0 — codex deferred this entirely. Every request to sf-admin bypasses OAuth, defeating the SESSION_SECRET fix above
- Codex's rereview noted "no new issues introduced" but missed that `ecosystem.config.js:50` reads from `admin/.session-secret` at pm2 config parse time; if that file is absent on a fresh clone/deploy, pm2 will refuse to start all apps in the ecosystem, not just sf-admin

## Operational concerns codex missed
- Steve already injected SESSION_SECRET into the pm2 env — but `ecosystem.config.js` still reads from the file at parse time. Any `pm2 reload ecosystem.config.js` on a machine without `admin/.session-secret` will crash pm2 startup for all 7 processes
- `admin/.session-secret` is not in `.gitignore` (no `.gitignore` exists per original review); if this repo is ever pushed, the session signing secret ships with it
- The three Next.js site pm2 processes (ports 3001-3003) have not restarted since the source patches were applied, so currently running code is the pre-patch build in all three cases — this is the actual P0 in production right now
- `SF_ADMIN_DEV: '1'` at ecosystem.config.js:49 means sf-admin on :9883 auto-authenticates every request as steve@designerwallcoverings.com — the 127.0.0.1 bind helps, but any process on localhost can hit every admin write endpoint unauthenticated

## Final action items (priorities)
- P0: Run `next build` in each of the three site directories, then `pm2 restart site-wholivedthere site-bubbesblock site-claimmyaddress` — this is the only action that actually closes the unsigned-webhook gap; all three compiled bundles are currently vulnerable
- P0: Set `SF_ADMIN_DEV: '0'` in `ecosystem.config.js:49` and set `GOOGLE_CLIENT_ID`/`GOOGLE_CLIENT_SECRET` in the sf-admin env, then `pm2 restart sf-admin` — the SESSION_SECRET fix is meaningless while DEV_BYPASS is active
- P1: Add `admin/.session-secret` to `.gitignore` (create the file; none exists); also add `.env`, `.env.local`, `.next/`, `node_modules/`, `logs/`
- P1: Move the `readFileSync` call in `ecosystem.config.js:50` to a try/catch or guard with `fs.existsSync` so a missing file doesn't crash the entire pm2 ecosystem on startup