Return 400 in case of parsing error#3515
Return 400 in case of parsing error#3515JonathanBerrew wants to merge 3 commits intoowasp-modsecurity:v2/masterfrom
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR aims to change ModSecurity’s Apache2 request-body handling so that a request-body parsing failure results in an HTTP 400 response instead of being treated as an internal server error.
Changes:
- Added a special-case mapping in
read_request_body()to returnHTTP_BAD_REQUESTwhenmodsecurity_request_body_end()returns-2.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (rcbe == -2) { | ||
| return HTTP_BAD_REQUEST; | ||
| } | ||
| if (rcbe < 0) { |
There was a problem hiding this comment.
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.
| 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; | |
| } |
|
#2807 |
|
This is a Marc Stern modification, I don't have much more insight on the code he made. To be reviewed with caution and check if this is still relevant