server: assert timeouts on the served http.Server, not just the constructor (closes #120 )

The timeout tests called newHTTPServer directly, leaving Run's call site unguarded: a Run that built its http.Server inline would drop every timeout with the suite still green. TestRunWiresSocketTimeouts now wires a Server as cmd/dnswatcher does (minus watcher and resolver, so no live DNS), drives Run with an unbindable port so it stores its http.Server and returns without listening, and asserts the served server carries all four timeouts and both required relationships. Reverting Run to an inline http.Server without timeouts fails it. The near-tautological addr/handler test is dropped; the wiring test covers the handler end to end. The read-deadline note is corrected: a too-small ReadTimeout does not make ReadHeaderTimeout unreachable (net/http applies it directly); it installs an already-expired whole-request deadline. Verified against the pinned go1.25 net/http. Model: opus-4-8
2026-09-21 07:51:28 +00:00
7 changed files with 123 additions and 560 deletions
@@ -203,46 +203,6 @@ are read, so a smaller value would sever the connection before a handler
 using its full budget could respond. `IdleTimeout` exceeds common
 Prometheus scrape intervals so the scraper reuses its connection.
 ### Security Headers
 Every response — the dashboard, the static assets under `/s/...`, the
 healthchecks, the JSON API, and `/metrics` — carries the following
 headers, set by a global middleware:
 | Header                      | Value                                 |
 |-----------------------------|---------------------------------------|
 | `Strict-Transport-Security` | `max-age=31536000; includeSubDomains` |
 | `Content-Security-Policy`   | see below                             |
 | `X-Frame-Options`           | `DENY`                                |
 | `X-Content-Type-Options`    | `nosniff`                             |
 | `Referrer-Policy`           | `no-referrer`                         |
 | `Permissions-Policy`        | all unused browser features denied    |
 The content security policy is:
 ```
 default-src 'self'; script-src 'none'; style-src 'self'; img-src 'self';
 font-src 'none'; connect-src 'none'; object-src 'none'; base-uri 'none';
 form-action 'none'; frame-ancestors 'none'
 ```
 The dashboard ships no JavaScript (the 30-second refresh is a
 `<meta http-equiv="refresh">`), no inline styles, no inline event
 handlers, and no images; its only subresource is the embedded stylesheet
 at `/s/css/tailwind.min.css`, which `style-src 'self'` permits. The
 policy therefore needs neither `unsafe-inline` nor `unsafe-eval`.
 `frame-ancestors 'none'` is the primary anti-framing control, with
 `X-Frame-Options: DENY` retained as the legacy fallback.
 HSTS is emitted unconditionally, including over plain HTTP. dnswatcher is
 expected to run behind a TLS-terminating reverse proxy, and the browser
 must still be told to enforce HTTPS end to end, so the header is never
 gated on whether the request itself arrived over TLS.
 `Referrer-Policy: no-referrer` is stricter than the
 `strict-origin-when-cross-origin` baseline: the dashboard has no
 cross-origin navigation needs, and its URL may name internal hosts.
 ---
 ## Architecture
@@ -255,8 +215,7 @@ internal/
  globals/globals.go           Build-time variables (version)
  logger/logger.go             slog structured logging (TTY detection)
  healthcheck/healthcheck.go   Health check service
-  middleware/middleware.go      HTTP middleware (logging, CORS, security
+  middleware/middleware.go      HTTP middleware (logging, CORS, metrics auth)
                               headers, metrics auth)
  handlers/handlers.go         HTTP request handlers
  server/
    server.go                  HTTP server lifecycle
@@ -23,8 +23,13 @@ Rationale, Design, TODO, License, Author) if any are still missing.
 # Completed Steps
 - 2026-09-21: server timeout test now drives `Run` and asserts the
  served `http.Server` carries the timeouts; corrected the inverted
  `ReadTimeout` rationale note (#120).
 - 2026-09-21: `go mod tidy` dropped the redundant `golang.org/x/sync`
  `// indirect` line so `script/bootstrap` leaves a clean tree (#132)
 - 2026-08-10: comment-only corrections to `script/bootstrap`,
  `script/cibuild`, and `Dockerfile.lint`. The `goimports` pin in
  `script/bootstrap` was justified by a claim that `script/fmt-check`
@@ -111,16 +116,6 @@ Rationale, Design, TODO, License, Author) if any are still missing.
  than the 60s `chimw.Timeout` handler budget so that budget stays
  reachable, and tests in `internal/server` pin both the non-zero
  values and that relationship (#99)
 - 2026-08-09: security response headers middleware
  (`SecurityHeaders()` in `internal/middleware/middleware.go`)
  registered globally in `internal/server/routes.go`, so HSTS, CSP,
  `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy`, and
  `Permissions-Policy` are set on every response including `/s/...` and
  `/metrics`; the CSP needs no `unsafe-inline`/`unsafe-eval` because the
  dashboard ships no JavaScript and no inline styles; HSTS is emitted
  unconditionally per policy (TLS-terminating proxy in front). Remaining
  1.0 hardening items — `http.Server` timeouts, request body limits,
  rate limiting, CORS scoping — are tracked separately
 - 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs
  in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the
  org-standard v2-schema config used across the org's repos
@@ -21,60 +21,6 @@ import (
 // corsMaxAge is the maximum age for CORS preflight responses.
 const corsMaxAge = 300
 // Security response header values applied to every response.
 //
 // The CSP is as strict as the dashboard allows: the template ships no
 // JavaScript, no inline styles, no inline event handlers and no images,
 // and its only subresource is the embedded stylesheet at
 // /s/css/tailwind.min.css, which style-src 'self' permits. Neither
 // unsafe-inline nor unsafe-eval is used. frame-ancestors 'none' is the
 // primary anti-framing control; X-Frame-Options is the legacy fallback.
 const (
 	// hstsValue is emitted unconditionally, including over plain HTTP,
 	// because the service runs behind a TLS-terminating proxy and the
 	// browser must still enforce HTTPS end to end.
 	hstsValue = "max-age=31536000; includeSubDomains"
 	cspValue = "default-src 'self'; " +
 		"script-src 'none'; " +
 		"style-src 'self'; " +
 		"img-src 'self'; " +
 		"font-src 'none'; " +
 		"connect-src 'none'; " +
 		"object-src 'none'; " +
 		"base-uri 'none'; " +
 		"form-action 'none'; " +
 		"frame-ancestors 'none'"
 	frameOptionsValue = "DENY"
 	contentTypeOptionsValue = "nosniff"
 	// referrerPolicyValue is stricter than the policy minimum of
 	// strict-origin-when-cross-origin: the dashboard has no
 	// cross-origin navigation needs and its URL may name internal
 	// hosts.
 	referrerPolicyValue = "no-referrer"
 	permissionsPolicyValue = "accelerometer=(), " +
 		"autoplay=(), " +
 		"camera=(), " +
 		"display-capture=(), " +
 		"encrypted-media=(), " +
 		"fullscreen=(), " +
 		"geolocation=(), " +
 		"gyroscope=(), " +
 		"magnetometer=(), " +
 		"microphone=(), " +
 		"midi=(), " +
 		"payment=(), " +
 		"picture-in-picture=(), " +
 		"publickey-credentials-get=(), " +
 		"screen-wake-lock=(), " +
 		"usb=(), " +
 		"xr-spatial-tracking=()"
 )
 // Params contains dependencies for Middleware.
 type Params struct {
 	fx.In
@@ -240,37 +186,6 @@ func (m *Middleware) CORS() func(http.Handler) http.Handler {
 	})
 }
 // SecurityHeaders returns middleware that sets the security response
 // headers required for production internet exposure on every response.
 //
 // The headers are set before the request reaches the next handler so
 // that they are present on every response, including panics recovered
 // by chi's Recoverer and timeouts produced by chi's Timeout.
 func (m *Middleware) SecurityHeaders() func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(
 			writer http.ResponseWriter,
 			request *http.Request,
 		) {
 			header := writer.Header()
 			header.Set("Strict-Transport-Security", hstsValue)
 			header.Set("Content-Security-Policy", cspValue)
 			header.Set("X-Frame-Options", frameOptionsValue)
 			header.Set(
 				"X-Content-Type-Options",
 				contentTypeOptionsValue,
 			)
 			header.Set("Referrer-Policy", referrerPolicyValue)
 			header.Set(
 				"Permissions-Policy",
 				permissionsPolicyValue,
 			)
 			next.ServeHTTP(writer, request)
 		})
 	}
 }
 // MetricsAuth returns basic auth middleware for /metrics.
 func (m *Middleware) MetricsAuth() func(http.Handler) http.Handler {
 	if m.params.Config.MetricsUsername == "" {
@@ -1,334 +0,0 @@
 package middleware_test
 import (
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
 	"github.com/go-chi/chi/v5"
 	"go.uber.org/fx/fxtest"
 	"sneak.berlin/go/dnswatcher/internal/config"
 	"sneak.berlin/go/dnswatcher/internal/globals"
 	"sneak.berlin/go/dnswatcher/internal/handlers"
 	"sneak.berlin/go/dnswatcher/internal/logger"
 	"sneak.berlin/go/dnswatcher/internal/middleware"
 	"sneak.berlin/go/dnswatcher/internal/notify"
 	"sneak.berlin/go/dnswatcher/internal/state"
 )
 // Expected security header values, spelled out literally so that any
 // change to the middleware has to be made deliberately here as well.
 const (
 	wantHSTS = "max-age=31536000; includeSubDomains"
 	wantCSP = "default-src 'self'; " +
 		"script-src 'none'; " +
 		"style-src 'self'; " +
 		"img-src 'self'; " +
 		"font-src 'none'; " +
 		"connect-src 'none'; " +
 		"object-src 'none'; " +
 		"base-uri 'none'; " +
 		"form-action 'none'; " +
 		"frame-ancestors 'none'"
 	wantFrameOptions = "DENY"
 	wantContentTypeOptions = "nosniff"
 	wantReferrerPolicy = "no-referrer"
 	wantPermissionsPolicy = "accelerometer=(), " +
 		"autoplay=(), " +
 		"camera=(), " +
 		"display-capture=(), " +
 		"encrypted-media=(), " +
 		"fullscreen=(), " +
 		"geolocation=(), " +
 		"gyroscope=(), " +
 		"magnetometer=(), " +
 		"microphone=(), " +
 		"midi=(), " +
 		"payment=(), " +
 		"picture-in-picture=(), " +
 		"publickey-credentials-get=(), " +
 		"screen-wake-lock=(), " +
 		"usb=(), " +
 		"xr-spatial-tracking=()"
 )
 // stylesheetPath is the only subresource the dashboard loads.
 const stylesheetPath = "/s/css/tailwind.min.css"
 // newTestLogger builds a logger for direct component construction.
 func newTestLogger(t *testing.T) *logger.Logger {
 	t.Helper()
 	glob, err := globals.New(nil)
 	if err != nil {
 		t.Fatalf("globals.New: %v", err)
 	}
 	log, err := logger.New(nil, logger.Params{Globals: glob})
 	if err != nil {
 		t.Fatalf("logger.New: %v", err)
 	}
 	return log
 }
 // newTestMiddleware builds a Middleware without an fx application.
 func newTestMiddleware(t *testing.T) *middleware.Middleware {
 	t.Helper()
 	glob, err := globals.New(nil)
 	if err != nil {
 		t.Fatalf("globals.New: %v", err)
 	}
 	mw, err := middleware.New(nil, middleware.Params{
 		Logger:  newTestLogger(t),
 		Globals: glob,
 		Config:  &config.Config{},
 	})
 	if err != nil {
 		t.Fatalf("middleware.New: %v", err)
 	}
 	return mw
 }
 // serveWithSecurityHeaders runs a GET through SecurityHeaders and
 // returns the recorded response.
 func serveWithSecurityHeaders(
 	t *testing.T,
 	target string,
 	handler http.Handler,
 ) *httptest.ResponseRecorder {
 	t.Helper()
 	mw := newTestMiddleware(t)
 	rec := httptest.NewRecorder()
 	req := httptest.NewRequestWithContext(
 		t.Context(), http.MethodGet, target, nil,
 	)
 	mw.SecurityHeaders()(handler).ServeHTTP(rec, req)
 	return rec
 }
 // okHandler writes a trivial 200 response.
 func okHandler() http.Handler {
 	return http.HandlerFunc(func(
 		writer http.ResponseWriter,
 		_ *http.Request,
 	) {
 		writer.WriteHeader(http.StatusOK)
 	})
 }
 func TestSecurityHeaders(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		name   string
 		header string
 		want   string
 	}{
 		{
 			"hsts",
 			"Strict-Transport-Security",
 			wantHSTS,
 		},
 		{
 			"csp",
 			"Content-Security-Policy",
 			wantCSP,
 		},
 		{
 			"frame options",
 			"X-Frame-Options",
 			wantFrameOptions,
 		},
 		{
 			"content type options",
 			"X-Content-Type-Options",
 			wantContentTypeOptions,
 		},
 		{
 			"referrer policy",
 			"Referrer-Policy",
 			wantReferrerPolicy,
 		},
 		{
 			"permissions policy",
 			"Permissions-Policy",
 			wantPermissionsPolicy,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 			rec := serveWithSecurityHeaders(t, "/", okHandler())
 			got := rec.Header().Get(tt.header)
 			if got != tt.want {
 				t.Errorf(
 					"%s = %q, want %q",
 					tt.header, got, tt.want,
 				)
 			}
 		})
 	}
 }
 // TestSecurityHeadersCSPDirectives guards the properties the repo
 // policy requires of the content security policy itself.
 func TestSecurityHeadersCSPDirectives(t *testing.T) {
 	t.Parallel()
 	rec := serveWithSecurityHeaders(t, "/", okHandler())
 	csp := rec.Header().Get("Content-Security-Policy")
 	forbidden := []string{"unsafe-inline", "unsafe-eval"}
 	for _, directive := range forbidden {
 		if strings.Contains(csp, directive) {
 			t.Errorf("CSP must not contain %q: %q", directive, csp)
 		}
 	}
 	required := []string{
 		"default-src 'self'",
 		"script-src 'none'",
 		"style-src 'self'",
 		"frame-ancestors 'none'",
 	}
 	for _, directive := range required {
 		if !strings.Contains(csp, directive) {
 			t.Errorf("CSP must contain %q: %q", directive, csp)
 		}
 	}
 }
 // TestSecurityHeadersOnErrorResponse verifies the headers are emitted
 // even when the wrapped handler fails, since they are set before the
 // handler runs.
 func TestSecurityHeadersOnErrorResponse(t *testing.T) {
 	t.Parallel()
 	failing := http.HandlerFunc(func(
 		writer http.ResponseWriter,
 		_ *http.Request,
 	) {
 		http.Error(
 			writer,
 			"boom",
 			http.StatusInternalServerError,
 		)
 	})
 	rec := serveWithSecurityHeaders(t, "/api/v1/status", failing)
 	if rec.Code != http.StatusInternalServerError {
 		t.Fatalf("status = %d, want 500", rec.Code)
 	}
 	if got := rec.Header().Get(
 		"X-Content-Type-Options",
 	); got != wantContentTypeOptions {
 		t.Errorf(
 			"X-Content-Type-Options = %q, want %q",
 			got, wantContentTypeOptions,
 		)
 	}
 	if got := rec.Header().Get(
 		"Strict-Transport-Security",
 	); got != wantHSTS {
 		t.Errorf(
 			"Strict-Transport-Security = %q, want %q",
 			got, wantHSTS,
 		)
 	}
 }
 // newTestHandlers builds real Handlers with empty monitoring state.
 func newTestHandlers(t *testing.T) *handlers.Handlers {
 	t.Helper()
 	glob, err := globals.New(nil)
 	if err != nil {
 		t.Fatalf("globals.New: %v", err)
 	}
 	log := newTestLogger(t)
 	notifier, err := notify.New(fxtest.NewLifecycle(t), notify.Params{
 		Logger: log,
 		Config: &config.Config{},
 	})
 	if err != nil {
 		t.Fatalf("notify.New: %v", err)
 	}
 	hnd, err := handlers.New(nil, handlers.Params{
 		Logger:  log,
 		Globals: glob,
 		State:   state.NewForTest(),
 		Notify:  notifier,
 	})
 	if err != nil {
 		t.Fatalf("handlers.New: %v", err)
 	}
 	return hnd
 }
 // TestDashboardRendersWithSecurityHeaders renders the real dashboard
 // through the middleware and checks that the policy still permits the
 // one stylesheet the page loads.
 func TestDashboardRendersWithSecurityHeaders(t *testing.T) {
 	t.Parallel()
 	mw := newTestMiddleware(t)
 	hnd := newTestHandlers(t)
 	router := chi.NewRouter()
 	router.Use(mw.SecurityHeaders())
 	router.Get("/", hnd.HandleDashboard())
 	rec := httptest.NewRecorder()
 	req := httptest.NewRequestWithContext(
 		t.Context(), http.MethodGet, "/", nil,
 	)
 	router.ServeHTTP(rec, req)
 	if rec.Code != http.StatusOK {
 		t.Fatalf("status = %d, want 200", rec.Code)
 	}
 	body := rec.Body.String()
 	if !strings.Contains(body, stylesheetPath) {
 		t.Errorf("dashboard does not reference %q", stylesheetPath)
 	}
 	if !strings.Contains(body, "dnswatcher") {
 		t.Errorf("dashboard body looks empty: %d bytes", len(body))
 	}
 	csp := rec.Header().Get("Content-Security-Policy")
 	if csp != wantCSP {
 		t.Errorf("CSP = %q, want %q", csp, wantCSP)
 	}
 	// The stylesheet is same-origin, so style-src 'self' allows it.
 	if !strings.Contains(csp, "style-src 'self'") {
 		t.Errorf("CSP would block %q: %q", stylesheetPath, csp)
 	}
 }
