feat(auditlog): add libmodsecurity v3 JSON formatter. - #1638
Conversation
|
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:
📝 WalkthroughWalkthroughAdds structured match and reference data to audit messages, introduces a libmodsecurity-compatible JSONV3 formatter, integrates it with audit-log generation, registers it across builds, and documents and tests the new format. ChangesAudit-log JSONV3
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
| } | ||
|
|
||
| func (m Message) Data() plugintypes.AuditLogMessageData { | ||
| if m.Data_ == nil { |
There was a problem hiding this comment.
Based on my understanding of Go, it's needed because returning m.Data_ as an interface produces a non-nil typed-nil value, so the JSONV3 formatter's data == nil guard would not work and it could panic when calling methods on it
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/corazawaf/transaction.go (1)
1647-1667: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the repeated
strings.Joinout of the per-message loop.
strings.Join(tx.WAF.ComponentNames, " ")doesn't vary across messages or rules but is recomputed for every matchData iteration.♻️ Proposed fix
func (tx *Transaction) auditLogMessages(includeErrorMessage bool) []plugintypes.AuditLogMessage { var messages []plugintypes.AuditLogMessage + actionset := strings.Join(tx.WAF.ComponentNames, " ") for _, mr := range tx.matchedRules { ... messages = append(messages, auditlog.Message{ - Actionset_: strings.Join(tx.WAF.ComponentNames, " "), + Actionset_: actionset, Message_: matchData.Message(),🤖 Prompt for 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. In `@internal/corazawaf/transaction.go` around lines 1647 - 1667, Compute strings.Join(tx.WAF.ComponentNames, " ") once before the per-message matchData loop, store the result in a local variable, and use that variable for Message.Actionset_ within the auditlog.Message construction.
🤖 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 `@internal/auditlog/formats_json_test.go`:
- Around line 202-272: Add an assertion in the formatted message checks for
(*transaction.Messages)[0].Details.Match, verifying it equals the Match_ value
configured in the test message data. Place it alongside the existing RuleID and
Reference assertions.
In `@internal/corazawaf/transaction.go`:
- Around line 567-572: The Match_ fallback in the transaction audit-log loop
uses the parent rule for chained child matches. Update MatchData creation or its
record structure so each MatchData retains its originating rule/operator, then
use that source when formatting Match_ instead of r.auditLogMatch(md); preserve
existing behavior for matches already populated.
---
Nitpick comments:
In `@internal/corazawaf/transaction.go`:
- Around line 1647-1667: Compute strings.Join(tx.WAF.ComponentNames, " ") once
before the per-message matchData loop, store the result in a local variable, and
use that variable for Message.Actionset_ within the auditlog.Message
construction.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d856f040-4e76-4d1e-96c2-0bfa2aca1941
📒 Files selected for processing (12)
internal/auditlog/auditlog.gointernal/auditlog/formats.gointernal/auditlog/formats_json_test.gointernal/auditlog/formats_json_v3.gointernal/auditlog/init.gointernal/auditlog/init_tinygo.gointernal/auditlog/init_windows.gointernal/corazarules/rule_match.gointernal/corazawaf/rule.gointernal/corazawaf/transaction.gointernal/corazawaf/transaction_test.gointernal/seclang/directives.go
|
|
||
| variable := md.Variable().Name() | ||
| if key := md.Key(); key != "" { | ||
| variable += ":" + key |
There was a problem hiding this comment.
use strings.Builder to improve performance
|
Shall we name it |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1638 +/- ##
==========================================
- Coverage 87.68% 87.59% -0.09%
==========================================
Files 178 179 +1
Lines 9148 9322 +174
==========================================
+ Hits 8021 8166 +145
- Misses 858 877 +19
- Partials 269 279 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
I agree that is a bit clearer. Thanks for the suggestion! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/corazawaf/transaction_test.go (1)
942-944: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
deferto ensure transaction cleanup.If
t.Fatalfis called earlier in the test, this cleanup code will be skipped, potentially leaking the transaction in the WAF's transaction pool. As per coding guidelines, you should always clean up resources after transaction completion. Consider moving this cleanup logic to adeferblock immediately after the transaction is instantiated.🛠️ Proposed refactor
Apply this change near line 915:
tx := NewWAF().NewTransaction() + defer func() { + if err := tx.Close(); err != nil { + t.Errorf("Failed to close transaction: %s", err.Error()) + } + }() tx.AuditLogParts = types.AuditLogParts("H")And remove these lines at the end of the test:
- if err := tx.Close(); err != nil { - t.Fatalf("Failed to close transaction: %s", err.Error()) - } }🤖 Prompt for 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. In `@internal/corazawaf/transaction_test.go` around lines 942 - 944, Move the tx.Close cleanup from the test’s final assertions into a defer immediately after the transaction is instantiated, preserving fatal error handling in the deferred cleanup so the transaction is always released even when earlier t.Fatalf calls exit the test.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@internal/corazawaf/transaction_test.go`:
- Around line 942-944: Move the tx.Close cleanup from the test’s final
assertions into a defer immediately after the transaction is instantiated,
preserving fatal error handling in the deferred cleanup so the transaction is
always released even when earlier t.Fatalf calls exit the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c078efbc-0bfc-4011-b521-582d5c6c14e9
📒 Files selected for processing (9)
internal/auditlog/formats.gointernal/auditlog/formats_json_test.gointernal/auditlog/init.gointernal/auditlog/init_tinygo.gointernal/auditlog/init_windows.gointernal/corazawaf/rule.gointernal/corazawaf/transaction.gointernal/corazawaf/transaction_test.gointernal/seclang/directives.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/auditlog/init_windows.go
- internal/seclang/directives.go
- internal/corazawaf/rule.go
- internal/corazawaf/transaction.go
Add a new JSONV3 audit formatter that emits the libmodsecurity v3 JSON schema without changing the existing JSON and JSONLEGACY formats. Also make H-only audit logs retain structured rule message data so the new formatter can include producer and message details expected by libmodsecurity-compatible consumers.
9228113 to
3b6ef69
Compare
|
I squashed the commit down, let me know if further changes are needed! |
2 participants
Closes: #856
I added a new
JSONV3audit log formatter that emits the libmodsecurity v3 JSON schema and kept the existingJSONandJSONLEGACYformatters unchanged. TheSecAuditLogFomrat JSONV3is registered and H-only audit logs retain structured match-rule data somessagescan be emitted in the v3 format. Best-effortmatchdetails are populated for rule messages.I am new to this codebase, so let me know what I can improve. Hope this helps!
Summary by CodeRabbit
JSONV3(libModSecurity v3) audit log format, registered asmodsecurityv3.SecAuditLogFormatdirective docs to includeJSONV3.