Set timeouts/limits on all request stages

cmd/meshtender/main.go

@@ -127,7 +127,10 @@ func run(logger *slog.Logger) error {
 	httpSrv := &http.Server{
 		Addr:              cfg.Addr,
 		Handler:           rec.Handler(srv.Handler()),
-		ReadHeaderTimeout: 10 * time.Second,
+		ReadHeaderTimeout: readHeaderTimeout,
+		ReadTimeout:       readTimeout,
+		WriteTimeout:      writeTimeout,
+		IdleTimeout:       idleTimeout,
 	}
 
 	tls := cfg.TLSCert != "" && cfg.TLSKey != ""
@@ -190,6 +193,32 @@ func run(logger *slog.Logger) error {
 // indexed statement, which isn't worth a second ticker to avoid.
 const janitorInterval = 5 * time.Minute
 
+// Connection timeouts. With all of these at zero (net/http's default) a slow or idle
+// peer can hold a connection open indefinitely after sending headers.
+//
+//   - readHeaderTimeout guards the classic slowloris: headers dribbled out forever.
+//   - readTimeout covers the body too. limitBody caps the SIZE at 1 MiB, but with no
+//     deadline a client could still take days to deliver it.
+//   - writeTimeout is deliberately generous. It has to cover a genuine download over a
+//     bad link — this product's users are on rural, marginal connections, and the
+//     largest asset is a few hundred KB — while still bounding a peer that stops
+//     reading mid-response.
+//   - idleTimeout reaps keep-alive connections between requests.
+//
+// The console WebSocket is unaffected. These become deadlines on the underlying
+// connection, which looks like it should sever an upgraded socket — but net/http clears
+// the deadline when a handler hijacks the connection (hijackLocked calls
+// rwc.SetDeadline(time.Time{})), so the socket inherits nothing and is bounded instead
+// by consoleIdleTimeout and the shutdown drain. That's the one thing these timeouts
+// could plausibly have broken, so
+// core.TestConsoleWebSocketOutlivesServerReadTimeout pins it.
+const (
+	readHeaderTimeout = 10 * time.Second
+	readTimeout       = 30 * time.Second
+	writeTimeout      = 120 * time.Second
+	idleTimeout       = 120 * time.Second
+)
+
 // janitorSweep is one periodic cleanup job: a name for the log, and the delete to
 // run. Taking a closure rather than an interface keeps the janitor decoupled from
 // the store (a store method value satisfies it directly) and lets the loop be

cmd/meshtender/main_test.go

@@ -135,6 +135,38 @@ func TestJanitorRunsEverySweep(t *testing.T) {
 	waitForCalls(t, healthy, 2, 2*time.Second)
 }
 
+// TestConnectionTimeoutsAreSet pins audit O1: every phase of a connection is bounded.
+// Zero means "no limit" in net/http, so a single omission silently reopens the hole
+// this closed, and nothing else would notice.
+func TestConnectionTimeoutsAreSet(t *testing.T) {
+	t.Parallel()
+	for _, c := range []struct {
+		name string
+		got  time.Duration
+	}{
+		{"readHeaderTimeout", readHeaderTimeout},
+		{"readTimeout", readTimeout},
+		{"writeTimeout", writeTimeout},
+		{"idleTimeout", idleTimeout},
+	} {
+		if c.got <= 0 {
+			t.Errorf("%s = %v; zero means unlimited, so a slow or idle peer can hold a "+
+				"connection open forever", c.name, c.got)
+		}
+	}
+
+	// Headers must not outlast the whole request they belong to.
+	if readHeaderTimeout > readTimeout {
+		t.Errorf("readHeaderTimeout (%v) exceeds readTimeout (%v)", readHeaderTimeout, readTimeout)
+	}
+	// The write budget has to cover a real download on a bad link. The largest asset is
+	// a few hundred KB, so anything under ~30s risks truncating legitimate responses for
+	// exactly the rural, marginal connections this product targets.
+	if writeTimeout < 30*time.Second {
+		t.Errorf("writeTimeout = %v; too tight for a large asset over a slow link", writeTimeout)
+	}
+}
+
 // TestJanitorIntervalOutlivesCodeTTL: the sweep only has to keep the table
 // small, but an interval shorter than the code TTL would mean pointless work, and a
 // wildly long one would defeat the purpose. Pin it to a sane band so a future edit

internal/core/ws_deadline_test.go

@@ -0,0 +1,118 @@
+package core
+
+import (
+	"context"
+	"crypto/rand"
+	"net/http"
+	"net/http/cookiejar"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/coder/websocket"
+	meshcore "github.com/meshcore-go/meshcore-go"
+
+	"github.com/jleight/meshtender/internal/auth"
+	"github.com/jleight/meshtender/internal/identity"
+	"github.com/jleight/meshtender/internal/store"
+)
+
+// TestConsoleWebSocketOutlivesServerReadTimeout pins the one thing audit O1's server
+// timeouts could plausibly have broken: a long-lived console session.
+//
+// http.Server's ReadTimeout/WriteTimeout/IdleTimeout become deadlines on the
+// underlying connection, which looks like it should sever an upgraded WebSocket the
+// moment one elapses. It doesn't — net/http clears the deadline when a handler hijacks
+// the connection (hijackLocked calls rwc.SetDeadline(time.Time{})), so the socket
+// inherits nothing and is bounded only by consoleIdleTimeout and the shutdown drain.
+//
+// That behaviour is load-bearing but belongs to the standard library, not to this
+// codebase, so it's worth an explicit test: it would catch a future WebSocket library
+// that re-arms deadlines after hijacking, or a change in Go's behaviour, either of
+// which would silently start cutting console sessions mid-command.
+//
+// The server here uses a deliberately tiny 250ms ReadTimeout rather than the
+// production 30s so the assertion takes a second instead of half a minute.
+func TestConsoleWebSocketOutlivesServerReadTimeout(t *testing.T) {
+	t.Parallel()
+	st, ctx := coreStore(t)
+
+	var masterKey [32]byte
+	if _, err := rand.Read(masterKey[:]); err != nil {
+		t.Fatalf("master key: %v", err)
+	}
+	idSvc, err := identity.LoadOrCreate(ctx, st, masterKey)
+	if err != nil {
+		t.Fatalf("identity: %v", err)
+	}
+	authSvc, err := auth.New(st, st.Pool(), testAuthConfig())
+	if err != nil {
+		t.Fatalf("auth: %v", err)
+	}
+	srv, err := NewServer(st, authSvc, idSvc, testConfig())
+	if err != nil {
+		t.Fatalf("server: %v", err)
+	}
+
+	// The production timeouts live in main.go; mirror their SHAPE here with a tiny
+	// read deadline so the hazard reproduces in milliseconds instead of 30 seconds.
+	const readTimeout = 250 * time.Millisecond
+	ts := httptest.NewUnstartedServer(srv.Handler())
+	ts.Config.ReadHeaderTimeout = readTimeout
+	ts.Config.ReadTimeout = readTimeout
+	ts.Config.WriteTimeout = readTimeout
+	ts.Config.IdleTimeout = readTimeout
+	ts.Start()
+	defer ts.Close()
+
+	jar, _ := cookiejar.New(nil)
+	user := seedSession(t, ts, st, ctx, jar, "wsdeadline")
+
+	repeater, err := meshcore.GenerateLocalIdentity(rand.Reader)
+	if err != nil {
+		t.Fatalf("repeater identity: %v", err)
+	}
+	rep, err := st.CreateRepeater(ctx, &store.Repeater{
+		OwnerID: user.ID, Name: "Deadline", PublicKeyHex: repeater.String(),
+		RadioFreqHz: 869525000, RadioBwHz: 250000, RadioSF: 11, RadioCR: 5,
+	})
+	if err != nil {
+		t.Fatalf("create repeater: %v", err)
+	}
+
+	wsURL := "ws" + strings.TrimPrefix(ts.URL, "http") + "/repeaters/" + rep.PublicID + "/console/ws"
+	hdr := http.Header{}
+	var parts []string
+	for _, c := range jar.Cookies(mustURL(t, ts.URL)) {
+		parts = append(parts, c.Name+"="+c.Value)
+	}
+	if len(parts) > 0 {
+		hdr.Set("Cookie", strings.Join(parts, "; "))
+	}
+
+	dctx, dcancel := context.WithTimeout(ctx, 5*time.Second)
+	defer dcancel()
+	ws, _, err := websocket.Dial(dctx, wsURL, &websocket.DialOptions{HTTPHeader: hdr})
+	if err != nil {
+		t.Fatalf("ws dial: %v", err)
+	}
+	defer ws.Close(websocket.StatusNormalClosure, "")
+
+	// Idle well past every deadline above, then use the socket. A surviving write and
+	// read prove the connection wasn't torn down by an inherited request deadline.
+	time.Sleep(4 * readTimeout)
+
+	wctx, wcancel := context.WithTimeout(ctx, 10*time.Second)
+	defer wcancel()
+	if err := ws.Write(wctx, websocket.MessageText, []byte(`{"type":"ready"}`)); err != nil {
+		t.Fatalf("write after %v idle failed — the socket inherited the server's read/write "+
+			"deadline instead of clearing it: %v", 4*readTimeout, err)
+	}
+	// The handler answers a "ready" with its tuning/status frames, so a successful read
+	// confirms the session is still live end to end, not just that the write buffered.
+	if _, _, err := ws.Read(wctx); err != nil {
+		t.Fatalf("read after %v idle failed — the session did not survive the server's "+
+			"request deadlines: %v", 4*readTimeout, err)
+	}
+}