Update SQLi/XSS operators for libinjection v4.0.0 cleaned#3528
Update SQLi/XSS operators for libinjection v4.0.0 cleaned#3528Easton97-Jens wants to merge 20 commits intoowasp-modsecurity:v3/masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates ModSecurity’s @detectSQLi / @detectXSS operators to support libinjection v4’s injection_result_t return codes, including explicit fail-safe handling for parser errors, and expands regression coverage around detection/false-positive behavior.
Changes:
- Add shared helpers for interpreting libinjection
TRUE/FALSE/ERRORresults. - Update
DetectSQLi/DetectXSSto treatLIBINJECTION_RESULT_ERRORas a match and preserve capture behavior. - Expand regression test cases for multiple XSS/SQLi payloads and benign inputs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/operators/libinjection_utils.h |
Adds shared helpers to map libinjection results to match/no-match semantics and diagnostic strings. |
src/operators/detect_xss.cc |
Switches XSS operator logic to injection_result_t and adds explicit handling for TRUE/FALSE/ERROR. |
src/operators/detect_sqli.cc |
Switches SQLi operator logic to injection_result_t, modernizes fingerprint storage, and handles TRUE/FALSE/ERROR. |
test/test-cases/regression/operator-detectxss.json |
Adds multiple positive and negative XSS regression cases. |
test/test-cases/regression/operator-detectsqli.json |
Adds multiple positive and negative SQLi regression cases (including fingerprint expectations). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…for-modsecurity-operators Add regression coverage for detectSQLi/detectXSS capture semantics
…for-libinjection-result_error Add libinjection adapter for test overrides and improve libinjection error handling
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (t.capture) { | ||
| auto tx0 = transaction.m_collections.m_tx_collection->resolveFirst("0"); | ||
| if (tx0 != nullptr) { | ||
| result.output = *tx0; | ||
| } |
There was a problem hiding this comment.
OperatorTest::eval reads TX.0 from the shared Transaction when capture is enabled. In mtstress mode the same transaction instance is shared across many threads, so concurrent writes/reads of the TX collection can cause data races and flaky/crashing unit tests (especially now that output comparisons are enforced). Consider using a per-thread Transaction in mtstress runs (or disabling capture/output assertions under mtstress) so TX.0 access is thread-safe/deterministic.
There was a problem hiding this comment.
even thought this case (shared transaction objects) is a bit far from the reality, I think it would be nice to find a solution to meet this expectation.
(Moreover, there is no mtstress option at all between test cases.)
Could you take a look at this?
If you think it's not necessarily now, we can skip now, and we might add this later (with new test cases...)
…transaction-usage Use ModSecurityTestContext to create per-thread transactions in multithreaded tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor multithreaded unit test to use thread-specific context.
|



what
libinjectionreturn codes (injection_result_t).TRUE,FALSE, andERRORresults fromlibinjection_sqliandlibinjection_xss.LIBINJECTION_RESULT_ERRORas a fail-safe match to avoid missing potentially malicious input.TX.0whencaptureis enabled, even on parser errors.why
libinjectionintroducedinjection_result_t, requiring explicit handling in ModSecurity operators.references
libinjectionAPIs.