v2: Return HTTP 400 for JSON completion errors without relaxing rejection

[vortexopenclaw]

Summary

Problem: ModSecurity v2 rejects an incomplete JSON request with 500 Internal Server Error before phase 2 runs. The request is rejected, but the response incorrectly reports a server failure.

Fix: Return 400 Bad Request for JSON completion failures while preserving the existing unconditional rejection. JSON inspection stays enabled. Missing, excluded, log-only, or DetectionOnly error policies do not cause previously rejected requests to reach the application.

This is a deliberately conservative status-code correction, not a change to who controls JSON error policy.

Review status: ready for maintainer review, not merge-cleared. SonarCloud now passes with no open issues. The standard-build local suites below pass. The current-head Linux/IIS workflows need upstream approval; CodeRabbit completed review with no actionable comments. Supplemental optional-build limitations are stated separately below.

Relationship to #3515

PR #3515, carrying Marc Stern's proposed change, already implements the same return-code approach. This PR adapts its JSON portion; it is overlapping work, not a separate discovery or a second patch that needs to be merged alongside it.

• Return 400 in case of parsing error #3515: changes JSON, XML, multipart completion errors and multipart argument-retrieval errors to 400.
• This PR: changes JSON completion errors only, adds 14 JSON and 24 body-limit native Apache regressions, and includes a behavior-preserving request-reader refactor to satisfy the complexity gate. XML/multipart and other failure policies remain unchanged, although the refactor touches their shared reader.

For the JSON case, this revision intentionally agrees with #3515. Its additional value is the narrower parser-status scope and regression evidence. The supporting shared-reader refactor broadens the implementation diff and is independently compared against the pre-refactor revision. The tests can also be reused with #3515 if maintainers prefer that broader proposal.

Design decision and tradeoffs

An earlier revision removed the early return so REQBODY_ERROR rules could choose the outcome. Comparative testing showed that it also forwarded incomplete JSON when no effective blocking rule applied. I replaced that design rather than treating the compatibility change as incidental to a status-code fix.

Configuration	Current v2	This revision / #3515 (JSON)
Error rule denies with 400	500, rejected	400, rejected
Error rule denies with 422	500, rejected	400, rejected
Missing or path-excluded error rule	500, rejected	400, rejected
Log/pass-only rule	500, rejected	400, rejected
DetectionOnly	500, rejected	400, rejected
Engine or request-body access explicitly disabled	Existing opt-out behavior	Unchanged

Limitations retained: Completion failures still do not run phase 2, do not honor a custom phase-2 response, and still reject in DetectionOnly. In-stream JSON syntax errors continue using the existing rule-controlled path. This does not resolve the broader request for configurable error handling discussed in #2807; that would be a separate intentional compatibility decision.

Implementation

• Return -2 from the existing built-in JSON completion-failure branch in modsecurity_request_body_end().
• Map that result to HTTP_BAD_REQUEST in read_request_body(), preserving the -5 → 413 and other negative results → 500 mappings.
• Retain existing error variables, diagnostics, parser invocation and early rejection.
• Preserve XML/multipart, unavailable JSON support, custom request-body processors, streaming errors, storage/I/O failures and body-limit policies.
• Extract bucket reading, whole-body limit checks and chunk-storage error handling into static helpers. Preserve ordering, end-of-stream flags, accounting and the distinct whole-body/no-files limit behavior. This resolves Sonar complexity without suppressing the rule.

“Completion failure” includes a parser/callback failure detected while finishing the JSON document, not solely premature EOF. No new dependencies, options or parsing work are introduced.

Verification

Base: v2/master at 0875b1928003fa77ad5d2dcd61c531149ded7781 (2.9.14). Comparison patch: #3515 at a97a84b56cd1b2289be07bcb9ea11a0e358a8e01. Environment: Ubuntu 24.04 arm64, Apache 2.4.58, YAJL 2.1.0, GCC and PCRE2. All HTTP tests ran on container loopback with external networking disabled and no published ports.

