Repository navigation
chore(sync): merge upstream v3.8.1 - #25
Merged
Merged
Conversation
…af#1602) The examples test is a smoke test that exercises a small fraction of the codebase via a handful of HTTP requests. Using -coverpkg=./... instruments the entire coraza codebase but only runs these few integration tests against it, producing a misleading ~16% coverage number that dilutes the overall report and creates noise on every PR. Without -coverpkg, Go only instruments the package under test, which correctly reflects what the example tests actually cover. The FTW and tinygo runs keep -coverpkg=./... because they exercise the full codebase through comprehensive rule evaluation. Co-authored-by: Matteo Pace <pace.matteo96@gmail.com>
…nded config (corazawaf#1606) * fix: apply default log/auditlog/pass to all phases and expand rule.msg in collection Agent-Logs-Url: https://github.com/corazawaf/coraza/sessions/e5d3c021-3219-4ae2-a886-652ae426bddd Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * test: add comprehensive coverage for built-in default actions across all phases Agent-Logs-Url: https://github.com/corazawaf/coraza/sessions/74691b5d-9391-4f98-89ea-7b392a4b49a6 Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * refactor: move phase 3-5 default actions to coraza.conf-recommended Instead of hardcoding per-phase default actions in the parser, add SecDefaultAction for phases 3, 4, and 5 to coraza.conf-recommended. ModSecurity v2 merges its default actionset (which includes log and auditlog) into every rule regardless of phase. Rather than replicating this as hardcoded behavior in Coraza's parser, make it an explicit configuration choice that users can tune — especially important for phases 3/4 where response data may be sensitive. Restore the original phase-2-only hardcoded default in the parser. Update tests to match: phase 5 test now uses explicit SecDefaultAction. * fix: remove duplicate SecDefaultAction from CRS test config These directives are now provided by coraza.conf-recommended, which the test already loads. Having both causes a parse error. * Update coraza.conf-recommended Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> * Update coraza.conf-recommended Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> Co-authored-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com>
…d prefilter (corazawaf#1597) * Improve pre filtering * perf: hybrid anyRequired strategy — indexed matcher for small N, AC for large N CRS v4.25 analysis shows alternation branch counts range from 2 to 762. The indexed bitmap matcher excels at small N (<=16) with 3-7x speedups, but with hundreds of needles the bitmap becomes dense and nearly every position is a candidate. For large N (>16), Aho-Corasick's O(H) single-pass DFA is more efficient. Adds anyRequiredACThreshold constant (16) to switch between strategies. Both algorithms are correct at any N; only performance differs. Made-with: Cursor * perf: replace bitmap scanner with Wu-Manber shift-table for sub-linear matching The bitmap-based indexedMatcher examined every byte in the haystack O(H). The new shift-table approach skips past non-candidate positions, achieving O(H/minLen) average case — for typical CRS keywords (minLen ~5-6), this examines only 1/5 to 1/6 of the haystack. Benchmarks show 3-4x improvement over the bitmap approach: 10 needles 80B: 155 ns → 51 ns (3.0x faster) 50 needles 80B: 493 ns → 129 ns (3.8x faster) 10 needles CI 1040B: shift=731ns vs contains_loop=9039ns (12.4x) Zero allocations, zero dependencies for the small-N path. Made-with: Cursor * test: comprehensive indexedMatcher tests across needle counts Add 6 new test functions (278 subtests) covering: - TestIndexedMatcherNeedleCounts: CS/CI match/no-match for N=2,3,5,8,10,15,16 cross-checked against brute-force reference - TestIndexedMatcherEveryNeedle: verifies each needle is found at start/middle/end/exact positions (10 needles × 4 positions × 2 modes) - TestIndexedMatcherEdgeCases: shared first/last bytes, varying lengths, boundary positions, overlapping needles, minimal haystacks - TestAnyRequiredThresholdBoundary: tests at N=15,16,17,20,30 to verify the indexed→AC threshold boundary - TestAnyRequiredViaPrefilterFuncNeedleCounts: end-to-end through prefilterFunc for N=2,3,5,8,10,16 - TestIndexedMatcherVsBruteForceExhaustive: property-based test with 16 inputs × 5 needle counts × 2 modes, all cross-checked Total test count: 1373 → 1651 Made-with: Cursor * prefilter: trie reconstruction + Wu-Manber-only for anyRequired Problem: regexp/syntax.Simplify() factors large alternations into trie structures (select|sleep|substr → s(?:elect|leep|ubstr)). This causes extractLiterals to return nil for those patterns — no prefilter is built and the full regex runs on every request. Changes: - extractLiterals/OpConcat: add trie reconstruction fallback. When the standard path returns nil (short prefix + anyRequired ignored), call trieReconstruct() to prepend the raw prefix to each alternation branch and recover the full literal words. Handles arbitrary nesting depth via recursive rawExtractSuffixes(). - anyRequiredMaxN raised from 64 → 256: Wu-Manber consistently beats AC libraries for WAF inputs because AC SIMD prefilters call bytes.IndexByte once per unique start byte (O(H×K)), while Wu-Manber's 256-byte shift table fits in L1 cache. Benchmarks: Wu-Manber N=20 at 82B input = 47 ns vs coregx/ahocorasick = 727 ns (15x slower), gap widens to 54x at 5KB. - Remove Aho-Corasick fallback for large N entirely. The large-N path (N > anyRequiredMaxN) now returns nil (full regex) instead of building AC. In practice, regexp/syntax.Simplify() already prevents extractLiterals from returning anything for patterns with 100+ branches, so this branch was rarely reached. - Remove github.com/coregx/ahocorasick and petar-dambovaliev/aho-corasick from rxprefilter.go (petar-dambovaliev remains for the @pm operator). Impact: 20-keyword SQL patterns like (?i)(?:select|sleep|...|upper) that previously had no prefilter now get a Wu-Manber prefilter, skipping regex evaluation for benign HTTP traffic. Safety: trieReconstruct only handles the exact 2-child [prefix, alternation] pattern emitted by Simplify(). Patterns with intervening optional nodes (e.g. a.*b) are rejected to prevent false negatives. Made-with: Cursor * bench: add trie_reconstruction_sqli benchmark case to BenchmarkRxPrefilter Made-with: Cursor * chore: remove *.conf from gitignore Made-with: Cursor * prefilter: anyRequired propagation in OpConcat + benchmark harness 1. anyRequired propagation in OpConcat Previously OpConcat silently dropped anyRequired from its children. Patterns like (?:^|sep)\s*(?:select|union|drop) got no prefilter because the keyword alternation's anyRequired was thrown away. Now OpConcat collects anyRequired from required children and uses it as a fallback (trieReconstruct takes priority to avoid short fragments). Coverage: 64 → 26 rules without prefilter (55% → 82%). Speedup for anchor+SQLi patterns: 1.2× → ~16×. 2. Fix Go interface-nil gotcha in trieReconstruct trieReconstruct returned anyRequired(nil) (nil concrete value). Stored in interface{} this becomes (type=anyRequired, value=nil) which != nil at the interface level, fooling the if lits == nil guard in OpAlternate and silently building unsafe prefilters. Fix: explicitly return the interface nil. 3. Test coverage - anchor_or_separator_then_keywords case in TestTrieReconstructionBasic - Updated nested_trie_reconstruction comment to document ordering 4. Benchmark harness (bench_compare_test.go) Covers 5 regex categories × 16 request types for apples-to-apples comparison between branch and main via benchstat. Made-with: Cursor * test: CRS-driven prefilter coverage, safety, and benchmark Adds rxprefilter_crs_test.go — a fully self-contained test file that drives the @rx prefilter against the real OWASP CRS. No proprietary data. All inputs come from the open-source CRS packages already in go.mod: - github.com/corazawaf/coraza-coreruleset (rule conf files) - github.com/corazawaf/coraza-coreruleset/tests (official FTW YAMLs) Three test / benchmark functions: TestCRSPrefilterCoverage Reports what fraction of CRS @rx rules get a prefilter, broken down by attack category (SQLi, XSS, RCE, PHP, LFI, …). TestCRSPrefilterSafety Verifies 1834 official FTW attack payloads across all CRS rule categories: if the prefilter rejects an input the full regex must also reject it. Zero false negatives confirmed. BenchmarkCRSPrefilterVsRegex Per-category regex-only vs prefilter+regex, using real attack payloads mixed with benign traffic. Category regex-only prefilter speedup SQLi 6.0 ms 4.5 ms 1.35x XSS 360 µs 269 µs 1.34x RCE 1604 µs 1480 µs 1.08x PHP 407 µs 257 µs 1.58x LFI 35 µs 35 µs 1.0x (no prefilter yet) Made-with: Cursor * prefilter: add minMatchLength fallback + realistic CRS benchmark corpus Three improvements: 1. **minMatchLength as fallback prefilter** (rxprefilter.go) - When extractLiterals() returns nil, fall back to a length-only prefilter when minMatchLength >= 4 bytes. O(1) cost, always safe. - Prepend the same length guard to every literal prefilter so very-short inputs are rejected before the O(H) string scan runs. - CRS prefilter coverage: 53% → 68% (+31 rules, zero false negatives on 2238 official attack payloads). 2. **Realistic 9:1 benign-to-attack ratio in CRS benchmark** (rxprefilter_crs_test.go) - benignCRSPayloads expanded from 10 → 60 diverse payloads (REST, headers, form data, JSON, short values). - Benchmark now uses 9 copies of benign per 1 copy of attack traffic, matching real production traffic mix. - Observed speedups (with realistic corpus): XSS 1.95x, PHP 2.25x, LFI 1.67x, SQLi 1.40x vs regex-only baseline. 3. **Update TestExtractLiteralsRuneError** (rxprefilter_test.go) - Pattern containing U+FFFD now correctly returns a length-only prefilter (mml=8) instead of nil. Updated test expectations accordingly. Made-with: Cursor * test: consolidate bloated tests — remove 1,131 lines - 20+ tiny single-purpose tests → one table-driven TestPrefilterCodePaths - Remove TestPrefilterCRSPatterns (superseded by rxprefilter_crs_test.go) - 5 overlapping Wu-Manber benchmarks → one BenchmarkIndexedMatcher - Delete rxprefilter_waf_test.go (superseded by CRS-driven tests) All 2,238 CRS attack payloads still pass safety check. Zero false negatives. Made-with: Cursor * test: remove bloated tests and shrink data tables - rxprefilter_test.go: delete TestTrieReconstructionEndToEnd, TestIndexedMatcherVsBruteForceExhaustive, TestPrefilterConcurrentSafety, TestAllASCIIStrings, TestPrefilterSafetyInvariants; shrink NeedleCounts (7→3 counts), EveryNeedle (10→needles×2 positions), ThresholdBoundary (6 counts×all-needles → 2 counts×1 each), AnyRequiredViaPrefilterFunc (6 counts → 2); trim MinMatchLength (35→15), fuzz seeds (25×37 → 12×15), BenchmarkIndexedMatcher (4 suites×3 sizes → 3 suites×1 size) - rxprefilter_crs_test.go: shrink benignCRSPayloads from 74 to 10 entries - bench_compare_test.go: delete BenchmarkCompareSummary and BenchmarkPrefilterOnly; shrink bcRequests from 16 to 5 payloads Net: -679 lines. All 264 tests still pass. Made-with: Cursor * chore: downgrade go.work to go 1.25.0 Made-with: Cursor * test: add BenchmarkCRSPrefilterVsRegexPL1 for paranoia-level/1 rules Made-with: Cursor * fix: replace stale N>64 comment with anyRequiredMaxN reference Made-with: Cursor * test: improve line 352 coverage + silence codecov for build-tag files - TestPrefilterFuncBuildability: add two more OpLiteral → allRequired test cases (sql keyword, longer literal) so the critical single-literal fast-path (rxprefilter.go:352) is hit by multiple explicit tests rather than just one - codecov.yml: ignore rxprefilter*.go and bench_compare_test.go in coverage reports. These files are gated behind -tags coraza.rule.rx_prefilter and compile to nothing in untagged runs, producing a misleading 0% / -88% delta. Coverage for these files is captured in the dedicated rx_prefilter CI matrix entry. Made-with: Cursor * fix: gocritic ifElseChain + gofmt formatting in rxprefilter - rxprefilter.go: rewrite anyRequired if-else chain as switch statement (gocritic ifElseChain) - rxprefilter_test.go: fix gofmt comment alignment and double blank lines Made-with: Cursor * chore: rename bench_compare_test → rxprefilter_bench_compare_test, simplify codecov ignore glob Made-with: Cursor * revert: remove rxprefilter* from codecov ignore list Ignoring the files caused Codecov to treat rxprefilter.go as a deleted file, producing a spurious -70% coverage change. The untagged 0% issue is less harmful than making the file appear deleted. Made-with: Cursor * perf: three micro-optimisations to rxprefilter hot path 1. Eliminate double regex parse in prefilterFunc prefilterFunc already held the parsed, simplified *syntax.Regexp but was calling minMatchLength(pattern) which re-ran syntax.Parse + Simplify internally. Replace with minLen(re) to reuse the AST. Rule-compile benchmarks: 30–47% faster, ~30% fewer allocations. 2. filterShort: compact in place, zero heap allocation Was using ss[:0:0] (capacity 0) which forces a heap allocation on the first append. Replaced with an in-place compaction (ss[k]=s; k++) that returns ss[:k] — no allocation when all or most elements pass. 3. containsFoldASCIIOnly: SIMD first-byte skip via strings.IndexByte The old inner loop was a byte-by-byte brute-force scan. Use strings.IndexByte (maps to SIMD on amd64/arm64) to jump directly to candidate positions, then verify with equalFoldASCIIBytes only there. Benchmarks on Apple M4 Pro (3s runs, 227-byte HTTP haystack): miss "union" (5b): 206 ns → 34 ns 6.0× miss "select" (6b): 194 ns → 91 ns 2.1× miss "javascript" (10b): 190 ns → 12 ns 16.0× hit "cookie" (6b): 171 ns → 80 ns 2.1× All 590 tests pass, zero new allocations on any hot path. Made-with: Cursor * test: add TestInternalNodeCoverage to close helper-function gaps 16 new sub-tests covering AST-node branches in minLen, extractLiterals, rawLiteral, rawExtractSuffixes, newIndexedMatcher, matchCI, and containsFoldASCIIOnly that are unreachable through the public prefilterFunc / minMatchLength APIs (syntax.Simplify() expands or eliminates those nodes before they are called). Coverage changes (rxprefilter.go): minLen 84.6% → 100% extractLiterals 91.1% → 100% rawExtractSuffixes 90.0% → 100% rawLiteral 88.9% → 100% newIndexedMatcher 93.5% → 100% matchCI 95.0% → 100% containsFoldASCIIOnly 91.7% → 100% Remaining <100% lines (prefilterFunc 90.5%, trieReconstruct 95%) are defensive guards against impossible states after Simplify() — e.g. allRequired/anyRequired with all-short elements, >anyRequiredMaxN needles, empty OpAlternate branchLits — which cannot be reached through a real pattern string. Made-with: Cursor * fix: prevent false negatives from phantom joined literals in trie reconstruction rawExtractSuffixes collapsed allRequired fragments from an OpConcat suffix using strings.Join, producing a string that never appears contiguously in a real input: s(?:elect.*from|leep) "elect.*from" → allRequired{"elect","from"} → joined "electfrom" trieReconstruct builds {"selectfrom","sleep"} Input "select x from users" has neither → prefilter returns false → MISSED ATTACK (false negative, breaks safety guarantee) Fix: only accept a single-element allRequired as a trie suffix. When a concat under an alternation branch resolves to multiple disjoint literals (wildcards between them), return nil so the caller falls back to the safe anyRequired propagation inherited from the branch literals instead. The fallback for s(?:elect.*from|leep) now yields anyRequired{"from","leep"} (longest literal from each branch), which is correct: "select x from users" → contains "from" → prefilter true ✓ "sleep(5)" → contains "leep" → prefilter true ✓ "unrelated" → neither present → prefilter false ✓ Regression tests added to TestPrefilterNeverCausesFalseNegatives covering both the original example and a CI variant. Made-with: Cursor * docs: clarify IndexByte portability in containsFoldASCIIOnly comment Made-with: Cursor * test: add concurrent safety and CI non-ASCII prefilter tests Made-with: Cursor * Improve pre filtering * perf: hybrid anyRequired strategy — indexed matcher for small N, AC for large N CRS v4.25 analysis shows alternation branch counts range from 2 to 762. The indexed bitmap matcher excels at small N (<=16) with 3-7x speedups, but with hundreds of needles the bitmap becomes dense and nearly every position is a candidate. For large N (>16), Aho-Corasick's O(H) single-pass DFA is more efficient. Adds anyRequiredACThreshold constant (16) to switch between strategies. Both algorithms are correct at any N; only performance differs. Made-with: Cursor * perf: replace bitmap scanner with Wu-Manber shift-table for sub-linear matching The bitmap-based indexedMatcher examined every byte in the haystack O(H). The new shift-table approach skips past non-candidate positions, achieving O(H/minLen) average case — for typical CRS keywords (minLen ~5-6), this examines only 1/5 to 1/6 of the haystack. Benchmarks show 3-4x improvement over the bitmap approach: 10 needles 80B: 155 ns → 51 ns (3.0x faster) 50 needles 80B: 493 ns → 129 ns (3.8x faster) 10 needles CI 1040B: shift=731ns vs contains_loop=9039ns (12.4x) Zero allocations, zero dependencies for the small-N path. Made-with: Cursor * test: comprehensive indexedMatcher tests across needle counts Add 6 new test functions (278 subtests) covering: - TestIndexedMatcherNeedleCounts: CS/CI match/no-match for N=2,3,5,8,10,15,16 cross-checked against brute-force reference - TestIndexedMatcherEveryNeedle: verifies each needle is found at start/middle/end/exact positions (10 needles × 4 positions × 2 modes) - TestIndexedMatcherEdgeCases: shared first/last bytes, varying lengths, boundary positions, overlapping needles, minimal haystacks - TestAnyRequiredThresholdBoundary: tests at N=15,16,17,20,30 to verify the indexed→AC threshold boundary - TestAnyRequiredViaPrefilterFuncNeedleCounts: end-to-end through prefilterFunc for N=2,3,5,8,10,16 - TestIndexedMatcherVsBruteForceExhaustive: property-based test with 16 inputs × 5 needle counts × 2 modes, all cross-checked Total test count: 1373 → 1651 Made-with: Cursor * prefilter: trie reconstruction + Wu-Manber-only for anyRequired Problem: regexp/syntax.Simplify() factors large alternations into trie structures (select|sleep|substr → s(?:elect|leep|ubstr)). This causes extractLiterals to return nil for those patterns — no prefilter is built and the full regex runs on every request. Changes: - extractLiterals/OpConcat: add trie reconstruction fallback. When the standard path returns nil (short prefix + anyRequired ignored), call trieReconstruct() to prepend the raw prefix to each alternation branch and recover the full literal words. Handles arbitrary nesting depth via recursive rawExtractSuffixes(). - anyRequiredMaxN raised from 64 → 256: Wu-Manber consistently beats AC libraries for WAF inputs because AC SIMD prefilters call bytes.IndexByte once per unique start byte (O(H×K)), while Wu-Manber's 256-byte shift table fits in L1 cache. Benchmarks: Wu-Manber N=20 at 82B input = 47 ns vs coregx/ahocorasick = 727 ns (15x slower), gap widens to 54x at 5KB. - Remove Aho-Corasick fallback for large N entirely. The large-N path (N > anyRequiredMaxN) now returns nil (full regex) instead of building AC. In practice, regexp/syntax.Simplify() already prevents extractLiterals from returning anything for patterns with 100+ branches, so this branch was rarely reached. - Remove github.com/coregx/ahocorasick and petar-dambovaliev/aho-corasick from rxprefilter.go (petar-dambovaliev remains for the @pm operator). Impact: 20-keyword SQL patterns like (?i)(?:select|sleep|...|upper) that previously had no prefilter now get a Wu-Manber prefilter, skipping regex evaluation for benign HTTP traffic. Safety: trieReconstruct only handles the exact 2-child [prefix, alternation] pattern emitted by Simplify(). Patterns with intervening optional nodes (e.g. a.*b) are rejected to prevent false negatives. Made-with: Cursor * bench: add trie_reconstruction_sqli benchmark case to BenchmarkRxPrefilter Made-with: Cursor * chore: remove *.conf from gitignore Made-with: Cursor * prefilter: anyRequired propagation in OpConcat + benchmark harness 1. anyRequired propagation in OpConcat Previously OpConcat silently dropped anyRequired from its children. Patterns like (?:^|sep)\s*(?:select|union|drop) got no prefilter because the keyword alternation's anyRequired was thrown away. Now OpConcat collects anyRequired from required children and uses it as a fallback (trieReconstruct takes priority to avoid short fragments). Coverage: 64 → 26 rules without prefilter (55% → 82%). Speedup for anchor+SQLi patterns: 1.2× → ~16×. 2. Fix Go interface-nil gotcha in trieReconstruct trieReconstruct returned anyRequired(nil) (nil concrete value). Stored in interface{} this becomes (type=anyRequired, value=nil) which != nil at the interface level, fooling the if lits == nil guard in OpAlternate and silently building unsafe prefilters. Fix: explicitly return the interface nil. 3. Test coverage - anchor_or_separator_then_keywords case in TestTrieReconstructionBasic - Updated nested_trie_reconstruction comment to document ordering 4. Benchmark harness (bench_compare_test.go) Covers 5 regex categories × 16 request types for apples-to-apples comparison between branch and main via benchstat. Made-with: Cursor * test: CRS-driven prefilter coverage, safety, and benchmark Adds rxprefilter_crs_test.go — a fully self-contained test file that drives the @rx prefilter against the real OWASP CRS. No proprietary data. All inputs come from the open-source CRS packages already in go.mod: - github.com/corazawaf/coraza-coreruleset (rule conf files) - github.com/corazawaf/coraza-coreruleset/tests (official FTW YAMLs) Three test / benchmark functions: TestCRSPrefilterCoverage Reports what fraction of CRS @rx rules get a prefilter, broken down by attack category (SQLi, XSS, RCE, PHP, LFI, …). TestCRSPrefilterSafety Verifies 1834 official FTW attack payloads across all CRS rule categories: if the prefilter rejects an input the full regex must also reject it. Zero false negatives confirmed. BenchmarkCRSPrefilterVsRegex Per-category regex-only vs prefilter+regex, using real attack payloads mixed with benign traffic. Category regex-only prefilter speedup SQLi 6.0 ms 4.5 ms 1.35x XSS 360 µs 269 µs 1.34x RCE 1604 µs 1480 µs 1.08x PHP 407 µs 257 µs 1.58x LFI 35 µs 35 µs 1.0x (no prefilter yet) Made-with: Cursor * prefilter: add minMatchLength fallback + realistic CRS benchmark corpus Three improvements: 1. **minMatchLength as fallback prefilter** (rxprefilter.go) - When extractLiterals() returns nil, fall back to a length-only prefilter when minMatchLength >= 4 bytes. O(1) cost, always safe. - Prepend the same length guard to every literal prefilter so very-short inputs are rejected before the O(H) string scan runs. - CRS prefilter coverage: 53% → 68% (+31 rules, zero false negatives on 2238 official attack payloads). 2. **Realistic 9:1 benign-to-attack ratio in CRS benchmark** (rxprefilter_crs_test.go) - benignCRSPayloads expanded from 10 → 60 diverse payloads (REST, headers, form data, JSON, short values). - Benchmark now uses 9 copies of benign per 1 copy of attack traffic, matching real production traffic mix. - Observed speedups (with realistic corpus): XSS 1.95x, PHP 2.25x, LFI 1.67x, SQLi 1.40x vs regex-only baseline. 3. **Update TestExtractLiteralsRuneError** (rxprefilter_test.go) - Pattern containing U+FFFD now correctly returns a length-only prefilter (mml=8) instead of nil. Updated test expectations accordingly. Made-with: Cursor * test: consolidate bloated tests — remove 1,131 lines - 20+ tiny single-purpose tests → one table-driven TestPrefilterCodePaths - Remove TestPrefilterCRSPatterns (superseded by rxprefilter_crs_test.go) - 5 overlapping Wu-Manber benchmarks → one BenchmarkIndexedMatcher - Delete rxprefilter_waf_test.go (superseded by CRS-driven tests) All 2,238 CRS attack payloads still pass safety check. Zero false negatives. Made-with: Cursor * test: remove bloated tests and shrink data tables - rxprefilter_test.go: delete TestTrieReconstructionEndToEnd, TestIndexedMatcherVsBruteForceExhaustive, TestPrefilterConcurrentSafety, TestAllASCIIStrings, TestPrefilterSafetyInvariants; shrink NeedleCounts (7→3 counts), EveryNeedle (10→needles×2 positions), ThresholdBoundary (6 counts×all-needles → 2 counts×1 each), AnyRequiredViaPrefilterFunc (6 counts → 2); trim MinMatchLength (35→15), fuzz seeds (25×37 → 12×15), BenchmarkIndexedMatcher (4 suites×3 sizes → 3 suites×1 size) - rxprefilter_crs_test.go: shrink benignCRSPayloads from 74 to 10 entries - bench_compare_test.go: delete BenchmarkCompareSummary and BenchmarkPrefilterOnly; shrink bcRequests from 16 to 5 payloads Net: -679 lines. All 264 tests still pass. Made-with: Cursor * chore: downgrade go.work to go 1.25.0 Made-with: Cursor * test: add BenchmarkCRSPrefilterVsRegexPL1 for paranoia-level/1 rules Made-with: Cursor * fix: replace stale N>64 comment with anyRequiredMaxN reference Made-with: Cursor * test: improve line 352 coverage + silence codecov for build-tag files - TestPrefilterFuncBuildability: add two more OpLiteral → allRequired test cases (sql keyword, longer literal) so the critical single-literal fast-path (rxprefilter.go:352) is hit by multiple explicit tests rather than just one - codecov.yml: ignore rxprefilter*.go and bench_compare_test.go in coverage reports. These files are gated behind -tags coraza.rule.rx_prefilter and compile to nothing in untagged runs, producing a misleading 0% / -88% delta. Coverage for these files is captured in the dedicated rx_prefilter CI matrix entry. Made-with: Cursor * fix: gocritic ifElseChain + gofmt formatting in rxprefilter - rxprefilter.go: rewrite anyRequired if-else chain as switch statement (gocritic ifElseChain) - rxprefilter_test.go: fix gofmt comment alignment and double blank lines Made-with: Cursor * chore: rename bench_compare_test → rxprefilter_bench_compare_test, simplify codecov ignore glob Made-with: Cursor * revert: remove rxprefilter* from codecov ignore list Ignoring the files caused Codecov to treat rxprefilter.go as a deleted file, producing a spurious -70% coverage change. The untagged 0% issue is less harmful than making the file appear deleted. Made-with: Cursor * perf: three micro-optimisations to rxprefilter hot path 1. Eliminate double regex parse in prefilterFunc prefilterFunc already held the parsed, simplified *syntax.Regexp but was calling minMatchLength(pattern) which re-ran syntax.Parse + Simplify internally. Replace with minLen(re) to reuse the AST. Rule-compile benchmarks: 30–47% faster, ~30% fewer allocations. 2. filterShort: compact in place, zero heap allocation Was using ss[:0:0] (capacity 0) which forces a heap allocation on the first append. Replaced with an in-place compaction (ss[k]=s; k++) that returns ss[:k] — no allocation when all or most elements pass. 3. containsFoldASCIIOnly: SIMD first-byte skip via strings.IndexByte The old inner loop was a byte-by-byte brute-force scan. Use strings.IndexByte (maps to SIMD on amd64/arm64) to jump directly to candidate positions, then verify with equalFoldASCIIBytes only there. Benchmarks on Apple M4 Pro (3s runs, 227-byte HTTP haystack): miss "union" (5b): 206 ns → 34 ns 6.0× miss "select" (6b): 194 ns → 91 ns 2.1× miss "javascript" (10b): 190 ns → 12 ns 16.0× hit "cookie" (6b): 171 ns → 80 ns 2.1× All 590 tests pass, zero new allocations on any hot path. Made-with: Cursor * test: add TestInternalNodeCoverage to close helper-function gaps 16 new sub-tests covering AST-node branches in minLen, extractLiterals, rawLiteral, rawExtractSuffixes, newIndexedMatcher, matchCI, and containsFoldASCIIOnly that are unreachable through the public prefilterFunc / minMatchLength APIs (syntax.Simplify() expands or eliminates those nodes before they are called). Coverage changes (rxprefilter.go): minLen 84.6% → 100% extractLiterals 91.1% → 100% rawExtractSuffixes 90.0% → 100% rawLiteral 88.9% → 100% newIndexedMatcher 93.5% → 100% matchCI 95.0% → 100% containsFoldASCIIOnly 91.7% → 100% Remaining <100% lines (prefilterFunc 90.5%, trieReconstruct 95%) are defensive guards against impossible states after Simplify() — e.g. allRequired/anyRequired with all-short elements, >anyRequiredMaxN needles, empty OpAlternate branchLits — which cannot be reached through a real pattern string. Made-with: Cursor * fix: prevent false negatives from phantom joined literals in trie reconstruction rawExtractSuffixes collapsed allRequired fragments from an OpConcat suffix using strings.Join, producing a string that never appears contiguously in a real input: s(?:elect.*from|leep) "elect.*from" → allRequired{"elect","from"} → joined "electfrom" trieReconstruct builds {"selectfrom","sleep"} Input "select x from users" has neither → prefilter returns false → MISSED ATTACK (false negative, breaks safety guarantee) Fix: only accept a single-element allRequired as a trie suffix. When a concat under an alternation branch resolves to multiple disjoint literals (wildcards between them), return nil so the caller falls back to the safe anyRequired propagation inherited from the branch literals instead. The fallback for s(?:elect.*from|leep) now yields anyRequired{"from","leep"} (longest literal from each branch), which is correct: "select x from users" → contains "from" → prefilter true ✓ "sleep(5)" → contains "leep" → prefilter true ✓ "unrelated" → neither present → prefilter false ✓ Regression tests added to TestPrefilterNeverCausesFalseNegatives covering both the original example and a CI variant. Made-with: Cursor * docs: clarify IndexByte portability in containsFoldASCIIOnly comment Made-with: Cursor * test: add concurrent safety and CI non-ASCII prefilter tests Made-with: Cursor * Add improvements * fix: unused types Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * Add improvements * lint --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: Felipe Zipitria <felipe.zipitria@owasp.org>
* perf: add minLen prefilter to @pm operator Compute the length of the shortest pattern at operator init time and skip the Aho-Corasick automaton entirely when the input is shorter — no pattern can possibly match. This mirrors the minMatchLength prefilter already used by @rx. For @pm the computation is trivial (min of literal lengths) vs @rx which walks the regexp AST. Applies to all @pm variants: pm, pmFromFile, pmFromDataset. Micro-benchmark (10 patterns, Apple M2): below_minLen: 2ns/op, 0 allocs (was ~98ns with Aho-Corasick) at_minLen: 98ns/op, 4 allocs (unchanged — hits automaton) CRS benchmark (benchstat, count=6): Memory: -3.04% geomean (p=0.015) Allocations: -1.87% geomean (p=0.002) Time: -2.92% geomean * fix: minPatternLen returns 0 on empty pattern to disable incorrect short-circuit Agent-Logs-Url: https://github.com/corazawaf/coraza/sessions/a3dfe946-be0f-4dbf-8d33-ded64c17a59c Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Juan Pablo Tosso <jptosso@gmail.com>
…zawaf#1618) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…razawaf#1619) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* first * more coverage * go mod tidy
…orazawaf#1624) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…zawaf#1625) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* refactor * typo * enums and removes useless check
…zawaf#1610 (corazawaf#1622) Chain children carry no own ID (noID), but ctl:ruleRemoveTarget* stores exceptions under the parent's ID, which we must use while searching rules. This matches the pattern already used by matchVariable.
…zawaf#1630) * chore: update recommended aligning to crs * bumps
enforce-all-checks is the only required status check on main, but it lived in regression.yml, which skips docs-only changes via paths-ignore. On such PRs the required check was never reported and the PR stayed blocked forever (e.g. corazawaf#1633, corazawaf#1634). Move the job to its own workflow with no path filters: it always runs, and wait-for-status-checks simply passes when no other checks are pending. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…azawaf#1631) * fix(multimatch): cmdLine and urlDecodeUni over reporting changes * tidye
…N body processor is running (corazawaf#1615) * Fill ARGS_POST variable even if body bytes were invalid JSON when JSON body processor is running * Add e2e test to check if process ProcessPartial and JSON body processor can work together * Typo Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Return gjson error instead of silently dropping it Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch> * Add comments for more clearance * Fix wrong author * Uncomment rule because we expect REQBODY_ERROR AND ARGS_POST to be set * Remove obsolete comment * tests: adds unit coverage to json body processor --------- Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch> Co-authored-by: kabbohus <hussein.kabbout@united-security-providers.ch> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: jcchavezs <josecarlos.chavez@owasp.org> Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com>
…zawaf#1646) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…razawaf#1647) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…orazawaf#1648) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* fix: implement Unicode best-fit mapping in urlDecodeUni Replaces the dead TODO stub with a codepage-20127 best-fit table mirroring ModSecurity's default SecUnicodeMapFile behavior, so %uXXXX sequences like curly quotes fold to ASCII instead of always falling back to the raw low byte. Fixes corazawaf#53 * refactor: use descriptive names for %uXXXX decode temporaries hmap, fact, and xv become mappedByte, placeValue, and hexValue. No behavior change. * docs: annotate unicode_bestfit table with codepoint names Each entry now documents its Unicode codepoint, name, and destination character, making the 370-entry table auditable against ModSecurity's unicode.mapping without decoding hex by hand. No behavior change. * docs: note best-fit table's homoglyph coverage limits unicodeBestFitASCII mirrors ModSecurity's legacy IIS %uXXXX best-fit folding table, not a Unicode confusables/homoglyph defense. Documents concrete gaps (cross-script lookalikes, mathematical alphanumeric symbols, enclosed/letterlike alphanumerics) so future readers don't assume it covers evasion via those codepoints. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: rewrite inplaceUniDecode for throughput Replace the repeated strings.ValidHex/strings.X2c calls (up to 8 per %uXXXX escape) with a single branch-free hex-nibble lookup table, and bulk-copy literal runs between escapes instead of shifting one byte at a time, with a size-tiered fallback since copy()'s memmove call costs more than it saves below ~8 bytes. Adds BenchmarkInplaceUniDecode covering plain text, +-heavy, valid and invalid %XX, %uXXXX plain and best-fit-mapped, a mixed query string, and a truncated trailing %. Also adds regression cases for %U uppercase, the fullwidth-fallback path, the zero-byte edge, an out-of-table codepoint, and one-byte-short truncation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…awaf#1664) doBase64decode stopped decoding entirely on any byte outside the base64 alphabet, including any byte > 127. A single such byte anywhere in the input -- not just the JWT-specific '-'/'_' case from corazawaf#1651 -- made the entire decoded value disappear if it came first, or silently truncated it otherwise, while real-world consumers (PHP's base64_decode, Node's Buffer.from(str, 'base64')) just skip the byte and keep decoding. ModSecurity's own forgiving decoder (decode_forgiven_engine) does the same. The ext (forgiving) decoder now skips any invalid byte and continues, matching that reference behavior. The strict decoder's documented stop-at-first-invalid-character behavior is unchanged -- that's an intentional, existing design choice (see the comment referencing corazawaf#940), not addressed here. See corazawaf#1661 Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…corazawaf#1659) * fix: decode runes in compressWhitespace instead of indexing raw bytes isLatinSpace matched raw bytes 0x85/0xA0 (meant to catch U+0085 NEL and U+00A0 NBSP) against a UTF-8 string one byte at a time. Those code points are never single bytes in UTF-8 -- they're two-byte sequences (0xC2 0x85 / 0xC2 0xA0) -- but the raw bytes 0x85/0xA0 are common as the trailing byte of unrelated 2-byte characters (e.g. a, A, a with low 6 bits 100000 or 000101). The byte-level check fired on that trailing byte, splicing it out mid-character and replacing it with a literal space, producing invalid UTF-8 for input with no whitespace in that position at all. doCompressWhitespace now decodes a full rune per iteration instead of indexing a single byte, so NEL/NBSP are only matched when they're actually present, and any other character is copied through whole. Also fixes a related bug in the same loop: canonicalizing a single non-space whitespace character (e.g. a tab) to a literal space didn't set changed=true, even though the output differed from the input. See corazawaf#1655 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: treat a stray raw 0xA0 byte as whitespace again Decoding a full rune instead of indexing a byte fixed the UTF-8 corruption, but dropped ModSecurity parity for a bare 0xA0 that isn't part of a valid UTF-8 sequence: utf8.DecodeRuneInString returns RuneError for it, isLatinSpace(RuneError) is false, and the byte now passes through unnormalized. ModSecurity's compressWhitespace matches this byte directly regardless of encoding, and Coraza transforms arbitrary request bytes rather than guaranteed-UTF-8 strings, so "SELECT\xa01" must still collapse to "SELECT 1". 0xA0 is never a valid UTF-8 leading byte (it falls in the continuation-byte range), so checking the raw byte at a rune boundary is safe: it can never misfire on a genuine multi-byte character, only catch the case decoding just failed on. Raw 0x85 intentionally isn't given the same treatment -- isspace() in the C locale ModSecurity uses never matched it, so byte-matching it here would be more permissive than ModSecurity, not parity with it. Also clarifies isLatinSpace's doc comment: "copied from unicode.IsSpace" was misleading since it doesn't cover the rest of unicode.IsSpace (U+2000-U+200A, U+2028, U+3000, etc.), which the sibling removeWhitespace does handle via unicode.IsSpace directly. Reported by @M4tteoP in review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com>
'-' and '_' (RFC 4648 §5's substitutes for '+' and '/') were treated as invalid characters, so doBase64decode stopped decoding at the first one it saw. Any base64url-encoded token (e.g. a JWT segment) containing '-' or '_' before a match target would silently truncate it out of the decoded output, undetected by rules relying on t:base64DecodeExt. Scoped to the ext path only; the strict base64decode transform still rejects '-'/'_' as before. See coreruleset/coreruleset#4663 (comment) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…PathWin (corazawaf#1660) * fix: strip Windows trailing dots/spaces and ADS suffixes in normalisePathWin normalisePathWin only converted backslashes to slashes and delegated to the POSIX-style normalisePath (filepath.Clean). It didn't replicate two Windows-specific filesystem behaviors that are classic WAF-evasion vectors: - CreateFile et al. silently strip trailing dots and spaces from every path component before resolving it ("web.config." and "web.config " both open "web.config"). - Windows resolves NTFS Alternate Data Stream syntax ("file:stream", "file::$DATA") against the base file. Neither was stripped, so a rule matching a blocklisted filename/extension against the normalized path could be defeated by appending either, while Windows/IIS still resolves the request to the exact blocked resource. stripWindowsTrailingDotsAndSpaces leaves a component that's entirely dots (".", "..", an unresolved leading ".." run, or an empty component from a repeated "/") untouched -- those are navigation syntax, not a filename to canonicalize, and trimming them would otherwise collapse ".." into an empty segment. Also fixes the changed flag, which only ever compared the already-slash-converted string against filepath.Clean's output, so a pure backslash-only path (e.g. "C:\inetpub\wwwroot\index.html", no further change needed) was reported as changed=false even though the output differs from the true original input. See corazawaf#1656 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: close ADS trailing-slash bypass and empty-segment leak in normalisePathWin stripWindowsADS looked for the last '/' to bound the final segment, but normalisePath preserves a trailing separator, so a path ending in '\' after an ADS suffix put an empty final segment past it and the colon search always failed, leaving the ADS suffix unstripped. stripWindowsTrailingDotsAndSpaces also trimmed an all-spaces component down to an empty string but still emitted it, producing a double slash that a rule matching the normalized path would miss. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…azawaf#1658) * fix: encode cssDecode hex escapes as UTF-8, not a truncated byte cssDecodeInplace resolved a CSS hex escape (\HHHHHH, 1-6 hex digits) by taking at most the last two hex digits and writing them as a single raw byte, special-casing only the fullwidth-ASCII block. Per the CSS Syntax spec, a hex escape always represents one full code point, which any spec-compliant consumer (a browser) produces as that code point's actual UTF-8 encoding. Now the full code point is computed from all of the escape's hex digits, the fullwidth-ASCII fold (U+FF01-U+FF5E) is applied precisely against that value instead of guessed from a truncated view of it, and the result is UTF-8-encoded. This also fixes 3+ digit escapes whose resolved value didn't fit in the last two digits alone (e.g. \123 was truncated to the last-two-digit value 0x23 instead of the real 0x123), and out-of-range escapes (> 0x10FFFF) now resolve to the Unicode replacement character instead of an arbitrary leftover byte. The output can only shrink or stay the same size relative to the input for every digit count (1-6), so the existing in-place buffer approach remains safe. See corazawaf#1654 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: resolve zero-value CSS escapes to U+FFFD, fix resulting overflow Per the CSS Syntax spec, a zero-value escaped code point is invalid and must resolve to U+FFFD, same as a surrogate or an out-of-range value. Those two cases were already handled for free by utf8.EncodeRune, but code == 0 (a valid Unicode code point as far as Go's stdlib is concerned) was not, so "\0" decoded to a literal NUL byte instead. Naively special-casing code == 0 exposed a real bug in the in-place buffer: "\0" is 2 input bytes, but U+FFFD needs 3 output bytes, exceeding the fixed-size buffer's remaining room and panicking with an index-out-of-range on this trivial input. cssDecodeInplace now grows into a separate buffer rather than assuming output can never exceed the input's own backing array -- true for every other escape (more hex digits are always needed to reach a larger code point than the UTF-8 bytes it encodes to), but not for this one. Found by CodeRabbit review on corazawaf#1658. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com>
…razawaf#1683) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* fix(corazawaf): parse Content-Type media type for body processor selection AddRequestHeader compared the entire lowercased Content-Type header against "application/x-www-form-urlencoded" with exact equality. A Content-Type carrying a parameter, e.g. "application/x-www-form-urlencoded; charset=UTF-8", failed the comparison, left REQBODY_PROCESSOR unset, and caused Coraza to evaluate phase 2 without parsing the body while a backend using net/http's form parser processed it normally (GHSA-w253-m66g-rx24). Use mime.ParseMediaType to extract the normalized media type before comparing, for both the URLENCODED and MULTIPART cases. Also fixes special_urlencoded.go registering its profile under the same Meta.Name as posturlencoded.go ("posturlencoded.yaml"). Profiles are stored in a map keyed by name, so one silently overwrote the other and special_urlencoded.yaml's test never actually ran. Renamed it to match its filename, consistent with every other profile in the package, so both run and the new regression case is observable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(corazawaf): match ModSecurity's prefix-based Content-Type selection mime.ParseMediaType tightened body-processor selection so that a Content-Type with a malformed or duplicate parameter (e.g. a repeated "boundary", or trailing garbage after a valid media type) selected no processor at all, silently skipping request body parsing instead of surfacing REQBODY_ERROR/MULTIPART_STRICT_ERROR. ModSecurity itself selects the urlencoded processor with a plain prefix comparison (strncasecmp on "application/x-www-form-urlencoded"), and Coraza's prior multipart handling used the same loose prefix match. Restore the lowercase-and-prefix comparison for both URLENCODED and MULTIPART selection. The body processors already call mime.ParseMediaType themselves to extract the boundary/params, so a malformed header still surfaces as an error there; only the initial "should we even attempt to parse this body" decision goes back to matching ModSecurity's semantics. Adds regression cases for a urlencoded Content-Type with a malformed trailing parameter, and multipart headers with a duplicate boundary parameter or trailing junk, addressing review feedback on PR #1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: prevent ARGS rule bypass via parameter flooding (GHSA-6r3q-mjv7-xr8m) When the per-collection argument count reached SecArgumentsLimit, AddGet/Post/PathRequestArgument silently dropped excess arguments with no observable flag, allowing attackers to evade ARGS-targeted rules by flooding requests with filler parameters. - Add ARGUMENTS_LIMIT_REACHED transaction variable, set to "1" when any Add*Argument method drops due to the limit - Add compensating rules in coraza.conf-recommended (phase:1 for GET/PATH, phase:2 for POST) that deny on the flag - Fix urlencoded body processor to enforce ArgumentLimit (previously bypassed checkArgumentLimit by writing directly to ArgsPost) - Make ExtractGetArguments order-deterministic via ParseQueryOrdered so argument drops are predictable (tail gets dropped, not random) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: bound JSON/urlencoded parsing and same-key arguments (GHSA-3ww9-vw83-9w5x) Extends the ARGS-flooding fix in this PR to close the overlapping GHSA-3ww9-vw83-9w5x report and two deeper gaps found while verifying it: 1. JSON body processor (the actual reported bug): readJSON/readItems flattened every field into ARGS_POST/RESPONSE_ARGS with no bound, so a small body decoding to a wide flat array (e.g. [1,1,1,...]) expanded into millions of entries regardless of SecArgumentsLimit, exhausting memory on a single request. Both now stop once ArgumentLimit entries are collected -- checked before iterating a level and inside each ForEach step, so nested structures stop too, not just the top level. ArgumentLimit is now threaded into BodyProcessorOptions for both the request and response call sites (response was previously passed an empty BodyProcessorOptions{}). 2. ParseQuery/ParseQueryOrdered still built their entire result before any caller-side cap ran: a query string or urlencoded body with millions of pairs already spent the memory during parsing itself, before urlencoded.go's or ExtractGetArguments's own limit checks ever got a chance to run. Both now accept a limit and stop parsing immediately once it's reached, so the intermediate map/slice itself never grows past it. urlencoded.go's own copy-out loop, now redundant, is simplified back to a plain copy. 3. checkArgumentLimit (and AddResponseArgument's equivalent check) compared against Len, which counts distinct keys -- Map.Add appends repeated-key values into the same entry without growing it, so "a=1&a=1&a=1..." never tripped the limit no matter how large it grew. Added Map.TotalValues (cheap even under this attack: it sums len(slice) per key, so its cost is bounded by distinct keys, not by how many values piled up under any one of them) and switched both checks to use it. ParseQuery/ParseQueryOrdered's new limit parameter counts total pairs, not distinct keys, so the parse-level fix in (2) independently closes the same-key gap for query strings too; (3) additionally closes it for any direct AddGetRequestArgument/AddPostRequestArgument/ AddPathRequestArgument/AddResponseArgument caller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: close the argument-limit escape and bound the flattened JSON Three findings from review, all in the JSON body processor. The array-length entry is written after ForEach returns, outside the guards inside the callback, so one entry escaped SecArgumentsLimit per array level and iterationTruncated was never set. 1024 nested arrays in a 2049 byte body produced 1025 arguments with truncated=false, so rules 200004/200005 -- the whole point of this change -- never fired. The write is now guarded and sets the flag. SecArgumentsLimit counts entries, not bytes. The flattened key is the full path rewritten for every leaf, so memory grows with depth x leaves while the count grows only with leaves. A 34506 byte body of long paths retained 1.6 MB across 999 arguments: under the limit, no flag, allowed. readJSON now also bounds the flattened form at flattenBytesFactor times the body size, checked per leaf rather than per call -- a wide object is walked entirely inside one ForEach, so an entry guard never re-evaluates while those writes accumulate. That body now stops at 167 arguments and 0.26 MB. A 13 KB API-shaped payload is untouched at 801 arguments, and a test guards that. The factor of 8 is a judgement call, not a derived number, and is the part of this worth arguing about. The third finding was that readItems runs before the validity check, so an invalid body is walked in full before being rejected. That ordering is deliberate: a body with a valid prefix still populates ARGS on a best-effort basis, which TestJSONProcessResponseBestEffortOnInvalidJSON asserts and corazawaf#1615 asked for. Reverting it fails that test. The cost, which is the real complaint, is what the byte budget bounds -- so the ordering stays and the walk is now short. Also from review: rule 200004 is phase 1 and header-driven, so it returns 431 rather than 413, and SecArgumentsLimit is documented in the recommended config, commented out so nothing changes by default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: quadratic checkArgumentLimit, false-positive truncation, status codes Addresses jptosso's post-approval review on this PR: - Map.TotalValues() recomputed by walking every distinct key on each call, and checkArgumentLimit calls it once per argument added, making argument insertion quadratic in the number of distinct keys (measured 1701ms for 20k distinct GET args). TotalValues is now backed by a counter maintained incrementally by Add/Set/SetIndex/Remove/Reset. - ParseQueryOrdered/doParseQuery checked the limit before skipping an empty key, so a trailing separator right at the limit ("a=1&b=2&&") reported truncated=true even though nothing more was dropped. The check now runs after the empty-key skip, immediately before a pair is actually added. - Rules 200004/200005 return 400 instead of 431/413, matching every other deny in coraza.conf-recommended (200002, 200003). - Documented that the JSON byte-budget heuristic can trip ARGUMENTS_LIMIT_REACHED independently of the configured argument count, so operators can tell the two causes apart. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(corazawaf): default ARGUMENTS_LIMIT_REACHED to "0" like its siblings Every other request-scoped error flag (urlencodedError, uriParseError, reqbodyError, ...) is initialized to "0" in WAF.NewTransaction, but argumentsLimitReached was left at its zero value (""). @eq comparisons are unaffected (strconv.Atoi("") and Atoi("0") both yield 0), but logdata:'%{ARGUMENTS_LIMIT_REACHED}' and any string-based rule against the raw value would see "" instead of "0" before the limit is ever hit. Found by M4tteoP on PR #1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(bodyprocessors): bound JSON recursion on response bodies (GHSA-3c6w-j9xm-8h2h) ProcessResponse passed a -1 "no limit" sentinel into readItems' depth guard, which only fires on == 0, so the guard never triggered. A deeply nested JSON response body (O(depth^2) parsing cost) could hold one CPU core for ~12s per 512 KiB response with SecResponseBodyAccess On, a CPU-exhaustion DoS. Mirrors the request-side fix from ADR-0030: adds SecResponseBodyJsonDepthLimit (default 1024), a ResponseBodyJsonDepthLimit WAF field, and a ResponseBodyRecursionLimit BodyProcessorOptions field, and wires ProcessResponse to use it instead of the unbounded sentinel. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(seclang): correct variable name in SecResponseBodyJsonDepthLimit doc The doc comment referenced RESBODY_ERROR, but the response-side variable defined in this codebase is RES_BODY_ERROR (internal/variables), matching the request-side directive's REQBODY_ERROR naming inconsistency that already exists between the two variables. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bodyprocessors): floor JSON recursion limit at the shared primitive readItems only guarded maxRecursion == 0, so a negative limit never reached the guard and recursed unbounded -- the same bug class this advisory fixes, just one level down. WAF.Validate() rejects <=0 for callers that go through coraza.NewWAF(), but that left the primitive itself with no floor for any other caller. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(adr): renumber to ADR-0059, 0057 taken by another advisory fork Both this fork and coraza-GHSA-3wr7-993q-jrff independently picked 0057 via `ls docs/adr | tail` against their own branch point, since neither fork can see the other's uncommitted ADRs. 0058 is already claimed by coraza-GHSA-5gj4-9gm7-2fx2. Moving this one to 0059, the next free number across all currently open advisory PRs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix: honor filename* under any charset, not just utf-8/us-ascii mime.ParseMediaType only decodes a Content-Disposition "filename*" parameter when its declared charset is exactly "us-ascii" or "utf-8" (mediatype.go's decode2231Enc), silently leaving any other charset -- including "iso-8859-1", which RFC 5987 explicitly permits -- undecoded with no error. Coraza's originFileName relied solely on that stdlib behavior, so a request with a decoy plain "filename" alongside the real filename in "filename*" under an unrecognized charset would report the decoy: rules matching on the resulting FILES entry never saw the real name, and a filename* with no plain fallback wasn't even recognized as a file upload (it fell into ARGS_POST as a field instead). Add findFilenameStar, a quoted-string-aware scanner that locates "filename*" directly in the raw header text independent of stdlib's charset restriction, and percentDecodeLenient to decode its value. mime.ParseMediaType is still used for overall header validation (malformed headers, conflicting duplicate parameters); per RFC 7578 section 4.2, filename* takes precedence over filename when both parse. Also implement MULTIPART_FILENAME and MULTIPART_NAME for the first time (both were defined but never populated), and add two new collections, MULTIPART_FILENAME_CHARSET and MULTIPART_FILENAME_LANGUAGE, exposing filename*'s declared charset/language as-is so rule writers can enforce their own allowlist rather than the engine silently dropping anything it doesn't recognize. * feat: flag malformed and duplicated multipart part headers The filename* fix left two discrepancies invisible to rules. A part whose Content-Disposition is present but unparseable, or that carries a "filename*" not shaped "charset'[language]'value", was silently reduced to a part with no filename -- routed into ARGS_POST, outside FILES-scoped rule coverage, with no signal that anything was dropped. That is the same silent fallback class the advisory itself is about. Report both through a malformed flag on filenameFields and raise MULTIPART_STRICT_ERROR for them. An absent Content-Disposition is not flagged: such a part simply carries no filename. mime.ParseMediaType accepts every real-world filename shape tested (raw UTF-8, spaces, backslashes, bare '%', unquoted values), so this stays off legitimate traffic. Add MULTIPART_DUPLICATE_PART_HEADER, set when a part repeats a header (two Content-Disposition headers) or repeats a parameter inside its Content-Disposition (two "filename" parameters). Duplicates are where backends disagree on which value wins, so the duplicate itself is the signal. It also feeds MULTIPART_STRICT_ERROR. ModSecurity adds the same variable in its fix for GHSA-5pww-8rfg-9crf, and lists it in the audit log format as "DH"; without it a rule set referencing the variable fails to parse under Coraza. Duplicate parameters are only scanned once mime.ParseMediaType has already rejected the header -- the stdlib treats a repeated parameter name as a parse error -- so the extra allocation stays off the path taken by well-formed parts. findFilenameStar and the new hasDuplicateParam share one quoted-string-aware walk, forEachParam. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: cover filename* precedence end to end with CRS rule 933110 The unit tests assert on FILES and the MULTIPART_* collections directly. This drives the same payloads through the engine against CRS's own PHP-upload rule, transformations included, so the regression test fails if the fix stops producing the filename a backend would actually use -- not merely if an internal field changes shape. Reverting originFileName to the stdlib-only extraction fails the non-utf-8 charset case and the malformed-filename* case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: log MULTIPART_DUPLICATE_PART_HEADER from rule 200003 MULTIPART_DUPLICATE_PART_HEADER feeds MULTIPART_STRICT_ERROR, so rule 200003 in the recommended config already catches a duplicated part header or Content-Disposition parameter -- but the alert didn't say which of the two it was. It is the only strict-error contributor Coraza reports separately (the ModSecurity sub-flags for boundary quoting, header folding, LF lines and the rest are declared but never populated), so log it and nothing else: a value of 0 tells the operator the failure was a parser-level error rather than a duplicate. ModSecurity carries the same variable in its recommended config for GHSA-5pww-8rfg-9crf, as the "DH" field of its multipart flag list. Verified against the shipped config: the macro expands to 1 for a part with two "filename" parameters and 0 for a malformed filename*. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: cover the charset allowlist rule the advisory recommends The advisory tells operators to police filename*'s declared charset with an allowlist rule of their own, and nothing tested that the rule behaves. Ported from the equivalent coverage on ModSecurity's fix for the same issue (owasp-modsecurity GHSA-5pww-8rfg-9crf), which added it after review. Two cases: a plain upload with no filename* must leave the rule quiet, and a filename* declaring an unlisted charset must fire it. Both rule spellings are exercised, because they disagree and only one of them is a real guard. Coraza populates MULTIPART_FILENAME_CHARSET only for parts that carry a filename*; populating it with an empty string for every part would be the alternative. Under "!@Within utf-8,..." those two behaviours are indistinguishable, since an empty value is trivially within any haystack and the negation never fires. Under "!@rx ^(?:utf-8|...)$" they differ: an empty value does not match the anchored pattern, so the negated rule fires on every ordinary form field. Checked by mutation: switching the processor to populate the collections unconditionally fails the quiet case, and only through the regex form. The first version of this test used the advisory's "!@Within" spelling alone and passed under both behaviours, which is to say it guarded nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: pin the empty-charset case the advisory's rule now depends on filename*=''shell.php declares no charset. The allowlist rule the advisory recommended until now, "!@Within utf-8,iso-8859-1,us-ascii", never fires on it: @Within treats its parameter as the haystack and the variable as the needle, so an empty value is trivially contained and the negation is always false. An operator following that recommendation literally had no charset check against this input. The advisory now recommends the anchored regex form, which does fire. This case pins both halves of that: 200011 (regex) triggers, 200010 (@Within) does not. Coraza accepts an empty charset rather than rejecting it, by design -- the engine exposes what was declared and leaves the policy to the rule writer -- so this input is reachable and worth a regression test rather than an assumption. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: close three filename* discrepancies flagged in review - percentDecodeLenient now reports an unresolved "%" escape so originFileName raises MULTIPART_STRICT_ERROR instead of silently keeping a value that a backend resolving the same escape differently (or rejecting it, as ModSecurity's GHSA-5pww-8rfg-9crf fix does) would disagree with. - A quoted filename* value -- accepted by a general Content-Disposition parser such as Go's mime.ParseMediaType for any parameter, though RFC 5987 does not permit it for ext-value -- is now unwrapped before parsing, so Coraza's parsed filename matches what such a backend resolves instead of retaining a stray closing quote that breaks anchored rules. - MULTIPART_FILENAME/_CHARSET/_LANGUAGE/_NAME now accumulate per part (Add) instead of overwriting per name (Set), so two parts sharing one "name" (e.g. a multi-file input) both stay visible to rules. This is the same "second, distinct bypass" ModSecurity's GHSA-5pww-8rfg-9crf fix names and addresses by keeping these variables multi-valued. Found reviewing GHSA-3wr7-993q-jrff's fix against 23446b5e; verified against ModSecurity's GHSA-5pww-8rfg-9crf fix for parity. * feat: wire up MULTIPART_INVALID_QUOTING for a quoted filename* MultipartInvalidQuoting was declared for ModSecurity name compatibility but never backed by a collection or set anywhere. A quoted filename* value (filename*="UTF-8''shell.php") is unwrapped and used, per GHSA-3wr7-993q-jrff's fix, but the quoting is invalid RFC 5987 and was being accepted silently -- the same "silently accept an anomaly" pattern this advisory exists to close. It now sets MULTIPART_INVALID_QUOTING (audit-log code IQ in ModSecurity, mirroring its GHSA-5pww-8rfg-9crf fix) and contributes to MULTIPART_STRICT_ERROR, while still keeping the unwrapped value visible to rules rather than dropping it. Wires the variable through Collection(), the multiphase-evaluation readiness map, and the public plugintypes interface, the same path MultipartDuplicatePartHeader already takes. Updates the recommended rule 200003's logdata and comment, which claimed MULTIPART_DUPLICATE_PART_HEADER was the only such contributor. Raised in review by a ModSecurity maintainer on the linked advisory. * docs(adr): add ADR-0057 for GHSA-3wr7-993q-jrff multipart filename* fix Records the filename*-authoritative precedence decision and the new MULTIPART_FILENAME/_NAME/_CHARSET/_LANGUAGE variables, including @jptosso's unresolved review objection that this choice can relocate the bypass rather than close it for some backends. * chore(adr): accept advisory-fork permalinks in quote validation An ADR drafted alongside embargoed security work quotes review discussion on the private "coraza-GHSA-xxxx-xxxx-xxxx" fork, which doesn't exist under corazawaf/coraza until the advisory publishes. mage adr rejected that permalink outright, forcing a choice between skipping real discussion or failing the check on every embargoed branch. Widen the accepted permalink pattern to include the documented advisory-fork naming convention. * docs(agents): document the advisory-fork ADR permalink exception The ADR quoting rule and mage adr both said permalinks must point into this repository, which no longer matched what the checker accepts for an ADR drafted during embargoed security work (see the mage adr fix in the previous commit). Cross-reference the exception from both the ADR section and the Security section so it's found from either direction. * fix(bodyprocessors): expose both filename readings when filename* and filename disagree Making filename* unconditionally authoritative over the plain filename closes the advisory's original PoC but relocates the same bypass: swap which field carries the real name and a backend that resolves the plain filename instead is missed, since the discarded reading never reached FILES or MULTIPART_FILENAME at all. Verified against jptosso's review comment and repro on the fork PR. Both readings are now added to FILES/FILES_SIZES/MULTIPART_FILENAME for the same part when they differ, so a rule catches either interpretation instead of the engine picking a side. The plain filename can no longer be read from mime.ParseMediaType's dispositionParams once filename* is present: the stdlib decodes filename* itself for utf-8/us-ascii charsets and overwrites the "filename" map entry with that value, so it is parsed independently instead, the same way filename* already is. Every existing test only covered one payload order (decoy in filename, real name in filename*); added the swapped order as a dedicated case in both TestMultipartFilenameStar and the multipart_filename_star.yaml engine profile (CRS rule 933110), so the gap that let this ship once cannot silently reopen. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(adr): resolve the ADR-0057 precedence-flip objection jptosso's review objection (Technical Discussion) is now addressed by 89399e67 rather than left open: update Decision Outcome, Technical Discussion and Consequences to reflect that both filename readings are exposed, and defer the optional MULTIPART_FILENAME_DISCREPANCY signal past this release since the blocklist path already covers the bypass without it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bodyprocessors): close three filename* gaps flagged in review - An extended filename* that decodes to the empty string (e.g. filename*=utf-8'') no longer misclassifies the part as a field: the part is now recognized as a file when either reading is non-empty, falling back to the plain filename when filename* is empty. PHP, Go mime/multipart, python-multipart and formidable all resolve this as a file. - An RFC 2231 numbered continuation (filename*0=, filename*0*=, ...) no longer silently drops or overrides the plain filename via mime.ParseMediaType's own partial, corrupting continuation handling. Full multi-segment reassembly/decoding was judged disproportionate to a form real browsers never emit; instead the plain filename is reparsed directly from the raw header and MULTIPART_STRICT_ERROR is raised so the discrepancy is surfaced rather than resolved silently. - MULTIPART_INVALID_QUOTING now defaults to "0" in WAF.newTransaction, matching MULTIPART_DUPLICATE_PART_HEADER -- otherwise logdata referencing it (as coraza.conf-recommended's rule 200003 does) printed an empty value for a part with no quoted filename* at all. All three are dedicated cases in TestMultipartFilenameStar, each confirmed to fail without its fix; the logdata default additionally becomes an engine profile case in multipart_filename_star.yaml. Raised by @M4tteoP in review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(adr): document M4tteoP's review on ADR-0057 Records the three gaps M4tteoP found in review (empty filename* misclassification, RFC 2231 continuation bypass, MULTIPART_INVALID_QUOTING default) and how each was resolved, alongside the existing jptosso precedence-flip discussion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(bodyprocessors): keep both readings for an RFC 2231 continuation The previous continuation fix discarded mime.ParseMediaType's own reading of a single-piece continuation entirely, even though that reading is reliable for a utf-8/us-ascii continuation (the same condition already trusted for the bare filename* case). For filename="safe.jpg"; filename*0="shell.php" (or the *0*=utf-8'' equivalent), that silently lost "shell.php" -- a value Coraza used to surface before the continuation fix even existed, and one a Go mime/multipart backend still resolves. The independently-reparsed plain filename stays primary (it's the only reading Coraza can vouch for without decoding the continuation itself), and mime.ParseMediaType's own reading is now kept as altFilename when it differs, reusing the same dual-reading mechanism already in place for the bare filename* case. Two cases added to TestMultipartFilenameStar covering both continuation forms with a plain-filename decoy, each confirmed to fail without this fix. Raised by @M4tteoP in a follow-up review comment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(adr): document M4tteoP's follow-up on the continuation fix Records the follow-up finding that the first RFC 2231 continuation fix dropped mime.ParseMediaType's own (sometimes reliable) reading, and how it was resolved by keeping both readings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(bodyprocessors): preserve both values on a JSON-flattened key collision
readItems flattens nested JSON into a dot-joined path (e.g.
json.account.role) using a plain map, so a literal property name
containing a dot ({"account.role": "x"}) produces the identical
flattened key as the equivalent nested path
({"account": {"role": "x"}}). Whichever property gjson visited second
silently overwrote the other's value before ProcessRequest ever copied
the result into ARGS_POST, letting an attacker hide a malicious nested
value behind an innocuous top-level decoy while a standard JSON parser
still exposes both properties separately to the backend.
readItems's intermediate map is now map[string][]string; a write to an
existing key appends instead of overwriting, and ProcessRequest/
ProcessResponse copy every value into ARGS_POST/RESPONSE_ARGS via
SetIndex(key, i, value) per index. A colliding key becomes a
multi-valued collection entry, the same pattern already used for
MULTIPART_FILENAME (ADR-0057), so a rule matching that variable is
evaluated against every value instead of just the last one written.
Escaping the '.' separator in literal property names was tried first,
since the advisory names it as the primary remediation, but reverted:
it breaks OWASP CRS rule 944130, which matches suspicious literal
property names (e.g. Java class names used in deserialization attacks)
as a substring of the generated key. Preserving both values instead
closes the reported bypass without changing what a non-colliding key
looks like, so that detection is unaffected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* docs(adr): add ADR-0058 for GHSA-5gj4-9gm7-2fx2 JSON key collision fix
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…razawaf#1724) hasBeginAnchor/hasEndAnchor only prove a pattern is anchored somewhere; prefilterFunc's allRequired case assumed that whenever a pattern was anchored, the first/last literal it managed to extract sat immediately next to that anchor and built a HasPrefix/HasSuffix check on that assumption. That doesn't hold when other, non-extractable children (an alternation branch with no literal, a trailing .*, ...) sit between the literal and the anchor. CRS rule 933110's pattern .*\.ph(?:p\d*|tml|ar|ps|t|pt)\.*$ hit this exactly: hasEndAnchor was true and the last extracted literal was ".ph", so the prefilter required strings.HasSuffix(s, ".ph") -- but "shell.php" ends in "php", not ".ph", so the prefilter rejected it outright and the regex never ran. That's a false negative: a WAF bypass whenever SecRxPreFilter is enabled, violating the module's own documented safety guarantee that a prefilter bug can only degrade performance, never miss a match. Add literalAtStart/literalAtEnd to verify the extracted literal is the actual node adjacent to the anchor in the concatenation before enabling the prefix/suffix optimization; anything else conservatively falls back to the substring-scan prefilter (or the full regex), matching the existing "when in doubt, run the regex" design principle. Add regression rows to TestPrefilterNeverCausesFalseNegatives, confirmed to fail without the fix. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
readJSON calls gjson.Valid(s) unconditionally once readItems returns without error. readItems's own recursion guard never fires when the byte-budget or argument-limit guards (GHSA-3ww9-vw83-9w5x, GHSA-6r3q-mjv7-xr8m) truncate the walk before it recurses into a deeply nested tail later in the same document: the ForEach loop stops with truncated=true, err=nil without ever visiting that tail. gjson.Valid then recurses over the whole raw body with no depth bound of its own, and a body under the recommended SecRequestBodyLimit can crash the process with an unrecoverable fatal error: stack overflow (GHSA-6gcq-wc29-5xf2). Add jsonNestingExceedsLimit, an iterative (non-recursive) bracket-depth pre-scan, and run it before gjson.Valid: when the raw body nests past maxRecursion, return the same "max recursion reached" error readItems would have produced, without ever calling Valid. Applies to both ProcessRequest and ProcessResponse, which share readJSON. Verified against the original 13MB repro (1000 shallow scalars followed by 13,000,000 nested '[' characters): crashes the process before this fix, returns a clean error in <50ms after it. Full suite, build-tag matrix (coraza.no_memoize, coraza.rule.multiphase_evaluation, coraza.rule.no_regex_multiline) and testing/coreruleset all pass. See ADR-0060. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…commended (corazawaf#1727) improve coraza.conf
…ial truncation (corazawaf#1725) * no fp on processpartial truncation * bumps * address review, fix tests * add as transaction vars * fix, process partial best effort
* fix(bodyprocessors): use Add instead of SetIndex for JSON collection writes
ProcessRequest and ProcessResponse copy readJSON's flattened
map[string][]string into ARGS_POST/RESPONSE_ARGS via
col.SetIndex(key, i, value) per index, per key.
ARGS_POST is case-insensitive by default (case-sensitive only under
coraza.rule.case_sensitive_args_keys) and RESPONSE_ARGS always is,
regardless of that tag. Two flattened keys that differ only by case --
e.g. "json.account.role" vs "json.account.Role" -- are distinct entries
in readJSON's own case-sensitive intermediate map, but fold to the same
collection bucket once written through SetIndex. Each SetIndex(key, i,
value) call addresses index i of whatever bucket key case-folds to, with
no knowledge that a different-cased key also writes there. Iteration
order over the source map is randomized, so whichever key is visited
second overwrites index 0 of whichever was visited first --
deterministically leaving exactly one survivor every request, just an
unpredictable one.
Over 200 trials of {"account":{"role":"1' OR '1'='1","Role":"safe"}},
the attack value survived only 22 times (11%).
Use col.Add(key, value) instead: Add always appends regardless of any
index, so both a same-case collision (readJSON's own ordered slice per
key) and a case-folding collision (two different source keys landing in
the same bucket) end up with every value preserved. The existing
order-sensitive regression test for the original same-case collision
(TestJSONProcessRequestDottedKeyDoesNotHideNestedValue) still passes
unchanged, since a single source key's own value order is unaffected by
switching from indexed writes to appends.
Adds TestJSONProcessRequestCaseVariantKeyDoesNotHideNestedValue,
confirmed failing deterministically against the pre-fix code (always
exactly 1 survivor) and passing after it. Amends ADR-0058 (still
proposed) with the same follow-up note.
Follow-up to GHSA-5gj4-9gm7-2fx2; addendum added to the advisory rather
than a new one, since this is the same collision mechanism the advisory
already documents.
Full suite, build-tag matrix (coraza.no_memoize,
coraza.rule.multiphase_evaluation, coraza.rule.no_regex_multiline),
testing/coreruleset and lint all pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* test(bodyprocessors): merge JSON key collision tests into one table
Address review: the dotted-key and case-variant collision tests become rows
of TestJSONProcessRequestKeyCollisionDoesNotHideNestedValue, with an ordered
flag for the dotted row. The case-variant row unions the values of both key
spellings, so it no longer fails under coraza.rule.case_sensitive_args_keys,
where the keys do not collide. ADR-0058 now names the table.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* fix(bodyprocessors): enforce byte budget on JSON array-length write The array-length summary entry written after each gjson.ForEach call (the "if arrayLen > 0" block in readItems) checked argumentLimit but never byteBudget, unlike the per-leaf write three lines above it. objKey (the full flattened path) grows by roughly a fixed amount per nesting level while argCount grows by only one per level -- exactly the amplification byteBudget exists to bound. A long property name nested under many single-element arrays inflates memory far past the budget while argumentLimit alone never trips, since every nesting level counts as only one argument however long its path. A 20,405-byte body (a ~20KB property name under 200 levels of single-element arrays) produced 4,020,596 bytes of flattened keys (~197x the body size) with truncated=false before this fix; after it, the same body is bounded to 163,160 bytes with truncated=true. Move the byte-budget check into the existing argumentLimit branch, computing lenStr first so its length is known before the check, mirroring the per-leaf write's own check. ProcessRequest and ProcessResponse both call readJSON, so both are fixed by the same change. Incomplete-fix follow-up to GHSA-6r3q-mjv7-xr8m / GHSA-3ww9-vw83-9w5x; addendum added to the advisory rather than a new one, since this is the same underlying guard this advisory already introduced. Full suite, build-tag matrix (coraza.no_memoize, coraza.rule.multiphase_evaluation, coraza.rule.no_regex_multiline), testing/coreruleset and lint all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(bodyprocessors): fold array-length byte budget case into existing table Address review: the regression case becomes a t.Run row of TestReadJSONBoundsFlattenedBytes, which owns the byte budget, and asserts against len(body) * flattenBytesFactor instead of a hard-coded bound. The table now sums value lengths rather than counting values, and the guard comment in readItems is shortened. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…azawaf#1726) hasDuplicateParam tracked seen Content-Disposition parameter names in a slice and scanned it for every new name. Use a set instead, so the check is linear in the number of parameters. Duplicate detection is unchanged; table rows now pin how empty parameter names are handled. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* fix(corazawaf): only the first Content-Type header selects the body processor
AddRequestHeader is called once per header, and its content-type case
ran on every call, unconditionally overwriting reqbodyProcessor. A
request with two Content-Type headers -- e.g. a genuine
"multipart/form-data" followed by a spoofed
"application/x-www-form-urlencoded" -- had its body-processor
selection silently overwritten by the last one.
Meanwhile ProcessRequestBody's own mimeType (passed to whichever
processor gets selected) comes from
requestHeaders.Get("content-type")[0] -- the first value -- and Go's
net/http Header.Get (what a typical backend uses) also reads only the
first value. Selection followed the last header while everything else
followed the first, so a two-Content-Type request desynchronized what
Coraza inspected from what the backend parsed: Coraza ran the
URL-encoded processor against a genuine multipart body, which parses
without error and produces nothing, leaving ARGS_POST, FILES and
FILES_NAMES all empty while the backend received and parsed the real
multipart fields (a file upload and a form value) untouched.
Guard the existing logic with reqbodyProcessor.Get() == "" so only the
first Content-Type header can set it, matching mimeType's own
first-value semantics. No new field needed: ctl:requestBodyProcessor
and ForceRequestBodyVariable both run after header processing
completes and are unaffected.
Adds TestDuplicateContentTypeHeaderUsesFirstForBodyProcessorSelection
in internal/corazawaf (not a testing/engine profile:
StageInput.Headers is map[string]string and cannot express two
Content-Type headers on one request). Confirmed failing
deterministically against the pre-fix code and passing after it.
Follow-up to GHSA-w253-m66g-rx24; addendum added to the advisory
rather than a new one, since this is the same body-processor-selection
code the advisory already covers.
Full suite, build-tag matrix (coraza.no_memoize,
coraza.rule.multiphase_evaluation, coraza.rule.no_regex_multiline),
testing/coreruleset and lint all pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(corazawaf): select body processor from the first Content-Type header only
The previous guard (reqbodyProcessor empty) picked the first *recognized*
Content-Type, not the first one: "text/plain" followed by
"multipart/form-data" still selected MULTIPART while mimeType stayed
"text/plain". requestHeaders.Add runs before the switch, so gate on the
header count being 1 instead.
Table-drive the regression test and add the unrecognized-first rows,
which fail against the previous guard.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
* docs(variables): fix stale multipart variable docs MULTIPART_FILENAME_CHARSET recommended a !@Within allowlist, which an empty charset bypasses; use an anchored !@rx instead. MULTIPART_FILENAME no longer claims filename* takes precedence, and MULTIPART_NAME is no longer marked as not implemented. * chore: go mod tidy
…or (corazawaf#1730) bytes budget error Signed-off-by: Matteo Pace <pace.matteo96@gmail.com>
… SecArgumentsLimit (corazawaf#1729) * json array length arg limit, split limit * more * concise comments
…azawaf#1722) * docs(security): score exploitability preconditions during triage Most incoming reports propose a High/Critical CVSS score based only on the impact once the bug fires, leaving out what has to be true beforehand for an attacker to reach it: a specific backend behavior outside the attacker's control, or a PoC that only reproduces under one narrow, pinned environment (an exact runtime patch version, an unrelated framework quirk) rather than the affected code path in general. Adds explicit triage guidance to SECURITY.md (reporter-facing) and AGENTS.md (agent-facing) so both preconditions are checked and the score/affected- version range corrected accordingly, independent of what the report proposed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(security): assess AC:H for configs and chains instead of assuming it Score Attack Complexity per the CVSS version's own metric guidance rather than treating a non-default configuration or a chained-in bug as automatically High: a required configuration is scored assuming the vulnerable component already has it (unless the configuration deliberately weakens security or conflicts with vendor guidance), and a chained bug only earns High if the chain itself is uncertain or effortful to execute. Also removes privileged access from the Attack Complexity examples in SECURITY.md — that precondition is scored under Privileges Required, not Attack Complexity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(security): split CVSS v3.1 Attack Complexity from v4.0 Attack Requirements CVSS v4.0 introduces a separate Attack Requirements metric for deployment and runtime prerequisites (a required configuration, a specific system state), narrowing Attack Complexity to built-in protections the attacker must evade or circumvent. v3.1 has no such split and folds those prerequisites into Attack Complexity. Clarify which metric carries a required-configuration precondition under each version, while keeping the existing assumption that scoring treats the vulnerable component as already in that configuration either way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(docs): don't map a required configuration onto AC/AT by CVSS version Both CVSS v3.1 and v4.0 say explicitly that Base metrics are scored assuming the vulnerable component is already in whatever configuration the attack requires (FIRST's spec: "the vulnerable system should be assessed assuming it is in that configuration"). Neither Attack Complexity nor Attack Requirements is about the system's own configuration; a required configuration is simply excluded from both, in both versions. The previous wording said the opposite: that v3.1 "folds" a required configuration into Attack Complexity and v4.0 "records" it as Attack Requirements: Present, just under different metrics depending on version. That's an incorrect reading of a required configuration as itself a scorable precondition, when it's assumed away in the base score regardless of version. Replaces that mapping with what each metric actually covers per version: v3.1's Attack Complexity is attacker-side difficulty (target reconnaissance, a race condition); v4.0 narrows that to built-in protections the attacker must evade, and splits deployment/execution conditions (a race condition, an on-path network position -- not a configuration choice) into the new Attack Requirements metric. Verified against FIRST's CVSS v3.1 and v4.0 specification documents. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…1721) Enables CodeRabbit to merge repository-level review settings with OWASP's organization-level configuration instead of relying solely on repo defaults. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
originFileName only checked for RFC 2231 continuations (filename*0=, filename*0*=, ...) when no bare filename* was present. With both, the continuation was ignored and MULTIPART_STRICT_ERROR stayed unset, while Werkzeug/Flask lets the continuation win, e.g. filename="safe.jpg"; filename*=iso-8859-1''x.jpg; filename*0*=iso-8859-1''shell.php reaches a Flask backend as shell.php while Coraza's FILES holds only x.jpg and safe.jpg. Set malformed for this case too, matching the continuation-only path. Follow-up to GHSA-3wr7-993q-jrff. Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Body processor selection compared the raw Content-Type value against the media-type prefixes. mime.ParseMediaType, which a typical backend uses, trims with strings.TrimSpace, so a leading Unicode space (U+0085, U+00A0, U+3000) that net/http accepts made Coraza skip body processing while the backend still parsed the body as a form or multipart. Follow-up to GHSA-w253-m66g-rx24. Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* fix(cookies): keep cookies whose name trims to empty The CTL trim from GHSA-g4qm-m288-5cp9 turns a name made only of control characters ("\x01=payload") into an empty name, and ParseCookies skipped empty names, so the pair was dropped and its value never reached REQUEST_COOKIES. v3.7.0 inspected it under the name "\x01". The plain empty-name case ("=payload") was already dropped before. Node's cookie package (0.7.2 and 2.0.1) still hands both to the application, as {"\u0001": "payload"} and {"": "payload"}, so a payload in either shape reached Express backends uninspected. Keep such a pair under the key "" instead. Only "=" with nothing on either side is still skipped, since there is nothing to inspect. Also correct the comments that said Node's cookie package and Werkzeug split "a\v=x" as name "a": both keep "a\v". Python's http.cookies does split it that way, which is the precedent the trim follows along with RFC 6265. * fix(corazawaf): don't let a regex ctl exception match an empty key parseCtl clears the key string when the ctl target uses a regex, so the exception carries KeyStr "" alongside KeyRx. The exception check compared KeyStr against every key regardless, and "" == "" matched any argument or cookie named "". A removal such as ctl:ruleRemoveTargetById=100;REQUEST_COOKIES:/^__utm/ therefore also hid "=payload" and "\x01=payload" cookies from rule 100, undoing the previous commit. ARGS was affected the same way ("?=payload") since v3.0.0. Compare KeyStr only when there is no KeyRx. The in-rule form !VAR:/regex/ keeps the raw pattern in KeyStr and was not affected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
DEF-57620: sync the fork with upstream Coraza v3.8.1, which includes the v3.8.0 and v3.8.1 security fixes. Every fork patch is kept: rule filter, TruncateRequestBody (DEF-51195), persistent collections (DEF-51172), body limits and leaks (DEF-51539), XML void elements (DEF-51266), RBL (DEF-52893, DEF-52371) and the part-H audit messages.
ggrigoras-ops
marked this pull request as ready for review
October 12, 2026 14:41
cl-vkuznetsov
approved these changes
Oct 13, 2026
…er merging DEF-57619 TestTransactionFieldCount: 37 fields since PR #24 added canceled. RuleGroup.Eval: check the transaction context only for a rule about to run, not for rules of other phases or skipped ones.
ggrigoras-ops
changed the title
ggrigoras-ops
changed the title
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.
Syncs the fork with upstream Coraza v3.8.1.
What
Merges upstream tag
v3.8.1(which includes v3.8.0) into the fork. 240 files, 20 textual conflicts, all resolved with every fork patch kept: rule filter,TruncateRequestBody, persistent collections, body limits and leaks, XML void elements, RBL and the part-H audit messages.Why
Upstream published 12 security advisories in v3.8.0 and v3.8.1 (two High: GHSA-6r3q-mjv7-xr8m, GHSA-6gcq-wc29-5xf2).
Conflict decisions
Upstream first for APIs, data structures and the security fixes. Where the fork's behaviour is observable it stays:
ErrJSONDecodedSize,ErrJSONRecursionLimitandMaxEntriesPerArgumentare removed. A limit trip on a valid body isErrJSONLimitand logs at Warn, as the fork's no-error-log-for-valid-bodies rule requires.ErrArgumentsLimit, so REQBODY_ERROR "SecArgumentsLimit exceeded" stays fail-closed. They also set ARGUMENTS_LIMIT_REACHED, which is upstream's signal for the new rules 200004 and 200005 incoraza.conf-recommended.TX:json_request_body/json_response_bodybefore returning a parse error (fork behaviour, for@validateSchema).url.EachQueryValue);internal/urlcarries both it and upstream'sParseQuery(limit).TransactionVariablescarries the union of both sides' variables;URI_PARSE_ERRORandARGUMENTS_LIMIT_REACHEDnow followSCRIPT_USERNAMEin the enum.Behaviour changes
SecResponseBodyJsonDepthLimitis new and separate from the request depth limit.coraza.conf-recommendedneeds its own rule on ARGUMENTS_LIMIT_REACHED.Tests
Existing tests only; no new ADR (upstream's ADR-0057 to 0060 arrive with the merge, numbers do not collide).
json_integration_test.go,urlencoded_test.go) now expectErrArgumentsLimit, the fork's contract.TestReadJSONBoundsFlattenedBytesandTestReadJSONArrayLengthRespectsArgumentLimit:TestJSONAmplifyingNestingStopped,TestJSONArrayLengthChargedAgainstDecodedBytes,TestJSONArrayLengthNotCountedAsArgument,TestJSONArrayLengthEntriesBounded,TestJSONDecodedBytesBudgetScalesWithSmallBodies. Coverage of those shapes is upstream's.TestReadJSON/bombreturns early under TinyGo: pristine v3.8.1 kills the TinyGo 0.40.1 test binary on that row as well, whilev3.7.0-cl5passed it.Verification
On the merged tree, locally (Actions is disabled on the fork):
go build ./...,go vet ./...,go test ./...,go test -race ./...,go run mage.go lint,go run mage.go test(default and no_memoize, CRS under default, no_memoize, multiphase and no_regex_multiline,TestRx*andTestCaseSensitive*),tinygo test -short ./internal/...with and withoutcoraza.no_memoize.No performance claim is made.
Not included
Request-phase cancellation (#24) and the
@rx/@pmwork (#26, #27). The next fork tag is cut after them.