Fill ARGS_POST variable even if body bytes were invalid JSON when JSON body processor is running#1615
Fill ARGS_POST variable even if body bytes were invalid JSON when JSON body processor is running#1615HusseinKabbout wants to merge 11 commits into
Conversation
…N body processor is running
…or can work together
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughJSON body processing now populates flattened request and response arguments before returning read errors, and ChangesJSON body processing
JSON engine test profile
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
testing/engine/json.go (1)
78-162: Good coverage of the partial-body path.The new profile correctly asserts both positive (test/test2/test3.0 parsed → 1100/1101/1104/1200/1201 fire) and negative (test3.1/test3.2 never reach
ARGS/ARGS_POST→ 1102/1103/1202/1203 don't fire) cases for truncation at 40 bytes, and1010validates the array-length semantic with only one element flattened. One small suggestion for robustness: consider adding a companion stage that keeps rule1111(REQBODY_ERROR !@eq 0) enabled to explicitly assert that the error flag is still raised alongside the populated args — that's the core contract of this PR and currently only the absence of blocking is checked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/engine/json.go` around lines 78 - 162, Add a companion stage to the "truncatedjsonyaml" profile that uses the same StageInput as the existing "json" stage but expects rule id 1111 to be triggered (i.e., include 1111 in TriggeredRules) to assert the REQBODY_ERROR flag is set; also enable the SecRule REQBODY_ERROR "!@eq 0" "id:1111, phase:2, log, block" in the Rules block (it is currently commented out) so the test can observe that error rule. Locate the profile by name "truncatedjsonyaml", the existing Stage under Tests -> Title "json" to copy the input, and the SecRule with id 1111 to uncomment/activate. Ensure the new stage's Output still includes the previously expected positive rule ids (e.g., 1100,1101,1104,1200,1201) plus 1111 as a triggered rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/bodyprocessors/json.go`:
- Around line 83-88: The current logic calls readItems(...) and then overwrites
its error when gjson.Valid(s) is false, which hides specific errors from
readItems (e.g., max recursion); change the return logic so you preserve or wrap
the readItems error instead of discarding it: after err := readItems(json, key,
maxRecursion, res) check gjson.Valid(s) and if it's invalid, return the
validation error but if err != nil wrap the readItems error (e.g.,
fmt.Errorf("invalid JSON: %w", err)) or return err directly so the more specific
readItems error from readItems(...) is not lost; refer to the variables json :=
gjson.Parse(s), err, readItems and gjson.Valid(s) when implementing the change.
In `@testing/engine/json.go`:
- Around line 140-141: Fix the typo in the inline comment by changing "commented
our" to "commented out" in the commented lines above the SecRule (the comment
containing "This is commented our because we want to check if body processor can
work with partial JSON"); update that comment text so it reads "This is
commented out because we want to check if body processor can work with partial
JSON" while leaving the SecRule line unchanged.
---
Nitpick comments:
In `@testing/engine/json.go`:
- Around line 78-162: Add a companion stage to the "truncatedjsonyaml" profile
that uses the same StageInput as the existing "json" stage but expects rule id
1111 to be triggered (i.e., include 1111 in TriggeredRules) to assert the
REQBODY_ERROR flag is set; also enable the SecRule REQBODY_ERROR "!@eq 0"
"id:1111, phase:2, log, block" in the Rules block (it is currently commented
out) so the test can observe that error rule. Locate the profile by name
"truncatedjsonyaml", the existing Stage under Tests -> Title "json" to copy the
input, and the SecRule with id 1111 to uncomment/activate. Ensure the new
stage's Output still includes the previously expected positive rule ids (e.g.,
1100,1101,1104,1200,1201) plus 1111 as a triggered rule.
🪄 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: d0c74c98-966d-43e9-b830-3fe903f66641
📒 Files selected for processing (2)
internal/bodyprocessors/json.gotesting/engine/json.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1615 +/- ##
=======================================
Coverage 87.66% 87.66%
=======================================
Files 178 178
Lines 9134 9136 +2
=======================================
+ Hits 8007 8009 +2
Misses 858 858
Partials 269 269
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @HusseinKabbout, is there any related issue for this one? Would like to understand which problem are you solving. Also, I don't think making this the default would be my choice, as people might rely on the previous semantics. |
The issue that I am trying to solve is described in the "What does this PR do?" section. There I also link to a existing GitHub issue. That should be the full context.
The thing is that this was already the default before 3.4.0. The JSON syntax check was not in 3.3.3. |
|
Any update on this PR? Is the provided context enough to understand what this PR is trying to solve? |
|
I think you have. I'm afraid of changing the default dehaviour now because users might expect to only have valid json, and now it will be partial. That's why adding it behind a configuration flag make sense. |
|
Ok, maybe this way it works: let's piggyback on And make it clear on documentation that this is how it works for JSON. |
|
Another option is to create a new directive |
This would make the configuration more explicit but is not needed, AFAIK. The behavior should be configurable with just my PR, it makes the population of Also, since JSON is not the only body processor, maybe we should also check if this is also desirable for the other body processors. |
|
Any update on this PR? |
There was a problem hiding this comment.
Added some comments, but LGTM.
Just to recap: in the context of ProcessPartial, we used to ignore any validity checks and simply perform a best-effort parsing of the truncated body. This was a potential bypass issue.
Starting with #1110, we began enforcing validation and completely skipped any further best-effort inspection when validation failed. As a result, all JSON payloads larger than the configured limit would be blocked becuase REQBODY_ERROR is set, and disabling that rule would lead to 0 checks on the payload.
This PR brings back the best-effort inspection without removing validation. REQBODY_ERROR will still be set to 1 (we should add a test for this), but users will be able to write a rule that allows the request when running in ProcessPartial mode, relying on the best-effort parsing behavior.
It would be great to document this exclusion rule, perhaps even directly in the recommended configuration file alongside the REQBODY_ERROR comments.
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
|
@M4tteoP Thanks for the review! I addressed your comments |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testing/engine/json.go`:
- Around line 140-141: The stale inline note above SecRule REQBODY_ERROR is
misleading because the rule is now active, not commented out. Update the comment
in testing/engine/json.go to accurately describe the current intent of this test
block, keeping the explanation aligned with the active SecRule and the partial
JSON/body processor behavior it is meant to validate.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60cfffbe-9861-4172-8c14-d94a145e03f5
📒 Files selected for processing (2)
internal/bodyprocessors/json.gotesting/engine/json.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/bodyprocessors/json.go
M4tteoP
left a comment
M4tteoP
left a comment
There was a problem hiding this comment.
Thanks! LGTM. I'll leave it approved for a bit, in case anyone else wants to chime in, and then I'll merge it
3 participants
Dear maintainers, the link above is broken
Make sure that you've checked the boxes below before you submit PR:
Thanks for your contribution ❤️
What does this PR do?
This PR addresses #1611.
The JSON body parser underwent a behavior change (introduced around 2024, but only recently released) that broke the
ProcessPartialflow. WhenSecRequestBodyLimitActionis set toProcessPartialand the body limit is reached, Coraza now always returns a body processing error for JSON bodies without populatingARGS_POST. This makes usingProcessPartildifficult if you want to validate the body but only on a "best effort" basis.The expected behavior (at least what I would expect) for
ProcessPartialis to parse + validate as much of the body as possible, even if the body is truncated. While it is true that truncated bodies will always return an error for the JSON processor, Coraza should still populateARGS_POSTso that rules can be applied to whatever was successfully parsed. Then users can still decide if they want to block the request onREQBODY_ERROR != 0or if they want to simply let it pass and validate whatever is inARGS_POST.This PR tries to enable that behavior. The parser will keep throwing the "invalid JSON" error but it will now also fill
ARGS_POSTwith whatever values it managed to parse before the truncation.The added test tries to simulate what I described above:
ARGSandARGS_POSTcontain what woudl be expectedSummary by CodeRabbit
Bug Fixes
Tests