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

Three tests closed a test server 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.

No change to non-test code.

Model: opus-5-5

internal/coreruleset/coreruleset_test.go

@@ -25,41 +25,14 @@ import (
 //nolint:gochecknoglobals // a constant cannot be a list
 var defaultDisabledRules = []int{920340, 920420, 920440, 920640, 930130, 930140}
 
-// loaded holds, for each set of settings the tests have asked for, the
+// newCoreRuleSet returns the Core Rule Set at paranoia level level, with
-// function that loads the Core Rule Set with them once.
+// the rules in disabled switched off.
-//
+func newCoreRuleSet(t *testing.T, level int, disabled ...int) *coreruleset.CoreRuleSet {
-//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 := loaderFor(params)()
+	crs, err := coreruleset.New(coreruleset.Params{
+		ParanoiaLevel: level, DisabledRules: disabled,
+	})
 	if err != nil {
 		t.Fatalf("load the Core Rule Set: %v", err)
 	}
@@ -67,17 +40,6 @@ func shared(t *testing.T, params coreruleset.Params) *coreruleset.CoreRuleSet {
 	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.
@@ -198,7 +160,7 @@ func matched(ids ...int) coreruleset.Result {
 func atDefaults(t *testing.T) *coreruleset.CoreRuleSet {
 	t.Helper()
 
-	return sharedCoreRuleSet(t, 1, defaultDisabledRules...)
+	return newCoreRuleSet(t, 1, defaultDisabledRules...)
 }
 
 // wantChange checks that crs lets through passes, a request one of the
@@ -433,8 +395,8 @@ func TestParanoiaLevel(t *testing.T) {
 	// Accept-Charset is refused from paranoia level 2.
 	r := get("/", "Accept-Charset: utf-8")
 
-	wantResult(t, sharedCoreRuleSet(t, 1), r, coreruleset.Result{})
+	wantResult(t, newCoreRuleSet(t, 1), r, coreruleset.Result{})
-	wantResult(t, sharedCoreRuleSet(t, 2), r, matched(920451))
+	wantResult(t, newCoreRuleSet(t, 2), r, matched(920451))
 }
 
 func TestEachDisabledRuleIsSwitchedOff(t *testing.T) {
@@ -444,9 +406,9 @@ func TestEachDisabledRuleIsSwitchedOff(t *testing.T) {
 	// which adds 3.
 	r := request{http.MethodTrace, "/", []string{"Host: 192.0.2.1"}}
 
-	wantResult(t, sharedCoreRuleSet(t, 1), r,
+	wantResult(t, newCoreRuleSet(t, 1), r,
 		coreruleset.Result{RuleIDs: []int{911100, 920350}, Score: 8})
-	wantResult(t, sharedCoreRuleSet(t, 1, 920350, 911100), r, coreruleset.Result{})
+	wantResult(t, newCoreRuleSet(t, 1, 920350, 911100), r, coreruleset.Result{})
 }
 
 func TestImagePushThatReusesALayerIsNotCheckedForCommandNames(t *testing.T) {
@@ -695,9 +657,14 @@ func recordedRequests(t *testing.T, name string) []recordedRequest {
 func readingBodiesUpTo64K(t *testing.T) *coreruleset.CoreRuleSet {
 	t.Helper()
 
-	return shared(t, coreruleset.Params{
+	crs, err := coreruleset.New(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
@@ -763,23 +730,19 @@ func TestOrdinaryWordPressRequestsPassAndAttacksAreRefused(t *testing.T) {
 	defaults, bodies := atDefaults(t), readingBodiesUpTo64K(t)
 
 	for _, recorded := range recordedRequests(t, "wordpress-requests.txt") {
-		t.Run(recorded.Method+" "+recorded.Target, func(t *testing.T) {
+		r := request{recorded.Method, recorded.Target, recorded.Headers}
-			t.Parallel()
 
-			r := request{recorded.Method, recorded.Target, recorded.Headers}
+		crsSets := []*coreruleset.CoreRuleSet{defaults}
+		if recorded.Body != "" {
+			crsSets = append(crsSets, bodies)
+		}
 
-			crsSets := []*coreruleset.CoreRuleSet{defaults}
+		for _, crs := range crsSets {
-			if recorded.Body != "" {
+			got, _ := inspectBody(t, crs, r, recorded.Body)
-				crsSets = append(crsSets, bodies)
+			if got.Score >= threshold {
+				t.Errorf("%s %s %.200q refused: %+v", r.method, r.target, recorded.Body, got)
 			}
-
+		}
-			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{
@@ -1326,9 +1289,14 @@ const injection = "1' OR '1'='1"
 func readingBodies(t *testing.T) *coreruleset.CoreRuleSet {
 	t.Helper()
 
-	return shared(t, coreruleset.Params{
+	crs, err := coreruleset.New(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
@@ -1613,19 +1581,10 @@ 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, crs, post(multipart), body, matched(942100), body)
+	wantBody(t, readingBodies(t), post(multipart), body, matched(942100), body)
 }
 
 func TestBodyLimitOf1GLoads(t *testing.T) {

internal/coreruleset/drupal_test.go

@@ -34,22 +34,18 @@ func TestOrdinaryDrupalRequestsPass(t *testing.T) {
 	defaults, bodies := atDefaults(t), readingBodies(t)
 
 	for _, recorded := range recordedRequests(t, "drupal-requests.txt") {
-		t.Run(recorded.Method+" "+recorded.Target, func(t *testing.T) {
+		r := request{recorded.Method, recorded.Target, recorded.Headers}
-			t.Parallel()
 
-			r := request{recorded.Method, recorded.Target, recorded.Headers}
+		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, defaults, r, recorded.Body)
+		got, _ = inspectBody(t, bodies, r, recorded.Body)
-			if got.Score >= drupalThreshold {
+		if got.Score >= drupalThreshold {
-				t.Errorf("%s %s refused: %+v", r.method, r.target, got)
+			t.Errorf("%s %s %.200q refused while bodies are read: %+v",
-			}
+				r.method, r.target, recorded.Body, 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,6 +5,7 @@ import (
 	"context"
 	"encoding/pem"
 	"io"
+	"maps"
 	"net"
 	"net/http"
 	"net/http/httptest"
@@ -13,7 +14,6 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
-	"sync"
 	"sync/atomic"
 	"syscall"
 	"testing"
@@ -195,13 +195,7 @@ 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{})
@@ -227,10 +221,6 @@ 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.
@@ -242,14 +232,12 @@ func TestTheSiteAnswersWhileDiscriminatorIsStoppedAndStartedAgain(t *testing.T)
 		t.Errorf("rproxy's line is %v", line)
 	}
 
-	// Started again with its state files, it applies the ban again.
+	// Back at its address, with its state files, it applies the ban again.
-	addr, _, stop = startDiscriminator(t, env)
+	restarted := maps.Clone(env)
-	forwardTo(addr)
+	restarted[listenAddr] = addr
+	_, _, stop = startDiscriminator(t, restarted)
 
-	defer func() {
+	defer stop()
-		forwardTo("")
-		stop()
-	}()
 
 	wantStatus(t, site+"/", placed, http.StatusForbidden)
 
@@ -261,83 +249,6 @@ 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 {