fix: Cloudflare keyed header/cookie conditions emit invalid wirefilter - #276
Closed
gfargo wants to merge 1 commit into
Closed
fix: Cloudflare keyed header/cookie conditions emit invalid wirefilter#276gfargo wants to merge 1 commit into
gfargo wants to merge 1 commit into
Conversation
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<Array<String>>, 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"][*] <op> 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.
Owner
Author
|
Closing without merging — #278 landed first (merged as c529e57, ~seconds before this PR's CI finished) and independently arrived at the same fix via the same research (same
No action needed here beyond closing — see #278 for the real fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #269
Summary
A keyed
headerorcookiecondition (e.g. "match this specific header/cookie name") compiled to wirefilter Cloudflare's real API rejects — found while researching #263'squeryfix, which surfaced that the existingheader/cookiekey-scoping code shares (and in cookie's case, exceeds) the same construct problemqueryhad.header: compiled tohttp.request.headers["key"] eq "value".http.request.headersisMap<Array<String>>— indexing it yields an Array, and comparing an Array against a String literal viaeqis a type mismatch. Verified against Cloudflare's own "Require specific headers" WAF guide, which usesany(http.request.headers["key"][*] eq "value")for exactly this case.cookie: worse — compiled tohttp.cookie["key"] eq "value", buthttp.cookieis a scalarString(the wholeCookieheader), not a Map at all. Verified Cloudflare has a separatehttp.request.cookiesfield that is the rightMap<Array<String>>shape (sameany(...)idiom), so the fix points the keyed case at that field instead, leaving the scalarhttp.cookieuntouched for the unkeyed case.key: 'Content-Type'would previously have silently never matched a real request (this was true before this PR too, on the old bracket-index shape). Cookie keys are left as-is — Cloudflare doesn't case-normalize those.exists/not_existsfor both now usehas_key(map, "key"), matching the idiom Cloudflare's own changelog documents for map-key existence checks (same as #263'squeryfix already used).Scope notes
ExpressionBuilder.fromVercelCondition/FieldMapper(the direct Vercel-native → Cloudflare path, not the unified-config path this PR fixes) has the identical bug, plus an unfixed keyed-querycase of its own — but it's dead code, unreachable from any live command (confirmed viagrep). Left alone with a code comment explaining why and what to fix first if it's ever wired back in.WirefilterParser(it doesn't parseany(...)/has_key(...)syntax) — this is a deliberate, documented degradation matching the parser's own existing contract (unsupported construct →null, caller warns), not a silent misparse. Extending the parser to understand the new construct is real follow-up work, intentionally not bundled into this fix.Test plan
pnpm compile && pnpm test && pnpm lintall pass (1764 tests, 0 lint errors)