[vortexopenclaw]

Summary

Return 400 Bad Request, rather than 500 Internal Server Error, when the built-in JSON parser fails during request-body completion. Preserve the existing early rejection; this is not a redesign of request-body error policy.

Following maintainer review, this PR is rebased onto v2/master at 42b02c6b23a6c5ca3c8f067efd28097ae3a41935 and squashed into one commit. The abandoned rule-controlled implementation is no longer in the PR history.

Scope

• Return -2 for built-in JSON completion errors in modsecurity_request_body_end().
• Map -2 to HTTP_BAD_REQUEST in read_request_body(); retain -5 → 413 and other negative results → 500.
• Add 13 plain declarative JSON regression cases alongside 15-json.t. These parser-specific assertions require YAJL; the per-case no-YAJL callback wrapper has been removed.
• Remove the request-reader helper refactor and 10-request-buckets.t from this PR. They are now in v2: Separate request-body bucket handling from the reader #3716 and are not prerequisites for this fix.

Remaining behavior and #2807

Completion failures still return before phase 2. Recommended-config rule 200002, other REQBODY_ERROR rules, and CRS phase-2 rules do not see these requests; custom phase-2 status: values remain ignored. In-stream JSON syntax errors still follow the existing rule-controlled path.

DetectionOnly still rejects completion failures, which is known-wrong existing behavior, not a desired contract. The DetectionOnly regression case has been removed to avoid locking that behavior in. #2807 remains open for rule-controlled handling and DetectionOnly behavior. XML/multipart, unavailable JSON support, custom processors, storage failures and body-limit policies are unchanged.

Relationship to #3515: maintainer decision required

This JSON change adapts the JSON portion of Marc Stern's proposal in #3515; it is overlapping work, not an independent fix to merge alongside it. #3515 additionally changes XML/multipart handling.

My recommendation is to take this narrowly scoped JSON correction and its tests, and consider XML/multipart separately. If maintainers prefer #3515 instead, these tests can be carried over and this PR closed. Please choose one implementation path; do not merge both overlapping JSON changes. This recommendation does not resolve the maintainer decision by itself.

Verification of the revised commit

Ubuntu 24.04 arm64 container, Apache 2.4, YAJL enabled; container networking disabled and no published ports. Fresh source export of the revised commit, including the pinned libinjection submodule:

• Build succeeds with --enable-pcre-study=yes --enable-pcre-jit CFLAGS=-Werror=format-security.
• make test: 4,250/4,250 unit tests pass.
• Full native Apache regression suite: 325/325 pass, including all 13 new JSON cases.
• New cases cover completion rejection with default/custom, absent, excluded and non-disruptive policies; chunked completion; valid/empty bodies, argument inspection, explicit opt-outs and in-stream error-rule controls.

Reproduce:

git submodule update --init --recursive
./autogen.sh
./configure --enable-pcre-study=yes --enable-pcre-jit CFLAGS=-Werror=format-security
make -j4
make test
make test-regression

The actual HTTP runner in the root-owned isolated test container was USER=root LOGNAME=root perl run-regression-tests.pl -S . -p 8090 -v from tests/. Set USER/LOGNAME to the actual test-process user.

These are current local results, not claims of current-head GitHub Actions success. Cross-platform CI and maintainer approval remain merge gates. Prior SonarCloud and CodeRabbit results applied to the superseded refactored head and do not establish this revision's status. No production deployment is included.

Optional-build investigation is separate from this standard-build status fix; any security-sensitive findings belong in private vulnerability reporting, not this public thread.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0e8b745a-d303-4620-bcdb-2e922b4f4712

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 43919e51-a9a6-4acc-b514-5e55a7830901

Commits

Reviewing files that changed from the base of the PR and between 59b1dfe and 9804b96.

Files selected for processing: 2

• apache2/apache2_io.c
• tests/regression/rule/15-json-eof.t

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

Walkthrough

JSON completion failures on non-empty request bodies now return a distinct code. Apache maps that code to HTTP 400. Regression tests cover incomplete JSON, valid JSON, parser errors, and request-processing settings.

Changes: Request body processing

Layer / File(s)	Summary
JSON completion status mapping apache2/msc_reqbody.c, apache2/apache2_io.c	JSON completion failures on non-empty bodies now return -2. Apache maps -2 to HTTP 400. The function documentation describes the return codes.
JSON EOF regression coverage tests/regression/rule/15-json-eof.t	Tests cover incomplete JSON, empty and valid bodies, in-stream syntax errors, rule handling, and disabled request-body processing.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related issues

