security — prototype-pollution + pin-spam DoS guards in src/review.js. Adds validId(raw) helper returning a positive-integer string or null; rejects 'proto', 'constructor', 'toString', '0', '01', '1abc', etc. with HTTP 400 before id is used as an object key (reviews/cache/moodboards). Adds sanitizePinSuggestion() whitelisting suggestion fields (type/hex/label/reason/thumb_url/design_id/icon) so '...spread' can't leak surprise keys. /api/moodboard/:id/pin: 16kb content-length pre-check (global json parser has 25mb limit, route-local check is too late), pin_key required-string ≤200 chars, 200-pins-per-design cap. /api/chip/:id/:chipKey: chipKey restricted to [a-z0-9_-] ≤80 chars, message ≤2000 chars, history capped to last 200 turns. +11 integration tests covering attack + valid paths.

0772bf0dff84dc82afe41edc9ef540ad7f368abe · 2026-05-12 06:44:53 -0700 · Steve Abrams
Files touched

M src/review.js
A tests/integration/review-id-validation.spec.js
Diff

commit 0772bf0dff84dc82afe41edc9ef540ad7f368abe
Author: Steve Abrams <steve@designerwallcoverings.com>
Date:   Tue May 12 06:44:53 2026 -0700

    security — prototype-pollution + pin-spam DoS guards in src/review.js. Adds validId(raw) helper returning a positive-integer string or null; rejects '__proto__', 'constructor', 'toString', '0', '01', '1abc', etc. with HTTP 400 before id is used as an object key (reviews/cache/moodboards). Adds sanitizePinSuggestion() whitelisting suggestion fields (type/hex/label/reason/thumb_url/design_id/icon) so '...spread' can't leak surprise keys. /api/moodboard/:id/pin: 16kb content-length pre-check (global json parser has 25mb limit, route-local check is too late), pin_key required-string ≤200 chars, 200-pins-per-design cap. /api/chip/:id/:chipKey: chipKey restricted to [a-z0-9_-] ≤80 chars, message ≤2000 chars, history capped to last 200 turns. +11 integration tests covering attack + valid paths.
---
 src/review.js                                  |  95 +++++++++++++++---
 tests/integration/review-id-validation.spec.js | 131 +++++++++++++++++++++++++
 2 files changed, 210 insertions(+), 16 deletions(-)

diff --git a/src/review.js b/src/review.js
index 5f88b0a..989fdc3 100644
--- a/src/review.js
+++ b/src/review.js
@@ -32,6 +32,32 @@ const OLLAMA_VISION_URL = process.env.OLLAMA_VISION_URL || 'http://localhost:114
 const OLLAMA = process.env.OLLAMA_URL || 'http://192.168.1.133:11434';
 const OLLAMA_MODEL = process.env.OLLAMA_REVIEW_MODEL || 'qwen3:14b';
 
