protect: input-handling robustness pass (canonicalization + edge cases)

.github/workflows/ci.yml

@@ -26,6 +26,7 @@ jobs:
           - 18.x
           - 20.x
           - 22.x
+          - 24.x
 
     steps:
       - name: Checkout

src/protect/engine/fetch.js

@@ -42,25 +42,9 @@ export async function fromFetchRequest(request, options = {}) {
   let body = {};
   let files;
   if (rawBody) {
-    if (contentType.includes('application/json')) {
-      try {
-        body = JSON.parse(rawBody);
-      } catch {
-        body = {};
-      }
-    } else if (contentType.includes('application/x-www-form-urlencoded')) {
-      body = {};
-      for (const [k, v] of new URLSearchParams(rawBody)) {
-        body[k] = k in body ? [].concat(body[k], v) : v;
-      }
-    } else if (contentType.includes('multipart/form-data')) {
-      const boundary = /boundary=("?)([^";]+)\1/i.exec(contentType)?.[2];
-      if (boundary) {
-        const parsed = parseMultipart(rawBody, boundary);
-        body = parsed.body;
-        files = parsed.files;
-      }
-    }
+    const parsed = parseBody(rawBody, contentType);
+    body = parsed.body;
+    files = parsed.files;
   }
 
   const uri = url.pathname + url.search;
