fix: respect -pr http11 flag by preventing HTTP/2 fallback#2420
fix: respect -pr http11 flag by preventing HTTP/2 fallback#2420Bushi-gg wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Hardening Notes
Comment |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaced string protocol checks with the Changes
Sequence Diagram(s)mermaid 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
🧹 Nitpick comments (1)
common/httpx/httpx.go (1)
156-160: Consider consolidating the twoProtocol == HTTP11blocks.There are now two separate
if httpx.Options.Protocol == HTTP11checks at lines 156–160 and lines 188–190. The second block can't be merged with the first (sincehttpx.clientdoesn't exist yet at line 156), but it could carry a comment to that effect so it's clear why the split is intentional.Additionally, the issue (
#2240) explicitly suggested adding an upstreamOptions.DisableHTTPFallbackfield in retryablehttp-go as the cleaner long-term fix. AliasingHTTPClient2toHTTPClientis a functional workaround but couples httpx to the internal layout of retryablehttp-go's fallback path. If retryablehttp-go later adds aDisableHTTPFallbackoption, this workaround should be replaced.Also applies to: 188-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/httpx/httpx.go` around lines 156 - 160, There are two separate checks for httpx.Options.Protocol == HTTP11 (one that sets GODEBUG and transport.TLSNextProto and another later that aliases HTTPClient2) which is intentional because httpx.client doesn't exist yet in the first block; consolidate intent by adding a clear comment to the earlier block explaining why the second check is needed later, and update the later block (where HTTPClient2 is aliased to HTTPClient) to include a TODO referencing issue `#2240` and Prefer adding a DisableHTTPFallback option in retryablehttp-go; this keeps the current workaround (HTTPClient2 alias) but documents the coupling and the planned migration target (DisableHTTPFallback) so future maintainers know to replace the alias when that upstream option exists.
🤖 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 45-53: The test passes a pointer to the package-level
DefaultOptions into New (TestDefaultProtocolKeepsHTTP2Fallback), which New
stores and then mutates via parseCustomCookies, causing shared mutable state
across tests; fix by creating a local copy of DefaultOptions inside the test
(e.g., opts := DefaultOptions) and pass &opts to New so New and
httpx.Options.parseCustomCookies operate on a test-local struct instead of the
package-level DefaultOptions; do the same pattern for other tests like TestDo
that currently pass &DefaultOptions.
- Around line 32-43: TestHTTP11DisablesHTTP2Fallback leaks a global GODEBUG
change because New(&opts) calls os.Setenv; to prevent cross-test pollution
capture the current GODEBUG with prev := os.Getenv("GODEBUG") and after
constructing ht call t.Setenv("GODEBUG", prev) so the environment is restored
automatically by the test harness; reference TestHTTP11DisablesHTTP2Fallback and
New in your change (or alternatively remove the global os.Setenv in httpx.go as
a larger refactor).
---
Nitpick comments:
In `@common/httpx/httpx.go`:
- Around line 156-160: There are two separate checks for httpx.Options.Protocol
== HTTP11 (one that sets GODEBUG and transport.TLSNextProto and another later
that aliases HTTPClient2) which is intentional because httpx.client doesn't
exist yet in the first block; consolidate intent by adding a clear comment to
the earlier block explaining why the second check is needed later, and update
the later block (where HTTPClient2 is aliased to HTTPClient) to include a TODO
referencing issue `#2240` and Prefer adding a DisableHTTPFallback option in
retryablehttp-go; this keeps the current workaround (HTTPClient2 alias) but
documents the coupling and the planned migration target (DisableHTTPFallback) so
future maintainers know to replace the alias when that upstream option exists.
| func TestHTTP11DisablesHTTP2Fallback(t *testing.T) { | ||
| opts := DefaultOptions | ||
| opts.Protocol = HTTP11 | ||
|
|
||
| ht, err := New(&opts) | ||
| require.Nil(t, err) | ||
|
|
||
| // When http11 is requested, HTTPClient2 must point to the same client | ||
| // as HTTPClient so retryablehttp-go's fallback path does not switch to HTTP/2. | ||
| require.Same(t, ht.client.HTTPClient, ht.client.HTTPClient2, | ||
| "HTTPClient2 should equal HTTPClient when Protocol is HTTP11") | ||
| } |
There was a problem hiding this comment.
os.Setenv("GODEBUG", ...) side effect leaks across tests — use t.Setenv instead.
New(&opts) with Protocol == HTTP11 calls os.Setenv("GODEBUG", "http2client=0") (line 158 of httpx.go), which mutates the global process environment and is never restored. If TestHTTP11DisablesHTTP2Fallback runs before TestDo, it will disable HTTP/2 for the live requests in TestDo, potentially changing their behavior in a non-obvious way.
Replace the os.Setenv call in httpx.go with t.Setenv inside the test, or (more practically) clean up the env var after construction in the test:
🛡️ Proposed fix to restore the environment variable
func TestHTTP11DisablesHTTP2Fallback(t *testing.T) {
+ prevGodebug := os.Getenv("GODEBUG")
+ t.Cleanup(func() { os.Setenv("GODEBUG", prevGodebug) })
+
opts := DefaultOptions
opts.Protocol = HTTP11Alternatively, the os.Setenv call inside httpx.go should itself be wrapped in a mechanism that can be undone per-instance (e.g., only set when no previous value was present, or tracked for cleanup), but that's a broader refactor of the pre-existing code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestHTTP11DisablesHTTP2Fallback(t *testing.T) { | |
| opts := DefaultOptions | |
| opts.Protocol = HTTP11 | |
| ht, err := New(&opts) | |
| require.Nil(t, err) | |
| // When http11 is requested, HTTPClient2 must point to the same client | |
| // as HTTPClient so retryablehttp-go's fallback path does not switch to HTTP/2. | |
| require.Same(t, ht.client.HTTPClient, ht.client.HTTPClient2, | |
| "HTTPClient2 should equal HTTPClient when Protocol is HTTP11") | |
| } | |
| func TestHTTP11DisablesHTTP2Fallback(t *testing.T) { | |
| prevGodebug := os.Getenv("GODEBUG") | |
| t.Cleanup(func() { os.Setenv("GODEBUG", prevGodebug) }) | |
| opts := DefaultOptions | |
| opts.Protocol = HTTP11 | |
| ht, err := New(&opts) | |
| require.Nil(t, err) | |
| // When http11 is requested, HTTPClient2 must point to the same client | |
| // as HTTPClient so retryablehttp-go's fallback path does not switch to HTTP/2. | |
| require.Same(t, ht.client.HTTPClient, ht.client.HTTPClient2, | |
| "HTTPClient2 should equal HTTPClient when Protocol is HTTP11") | |
| } |
🤖 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 32 - 43,
TestHTTP11DisablesHTTP2Fallback leaks a global GODEBUG change because New(&opts)
calls os.Setenv; to prevent cross-test pollution capture the current GODEBUG
with prev := os.Getenv("GODEBUG") and after constructing ht call
t.Setenv("GODEBUG", prev) so the environment is restored automatically by the
test harness; reference TestHTTP11DisablesHTTP2Fallback and New in your change
(or alternatively remove the global os.Setenv in httpx.go as a larger refactor).
…omments and godoc
1 participant
Proposed changes
Fixes #2240
/claim #2240
When
-pr http11is set, httpx disables HTTP/2 on the main transport but retryablehttp-go still falls back to its internalHTTPClient2(configured with HTTP/2) whenever it encounters malformed HTTP version errors indo.go. This effectively ignores the user's protocol preference.This patch points
HTTPClient2to the same HTTP/1.1 client whenProtocol == HTTP11, so the fallback path stays on HTTP/1.1. Also replaces the string literal with the existingHTTP11constant.Proof
Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Documentation