From 930e4eb0263ac8c6f5df011d2e97129ac4f814b5 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 18:22:22 +0000 Subject: [PATCH 1/4] test: assert http.Server carries ReadHeaderTimeout and IdleTimeout Failing test first: the server-hardening policy requires a slowloris defense (ReadHeaderTimeout) and a keep-alive bound (IdleTimeout) on the http.Server, neither of which is set today. The test asserts every timeout field is wired onto the constructed server. Model: opus-4-8 --- internal/server/http_internal_test.go | 65 +++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 internal/server/http_internal_test.go diff --git a/internal/server/http_internal_test.go b/internal/server/http_internal_test.go new file mode 100644 index 0000000..d94a380 --- /dev/null +++ b/internal/server/http_internal_test.go @@ -0,0 +1,65 @@ +package server + +import ( + "testing" + "time" + + "sneak.berlin/go/pixa/internal/config" +) + +// TestNewHTTPServerTimeouts verifies that the constructed http.Server +// carries every hardening timeout wired onto it, including the slowloris +// defense (ReadHeaderTimeout) and the keep-alive bound (IdleTimeout). This +// guards against a field being defined but never set on the server, so +// each assertion compares the server field to its constant. +func TestNewHTTPServerTimeouts(t *testing.T) { + t.Parallel() + + s := &Server{config: &config.Config{Port: 8080}} + + srv := s.newHTTPServer() + + fields := []struct { + name string + got time.Duration + want time.Duration + }{ + {"ReadTimeout", srv.ReadTimeout, HTTPReadTimeout}, + {"ReadHeaderTimeout", srv.ReadHeaderTimeout, HTTPReadHeaderTimeout}, + {"WriteTimeout", srv.WriteTimeout, HTTPWriteTimeout}, + {"IdleTimeout", srv.IdleTimeout, HTTPIdleTimeout}, + } + + for _, f := range fields { + if f.got != f.want { + t.Errorf("%s = %v, want %v", f.name, f.got, f.want) + } + } + + if srv.MaxHeaderBytes != HTTPMaxHeaderBytes { + t.Errorf("MaxHeaderBytes = %d, want %d", + srv.MaxHeaderBytes, HTTPMaxHeaderBytes) + } + + if srv.Handler != s { + t.Error("Handler is not the server") + } +} + +// TestHardeningTimeoutValues pins the intent behind the two new timeouts +// without hard-coding brittle exact durations: the header-read phase is +// bounded strictly shorter than the whole-request read (the slowloris +// dribble), and idle keep-alive connections are bounded rather than held +// open forever. +func TestHardeningTimeoutValues(t *testing.T) { + t.Parallel() + + if HTTPReadHeaderTimeout <= 0 || HTTPReadHeaderTimeout > HTTPReadTimeout { + t.Errorf("ReadHeaderTimeout = %v, want positive and <= ReadTimeout %v", + HTTPReadHeaderTimeout, HTTPReadTimeout) + } + + if HTTPIdleTimeout <= 0 { + t.Errorf("IdleTimeout = %v, want positive bound", HTTPIdleTimeout) + } +} -- 2.54.0 From 78cbb2617eda8cf824a5eab678bd508646579e20 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 18:22:22 +0000 Subject: [PATCH 2/4] feat: set ReadHeaderTimeout and IdleTimeout on the http.Server Add HTTPReadHeaderTimeout (10s) and HTTPIdleTimeout (120s) constants and wire them onto the server. ReadHeaderTimeout bounds the header-read phase specifically, dropping a slowloris client that dribbles headers; ReadTimeout alone bounds the whole request but not that phase. IdleTimeout bounds keep-alive reuse so idle connections cannot accumulate without limit. Server construction moves into newHTTPServer so the timeout configuration is assertable without binding a listener. Model: opus-4-8 --- internal/server/http.go | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/internal/server/http.go b/internal/server/http.go index cc904ec..bdd1238 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -9,24 +9,40 @@ import ( // HTTP server configuration constants. const ( - HTTPReadTimeout = 30 * time.Second - HTTPWriteTimeout = 60 * time.Second + HTTPReadTimeout = 30 * time.Second + // HTTPReadHeaderTimeout bounds the request-header read on its own, + // short, so a slowloris client dribbling headers is dropped well + // before it ties up a connection for the whole ReadTimeout window. + HTTPReadHeaderTimeout = 10 * time.Second + HTTPWriteTimeout = 60 * time.Second + // HTTPIdleTimeout bounds how long an idle keep-alive connection is + // held open, so idle connections cannot accumulate without limit on a + // service targeting high concurrency. + HTTPIdleTimeout = 120 * time.Second HTTPMaxHeaderBytes = 8 << 10 // 8KB ) -func (s *Server) serveUntilShutdown() { - listenAddr := fmt.Sprintf(":%d", s.config.Port) - s.httpServer = &http.Server{ - Addr: listenAddr, - ReadTimeout: HTTPReadTimeout, - WriteTimeout: HTTPWriteTimeout, - MaxHeaderBytes: HTTPMaxHeaderBytes, - Handler: s, +// newHTTPServer builds the http.Server with the hardening timeouts and +// limits applied. It is separate from serveUntilShutdown so the +// configuration can be asserted in a test without binding a listener. +func (s *Server) newHTTPServer() *http.Server { + return &http.Server{ + Addr: fmt.Sprintf(":%d", s.config.Port), + ReadTimeout: HTTPReadTimeout, + ReadHeaderTimeout: HTTPReadHeaderTimeout, + WriteTimeout: HTTPWriteTimeout, + IdleTimeout: HTTPIdleTimeout, + MaxHeaderBytes: HTTPMaxHeaderBytes, + Handler: s, } +} + +func (s *Server) serveUntilShutdown() { + s.httpServer = s.newHTTPServer() s.SetupRoutes() - s.log.Info("http begin listen", "listenaddr", listenAddr) + s.log.Info("http begin listen", "listenaddr", s.httpServer.Addr) err := s.httpServer.ListenAndServe() if err != nil && !errors.Is(err, http.ErrServerClosed) { -- 2.54.0 From c53e53b7b1d28e4460fd7d9fe1d30244b4762c63 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 18:22:22 +0000 Subject: [PATCH 3/4] test: reject oversized form POST body with 413 before CSRF parses it Failing test first for the per-route body limit. The key regression: an oversized POST / or POST /generate carrying an otherwise valid CSRF cookie and token must return 413. CSRF landed on next and reads its token from the form body, so if the limit ran after CSRF the truncated body would read as a missing token (403) and if it ran after the handler a valid token would succeed (303). 413 proves the limit fires first. Two within-limit tests guard against the limit disturbing a normal request. Model: opus-4-8 --- internal/handlers/bodylimit_internal_test.go | 177 +++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 internal/handlers/bodylimit_internal_test.go diff --git a/internal/handlers/bodylimit_internal_test.go b/internal/handlers/bodylimit_internal_test.go new file mode 100644 index 0000000..59e5fd7 --- /dev/null +++ b/internal/handlers/bodylimit_internal_test.go @@ -0,0 +1,177 @@ +package handlers + +import ( + "log/slog" + "net/http" + "net/url" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + + "sneak.berlin/go/pixa/internal/config" + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/session" +) + +// Form field names and a throwaway source image URL for the body-limit +// tests. +const ( + sourceURLField = "url" + testSourceURL = "https://example.com/a.jpg" +) + +// newBodyLimitTestRouter mirrors the production wiring for the form POST +// routes (see server.SetupRoutes): LimitBody sits in front of the CSRF +// middleware, which sits in front of the handlers. maxBytes is the body +// cap under test, so a test can trip the limit with a small body. +func newBodyLimitTestRouter( + t *testing.T, maxBytes int64, +) (*Handlers, http.Handler) { + t.Helper() + + cfg := &config.Config{SigningKey: testSigningKey, Debug: true} + + sessMgr, err := session.NewManager(testSigningKey) + if err != nil { + t.Fatalf("session.NewManager() error = %v", err) + } + + encGen, err := encurl.NewGenerator(testSigningKey) + if err != nil { + t.Fatalf("encurl.NewGenerator() error = %v", err) + } + + protect, err := newCSRFProtect(testSigningKey, cfg.Debug) + if err != nil { + t.Fatalf("newCSRFProtect() error = %v", err) + } + + h := &Handlers{ + log: slog.New(slog.DiscardHandler), + config: cfg, + sessMgr: sessMgr, + encGen: encGen, + csrfProtect: protect, + } + + r := chi.NewRouter() + r.Group(func(r chi.Router) { + r.Use(h.LimitBody(maxBytes)) + r.Use(h.CSRF()) + r.Get("/", h.HandleRoot()) + r.Post("/", h.HandleRoot()) + r.Post("/generate", h.HandleGenerateURL()) + }) + + return h, r +} + +// TestOversizedLoginPostRejectedBeforeCSRF is the core regression: an +// oversized POST / carrying an otherwise valid CSRF cookie and token must +// be rejected with 413. If the body limit ran after CSRF, the truncated +// body would read as a missing token and return 403; if it ran after the +// handler, a valid token would return 303. Getting 413 proves the limit +// fires before CSRF parses the form. +func TestOversizedLoginPostRejectedBeforeCSRF(t *testing.T) { + t.Parallel() + + _, srv := newBodyLimitTestRouter(t, 16) + + cookies, token := csrfCredentials(t, srv, nil) + + rec := postForm(srv, "/", cookies, url.Values{ + loginKeyField: {testSigningKey}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusRequestEntityTooLarge { + t.Errorf("oversized POST / status = %d, want %d", + rec.Code, http.StatusRequestEntityTooLarge) + } +} + +// TestOversizedGeneratePostRejectedBeforeCSRF is the same regression for +// POST /generate, which also parses a form behind CSRF. +func TestOversizedGeneratePostRejectedBeforeCSRF(t *testing.T) { + t.Parallel() + + h, srv := newBodyLimitTestRouter(t, 16) + + sessionCookie := newSessionCookie(t, h) + + cookies, token := csrfCredentials(t, srv, []*http.Cookie{sessionCookie}) + cookies = append(cookies, sessionCookie) + + rec := postForm(srv, "/generate", cookies, url.Values{ + sourceURLField: {testSourceURL}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusRequestEntityTooLarge { + t.Errorf("oversized POST /generate status = %d, want %d", + rec.Code, http.StatusRequestEntityTooLarge) + } +} + +// TestWithinLimitLoginPostSucceeds verifies the limit does not disturb a +// normal request: under the production cap, a valid login still parses and +// establishes a session (303). This guards against the body limit +// consuming or corrupting the form the CSRF check and handler depend on. +func TestWithinLimitLoginPostSucceeds(t *testing.T) { + t.Parallel() + + _, srv := newBodyLimitTestRouter(t, MaxFormBytes) + + cookies, token := csrfCredentials(t, srv, nil) + + rec := postForm(srv, "/", cookies, url.Values{ + loginKeyField: {testSigningKey}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusSeeOther { + t.Fatalf("within-limit POST / status = %d, want %d", + rec.Code, http.StatusSeeOther) + } + + var authed bool + + for _, c := range rec.Result().Cookies() { + if c.Name == session.CookieName && c.Value != "" { + authed = true + } + } + + if !authed { + t.Error("within-limit valid login did not set a session cookie") + } +} + +// TestWithinLimitGeneratePostSucceeds is the same non-regression check for +// POST /generate. +func TestWithinLimitGeneratePostSucceeds(t *testing.T) { + t.Parallel() + + h, srv := newBodyLimitTestRouter(t, MaxFormBytes) + + sessionCookie := newSessionCookie(t, h) + + cookies, token := csrfCredentials(t, srv, []*http.Cookie{sessionCookie}) + cookies = append(cookies, sessionCookie) + + rec := postForm(srv, "/generate", cookies, url.Values{ + sourceURLField: {testSourceURL}, + "format": {"jpeg"}, + csrfTokenField: {token}, + }) + + if rec.Code != http.StatusOK { + t.Fatalf("within-limit POST /generate status = %d, want %d", + rec.Code, http.StatusOK) + } + + if !strings.Contains(rec.Body.String(), "/v1/e/") { + t.Error("within-limit generate response did not contain a generated URL") + } +} -- 2.54.0 From 345a002cc285483674d80b93b95d92b0f41a09fb Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 18:22:22 +0000 Subject: [PATCH 4/4] feat: cap form POST body size and return 413 (closes #92) Add a LimitBody middleware that caps the request body at MaxFormBytes (1 MiB) on POST / and POST /generate and rejects an oversized body with 413. It parses the form under the cap before the CSRF middleware, which reads its token from the body with PostFormValue and would otherwise see a truncated body as a missing token (403); a successful parse is cached, so the CSRF check and handler reuse it. Wired ahead of CSRF in SetupRoutes. This makes the limit explicit rather than resting on ParseForm's incidental 10 MB cap, which would silently vanish if a handler switched to io.ReadAll or multipart. Model: opus-4-8 --- TODO.md | 8 ++++++ internal/handlers/bodylimit.go | 45 ++++++++++++++++++++++++++++++++++ internal/server/routes.go | 5 +++- 3 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 internal/handlers/bodylimit.go diff --git a/TODO.md b/TODO.md index c62db67..dab3310 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,14 @@ P1: implement blocked networks configuration to extend SSRF protection # Completed Steps +- 2026-09-21 http.Server hardening (closes #92): added + `HTTPReadHeaderTimeout` (10s, bounds the slowloris header dribble) and + `HTTPIdleTimeout` (120s, bounds keep-alive reuse) alongside the + existing timeouts and wired them onto the server; added a `LimitBody` + middleware capping the two form POST bodies (`POST /`, `POST /generate`) + at `MaxFormBytes` (1 MiB) and returning 413, applied ahead of the CSRF + middleware so an oversized body is refused as 413 rather than being read + as a missing CSRF token (403); left `WriteTimeout` at 60s unchanged - 2026-08-07 update golangci-lint to v2.12.2 with the canonical `.golangci.yml` (v2 schema, `default: all` minus six disabled linters, `lll` 88, tests included): bumped the pinned diff --git a/internal/handlers/bodylimit.go b/internal/handlers/bodylimit.go new file mode 100644 index 0000000..a2c9adc --- /dev/null +++ b/internal/handlers/bodylimit.go @@ -0,0 +1,45 @@ +package handlers + +import ( + "errors" + "net/http" +) + +// MaxFormBytes bounds the request body accepted on the HTML form POST +// routes (POST / and POST /generate). The forms carry a handful of short +// fields, so 1 MiB is generous while making the bound explicit rather than +// resting on ParseForm's incidental 10 MB cap. +const MaxFormBytes = 1 << 20 // 1 MiB + +// LimitBody returns middleware that caps the request body on POST requests +// at maxBytes and rejects an oversized body with 413 Request Entity Too +// Large. +// +// It parses the form here, before the CSRF middleware reads the token from +// it. The CSRF middleware reads the token with PostFormValue, which +// swallows a parse error, so if the body were only capped there an +// oversized body would read as a missing token and be refused as 403. By +// parsing under the cap first, an oversized body is refused as 413. A +// successful parse is cached on the request, so the CSRF check and the +// handler reuse it rather than reading the body again. +func (s *Handlers) LimitBody(maxBytes int64) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + r.Body = http.MaxBytesReader(w, r.Body, maxBytes) + + err := r.ParseForm() + + var tooLarge *http.MaxBytesError + if errors.As(err, &tooLarge) { + http.Error(w, "Request body too large", + http.StatusRequestEntityTooLarge) + + return + } + } + + next.ServeHTTP(w, r) + }) + } +} diff --git a/internal/server/routes.go b/internal/server/routes.go index 1fe054e..3b7dfc5 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -8,6 +8,7 @@ import ( "github.com/go-chi/chi/v5/middleware" "github.com/prometheus/client_golang/prometheus/promhttp" + "sneak.berlin/go/pixa/internal/handlers" "sneak.berlin/go/pixa/internal/static" ) @@ -46,8 +47,10 @@ func (s *Server) SetupRoutes() { // Login/generator UI. The form routes carry CSRF protection; the // token cookie is independent of the session cookie, so it also - // covers the login POST, where no session exists yet. + // covers the login POST, where no session exists yet. LimitBody caps + // the POST body ahead of CSRF, which reads its token from that body. s.router.Group(func(r chi.Router) { + r.Use(s.h.LimitBody(handlers.MaxFormBytes)) r.Use(s.h.CSRF()) r.Get("/", s.h.HandleRoot()) r.Post("/", s.h.HandleRoot()) -- 2.54.0