• owasp-modsecurity/ModSecurity issue 3655: The change introduces a distinct internal result code for JSON completion failures, which relates to the issue’s report about unnamed negative failure codes.
• owasp-modsecurity/ModSecurity issue 3581: The change addresses JSON completion error handling, but the supplied summary says it retains early rejection and DetectionOnly behavior.

Suggested reviewers: fzipi, airween

Merge Risk: Minimal · previous review 9804b

The change is ready for normal merge checks: JSON completion failures receive HTTP 400, with no established regression in other request-body handling.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: mapping JSON completion errors to HTTP 400 while preserving the existing rejection behavior.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

🔵 Needs a closer look

Security-sensitive request-body handling and outstanding upstream CI/quality gates require maintainer review.

Pull request overview

Updates ModSecurity v2 to return HTTP 400 for JSON completion failures while preserving unconditional rejection and JSON inspection.

Changes:

• Adds a distinct JSON completion-failure result.
• Maps completion failures to HTTP 400.
• Adds comprehensive regression coverage.

File summaries

File	Description
tests/regression/rule/15-json-eof.t	Adds JSON completion and rejection regression tests.
apache2/msc_reqbody.c	Identifies JSON completion failures separately.
apache2/apache2_io.c	Maps JSON completion failures to HTTP 400.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[vortexopenclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[fzipi]

Thanks @vortexopenclaw for the very thorough write-up and test matrix!

What I checked:

• I built this head and v2/master in an Ubuntu 24.04 container (Apache 2.4.58, YAJL) and ran the new .t files against both. On v2/master a truncated JSON body ({) gets a 500 before phase 2, including under DetectionOnly. With this PR it's a 400.
  • 15-json-eof.t: 14/14 pass on the PR; on the base, exactly the 7 completion-status cases fail.
  • 10-request-buckets.t: 24/24 pass on both.
• The full regression suite, on the PR head and on the PR merged into current v2/master, shows the same 13 environmental failures as the base and no new ones (the XML body tests included).
• The -2 return from modsecurity_request_body_end() can't collide with anything: the urlencoded/raw paths only return -1, and read_request_body() is the only caller.

So the actual fix looks right to me. Some things to sort out before we can merge:

1. (blocking) Please split the read_request_body() refactor (check_request_body_limit / store_request_body_chunk / read_request_body_bucket) and 10-request-buckets.t into a separate PR. I walked through the branches and they look equivalent, and the bucket tests pass on the unrefactored base too. But it's ~200 lines of churn in the request-body reader on the maintenance branch for what is a ~10-line status-code fix, and we keep functional changes and refactors apart. This PR should be the -2 return, the 400 mapping, and the JSON tests.
2. (blocking) This overlaps #3515, which makes the same JSON change plus XML/multipart. We need to pick one direction and not merge both. Let's settle that here; your tests are useful either way and could be carried over.
3. (blocking) In 15-json-eof.t, the "premature EOF retains rejection in DetectionOnly" case asserts a 400 under SecRuleEngine DetectionOnly. That's existing behaviour (it was a 500), but DetectionOnly is expected not to block, and I'd rather not lock that in with a regression test. Please drop that case, or make it clear in the comment that it documents current, known-wrong behaviour that #2807 still tracks.
4. (non-blocking) To be clear on scope: completion errors still return before phase 2, so REQBODY_ERROR rules (e.g. 200002 in the recommended config) and CRS never see these requests, and a custom status: on the error rule is still ignored. That's fine for this PR, but it means #2807 stays open for the rule-controlled part.
5. (non-blocking) The per-case test => sub wrapper with no_yajl_status is a lot of machinery for a no-YAJL build CI doesn't produce. Plain declarative cases alongside 15-json.t would be simpler to maintain.
6. (non-blocking) Please squash. The first commit implements the approach you later dropped.
7. (non-blocking) About the optional-build failure you mention in the description: if you think it has security impact, please send the details through the repository's private vulnerability reporting ("Report a vulnerability" under the Security tab) rather than this thread, and we'll take it from there.

Also, the Actions runs for this head never started (0 jobs). A new push will create fresh runs; if they fail to start again, we'll look at the workflow side.

[vortexopenclaw]

Thanks @fzipi. Updated in 9804b96468e8dabe528eaf15739acceb86dc47f4:

1. Split the reader helpers and 24 bucket tests into v2: Separate request-body bucket handling from the reader #3716, independently based on current v2/master. This PR now changes only the JSON completion return, its 400 mapping and JSON tests.
2. On Return 400 in case of parsing error #3515, my preference is the narrow JSON correction here and separate consideration of XML/multipart. If you prefer the broader proposal, please carry these JSON tests over and close this PR. I agree these overlapping JSON implementations should not both merge; this remains your decision.
3. Removed the DetectionOnly case. The file comment and description explicitly call the retained rejection known-wrong behavior tracked by Bug: Premature EOF in JSON leads to status 500 instead of REQBODY_ERROR #2807, not a contract to preserve indefinitely.
4. Explicitly retained Bug: Premature EOF in JSON leads to status 500 instead of REQBODY_ERROR #2807 as open: completion errors bypass phase 2, including recommended-config/CRS error rules and custom statuses.
5. Removed the no-YAJL callback wrapper and alternate statuses. The 13 cases are plain declarative tests with an explicit YAJL prerequisite.
6. Rebased onto 42b02c6b23a6c5ca3c8f067efd28097ae3a41935 and squashed into one commit, removing the abandoned approach from the PR history.
7. Security-sensitive optional-build details will not be posted here; that investigation remains separate from this standard-build fix.

Fresh isolated-container verification: 4,250 unit tests and 325 HTTP regressions pass for this revision. On the unchanged base with the new tests, 319/325 pass: exactly the six completion-status cases return 500 instead of 400, while all seven new controls pass. The separate refactor passes 4,250 unit tests and 336 HTTP regressions, including all 24 bucket tests.

The new push created fresh Linux QA and IIS runs, but both currently report action_required and need approval. SonarCloud's analysis gate now reports c:S3776 on the unrefactored read_request_body() (complexity 86, allowed 25). I have deliberately neither folded the refactor back in nor suppressed the finding. Please advise how you want that gate handled for this small maintenance fix while keeping the refactor separate. CodeRabbit skipped the automatic review of this revision; its prior review should not be read as a review of this head.

[vortexopenclaw]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[sonarqubecloud]

Quality Gate failed

Failed conditions
E Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[airween]

Thanks @vortexopenclaw for the quick and thorough update, and thanks @fzipi for the detailed review!

First, I'd like to apologize: I should have spoken up much earlier. There are two older open PRs by @hnakamur that deal with request body handling when a limit is exceeded, and I'm only now pointing them out:

• v2: Allow partial processing of XML, JSON, and multipart request body  #3483 (v2): allows partial processing of XML, JSON, multipart and urlencoded bodies with SecRequestBodyLimitAction ProcessPartial, and changes how limit violations are handled outside On + Reject, including DetectionOnly.
• Allow partial processing of XML, JSON, and multipart request body #3476 (v3): the same change for libmodsecurity.

These overlap with this PR:

• Same code: v2: Allow partial processing of XML, JSON, and multipart request body  #3483 rewrites the error handling after the read loop in read_request_body(). With it, a -5 from modsecurity_request_body_end() still returns 413, but any other completion error returns 500 only when the engine is On and the limit action is Reject. In every other case processing continues. This PR adds the -2 → 400 mapping in exactly the same place, so the two conflict in apache2/apache2_io.c.
• Opposite direction: the 400 here is returned unconditionally, also under DetectionOnly, which is what point 3 of the review was about. v2: Allow partial processing of XML, JSON, and multipart request body  #3483 goes the other way: outside On + Reject it doesn't block. That matches v3, where these errors only set REQBODY_ERROR and the rules decide.

Since @hnakamur's PRs have been waiting longer and keep v2 and v3 consistent, I'd like to start with those: review #3483 and #3476 first, and merge them if they are OK. After that, I think this PR should be rebased on top of #3483, and the -2 → 400 mapping should follow the same rule (only under On + Reject, otherwise continue), with the JSON tests adjusted accordingly. The question of #3515 vs. this PR (point 2 of the review) is still open, and I'd like to settle it at the same time, once the direction of #3483 is clear.

Regarding SonarCloud's c:S3776 on read_request_body(): that complexity is pre-existing and isn't introduced by this PR, so it won't block this fix. We'll handle the gate on our side.

Sorry again for the delay and for the extra round. Your tests are very useful either way, and I'd like to keep them.