Skip to content

Commit 53c4851

Browse files
radimsemclaude
andauthored
fix(mcp): set http server timeouts to close pre-auth slowloris hole (#214)
## Summary - Adds `ReadHeaderTimeout` (10s), `ReadTimeout` (60s), `IdleTimeout` (120s), and `MaxHeaderBytes` (1 MiB) to the MCP HTTP server, sourced from named constants in the existing `const(...)` block alongside `httpShutdownTimeout`. - Leaves `WriteTimeout` at its zero value (intentional, commented): the streamable HTTP transport keeps responses open for long-running POSTs and SSE GETs; a server-wide cap would truncate them. Slow-read defense belongs in the streaming handler's per-write deadlines. - Adds `TestRunHttp_ReadHeaderTimeout_ClosesSlowClient` — opens a TCP conn, writes a partial request line, asserts the server closes the connection within `ReadHeaderTimeout + 3s` and rejects false positives where the client-side read deadline (set well beyond the server's window) fires first. Closes #205. ## Test plan - [x] `make build` clean - [x] `make test` green (38 packages; `pkg/mcp` 10.7s incl. new Slowloris regression test) - [x] `gofmt -l pkg/mcp/` empty - [x] `go vet ./...` clean - [x] Codex review converged at pass 3 (caught the `WriteTimeout` streaming-truncation issue + the test client-deadline false-positive — both fixed before merge) - [x] `go-style-reviewer` subagent: zero findings against `.claude/rules/go-concise.md` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent efcb9a5 commit 53c4851

2 files changed

Lines changed: 78 additions & 4 deletions

File tree

pkg/mcp/http.go

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,13 @@ import (
1414
)
1515

1616
const (
17-
httpShutdownTimeout = 5 * time.Second
18-
bearerPrefix = "Bearer "
19-
bearerRealm = `Bearer realm="remindb"`
17+
httpReadHeaderTimeout = 10 * time.Second
18+
httpReadTimeout = 60 * time.Second
19+
httpIdleTimeout = 120 * time.Second
20+
httpMaxHeaderBytes = 1 << 20 // 1 MiB
21+
httpShutdownTimeout = 5 * time.Second
22+
bearerPrefix = "Bearer "
23+
bearerRealm = `Bearer realm="remindb"`
2024
)
2125

2226
func (s *Server) runHttp(ctx context.Context) error {
@@ -48,7 +52,17 @@ func (s *Server) runHttp(ctx context.Context) error {
4852
if s.authToken != "" {
4953
handler = bearerAuthMiddleware(s.authToken, handler)
5054
}
51-
httpSrv := &http.Server{Handler: handler}
55+
// WriteTimeout is intentionally left at the zero value: streamable HTTP
56+
// keeps responses open for long-running POSTs and SSE GETs, and a
57+
// server-wide cap would truncate them. Slow-read defense is left to the
58+
// streaming handler's own per-write deadlines.
59+
httpSrv := &http.Server{
60+
Handler: handler,
61+
ReadHeaderTimeout: httpReadHeaderTimeout,
62+
ReadTimeout: httpReadTimeout,
63+
IdleTimeout: httpIdleTimeout,
64+
MaxHeaderBytes: httpMaxHeaderBytes,
65+
}
5266

5367
s.logger.Info("serve: HTTP transport ready", "listen", addr, "auth", s.authToken != "")
5468

pkg/mcp/http_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package mcp
33
import (
44
"bytes"
55
"context"
6+
"errors"
67
"log/slog"
78
"net"
89
"net/http"
@@ -204,6 +205,65 @@ func TestRunHttp_WithAuthToken_EndToEnd(t *testing.T) {
204205
}
205206
}
206207

208+
func TestRunHttp_ReadHeaderTimeout_ClosesSlowClient(t *testing.T) {
209+
if testing.Short() {
210+
t.Skipf("skipping Slowloris regression in -short mode (waits ~%s)", httpReadHeaderTimeout)
211+
}
212+
213+
ln, err := net.Listen("tcp", "127.0.0.1:0")
214+
if err != nil {
215+
t.Fatalf("listen: %v", err)
216+
}
217+
addr := ln.Addr().String()
218+
219+
srv := newHttpTestServer(t, ln)
220+
221+
ctx, cancel := context.WithCancel(context.Background())
222+
done := make(chan error, 1)
223+
go func() { done <- srv.Run(ctx) }()
224+
t.Cleanup(func() {
225+
cancel()
226+
<-done
227+
})
228+
229+
time.Sleep(50 * time.Millisecond)
230+
231+
conn, err := net.DialTimeout("tcp", addr, time.Second)
232+
if err != nil {
233+
t.Fatalf("dial: %v", err)
234+
}
235+
defer func() { _ = conn.Close() }()
236+
237+
if _, err := conn.Write([]byte("GET / HTTP/1.1\r\n")); err != nil {
238+
t.Fatalf("write partial: %v", err)
239+
}
240+
241+
// Client deadline is intentionally much larger than the server's
242+
// ReadHeaderTimeout. If the client deadline fires first, the server
243+
// did not enforce its timeout and the test must fail — not pass on
244+
// the client-side timeout it accidentally observed.
245+
clientDeadline := httpReadHeaderTimeout + 10*time.Second
246+
if err := conn.SetReadDeadline(time.Now().Add(clientDeadline)); err != nil {
247+
t.Fatalf("set deadline: %v", err)
248+
}
249+
250+
buf := make([]byte, 256)
251+
start := time.Now()
252+
_, readErr := conn.Read(buf)
253+
elapsed := time.Since(start)
254+
255+
var netErr net.Error
256+
if errors.As(readErr, &netErr) && netErr.Timeout() {
257+
t.Fatalf("client deadline fired after %v before server closed — server did not enforce ReadHeaderTimeout", elapsed)
258+
}
259+
if readErr == nil {
260+
t.Fatalf("read returned nil err after %v — server did not enforce ReadHeaderTimeout", elapsed)
261+
}
262+
if elapsed > httpReadHeaderTimeout+3*time.Second {
263+
t.Errorf("server closed after %v, want within %v", elapsed, httpReadHeaderTimeout+3*time.Second)
264+
}
265+
}
266+
207267
func TestRunHttp_NonLoopbackInsecurePublic_LogsWarn(t *testing.T) {
208268
ln, err := net.Listen("tcp", "0.0.0.0:0")
209269
if err != nil {

0 commit comments

Comments
 (0)