feat: add WithRequestBodyTimeout to bound slow request bodies#2465
feat: add WithRequestBodyTimeout to bound slow request bodies#2465iliaal wants to merge 3 commits into
Conversation
ce3a248 to
1537e4a
Compare
henderkes
left a comment
There was a problem hiding this comment.
Generally good, but the option is never actually registered in ServeHTTP, so it's never actually taking effect?
| require.Contains(t, string(resp), "200 OK") | ||
| // PHP saw an empty body: php://input read zero bytes. | ||
| require.Contains(t, string(resp), "read=0") |
There was a problem hiding this comment.
do we want to silently pass a failed read to php as if everything was fine and then expect 200 back? I'm a bit confused here.
|
The option does take effect at the library level: it sets |
go_read_post blocks in request.Body.Read with no per-read deadline, so a slow client that announces a body and then stalls holds a PHP thread for the lifetime of the connection. With a bounded thread pool this is a slow-POST denial of service. WithRequestBodyTimeout sets an idle timeout: the read deadline is reset before every read, so a steady upload of any size still succeeds while a stalled one is cut off and the thread is released. Zero (the default) keeps the previous blocking behaviour. The timeout uses http.ResponseController.SetReadDeadline; when the ResponseWriter cannot expose a deadline the read proceeds without one.
Wire WithRequestBodyTimeout into the Caddy module so the body timeout is
configurable from a Caddyfile, not only the library API:
php {
request_body_timeout 5s
}
4929917 to
35f90d5
Compare
alexandre-daubois
left a comment
There was a problem hiding this comment.
Good to me as well!
|
LGTM too, but:
|
The directive existed but defaulted to disabled and was undocumented. Enable it by default at the Caddy layer with a 60s idle timeout, matching nginx's client_body_timeout, so slow-POST protection is on out of the box. RequestBodyTimeout becomes *caddy.Duration so an omitted directive (apply the 60s default) is distinguishable from an explicit `request_body_timeout 0` (disable). The library-level WithRequestBodyTimeout zero value stays disabled: embedders opt in, only Caddyfile/JSON deployments get the default. Document the directive in config.md and add a slow-request-body entry to the security model.
|
Both done in
Worth calling out in the release notes: a deployment that doesn't set the directive starts enforcing the 60s idle timeout after upgrading. |
Problem
go_read_postloops onrequest.Body.Readwith no per-read deadline. A client that announces a body (or a largeContent-Length) then dribbles or stalls holds a PHP thread for the lifetime of the connection. With a bounded thread pool,max_threadssuch connections exhaust the pool and legitimate requests get rejected atmax_wait_time. This is a slow-POST DoS. It's bounded and recoverable, not a code defect, but the default posture is weaker than the common nginx+php-fpm stack, which buffers the body and ships a defaultclient_body_timeout.What this adds
WithRequestBodyTimeout(d)sets an idle timeout on body reads: the deadline resets before each read viahttp.ResponseController.SetReadDeadline, so a steady upload of any size succeeds while a stalled one is cut off and the thread is released. Best effort: if theResponseWritercan't expose a deadline, the read proceeds without one.The
request_body_timeoutCaddyfile directive (and its JSON equivalent onphp/php_server) exposes the same control. It now defaults to 60s, matching nginx'sclient_body_timeout, so slow-POST protection is on without configuration. Setrequest_body_timeout 0to disable. The library-levelWithRequestBodyTimeoutzero value still disables it: embedders opt in, only Caddyfile and JSON deployments get the default.Behavior change: a
php_serverorphpdeployment that doesn't set the directive starts enforcing a 60s idle timeout on request body reads after upgrading. The timeout is idle, not total, so a steady upload of any size still completes; only a stalled body is cut off.Docs: the directive is documented in
docs/config.md, anddocs/security.mdgains a slow-request-body entry under the in-scope attack surface.Why not just Caddy
read_timeoutread_timeoutmaps tohttp.Server.ReadTimeout, a total deadline for the entire request including the body, set server-wide. That's the wrong tool for three reasons, which is why it's off by default:read_timeout(ReadTimeout)WithRequestBodyTimeoutIt also does nothing for library users of frankenphp, who have no Caddy. For a deployment with no SSE and no large uploads,
read_timeoutis the simpler zero-code answer; this option is for the cases it can't express.