A ban's earlier_bans counts a ban dropped after an admin's ban (closes #128)

earlierBans took the counts in the notes of the netblock's first ban
held, so a ban dropped after an admin's ban, which came first, went
uncounted. Each ban held now gives a count, its notes' counts plus one
for it and each ban held after it, and each cause takes the largest.
Counting from the newest ban alone would lose the bans before an
admin's ban added to bans.json without notes.

Model: opus-5-5

internal/bans/admin_test.go

@@ -90,30 +90,6 @@ func TestEarlierBansCountABanDroppedAfterAnAdminsBan(t *testing.T) {
 	}
 }
 
-func TestEarlierBansCountABanAnAdminKeepsOnce(t *testing.T) {
-	t.Parallel()
-
-	ledger := bans.New(defaultRules())
-	netblock := netip.MustParsePrefix("203.0.113.1/32")
-
-	// The netblock breaks a limit twice, the second time after its first
-	// ban has ended, and the second ban's notes count the first. An admin
-	// then keeps the first ban.
-	first, _ := ledger.BanForLimit(netblock, midnight(), bans.Notes{})
-	second, _ := ledger.BanForLimit(netblock, first.Expires, bans.Notes{})
-
-	kept := first
-	kept.Cause = bans.CauseAdmin
-	ledger.LoadEdit([]bans.Ban{kept, second})
-
-	// The netblock's next ban counts the kept ban once, as an admin's.
-	third, _ := ledger.BanForLimit(netblock, second.Expires, bans.Notes{})
-	if third.Notes.EarlierBans != (bans.EarlierBans{Limit: 1, Admin: 1}) {
-		t.Errorf("the third ban for a limit has earlier bans %+v, "+
-			"want 1 for a limit and 1 an admin's", third.Notes.EarlierBans)
-	}
-}
-
 func TestReasonOfTheBansSmallwebwafMakes(t *testing.T) {
 	t.Parallel()
 

internal/bans/bans.go

@@ -691,10 +691,7 @@ func (l *Ledger) ban(
 // notes count before it, and one for it and each ban held after it. A
 // ban's count misses a ban dropped after it to make room, and that of an
 // admin's ban added to bans.json without notes misses the bans before it,
-// so the count that names the most bans is taken, whole: a ban an admin
-// keeps by giving it CauseAdmin is still counted under its old cause in
-// later bans' notes, and taking each cause from another count would count
-// it twice.
+// so each cause gets the largest of these counts.
 func earlierBans(held []Ban) EarlierBans {
 	// fromHere counts ban and the bans held after it.
 	var most, fromHere EarlierBans
@@ -712,28 +709,15 @@ func earlierBans(held []Ban) EarlierBans {
 		}
 
 		notes := ban.Notes.EarlierBans
-		count := EarlierBans{
-			Limit:    notes.Limit + fromHere.Limit,
-			Attack:   notes.Attack + fromHere.Attack,
-			Admin:    notes.Admin + fromHere.Admin,
-			CrowdSec: notes.CrowdSec + fromHere.CrowdSec,
-		}
-
-		// Of counts that name as many bans, the earlier ban's is taken: it
-		// counts more of the bans held by the cause they have now.
-		if count.total() >= most.total() {
-			most = count
-		}
+		most.Limit = max(most.Limit, notes.Limit+fromHere.Limit)
+		most.Attack = max(most.Attack, notes.Attack+fromHere.Attack)
+		most.Admin = max(most.Admin, notes.Admin+fromHere.Admin)
+		most.CrowdSec = max(most.CrowdSec, notes.CrowdSec+fromHere.CrowdSec)
 	}
 
 	return most
 }
 
-// total returns how many bans e counts, of all causes.
-func (e EarlierBans) total() int {
-	return e.Limit + e.Attack + e.Admin + e.CrowdSec
-}
-
 // markChanged has Changed receive a value, unless one is waiting already.
 func (l *Ledger) markChanged() {
 	select {

internal/proxy/request.go

@@ -795,19 +795,9 @@ func (rq *request) responseTimedOut() {
 	}
 }
 
-// responseReceived is called once the app has sent its whole answer. An
-// app may answer before the client has sent its whole body, as with a 404
-// it sends without reading it. The timeouts then run on: until the
-// transport's read of the body returns, Go's server cannot send the
-// answer, and SWWAF_CLIENT_REQUEST_TIMEOUT is what ends that read for a
-// client that stops sending.
+// responseReceived is called once the app has sent its whole answer.
 func (rq *request) responseReceived() {
 	rq.complete = true
-
-	if rq.body != nil && !rq.body.received.Load() {
-		return
-	}
-
 	rq.stopTimers()
 }
 

internal/proxy/timeouts_test.go

@@ -194,60 +194,6 @@ func sendLargeBody(t *testing.T, addr string) net.Conn {
 	return conn
 }
 
-func TestClientStopsSendingAfterTheAppHasAnswered(t *testing.T) {
-	t.Parallel()
-
-	// answeredAt is when the app answered, or nil if the bytes the client
-	// sent never reached it.
-	var answeredAt atomic.Pointer[time.Time]
-
-	// The app answers without reading the whole body, as an app does for
-	// a path it does not serve, and only once smallwebwaf waits for the
-	// rest of it: the app takes the bytes the client sent, and then waits
-	// half the timeout, far longer than smallwebwaf takes to go on to
-	// wait.
-	app := startApp(t, func(w http.ResponseWriter, r *http.Request) {
-		_, err := io.ReadFull(r.Body, make([]byte, len("the first bytes")))
-		if err != nil {
-			return
-		}
-
-		time.Sleep(shortTimeout / 2)
-		answeredAt.Store(new(time.Now()))
-		http.NotFound(w, r)
-	})
-	addr, out := startProxy(t, app.URL, map[string]string{
-		clientRequestTimeout: shortTimeoutSetting,
-		metricsToken:         token,
-	})
-
-	start := time.Now()
-	got := readResponse(t, sendPartOfBody(t, addr))
-	wantTimedOut(t, start)
-
-	// Close returns once the app has finished with the request.
-	app.Close()
-
-	// The app's answer came before the time ran out, so the client is
-	// sent that answer, not smallwebwaf's 408. Only a hold-up of the test
-	// process can make it come later, and SPEC.md then asks for 408, as
-	// smallwebwaf was waiting for the client, or for 504 if the bytes the
-	// client sent had not reached the app.
-	at := answeredAt.Load()
-	want := http.StatusNotFound
-
-	switch {
-	case at == nil:
-		want = http.StatusGatewayTimeout
-	case at.Sub(start) >= shortTimeout:
-		want = http.StatusRequestTimeout
-	}
-
-	wantStatus(t, got, want)
-	wantLine(t, out.requestLine(t), want, requestlog.ActionTimedOut)
-	wantLimitHits(t, addr, clientRequestTimeout, 1)
-}
-
 func TestAppTooSlowToAnswer(t *testing.T) {
 	t.Parallel()