sneak/pixa
1
0
Fork 0
Code Issues 11 Pull Requests 4 Actions 11 Packages Projects Releases Wiki Activity

Blocks 1.0: http.Server missing IdleTimeout and ReadHeaderTimeout (slowloris), no request body size limits #92

New Issue
Closed
opened 2026-08-09 07:02:42 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 07:02:42 +02:00
Collaborator
Copy Link
Copy Source

From the audit against the canonical REPO_POLICIES.md. Independently verified against main at 61f42e6.

1. Missing server timeouts. The policy requires:

> ReadTimeout and ReadHeaderTimeout on the http.Server to defend against slowloris attacks. ... IdleTimeout on the http.Server.

internal/server/http.go:16-24 constructs the server with Addr, ReadTimeout (30 s), WriteTimeout (60 s), MaxHeaderBytes (8 KiB), and Handler — and nothing else. Grep for IdleTimeout|ReadHeaderTimeout across internal/ and cmd/ returns zero matches — verified.

This is not covered by the existing settings. ReadTimeout bounds the whole request read but does not specifically bound the header phase, which is the slowloris dribble the policy names. With no IdleTimeout, a keep-alive connection is held open indefinitely — so a trivial number of idle connections can exhaust the accept path on a service explicitly targeting high concurrency.

2. No application-enforced request body limit. The policy requires:

> Maximum request body size enforced on all endpoints (e.g. Go http.MaxBytesReader). Choose a sane default per-route; never accept unbounded input.

Grep for MaxBytesReader returns zero matches. POST / and POST /generate both call r.ParseForm() (internal/handlers/auth.go:38,86) with no wrapper.

Stated honestly: these routes are not literally unbounded — Go's ParseForm applies its own 10 MB cap to URL-encoded bodies. The divergence is that the limit is incidental rather than chosen, and it would silently evaporate if a handler ever switched to io.ReadAll or multipart. Lower severity than item 1, but it belongs in the same pass since both are HTTP-server hardening.

Definition of done

  1. HTTPIdleTimeout and HTTPReadHeaderTimeout constants added alongside the existing ones (internal/server/http.go:10-14) and set on the http.Server. Choose values deliberately — ReadHeaderTimeout should be short (seconds), IdleTimeout longer than typical keep-alive reuse but bounded.
  2. http.MaxBytesReader (or middleware) applies an explicit per-route body limit on POST / and POST /generate; exceeding it returns 413, not a generic 400.
  3. Consider whether WriteTimeout of 60 s is right given large image responses over slow links — flag if it needs raising, but do not change it without saying why.
  4. Tests asserting the configured timeout values and that an oversized body is rejected.
  5. make check green.

Priority

Blocks 1.0.0 for item 1 per the policy's pre-1.0 hardening section. Item 2 is cleanup. internal/server/http.go is untouched by both open PRs.

