Alerts to a JSON webhook, with a cooldown and an hourly summary (closes #26)

SWWAF_ALERT_WEBHOOK_URL gets one JSON POST per alert, in SPEC.md's
schema, with SWWAF_ALERT_WEBHOOK_HEADERS: ban and permanent_ban, with
the ban's notes, in observe mode too, marked mode observe;
source_failure for GeoJS; file_error for a rule or state file with an
error. SWWAF_ALERT_EVENTS chooses; SWWAF_ALERT_COOLDOWN holds back
repeats by netblock, file or source; past SWWAF_ALERT_MAX_PER_HOUR the
hour ends in one summary. A bounded queue, retried with backoff, holds
up no request; a 4xx other than 408 and 429 gives the alert up.
alerts.json keeps the queue, the cooldowns and the hour. Nothing shows
the URL's path or query.

Judgement call: the summary's event is summary, which SPEC.md omits.
Judgement call: an admin's ban raises no alert.

Model: opus-5-5

README.md

@@ -551,10 +551,8 @@ The bans you make, in `bans.json` or through the ban endpoints, raise no alert.
 In `observe` mode, a request that would have made a ban, or made one permanent,
 raises the alert `enforce` mode would have raised, for the ban as it would have
 been, with `mode`, `observe`, in its `detail`: no ban was made, or made
-permanent. A request that would have made a ban whose alert would be held back,
+permanent. This is the alert for a ban for a broken rate limit, shown indented;
-by the cooldown or past `SWWAF_ALERT_MAX_PER_HOUR`, raises none, and is not
+it is sent on one line:
-counted. This is the alert for a ban for a broken rate limit, shown indented; it
-is sent on one line:
 
 ```json
 {

internal/alerts/alerts.go

@@ -259,31 +259,6 @@ func (q *Queue) Raise(alert Alert) {
 	q.queue(&alert)
 }
 
-// WouldSend reports whether Raise would let an alert for event on
-// netblock through now: a webhook is set, SWWAF_ALERT_EVENTS chooses
-// event, no alert for event on netblock was let through less than
-// Cooldown before, and fewer than MaxPerHour alerts have been let through
-// in the hour under way. Unlike Raise, it counts nothing.
-func (q *Queue) WouldSend(event string, netblock netip.Prefix) bool {
-	if q.params.WebhookURL == nil || !slices.Contains(q.params.Events, event) {
-		return false
-	}
-
-	q.mu.Lock()
-	defer q.mu.Unlock()
-
-	now := q.params.Now()
-
-	last, found := q.cooldowns[cooldownKey{event: event, netblock: netblock}]
-	if q.params.Cooldown > 0 && found && now.Sub(last.Sent) < q.params.Cooldown {
-		return false
-	}
-
-	q.endHour(now)
-
-	return q.params.MaxPerHour == 0 || q.hour.Sent < q.params.MaxPerHour
-}
-
 // Run sends the alerts waiting, oldest first, until ctx is done. An alert
 // stays in the queue until the webhook answers it with a 2xx status, or
 // refuses it with a 4xx status other than 408 and 429: a refused alert is

internal/bans/bans.go

@@ -290,27 +290,6 @@ func (l *Ledger) WouldBanForAttack(
 	return l.ban(netblock, now, CauseAttack, attackReason(notes), notes, false)
 }
 
-// WouldBePermanent reports whether a ban on netblock for cause, CauseLimit
-// or CauseAttack, made at now would be permanent, as BanForLimit or
-// BanForAttack would make it. It works out nothing else of the ban.
-func (l *Ledger) WouldBePermanent(
-	netblock netip.Prefix, now time.Time, cause string,
-) bool {
-	l.mu.Lock()
-	defer l.mu.Unlock()
-
-	var held []Ban
-	if bans, found := l.netblocks.Peek(netblock); found {
-		held = *bans
-	}
-
-	if cause == CauseAttack {
-		return l.attackExpiry(held, now).IsZero()
-	}
-
-	return l.limitExpiry(held, now).IsZero()
-}
-
 // limitReason is the reason of a ban for a broken limit, with notes.
 func limitReason(notes Notes) string {
 	return fmt.Sprintf("requests per %s over the limit of %d", notes.Window, notes.Limit)

internal/proxy/alerts_test.go

@@ -16,7 +16,6 @@ import (
 
 const (
 	alertWebhookURL = "SWWAF_ALERT_WEBHOOK_URL"
-	alertMaxPerHour = "SWWAF_ALERT_MAX_PER_HOUR"
 	// alertInstance is the instance every alert of these tests gives.
 	alertInstance = "fsn1app1/gitea"
 )
@@ -108,8 +107,8 @@ func TestObserveModeRaisesTheBanAlertsItWouldHave(t *testing.T) {
 	group := netip.MustParsePrefix(ipv6Group)
 	attackBan, _ := server.Ledger.BanForAttack(group, start, bans.Notes{RuleID: "probe"})
 
-	// The third request breaks the limit, and so does the fourth, within the
+	// The third request breaks the limit, and so does the fourth, a repeat
-	// cooldown, which raises nothing. The probe is a clear sign of attack.
+	// the cooldown holds back. The probe is a clear sign of attack.
 	for range 4 {
 		s.get(client, http.StatusOK, requestlog.ActionForward)
 	}
@@ -125,8 +124,8 @@ func TestObserveModeRaisesTheBanAlertsItWouldHave(t *testing.T) {
 	}
 
 	waiting := queue.Snapshot().Waiting
-	if len(waiting) != 3 || queue.Suppressed() != 0 {
+	if len(waiting) != 3 || queue.Suppressed() != 1 {
-		t.Fatalf("%d alerts wait and %d are held back, want 3 and 0: %+v",
+		t.Fatalf("%d alerts wait and %d are held back, want 3 and 1: %+v",
 			len(waiting), queue.Suppressed(), waiting)
 	}
 
@@ -158,43 +157,6 @@ func TestObserveModeRaisesTheBanAlertsItWouldHave(t *testing.T) {
 	wantAlerts(t, queue, want...)
 }
 
-func TestObserveModeWorksOutABanOnlyWhenItsAlertWouldBeSent(t *testing.T) {
-	t.Parallel()
-
-	s, _, _, queue := startWithAlerts(t, map[string]string{
-		mode:               observe,
-		rateLimitPerMinute: "2",
-		rulesDir:           writeRules(t, testRules),
-		alertMaxPerHour:    "2",
-	})
-
-	// The client's third request breaks the limit, and raises the first
-	// alert of the hour. Its fourth is within the cooldown.
-	for range 4 {
-		s.get(client, http.StatusOK, requestlog.ActionForward)
-	}
-
-	// The other client's first probe raises the second. Its second probe is
-	// within the cooldown.
-	for range 2 {
-		s.request(otherClient, "/.env", http.StatusOK, requestlog.ActionForward)
-	}
-
-	// The IPv6 client's third request breaks the limit past the two alerts
-	// an hour.
-	for range 3 {
-		s.get(ipv6Client, http.StatusOK, requestlog.ActionForward)
-	}
-
-	// Had the ban been worked out for any of the requests within the
-	// cooldown or past the two an hour, its alert would have been raised,
-	// held back and counted.
-	if waiting := queue.Snapshot().Waiting; len(waiting) != 2 || queue.Suppressed() != 0 {
-		t.Errorf("%d alerts wait and %d are held back, want 2 and 0: %+v",
-			len(waiting), queue.Suppressed(), waiting)
-	}
-}
-
 // startWithAlerts is startWithClock with alerts to a webhook, which is
 // never sent them, and returns the queue they wait in as well.
 func startWithAlerts(

internal/proxy/bans.go

@@ -44,7 +44,7 @@ func (rq *request) banned(now time.Time) bool {
 // the client over a limit. In enforce mode such a request bans the
 // client's netblock, and sets the client's counters back to zero; in
 // observe mode it does neither, and raises the alert for the ban it would
-// have made, if that alert would be sent.
+// have made.
 func (rq *request) limitBroken(now time.Time) bool {
 	group := clientGroup(rq.client)
 
@@ -59,10 +59,6 @@ func (rq *request) limitBroken(now time.Time) bool {
 	rq.line.Offence = requestlog.OffenceLimit
 
 	netblock := rq.h.netblock(rq.client)
-	if rq.h.config.Observe && !rq.wouldAlertBan(netblock, now, bans.CauseLimit) {
-		return true
-	}
-
 	notes := bans.Notes{
 		Country:  rq.line.Country,
 		Limit:    hit.Limit,
@@ -94,14 +90,9 @@ func (rq *request) limitBroken(now time.Time) bool {
 
 // banForAttack bans the client's netblock at now for a clear sign of
 // attack, the match of rule, a ban rule. In observe mode it makes no ban,
-// and raises the alert for the ban it would have made, if that alert
+// and raises the alert for the ban it would have made.
-// would be sent.
 func (rq *request) banForAttack(now time.Time, rule rules.Rule) {
 	netblock := rq.h.netblock(rq.client)
-	if rq.h.config.Observe && !rq.wouldAlertBan(netblock, now, bans.CauseAttack) {
-		return
-	}
-
 	notes := bans.Notes{
 		Country:  rq.line.Country,
 		RuleID:   rule.ID,
@@ -127,22 +118,6 @@ func (rq *request) banForAttack(now time.Time, rule rules.Rule) {
 	}
 }
 
-// wouldAlertBan reports whether the alert for a ban on netblock for cause
-// made at now would be sent. In observe mode the ban the request would
-// have made is worked out only then, at most once per
-// SWWAF_ALERT_COOLDOWN and never with no webhook set: its notes count the
-// netblock's requests, which can mean going through every client.
-func (rq *request) wouldAlertBan(
-	netblock netip.Prefix, now time.Time, cause string,
-) bool {
-	event := alerts.EventBan
-	if rq.h.ledger.WouldBePermanent(netblock, now, cause) {
-		event = alerts.EventPermanentBan
-	}
-
-	return rq.h.alerts.WouldSend(event, netblock)
-}
-
 // alertBan raises the alert for ban, which the request made, or made
 // permanent: permanent_ban for a permanent ban, ban for another. Its
 // detail gives the ban's cause, when it ends, and its notes, and in

internal/state/state_test.go

@@ -210,37 +210,6 @@ func TestAlertsJSONIsIndentedWithTheCooldownsTheHourAndTheAlertsWaiting(t *testi
 	}
 }
 
-func TestSourceFailureCooldownKeptInAlertsJSONAcrossARestart(t *testing.T) {
-	t.Parallel()
-
-	dir := t.TempDir()
-	before := newParams(dir)
-	files := load(t, before)
-
-	failure := alerts.Alert{
-		Event: alerts.EventSourceFailure, Reason: "asking GeoJS failed",
-		Detail: map[string]any{"source": "geojs"},
-	}
-	before.Alerts.Raise(failure)
-
-	err := files.WriteAll()
-	if err != nil {
-		t.Fatalf("write: %v", err)
-	}
-
-	// After the restart, the cooldown read back holds back a repeat for the
-	// same source.
-	after := newParams(dir)
-	load(t, after)
-	after.Alerts.Raise(failure)
-
-	waiting := after.Alerts.Snapshot().Waiting
-	if len(waiting) != 1 || after.Alerts.Suppressed() != 1 {
-		t.Errorf("%d alerts wait and %d are held back, want the one read back and 1",
-			len(waiting), after.Alerts.Suppressed())
-	}
-}
-
 func TestBansJSONIsIndentedWithNullForAPermanentBan(t *testing.T) {
 	t.Parallel()
 
@@ -614,12 +583,11 @@ func TestWriteThatFailsWhileRunningRaisesAFileErrorAlertOncePerCooldown(t *testi
 		params.CounterInterval = time.Minute
 		run(t, load(t, params).Run)
 
-		// A directory in the way of clients.json's temporary file fails each
+		// A directory in the way of bans.json's temporary file fails each of
-		// of its writes, while the other files are written. It holds a file,
+		// its writes. It holds a file, so that the write cannot remove it.
-		// so that the write cannot remove it.
+		err := os.Mkdir(filepath.Join(dir, bansJSON+".tmp"), 0o700)
-		err := os.Mkdir(filepath.Join(dir, clientsJSON+".tmp"), 0o700)
 		if err == nil {
-			err = os.WriteFile(filepath.Join(dir, clientsJSON+".tmp", "kept"), nil, 0o600)
+			err = os.WriteFile(filepath.Join(dir, bansJSON+".tmp", "kept"), nil, 0o600)
 		}
 
 		if err != nil {
@@ -638,10 +606,10 @@ func TestWriteThatFailsWhileRunningRaisesAFileErrorAlertOncePerCooldown(t *testi
 
 		if waiting[0].Event != alerts.EventFileError ||
 			waiting[0].Reason != "writing the state files failed" ||
-			waiting[0].Detail["file"] != filepath.Join(dir, clientsJSON) ||
+			waiting[0].Detail["file"] != filepath.Join(dir, bansJSON) ||
-			!strings.Contains(message, clientsJSON+".tmp") {
+			!strings.Contains(message, bansJSON+".tmp") {
-			t.Fatalf("alerts waiting %+v, want a file_error alert for clients.json, "+
+			t.Fatalf("alerts waiting %+v, want a file_error alert for bans.json, naming "+
-				"naming its temporary file", waiting)
+				"its temporary file", waiting)
 		}
 
 		// The next write fails too, within the cooldown, which holds it back.