Bug 234403 – [Cocoa] Web Driver: WebSocket over TLS failing over WebDriver with acceptInsecureCerts on Big Sur

• Status: RESOLVED
• Resolution: FIXED
• Component: WebDriver

[Patrick Angle]

<rdar://85460617>

[Patrick Angle]

Created attachment 447389 [details]
Patch v1.0

[Blaze Burg]

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

[Patrick Angle]

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.

[Patrick Angle]

Created attachment 447481 [details]
Patch v2.0

[Darin Adler]

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.

[Patrick Angle]

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.

[Patrick Angle]

Created attachment 447492 [details]
Patch v2.1 - Implement naming suggestion, fix non-Cocoa builds

[Blaze Burg]

Comment on attachment 447492 [details]
Patch v2.1 - Implement naming suggestion, fix non-Cocoa builds

r=me

[EWS]

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].