feat(operators): implement Snort data syntax support for @pm#1604
@pm#1604Conversation
@pm
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1604 +/- ##
==========================================
+ Coverage 87.19% 87.27% +0.08%
==========================================
Files 178 178
Lines 8845 8890 +45
==========================================
+ Hits 7712 7759 +47
+ Misses 867 866 -1
+ Partials 266 265 -1
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:
|
|
@copilot Add full coverage tests for your changes. |
Added full coverage tests in
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughImplements Snort-style pipe-delimited hex syntax parsing in the Changes
Possibly related issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/operators/pm_test.go (1)
186-198: Consider closing transactions to avoid resource leaks in tests.Transactions should be closed after use to release pooled resources. While this is less critical in tests, it's good practice to ensure proper cleanup.
♻️ Suggested fix
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { op, err := newPM(plugintypes.OperatorOptions{Arguments: tt.param}) if err != nil { t.Fatalf("newPM(%q) unexpected error: %v", tt.param, err) } tx := waf.NewTransaction() + defer tx.Close() got := op.Evaluate(tx, tt.input) if got != tt.wantMatch { t.Errorf("pm(%q).Evaluate(%q) = %v, want %v", tt.param, tt.input, got, tt.wantMatch) } }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/operators/pm_test.go` around lines 186 - 198, The test creates transactions via waf.NewTransaction() (tx) but never closes them, risking pooled resource leaks; after creating tx in the t.Run closure (where newPM and op.Evaluate are used), ensure you call tx.Close() to release resources—preferably immediately after tx creation use defer tx.Close() so the transaction is closed when the subtest exits; update the test loop around newPM/tx/Evaluate to defer tx.Close() (or call tx.Close() explicitly before t.Run returns) to guarantee cleanup.
🤖 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/operators/pm_test.go`:
- Around line 186-198: The test creates transactions via waf.NewTransaction()
(tx) but never closes them, risking pooled resource leaks; after creating tx in
the t.Run closure (where newPM and op.Evaluate are used), ensure you call
tx.Close() to release resources—preferably immediately after tx creation use
defer tx.Close() so the transaction is closed when the subtest exits; update the
test loop around newPM/tx/Evaluate to defer tx.Close() (or call tx.Close()
explicitly before t.Run returns) to guarantee cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ade51cdd-66da-481e-8430-2a1fc7e823b1
📒 Files selected for processing (3)
internal/operators/pm.gointernal/operators/pm_test.gointernal/operators/testdata/pm.json
Agent-Logs-Url: https://github.com/corazawaf/coraza/sessions/2ebb487b-5f51-4c42-b857-602201c80801 Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com>
…back Agent-Logs-Url: https://github.com/corazawaf/coraza/sessions/2ebb487b-5f51-4c42-b857-602201c80801 Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com>
Agent-Logs-Url: https://github.com/corazawaf/coraza/sessions/bf792533-4ef4-4be3-8c71-38d5647544f9 Co-authored-by: fzipi <3012076+fzipi@users.noreply.github.com>
fzipi
force-pushed
the
copilot/add-support-short-syntax-pm-operator
branch
from
4973756 to
842d22a
Compare
|
I don't feel ok adding a custom parser and there are no reported use cases for this, I would stale this until there is a valid case. |
The
@pmoperator silently ignored Snort-style hex notation (|42|), treating pipe characters as literals. This implements the long-standing TODO to properly parse@pm A|42|C|44|F-style arguments.Changes
internal/operators/pm.gostrings.SplitwithparsePMArgs()— a byte-level parser that handles Snort data syntax:|42|→ single byte (0x42)|0d 0a|→ multiple space-separated bytes (CRLF)parseHexByte()helperinternal/operators/testdata/pm.json— 5 new JSON test cases for snort syntaxinternal/operators/pm_test.go(new) — unit tests forparsePMArgs(including error paths) and integration tests for end-to-end evaluationExample
Summary by CodeRabbit