Skip to content

Fix audit RelevantOnly logs with DetectionOnly rule engine #1339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rse173
Copy link

@rse173 rse173 commented Mar 31, 2025

Hello, this is a patch to fix #1333 simply without touching at the interruption. The only downside is that we add a variable to the Transaction struct (If you have a better idea let me know).

For now it is enough for my purpose, we have RelevantOnly audit logs when we are in RuleEngineDetectionOnly mode.

The variable detectedInterruption is set to the tx.interruption when an interruption would have been triggered if we were in RuleEngineOn.

@rse173 rse173 requested a review from a team as a code owner March 31, 2025 17:43
Copy link

codecov bot commented Mar 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.00%. Comparing base (681afa9) to head (a8bb630).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1339   +/-   ##
=======================================
  Coverage   81.99%   82.00%           
=======================================
  Files         170      170           
  Lines        9803     9808    +5     
=======================================
+ Hits         8038     8043    +5     
  Misses       1518     1518           
  Partials      247      247           
Flag Coverage Δ
coraza.rule.case_sensitive_args_keys 81.96% <100.00%> (+<0.01%) ⬆️
coraza.rule.multiphase_valuation 82.00% <100.00%> (+<0.01%) ⬆️
coraza.rule.no_regex_multiline 81.94% <100.00%> (+<0.01%) ⬆️
default 82.00% <100.00%> (+<0.01%) ⬆️
examples+ 16.49% <0.00%> (-0.01%) ⬇️
examples+coraza.rule.case_sensitive_args_keys 81.96% <100.00%> (+<0.01%) ⬆️
examples+coraza.rule.multiphase_valuation 81.84% <100.00%> (+<0.01%) ⬆️
examples+coraza.rule.no_regex_multiline 81.86% <100.00%> (+<0.01%) ⬆️
examples+memoize_builders 81.96% <100.00%> (+<0.01%) ⬆️
examples+no_fs_access 81.29% <100.00%> (+<0.01%) ⬆️
ftw 82.00% <100.00%> (+<0.01%) ⬆️
memoize_builders 82.09% <100.00%> (+<0.01%) ⬆️
no_fs_access 81.45% <100.00%> (+<0.01%) ⬆️
tinygo 81.97% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jcchavezs
Copy link
Member

LGTM, any input @M4tteoP ?

} else if a.Function.Type() == plugintypes.ActionTypeDisruptive && tx.RuleEngine == types.RuleEngineOn {
} else if a.Function.Type() == plugintypes.ActionTypeDisruptive && tx.RuleEngine != types.RuleEngineOff {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
I think this is interesting, but something we should be quite careful with. It tweaks the core definition of DetectionOnly, which currently means that rules are processed but disruptive actions are never executed.
If I'm getting this right, with this PR, the logic becomes: disruptive actions are executed, but no interruption is triggered.

I see value in this change, it closes the gap between how the system behaves in DetectionOnly/On mode ( See #1051), but we should consider all the implications, as this changes the engine's behavior in DetectionOnly mode.

I'll take a more in-depth look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ineffective RelevantOnly audit engine in DetectionOnly mode
3 participants