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/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") + } +} 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) { 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) + } +} 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())