fix: align HIGHEST_SEVERITY with ModSecurity behavior#1569
fix: align HIGHEST_SEVERITY with ModSecurity behavior#1569majiayu000 wants to merge 10 commits intocorazawaf:mainfrom
Conversation
Default HIGHEST_SEVERITY to 255 (no severity set) instead of 0 (emergency). Fix comparison to use < instead of > since lower numbers represent higher severity. Use strconv.Atoi instead of ParseRuleSeverity which cannot parse the sentinel value 255. Fixes corazawaf#1567 Signed-off-by: majiayu000 <1835304752@qq.com>
Add test verifying HIGHEST_SEVERITY defaults to 255 (ModSecurity sentinel) when no rules with severity have fired. Add TODO comment documenting that rules without an explicit severity action have Severity_=0 (Go zero value = Emergency), which may incorrectly update HIGHEST_SEVERITY. Signed-off-by: majiayu000 <1835304752@qq.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1569 +/- ##
==========================================
- Coverage 86.68% 86.67% -0.02%
==========================================
Files 179 179
Lines 8790 8794 +4
==========================================
+ Hits 7620 7622 +2
- Misses 911 912 +1
- Partials 259 260 +1
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:
|
M4tteoP
left a comment
M4tteoP
left a comment
There was a problem hiding this comment.
Please see also #1567 (comment)
Address review feedback: replace hardcoded "255" with a defaultHighestSeverity const and add ModSecurity v2/v3 source references. Signed-off-by: majiayu000 <1835304752@qq.com>
|
Hi @M4tteoP, here are the specific ModSecurity v2 and v3 source references confirming the 255 default and comparison logic: ModSec v2 (
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDefault HIGHEST_SEVERITY is set to "255"; Transaction now initializes the variable to that value and only updates it when a matched rule explicitly sets a severity (marked by a new Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 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/corazawaf/transaction.go`:
- Around line 558-566: Highest-severity tracking is poisoned by rules that omit
severity because Severity_ defaults to 0; change the logic to only consider a
rule's severity when it was explicitly set: add a boolean marker on the rule
metadata (e.g., HasSeverity on RuleMetadata or make Severity_ an optional type)
and update the comparison around tx.variables.highestSeverity
(hs.Get()/hs.Set()) to skip rules that do not have HasSeverity true; initialize
the highest-severity sentinel to a value that cannot be a real severity and
ensure the update uses r.Severity_.Int() only when the rule’s HasSeverity flag
is true so a later explicit severity can win (also update any rule
creation/parsing paths that populate HasSeverity).
🪄 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: b0f7af28-bc26-464a-98a7-40837357fde1
📒 Files selected for processing (3)
internal/corazawaf/transaction.gointernal/corazawaf/waf.gotesting/engine/rulemetadata.go
Rules without a severity action have Severity_=0 (Go zero value), which is indistinguishable from RuleSeverityEmergency. This poisoned HIGHEST_SEVERITY by writing 0 on the first no-severity match, making later explicit severities unable to win. Add HasSeverity_ flag to RuleMetadata, set it in the severity action, and only update HIGHEST_SEVERITY when the flag is true. This mirrors ModSecurity v3 which checks m_severity > -1 before updating. Adds regression test: no-severity rule followed by severity:5 rule must result in HIGHEST_SEVERITY=5, not 0. Signed-off-by: majiayu000 <1835304752@qq.com>
|
@M4tteoP The const comment now references both ModSec v2 ( Also just pushed a fix for the zero-value poisoning issue — rules without an explicit |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/corazawaf/transaction.go`:
- Around line 562-564: The code calls strconv.Atoi(hs.Get()) and ignores the
returned error; update the block around hs.Get()/strconv.Atoi to check the error
returned by strconv.Atoi and handle it (e.g., log and/or return, or fall back to
a safe default) before comparing with r.Severity_.Int(); specifically modify the
logic that assigns currentVal and uses hs.Set(strconv.Itoa(...)) so that
strconv.Atoi's error is checked and handled consistently with project guidelines
while preserving existing behavior of hs.Set and the comparison with
r.Severity_.Int().
🪄 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: 24a740d9-f0e4-447b-a37f-5743583663fd
📒 Files selected for processing (4)
internal/actions/severity.gointernal/corazarules/rule.gointernal/corazawaf/transaction.gotesting/engine/rulemetadata.go
🚧 Files skipped from review as they are similar to previous changes (1)
- testing/engine/rulemetadata.go
Fall back to sentinel value 255 if Atoi fails, so any real severity can still win. In practice this path is unreachable since the variable is always set via strconv.Itoa, but handle it explicitly for correctness. Signed-off-by: majiayu000 <1835304752@qq.com>
|
@majiayu000 Can you apply |
Signed-off-by: majiayu000 <1835304752@qq.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/corazarules/rule.go (1)
22-24: DocumentHasSeverity_semantics inline.
HasSeverity_changes rule-metadata semantics, but without a field comment it’s easy to misreadfalseas severity0instead of “unset”. Please add an inline comment clarifying that it tracks whether severity was explicitly configured.Proposed doc-only patch
Severity_ types.RuleSeverity + // HasSeverity_ reports whether the rule explicitly set a severity action. + // This distinguishes an unset value from RuleSeverityEmergency (0). HasSeverity_ bool Version_ stringBased on learnings: Document any intentional deviations from ModSecurity compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/corazarules/rule.go` around lines 22 - 24, Add an inline comment on the HasSeverity_ field clarifying its semantics: explain that HasSeverity_ is a boolean sentinel indicating whether Severity_ was explicitly configured (true) versus unset (false), and that a false value does not mean severity zero; reference the Severity_ and HasSeverity_ fields in the comment and note any intentional ModSecurity compatibility deviation if applicable so readers understand the difference between "unset" and an actual severity value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/corazarules/rule.go`:
- Around line 22-24: Add an inline comment on the HasSeverity_ field clarifying
its semantics: explain that HasSeverity_ is a boolean sentinel indicating
whether Severity_ was explicitly configured (true) versus unset (false), and
that a false value does not mean severity zero; reference the Severity_ and
HasSeverity_ fields in the comment and note any intentional ModSecurity
compatibility deviation if applicable so readers understand the difference
between "unset" and an actual severity value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee1eadff-65ce-4342-a078-0d285140a68f
📒 Files selected for processing (1)
internal/corazarules/rule.go
fzipi
left a comment
fzipi
left a comment
There was a problem hiding this comment.
Probably it is better to add this to the types/severity.go instead?
internal/corazawaf/waf.go
Outdated
| // https://github.com/owasp-modsecurity/ModSecurity/blob/v2/master/apache2/msc_util.c | ||
| // - ModSec v3: src/transaction.cc m_highestSeverity initialized to 255 | ||
| // https://github.com/owasp-modsecurity/ModSecurity/blob/v3/master/src/transaction.cc | ||
| defaultHighestSeverity = "255" |
There was a problem hiding this comment.
I would use an int here instead
| defaultHighestSeverity = "255" | |
| defaultHighestSeverity = 255 |
There was a problem hiding this comment.
Thanks, will change defaultHighestSeverity to int and remove the strconv roundtrip.
Suggestion: encode "unset" in
|
Signed-off-by: majiayu000 <1835304752@qq.com>
Signed-off-by: majiayu000 <1835304752@qq.com>
3 participants
Fixes #1567
Make sure that you've checked the boxes below before you submit PR:
Summary
HIGHEST_SEVERITYvariable was not aligned with ModSecurity behavior in two ways:Wrong default (
waf.go): Initialized to"0"instead of"255". In ModSecurity, 255 is the sentinel value meaning "no severity set yet".Inverted comparison (
transaction.go): Used>but severity is inverted (lower number = higher severity), so it should be<. Additionally,ParseRuleSeveritycan't parse"255", which returned 0 on error and masked the sentinel.Changes
internal/corazawaf/waf.go: Change default from"0"to"255"internal/corazawaf/transaction.go: Fix comparison to usestrconv.Atoi+<instead ofParseRuleSeverity+>testing/engine/rulemetadata.go: Add test profiles for default value (255) and multiple severity trackingEdge case note
Rules without an explicit
severityaction haveSeverity_=0(Go zero value =RuleSeverityEmergency), which is indistinguishable from an explicitly set Emergency severity. In ModSecurity, rule default severity is-1(not set) and doesn't affectHIGHEST_SEVERITY. A sentinel value orHasSeverityflag onRuleMetadatacould address this, but that's a larger change — happy to discuss the best approach.Thanks for your contribution ❤️
Summary by CodeRabbit
Bug Fixes
Tests