@@ -90,9 +74,10 @@ export async function fromFetchRequest(request, options = {}) {
 // downstream handler keeps an intact body. (`max` is compared in bytes against Content-Length; the
 // prefix slice is by character, which can only over-scan a multibyte body — the safe direction.)
 async function readCappedText(request, max) {
-  const ceiling = Math.max(max, max * 4);
-  const declared = Number(request.headers?.get?.('content-length') || 0);
-  if (declared && declared > ceiling) return '';
+  // Do NOT skip scanning based on a declared Content-Length: an attacker can declare a huge length
+  // (or none) to dodge inspection while sending a small exploit body. Always stream-scan the prefix
+  // up to `max` (buffering is bounded to `max`; the rest is drained but not retained). Anything past
+  // the cap is unscanned — the documented prefix-scan tradeoff — but the body is never skipped whole.
   let clone;
   try {
     clone = request.clone();
@@ -150,6 +135,41 @@ function concatChunks(chunks, total) {
 // Minimal multipart/form-data parser: enough to expose field names + values (so `post.<field>`
 // and `raw` rules match uploads, e.g. a `__proto__` field name) and file metadata (filename via
 // `files.<field>`). We only need the textual structure, not the binary file contents.
+// Parse a request body into { body, files } for parameter-scoped rules. Content-type detection is
+// deliberately permissive: many AI-built apps `JSON.parse(await req.text())` regardless of the
+// declared type, so a JSON body arriving as `application/vnd.api+json`, `application/ld+json`,
+// `text/plain`, `application/csp-report`, or with NO content-type must still populate post.<field>
+// (otherwise a field-scoped rule silently resolves to nothing). Unrecognized/binary bodies stay `{}`
+// and are still matchable via `raw`.
+export function parseBody(rawBody, contentType) {
+  const ct = String(contentType || '').toLowerCase();
+  const isJson = ct.includes('application/json') || /\+json\b/.test(ct);
+  const isForm = ct.includes('application/x-www-form-urlencoded');
+  const isMultipart = ct.includes('multipart/form-data');
+  // "ambiguous" = a type an app commonly parses as JSON/form even though it isn't declared as such.
+  const isAmbiguous = ct === '' || ct.startsWith('text/plain') || ct.includes('csp-report') || ct.includes('/json');
+
+  if (isMultipart) {
+    const boundary = /boundary=("?)([^";]+)\1/i.exec(contentType)?.[2];
+    if (boundary) return parseMultipart(rawBody, boundary);
+    return { body: {}, files: undefined };
+  }
+  if (isForm) {
+    const body = {};
+    for (const [k, v] of new URLSearchParams(rawBody)) body[k] = k in body ? [].concat(body[k], v) : v;
+    return { body, files: undefined };
+  }
+  if (isJson || isAmbiguous) {
+    try {
+      const parsed = JSON.parse(rawBody);
+      if (parsed && typeof parsed === 'object') return { body: parsed, files: undefined };
+    } catch {
+      /* not JSON — leave body empty; `raw`/`all` still see the verbatim text */
+    }
+  }
+  return { body: {}, files: undefined };
+}
+
 export function parseMultipart(rawBody, boundary) {
   const body = {};
   const files = {};

src/protect/engine/node.js

@@ -7,7 +7,7 @@
 // are already populated) and the Web-Fetch adapter (Workers/edge). Mount it FIRST, before
 // any body-parser — it consumes the stream and exposes the parsed body as `req.body`.
 import { RuleEngine } from './engine.js';
-import { parseMultipart } from './fetch.js';
+import { parseBody } from './fetch.js';
 
 // Build the engine's request shape from a Node IncomingMessage + its raw body text.
 export function fromNodeRequest(req, rawBody = '') {
@@ -45,27 +45,11 @@ export function fromNodeRequest(req, rawBody = '') {
   let body = {};
   let files;
   if (rawBody) {
-    if (contentType.includes('application/json')) {
-      try {
-        body = JSON.parse(rawBody);
-      } catch {
-        body = {};
-      }
-    } else if (contentType.includes('application/x-www-form-urlencoded')) {
-      body = {};
-      for (const [k, v] of new URLSearchParams(rawBody)) {
-        body[k] = k in body ? [].concat(body[k], v) : v;
-      }
-    } else if (contentType.includes('multipart/form-data')) {
-      // Same parsing as the fetch adapter — expose field names/values via post.<field> and file
-      // metadata via files.<field>, so field-scoped rules match uploads on a raw-Node server too.
-      const boundary = /boundary=("?)([^";]+)\1/i.exec(contentType)?.[2];
-      if (boundary) {
-        const parsed = parseMultipart(rawBody, boundary);
-        body = parsed.body;
-        files = parsed.files;
-      }
-    }
+    // Same permissive content-type handling as the fetch adapter (+json / text/plain / no-CT bodies
+    // still populate post.<field>; multipart exposes field + file metadata) on a raw-Node server too.
+    const parsed = parseBody(rawBody, contentType);
+    body = parsed.body;
+    files = parsed.files;
   }
 
   const uri = url.pathname + url.search;

src/protect/engine/normalizer.js

@@ -129,12 +129,16 @@ export function removeSqlComments(value) {
         return value;
     }
 
+    // Collapse inline block comments to a space (the anti-obfuscation goal). We must NOT strip the
+    // line-comment forms (`--…`, `#…`) to end-of-line: on the WAF inspection path that DELETES
+    // attacker-controlled spans from the value the engine sees while the app still processes the
+    // original — e.g. `#<script>…` becomes empty and evades an XSS rule, though the browser still
+    // runs it. Keeping the content only ever ADDS matches (more of the value is inspected), never
+    // hides one. SQLi keyword detection is unaffected — the keywords remain visible.
     let result = value;
 
     result = result.replace(/\/\*[\s\S]*?\*\//g, ' ');
     result = result.replace(/\/\*![\s\S]*?\*\//g, ' ');
-    result = result.replace(/--[^\r\n]*/g, '');
-    result = result.replace(/#[^\r\n]*/g, '');
 
     return result;
 }
@@ -216,20 +220,29 @@ export function normalizeRequest(req, options = {}) {
     };
 }
 
-export function normalizeObject(value, options = {}) {
+// Depth bound for the recursive walk: a pathologically deep object would otherwise overflow the
+// stack, and the engine's per-rule catch would swallow that into a fail-open. Beyond the bound the
+// sub-value is left un-normalized (still matched, just in its raw form) rather than crashing.
+const MAX_NORMALIZE_DEPTH = 200;
+
+export function normalizeObject(value, options = {}, depth = 0) {
     if (typeof value === 'string') {
         return normalize(value, options);
     }
 
+    if (depth >= MAX_NORMALIZE_DEPTH) {
+        return value;
+    }
+
     if (Array.isArray(value)) {
-        return value.map(item => normalizeObject(item, options));
+        return value.map(item => normalizeObject(item, options, depth + 1));
     }
 
     if (typeof value === 'object' && value !== null) {
         const result = {};
 
         for (const [key, val] of Object.entries(value)) {
-            result[key] = normalizeObject(val, options);
+            result[key] = normalizeObject(val, options, depth + 1);
         }
 
         return result;

src/protect/engine/request.js

@@ -306,6 +306,12 @@ export class RequestResolver {
     if (body) {
       parts.push(typeof body === 'string' ? body : JSON.stringify(body));
     }
+    // Also fold in the verbatim body: a body the adapter couldn't structurally parse (an unusual
+    // content-type, a non-JSON payload) leaves `body` empty, but the raw text must still be matchable
+    // by an `all` rule — otherwise it's only visible via `raw`.
+    if (typeof this.#req._rawBody === 'string' && this.#req._rawBody) {
+      parts.push(this.#req._rawBody);
+    }
 
     const headers = this.#req.headers ?? {};
     const excludedHeaders = new Set([

src/protect/runtime.js

@@ -133,6 +133,11 @@ export async function createProtection(options = {}) {
       rule,
       engine: new RuleEngine({ firewall: [rule], onError }),
       redactors: rule.action === 'redact' || rule.action === 'encode' ? extractRedactors(rule) : null,
+      // A redact/encode condition that carries body-transforming mutations (base64_decode, urldecode,
+      // json_decode, …) detects on the DECODED body but the span redactors run on the RAW body — so
+      // they mask nothing and the secret is served while the log says "redacted". Flag it so screenText
+      // fails such a rule CLOSED (block) instead of serving a no-op redaction.
+      mutatedSpan: (rule.action === 'redact' || rule.action === 'encode') && hasSpanMutations(rule),
       // Optional cheap pre-filter: literal anchor(s) that MUST appear for the (expensive) regex to
       // have any chance of matching. Lets screenText skip the full scan on bodies with no candidate —
       // the common case — cutting CPU/latency and shrinking the regex/ReDoS surface. Case-insensitive.
@@ -170,20 +175,12 @@ export async function createProtection(options = {}) {
   // Response phase core: screen a text body → { verdict: 'pass'|'block'|'redact', body? }.
   // redact masks matched spans; block withholds; block wins over redact. Enforcement only in
   // block mode (dry-run records via onDetect but returns 'pass').
-  const isTextCT = (ct) => {
-    ct = (ct || '').toLowerCase();
-    // Exclude live streams: a Server-Sent-Events / token stream must pass through unbuffered.
-    // Screening buffers the whole body, so it would withhold every chunk until the stream ends —
-    // breaking incremental LLM streaming, which AI-built apps lean on heavily.
-    if (ct.includes('event-stream')) return false;
-    return ct === '' || /(json|text|xml|html|javascript|csv|yaml|x-www-form-urlencoded)/.test(ct);
-  };
   const screenText = (text, meta, reqCtx) => {
     let blockRule = null;
     const redactions = [];
     const headerMutations = [];
     let lowerText = null; // lazily lowercased body, only if a rule uses a prefilter
-    for (const { rule, engine: re, redactors, prefilter } of responseRuleSet) {
+    for (const { rule, engine: re, redactors, prefilter, mutatedSpan } of responseRuleSet) {
       // Cheap pre-filter: if none of the rule's literal anchors is in the body, its regex can't
       // match — skip the full scan (the common no-secret case) before touching the engine.
       if (prefilter) {
@@ -204,8 +201,15 @@ export async function createProtection(options = {}) {
       if (!result.blocked) continue;
       onDetect({ phase: 'response', mode, category: rule.category, rule, message: result.message });
       if (mode !== 'block') continue; // dry-run: observe only
-      if (redactors && redactors.length) redactions.push({ rule, redactors });
-      else if (isHeaderMutation(rule.action)) headerMutations.push(rule);
+      if (redactors && redactors.length) {
+        // Span redactors on a mutation-decoded rule can't map back to the raw body → fail closed.
+        const spanRedactors = redactors.filter((r) => !r.jsonPath);
+        if (mutatedSpan && spanRedactors.length) {
+          if (!blockRule) blockRule = rule;
+        } else {
+          redactions.push({ rule, redactors });
+        }
+      } else if (isHeaderMutation(rule.action)) headerMutations.push(rule);
       else if (!blockRule) blockRule = rule;
     }
     if (mode !== 'block' || (!blockRule && !redactions.length && !headerMutations.length)) return { verdict: 'pass' };
@@ -306,10 +310,16 @@ export async function createProtection(options = {}) {
       if (overflow) { if (chunk != null) origWrite(chunk, enc); return origEnd(cb); }
       collect(chunk, enc);
       if (overflow) return origEnd(cb); // collect just flushed head + final chunk on overflow
-      const text = Buffer.concat(chunks).toString('utf8');
+      const buffer = Buffer.concat(chunks);
       let ct = res.getHeader ? res.getHeader('content-type') : undefined;
       if (Array.isArray(ct)) ct = ct[0];
-      if (!isTextCT(ct)) { for (const c of chunks) origWrite(c); return origEnd(cb); }
+      const kind = screenableContentType(ct);
+      // Skip live streams / binary bodies (incl. an octet-stream that sniffs as binary) — untouched.
+      if (kind === 'skip' || (kind === 'sniff' && looksBinary(buffer))) {
+        for (const c of chunks) origWrite(c);
+        return origEnd(cb);
+      }
+      const text = buffer.toString('utf8');
       let r;
       try {
         r = screenText(text, { status: res.statusCode, headers: res.getHeaders ? res.getHeaders() : {} }, reqCtx);
@@ -595,14 +605,39 @@ function byPhase(rules, phase) {
   return (rules ?? []).filter((r) => (r.phase ?? 'request') === phase);
 }
 
+// Classify a content-type for response screening: 'text' = screen; 'sniff' = screen only if the
+// bytes aren't binary (octet-stream is often a misdeclared JSON export/config); 'skip' = pass
+// through unscreened (live streams, known binary families). SSE is matched on the EXACT base type,
+// not a loose substring — `application/json; profile="event-stream"` is not a stream.
+function baseContentType(ct) {
+  return String(ct || '').toLowerCase().split(';')[0].trim();
+}
+function screenableContentType(ct) {
+  const base = baseContentType(ct);
+  if (base === 'text/event-stream') return 'skip'; // live token/SSE stream — never buffer
+  if (base === '') return 'text';
+  if (/(json|text|xml|html|javascript|csv|yaml|x-www-form-urlencoded)/.test(base)) return 'text';
+  if (base === 'application/octet-stream') return 'sniff'; // maybe a text/JSON export mislabeled
+  return 'skip'; // image/video/audio/font/pdf/zip/wasm/… — don't buffer binary
+}
+// Cheap binary sniff over a byte prefix: a NUL byte, or many control chars, means "don't treat as text".
+function looksBinary(bytes) {
+  const n = Math.min(bytes.length, 512);
+  let ctrl = 0;
+  for (let i = 0; i < n; i++) {
+    const b = bytes[i];
+    if (b === 0) return true;
+    if (b < 9 || (b > 13 && b < 32)) ctrl++;
+  }
+  return n > 0 && ctrl / n > 0.1;
+}
+
 async function readTextResponse(response, cap = DEFAULT_SCREEN_CAP) {
   if (!response || typeof response.clone !== 'function') return null;
-  const ct = (response.headers?.get?.('content-type') || '').toLowerCase();
-  // Live stream (SSE / token stream): never buffer it — reading to completion would withhold the
-  // response until the stream ends, breaking incremental streaming. Pass it through unscreened.
-  if (ct.includes('event-stream')) return null;
-  const isText = ct === '' || /(json|text|xml|html|javascript|csv|yaml|x-www-form-urlencoded)/.test(ct);
-  if (!isText) return null;
+  const ct = response.headers?.get?.('content-type') || '';
+  const kind = screenableContentType(ct);
+  if (kind === 'skip') return null;
+  const sniff = kind === 'sniff';
   const len = Number(response.headers?.get?.('content-length') || 0);
   if (len && len > cap) return null;
   let clone;
@@ -620,11 +655,16 @@ async function readTextResponse(response, cap = DEFAULT_SCREEN_CAP) {
     const chunks = [];
     let size = 0;
     let over = false;
+    let sniffed = !sniff;
     try {
       for (;;) {
         const { done, value } = await reader.read();
         if (done) break;
         if (!value) continue;
+        if (!sniffed) {
+          sniffed = true;
+          if (looksBinary(value)) return null; // octet-stream that's actually binary — skip
+        }
         size += value.byteLength;
         if (over) continue; // keep draining, stop buffering
         if (size > cap) { over = true; continue; }
@@ -643,7 +683,9 @@ async function readTextResponse(response, cap = DEFAULT_SCREEN_CAP) {
 
   try {
     const text = await clone.text();
-    return text.length > cap ? null : text;
+    if (text.length > cap) return null;
+    if (sniff && looksBinary(new TextEncoder().encode(text.slice(0, 512)))) return null;
+    return text;
   } catch {
     return null;
   }
@@ -669,6 +711,24 @@ function headerObject(headers) {
   return out;
 }
 
+// True if any of the rule's conditions carries a body-transforming mutation on a SPAN match
+// (regex/contains/stripos) — those decode the body before matching, so a span redactor derived from
+// the literal/regex can't be located in the raw body. (array_key_value structural redaction decodes
+// the JSON itself, so json_decode there is fine and doesn't count.)
+function hasSpanMutations(rule) {
+  let found = false;
+  const walk = (conds) => {
+    for (const c of conds ?? []) {
+      if (found) return;
+      if (Array.isArray(c.rules)) walk(c.rules);
+      const isSpan = c.match && (c.match.type === 'regex' || c.match.type === 'contains' || c.match.type === 'stripos');
+      if (isSpan && Array.isArray(c.mutations) && c.mutations.length) found = true;
+    }
+  };
+  walk(rule.rule_v2);
+  return found;
+}
+
 // Derive redaction targets from a rule's own conditions: regex → mask every match;
 // contains/stripos → mask the literal. (Other match types can't identify a span → the
 // rule falls back to block.)
@@ -707,7 +767,10 @@ function extractRedactors(rule) {
   return out;
 }
 
-// HTML-entity escape, for the `encode` action (neutralize markup rather than mask it).
+// HTML-entity escape, for the `encode` action (neutralize markup rather than mask it). NOTE: this is
+// sound only for HTML text / attribute-VALUE contexts. It does NOT neutralize a `javascript:` / `data:`
+// URI or an event-handler name (those carry no HTML metacharacters) — use `block` for a rule that
+// targets a URL/scheme context. See the rule-authoring guidance in the triage-vpatch-npm skill.
 function htmlEscape(str) {
   return String(str).replace(/[&<>"']/g, (c) => ({ '&': '&amp;', '<': '&lt;', '>': '&gt;', '"': '&quot;', "'": '&#39;' })[c]);
 }