Skip to content

fix(plugins/rule-flood-guard): scope flood threshold per rule and data source on v11#2370

Open
yllada wants to merge 4 commits into
v11from
backlog/rule-flood-guard-v11-per-datasource-threshold
Open

fix(plugins/rule-flood-guard): scope flood threshold per rule and data source on v11#2370
yllada wants to merge 4 commits into
v11from
backlog/rule-flood-guard-v11-per-datasource-threshold

Conversation

@yllada

@yllada yllada commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

What

Changes how rule-flood-guard counts alerts before deciding to auto-disable a rule: the threshold now applies per (rule, data source) pair instead of per rule globally. Also raises the notification message length limit and adds a couple of small reliability/usability improvements found while implementing this.

Why

The original counting logic grouped alerts only by rule name. This meant a rule firing legitimately once each across many different data sources/assets (e.g. the same detection catching a real, spreading incident on many endpoints) could cross the threshold and get auto-disabled — exactly the wrong moment to lose that detection. Grouping by rule and data source correctly distinguishes "the same signal repeating without dedup on one source" (real fatigue) from "the same rule legitimately firing across many distinct sources" (not fatigue).

Changes

Commit What
fix(backend/notification) Raised NotificationDTO.message's length limit from 280 to 500 characters (DB column is TEXT, unbounded — verified safe) so the notification can name both the rule and the data source without truncation.
feat(plugins/rule-flood-guard) — threshold Nested a dataSource.keyword sub-aggregation inside the existing rule-name aggregation; the threshold now applies per (rule, data source) pair. The notification message now names the specific data source that tripped it. Also fixes a bug where a rule that WAS successfully disabled could silently lose its notification if another rule failed in the same name-collision batch.
feat(plugins/rule-flood-guard) — config Plugin now creates system_plugins_rule-flood-guard.yaml with default values on first run if it doesn't exist (never overwrites an existing file).
chore(deps) Routine Go dependency updates for plugins/aws, plugins/gcp, plugins/modules-config (unrelated to this feature, bundled since they were already pending).

@yllada
yllada requested a review from a team July 22, 2026 18:05
@github-actions

Copy link
Copy Markdown

🛑 AI review — Blocking issues

One or more high/critical issues can break things and must be fixed before merging. Details below.

⚠️ architecture (gemini-3-flash-lite) — non-blocking warnings

Summary: Updated flood guard logic to include data source in aggregation and notification; routine dependency updates.

  • medium plugins/rule-flood-guard/guard.go:41 — The loop continues even if Deactivate fails, potentially leading to multiple notification attempts for the same rule/data source pair. Consider breaking or tracking state.
  • low plugins/rule-flood-guard/config.go:105 — The use of os.Rename for atomic file creation is good, but ensure the directory permissions (0755) and file permissions (0600) align with the existing security policy for configuration files.

🛑 bugs (gemini-3-flash-lite) — blocking — must fix before merge

Summary: Logic error in guard.go: failed deactivation now triggers a notification, potentially spamming users for rules that are already disabled or failing to deactivate.

  • high plugins/rule-flood-guard/guard.go:41 — The 'continue' statement was removed after the error check. If client.Deactivate fails, the code now proceeds to call client.Notify, which will send a notification for a rule that was not successfully disabled, leading to incorrect alerts and potential spam.

security (gemini-3-flash-lite) — clean

Summary: The PR updates dependencies and improves the rule-flood-guard plugin logic; no security vulnerabilities or information disclosures were introduced.

No findings.

@utmstackprapprover utmstackprapprover Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Changes requested — AI review found blocking issues (high/critical, or engineer review required). See above.

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.

1 participant