discriminator: the header limits are the first check (closes #311)

A request over 50 cookies or 100 header fields is now refused before any
other check. It causes no lookup, no query to RouteWatch, a DNS blocklist
or AbuseIPDB, no rule file match and no Core Rule Set. The rate limits no
longer count it, and a banned or denied client gets 431 for it. In observe
mode no lookup is made for it. Requests within the limits go through the
same checks in the same order.

Model: opus-5-5

README.md

@@ -511,7 +511,7 @@ of the request reaches the app.
 
 `discriminator` takes the checks:
 
-3. The tag lists.
+3. The header limits, then the tag lists.
 4. The static lists, `SWW_ALLOW_NETS` and `SWW_DENY_NETS`.
 5. The bans.
 6. The lookup of the client's AS number and country.
@@ -523,7 +523,7 @@ of the request reaches the app.
 11. The rate limits.
 12. The trap paths.
 13. The rule files.
-14. The header limits, and then the Core Rule Set.
+14. The Core Rule Set.
 15. The tag refusals again, so that `exploitsignature` can refuse too.
 16. A path under `/_sww/` is answered `404`, so that the app never sees that
     prefix.
@@ -539,7 +539,7 @@ of the request reaches the app.
 A refused visitor is answered with `SWW_BAN_RESPONSE`. A `block` rule or a Core
 Rule Set match answers `403` and bans nobody, since the Core Rule Set has false
 positives; such refusals count toward `SWW_ERROR_BURST_THRESHOLD` instead. A
-request over the header limits of step 14 is answered `431`, and bans nobody
+request over the header limits of step 3 is answered `431`, and bans nobody
 either. `rproxy` itself answers `405` to a method it does not pass on, and the
 status for a size or time limit, such as `413` for a body too large.
 

SPEC.md

@@ -147,10 +147,12 @@ request log's `heuristic` field gives that name. `refusal_limit` and
     - An IPv6 client is its group: the netblock of `SWW_IPV6_GROUP_PREFIX`
       around its address, a /64 by default, since one abuser usually controls a
       whole /64. A client is therefore one IPv4 address or one IPv6 group.
-3. The tag lists (`tag_lists`; see "Request tags"). Each list that holds the
-   client adds its tag. Next, `routewatch_tags` adds the client's `asn=` and
-   `prefix=` tags while `SWW_ROUTEWATCH_URL` is set. A client in
-   `SWW_ALLOW_NETS` gets no tag.
+3. The header limits (`header_limits`; see "Answering in time"). They come
+   before every other check, since they only count. Next, the tag lists
+   (`tag_lists`; see "Request tags"). Each list that holds the client adds its
+   tag. Next, `routewatch_tags` adds the client's `asn=` and `prefix=` tags
+   while `SWW_ROUTEWATCH_URL` is set. A client in `SWW_ALLOW_NETS` skips the
+   header limits and gets no tag.
 4. The static lists. A client in `SWW_ALLOW_NETS` skips steps 5 to 15, the byte
    limits and the error burst. A client in `SWW_DENY_NETS` is refused
    (`deny_nets`); a client in both lists is let through.
@@ -183,10 +185,9 @@ request log's `heuristic` field gives that name. `refusal_limit` and
     refused and bans the client.
 13. The rule files (`rule_files`; see "Rule files"). Matching stops at the first
     rule that refuses.
-14. The header limits (`header_limits`; see "Answering in time"), and then the
-    Core Rule Set (`core_rule_set`; see "Attack detection"). A Core Rule Set
-    match is refused with `403` while `SWW_WAF_MODE` is `block`, and only logged
-    and alerted while it is `detect`.
+14. The Core Rule Set (`core_rule_set`; see "Attack detection"). A match is
+    refused with `403` while `SWW_WAF_MODE` is `block`, and only logged and
+    alerted while it is `detect`.
 15. The tag refusals again (`tag_deny`), so that `exploitsignature` can refuse
     too.
 16. A path under `/_sww/` is answered `404`, so that the app never sees that
@@ -2029,7 +2030,6 @@ waits on these, which run in the background:
 
 In `enforce` mode, a question does not wait for GeoJS when it is a request:
 
-- over the header limits;
 - from a client on a fetched blocklist, while `SWW_BLOCKLIST_ACTION` is `deny`;
 - from a client on the CrowdSec decision list;
 - that would take its client over a rate limit at 100% of it, since a percentage
@@ -2041,8 +2041,8 @@ number and country stay unknown until GeoJS has answered. So while
 `SWW_UNKNOWN_LIMIT_PERCENT` does not apply to it until then, so not waiting
 never lowers its limit.
 
-The static lists and bans come before the lookup, so they never wait. These can
-still wait for GeoJS:
+The header limits, the static lists and bans come before the lookup, so they
+never wait. These can still wait for GeoJS:
 
 - a country list, which needs the answer;
 - a rate limit a percentage has lowered, which needs the answer;
@@ -2061,13 +2061,17 @@ over either one:
 
 Real clients never reach these limits. The answer is `deny` with `431`, and the
 log line's action is `request_header_too_large`. The refusal makes no ban and is
-no offence. The rate limits count the request as usual, so a client that keeps
-sending such requests breaks a limit.
+no offence.
+
+The header limits are the first check. A request over them is refused before any
+other check runs. So it causes no lookup, no query to an outside source and no
+rule match, and the rate limits do not count it. A client under a ban or in
+`SWW_DENY_NETS` also gets `431` for such a request.
 
 The limits apply on every path and in every `SWW_WAF_MODE`. A client in
 `SWW_ALLOW_NETS` skips them, as it skips the Core Rule Set. In `observe` mode
-such a request is answered `allow` with `would_action`, and the Core Rule Set
-does not read it.
+such a request is answered `allow` with `would_action`, and no other check runs
+on it.
 
 The Core Rule Set runs each of its rules on each cookie, so thousands of small
 cookies would keep it busy for over a second. Within the limits, one long cookie

internal/discriminator/answertime_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/netip"
+	"path/filepath"
 	"slices"
 	"strconv"
 	"strings"
@@ -17,6 +18,7 @@ import (
 	"sneak.berlin/go/smallwebwaf/internal/bans"
 	"sneak.berlin/go/smallwebwaf/internal/discriminator"
 	"sneak.berlin/go/smallwebwaf/internal/lookup"
+	"sneak.berlin/go/smallwebwaf/internal/lookup/lookuptest"
 	"sneak.berlin/go/smallwebwaf/internal/ratelimit"
 	"sneak.berlin/go/smallwebwaf/internal/reputation"
 	"sneak.berlin/go/smallwebwaf/internal/requestlog"
@@ -216,93 +218,166 @@ func TestObserveModeAllowsARequestOverTheHeaderLimitsUnread(t *testing.T) {
 	}
 }
 
-func TestRequestsOverTheHeaderLimitsCountTowardTheRateLimits(t *testing.T) {
+func TestTheRateLimitsDoNotCountRequestsOverTheHeaderLimits(t *testing.T) {
 	t.Parallel()
 
 	s := startDiscriminator(t, "", map[string]string{rateLimitPerMinute: "1"})
-	ask := func() answer {
+
+	for range 2 {
 		req := s.question(siteA, client, "/")
 		req.Header.Set(cookieHeader, cookies(cookieLimit+1))
-
-		return do(t, req)
+		wantDisposition(t, do(t, req), tooLarge, discriminator.DispositionDeny)
 	}
 
-	wantDisposition(t, ask(), tooLarge, discriminator.DispositionDeny)
-	wantDisposition(t, ask(), http.StatusForbidden, discriminator.DispositionDeny)
-
-	lines := s.out.requestLines(t, 2)
-	wantLine(t, lines[1], http.StatusForbidden, requestlog.ActionClientIsRateLimited)
+	wantDisposition(t, s.ask(client, "/"), http.StatusOK, discriminator.DispositionAllow)
 }
 
-func TestTheHeaderLimitsComeAfterTheTrapPathsAndTheRuleFiles(t *testing.T) {
+func TestTheHeaderLimitsComeBeforeEveryCheckThatRefuses(t *testing.T) {
 	t.Parallel()
 
 	for _, tc := range []struct {
-		name, path, action string
+		name string
+		env  map[string]string
+		path string
+		// prepare gets the server ready, at now, before the question.
+		prepare func(t *testing.T, server *discriminator.Server, now time.Time)
 	}{
-		{"a trap path", "/wp-login.php", requestlog.ActionTemporarilyBanned},
-		{"a block rule", "/blocked", requestlog.ActionBlockedByBlockRule},
+		{"SWW_DENY_NETS", map[string]string{denyNets: client}, "/", leaveAsItIs},
+		{"a ban", nil, "/", banClient},
+		{"a blocklist", map[string]string{blocklistURLs: dropURL}, "/", listClient},
+		{
+			"the CrowdSec decision list",
+			map[string]string{crowdSecURL: lapi, crowdSecKey: bouncerKey}, "/", decideOnClient,
+		},
+		{"the rate limits", map[string]string{rateLimitPerMinute: "1"}, "/", countClient},
+		{
+			"a trap path", map[string]string{trapPaths: trapPathList}, "/wp-login.php",
+			leaveAsItIs,
+		},
+		{
+			"a block rule", map[string]string{rulesDir: writeRules(t, testRules)}, "/blocked",
+			leaveAsItIs,
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
-			s := startDiscriminator(t, "", map[string]string{
-				trapPaths: trapPathList, rulesDir: writeRules(t, testRules),
-			})
+			s := startDiscriminator(t, "", tc.env)
+			tc.prepare(t, s.server, s.clock.Now())
+
 			req := s.question(siteA, client, tc.path)
 			req.Header.Set(cookieHeader, cookies(cookieLimit+1))
 
-			wantDisposition(t, do(t, req), http.StatusForbidden, discriminator.DispositionDeny)
-			wantLine(t, s.out.requestLine(t), http.StatusForbidden, tc.action)
+			wantDisposition(t, do(t, req), tooLarge, discriminator.DispositionDeny)
+			wantLine(t, s.out.requestLine(t), tooLarge, requestlog.ActionRequestHeaderTooLarge)
 		})
 	}
 }
 
+func TestARequestOverTheHeaderLimitsRunsNoOtherCheck(t *testing.T) {
+	t.Parallel()
+
+	// Each check after the header limits leaves a mark on the log line of a
+	// request it reads: a tag, routewatch_tags_missing, a rule id, the
+	// counts, or the Core Rule Set's time.
+	path := filepath.Join(t.TempDir(), "ipinfo_lite.mmdb")
+	lookuptest.Write(t, path, map[string]lookuptest.Network{
+		client + "/32": {ASN: asnDE, ASName: asNameDE, Country: "DE"},
+	})
+	s := startDiscriminator(t, "", map[string]string{
+		lookupSource: fileSource, lookupDBPath: path,
+		tagLists:      "vpn=" + vpnList,
+		routeWatchURL: startRouteWatch(t, map[string]standInRoute{client: {prefixDE, 64496}}),
+		dnsblZones:    dnsblZone, reputationAction: actionLog,
+		rulesDir: writeRules(t, "noted path log ^/\n"),
+		wafMode:  block,
+	})
+	loadLists(t, s.server, map[string][]string{vpnList: {client}})
+	loadVerdicts(s.server, map[string][]string{client: {dnsblZone}})
+
+	over := s.question(siteA, client, "/")
+	over.Header.Set(cookieHeader, cookies(cookieLimit+1))
+	wantDisposition(t, do(t, over), tooLarge, discriminator.DispositionDeny)
+
+	within := s.question(siteA, client, "/")
+	within.Header.Set(cookieHeader, cookies(cookieLimit))
+	wantDisposition(t, do(t, within), http.StatusOK, discriminator.DispositionAllow)
+
+	lines := s.out.requestLines(t, 2)
+
+	wantTags(t, lines[0])
+	wantTagsMissing(t, lines[0], "")
+
+	if lines[0].RuleIDs != nil || lines[0].Counts != (ratelimit.Counts{}) ||
+		lines[0].DurationCoreRuleSetMS != nil {
+		t.Errorf("rule ids %v, counts %+v and Core Rule Set time %v, want none",
+			lines[0].RuleIDs, lines[0].Counts, lines[0].DurationCoreRuleSetMS)
+	}
+
+	// The request within the limits shows that each check leaves its mark.
+	wantTags(t, lines[1],
+		"asn=64496", "country=DE", "dnsbl="+dnsblZone, "exploitsignature", "vpn")
+	wantTagsMissing(t, lines[1], "no_answer_yet")
+
+	if lines[1].RuleIDs == nil || lines[1].Counts.Minute != 1 ||
+		lines[1].DurationCoreRuleSetMS == nil {
+		t.Errorf("rule ids %v, counts %+v and Core Rule Set time %v, want each",
+			lines[1].RuleIDs, lines[1].Counts, lines[1].DurationCoreRuleSetMS)
+	}
+}
+
 func TestNotWaitingForGeoJSNeverLowersAClientsLimit(t *testing.T) {
 	t.Parallel()
 
+	// The client's next request takes it over the whole limit, so it does
+	// not wait.
 	geojsURL, _ := startGeoJS(t)
 	s := startDiscriminator(t, geojsURL, map[string]string{
-		unknownLimitPercent: "0", lookupTimeout: "1h",
+		unknownLimitPercent: "0", lookupTimeout: "1h", rateLimitPerMinute: "1",
 	})
-	req := s.question(siteA, fromDE, "/")
-	req.Header.Set(cookieHeader, cookies(cookieLimit+1))
+	countClient(t, s.server, s.clock.Now())
 
-	wantDisposition(t, do(t, req), tooLarge, discriminator.DispositionDeny)
+	wantDisposition(t, s.ask(fromDE, "/"), http.StatusForbidden,
+		discriminator.DispositionDeny)
 
 	line := s.out.requestLine(t)
-	wantLine(t, line, tooLarge, requestlog.ActionRequestHeaderTooLarge)
+	wantLine(t, line, http.StatusForbidden, requestlog.ActionClientIsRateLimited)
 
 	if line.RateLimitPercent != nil {
 		t.Errorf("limit percentage %d, want none", *line.RateLimitPercent)
 	}
-
-	// The client is under no ban, and GeoJS places it in a country.
-	wantDisposition(t, s.ask(fromDE, "/"), http.StatusOK, discriminator.DispositionAllow)
 }
 
 func TestAClientGeoJSPlacedInNoCountryCountsAsUnknownWithoutWaiting(t *testing.T) {
 	t.Parallel()
 
+	// The client's next request takes it over the whole limit, so it does
+	// not wait.
 	geojsURL, _ := startGeoJS(t)
-	s := startDiscriminator(t, geojsURL, map[string]string{unknownLimitPercent: "0"})
+	s := startDiscriminator(t, geojsURL, map[string]string{
+		unknownLimitPercent: "0", rateLimitPerMinute: "1",
+	})
 	s.server.GeoJS.Load([]lookup.Answer{{
 		Client: netip.MustParsePrefix(client + "/32"), ASN: "AS64512",
 		Answered: s.clock.Now(), Used: s.clock.Now(),
 	}})
+	countClient(t, s.server, s.clock.Now())
 
-	req := s.question(siteA, client, "/")
-	req.Header.Set(cookieHeader, cookies(cookieLimit+1))
+	wantDisposition(t, s.ask(client, "/"), http.StatusForbidden,
+		discriminator.DispositionDeny)
 
-	wantDisposition(t, do(t, req), http.StatusForbidden, discriminator.DispositionDeny)
-	wantLine(t, s.out.requestLine(t), http.StatusForbidden,
-		requestlog.ActionClientIsRateLimited)
+	line := s.out.requestLine(t)
+	wantLine(t, line, http.StatusForbidden, requestlog.ActionClientIsRateLimited)
+
+	if line.RateLimitPercent == nil || *line.RateLimitPercent != 0 {
+		t.Errorf("limit percentage %v, want 0", line.RateLimitPercent)
+	}
 }
 
-func TestObserveModeWaitsForGeoJSForARequestOverTheHeaderLimits(t *testing.T) {
+func TestObserveModeAsksGeoJSNothingForARequestOverTheHeaderLimits(t *testing.T) {
 	t.Parallel()
 
-	geojsURL, _ := startGeoJS(t)
+	geojsURL, asked := startGeoJS(t)
 	s := startDiscriminator(t, geojsURL, map[string]string{
 		mode: observe, addLookupHeaders: on, lookupTimeout: "1h",
 	})
@@ -312,10 +387,9 @@ func TestObserveModeWaitsForGeoJSForARequestOverTheHeaderLimits(t *testing.T) {
 	got := do(t, req)
 	wantDisposition(t, got, http.StatusOK, discriminator.DispositionAllow)
 
-	asn, country := got.header.Get("X-Client-Asn"), got.header.Get("X-Client-Country")
-	if asn != asnDE || country != "DE" {
-		t.Errorf("answered with the AS number %q and the country %q, want %q and DE",
-			asn, country, asnDE)
+	if len(asked()) != 0 || got.header.Get("X-Client-Asn") != "" {
+		t.Errorf("GeoJS was asked about %v, and the answer gave the AS number %q, "+
+			"want neither", asked(), got.header.Get("X-Client-Asn"))
 	}
 }
 

internal/discriminator/discriminator.go

@@ -143,10 +143,10 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 // refusedWhateverTheAnswer reports whether the request is one that does not wait
 // for GeoJS, since the checks will refuse it at now whatever GeoJS answers about
-// its client: the heuristic crowdsec, header_limits, blocklists or rate_limits
-// will refuse it, as a function in that heuristic's file says. In observe mode,
-// which refuses nothing, none is. rproxy lets a request through when
-// discriminator answers too late.
+// its client: the heuristic crowdsec, blocklists or rate_limits will refuse it,
+// as a function in that heuristic's file says. In observe mode, which refuses
+// nothing, none is. rproxy lets a request through when discriminator answers too
+// late.
 func (rq *request) refusedWhateverTheAnswer(now time.Time) bool {
 	if rq.h.config.Observe {
 		return false
@@ -154,8 +154,7 @@ func (rq *request) refusedWhateverTheAnswer(now time.Time) bool {
 
 	_, decided := rq.crowdSecDecisionInForce(now)
 
-	return decided || overHeaderLimits(rq.headerCounts()) ||
-		rq.refusedByBlocklists(rq.h.lists.ListedBy(rq.client)) ||
+	return decided || rq.refusedByBlocklists(rq.h.lists.ListedBy(rq.client)) ||
 		rq.overAWholeRateLimit(now)
 }
 

internal/discriminator/headerlimits.go

@@ -10,22 +10,23 @@ import (
 	"sneak.berlin/go/smallwebwaf/internal/requestlog"
 )
 
-// The header limits: the most cookies and header fields a request may have
-// when the checks reach the Core Rule Set. Real clients never reach them.
+// The header limits: the most cookies and header fields a request may have.
+// Real clients never reach them.
 const (
 	maxCookies      = 50
 	maxHeaderFields = 100
 )
 
-// headerLimits refuses a request over the header limits, with 431. Its figures
-// are the request's header fields and cookies, and the limit of each.
+// headerLimits refuses a request with more than maxHeaderFields header fields
+// or more than maxCookies cookies, with 431. Its figures are the request's
+// header fields and cookies, and the limit of each.
 type headerLimits struct{}
 
 func (headerLimits) name() string { return "header_limits" }
 
 func (headerLimits) check(_ context.Context, rq *request, _ time.Time) *verdict {
 	fields, cookies := rq.headerCounts()
-	if !overHeaderLimits(fields, cookies) {
+	if fields <= maxHeaderFields && cookies <= maxCookies {
 		return nil
 	}
 
@@ -38,13 +39,6 @@ func (headerLimits) check(_ context.Context, rq *request, _ time.Time) *verdict
 	}
 }
 
-// overHeaderLimits reports whether a request with these counts of header fields
-// and cookies, as headerCounts counts them, is over the header limits: it has
-// more than maxHeaderFields fields or more than maxCookies cookies.
-func overHeaderLimits(fields, cookies int) bool {
-	return fields > maxHeaderFields || cookies > maxCookies
-}
-
 // headerCounts returns the request's header fields, each value of a header sent
 // twice being one, and its cookies, each part of a Cookie header between
 // semicolons being one unless it is empty or only spaces and tabs. The Core

internal/discriminator/heuristics.go

@@ -41,8 +41,11 @@ const (
 
 // beforeLookup are the heuristics that run before the client's AS number and
 // country are looked up, so that a client they refuse is not looked up.
+// header_limits comes first: it only counts, and a request it refuses causes no
+// other work.
 func beforeLookup() []heuristic {
 	return []heuristic{
+		headerLimits{},
 		tagLists{},
 		routeWatchTags{},
 		denyNets{},
@@ -64,7 +67,6 @@ func afterLookup() []heuristic {
 		rateLimits{},
 		trapPaths{},
 		ruleFiles{},
-		headerLimits{},
 		coreRuleSet{},
 		tagDeny{},
 	}