Summary: | [Cocoa] Web Driver: WebSocket over TLS failing over WebDriver with acceptInsecureCerts on Big Sur | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick Angle <pangle> | ||||||||
Component: | WebDriver | Assignee: | Patrick Angle <pangle> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, bburg, berto, cgarcia, darin, ews-watchlist, galpeter, gustavo, toyoshim, webkit-bug-importer, wilander, yutak | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Patrick Angle
2021-12-16 13:11:55 PST
Created attachment 447389 [details]
Patch v1.0
Comment on attachment 447389 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=447389&action=review I think this should have an availability annotation to prevent use on macOS after Big Sur. Something like this from _WKAttachment.h: - (void)requestInfo:(void(^)(_WKAttachmentInfo * _Nullable, NSError * _Nullable))completionHandler WK_API_DEPRECATED_WITH_REPLACEMENT("-info", macos(10.14, 10.14.4), ios(12.0, 12.2)); > Source/WebKit/ChangeLog:12 > + automationmation clients when using the `acceptInsecureCerts` capability. Nit: typo Comment on attachment 447389 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=447389&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:671 > + _websiteDataStore->setAllowsAnySSLCertificateForWebSocket(allow); I've made a mistake here. This ends up enabling/disabling insecure certs globally, not scoped to a network session. I need to reengineer this. Created attachment 447481 [details]
Patch v2.0
Comment on attachment 447481 [details] Patch v2.0 View in context: https://bugs.webkit.org/attachment.cgi?id=447481&action=review Not a full review, but one style comment (please other reviewers, don’t hesitate to review) > Source/WebKit/NetworkProcess/NetworkSession.h:210 > + bool acceptInsecureCertificatesForWebSockets() const { return m_acceptInsecureCertificatesForWebSockets; } Often we steer away from names like this for getters, because they are ambiguous. This could be a function telling the network session that it should accept certificates. And a caller that thought it would do that won’t even get a compile time error. An alternative is to name such functions shouldAccept... or some other non-verb phrase, a style we have borrowed from Apple’s Cocoa naming conventions. > Source/WebKit/UIProcess/API/Cocoa/_WKWebsiteDataStoreConfiguration.h:58 > +@property (nonatomic, setter=_setAcceptInsecureCertificatesForWebSockets:) BOOL _acceptInsecureCertificatesForWebSockets WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); Same comment for this public property. Note for example, for above it says "shouldRunServiceWorkersOnMainThreadForTesting.". > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:201 > + bool acceptInsecureCertificatesForWebSockets() const { return m_acceptInsecureCertificatesForWebSockets; } Here too. Comment on attachment 447481 [details]
Patch v2.0
Oops, looks like I missed that we have three files named `SocketStreamHandleImpl.h`, and so platforms that use soup or curl backed sockets are failing to build.
Created attachment 447492 [details]
Patch v2.1 - Implement naming suggestion, fix non-Cocoa builds
Comment on attachment 447492 [details]
Patch v2.1 - Implement naming suggestion, fix non-Cocoa builds
r=me
Committed r287781 (245841@main): <https://commits.webkit.org/245841@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447492 [details]. |