Improve security alerts notifications

database/migrations/functions/packages/update_snapshot_security_report.sql

@@ -1,25 +1,26 @@
 -- update_snapshot_security_report updates the security report of the package's
 -- snapshot provides.
-create or replace function update_snapshot_security_report(p_report jsonb)
+create or replace function update_snapshot_security_report(
+    p_report jsonb,
+    p_emit_alert boolean
+)
 returns void as $$
 declare
     v_package_id uuid := (p_report->>'package_id')::uuid;
     v_version text := p_report->>'version';
     v_alert_digest text := nullif(p_report->>'alert_digest', '');
-    v_previous_alert_digest text;
 begin
-    -- Register security alert event for the associated package if the package's
-    -- version is the latest and the security report's alert digest has changed
-    select security_report_alert_digest
-    from snapshot s
-    join package p using (package_id)
-    where package_id = v_package_id
-    and s.version = v_version
-    and s.version = p.latest_version
-    into v_previous_alert_digest;
-    if found then
-        if v_alert_digest is not null
-        and (v_previous_alert_digest is null or v_alert_digest <> v_previous_alert_digest) then
+    -- Register a security alert event when the caller indicates it should be
+    -- emitted and the scanned version is still the latest
+    if p_emit_alert and v_alert_digest is not null then
+        if exists (
+            select 1
+            from snapshot s
+            join package p using (package_id)
+            where package_id = v_package_id
+            and s.version = v_version
+            and s.version = p.latest_version
+        ) then
             insert into event (package_id, package_version, event_kind_id)
             values (v_package_id, v_version, 1);
         end if;

database/migrations/schema/064_drop_old_update_snapshot_security_report.sql

@@ -0,0 +1,5 @@
+drop function if exists update_snapshot_security_report(jsonb);
+
+---- create above / drop below ----
+
+-- Nothing to do

database/tests/functions/packages/update_snapshot_security_report.sql

@@ -81,7 +81,7 @@ select update_snapshot_security_report('{
             {"k": "v"}
         ]
     }
-}');
+}', false);
 select is(security_report, '{
     "quay.io/org/pkg1:1.0.0": [
             {"k": "v"}
@@ -104,7 +104,7 @@ select update_snapshot_security_report('{
     "package_id": "00000000-0000-0000-0000-000000000002",
     "version": "0.0.9",
     "alert_digest": "digest-a"
-}');
+}', true);
 select is(
     count(*)::int,
     0::int,
@@ -117,11 +117,11 @@ where p.name = 'package2' and e.package_version = '0.0.9';
 select update_snapshot_security_report('{
     "package_id": "00000000-0000-0000-0000-000000000002",
     "version": "1.0.0"
-}');
+}', false);
 select is(
     count(*)::int,
     0::int,
-    'No security alert event should exist for package 2 version 1.0.0 as the alert digest is null'
+    'No security alert event should exist for package 2 version 1.0.0 when emit alert is false'
 )
 from event e
 join package p using (package_id)
@@ -131,7 +131,7 @@ select update_snapshot_security_report('{
     "package_id": "00000000-0000-0000-0000-000000000002",
     "version": "1.0.0",
     "alert_digest": "digest-b"
-}');
+}', true);
 select is(
     count(*)::int,
     1::int,
@@ -145,11 +145,11 @@ select update_snapshot_security_report('{
     "package_id": "00000000-0000-0000-0000-000000000002",
     "version": "1.0.0",
     "alert_digest": "digest-b"
-}');
+}', false);
 select is(
     count(*)::int,
     1::int,
-    'No new security alert event should exist for package 2 version 1.0.0 as the alert digest has not changed'
+    'No new security alert event should exist for package 2 version 1.0.0 when emit alert is false'
 )
 from event e
 join package p using (package_id)
