fix(http11): prevent retryablehttp HTTP/2 fallback from bypassing -pr http11 (#2240)#2447
Open
TheAuroraAI wants to merge 2 commits intoprojectdiscovery:devfrom
Open
fix(http11): prevent retryablehttp HTTP/2 fallback from bypassing -pr http11 (#2240)#2447TheAuroraAI wants to merge 2 commits intoprojectdiscovery:devfrom
TheAuroraAI wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
… http11 When `-pr http11` is specified, httpx correctly disables HTTP/2 on the main transport (GODEBUG=http2client=0, TLSNextProto cleared). However, retryablehttp-go's do.go falls back to HTTPClient2 — a separate HTTP/2 client — whenever it receives a "malformed HTTP version" error from a server that speaks HTTP/2 natively. This bypass caused the `-pr http11` flag to be silently ignored in practice (projectdiscovery#2240). Fix: after constructing the retryablehttp client, replace its HTTPClient2 with the already-configured HTTP/1.1-only HTTPClient when Protocol == "http11". This neutralises the fallback without requiring any upstream changes to retryablehttp-go and without affecting HTTP/2 detection (client2/SupportHTTP2) for non-restricted clients. Adds a unit test that asserts HTTPClient2 == HTTPClient in http11 mode and HTTPClient2 != HTTPClient in default mode. Fixes projectdiscovery#2240
Neo - PR Security ReviewNo security issues found Highlights
Comment |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe pull request enforces HTTP/1.1-only mode by assigning the primary HTTP/1.1 client to the HTTP/2 fallback client when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/httpx/httpx_test.go`:
- Around line 14-28: The test mutates the process-global GODEBUG via New, making
the second subtest flaky; capture the original GODEBUG at the start of each
subtest (or before the first one), set or let the subtest call New as needed,
and then restore the original GODEBUG (e.g., with defer os.Setenv("GODEBUG",
orig) or os.Unsetenv if orig was empty) so the "http11 mode" and "default mode"
cases run with independent global state; update the subtests around the calls to
New (referencing New and DefaultOptions and the ht.client assertions) to save
and restore GODEBUG.
- Around line 20-23: The test is using value equality but needs pointer-identity
assertions: replace require.Equal/NotEqual checks on ht.client.HTTPClient and
ht.client.HTTPClient2 with require.Same/require.NotSame to assert they are the
same (or different) instance as described in the comment; update the assertion
calls that reference ht.client.HTTPClient and ht.client.HTTPClient2 accordingly
so the test verifies pointer identity rather than value equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f28db00a-c6cd-4bf9-b865-e92623c44ee7
📒 Files selected for processing (2)
common/httpx/httpx.gocommon/httpx/httpx_test.go
Comment on lines
+14
to
+28
| t.Run("http11 mode disables HTTPClient2 fallback", func(t *testing.T) { | ||
| opts := DefaultOptions | ||
| opts.Protocol = "http11" | ||
| ht, err := New(&opts) | ||
| require.Nil(t, err) | ||
|
|
||
| // When Protocol == "http11", HTTPClient2 must point to the same underlying | ||
| // client as HTTPClient so the retryablehttp-go fallback is neutralised. | ||
| require.Equal(t, ht.client.HTTPClient, ht.client.HTTPClient2, | ||
| "HTTPClient2 must equal HTTPClient in http11 mode to prevent HTTP/2 fallback") | ||
| }) | ||
|
|
||
| t.Run("default mode keeps distinct HTTPClient2", func(t *testing.T) { | ||
| ht, err := New(&DefaultOptions) | ||
| require.Nil(t, err) |
There was a problem hiding this comment.
Restore GODEBUG between these subtests.
Line 17 can mutate a process-global env var via New, so the "default mode" case is currently running after a polluted global state. Make each subtest restore GODEBUG so the test stays order-independent.
Suggested fix
import (
"net/http"
+ "os"
"testing"
"github.com/projectdiscovery/retryablehttp-go"
"github.com/stretchr/testify/require"
)
@@
func TestHTTP11ProtocolEnforcement(t *testing.T) {
t.Run("http11 mode disables HTTPClient2 fallback", func(t *testing.T) {
+ t.Setenv("GODEBUG", os.Getenv("GODEBUG"))
opts := DefaultOptions
opts.Protocol = "http11"
ht, err := New(&opts)
require.Nil(t, err)
@@
t.Run("default mode keeps distinct HTTPClient2", func(t *testing.T) {
+ t.Setenv("GODEBUG", os.Getenv("GODEBUG"))
ht, err := New(&DefaultOptions)
require.Nil(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/httpx/httpx_test.go` around lines 14 - 28, The test mutates the
process-global GODEBUG via New, making the second subtest flaky; capture the
original GODEBUG at the start of each subtest (or before the first one), set or
let the subtest call New as needed, and then restore the original GODEBUG (e.g.,
with defer os.Setenv("GODEBUG", orig) or os.Unsetenv if orig was empty) so the
"http11 mode" and "default mode" cases run with independent global state; update
the subtests around the calls to New (referencing New and DefaultOptions and the
ht.client assertions) to save and restore GODEBUG.
…ween subtests
- Replace require.Equal/NotEqual with require.Same/NotSame for pointer
identity semantics (HTTPClient2 vs HTTPClient instance comparison)
- Add t.Setenv("GODEBUG", ...) to each subtest to prevent GODEBUG
mutation in one subtest from polluting the next
Author
|
Addressed CodeRabbit review:
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Root Cause
When
-pr http11is specified, httpx correctly disables HTTP/2 on the main transport:GODEBUG=http2client=0transport.TLSNextProtoHowever,
retryablehttp-go'sdo.gohas an unconditional fallback at line 63–64:HTTPClient2is a separate*http.Clientconfigured withgolang.org/x/net/http2.ConfigureTransport. Because it is set independently of the HTTP/1.1-onlyHTTPClient, this fallback silently bypasses the-pr http11restriction whenever a server responds with HTTP/2 framing.Fix
After constructing the
retryablehttpclient, whenProtocol == "http11", replace itsHTTPClient2with the already-configured HTTP/1.1-onlyHTTPClient:This ensures the retryablehttp fallback also honours the HTTP/1.1-only constraint without requiring any upstream changes to
retryablehttp-goand without affectingclient2/SupportHTTP2for non-restricted clients.Tests
Added
TestHTTP11ProtocolEnforcementwith two sub-tests:http11 mode disables HTTPClient2 fallback— assertsHTTPClient2 == HTTPClientwhenProtocol == "http11"default mode keeps distinct HTTPClient2— asserts the two clients differ in default modeChecklist
devbranchProtocol == "http11"go build ./...passesSummary by CodeRabbit
Bug Fixes
Tests