From 4aa7363dd86141a86a1f65378bf477191cafc8b6 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 13:21:41 +0200 Subject: [PATCH] protect: make fail-open observable, and stop startup hanging on the rule API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two enforcement-visibility gaps from an external review. 1. FAIL-OPEN WAS SILENT. The guard deliberately passes traffic it can't inspect — a request body over the cap, a response over the screening cap, a live stream, a binary body, a read/decode failure, a DNS resolver failure (or no resolver on this runtime). Each is a real hole in enforcement, and nothing recorded it, so "always-on" read as "always inspected". Every such bypass is now counted and reported: `onSkip({ phase, reason, detail, count })` plus `protection.coverage() -> { skipped: { 'response:body-cap': n, … } }`. readTextResponse now returns { text } | { skip: reason } so the reason is precise (body-cap / live-stream / binary-body / read-failed / decode-failed), and the node response + request paths and the egress resolver report through the same channel. A throwing onSkip can never affect request handling. 2. STARTUP COULD HANG FOR 30s. createProtection awaited the rule fetch with the client's full 30s timeout, so a slow API delayed app boot — and hosted platforms fail a deploy whose health check is slow. The INITIAL load now gets a short budget (bootTimeoutMs, default 5s) and falls back to last-known-good / bundled rules, which was already the fallback chain; refreshes keep the full budget (refreshTimeoutMs). Verified: a hanging fetch boots in ~300ms, still protected by the fallback ruleset. Also documents the mode-default discrepancy honestly: this API defaults to dry-run while the scaffolded guard passes mode: 'block' — both intentional, previously confusing. Co-Authored-By: Claude Opus 4.8 --- src/protect/egress.js | 17 ++++- src/protect/engine/client.js | 7 +- src/protect/engine/pulse-client.js | 8 ++- src/protect/rules/source.js | 10 ++- src/protect/runtime.js | 95 ++++++++++++++++++++++------ tests/protect/coverage-skips.test.ts | 95 ++++++++++++++++++++++++++++ 6 files changed, 201 insertions(+), 31 deletions(-) create mode 100644 tests/protect/coverage-skips.test.ts diff --git a/src/protect/egress.js b/src/protect/egress.js index fc0336e..7986f27 100644 --- a/src/protect/egress.js +++ b/src/protect/egress.js @@ -12,7 +12,7 @@ * lookup?: Function }} opts * @returns {Promise<() => void>} uninstall (restores every patched surface) */ -export async function installEgressGuard({ shouldBlock, onBlock, dnsScreen = true, lookup, allowHosts } = {}) { +export async function installEgressGuard({ shouldBlock, onBlock, onSkip, dnsScreen = true, lookup, allowHosts } = {}) { const restores = []; if (typeof shouldBlock !== 'function') return () => {}; const exempt = new Set((allowHosts ?? []).map((h) => String(h).toLowerCase())); @@ -41,13 +41,23 @@ export async function installEgressGuard({ shouldBlock, onBlock, dnsScreen = tru } } + // A resolver failure means the destination was NOT screened by IP — the hostname check alone let it + // through. That's a real (if rare) coverage hole, so report it via onSkip instead of failing open + // silently. Still fail-open: a broken resolver must not take the app's outbound traffic down. + const skip = (reason, detail) => { try { onSkip?.({ phase: 'egress', reason, detail }); } catch { /* never affect traffic */ } }; + // True when a hostname resolves to a disallowed address. Fail-open: any resolver error → false. const resolvesToDisallowed = (url, host, method) => new Promise((resolve) => { - if (!screen || !host || screen.isIP(host) !== 0 || screen.isExempt(host)) return resolve(false); + if (!screen) { + // No node:dns/net here (edge runtime) or screening disabled — hostname rules only. + if (host && dnsScreen) skip('resolver-unavailable', { host }); + return resolve(false); + } + if (!host || screen.isIP(host) !== 0 || screen.isExempt(host)) return resolve(false); try { screen.lookup(host, { all: true }, (err, addresses) => { - if (err || !Array.isArray(addresses)) return resolve(false); + if (err || !Array.isArray(addresses)) { skip('resolver-failed', { host }); return resolve(false); } for (const a of addresses) { const ip = a && typeof a === 'object' ? a.address : a; if (ip && block(url, ip, method)) return resolve(true); @@ -55,6 +65,7 @@ export async function installEgressGuard({ shouldBlock, onBlock, dnsScreen = tru resolve(false); }); } catch { + skip('resolver-failed', { host }); resolve(false); } }); diff --git a/src/protect/engine/client.js b/src/protect/engine/client.js index c81cf14..7a8dff3 100644 --- a/src/protect/engine/client.js +++ b/src/protect/engine/client.js @@ -15,8 +15,11 @@ export class PatchstackRuleClient { #cacheTime = null; #ttlEffective = 0; #etag; + #timeoutMs; - constructor({ token, baseUrl, cacheTtl, etag } = {}) { + constructor({ token, baseUrl, cacheTtl, etag, timeoutMs } = {}) { + // Bounded so app STARTUP can't hang on a slow API (see the boot budget in rules/source.js). + this.#timeoutMs = Number(timeoutMs) > 0 ? Number(timeoutMs) : 30_000; this.#token = token ?? process.env.PATCHSTACK_WAF_TOKEN; this.#baseUrl = baseUrl ?? process.env.PATCHSTACK_WAF_API_URL ?? DEFAULT_BASE_URL; this.#cacheTtl = Number.isFinite(cacheTtl) && cacheTtl > 0 ? cacheTtl : DEFAULT_CACHE_TTL; @@ -48,7 +51,7 @@ export class PatchstackRuleClient { method: 'POST', headers, body: JSON.stringify({}), - signal: AbortSignal.timeout(30_000) + signal: AbortSignal.timeout(this.#timeoutMs) }); if (response.status === 304) { diff --git a/src/protect/engine/pulse-client.js b/src/protect/engine/pulse-client.js index 371fbbc..1271f4c 100644 --- a/src/protect/engine/pulse-client.js +++ b/src/protect/engine/pulse-client.js @@ -15,6 +15,7 @@ const JITTER_FRACTION = 0.1; // the client simply behaves as before (a full fetch every refresh). export class PulseRuleClient { #siteUuid; + #timeoutMs; #baseUrl; #cacheTtl; #cache = null; @@ -22,7 +23,10 @@ export class PulseRuleClient { #ttlEffective = 0; #etag; - constructor({ siteUuid, baseUrl, cacheTtl, etag } = {}) { + constructor({ siteUuid, baseUrl, cacheTtl, etag, timeoutMs } = {}) { + // Bounded so app STARTUP can't hang on a slow API: hosted platforms fail a deploy whose health + // check is slow, and we always have a cache/bundled fallback to boot from. + this.#timeoutMs = Number(timeoutMs) > 0 ? Number(timeoutMs) : 30_000; this.#siteUuid = siteUuid ?? process.env.PATCHSTACK_SITE_UUID; this.#baseUrl = baseUrl ?? process.env.PATCHSTACK_PULSE_RULES_URL ?? DEFAULT_BASE_URL; this.#cacheTtl = Number.isFinite(cacheTtl) && cacheTtl > 0 ? cacheTtl : DEFAULT_CACHE_TTL; @@ -41,7 +45,7 @@ export class PulseRuleClient { try { const headers = { Accept: 'application/json' }; if (this.#etag) headers['If-None-Match'] = this.#etag; - const response = await fetch(url, { method: 'GET', headers, signal: AbortSignal.timeout(30_000) }); + const response = await fetch(url, { method: 'GET', headers, signal: AbortSignal.timeout(this.#timeoutMs) }); if (response.status === 304) { this.#touch(now); // revalidated — reset the clock (fresh jitter) diff --git a/src/protect/rules/source.js b/src/protect/rules/source.js index 62c5a6f..0897088 100644 --- a/src/protect/rules/source.js +++ b/src/protect/rules/source.js @@ -5,10 +5,14 @@ import { PatchstackRuleClient } from '../engine/index.js'; import { PulseRuleClient } from '../engine/pulse-client.js'; -export async function resolveRules(options, store) { +export async function resolveRules(options, store, ctx = {}) { + // The INITIAL load is on the app's startup path, so the runtime gives it a short budget (see + // bootTimeoutMs) and falls back to cache/bundled rather than delaying boot; refreshes get the full + // budget. A timeout here is not a protection gap by itself — last-known-good still applies. + const timeoutMs = ctx.timeoutMs; if (options.siteUuid) { const prior = await store.read(); // { bundle, etag } | null - const client = new PulseRuleClient({ siteUuid: options.siteUuid, baseUrl: options.pulseRulesUrl, etag: prior?.etag }); + const client = new PulseRuleClient({ siteUuid: options.siteUuid, baseUrl: options.pulseRulesUrl, etag: prior?.etag, timeoutMs }); const res = await client.getRules(); if (res.success && res.notModified && prior?.bundle) return normalizeBundle(prior.bundle); if (res.success && !res.notModified) { @@ -30,7 +34,7 @@ export async function resolveRules(options, store) { if (options.token) { const prior = await store.read(); - const client = new PatchstackRuleClient({ token: options.token, baseUrl: options.baseUrl, etag: prior?.etag }); + const client = new PatchstackRuleClient({ token: options.token, baseUrl: options.baseUrl, etag: prior?.etag, timeoutMs }); const res = await client.getRules(); if (res.success && res.notModified && prior?.bundle) return normalizeBundle(prior.bundle); if (res.success && !res.notModified) { diff --git a/src/protect/runtime.js b/src/protect/runtime.js index 4e294ae..084d867 100644 --- a/src/protect/runtime.js +++ b/src/protect/runtime.js @@ -3,9 +3,14 @@ // One entry point that composes the node-waf engine + adapters with: // - a rule source: an explicit bundle, or fetched from the Patchstack API (token), // with a disk cache so the engine keeps working on last-known-good if the API is down -// - execution modes: 'dry-run' (detect + log, never block — the safe onramp) and -// 'block' (enforce). Default is 'dry-run'. -// - fail-open everywhere: a rule/engine error never blocks (or crashes) a request. +// - execution modes: 'dry-run' (detect + log, never block — the safe onramp) and 'block' (enforce). +// This API's default is 'dry-run'. NOTE the scaffolded guard (`patchstack-connect protect`) +// deliberately passes mode: 'block' and only drops to dry-run when PATCHSTACK_MODE=dry-run — so an +// installed guard ENFORCES by default even though this constructor's default doesn't. Precedence: +// PATCHSTACK_MODE env > API `enforcement` > options.mode > dry-run. +// - fail-open everywhere: a rule/engine error never blocks (or crashes) a request. Where the guard +// fails open *without* inspecting (body caps, live streams, binary bodies, resolver failures) it +// is counted and reported — see `protection.coverage()` / the `onSkip` option. // // Runtime guards: .express(), .node(), .fetch(handler) / .fetchGuard() — same policy, // every runtime an AI builder deploys to. @@ -98,7 +103,11 @@ export async function createProtection(options = {}) { // One tiered store (memory → filesystem/pluggable) shared by the initial load and every refresh. const store = makeStore(options); - const bundle = await resolveRules(options, store); + // Startup must not hang on the network: hosted platforms (Replit et al.) fail a deploy whose health + // check is slow, and the guard can always boot from last-known-good / the bundled fallback. Refreshes + // keep the full budget. Override with { bootTimeoutMs }. + const bootTimeoutMs = Number(options.bootTimeoutMs) > 0 ? Number(options.bootTimeoutMs) : 5_000; + const bundle = await resolveRules(options, store, { timeoutMs: bootTimeoutMs }); // Mode is mutable so a Pulse refresh can flip dry-run ↔ block when SaaS enables production. // Precedence: PATCHSTACK_MODE env (local override) > API enforcement > options.mode > dry-run. let mode = resolveMode(options, bundle); @@ -150,6 +159,25 @@ export async function createProtection(options = {}) { applyBundle(bundle); + // Fail-open COVERAGE. The guard deliberately passes traffic through rather than risk breaking the + // app: an oversized request body, a response past the screening cap, a live stream, a binary body, a + // parse failure, a DNS resolver failure. Each of those is a real hole in enforcement, and until now + // it was SILENT — "always-on" read as "always inspected". Every such bypass is now counted and + // reported to `onSkip`, so a host can alert on it and `protection.coverage()` can be surfaced. + const skipCounts = Object.create(null); + const onSkip = typeof options.onSkip === 'function' ? options.onSkip : null; + const recordSkip = (phase, reason, detail) => { + const key = `${phase}:${reason}`; + skipCounts[key] = (skipCounts[key] ?? 0) + 1; + if (onSkip) { + try { + onSkip({ phase, reason, detail, count: skipCounts[key] }); + } catch { + /* a reporting callback must never affect request handling */ + } + } + }; + const maskFn = typeof options.maskWith === 'function' ? options.maskWith @@ -263,8 +291,14 @@ export async function createProtection(options = {}) { // Screen a fetch Response (used by .fetch() and — via protection.screenResponse — the Supabase guard). const screenResp = async (response, reqCtx) => { - const text = await readTextResponse(response, screenCap); - if (text == null) return response; + const read = await readTextResponse(response, screenCap); + if (read.skip) { + // Nothing was screened — a leak/PII rule cannot have applied. Record it (a live stream and a + // binary body are by design; a body-cap or read failure is a coverage hole worth alerting on). + if (read.skip !== 'not-a-response') recordSkip('response', read.skip, { status: response?.status }); + return response; + } + const text = read.text; const r = screenText(text, { status: response.status, headers: headerObject(response.headers) }, reqCtx); if (r.verdict === 'block') return leakResponse(); if (r.verdict === 'redact') return rebuildResponse(response, r.body, r.headers); @@ -293,6 +327,7 @@ export async function createProtection(options = {}) { chunks.length = 0; origWrite(buf); overflow = true; + recordSkip('response', 'body-cap', { bytes: size }); return; } chunks.push(buf); @@ -316,6 +351,7 @@ export async function createProtection(options = {}) { 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))) { + recordSkip('response', kind === 'skip' ? (baseContentType(ct) === 'text/event-stream' ? 'live-stream' : 'non-text-content-type') : 'binary-body'); for (const c of chunks) origWrite(c); return origEnd(cb); } @@ -381,6 +417,16 @@ export async function createProtection(options = {}) { return { request: requestRules, response: responseRules, egress: egressRules }; }, + /** + * Enforcement coverage: how often the guard FAILED OPEN rather than inspecting, keyed + * `:` (e.g. `response:body-cap`, `request:body-cap`, `response:live-stream`, + * `egress:resolver-failed`). "Always-on" is not "always inspected" — surface this (or pass + * `onSkip`) so an unscreened path is visible and alertable rather than silent. + */ + coverage() { + return { skipped: { ...skipCounts } }; + }, + // Screen a fetch Response through the response-phase rules (redact/block). Used by // .fetch(), and by the Supabase guard on its forwarded upstream response. screenResponse: (response, request) => screenResp(response, request ? reqContextFromFetch(request) : undefined), @@ -462,6 +508,7 @@ export async function createProtection(options = {}) { next(); }); req.on('end', () => { + if (overflow) recordSkip('request', 'body-cap', { bytes: size, limit: maxBytes }); const rawBody = overflow ? '' : Buffer.concat(chunks).toString('utf8'); let shaped; let result; @@ -504,6 +551,9 @@ export async function createProtection(options = {}) { protection.uninstallEgress = await installEgressGuard({ shouldBlock: egressShouldBlock, onBlock: options.onEgressBlock, + // Route egress coverage gaps (a DNS resolver failure / no resolver on this runtime) into the + // same skip accounting as the request/response phases. + onSkip: ({ reason, detail }) => recordSkip('egress', reason, detail), dnsScreen: options.screenDns !== false, allowHosts: options.allowHosts, }); @@ -541,7 +591,7 @@ export async function createProtection(options = {}) { onError?.(err); // a failed report must not stop the rule refresh } } - const next = await resolveRules(options, store); + const next = await resolveRules(options, store, { timeoutMs: options.refreshTimeoutMs }); mode = resolveMode(options, next); applyBundle(next); }; @@ -632,23 +682,26 @@ function looksBinary(bytes) { return n > 0 && ctrl / n > 0.1; } +// Returns { text } when the body was fully buffered for screening, or { skip: } when it was +// NOT screened — the reason is surfaced to `onSkip`/coverage so a fail-open bypass is observable +// instead of silent (an unscreened response is a real hole in enforcement). async function readTextResponse(response, cap = DEFAULT_SCREEN_CAP) { - if (!response || typeof response.clone !== 'function') return null; + if (!response || typeof response.clone !== 'function') return { skip: 'not-a-response' }; const ct = response.headers?.get?.('content-type') || ''; const kind = screenableContentType(ct); - if (kind === 'skip') return null; + if (kind === 'skip') return { skip: baseContentType(ct) === 'text/event-stream' ? 'live-stream' : 'non-text-content-type' }; const sniff = kind === 'sniff'; const len = Number(response.headers?.get?.('content-length') || 0); - if (len && len > cap) return null; + if (len && len > cap) return { skip: 'body-cap' }; let clone; try { clone = response.clone(); } catch { - return null; + return { skip: 'clone-failed' }; } // Stream the read so a body WITHOUT a Content-Length can't buffer past the cap. Over the cap the - // response is left UNSCREENED (null) — but we keep draining the clone so the original stays intact. + // response is left UNSCREENED — but we keep draining the clone so the original stays intact. const body = clone.body; if (body && typeof body.getReader === 'function') { const reader = body.getReader(); @@ -663,7 +716,7 @@ async function readTextResponse(response, cap = DEFAULT_SCREEN_CAP) { if (!value) continue; if (!sniffed) { sniffed = true; - if (looksBinary(value)) return null; // octet-stream that's actually binary — skip + if (looksBinary(value)) return { skip: 'binary-body' }; } size += value.byteLength; if (over) continue; // keep draining, stop buffering @@ -671,23 +724,23 @@ async function readTextResponse(response, cap = DEFAULT_SCREEN_CAP) { chunks.push(value); } } catch { - return null; + return { skip: 'read-failed' }; } - if (over) return null; + if (over) return { skip: 'body-cap' }; try { - return new TextDecoder().decode(concatBytes(chunks, size)); + return { text: new TextDecoder().decode(concatBytes(chunks, size)) }; } catch { - return null; + return { skip: 'decode-failed' }; } } try { const text = await clone.text(); - if (text.length > cap) return null; - if (sniff && looksBinary(new TextEncoder().encode(text.slice(0, 512)))) return null; - return text; + if (text.length > cap) return { skip: 'body-cap' }; + if (sniff && looksBinary(new TextEncoder().encode(text.slice(0, 512)))) return { skip: 'binary-body' }; + return { text }; } catch { - return null; + return { skip: 'read-failed' }; } } diff --git a/tests/protect/coverage-skips.test.ts b/tests/protect/coverage-skips.test.ts new file mode 100644 index 0000000..47949dc --- /dev/null +++ b/tests/protect/coverage-skips.test.ts @@ -0,0 +1,95 @@ +import { describe, it, expect, vi } from 'vitest'; +import { createProtection } from '../../src/protect/runtime.js'; + +// Fail-open coverage must be OBSERVABLE. The guard deliberately passes traffic it can't inspect +// (oversized bodies, live streams, binary bodies, resolver failures) — each of those is a real hole in +// enforcement, so it must be counted in `protection.coverage()` and reported to `onSkip`, not silent. + +const AWS = 'AKIA' + 'IOSFODNN7' + 'EXAMPLE'; + +describe('response-phase skips are recorded', () => { + it('records a body-cap bypass and reports it to onSkip', async () => { + const skips: any[] = []; + const p: any = await createProtection({ mode: 'block', onSkip: (s: any) => skips.push(s) }); + // A body past the screening cap is served UNSCREENED — the secret survives, which is exactly why + // it must be observable. + const big = JSON.stringify({ apiKey: AWS, pad: 'x'.repeat(600 * 1024) }); + const out = await p.screenResponse( + new Response(big, { status: 200, headers: { 'content-type': 'application/json' } }), + new Request('https://app.com/x'), + ); + expect((await out.text()).includes(AWS)).toBe(true); // unscreened (documented fail-open) + expect(p.coverage().skipped['response:body-cap']).toBe(1); + expect(skips).toEqual([expect.objectContaining({ phase: 'response', reason: 'body-cap' })]); + }); + + it('records a live-stream passthrough distinctly from a cap', async () => { + const p: any = await createProtection({ mode: 'block' }); + await p.screenResponse( + new Response('data: hi\n\n', { status: 200, headers: { 'content-type': 'text/event-stream' } }), + new Request('https://app.com/x'), + ); + expect(p.coverage().skipped['response:live-stream']).toBe(1); + }); + + it('records a binary body skip', async () => { + const p: any = await createProtection({ mode: 'block' }); + await p.screenResponse( + new Response(new Uint8Array([0, 1, 2, 3, 255]), { status: 200, headers: { 'content-type': 'application/octet-stream' } }), + new Request('https://app.com/x'), + ); + expect(p.coverage().skipped['response:binary-body']).toBe(1); + }); + + it('counts repeats and leaves coverage empty when everything was inspected', async () => { + const p: any = await createProtection({ mode: 'block' }); + const ok = () => new Response(JSON.stringify({ ok: true }), { status: 200, headers: { 'content-type': 'application/json' } }); + await p.screenResponse(ok(), new Request('https://app.com/x')); + expect(p.coverage().skipped).toEqual({}); + + const stream = () => new Response('data: x\n\n', { status: 200, headers: { 'content-type': 'text/event-stream' } }); + await p.screenResponse(stream(), new Request('https://app.com/x')); + await p.screenResponse(stream(), new Request('https://app.com/x')); + expect(p.coverage().skipped['response:live-stream']).toBe(2); + }); + + it('never lets a throwing onSkip affect request handling', async () => { + const p: any = await createProtection({ + mode: 'block', + onSkip: () => { throw new Error('reporting blew up'); }, + }); + const out = await p.screenResponse( + new Response('data: x\n\n', { status: 200, headers: { 'content-type': 'text/event-stream' } }), + new Request('https://app.com/x'), + ); + expect(out.status).toBe(200); // served fine despite the callback throwing + }); +}); + +describe('startup is not blocked by a slow rule API', () => { + it('boots from the bundled fallback within the boot budget when the fetch hangs', async () => { + // A hosted platform fails a deploy whose health check is slow, so the INITIAL fetch gets a short + // budget (bootTimeoutMs) and we boot on last-known-good / bundled rules instead of waiting. + const fallback = { + firewall: [{ id: 'fb-1', rule_v2: [{ parameter: 'raw', match: { type: 'contains', value: '__proto__' } }] }], + whitelists: [], + whitelist_keys: {}, + }; + vi.stubGlobal('fetch', vi.fn((_u: any, init: any) => new Promise((_res, rej) => { + // Never resolve: only the abort signal ends this, which is the point of the budget. + init?.signal?.addEventListener?.('abort', () => rej(new Error('The operation was aborted'))); + }))); + const started = Date.now(); + const p: any = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + rules: fallback as any, + mode: 'block', + bootTimeoutMs: 300, + }); + const elapsed = Date.now() - started; + expect(elapsed).toBeLessThan(3000); // did NOT wait the full 30s client timeout + expect(p.rules.request.map((r: any) => r.id)).toEqual(['fb-1']); // protected via the fallback + vi.restoreAllMocks(); + }); +});