From aef7eb13ad51a2fbac556ef4a5acf15597b96fc7 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 20 Aug 2026 09:05:10 +0200 Subject: [PATCH 1/2] Read the host out of a URL for the internal_host match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by running the whole chain on a real app: a vulnerability detected on a site, with a reachability flow and a vPatch template, generated a rule pinned to the app's own parameter (`get.url` on `/api/preview`), served it — and the rule matched nothing at all. `internal_host` classifies its value AS a hostname. That is right for the egress phase, where the value is the destination host, and wrong for the request phase, where the same question arrives as an application parameter whose value is a full URL. `isInternalHost('http://169.254.169.254/…')` is false, so every request-phase SSRF rule was expressible, servable, and permanently inert — the exact failure this engine has been hardened against repeatedly, in the one match type meant to prevent it. Six of fifteen destinations were wrong before this, every one a false negative. The match now extracts a host first: a URL (any scheme, including protocol-relative) is parsed and its hostname classified; `host:port` and `[v6]:port` lose the port; a bare host passes through untouched, so the egress path and the built-in default rule behave exactly as before. Parsed rather than sliced, deliberately. Userinfo puts a trusted-looking name before the real host (`http://api.stripe.example@169.254.169.254/` must match) and a fragment puts one after it (`http://evil.example/#@127.0.0.1` must not). A substring check reads the wrong host in both directions — a bypass in the first case, a false positive in the second — and the slice-instead-of- parse mutation fails on exactly those two. Classification itself is unchanged, so every canonicalisation defence still applies to what comes out: decimal/hex IPv4, expanded and IPv4-mapped IPv6, trailing dots. Verified end to end after the fix, against the rule the platform actually served: as served it detects and does not block (per-rule `dry-run` overriding the site's block mode, as designed); with enforcement promoted it returns 403; a third-party URL on the same route passes; an internal URL on a different route passes, so the route scope holds. 1216 tests, typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- src/protect/engine/engine.js | 56 +++++++++++++++++++++++++++++++++--- tests/protect/engine.test.ts | 46 +++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 4 deletions(-) diff --git a/src/protect/engine/engine.js b/src/protect/engine/engine.js index a025330..e10ef81 100644 --- a/src/protect/engine/engine.js +++ b/src/protect/engine/engine.js @@ -171,11 +171,58 @@ function warnUnsupportedMatchType(type) { ); } -// Internal / private / loopback / link-local / cloud-metadata host check, used by the -// `internal_host` match type for SSRF egress rules. It CANONICALIZES the host before classifying — +// Internal / private / loopback / link-local / cloud-metadata host check behind the `internal_host` +// match type. It CANONICALIZES the host before classifying — // a textual/prefix check is bypassable by alternate encodings (decimal/hex/octal IPv4, expanded or // IPv4-mapped IPv6), which is a classic SSRF evasion. Handles localhost / *.local / GCP metadata // names, every IPv4 spelling inet_aton accepts, and IPv6 loopback/link-local/unique-local/mapped. +/** + * The host to classify out of a rule parameter's value. + * + * `internal_host` was written for the egress phase, where the value IS the destination host. On the + * request phase the same question arrives as an application parameter, and there the value is almost + * always a full URL (`?url=http://169.254.169.254/latest/meta-data/`) or a `host:port` pair — neither of + * which is a hostname, so classifying the raw string answered "not internal" for every one of them. A + * request-phase SSRF rule was therefore expressible, servable and permanently inert: the exact failure + * this engine has been repeatedly hardened against, in the one match type meant to prevent it. + * + * Only the host is extracted; the classification itself is unchanged, so every canonicalization defence + * (decimal/hex IPv4, expanded and IPv4-mapped IPv6, trailing dots) still applies to what comes out. A + * value that is already a bare host passes through untouched, which is what keeps the egress path and + * the built-in default rule behaving exactly as before. + */ +function hostFromValue(value) { + const raw = String(value ?? '').trim(); + if (raw === '') return ''; + + // A scheme (`http://`, and deliberately any other) or a protocol-relative URL. Parsing rather than + // string-slicing is what makes the userinfo evasion (`http://trusted@169.254.169.254/`) resolve to the + // host actually contacted, and keeps `http://evil.com#@127.0.0.1` resolving to evil.com. + if (/^[a-z][a-z0-9+.-]*:\/\//i.test(raw) || raw.startsWith('//')) { + try { + return new URL(raw.startsWith('//') ? `http:${raw}` : raw).hostname; + } catch { + // Unparseable: hand the raw value on, where the host check rejects it rather than guessing. + return raw; + } + } + + // `[::1]:8080` — bracketed IPv6 with or without a port. + if (raw.startsWith('[')) { + const end = raw.indexOf(']'); + if (end > 0) return raw.slice(1, end); + } + + // `169.254.169.254:80`. Only a single colon followed by digits: a bare IPv6 address has several, and + // must not have its last group mistaken for a port. + const colon = raw.indexOf(':'); + if (colon > 0 && raw.indexOf(':', colon + 1) === -1 && /^\d+$/.test(raw.slice(colon + 1))) { + return raw.slice(0, colon); + } + + return raw; +} + function isInternalHost(hostname) { if (!hostname) return false; let host = String(hostname).toLowerCase().replace(/^\[|\]$/g, ''); @@ -502,8 +549,9 @@ export function matchValue(type, value, matchVal, matchObj) { } case 'internal_host': - // SSRF egress: private / loopback / link-local / cloud-metadata destinations. - return isInternalHost(strValue); + // SSRF: private / loopback / link-local / cloud-metadata destinations. The value may be a bare + // host (egress) or a URL / host:port in an application parameter (request) — see `hostFromValue`. + return isInternalHost(hostFromValue(strValue)); case 'quotes': // engine-php exposes `inline_js_xss` as an alias of `quotes`. diff --git a/tests/protect/engine.test.ts b/tests/protect/engine.test.ts index c416804..942d155 100644 --- a/tests/protect/engine.test.ts +++ b/tests/protect/engine.test.ts @@ -123,6 +123,52 @@ describe('RuleEngine', () => { } }); + it('should match internal_host when the value is a URL, not a bare host', () => { + // The gap this closes: `internal_host` was written for the egress phase, where the value IS the + // destination host. On the request phase the same question arrives as an application parameter and + // the value is a full URL — so a served, correctly-pinned SSRF rule matched nothing at all. + for (const value of [ + 'http://169.254.169.254/latest/meta-data/', + 'http://localhost:3000/admin', + 'https://127.0.0.1/x', + 'http://[::1]/x', + 'http://metadata.google.internal/computeMetadata/v1/', + '//10.0.0.5/x', + '169.254.169.254:80', + '[::1]:8080', + ]) { + assert.strictEqual(matchValue('internal_host', value, null), true, `${value} should be internal`); + } + + for (const value of [ + 'https://api.stripe.example/v1/charges', + 'http://8.8.8.8/resolve', + 'how to use localhost in docker', + 'https://example.com/?next=/admin', + ]) { + assert.strictEqual(matchValue('internal_host', value, null), false, `${value} should be external`); + } + }); + + it('should read the host a URL actually contacts, not the one it advertises', () => { + // Why the host is parsed rather than sliced out of the string. Userinfo puts a trusted-looking name + // before the real host, and a fragment puts one after it; a substring check reads the wrong one in + // both directions, which is a bypass in the first case and a false positive in the second. + assert.strictEqual(matchValue('internal_host', 'http://api.stripe.example@169.254.169.254/', null), true); + assert.strictEqual(matchValue('internal_host', 'http://evil.example/#@127.0.0.1', null), false); + assert.strictEqual(matchValue('internal_host', 'http://evil.example/?next=http://127.0.0.1/', null), false); + }); + + it('should leave a bare host classified exactly as before', () => { + // The egress path and the built-in default rule pass `egress.host`, which is already a hostname. + // Extraction must be a no-op for those, or this change would alter what a live guard blocks today. + assert.strictEqual(matchValue('internal_host', '169.254.169.254', null), true); + assert.strictEqual(matchValue('internal_host', '::1', null), true); + assert.strictEqual(matchValue('internal_host', '2130706433', null), true); // decimal 127.0.0.1 + assert.strictEqual(matchValue('internal_host', 'example.com', null), false); + assert.strictEqual(matchValue('internal_host', '', null), false); + }); + it('should match quotes (and the inline_js_xss alias)', () => { assert.strictEqual(matchValue('quotes', `x' OR 1=1`, null), true); assert.strictEqual(matchValue('quotes', 'no quotes here', null), false); From b1fc7f794acdf367b8bc9efc5eec8117e912dfdc Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 20 Aug 2026 09:14:21 +0200 Subject: [PATCH 2/2] Regression-test the generated rule as a composition, not a matcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The matcher test that came with the fix exercises `matchValue()` directly, and the defect was not a matcher in isolation: it was a rule bound to the right parameter, scoped to the right route, carrying the right per-rule enforcement, arriving at the runtime intact, and never firing. Every part was individually correct and every unit test passed. `pulse-chain.test.ts` cannot catch that. It covers the same transport with a static lodash rule whose conditions read `raw` — no route scope, no `get.*` source, and a match type that had only ever been exercised on the egress path. So the pinned shape now has its own chain test, fetching from a mock Pulse and enforcing through the HTTP guard, with the four assertions that separate "protecting" from "present": detected in dry-run, 403 once promoted, a third-party destination on the same route allowed, and an internal destination on another route allowed. The fixture is captured from a real `GET pulse/rules/{uuid}` response and carries the template beside the served copy, so the substitution is visible rather than described — and the unbound template is asserted to be inert, which is the other half of the same failure: `` is not a parameter source, so a rule that reached an app with placeholders intact would load, report as shipped protection, and never match. Writing it turned up a second silent widening. The engine's scope key is `when.path`; the first draft wrote `when.route`, which is not an error — the scope is ignored and the rule applies to every request. Fail-open is right for a scope that cannot be EVALUATED, but a scope that cannot be UNDERSTOOD is an authoring mistake with the opposite consequence: for a blocking rule it is a false-positive surface across the whole app instead of one endpoint. The engine now warns once and says which keys it understands; behaviour is unchanged, and the widening is pinned by a test so it is documented rather than rediscovered. 1221 tests, typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- src/protect/engine/engine.js | 18 ++ .../fixtures/generated-pinned-rule.json | 44 ++++ tests/protect/generated-rule-chain.test.ts | 211 ++++++++++++++++++ 3 files changed, 273 insertions(+) create mode 100644 tests/protect/fixtures/generated-pinned-rule.json create mode 100644 tests/protect/generated-rule-chain.test.ts diff --git a/src/protect/engine/engine.js b/src/protect/engine/engine.js index e10ef81..f8f1b86 100644 --- a/src/protect/engine/engine.js +++ b/src/protect/engine/engine.js @@ -329,10 +329,28 @@ function expandIPv6(host) { return out; } +// Report a `when` block that names nothing this engine understands. Fail-open is correct for a scope that +// cannot be EVALUATED, but a scope that cannot be UNDERSTOOD is an authoring mistake with the opposite +// consequence: the rule silently applies to every request instead of one route, which for a blocking rule +// is a false-positive surface across the whole app. Warned once so it is discoverable in a log. +const warnedScopes = new Set(); +function warnUnrecognisedScope(when) { + const key = Object.keys(when).sort().join(','); + if (warnedScopes.has(key)) return; + warnedScopes.add(key); + console.warn( + `[patchstack] Rule scope \`when: { ${key} }\` names no supported key — the engine understands ` + + `\`method\` and \`path\`. The scope is IGNORED and the rule applies to every request.` + ); +} + // Route/method scope for a rule's optional `when: { method, path }`. Fail-open: if the scope can't // be evaluated, the rule still applies (never silently suppress a rule). function ruleAppliesTo(when, resolver) { try { + if (when.method === undefined && when.path === undefined && Object.keys(when).length > 0) { + warnUnrecognisedScope(when); + } if (when.method) { const methods = (Array.isArray(when.method) ? when.method : [when.method]).map((m) => String(m).toUpperCase()); const actual = String(resolver.resolve('server.REQUEST_METHOD')[0] ?? 'GET').toUpperCase(); diff --git a/tests/protect/fixtures/generated-pinned-rule.json b/tests/protect/fixtures/generated-pinned-rule.json new file mode 100644 index 0000000..02f6359 --- /dev/null +++ b/tests/protect/fixtures/generated-pinned-rule.json @@ -0,0 +1,44 @@ +{ + "note": "Captured from a platform `GET pulse/rules/{uuid}` response for an app whose attack-surface map proved request input reaching an HTTP client. This is the SHAPE a coordinate-pinned rule arrives in — the parameter and route are the app's own, bound from a template at generation time. No advisory identifier: which advisories are shielded is not a public list.", + "shape": { + "routeScopedInRuleV2": "The platform scopes the route with an ANDed `server.REQUEST_URI contains` condition rather than a `when` block. Both forms are exercised by the suite, because the scope moving into `when` must not silently drop the scope.", + "enforcement": "A generated rule arrives dry-run regardless of the site's mode, and only promotion flips it. The per-rule value overriding a site-wide `block` is the safety property that keeps an unproven pinned rule from blocking traffic.", + "boundPlaceholders": "`` and `` are substituted before serving. A rule that reached an app with them intact would load and never match — `` is not a parameter source — which is indistinguishable from protection until someone fires an exploit at it." + }, + "served": { + "id": "pulse-1", + "title": "Block internal-host URLs reaching the vulnerable HTTP client", + "rule_v2": [ + { + "match": { "type": "contains", "value": "/api/preview" }, + "inclusive": true, + "mutations": ["urldecode"], + "parameter": "server.REQUEST_URI" + }, + { + "match": { "type": "internal_host" }, + "inclusive": true, + "mutations": ["urldecode"], + "parameter": "get.url" + } + ], + "enforcement": "dry-run" + }, + "template": { + "note": "The same rule before binding, as authored. Kept beside the served copy so the substitution is visible rather than described, and so a test can prove the unbound form is inert.", + "rule_v2": [ + { + "match": { "type": "contains", "value": "" }, + "inclusive": true, + "mutations": ["urldecode"], + "parameter": "server.REQUEST_URI" + }, + { + "match": { "type": "internal_host" }, + "inclusive": true, + "mutations": ["urldecode"], + "parameter": "" + } + ] + } +} diff --git a/tests/protect/generated-rule-chain.test.ts b/tests/protect/generated-rule-chain.test.ts new file mode 100644 index 0000000..f92075d --- /dev/null +++ b/tests/protect/generated-rule-chain.test.ts @@ -0,0 +1,211 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { createProtection } from '../../src/protect/runtime.js'; + +/** + * The COORDINATE-PINNED rule chain: a rule generated from an app's own attack-surface map, served by + * Pulse, and enforced through the HTTP guard. + * + * Why this exists as its own file. The defect it guards against was not a broken matcher — it was a + * composition: a rule bound to the right parameter, scoped to the right route, carrying the right + * per-rule enforcement, arriving at the runtime intact, and never firing. Every part was individually + * correct and every unit test passed. `pulse-chain.test.ts` covers the same transport with a STATIC + * lodash rule whose conditions read `raw`, so it cannot see a failure in the pinned shape: different + * parameter sources, a route scope, and a match type that was only ever exercised on the egress path. + * + * What broke, concretely: `internal_host` classified its value as a hostname, which is what the egress + * phase hands it. In an application parameter the value is a full URL, so the rule matched nothing for + * every request-phase SSRF rule the platform could generate. It was found by firing an exploit at a + * served rule, not by a test — hence this file. + * + * The four assertions are the ones that distinguish "protecting" from "present": + * detected in dry-run · 403 once promoted · external URL allowed · internal URL on another route allowed. + */ +const FIXTURE = JSON.parse( + readFileSync(join(dirname(fileURLToPath(import.meta.url)), 'fixtures', 'generated-pinned-rule.json'), 'utf8'), +); + +/** The rule exactly as the platform serves it — route scope inside `rule_v2`, enforcement `dry-run`. */ +const servedRule = FIXTURE.served; + +/** + * The same coverage with the scope expressed as `when`, the other form the engine supports. + * + * The key is `path`, not `route` — worth stating, because writing `route` here is not an error: the scope + * is simply ignored and the rule applies to every request. That is how the first draft of this test + * passed on the exploit and then also detected on a route it was scoped away from. + */ +const whenScopedRule = { + id: 'pulse-1-when', + title: servedRule.title, + when: { path: '/api/preview' }, + enforcement: 'dry-run', + rule_v2: [servedRule.rule_v2[1]], +}; + +/** A scope nobody can honour: the key is not one the engine knows, so the rule is unscoped. */ +const misspelledScopeRule = { + id: 'pulse-1-misspelled-scope', + title: servedRule.title, + when: { route: '/api/preview' }, + enforcement: 'dry-run', + rule_v2: [servedRule.rule_v2[1]], +}; + +/** The template before binding. Must be inert: `` is not a parameter source. */ +const unboundTemplate = { id: 'pulse-1-unbound', title: servedRule.title, rule_v2: FIXTURE.template.rule_v2 }; + +/** + * A mock Pulse endpoint whose per-rule enforcement can be flipped, which is how promotion reaches a + * running guard. The site stays in `block` throughout: the point is that a generated rule's own + * `dry-run` overrides it until promotion, so a site-wide mode change cannot promote a rule by accident. + */ +function mockPulse(rule: Record) { + const state = { enforcement: 'dry-run', etag: '"v1"' }; + const fetchMock = vi.fn(async (_url: string, init?: RequestInit) => { + const inm = (init?.headers as Record | undefined)?.['If-None-Match']; + if (inm === state.etag) return new Response(null, { status: 304, headers: { ETag: state.etag } }); + return new Response( + JSON.stringify({ + firewall: [{ ...rule, enforcement: state.enforcement }], + whitelists: [], + whitelist_keys: {}, + enforcement: 'block', + }), + { status: 200, headers: { 'Content-Type': 'application/json', ETag: state.etag } }, + ); + }); + return { state, fetchMock }; +} + +const req = (url: string) => new Request(url, { method: 'GET' }); +const appHandler = async () => new Response(JSON.stringify({ ok: true }), { status: 200 }); + +/** The exploit: an internal metadata address in the parameter the map proved reaches the HTTP client. */ +const SSRF = 'https://app.demo/api/preview?url=http://169.254.169.254/latest/meta-data/'; +/** The same route, a legitimate third-party destination. */ +const THIRD_PARTY = 'https://app.demo/api/preview?url=https://api.stripe.example/v1/charges'; +/** An internal destination on a route this rule is NOT scoped to. */ +const OTHER_ROUTE = 'https://app.demo/api/orders?url=http://169.254.169.254/'; + +describe('generated coordinate-pinned rule, through Pulse and the HTTP guard', () => { + const prevMode = process.env.PATCHSTACK_MODE; + afterEach(() => { + if (prevMode === undefined) delete process.env.PATCHSTACK_MODE; + else process.env.PATCHSTACK_MODE = prevMode; + vi.restoreAllMocks(); + }); + + it.each([ + ['as the platform serves it (route scoped in rule_v2)', servedRule], + ['with the scope expressed as when.path', whenScopedRule], + ])('detects in dry-run and blocks once promoted — %s', async (_label, rule) => { + delete process.env.PATCHSTACK_MODE; + const { state, fetchMock } = mockPulse(rule as Record); + vi.stubGlobal('fetch', fetchMock); + const detections: Array<{ rule?: { id?: string } }> = []; + + const p = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + onDetect: (d: { rule?: { id?: string } }) => detections.push(d), + }); + + // The site is in block mode, and the rule is not. A generated rule that inherited the site's mode + // would start blocking traffic on evidence that has not been corroborated against the running build. + expect(p.mode).toBe('block'); + + // 1. DETECTED, not blocked. This is the assertion the whole file exists for: the composition fires. + const dry = await p.fetch(appHandler)(req(SSRF)); + expect(dry.status).toBe(200); + expect(detections.some((d) => d.rule?.id === (rule as { id: string }).id), 'the pinned rule must fire').toBe(true); + + // 2. A legitimate destination on the same route is untouched — the rule screens the DESTINATION, and a + // rule that blocked this would be withdrawn before it ever reached a customer. + expect((await p.fetch(appHandler)(req(THIRD_PARTY))).status).toBe(200); + expect(detections.length, 'a third-party destination must not detect').toBe(1); + + // 3. The route scope holds: same exploit, different route, no detection at all. + expect((await p.fetch(appHandler)(req(OTHER_ROUTE))).status).toBe(200); + expect(detections.length, 'the route scope must exclude other endpoints').toBe(1); + + // 4. Promotion — the platform flips this rule's own enforcement, the guard picks it up on refresh. + state.enforcement = 'block'; + state.etag = '"v2"'; + await p.refresh(); + + expect((await p.fetch(appHandler)(req(SSRF))).status).toBe(403); + // Still no false positive after promotion, which is the state that actually reaches traffic. + expect((await p.fetch(appHandler)(req(THIRD_PARTY))).status).toBe(200); + expect((await p.fetch(appHandler)(req(OTHER_ROUTE))).status).toBe(200); + + p.stopRefresh?.(); + }); + + it('warns, and applies everywhere, when a scope names no key the engine knows', async () => { + // Not hypothetical: this is the shape the first draft of this test used. `when: { route }` is silently + // unscoped — the rule then applies to every request, which for a promoted rule is a false-positive + // surface across the whole app rather than one endpoint. Fail-open is right for a scope that cannot be + // EVALUATED; a scope that cannot be UNDERSTOOD is an authoring mistake, so the engine now says so once. + delete process.env.PATCHSTACK_MODE; + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const { fetchMock } = mockPulse(misspelledScopeRule); + vi.stubGlobal('fetch', fetchMock); + const detections: unknown[] = []; + + const p = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + onDetect: (d: unknown) => detections.push(d), + }); + + // The route it was meant to be scoped to, and one it was not: both detect. + await p.fetch(appHandler)(req(SSRF)); + await p.fetch(appHandler)(req(OTHER_ROUTE)); + expect(detections.length, 'an unrecognised scope key leaves the rule unscoped').toBe(2); + + expect(warn.mock.calls.flat().join(' ')).toMatch(/scope|when/i); + + p.stopRefresh?.(); + }); + + it('is inert if the template reaches the app with its placeholders unbound', async () => { + // The other half of the same failure. `` is not a parameter source, so an unbound template + // loads, reports as a shipped rule, and can never match — and the only way to tell it apart from a + // working rule is to fire an exploit at it. Asserted so that "generation bound the coordinates" is a + // property of the chain rather than an assumption about it. + delete process.env.PATCHSTACK_MODE; + const { state, fetchMock } = mockPulse(unboundTemplate); + vi.stubGlobal('fetch', fetchMock); + const detections: unknown[] = []; + + const p = await createProtection({ + siteUuid: 'site-1', + pulseRulesUrl: 'https://x.test/monitor/pulse', + onDetect: (d: unknown) => detections.push(d), + }); + + state.enforcement = 'block'; + state.etag = '"v2"'; + await p.refresh(); + + expect((await p.fetch(appHandler)(req(SSRF))).status).toBe(200); + expect(detections.length, 'an unbound template cannot match anything').toBe(0); + + p.stopRefresh?.(); + }); + + it('binds the fixture from a real serve, with no placeholder left in it', () => { + // Guards the fixture itself: if someone regenerates it from a template rather than from a served + // response, every assertion above would still pass while testing the wrong shape. + const json = JSON.stringify(servedRule); + expect(json).not.toMatch(/|/); + expect(json).toContain('get.url'); + expect(json).toContain('/api/preview'); + expect(servedRule.enforcement).toBe('dry-run'); + // And the template half must still carry them, or the inertness test above proves nothing. + expect(JSON.stringify(FIXTURE.template)).toMatch(//); + }); +});