From f8c38242395148b4c52e56663885bc0733c104b2 Mon Sep 17 00:00:00 2001 From: Vortex Openclaw Date: Thu, 17 Sep 2026 06:02:43 -0700 Subject: [PATCH 1/5] Let JSON completion errors reach phase 2 rules Keep REQBODY_ERROR and its diagnostic when JSON parsing fails at EOF, without converting the completed body read into an unconditional HTTP 500. This aligns completion errors with chunk-time JSON errors and leaves the configured rule in control of the response. Add Apache regression coverage for configurable denial, detection-only and non-disruptive policies, valid and empty bodies, in-stream errors, argument inspection, and chunked input. Related to #2807. --- apache2/msc_reqbody.c | 6 +- tests/regression/rule/15-json-eof.t | 241 ++++++++++++++++++++++++++++ 2 files changed, 245 insertions(+), 2 deletions(-) create mode 100644 tests/regression/rule/15-json-eof.t diff --git a/apache2/msc_reqbody.c b/apache2/msc_reqbody.c index e00a4fc3fb..356c3aed17 100644 --- a/apache2/msc_reqbody.c +++ b/apache2/msc_reqbody.c @@ -728,8 +728,10 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) { msr->msc_reqbody_error = 1; msr->msc_reqbody_error_msg = *error_msg; msr_log(msr, 2, "%s", *error_msg); - return -1; - } + /* Like errors found by json_process_chunk(), expose completion + * errors through REQBODY_ERROR for phase 2 rules to handle. + */ + } #else *error_msg = apr_psprintf(msr->mp, "JSON support was not enabled"); msr->msc_reqbody_error = 1; diff --git a/tests/regression/rule/15-json-eof.t b/tests/regression/rule/15-json-eof.t new file mode 100644 index 0000000000..b1f5e5158c --- /dev/null +++ b/tests/regression/rule/15-json-eof.t @@ -0,0 +1,241 @@ +### Regression tests for issue #2807: JSON completion errors must reach phase 2. + +{ + type => "rule", + comment => "json parser - issue #2807 - premature EOF uses the phase 2 error rule", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + error => [ qr/Access denied with code 400.*\[id "200002"\].*premature EOF/s, 1 ], + }, + match_response => { + status => qr/^400$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{', + ), +}, + +{ + type => "rule", + comment => "json parser - issue #2807 - incomplete object honors a custom error status", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:422,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + error => [ qr/Access denied with code 422.*\[id "200002"\].*premature EOF/s, 1 ], + }, + match_response => { + status => qr/^422$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{ "id" : "123"', + ), +}, + +{ + type => "rule", + comment => "json parser - issue #2807 - premature EOF in DetectionOnly still runs phase 2", + conf => qq( + SecRuleEngine DetectionOnly + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + error => [ qr/Warning.*\[id "200002"\].*premature EOF/s, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{', + ), +}, + +{ + type => "rule", + comment => "json parser - issue #2807 - premature EOF permits a non-disruptive error rule", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,pass,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + error => [ qr/Warning.*\[id "200002"\].*premature EOF/s, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{', + ), +}, + +{ + type => "rule", + comment => "json parser - issue #2807 - valid empty object does not set REQBODY_ERROR", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + -error => [ qr/\[id "200002"\]/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{}', + ), +}, + +{ + type => "rule", + comment => "json parser - issue #2807 - empty body retains existing behavior", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + -error => [ qr/\[id "200002"\]/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '', + ), +}, + +{ + type => "rule", + comment => "json parser - issue #2807 - in-stream syntax errors still use the error rule", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + error => [ qr/Access denied with code 400.*\[id "200002"\]/s, 1 ], + }, + match_response => { + status => qr/^400$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{ id : "123" }', + ), +}, + +{ + type => "rule", + comment => "json parser - issue #2807 - valid JSON arguments are still inspected", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + SecRule ARGS:foo "\@streq bar" "id:200003,phase:2,t:none,log,deny,status:403" + ), + match_log => { + error => [ qr/Access denied with code 403.*\[id "200003"\]/s, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{"foo":"bar"}', + ), +}, + +{ + type => "rule", + comment => "json parser - issue #2807 - chunked premature EOF reaches phase 2", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + error => [ qr/Access denied with code 400.*\[id "200002"\].*premature EOF/s, 1 ], + }, + match_response => { + status => qr/^400$/, + }, + request => normalize_raw_request_data( + qq( + POST /test.txt HTTP/1.1 + Host: $ENV{SERVER_NAME}:$ENV{SERVER_PORT} + User-Agent: $ENV{USER_AGENT} + Content-Type: application/json + Transfer-Encoding: chunked + + ), + ) . encode_chunked('{ "id" : "123"', 3), +}, From e45a8b38b275f323239acca67dfbd6dbb10f1596 Mon Sep 17 00:00:00 2001 From: Vortex Openclaw Date: Thu, 17 Sep 2026 07:39:01 -0700 Subject: [PATCH 2/5] test: cover absent and excluded JSON error policies --- tests/regression/rule/15-json-eof.t | 50 +++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/tests/regression/rule/15-json-eof.t b/tests/regression/rule/15-json-eof.t index b1f5e5158c..ed0b51a4f1 100644 --- a/tests/regression/rule/15-json-eof.t +++ b/tests/regression/rule/15-json-eof.t @@ -239,3 +239,53 @@ ), ) . encode_chunked('{ "id" : "123"', 3), }, + +{ + type => "rule", + comment => "json parser - issue #2807 - no error rule allows the request", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" + ), + match_log => { + error => [ qr/JSON parser error:.*premature EOF.*\[id "200005"\].*JSON phase 2 reached/s, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{', + ), +}, + +{ + type => "rule", + comment => "json parser - issue #2807 - excluded error rule allows the request", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQUEST_URI "^/test[.]txt\$" \\ + "id:200004,phase:1,pass,nolog,ctl:ruleRemoveById=200002" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error'" + SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" + ), + match_log => { + error => [ qr/JSON parser error:.*premature EOF.*\[id "200005"\].*JSON phase 2 reached/s, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{', + ), +}, From 6c53d959ad35d1bf31c358ded2792462eab39e2f Mon Sep 17 00:00:00 2001 From: Vortex Openclaw Date: Thu, 17 Sep 2026 08:17:15 -0700 Subject: [PATCH 3/5] Return HTTP 400 for JSON completion errors without relaxing rejection Replace the earlier rule-controlled proposal with a conservative JSON-only status correction. Preserve unconditional rejection, including when the error policy is absent, excluded, non-disruptive or DetectionOnly. Leave XML, multipart, streaming errors and generic failures unchanged. Adapt the JSON portion of the return-code approach proposed by Marc Stern in upstream PR #3515. Add explicit fail-closed, opt-out and optional-YAJL regression coverage. Related to #2807 and #3515. --- apache2/apache2_io.c | 3 + apache2/msc_reqbody.c | 7 +- tests/regression/rule/15-json-eof.t | 182 +++++++++++++++++++++++++--- 3 files changed, 168 insertions(+), 24 deletions(-) diff --git a/apache2/apache2_io.c b/apache2/apache2_io.c index 8deeb01c9a..33788ab531 100644 --- a/apache2/apache2_io.c +++ b/apache2/apache2_io.c @@ -354,6 +354,9 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) { if (rcbe == -5) { return HTTP_REQUEST_ENTITY_TOO_LARGE; } + if (rcbe == -2) { + return HTTP_BAD_REQUEST; + } if (rcbe < 0) { return HTTP_INTERNAL_SERVER_ERROR; } diff --git a/apache2/msc_reqbody.c b/apache2/msc_reqbody.c index 356c3aed17..c5cfa0aa9b 100644 --- a/apache2/msc_reqbody.c +++ b/apache2/msc_reqbody.c @@ -651,7 +651,8 @@ static apr_status_t modsecurity_request_body_end_urlencoded(modsec_rec *msr, cha } /** - * Stops receiving the request body. + * Stops receiving the request body. Returns -2 for a JSON completion error, + * -5 for the no-files limit, and -1 for other failures. */ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) { assert(msr != NULL); @@ -728,9 +729,7 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) { msr->msc_reqbody_error = 1; msr->msc_reqbody_error_msg = *error_msg; msr_log(msr, 2, "%s", *error_msg); - /* Like errors found by json_process_chunk(), expose completion - * errors through REQBODY_ERROR for phase 2 rules to handle. - */ + return -2; } #else *error_msg = apr_psprintf(msr->mp, "JSON support was not enabled"); diff --git a/tests/regression/rule/15-json-eof.t b/tests/regression/rule/15-json-eof.t index ed0b51a4f1..a2f04327fb 100644 --- a/tests/regression/rule/15-json-eof.t +++ b/tests/regression/rule/15-json-eof.t @@ -1,8 +1,43 @@ -### Regression tests for issue #2807: JSON completion errors must reach phase 2. - +### Regression tests for issue #2807: preserve rejection, but return 400. +### YAJL is optional. Check the existing no-YAJL policy separately, without +### accepting its response codes for a build that actually parsed the request. +map { + my $case = $_; + my $request = delete $case->{request}; + my $response = delete $case->{match_response}; + my $logs = delete $case->{match_log}; + my $no_yajl_status = delete $case->{no_yajl_status}; + $case->{test} = sub { + my $resp = do_request($request); + return 1 unless $resp; + if (defined match_log("error", qr/JSON support was not enabled/, 0)) { + if ($resp->code != $no_yajl_status) { + msg("No-YAJL response: expected $no_yajl_status, got " . $resp->code); + return 1; + } + return 0; + } + unless (defined match_response("status", $resp, $response->{status})) { + msg("Response status failed to match: " . $response->{status}); + vrb($resp); + return 1; + } + for my $key (keys %$logs) { + my ($neg, $name) = ($key =~ /^(-?)(.*)$/); + my $match = match_log($name, @{$logs->{$key}}); + if (($neg && defined $match) || (!$neg && !defined $match)) { + msg("Log expectation failed: $key " . $logs->{$key}[0]); + return 1; + } + } + return 0; + }; + $case; +} ( { type => "rule", - comment => "json parser - issue #2807 - premature EOF uses the phase 2 error rule", + no_yajl_status => 400, + comment => "json parser - issue #2807 - premature EOF is rejected before phase 2", conf => qq( SecRuleEngine On SecRequestBodyAccess On @@ -12,9 +47,11 @@ "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" SecRule REQBODY_ERROR "!\@eq 0" \\ "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" ), match_log => { - error => [ qr/Access denied with code 400.*\[id "200002"\].*premature EOF/s, 1 ], + error => [ qr/JSON parser error:.*premature EOF/s, 1 ], + -error => [ qr/\[id "20000[25]"\]/, 1 ], }, match_response => { status => qr/^400$/, @@ -28,7 +65,8 @@ { type => "rule", - comment => "json parser - issue #2807 - incomplete object honors a custom error status", + no_yajl_status => 422, + comment => "json parser - issue #2807 - incomplete object retains rejection before a custom error rule", conf => qq( SecRuleEngine On SecRequestBodyAccess On @@ -38,12 +76,14 @@ "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" SecRule REQBODY_ERROR "!\@eq 0" \\ "id:200002,phase:2,t:none,log,deny,status:422,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" ), match_log => { - error => [ qr/Access denied with code 422.*\[id "200002"\].*premature EOF/s, 1 ], + error => [ qr/JSON parser error:.*premature EOF/s, 1 ], + -error => [ qr/\[id "20000[25]"\]/, 1 ], }, match_response => { - status => qr/^422$/, + status => qr/^400$/, }, request => new HTTP::Request( POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", @@ -54,7 +94,8 @@ { type => "rule", - comment => "json parser - issue #2807 - premature EOF in DetectionOnly still runs phase 2", + no_yajl_status => 200, + comment => "json parser - issue #2807 - premature EOF retains rejection in DetectionOnly", conf => qq( SecRuleEngine DetectionOnly SecRequestBodyAccess On @@ -64,12 +105,14 @@ "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" SecRule REQBODY_ERROR "!\@eq 0" \\ "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" ), match_log => { - error => [ qr/Warning.*\[id "200002"\].*premature EOF/s, 1 ], + error => [ qr/JSON parser error:.*premature EOF/s, 1 ], + -error => [ qr/\[id "20000[25]"\]/, 1 ], }, match_response => { - status => qr/^200$/, + status => qr/^400$/, }, request => new HTTP::Request( POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", @@ -80,7 +123,8 @@ { type => "rule", - comment => "json parser - issue #2807 - premature EOF permits a non-disruptive error rule", + no_yajl_status => 200, + comment => "json parser - issue #2807 - premature EOF retains rejection with a non-disruptive error rule", conf => qq( SecRuleEngine On SecRequestBodyAccess On @@ -90,12 +134,14 @@ "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" SecRule REQBODY_ERROR "!\@eq 0" \\ "id:200002,phase:2,t:none,log,pass,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" ), match_log => { - error => [ qr/Warning.*\[id "200002"\].*premature EOF/s, 1 ], + error => [ qr/JSON parser error:.*premature EOF/s, 1 ], + -error => [ qr/\[id "20000[25]"\]/, 1 ], }, match_response => { - status => qr/^200$/, + status => qr/^400$/, }, request => new HTTP::Request( POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", @@ -106,6 +152,7 @@ { type => "rule", + no_yajl_status => 400, comment => "json parser - issue #2807 - valid empty object does not set REQBODY_ERROR", conf => qq( SecRuleEngine On @@ -132,6 +179,7 @@ { type => "rule", + no_yajl_status => 200, comment => "json parser - issue #2807 - empty body retains existing behavior", conf => qq( SecRuleEngine On @@ -158,6 +206,7 @@ { type => "rule", + no_yajl_status => 400, comment => "json parser - issue #2807 - in-stream syntax errors still use the error rule", conf => qq( SecRuleEngine On @@ -184,6 +233,7 @@ { type => "rule", + no_yajl_status => 400, comment => "json parser - issue #2807 - valid JSON arguments are still inspected", conf => qq( SecRuleEngine On @@ -211,7 +261,8 @@ { type => "rule", - comment => "json parser - issue #2807 - chunked premature EOF reaches phase 2", + no_yajl_status => 400, + comment => "json parser - issue #2807 - chunked premature EOF is rejected before phase 2", conf => qq( SecRuleEngine On SecRequestBodyAccess On @@ -221,9 +272,11 @@ "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" SecRule REQBODY_ERROR "!\@eq 0" \\ "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" ), match_log => { - error => [ qr/Access denied with code 400.*\[id "200002"\].*premature EOF/s, 1 ], + error => [ qr/JSON parser error:.*premature EOF/s, 1 ], + -error => [ qr/\[id "20000[25]"\]/, 1 ], }, match_response => { status => qr/^400$/, @@ -242,7 +295,8 @@ { type => "rule", - comment => "json parser - issue #2807 - no error rule allows the request", + no_yajl_status => 200, + comment => "json parser - issue #2807 - no error rule still rejects incomplete JSON", conf => qq( SecRuleEngine On SecRequestBodyAccess On @@ -251,10 +305,11 @@ SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" ), match_log => { - error => [ qr/JSON parser error:.*premature EOF.*\[id "200005"\].*JSON phase 2 reached/s, 1 ], + error => [ qr/JSON parser error:.*premature EOF/s, 1 ], + -error => [ qr/\[id "20000[25]"\]/, 1 ], }, match_response => { - status => qr/^200$/, + status => qr/^400$/, }, request => new HTTP::Request( POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", @@ -265,7 +320,8 @@ { type => "rule", - comment => "json parser - issue #2807 - excluded error rule allows the request", + no_yajl_status => 200, + comment => "json parser - issue #2807 - excluded error rule still rejects incomplete JSON", conf => qq( SecRuleEngine On SecRequestBodyAccess On @@ -278,7 +334,36 @@ SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" ), match_log => { - error => [ qr/JSON parser error:.*premature EOF.*\[id "200005"\].*JSON phase 2 reached/s, 1 ], + error => [ qr/JSON parser error:.*premature EOF/s, 1 ], + -error => [ qr/\[id "20000[25]"\]/, 1 ], + }, + match_response => { + status => qr/^400$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{', + ), +}, + +{ + type => "rule", + no_yajl_status => 200, + comment => "json parser - issue #2807 - disabled engine retains existing behavior", + conf => qq( + SecRuleEngine Off + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" + ), + match_log => { + -error => [ qr/JSON parser error:/, 1 ], }, match_response => { status => qr/^200$/, @@ -289,3 +374,60 @@ '{', ), }, + +{ + type => "rule", + no_yajl_status => 200, + comment => "json parser - issue #2807 - disabled request body access retains existing behavior", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess Off + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:400,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + SecAction "id:200005,phase:2,pass,log,msg:'JSON phase 2 reached'" + ), + match_log => { + -error => [ qr/JSON parser error:/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{', + ), +}, + +{ + type => "rule", + no_yajl_status => 422, + comment => "json parser - issue #2807 - in-stream syntax errors retain custom error status", + conf => qq( + SecRuleEngine On + SecRequestBodyAccess On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_HEADERS:Content-Type "^application/json" \\ + "id:200001,phase:1,t:none,pass,nolog,ctl:requestBodyProcessor=JSON" + SecRule REQBODY_ERROR "!\@eq 0" \\ + "id:200002,phase:2,t:none,log,deny,status:422,msg:'JSON body error',logdata:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + error => [ qr/Access denied with code 422.*\[id "200002"\]/s, 1 ], + }, + match_response => { + status => qr/^422$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ "Content-Type" => "application/json" ], + '{ id : "123" }', + ), +}, + +) From b2f616169364ed63a68f197fd7c95b7cedc95b9b Mon Sep 17 00:00:00 2001 From: Vortex Openclaw Date: Thu, 17 Sep 2026 08:23:11 -0700 Subject: [PATCH 4/5] Group completion status dispatch without adding branch complexity Use a switch for the explicit -5 and -2 completion results, retaining the generic negative-result fallback. This preserves the tested status mapping and avoids increasing the existing request reader cognitive complexity flagged by SonarCloud. --- apache2/apache2_io.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/apache2/apache2_io.c b/apache2/apache2_io.c index 33788ab531..79a2cc0586 100644 --- a/apache2/apache2_io.c +++ b/apache2/apache2_io.c @@ -351,11 +351,13 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) { msr->if_status = IF_STATUS_WANTS_TO_RUN; - if (rcbe == -5) { - return HTTP_REQUEST_ENTITY_TOO_LARGE; - } - if (rcbe == -2) { - return HTTP_BAD_REQUEST; + switch (rcbe) { + case -5: + return HTTP_REQUEST_ENTITY_TOO_LARGE; + case -2: + return HTTP_BAD_REQUEST; + default: + break; } if (rcbe < 0) { return HTTP_INTERNAL_SERVER_ERROR; From 59b1dfe924438215198756429b4e87d21bd208c3 Mon Sep 17 00:00:00 2001 From: Vortex Openclaw Date: Thu, 17 Sep 2026 09:02:46 -0700 Subject: [PATCH 5/5] Simplify request-body bucket handling without changing limit policies Separate bucket reads, nonempty chunk storage and whole-body limit handling. Collapse repeated limit diagnostics while preserving the distinct no-files limit policy, stream compile-time paths, byte accounting, EOS state and completion status mapping. Add 24 native chunked boundary tests across both engines and limit actions, validated against the pre-refactor implementation. This addresses the SonarCloud cognitive-complexity finding without suppressing it. --- apache2/apache2_io.c | 207 +++++++++++-------- tests/regression/config/10-request-buckets.t | 47 +++++ 2 files changed, 163 insertions(+), 91 deletions(-) create mode 100644 tests/regression/config/10-request-buckets.t diff --git a/apache2/apache2_io.c b/apache2/apache2_io.c index 79a2cc0586..e75c1b1d7b 100644 --- a/apache2/apache2_io.c +++ b/apache2/apache2_io.c @@ -175,6 +175,120 @@ apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *bb_out, return APR_SUCCESS; } +/** + * Check the whole-body limit before storing a bucket. DetectionOnly retains + * its existing non-disruptive behavior for both configured limit actions. + */ +static apr_status_t check_request_body_limit(modsec_rec *msr, apr_size_t buflen, + char **error_msg) +{ + if (msr->reqbody_length + buflen <= (apr_size_t)msr->txcfg->reqbody_limit) { + return APR_SUCCESS; + } + + *error_msg = apr_psprintf(msr->mp, "Request body is larger than the " + "configured limit (%ld).", msr->txcfg->reqbody_limit); + + if (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL + && (msr->txcfg->is_enabled == MODSEC_ENABLED + || msr->txcfg->is_enabled == MODSEC_DETECTION_ONLY)) { + return APR_SUCCESS; + } + if (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT + && msr->txcfg->is_enabled == MODSEC_DETECTION_ONLY) { + return APR_SUCCESS; + } + return HTTP_REQUEST_ENTITY_TOO_LARGE; +} + +/** + * Store a nonempty chunk and apply the existing storage-error policy. + */ +static apr_status_t store_request_body_chunk(modsec_rec *msr, const char *buf, + apr_size_t buflen, unsigned int *finished_reading, char **error_msg) +{ + int rc = modsecurity_request_body_store(msr, buf, buflen, error_msg); + + if (msr->reqbody_length > (apr_size_t)msr->txcfg->reqbody_limit + && msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL) { + *finished_reading = 1; + } + if (rc >= 0) { + return APR_SUCCESS; + } + + if (rc == -5) { + *error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the " + "configured limit (%ld).", msr->txcfg->reqbody_no_files_limit); + + /* Unlike the whole-body limit, this path only continues for + * ProcessPartial, including when the engine is DetectionOnly. + */ + if (msr->txcfg->if_limit_action != REQUEST_BODY_LIMIT_ACTION_PARTIAL + || (msr->txcfg->is_enabled != MODSEC_ENABLED + && msr->txcfg->is_enabled != MODSEC_DETECTION_ONLY)) { + return HTTP_REQUEST_ENTITY_TOO_LARGE; + } + } + + if (msr->txcfg->is_enabled == MODSEC_ENABLED + && msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT) { + return HTTP_INTERNAL_SERVER_ERROR; + } + return APR_SUCCESS; +} + +/** + * Read and process one bucket, preserving accounting and end-of-stream state. + */ +static apr_status_t read_request_body_bucket(modsec_rec *msr, apr_bucket *bucket, + unsigned int *finished_reading, char **error_msg) +{ + const char *buf; + apr_size_t buflen; + apr_status_t rc = apr_bucket_read(bucket, &buf, &buflen, APR_BLOCK_READ); + + if (rc != APR_SUCCESS) { + *error_msg = apr_psprintf(msr->mp, "Failed reading input / bucket (%d): %s", rc, get_apr_error(msr->mp, rc)); + return HTTP_INTERNAL_SERVER_ERROR; + } + if (msr->txcfg->debuglog_level >= 9) { + msr_log(msr, 9, "Input filter: Bucket type %s contains %" APR_SIZE_T_FMT " bytes.", + bucket->type->name, buflen); + } + + /* This limit check should only trigger on chunked requests. */ + rc = check_request_body_limit(msr, buflen, error_msg); + if (rc != APR_SUCCESS) { + return rc; + } + + if (msr->txcfg->stream_inbody_inspection == 1) { +#ifndef MSC_LARGE_STREAM_INPUT + msr->stream_input_length += buflen; + modsecurity_request_body_to_stream(msr, buf, buflen, error_msg); +#else + if (modsecurity_request_body_to_stream(msr, buf, buflen, error_msg) < 0) { + return HTTP_INTERNAL_SERVER_ERROR; + } +#endif + } + + msr->reqbody_length += buflen; + if (buflen != 0) { + rc = store_request_body_chunk(msr, buf, buflen, finished_reading, error_msg); + if (rc != APR_SUCCESS) { + return rc; + } + } + + if (APR_BUCKET_IS_EOS(bucket)) { + *finished_reading = 1; + msr->if_seen_eos = 1; + } + return APR_SUCCESS; +} + /** * Reads request body from a client. */ @@ -244,98 +358,9 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) { bucket != APR_BRIGADE_SENTINEL(bb_in); bucket = APR_BUCKET_NEXT(bucket)) { - const char *buf; - apr_size_t buflen; - - rc = apr_bucket_read(bucket, &buf, &buflen, APR_BLOCK_READ); + rc = read_request_body_bucket(msr, bucket, &finished_reading, error_msg); if (rc != APR_SUCCESS) { - *error_msg = apr_psprintf(msr->mp, "Failed reading input / bucket (%d): %s", rc, get_apr_error(msr->mp, rc)); - return HTTP_INTERNAL_SERVER_ERROR; - } - - if (msr->txcfg->debuglog_level >= 9) { - msr_log(msr, 9, "Input filter: Bucket type %s contains %" APR_SIZE_T_FMT " bytes.", - bucket->type->name, buflen); - } - - /* Check request body limit (should only trigger on chunked requests). */ - if (msr->reqbody_length + buflen > (apr_size_t)msr->txcfg->reqbody_limit) { - if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)) { - *error_msg = apr_psprintf(msr->mp, "Request body is larger than the " - "configured limit (%ld).", msr->txcfg->reqbody_limit); - return HTTP_REQUEST_ENTITY_TOO_LARGE; - } else if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL)) { - - *error_msg = apr_psprintf(msr->mp, "Request body is larger than the " - "configured limit (%ld).", msr->txcfg->reqbody_limit); - - } else if ((msr->txcfg->is_enabled == MODSEC_DETECTION_ONLY) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL)){ - - *error_msg = apr_psprintf(msr->mp, "Request body is larger than the " - "configured limit (%ld).", msr->txcfg->reqbody_limit); - - } else if ((msr->txcfg->is_enabled == MODSEC_DETECTION_ONLY) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)){ - - *error_msg = apr_psprintf(msr->mp, "Request body is larger than the " - "configured limit (%ld).", msr->txcfg->reqbody_limit); - - } else { - - *error_msg = apr_psprintf(msr->mp, "Request body is larger than the " - "configured limit (%ld).", msr->txcfg->reqbody_limit); - - return HTTP_REQUEST_ENTITY_TOO_LARGE; - } - } - - if (msr->txcfg->stream_inbody_inspection == 1) { -#ifndef MSC_LARGE_STREAM_INPUT - msr->stream_input_length+=buflen; - modsecurity_request_body_to_stream(msr, buf, buflen, error_msg); -#else - if (modsecurity_request_body_to_stream(msr, buf, buflen, error_msg) < 0) { - return HTTP_INTERNAL_SERVER_ERROR; - } -#endif - } - - msr->reqbody_length += buflen; - - if (buflen != 0) { - int rcbs = modsecurity_request_body_store(msr, buf, buflen, error_msg); - - if (msr->reqbody_length > (apr_size_t)msr->txcfg->reqbody_limit && msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL) { - finished_reading = 1; - } - - if (rcbs < 0) { - if (rcbs == -5) { - if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)) { - *error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the " - "configured limit (%ld).", msr->txcfg->reqbody_no_files_limit); - return HTTP_REQUEST_ENTITY_TOO_LARGE; - } else if ((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL)) { - *error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the " - "configured limit (%ld).", msr->txcfg->reqbody_no_files_limit); - } else if ((msr->txcfg->is_enabled == MODSEC_DETECTION_ONLY) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_PARTIAL)) { - *error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the " - "configured limit (%ld).", msr->txcfg->reqbody_no_files_limit); - } else { - *error_msg = apr_psprintf(msr->mp, "Request body no files data length is larger than the " - "configured limit (%ld).", msr->txcfg->reqbody_no_files_limit); - return HTTP_REQUEST_ENTITY_TOO_LARGE; - } - } - - if((msr->txcfg->is_enabled == MODSEC_ENABLED) && (msr->txcfg->if_limit_action == REQUEST_BODY_LIMIT_ACTION_REJECT)) - return HTTP_INTERNAL_SERVER_ERROR; - } - - } - - if (APR_BUCKET_IS_EOS(bucket)) { - finished_reading = 1; - msr->if_seen_eos = 1; + return rc; } } diff --git a/tests/regression/config/10-request-buckets.t b/tests/regression/config/10-request-buckets.t new file mode 100644 index 0000000000..966e85b46a --- /dev/null +++ b/tests/regression/config/10-request-buckets.t @@ -0,0 +1,47 @@ +### Preserve bucket-level limits and phase-2 visibility while refactoring I/O. +do { + my @tests; + for my $policy (["On", "Reject"], ["On", "ProcessPartial"], + ["DetectionOnly", "Reject"], ["DetectionOnly", "ProcessPartial"]) { + my ($engine, $action) = @$policy; + for my $limit_kind ("body", "no-files") { + for my $size (31, 32, 33) { + my $body_limit = $limit_kind eq "body" ? 32 : 4096; + my $no_files_limit = $limit_kind eq "no-files" ? 32 : 4096; + my $content_type = $limit_kind eq "no-files" + ? "application/x-www-form-urlencoded" : "text/plain"; + my $status = 200; + if ($size > 32 && ($limit_kind eq "no-files" + || ($engine eq "On" && $action eq "Reject"))) { + $status = 413; + } + push @tests, { + type => "config", + comment => "chunked $limit_kind limit: $engine/$action, $size bytes", + conf => qq( + SecRuleEngine $engine + SecRequestBodyAccess On + SecRequestBodyLimit $body_limit + SecRequestBodyNoFilesLimit $no_files_limit + SecRequestBodyLimitAction $action + SecAction "id:200010,phase:2,pass,log,msg:'BUCKET_PHASE2'" + ), + match_response => { status => qr/^$status\z/ }, + match_log => { + ($status == 200 ? "error" : "-error") => + [ qr/\[id "200010"\].*BUCKET_PHASE2/, 1 ], + }, + request => normalize_raw_request_data(qq( + POST /test.txt HTTP/1.1 + Host: $ENV{SERVER_NAME}:$ENV{SERVER_PORT} + User-Agent: $ENV{USER_AGENT} + Content-Type: $content_type + Transfer-Encoding: chunked + + )) . encode_chunked("x" x $size, 7), + }; + } + } + } + @tests; +}