From 1250b27be84f3625720ee68ae5553dc0c17e9887 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Mon, 5 Oct 2026 09:59:31 +0200 Subject: [PATCH] Add boundary-preserving request path source --- README.md | 17 +++++ rule-contract.json | 3 +- src/protect/engine/node.js | 2 + src/protect/engine/normalizer.js | 23 ++++++ src/protect/engine/pulse-client.js | 4 +- src/protect/engine/request.js | 5 +- src/protect/rules/contract.js | 4 +- tests/protect/pulse-client.test.ts | 3 + tests/protect/request-path.test.ts | 110 ++++++++++++++++++++++++++++ tests/protect/rule-contract.test.ts | 2 +- 10 files changed, 167 insertions(+), 6 deletions(-) create mode 100644 tests/protect/request-path.test.ts diff --git a/README.md b/README.md index 32229510..ab4510a3 100644 --- a/README.md +++ b/README.md @@ -484,6 +484,23 @@ console.log(result.response.stored ? 'Reported' : 'Unchanged'); Lower-level pieces are also exported: `scanLockfile`, `buildWirePayload`, `postManifest`, `resolveConfig`. +### Path-only rule matching + +Rule contract 2.12 adds `server.REQUEST_PATH`: the path is separated from the query and fragment +**before one percent-decoding pass**. Encoded `?` and `#` remain path data. Unlike `server.REQUEST_URI`, +this source does not apply iterative decoding, HTML decoding, comment stripping, whitespace changes, +or dot-segment normalization. Invalid percent encoding and unsupported request-target forms yield no +value. Additional decoding requires an explicit rule mutation justified by the application's behavior. + +The Node adapter preserves the original request target; Fetch-based runtimes expose only the URL +provided by their platform, so earlier URL canonicalization cannot be reversed. `when.path` and +existing URI rules are unchanged. For path-sensitive signatures, match the route prefix and payload +together against `server.REQUEST_PATH`. + +Authenticated rules requests advertise `X-Patchstack-Request-Path: 1`. The rules service must withhold +rules using this source from clients without that capability, and vary its response and ETag by the +resulting bundle. Update the rules service before distributing rules that require the new source. + ## What gets sent ```json diff --git a/rule-contract.json b/rule-contract.json index 3c9a5bf1..146e842b 100644 --- a/rule-contract.json +++ b/rule-contract.json @@ -1,6 +1,6 @@ { "$comment": "Generated from src/protect/rules/contract.js by scripts/emit-rule-contract.mjs. Do not edit.", - "version": "2.11", + "version": "2.12", "sources": { "raw": { "keyed": false @@ -49,6 +49,7 @@ "keyed": true, "keys": [ "REQUEST_URI", + "REQUEST_PATH", "REQUEST_METHOD", "HTTP_USER_AGENT", "HTTP_REFERER", diff --git a/src/protect/engine/node.js b/src/protect/engine/node.js index 9670264c..e3ca7733 100644 --- a/src/protect/engine/node.js +++ b/src/protect/engine/node.js @@ -12,6 +12,7 @@ import { parseBody } from './fetch.js'; import { notify } from '../notify.js'; import { parseCookieHeader } from './cookies.js'; import { appendOwn, setOwn } from './own.js'; +import { REQUEST_TARGET, requestField } from './normalizer.js'; /** * Read a Node request body, keeping at most `maxBytes` of it. @@ -125,6 +126,7 @@ export function fromNodeRequest(req, rawBody = '', options = {}) { }); return { + [REQUEST_TARGET]: requestField(req, 'originalUrl') ?? requestField(req, 'url'), method, url: uri, originalUrl: uri, diff --git a/src/protect/engine/normalizer.js b/src/protect/engine/normalizer.js index dab3208a..f9119217 100644 --- a/src/protect/engine/normalizer.js +++ b/src/protect/engine/normalizer.js @@ -8,6 +8,27 @@ import { setOwn } from './own.js'; import { parseCookieHeader } from './cookies.js'; +// Adapters preserve the transport's target before URL parsing can collapse dot segments. +export const REQUEST_TARGET = Symbol('requestTarget'); +export const REQUEST_PATH = Symbol('requestPath'); + +/** Path only, split before one percent-decoding pass. No filesystem or text normalization. */ +export function requestPath(target) { + if (typeof target !== 'string') return undefined; + if (/^https?:\/\//i.test(target)) { + target = target.replace(/^https?:\/\/[^/?#]+/i, ''); + if (target === '' || target.startsWith('?') || target.startsWith('#')) target = '/' + target; + } + if (!target.startsWith('/')) return undefined; + const path = target.split(/[?#]/, 1)[0]; + try { + return decodeURIComponent(path); + } catch { + // An invalid encoded path has no decoded representation. Do not invent a partial one. + return undefined; + } +} + const HTML_ENTITIES = { '&': '&', @@ -392,6 +413,8 @@ export function normalizeRequest(req, options = {}) { const headers = requestField(req, 'headers') || {}; return { + [REQUEST_PATH]: requestPath(requestField(req, REQUEST_TARGET) + ?? requestField(req, 'originalUrl') ?? url), query: normalizeObject(requestField(req, 'query') || {}, options), body: normalizeObject(body || {}, options), headers: normalizeObject(headers, options), diff --git a/src/protect/engine/pulse-client.js b/src/protect/engine/pulse-client.js index 88b9d69d..47b47114 100644 --- a/src/protect/engine/pulse-client.js +++ b/src/protect/engine/pulse-client.js @@ -8,6 +8,7 @@ const DEFAULT_CACHE_TTL = 300_000; const BUILD_VERDICT_HEADER = 'X-Patchstack-Build-Match'; const BUILD_IDENTITY_HEADER = 'X-Patchstack-Build-ID'; const PER_RULE_DRY_RUN_HEADER = 'X-Patchstack-Per-Rule-Dry-Run'; +const REQUEST_PATH_HEADER = 'X-Patchstack-Request-Path'; // Randomly shorten the effective TTL by up to this fraction so many long-lived clients don't all // revalidate on the same tick (spreads load / avoids a thundering herd against the rules API). const JITTER_FRACTION = 0.1; @@ -104,9 +105,10 @@ export class PulseRuleClient { if (this.#buildId !== null && typeof auth.Authorization === 'string') { headers['X-Patchstack-Build'] = this.#buildId; } - // The rules service may include detect-only rules only when this guard can honor their own mode. + // Advertise the optional rule semantics this guard can honor to the authenticated rules service. if (typeof auth.Authorization === 'string') { headers[PER_RULE_DRY_RUN_HEADER] = '1'; + headers[REQUEST_PATH_HEADER] = '1'; } if (this.#etag) headers['If-None-Match'] = this.#etag; const response = await fetch(url, { method: 'GET', headers, signal: AbortSignal.timeout(this.#timeoutMs) }); diff --git a/src/protect/engine/request.js b/src/protect/engine/request.js index 9e7c0323..8c297cfb 100644 --- a/src/protect/engine/request.js +++ b/src/protect/engine/request.js @@ -1,5 +1,5 @@ import { parseCookieHeader } from './cookies.js'; -import { decodeHtmlEntities, safeUrlDecode } from './normalizer.js'; +import { decodeHtmlEntities, safeUrlDecode, REQUEST_PATH } from './normalizer.js'; import { setOwn } from './own.js'; // Resolvable DATA attributes of an uploaded file part (files..). The engine only exposes @@ -342,6 +342,9 @@ export class RequestResolver { switch (key) { case 'REQUEST_URI': return [req.originalUrl ?? req.url ?? '/']; + case 'REQUEST_PATH': + return Object.hasOwn(req, REQUEST_PATH) && typeof req[REQUEST_PATH] === 'string' + ? [req[REQUEST_PATH]] : []; case 'REQUEST_METHOD': return [req.method ?? 'GET']; case 'HTTP_USER_AGENT': diff --git a/src/protect/rules/contract.js b/src/protect/rules/contract.js index d5a597b9..3081d93a 100644 --- a/src/protect/rules/contract.js +++ b/src/protect/rules/contract.js @@ -12,7 +12,7 @@ // `rule-contract.json` is the published form. `tests/protect/rule-contract.test.ts` reads the engine's own // source and asserts these descriptions match what it implements. -export const CONTRACT_VERSION = '2.11'; +export const CONTRACT_VERSION = '2.12'; /** * Every parameter source, and what it accepts after the dot. @@ -47,7 +47,7 @@ export const SOURCES = Object.freeze({ server: Object.freeze({ keyed: true, keys: Object.freeze([ - 'REQUEST_URI', 'REQUEST_METHOD', 'HTTP_USER_AGENT', 'HTTP_REFERER', 'HTTP_HOST', + 'REQUEST_URI', 'REQUEST_PATH', 'REQUEST_METHOD', 'HTTP_USER_AGENT', 'HTTP_REFERER', 'HTTP_HOST', 'REMOTE_ADDR', 'ip', 'CONTENT_TYPE', 'CONTENT_LENGTH', ]), key_prefixes: Object.freeze(['HTTP_']), diff --git a/tests/protect/pulse-client.test.ts b/tests/protect/pulse-client.test.ts index 4bd6d609..fe129a2f 100644 --- a/tests/protect/pulse-client.test.ts +++ b/tests/protect/pulse-client.test.ts @@ -37,6 +37,7 @@ describe('PulseRuleClient', () => { expect(fetchMock.mock.calls[0][0]).toBe('https://x.test/monitor/pulse/token'); expect(fetchMock.mock.calls[1][1].headers.Authorization).toBe('Bearer tok'); expect(fetchMock.mock.calls[1][1].headers['X-Patchstack-Per-Rule-Dry-Run']).toBe('1'); + expect(fetchMock.mock.calls[1][1].headers['X-Patchstack-Request-Path']).toBe('1'); }); it('fetches unauthenticated when no credential is configured', async () => { @@ -50,6 +51,7 @@ describe('PulseRuleClient', () => { expect(fetchMock).toHaveBeenCalledOnce(); expect(fetchMock.mock.calls[0][1].headers.Authorization).toBeUndefined(); expect(fetchMock.mock.calls[0][1].headers['X-Patchstack-Per-Rule-Dry-Run']).toBeUndefined(); + expect(fetchMock.mock.calls[0][1].headers['X-Patchstack-Request-Path']).toBeUndefined(); }); it('still fetches rules when the credential exchange fails', async () => { @@ -70,6 +72,7 @@ describe('PulseRuleClient', () => { expect(res.success).toBe(true); expect(fetchMock.mock.calls[1][1].headers.Authorization).toBeUndefined(); expect(fetchMock.mock.calls[1][1].headers['X-Patchstack-Per-Rule-Dry-Run']).toBeUndefined(); + expect(fetchMock.mock.calls[1][1].headers['X-Patchstack-Request-Path']).toBeUndefined(); }); it('fails open (success:false, empty rules) on a non-200', async () => { diff --git a/tests/protect/request-path.test.ts b/tests/protect/request-path.test.ts new file mode 100644 index 00000000..c97b79b4 --- /dev/null +++ b/tests/protect/request-path.test.ts @@ -0,0 +1,110 @@ +import { describe, expect, it } from 'vitest'; +import { RuleEngine } from '../../src/protect/engine/engine.js'; +import { fromNodeRequest } from '../../src/protect/engine/node.js'; +import { fromFetchRequest } from '../../src/protect/engine/fetch.js'; +import { normalizeRequest, REQUEST_PATH, REQUEST_TARGET } from '../../src/protect/engine/normalizer.js'; +import { RequestResolver } from '../../src/protect/engine/request.js'; +import { validateBundle } from '../../src/protect/rules/validate.js'; + +function pathOf(req: any) { + return new RequestResolver({ ...req, ...normalizeRequest(req) }).resolve('server.REQUEST_PATH'); +} + +const rule = { + id: 'synthetic-path-rule', + phase: 'request', + action: 'block', + when: { method: ['GET', 'DELETE'] }, + rule_v2: [{ + parameter: 'server.REQUEST_PATH', + match: { type: 'regex', value: String.raw`/^\/api\/documents\/(?:\.\.[\/\\]|[\s\S]*[\/\\]\.\.[\/\\])/` }, + }], +}; + +describe('server.REQUEST_PATH', () => { + it.each([ + ['/api/a?next=/../private', '/api/a'], + ['/api/a%3Fb%2F..%2Fprivate?x=1', '/api/a?b/../private'], + ['/api/a%23b%2F..%2Fprivate#ignored', '/api/a#b/../private'], + ['/api/%252e%252e%252fprivate', '/api/%2e%2e%2fprivate'], + ['/api/a+%2B%20b', '/api/a++ b'], + ['/api/a%0A%00b', '/api/a\n\0b'], + ['/api/a/*unchanged*/../b', '/api/a/*unchanged*/../b'], + ['/api/../b', '/api/&'], + ['/api/%26%2346%3B/b', '/api/./b'], + ['//api/./a/../b\\c', '//api/./a/../b\\c'], + ['https://app.test/api/a/../b?x=/../z', '/api/a/../b'], + ['HTTP://app.test?x=1', '/'], + ['https://app.test', '/'], + ['/api/%E2%9C%93', '/api/✓'], + ])('preserves path semantics for %s', (url, expected) => { + expect(pathOf({ url })).toEqual([expected]); + }); + + it.each([undefined, '', '*', 'app.test:443', 'relative', '/bad%zz', '/bad%C0%AF', '/bad%E2']) + ('does not fabricate a decoded path from %s', (url) => { + expect(pathOf({ url })).toEqual([]); + }); + + it('uses the original target before mounted middleware rewrites the URL', () => { + expect(pathOf({ originalUrl: '/mounted/a%3Fb?x=1', url: '/a%3Fb?x=1' })).toEqual(['/mounted/a?b']); + expect(pathOf(fromNodeRequest({ originalUrl: '/mounted/a%3Fb?x=1', url: '/a%3Fb?x=1' }))) + .toEqual(['/mounted/a?b']); + }); + + it('ignores inherited evidence and overwrites a supplied derived path', () => { + const req = Object.create({ url: '/fake', originalUrl: '/fake', [REQUEST_TARGET]: '/fake', [REQUEST_PATH]: '/fake' }); + expect(pathOf(req)).toEqual([]); + expect(pathOf({ url: '/real', [REQUEST_PATH]: '/fake' })).toEqual(['/real']); + }); + + it('preserves raw Node dot segments without changing the legacy URI adapter', () => { + const req = fromNodeRequest({ url: '/api/a/../b?x=1' }); + expect(req.originalUrl).toBe('/api/b?x=1'); + expect(pathOf(req)).toEqual(['/api/a/../b']); + }); + + it('leaves legacy REQUEST_URI normalization unchanged', () => { + const req = normalizeRequest({ url: '/api/a%253Fb%2F..%2Fprivate?x=1' }); + expect(new RequestResolver(req).resolve('server.REQUEST_URI')).toEqual(['/api/a?b/../private?x=1']); + expect(new RequestResolver(req).resolve('server.REQUEST_PATH')).toEqual(['/api/a%3Fb/../private']); + }); + + it('accepts the source in delivered bundles', () => { + expect(validateBundle({ firewall: [rule] }).rejected).toEqual([]); + }); + + it.each([ + ['/api/documents/..%2Fprivate', true], + ['/api/documents/a%3Fb%2F..%2F..%2Fprivate', true], + ['/api/documents/a%23b%2F..%2F..%2Fprivate', true], + ['/api/documents/a%0Ab%2F..%2Fprivate', true], + ['/api/documents/a%5C..%5Cprivate', true], + ['/api/documents/a%2F*b%2F..%2Fc*%2Fd', true], + ['/api/documents/safe?next=/../private', false], + ['/api/documents/safe?next=%2F..%2Fprivate', false], + ['/api/documents/%252e%252e%252fprivate', false], + ['/other/a%2F..%2Fprivate', false], + ['/api/documents/safe', false], + ])('evaluates %s consistently through direct, Node and Fetch adapters', async (url, blocked) => { + const engine = new RuleEngine({ firewall: [rule] }); + const requests = [ + { method: 'GET', url }, + fromNodeRequest({ method: 'GET', url }), + await fromFetchRequest(new Request(`https://app.test${url}`)), + ]; + for (const req of requests) expect(engine.evaluate(req).blocked).toBe(blocked); + }); + + it('keeps the method constraint and recognizes DELETE', () => { + const engine = new RuleEngine({ firewall: [rule] }); + const url = '/api/documents/..%2Fprivate'; + expect(engine.evaluate({ method: 'DELETE', url }).blocked).toBe(true); + expect(engine.evaluate({ method: 'POST', url }).blocked).toBe(false); + }); + + it('uses only what Fetch exposes, without claiming to recover earlier canonicalization', async () => { + const req = await fromFetchRequest(new Request('https://app.test/api/a/../b')); + expect(pathOf(req)).toEqual(['/api/b']); + }); +}); diff --git a/tests/protect/rule-contract.test.ts b/tests/protect/rule-contract.test.ts index f11184d8..875656a9 100644 --- a/tests/protect/rule-contract.test.ts +++ b/tests/protect/rule-contract.test.ts @@ -118,7 +118,7 @@ describe('the build scope the contract publishes', () => { it('states the firewall-only, detect-only-on-unusable contract', () => { const contract = ruleContract(); - expect(CONTRACT_VERSION).toBe('2.11'); + expect(CONTRACT_VERSION).toBe('2.12'); expect(contract.build_scope).toEqual({ applies_to: ['firewall'], usable: {