From 5a0bf703e7e2abb12cd11f270a6a4f79a4e2845b Mon Sep 17 00:00:00 2001 From: Nguyen Manh <0xmanhnv@gmail.com> Date: Mon, 3 Aug 2026 15:59:24 +0000 Subject: [PATCH] fix(notifications): stop the severity filter from eating approval events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three approval_* notifications are undeliverable on a default configuration, so a finding awaiting a decision blocks forever and nobody is told. Two independent gates run before a notification is delivered: ShouldNotifyEventType empty enabled_event_types -> the platform defaults ShouldNotify empty enabled_severities -> critical + high only DefaultEnabledEventTypes deliberately turns EventTypeApprovalRequested ON. The reasoning is recorded next to it: "An approval request is addressed to a human; if it reaches nobody the finding stays blocked indefinitely." The enqueue site then stamps a constant severity — "medium" for requested and rejected, "low" for approved (finding/vulnerability_service.go:2355, :2458, :2542). Neither is in the default critical+high set, so the severity gate drops every one of them. The event-type gate was opened on purpose and the severity gate closed it again, producing exactly the outcome that comment exists to prevent. The root cause is that EnqueueParams.Severity carries two different things. For new_finding, sla_breach and friends it IS the finding's severity, and an operator who leaves the filter at its default is saying "only critical and high findings" — honoring that is the whole point. For approval lifecycle events it is a constant chosen by the enqueue site that describes no finding at all, and no operator ever asked to suppress it. So the fix is not to bump the constants to "high", which would just launder a workflow event through a field that does not apply to it. SeverityFilterApplies decides per event type whether the severity gate is meaningful, and the approval trio is exempt. They remain fully controllable through the event-type filter, which is the switch that actually means "I do not want these". SendNotificationInput gains EventType so the single-integration path makes the same decision as the broadcast path. Empty is treated as filterable, so every existing caller keeps its current behavior. Adds a completeness gate: a new event type must be classified as severity-bearing or not, and the build fails if one is added without that decision. Verified to fire by removing a classification — it names the offending event type. Inheriting the default silently is how this bug happened. --- internal/app/integration/service.go | 22 ++- .../integration/notification_extension.go | 33 ++++ .../integration/severity_filter_test.go | 145 ++++++++++++++++++ 3 files changed, 194 insertions(+), 6 deletions(-) create mode 100644 pkg/domain/integration/severity_filter_test.go diff --git a/internal/app/integration/service.go b/internal/app/integration/service.go index 0557f90c..b72c3a72 100644 --- a/internal/app/integration/service.go +++ b/internal/app/integration/service.go @@ -2020,8 +2020,12 @@ type SendNotificationInput struct { Title string Body string Severity string // critical, high, medium, low - URL string - Fields map[string]string + // EventType decides whether Severity is filterable at all. Empty means + // "unknown", and SeverityFilterApplies treats unknown as filterable — the + // pre-existing behavior for every caller that does not set it. + EventType string + URL string + Fields map[string]string } // SendNotificationResult represents the result of sending a notification. @@ -2072,8 +2076,10 @@ func (s *IntegrationService) SendNotification(ctx context.Context, input SendNot notifExt, _ = s.notificationExtRepo.GetByIntegrationID(ctx, intgID) } - // Check if we should notify for this severity - if notifExt != nil && !notifExt.ShouldNotify(input.Severity) { + // Check if we should notify for this severity. Skipped for event types whose + // Severity is not a finding severity — see integrationdom.SeverityFilterApplies. + if notifExt != nil && integrationdom.SeverityFilterApplies(integrationdom.EventType(input.EventType)) && + !notifExt.ShouldNotify(input.Severity) { return &SendNotificationResult{ Success: false, Error: fmt.Sprintf("notifications disabled for severity: %s", input.Severity), @@ -2161,8 +2167,11 @@ func (s *IntegrationService) BroadcastNotification(ctx context.Context, input Br } if iwn.Notification != nil { - // Check if this integration should receive notifications for this severity - if !iwn.Notification.ShouldNotify(input.Severity) { + // Check if this integration should receive notifications for this + // severity. Not applied to event types whose Severity is a constant + // rather than a finding severity — see SeverityFilterApplies. + if integrationdom.SeverityFilterApplies(input.EventType) && + !iwn.Notification.ShouldNotify(input.Severity) { continue } @@ -2178,6 +2187,7 @@ func (s *IntegrationService) BroadcastNotification(ctx context.Context, input Br Title: input.Title, Body: input.Body, Severity: input.Severity, + EventType: string(input.EventType), URL: input.URL, Fields: input.Fields, }) diff --git a/pkg/domain/integration/notification_extension.go b/pkg/domain/integration/notification_extension.go index 0957bb61..880bf8ce 100644 --- a/pkg/domain/integration/notification_extension.go +++ b/pkg/domain/integration/notification_extension.go @@ -253,6 +253,39 @@ func DefaultEnabledEventTypes() []EventType { } } +// SeverityFilterApplies reports whether the per-integration severity filter is +// meaningful for this event type. +// +// EnqueueParams.Severity carries two different things depending on the event: +// +// - For finding-shaped events (new_finding, sla_breach, ...) it IS the +// finding's severity. An operator who leaves the filter at its default is +// saying "only tell me about critical and high findings", and honoring +// that is the whole point of the filter. +// +// - For approval lifecycle events it is a hardcoded constant chosen by the +// enqueue site ("medium" for requested/rejected, "low" for approved). It +// describes nothing about a finding, and no operator ever asked to +// suppress it. +// +// Running the second kind through a filter built for the first kind silently +// defeats it. That is not hypothetical: DefaultEnabledEventTypes deliberately +// includes EventTypeApprovalRequested, with the comment "if it reaches nobody +// the finding stays blocked indefinitely" — and then the severity gate dropped +// it anyway, because "medium" is not in the default critical+high set. The +// event-type gate was opened on purpose and the severity gate closed it again. +// +// Events exempted here remain fully controllable through the event-type filter, +// which is the switch that actually means "I do not want these". +func SeverityFilterApplies(eventType EventType) bool { + switch MapLegacyEventType(eventType) { + case EventTypeApprovalRequested, EventTypeApprovalApproved, EventTypeApprovalRejected: + return false + default: + return true + } +} + // AllKnownEventTypes returns all known event types (for backward compatibility API). func AllKnownEventTypes() []EventType { types := make([]EventType, 0, len(AllEventTypes())) diff --git a/pkg/domain/integration/severity_filter_test.go b/pkg/domain/integration/severity_filter_test.go new file mode 100644 index 00000000..5cb56e3c --- /dev/null +++ b/pkg/domain/integration/severity_filter_test.go @@ -0,0 +1,145 @@ +package integration + +import "testing" + +// The severity filter was silently defeating the event-type filter. +// +// DefaultEnabledEventTypes deliberately turns EventTypeApprovalRequested ON, +// with the reasoning recorded next to it: "An approval request is addressed to +// a human; if it reaches nobody the finding stays blocked indefinitely." +// +// The enqueue site stamps Severity: "medium" (vulnerability_service.go), and +// IsSeverityEnabled treats an empty enabled_severities list as critical+high. +// So the event-type gate was opened on purpose and the severity gate closed it +// again — the exact outcome that comment exists to prevent. +// +// These tests pin the two halves together so the same defeat cannot reappear. + +func TestSeverityFilter_DoesNotApplyToApprovalEvents(t *testing.T) { + approvals := []EventType{ + EventTypeApprovalRequested, + EventTypeApprovalApproved, + EventTypeApprovalRejected, + } + + for _, et := range approvals { + t.Run(string(et), func(t *testing.T) { + if SeverityFilterApplies(et) { + t.Fatalf("%s is severity-filtered, but its Severity is a constant "+ + "chosen by the enqueue site, not a finding severity", et) + } + }) + } +} + +// The filter must keep working for the events it was built for — a fix that +// delivers everything is not a fix. +func TestSeverityFilter_StillAppliesToFindingEvents(t *testing.T) { + findingShaped := []EventType{ + EventTypeNewFinding, + EventTypeNewExposure, + EventTypeFindingAssigned, + EventTypeFindingPriorityEscalated, + EventTypeSLABreach, + } + + for _, et := range findingShaped { + t.Run(string(et), func(t *testing.T) { + if !SeverityFilterApplies(et) { + t.Fatalf("%s stopped being severity-filtered: an operator who asked "+ + "for critical+high only would start receiving everything", et) + } + }) + } +} + +// The end-to-end statement of the bug: approval_requested is default-ON at the +// event-type gate, and must survive the severity gate on a default (empty) +// configuration. +func TestApprovalRequested_SurvivesBothGatesOnDefaults(t *testing.T) { + ext := &NotificationExtension{} // empty config = platform defaults + + var defaultOn bool + for _, et := range DefaultEnabledEventTypes() { + if et == EventTypeApprovalRequested { + defaultOn = true + break + } + } + if !defaultOn { + t.Fatal("EventTypeApprovalRequested is no longer default-on; if that was " + + "deliberate, this test should be deleted along with the reasoning " + + "recorded in DefaultEnabledEventTypes") + } + + if !ext.ShouldNotifyEventType(EventTypeApprovalRequested) { + t.Fatal("blocked by the event-type gate") + } + + // This is the half that was broken: "medium" is not in the default + // critical+high set, so the severity gate dropped it. + const enqueuedSeverity = "medium" // vulnerability_service.go RequestApproval + if ext.ShouldNotify(enqueuedSeverity) { + t.Fatal("test is not exercising the bug: the default severity set now " + + "includes medium, so this would pass without the fix") + } + if SeverityFilterApplies(EventTypeApprovalRequested) { + t.Fatal("approval_requested is still severity-filtered, so it is still " + + "dropped on a default configuration and the finding stays blocked") + } +} + +// Completeness gate. A new event type must be a deliberate decision about +// whether its Severity is a real severity, not a default inherited by whoever +// adds the constant. This fails the build on any unclassified addition. +func TestSeverityFilter_EveryEventTypeIsClassified(t *testing.T) { + // Severity is a constant at the enqueue site, so filtering it is meaningless. + notFilterable := map[EventType]bool{ + EventTypeApprovalRequested: true, + EventTypeApprovalApproved: true, + EventTypeApprovalRejected: true, + } + + // Severity describes a finding/exposure, so the operator's filter is real. + filterable := map[EventType]bool{ + EventTypeSecurityAlert: true, + EventTypeSystemError: true, + EventTypeNewAsset: true, + EventTypeAssetChanged: true, + EventTypeAssetDeleted: true, + EventTypeScanStarted: true, + EventTypeScanCompleted: true, + EventTypeScanFailed: true, + EventTypeNewFinding: true, + EventTypeFindingConfirmed: true, + EventTypeFindingTriaged: true, + EventTypeFindingFixed: true, + EventTypeFindingReopened: true, + EventTypeFindingPriorityEscalated: true, + EventTypeFindingAssigned: true, + EventTypeSLABreach: true, + EventTypeWorkflowNotification: true, + EventTypeNewExposure: true, + EventTypeExposureResolved: true, + } + + for _, info := range AllEventTypes() { + et := info.Type + switch { + case notFilterable[et]: + if SeverityFilterApplies(et) { + t.Errorf("%s is listed as not-filterable here but SeverityFilterApplies says otherwise", et) + } + case filterable[et]: + if !SeverityFilterApplies(et) { + t.Errorf("%s is listed as filterable here but SeverityFilterApplies says otherwise", et) + } + default: + t.Errorf("event type %q is not classified. Decide whether its "+ + "EnqueueParams.Severity is a real finding severity (add it to "+ + "`filterable`) or a constant picked by the enqueue site (add it to "+ + "`notFilterable` AND to SeverityFilterApplies). Inheriting the "+ + "default silently is how approval_requested became undeliverable.", et) + } + } +}