From 7dc582599a96dc5c9e57b5f86744b9c4df80606a Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 13:24:53 +0200 Subject: [PATCH 1/2] protect: validate delivered rule bundles; bound patterns; harden the telemetry origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Defense-in-depth follow-ups from an external review. Delivered rules are policy fetched over the network and executed on every request, so an upstream problem (schema drift, a corpus mistake, a compromised response) could hand the engine unbounded work or silently unenforceable rules. - NEW rules/validate.js + wired into normalizeBundle, the single chokepoint every rule path (live fetch, cache, bundled fallback) already funnels through. Bounds rule count, whitelist count, conditions per rule, nesting depth, regex length and match-value length, and requires a known phase/action. A failing rule is DROPPED WITH A REPORTED REASON (`onRuleRejected`, else a one-line warning) — never kept while protecting nothing. Whitelists are validated too: a malformed whitelist suppresses real rules, so it's a protection risk, not just a detection one. - safeRegExp now refuses a pattern over 1000 chars as a backstop for caller-supplied bundles that never passed through the validator. (A complete ReDoS analysis still isn't possible statically — a bounded/off-loop matcher remains the real fix.) - resolveApiBase: PATCHSTACK_API_BASE is the origin the site api_key is exchanged against, so an injected value was a credential-exfiltration path. It must now be https (localhost permitted for local testing); anything else is refused with a warning and falls back to the default origin. - egress-dns tests bind a loopback listener, which some sandboxed/CI environments refuse (EPERM) — they now skip cleanly instead of failing/hanging. The suite is green both with and without a binding-capable environment. Co-Authored-By: Claude Opus 4.8 --- src/protect/engine/engine.js | 8 ++ src/protect/firewall-log.js | 22 ++++- src/protect/rules/source.js | 44 +++++++--- src/protect/rules/validate.js | 112 ++++++++++++++++++++++++++ tests/protect/egress-dns.test.ts | 26 +++++- tests/protect/rule-validation.test.ts | 106 ++++++++++++++++++++++++ 6 files changed, 305 insertions(+), 13 deletions(-) create mode 100644 src/protect/rules/validate.js create mode 100644 tests/protect/rule-validation.test.ts diff --git a/src/protect/engine/engine.js b/src/protect/engine/engine.js index 956a81c..a025330 100644 --- a/src/protect/engine/engine.js +++ b/src/protect/engine/engine.js @@ -29,10 +29,18 @@ function warnRejectedPatternOnce(pattern) { ); } +// An absurdly long pattern is either a mistake or an attack on our own matcher; compiling and running +// it on every request is unbounded work. The rule-bundle validator rejects these upstream — this is the +// backstop for a caller-supplied bundle that never went through it. +const MAX_PATTERN_LENGTH = 1000; + export function safeRegExp(pattern) { if (!pattern) { return null; } + if (typeof pattern !== 'string' || pattern.length > MAX_PATTERN_LENGTH) { + return null; + } for (const dangerous of REDOS_PATTERNS) { if (dangerous.test(pattern)) { diff --git a/src/protect/firewall-log.js b/src/protect/firewall-log.js index 4b7c0bd..29f3e1d 100644 --- a/src/protect/firewall-log.js +++ b/src/protect/firewall-log.js @@ -27,10 +27,30 @@ export function parseApiKey(apiKey) { * Derive api.patchstack.com origin from a Pulse manifest/rules URL override. * @param {string | undefined} pulseOrManifestUrl */ +// https, or an explicit local origin for development. Anything else is refused as an api-key target. +function isSafeApiOrigin(value) { + try { + const u = new URL(value); + if (u.protocol === 'https:') return true; + return u.protocol === 'http:' && (u.hostname === 'localhost' || u.hostname === '127.0.0.1' || u.hostname === '::1'); + } catch { + return false; + } +} + export function resolveApiBase(pulseOrManifestUrl) { const fromEnv = typeof process !== 'undefined' ? process.env?.PATCHSTACK_API_BASE : undefined; if (typeof fromEnv === 'string' && fromEnv.length > 0) { - return fromEnv.replace(/\/$/, ''); + // The site api_key is exchanged for a token against this origin, so a hostile/injected env value + // would be a credential-exfiltration path. Require HTTPS (localhost excepted for local testing); + // anything else falls back to the default origin rather than shipping the key off-platform. + const candidate = fromEnv.replace(/\/$/, ''); + if (isSafeApiOrigin(candidate)) return candidate; + // eslint-disable-next-line no-console + console.warn( + '[patchstack] ignoring PATCHSTACK_API_BASE: block-log reporting requires an https origin ' + + '(or localhost). Falling back to the default API origin.', + ); } if (typeof pulseOrManifestUrl === 'string' && pulseOrManifestUrl.length > 0) { try { diff --git a/src/protect/rules/source.js b/src/protect/rules/source.js index 62c5a6f..9adcb69 100644 --- a/src/protect/rules/source.js +++ b/src/protect/rules/source.js @@ -4,25 +4,26 @@ // The `store` (see ./store.js) is passed in so a refresh reuses the same tiered cache. import { PatchstackRuleClient } from '../engine/index.js'; import { PulseRuleClient } from '../engine/pulse-client.js'; +import { validateBundle } from './validate.js'; export async function resolveRules(options, store) { 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 res = await client.getRules(); - if (res.success && res.notModified && prior?.bundle) return normalizeBundle(prior.bundle); + if (res.success && res.notModified && prior?.bundle) return normalizeBundle(prior.bundle, options); if (res.success && !res.notModified) { - const bundle = normalizeBundle(res); + const bundle = normalizeBundle(res, options); await store.write({ bundle, etag: res.etag ?? null }); return bundle; } if (prior?.bundle) { options.onError?.(new Error(`pulse rule fetch failed (${res.error ?? 'no usable response'}); using cached bundle`)); - return normalizeBundle(prior.bundle); + return normalizeBundle(prior.bundle, options); } if (options.rules) { options.onError?.(new Error(`pulse rule fetch failed (${res.error ?? 'no usable response'}); using bundled fallback`)); - return normalizeBundle(options.rules); + return normalizeBundle(options.rules, options); } options.onError?.(new Error(`pulse rule fetch failed (${res.error ?? 'no usable response'}); no cache — running with no rules`)); return emptyBundle(); @@ -32,32 +33,55 @@ export async function resolveRules(options, store) { const prior = await store.read(); const client = new PatchstackRuleClient({ token: options.token, baseUrl: options.baseUrl, etag: prior?.etag }); const res = await client.getRules(); - if (res.success && res.notModified && prior?.bundle) return normalizeBundle(prior.bundle); + if (res.success && res.notModified && prior?.bundle) return normalizeBundle(prior.bundle, options); if (res.success && !res.notModified) { - const bundle = normalizeBundle(res); + const bundle = normalizeBundle(res, options); await store.write({ bundle, etag: res.etag ?? null }); return bundle; } if (prior?.bundle) { options.onError?.(new Error(`rule fetch failed (${res.error ?? 'no usable response'}); using cached bundle`)); - return normalizeBundle(prior.bundle); + return normalizeBundle(prior.bundle, options); } options.onError?.(new Error(`rule fetch failed (${res.error ?? 'no usable response'}); no cache — running with no rules`)); return emptyBundle(); } if (options.rules) { - return normalizeBundle(options.rules); + return normalizeBundle(options.rules, options); } return emptyBundle(); } -export function normalizeBundle(b) { +// Every rule path (live fetch, cache, bundled fallback) funnels through here, so this is where the +// delivered policy is VALIDATED before the engine ever executes it: bounded rule count / conditions / +// nesting / pattern length, known phases + actions. A rule that fails is dropped with a reported reason +// (`onRuleRejected`) rather than silently kept — an unenforceable rule must never look enforced. +export function normalizeBundle(b, options = {}) { const enforcement = b?.enforcement ?? b?.mode; - return { + const { bundle: checked, rejected } = validateBundle({ firewall: Array.isArray(b.firewall) ? b.firewall : [], whitelists: Array.isArray(b.whitelists) ? b.whitelists : [], + }); + if (rejected.length > 0) { + const report = options.onRuleRejected; + if (typeof report === 'function') { + for (const r of rejected) { + try { report(r); } catch { /* reporting must never break rule loading */ } + } + } else { + const sample = rejected.slice(0, 3).map((r) => `${r.id} (${r.reason})`).join('; '); + // eslint-disable-next-line no-console + console.warn( + `[patchstack] ${rejected.length} delivered rule(s) rejected as invalid/oversized and are NOT enforced: ${sample}` + + (rejected.length > 3 ? ', …' : ''), + ); + } + } + return { + firewall: checked.firewall, + whitelists: checked.whitelists, whitelist_keys: b.whitelist_keys ?? {}, ...(enforcement === 'block' || enforcement === 'dry-run' ? { enforcement } : {}), }; diff --git a/src/protect/rules/validate.js b/src/protect/rules/validate.js new file mode 100644 index 0000000..8015a66 --- /dev/null +++ b/src/protect/rules/validate.js @@ -0,0 +1,112 @@ +// Rule-bundle validation. Delivered rules are POLICY fetched over the network, and the engine executes +// them on every request — so an upstream compromise, a schema drift, or a mistake in the corpus could +// otherwise hand the app unbounded work (a 50k-rule bundle, a 500-deep condition tree, a pathological +// regex) or silently unenforceable junk. +// +// Two principles: +// 1. REJECT, don't silently skip. A rule that fails validation is dropped WITH a reported reason, so +// an unenforceable rule is visible instead of quietly protecting nothing. +// 2. Bound everything the engine will walk: rule count, conditions per rule, nesting depth, and +// pattern length. Caps are deliberately far above any real corpus rule. +// +// Fail-open in spirit: validation never throws, and a bundle whose rules are all rejected simply means +// "no rules" (the app keeps serving) — never a crash. + +export const LIMITS = { + maxRules: 5000, + maxWhitelists: 2000, + maxConditionsPerRule: 250, + maxNestingDepth: 12, + maxRegexLength: 1000, + maxValueLength: 8192, +}; + +const PHASES = new Set(['request', 'response', 'egress']); +const ACTIONS = new Set(['block', 'redact', 'encode', 'set-header', 'remove-header', 'harden-cookie']); + +/** + * Validate a delivered bundle. Returns `{ bundle, rejected }` where `bundle` contains only rules that + * passed and `rejected` is `[{ id, reason }]` for everything dropped. + * @param {object} bundle + * @returns {{ bundle: object, rejected: Array<{id: string, reason: string}> }} + */ +export function validateBundle(bundle) { + const rejected = []; + const inFirewall = Array.isArray(bundle?.firewall) ? bundle.firewall : []; + const inWhitelists = Array.isArray(bundle?.whitelists) ? bundle.whitelists : []; + + const firewall = []; + for (const rule of inFirewall) { + if (firewall.length >= LIMITS.maxRules) { + rejected.push({ id: idOf(rule), reason: `bundle exceeds maxRules (${LIMITS.maxRules})` }); + continue; + } + const reason = ruleProblem(rule); + if (reason) rejected.push({ id: idOf(rule), reason }); + else firewall.push(rule); + } + + const whitelists = []; + for (const wl of inWhitelists) { + if (whitelists.length >= LIMITS.maxWhitelists) { + rejected.push({ id: idOf(wl), reason: `bundle exceeds maxWhitelists (${LIMITS.maxWhitelists})` }); + continue; + } + // A whitelist SUPPRESSES rules, so a malformed one is a protection risk, not a detection risk. + const reason = conditionsProblem(wl?.rule_v2); + if (reason) rejected.push({ id: idOf(wl), reason: `whitelist: ${reason}` }); + else whitelists.push(wl); + } + + return { + bundle: { ...bundle, firewall, whitelists }, + rejected, + }; +} + +function idOf(rule) { + const id = rule?.id ?? rule?.rule_id; + return id === undefined || id === null ? '(unidentified)' : String(id); +} + +/** @returns {string|null} a reason the rule must be dropped, or null when it's acceptable. */ +function ruleProblem(rule) { + if (!rule || typeof rule !== 'object') return 'not an object'; + if (rule.phase !== undefined && !PHASES.has(rule.phase)) return `unknown phase "${rule.phase}"`; + if (rule.action !== undefined && !ACTIONS.has(rule.action)) return `unknown action "${rule.action}"`; + const capOverride = rule.max_bytes; + if (capOverride !== undefined && !(Number(capOverride) > 0)) return 'max_bytes must be a positive number'; + return conditionsProblem(rule.rule_v2); +} + +function conditionsProblem(conditions, depth = 0) { + if (!Array.isArray(conditions)) return 'rule_v2 must be an array of conditions'; + if (conditions.length === 0) return 'rule_v2 is empty (would never match)'; + if (depth > LIMITS.maxNestingDepth) return `nesting deeper than ${LIMITS.maxNestingDepth}`; + if (conditions.length > LIMITS.maxConditionsPerRule) { + return `more than ${LIMITS.maxConditionsPerRule} conditions`; + } + for (const c of conditions) { + if (!c || typeof c !== 'object') return 'condition is not an object'; + if (Array.isArray(c.rules)) { + const nested = conditionsProblem(c.rules, depth + 1); + if (nested) return nested; + continue; // a group carries no match of its own + } + const m = c.match; + if (!m || typeof m !== 'object') return 'condition has no match object'; + if (typeof m.type !== 'string' || m.type === '') return 'match.type must be a non-empty string'; + if (m.type === 'regex') { + if (typeof m.value !== 'string') return 'regex match.value must be a string'; + if (m.value.length > LIMITS.maxRegexLength) return `regex longer than ${LIMITS.maxRegexLength} chars`; + } else if (typeof m.value === 'string' && m.value.length > LIMITS.maxValueLength) { + return `match.value longer than ${LIMITS.maxValueLength} chars`; + } + if (m.match) { + // array_key_value nests a sub-match; count it toward depth so a chain can't be unbounded. + const nested = conditionsProblem([{ match: m.match }], depth + 1); + if (nested) return nested; + } + } + return null; +} diff --git a/tests/protect/egress-dns.test.ts b/tests/protect/egress-dns.test.ts index a2a64b5..18768f6 100644 --- a/tests/protect/egress-dns.test.ts +++ b/tests/protect/egress-dns.test.ts @@ -48,7 +48,10 @@ describe('egress DNS-rebinding screen (node:http)', () => { it('allows and pins a hostname that resolves to a permitted address', async () => { const http = await nodeHttp(); const server = http.createServer((_req: any, res: any) => res.end('ok')); - await new Promise((r) => server.listen(0, '127.0.0.1', r)); + // Some sandboxed/CI environments refuse to bind a listener (EPERM). That's an environment limit, + // not a product failure — skip rather than fail or hang. + const bound = await listenOrSkip(server); + if (!bound) return; const { port } = server.address(); try { // 127.* is permitted by this predicate, so the pinned resolution reaches the local server. @@ -74,7 +77,10 @@ describe('egress DNS-rebinding screen (node:http)', () => { it('does not screen when dnsScreen is disabled (our resolver is never wired in)', async () => { const http = await nodeHttp(); const server = http.createServer((_req: any, res: any) => res.end('ok')); - await new Promise((r) => server.listen(0, '127.0.0.1', r)); + // Some sandboxed/CI environments refuse to bind a listener (EPERM). That's an environment limit, + // not a product failure — skip rather than fail or hang. + const bound = await listenOrSkip(server); + if (!bound) return; const { port } = server.address(); let called = false; try { @@ -102,3 +108,19 @@ describe('egress DNS-rebinding screen (node:http)', () => { } }); }); + +// Bind a loopback listener, returning false when the environment forbids it (EPERM in some sandboxes). +async function listenOrSkip(server: any): Promise { + return new Promise((resolve) => { + const onError = () => resolve(false); + server.once('error', onError); + try { + server.listen(0, '127.0.0.1', () => { + server.removeListener('error', onError); + resolve(true); + }); + } catch { + resolve(false); + } + }); +} diff --git a/tests/protect/rule-validation.test.ts b/tests/protect/rule-validation.test.ts new file mode 100644 index 0000000..aa932d8 --- /dev/null +++ b/tests/protect/rule-validation.test.ts @@ -0,0 +1,106 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { validateBundle, LIMITS } from '../../src/protect/rules/validate.js'; +import { normalizeBundle } from '../../src/protect/rules/source.js'; +import { resolveApiBase } from '../../src/protect/firewall-log.js'; +import { _testExports } from '../../src/protect/engine/engine.js'; + +// Delivered rules are policy fetched over the network and executed on every request, so the bundle is +// validated before the engine sees it: bounded size/nesting/pattern length, known phases + actions, and +// a rejected rule is REPORTED (never silently "loaded" while protecting nothing). + +const ok = (over: Record = {}) => ({ + id: 'r1', + rule_v2: [{ parameter: 'raw', match: { type: 'contains', value: '__proto__' } }], + ...over, +}); + +describe('validateBundle', () => { + it('keeps a well-formed rule untouched', () => { + const { bundle, rejected } = validateBundle({ firewall: [ok()], whitelists: [] }); + expect(rejected).toEqual([]); + expect(bundle.firewall).toHaveLength(1); + }); + + it.each([ + ['unknown phase', ok({ phase: 'sideways' }), /unknown phase/], + ['unknown action', ok({ action: 'destroy' }), /unknown action/], + ['empty rule_v2', ok({ rule_v2: [] }), /empty/], + ['non-array rule_v2', ok({ rule_v2: 'nope' }), /must be an array/], + ['condition without match', ok({ rule_v2: [{ parameter: 'raw' }] }), /no match object/], + ['bad max_bytes', ok({ max_bytes: -1 }), /max_bytes/], + ])('rejects %s with a reason', (_label, rule, reason) => { + const { bundle, rejected } = validateBundle({ firewall: [rule as any], whitelists: [] }); + expect(bundle.firewall).toHaveLength(0); + expect(rejected[0].reason).toMatch(reason); + expect(rejected[0].id).toBe('r1'); + }); + + it('rejects an over-long regex and deep nesting', () => { + const longRe = ok({ rule_v2: [{ parameter: 'raw', match: { type: 'regex', value: '/' + 'a'.repeat(LIMITS.maxRegexLength + 5) + '/' } }] }); + expect(validateBundle({ firewall: [longRe], whitelists: [] }).rejected[0].reason).toMatch(/regex longer/); + + let nested: any = { parameter: 'raw', match: { type: 'contains', value: 'x' } }; + for (let i = 0; i < LIMITS.maxNestingDepth + 3; i++) nested = { parameter: 'rules', rules: [nested] }; + expect(validateBundle({ firewall: [ok({ rule_v2: [nested] })], whitelists: [] }).rejected[0].reason).toMatch(/nesting deeper/); + }); + + it('caps the number of rules rather than accepting an unbounded bundle', () => { + const many = Array.from({ length: LIMITS.maxRules + 3 }, (_, i) => ok({ id: `r${i}` })); + const { bundle, rejected } = validateBundle({ firewall: many, whitelists: [] }); + expect(bundle.firewall).toHaveLength(LIMITS.maxRules); + expect(rejected).toHaveLength(3); + expect(rejected[0].reason).toMatch(/maxRules/); + }); + + it('validates whitelists too (a malformed one would suppress real rules)', () => { + const { bundle, rejected } = validateBundle({ firewall: [], whitelists: [{ rule_id: 'r1', rule_v2: [] } as any] }); + expect(bundle.whitelists).toHaveLength(0); + expect(rejected[0].reason).toMatch(/whitelist/); + }); +}); + +describe('normalizeBundle reports rejections', () => { + it('drops invalid rules and reports each one', () => { + const seen: any[] = []; + const out = normalizeBundle( + { firewall: [ok(), ok({ id: 'bad', phase: 'nope' })], whitelists: [] } as any, + { onRuleRejected: (r: any) => seen.push(r) }, + ); + expect(out.firewall.map((r: any) => r.id)).toEqual(['r1']); + expect(seen).toEqual([expect.objectContaining({ id: 'bad', reason: expect.stringMatching(/unknown phase/) })]); + }); +}); + +describe('regex pattern length backstop', () => { + it('refuses to compile an absurdly long pattern', () => { + const { safeRegExp } = _testExports as any; + expect(safeRegExp('/' + 'a'.repeat(2000) + '/')).toBeNull(); + expect(safeRegExp('/AKIA[0-9A-Z]{16}/')).not.toBeNull(); + }); +}); + +describe('telemetry API origin', () => { + const prev = process.env.PATCHSTACK_API_BASE; + afterEach(() => { + if (prev === undefined) delete process.env.PATCHSTACK_API_BASE; + else process.env.PATCHSTACK_API_BASE = prev; + vi.restoreAllMocks(); + }); + + it('accepts an https override', () => { + process.env.PATCHSTACK_API_BASE = 'https://api.example.com'; + expect(resolveApiBase(undefined)).toBe('https://api.example.com'); + }); + + it('accepts localhost http for local testing', () => { + process.env.PATCHSTACK_API_BASE = 'http://localhost:8080'; + expect(resolveApiBase(undefined)).toBe('http://localhost:8080'); + }); + + it('refuses a plaintext remote origin (api-key exfiltration path) and warns', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + process.env.PATCHSTACK_API_BASE = 'http://evil.example.com'; + expect(resolveApiBase(undefined)).not.toBe('http://evil.example.com'); + expect(warn).toHaveBeenCalled(); + }); +}); From b9787d44ee20a5ee37dfb4e441a4a26b8e90c260 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 13 Aug 2026 13:36:23 +0200 Subject: [PATCH 2/2] protect: restrict the RULE endpoint override to safe origins too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit hardened only the telemetry API base, but the rule endpoint is the more security-relevant of the two: rules are policy the engine executes on every request, so an injected PATCHSTACK_PULSE_RULES_URL / PATCHSTACK_WAF_API_URL (or a baseUrl passed by a compromised config) could remove protection wholesale by serving an empty bundle, or serve a deliberately expensive ruleset. Both rule clients now accept a non-default base only when it is https — localhost is permitted so local development, the Pulse-chain demo and tests keep working — otherwise they warn once and fall back to the default origin. Extracted the check into src/protect/safe-origin.js so the rule clients and the telemetry reporter share one policy instead of duplicating it. Co-Authored-By: Claude Opus 4.8 --- src/protect/engine/client.js | 4 +++- src/protect/engine/pulse-client.js | 5 +++- src/protect/firewall-log.js | 14 ++---------- src/protect/safe-origin.js | 33 +++++++++++++++++++++++++++ tests/protect/rule-validation.test.ts | 33 +++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 14 deletions(-) create mode 100644 src/protect/safe-origin.js diff --git a/src/protect/engine/client.js b/src/protect/engine/client.js index c81cf14..1c0847a 100644 --- a/src/protect/engine/client.js +++ b/src/protect/engine/client.js @@ -1,3 +1,5 @@ +import { safeBaseUrl } from '../safe-origin.js'; + const DEFAULT_BASE_URL = 'https://api.patchstack.com'; const DEFAULT_CACHE_TTL = 300_000; // Randomly shorten the effective TTL by up to this fraction so many long-lived clients don't all @@ -18,7 +20,7 @@ export class PatchstackRuleClient { constructor({ token, baseUrl, cacheTtl, etag } = {}) { this.#token = token ?? process.env.PATCHSTACK_WAF_TOKEN; - this.#baseUrl = baseUrl ?? process.env.PATCHSTACK_WAF_API_URL ?? DEFAULT_BASE_URL; + this.#baseUrl = safeBaseUrl(baseUrl ?? process.env.PATCHSTACK_WAF_API_URL, DEFAULT_BASE_URL, 'rule endpoint'); this.#cacheTtl = Number.isFinite(cacheTtl) && cacheTtl > 0 ? cacheTtl : DEFAULT_CACHE_TTL; this.#etag = etag ?? null; diff --git a/src/protect/engine/pulse-client.js b/src/protect/engine/pulse-client.js index 371fbbc..3f618f4 100644 --- a/src/protect/engine/pulse-client.js +++ b/src/protect/engine/pulse-client.js @@ -1,3 +1,5 @@ +import { safeBaseUrl } from '../safe-origin.js'; + const DEFAULT_BASE_URL = 'https://api.patchstack.com/monitor/pulse'; const DEFAULT_CACHE_TTL = 300_000; // Randomly shorten the effective TTL by up to this fraction so many long-lived clients don't all @@ -24,7 +26,8 @@ export class PulseRuleClient { constructor({ siteUuid, baseUrl, cacheTtl, etag } = {}) { this.#siteUuid = siteUuid ?? process.env.PATCHSTACK_SITE_UUID; - this.#baseUrl = baseUrl ?? process.env.PATCHSTACK_PULSE_RULES_URL ?? DEFAULT_BASE_URL; + // Rules are executed policy — refuse a plaintext remote override (see safe-origin.js). + this.#baseUrl = safeBaseUrl(baseUrl ?? process.env.PATCHSTACK_PULSE_RULES_URL, DEFAULT_BASE_URL, 'rule endpoint'); this.#cacheTtl = Number.isFinite(cacheTtl) && cacheTtl > 0 ? cacheTtl : DEFAULT_CACHE_TTL; this.#etag = etag ?? null; if (!this.#siteUuid) { diff --git a/src/protect/firewall-log.js b/src/protect/firewall-log.js index 29f3e1d..7733a3b 100644 --- a/src/protect/firewall-log.js +++ b/src/protect/firewall-log.js @@ -1,3 +1,4 @@ +import { isSafeOrigin } from './safe-origin.js'; // Fire-and-forget reporter: Connect runtime → existing connector POST /api/logs/log // (same path WordPress uses). Auth: WP-style api_key (`{secret}-{oauth.id}`) → // POST /oauth/token (client_credentials) → Bearer JWT on /api/logs/log. @@ -27,17 +28,6 @@ export function parseApiKey(apiKey) { * Derive api.patchstack.com origin from a Pulse manifest/rules URL override. * @param {string | undefined} pulseOrManifestUrl */ -// https, or an explicit local origin for development. Anything else is refused as an api-key target. -function isSafeApiOrigin(value) { - try { - const u = new URL(value); - if (u.protocol === 'https:') return true; - return u.protocol === 'http:' && (u.hostname === 'localhost' || u.hostname === '127.0.0.1' || u.hostname === '::1'); - } catch { - return false; - } -} - export function resolveApiBase(pulseOrManifestUrl) { const fromEnv = typeof process !== 'undefined' ? process.env?.PATCHSTACK_API_BASE : undefined; if (typeof fromEnv === 'string' && fromEnv.length > 0) { @@ -45,7 +35,7 @@ export function resolveApiBase(pulseOrManifestUrl) { // would be a credential-exfiltration path. Require HTTPS (localhost excepted for local testing); // anything else falls back to the default origin rather than shipping the key off-platform. const candidate = fromEnv.replace(/\/$/, ''); - if (isSafeApiOrigin(candidate)) return candidate; + if (isSafeOrigin(candidate)) return candidate; // eslint-disable-next-line no-console console.warn( '[patchstack] ignoring PATCHSTACK_API_BASE: block-log reporting requires an https origin ' + diff --git a/src/protect/safe-origin.js b/src/protect/safe-origin.js new file mode 100644 index 0000000..bc07dd0 --- /dev/null +++ b/src/protect/safe-origin.js @@ -0,0 +1,33 @@ +// Which origins this guard is willing to talk to. Both of its remote conversations are security +// sensitive: the RULE endpoint delivers policy the engine then executes on every request (an +// attacker-controlled endpoint could remove protection wholesale or serve a CPU-expensive ruleset), and +// the telemetry endpoint receives the site api_key. Env/CI injection is the realistic threat, so a +// non-default override must be https — with localhost permitted so local development and tests work. +export function isSafeOrigin(value) { + try { + const u = new URL(value); + if (u.protocol === 'https:') return true; + return u.protocol === 'http:' && (u.hostname === 'localhost' || u.hostname === '127.0.0.1' || u.hostname === '[::1]' || u.hostname === '::1'); + } catch { + return false; + } +} + +/** + * Accept `candidate` only if it is a safe origin; otherwise warn once and fall back to `fallback`. + * @param {string|undefined} candidate @param {string} fallback @param {string} label + */ +export function safeBaseUrl(candidate, fallback, label) { + if (typeof candidate !== 'string' || candidate === '') return fallback; + if (isSafeOrigin(candidate)) return candidate; + warnOnce(label, `[patchstack] ignoring unsafe ${label} override (${candidate}): must be https (or localhost). Using the default.`); + return fallback; +} + +const warned = new Set(); +function warnOnce(key, message) { + if (warned.has(key)) return; + warned.add(key); + // eslint-disable-next-line no-console + console.warn(message); +} diff --git a/tests/protect/rule-validation.test.ts b/tests/protect/rule-validation.test.ts index aa932d8..b4c91d5 100644 --- a/tests/protect/rule-validation.test.ts +++ b/tests/protect/rule-validation.test.ts @@ -104,3 +104,36 @@ describe('telemetry API origin', () => { expect(warn).toHaveBeenCalled(); }); }); + +describe('rule endpoint origin', () => { + // Rules are POLICY the engine executes on every request, so an attacker-controlled endpoint could + // remove protection wholesale (empty bundle) or serve an expensive ruleset — a stronger threat than + // the telemetry key. A non-default override must be https (localhost allowed for dev/tests). + it('refuses a plaintext remote rule endpoint and falls back to the default', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const seen: string[] = []; + vi.stubGlobal('fetch', vi.fn(async (u: any) => { + seen.push(String(u)); + return new Response(JSON.stringify({ firewall: [], whitelists: [], whitelist_keys: {} }), { status: 200 }); + })); + const { PulseRuleClient } = await import('../../src/protect/engine/pulse-client.js'); + await new PulseRuleClient({ siteUuid: 's1', baseUrl: 'http://evil.example.com/pulse' }).getRules(); + expect(seen[0]).toContain('https://api.patchstack.com'); // default, not the injected origin + expect(warn).toHaveBeenCalled(); + vi.restoreAllMocks(); + }); + + it('still accepts https and localhost rule endpoints', async () => { + const seen: string[] = []; + vi.stubGlobal('fetch', vi.fn(async (u: any) => { + seen.push(String(u)); + return new Response(JSON.stringify({ firewall: [], whitelists: [], whitelist_keys: {} }), { status: 200 }); + })); + const { PulseRuleClient } = await import('../../src/protect/engine/pulse-client.js'); + await new PulseRuleClient({ siteUuid: 's1', baseUrl: 'https://x.test/monitor/pulse' }).getRules(); + await new PulseRuleClient({ siteUuid: 's2', baseUrl: 'http://127.0.0.1:8080' }).getRules(); + expect(seen[0]).toContain('https://x.test'); + expect(seen[1]).toContain('http://127.0.0.1:8080'); + vi.restoreAllMocks(); + }); +});