fix(waf-ng): vhost signal excludes WAF-blocked/banned traffic (real activity only) (ref #896)

The Begin/End hook was placed before the WAF-inspection block, so a
request the WAF blocks (403 warning/ban) still refreshed last_request_ts —
scanner/bot traffic against public on-demand vhosts (near-constant
internet-wide scanning) kept the signal "fresh" forever, so
should_sleep()'s idle-age check never passed. Defeated auto-sleep for the
feature's primary deployment (public on-demand vhosts).

Move the Begin/defer End bracket to right after the WAF-inspection block's
403/warning/ban early-returns, immediately before the media-cache-hit
check. One placement still covers three real-response exit paths via the
single defer: the media-cache hit (counts — genuine content served),
media-cache-miss proxy, and the plain proxy. Blocked, banned, waker, and
421 requests never reach this line.

Adds TestVhostSignalsExcludedForWAFBlock (verified RED against the old
placement, GREEN after the move).

Co-Authored-By: Gerald KERMA <devel@cybermind.fr>

.superpowers/sdd/task-15-report.md

@@ -1,6 +1,54 @@
 # Task 15 report — per-vhost signal source for scale-to-zero AUTO-sleep (#896)
 
-## Summary
+## Fix — review-found Critical bug: WAF-blocked/banned traffic counted as activity
+
+Review caught a Critical correctness bug in the original hook placement: the
+`Begin`/`defer End` bracket sat right after the on-demand/waker/421 checks but
+**before** the entire WAF-inspection block (rules match → detect/escalate/
+block → graduated warning/ban, main.go's ~lines 373-548). So a request the
+WAF blocked with 403 — never reaching the real backend — still called
+`Begin`, refreshing `lastSeen[host]` to "now". Public on-demand vhosts sit
+under near-constant internet scanning (masscan/shodan/bots) that trips
+block-mode rules, so `last_request_ts` kept getting refreshed by BLOCKED
+traffic and `should_sleep()`'s `last_request_age >= idle_threshold` never
+passed — defeating auto-sleep for exactly the deployment this feature exists
+for (public on-demand vhosts). The sibling visit-stats recorder in the same
+function already excludes 403/421/waker from its tally (via a deferred
+status check) but the vhostsignals hook had only excluded the waker/421
+branch, not the WAF-block/ban early-returns.
+
+**Fix**: moved the `Begin`/`defer End` bracket from its original spot (right
+after the on-demand/waker + 421 checks, before WAF inspection) to
+immediately after the WAF-inspection block closes — right before the
+"Task 6.1 — media cache hit" comment. All of the WAF-inspection block's
+early `return`s (escalate-ban at ~line 502, plain 403 at ~521, graduated
+ban at ~557, graduated warning at ~559) now execute, if they're going to,
+*before* reaching the hook. A single placement there still covers three
+exit paths via the one `defer`: the media-cache-hit `return` just below it,
+the media-cache-miss `proxy.ServeHTTP` further down, and the plain
+`proxy.ServeHTTP` at the very end of the handler. I judged the media-cache
+hit to count as legitimate vhost activity per the coordinator's guidance —
+the client gets a genuine response for that vhost's content, same as a
+backend-served response — so it is included, not excluded.
+
+Regression test `TestVhostSignalsExcludedForWAFBlock` (in `main_test.go`,
+next to `TestVhostSignalsExcludedForWakerBranch`): sends an on-demand vhost
+*with a live route* a SQLi payload (same fixture/payload as
+`TestHandlerWarningThenBan`) and asserts the vhost snapshot stays empty
+after the 403. Verified RED against the pre-fix placement (`git stash` on
+just `main.go` to isolate the placement regression, confirmed the test
+failed with the vhost recorded — `map[sleepy.example.com:{LastRequestTS:...
+ActiveConns:0}]` — then `git stash pop` to restore the fix) and GREEN after.
+
+Re-verified after the fix: `go build ./...` clean, `go vet ./cmd/sbxwaf/`
+clean, `go test ./cmd/sbxwaf/ -count=1 -v` → **110 PASS / 0 FAIL** (whole
+package, one more than the original 109 for this new test), `go test
+./cmd/sbxwaf/ -race -run 'TestVhostSignals|TestOnDemand' -count=1 -v` → all
+PASS. Fixed in a follow-up commit
+`fix(waf-ng): vhost signal excludes WAF-blocked/banned traffic (real
+activity only) (ref #896)`.
+
+## Summary (original implementation)
 
 Two independent changes, one per package:
 

packages/secubox-toolbox-ng/cmd/sbxwaf/main.go

@@ -316,22 +316,6 @@ func (s *Server) handler() http.Handler {
 			return
 		}
 
-		// #896 Task 15 — bracket this request in the per-vhost signal emitter.
-		// Reached ONLY when routeLookup found a live route (ok == true) and we
-		// did NOT take the waker branch above (that branch already returned) —
-		// so this is exactly "a real request is about to be proxied to a real
-		// backend". Gated to on-demand vhosts: those are the only ones the
-		// sleeper ever acts on, so recording anything else would just grow the
-		// map for no benefit. A single placement here (rather than one at each
-		// of the two proxy.ServeHTTP call sites below — the media-cache-miss
-		// path and the plain path) covers both: `defer` fires whichever one the
-		// function returns through, so one Begin always pairs with exactly one
-		// End regardless of which branch handles the request.
-		if s.vhostSignals != nil && s.onDemand != nil && s.onDemand.Contains(host) {
-			s.vhostSignals.Begin(host)
-			defer s.vhostSignals.End(host)
-		}
-
 		// Use the cached proxy from Routes when available (Task 1.2 perf goal:
 		// no per-request *httputil.ReverseProxy allocation).
 		var proxy *httputil.ReverseProxy
@@ -563,6 +547,29 @@ func (s *Server) handler() http.Handler {
 			}
 		}
 
+		// #896 Task 15 — bracket this request in the per-vhost signal emitter.
+		// Placed here, AFTER the entire WAF-inspection block above (all its
+		// escalate-ban/warning/ban early `return`s at lines ~502-545 have
+		// already happened by this point) and after the earlier on-demand/
+		// waker + 421 checks — so reaching this line means the request is
+		// getting a REAL response for this vhost: either a media-cache hit
+		// just below (genuine content served to the client — counts as
+		// activity) or a proxied backend response (media-cache-miss path or
+		// the plain path further down). A WAF 403/warning/ban, a graduated
+		// or escalate ban, or the waker/421 branch never reaches this line,
+		// so scanner/bot traffic that trips a block-mode rule can no longer
+		// refresh last_request_ts and defeat auto-sleep on a public vhost
+		// (the bug this placement fixes — see TestVhostSignalsExcludedForWAFBlock).
+		// Gated to on-demand vhosts: those are the only ones the sleeper ever
+		// acts on. A single placement here (rather than one at each of the
+		// two proxy.ServeHTTP call sites, plus the cache-hit return) covers
+		// all three: `defer` fires whichever exit path the function takes
+		// from here on, so one Begin always pairs with exactly one End.
+		if s.vhostSignals != nil && s.onDemand != nil && s.onDemand.Contains(host) {
+			s.vhostSignals.Begin(host)
+			defer s.vhostSignals.End(host)
+		}
+
 		// Task 6.1 — media cache hit: serve from disk, bypass upstream.
 		// Only for GET requests; cache is nil-safe.
 		//

packages/secubox-toolbox-ng/cmd/sbxwaf/main_test.go

@@ -314,6 +314,61 @@ func TestVhostSignalsExcludedForWakerBranch(t *testing.T) {
 	}
 }
 
