-
Notifications
You must be signed in to change notification settings - Fork 5k
fetch: make the response-header phase an absolute deadline #36146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 The proxy-tunnel path still re-arms the idle timer on every dripped header byte:
on_dataat src/http/lib.rs:3833 callsself.set_timeout(&socket)unconditionally before dispatching toreceive()→handle_on_data_headers, so the removal ofset_timeoutfromshort_read!()is moot when the origin is behind an HTTPS CONNECT proxy. The non-tunnel arm (3847-3849) already skips the re-arm forPending|Headers; gating the re-arm at 3833 onself.state.response_stage != HTTPStage::ProxyHeaders(mirroring that structure) closes the sibling site — otherwise the same drip attack still pins the request forever via a proxy, and the new comment claiming the timer "stays monotonic for the whole response-header phase" is false on that path.Extended reasoning...
What the bug is
The PR converts the response-header phase from an idle timer into an absolute deadline by removing
self.set_timeout(&socket)from theshort_read!()macro inhandle_on_data_headers, so partial header reads no longer re-arm the socket timer. The new comment (src/http/lib.rs:3627-3632) documents the invariant: "the timer armed at request-write time stays monotonic for the whole response-header phase".However,
handle_on_data_headershas two entry points, and the fix only covers one. For a request routed through an HTTPS CONNECT proxy, encrypted TLS records from the origin arrive atHTTPClient::on_data(lib.rs:3819), which at lines 3831-3835 does:receive()decrypts and, whenresponse_stage == ProxyHeaders, dispatches tohandle_on_data_headers(src/http/ProxyTunnel.rs:340-352). So the outeron_datahas already re-armed the idle timer beforeshort_read!()ever runs — the removal ofset_timeoutfrom the macro has no effect on this path.Why the non-tunnel path is different
Contrast the direct (non-tunnel) branch of the same function at lines 3847-3893: the
match self.state.response_stagesendsPending | Headersstraight tohandle_on_data_headerswithout re-arming (line 3849); only theBody(3853) andBodyChunk(3871) arms callset_timeout. The tunnel branch at 3833 re-arms unconditionally regardless of response stage, so the two entry points are now asymmetric.Step-by-step proof
Consider an origin behind an HTTPS CONNECT proxy, with
BUN_CONFIG_HTTP_IDLE_TIMEOUT=5, that drips one header line per second (the exact scenario in the new test, but through a proxy):HTTP/1.1 200 OK\r\ninside a TLS record → arrives aton_data(lib.rs:3819).proxy_tunnel.is_some()→ line 3833 callsself.set_timeout(&socket)→ timer re-armed to 5s.receive()decrypts →response_stage == ProxyHeaders→handle_on_data_headers(ProxyTunnel.rs:345/349).Status::Partial→short_read!()persists the tail and returns (no longer re-arms — but step 3 already did).X-Drip-0: v\r\n→ back to step 2. Timer re-armed again.The direct-connection variant of this exact sequence is what the PR fixes and tests; the tunneled variant is untouched.
Impact
An origin (or misbehaving upstream) reachable via an HTTPS proxy can drip one header line per
<idle-timeout>and pin the request, its socket, and a request-cap slot indefinitely — precisely the resource-pinning hole described in the PR body, on the sibling variant. The added comment now misdocuments the invariant for tunneled requests. Per REVIEW.md — "Fix the whole class in the same PR — grep for every sibling site sharing the pattern… SSL/non-SSL variants… If a site is intentionally excluded, say so in the PR" — this is required scope, and the PR description does not mention excluding the proxy path.Suggested fix
Gate the re-arm at line 3833 on the response stage, mirroring the non-tunnel
match:(
ProxyHandshakecould be excluded too for symmetry, though the handshake completes before header parsing so it's less material.) The newset_timeoutafterhandle_response_metadata(line 3737) already covers the header→body boundary for both paths, so body-phase re-arm is preserved.