feat: experimental xmlquery body processor with lazy XPath#1594
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new experimental XML query body processor has been added, enabling XPath-based querying of incoming XML payloads. The feature includes a lazy-evaluated collection implementation ( Changes
Sequence DiagramsequenceDiagram
actor Client
participant Plugin as XMLQuery Plugin
participant Parser as xmlquery Parser
participant Map as XPathMap
participant Vars as TransactionVariables
Client->>Plugin: ProcessRequest (XML body)
Plugin->>Parser: Parse XML stream to DOM
Parser-->>Plugin: *xmlquery.Node
Plugin->>Map: NewXPathMap(variable, doc)
Map-->>Plugin: XPathMap instance
Plugin->>Vars: SetRequestXML(xpathMap)
Vars->>Vars: Store XPathMap as<br/>requestXML collection
Plugin-->>Client: Request processed
Client->>Vars: RequestXML().Get("//path")
Vars->>Map: Get(key)
Map->>Parser: Evaluate XPath on DOM
Parser-->>Map: Matching node values
Map-->>Vars: Trimmed strings
Vars-->>Client: Query results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1594 +/- ##
==========================================
+ Coverage 87.41% 87.58% +0.17%
==========================================
Files 179 181 +2
Lines 8825 8917 +92
==========================================
+ Hits 7714 7810 +96
+ Misses 852 848 -4
Partials 259 259
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
go.mod (1)
20-20: Document the new dependency.Per coding guidelines, dependencies should be documented with their purpose. The
xmlquerylibrary enables the experimental XPath body processor but isn't mentioned in the dependency comments above.📝 Suggested documentation update
Add to the build dependencies comment block (around line 12-17):
// Build dependencies: // - libinjection-go // - aho-corasick // - gjson // - binaryregexp // - ocsf-schema-golang +// - xmlquery (experimental XPath body processor)Based on learnings: "Applies to go.mod : Document why each dependency is needed"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 20, Add a comment documenting the new dependency github.com/antchfx/xmlquery v1.5.1 in the go.mod build dependencies block: state that xmlquery is used to enable the experimental XPath body processor (parsing and querying XML payloads), so readers understand why the dependency is present; update the comment block near the other dependency notes to reference the xmlquery purpose.experimental/plugins/xmlquery.go (1)
66-88: Consider logging XPath evaluation errors for debugging.Lines 69 and 75 silently discard errors from
xmlquery.QueryAll. While these hardcoded XPath expressions (//@*and//*[text()]) should always be valid, logging errors at debug level would aid troubleshooting unexpected failures.📝 Optional: Add debug logging for errors
This would require passing a logger through the call chain, so it may not be worth the complexity for this fallback path. The current behavior (returning empty slices on error) is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experimental/plugins/xmlquery.go` around lines 66 - 88, Modify extractFromDoc to check and handle the errors returned by both xmlquery.QueryAll calls (the "//@*" and "//*[text()]" queries): capture the error, and if non-nil, emit a debug-level log that includes the XPath expression and the error before continuing to return empty slices; use the existing logging mechanism available in this package (or a passed-in logger if this function is later refactored) so the function still returns attrs/content safely on error but logs the failure for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/collections/xpath_map.go`:
- Around line 103-116: The FindAll method in XPathMap uses two for-loops to
append slices from c.FindString("//@*") and c.FindString("//*[text()]") into
result, which triggers staticcheck S1011; replace the loops by appending the
returned slices with variadic append calls (i.e., result = append(result,
<slice>... ) ) for both uses of c.FindString so FindAll collects attributes and
text nodes without per-element loops.
---
Nitpick comments:
In `@experimental/plugins/xmlquery.go`:
- Around line 66-88: Modify extractFromDoc to check and handle the errors
returned by both xmlquery.QueryAll calls (the "//@*" and "//*[text()]" queries):
capture the error, and if non-nil, emit a debug-level log that includes the
XPath expression and the error before continuing to return empty slices; use the
existing logging mechanism available in this package (or a passed-in logger if
this function is later refactored) so the function still returns attrs/content
safely on error but logs the failure for debugging.
In `@go.mod`:
- Line 20: Add a comment documenting the new dependency
github.com/antchfx/xmlquery v1.5.1 in the go.mod build dependencies block: state
that xmlquery is used to enable the experimental XPath body processor (parsing
and querying XML payloads), so readers understand why the dependency is present;
update the comment block near the other dependency notes to reference the
xmlquery purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46f981fa-7aa8-45ad-8b66-bf5885a13226
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
experimental/plugins/xmlquery.goexperimental/plugins/xmlquery_test.gogo.modinternal/collections/xpath_map.gointernal/collections/xpath_map_test.gointernal/corazawaf/transaction.go
Benchmark Results
Small documents (~1KB)
Medium documents (~9–22KB)
Large document (~298KB, 500 employees, 4 namespaces)
Coverage(Set/Add/SetIndex/Remove are empty-body no-ops — 0 statements, so 100% total is correct.) |
|
IMHO we should deprecate xml... now even libxml is getting deprecated |
|
Sure. Let's remove then that we are CRS compatible. Why do you want to remove support or move it as a plugin? If people don't use the bodyprocessor, nothing changes. 🤷 |
|
Once CRS moves to CRSLang, then this might be a viable option depending on people's needs. For now, we are tied to CRS, and Seclang, sadly. |
|
You are importing a library that is not battle tested for a sensitive feature. We might end up with cves for free... we can do this in a plugin and whoever wants full support for xml they can use this, we can even support libxml2 like modsecurity using cgo. I personally prefer to use libxml2, although it's deprecated |
|
libxml2 is not deprecated, has new maintainers. Also, it's CGo. The actual library we are using now is not battle tested, either. How come it is sensitive now, if you wanted to deprecate the support for it? |
|
I would prefer even libxml2 with a cgo wrapper and a build tag. We can copy the code parameters straight from modsecurity if needed, we don't need to reinvent it. |
|
Can we focus on the features we want?
Bottom line comparision is here: #1594 (comment) |
CRS Benchmark Comparison: XML vs XMLQuery Body ProcessorFull CRS rule pipeline benchmarks on Apple M2, Simple XML-RPC POST (~250B payload)
Large SOAP POST (~15KB, 100 items, namespaces)
AnalysisThe xmlquery processor is ~1.8–2x slower with ~1.7–2x more memory. This is the expected cost of building a full DOM tree for lazy XPath evaluation vs. the original's simple single-pass streaming extraction. The tradeoff enables real XPath support ( CRS CompatibilityThe full CRS FTW regression suite passes with both processors — all tests pass when switching from |
XPath Compatibility Analysis: ModSecurity vs Coraza XML processorsHow XPath works in ModSecurity rulesModSecurity supports any XPath 1.0 expression via libxml2. The syntax in rules is: For namespace-qualified XPath, ModSecurity uses an What CRS actually usesCRS only uses CRS never uses Feature comparison
The remaining gap:
|
fzipi
force-pushed
the
feat/experimental-xmlquery-bodyprocessor
branch
from
3bf8382 to
aebd8a3
Compare
…tion Add a new "xmlquery" body processor backed by antchfx/xmlquery that parses XML request bodies into a DOM and evaluates XPath expressions lazily on demand. This enables real XPath support (e.g. XML:/soap:Envelope) that the existing XML processor cannot provide. Key changes: - New XPathMap collection type implementing collection.Map with lazy XPath evaluation against a parsed XML document - New xmlquery body processor in experimental/plugins registered as "xmlquery", activated via ctl:requestBodyProcessor=XMLQUERY - Widen requestXML/responseXML/xml transaction fields from concrete *collections.Map to collection.Map interface, enabling polymorphic collection implementations - Add SetRequestXML/SetResponseXML setters on TransactionVariables (concrete struct only, no interface change) so body processors can install custom collections via type assertion - Fallback path for TransactionVariables implementations that don't support the setter interface Relates to #1441
Move the XPathMap collection from internal/collections to experimental/plugins/collections, keeping it alongside the other experimental plugin code. Update xmlquery body processor to import from the new location.
Achieve 100% statement coverage on xpath_map.go with tests covering: - All query methods (Get, FindString, FindRegex, FindAll) - Edge cases (nil doc, invalid XPath, whitespace-only nodes, no match) - No-op mutation methods and Format - Diverse XML structures: SOAP namespaces, CDATA, XML-RPC, deep nesting Add benchmarks for small (~1KB), medium (~22KB), and large (~298KB) documents measuring parse time, element/attribute queries, predicate filters, namespace-aware queries, and FindAll/FindString/FindRegex. Add testdata with generated XML fixtures of varying complexity: - small_simple.xml, small_soap.xml, small_cdata.xml, small_xmlrpc.xml - medium_mixed.xml (50 books, namespaces, reviews, CDATA, metadata) - medium_deep.xml (30-level nesting with siblings) - large_catalog.xml (500 employees, 4 namespaces, access logs, CDATA)
Ensures the setter methods in transaction.go are covered by the default test run, fixing Codecov patch coverage for these lines.
Run the full OWASP CRS regression test suite using the experimental xmlquery body processor to verify compatibility. Uses SecRuleUpdateActionById to override rule 200000, switching from requestBodyProcessor=XML to requestBodyProcessor=XMLQUERY. All CRS tests pass, confirming the lazy XPath-backed collection is fully compatible with CRS rules that inspect XML request bodies.
Add matching benchmarks for the original XML and experimental xmlquery body processors processing XML request bodies through the full CRS rule pipeline: - BenchmarkCRSXMLSimplePOST / BenchmarkCRSXMLQuerySimplePOST: Small XML-RPC payload (~250B) - BenchmarkCRSXMLLargeSOAP / BenchmarkCRSXMLQueryLargeSOAP: Large SOAP envelope (~15KB, 100 items, namespaces) Results on Apple M2 show xmlquery is ~1.8-2x slower due to full DOM construction, which is the expected tradeoff for real XPath support.
fzipi
force-pushed
the
feat/experimental-xmlquery-bodyprocessor
branch
from
aebd8a3 to
5e3c8a3
Compare
2 participants
Summary
Adds a new experimental
xmlquerybody processor that enables real XPath 1.0 evaluation for XML request bodies in Coraza, closing the gap with ModSecurity's XML inspection capabilities.Problem
The existing
xmlbody processor uses Go'sencoding/xmlto extract two hardcoded keys (//@*for all attribute values,/*for all text content). XPath expressions in rules likeXML:/soap:Envelope/soap:Bodyare parsed by the seclang parser but treated as literal map keys — since those keys never exist in the collection, they silently match nothing. This means Coraza cannot inspect specific XML elements, use descendant axes, predicates, or any real XPath beyond the two hardcoded keys.Solution
xmlquerybody processor inexperimental/plugins/, backed by antchfx/xmlquery. Parses XML into an in-memory DOM and evaluates XPath expressions lazily on demand via a newXPathMapcollection typectl:requestBodyProcessor=XMLQUERY; the existingxmlprocessor is unchangedXPathMapcollection (experimental/plugins/collections/) implementscollection.Mapinterface.Get/FindString/FindRegex/FindAllevaluate XPath against the stored DOM at query time rather than requiring pre-computed keysrequestXML/responseXML/xml) from*collections.Maptocollection.Mapinterface, keeping transaction code fully agnostic of which body processor is in useSetRequestXML/SetResponseXMLsetters added to the concreteTransactionVariablesstruct (not the interface — no breaking change). The body processor type-asserts to install the custom collection; falls back to populating the standard//@*and/*keys if the setter is unavailableXPath feature comparison
XMLXMLQUERYXML:/*andXML://@*/a/b/c)//descendant axis[text()],[local-name()='x']predicates[@attr='value']predicatesxmlns:actionPerformance
The xmlquery processor is ~1.8–2x slower than the original due to full DOM construction (vs. single-pass streaming). This is the expected tradeoff for real XPath support. See benchmark results in PR comments.
CRS compatibility
The full OWASP CRS FTW regression test suite passes with both processors. CRS only uses
XML:/*, which both processors support identically.Files changed
experimental/plugins/xmlquery.go— Body processor registered as"xmlquery"experimental/plugins/xmlquery_test.go— Body processor tests (14 tests)experimental/plugins/collections/xpath_map.go— Lazy XPath collectionexperimental/plugins/collections/xpath_map_test.go— Collection tests (22 tests) + benchmarksexperimental/plugins/collections/testdata/— XML fixtures (simple, SOAP, CDATA, XML-RPC, medium mixed, deep nesting, large catalog)internal/corazawaf/transaction.go— Widened field types, added setterstesting/coreruleset/coreruleset_xmlquery_test.go— CRS FTW test + benchmarks with xmlqueryTest plan
xpath_map.goandxmlquery.gotransaction_test.gofor SetRequestXML/SetResponseXMLencoding/xmldoes not support external entity resolutionRelates to #1441