fix(httpx): honor -pr http11 by disabling retryablehttp h2 fallback#2414
fix(httpx): honor -pr http11 by disabling retryablehttp h2 fallback#2414cococlaw wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
Neo Security AuditNo security issues found Hardening Notes
Comment |
|
No actionable comments were generated in the recent review. 🎉 WalkthroughWhen Protocol is set to HTTP11, HTTPX now reuses the primary HTTP/1.1 client for HTTPClient2 instead of creating a separate HTTP/2 probing client; for other protocols the prior behavior (creating a separate http2.Transport and client2) remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HTTPX_Init as HTTPX Init
participant Client1 as PrimaryClient (retryable)
participant Client2 as SecondaryClient
participant Transport2 as http2.Transport
rect rgba(0,128,0,0.5)
User->>HTTPX_Init: create with Protocol=="http11"
HTTPX_Init->>Client1: create primary HTTP/1.1 client
HTTPX_Init->>Client2: assign Client2 = Client1 (reuse)
Client2-->>User: returns clients (same underlying transport)
end
rect rgba(0,0,128,0.5)
User->>HTTPX_Init: create with Protocol!="http11"
HTTPX_Init->>Client1: create primary client (retryable)
HTTPX_Init->>Transport2: build http2.Transport with TLS config
HTTPX_Init->>Client2: create secondary client using Transport2
Client1-->>User: returns primary client
Client2-->>User: returns secondary (http2-capable) client
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/httpx/httpx.go (1)
191-204:⚠️ Potential issue | 🟠 Major
httpx.client2is still initialized withhttp2.Transporteven whenProtocol == HTTP11— incomplete fix.The PR correctly patches
retryablehttp.Client.HTTPClient2(line 185–188), setting it to use HTTP/1.1 when the protocol is forced. However,httpx.client2— the HTTPX struct's own*http.Clientfield — is created unconditionally withhttp2.Transport(lines 191–204) without checkinghttpx.Options.Protocol.When
SupportHTTP2(protocol, method, targetURL)is called during HTTP/2 probing on HTTPS targets, it receives the scheme ("https") and dispatches throughh.client2.Do(req)(http2.go:55), which uses HTTP/2. This violates the user's explicit HTTP/1.1 constraint.Fix: Apply the same conditional logic to
client2initialization:if httpx.Options.Protocol != HTTP11 { httpx.client2 = &http.Client{ Transport: transport2, Timeout: httpx.Options.Timeout, } }Or, when
Protocol == HTTP11, initializeclient2with the same HTTP/1.1-only transport used for the main client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/httpx/httpx.go` around lines 191 - 204, The httpx.client2 is always built with an http2.Transport which ignores the user's HTTP/1.1 override; update the client2 initialization to honor httpx.Options.Protocol by either (a) only assigning an http2.Transport when httpx.Options.Protocol != HTTP11, or (b) when httpx.Options.Protocol == HTTP11, initialize httpx.client2 with the same HTTP/1.1-only transport used for the main client (i.e., mirror the logic applied to retryablehttp.Client.HTTPClient2). Locate the client2 construction around the http2.Transport block and modify it to check httpx.Options.Protocol (compare against HTTP11) so SupportHTTP2 and h.client2.Do(req) will not force HTTP/2 when the user requested HTTP/1.1.
🧹 Nitpick comments (1)
common/httpx/httpx_protocol_test.go (1)
9-17: LGTM — assertion and test logic are correct.
require.Samecorrectly validates pointer identity afterHTTPClient2 = HTTPClient. One minor note:New()allocates afastdialer.Dialer(and other resources) that are never released. Consider adding at.Cleanupto closeh.Dialerif the dialer exposes aClose()method, to avoid resource leaks in the test binary.🧹 Suggested cleanup
h, err := New(&options) require.NoError(t, err) +t.Cleanup(func() { _ = h.Dialer.Close() }) require.NotNil(t, h.client) require.Same(t, h.client.HTTPClient, h.client.HTTPClient2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/httpx/httpx_protocol_test.go` around lines 9 - 17, Test allocates resources (fastdialer.Dialer) that aren't released; add a cleanup to close the dialer. In the TestNewHTTP11DisablesRetryableHTTP2FallbackClient test, after successfully creating h via New(&options) register t.Cleanup to call Close on h.Dialer (or the appropriate Close/CloseIdleConnections method exposed by the fastdialer.Dialer) so the dialer and any associated resources are released when the test finishes; reference the New function, the returned h value and its h.Dialer field when adding the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@common/httpx/httpx.go`:
- Around line 191-204: The httpx.client2 is always built with an http2.Transport
which ignores the user's HTTP/1.1 override; update the client2 initialization to
honor httpx.Options.Protocol by either (a) only assigning an http2.Transport
when httpx.Options.Protocol != HTTP11, or (b) when httpx.Options.Protocol ==
HTTP11, initialize httpx.client2 with the same HTTP/1.1-only transport used for
the main client (i.e., mirror the logic applied to
retryablehttp.Client.HTTPClient2). Locate the client2 construction around the
http2.Transport block and modify it to check httpx.Options.Protocol (compare
against HTTP11) so SupportHTTP2 and h.client2.Do(req) will not force HTTP/2 when
the user requested HTTP/1.1.
---
Nitpick comments:
In `@common/httpx/httpx_protocol_test.go`:
- Around line 9-17: Test allocates resources (fastdialer.Dialer) that aren't
released; add a cleanup to close the dialer. In the
TestNewHTTP11DisablesRetryableHTTP2FallbackClient test, after successfully
creating h via New(&options) register t.Cleanup to call Close on h.Dialer (or
the appropriate Close/CloseIdleConnections method exposed by the
fastdialer.Dialer) so the dialer and any associated resources are released when
the test finishes; reference the New function, the returned h value and its
h.Dialer field when adding the cleanup.
|
Implemented the follow-up for the CodeRabbit major feedback (plus cleanup in tests) in commit |
|
Quick merge-readiness follow-up:\n\n- CodeRabbit + Neo are both green\n- the follow-up fix for http11 fallback/probe consistency is ready\n- helper PR for upstream-fork apply/cherry-pick remains available: https://github.com/cococlaw/httpx/pull/1\n\nIf this looks good, could a maintainer take a final pass and merge/cherry-pick? 🙏 |
|
Implemented the follow-up from the CodeRabbit major feedback in commit |
|
gentle nudge for the follow-up fix 🙏\n\nI implemented the follow-up addressing the CodeRabbit feedback (HTTP/1.1 path now aligns fallback/probe clients and added protocol coverage tests) and pushed it on #2414 (branch fix/algora-2240-http11-no-h2-fallback). Could you please give this a quick review when you have a moment? |
3 participants
Fixes #2240
/claim #2240
Summary
-pr http11is set.Why
When users explicitly force HTTP/1.1, protocol fallback to HTTP/2 violates expectation and can bypass transport policy differences.
Validation
GOTOOLCHAIN=auto go test ./common/httpxSummary by CodeRabbit
Bug Fixes
Tests