feat: add lint rule for ARGS_NAMES start-anchored operators missing json prefix#155
Open
fzipi wants to merge 4 commits into
Open
feat: add lint rule for ARGS_NAMES start-anchored operators missing json prefix#155fzipi wants to merge 4 commits into
fzipi wants to merge 4 commits into
Conversation
…son prefix libModSecurity3 and Coraza prefix JSON body parameter names with 'json.' (e.g. 'username' becomes 'json.username'), while ModSecurity2 does not. Rules targeting ARGS_NAMES with @rx anchored to '^', @beginswith, or @Streq will silently miss JSON parameters on libModSecurity3/Coraza unless they include a '(?:json\.)?' prefix. The new 'args_names_json_prefix' rule flags these cases and guides rule-writers to use '^(?:json\.)?...' instead. Closes #154 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
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)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new ChangesArgsNamesJsonPrefix lint rule
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/crs_linter/rules/args_names_json_prefix.py`:
- Around line 8-9: The missing-prefix check in args_names_json_prefix.py is too
broad because _MISSING_JSON_PREFIX_RE only excludes the partial "^(?:json" stem,
so anchored `@rx` patterns like "^(?:json\\.)foo" or "^(?:jsonx)" are incorrectly
exempted. Tighten the regex used by the rule to match only the full optional
"json." prefix form, and update the related logic in args_names_json_prefix.py
so it compares against the complete allowed engine prefix rather than just the
beginning of it.
🪄 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 Plus
Run ID: 8fdeafe1-0173-4a0f-af51-a2dc34b3ec40
📒 Files selected for processing (4)
README.mdsrc/crs_linter/linter.pysrc/crs_linter/rules/args_names_json_prefix.pytests/test_args_names_json_prefix.py
…token The previous lookahead (?!\(\?:json) only checked for the stem "(?:json", which incorrectly exempted malformed patterns such as: - ^(?:json\.)foo — mandatory group (missing trailing ?) - ^(?:jsonx)?foo — wrong prefix name - ^(?:json)?foo — missing dot The new lookahead (?!\(\?:json\\?\.\)\?) matches the complete optional group, so only well-formed ^(?:json\.)? or ^(?:json.)? prefixes are accepted. Three regression test cases added. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new CRS Linter rule to catch a subtle portability issue in ModSecurity rules: when targeting ARGS_NAMES, start-anchored operators can miss JSON parameters on libModSecurity3/Coraza unless the pattern accounts for the json. prefix.
Changes:
- Introduces a new lint rule
args_names_json_prefixto flagARGS_NAMESusage with start-anchored@rx ^..., plus@beginsWithand@streq. - Adds a dedicated test suite covering a range of operator/target combinations, chaining, and multi-rule input.
- Updates rule auto-registration import and README rule documentation/index.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/crs_linter/rules/args_names_json_prefix.py |
Implements the new lint rule and its detection logic + messaging. |
tests/test_args_names_json_prefix.py |
Adds parametrized coverage and additional assertion-focused tests for the new rule. |
src/crs_linter/linter.py |
Imports the new rule module to trigger metaclass auto-registration. |
README.md |
Adds generated documentation entry for the new rule. |
Skip flagging ^-anchored @rx patterns whose leading branch is a negated character class that doesn't exclude any character in "json." (e.g. CRS rules 921200, 942540) — that class absorbs the prefix, so the pattern still matches JSON-prefixed ARGS_NAMES values without an explicit (?:json\.)? group. Verified against the real CRS 4.28.0 rules: 932171 is still flagged since it has no such tolerance. Also keep the original operator casing (e.g. @beginswith) in error messages instead of the lowercased comparison value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
Author
|
Needs coreruleset/coreruleset#4703 and release 4.29.0 before merging. |
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
args_names_json_prefixrule that flagsSecRuledirectives targetingARGS_NAMESwith start-anchored operators that don't account for thejson.prefix added by libModSecurity3/Coraza to JSON body parameter names@rx ^pattern(start-anchored without(?:json\.)?),@beginsWith, and@streqwhen used withARGS_NAMESBackground
libModSecurity3 and Coraza prefix JSON body parameter names with
json.(e.g.username→json.username), while ModSecurity2 does not. A rule likeSecRule ARGS_NAMES "@rx ^username$"silently misses JSON parameters on libModSecurity3/Coraza. The fix is to use^(?:json\.)?username$instead.This is the same class of bug fixed in coreruleset/coreruleset#4672.
Prerequisites
Test plan
uv run pytest tests/test_args_names_json_prefix.py -vs— all 24 new tests passuv run pytest -q— full suite (407 tests) passesuv run python generate_rules_docs.py --check— README is up to dateCloses #154
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
SecRuleuses of start-anchoredARGS_NAMESthat don’t account for the optionaljson.-prefixed parameter naming.Tests
ARGS_NAMESscenarios, including chained rules, negation behavior, and JSON-prefix matching edge cases.