[object Object]

← back to Visual Factory

fix(ssrf): IPv6 reserved-range blocklist now catches in-range forms (codex P2)

ee44a369aa864e1d553f5c944f2552496f142786 · 2026-05-01 18:50:22 -0700 · Steve

The textual prefix checks at server.js:190-194 missed valid in-range IPv6
forms because they only matched specific shapes:
  - 100::1 sits inside 100::/64 but failed /^100:0?:0?:0?:/
  - fed0::1 and fee0::1 sit inside fec0::/10 but didn't match /^fec/
  - any compressed (::) form drifted from the prefix regex

Replaced with proper expand-then-bit-prefix-match:
  _expandIPv6(addr) — normalize to 8-group, 4-hex-char-each canonical form
  _ipv6InRange(addr, prefix, bits) — CIDR mask comparison

7 RFC reserved ranges checked: fe80::/10, fc00::/7, 100::/64, 64:ff9b::/96,
2001:db8::/32, fec0::/10, ff00::/8.

Verified with node test: 100::1, fed0::1, fee0::1, 2001:db8::1 all now
return true; IPv4 + hostnames return null (untouched).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Files touched

Diff

commit ee44a369aa864e1d553f5c944f2552496f142786
Author: Steve <steve@designerwallcoverings.com>
Date:   Fri May 1 18:50:22 2026 -0700

    fix(ssrf): IPv6 reserved-range blocklist now catches in-range forms (codex P2)
    
    The textual prefix checks at server.js:190-194 missed valid in-range IPv6
    forms because they only matched specific shapes:
      - 100::1 sits inside 100::/64 but failed /^100:0?:0?:0?:/
      - fed0::1 and fee0::1 sit inside fec0::/10 but didn't match /^fec/
      - any compressed (::) form drifted from the prefix regex
    
    Replaced with proper expand-then-bit-prefix-match:
      _expandIPv6(addr) — normalize to 8-group, 4-hex-char-each canonical form
      _ipv6InRange(addr, prefix, bits) — CIDR mask comparison
    
    7 RFC reserved ranges checked: fe80::/10, fc00::/7, 100::/64, 64:ff9b::/96,
    2001:db8::/32, fec0::/10, ff00::/8.
    
    Verified with node test: 100::1, fed0::1, fee0::1, 2001:db8::1 all now
    return true; IPv4 + hostnames return null (untouched).
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 server.js | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 121 insertions(+), 14 deletions(-)

diff --git a/server.js b/server.js
index cc9ba7e..98c0bb9 100644
--- a/server.js
+++ b/server.js
@@ -6,6 +6,7 @@ const { constants: fsConstants } = require('node:fs');
 const path = require('node:path');
 const os = require('node:os');
 const { spawn } = require('node:child_process');
+const crypto = require('node:crypto');
 const express = require('express');
 const { Pool } = require('pg');
 
@@ -141,29 +142,106 @@ function _normalizeHost(rawHost) {
 function _isBlockedIPv4(addr) {
   const v4 = addr.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/);
   if (!v4) return false;
-  const [, a, b] = v4.map(Number);
+  const [, a, b, c] = v4.map(Number);
   if (a === 127 || a === 10 || a === 0) return true;
   if (a === 169 && b === 254) return true;             // link-local + AWS metadata
   if (a === 172 && b >= 16 && b <= 31) return true;
   if (a === 192 && b === 168) return true;
   if (a === 100 && b >= 64 && b <= 127) return true;   // CGNAT
+  // Audit P2: reserved/special-purpose ranges that shouldn't be reachable
+  // from a public-image fetcher. Cheap to block; prevents probing of internal
+  // protocol assignments and benchmarking infra.
+  if (a === 192 && b === 0 && c === 0) return true;    // 192.0.0/24 IETF protocol assignments (incl. 192.0.0.170 NAT64 disco)
+  if (a === 192 && b === 0 && c === 2) return true;    // 192.0.2/24 TEST-NET-1
+  if (a === 198 && b === 51 && c === 100) return true; // 198.51.100/24 TEST-NET-2
+  if (a === 203 && b === 0 && c === 113) return true;  // 203.0.113/24 TEST-NET-3
+  if (a === 198 && (b === 18 || b === 19)) return true; // 198.18/15 RFC2544 benchmark
+  if (a >= 224 && a <= 239) return true;               // 224/4 multicast
+  if (a >= 240) return true;                           // 240/4 reserved (incl. 255.255.255.255 broadcast)
   return false;
 }
