Fix case-insensitive request header target exclusions - #3616
TejasButani001 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change makes non-regex variable target matching case-insensitive. A regression test verifies that lowercase ChangesCase-Insensitive Target Matching
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR makes a localized case-insensitivity correction with accompanying regression coverage, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
Hi @TejasButani001, thanks for this PR - will this close #3609? |
Hi @airween, yes. This PR fixes the case-insensitive REQUEST_HEADERS target exclusion issue described in #3609, so it should close #3609 once merged. Thanks! |
I also noticed the Linux QA workflows were cancelled after 360 minutes. My local regression test passes all 6/6 tests, and SonarCloud/CodeRabbit checks have passed. |
There was a problem hiding this comment.
🟡 Changes recommended
The current case-insensitive comparison implementation introduces avoidable per-comparison string allocations in a hot evaluation path and should be rewritten to a non-allocating case-insensitive compare.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR fixes a mismatch between HTTP’s case-insensitive header semantics and ModSecurity’s target-exclusion matching by ensuring REQUEST_HEADERS exclusions match regardless of the header name’s casing.
Changes:
- Add a regression test covering
SecRuleUpdateTargetById ... !REQUEST_HEADERS:refererexcluding aRefererheader. - Update variable-value exclusion matching to compare the full
collection:keyname case-insensitively.
File summaries
| File | Description |
|---|---|
| test/test-cases/regression/config-update-target-by-id.json | Adds a regression test ensuring REQUEST_HEADERS exclusions work across header-name casing. |
| src/variables/variable.h | Adjusts Variables::contains(const VariableValue*) to perform case-insensitive matching. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (r) { | ||
| return r->m_r.searchAll(v->getKey()).size() > 0; | ||
| } | ||
| return v->getKeyWithCollection() == *m->m_fullName.get(); | ||
| return utils::string::toupper(v->getKeyWithCollection()) == utils::string::toupper(*m->m_fullName.get()); | ||
| }) != end(); |
| } | ||
|
|
||
| , | ||
| { |
Summary
Fix case-sensitive matching of REQUEST_HEADERS target exclusions.
Previously, an exclusion such as:
SecRuleUpdateTargetById 1 !REQUEST_HEADERS:referer
could fail to exclude the corresponding request header when the header name
used a different case.
This change makes REQUEST_HEADERS target exclusion matching case-insensitive.
Testing
Added a regression test covering case-insensitive REQUEST_HEADERS exclusion.