Skip to content

Return 400 in case of parsing error #3515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JonathanBerrew wants to merge 3 commits into
base: v2/master
Choose a base branch
from
+7 −4
Open

Return 400 in case of parsing error #3515

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
402f942
Return 400 in case of parsing error
JonathanBerrew Mar 18, 2026
46ad3cf
return -2 when leads to status 400, -1 when leads to status 500
JonathanBerrew Apr 23, 2026
a97a84b
Merge branch 'v2/master' into 400-if-parsing-error
JonathanBerrew Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions apache2/apache2_io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@
/**
* Reads request body from a client.
*/
apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {

Check failure on line 181 in apache2/apache2_io.c

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 86 to the 25 allowed. 


See more on https://sonarcloud.io/project/issues?id=owasp-modsecurity_ModSecurity&issues=AZ0BuCuGmjzjByUrmFkJ&open=AZ0BuCuGmjzjByUrmFkJ&pullRequest=3515
assert(msr != NULL);
assert(error_msg!= NULL);
request_rec *r = msr->r;
Expand Down Expand Up @@ -354,6 +354,9 @@
if (rcbe == -5) {
return HTTP_REQUEST_ENTITY_TOO_LARGE;
}
if (rcbe == -2) {
return HTTP_BAD_REQUEST;
}
if (rcbe < 0) {
Comment on lines +357 to 360
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modsecurity_request_body_end() (in apache2/msc_reqbody.c) never returns -2—it returns 1 on success, -5 for reqbody no-files limit, and -1 for parser/internal errors. As a result this new rcbe == -2 branch is effectively dead code and does not achieve the PR goal of returning 400 on parsing errors (those currently surface as -1 and will still map to 500 via rcbe < 0). Consider either (a) returning a distinct status code from modsecurity_request_body_end() for parse errors and mapping that to HTTP_BAD_REQUEST here, or (b) mapping the existing parse-error signal (-1 plus the parser error state/message) to 400 while keeping true internal failures as 500.

Suggested change
if (rcbe == -2) {
return HTTP_BAD_REQUEST;
}
if (rcbe < 0) {
if (rcbe < 0) {
if ((error_msg != NULL) && (*error_msg != NULL) && (**error_msg != '\0')) {
return HTTP_BAD_REQUEST;
}

Copilot uses AI. Check for mistakes.
return HTTP_INTERNAL_SERVER_ERROR;
}
Expand Down
Loading