Repository navigation
Conversation
When the XML body processor fails, REQBODY_ERROR_MSG and the debug log
only said "XML: Failed to parse document." libxml2 knows the actual
reason (tag mismatch, extra content, empty document, ...) with line and
column, and until 3.0.15 that text leaked to stderr by accident.
Read xmlCtxtGetLastError() after a failed xmlParseChunk() and build the
message from it:
XML: Failed to parse document: Opening and ending tag mismatch:
book line 1 and bookasdf (line 1, column 725)
The fixed text stays as the fallback when libxml2 has no message. The
same applies to the ARGS parsing context. xmlCtxtGetLastError() is
available and non-deprecated on every libxml2 we build against
(checked 2.9.13 and 2.15.4) and keeps working under XML_PARSE_NOERROR,
so no version conditionals are needed.
Adds a regression case that matches REQBODY_ERROR_MSG on the libxml2
text and asserts the debug log line.
Fixes owasp-modsecurity#3678
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: exposing libxml2 parse errors in REQBODY_ERROR_MSG. Linked Issues check Issue #3678requires libxml2 diagnostics in REQBODY_ERROR_MSG and the debug log for document and ARGS parsing failures.src/request_body_processor/xml.ccaddsparseError(), which reads `xmlCtxtGet…Out of Scope Changes check The changes are limited to XML parse-error formatting in src/request_body_processor/xml.ccand a regression test intest/test-cases/regression/variable-REQBODY_PROCESSOR_ERROR.json. These changes …
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ 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.
Comment @coderabbitai help to get the list of available commands.
|
This branch has not been deployed
2 participants
what
When the XML request body processor fails,
REQBODY_ERROR_MSGand the debug log now carry libxml2's own reason, with line and column, instead of the fixedXML: Failed to parse document.:Implementation: after a failed
xmlParseChunk(), readxmlCtxtGetLastError()on the context and build the message from itsmessage,lineandint2fields. The fixed text remains as the fallback when libxml2 has no message. The same is done for the ARGS parsing context. Incomplete()the error is read beforexmlFreeParserCtxt().why
Until 3.0.15 this detail leaked to stderr by accident (#3563). #3677 silences that properly, so without this change the reason is lost entirely. Operators debugging rule 200002 hits often cannot capture bodies (PCI), and the libxml2 message alone usually tells whether it was a truncated body, a wrong
Content-Type, or a probe. Rule writers can also matchREQBODY_ERROR_MSGon it.xmlCtxtGetLastError()is available and non-deprecated on every libxml2 we build against (checked 2.9.13 and 2.15.4), and the 2.15 headers state errors stay accessible through it underXML_PARSE_NOERROR. NoLIBXML_VERSIONconditionals and no SAX or structured-error callback.Independent of #3677 in code; either can merge first.
tests
New case in
variable-REQBODY_PROCESSOR_ERROR.json: a rule matchingREQBODY_ERROR_MSGwith@contains Opening and ending tag mismatch: book line 1 and bookasdfdenies with 403, and the debug log is asserted with a regex that leaves the column number free. Fails onv3/master(200, generic message), passes with this change.make checkotherwise unchanged (only the 9 pre-existingremoveCommentsfailures from the submodule mismatch). The file also compiles clean against libxml2 2.15.4 headers.references
Fixes #3678. Related: #3563, #3677.
🤖 Generated with Claude Code
Summary by CodeRabbit