fix:修复 SetTLSFingerprintSpec 连续握手失败的问题 - #507
Conversation
imroc
left a comment
There was a problem hiding this comment.
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.
|
@imroc All fixed. |
imroc
left a comment
There was a problem hiding this comment.
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:
-
Breaking signature change.
SetTLSFingerprintSpecchanges from(*utls.ClientHelloSpec)to(func() utls.ClientHelloSpec). Any existing code callingclient.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.
-
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.
-
Test connects to
server1but ignoresserver(the firsthttptest.NewTLSServer). The test creates two servers (serverandserver1) but only connects toserver1in the second call. The firstc.R().Get("/")hits the default test server, notserver. 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.
fix #504