Open
Conversation
WalkthroughTLS impersonation was changed from a boolean toggle to a string option accepting ""/ Changes
Sequence DiagramsequenceDiagram
participant CLIUser as CLI User
participant CLI as CLI Parser
participant Opts as Options
participant HTTPX as HTTPX
participant Resolver as Strategy Resolver
participant Dialer as TLS Dialer
participant Impersonator as Impersonator Engine
CLIUser->>CLI: run with --tls-impersonate "chrome"
CLI->>Opts: set TlsImpersonate = "chrome"
CLI->>HTTPX: request client build with Opts
HTTPX->>Resolver: resolveImpersonateStrategy("chrome")
Resolver-->>HTTPX: (impersonate.Chrome, nil)
HTTPX->>Dialer: build DialTLSContext using strategy
Dialer->>Impersonator: DialTLSWithConfigImpersonate(strategy, config)
Impersonator-->>Dialer: TLS connection with crafted ClientHello
Dialer-->>HTTPX: return TLS connection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 Notes
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
common/httpx/httpx.go (1)
237-241: Invalid JA3 strings silently fall back to random impersonation.When a user provides an invalid JA3 string, the function silently falls back to
impersonate.Randomwithout any warning. This could be confusing if the user expects their custom JA3 to be applied but gets random behavior instead.Consider logging a warning when JA3 parsing fails, or returning an error during options validation so users are informed their input is invalid.
💡 Suggested improvement
default: spec, err := ja3.ParseWithJa3(value) if err != nil { + // Consider logging: gologger.Warning().Msgf("invalid JA3 string, falling back to random: %v", err) return impersonate.Random, nil } identity := impersonate.Identity(*spec) return impersonate.Custom, &identity }Alternatively, validate the JA3 string in
runner/options.go:ValidateOptions()to fail early with a clear error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/httpx/httpx.go` around lines 237 - 241, The current default branch silently falls back to impersonate.Random when ja3.ParseWithJa3(value) fails, which hides invalid user input; update the code that calls ja3.ParseWithJa3 (the default branch) to either return the parse error up to callers or emit a warning log before returning impersonate.Random, and also add validation in ValidateOptions() to call ja3.ParseWithJa3(value) and return a clear validation error if parsing fails so invalid JA3 strings fail early with a descriptive message.
🤖 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/tls_impersonate_test.go`:
- Around line 218-219: Replace all unchecked conn.Close() calls in
tls_impersonate_test.go with explicit error discard assignments (e.g. _ =
conn.Close()) so the test obeys lint rules; update the occurrences where
conn.Close() is used after require.NoError(t, err) (currently at the instances
referenced around lines 219, 256, 285, 295, 322, 335, 374, and 399) to use _ =
conn.Close() instead, ensuring every conn.Close() call in functions in this test
file is changed to the `_ = conn.Close()` pattern.
- Around line 196-197: The test currently ignores the error returned by
conn.Close(); update the test to check and fail on any Close error by replacing
the bare conn.Close() with an assertion such as require.NoError(t, conn.Close())
(or handle via t.Cleanup with require.NoError inside the cleanup) so the Close
failure is surfaced; locate the usage of conn.Close() in tls_impersonate_test.go
and apply this change where conn is closed.
- Around line 174-175: The call to conn.Close() in the test ignores its error
return; update the test to assert the close succeeded by checking the error
(e.g., replace the bare conn.Close() with an assertion such as
require.NoError(t, conn.Close()) or call t.Cleanup with a closure that asserts
require.NoError on conn.Close()) so the connection close failure is treated as a
test error; locate the conn variable and its conn.Close() call in
tls_impersonate_test.go to make this change.
- Around line 78-93: The test currently ignores errors from ln.Close(),
conn.Close(), and conn.Read(), causing linter failures; update the cleanup and
goroutines to check and handle these errors: in the t.Cleanup closure replace
ln.Close() with a checked call (if err := ln.Close(); err != nil {
t.Logf("ln.Close error: %v", err) }), in the accept loop check ln.Accept() as
already done but when closing the accepted connection replace defer conn.Close()
with a checked close (defer func(){ if err := conn.Close(); err != nil {
t.Logf("conn.Close error: %v", err) } }()), and replace conn.Read(buf) with a
checked read (if _, err := conn.Read(buf); err != nil && err != io.EOF {
t.Logf("conn.Read error: %v", err) }); keep using the test's t for logging so
the errors are recorded without failing the test.
---
Nitpick comments:
In `@common/httpx/httpx.go`:
- Around line 237-241: The current default branch silently falls back to
impersonate.Random when ja3.ParseWithJa3(value) fails, which hides invalid user
input; update the code that calls ja3.ParseWithJa3 (the default branch) to
either return the parse error up to callers or emit a warning log before
returning impersonate.Random, and also add validation in ValidateOptions() to
call ja3.ParseWithJa3(value) and return a clear validation error if parsing
fails so invalid JA3 strings fail early with a descriptive message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9316c669-2482-41ad-954e-aad7b459edb7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
README.mdcmd/functional-test/testcases.txtcommon/httpx/httpx.gocommon/httpx/option.gocommon/httpx/tls_impersonate_test.gogo.modrunner/options.go
common/httpx/tls_impersonate_test.go
Outdated
Comment on lines
+174
to
+175
| require.NoError(t, err) | ||
| conn.Close() |
There was a problem hiding this comment.
Unchecked conn.Close() error return.
🔧 Proposed fix
require.NoError(t, err)
- conn.Close()
+ _ = conn.Close()📝 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.
Suggested change
| require.NoError(t, err) | |
| conn.Close() | |
| require.NoError(t, err) | |
| _ = conn.Close() |
🧰 Tools
🪛 GitHub Check: Lint Test
[failure] 175-175:
Error return value of conn.Close is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/httpx/tls_impersonate_test.go` around lines 174 - 175, The call to
conn.Close() in the test ignores its error return; update the test to assert the
close succeeded by checking the error (e.g., replace the bare conn.Close() with
an assertion such as require.NoError(t, conn.Close()) or call t.Cleanup with a
closure that asserts require.NoError on conn.Close()) so the connection close
failure is treated as a test error; locate the conn variable and its
conn.Close() call in tls_impersonate_test.go to make this change.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
common/httpx/httpx.go (1)
231-243: Silent fallback on invalid JA3 may mask configuration errors.When
ja3.ParseWithJa3fails, the function silently falls back to Chrome impersonation. If a user provides a malformed JA3 string, they won't know their custom fingerprint isn't being used.Consider logging a warning when JA3 parsing fails so users can diagnose misconfiguration:
♻️ Proposed improvement with warning log
func resolveImpersonateStrategy(value string) (impersonate.Strategy, *impersonate.Identity) { switch strings.ToLower(value) { case "", "chrome": return impersonate.Chrome, nil default: spec, err := ja3.ParseWithJa3(value) if err != nil { + // Consider adding: gologger.Warning().Msgf("invalid JA3 string '%s', falling back to Chrome: %v", value, err) return impersonate.Chrome, nil } identity := impersonate.Identity(*spec) return impersonate.Custom, &identity } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/httpx/httpx.go` around lines 231 - 243, The resolveImpersonateStrategy function currently swallows ja3.ParseWithJa3 errors and silently falls back to impersonate.Chrome; update it to emit a warning when parsing fails (include the provided value and the parse error) so users know their custom JA3 was invalid. Modify the error branch around ja3.ParseWithJa3 to log a warning (using the package logger or standard log) with context (value and err) and then continue to return impersonate.Chrome, nil; keep the existing return behavior but add the diagnostic log entry. Ensure the change references resolveImpersonateStrategy and ja3.ParseWithJa3 so reviewers can locate it easily.
🤖 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 231-243: The resolveImpersonateStrategy function currently
swallows ja3.ParseWithJa3 errors and silently falls back to impersonate.Chrome;
update it to emit a warning when parsing fails (include the provided value and
the parse error) so users know their custom JA3 was invalid. Modify the error
branch around ja3.ParseWithJa3 to log a warning (using the package logger or
standard log) with context (value and err) and then continue to return
impersonate.Chrome, nil; keep the existing return behavior but add the
diagnostic log entry. Ensure the change references resolveImpersonateStrategy
and ja3.ParseWithJa3 so reviewers can locate it easily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 582bf958-0258-4590-a339-2bf00173a156
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
common/httpx/httpx.gocommon/httpx/tls_impersonate_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Close #2044
Fixed a bug in elliptic curves discovered while testing in projectdiscovery/fastdialer#535
Checklist
Summary by CodeRabbit
New Features
--tls-impersonate/-tlsiCLI flag now accepts a string value ("random", "chrome", or a full JA3 string) to select TLS client-hello impersonation modes instead of a boolean toggle.Documentation
Tests