• Unit suite: 4,210/4,210 passed.
• Full native Apache regression suite: 286/286 passed, including 14 JSON and 24 bucket/limit cases added here.
• Before/after: unchanged upstream fails exactly seven new completion-status cases (500 instead of 400); the seven controls pass. The revised patch passes all 14.
• Optional-parser portability: built with --without-yajl; all 14 JSON cases pass their explicit no-YAJL expectations; all 24 new bucket/limit cases also pass. Enabled-parser builds still require the strict parser-error/status/phase-2 assertions.
• Existing proposal: all 14 focused cases pass against Return 400 in case of parsing error #3515 applied to the same base.
• 675 comparative HTTP requests: three builds × nine policies × 25 payloads. This revision changes 77 responses, all from 500 to 400; zero backend-reachability changes versus the baseline. Every forwarded body retains its exact length and SHA-256. Backend receipts are checked independently of the response header.
• 27 additional completion-callback checks: a terminal JSON scalar hits the argument limit during parser completion. Across the same three builds and nine policies, the seven changed responses are 500 → 400, with no backend-reachability changes (702 comparison requests total).
• Refactor-only differential testing: 912 requests, comparing this revision with the pre-refactor patch: 768 covering buffering/streaming, body/no-files limits, both limit actions, On/DetectionOnly and malformed transport, plus 144 targeted multipart-file/form-field and no-files boundary cases. Zero differences in response, backend reachability or received-body length/hash. All 24 new native limit cases also pass against the pre-refactor build.
• Supplemental optional build: both pre-refactor and revised code compile with --enable-large-stream-input. A 384-request comparison found zero pre/post differences, but reproduced an existing failure in both versions. This is not counted as an all-passing optional-build suite; that limitation needs separate maintainer/security triage. The standard-build results above do not use this optional mode.
• Coverage includes incomplete objects, arrays, strings, literals, numbers and escapes; whitespace-only, valid and empty bodies; content-length/chunked requests and larger bodies; custom statuses, missing/excluded rules, log-only, DetectionOnly, ProcessPartial and explicit opt-outs; argument inspection, depth and size limits; XML/multipart and non-JSON controls.

Reproduce the native suites after checking out this revision and initializing the pinned submodule:

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

The direct HTTP runner used in the root-owned isolated container was USER=root LOGNAME=root perl run-regression-tests.pl -S . -p 8090 -v from tests/. Use the actual test-process user for USER/LOGNAME. Repeat the focused regression/rule/15-json-eof.t file in a separate --without-yajl build to exercise optional-parser behavior.

Automated review and remaining gates

• SonarCloud: both checks pass on 59b1dfe924438215198756429b4e87d21bd208c3; the issue API reports zero open issues. The reader was refactored rather than adding a suppression or changing the quality gate.
• CodeRabbit: completed a manually requested review from the base through 59b1dfe, covering all four changed files, with no actionable comments. Its successful status is now a completed review, not a skip. This is not a substitute for maintainer approval.
• GitHub Actions: the previous revision passed Linux Quality Assurance and Windows/IIS workflows. The current refactor has new runs requiring maintainer approval; those current-head runs have not passed yet. Please approve and run both before merging.

Independent design/security review found no new implementation blocker in the refactor; standard-build differential and regression evidence is above. This does not certify every supported build or erase the separately observed optional-build failure. Maintainer review and successful current-head cross-platform CI remain merge prerequisites. No production deployment is included.

Related: #2807, #3515. The partial-body redesign in #3483 is outside this change.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: f1da2b57-16af-4210-a464-e0523beeb1d1

📥 Commits

Reviewing files that changed from the base of the PR and between 0875b19 and 59b1dfe.

📒 Files selected for processing (4)

• apache2/apache2_io.c
• apache2/msc_reqbody.c
• tests/regression/config/10-request-buckets.t
• tests/regression/rule/15-json-eof.t

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

---

📝 Walkthrough

Walkthrough

Request-body processing now separates limit and bucket handling. JSON completion errors return a distinct code that maps to HTTP 400. Regression tests cover body limits, chunked requests, premature JSON EOF, parser errors, and YAJL-disabled builds.

Changes

Request body processing

Layer / File(s)	Summary
Request-body limit handling apache2/apache2_io.c, tests/regression/config/10-request-buckets.t	Request-body limit checks and bucket processing use separate helpers. The tests cover 24 policy, limit, and size combinations.
JSON completion status mapping apache2/msc_reqbody.c, apache2/apache2_io.c	modsecurity_request_body_end returns -2 for JSON completion errors. Request finalization maps -2 to HTTP 400 and -5 to HTTP 413.
JSON EOF regression coverage tests/regression/rule/15-json-eof.t	Regression cases cover premature EOF, syntax errors, valid JSON, engine settings, rule actions, chunked requests, and builds without YAJL.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 59b1d

The change preserves request-body limit behavior while returning 400 for JSON completion failures, with no concrete unresolved merge risk identified.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (2 skipped: 2 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: returning HTTP 400 for JSON completion errors while preserving rejection behavior.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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