`REVIEW-2026-05-04.md`

75 lines
# site-factory — overnight debate-team review (2026-05-04)

Code-reviewer + architect-reviewer parallel run. **8 patches applied** (3 P0 + 4 P1 + 1 .gitignore creation). **1 P0 needs Steve's hands** (.env was committed in `[overnight] pre-debate baseline` today — needs rotation).

## P0 patches APPLIED

| # | File:line | Issue | Fix |
|---|---|---|---|
| 1 | `ecosystem.config.js:49` | **`SF_ADMIN_DEV: '1'` hardcoded ON in production** — bypasses ALL Google OAuth, stubs in fake admin user. Any process reaching :9883 = admin. | Removed env var from prod manifest; left comment explaining `ecosystem.local.js` for dev |
| 2 | `admin/server.js:18` | (defense-in-depth for #1) DEV_BYPASS could still be set elsewhere | Added boot guard: `throw new Error()` if `DEV_BYPASS && NODE_ENV === 'production'` |
| 3 | `orchestrator/publish.js:8` | **Path traversal**: `domain` from DB → `path.join(SITES_ROOT, domain, 'app')` with no validation. Poisoned row `domain = '../../../.ssh'` would write anywhere on disk | Added `DOMAIN_RE` validation + `path.resolve()` boundary check |

## P1 patches APPLIED

| # | File:line | Issue | Fix |
|---|---|---|---|
| 4 | `orchestrator/server.js:37` | `POST /sites` accepted any string as `domain` (downstream path-traversal vector — see #3) | Added `DOMAIN_RE` validation before INSERT |
| 5 | `orchestrator/server.js:216` | `POST /sites/:domain/run-stage/:n` no bounds check on `n` (NaN/negative reached `runners[NaN]`, leaked stack traces via 500) | `Number.isInteger(n) && 0..10` |
| 6 | `orchestrator/server.js:245` | sf-orchestrator was binding 0.0.0.0 — combined with wildcard CORS + no auth on POST routes, made all pipeline mutation reachable from any local process (including the 3 generated Next.js sites) | Bound to `127.0.0.1` (override via `SF_ORCH_BIND` env) |
| 7 | `critic/server.js:40` | `paths[]` from request body passed straight to check modules — attacker could request `paths: ['/Users/stevestudio2']` for full-disk scan + secret exfil via findings | Validate every path is under `SITES_ROOT` before passing to check ctx |
| 8 | `critic/server.js:169` | sf-critic also binding 0.0.0.0 (compounds #7) | Bound to `127.0.0.1` (override via `SF_CRITIC_BIND` env) |
| 9 | `admin/server.js:27` | Session cookie missing `secure: true` — would transmit over HTTP if nginx proxies w/ TLS termination | `secure: process.env.NODE_ENV === 'production'` |

## NEW: `.gitignore` created

Repo had no `.gitignore` and `.env` was actively tracked. New `.gitignore` covers `.env*` (with `!.env.example` allow), `*.session-secret`, `node_modules/`, `logs/`, `.uptime-state/`, `dist/`, `.next/`, `ecosystem.local.js`, `.DS_Store`.

`node --check` passes on all 4 modified files.

## P0 NEEDS STEVE — `.env` already in git history (4 actions)

Today's commit `5b2b1e1 [overnight] pre-debate baseline` **committed `.env` (2128 bytes) into git history**. The `.gitignore` I just added prevents future commits but does NOT remove what's already there.

Required actions in order:
1. `cd ~/Projects/site-factory && git rm --cached .env` — stops tracking but keeps the local file
2. `git commit -m "untrack .env (was committed accidentally)"`
3. **Rotate every key that was in `.env`** — they're in git history forever (even after rm-cached). Likely candidates: any DB passwords, OAuth client secrets, API keys. Use `/secrets` skill to mint replacements.
4. Optionally: `git filter-repo` or BFG to scrub from history (slow + rewrites refs — only if repo was pushed somewhere public).

## P2 deferred (in REVIEW)

- `orchestrator/publish.js:25` — `paletteName` interpolated into TS file content without escape (low risk: palettes are seeded, not user input).
- `stages/runners.js:224` — `Content-Length: data.length` should be `Buffer.byteLength(data)` (would truncate body for non-ASCII domain names; line 39 of same file does this correctly).
- `stages/runners.js:33` — `postJSON` swallows connection errors with `req.on('error', () => resolve())` — at minimum log them.
- `orchestrator/server.js:227` — WebSocket has no `verifyClient` origin check; broadcasts pipeline events to anyone who can connect.
- No helmet on any of 4 servers — `npm install helmet` (touches package.json).

## Architecture roadmap (deferred)

**Architectural Impact: MEDIUM-HIGH.** Core pipeline shape is sound but: 4-service split is over-fanned at one node (sf-viewer = 12 LOC `express.static`), under-isolated at another (orchestrator + stages share state via circular import).

### R1 — Extract `factory-core/` shared package
3 sibling factories (site-factory, the-ai-factory, visual-factory) reimplement: pg pool, `/health`, WS broadcast, action/finding REST routes, stage state machine. Lift to `~/Projects/factory-core/`: `core/db.js`, `core/pipeline.js`, `core/critic-client.js`, `core/express-bootstrap.js`. ~400 LOC dedup. Same theme as the directory-core extraction recommended in lawyer/doctor/animals reviews.

### R2 — Break `stages/runners.js ↔ orchestrator/db.js` circular import
`stages/runners.js:5` does `require('../orchestrator/db')` while `orchestrator/server.js:193` requires `../stages/runners`. Move pool to `db/pool.js`, both packages depend on it. Lets stages run in a worker without booting the HTTP server.

### R3 — Single source of truth for site→port routing (`db/sites-registry.json`)
The "all 3 sites served pastdoor's HTML" incident (per MEMORY) was caused by inline-heredoc nginx vhosts in `scripts/ship-to-kamatera.sh:108` with no central manifest. Schema: `[{ domain, port, app_dir, pm2_name }]`. Generate ecosystem entries + nginx vhosts from this file. Add `scripts/verify-routing.sh` that curls each domain and asserts headers match expected app — the missing regression test for that incident.

### R4 — Fold sf-viewer into sf-admin
`viewer/server.js` is 12 LOC of `express.static`. Doesn't need its own pm2 slot/port/log/watchdog. Mount as `app.use('/floor', express.static(viewerDir))` in admin (or orchestrator).

### R5 — Move generated sites to a sibling pm2 ecosystem
Create `~/Projects/site-factory-sites/ecosystem.config.js` with only `site-*` entries. A factory `pm2 reload all` shouldn't bounce live customer traffic. Makes the "factory builds it → handoff" boundary explicit.

## Files touched

- `/Users/stevestudio2/Projects/site-factory/.gitignore` (NEW)
- `/Users/stevestudio2/Projects/site-factory/ecosystem.config.js`
- `/Users/stevestudio2/Projects/site-factory/admin/server.js`
- `/Users/stevestudio2/Projects/site-factory/orchestrator/publish.js`
- `/Users/stevestudio2/Projects/site-factory/orchestrator/server.js`
- `/Users/stevestudio2/Projects/site-factory/critic/server.js`