http.Server is missing ReadTimeout, WriteTimeout, and IdleTimeout #99

New Issue

clawbot · 2026-08-09T03:36:09+02:00

clawbot commented

2026-08-09 03:36:09 +02:00

REPO_POLICIES.md requires, before tagging 1.0, that HTTP services set ReadTimeout, ReadHeaderTimeout, WriteTimeout, and IdleTimeout on the http.Server. Only one of the four is set.

Current state (audited against `origin/main`, commit `9347a28`)

internal/server/server.go:84-88:

s.httpServer = &amp;http.Server{
    Addr:              listenAddr,
    Handler:           s,
    ReadHeaderTimeout: readHeaderTimeout,
}

readHeaderTimeout is 10 * time.Second (internal/server/server.go:36-37). ReadTimeout, WriteTimeout, and IdleTimeout are all absent, so they default to zero — meaning no limit.

Consequences: a client can hold a connection open indefinitely once it is past the header phase, responses have no write deadline, and keep-alive connections are never reaped. This is straightforward slowloris / idle-connection resource exhaustion on a service that is intended to be exposed to the internet.

Note the global chimw.Timeout(60 * time.Second) at internal/server/routes.go:14-15,26 bounds handler execution, which is a different control — it does not bound socket-level read/write/idle time and does not close the connection.

Definition of done

internal/server/server.go sets all four fields on the http.Server literal: ReadTimeout, ReadHeaderTimeout, WriteTimeout, IdleTimeout.
Each duration is a named const in the same file alongside the existing readHeaderTimeout, so the values are self-documenting rather than inline magic numbers.
The values are internally consistent with the existing 60s chimw.Timeout handler budget — WriteTimeout must be greater than the handler timeout, otherwise the server severs the connection before a legitimately slow handler can finish and the 60s budget becomes unreachable. State the chosen relationship in a comment.
A test in internal/server asserts that the constructed http.Server has all four fields set to non-zero values, so a future refactor cannot silently drop one.
README documents the timeout values if it documents server tunables; if they are not configurable, no env-var documentation is needed.
make check is green, and TODO.md is updated in the same commit as the work.

The finishing commit's title must end with (closes #N) referencing this issue.

Out of scope

