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/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 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)', () => {