fix: enforce -pr http11 across retry fallback path#2421
fix: enforce -pr http11 across retry fallback path#2421juzigu40-ui wants to merge 4 commits intoprojectdiscovery:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
WalkthroughWhen Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
Neo - PR Security ReviewNo security issues found Highlights
Hardening NotesComment |
juzigu40-ui
force-pushed
the
codex/v100-httpx-2240
branch
from
5af6e9f to
83b3c34
Compare
![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_test.go`:
- Around line 36-37: After calling New(&opts) in the test (which sets
GODEBUG=http2client=0 via the code in New), restore the original GODEBUG
environment variable to avoid cross-test state leakage: capture
os.Getenv("GODEBUG") before calling New(&opts) and defer restoring it with
os.Setenv (or os.Unsetenv if it was empty) after the call so subsequent tests
don't inherit the modified GODEBUG; update the test around the New(&opts)
invocation to save/restore the environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d82d46ac-5d3e-4596-a485-29903e301ec1
📒 Files selected for processing (2)
common/httpx/httpx.gocommon/httpx/httpx_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- common/httpx/httpx.go
|
Update: refreshed this PR to a clean single-commit patch on top of current Proof: Change scope remains minimal (2 files):
This keeps HTTP/1.1 behavior deterministic when |
|
Addressed the CodeRabbit isolation finding. What changed:
Validation re-run:
|
|
@Mzack9999 @dogancanbakir quick update:
Patch scope remains minimal (2 files) and focused on #2240 behavior. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
common/httpx/httpx_test.go (1)
32-47: Add one mixed-case protocol regression testThese tests are solid for the lowercase constant path. Please add a case using
opts.Protocol = Proto("HTTP11")to lock in behavior for the case-insensitive CLI contract as well.🧪 Suggested test
+func TestHTTP11MixedCaseDisablesRetryableHTTP2Fallback(t *testing.T) { + opts := DefaultOptions + opts.Protocol = Proto("HTTP11") + + ht, err := New(&opts) + require.NoError(t, err) + require.Same(t, ht.client.HTTPClient, ht.client.HTTPClient2) +}🤖 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 - 47, Add a new test that mirrors TestHTTP11DisablesRetryableHTTP2Fallback but sets opts.Protocol = Proto("HTTP11") (mixed-case) to ensure the protocol handling is case-insensitive; instantiate with New(&opts), assert no error, and assert ht.client.HTTPClient and ht.client.HTTPClient2 are the same (using require.Same), similar to the existing test patterns; place it next to TestHTTP11DisablesRetryableHTTP2Fallback so it covers the CLI contract for mixed-case input.
🤖 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 155-158: The protocol checks use exact equality against the HTTP11
constant so case-variants (e.g., "HTTP11" or "http11") slip through; update the
checks that compare httpx.Options.Protocol to HTTP11 (the branches that set
transport.TLSNextProto and the related branch around lines ~183-186) to perform
a case-insensitive comparison (e.g., use strings.EqualFold or normalize to
upper/lowercase) when evaluating httpx.Options.Protocol, ensuring any casing of
the CLI flag correctly triggers the HTTP/1.1-only behavior.
---
Nitpick comments:
In `@common/httpx/httpx_test.go`:
- Around line 32-47: Add a new test that mirrors
TestHTTP11DisablesRetryableHTTP2Fallback but sets opts.Protocol =
Proto("HTTP11") (mixed-case) to ensure the protocol handling is
case-insensitive; instantiate with New(&opts), assert no error, and assert
ht.client.HTTPClient and ht.client.HTTPClient2 are the same (using
require.Same), similar to the existing test patterns; place it next to
TestHTTP11DisablesRetryableHTTP2Fallback so it covers the CLI contract for
mixed-case input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c555fb4f-8566-4fea-b636-de1449836447
📒 Files selected for processing (2)
common/httpx/httpx.gocommon/httpx/httpx_test.go
|
@Mzack9999 quick review ping on #2240: This PR keeps scope intentionally small (2 files in Current regression coverage in this PR:
Revalidated with:
If you prefer this minimal-path approach, I can follow up immediately with any requested adjustments. |
1 participant
Proposed Changes
This PR keeps
-pr http11deterministic by preventing retry fallback from switching protocol unexpectedly, while keeping change scope minimal.What changed
common/httpx/httpx.go:HTTP11constant for protocol gatingTLSNextProtoin HTTP/1.1 modeGODEBUGin client initialization-pr http11is requested:httpx.client.HTTPClient2 = httpx.client.HTTPClientcommon/httpx/httpx_test.go:TestHTTP11DisablesRetryableHTTP2FallbackTestDefaultProtocolKeepsRetryableHTTP2FallbackWhy this approach
retryablehttp-gofallback path currently callsHTTPClient2on malformed HTTP/2 framing errors. For explicit HTTP/1.1 mode, this patch keeps fallback on the HTTP/1.1 client path and avoids protocol drift.Proof
Checklist
dev/claim #2240includedRef: #2240
/claim #2240
Summary by CodeRabbit
Bug Fixes
Tests
Refactor