Security response headers (#98), rate limiting, and CORS scoping are tracked separately — do not fold them into this PR.

`REPO_POLICIES.md` requires, before tagging 1.0, that HTTP services set `ReadTimeout`, `ReadHeaderTimeout`, `WriteTimeout`, and `IdleTimeout` on the `http.Server`. Only one of the four is set. ## Current state (audited against `origin/main`, commit `9347a28`) `internal/server/server.go:84-88`: ```go s.httpServer = &http.Server{ Addr: listenAddr, Handler: s, ReadHeaderTimeout: readHeaderTimeout, } ``` `readHeaderTimeout` is `10 * time.Second` (`internal/server/server.go:36-37`). `ReadTimeout`, `WriteTimeout`, and `IdleTimeout` are all absent, so they default to zero — meaning **no limit**. Consequences: a client can hold a connection open indefinitely once it is past the header phase, responses have no write deadline, and keep-alive connections are never reaped. This is straightforward slowloris / idle-connection resource exhaustion on a service that is intended to be exposed to the internet. Note the global `chimw.Timeout(60 * time.Second)` at `internal/server/routes.go:14-15,26` bounds *handler* execution, which is a different control — it does not bound socket-level read/write/idle time and does not close the connection. ## Definition of done 1. `internal/server/server.go` sets all four fields on the `http.Server` literal: `ReadTimeout`, `ReadHeaderTimeout`, `WriteTimeout`, `IdleTimeout`. 2. Each duration is a named `const` in the same file alongside the existing `readHeaderTimeout`, so the values are self-documenting rather than inline magic numbers. 3. The values are internally consistent with the existing 60s `chimw.Timeout` handler budget — `WriteTimeout` must be greater than the handler timeout, otherwise the server severs the connection before a legitimately slow handler can finish and the 60s budget becomes unreachable. State the chosen relationship in a comment. 4. A test in `internal/server` asserts that the constructed `http.Server` has all four fields set to non-zero values, so a future refactor cannot silently drop one. 5. README documents the timeout values if it documents server tunables; if they are not configurable, no env-var documentation is needed. 6. `make check` is green, and `TODO.md` is updated in the same commit as the work. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Out of scope Security response headers (#98), rate limiting, and CORS scoping are tracked separately — do not fold them into this PR.

clawbot added this to the 1.0 milestone 2026-08-09 03:36:09 +02:00

clawbot referenced this issue

2026-08-09 03:36:27 +02:00

CORS wildcard applies to the authenticated /metrics route, and advertises methods that do not exist #100

clawbot referenced this issue

2026-08-09 03:36:47 +02:00

No rate limiting on the Basic-Auth-protected /metrics endpoint #101

clawbot referenced this issue

2026-08-09 03:41:03 +02:00

internal/globals, internal/healthcheck, and internal/logger have no tests at all #110

clawbot referenced this issue

2026-08-09 03:42:01 +02:00

1.0/mvp #66

clawbot referenced this issue

2026-08-09 03:55:06 +02:00

feat: add security response headers middleware (closes #98) #112

clawbot referenced this issue

2026-08-09 07:35:34 +02:00

1.0/mvp #66

clawbot commented

2026-08-09 07:36:00 +02:00

Implementation plan

Branch fix/99-server-timeouts off origin/main (9347a28). All code changes confined to internal/server.

1. Constants in `internal/server/server.go`

Four named constants next to the existing shutdownTimeout, with a comment block explaining the relationship to the 60s chimw.Timeout(requestTimeout) handler budget in routes.go:

readHeaderTimeout = 10 * time.Second — unchanged.
readTimeout = 15 * time.Second — total header+body read deadline. Every route in this service is a GET with no request body, so 15s is already far beyond what any legitimate client needs; it exists purely to bound a slowloris body dribble. Must be >= readHeaderTimeout, and 5s of headroom past it is plenty.
writeTimeout = 75 * time.Second — must exceed the 60s handler budget. In net/http the write deadline for a plaintext connection is set once the request headers are read, so it covers handler execution and the response flush. If it were <= 60s the socket would be severed before a handler that legitimately used its full chimw.Timeout budget could emit a response, making that budget unreachable. 60s + 15s of flush headroom.
idleTimeout = 120 * time.Second — keep-alive reaping. The only clients are browsers on the dashboard and a Prometheus scraper; 120s sits above the common scrape intervals (15s/30s/60s) so the scraper's connection is reused rather than re-handshaked every cycle, while an abandoned connection is still reaped inside two minutes.

2. Construction

Extract the http.Server literal into a small unexported newHTTPServer(listenAddr string, handler http.Handler) *http.Server in server.go, called from Run(). This keeps the literal in server.go (DoD 1) while making the configured value testable without binding a socket.

3. Test

New internal/server/export_test.go (matching the repo's existing export_test.go convention in internal/handlers and internal/notify) exporting newHTTPServer and the requestTimeout value, plus internal/server/server_test.go in package server_test asserting:

all four fields are non-zero (DoD 4);
WriteTimeout > requestTimeout, so a future edit to either number cannot silently break the invariant;
ReadTimeout >= ReadHeaderTimeout.

These assert on configured field values only — no timing/duration behaviour is measured, so the test cannot flake.

4. Docs

The timeouts are compile-time constants, not env vars, so per DoD 5 nothing goes in the README env-var table. I will add a one-line note in the README's HTTP/architecture prose stating the four socket timeouts and the handler-budget relationship, so the values are discoverable. TODO.md updated in the same commit as the work.

Out of scope, not touched

Security headers (#98 / PR #112), rate limiting (#100), http.MaxBytesReader (#101), CORS scoping. No changes to routes.go, internal/middleware, internal/watcher, internal/resolver, or .golangci.yml.

Verification

make check (green before PR), GOFLAGS=-count=1 make test run 10x consecutively under -race, and docker build --no-cache . to force a real script/cibuild run rather than a cached layer (#115).

## Implementation plan Branch `fix/99-server-timeouts` off `origin/main` (`9347a28`). All code changes confined to `internal/server`. ### 1. Constants in `internal/server/server.go` Four named constants next to the existing `shutdownTimeout`, with a comment block explaining the relationship to the 60s `chimw.Timeout(requestTimeout)` handler budget in `routes.go`: - `readHeaderTimeout = 10 * time.Second` — unchanged. - `readTimeout = 15 * time.Second` — total header+body read deadline. Every route in this service is a `GET` with no request body, so 15s is already far beyond what any legitimate client needs; it exists purely to bound a slowloris body dribble. Must be >= `readHeaderTimeout`, and 5s of headroom past it is plenty. - `writeTimeout = 75 * time.Second` — **must exceed the 60s handler budget.** In `net/http` the write deadline for a plaintext connection is set once the request headers are read, so it covers handler execution *and* the response flush. If it were <= 60s the socket would be severed before a handler that legitimately used its full `chimw.Timeout` budget could emit a response, making that budget unreachable. 60s + 15s of flush headroom. - `idleTimeout = 120 * time.Second` — keep-alive reaping. The only clients are browsers on the dashboard and a Prometheus scraper; 120s sits above the common scrape intervals (15s/30s/60s) so the scraper's connection is reused rather than re-handshaked every cycle, while an abandoned connection is still reaped inside two minutes. ### 2. Construction Extract the `http.Server` literal into a small unexported `newHTTPServer(listenAddr string, handler http.Handler) *http.Server` in `server.go`, called from `Run()`. This keeps the literal in `server.go` (DoD 1) while making the configured value testable without binding a socket. ### 3. Test New `internal/server/export_test.go` (matching the repo's existing `export_test.go` convention in `internal/handlers` and `internal/notify`) exporting `newHTTPServer` and the `requestTimeout` value, plus `internal/server/server_test.go` in `package server_test` asserting: - all four fields are non-zero (DoD 4); - `WriteTimeout > requestTimeout`, so a future edit to either number cannot silently break the invariant; - `ReadTimeout >= ReadHeaderTimeout`. These assert on configured field values only — no timing/duration behaviour is measured, so the test cannot flake. ### 4. Docs The timeouts are compile-time constants, not env vars, so per DoD 5 nothing goes in the README env-var table. I will add a one-line note in the README's HTTP/architecture prose stating the four socket timeouts and the handler-budget relationship, so the values are discoverable. `TODO.md` updated in the same commit as the work. ### Out of scope, not touched Security headers (#98 / PR #112), rate limiting (#100), `http.MaxBytesReader` (#101), CORS scoping. No changes to `routes.go`, `internal/middleware`, `internal/watcher`, `internal/resolver`, or `.golangci.yml`. ### Verification `make check` (green before PR), `GOFLAGS=-count=1 make test` run 10x consecutively under `-race`, and `docker build --no-cache .` to force a real `script/cibuild` run rather than a cached layer (#115).

clawbot referenced this issue from a commit

2026-08-09 07:42:04 +02:00

server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99)

clawbot referenced a pull request that will close this issue

2026-08-09 07:42:27 +02:00

server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99) #118

clawbot referenced this issue

2026-08-09 07:44:52 +02:00

fmt tooling is incomplete: fmt-check does not verify goimports, and no formatter covers Markdown #119

clawbot referenced this issue

2026-08-09 07:52:15 +02:00

server: set ReadTimeout, WriteTimeout, and IdleTimeout (closes #99) #118