+// TestVhostSignalsExcludedForWAFBlock is the regression test for a Task 15
+// review finding: an on-demand vhost WITH a live backend that the WAF blocks
+// (403 warning/ban) must NOT update its vhost signal. Public on-demand
+// vhosts sit under near-constant internet scanning (masscan/shodan/bots)
+// that trips block-mode rules; if that blocked traffic kept refreshing
+// last_request_ts, last_request_age would never cross idle_threshold and
+// auto-sleep would never fire for the vhost — defeating the feature for its
+// primary deployment. The fix moves the Begin/End hook to AFTER the WAF-
+// inspection block's 403 early-returns (see the placement comment in
+// handler()); before the fix, this test failed because Begin ran before
+// s.rules.MatchModes/writeWarning ever had a chance to return early.
+func TestVhostSignalsExcludedForWAFBlock(t *testing.T) {
+	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = io.WriteString(w, "backend ok")
+	}))
+	defer backend.Close()
+	backendAddr := strings.TrimPrefix(backend.URL, "http://")
+
+	rulesPath := buildSQLiRulesFile(t) // reuse helper from inspect_test.go
+	vs := NewVhostSignals("")
+	srv := &Server{
+		routeLookup: func(host string) (string, int, bool) {
+			h, p, err := splitHostPort(backendAddr)
+			if err != nil {
+				return "", 0, false
+			}
+			return h, p, true
+		},
+		rules:        LoadRules(rulesPath),
+		ban:          NewBan(300*time.Second, 3),
+		onDemand:     &OnDemand{entries: map[string]bool{"sleepy.example.com": true}},
+		vhostSignals: vs,
+	}
+
+	handler := srv.handler()
+	// UNION SELECT in the query → triggers the sqli rule (same payload as
+	// TestHandlerWarningThenBan) against an on-demand vhost that DOES have a
+	// live route — the WAF must still block it before ever reaching the
+	// backend or touching the vhost signal.
+	req := httptest.NewRequest(http.MethodGet,
+		"http://sleepy.example.com/?q=1+union+select+1,2,3", nil)
+	req.Host = "sleepy.example.com"
+	req.RemoteAddr = "203.0.113.42:12345" // public IP (TEST-NET-3, non-RFC1918)
+	rec := httptest.NewRecorder()
+	handler.ServeHTTP(rec, req)
+
+	if rec.Code != http.StatusForbidden {
+		t.Fatalf("expected the WAF to block this SQLi payload with 403, got %d", rec.Code)
+	}
+	if snap := vs.snapshot(); len(snap) != 0 {
+		t.Fatalf("a WAF-blocked (warning/ban) request must not update vhost signals; got %+v", snap)
+	}
+}
+
 // TestTrustedHostSkipsWAF verifies that a request to a trusted host is NOT
 // blocked even when the payload would normally trigger the WAF.
 // Mirrors Python check_request whitelist (secubox_waf.py:761-763).