+
+// Expand fully-zero-compressed IPv4-mapped to dotted-quad. Handles forms
+// like `0:0:0:0:0:ffff:7f00:1` that `dns.resolve6` may emit instead of the
+// `::ffff:127.0.0.1` shorthand the original regex assumed.
+function _expandV4Mapped(a) {
+  // Already in dotted form: ::ffff:1.2.3.4
+  let m = a.match(/^(?:0:){5}ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/);
+  if (m) return m[1];
+  m = a.match(/^::ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/);
+  if (m) return m[1];
+  // Hex pair form: ::ffff:7f00:1  or  0:0:0:0:0:ffff:7f00:1
+  m = a.match(/^(?:0:){5}ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/) ||
+      a.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/);
+  if (m) {
+    const hi = parseInt(m[1], 16), lo = parseInt(m[2], 16);
+    return `${(hi >> 8) & 0xff}.${hi & 0xff}.${(lo >> 8) & 0xff}.${lo & 0xff}`;
+  }
+  return null;
+}
+
+// Expand any IPv6 string (including compressed `::` and `100::1` shorthand)
+// to a full 8-group form so prefix matching is reliable. Returns null on
+// invalid input. Codex 2026-05-01 P2: textual prefix checks miss valid
+// in-range forms like `100::1` (in 100::/64) and `fed0::1` (in fec0::/10).
+function _expandIPv6(addr) {
+  if (!addr || typeof addr !== 'string') return null;
+  const a = addr.toLowerCase().split('%')[0]; // strip zone id if any
+  if (!a.includes(':')) return null;
+  const [head, tail = ''] = a.split('::');
+  const headParts = head ? head.split(':') : [];
+  const tailParts = tail ? tail.split(':') : [];
+  const fill = 8 - headParts.length - tailParts.length;
+  if (fill < 0) return null;
+  const groups = [...headParts, ...Array(fill).fill('0'), ...tailParts];
+  if (groups.length !== 8) return null;
+  // Pad each group to 4 hex chars so leading-zero forms compare correctly.
+  return groups.map(g => g.padStart(4, '0')).join(':');
+}
+
+// Numeric-prefix check: does `addr` fall within `prefix/bits`? Both args are
+// expanded IPv6 strings (16 colon-separated 4-hex-char groups). Bits is the
+// CIDR mask length. Returns false on parse error (caller should treat as
+// untrusted and reject elsewhere).
+function _ipv6InRange(addr, prefix, bits) {
+  const a = addr.replace(/:/g, '');
+  const p = prefix.replace(/:/g, '');
+  if (a.length !== 32 || p.length !== 32) return false;
+  // Compare bit-by-bit up to `bits` bits.
+  const fullNibbles = Math.floor(bits / 4);
+  if (a.slice(0, fullNibbles) !== p.slice(0, fullNibbles)) return false;
+  const remainder = bits % 4;
+  if (remainder === 0) return true;
+  const aN = parseInt(a[fullNibbles], 16);
+  const pN = parseInt(p[fullNibbles], 16);
+  const mask = 0xf << (4 - remainder);
+  return (aN & mask) === (pN & mask);
+}
+
 function _isBlockedIPv6(addr) {
   const a = addr.toLowerCase();
   if (a === '::1' || a === '::') return true;
-  if (a.startsWith('fe80:') || a.startsWith('fe80::')) return true;
-  // ULA: fc00::/7
-  if (/^f[cd][0-9a-f]{2}:/.test(a)) return true;
-  // IPv4-mapped (::ffff:a.b.c.d  or  ::ffff:7f00:1)
-  const mappedDot = a.match(/^::ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/);
-  if (mappedDot && _isBlockedIPv4(mappedDot[1])) return true;
-  const mappedHex = a.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/);
-  if (mappedHex) {
-    const hi = parseInt(mappedHex[1], 16), lo = parseInt(mappedHex[2], 16);
-    const v4 = `${(hi >> 8) & 0xff}.${hi & 0xff}.${(lo >> 8) & 0xff}.${lo & 0xff}`;
-    if (_isBlockedIPv4(v4)) return true;
+  // Expand once for all numeric range checks below — handles `100::1`,
+  // `fed0::1`, `fee0::1`, etc. that the old textual prefix regex missed.
+  const expanded = _expandIPv6(a);
+  if (expanded) {
+    // Reserved blocks specified by CIDR. Each entry: [prefix-expanded, bits, label]
+    const RANGES = [
+      ['fe80:0000:0000:0000:0000:0000:0000:0000', 10, 'fe80::/10 link-local'],
+      ['fc00:0000:0000:0000:0000:0000:0000:0000',  7, 'fc00::/7 ULA'],
+      ['0100:0000:0000:0000:0000:0000:0000:0000', 64, '100::/64 RFC6666 discard'],
+      ['0064:ff9b:0000:0000:0000:0000:0000:0000', 96, '64:ff9b::/96 NAT64'],
+      ['2001:0db8:0000:0000:0000:0000:0000:0000', 32, '2001:db8::/32 documentation'],
+      ['fec0:0000:0000:0000:0000:0000:0000:0000', 10, 'fec0::/10 deprecated site-local'],
+      ['ff00:0000:0000:0000:0000:0000:0000:0000',  8, 'ff00::/8 multicast'],
+    ];
+    for (const [prefix, bits] of RANGES) {
+      if (_ipv6InRange(expanded, prefix, bits)) return true;
+    }
   }
+  // IPv4-mapped — handle every canonical and expanded form.
+  const v4 = _expandV4Mapped(a);
+  if (v4 && _isBlockedIPv4(v4)) return true;
   return false;
 }
 function isAllowedExternalUrl(input) {
@@ -172,6 +250,10 @@ function isAllowedExternalUrl(input) {
   if (!/^https?:$/.test(u.protocol)) return false;
   // userinfo is a confusion vector — `http://user@127.0.0.1/` etc. — disallow.
   if (u.username || u.password) return false;
+  // Audit P3: reject IPv6 zone IDs (`http://[fe80::1%eth0]/` style). The fe80
+  // prefix would already trip _isBlockedIPv6, but other v6 with zones could
+  // slip through and confuse http.request. Cheap and total.
+  if (u.hostname.includes('%')) return false;
   const host = _normalizeHost(u.hostname);
   if (!host) return false;
   if (host === 'localhost') return false;
@@ -849,8 +931,33 @@ app.post('/runs/:id/activate', async (req, res) => {
       await fs.copyFile(srcPng, tmpPng);
       if (tmpHtml) await fs.copyFile(srcHtml, tmpHtml);
       if (overwrite) {
-        await fs.rename(tmpPng, destPng); pngCommitted = true;
-        if (tmpHtml) { await fs.rename(tmpHtml, destHtml); htmlCommitted = true; }
+        // Cycle-3 Claude review P2: surface EXDEV/ENOTSUP on the overwrite
+        // path too (not just the link path). Without this, cross-device
+        // PUBLISH_ROOT fails with an opaque 500 instead of the helpful hint.
+        try { await fs.rename(tmpPng, destPng); pngCommitted = true; }
+        catch (e) {
+          if (e.code === 'EXDEV' || e.code === 'ENOTSUP') {
+            await fs.rm(tmpPng, { force: true }).catch(() => {});
+            if (tmpHtml) await fs.rm(tmpHtml, { force: true }).catch(() => {});
+            return res.status(500).json({
+              error: `cross-device rename not supported (${e.code})`,
+              hint: 'PUBLISH_ROOT and output/ are on different filesystems. Set PUBLISH_ROOT to a path on the same device.',
+            });
+          }
+          throw e;
+        }
+        if (tmpHtml) {
+          try { await fs.rename(tmpHtml, destHtml); htmlCommitted = true; }
+          catch (e) {
+            if (e.code === 'EXDEV' || e.code === 'ENOTSUP') {
+              // PNG already committed; tolerate HTML failure with partial
+              // status (matches existing partial-success pattern below).
+              await fs.rm(tmpHtml, { force: true }).catch(() => {});
+              throw new Error(`HTML rename cross-device (${e.code}); PNG committed`);
+            }
+            throw e;
+          }
+        }
       } else {
         // Atomic claim. If two activations race on the same artifact_name,
         // exactly one wins; the loser sees EEXIST and 409s.

← 19c935e prefetch: agent:false to force fresh socket per request  ·  back to Visual Factory  ·  tighten .gitignore: add missing standing-rule patterns (tmp/ 407e25d →