fix: respect -pr http11 during retryablehttp fallback#2456
fix: respect -pr http11 during retryablehttp fallback#2456drmabus wants to merge 4 commits intoprojectdiscovery:devfrom
Conversation
Neo - PR Security ReviewNo security issues found Highlights
Comment |
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant NewConstructor as New()
participant Primary as HTTPClient
participant Retryable as HTTPClient2
Caller->>NewConstructor: call New(opts) with opts.Protocol == HTTP11
NewConstructor->>Primary: create HTTP/1.1-only transport
NewConstructor->>Retryable: assign HTTPClient2 = HTTPClient
NewConstructor-->>Caller: return client (both clients share transport)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
Thanks for reviewing! Let me know if you'd like any changes or additional test cases. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.go`:
- Around line 186-191: Replace the string literal comparison against "http11"
with the Proto constant HTTP11: change the conditional that checks
httpx.Options.Protocol in the block that sets httpx.client.HTTPClient2 so it
compares against the HTTP11 constant (defined in proto.go) instead of the raw
string; also update the earlier occurrence around line 156 that currently
compares with "http11" to use HTTP11 for consistency, ensuring both checks use
the same Proto-typed constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cb9d3cd-e3b0-4a42-ac05-f1b346be220c
📒 Files selected for processing (1)
common/httpx/httpx.go
|
Updated to use the HTTP11 constant instead of string literals for consistency. Thanks for the suggestion. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
common/httpx/httpx.go (1)
186-191: Consider adding a regression test for this fallback path.This fix is small and targeted, but the bug only shows up when retryable fallback is triggered on specific transport failures. A test that forces that path and asserts HTTP/2 is never used when
Protocol == HTTP11would make this much harder to reintroduce.🤖 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 - 191, Add a regression test that simulates a transport failure which triggers retryablehttp's fallback path and verifies that when httpx.Options.Protocol == HTTP11 the code path in httpx.go that sets httpx.client.HTTPClient2 = httpx.client.HTTPClient is effective (i.e., HTTP/2 is never used). Create a unit test (e.g., TestHTTP11NoHTTP2OnRetryFallback) that forces a retryable transport error, sets Protocol to HTTP11, invokes the request flow that uses httpx.client, and asserts the underlying client never negotiated or used HTTP/2 (inspect the transport or request metadata/mock server to confirm). Ensure the test exercises the fallback logic and fails if HTTPClient2 falls back to an HTTP/2-capable transport.
🤖 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-191: Add a regression test that simulates a transport failure
which triggers retryablehttp's fallback path and verifies that when
httpx.Options.Protocol == HTTP11 the code path in httpx.go that sets
httpx.client.HTTPClient2 = httpx.client.HTTPClient is effective (i.e., HTTP/2 is
never used). Create a unit test (e.g., TestHTTP11NoHTTP2OnRetryFallback) that
forces a retryable transport error, sets Protocol to HTTP11, invokes the request
flow that uses httpx.client, and asserts the underlying client never negotiated
or used HTTP/2 (inspect the transport or request metadata/mock server to
confirm). Ensure the test exercises the fallback logic and fails if HTTPClient2
falls back to an HTTP/2-capable transport.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84628cf5-a39c-4f89-abae-b5d6de9a66ba
📒 Files selected for processing (1)
common/httpx/httpx.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 16-24: The tests are asserting pointer identity but use
value-equality helpers; update TestDefaultProtocolKeepsHTTP2Fallback and the
other test to use require.Same/require.NotSame instead of
require.Equal/require.NotEqual when comparing ht.client.HTTPClient and
ht.client.HTTPClient2 so the assertions check that HTTPClient2 either references
the exact same instance (when Protocol is HTTP11) or a different instance (with
DefaultOptions/New); keep the rest of the test logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57c5dfa8-6f7b-4ada-922d-9e19840dc83e
📒 Files selected for processing (1)
common/httpx/httpx_test.go
|
Thank you for the contribution! This issue has been resolved by merging #2424, which implements the same approach ( |
2 participants
/claim #2240
Closes #2240
Summary
When
-pr http11is specified, httpx should strictly use HTTP/1.1.However, retryablehttp may fall back to HTTPClient2 (which has HTTP/2 enabled)
on certain transport errors, bypassing the user's explicit protocol selection.
Fix
If HTTP/1.1 is forced, ensure HTTPClient2 reuses the same HTTP/1.1-only client,
preventing unintended HTTP/2 fallback.
Impact
Testing
Verified locally:
Summary by CodeRabbit
Bug Fixes
Tests