fix: ctl:ruleRemoveTargetByTag not removing XML attribute targets - #3592
Merged
airween merged 2 commits intoJul 2, 2026
Merged
Conversation
fetch_target_exception() only derived a target's parameter from a ":"-suffix embedded in var->name, which is how most variables (ARGS, TX, REQUEST_HEADERS, ...) represent their parameter. XML variables keep the XPath expression in the separate var->param field instead, so their parameter was never compared, and ctl:ruleRemoveTargetBy* could never match an XPath target like XML://@*. Fall back to var->param when var->name has no embedded parameter, so target-exception matching also works for XML XPath targets. Fixes owasp-modsecurity#3591 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fzipi
force-pushed
the
v2/fix-3591-xml-attr-target-removal
branch
from
July 1, 2026 21:19
bfd5a6e to
091ba17
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes target-exception matching for XML XPath targets by teaching fetch_target_exception() to use var->param when the variable name doesn’t embed a :param suffix, enabling ctl:ruleRemoveTargetBy* to correctly remove targets like XML://@* during a transaction.
Changes:
- Update
fetch_target_exception()to fall back tovar->paramfor parameter comparison whenvar->namehas no embedded parameter. - Add a regression test proving
ctl:ruleRemoveTargetByTagcan suppress matches onXML://@*attribute-value inspection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apache2/re.c | Uses var->param as the parameter source when var->name lacks an embedded :param, allowing XPath targets to be matched/removed. |
| tests/regression/rule/10-xml.t | Adds baseline + regression coverage for removing XML://@* via ctl:ruleRemoveTargetByTag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
airween
approved these changes
Jul 2, 2026
theseion
approved these changes
Jul 2, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fetch_target_exception()only read a target's parameter from a:-suffix embedded invar->name, which is how most variables (ARGS,TX,REQUEST_HEADERS, ...) represent their parameter.XMLvariables keep the XPath expression in the separatevar->paramfield instead, so their parameter was never compared, andctl:ruleRemoveTargetBy*could never match an XPath target likeXML://@*.var->paramwhenvar->namehas no embedded parameter, so target-exception matching also works for XML XPath targets.Fixes #3591
Test plan
tests/regression/rule/10-xml.tcovering: (1) baseline match ofXML://@*against an XML attribute value, and (2)ctl:ruleRemoveTargetByTagcorrectly suppressing that match once the target is removed.gcc -fsyntax-only) with no new warnings.qr///qq()escaping renders the expected literal strings.