+// ── input validation ────────────────────────────────────────────────
+// Designs are numeric IDs (see DESIGNS in server.js). req.params.id is used
+// as an object key (reviews[id], cache[id], moodboards[id]) so a literal
+// "__proto__" / "constructor" / "toString" would either pollute the prototype
+// or hit a non-data property. validId() returns the string form of a positive
+// integer, or null. Routes return 400 on null. (Security pickup-list item.)
+function validId(raw) {
+  const s = String(raw || '').trim();
+  if (!/^[0-9]{1,10}$/.test(s)) return null;
+  if (s === '0' || s.startsWith('0')) return null;       // no leading-zero ids
+  return s;
+}
+
+// Whitelist the suggestion shape so `...suggestion` can't spread surprise keys
+// into the stored moodboard record. Only the fields the UI actually renders.
+function sanitizePinSuggestion(s) {
+  if (!s || typeof s !== 'object') return {};
+  const out = {};
+  for (const k of ['type', 'hex', 'label', 'reason', 'thumb_url', 'design_id', 'icon']) {
+    if (s[k] == null) continue;
+    if (typeof s[k] === 'string') out[k] = s[k].slice(0, 600);
+    else if (typeof s[k] === 'number' && isFinite(s[k])) out[k] = s[k];
+  }
+  return out;
+}
+
 // ── persistence helpers ──────────────────────────────────────────────
 function loadJSON(file, fallback) {
   try { return JSON.parse(fs.readFileSync(file, 'utf8')); } catch { return fallback; }
@@ -324,12 +350,15 @@ function mount(app, getDesigns) {
   if (!fs.existsSync(VISION_FILE)) saveJSON(VISION_FILE, {});
 
   app.get('/api/review/:id', requireAdmin, (req, res) => {
+    const id = validId(req.params.id);
+    if (!id) return res.status(400).json({ error: 'invalid id' });
     const reviews = loadJSON(REVIEWS_FILE, {});
-    res.json(reviews[req.params.id] || null);
+    res.json(reviews[id] || null);
   });
 
   app.post('/api/review/:id', requireAdmin, async (req, res) => {
-    const id = req.params.id;
+    const id = validId(req.params.id);
+    if (!id) return res.status(400).json({ error: 'invalid id' });
     const designs = getDesigns();
     const design = designs.find(d => String(d.id) === String(id));
     if (!design) return res.status(404).json({ error: 'design not found' });
@@ -355,26 +384,38 @@ function mount(app, getDesigns) {
   });
 
   app.get('/api/chips/:id', requireAdmin, (req, res) => {
+    const id = validId(req.params.id);
+    if (!id) return res.status(400).json({ error: 'invalid id' });
     const designs = getDesigns();
-    const design = designs.find(d => String(d.id) === String(req.params.id));
+    const design = designs.find(d => String(d.id) === String(id));
     if (!design) return res.status(404).json({ error: 'design not found' });
     res.json({ design_id: design.id, chips: generateChips(design) });
   });
 
   app.get('/api/chip/:id/:chipKey/chat', requireAdmin, (req, res) => {
+    const id = validId(req.params.id);
+    if (!id) return res.status(400).json({ error: 'invalid id' });
+    // chipKey is user-controlled but only used in a composite string key, not as a
+    // standalone object key. Restrict to safe chars (no __proto__, no separators).
+    const chipKey = String(req.params.chipKey || '').replace(/[^a-z0-9_-]/gi, '').slice(0, 80);
+    if (!chipKey) return res.status(400).json({ error: 'invalid chip key' });
     const chats = loadJSON(CHATS_FILE, {});
-    const key = `${req.params.id}:${req.params.chipKey}`;
+    const key = `${id}:${chipKey}`;
     res.json({ key, history: chats[key] || [] });
   });
 
   app.post('/api/chip/:id/:chipKey/chat', requireAdmin, async (req, res) => {
-    const id = req.params.id, chipKey = req.params.chipKey;
+    const id = validId(req.params.id);
+    if (!id) return res.status(400).json({ error: 'invalid id' });
+    const chipKey = String(req.params.chipKey || '').replace(/[^a-z0-9_-]/gi, '').slice(0, 80);
+    if (!chipKey) return res.status(400).json({ error: 'invalid chip key' });
     const designs = getDesigns();
     const design = designs.find(d => String(d.id) === String(id));
     if (!design) return res.status(404).json({ error: 'design not found' });
     const chip = generateChips(design).find(c => c.key === chipKey);
     if (!chip) return res.status(404).json({ error: 'chip not found' });
-    const message = (req.body && req.body.message || '').trim();
+    // Cap chat message length — 2k chars is plenty for a chip Q&A; longer is abuse
+    const message = String(req.body && req.body.message || '').trim().slice(0, 2000);
     if (!message) return res.status(400).json({ error: 'message required' });
 
     const chats = loadJSON(CHATS_FILE, {});
@@ -394,9 +435,10 @@ function mount(app, getDesigns) {
 
     history.push({ role: 'user',      content: message, ts: new Date().toISOString() });
     history.push({ role: 'assistant', content: reply,    ts: new Date().toISOString() });
-    chats[key] = history;
+    // Cap history to last 200 turns per chip-chat — prevents unbounded growth
+    chats[key] = history.slice(-200);
     saveJSON(CHATS_FILE, chats);
-    res.json({ reply, history });
+    res.json({ reply, history: chats[key] });
   });
 
   app.get('/api/reviews/all', requireAdmin, (req, res) => {
@@ -405,7 +447,8 @@ function mount(app, getDesigns) {
 
   // GET cached pairings — generate-on-miss
   app.get('/api/pairings/:id', requireAdmin, async (req, res) => {
-    const id = req.params.id;
+    const id = validId(req.params.id);
+    if (!id) return res.status(400).json({ error: 'invalid id' });
     const designs = getDesigns();
     const design = designs.find(d => String(d.id) === String(id));
     if (!design) return res.status(404).json({ error: 'design not found' });
@@ -419,7 +462,8 @@ function mount(app, getDesigns) {
 
   // POST refresh — invalidate cache (both pairings + vision)
   app.post('/api/pairings/:id/refresh', requireAdmin, async (req, res) => {
-    const id = req.params.id;
+    const id = validId(req.params.id);
+    if (!id) return res.status(400).json({ error: 'invalid id' });
     const designs = getDesigns();
     const design = designs.find(d => String(d.id) === String(id));
     if (!design) return res.status(404).json({ error: 'design not found' });
@@ -433,18 +477,35 @@ function mount(app, getDesigns) {
   });
 
   // Moodboard: pin/unpin a suggestion.
-  app.post('/api/moodboard/:id/pin', requireAdmin, (req, res) => {
-    const id = req.params.id;
+  // Content-Length pre-check (16kb cap) — the global express.json parser has a
+  // 25mb limit that already ran by the time we get here, so a route-local
+  // json() is too late. Reject the raw request size before doing any parsing.
+  // A pin is just pin_key + a small whitelisted suggestion blob; legit clients
+  // never hit 16kb.
+  app.post('/api/moodboard/:id/pin', requireAdmin, (req, res, next) => {
+    const cl = parseInt(req.headers['content-length'] || '0', 10);
+    if (cl > 16 * 1024) return res.status(413).json({ error: 'body too large (>16kb)' });
+    next();
+  }, (req, res) => {
+    const id = validId(req.params.id);
+    if (!id) return res.status(400).json({ error: 'invalid id' });
     const { pin_key, suggestion } = req.body || {};
-    if (!pin_key) return res.status(400).json({ error: 'pin_key required' });
+    if (typeof pin_key !== 'string' || !pin_key || pin_key.length > 200) {
+      return res.status(400).json({ error: 'pin_key required (string, ≤200 chars)' });
+    }
     const moodboards = loadJSON(MOODBOARD_FILE, {});
     moodboards[id] = moodboards[id] || [];
+    // Cap pins per design — a curator-tool, not a logging system
+    if (moodboards[id].length >= 200 && !moodboards[id].some(p => p.pin_key === pin_key)) {
+      return res.status(400).json({ error: 'pin cap reached (200 per design)' });
+    }
+    const safeSuggestion = sanitizePinSuggestion(suggestion);
     if (moodboards[id].some(p => p.pin_key === pin_key)) {
       // unpin (toggle)
       moodboards[id] = moodboards[id].filter(p => p.pin_key !== pin_key);
     } else {
-      // pin — store the whole suggestion
-      moodboards[id].push({ pin_key, ...suggestion, pinned_at: new Date().toISOString() });
+      // pin — store the whitelisted suggestion
+      moodboards[id].push({ pin_key, ...safeSuggestion, pinned_at: new Date().toISOString() });
     }
     saveJSON(MOODBOARD_FILE, moodboards);
     // sync the pairings cache `pinned` flag so UI is consistent
@@ -459,8 +520,10 @@ function mount(app, getDesigns) {
 
   // GET moodboard for one design
   app.get('/api/moodboard/:id', requireAdmin, (req, res) => {
+    const id = validId(req.params.id);
+    if (!id) return res.status(400).json({ error: 'invalid id' });
     const moodboards = loadJSON(MOODBOARD_FILE, {});
-    res.json({ design_id: req.params.id, items: moodboards[req.params.id] || [] });
+    res.json({ design_id: id, items: moodboards[id] || [] });
   });
 
   // GET all moodboards (admin)
diff --git a/tests/integration/review-id-validation.spec.js b/tests/integration/review-id-validation.spec.js
new file mode 100644
index 0000000..6c6a456
--- /dev/null
+++ b/tests/integration/review-id-validation.spec.js
@@ -0,0 +1,131 @@
+'use strict';
+// Regression tests for the prototype-pollution + pin-spam-DoS fix in src/review.js
+// (2026-05-12 security pickup).
+//
+// What we're guarding against:
+// 1. req.params.id was used as an object key — `__proto__`, `constructor`, `toString`
+//    would either pollute Object.prototype or hit a non-data property. validId()
+//    now requires positive-integer ids and routes return 400 on anything else.
+// 2. /api/moodboard/:id/pin inherited the global 25mb json limit — was a DoS vector.
+//    Local limit is now 16kb + per-design pin cap (200).
+// 3. pin_key wasn't validated as a string; suggestion was spread without sanitization.
+//    Now pin_key is required-string ≤200 chars, suggestion is whitelisted to known keys.
+
+const { test, describe, before } = require('node:test');
+const assert = require('node:assert/strict');
+const http = require('node:http');
+
+const BASE = 'http://127.0.0.1:9792';
+
+function req(method, urlPath, body) {
+  return new Promise((resolve, reject) => {
+    const u = new URL(BASE + urlPath);
+    const data = body ? Buffer.from(JSON.stringify(body)) : null;
+    const r = http.request({
+      hostname: u.hostname, port: u.port, path: u.pathname + u.search, method,
+      headers: data ? { 'Content-Type': 'application/json', 'Content-Length': data.length } : {},
+    }, res => {
+      let chunks = [];
+      res.on('data', c => chunks.push(c));
+      res.on('end', () => {
+        const text = Buffer.concat(chunks).toString('utf8');
+        let json = null;
+        try { json = JSON.parse(text); } catch {}
+        resolve({ status: res.statusCode, json, text });
+      });
+    });
+    r.on('error', reject);
+    if (data) r.write(data);
+    r.end();
+  });
+}
+
+let serverUp = false;
+before(async () => {
+  try {
+    const r = await req('GET', '/');
+    serverUp = r.status === 200;
+  } catch { serverUp = false; }
+});
+
+describe('prototype-pollution-resistant id validation', () => {
+  test('GET /api/review/__proto__ → 400', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    const r = await req('GET', '/api/review/__proto__');
+    assert.equal(r.status, 400, 'invalid id rejected');
+    assert.equal(r.json && r.json.error, 'invalid id');
+  });
+
+  test('GET /api/review/constructor → 400', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    const r = await req('GET', '/api/review/constructor');
+    assert.equal(r.status, 400);
+  });
+
+  test('GET /api/review/0 → 400 (no leading-zero or zero)', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    const r = await req('GET', '/api/review/0');
+    assert.equal(r.status, 400);
+  });
+
+  test('GET /api/review/1abc → 400 (non-digit)', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    const r = await req('GET', '/api/review/1abc');
+    assert.equal(r.status, 400);
+  });
+
+  test('GET /api/review/1 → 200 (valid)', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    const r = await req('GET', '/api/review/1');
+    assert.equal(r.status, 200);
+  });
+
+  test('Object.prototype is not polluted after attack attempts', () => {
+    // Attempt to pollute the in-memory object would not have shown up via the
+    // HTTP layer alone (the route doesn't reflect the value back), so we just
+    // sanity-check that our test runner's Object.prototype is intact.
+    const probe = {};
+    assert.equal(probe.polluted, undefined, 'no global prototype pollution');
+  });
+});
+
+describe('moodboard pin — DoS + injection guards', () => {
+  test('POST /api/moodboard/__proto__/pin → 400', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    const r = await req('POST', '/api/moodboard/__proto__/pin', { pin_key: 'x', suggestion: {} });
+    assert.equal(r.status, 400);
+  });
+
+  test('POST with non-string pin_key → 400', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    const r = await req('POST', '/api/moodboard/1/pin', { pin_key: { evil: true }, suggestion: {} });
+    assert.equal(r.status, 400);
+  });
+
+  test('POST with oversized body → 413 (16kb limit)', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    const huge = 'A'.repeat(20 * 1024); // 20kb of A's
+    const r = await req('POST', '/api/moodboard/1/pin', { pin_key: 'x', suggestion: { reason: huge } });
+    assert.ok(r.status === 413 || r.status === 400, `expected 413/400 for >16kb body, got ${r.status}`);
+  });
+
+  test('POST with pin_key > 200 chars → 400', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    const r = await req('POST', '/api/moodboard/1/pin', { pin_key: 'x'.repeat(250), suggestion: {} });
+    assert.equal(r.status, 400);
+  });
+
+  test('POST with suggestion.__proto__ keys → spread is sanitized', async (t) => {
+    if (!serverUp) { t.skip('server down'); return; }
+    // Even if the server accepts the request, the whitelist drops __proto__
+    // because it's not in the allowed field list. The Object.prototype check
+    // below would catch any leak.
+    const r = await req('POST', '/api/moodboard/99999/pin', {
+      pin_key: 'sanitize-test-' + Date.now(),
+      suggestion: { __proto__: { polluted: 'YES' }, type: 'color', hex: '#abcdef', label: 'ok' },
+    });
+    assert.ok(r.status === 200 || r.status === 404, `valid input should not 400, got ${r.status}`);
+    const probe = {};
+    assert.equal(probe.polluted, undefined, 'no prototype pollution');
+  });
+});
← 486986e wallco.ai · gzip compression middleware — 86% bandwidth redu · back to Wallco Ai · wallco.ai · /designs JSON-LD — ItemList of Products + Breadc 1d2bb76 →