diff --git a/skills/doorman/references/cloudflare.md b/skills/doorman/references/cloudflare.md index 103f0af..722ea64 100644 --- a/skills/doorman/references/cloudflare.md +++ b/skills/doorman/references/cloudflare.md @@ -60,24 +60,26 @@ Doorman translates its unified rule format (`conditions`/`enabled`/`action: {typ Cloudflare supports all 16 unified condition fields: -| Doorman Field | Cloudflare Field | Notes | -| -------------- | ----------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | -| `ip` | `ip.src` | | -| `country` | `ip.geoip.country` | | -| `region` | `ip.geoip.subdivision_1` | Client geo subdivision (e.g. `"CA"`) — see the legacy-field note below for how this differs from Vercel's own `region` | -| `city` | `ip.geoip.city` | | -| `asn` | `ip.geoip.asnum` | | -| `path` | `http.request.uri.path` | | -| `host` | `http.host` | | -| `method` | `http.request.method` | | -| `header` | `http.request.headers["key"]` | Requires `key` (the header name) | -| `query` | `http.request.uri.query` | ⚠️ `key` is currently ignored — matches the whole query string, not one parameter ([doorman#263](https://github.com/gfargo/doorman/issues/263)) | -| `cookie` | `http.cookie["key"]` | Requires `key` (the cookie name) | -| `user_agent` | `http.user_agent` | | -| `referer` | `http.referer` | | -| `scheme` | `ssl` (boolean) | | -| `port` | `cf.edge.server_port` | | -| `threat_score` | `cf.threat_score` | Cloudflare bot/attack threat score (0-100) | +| Doorman Field | Cloudflare Field | Notes | +| -------------- | --------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------- | +| `ip` | `ip.src` | | +| `country` | `ip.geoip.country` | | +| `region` | `ip.geoip.subdivision_1` | Client geo subdivision (e.g. `"CA"`) — see the legacy-field note below for how this differs from Vercel's own `region` | +| `city` | `ip.geoip.city` | | +| `asn` | `ip.geoip.asnum` | | +| `path` | `http.request.uri.path` | | +| `host` | `http.host` | | +| `method` | `http.request.method` | | +| `header` | `any(http.request.headers["key"][*] op value)` / `has_key(...)` | Requires `key` (the header name, lowercased before compiling) — see the keyed-field note below | +| `query` | `http.request.uri.query`, or `any(http.request.uri.args["key"][*] op value)` when keyed | Unkeyed matches the whole query string; `key` scopes to one parameter — see the keyed-field note below | +| `cookie` | `http.cookie`, or `any(http.request.cookies["key"][*] op value)` when keyed | Unkeyed matches the whole `Cookie` header; keyed requires Cloudflare Pro/Business/Enterprise — see the keyed-field note below | +| `user_agent` | `http.user_agent` | | +| `referer` | `http.referer` | | +| `scheme` | `ssl` (boolean) | | +| `port` | `cf.edge.server_port` | | +| `threat_score` | `cf.threat_score` | Cloudflare bot/attack threat score (0-100) | + +**Keyed `header`/`cookie`/`query` conditions** ([doorman#269](https://github.com/gfargo/doorman/issues/269)): `http.request.headers`, `http.request.cookies`, and (when keyed) `http.request.uri.args` all type as Cloudflare's `Map>`, not a scalar `String` — indexing one yields an `Array`, so a bare `field["key"] eq "value"` is an Array-vs-String type mismatch the Cloudflare API rejects. Doorman compiles a keyed condition to the idiom Cloudflare's Ruleset Engine actually documents instead: `any(field["key"][*] value)` for a value comparison, `has_key(field, "key")` for `exists`/`not_exists`. A `header` condition always requires `key` — there's no "all headers as one value" fallback the way `cookie`'s unkeyed `http.cookie` is. `http.request.cookies` requires Cloudflare Pro, Business, or Enterprise; doorman emits it regardless of plan and lets Cloudflare's API reject it on an unsupported plan, the same policy already applied to `matches`/regex below. ### Operator Mapping @@ -134,7 +136,7 @@ The `RuleTranslator` surfaces warnings when a translation is lossy: - **Unmapped managed-ruleset override action** — an `action`/`overrides[].action` in `managedRules` with no Cloudflare equivalent (only `log`/`deny`/`challenge`/`allow` map cleanly) is dropped with a warning - **Negation edge cases** — complex negated conditions may produce subtly different behavior in Wirefilter -Doorman does **not** currently warn on the `rate_limit`/`redirect` omitted-config-object cases noted in Action Mapping above, or the `query`-condition `key`-ignored case noted in Field Mapping above ([doorman#263](https://github.com/gfargo/doorman/issues/263)) — both are known gaps, not something `doorman validate` catches today. Double-check those specifically rather than relying on validation output. +Doorman does **not** currently warn on the `rate_limit`/`redirect` omitted-config-object cases noted in Action Mapping above — a known gap, not something `doorman validate` catches today. Double-check those specifically rather than relying on validation output. Run `doorman validate` to surface the warnings that do exist before deploying — it auto-detects Cloudflare from the config's `provider` field. diff --git a/src/lib/providers/cloudflare/__tests__/CloudflareRuleScenarios.test.ts b/src/lib/providers/cloudflare/__tests__/CloudflareRuleScenarios.test.ts index c6f1f23..a5e1ffc 100644 --- a/src/lib/providers/cloudflare/__tests__/CloudflareRuleScenarios.test.ts +++ b/src/lib/providers/cloudflare/__tests__/CloudflareRuleScenarios.test.ts @@ -440,15 +440,21 @@ describe('Cloudflare Rule Scenarios', () => { type: 'challenge', }, conditions: [ + // The header name belongs in `key`, not folded into `value` — a + // header condition with no `key` has no Cloudflare translation at + // all (there's no "match any header" concept) and now throws + // rather than silently producing a comparison against the literal + // string this fixture used to pass as `value` (#269). { field: 'header', + key: 'X-Forwarded-For', operator: 'eq', - value: 'X-Forwarded-For: suspicious-proxy', + value: 'suspicious-proxy', }, { field: 'header', + key: 'X-Real-IP', operator: 'not_exists', - value: 'X-Real-IP', }, ], } diff --git a/src/lib/translators/ExpressionBuilder.ts b/src/lib/translators/ExpressionBuilder.ts index 5e53260..2e9b744 100644 --- a/src/lib/translators/ExpressionBuilder.ts +++ b/src/lib/translators/ExpressionBuilder.ts @@ -12,12 +12,28 @@ import { ipAddressSchema } from '../schemas/commonSchemas' const UNQUOTED_IP_FIELDS = new Set(['ip.src']) /** - * Cloudflare's indexable query-args field. Distinct from - * `http.request.uri.query` (the whole query string, a scalar `String`) — - * this one is a `Map>` keyed by argument name, used only when - * a query condition carries a `key`. See `buildKeyedQueryExpression`. + * Cloudflare fields that type as `Map>` rather than a scalar + * `String` — indexing one yields an `Array`, so a naive + * `field["key"] eq "value"` is an Array-vs-String type mismatch the + * Cloudflare API rejects (verified against Cloudflare's Ruleset Engine + * field + function references, #263/#269). The correct idiom is + * `any(field["key"][*] value)` for comparisons, `has_key(field, "key")` + * for existence — see `buildKeyedMapExpression`. + * + * `QUERY_ARGS_FIELD` is distinct from `http.request.uri.query` (the whole + * query string, a scalar `String`); `HEADERS_FIELD` is Cloudflare's real + * `header` field regardless of keying (there's no separate "all headers as + * one value" concept, unlike cookie); `COOKIES_MAP_FIELD` is distinct from + * `http.cookie` (the whole Cookie header as one scalar `String`, still used + * for a non-keyed cookie condition) — `http.request.cookies` requires + * Cloudflare Pro/Business/Enterprise (Ruleset Engine field reference), so + * doorman emits it regardless and lets Cloudflare's API reject it on an + * unsupported plan, the same policy already applied to `matches`/regex + * (see cloudflare.md). */ const QUERY_ARGS_FIELD = 'http.request.uri.args' +const HEADERS_FIELD = 'http.request.headers' +const COOKIES_MAP_FIELD = 'http.request.cookies' /** * Builds Cloudflare wirefilter expressions from structured conditions @@ -111,15 +127,32 @@ export class ExpressionBuilder { * Build expression from a single unified condition */ public static fromUnifiedCondition(condition: UnifiedCondition): string { - // A keyed query condition can't reuse the generic bracket-index path - // below the way header/cookie do: Cloudflare's indexable query-args - // field (`http.request.uri.args`, aliased as QUERY_ARGS_FIELD) types as - // `Map>`, so `args["key"] eq "value"` is an Array-vs-String - // type mismatch the Cloudflare API rejects — it needs `any(args["key"][*] - // eq "value")` (and `has_key(...)` for exists/not_exists) instead. See - // buildKeyedQueryExpression. + // Keyed query/cookie conditions, and header conditions (always keyed — + // see the throw below), can't reuse the generic bracket-index path + // further down: their Cloudflare fields are `Map>` (see + // the field-constants comment above `QUERY_ARGS_FIELD`), so they need + // `buildKeyedMapExpression`'s `any(...)`/`has_key(...)` construct + // instead of a bare bracket comparison. if (condition.key && condition.field === 'query') { - return this.buildKeyedQueryExpression(condition, condition.key) + return this.buildKeyedMapExpression(QUERY_ARGS_FIELD, condition.key, condition) + } + if (condition.field === 'header') { + if (!condition.key) { + // Unlike `cookie` below, `header` has no scalar "all headers as one + // string" field to fall back to — Cloudflare's header field is a + // Map, full stop — so a header condition genuinely needs to name + // which header it means. Fail loudly rather than silently emit + // `http.request.headers eq "..."`, comparing a Map against a String. + throw new Error('A "header" condition requires a key naming the header') + } + // Cloudflare's header map is keyed by lowercased header name (Ruleset + // Engine field reference, verified #269) — a mixed-case key would + // silently never match otherwise, since `any()` over a missing map + // entry is simply false, not an error. + return this.buildKeyedMapExpression(HEADERS_FIELD, condition.key.toLowerCase(), condition) + } + if (condition.key && condition.field === 'cookie') { + return this.buildKeyedMapExpression(COOKIES_MAP_FIELD, condition.key, condition) } const baseField = this.mapUnifiedFieldToCloudflare(condition.field) @@ -132,16 +165,8 @@ export class ExpressionBuilder { `Unsupported condition field '${condition.field}' for Cloudflare — filter it out with a warning before calling fromUnifiedCondition (see unifiedToCloudflare).`, ) } - // `key` only makes sense as a bracket index for header/cookie fields - // (matching FieldMapper's Vercel-side behavior) — a header or cookie - // condition's key must not fall through to the headers field regardless - // of which of the two it actually is. - const field = - condition.key && (condition.field === 'header' || condition.field === 'cookie') - ? `${baseField}["${escapeWirefilterString(condition.key)}"]` - : baseField - let expression = this.buildUnifiedExpression(field, condition.operator, condition.value) + let expression = this.buildUnifiedExpression(baseField, condition.operator, condition.value) if (condition.negated) { expression = `not (${expression})` @@ -151,31 +176,30 @@ export class ExpressionBuilder { } /** - * Build a keyed query-parameter expression against Cloudflare's - * `Map>`-typed `http.request.uri.args` field — see the - * comment in `fromUnifiedCondition` for why this can't share the generic - * field-string path the way header/cookie conditions do. Mirrors - * Cloudflare's own documented idioms: `any(args["key"][*] value)` for - * value comparisons (a query param can repeat, so this matches if *any* - * occurrence satisfies the operator) and `has_key(args, "key")` for - * existence. + * Builds a keyed comparison/exists expression against a Cloudflare + * `Map>`-typed field's keyed entry — query args, headers, + * or (Pro+) per-cookie values. See the field-constants comment above + * `QUERY_ARGS_FIELD`. Mirrors Cloudflare's own documented idioms: + * `any(field["key"][*] value)` for value comparisons (an entry can + * repeat, so this matches if *any* occurrence satisfies the operator) and + * `has_key(field, "key")` for existence. */ - private static buildKeyedQueryExpression(condition: UnifiedCondition, key: string): string { + private static buildKeyedMapExpression(mapField: string, key: string, condition: UnifiedCondition): string { const escapedKey = escapeWirefilterString(key) - const keyedField = `${QUERY_ARGS_FIELD}["${escapedKey}"]` + const keyedField = `${mapField}["${escapedKey}"]` let expression: string if (condition.operator === 'exists') { - expression = `has_key(${QUERY_ARGS_FIELD}, "${escapedKey}")` + expression = `has_key(${mapField}, "${escapedKey}")` } else if (condition.operator === 'not_exists') { - expression = `not (has_key(${QUERY_ARGS_FIELD}, "${escapedKey}"))` + expression = `not (has_key(${mapField}, "${escapedKey}"))` } else if (condition.operator === 'not_contains') { - expression = `not (any(${keyedField}[*] contains ${this.formatValue(QUERY_ARGS_FIELD, condition.value)}))` + expression = `not (any(${keyedField}[*] contains ${this.formatValue(mapField, condition.value)}))` } else if (condition.operator === 'not_in') { - expression = `not (any(${keyedField}[*] in ${this.formatValue(QUERY_ARGS_FIELD, condition.value)}))` + expression = `not (any(${keyedField}[*] in ${this.formatValue(mapField, condition.value)}))` } else { const operator = this.mapUnifiedOperator(condition.operator) - expression = `any(${keyedField}[*] ${operator} ${this.formatValue(QUERY_ARGS_FIELD, condition.value)})` + expression = `any(${keyedField}[*] ${operator} ${this.formatValue(mapField, condition.value)})` } return condition.negated ? `not (${expression})` : expression diff --git a/src/lib/translators/WirefilterParser.ts b/src/lib/translators/WirefilterParser.ts index 8353dab..43a1360 100644 --- a/src/lib/translators/WirefilterParser.ts +++ b/src/lib/translators/WirefilterParser.ts @@ -12,15 +12,21 @@ import { orGroupsToConditions } from './orGroupsToConditions' * subset `ExpressionBuilder` can produce (field/op/value comparisons, * `exists`, `not (...)` wrapping a single comparison/exists, `and`/`or` * joins, one level of AND-within-groups OR-across-groups nesting, bracket - * key access for header/cookie, `{...}` set values, quoted-string - * escaping). Anything outside that subset — a hand-authored expression, or - * one from another tool — is reported as unsupported (`null`) rather than - * guessed at, so a caller can fall back to a clearly-flagged lossy - * conversion instead of silently misrepresenting what the rule matches. + * key access for a plain field, `any(field["key"][*] op value)`/ + * `has_key(field, "key")` for a keyed query/header/cookie condition (#269), + * `{...}` set values, quoted-string escaping). Anything outside that + * subset — a hand-authored expression, or one from another tool — is + * reported as unsupported (`null`) rather than guessed at, so a caller can + * fall back to a clearly-flagged lossy conversion instead of silently + * misrepresenting what the rule matches. */ -// wirefilter field path -> unified condition field. Exact inverse of -// ExpressionBuilder's private `mapUnifiedFieldToCloudflare` — keep in sync. +// wirefilter field path -> unified condition field. Inverse of +// ExpressionBuilder's private `mapUnifiedFieldToCloudflare` table, plus the +// keyed `Map>` fields `buildKeyedMapExpression` uses inside +// `any(...)`/`has_key(...)` calls (`http.request.uri.args`, +// `http.request.cookies` — see parseAnyCall/parseHasKeyCall) — keep all of +// this in sync with ExpressionBuilder. const CLOUDFLARE_FIELD_TO_UNIFIED: Record = { 'ip.src': 'ip', 'ip.geoip.country': 'country', @@ -32,11 +38,14 @@ const CLOUDFLARE_FIELD_TO_UNIFIED: Record = { 'http.request.method': 'method', 'http.request.headers': 'header', 'http.request.uri.query': 'query', + 'http.request.uri.args': 'query', 'http.cookie': 'cookie', + 'http.request.cookies': 'cookie', 'http.user_agent': 'user_agent', 'http.referer': 'referer', ssl: 'scheme', 'cf.edge.server_port': 'port', + 'cf.threat_score': 'threat_score', } const COMPARISON_OPERATORS = new Set([ @@ -75,6 +84,16 @@ function tokenize(expression: string): Token[] { continue } + // Argument separator inside a function call (`has_key(field, "key")`) + // — the only place a bare `,` appears in anything ExpressionBuilder + // generates. No dedicated token type needed: skipped exactly like + // whitespace, since parseHasKeyCall consumes its two arguments + // positionally rather than validating comma placement. + if (ch === ',') { + i++ + continue + } + if (ch === '(') { tokens.push({ type: 'LPAREN', value: ch }) i++ @@ -135,9 +154,12 @@ function tokenize(expression: string): Token[] { // A maximal run of anything that isn't whitespace or a structural // character — covers field paths, keywords, operators, numbers, and - // bare (unquoted) IP/CIDR literals uniformly. + // bare (unquoted) IP/CIDR literals uniformly. `,` is structural too + // (see above) so a run stops before it instead of swallowing it, e.g. + // `has_key(http.request.headers, "x")` tokenizes the field as + // `http.request.headers`, not `http.request.headers,`. let j = i - while (j < len && !/[\s(){}[\]"]/.test(expression[j]!)) { + while (j < len && !/[\s(){}[\]",]/.test(expression[j]!)) { j++ } if (j === i) { @@ -244,7 +266,64 @@ function parseValue(stream: WirefilterTokenStream): string | number | (string | return parseValueSingle(stream) } +/** + * Parses `any(FIELD["key"][*] OP VALUE)` — the wirefilter idiom for a value + * comparison against a keyed entry of a `Map>`-typed field + * (header/cookie/query — see ExpressionBuilder.buildKeyedMapExpression, + * #263/#269). Produces the same `comparison` node shape the plain + * bracket-index path in `parseComparisonOrExists` already does, so + * `leafToCondition`/`isLeaf`/`orGroupsToConditions` need no changes to + * understand it — negation (`not (any(...))`) falls out of the existing + * top-level `not (...)` handling in `parseUnary` for free, since this is + * still ordinary recursive descent. + */ +function parseAnyCall(stream: WirefilterTokenStream): WirefilterNode { + stream.next() // 'any', already confirmed by the caller's stream.is check + stream.expect('LPAREN') + const fieldToken = stream.expect('WORD') + stream.expect('LBRACKET') + const keyToken = stream.expect('STRING') + stream.expect('RBRACKET') + stream.expect('LBRACKET') + const star = stream.expect('WORD') + if (star.value !== '*') { + throw new ParseError(`Expected '*' in any(...) array index but got '${star.value}'`) + } + stream.expect('RBRACKET') + + const opToken = stream.next() + if (opToken.type !== 'WORD' || !COMPARISON_OPERATORS.has(opToken.value)) { + throw new ParseError(`Unsupported or unrecognized operator '${opToken.value}' inside any(...)`) + } + const value = parseValue(stream) + stream.expect('RPAREN') + + return { type: 'comparison', field: fieldToken.value, key: keyToken.value, operator: opToken.value, value } +} + +/** + * Parses `has_key(FIELD, "key")` — the wirefilter idiom for an existence + * check against a keyed entry of a `Map>`-typed field. See + * `parseAnyCall`. + */ +function parseHasKeyCall(stream: WirefilterTokenStream): WirefilterNode { + stream.next() // 'has_key', already confirmed by the caller's stream.is check + stream.expect('LPAREN') + const fieldToken = stream.expect('WORD') + const keyToken = stream.expect('STRING') // the tokenizer skips the separating ',' like whitespace + stream.expect('RPAREN') + + return { type: 'exists', field: fieldToken.value, key: keyToken.value } +} + function parseComparisonOrExists(stream: WirefilterTokenStream): WirefilterNode { + if (stream.is('WORD', 'any')) { + return parseAnyCall(stream) + } + if (stream.is('WORD', 'has_key')) { + return parseHasKeyCall(stream) + } + const { field, key } = parseFieldRef(stream) if (stream.is('WORD', 'exists')) { diff --git a/src/lib/translators/__tests__/ExpressionBuilder.test.ts b/src/lib/translators/__tests__/ExpressionBuilder.test.ts index c311fbf..a9e7907 100644 --- a/src/lib/translators/__tests__/ExpressionBuilder.test.ts +++ b/src/lib/translators/__tests__/ExpressionBuilder.test.ts @@ -356,14 +356,29 @@ describe('ExpressionBuilder', () => { expect(result).toBe('not (http.request.uri.path eq "/public")') }) - it('handles header conditions with key', () => { + it('handles header conditions with key as any(...) against http.request.headers, lowercased (#269)', () => { + // http.request.headers is Map> (Cloudflare Ruleset + // Engine field reference) — indexing it yields Array, so a + // bare `headers["Authorization"] eq "Bearer token"` is an + // Array-vs-String type mismatch; `any(...[*] eq ...)` is the type-valid + // idiom. Cloudflare's header map is also keyed by lowercased header + // name, so the key is lowercased before it's compiled in. const result = ExpressionBuilder.fromUnifiedCondition({ field: 'header', operator: 'eq', value: 'Bearer token', key: 'Authorization', }) - expect(result).toBe('http.request.headers["Authorization"] eq "Bearer token"') + expect(result).toBe('any(http.request.headers["authorization"][*] eq "Bearer token")') + }) + + it('throws when a header condition has no key — there is no "all headers as one value" fallback (#269)', () => { + // Unlike cookie (http.cookie, a scalar String), Cloudflare's header + // field is a Map, full stop — a header condition genuinely needs to + // name which header it means. + expect(() => ExpressionBuilder.fromUnifiedCondition({ field: 'header', operator: 'eq', value: 'x' })).toThrow( + /header.*requires a key/, + ) }) it('escapes quotes in a unified header key so it cannot break out of the field reference', () => { @@ -373,10 +388,31 @@ describe('ExpressionBuilder', () => { value: 'x', key: 'x"] or (true) or http.request.headers["x', }) - expect(result).toBe('http.request.headers["x\\"] or (true) or http.request.headers[\\"x"] eq "x"') + expect(result).toBe('any(http.request.headers["x\\"] or (true) or http.request.headers[\\"x"][*] eq "x")') expect(result).not.toMatch(/headers\["[^"\\]*"\] or/) }) + it('builds a header not_contains expression as a positive any(...) wrapped in not(...) (#269)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'header', + operator: 'not_contains', + value: '1.2.3.4', + key: 'X-Forwarded-For', + }) + expect(result).toBe('not (any(http.request.headers["x-forwarded-for"][*] contains "1.2.3.4"))') + }) + + it('wraps a negated header condition in an outer not(...) around the any(...) expression (#269)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'header', + operator: 'eq', + value: '1', + key: 'X-Debug', + negated: true, + }) + expect(result).toBe('not (any(http.request.headers["x-debug"][*] eq "1"))') + }) + it('escapes backslashes in string values so a trailing backslash cannot consume the closing quote', () => { const result = ExpressionBuilder.fromUnifiedCondition({ field: 'path', @@ -386,14 +422,47 @@ describe('ExpressionBuilder', () => { expect(result).toBe('http.request.uri.path eq "a\\\\"') }) - it('handles cookie conditions with key as http.cookie, not http.request.headers', () => { + it('handles keyed cookie conditions as any(...) against http.request.cookies, not http.cookie (#269)', () => { + // http.cookie is a scalar String (the whole Cookie header) with no Map + // to index — http.request.cookies is Cloudflare's actual + // Map> per-cookie field (Ruleset Engine field + // reference; requires Pro/Business/Enterprise, same policy as + // matches/regex — doorman emits it regardless). const result = ExpressionBuilder.fromUnifiedCondition({ field: 'cookie', operator: 'eq', value: 'abc123', key: 'session_id', }) - expect(result).toBe('http.cookie["session_id"] eq "abc123"') + expect(result).toBe('any(http.request.cookies["session_id"][*] eq "abc123")') + }) + + it('does not lowercase the cookie key (unlike header — Cloudflare only documents lowercasing for headers) (#269)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'cookie', + operator: 'eq', + value: 'x', + key: 'SessionId', + }) + expect(result).toContain('["SessionId"]') + }) + + it('leaves an unkeyed cookie condition matching the whole Cookie header via http.cookie (no regression, #269)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'cookie', + operator: 'contains', + value: 'session=', + }) + expect(result).toBe('http.cookie contains "session="') + }) + + it('builds a valueless exists expression for a keyed cookie condition via has_key (#269)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'cookie', + operator: 'exists', + key: 'session_id', + } as UnifiedCondition) + expect(result).toBe('has_key(http.request.cookies, "session_id")') }) it('escapes quotes in a unified cookie key so it cannot break out of the field reference', () => { @@ -403,7 +472,7 @@ describe('ExpressionBuilder', () => { value: 'x', key: 'a" or true or http.cookie["a', }) - expect(result).toBe('http.cookie["a\\" or true or http.cookie[\\"a"] eq "x"') + expect(result).toBe('any(http.request.cookies["a\\" or true or http.cookie[\\"a"][*] eq "x")') }) it('scopes a keyed query condition to that argument via http.request.uri.args, not the whole query string', () => { @@ -481,24 +550,28 @@ describe('ExpressionBuilder', () => { expect(result).toBe('any(http.request.uri.args["a\\" or true or http.request.uri.args[\\"a"][*] eq "x")') }) - it('builds a valueless exists expression (regression test for #85)', () => { + it('builds a valueless exists expression via has_key, not a bracket-exists (regression test for #85, updated for #269)', () => { // `value` is omitted here the same way RuleTranslator's Vercel->unified // conversion leaves it undefined for exists/not_exists conditions. + // `field["key"] exists` was never valid wirefilter for a Map-typed + // field like http.request.headers — has_key(field, "key") is the + // documented idiom (Ruleset Engine function reference), same as #263 + // already established for keyed query conditions. const result = ExpressionBuilder.fromUnifiedCondition({ field: 'header', operator: 'exists', key: 'x-api-version', } as UnifiedCondition) - expect(result).toBe('http.request.headers["x-api-version"] exists') + expect(result).toBe('has_key(http.request.headers, "x-api-version")') }) - it('builds a valueless not_exists expression wrapped in not(...) (regression test for #85)', () => { + it('builds a valueless not_exists expression wrapped in not(...) (regression test for #85, updated for #269)', () => { const result = ExpressionBuilder.fromUnifiedCondition({ field: 'header', operator: 'not_exists', key: 'x-api-version', } as UnifiedCondition) - expect(result).toBe('not (http.request.headers["x-api-version"] exists)') + expect(result).toBe('not (has_key(http.request.headers, "x-api-version"))') expect(result).not.toContain('undefined') expect(result).not.toContain('not exists') }) diff --git a/src/lib/translators/__tests__/WirefilterParser.test.ts b/src/lib/translators/__tests__/WirefilterParser.test.ts index a8e4359..89f5eb8 100644 --- a/src/lib/translators/__tests__/WirefilterParser.test.ts +++ b/src/lib/translators/__tests__/WirefilterParser.test.ts @@ -197,14 +197,20 @@ describe('parseWirefilterExpression', () => { expect(parsed!.conditions[0]).toMatchObject({ field: 'user_agent', operator: 'not_contains', value: 'bot' }) }) - it('round-trips a header condition with a bracket key', () => { + // #269: a keyed header condition now compiles to any(...), not a bare + // bracket comparison — see the `any(...)/has_key(...)` describe block + // below for the full grammar coverage. The key comes back lowercased, + // matching Cloudflare's own lowercase-keyed header map (verified via + // the Ruleset Engine field reference) — not a bug, an inherent property + // of what a real Cloudflare zone would report back too. + it('round-trips a header condition with a bracket key, lowercased', () => { const conditions: UnifiedCondition[] = [{ field: 'header', key: 'X-Custom', operator: 'eq', value: 'value' }] const expression = ExpressionBuilder.fromUnifiedConditions(conditions, 'AND') const parsed = parseWirefilterExpression(expression) expect(parsed).not.toBeNull() - expect(parsed!.conditions[0]).toMatchObject({ field: 'header', key: 'X-Custom', value: 'value' }) + expect(parsed!.conditions[0]).toMatchObject({ field: 'header', key: 'x-custom', value: 'value' }) }) it('round-trips an "in" condition with an array value', () => { @@ -218,6 +224,129 @@ describe('parseWirefilterExpression', () => { }) }) + // #269: the parser previously had no concept of wirefilter function-call + // syntax at all — `any(field["key"][*] op value)` and + // `has_key(field, "key")` (the idiom #263 already shipped for keyed query + // conditions, and #269 extends to header/cookie) tokenized as a bare + // field named "any"/"has_key" followed by an unexpected `(`, so parsing + // always threw and `cloudflareToUnified` fell back to its "could not + // parse" warning with empty conditions. That means the #263 fix, live + // since it shipped, could never actually round-trip a keyed query + // condition back from a real Cloudflare zone — verified directly below, + // not just inferred. + describe('any(...)/has_key(...) function-call syntax (#269)', () => { + it('parses a direct any(...) value comparison', () => { + const result = parseWirefilterExpression('any(http.request.headers["content-type"][*] eq "application/json")') + expect(result).not.toBeNull() + expect(result!.conditions[0]).toMatchObject({ + field: 'header', + key: 'content-type', + operator: 'eq', + value: 'application/json', + }) + }) + + it('parses a direct has_key(...) existence check', () => { + const result = parseWirefilterExpression('has_key(http.request.headers, "x-api-version")') + expect(result).not.toBeNull() + expect(result!.conditions[0]).toMatchObject({ field: 'header', key: 'x-api-version', operator: 'exists' }) + }) + + it('parses not (has_key(...)) as not_exists', () => { + const result = parseWirefilterExpression('not (has_key(http.request.headers, "x-api-version"))') + expect(result).not.toBeNull() + expect(result!.conditions[0]).toMatchObject({ field: 'header', key: 'x-api-version', operator: 'not_exists' }) + }) + + it('parses not (any(field[*] contains value)) as the dedicated not_contains operator', () => { + const result = parseWirefilterExpression( + 'not (any(http.request.headers["x-forwarded-for"][*] contains "1.2.3.4"))', + ) + expect(result).not.toBeNull() + expect(result!.conditions[0]).toMatchObject({ + field: 'header', + key: 'x-forwarded-for', + operator: 'not_contains', + value: '1.2.3.4', + }) + }) + + it('parses not (any(...)) with no dedicated not_X form as negated: true', () => { + const result = parseWirefilterExpression('not (any(http.request.headers["x-debug"][*] eq "1"))') + expect(result).not.toBeNull() + expect(result!.conditions[0]).toMatchObject({ + field: 'header', + key: 'x-debug', + operator: 'eq', + value: '1', + negated: true, + }) + }) + + it('parses a keyed cookie any(...) against http.request.cookies', () => { + const result = parseWirefilterExpression('any(http.request.cookies["session"][*] eq "abc123")') + expect(result).not.toBeNull() + expect(result!.conditions[0]).toMatchObject({ field: 'cookie', key: 'session', value: 'abc123' }) + }) + + it('rejects any(...) with something other than * inside the array index', () => { + expect(parseWirefilterExpression('any(http.request.headers["x"][0] eq "y")')).toBeNull() + }) + + it('rejects a bare identifier call that is neither any nor has_key (fails closed, not guessed at)', () => { + expect(parseWirefilterExpression('lower(http.host) eq "example.com"')).toBeNull() + }) + + describe('round-trip fidelity against ExpressionBuilder', () => { + it('round-trips a keyed header value comparison', () => { + const conditions: UnifiedCondition[] = [ + { field: 'header', key: 'Content-Type', operator: 'eq', value: 'application/json' }, + ] + const expression = ExpressionBuilder.fromUnifiedConditions(conditions, 'AND') + const parsed = parseWirefilterExpression(expression) + + expect(parsed).not.toBeNull() + expect(parsed!.conditions[0]).toMatchObject({ field: 'header', key: 'content-type', value: 'application/json' }) + }) + + it('round-trips a header exists/not_exists pair', () => { + const exists = parseWirefilterExpression( + ExpressionBuilder.fromUnifiedConditions([{ field: 'header', key: 'X-Trace', operator: 'exists' }]), + ) + expect(exists!.conditions[0]).toMatchObject({ field: 'header', key: 'x-trace', operator: 'exists' }) + + const notExists = parseWirefilterExpression( + ExpressionBuilder.fromUnifiedConditions([{ field: 'header', key: 'X-Trace', operator: 'not_exists' }]), + ) + expect(notExists!.conditions[0]).toMatchObject({ field: 'header', key: 'x-trace', operator: 'not_exists' }) + }) + + it('round-trips a keyed cookie condition', () => { + const conditions: UnifiedCondition[] = [{ field: 'cookie', key: 'session', operator: 'eq', value: 'abc123' }] + const expression = ExpressionBuilder.fromUnifiedConditions(conditions, 'AND') + const parsed = parseWirefilterExpression(expression) + + expect(parsed).not.toBeNull() + expect(parsed!.conditions[0]).toMatchObject({ field: 'cookie', key: 'session', value: 'abc123' }) + }) + + // Regression test for the pre-existing gap this investigation found: + // #263 shipped the any()/has_key() builder side for keyed query + // conditions, but the parser side was never updated to match — so + // `doorman download`/`diff`/`status` against a real Cloudflare zone + // could never actually read a keyed query condition back. Fixed as a + // side effect of building the same grammar support for #269. + it('round-trips a keyed query condition (previously unparseable — see #263)', () => { + const conditions: UnifiedCondition[] = [{ field: 'query', key: 'debug', operator: 'eq', value: '1' }] + const expression = ExpressionBuilder.fromUnifiedConditions(conditions, 'AND') + const parsed = parseWirefilterExpression(expression) + + expect(parsed).not.toBeNull() + expect(parsed!.conditions[0]).toMatchObject({ field: 'query', key: 'debug', value: '1' }) + }) + }) + }) + describe('fail-closed OR branches (#252)', () => { // Shared with CelParser via orGroupsToConditions — a silently-dropped OR // branch would change what the rule matches (e.g. `A or B` parsing back