fix(httpx): honor -pr http11 in h2 probe client#2416
fix(httpx): honor -pr http11 in h2 probe client#2416wh0amibjm wants to merge 3 commits intoprojectdiscovery:devfrom
Conversation
Neo Security AuditNo security issues found Hardening Notes
Comment |
WalkthroughReplaces a string-literal protocol check with a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🧹 Nitpick comments (1)
common/httpx/httpx.go (1)
186-206: Use theHTTP11typed constant instead of the"http11"string literalThe test file uses
opts.Protocol = HTTP11(the typedProtoconstant defined inproto.go), but the production code at lines 156 and 188 compares against the bare string"http11". Using the constant in both places keeps the comparison DRY and eliminates a silent mismatch risk if the constant value ever changes.♻️ Proposed change (applies to lines 156 and 188)
- if httpx.Options.Protocol == "http11" { + if httpx.Options.Protocol == HTTP11 { // ... protocol handling }🤖 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 - 206, Replace the string literal "http11" comparisons with the typed constant HTTP11 (the Proto constant defined in proto.go) so the checks use httpx.Options.Protocol == HTTP11 instead of == "http11"; update both locations where the literal is used (the branch deciding HTTP/1.1 mode and the related branch later in the file) to reference the HTTP11 constant to keep comparisons consistent and DRY.
🤖 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_protocol_test.go`:
- Around line 11-24: The test leaks a process-wide GODEBUG change because
New(&opts) with opts.Protocol = HTTP11 calls os.Setenv("GODEBUG",
"http2client=0") and never restores it; modify
TestNew_HTTP11DisablesRetryableHTTP2Fallback to capture the original
os.Getenv("GODEBUG") before calling New, and register a t.Cleanup that restores
the original value (use os.Setenv to restore non-empty original, or os.Unsetenv
if it was empty) so the GODEBUG setting is returned to its prior state after the
test; reference the New function, opts.Protocol/HTTP11 setup, and the
os.Setenv("GODEBUG", ...) side effect when applying the fix.
---
Nitpick comments:
In `@common/httpx/httpx.go`:
- Around line 186-206: Replace the string literal "http11" comparisons with the
typed constant HTTP11 (the Proto constant defined in proto.go) so the checks use
httpx.Options.Protocol == HTTP11 instead of == "http11"; update both locations
where the literal is used (the branch deciding HTTP/1.1 mode and the related
branch later in the file) to reference the HTTP11 constant to keep comparisons
consistent and DRY.
|
Addressed CodeRabbit follow-up in
Validation: go test ./common/httpx -run TestNew_ -count=1 |
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@common/httpx/httpx_protocol_test.go`:
- Around line 16-23: The code correctly uses os.LookupEnv to capture
originalGODEBUG and hadGODEBUG and restores state in t.Cleanup using os.Setenv
or os.Unsetenv; keep this implementation as-is (no change needed) and ensure the
variables originalGODEBUG, hadGODEBUG and the t.Cleanup block remain present so
an originally unset GODEBUG is restored to unset rather than set to an empty
string.
|
Quick follow-up after latest bot review pass:
Ready for maintainer review/merge when convenient. |
|
Pushed a small follow-up to address the docstring-coverage warning by adding doc comments to the exported protocol tests (no functional changes). Proof: go test ./common/httpx -run TestNew_ -count=1 |
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_protocol_test.go`:
- Around line 48-49: Add a nil-guard before dereferencing ht.client2: ensure
ht.client2 is non-nil (e.g., call require.NotNil(t, ht.client2)) before doing
the type assertion on ht.client2.Transport; this prevents a nil-pointer panic
when require.NotSame passes and lets the test fail cleanly if client2 is nil.
---
Duplicate comments:
In `@common/httpx/httpx_protocol_test.go`:
- Around line 17-24: The GODEBUG save/restore is already correct: os.LookupEnv
is used to distinguish unset vs empty and t.Cleanup is registered before any
environment modification (e.g., before New changes GODEBUG), so no change is
needed—leave the os.LookupEnv, hadGODEBUG assignment and the t.Cleanup restore
logic as-is.
| require.NotSame(t, ht.client.HTTPClient, ht.client2) | ||
| _, isHTTP2 := ht.client2.Transport.(*http2.Transport) |
There was a problem hiding this comment.
Add a nil guard for ht.client2 before accessing .Transport.
require.NotSame at line 48 passes silently when ht.client2 is nil (nil address ≠ non-nil address), so execution reaches line 49 and panics with a nil-pointer dereference instead of producing a clean test-failure message. Test 1 is safe because require.Same would fail if client2 were nil, but require.NotSame provides no such protection here.
🛡️ Proposed fix
require.NotSame(t, ht.client.HTTPClient, ht.client2)
+ require.NotNil(t, ht.client2)
_, isHTTP2 := ht.client2.Transport.(*http2.Transport)📝 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.
| require.NotSame(t, ht.client.HTTPClient, ht.client2) | |
| _, isHTTP2 := ht.client2.Transport.(*http2.Transport) | |
| require.NotSame(t, ht.client.HTTPClient, ht.client2) | |
| require.NotNil(t, ht.client2) | |
| _, isHTTP2 := ht.client2.Transport.(*http2.Transport) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/httpx/httpx_protocol_test.go` around lines 48 - 49, Add a nil-guard
before dereferencing ht.client2: ensure ht.client2 is non-nil (e.g., call
require.NotNil(t, ht.client2)) before doing the type assertion on
ht.client2.Transport; this prevents a nil-pointer panic when require.NotSame
passes and lets the test fail cleanly if client2 is nil.
|
Hi, thanks for your interest in contributing! Just a heads up, we ask contributors to work on 1 active issue at a time (see). Also, we welcome AI-assisted development, but submissions must be complete, tested, and ready to merge. Please also make sure to fill out the PR template with proof that your changes work. We're closing this PR along with your other open submissions. Once you're ready, feel free to pick one issue to focus on and resubmit; we'd be happy to review it. Appreciate your understanding! |
3 participants
Follow-up to #2414 and issue #2240.
This patch ensures explicit
-pr http11is honored across both retry fallback and HTTP/2 probe paths:httpx.client2 = httpx.client.HTTPClientin HTTP11 modet.Cleanupin protocol testsValidation:
go test ./common/httpx -run TestNew_ -count=1/claim #2240
Summary by CodeRabbit
New Features
Tests