@@ -168,7 +168,7 @@ select update_snapshot_security_report('{
     "package_id": "00000000-0000-0000-0000-000000000002",
     "version": "1.1.0",
     "alert_digest": "digest-b"
-}');
+}', true);
 select is(
     count(*)::int,
     1::int,
@@ -182,7 +182,7 @@ select update_snapshot_security_report('{
     "package_id": "00000000-0000-0000-0000-000000000002",
     "version": "1.1.0",
     "alert_digest": "digest-c"
-}');
+}', true);
 select is(
     count(*)::int,
     2::int,

internal/notification/template/security_alert_email.tmpl

@@ -46,7 +46,7 @@
                       <p class="text-muted" style="font-size: 11px; text-decoration: none; Margin-bottom: 30px;">Or you can copy-paste this link: <span class="copy-link">{{ .Package.URL }}?modal=security-report&event-id={{ .Event.ID }}</span></p>
 
                       <p style="font-family: sans-serif; font-size: 14px; font-weight: normal; margin: 0; Margin-bottom: 30px; text-align: left;">
-                        Please note that security alerts only consider vulnerabilities of <b>high</b> and <b>critical</b> severity. Any time a new potential security vulnerability is detected you'll be notified again.
+                        Please note that security alerts only consider vulnerabilities of <b>high</b> and <b>critical</b> severity. You'll be notified again if a new potential security vulnerability is detected or if an existing one is upgraded from <b>high</b> to <b>critical</b>.
                       </p>
                     </td>
                   </tr>

internal/pkg/manager.go

@@ -3,6 +3,7 @@ package pkg
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/url"
 	"sort"
@@ -11,7 +12,10 @@ import (
 
 	"github.com/Masterminds/semver/v3"
 	"github.com/artifacthub/hub/internal/hub"
+	"github.com/artifacthub/hub/internal/scanner"
 	"github.com/artifacthub/hub/internal/util"
+	"github.com/jackc/pgx/v4"
+	"github.com/rs/zerolog/log"
 	"github.com/satori/uuid"
 	stripmd "github.com/writeas/go-strip-markdown"
 )
@@ -32,14 +36,15 @@ const (
 	getPkgsStatsDBQ                 = `select get_packages_stats()`
 	getProductionUsageDBQ           = `select get_production_usage($1::uuid, $2::text, $3::text)`
 	getSnapshotSecurityReportDBQ    = `select security_report from snapshot where package_id = $1 and version = $2`
+	getSnapshotSecurityReportTxDBQ  = `select security_report from snapshot where package_id = $1 and version = $2 for update`
 	getSnapshotsToScanDBQ           = `select get_snapshots_to_scan()`
 	getRandomPkgsDBQ                = `select get_random_packages()`
 	getValuesSchemaDBQ              = `select values_schema from snapshot where package_id = $1 and version = $2`
 	registerPkgDBQ                  = `select register_package($1::jsonb)`
 	searchPkgsDBQ                   = `select * from search_packages($1::jsonb)`
 	searchPkgsMonocularDBQ          = `select search_packages_monocular($1::text, $2::text)`
 	togglePkgStarDBQ                = `select toggle_star($1::uuid, $2::uuid)`
-	updateSnapshotSecurityReportDBQ = `select update_snapshot_security_report($1::jsonb)`
+	updateSnapshotSecurityReportDBQ = `select update_snapshot_security_report($1::jsonb, $2::boolean)`
 	unregisterPkgDBQ                = `select unregister_package($1::jsonb)`
 )
 
@@ -385,10 +390,58 @@ func (m *Manager) UpdateSnapshotSecurityReport(ctx context.Context, r *hub.Snaps
 		return fmt.Errorf("%w: %s", hub.ErrInvalidInput, "version not provided")
 	}
 
-	// Update snapshot security report in database
-	rJSON, _ := json.Marshal(r)
-	_, err := m.db.Exec(ctx, updateSnapshotSecurityReportDBQ, rJSON)
-	return err
+	return util.DBTransact(ctx, m.db, func(tx pgx.Tx) error {
+		// Lock the snapshot row while computing the alert decision
+		previousReportJSON, err := getSnapshotSecurityReportJSONForUpdate(
+			ctx,
+			tx,
+			r.PackageID,
+			r.Version,
+		)
+		if err != nil && !errors.Is(err, hub.ErrNotFound) {
+			return err
+		}
+		if errors.Is(err, hub.ErrNotFound) {
+			previousReportJSON = nil
+		}
+
+		// Compare against the stored report to avoid noisy security alerts
+		emitAlert, err := scanner.ShouldNotifyOnNewOrEscalatedAlerts(
+			previousReportJSON,
+			r.ImagesReports,
+		)
+		if err != nil {
+			log.Error().
+				Err(err).
+				Str("package_id", r.PackageID).
+				Str("version", r.Version).
+				Msg("error processing previous security report")
+			emitAlert = false
+		}
+
+		// Update snapshot security report in database
+		rJSON, _ := json.Marshal(r)
+		_, err = tx.Exec(ctx, updateSnapshotSecurityReportDBQ, rJSON, emitAlert)
+		return err
+	})
+}
+
+// getSnapshotSecurityReportJSONForUpdate returns the stored security report for
+// the requested snapshot while holding a row lock for the current transaction.
+func getSnapshotSecurityReportJSONForUpdate(
+	ctx context.Context,
+	tx pgx.Tx,
+	pkgID, version string,
+) ([]byte, error) {
+	// Lock the snapshot row so concurrent workers observe the latest report
+	var dataJSON []byte
+	if err := tx.QueryRow(ctx, getSnapshotSecurityReportTxDBQ, pkgID, version).Scan(&dataJSON); err != nil {
+		if errors.Is(err, pgx.ErrNoRows) {
+			return nil, hub.ErrNotFound
+		}
+		return nil, err
+	}
+	return dataJSON, nil
 }
 
 // Unregister unregisters the package provided from the database.