From a0dabe2c6a72d4a843fe3457ec9d8cfc22f9f7ba Mon Sep 17 00:00:00 2001 From: Griffen Fargo <3642037+gfargo@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:00:57 -0400 Subject: [PATCH 1/2] fix: scope keyed Cloudflare query conditions to the matched argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A query condition with a key (e.g. field: 'query', key: 'debug', matching ?debug=1 specifically) compiled to a bare http.request.uri.query comparison against the entire query string — the key-scoping check only covered header/cookie, so it was silently ignored. Wider matching than intended, with no warning. Fixing it isn't just adding query to that check: http.request.uri.query is a scalar String (the whole query string), and Cloudflare's actual indexable field, http.request.uri.args, types as Map> — a bare args["key"] eq "value" is an Array-vs-String mismatch the API rejects. Verified against Cloudflare's Ruleset Engine docs and the "Require specific headers" WAF guide for the correct idioms: any(args["key"][*] eq "value") for value comparisons (a param can repeat) and has_key(args, "key") for exists/not_exists. --- .../cloudflare/__tests__/translator.test.ts | 14 ++++ src/lib/translators/ExpressionBuilder.ts | 50 ++++++++++++++ .../__tests__/ExpressionBuilder.test.ts | 68 ++++++++++++++++++- 3 files changed, 130 insertions(+), 2 deletions(-) diff --git a/src/lib/providers/cloudflare/__tests__/translator.test.ts b/src/lib/providers/cloudflare/__tests__/translator.test.ts index f92170c..0746fb6 100644 --- a/src/lib/providers/cloudflare/__tests__/translator.test.ts +++ b/src/lib/providers/cloudflare/__tests__/translator.test.ts @@ -134,6 +134,20 @@ describe('cloudflare/translator', () => { expect(result.enabled).toBe(true) }) + // Regression test: a keyed `query` condition (e.g. "match ?debug=1 + // specifically") previously compiled to a bare `http.request.uri.query` + // comparison against the *entire* query string, silently ignoring `key` + // and matching far more broadly than intended — see ExpressionBuilder's + // fromUnifiedCondition/buildKeyedQueryExpression for the fix. + it('scopes a keyed query condition to that argument, not the entire query string', () => { + const rule = makeUnifiedRule({ + conditions: [{ field: 'query', operator: 'eq', value: '1', key: 'debug' }], + }) + const { result } = unifiedToCloudflare(rule) + expect(result.expression).toBe('any(http.request.uri.args["debug"][*] eq "1")') + expect(result.expression).not.toBe('http.request.uri.query eq "1"') + }) + it('maps unified actions to Cloudflare actions', () => { const actionMappings: Array<{ unified: string; cf: string }> = [ { unified: 'log', cf: 'log' }, diff --git a/src/lib/translators/ExpressionBuilder.ts b/src/lib/translators/ExpressionBuilder.ts index 0a1a177..13a8cd4 100644 --- a/src/lib/translators/ExpressionBuilder.ts +++ b/src/lib/translators/ExpressionBuilder.ts @@ -11,6 +11,14 @@ 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`. + */ +const QUERY_ARGS_FIELD = 'http.request.uri.args' + /** * Builds Cloudflare wirefilter expressions from structured conditions */ @@ -103,6 +111,17 @@ 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) + } + const baseField = this.mapUnifiedFieldToCloudflare(condition.field) // `key` only makes sense as a bracket index for header/cookie fields // (matching FieldMapper's Vercel-side behavior) — a header or cookie @@ -122,6 +141,37 @@ export class ExpressionBuilder { return expression } + /** + * 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. + */ + private static buildKeyedQueryExpression(condition: UnifiedCondition, key: string): string { + const escapedKey = escapeWirefilterString(key) + const keyedField = `${QUERY_ARGS_FIELD}["${escapedKey}"]` + + let expression: string + if (condition.operator === 'exists') { + expression = `has_key(${QUERY_ARGS_FIELD}, "${escapedKey}")` + } else if (condition.operator === 'not_exists') { + expression = `not (has_key(${QUERY_ARGS_FIELD}, "${escapedKey}"))` + } else if (condition.operator === 'not_contains') { + expression = `not (any(${keyedField}[*] contains ${this.formatValue(QUERY_ARGS_FIELD, condition.value)}))` + } else if (condition.operator === 'not_in') { + expression = `not (any(${keyedField}[*] in ${this.formatValue(QUERY_ARGS_FIELD, condition.value)}))` + } else { + const operator = this.mapUnifiedOperator(condition.operator) + expression = `any(${keyedField}[*] ${operator} ${this.formatValue(QUERY_ARGS_FIELD, condition.value)})` + } + + return condition.negated ? `not (${expression})` : expression + } + /** * `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 b7ebfe9..dd5ef64 100644 --- a/src/lib/translators/__tests__/ExpressionBuilder.test.ts +++ b/src/lib/translators/__tests__/ExpressionBuilder.test.ts @@ -353,15 +353,79 @@ describe('ExpressionBuilder', () => { expect(result).toBe('http.cookie["a\\" or true or http.cookie[\\"a"] eq "x"') }) - it('ignores an unsupported key on a query condition instead of mislabeling it as a header', () => { + it('scopes a keyed query condition to that argument via http.request.uri.args, not the whole query string', () => { const result = ExpressionBuilder.fromUnifiedCondition({ field: 'query', operator: 'eq', value: 'abc', key: 'utm_source', }) - expect(result).toBe('http.request.uri.query eq "abc"') + // http.request.uri.query is a scalar String (the whole query string); + // http.request.uri.args is the Map> Cloudflare actually + // indexes by argument name, and `any(...[*] eq ...)` is the type-valid + // way to compare one of its (possibly-repeated) values — a bare + // `args["utm_source"] eq "abc"` is an Array-vs-String mismatch. + expect(result).toBe('any(http.request.uri.args["utm_source"][*] eq "abc")') expect(result).not.toContain('headers') + expect(result).not.toBe('http.request.uri.query eq "abc"') + }) + + it('leaves an unkeyed query condition matching the whole query string', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'query', + operator: 'contains', + value: 'debug=true', + }) + expect(result).toBe('http.request.uri.query contains "debug=true"') + }) + + it('builds a valueless exists expression for a keyed query condition via has_key', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'query', + operator: 'exists', + key: 'utm_source', + } as UnifiedCondition) + expect(result).toBe('has_key(http.request.uri.args, "utm_source")') + }) + + it('builds a valueless not_exists expression for a keyed query condition wrapped in not(...)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'query', + operator: 'not_exists', + key: 'utm_source', + } as UnifiedCondition) + expect(result).toBe('not (has_key(http.request.uri.args, "utm_source"))') + }) + + it('builds a not_contains expression for a keyed query condition as a positive any(...) wrapped in not(...)', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'query', + operator: 'not_contains', + value: 'admin', + key: 'redirect', + }) + expect(result).toBe('not (any(http.request.uri.args["redirect"][*] contains "admin"))') + }) + + it('wraps a negated keyed query condition in an outer not(...) around the any(...) expression', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'query', + operator: 'eq', + value: '1', + key: 'debug', + negated: true, + }) + expect(result).toBe('not (any(http.request.uri.args["debug"][*] eq "1"))') + }) + + it('escapes quotes in a keyed query key so it cannot break out of the field reference', () => { + const result = ExpressionBuilder.fromUnifiedCondition({ + field: 'query', + operator: 'eq', + value: 'x', + key: 'a" or true or http.request.uri.args["a', + }) + 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)', () => { From d7c977165e644737e4f271b9dd51b77fc58db188 Mon Sep 17 00:00:00 2001 From: Griffen Fargo <3642037+gfargo@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:03:55 -0400 Subject: [PATCH 2/2] fix: validate rateLimit.mitigationTimeout/countingExpression in the Zod schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both fields are on UnifiedAction['rateLimit'] and read by every translator (unifiedToCloudflare, unifiedToVercel, buildFastlyRateLimit), but rateLimitSchema only validated requests/window/characteristics. Zod strips unrecognized keys by default, and VercelFirewallService.getChanges/ syncRules and FastlyFirewallService.getChanges both diff against the *parsed* config rather than the raw one — so a rule authored with either field silently lost it before ever reaching those translators, and each translator's own fallback (?? interval, etc.) fired instead of the value the user actually set. Cloudflare's service diffs against the raw config directly, so it didn't hit this specific path, but the schema gap itself was provider-independent. --- .../__tests__/VercelFirewallService.test.ts | 49 +++++++++++++++++++ .../schemas/__tests__/commonSchemas.test.ts | 22 +++++++++ .../schemas/__tests__/unifiedSchemas.test.ts | 38 ++++++++++++++ src/lib/schemas/commonSchemas.ts | 7 +++ 4 files changed, 116 insertions(+) diff --git a/src/lib/providers/vercel/__tests__/VercelFirewallService.test.ts b/src/lib/providers/vercel/__tests__/VercelFirewallService.test.ts index 387f41f..c773543 100644 --- a/src/lib/providers/vercel/__tests__/VercelFirewallService.test.ts +++ b/src/lib/providers/vercel/__tests__/VercelFirewallService.test.ts @@ -1063,6 +1063,55 @@ describe('VercelFirewallService', () => { expect(changes.hasChanges).toBe(false) }) + // Regression test: rateLimitSchema didn't validate mitigationTimeout/ + // countingExpression, so Zod silently stripped them — getChanges diffs + // against `configValidation.data` (the *parsed* config), not the raw + // one, so a rule authored with either field lost it before ever + // reaching unifiedToVercel. Asserting on rulesToAdd (sourced straight + // from configValidation.data.rules for a pure addition) proves the + // fields now survive that parse. + it('preserves rateLimit.mitigationTimeout and countingExpression through getChanges (regression test)', async () => { + jest.spyOn(client, 'fetchFirewallConfig').mockResolvedValue({ + ...mockVercelConfig, + rules: [], + ips: [], + }) + + const config: UnifiedConfig = { + version: '2.0', + provider: 'vercel', + rules: [ + { + id: 'rule_rl', + name: 'Rate Limit API', + enabled: true, + conditionLogic: 'AND', + conditions: [{ field: 'path', operator: 'starts_with', value: '/api/', group: 0 }], + action: { + type: 'rate_limit', + rateLimit: { + requests: 100, + window: '1m', + mitigationTimeout: 7200, + countingExpression: 'http.request.method eq "POST"', + }, + }, + }, + ], + ips: [], + } + + const changes = await service.getChanges(config) + + expect(changes.rulesToAdd).toHaveLength(1) + expect(changes.rulesToAdd[0]?.action.rateLimit).toEqual({ + requests: 100, + window: '1m', + mitigationTimeout: 7200, + countingExpression: 'http.request.method eq "POST"', + }) + }) + it('should detect no changes for a redirect rule with permanent unset (regression test for #203)', async () => { jest.spyOn(client, 'fetchFirewallConfig').mockResolvedValue({ ...mockVercelConfig, diff --git a/src/lib/schemas/__tests__/commonSchemas.test.ts b/src/lib/schemas/__tests__/commonSchemas.test.ts index 2fcfb68..e089ce0 100644 --- a/src/lib/schemas/__tests__/commonSchemas.test.ts +++ b/src/lib/schemas/__tests__/commonSchemas.test.ts @@ -155,6 +155,28 @@ describe('commonSchemas', () => { it('rejects invalid window format', () => { expect(rateLimitSchema.safeParse({ requests: 100, window: 'invalid' }).success).toBe(false) }) + + // Regression test: mitigationTimeout/countingExpression are on + // UnifiedAction['rateLimit'] (types/unified.ts) and read by every + // translator, but were missing from this schema — Zod silently strips + // unrecognized keys, so `success: true` alone doesn't prove they + // survive; asserting on the parsed data does. + it('round-trips mitigationTimeout and countingExpression without stripping them', () => { + const result = rateLimitSchema.safeParse({ + requests: 100, + window: '1m', + mitigationTimeout: 7200, + countingExpression: 'http.request.method eq "POST"', + }) + expect(result.success).toBe(true) + expect(result.success && result.data.mitigationTimeout).toBe(7200) + expect(result.success && result.data.countingExpression).toBe('http.request.method eq "POST"') + }) + + it('rejects a non-positive mitigationTimeout', () => { + const result = rateLimitSchema.safeParse({ requests: 100, window: '1m', mitigationTimeout: 0 }) + expect(result.success).toBe(false) + }) }) describe('redirectSchema', () => { diff --git a/src/lib/schemas/__tests__/unifiedSchemas.test.ts b/src/lib/schemas/__tests__/unifiedSchemas.test.ts index 90da86d..0ca0538 100644 --- a/src/lib/schemas/__tests__/unifiedSchemas.test.ts +++ b/src/lib/schemas/__tests__/unifiedSchemas.test.ts @@ -320,6 +320,44 @@ describe('unifiedSchemas', () => { expect(result.success).toBe(true) expect(result.success && result.data.managedRules).toEqual(managedRules) }) + + // Same class of bug as managedRules above, on action.rateLimit instead: + // mitigationTimeout/countingExpression are on UnifiedAction['rateLimit'] + // (types/unified.ts) and read by unifiedToCloudflare/unifiedToVercel/ + // buildFastlyRateLimit, but rateLimitSchema didn't validate them — + // silently stripped on every safeParse. VercelFirewallService.getChanges + // and FastlyFirewallService.getChanges both diff against the *parsed* + // config, so a rule authored with either field lost it before it ever + // reached those translators. + it('round-trips action.rateLimit.mitigationTimeout and countingExpression through safeParse without stripping them', () => { + const result = unifiedConfigSchema.safeParse({ + ...validConfig, + rules: [ + { + name: 'Rate limited rule', + enabled: true, + conditions: [{ field: 'path', operator: 'eq', value: '/api' }], + action: { + type: 'rate_limit', + rateLimit: { + requests: 100, + window: '1m', + mitigationTimeout: 7200, + countingExpression: 'http.request.method eq "POST"', + }, + }, + }, + ], + }) + + expect(result.success).toBe(true) + expect(result.success && result.data.rules[0]?.action.rateLimit).toEqual({ + requests: 100, + window: '1m', + mitigationTimeout: 7200, + countingExpression: 'http.request.method eq "POST"', + }) + }) }) describe('validateUnifiedConfig', () => { diff --git a/src/lib/schemas/commonSchemas.ts b/src/lib/schemas/commonSchemas.ts index 12cf7f7..9adec96 100644 --- a/src/lib/schemas/commonSchemas.ts +++ b/src/lib/schemas/commonSchemas.ts @@ -74,6 +74,13 @@ export const rateLimitSchema = z.object({ requests: z.number().positive().int(), window: z.string().regex(/^\d+[smhd]$/), characteristics: z.array(z.string()).optional(), + // Zod strips unrecognized keys by default — without these, a rule authored + // with either field silently lost it on the first safeParse (every + // provider's getChanges/syncRules diffs against the parsed config, not the + // raw one), even though both are on UnifiedAction['rateLimit'] and read by + // every translator (unifiedToCloudflare/unifiedToVercel/buildFastlyRateLimit). + mitigationTimeout: z.number().positive().optional(), + countingExpression: z.string().optional(), }) // Redirect schema