Tests no longer connect to test servers they closed (closes #257)

Four tests closed a test server or stopped discriminator, and then
connected to its port. Another test process could take that port in
between and receive the connection.

- TestUnreachableRouteWatchLetsTheRequestThroughAndSaysWhy and
  TestUnreachableRoutewatchFailsTheQueryAndTheLogOmitsTheClient, formerly
  TestRefusedConnection..., point routewatch at 224.0.0.1, a multicast
  address to which TCP cannot connect.
- TestStalledRemoteLogEndpointHoldsUpNoRequest keeps its endpoint open
  and closes each connection it takes, instead of closing the endpoint.
- TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain gives
  rproxy an address the test holds throughout. The test forwards each
  connection there to the running discriminator, and closes it while
  discriminator is stopped.

No change to non-test code.

Model: opus-5-5

internal/coreruleset/coreruleset_test.go

@@ -25,14 +25,41 @@ import (
 //nolint:gochecknoglobals // a constant cannot be a list
 var defaultDisabledRules = []int{920340, 920420, 920440, 920640, 930130, 930140}
 
-// newCoreRuleSet returns the Core Rule Set at paranoia level level, with
-// the rules in disabled switched off.
-func newCoreRuleSet(t *testing.T, level int, disabled ...int) *coreruleset.CoreRuleSet {
+// loaded holds, for each set of settings the tests have asked for, the
+// function that loads the Core Rule Set with them once.
+//
+//nolint:gochecknoglobals // the tests share it
+var loaded = struct {
+	sync.Mutex
+
+	loaders map[string]func() (*coreruleset.CoreRuleSet, error)
+}{loaders: map[string]func() (*coreruleset.CoreRuleSet, error){}}
+
+// loaderFor returns the function that loads the Core Rule Set with params
+// once, making it the first time params are asked for.
+func loaderFor(params coreruleset.Params) func() (*coreruleset.CoreRuleSet, error) {
+	loaded.Lock()
+	defer loaded.Unlock()
+
+	settings := fmt.Sprintf("%+v", params)
+
+	load, found := loaded.loaders[settings]
+	if !found {
+		load = sync.OnceValues(func() (*coreruleset.CoreRuleSet, error) {
+			return coreruleset.New(params)
+		})
+		loaded.loaders[settings] = load
+	}
+
+	return load
+}
+
+// shared returns the Core Rule Set with params, loaded once for every
+// test that asks for the same settings.
+func shared(t *testing.T, params coreruleset.Params) *coreruleset.CoreRuleSet {
 	t.Helper()
 
-	crs, err := coreruleset.New(coreruleset.Params{
-		ParanoiaLevel: level, DisabledRules: disabled,
-	})
+	crs, err := loaderFor(params)()
 	if err != nil {
 		t.Fatalf("load the Core Rule Set: %v", err)
 	}
@@ -40,6 +67,17 @@ func newCoreRuleSet(t *testing.T, level int, disabled ...int) *coreruleset.CoreR
 	return crs
 }
 
+// sharedCoreRuleSet returns the Core Rule Set at paranoia level level,
+// with the rules in disabled switched off. The set is shared with every
+// test that asks for the same settings.
+func sharedCoreRuleSet(
+	t *testing.T, level int, disabled ...int,
+) *coreruleset.CoreRuleSet {
+	t.Helper()
+
+	return shared(t, coreruleset.Params{ParanoiaLevel: level, DisabledRules: disabled})
+}
+
 // TestConcurrentRequestsKeepTheirOwnExclusions inspects a Drupal and a
 // WordPress request at once, many times over, on one Core Rule Set, as
 // sww's server does. Each passes only while its own exclusions apply.
@@ -160,7 +198,7 @@ func matched(ids ...int) coreruleset.Result {
 func atDefaults(t *testing.T) *coreruleset.CoreRuleSet {
 	t.Helper()
 
-	return newCoreRuleSet(t, 1, defaultDisabledRules...)
+	return sharedCoreRuleSet(t, 1, defaultDisabledRules...)
 }
 
 // wantChange checks that crs lets through passes, a request one of the
@@ -395,8 +433,8 @@ func TestParanoiaLevel(t *testing.T) {
 	// Accept-Charset is refused from paranoia level 2.
 	r := get("/", "Accept-Charset: utf-8")
 
-	wantResult(t, newCoreRuleSet(t, 1), r, coreruleset.Result{})
-	wantResult(t, newCoreRuleSet(t, 2), r, matched(920451))
+	wantResult(t, sharedCoreRuleSet(t, 1), r, coreruleset.Result{})
+	wantResult(t, sharedCoreRuleSet(t, 2), r, matched(920451))
 }
 
 func TestEachDisabledRuleIsSwitchedOff(t *testing.T) {
@@ -406,9 +444,9 @@ func TestEachDisabledRuleIsSwitchedOff(t *testing.T) {
 	// which adds 3.
 	r := request{http.MethodTrace, "/", []string{"Host: 192.0.2.1"}}
 
-	wantResult(t, newCoreRuleSet(t, 1), r,
+	wantResult(t, sharedCoreRuleSet(t, 1), r,
 		coreruleset.Result{RuleIDs: []int{911100, 920350}, Score: 8})
-	wantResult(t, newCoreRuleSet(t, 1, 920350, 911100), r, coreruleset.Result{})
+	wantResult(t, sharedCoreRuleSet(t, 1, 920350, 911100), r, coreruleset.Result{})
 }
 
 func TestImagePushThatReusesALayerIsNotCheckedForCommandNames(t *testing.T) {
@@ -657,14 +695,9 @@ func recordedRequests(t *testing.T, name string) []recordedRequest {
 func readingBodiesUpTo64K(t *testing.T) *coreruleset.CoreRuleSet {
 	t.Helper()
 
-	crs, err := coreruleset.New(coreruleset.Params{
+	return shared(t, coreruleset.Params{
 		ParanoiaLevel: 1, DisabledRules: defaultDisabledRules, BodyLimit: 64<<10 - 1,
 	})
-	if err != nil {
-		t.Fatalf("load the Core Rule Set: %v", err)
-	}
-
-	return crs
 }
 
 // wordpressAttackTargets returns attack in a query parameter no exclusion
@@ -730,19 +763,23 @@ func TestOrdinaryWordPressRequestsPassAndAttacksAreRefused(t *testing.T) {
 	defaults, bodies := atDefaults(t), readingBodiesUpTo64K(t)
 
 	for _, recorded := range recordedRequests(t, "wordpress-requests.txt") {
-		r := request{recorded.Method, recorded.Target, recorded.Headers}
+		t.Run(recorded.Method+" "+recorded.Target, func(t *testing.T) {
+			t.Parallel()
 
-		crsSets := []*coreruleset.CoreRuleSet{defaults}
-		if recorded.Body != "" {
-			crsSets = append(crsSets, bodies)
-		}
+			r := request{recorded.Method, recorded.Target, recorded.Headers}
 
-		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)
+			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)
+				}
+			}
+		})
 	}
 
 	for _, attack := range []string{
@@ -1289,14 +1326,9 @@ const injection = "1' OR '1'='1"
 func readingBodies(t *testing.T) *coreruleset.CoreRuleSet {
 	t.Helper()
 
-	crs, err := coreruleset.New(coreruleset.Params{
+	return shared(t, coreruleset.Params{
 		ParanoiaLevel: 1, DisabledRules: defaultDisabledRules, BodyLimit: bodyLimit,
 	})
-	if err != nil {
-		t.Fatalf("load the Core Rule Set: %v", err)
-	}
-
-	return crs
 }
 
 // post is a POST request for / with a body of the type contentType, a
@@ -1581,10 +1613,19 @@ func TestCorazaWritesNoFile(t *testing.T) {
 	// refuses to load, and could not write a file of a multipart body.
 	t.Setenv("TMPDIR", filepath.Join(t.TempDir(), "missing"))
 
+	// The test loads its own Core Rule Set, since loading is part of what
+	// it checks.
+	crs, err := coreruleset.New(coreruleset.Params{
+		ParanoiaLevel: 1, DisabledRules: defaultDisabledRules, BodyLimit: bodyLimit,
+	})
+	if err != nil {
+		t.Fatalf("load the Core Rule Set: %v", err)
+	}
+
 	body := "--b\r\nContent-Disposition: form-data; name=\"file\"; " +
 		"filename=\"notes.txt\"\r\nContent-Type: text/plain\r\n\r\n" +
 		strings.Repeat("a", 1000) + "\r\n" + field("q", injection) + end
-	wantBody(t, readingBodies(t), post(multipart), body, matched(942100), body)
+	wantBody(t, crs, post(multipart), body, matched(942100), body)
 }
 
 func TestBodyLimitOf1GLoads(t *testing.T) {

internal/coreruleset/drupal_test.go

@@ -34,18 +34,22 @@ func TestOrdinaryDrupalRequestsPass(t *testing.T) {
 	defaults, bodies := atDefaults(t), readingBodies(t)
 
 	for _, recorded := range recordedRequests(t, "drupal-requests.txt") {
-		r := request{recorded.Method, recorded.Target, recorded.Headers}
+		t.Run(recorded.Method+" "+recorded.Target, func(t *testing.T) {
+			t.Parallel()
 
-		got, _ := inspectBody(t, defaults, r, recorded.Body)
-		if got.Score >= drupalThreshold {
-			t.Errorf("%s %s refused: %+v", r.method, r.target, got)
-		}
+			r := request{recorded.Method, recorded.Target, recorded.Headers}
 
-		got, _ = inspectBody(t, bodies, r, recorded.Body)
-		if got.Score >= drupalThreshold {
-			t.Errorf("%s %s %.200q refused while bodies are read: %+v",
-				r.method, r.target, recorded.Body, got)
-		}
+			got, _ := inspectBody(t, defaults, r, recorded.Body)
+			if got.Score >= drupalThreshold {
+				t.Errorf("%s %s refused: %+v", r.method, r.target, got)
+			}
+
+			got, _ = inspectBody(t, bodies, r, recorded.Body)
+			if got.Score >= drupalThreshold {
+				t.Errorf("%s %s %.200q refused while bodies are read: %+v",
+					r.method, r.target, recorded.Body, got)
+			}
+		})
 	}
 }
 

internal/sww/rproxy_test.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"encoding/pem"
 	"io"
-	"maps"
 	"net"
 	"net/http"
 	"net/http/httptest"
@@ -14,6 +13,7 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
+	"sync"
 	"sync/atomic"
 	"syscall"
 	"testing"
@@ -195,7 +195,13 @@ func TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(t *testing.T)
 
 	env := discriminatorSettings(t)
 	addr, _, stop := startDiscriminator(t, env)
-	site, out := startRProxy(t, "http://"+addr, startApp(t))
+
+	// rproxy asks discriminator through an address the test holds throughout,
+	// so no other test can take it while discriminator is stopped.
+	backend, forwardTo := startForwarder(t)
+	forwardTo(addr)
+
+	site, out := startRProxy(t, "http://"+backend, startApp(t))
 
 	// A visitor who keeps asking throughout, and is never refused.
 	visitorDone := make(chan struct{})
@@ -221,6 +227,10 @@ func TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(t *testing.T)
 
 	wantStatus(t, site+bannedProbe, placed, http.StatusForbidden)
 	wantStatus(t, site+"/", placed, http.StatusForbidden)
+
+	// rproxy's connections are closed at once from here, before discriminator
+	// stops and frees its port.
+	forwardTo("")
 	stop()
 
 	// With discriminator down, the site answers, and rproxy says the default did it.
@@ -232,12 +242,14 @@ func TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(t *testing.T)
 		t.Errorf("rproxy's line is %v", line)
 	}
 
-	// Back at its address, with its state files, it applies the ban again.
-	restarted := maps.Clone(env)
-	restarted[listenAddr] = addr
-	_, _, stop = startDiscriminator(t, restarted)
+	// Started again with its state files, it applies the ban again.
+	addr, _, stop = startDiscriminator(t, env)
+	forwardTo(addr)
 
-	defer stop()
+	defer func() {
+		forwardTo("")
+		stop()
+	}()
 
 	wantStatus(t, site+"/", placed, http.StatusForbidden)
 
@@ -249,6 +261,83 @@ func TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(t *testing.T)
 	}
 }
 
+// startForwarder listens at a free port until the test ends, and returns its
+// address and what sets the address it forwards to. It forwards each
+// connection it takes to that address, and closes the connection at once while
+// the address is "".
+func startForwarder(t *testing.T) (string, func(addr string)) {
+	t.Helper()
+
+	ctx := t.Context()
+
+	listener, err := (&net.ListenConfig{}).Listen(ctx, "tcp", localhost+":0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+
+	t.Cleanup(func() {
+		_ = listener.Close()
+	})
+
+	var (
+		lock sync.Mutex
+		to   string
+	)
+
+	// dial connects to the address set, and returns nil while it is "". It
+	// holds the lock throughout, so once another address is set, nothing
+	// connects to the one before.
+	dial := func() net.Conn {
+		lock.Lock()
+		defer lock.Unlock()
+
+		if to == "" {
+			return nil
+		}
+
+		conn, err := (&net.Dialer{}).DialContext(ctx, "tcp", to)
+		if err != nil {
+			return nil
+		}
+
+		return conn
+	}
+
+	go func() {
+		for {
+			conn, err := listener.Accept()
+			if err != nil {
+				return
+			}
+
+			upstream := dial()
+			if upstream == nil {
+				_ = conn.Close()
+
+				continue
+			}
+
+			go pipe(upstream, conn)
+			go pipe(conn, upstream)
+		}
+	}()
+
+	return listener.Addr().String(), func(addr string) {
+		lock.Lock()
+		defer lock.Unlock()
+
+		to = addr
+	}
+}
+
+// pipe copies src to dst until src ends, then closes both, which ends the copy
+// the other way too.
+func pipe(dst, src net.Conn) {
+	_, _ = io.Copy(dst, src)
+	_ = dst.Close()
+	_ = src.Close()
+}
+
 // answersNormally reports whether a request to url from the client at from, as
 // X-Forwarded-For names it, gets the app's answer.
 func answersNormally(ctx context.Context, url, from string) bool {