Add chrome and JA3 strategies for -tls-impersonate#2461
Merged
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:
WalkthroughThe PR converts the ChangesTLS Impersonation with JA3 Support
Sequence DiagramsequenceDiagram
participant Transport as http.Transport
participant buildTLSDialer as buildTLSDialer()
participant resolveImpersonate as resolveImpersonateStrategy()
participant ja3Parser as ja3.ParseWithJa3()
participant Dialer as h.Dialer.DialTLSWithConfigImpersonate()
Transport->>buildTLSDialer: DialTLSContext with options
buildTLSDialer->>buildTLSDialer: check if TlsImpersonate is empty
alt TlsImpersonate is empty
buildTLSDialer->>Dialer: use h.Dialer.DialTLS
else TlsImpersonate is not empty
buildTLSDialer->>resolveImpersonate: resolve strategy from string value
resolveImpersonate->>ja3Parser: parse string as JA3 spec
alt JA3 parse succeeds
ja3Parser-->>resolveImpersonate: parsed identity
resolveImpersonate-->>buildTLSDialer: impersonate.Custom with identity
else JA3 parse fails
resolveImpersonate-->>buildTLSDialer: impersonate.Chrome (fallback)
end
buildTLSDialer->>Dialer: call with resolved strategy and TLS config
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/httpx/httpx.go`:
- Around line 233-245: resolveImpersonateStrategy currently doesn't handle the
"random" value and silently falls back to Chrome on JA3 parse errors; update
resolveImpersonateStrategy to explicitly handle the "random" case (check for
strings.ToLower(value) == "random") and return the appropriate strategy (if the
impersonate package exposes a Random constant use impersonate.Random, otherwise
implement a deterministic alternative or surface an error to the caller), and
change the default branch that calls ja3.ParseWithJa3 to not silently ignore
parse errors—log or return a clear warning/error when ja3.ParseWithJa3 fails
(instead of quietly returning impersonate.Chrome) so misconfigured JA3 strings
are visible; refer to resolveImpersonateStrategy and ja3.ParseWithJa3 when
making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc91c8e4-bb92-4c7d-b3e3-8de419b12824
📒 Files selected for processing (3)
README.mdcommon/httpx/httpx.gorunner/options.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- runner/options.go
Mzack9999
changed the title
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
2 participants
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
Closes #2044
Converts
-tls-impersonate/-tlsifrom a boolean to a string strategy:chrome<ja3 string>randomchrome(random JA3 removed due to unsupported curve picks — see fastdialer#535)chromeAlso includes the fastdialer elliptic-curves fix discovered while testing that issue.
Checklist
Test plan
resolveImpersonateStrategy(chrome / random / JA3 / invalid)httpx.Newdialer with chrome / JA3-tls-impersonate chromeincmd/functional-test/testcases.txtscanme.shwithchrome, custom JA3, invalid JA3 (warning),random