@@ -5,15 +5,20 @@ import (
 	"time"
 )
 // NewHTTPServer exports newHTTPServer for testing.
 func NewHTTPServer(
 	listenAddr string,
 	handler http.Handler,
 ) *http.Server {
 	return newHTTPServer(listenAddr, handler)
 }
 // RequestTimeout exports the handler execution budget applied by
 // chimw.Timeout in SetupRoutes, so tests can assert the relationship
 // between it and the server's WriteTimeout.
 const RequestTimeout time.Duration = requestTimeout
 // SetListenPort overrides the port Run binds. A test uses it to hand
 // Run an unbindable port so ListenAndServe fails immediately and Run
 // returns after storing its http.Server.
 func SetListenPort(s *Server, port int) {
 	s.port = port
 }
 // HTTPServerOf returns the http.Server that Run built and stored, so a
 // test can inspect the timeouts the running server actually carries.
 func HTTPServerOf(s *Server) *http.Server {
 	return s.httpServer
 }
@@ -21,7 +21,6 @@ func (s *Server) SetupRoutes() {
 	// Global middleware
 	s.router.Use(chimw.Recoverer)
 	s.router.Use(chimw.RequestID)
 	s.router.Use(s.mw.SecurityHeaders())
 	s.router.Use(s.mw.Logging())
 	s.router.Use(s.mw.CORS())
 	s.router.Use(chimw.Timeout(requestTimeout))
@@ -1,112 +1,136 @@
 package server_test
 import (
 	"net/http"
 	"testing"
 	"github.com/spf13/viper"
 	"go.uber.org/fx"
 	"sneak.berlin/go/dnswatcher/internal/config"
 	"sneak.berlin/go/dnswatcher/internal/globals"
 	"sneak.berlin/go/dnswatcher/internal/handlers"
 	"sneak.berlin/go/dnswatcher/internal/healthcheck"
 	"sneak.berlin/go/dnswatcher/internal/logger"
 	"sneak.berlin/go/dnswatcher/internal/middleware"
 	"sneak.berlin/go/dnswatcher/internal/notify"
 	"sneak.berlin/go/dnswatcher/internal/server"
 	"sneak.berlin/go/dnswatcher/internal/state"
 )
-// noopHandler stands in for the router; newHTTPServer only stores it.
+// buildServer wires a *server.Server exactly as cmd/dnswatcher does,
-func noopHandler() http.Handler {
+// minus the watcher/resolver subtree that would touch live DNS. fx
-	return http.HandlerFunc(
+// builds the object graph but the lifecycle is never started, so no
-		func(w http.ResponseWriter, _ *http.Request) {
+// OnStart hook runs and nothing listens or resolves. The caller must
-			w.WriteHeader(http.StatusOK)
+// first configure viper (config.New reads it), which is also why the
-		},
+// caller cannot run in parallel.
 func buildServer(t *testing.T) *server.Server {
 	t.Helper()
 	var srv *server.Server
 	app := fx.New(
 		fx.NopLogger,
 		fx.Provide(
 			globals.New,
 			logger.New,
 			config.New,
 			state.New,
 			healthcheck.New,
 			notify.New,
 			middleware.New,
 			handlers.New,
 			server.New,
 		),
 		fx.Populate(&srv),
 	)
 }
-// TestHTTPServerTimeoutsAreSet asserts that every socket-level
+	err := app.Err()
-// timeout is configured. A zero value in net/http means "no limit",
+	if err != nil {
-// so a refactor that silently drops one of these reintroduces the
+		t.Fatalf("building server graph: %v", err)
 // slowloris / unreaped-keep-alive exposure this guards against.
 //
 // The assertions are on the configured field values only; nothing
 // here measures elapsed time, so the test cannot flake on timing.
 func TestHTTPServerTimeoutsAreSet(t *testing.T) {
 	t.Parallel()
 	srv := server.NewHTTPServer(":8080", noopHandler())
 	if srv.ReadTimeout <= 0 {
 		t.Errorf(
 			"ReadTimeout must be non-zero, got %v",
 			srv.ReadTimeout,
 		)
 	}
-	if srv.ReadHeaderTimeout <= 0 {
+	return srv
 }
 // TestRunWiresSocketTimeouts pins that the http.Server the running
 // server actually serves — the one Run builds and hands to
 // ListenAndServe — carries every socket-level timeout, plus the two
 // relationships the values must satisfy. Earlier tests asserted these
 // on newHTTPServer directly, which left the call site unguarded: a Run
 // that built its http.Server inline would drop every timeout with the
 // suite still green (https://git.eeqj.de/sneak/dnswatcher/issues/120).
 //
 // Run is driven to completion with an unbindable port: it builds and
 // stores s.httpServer, then ListenAndServe fails at once and Run
 // returns without ever listening. The assertions run in the same
 // goroutine after Run returns, so reading s.httpServer is free of any
 // data race. Nothing here measures elapsed time.
 //
 // On the ReadTimeout >= ReadHeaderTimeout relationship: a smaller
 // ReadTimeout does NOT make the header phase unreachable. net/http's
 // (*Server).readHeaderTimeout applies ReadHeaderTimeout directly, so
 // the header read keeps its full budget. What breaks is the
 // whole-request deadline: once the headers are read, readRequest
 // installs a read deadline of t0+ReadTimeout, which is already in the
 // past when ReadTimeout is the smaller value, severing the request.
 // Verified against the pinned go1.25 net/http (Dockerfile golang
 // 1.25-alpine; go.mod go 1.25.5): src/net/http/server.go readRequest
 // and (*Server).readHeaderTimeout.
 func TestRunWiresSocketTimeouts(t *testing.T) {
 	// Sets an env var and touches viper global state, so like the
 	// config tests it cannot use t.Parallel.
 	viper.Reset()
 	t.Setenv("DNSWATCHER_TARGETS", "example.com")
 	srv := buildServer(t)
 	server.SetListenPort(srv, -1)
 	srv.Run()
 	hs := server.HTTPServerOf(srv)
 	if hs == nil {
 		t.Fatal("Run did not build an http.Server")
 	}
 	if hs.ReadTimeout <= 0 {
 		t.Errorf("ReadTimeout must be non-zero, got %v", hs.ReadTimeout)
 	}
 	if hs.ReadHeaderTimeout <= 0 {
 		t.Errorf(
 			"ReadHeaderTimeout must be non-zero, got %v",
-			srv.ReadHeaderTimeout,
+			hs.ReadHeaderTimeout,
 		)
 	}
-	if srv.WriteTimeout <= 0 {
+	if hs.WriteTimeout <= 0 {
-		t.Errorf(
+		t.Errorf("WriteTimeout must be non-zero, got %v", hs.WriteTimeout)
 			"WriteTimeout must be non-zero, got %v",
 			srv.WriteTimeout,
 		)
 	}
-	if srv.IdleTimeout <= 0 {
+	if hs.IdleTimeout <= 0 {
-		t.Errorf(
+		t.Errorf("IdleTimeout must be non-zero, got %v", hs.IdleTimeout)
 			"IdleTimeout must be non-zero, got %v",
 			srv.IdleTimeout,
 		)
 	}
 }
-// TestWriteTimeoutExceedsHandlerBudget pins the one relationship the
+	if hs.WriteTimeout <= server.RequestTimeout {
 // values must satisfy. net/http arms the write deadline once request
 // headers are read, so it covers handler execution plus the response
 // flush. If WriteTimeout were not greater than the chimw.Timeout
 // handler budget, the connection would be severed before a handler
 // that used its full budget could respond, making that budget
 // unreachable.
 func TestWriteTimeoutExceedsHandlerBudget(t *testing.T) {
 	t.Parallel()
 	srv := server.NewHTTPServer(":8080", noopHandler())
 	if srv.WriteTimeout <= server.RequestTimeout {
 		t.Errorf(
 			"WriteTimeout (%v) must exceed handler budget (%v)",
-			srv.WriteTimeout,
+			hs.WriteTimeout,
 			server.RequestTimeout,
 		)
 	}
 }
-// TestReadTimeoutCoversHeaderTimeout asserts the read deadline for
+	if hs.ReadTimeout < hs.ReadHeaderTimeout {
 // the whole request is at least as long as the header-only deadline;
 // a smaller ReadTimeout would make ReadHeaderTimeout unreachable.
 func TestReadTimeoutCoversHeaderTimeout(t *testing.T) {
 	t.Parallel()
 	srv := server.NewHTTPServer(":8080", noopHandler())
 	if srv.ReadTimeout < srv.ReadHeaderTimeout {
 		t.Errorf(
 			"ReadTimeout (%v) must be >= ReadHeaderTimeout (%v)",
-			srv.ReadTimeout,
+			hs.ReadTimeout,
-			srv.ReadHeaderTimeout,
+			hs.ReadHeaderTimeout,
 		)
 	}
 	if hs.Handler != srv {
 		t.Errorf(
 			"Run wired handler %T, want the *server.Server",
 			hs.Handler,
 		)
 	}
 }
 // TestHTTPServerAddrAndHandler covers the rest of the constructor so
 // a future edit cannot drop the listen address or the handler.
 func TestHTTPServerAddrAndHandler(t *testing.T) {
 	t.Parallel()
 	srv := server.NewHTTPServer(":9999", noopHandler())
 	if srv.Addr != ":9999" {
 		t.Errorf("Addr = %q, want %q", srv.Addr, ":9999")
 	}
 	if srv.Handler == nil {
 		t.Error("Handler must not be nil")
 	}
 }