Repository navigation
fix(httpx): close connection on 101 Switching Protocols responses (#2708) - #2716
Conversation
…ojectdiscovery#2708) When wrapping httpresp.Body with io.LimitReader for MaxResponseBodySizeToRead, using io.NopCloser discarded the underlying Closer implementation. When shouldSkipBodyRead is true (such as on 101 Switching Protocols or 304 Not Modified), the deferred body drain is skipped to avoid hanging on websocket duplex streams, and subsequent calls to httpresp.Body.Close() called the nop closer, leaving the upgraded connection/socket open indefinitely. Wrap the limited reader in a readCloser that preserves and delegates to the underlying Closer, and ensure httpresp.Body.Close() is deferred when skipping body reads so the connection is always closed. Signed-off-by: TassioSales <tassiolucian.ljs@gmail.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Close response bodies common/httpx/httpx.go, common/httpx/httpx_test.go |
HTTPX.Do closes response bodies when it skips reading them and retains the underlying closer for limited bodies. The new test checks that a 101 response body is closed with limits of 0, 1024, and 2 MiB. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- projectdiscovery/httpx#2705: Both PRs change behavior related to
MaxResponseBodySizeToRead.
Suggested reviewers: mzack9999
Merge Risk: 
c2eca
-
The response-body closure change is mergeable after normal checks. The identified 304 timeout behavior is pre-existing.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the main change: httpx's Do now closes the connection for 101 Switching Protocols responses. The raw summary shows Do wraps capped bodies in a readCloser that closes the original body,… Linked Issues check [ #2708] Requirement: Do must close the connection of a 101 Switching Protocols response when body reads are skipped.
Base behavior: with MaxResponseBodySizeToRead > 0, httpresp.Body was wrapped in io…Out of Scope Changes check All source changes are in common/httpx/httpx.go and concern response body closing for skipped reads, which is the subject of [ #2708]. The new readCloser type and its Close method exist only to keep th…Full details: Title check
-
Explanation
The title names the main change: httpx's Do now closes the connection for 101 Switching Protocols responses. The raw summary shows Do wraps capped bodies in a readCloser that closes the original body, and closes bodies when reads are skipped. The new test checks that Do closes the body for 101 responses at each size limit. The title is specific and matches the changeset, so it passes.
Full details: Linked Issues check
-
Explanation
[
#2708] Requirement: Do must close the connection of a 101 Switching Protocols response when body reads are skipped.Base behavior: with MaxResponseBodySizeToRead > 0, httpresp.Body was wrapped in io.NopCloser(io.LimitReader(...)). The explicit httpresp.Body.Close() call after the skip check then closed only the NopCloser, which did nothing. The upgraded connection stayed open.
Head behavior (common/httpx/httpx.go, Do):
- With a positive limit, the body is wrapped in readCloser{Reader: io.LimitReader(origBody, ...), Closer: origBody}. Close() delegates to origBody. The existing explicit httpresp.Body.Close() after the read section now closes the upgraded body for 101 and 304 responses.
- With a zero limit, the body is not wrapped. The explicit Close() already closed the raw body in the base code. This behavior is unchanged.
- New defers call httpresp.Body.Close() when shouldSkipBodyRead is true, in both the positive-limit and zero-limit branches. These cover early error returns before the explicit Close() call, such as the DumpResponseHeadersAndRaw error return.
Double Close calls are safe. readCloser sets Closer to nil after the first close, and later close errors are discarded with _ =.
Automated tests: the test file diff could not be read because the changed blobs were missing from the checkout. The PR description and the raw summary report tests in common/httpx/httpx_test.go for 101 responses with limits 0, 1024, and 2 MiB. I did not verify these tests. The PASS result rests on the source diff in httpx.go. The issue does not request tests.
Full details: Out of Scope Changes check
-
Explanation
All source changes are in common/httpx/httpx.go and concern response body closing for skipped reads, which is the subject of [
#2708]. The new readCloser type and its Close method exist only to keep the body closeable after the size cap. The added defers cover the same skipped-body path. No unrelated behavior changes appear in the diff.Non-scope observations, not failures: the diff adds an extra blank line before type readCloser, which gofmt would remove. The PR description contains a truncated identifier ("eadCloser" for readCloser). The test file diff could not be read, so its contents are not assessed. The tests reported in the PR description would support the linked objective.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the response stream,
Then closes what was left unseen.
Through limits small and limits wide,
The body’s closer stays inside.
The 101 now shuts its door.
Proposed Changes
Fixes #2708
Problem
When capping the response body reader with io.LimitReader under h.Options.MaxResponseBodySizeToRead > 0, httpresp.Body was wrapped in io.NopCloser. Wrapping in io.NopCloser discarded the underlying Closer implementation.
When shouldSkipBodyRead is true (such as on 101 Switching Protocols or 304 Not Modified), the deferred body copy was intentionally skipped to avoid hanging on websocket duplex streams. However, because httpresp.Body was replaced with io.NopCloser, subsequent calls to httpresp.Body.Close() (and deferred cleanup) were no-ops, leaving the upgraded connection/socket open indefinitely.
Solution
eadCloser struct that preserves and delegates Close() to the underlying response closer.
Summary by CodeRabbit