Skip to content

fix:修复 SetTLSFingerprintSpec 连续握手失败的问题 - #507

Open
1992w wants to merge 2 commits into
imroc:masterfrom
1992w:fix/issue-504
Open

fix:修复 SetTLSFingerprintSpec 连续握手失败的问题#507
1992w wants to merge 2 commits into
imroc:masterfrom
1992w:fix/issue-504

Conversation

@1992w

@1992w 1992w commented Jul 14, 2026

Copy link
Copy Markdown

fix #504

@imroc imroc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! The approach is correct — using a factory function func() utls.ClientHelloSpec ensures a fresh spec per handshake, which fixes the root cause (reusing a mutable *utls.ClientHelloSpec across multiple TLS handshakes). Using a value return rather than a pointer return (as suggested in the issue) is actually a better design choice, since it prevents callers from accidentally returning a shared pointer.

A few things need to be addressed before merging:

1. go vet fails (blocker)

The test uses unkeyed struct fields for several utls types, which go vet flags:

client_test.go:786:6: SupportedCurvesExtension struct literal uses unkeyed fields
client_test.go:810:6: KeyShareExtension struct literal uses unkeyed fields
client_test.go:815:6: PSKKeyExchangeModesExtension struct literal uses unkeyed fields
client_test.go:818:6: SupportedVersionsExtension struct literal uses unkeyed fields
client_test.go:823:6: UtlsCompressCertExtension struct literal uses unkeyed fields

Please add field names, e.g. &utls.SupportedCurvesExtension{Curves: []utls.CurveID{...}} instead of &utls.SupportedCurvesExtension{[]utls.CurveID{...}}.

2. External URL dependencies in test (blocker)

The test hits https://tls.browserleaks.com/json and https://tools.scrapfly.io/api/fp/ja3. External services can be down, rate-limited, or unreachable from CI, making the test flaky. Since the purpose is just to verify that consecutive handshakes to different hosts succeed, a local TLS test server would be more reliable. The repo already has test certificates in internal/testdata (cert.pem, priv.key, ca.pem) that can be used with httptest.NewTLSServer.

3. Commit message (minor)

Per project conventions, commit messages should be in English. The current message mixes Chinese and English: fix:修复 SetTLSFingerprintSpec 连续握手失败的问题. Something like fix: SetTLSFingerprintSpec fails on consecutive handshakes to different domains would be better.

4. Test spec complexity (minor)

The test includes a very detailed Chrome TLS fingerprint spec with ShuffleChromeTLSExtensions and many extensions. For testing the bug fix (consecutive handshakes to different domains), a minimal ClientHelloSpec would suffice and would be easier to maintain.

@1992w

1992w commented Jul 22, 2026

Copy link
Copy Markdown
Author

@imroc All fixed.

@imroc imroc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review (PR Review Loop):

Scope: Fixes #504 by changing SetTLSFingerprintSpec to accept a factory function instead of a static spec. Files: client.go, client_test.go.

The fix is correct in intent — issue #504 is that reusing the same *utls.ClientHelloSpec across multiple TLS handshakes causes the second handshake to fail (the spec is consumed/mutated during ApplyPreset). Passing a func() utls.ClientHelloSpec factory ensures each connection gets a fresh spec. The test verifies two consecutive handshakes to different hosts.

However, this is a breaking API change that needs to be addressed:

  1. Breaking signature change. SetTLSFingerprintSpec changes from (*utls.ClientHelloSpec) to (func() utls.ClientHelloSpec). Any existing code calling client.SetTLSFingerprintSpec(&spec) will fail to compile. Since req v3 is a stable release, this needs either:

    • A migration note in the PR description and changelog, or
    • A new method (e.g. SetTLSFingerprintSpecFunc) to avoid breaking existing users, or
    • At minimum, the PR description should explicitly call out the breaking change so the maintainer can decide.
  2. PR description is empty. The body only says "fix #504" with no mention of the API breakage. The maintainer should be informed that this is a breaking change.

  3. Test connects to server1 but ignores server (the first httptest.NewTLSServer). The test creates two servers (server and server1) but only connects to server1 in the second call. The first c.R().Get("/") hits the default test server, not server. This is confusing and should either use both servers or explain why only one is used. Minor, but worth fixing for clarity.

Suggestion: If the maintainer accepts the breaking change, consider also updating the method documentation to recommend the factory pattern and warn against reusing specs.

Requesting changes to ensure the breaking API change is explicitly acknowledged and documented before merge.

@1992w
1992w requested a review from imroc July 29, 2026 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]SetTLSFingerprintSpec

2 participants