feat: add JSON Stream (NDJSON) body processor#1481
Conversation
Implements a new body processor for handling streaming JSON formats:
- NDJSON (Newline Delimited JSON)
- JSON Lines
- JSON Sequence (RFC 7464)
Features:
- Line-by-line processing for memory efficiency
- Each JSON object indexed by line number (json.0.field, json.1.field)
- Built-in DoS protection with 1024 recursion limit
- TX variables for raw body and line count
- Support for nested objects and arrays
- Comprehensive error handling
Configuration:
- Added rules to coraza.conf-recommended for NDJSON content types
- Optional line count limiting rule
- Registered under JSONSTREAM, NDJSON, and JSONLINES aliases
Testing:
- 13 comprehensive test cases covering:
- Single/multiple lines
- Nested objects and arrays
- Error cases (invalid JSON, empty stream)
- Recursion limit enforcement
- TX variable storage
- Benchmark: ~5,000 ops/sec for 100-object streams
Usage example:
SecRule REQUEST_HEADERS:Content-Type "^application/x-ndjson" \
"id:'200007',phase:1,pass,nolog,ctl:requestBodyProcessor=JSONSTREAM"
Closes: Related to streaming JSON support discussion
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1481 +/- ##
==========================================
- Coverage 85.30% 84.07% -1.24%
==========================================
Files 174 175 +1
Lines 8461 8811 +350
==========================================
+ Hits 7218 7408 +190
- Misses 994 1146 +152
- Partials 249 257 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for JSON Stream (NDJSON) body processing to Coraza WAF, enabling line-by-line processing of streaming JSON formats. The implementation includes a new body processor that handles NDJSON, JSON Lines, and claims support for JSON Sequence (RFC 7464).
Changes:
- New
jsonStreamBodyProcessorthat processes JSON objects line-by-line with memory-efficient streaming - Built-in DoS protection via configurable recursion limits (default 1024)
- TX variable storage for raw body and line count to enable custom validation rules
- Configuration rules in
coraza.conf-recommendedfor NDJSON content types
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| internal/bodyprocessors/jsonstream.go | Core implementation of NDJSON body processor with line-by-line parsing, recursion limits, and TX variable storage |
| internal/bodyprocessors/jsonstream_test.go | Comprehensive test suite with 13 test cases covering single/multiple lines, nested objects, arrays, error cases, and benchmarks |
| coraza.conf-recommended | Configuration rules for enabling NDJSON processing based on Content-Type headers, with optional line count limiting |
Memory Documentation:
- Add explicit documentation about 2x memory usage from TeeReader
- Clarify that this is necessary for TX variables (like regular JSON processor)
- Note memory implications: 2x body size (buffer + parsed variables)
Line Numbering:
- Use 1-based line numbers in error messages instead of 0-based
- More user-friendly: "line 1" instead of "line 0"
- Applied to both invalid JSON and parsing errors
Scanner Buffer Limit:
- Increase max scan token size from default 64KB to 1MB
- Prevents failure on large JSON objects per line
- Set initial buffer to 64KB, max to 1MB for memory efficiency
Configuration Consistency:
- Fix rule 200008 to use JSONSTREAM (was NDJSON)
- Now consistent with rule 200007
- Both rules use the same processor name
Test Code Quality:
- Replace string concatenation with fmt.Sprintf for line numbers
- Fix issue where rune('0'+tt.line) only works for single digits
- Add fmt import to test file
Documentation Accuracy:
- Remove RFC 7464 JSON Sequence from "supported formats"
- Add note that RS separator (0x1E) is not yet implemented
- Avoid misleading users about unsupported features
All tests passing: 13/13
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
|
Could we add this to experimental? |
|
I thought I mentioned this. Yes, that was my idea. |
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
- Extract inline interface to named indexedCollection type (jcchavezs) - Preserve original stream format in relay by including format-specific delimiters in rawRecord (NDJSON uses \n, RFC 7464 uses RS prefix + \n) - Update readItemsWithLimit TODO comments to reference #1110
|
@copilot Fix the conflicts first. Then add e2e tests to tests for this new body processor. |
* fix(deps): update module golang.org/x/net to v0.45.0 [security] (#1487) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update go modules in go.mod (#1433) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * docs(actions): update format and add package (#1475) * docs(actions): update format and add package Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: update documentation for package Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: go fmt Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: add A-Z to auditlog (#1479) Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: SecRuleUpdateActionById should replace disruptive actions (#1471) * fix: SecRuleUpdateActionById should replace disruptive actions Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: multiphase test with bad expectations Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * tests: improve coverage on engine Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * refactor: address SecRuleUpdateActionById review comments (#1484) * Initial plan * Address code review comments: improve documentation, fix double parsing, and fix range logic Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * Refactor: Extract hasDisruptiveActions helper to avoid code duplication Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * docs: Improve applyParsedActions documentation Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * docs: Clarify body parsing logic in SetRawRequest 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: fzipi <3012076+fzipi@users.noreply.github.com> * refactor: address review comments on SecRuleUpdateActionById - Rename ClearActionsOfType to ClearDisruptiveActions - Add comments explaining quote trimming in action parsing - Remove empty line after function brace in updateActionBySingleID - Split engine_test.go: move output/helper tests to engine_output_test.go * Apply suggestions from code review Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> * fix: use index-based iteration for SecRuleUpdateActionById range updates The range loop variable copied each Rule, so modifications to disruptive actions were lost. Use index-based iteration to modify rules in place. Also adds a test case exercising the range update path. --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> * refactor: remove root package dependency on experimental (#1494) * refactor: remove root package dependency on experimental Replace experimental.Options with corazawaf.Options in waf.go, breaking the import cycle that prevented the experimental package from importing the root coraza package. This unblocks PR #1478 and lets experimental helpers use coraza.WAFConfig with proper type safety instead of any. * Update waf.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * chore: min go version to 1.25 (#1497) * No content wants no body * Update .github/workflows/regression.yml Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> * one more place --------- Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> * feat: add optional rule observer callback to WAF config (#1478) * feat: add optional rule observer callback to WAF config Introduce an optional rule observer callback that is invoked for each rule successfully added to the WAF during initialization. The observer receives rule metadata via the existing RuleMetadata interface. * Move to the experimental package * Do not use reflection to keep the compatibility with older Go versions * Use coraza.WAFConfig, move the test to where it belongs. --------- Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com> * feat: add WAFWithRules interface with RulesCount() (#1492) Add WAFWithRules interface with RulesCount() * fix(deps): update module golang.org/x/net to v0.51.0 [security] (#1502) * fix(deps): update module golang.org/x/net to v0.51.0 [security] * chore: update go.work to 1.25.0 Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * chore: update golang to 1.25.0 Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Felipe Zipitria <felipe.zipitria@owasp.org> * chore(deps): update module golang.org/x/net to v0.51.0 [security] (#1506) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix: lowercase regex patterns for case-insensitive variable collections (#1505) * fix: lowercase regex patterns for case-insensitive variable collections When a rule uses regex-based variable selection (e.g. TX:/PATTERN/), the regex pattern was compiled from the raw uppercase string before any case normalization. Since TX collection keys are stored lowercase, the uppercase regex would never match, causing rules like CRS 922110 (which uses TX:/MULTIPART_HEADERS_CONTENT_TYPES_*/) to silently fail. Now AddVariable and AddVariableNegation lowercase the regex pattern before compilation for case-insensitive variables, matching the existing behavior for string keys in newRuleVariableParams. * chore: update coreruleset to v4.24.0 Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * chore: update libinjection-go and deps (#1496) * chore: update libinjection-go and deps Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * chore: update coreruleset v4.24.0 Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: ctl:ruleRemoveTargetById to support whole-collection exclusion (#1495) * Initial plan * Fix ruleRemoveTargetById to support removing entire collection (empty key) 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: fzipi <3012076+fzipi@users.noreply.github.com> * feat: add SecRequestBodyJsonDepthLimit directive (#1110) * feat: add SecRequestBodyJsonDepthLimit directive Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * Apply suggestions from code review * fix: mage format Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * Update internal/bodyprocessors/json_test.go * Update internal/bodyprocessors/json_test.go * fix: bad char Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: gofmt Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * docs: add clarifying comments for JSON recursion limit behavior - Explain why ResponseBodyRecursionLimit = -1 (unlimited for responses) - Document dual purpose of body reading (TX vars + ARGS_POST) - Clarify DoS protection mechanism in readItems() - Note how negative values bypass recursion check * fix: address PR review comments for JSON depth limit - Always enforce a positive recursion limit: change ResponseBodyRecursionLimit from -1 (unlimited) to 1024, matching the request body default - Rename test case "broken1" to "unbalanced_brackets" for clarity - Extract error check from the key iteration loop in TestReadJSON * test: add benchmarks for gjson.Valid pre-validation overhead Measures the cost of gjson.Valid() in the full readJSON pipeline. gjson.Parse is lazy (~9ns), so the real overhead is Valid vs the readItems traversal. Results show ~10-16% overhead for validation, which is acceptable for WAF safety. No single-pass alternative exists in the gjson API. * Apply suggestions from code review * Apply suggestion from @fzipi --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com> * fix: update constants for recursion limit (#1512) * fix: conflate the constants for recursion limit * fix: value setting * chore: remove panic from seclang compiler (#1514) * Initial plan * fix: replace panic with error return in parser.go evaluateLine Co-authored-by: jptosso <1236942+jptosso@users.noreply.github.com> * fix: revert go.sum changes - do not modify go.sum files in this PR Co-authored-by: jptosso <1236942+jptosso@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jptosso <1236942+jptosso@users.noreply.github.com> * ci: reduce regression matrix from 128 to 15 jobs (#1522) Replace dynamic 64-permutation tag matrix with a curated static list of 13 build-flag combinations. Run all combos on Go 1.25.x and only baseline + kitchen-sink on Go 1.26.x. Add concurrency groups to regression, lint, tinygo, and codeql workflows so stale PR runs are auto-cancelled on new pushes. * feat: ignore unexpected EOF in MIME multipart request body processor (#1453) * Ignore unexpected EOF in MIME multipart request body processor We need this behavior since we need to process an incomplete MIME multipart request body when SecRequestBodyLimitAction is set to ProcessPartial. * fix: add copilot code review comments Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com> Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> Co-authored-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: set changed flag in removeComments and escapeSeqDecode (#1532) Fix two bugs where transformation functions modified the input string but did not report changed=true: - removeComments: entering a C-style (/* */) or HTML (<!-- -->) comment block did not set changed=true, causing the multi-match optimization to skip the transformed result. - escapeSeqDecode: unrecognized escape sequences (e.g. \z) dropped the backslash but did not set changed=true. Add test coverage for both fixes including a new remove_comments_test.go and an additional unrecognized-escape test case for escape_seq_decode. * perf: use map for ruleRemoveByID for O(1) lookup (#1524) * perf: use map for ruleRemoveByID for O(1) lookup Replace []int slice with map[int]struct{} for the per-transaction rule exclusion list. The rule evaluation loop checks this list for every rule in every phase, making O(1) map lookup significantly faster than O(n) linear scan when rules are excluded via ctl actions. * test: add TestRemoveRuleByID for map-based rule exclusion * bench: add BenchmarkRuleEvalWithRemovedRules * refactor: use real unconditionalMatch operator from registry in tests * Fix HTTP middleware to process all Transfer-Encoding values (#1518) * Fix HTTP middleware to process all Transfer-Encoding values Co-authored-by: jptosso <1236942+jptosso@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jptosso <1236942+jptosso@users.noreply.github.com> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> * fix(deps): update module golang.org/x/sync to v0.20.0 in go.mod (#1543) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * feat: optimize ruleRemoveById range handling store ranges instead of expanding to int slices (#1538) * Initial plan * Optimize ruleRemoveById range handling to avoid generating massive int slices - Replace rangeToInts (which allocated []int of all matching rule IDs) with parseRange and parseIDOrRange helpers that return start/end integers - For ctlRuleRemoveByID with ranges: store the range in Transaction.ruleRemoveByIDRanges ([][2]int) and check it in the rule evaluation loop, avoiding both the intermediate []int and potentially large map expansions - For ctlRuleRemoveTargetByID: iterate rules once directly, eliminating the intermediate []int allocation - Add RemoveRuleByIDRange method to Transaction - Reset ruleRemoveByIDRanges on transaction pool reuse - Replace TestCtlParseRange with TestCtlParseIDOrRange to test the new helpers Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * Improve test coverage for range-based rule removal - Add TestRemoveRuleByIDRange in transaction_test.go: - range is stored in ruleRemoveByIDRanges - rules in range are skipped during Eval - multiple ranges work correctly - ruleRemoveByIDRanges is reset on transaction pool reuse - Add TestCtlParseRange in ctl_test.go to cover parseRange directly (including the no-separator and start>end error paths) - Add GetRuleRemoveByIDRanges() accessor on Transaction for cross-package test assertions - Enhance "ruleRemoveById range" TestCtl case to verify the range is stored - Add "ruleRemoveTargetById range" TestCtl case to verify range path works Coverage changes: parseRange: 83.3% → 100% parseIDOrRange: 100% (unchanged) RemoveRuleByIDRange: 0% → 100% 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: fzipi <3012076+fzipi@users.noreply.github.com> * fix(testing): Correct use of ProcessURI in Benchmarks (#1546) * perf: prefix-based transformation cache with inline values (#1544) Redesign the transformation cache to share intermediate results across rules with common transformation prefixes (e.g. rules using t:lowercase,t:urlDecodeUni reuse the t:lowercase result cached by an earlier rule using just t:lowercase). Key changes: - Add transformationPrefixIDs to Rule for backward prefix search - Cache every intermediate transformation step, not just the final result - Store cache values inline (not pointers) to avoid heap allocations - Fix ClearTransformations (t:none) to reset transformationsID Benchmarked against full CRS v4 ruleset (8 runs, benchstat): Allocations: -2% (small) to -19% (30 params) Memory: -2% (small) to -12% (30 params) Timing: -5% (small/large), neutral (medium) No regressions on any metric. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * perf: bulk-allocate MatchData in collection Find methods (#1530) * perf: bulk-allocate MatchData in collection Find methods Pre-allocate a contiguous []corazarules.MatchData buffer and take pointers into it instead of individually heap-allocating each MatchData. This reduces per-result allocations from N to 2 (one buf slice + one result slice), improving GC pressure for large result sets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf: avoid double regex evaluation in FindRegex Collect matching data slices during the counting pass so the second pass only iterates over already-matched entries, eliminating redundant MatchString calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * bench: add FindAll/FindRegex/FindString benchmarks --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> * perf: use FindStringSubmatchIndex to avoid capture allocations (#1547) * perf: use FindStringSubmatchIndex to avoid capture allocations Replace FindStringSubmatch (allocates a []string slice per match) with FindStringSubmatchIndex (returns index pairs). Substrings passed to CaptureField become slices of the original input — zero allocation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add BenchmarkRxCapture for submatch allocation comparison Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fix(DetectionOnly): fixed RelevantOnly audit logs, improved matchedRules (#1549) * add detectedInterruption var for DetectionOnly mode * IsDetectionOnly, refactor, populate matchedRules * nit * Apply suggestions from code review Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> --------- Co-authored-by: Romain SERVIERES <romain@madeformed.com> Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> * fix(deps): update module golang.org/x/net to v0.52.0 in go.mod (#1553) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * ci: increase fuzztime (#1554) * more fuzztime * go mod * chore(ci): harden GHA workflows with least-privilege permissions (#1559) - Add top-level `permissions: {}` (deny-all) to every workflow - Add scoped per-job permissions granting only what each job needs - Fix expression injection in regression.yml by using env instead of inline shell interpolation for BUILD_TAGS - Restrict regression.yml pull_request trigger to main branch only - Add explicit permissions to fuzz.yml (issues: write for failure reports) - Add security-events: write to CodeQL workflow * feat: enable regex memoize by default (#1540) * feat: enable regex memoize by default Memoization of regex and aho-corasick builders was previously opt-in via the `memoize_builders` build tag. Most users didn't know to enable it, missing a critical performance optimization. This commit: - Enables memoization by default (opt-out via `coraza.no_memoize` tag) - Refactors internal/memoize from package-level Do() to Memoizer struct - Adds Memoizer interface to plugintypes.OperatorOptions - Wires WAF's Memoizer through to all operator and rule consumers - Replaces `memoize_builders` build tag with `coraza.no_memoize` opt-out Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: document cache tradeoffs and add noop memoize test - Update README and memoize README to document global cache behavior and point to WAF.Close() for live-reload scenarios. - Add test file for coraza.no_memoize build variant to verify no-op behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add WAF.Close() with per-owner memoize cache tracking and scale benchmarks (#1541) * feat: add WAF.Close() with per-owner memoize cache tracking Add WAFCloser interface and per-owner tracking to the memoize cache so that long-lived processes can release compiled regex entries when a WAF instance is destroyed. Each WAF gets a uint64 ID; Release() removes the owner and tombstones entries with no remaining owners. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add memoize scale benchmarks and CRS integration tests Add benchmarks demonstrating memoize value at scale (1-100 WAFs × 300 patterns) and CRS integration tests verifying Close() releases memory. Results show ~27x speedup for 100 WAFs and 27MiB released on Close(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: add WAF.Close() calls to e2e and CRS tests Demonstrate proper WAFCloser usage in integration tests: e2e test, CRS FTW test, CRS benchmarks, and crsWAF helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * test: extend coraza.no_memoize coverage in noop_test.go (#1555) * Initial plan * test: extend noop_test.go coverage for coraza.no_memoize build tag 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: fzipi <3012076+fzipi@users.noreply.github.com> * fix: check error return of m.Do in benchmark to resolve errcheck lint failure (#1556) * Initial plan * fix: check error return of m.Do in benchmark test to fix errcheck lint 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: fzipi <3012076+fzipi@users.noreply.github.com> * fix: skip memoize scale tests in short mode The scale tests (TestMemoizeScaleMultipleOwners, TestCacheGrowthWithoutClose, TestCacheBoundedWithClose) compile hundreds of regexes across many owners/cycles. Under TinyGo's slower regex engine these take hours when run in CI with -short. Gate all three scale tests behind testing.Short() in both sync_test.go and nosync_test.go so TinyGo CI (which passes -short) completes in reasonable time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(memoize): avoid deadlock in TinyGo's sync.Map during Release and Reset TinyGo's sync.Map.Range() holds its internal lock for the entire iteration. Calling cache.Delete() inside the Range callback tries to re-acquire the same non-reentrant lock, causing a deadlock. Defer all cache.Delete() calls until after Range returns by collecting keys first. This also fixes t.Skip() in tests which does not halt execution in TinyGo due to unimplemented runtime.Goexit(). On standard Go this is a net performance win for Release (up to 60% faster at 100 owners) with negligible temporary memory (~9KB slice). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * feat: implement SecUploadKeepFiles directive (#1557) * feat: implement SecUploadKeepFiles with RelevantOnly support Add UploadKeepFilesStatus type supporting On, Off, and RelevantOnly values for the SecUploadKeepFiles directive. When set to On, uploaded files are preserved after transaction close. When set to RelevantOnly, files are kept only if rules matched during the transaction. Closes #1550 * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @M4tteoP Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> * docs: update SecUploadKeepFiles in coraza.conf-recommended Remove the "not supported" note and document the RelevantOnly option. * fix: filter nolog rules in RelevantOnly upload keep files check RelevantOnly now only considers rules with Log enabled, matching the same filtering used for audit log part K. This prevents CRS initialization rules (nolog) from making RelevantOnly behave like On. * fix: require SecUploadDir when SecUploadKeepFiles is enabled Add validation in WAF.Validate() to ensure SecUploadDir is configured when SecUploadKeepFiles is set to On or RelevantOnly, matching the ModSecurity requirement. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: directive docs Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> * fix: correct two compile errors in SecUploadKeepFiles implementation (#1560) * Initial plan * fix: correct lint errors - HasAccessToFS is a bool not a function, fix wrong constant name 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: fzipi <3012076+fzipi@users.noreply.github.com> * fix: gofmt Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> * fix: skip SecUploadKeepFiles tests when no_fs_access build tag is set The upload keep files tests expected success for On/RelevantOnly modes, but the implementation correctly rejects these when filesystem access is disabled. Guard these test cases behind environment.HasAccessToFS. --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> * feat: add regex support to ctl:ruleRemoveTargetById, ruleRemoveTargetByTag, and ruleRemoveTargetByMsg collection keys (#1561) * Initial plan * Add regex support to ctl:ruleRemoveTargetById for URI-scoped exclusions Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * Use memoization for regex compilation in parseCtl Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * Add benchmarks for short and medium regex exceptions in GetField Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * refactor: add HasRegex shared utility and use it in rule.go and ctl.go Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * test: add POST JSON body test for ruleRemoveTargetById regex key exclusion Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * docs: update RemoveRuleTargetByID comment to document keyRx parameter Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * docs: update ctl action doc comment to describe regex key syntax with example Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * test: add ruleRemoveTargetByTag and ruleRemoveTargetByMsg regex key integration tests Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * style: apply gofmt to internal/actions/ctl.go Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> * test: add memoizer coverage to TestParseCtl for ctl regex path 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: fzipi <3012076+fzipi@users.noreply.github.com> * Initial plan * test: add e2e tests for JSONSTREAM body processor Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com> Agent-Logs-Url: https://github.com/corazawaf/coraza/sessions/bebca76e-344f-4966-8675-8bf4e5fda0cb --------- Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Matteo Pace <pace.matteo96@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Alexander S. <126732+heaven@users.noreply.github.com> Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com> Co-authored-by: Pierre POMES <pierre.pomes@gmail.com> Co-authored-by: Felipe Zipitria <felipe.zipitria@owasp.org> Co-authored-by: jptosso <1236942+jptosso@users.noreply.github.com> Co-authored-by: Juan Pablo Tosso <jptosso@gmail.com> Co-authored-by: Hiroaki Nakamura <hnakamur@gmail.com> Co-authored-by: Marc W. <113890636+MarcWort@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Romain SERVIERES <romain@madeformed.com>
…on-processor # Conflicts: # .github/workflows/regression.yml # coraza.conf-recommended # examples/http-server/go.mod # examples/http-server/go.sum # experimental/plugins/plugintypes/operator.go # go.mod # internal/actions/actions.go # internal/auditlog/formats.go # internal/auditlog/formats_test.go # internal/corazawaf/transaction.go # internal/corazawaf/transaction_test.go # internal/corazawaf/waf.go # internal/operators/rx.go # internal/operators/rx_test.go # internal/seclang/rule_parser.go # magefile.go # testing/auditlog_test.go # testing/coreruleset/coreruleset_test.go # testing/coreruleset/go.mod # testing/coreruleset/go.sum # waf_test.go
📝 WalkthroughWalkthroughIntroduced streaming JSON body processor support enabling per-record evaluation of NDJSON/JSON Lines/RFC 7464 request and response bodies. Added Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP Middleware
participant WAF Transaction
participant Body Processor
participant Rules Engine
Client->>HTTP Middleware: POST with NDJSON body
HTTP Middleware->>WAF Transaction: ProcessRequestBodyFromStream(reader, writer)
activate WAF Transaction
WAF Transaction->>WAF Transaction: Detect StreamingBodyProcessor
loop For each record in stream
WAF Transaction->>Body Processor: ProcessRequestRecords callback
activate Body Processor
Body Processor->>Body Processor: Parse JSON record
Body Processor->>Body Processor: Flatten to fields
deactivate Body Processor
WAF Transaction->>WAF Transaction: Clear argsPost
WAF Transaction->>WAF Transaction: Populate argsPost from fields
WAF Transaction->>Rules Engine: Evaluate Phase 2 (RequestBody)
activate Rules Engine
Rules Engine-->>WAF Transaction: Decision (allow/deny)
deactivate Rules Engine
alt Rule interrupted
WAF Transaction->>WAF Transaction: Set errStreamInterrupted
WAF Transaction-->>Client: Early termination with 403
else Passed
WAF Transaction->>Client: Write rawRecord to output
end
end
deactivate WAF Transaction
Client->>Client: Receive response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coraza.conf-recommended`:
- Around line 120-122: The response MIME guidance only lists application/json
and application/x-ndjson but should also include the other documented JSON
stream types; update the note in coraza.conf-recommended to mention
application/jsonlines and application/json-seq (in addition to application/json
and application/x-ndjson) so response-inspection guidance matches the request
examples and covers all supported JSON stream MIME types.
In `@experimental/bodyprocessors/jsonstream.go`:
- Around line 175-190: The scanRecords function currently trims and reformats
the raw input before passing it to the consumer, causing canonicalization
(changing delimiters, CRLF vs LF, final newlines, and outer whitespace); instead
preserve and relay the original raw bytes/text verbatim. Modify scanRecords (and
the similar block at the later occurrence) to capture the unmodified
scanner.Text() (or the raw token) as rawRecord and pass that rawRecord unchanged
into fn rather than formatRecord(record); continue to use formatRecord only
where a formatted version is explicitly required (e.g., for parsing via
parseJSONRecord), ensuring ProcessRequestBodyFromStream and
ProcessResponseBodyFromStream receive the original rawRecord exactly as read.
In `@experimental/plugins/plugintypes/bodyprocessor.go`:
- Around line 49-56: Streaming JSON processors currently ignore the provided
BodyProcessorOptions and always use DefaultStreamRecursionLimit; update the JSON
stream implementation (experimental/bodyprocessors/jsonstream.go) used by
ProcessRequestRecords and ProcessResponseRecords to read the recursion limit
from the incoming options (e.g. options.RecursionLimit or the actual field name
on BodyProcessorOptions) and pass that value into the streaming JSON
parser/decoder instead of the hardcoded DefaultStreamRecursionLimit, falling
back to DefaultStreamRecursionLimit only when the options value is zero/unset so
configured recursion limits are honored in streaming mode.
In `@internal/corazawaf/transaction.go`:
- Around line 1235-1313: The streaming-relay path (around
bodyprocessors.GetBodyProcessor and sp.ProcessRequestRecords) lacks the same
guards as the buffered path and can bypass RequestBodyAccess /
ResponseBodyAccess, IsResponseBodyProcessable(), and
RequestBodyLimit/ResponseBodyLimit checks; mirror the checks used by
ReadRequestBodyFrom and ProcessRequestBody before calling
sp.ProcessRequestRecords: verify request/response body access flags
(RequestBodyAccess/ResponseBodyAccess), call IsResponseBodyProcessable() where
appropriate, enforce RequestBodyLimit/ResponseBodyLimit and
generateRequestBodyError + tx.WAF.Rules.Eval(...) on violations, and follow the
same fallback behavior as the non-streaming branch (use tx.ReadRequestBodyFrom /
tx.ProcessRequestBody semantics) so streaming processors cannot bypass limits or
disabled access.
- Around line 1138-1141: The streaming branch in transaction.go returns from
processRequestBodyStreaming/processResponseBodyStreaming when bodyprocessor
implements plugintypes.StreamingBodyProcessor, which skips calling the
processor's ProcessRequest/ProcessResponse that populate metadata (e.g.,
TX:jsonstream_*_body and TX:jsonstream_*_line_count) in
experimental/bodyprocessors/jsonstream.go; update the streaming path to either
call jsonStreamBodyProcessor's metadata-populating routines or explicitly invoke
the processor's ProcessRequest/ProcessResponse (or a shared helper used by
Process*Body) before/after
processRequestBodyStreaming/processResponseBodyStreaming so the same TX metadata
is emitted regardless of using StreamingBodyProcessor, keeping references to
bodyprocessor, StreamingBodyProcessor, ProcessRequest, ProcessResponse,
processRequestBodyStreaming and processResponseBodyStreaming.
- Around line 1333-1348: The code currently logs non-interruption streaming
errors (streamErr != nil && streamErr != errStreamInterrupted), sets
REQBODY/RESBODY via tx.generateRequestBodyError and tx.WAF.Rules.Eval, but then
falls through and returns (tx.interruption, nil) which hides relay I/O failures;
change both such blocks (the one around tx.generateRequestBodyError and the
similar block at the later mirror) to return the streaming error to the caller
after handling: i.e., after logging, calling tx.generateRequestBodyError and
possibly tx.WAF.Rules.Eval, return (tx.interruption, streamErr) instead of
allowing the function to return nil error so reader-side I/O failures are
propagated.
In `@magefile.go`:
- Around line 279-287: The tags slice used to build the tags matrix (the
variable named tags passed into combinations(tags)) is missing
"coraza.rule.rx_prefilter", causing the generated TagsMatrix to be stale; add
"coraza.rule.rx_prefilter" to that slice and then ensure TagsMatrix is the
single source of truth by either deriving the slice from the canonical build tag
list or updating the external workflow to read from the same central constant so
they cannot drift apart.
- Line 289: The code calls json.Marshal (see json.Marshal(combos)) but the
encoding/json package is not imported; add the import "encoding/json" to the
import block (where other stdlib imports live) so json.Marshal resolves, then
run gofmt/goimports to ensure imports are organized; update any existing import
grouping to include encoding/json alongside other standard library imports.
In `@testing/e2e/ndjson_e2e_test.go`:
- Around line 169-170: The test currently allows either http.StatusOK or
http.StatusForbidden for non-NDJSON content which is too permissive; update the
assertion in the test (replace the condition that checks resp.StatusCode against
http.StatusOK && http.StatusForbidden) to assert the single expected status code
(e.g., resp.StatusCode == http.StatusOK) and change the t.Errorf call to report
the single expected value (use resp.StatusCode and http.StatusOK in the
message); locate and modify the check referencing resp.StatusCode,
http.StatusOK, http.StatusForbidden and t.Errorf in the ndjson_e2e_test.go test
function.
- Around line 41-43: The test HTTP handler currently discards the error from
io.ReadAll(r.Body); change the handler to check the error returned by
io.ReadAll, and if non-nil respond with an appropriate failure status (e.g.,
w.WriteHeader(http.StatusInternalServerError)) and a descriptive message (and/or
t.Fatalf/log) instead of proceeding to write "OK". Locate the body-reading logic
that calls io.ReadAll(r.Body) and update error handling around that call and the
subsequent w.WriteHeader/fmt.Fprintf usage so test failures surface server-side
read errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 424d4e00-37aa-42e9-93d8-3b8ad2ebcabb
📒 Files selected for processing (12)
coraza.conf-recommendedexperimental/bodyprocessors/jsonstream.goexperimental/bodyprocessors/jsonstream_test.goexperimental/plugins/bodyprocessors.goexperimental/plugins/plugintypes/bodyprocessor.goexperimental/streaming.gohttp/middleware.gointernal/corazawaf/transaction.gomagefile.gostreaming_integration_test.gotesting/auditlog_test.gotesting/e2e/ndjson_e2e_test.go
| # Note: Add 'application/json' and 'application/x-ndjson' if you want to | ||
| # inspect JSON and NDJSON response bodies | ||
| # |
There was a problem hiding this comment.
Response MIME guidance is incomplete for the documented JSON stream types.
The note mentions only application/json and application/x-ndjson, but nearby request examples also document application/jsonlines and application/json-seq. This can lead to partial response-inspection configuration.
📝 Suggested wording update
-# Note: Add 'application/json' and 'application/x-ndjson' if you want to
-# inspect JSON and NDJSON response bodies
+# Note: Add 'application/json', 'application/x-ndjson', 'application/jsonlines',
+# and 'application/json-seq' if you want to inspect JSON and JSON-stream response bodies📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Note: Add 'application/json' and 'application/x-ndjson' if you want to | |
| # inspect JSON and NDJSON response bodies | |
| # | |
| # Note: Add 'application/json', 'application/x-ndjson', 'application/jsonlines', | |
| # and 'application/json-seq' if you want to inspect JSON and JSON-stream response bodies | |
| # |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coraza.conf-recommended` around lines 120 - 122, The response MIME guidance
only lists application/json and application/x-ndjson but should also include the
other documented JSON stream types; update the note in coraza.conf-recommended
to mention application/jsonlines and application/json-seq (in addition to
application/json and application/x-ndjson) so response-inspection guidance
matches the request examples and covers all supported JSON stream MIME types.
| func scanRecords(scanner *bufio.Scanner, maxRecursion int, formatRecord func(string) string, | ||
| fn func(recordNum int, fields map[string]string, rawRecord string) error) (int, error) { | ||
| recordNum := 0 | ||
|
|
||
| for scanner.Scan() { | ||
| record := strings.TrimSpace(scanner.Text()) | ||
| if record == "" { | ||
| continue | ||
| } | ||
|
|
||
| fields, err := parseJSONRecord(record, recordNum, maxRecursion) | ||
| if err != nil { | ||
| return recordNum, err | ||
| } | ||
|
|
||
| if err := fn(recordNum, fields, formatRecord(record)); err != nil { |
There was a problem hiding this comment.
rawRecord is being canonicalized before relay.
scanner.Text() drops the original delimiter, TrimSpace strips outer whitespace, and formatNDJSON/formatJSONSequence synthesize \n/RS again. ProcessRequestBodyFromStream and ProcessResponseBodyFromStream write rawRecord verbatim, so CRLF vs LF, missing final newlines, and any preserved outer whitespace are silently changed on the wire.
Also applies to: 208-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@experimental/bodyprocessors/jsonstream.go` around lines 175 - 190, The
scanRecords function currently trims and reformats the raw input before passing
it to the consumer, causing canonicalization (changing delimiters, CRLF vs LF,
final newlines, and outer whitespace); instead preserve and relay the original
raw bytes/text verbatim. Modify scanRecords (and the similar block at the later
occurrence) to capture the unmodified scanner.Text() (or the raw token) as
rawRecord and pass that rawRecord unchanged into fn rather than
formatRecord(record); continue to use formatRecord only where a formatted
version is explicitly required (e.g., for parsing via parseJSONRecord), ensuring
ProcessRequestBodyFromStream and ProcessResponseBodyFromStream receive the
original rawRecord exactly as read.
| // ProcessRequestRecords reads records one at a time from the reader and calls fn | ||
| // for each record's parsed fields. Processing stops if fn returns a non-nil error. | ||
| ProcessRequestRecords(reader io.Reader, options BodyProcessorOptions, | ||
| fn func(recordNum int, fields map[string]string, rawRecord string) error) error | ||
|
|
||
| // ProcessResponseRecords is the response equivalent of ProcessRequestRecords. | ||
| ProcessResponseRecords(reader io.Reader, options BodyProcessorOptions, | ||
| fn func(recordNum int, fields map[string]string, rawRecord string) error) error |
There was a problem hiding this comment.
BodyProcessorOptions is exposed but recursion limit is effectively bypassed in streaming implementations.
ProcessRequestRecords/ProcessResponseRecords receive options, but current JSON stream implementations still hardcode DefaultStreamRecursionLimit, so configured recursion limits are not enforced in streaming mode.
💡 Proposed fix (in experimental/bodyprocessors/jsonstream.go)
-func (js *jsonStreamBodyProcessor) ProcessRequestRecords(reader io.Reader, _ plugintypes.BodyProcessorOptions,
+func (js *jsonStreamBodyProcessor) ProcessRequestRecords(reader io.Reader, options plugintypes.BodyProcessorOptions,
fn func(recordNum int, fields map[string]string, rawRecord string) error) error {
- recordCount, err := processJSONStreamWithCallback(reader, DefaultStreamRecursionLimit, fn)
+ recursionLimit := options.RequestBodyRecursionLimit
+ if recursionLimit <= 0 {
+ recursionLimit = DefaultStreamRecursionLimit
+ }
+ recordCount, err := processJSONStreamWithCallback(reader, recursionLimit, fn)
if err != nil {
return err
}-func (js *jsonStreamBodyProcessor) ProcessResponseRecords(reader io.Reader, _ plugintypes.BodyProcessorOptions,
+func (js *jsonStreamBodyProcessor) ProcessResponseRecords(reader io.Reader, options plugintypes.BodyProcessorOptions,
fn func(recordNum int, fields map[string]string, rawRecord string) error) error {
- recordCount, err := processJSONStreamWithCallback(reader, DefaultStreamRecursionLimit, fn)
+ recursionLimit := options.RequestBodyRecursionLimit
+ if recursionLimit <= 0 {
+ recursionLimit = DefaultStreamRecursionLimit
+ }
+ recordCount, err := processJSONStreamWithCallback(reader, recursionLimit, fn)
if err != nil {
return err
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@experimental/plugins/plugintypes/bodyprocessor.go` around lines 49 - 56,
Streaming JSON processors currently ignore the provided BodyProcessorOptions and
always use DefaultStreamRecursionLimit; update the JSON stream implementation
(experimental/bodyprocessors/jsonstream.go) used by ProcessRequestRecords and
ProcessResponseRecords to read the recursion limit from the incoming options
(e.g. options.RecursionLimit or the actual field name on BodyProcessorOptions)
and pass that value into the streaming JSON parser/decoder instead of the
hardcoded DefaultStreamRecursionLimit, falling back to
DefaultStreamRecursionLimit only when the options value is zero/unset so
configured recursion limits are honored in streaming mode.
| // If the body processor supports streaming, evaluate rules per record | ||
| if sp, ok := bodyprocessor.(plugintypes.StreamingBodyProcessor); ok { | ||
| return tx.processRequestBodyStreaming(sp, reader, bpOpts) | ||
| } |
There was a problem hiding this comment.
Auto-streaming skips the processor’s metadata population.
Once these branches take Process*Records, jsonStreamBodyProcessor.ProcessRequest / ProcessResponse never run, so TX:jsonstream_*_body and TX:jsonstream_*_line_count from experimental/bodyprocessors/jsonstream.go are absent on the normal buffered Process*Body() path. That makes rule behavior depend on which transaction API was used.
Also applies to: 1665-1668
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/corazawaf/transaction.go` around lines 1138 - 1141, The streaming
branch in transaction.go returns from
processRequestBodyStreaming/processResponseBodyStreaming when bodyprocessor
implements plugintypes.StreamingBodyProcessor, which skips calling the
processor's ProcessRequest/ProcessResponse that populate metadata (e.g.,
TX:jsonstream_*_body and TX:jsonstream_*_line_count) in
experimental/bodyprocessors/jsonstream.go; update the streaming path to either
call jsonStreamBodyProcessor's metadata-populating routines or explicitly invoke
the processor's ProcessRequest/ProcessResponse (or a shared helper used by
Process*Body) before/after
processRequestBodyStreaming/processResponseBodyStreaming so the same TX metadata
is emitted regardless of using StreamingBodyProcessor, keeping references to
bodyprocessor, StreamingBodyProcessor, ProcessRequest, ProcessResponse,
processRequestBodyStreaming and processResponseBodyStreaming.
| if tx.RuleEngine == types.RuleEngineOff { | ||
| // Pass through: copy input to output without evaluation | ||
| if _, err := io.Copy(output, input); err != nil { | ||
| return nil, err | ||
| } | ||
| return nil, nil | ||
| } | ||
|
|
||
| if tx.interruption != nil { | ||
| return tx.interruption, nil | ||
| } | ||
|
|
||
| if tx.lastPhase != types.PhaseRequestHeaders { | ||
| tx.debugLogger.Debug().Msg("Skipping ProcessRequestBodyFromStream: wrong phase") | ||
| return nil, nil | ||
| } | ||
|
|
||
| rbp := tx.variables.reqbodyProcessor.Get() | ||
| if tx.ForceRequestBodyVariable && rbp == "" { | ||
| rbp = "URLENCODED" | ||
| tx.variables.reqbodyProcessor.Set(rbp) | ||
| } | ||
| rbp = strings.ToLower(rbp) | ||
| if rbp == "" { | ||
| // No body processor, pass through | ||
| if _, err := io.Copy(output, input); err != nil { | ||
| return nil, err | ||
| } | ||
| tx.WAF.Rules.Eval(types.PhaseRequestBody, tx) | ||
| return tx.interruption, nil | ||
| } | ||
|
|
||
| bodyprocessor, err := bodyprocessors.GetBodyProcessor(rbp) | ||
| if err != nil { | ||
| tx.generateRequestBodyError(errors.New("invalid body processor")) | ||
| tx.WAF.Rules.Eval(types.PhaseRequestBody, tx) | ||
| return tx.interruption, nil | ||
| } | ||
|
|
||
| sp, ok := bodyprocessor.(plugintypes.StreamingBodyProcessor) | ||
| if !ok { | ||
| // Non-streaming body processor: buffer input, process normally, then copy to output | ||
| it, _, err := tx.ReadRequestBodyFrom(input) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if it != nil { | ||
| return it, nil | ||
| } | ||
|
|
||
| it, err = tx.ProcessRequestBody() | ||
| if err != nil || it != nil { | ||
| return it, err | ||
| } | ||
|
|
||
| // Copy buffered body to output | ||
| rbr, err := tx.RequestBodyReader() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if _, err := io.Copy(output, rbr); err != nil { | ||
| return nil, err | ||
| } | ||
| return nil, nil | ||
| } | ||
|
|
||
| mime := "" | ||
| if m := tx.variables.requestHeaders.Get("content-type"); len(m) > 0 { | ||
| mime = m[0] | ||
| } | ||
|
|
||
| tx.debugLogger.Debug(). | ||
| Str("body_processor", rbp). | ||
| Msg("Attempting to process streaming request body with relay") | ||
|
|
||
| streamErr := sp.ProcessRequestRecords(input, plugintypes.BodyProcessorOptions{ | ||
| Mime: mime, | ||
| StoragePath: tx.WAF.UploadDir, | ||
| }, func(recordNum int, fields map[string]string, rawRecord string) error { |
There was a problem hiding this comment.
Mirror the buffered-path guards before streaming relay.
These entry points never apply the same RequestBodyAccess / ResponseBodyAccess, IsResponseBodyProcessable(), or body-limit handling that ProcessRequestBody() / ProcessResponseBody() and Read*BodyFrom() enforce. That lets streaming processors bypass RequestBodyLimit / ResponseBodyLimit, and when body access is off the non-streaming fallback relays an empty buffer instead of the original stream.
Also applies to: 1355-1412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/corazawaf/transaction.go` around lines 1235 - 1313, The
streaming-relay path (around bodyprocessors.GetBodyProcessor and
sp.ProcessRequestRecords) lacks the same guards as the buffered path and can
bypass RequestBodyAccess / ResponseBodyAccess, IsResponseBodyProcessable(), and
RequestBodyLimit/ResponseBodyLimit checks; mirror the checks used by
ReadRequestBodyFrom and ProcessRequestBody before calling
sp.ProcessRequestRecords: verify request/response body access flags
(RequestBodyAccess/ResponseBodyAccess), call IsResponseBodyProcessable() where
appropriate, enforce RequestBodyLimit/ResponseBodyLimit and
generateRequestBodyError + tx.WAF.Rules.Eval(...) on violations, and follow the
same fallback behavior as the non-streaming branch (use tx.ReadRequestBodyFrom /
tx.ProcessRequestBody semantics) so streaming processors cannot bypass limits or
disabled access.
| if _, err := io.WriteString(output, rawRecord); err != nil { | ||
| return fmt.Errorf("failed to write record to output: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| if streamErr != nil && streamErr != errStreamInterrupted { | ||
| tx.debugLogger.Error().Err(streamErr).Msg("Failed to process streaming request body relay") | ||
| tx.generateRequestBodyError(streamErr) | ||
| if tx.interruption == nil { | ||
| tx.WAF.Rules.Eval(types.PhaseRequestBody, tx) | ||
| } | ||
| } | ||
|
|
||
| return tx.interruption, nil |
There was a problem hiding this comment.
Return relay I/O failures to the caller.
The callback already wraps output write failures, but these blocks turn every non-interruption streamErr into REQBODY/RESBODY variables and still return nil. A truncated relay then looks successful to the connector, and the same suppression also hides reader-side I/O errors bubbled up by Process*Records.
Also applies to: 1428-1444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/corazawaf/transaction.go` around lines 1333 - 1348, The code
currently logs non-interruption streaming errors (streamErr != nil && streamErr
!= errStreamInterrupted), sets REQBODY/RESBODY via tx.generateRequestBodyError
and tx.WAF.Rules.Eval, but then falls through and returns (tx.interruption, nil)
which hides relay I/O failures; change both such blocks (the one around
tx.generateRequestBodyError and the similar block at the later mirror) to return
the streaming error to the caller after handling: i.e., after logging, calling
tx.generateRequestBodyError and possibly tx.WAF.Rules.Eval, return
(tx.interruption, streamErr) instead of allowing the function to return nil
error so reader-side I/O failures are propagated.
| tags := []string{ | ||
| "coraza.rule.mandatory_rule_id_check", | ||
| "coraza.rule.case_sensitive_args_keys", | ||
| "coraza.rule.no_regex_multiline", | ||
| "coraza.no_memoize", | ||
| "coraza.rule.multiphase_evaluation", | ||
| "no_fs_access", | ||
| } | ||
| combos := combinations(tags) |
There was a problem hiding this comment.
This matrix is already stale relative to CI.
The hardcoded slice omits coraza.rule.rx_prefilter, which is still present in .github/workflows/regression.yml:25-48. If TagsMatrix is meant to be the source of truth for build tags, its output is incomplete today and will keep drifting while the workflow remains hardcoded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magefile.go` around lines 279 - 287, The tags slice used to build the tags
matrix (the variable named tags passed into combinations(tags)) is missing
"coraza.rule.rx_prefilter", causing the generated TagsMatrix to be stale; add
"coraza.rule.rx_prefilter" to that slice and then ensure TagsMatrix is the
single source of truth by either deriving the slice from the canonical build tag
list or updating the external workflow to read from the same central constant so
they cannot drift apart.
| } | ||
| combos := combinations(tags) | ||
|
|
||
| jsonData, err := json.Marshal(combos) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that `json.Marshal` is referenced while `encoding/json` is not imported.
# Expected: one match for `json.Marshal` and no match for `encoding/json`.
rg -n 'json\.Marshal|encoding/json' magefile.go
sed -n '8,20p' magefile.goRepository: corazawaf/coraza
Length of output: 254
Add missing encoding/json import.
json.Marshal is used at line 289 but encoding/json is not imported.
Proposed fix
import (
+ "encoding/json"
"errors"
"fmt"
"io"
"os"
"os/exec"
"path/filepath"
"regexp"
"strings"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jsonData, err := json.Marshal(combos) | |
| import ( | |
| "encoding/json" | |
| "errors" | |
| "fmt" | |
| "io" | |
| "os" | |
| "os/exec" | |
| "path/filepath" | |
| "regexp" | |
| "strings" |
🧰 Tools
🪛 GitHub Actions: Lint (pre-commit)
[error] 289-289: Go lint failed with compile error: undefined: json.
🪛 GitHub Actions: Regression Tests
[error] 289-289: Build failed for step "go run mage.go coverage": /magefile.go:289:19: undefined: json
🪛 GitHub Check: lint
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.no_memoize)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.case_sensitive_args_keys)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.mandatory_rule_id_check,coraza.rule.case_sensitive_args_...
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.mandatory_rule_id_check)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.multiphase_evaluation,coraza.rule.case_sensitive_args_keys)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.multiphase_evaluation,coraza.rule.mandatory_rule_id_chec...
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.multiphase_evaluation,coraza.rule.mandatory_rule_id_check)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.multiphase_evaluation,coraza.rule.no_regex_multiline)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.multiphase_evaluation)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.no_regex_multiline)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, coraza.rule.rx_prefilter)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, no_fs_access,coraza.no_memoize)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest, no_fs_access)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.25.x, ubuntu-latest)
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.26.x, ubuntu-latest, coraza.rule.multiphase_evaluation,coraza.rule.mandatory_rule_id_chec...
[failure] 289-289:
undefined: json
🪛 GitHub Check: test (1.26.x, ubuntu-latest)
[failure] 289-289:
undefined: json
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@magefile.go` at line 289, The code calls json.Marshal (see
json.Marshal(combos)) but the encoding/json package is not imported; add the
import "encoding/json" to the import block (where other stdlib imports live) so
json.Marshal resolves, then run gofmt/goimports to ensure imports are organized;
update any existing import grouping to include encoding/json alongside other
standard library imports.
| body, _ := io.ReadAll(r.Body) | ||
| w.WriteHeader(http.StatusOK) | ||
| fmt.Fprintf(w, "OK: %s", body) |
There was a problem hiding this comment.
Handle request-body read errors in the test backend.
On Line 41, the io.ReadAll error is discarded, which can mask server-side failures and produce misleading test outcomes.
🛠️ Proposed fix
backend := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- body, _ := io.ReadAll(r.Body)
+ body, err := io.ReadAll(r.Body)
+ if err != nil {
+ http.Error(w, "failed to read request body", http.StatusInternalServerError)
+ return
+ }
w.WriteHeader(http.StatusOK)
- fmt.Fprintf(w, "OK: %s", body)
+ if _, err := fmt.Fprintf(w, "OK: %s", body); err != nil {
+ return
+ }
})As per coding guidelines: Always check and handle errors explicitly; Don't ignore errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testing/e2e/ndjson_e2e_test.go` around lines 41 - 43, The test HTTP handler
currently discards the error from io.ReadAll(r.Body); change the handler to
check the error returned by io.ReadAll, and if non-nil respond with an
appropriate failure status (e.g., w.WriteHeader(http.StatusInternalServerError))
and a descriptive message (and/or t.Fatalf/log) instead of proceeding to write
"OK". Locate the body-reading logic that calls io.ReadAll(r.Body) and update
error handling around that call and the subsequent w.WriteHeader/fmt.Fprintf
usage so test failures surface server-side read errors.
| if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusForbidden { | ||
| t.Errorf("unexpected status %d for non-NDJSON content type", resp.StatusCode) |
There was a problem hiding this comment.
This assertion is too permissive to catch regressions.
Allowing both 200 and 403 makes the test non-diagnostic. It should assert one expected outcome for application/json under this rule set.
✅ Suggested stricter assertion
- if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusForbidden {
- t.Errorf("unexpected status %d for non-NDJSON content type", resp.StatusCode)
- }
+ if resp.StatusCode != http.StatusOK {
+ t.Errorf("expected 200 for non-NDJSON content type, got %d", resp.StatusCode)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusForbidden { | |
| t.Errorf("unexpected status %d for non-NDJSON content type", resp.StatusCode) | |
| if resp.StatusCode != http.StatusOK { | |
| t.Errorf("expected 200 for non-NDJSON content type, got %d", resp.StatusCode) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testing/e2e/ndjson_e2e_test.go` around lines 169 - 170, The test currently
allows either http.StatusOK or http.StatusForbidden for non-NDJSON content which
is too permissive; update the assertion in the test (replace the condition that
checks resp.StatusCode against http.StatusOK && http.StatusForbidden) to assert
the single expected status code (e.g., resp.StatusCode == http.StatusOK) and
change the t.Errorf call to report the single expected value (use
resp.StatusCode and http.StatusOK in the message); locate and modify the check
referencing resp.StatusCode, http.StatusOK, http.StatusForbidden and t.Errorf in
the ndjson_e2e_test.go test function.
what
Implements a new body processor for handling streaming JSON formats with per-record rule evaluation:
Streaming body processor
json.0.field,json.1.field)JSONSTREAM,NDJSON, andJSONLINESaliasesPer-record rule evaluation
Instead of parsing all records into
ArgsPostand evaluating Phase 2 rules once, rules are now evaluated after each complete JSON record:ArgsPostis cleared between records so each record is evaluated in isolationstill trigger a block when the accumulated score exceeds it
Eval()is safe to call multiple times per phase —AllowTypePhase,Skip, and the transformation cache all reset correctly between callsNew
StreamingBodyProcessorinterfaceExtends
BodyProcessorwithProcessRequestRecords/ProcessResponseRecordsmethods that yield parsed records one at a time via callback. The transaction detects thisinterface via type assertion and switches to the per-record evaluation path automatically.
Streaming relay support
Added
ProcessRequestBodyFromStream(input, output)/ProcessResponseBodyFromStream(input, output)methods on the transaction for integrators building custom streamingmiddleware. These read records from input, evaluate rules per record, and write clean records to output. Exposed via an experimental
StreamingTransactioninterface.Usage example
Testing
auto-detection)
Benchmark Results (Apple M2)
ProcessRequest (buffered) vs Callback (streaming)
RFC 7464 (JSON Sequence) via Callback
Key Takeaways
TransactionVariablescollections (ArgsPost, TX vars, raw body viaTeeReader), which accounts for the reduced allocations.Record Templates
{"id":1,"name":"Alice"}(24 bytes){"user_id":1234567890,"name":"User Name","email":"user@example.com","role":"admin","active":true,"tags":["tag1","tag2","tag3"]}(128 bytes){"user":{"name":"Alice","address":{"city":"NYC","zip":"10001"}},"scores":[95,87,92],"meta":{"created":"2026-01-01","active":true}}(131 bytes)Summary by CodeRabbit
Release Notes
New Features
Documentation