Repository navigation
Conversation
Since 3.0.15 (b9dc9cc) the XML body processor sets XML_PARSE_NOERROR and XML_PARSE_NOWARNING by OR-ing them into ctxt->options directly. libxml2 only honours options stored that way from 2.13 on; older releases (2.9 through 2.12, e.g. Amazon Linux 2023, Ubuntu 22.04, RHEL 9) apply the flags inside xmlCtxtUseOptions() by clearing the SAX error callbacks, so the direct assignment is a no-op and every malformed body prints "body.xml:1: parser error : ..." to stderr, which nginx forwards to its error log without a timestamp prefix. 3.0.14 did not show this because it still called xmlSetGenericErrorFunc() with a null handler, which 624a0de removed as deprecated. Call xmlCtxtUseOptions() instead, which works on both old and new libxml2 and is not deprecated. Verified with a push-parser test program against libxml2 2.9.13 and 2.15.4, and with the existing variable-REQBODY_PROCESSOR_ERROR regression test, whose stderr output goes from one parser error line to none. Fixes owasp-modsecurity#3563 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 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Issue #3563reports unwanted libxml2 parser-error output for malformed XML request bodies. Insrc/request_body_processor/xml.cc, both push-parser contexts now callxmlCtxtUseOptions()with `XML_PA…Out of Scope Changes check The reviewed change is limited to XML body-parser option configuration in src/request_body_processor/xml.cc. Both modified calls support the#3563objective. The summary reports no public API change…Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main change: silencing libxml2 parser errors across supported libxml2 versions.
- 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
1 participant
what
Replace the direct
ctxt->options |= XML_PARSE_NOWARNING | XML_PARSE_NOERRORassignments in the XML body processor withxmlCtxtUseOptions().why
Since 3.0.15, a malformed XML request body makes libxml2 print its own error (
body.xml:1: parser error : Extra content at the end of the documentplus the context lines) to stderr. With the nginx connector that lands inerror.logwithout the usual timestamp prefix (#3563).xmlSetGenericErrorFunc()call with a null handler, which is what kept 3.0.14 quiet.NOERROR/NOWARNINGflags straight intoctxt->options.xmlCtxtUseOptions()by clearing the SAX error callbacks, so the direct assignment is a no-op there. That is why the reporters on Amazon Linux see the lines and the maintainers on newer distros could not reproduce.xmlCtxtUseOptions()works on both old and new libxml2 and is not deprecated.Verified with a small push-parser program against libxml2 2.9.13 and 2.15.4:
options |=(current)xmlCtxtUseOptions()and with the existing
variable-REQBODY_PROCESSOR_ERROR.jsonregression test on a build linked against 2.9.13: one parser error line on stderr before, none after. The regression harness cannot assert on stderr, so there is no new automated test.references
Fixes #3563. Related: #3455 (624a0de, b9dc9cc).
🤖 Generated with Claude Code
Summary by CodeRabbit