fix: preserve -pr http11 across retryablehttp-go fallback (#2240)#2449
fix: preserve -pr http11 across retryablehttp-go fallback (#2240)#2449CharlesWong wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
…covery#2240) retryablehttp-go falls back to HTTPClient2 (HTTP/2-capable) when it hits a "malformed HTTP version" error, silently upgrading the protocol even when -pr http11 is set. Point HTTPClient2 at the same HTTP/1.1-only client to neutralise the fallback. Fixes projectdiscovery#2240
|
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)
WalkthroughReplaces string literal protocol checks with a constant and synchronizes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
🧹 Nitpick comments (1)
common/httpx/httpx.go (1)
186-193: Consider using theHTTP11constant instead of string literal.The comparison uses the string literal
"http11"directly. The codebase defines a typed constantHTTP11 Proto = "http11"inproto.go. Using the constant improves type safety and maintainability, and aligns with how the test file references this value.Note: Line 156 has the same pattern, so both could be updated together for consistency.
♻️ Suggested change
- if httpx.Options.Protocol == "http11" { + if httpx.Options.Protocol == HTTP11 { httpx.client.HTTPClient2 = httpx.client.HTTPClient }And optionally update line 156 as well:
- if httpx.Options.Protocol == "http11" { + if httpx.Options.Protocol == HTTP11 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/httpx/httpx.go` around lines 186 - 193, Replace the string literal "http11" comparisons with the typed constant HTTP11 (defined as Proto = "http11") to improve type safety; specifically, update the conditional checking httpx.Options.Protocol in the block that sets httpx.client.HTTPClient2 = httpx.client.HTTPClient (and the similar comparison earlier near where Protocol is checked) to compare against the HTTP11 constant instead of the literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@common/httpx/httpx.go`:
- Around line 186-193: Replace the string literal "http11" comparisons with the
typed constant HTTP11 (defined as Proto = "http11") to improve type safety;
specifically, update the conditional checking httpx.Options.Protocol in the
block that sets httpx.client.HTTPClient2 = httpx.client.HTTPClient (and the
similar comparison earlier near where Protocol is checked) to compare against
the HTTP11 constant instead of the literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b41e964-d514-4d26-8c8a-4f8306d14544
📒 Files selected for processing (2)
common/httpx/httpx.gocommon/httpx/httpx_test.go
Neo - PR Security ReviewNo security issues found Highlights
Comment |
|
The HTTP11 constant nitpick is already applied in this branch — both occurrences in httpx.go (lines 156 and 191) use the typed HTTP11 constant rather than the string literal. No further changes needed. |
1 participant
Summary
Fixes #2240 —
-pr http11is silently ignored on retry becauseretryablehttp-gofalls back to an HTTP/2-capable client.Root Cause Analysis
The HTTP/1.1 enforcement has two layers, but only the first was implemented:
GODEBUG=http2client=0+TLSNextProto={}on the primary transportretryablehttp-goswitches toHTTPClient2(HTTP/2) on "malformed HTTP version" errorsWhen a server responds with HTTP/2, the primary HTTP/1.1 client returns an error.
retryablehttp-gothen retries withHTTPClient2— which succeeds via HTTP/2, defeating-pr http11.Fix
Point
HTTPClient2at the same HTTP/1.1-only client, so the fallback path still honours the protocol restriction:Why This PR
HTTPClient2 = HTTPClientDisableHTTP2Fallbackoption does not exist in retryablehttp-goNewWithHTTPClient, beforetransport2require.NotSameconfirms HTTPClient2 stays distinct in default modeSome alternative approaches (e.g.
DisableHTTP2Fallbackoption) reference APIs that don't exist inretryablehttp-go. This PR uses the only mechanism available: overriding the fallback client pointer directly.Testing
Summary by CodeRabbit
Bug Fixes
Tests