From 8359d8389d1f36f72adf0fc042b680604315c5c8 Mon Sep 17 00:00:00 2001 From: Griffen Fargo <3642037+gfargo@users.noreply.github.com> Date: Mon, 24 Aug 2026 16:34:06 -0400 Subject: [PATCH] fix: emit valid wirefilter for Cloudflare keyed header/cookie conditions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A keyed header condition (e.g. matching a specific Content-Type) compiled to a bare `http.request.headers["key"] eq "value"` — but that field is Map>, so indexing it yields an Array, and comparing an Array against a String literal is a type mismatch the Cloudflare API rejects. Keyed cookie conditions were worse: `http.cookie["key"]` brackets-indexes a plain scalar String field that isn't a Map at all. Verified against Cloudflare's Ruleset Engine docs (both fields' real types, and the "Require specific headers" WAF guide's actual idiom) rather than assuming: - header: any(http.request.headers["key"][*] value) / has_key(...) for exists, same construct #263 already used to fix keyed query conditions. Also lowercases the key — Cloudflare's header map keys are documented as always-lowercase, so a mixed-case key would silently never match. - cookie: same any()/has_key() construct, but pointed at the separate http.request.cookies map field instead of the scalar http.cookie (which stays as-is for the *unkeyed* case). Cookie keys are NOT lowercased — left in their original casing. fromVercelCondition/FieldMapper (the direct Vercel-native -> Cloudflare path) has the same bug plus an unfixed keyed-query case, but is unreachable from any live command as of this change — left alone with a comment explaining why, since fixing dead code protects no one. A keyed header condition no longer round-trips through WirefilterParser (it doesn't understand any()/has_key() syntax) — a deliberate, documented degradation matching the parser's existing "unsupported construct -> null, caller warns" contract, not a silent misparse. Extending the parser for the new syntax is tracked separately. --- .../cloudflare/__tests__/translator.test.ts | 23 ++++ src/lib/translators/ExpressionBuilder.ts | 114 ++++++++++++------ .../__tests__/ExpressionBuilder.test.ts | 89 ++++++++++++-- .../__tests__/WirefilterParser.test.ts | 21 +++- 4 files changed, 198 insertions(+), 49 deletions(-) diff --git a/src/lib/providers/cloudflare/__tests__/translator.test.ts b/src/lib/providers/cloudflare/__tests__/translator.test.ts index 59624e3..774abdc 100644 --- a/src/lib/providers/cloudflare/__tests__/translator.test.ts +++ b/src/lib/providers/cloudflare/__tests__/translator.test.ts @@ -148,6 +148,29 @@ describe('cloudflare/translator', () => { expect(result.expression).not.toBe('http.request.uri.query eq "1"') }) + // Regression tests for #269: keyed header/cookie conditions previously + // compiled to a bare `field["key"] eq value` — an Array-vs-String type + // mismatch (header) or an attempt to bracket-index a non-Map scalar + // field (cookie's `http.cookie`) that Cloudflare's real ruleset API + // rejects either way. See ExpressionBuilder's + // fromUnifiedCondition/buildKeyedMapExpression for the fix. + it("scopes a keyed header condition via any(...), lowercased to match Cloudflare's header-name map keys", () => { + const rule = makeUnifiedRule({ + conditions: [{ field: 'header', operator: 'eq', value: 'application/json', key: 'Content-Type' }], + }) + const { result } = unifiedToCloudflare(rule) + expect(result.expression).toBe('any(http.request.headers["content-type"][*] eq "application/json")') + }) + + it('scopes a keyed cookie condition to http.request.cookies, not the unindexable scalar http.cookie', () => { + const rule = makeUnifiedRule({ + conditions: [{ field: 'cookie', operator: 'eq', value: 'abc123', key: 'session_id' }], + }) + const { result } = unifiedToCloudflare(rule) + expect(result.expression).toBe('any(http.request.cookies["session_id"][*] eq "abc123")') + expect(result.expression).not.toContain('http.cookie[') + }) + // Regression tests for #273 Bug 1: `region` in the unified vocabulary // means the client's geo subdivision. A Vercel-originated condition // that collided with this name (fixed by renaming it to `vercel_region` diff --git a/src/lib/translators/ExpressionBuilder.ts b/src/lib/translators/ExpressionBuilder.ts index 5e53260..4a636d8 100644 --- a/src/lib/translators/ExpressionBuilder.ts +++ b/src/lib/translators/ExpressionBuilder.ts @@ -15,10 +15,32 @@ 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`. + * a query condition carries a `key`. See `buildKeyedMapExpression`. */ const QUERY_ARGS_FIELD = 'http.request.uri.args' +/** Cloudflare's `Map>` header field — same field used for both the bare and keyed header case. */ +const HEADERS_FIELD = 'http.request.headers' + +/** + * Cloudflare's indexable per-cookie field. Distinct from `http.cookie` (the + * raw `Cookie` header as a whole, a scalar `String`, used for the bare + * cookie case) — this one is a `Map>` keyed by cookie name, + * used only when a cookie condition carries a `key`. + */ +const COOKIES_MAP_FIELD = 'http.request.cookies' + +/** + * Unified/Vercel condition fields that key-scope onto one of Cloudflare's + * `Map>` fields when a `key` is present, and which Map field + * each one uses. See `buildKeyedMapExpression`. + */ +const KEYED_MAP_FIELDS: Record = { + query: QUERY_ARGS_FIELD, + header: HEADERS_FIELD, + cookie: COOKIES_MAP_FIELD, +} + /** * Builds Cloudflare wirefilter expressions from structured conditions */ @@ -26,6 +48,18 @@ export class ExpressionBuilder { /** * Build expression from Vercel condition groups * Vercel uses OR between groups, AND within groups + * + * NOTE: unreachable from any live command as of #269 (nothing in `src/` + * outside this file and its tests calls `fromVercelConditionGroups`/ + * `fromVercelCondition`/`FieldMapper` — the direct Vercel-native -> + * Cloudflare path was superseded by translating through `UnifiedCondition` + * instead). `fromVercelCondition` below has the *same* keyed-header/cookie + * bug `fromUnifiedCondition` was fixed for in #269 (still builds a bare + * `field["key"] eq value` via `FieldMapper.toCloudflare`), plus an + * unfixed keyed-`query` case (#263's original bug, on this path only — + * `FieldMapper` never special-cased `query` for key-scoping at all). Left + * as-is since fixing dead code protects no one, but if this ever gets + * wired into a live path again, it needs the same fix applied here first. */ public static fromVercelConditionGroups(conditionGroups: VercelConditionGroup[]): string { if (!conditionGroups || conditionGroups.length === 0) { @@ -111,15 +145,21 @@ 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. - if (condition.key && condition.field === 'query') { - return this.buildKeyedQueryExpression(condition, condition.key) + // A keyed query/header/cookie condition can't reuse the generic + // base-field path below: Cloudflare's indexable fields for these three + // (`http.request.uri.args`, `http.request.headers`, + // `http.request.cookies`) all type as `Map>`, so + // `field["key"] eq "value"` is an Array-vs-String type mismatch the + // Cloudflare API rejects — it needs `any(field["key"][*] eq "value")` + // (and `has_key(...)` for exists/not_exists) instead. See + // buildKeyedMapExpression. (`http.cookie`, the *bare*-cookie field used + // below, is a plain scalar String and isn't indexable at all — the keyed + // case must use the separate `http.request.cookies` map field instead.) + if (condition.key) { + const mapField = KEYED_MAP_FIELDS[condition.field] + if (mapField) { + return this.buildKeyedMapExpression(condition, condition.key, mapField) + } } const baseField = this.mapUnifiedFieldToCloudflare(condition.field) @@ -132,16 +172,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,36 +183,48 @@ 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. + * Build a keyed expression against one of Cloudflare's `Map>` + * fields — see the comment in `fromUnifiedCondition` for why query/header/ + * cookie conditions with a `key` can't share the generic base-field path. + * Mirrors Cloudflare's own documented idioms: `any(map["key"][*] + * value)` for value comparisons (a header/cookie/query-param can repeat, so + * this matches if *any* occurrence satisfies the operator) and + * `has_key(map, "key")` for existence. */ - private static buildKeyedQueryExpression(condition: UnifiedCondition, key: string): string { - const escapedKey = escapeWirefilterString(key) - const keyedField = `${QUERY_ARGS_FIELD}["${escapedKey}"]` + private static buildKeyedMapExpression(condition: UnifiedCondition, key: string, mapField: string): string { + const escapedKey = escapeWirefilterString(this.normalizeMapKey(mapField, key)) + 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 } + /** + * Cloudflare's `http.request.headers` map keys are lowercased internally + * ("the keys... are the names of HTTP request headers converted to + * lowercase" — Ruleset Engine field reference), so a header key built with + * its original casing (e.g. `Content-Type`) would silently never match a + * real request. `http.request.cookies`/`http.request.uri.args` keys are + * NOT case-normalized by Cloudflare, so those must keep their original + * casing instead. + */ + private static normalizeMapKey(mapField: string, key: string): string { + return mapField === HEADERS_FIELD ? key.toLowerCase() : key + } + /** * `exists`/`not_exists` (Vercel: `ex`/`nex`) conditions carry no value and * wirefilter has no `not exists` binary operator — negation must wrap the diff --git a/src/lib/translators/__tests__/ExpressionBuilder.test.ts b/src/lib/translators/__tests__/ExpressionBuilder.test.ts index c311fbf..6242f25 100644 --- a/src/lib/translators/__tests__/ExpressionBuilder.test.ts +++ b/src/lib/translators/__tests__/ExpressionBuilder.test.ts @@ -356,14 +356,20 @@ describe('ExpressionBuilder', () => { expect(result).toBe('not (http.request.uri.path eq "/public")') }) - it('handles header conditions with key', () => { + it("handles header conditions with key, lowercasing it to match Cloudflare's header-name map keys", () => { const result = ExpressionBuilder.fromUnifiedCondition({ field: 'header', operator: 'eq', value: 'Bearer token', key: 'Authorization', }) - expect(result).toBe('http.request.headers["Authorization"] eq "Bearer token"') + // http.request.headers is a Map> keyed by lowercased + // header name (Cloudflare's own field reference: "the keys... are the + // names of HTTP request headers converted to lowercase") — a mixed-case + // key like "Authorization" would silently never match if left as-is. + // any(...[*] eq ...) is also required: a bare `headers["authorization"] + // eq "Bearer token"` is an Array-vs-String type mismatch. + expect(result).toBe('any(http.request.headers["authorization"][*] eq "Bearer token")') }) it('escapes quotes in a unified header key so it cannot break out of the field reference', () => { @@ -373,10 +379,40 @@ 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 valueless exists expression for a keyed header condition via has_key', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'header', + operator: 'exists', + key: 'X-Api-Version', + } as UnifiedCondition) + expect(result).toBe('has_key(http.request.headers, "x-api-version")') + }) + + it('builds a not_contains expression for a keyed header condition as a positive any(...) wrapped in not(...)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'header', + operator: 'not_contains', + value: 'bot', + key: 'User-Agent', + }) + expect(result).toBe('not (any(http.request.headers["user-agent"][*] contains "bot"))') + }) + + it('wraps a negated keyed header condition in an outer not(...) around the any(...) expression', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'header', + operator: 'eq', + value: 'application/json', + key: 'Content-Type', + negated: true, + }) + expect(result).toBe('not (any(http.request.headers["content-type"][*] eq "application/json"))') + }) + it('escapes backslashes in string values so a trailing backslash cannot consume the closing quote', () => { const result = ExpressionBuilder.fromUnifiedCondition({ field: 'path', @@ -386,14 +422,30 @@ 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 cookie conditions with key against http.request.cookies, not the scalar http.cookie', () => { const result = ExpressionBuilder.fromUnifiedCondition({ field: 'cookie', operator: 'eq', value: 'abc123', key: 'session_id', }) - expect(result).toBe('http.cookie["session_id"] eq "abc123"') + // http.cookie (used for the *unkeyed* cookie case) is a scalar String — + // the raw Cookie header — and isn't indexable at all. The keyed case + // must use the separate http.request.cookies Map> field + // instead, with any(...[*] eq ...) for the same Array-vs-String reason + // as header/query. + expect(result).toBe('any(http.request.cookies["session_id"][*] eq "abc123")') + expect(result).not.toContain('http.cookie[') + }) + + it('does not lowercase a cookie key — unlike headers, Cloudflare does not case-normalize cookie names', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'cookie', + operator: 'eq', + value: 'abc123', + key: 'Session_ID', + }) + expect(result).toBe('any(http.request.cookies["Session_ID"][*] eq "abc123")') }) it('escapes quotes in a unified cookie key so it cannot break out of the field reference', () => { @@ -401,9 +453,28 @@ describe('ExpressionBuilder', () => { field: 'cookie', operator: 'eq', value: 'x', - key: 'a" or true or http.cookie["a', + key: 'a" or true or http.request.cookies["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.request.cookies[\\"a"][*] eq "x")') + }) + + it('builds a valueless not_exists expression for a keyed cookie condition wrapped in not(...)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'cookie', + operator: 'not_exists', + key: 'session_id', + } as UnifiedCondition) + expect(result).toBe('not (has_key(http.request.cookies, "session_id"))') + }) + + it('builds a not_in expression for a keyed cookie condition as a positive any(...) wrapped in not(...)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'cookie', + operator: 'not_in', + value: ['expired', 'invalid'], + key: 'session_status', + }) + expect(result).toBe('not (any(http.request.cookies["session_status"][*] in {"expired" "invalid"}))') }) it('scopes a keyed query condition to that argument via http.request.uri.args, not the whole query string', () => { @@ -489,7 +560,7 @@ describe('ExpressionBuilder', () => { 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)', () => { @@ -498,7 +569,7 @@ describe('ExpressionBuilder', () => { 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..452febf 100644 --- a/src/lib/translators/__tests__/WirefilterParser.test.ts +++ b/src/lib/translators/__tests__/WirefilterParser.test.ts @@ -197,14 +197,25 @@ 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', () => { + // A keyed header/cookie/query condition no longer round-trips through + // this parser as of #269 — ExpressionBuilder emits `any(field["key"][*] + // value)`/`has_key(field, "key")` for these (the type-valid + // Cloudflare construct; see ExpressionBuilder.buildKeyedMapExpression), + // and this parser understands only the bracket-index/`exists` grammar it + // previously produced. This is a deliberate, safe degradation matching + // the class's own documented contract (falls back to `null` — "unsupported, + // reported... rather than guessed at" — for anything outside the exact + // subset ExpressionBuilder currently generates), not a silent + // misparse — `cloudflareToUnified` already has a warning path for + // exactly this (see translator.test.ts's "falls back to empty + // conditions with a warning" test). Extending this parser to understand + // the new construct is tracked separately, not done here. + it("no longer round-trips a keyed header condition — any(...)/has_key(...) is outside this parser's grammar", () => { 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(expression).toBe('any(http.request.headers["x-custom"][*] eq "value")') + expect(parseWirefilterExpression(expression)).toBeNull() }) it('round-trips an "in" condition with an array value', () => {