Core Rule Set tests: inspect a request with a body only while bodies are read (closes #287)

A recorded request with a body or a Content-Type header is now inspected
only by the Core Rule Set that reads bodies, in the WordPress test as in
the other apps' tests. Reading bodies only adds rule matches, so passing
there means passing at the defaults. TestReadingBodiesOnlyAddsMatches
checks that on every 40th recorded request with a body of each app, as
recorded and with an SQL injection added to its query.

Model: opus-5-5

Dockerfile

@@ -43,12 +43,6 @@ RUN --mount=type=tmpfs,target=/root/.cache/go-build \
     { echo "--- Rerunning with -v for details ---"; \
       go test -tags no_fs_access -timeout 90s -race -v ./...; exit 1; }
 
-# Second pass: the recorded requests, too slow for make test under -race.
-RUN --mount=type=tmpfs,target=/root/.cache/go-build \
-    go test -tags no_fs_access,recorded -timeout 90s -cover ./internal/coreruleset/... || \
-    { echo "--- Rerunning with -v for details ---"; \
-      go test -tags no_fs_access,recorded -timeout 90s -v ./internal/coreruleset/...; exit 1; }
-
 # Tidy stage: `go mod tidy` in the test phase's Go, so that the files it
 # writes pass the test phase's check. Nothing else depends on it, so only
 # script/tidy, which names the stage after it, builds it.

README.md

@@ -695,8 +695,7 @@ on the host.
 - `script/projectname`: prints `smallwebwaf`, which the other scripts tag their
   images with.
 - `script/test`: checks that `go.mod` and `go.sum` are tidy, then runs the
-  tests, as the `test` phase of the `Dockerfile`. It runs them twice: once with
-  the race detector, and once without it for the recorded requests. `make test`.
+  tests, as the `test` phase of the `Dockerfile`. `make test`.
 - `script/tidy`: writes `go.mod` and `go.sum` as `go mod tidy` writes them, in
   the `tidy` stage of the `Dockerfile`. `make tidy`.
 - `script/lint`: runs golangci-lint, as the `lint` phase of the `Dockerfile`.

internal/coreruleset/coreruleset_test.go

@@ -1,6 +1,7 @@
 package coreruleset_test
 
 import (
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -572,6 +573,38 @@ func TestExclusionsFileThatCannotBeUsedIsAnError(t *testing.T) {
 	}
 }
 
+// giteaRequests returns the recorded ordinary gitea requests in testdata.
+// Each has the Referer of a page that names the same, as a page opened
+// from it has.
+func giteaRequests(t *testing.T) []request {
+	t.Helper()
+
+	text, err := os.ReadFile(filepath.Join("testdata", "gitea-requests.txt"))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var requests []request
+
+	for line := range strings.Lines(string(text)) {
+		line = strings.TrimSpace(line)
+		if line == "" || strings.HasPrefix(line, "#") {
+			continue
+		}
+
+		target, header, _ := strings.Cut(line, " ")
+
+		headers := []string{"Referer: http://git.example" + target}
+		if header != "" {
+			headers = append(headers, header)
+		}
+
+		requests = append(requests, get(target, headers...))
+	}
+
+	return requests
+}
+
 // An SQL injection, a script injection and a shell command after a ";", as
 // query values. Every app's tests check that its fields still refuse the
 // first two.
@@ -591,13 +624,134 @@ const passwordField = "password="
 //nolint:gochecknoglobals // a constant cannot be a list
 var hardValues = []string{"python3", "powershell", "ls%20-la", ".htaccess"}
 
-func TestGiteaAttacksAreRefused(t *testing.T) {
+// wantRecordedRequestsPass checks that none of the recorded ordinary
+// requests in the file name in testdata is refused. A request with a body
+// or a Content-Type header is checked while bodies are read, and any other
+// request at the defaults. Reading bodies only adds rule matches, so a
+// request that passes while bodies are read passes at the defaults too;
+// TestReadingBodiesOnlyAddsMatches checks that. Each request runs as its
+// own parallel subtest.
+func wantRecordedRequestsPass(t *testing.T, name string) {
+	t.Helper()
+
+	const threshold = 5 // SWW_WAF_ANOMALY_THRESHOLD's default
+
+	defaults, bodies := atDefaults(t), readingBodies(t)
+
+	for _, recorded := range recordedRequests(t, name) {
+		t.Run(recorded.Method+" "+recorded.Target, func(t *testing.T) {
+			t.Parallel()
+
+			r := request{recorded.Method, recorded.Target, recorded.Headers}
+
+			typed := slices.ContainsFunc(recorded.Headers, func(header string) bool {
+				name, _, _ := strings.Cut(header, ":")
+
+				return strings.EqualFold(name, "Content-Type")
+			})
+			if recorded.Body == "" && !typed {
+				got, _ := inspectBody(t, defaults, r, "")
+				if got.Score >= threshold {
+					t.Errorf("%s %s refused: %+v", r.method, r.target, got)
+				}
+
+				return
+			}
+
+			got, _ := inspectBody(t, bodies, r, recorded.Body)
+			if got.Score >= threshold {
+				t.Errorf("%s %s %.200q refused while bodies are read: %+v",
+					r.method, r.target, recorded.Body, got)
+			}
+		})
+	}
+}
+
+// TestReadingBodiesOnlyAddsMatches checks that every rule a recorded
+// request with a body matches at the defaults also matches while bodies
+// are read, with a score no lower. It checks every 40th such request of
+// each app, as recorded and with an SQL injection added to its query, which
+// the defaults must match.
+func TestReadingBodiesOnlyAddsMatches(t *testing.T) {
+	t.Parallel()
+
+	defaults, bodies := atDefaults(t), readingBodies(t)
+
+	for _, name := range []string{
+		"drupal-requests.txt",
+		"mastodon-requests.txt",
+		"mediawiki-requests.txt",
+		"nextcloud-requests.txt",
+		"vaultwarden-requests.txt",
+		"wordpress-requests.txt",
+	} {
+		withBody := 0
+
+		for _, recorded := range recordedRequests(t, name) {
+			if recorded.Body == "" {
+				continue
+			}
+
+			withBody++
+			if withBody%40 != 1 {
+				continue
+			}
+
+			separator := "?"
+			if strings.Contains(recorded.Target, "?") {
+				separator = "&"
+			}
+
+			r := request{recorded.Method, recorded.Target, recorded.Headers}
+			wantNoMatchLost(t, defaults, bodies, r, recorded.Body)
+
+			r.target += separator + "id=" + sqlInjection
+			if atDefault := wantNoMatchLost(t, defaults, bodies, r,
+				recorded.Body); len(atDefault.RuleIDs) == 0 {
+				t.Errorf("%s %s matched nothing at the defaults", r.method, r.target)
+			}
+		}
+	}
+}
+
+// wantNoMatchLost checks that every rule defaults matches in r with body,
+// bodies matches too, with a score no lower, and returns what defaults
+// found.
+func wantNoMatchLost(
+	t *testing.T, defaults, bodies *coreruleset.CoreRuleSet, r request, body string,
+) coreruleset.Result {
+	t.Helper()
+
+	atDefault, _ := inspectBody(t, defaults, r, body)
+	whileRead, _ := inspectBody(t, bodies, r, body)
+
+	for _, id := range atDefault.RuleIDs {
+		if !slices.Contains(whileRead.RuleIDs, id) {
+			t.Errorf("%s %s: %d matched at the defaults only", r.method, r.target, id)
+		}
+	}
+
+	if whileRead.Score < atDefault.Score {
+		t.Errorf("%s %s scored %d at the defaults and %d while bodies are read",
+			r.method, r.target, atDefault.Score, whileRead.Score)
+	}
+
+	return atDefault
+}
+
+func TestOrdinaryGiteaRequestsPassAndAttacksAreRefused(t *testing.T) {
 	t.Parallel()
 
 	const threshold = 5 // SWW_WAF_ANOMALY_THRESHOLD's default
 
 	crs := atDefaults(t)
 
+	for _, r := range giteaRequests(t) {
+		if got := inspect(t, crs, r); got.Score >= threshold {
+			t.Errorf("%s %q refused: %+v", r.target, r.headers[1:], got)
+		}
+	}
+
 	// In q too, off gitea's search pages.
 	for _, parameter := range []string{"id", "q"} {
 		for _, attack := range []string{
@@ -650,6 +804,46 @@ const (
 	wpTags    = "/wp-json/wp/v2/tags"
 )
 
+// recordedRequest is a line of a file of recorded requests in testdata.
+type recordedRequest struct {
+	Method  string   `json:"method"`
+	Target  string   `json:"target"`
+	Headers []string `json:"headers"`
+	Body    string   `json:"body"`
+}
+
+// recordedRequests returns the recorded ordinary requests in the file name
+// in testdata, each with its body.
+func recordedRequests(t *testing.T, name string) []recordedRequest {
+	t.Helper()
+
+	path := filepath.Join("testdata", name)
+
+	text, err := os.ReadFile(path) //nolint:gosec // a file in testdata
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var requests []recordedRequest
+
+	for line := range strings.Lines(string(text)) {
+		if strings.HasPrefix(line, "#") {
+			continue
+		}
+
+		var r recordedRequest
+
+		err := json.Unmarshal([]byte(line), &r)
+		if err != nil {
+			t.Fatalf("%q: %v", line, err)
+		}
+
+		requests = append(requests, r)
+	}
+
+	return requests
+}
+
 // readingBodiesUpTo64K returns the Core Rule Set as sww runs it by default,
 // but reading bodies up to the largest SWW_WAF_BODY_LIMIT.
 func readingBodiesUpTo64K(t *testing.T) *coreruleset.CoreRuleSet {
@@ -715,13 +909,33 @@ func wordpressAttackTargets(attack string) []string {
 	}
 }
 
-func TestWordPressAttacksAreRefused(t *testing.T) {
+func TestOrdinaryWordPressRequestsPassAndAttacksAreRefused(t *testing.T) {
 	t.Parallel()
 
 	const threshold = 5 // SWW_WAF_ANOMALY_THRESHOLD's default
 
 	defaults, bodies := atDefaults(t), readingBodiesUpTo64K(t)
 
+	for _, recorded := range recordedRequests(t, "wordpress-requests.txt") {
+		t.Run(recorded.Method+" "+recorded.Target, func(t *testing.T) {
+			t.Parallel()
+
+			r := request{recorded.Method, recorded.Target, recorded.Headers}
+
+			// As in wantRecordedRequestsPass, a request with a body passes at
+			// the defaults if it passes while bodies are read.
+			crs := defaults
+			if recorded.Body != "" {
+				crs = bodies
+			}
+
+			got, _ := inspectBody(t, crs, r, recorded.Body)
+			if got.Score >= threshold {
+				t.Errorf("%s %s %.200q refused: %+v", r.method, r.target, recorded.Body, got)
+			}
+		})
+	}
+
 	for _, attack := range []string{
 		sqlInjection,
 		"..%2F..%2Fwp-config.php",

internal/coreruleset/drupal_test.go

@@ -31,6 +31,12 @@ const drupalEditAccount = "/user/2/edit"
 // drupalName is the field for a person's name, as the start of a form body.
 const drupalName = "name="
 
+func TestOrdinaryDrupalRequestsPass(t *testing.T) {
+	t.Parallel()
+
+	wantRecordedRequestsPass(t, "drupal-requests.txt")
+}
+
 // drupalAttacks are an SQL injection, a path traversal and a script
 // injection, as a query value.
 //

internal/coreruleset/mastodon_test.go

@@ -54,6 +54,12 @@ const (
 	mastodonStatus   = "status="
 )
 
+func TestOrdinaryMastodonRequestsPass(t *testing.T) {
+	t.Parallel()
+
+	wantRecordedRequestsPass(t, "mastodon-requests.txt")
+}
+
 // mastodonQueryFields sends each field that skips 930120, 932120, 932250
 // and 932260 in the query, on a path where it skips them, ending in "=".
 //

internal/coreruleset/mediawiki_test.go

@@ -42,6 +42,12 @@ const (
 	mediawikiUploadPage = "/index.php/Special:Upload"
 )
 
+func TestOrdinaryMediaWikiRequestsPass(t *testing.T) {
+	t.Parallel()
+
+	wantRecordedRequestsPass(t, "mediawiki-requests.txt")
+}
+
 // mediawikiQueryFields and mediawikiBodyFields together hold every field that
 // skips 930120, 932120, 932250 and 932260, other than wikitext, wpTitles and
 // the JSON fields of /rest.php/. mediawikiQueryFields sends each in the query, on a

internal/coreruleset/nextcloud_test.go

@@ -48,6 +48,12 @@ const (
 // nextcloudOther is a JSON field no exclusion names.
 const nextcloudOther = `{"other":"python3"}`
 
+func TestOrdinaryNextcloudRequestsPass(t *testing.T) {
+	t.Parallel()
+
+	wantRecordedRequestsPass(t, "nextcloud-requests.txt")
+}
+
 // nextcloudQueryFields are the query fields that skip 930120, 932120, 932250
 // and 932260, each on a path where it skips them, ending in "=".
 //

internal/coreruleset/recorded_test.go

@@ -1,204 +0,0 @@
-//go:build recorded
-
-// The tests that check the apps' recorded requests in testdata. The recorded
-// tag keeps them out of the test phase's pass with the race detector, and
-// its second pass, without the race detector, runs them.
-
-package coreruleset_test
-
-import (
-	"encoding/json"
-	"os"
-	"path/filepath"
-	"slices"
-	"strings"
-	"testing"
-
-	"sneak.berlin/go/smallwebwaf/internal/coreruleset"
-)
-
-// giteaRequests returns the recorded ordinary gitea requests in testdata.
-// Each has the Referer of a page that names the same, as a page opened
-// from it has.
-func giteaRequests(t *testing.T) []request {
-	t.Helper()
-
-	text, err := os.ReadFile(filepath.Join("testdata", "gitea-requests.txt"))
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	var requests []request
-
-	for line := range strings.Lines(string(text)) {
-		line = strings.TrimSpace(line)
-		if line == "" || strings.HasPrefix(line, "#") {
-			continue
-		}
-
-		target, header, _ := strings.Cut(line, " ")
-
-		headers := []string{"Referer: http://git.example" + target}
-		if header != "" {
-			headers = append(headers, header)
-		}
-
-		requests = append(requests, get(target, headers...))
-	}
-
-	return requests
-}
-
-func TestOrdinaryGiteaRequestsPass(t *testing.T) {
-	t.Parallel()
-
-	const threshold = 5 // SWW_WAF_ANOMALY_THRESHOLD's default
-
-	crs := atDefaults(t)
-
-	for _, r := range giteaRequests(t) {
-		if got := inspect(t, crs, r); got.Score >= threshold {
-			t.Errorf("%s %q refused: %+v", r.target, r.headers[1:], got)
-		}
-	}
-}
-
-// recordedRequest is a line of a file of recorded requests in testdata.
-type recordedRequest struct {
-	Method  string   `json:"method"`
-	Target  string   `json:"target"`
-	Headers []string `json:"headers"`
-	Body    string   `json:"body"`
-}
-
-// recordedRequests returns the recorded ordinary requests in the file name
-// in testdata, each with its body.
-func recordedRequests(t *testing.T, name string) []recordedRequest {
-	t.Helper()
-
-	path := filepath.Join("testdata", name)
-
-	text, err := os.ReadFile(path) //nolint:gosec // a file in testdata
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	var requests []recordedRequest
-
-	for line := range strings.Lines(string(text)) {
-		if strings.HasPrefix(line, "#") {
-			continue
-		}
-
-		var r recordedRequest
-
-		err := json.Unmarshal([]byte(line), &r)
-		if err != nil {
-			t.Fatalf("%q: %v", line, err)
-		}
-
-		requests = append(requests, r)
-	}
-
-	return requests
-}
-
-// wantRecordedRequestsPass checks that none of the recorded ordinary
-// requests in the file name in testdata is refused, at the defaults and
-// while bodies are read. A request with neither a body nor a Content-Type
-// header is checked only at the defaults: with neither, the set that reads
-// bodies has nothing more to check. Each request runs as its own parallel
-// subtest.
-func wantRecordedRequestsPass(t *testing.T, name string) {
-	t.Helper()
-
-	const threshold = 5 // SWW_WAF_ANOMALY_THRESHOLD's default
-
-	defaults, bodies := atDefaults(t), readingBodies(t)
-
-	for _, recorded := range recordedRequests(t, name) {
-		t.Run(recorded.Method+" "+recorded.Target, func(t *testing.T) {
-			t.Parallel()
-
-			r := request{recorded.Method, recorded.Target, recorded.Headers}
-
-			got, _ := inspectBody(t, defaults, r, recorded.Body)
-			if got.Score >= threshold {
-				t.Errorf("%s %s refused: %+v", r.method, r.target, got)
-			}
-
-			typed := slices.ContainsFunc(recorded.Headers, func(header string) bool {
-				name, _, _ := strings.Cut(header, ":")
-
-				return strings.EqualFold(name, "Content-Type")
-			})
-			if recorded.Body == "" && !typed {
-				return
-			}
-
-			got, _ = inspectBody(t, bodies, r, recorded.Body)
-			if got.Score >= threshold {
-				t.Errorf("%s %s %.200q refused while bodies are read: %+v",
-					r.method, r.target, recorded.Body, got)
-			}
-		})
-	}
-}
-
-func TestOrdinaryDrupalRequestsPass(t *testing.T) {
-	t.Parallel()
-
-	wantRecordedRequestsPass(t, "drupal-requests.txt")
-}
-
-func TestOrdinaryMastodonRequestsPass(t *testing.T) {
-	t.Parallel()
-
-	wantRecordedRequestsPass(t, "mastodon-requests.txt")
-}
-
-func TestOrdinaryMediaWikiRequestsPass(t *testing.T) {
-	t.Parallel()
-
-	wantRecordedRequestsPass(t, "mediawiki-requests.txt")
-}
-
-func TestOrdinaryNextcloudRequestsPass(t *testing.T) {
-	t.Parallel()
-
-	wantRecordedRequestsPass(t, "nextcloud-requests.txt")
-}
-
-func TestOrdinaryVaultwardenRequestsPass(t *testing.T) {
-	t.Parallel()
-
-	wantRecordedRequestsPass(t, "vaultwarden-requests.txt")
-}
-
-func TestOrdinaryWordPressRequestsPass(t *testing.T) {
-	t.Parallel()
-
-	const threshold = 5 // SWW_WAF_ANOMALY_THRESHOLD's default
-
-	defaults, bodies := atDefaults(t), readingBodiesUpTo64K(t)
-
-	for _, recorded := range recordedRequests(t, "wordpress-requests.txt") {
-		t.Run(recorded.Method+" "+recorded.Target, func(t *testing.T) {
-			t.Parallel()
-
-			r := request{recorded.Method, recorded.Target, recorded.Headers}
-
-			crsSets := []*coreruleset.CoreRuleSet{defaults}
-			if recorded.Body != "" {
-				crsSets = append(crsSets, bodies)
-			}
-
-			for _, crs := range crsSets {
-				got, _ := inspectBody(t, crs, r, recorded.Body)
-				if got.Score >= threshold {
-					t.Errorf("%s %s %.200q refused: %+v", r.method, r.target, recorded.Body, got)
-				}
-			}
-		})
-	}
-}

internal/coreruleset/vaultwarden_test.go

@@ -27,6 +27,12 @@ const (
 // vaultwardenEmail is a JSON body that sends an address in email.
 const vaultwardenEmail = `{"email":"VALUE"}`
 
+func TestOrdinaryVaultwardenRequestsPass(t *testing.T) {
+	t.Parallel()
+
+	wantRecordedRequestsPass(t, "vaultwarden-requests.txt")
+}
+
 // vaultwardenField is a field Vaultwarden receives in plain text: the
 // request that sends it, and its body, with VALUE where the field's value
 // goes.