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. |
fix #504