From the audit against the canonical `REPO_POLICIES.md`. Independently verified against `main` at `61f42e6`. **1. Missing server timeouts.** The policy requires: > `ReadTimeout` and `ReadHeaderTimeout` on the `http.Server` to defend against slowloris attacks. ... `IdleTimeout` on the `http.Server`. `internal/server/http.go:16-24` constructs the server with `Addr`, `ReadTimeout` (30 s), `WriteTimeout` (60 s), `MaxHeaderBytes` (8 KiB), and `Handler` — and nothing else. Grep for `IdleTimeout|ReadHeaderTimeout` across `internal/` and `cmd/` returns zero matches — verified. This is not covered by the existing settings. `ReadTimeout` bounds the whole request read but does not specifically bound the header phase, which is the slowloris dribble the policy names. With no `IdleTimeout`, a keep-alive connection is held open indefinitely — so a trivial number of idle connections can exhaust the accept path on a service explicitly targeting high concurrency. **2. No application-enforced request body limit.** The policy requires: > Maximum request body size enforced on all endpoints (e.g. Go `http.MaxBytesReader`). Choose a sane default per-route; never accept unbounded input. Grep for `MaxBytesReader` returns zero matches. `POST /` and `POST /generate` both call `r.ParseForm()` (`internal/handlers/auth.go:38,86`) with no wrapper. **Stated honestly:** these routes are not literally unbounded — Go's `ParseForm` applies its own 10 MB cap to URL-encoded bodies. The divergence is that the limit is incidental rather than chosen, and it would silently evaporate if a handler ever switched to `io.ReadAll` or multipart. Lower severity than item 1, but it belongs in the same pass since both are HTTP-server hardening. ## Definition of done 1. `HTTPIdleTimeout` and `HTTPReadHeaderTimeout` constants added alongside the existing ones (`internal/server/http.go:10-14`) and set on the `http.Server`. Choose values deliberately — `ReadHeaderTimeout` should be short (seconds), `IdleTimeout` longer than typical keep-alive reuse but bounded. 2. `http.MaxBytesReader` (or middleware) applies an explicit per-route body limit on `POST /` and `POST /generate`; exceeding it returns 413, not a generic 400. 3. Consider whether `WriteTimeout` of 60 s is right given large image responses over slow links — flag if it needs raising, but do not change it without saying why. 4. Tests asserting the configured timeout values and that an oversized body is rejected. 5. `make check` green. ## Priority **Blocks 1.0.0** for item 1 per the policy's pre-1.0 hardening section. Item 2 is cleanup. `internal/server/http.go` is untouched by both open PRs.
clawbot added this to the 1.0.0 milestone 2026-08-09 07:02:42 +02:00
clawbot commented 2026-09-21 09:25:19 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan (base next, branch issue-92-server-hardening):

  1. internal/server/http.go: add HTTPReadHeaderTimeout (10s, bounds the header dribble specifically) and HTTPIdleTimeout (120s, bounds keep-alive reuse) constants and set both on the http.Server. Extract the server construction into a small helper so a test can assert every timeout field without binding a listener.
  2. Request body limits: a MaxFormBytes cap (1 MiB) applied to the two form POSTs (POST /, POST /generate) by wrapping the body in http.MaxBytesReader before ParseForm; exceeding it returns 413, not the generic form-error page. 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.
  3. WriteTimeout (60s): reviewed per the definition of done. Leaving it unchanged; will flag on the PR whether it needs raising for large images over slow links rather than changing it silently.
  4. TDD per CLAUDE.md: failing tests first (timeout values; oversized body rejected), committed separately, then the implementation.

Model: opus-4-8

Plan (base `next`, branch `issue-92-server-hardening`): 1. `internal/server/http.go`: add `HTTPReadHeaderTimeout` (10s, bounds the header dribble specifically) and `HTTPIdleTimeout` (120s, bounds keep-alive reuse) constants and set both on the `http.Server`. Extract the server construction into a small helper so a test can assert every timeout field without binding a listener. 2. Request body limits: a `MaxFormBytes` cap (1 MiB) applied to the two form POSTs (`POST /`, `POST /generate`) by wrapping the body in `http.MaxBytesReader` before `ParseForm`; exceeding it returns 413, not the generic form-error page. 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. 3. `WriteTimeout` (60s): reviewed per the definition of done. Leaving it unchanged; will flag on the PR whether it needs raising for large images over slow links rather than changing it silently. 4. TDD per `CLAUDE.md`: failing tests first (timeout values; oversized body rejected), committed separately, then the implementation. Model: opus-4-8
clawbot commented 2026-09-21 20:29:41 +02:00
Author
Collaborator
Copy Link
Copy Source

Implemented in #123 (base next).

  • Added HTTPReadHeaderTimeout (10s) and HTTPIdleTimeout (120s) on the
    http.Server: the header-read phase (slowloris) and idle keep-alive reuse
    are now bounded, not only the whole-request read.
  • Added a LimitBody middleware capping POST / and POST /generate at
    1 MiB and returning 413. It runs ahead of the CSRF middleware, which reads
    its token from the form body, so an oversized body is refused as 413 rather
    than looking like a missing token (403). Covered by tests.
  • Left WriteTimeout at 60s (definition-of-done item 3): it also caps
    image-response write time, and whether it should rise for large images over
    slow links is a load decision for you.

make check passes via the local Docker builder gate; hosted CI is down for
an infrastructure reason.

Model: opus-4-8

Implemented in https://git.eeqj.de/sneak/pixa/pulls/123 (base `next`). - Added `HTTPReadHeaderTimeout` (10s) and `HTTPIdleTimeout` (120s) on the `http.Server`: the header-read phase (slowloris) and idle keep-alive reuse are now bounded, not only the whole-request read. - Added a `LimitBody` middleware capping `POST /` and `POST /generate` at 1 MiB and returning 413. It runs ahead of the CSRF middleware, which reads its token from the form body, so an oversized body is refused as 413 rather than looking like a missing token (403). Covered by tests. - Left `WriteTimeout` at 60s (definition-of-done item 3): it also caps image-response write time, and whether it should rise for large images over slow links is a load decision for you. `make check` passes via the local Docker builder gate; hosted CI is down for an infrastructure reason. Model: opus-4-8
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#92