From 302c092cced3d0e8e7b029bcf193abfb7bf6daf0 Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Mon, 8 Jun 2026 23:58:36 +0300 Subject: [PATCH 1/2] Fix panic on GET /chunked from integer overflow HttpServerTimeout is a time.Duration (nanoseconds); the default-delay branch scaled it by time.Second again, overflowing int64 to a negative value and passing it to rand.Intn, which panics. The bare /chunked route (no wait param) hit this on every request. Convert the timeout to whole seconds in a guarded helper that keeps rand.Intn's argument positive, and add a regression test. --- pkg/api/http/chunked.go | 14 +++++++++++++- pkg/api/http/chunked_test.go | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/pkg/api/http/chunked.go b/pkg/api/http/chunked.go index 8438cee..d758d25 100644 --- a/pkg/api/http/chunked.go +++ b/pkg/api/http/chunked.go @@ -26,7 +26,7 @@ func (s *Server) chunkedHandler(w http.ResponseWriter, r *http.Request) { delay, err := strconv.Atoi(vars["wait"]) if err != nil { - delay = rand.Intn(int(s.config.HttpServerTimeout*time.Second)-10) + 10 + delay = randomDelaySeconds(s.config.HttpServerTimeout) } flusher, ok := w.(http.Flusher) @@ -46,3 +46,15 @@ func (s *Server) chunkedHandler(w http.ResponseWriter, r *http.Request) { flusher.Flush() } + +// randomDelaySeconds returns a random delay in seconds within [10, timeout), +// used when no explicit wait is provided. timeout is a time.Duration, so it is +// converted to whole seconds; the upper bound is clamped to keep rand.Intn's +// argument positive (it panics on a non-positive argument). +func randomDelaySeconds(timeout time.Duration) int { + maxDelay := int(timeout / time.Second) + if maxDelay <= 11 { + maxDelay = 12 + } + return rand.Intn(maxDelay-10) + 10 +} diff --git a/pkg/api/http/chunked_test.go b/pkg/api/http/chunked_test.go index 167b3b1..d753438 100644 --- a/pkg/api/http/chunked_test.go +++ b/pkg/api/http/chunked_test.go @@ -5,6 +5,7 @@ import ( "net/http/httptest" "regexp" "testing" + "time" ) func TestChunkedHandler(t *testing.T) { @@ -33,3 +34,19 @@ func TestChunkedHandler(t *testing.T) { rr.Body.String(), expected) } } + +// TestRandomDelaySeconds covers the default-delay branch taken by the bare +// /chunked route (no {wait} value). This used to panic because the +// duration-to-seconds math overflowed int64 and handed rand.Intn a negative +// argument. Every timeout must yield a valid delay in [10, max] without panicking. +func TestRandomDelaySeconds(t *testing.T) { + timeouts := []time.Duration{30 * time.Second, 12 * time.Second, time.Second, 0, -1} + for _, timeout := range timeouts { + for range 100 { + d := randomDelaySeconds(timeout) + if d < 10 { + t.Fatalf("timeout %s: delay %d below floor of 10", timeout, d) + } + } + } +} From 5037beda6cba0e7946ce916a3a601007cf4c5841 Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Mon, 8 Jun 2026 23:58:43 +0300 Subject: [PATCH 2/2] Cap HTTP request and websocket message sizes Unauthenticated clients could exhaust process memory (and, for /store, disk) by posting arbitrarily large bodies, and could OOM or pin goroutines via the /ws/echo websocket. - Wrap request bodies in http.MaxBytesReader (10 MiB) on the echo, store, cache and token handlers via a shared readLimitedBody helper - Bound /ws/echo: per-message read limit, idle read deadline with ping/pong keepalive, and write deadlines - Add regression tests for both limits --- pkg/api/http/body_limit_test.go | 42 +++++++++++++++++++++++++++++++++ pkg/api/http/cache.go | 8 +++---- pkg/api/http/echo.go | 6 ++--- pkg/api/http/echows.go | 37 +++++++++++++++++++++++++---- pkg/api/http/echows_test.go | 38 +++++++++++++++++++++++++++++ pkg/api/http/http.go | 26 ++++++++++++++++++++ pkg/api/http/store.go | 8 +++---- pkg/api/http/token.go | 8 ++----- 8 files changed, 149 insertions(+), 24 deletions(-) create mode 100644 pkg/api/http/body_limit_test.go diff --git a/pkg/api/http/body_limit_test.go b/pkg/api/http/body_limit_test.go new file mode 100644 index 0000000..a495eeb --- /dev/null +++ b/pkg/api/http/body_limit_test.go @@ -0,0 +1,42 @@ +package http + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" +) + +// TestRequestBodySizeLimit verifies body-reading handlers reject payloads larger +// than maxRequestBodySize instead of buffering them into memory. The echo +// handler is used because with no backends configured it simply reflects the +// body and needs no external dependencies. +func TestRequestBodySizeLimit(t *testing.T) { + srv := NewMockServer() + srv.router.HandleFunc("/echo", srv.echoHandler) + + // A body within the limit is accepted (202). + within := httptest.NewRequest("POST", "/echo", bytes.NewReader(make([]byte, 1024))) + rr := httptest.NewRecorder() + srv.router.ServeHTTP(rr, within) + if rr.Code != http.StatusAccepted { + t.Errorf("within-limit body: got status %d want %d", rr.Code, http.StatusAccepted) + } + + // A body over the limit is rejected with a 413 code in the response body. + over := httptest.NewRequest("POST", "/echo", bytes.NewReader(make([]byte, maxRequestBodySize+1))) + rr = httptest.NewRecorder() + srv.router.ServeHTTP(rr, over) + + var resp struct { + Code int `json:"code"` + Message string `json:"message"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("oversize body: response is not the expected error JSON: %v (body=%q)", err, rr.Body.String()) + } + if resp.Code != http.StatusRequestEntityTooLarge { + t.Errorf("oversize body: got code %d want %d", resp.Code, http.StatusRequestEntityTooLarge) + } +} diff --git a/pkg/api/http/cache.go b/pkg/api/http/cache.go index e335ac5..13010f8 100644 --- a/pkg/api/http/cache.go +++ b/pkg/api/http/cache.go @@ -2,7 +2,6 @@ package http import ( "fmt" - "io" "net/http" "net/url" "time" @@ -33,15 +32,14 @@ func (s *Server) cacheWriteHandler(w http.ResponseWriter, r *http.Request) { } key := mux.Vars(r)["key"] - body, err := io.ReadAll(r.Body) - if err != nil { - s.ErrorResponse(w, r, span, "reading the request body failed", http.StatusBadRequest) + body, ok := s.readLimitedBody(w, r, span) + if !ok { return } conn := s.pool.Get() defer conn.Close() - _, err = conn.Do("SET", key, string(body)) + _, err := conn.Do("SET", key, string(body)) if err != nil { s.logger.Warn("cache set failed", zap.Error(err)) s.ErrorResponse(w, r, span, "cache set failed", http.StatusInternalServerError) diff --git a/pkg/api/http/echo.go b/pkg/api/http/echo.go index 56305d1..a3d35ef 100644 --- a/pkg/api/http/echo.go +++ b/pkg/api/http/echo.go @@ -27,10 +27,8 @@ func (s *Server) echoHandler(w http.ResponseWriter, r *http.Request) { ctx, span := s.tracer.Start(r.Context(), "echoHandler") defer span.End() - body, err := io.ReadAll(r.Body) - if err != nil { - s.logger.Error("reading the request body failed", zap.Error(err)) - s.ErrorResponse(w, r, span, "invalid request body", http.StatusBadRequest) + body, ok := s.readLimitedBody(w, r, span) + if !ok { return } defer r.Body.Close() diff --git a/pkg/api/http/echows.go b/pkg/api/http/echows.go index d06dfb7..d8afff2 100644 --- a/pkg/api/http/echows.go +++ b/pkg/api/http/echows.go @@ -12,6 +12,20 @@ import ( var wsCon = websocket.Upgrader{} +const ( + // wsMaxMessageSize caps a single inbound websocket message so one large + // frame cannot exhaust process memory. + wsMaxMessageSize = 1 << 20 // 1 MiB + // wsReadTimeout is how long the server waits for the next client message or + // pong before closing an idle connection. + wsReadTimeout = 60 * time.Second + // wsWriteTimeout bounds a single write so a slow reader cannot block forever. + wsWriteTimeout = 10 * time.Second + // wsPingInterval is how often the server pings the client to keep the + // connection alive and detect dead peers; it must be shorter than wsReadTimeout. + wsPingInterval = 30 * time.Second +) + // EchoWS godoc // @Summary Echo over websockets // @Description echos content via websockets @@ -24,11 +38,18 @@ var wsCon = websocket.Upgrader{} func (s *Server) echoWsHandler(w http.ResponseWriter, r *http.Request) { c, err := wsCon.Upgrade(w, r, nil) if err != nil { - if err != nil { - s.logger.Warn("websocket upgrade error", zap.Error(err)) - return - } + s.logger.Warn("websocket upgrade error", zap.Error(err)) + return } + + // Bound per-message size and idle time; refresh the read deadline whenever + // the client responds to a ping so live connections stay open. + c.SetReadLimit(wsMaxMessageSize) + _ = c.SetReadDeadline(time.Now().Add(wsReadTimeout)) + c.SetPongHandler(func(string) error { + return c.SetReadDeadline(time.Now().Add(wsReadTimeout)) + }) + var wg sync.WaitGroup wg.Add(1) @@ -84,15 +105,23 @@ func (s *Server) sendHostWs(ws *websocket.Conn, in chan interface{}, done chan s } func (s *Server) writeWs(ws *websocket.Conn, in chan interface{}) { + ping := time.NewTicker(wsPingInterval) + defer ping.Stop() for { select { case msg := <-in: + _ = ws.SetWriteDeadline(time.Now().Add(wsWriteTimeout)) if err := ws.WriteJSON(msg); err != nil { if !strings.Contains(err.Error(), "close") { s.logger.Warn("websocket write error", zap.Error(err)) } return } + case <-ping.C: + _ = ws.SetWriteDeadline(time.Now().Add(wsWriteTimeout)) + if err := ws.WriteMessage(websocket.PingMessage, nil); err != nil { + return + } } } } diff --git a/pkg/api/http/echows_test.go b/pkg/api/http/echows_test.go index be73f84..28b5856 100644 --- a/pkg/api/http/echows_test.go +++ b/pkg/api/http/echows_test.go @@ -4,6 +4,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/gorilla/websocket" ) @@ -34,3 +35,40 @@ func TestEchoWsHandler(t *testing.T) { t.Error("received empty message") } } + +// TestEchoWsReadLimit verifies the server caps inbound message size: a message +// larger than wsMaxMessageSize must cause the server to close the connection +// instead of buffering it into memory. +func TestEchoWsReadLimit(t *testing.T) { + srv := NewMockServer() + srv.router.HandleFunc("/ws/echo", srv.echoWsHandler) + server := httptest.NewServer(srv.router) + defer server.Close() + + wsURL := "ws" + strings.TrimPrefix(server.URL, "http") + "/ws/echo" + ws, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("websocket dial failed: %v", err) + } + defer ws.Close() + + // A message just over the limit must not be echoed; the server closes it. + oversize := make([]byte, wsMaxMessageSize+1) + if err := ws.WriteMessage(websocket.TextMessage, oversize); err != nil { + t.Fatalf("write failed: %v", err) + } + + // Read until the connection errors. With the read limit in place the server + // sends a 1009 (message too big) close frame; without it the server would + // instead echo the oversize payload back. The 5s deadline bounds the loop. + // Status frames from the periodic ticker (err == nil) are skipped. + ws.SetReadDeadline(time.Now().Add(5 * time.Second)) + for { + if _, _, err = ws.ReadMessage(); err != nil { + break + } + } + if !websocket.IsCloseError(err, websocket.CloseMessageTooBig) { + t.Errorf("expected close code %d (message too big), got: %v", websocket.CloseMessageTooBig, err) + } +} diff --git a/pkg/api/http/http.go b/pkg/api/http/http.go index a928655..7a09819 100644 --- a/pkg/api/http/http.go +++ b/pkg/api/http/http.go @@ -3,6 +3,8 @@ package http import ( "bytes" "encoding/json" + "errors" + "io" "math/rand" "net/http" "time" @@ -13,6 +15,11 @@ import ( "go.uber.org/zap" ) +// maxRequestBodySize caps how much of a request body the body-reading handlers +// buffer into memory. Without this bound an unauthenticated client can POST an +// arbitrarily large body and exhaust process memory (and, for /store, disk). +const maxRequestBodySize = 10 << 20 // 10 MiB + func randomErrorMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { rand.Seed(time.Now().Unix()) @@ -87,6 +94,25 @@ func (s *Server) ErrorResponse(w http.ResponseWriter, r *http.Request, span trac w.Write(prettyJSON(body)) } +// readLimitedBody reads the request body up to maxRequestBodySize. It returns +// the body and true on success. On an oversized body it writes a 413 response, +// on any other read error a 400, and returns false so the caller returns early. +func (s *Server) readLimitedBody(w http.ResponseWriter, r *http.Request, span trace.Span) ([]byte, bool) { + r.Body = http.MaxBytesReader(w, r.Body, maxRequestBodySize) + body, err := io.ReadAll(r.Body) + if err != nil { + var maxErr *http.MaxBytesError + if errors.As(err, &maxErr) { + s.ErrorResponse(w, r, span, "request body too large", http.StatusRequestEntityTooLarge) + return nil, false + } + s.logger.Error("reading the request body failed", zap.Error(err)) + s.ErrorResponse(w, r, span, "invalid request body", http.StatusBadRequest) + return nil, false + } + return body, true +} + // setRawResponseHeaders prevents XSS by ensuring browsers never interpret raw responses as HTML. func setRawResponseHeaders(w http.ResponseWriter) { w.Header().Set("Content-Type", "application/octet-stream") diff --git a/pkg/api/http/store.go b/pkg/api/http/store.go index 5f593bb..c1656be 100644 --- a/pkg/api/http/store.go +++ b/pkg/api/http/store.go @@ -3,7 +3,6 @@ package http import ( "crypto/sha1" "encoding/hex" - "io" "net/http" "os" "path" @@ -27,14 +26,13 @@ func (s *Server) storeWriteHandler(w http.ResponseWriter, r *http.Request) { _, span := s.tracer.Start(r.Context(), "storeWriteHandler") defer span.End() - body, err := io.ReadAll(r.Body) - if err != nil { - s.ErrorResponse(w, r, span, "reading the request body failed", http.StatusBadRequest) + body, ok := s.readLimitedBody(w, r, span) + if !ok { return } hash := hash(string(body)) - err = os.WriteFile(path.Join(s.config.DataPath, hash), body, 0644) + err := os.WriteFile(path.Join(s.config.DataPath, hash), body, 0644) if err != nil { s.logger.Warn("writing file failed", zap.Error(err), zap.String("file", path.Join(s.config.DataPath, hash))) s.ErrorResponse(w, r, span, "writing file failed", http.StatusInternalServerError) diff --git a/pkg/api/http/token.go b/pkg/api/http/token.go index d9747a3..29c20c0 100644 --- a/pkg/api/http/token.go +++ b/pkg/api/http/token.go @@ -2,13 +2,11 @@ package http import ( "fmt" - "io" "net/http" "strings" "time" "github.com/golang-jwt/jwt/v4" - "go.uber.org/zap" ) type jwtCustomClaims struct { @@ -28,10 +26,8 @@ func (s *Server) tokenGenerateHandler(w http.ResponseWriter, r *http.Request) { _, span := s.tracer.Start(r.Context(), "tokenGenerateHandler") defer span.End() - body, err := io.ReadAll(r.Body) - if err != nil { - s.logger.Error("reading the request body failed", zap.Error(err)) - s.ErrorResponse(w, r, span, "invalid request body", http.StatusBadRequest) + body, ok := s.readLimitedBody(w, r, span) + if !ok { return } defer r.Body.Close()