Bug 231789 – WebDriver: [Cocoa] support `acceptInsecureCerts` capability

• Component: WebDriver
• Status: RESOLVED FIXED

[Patrick Angle]

<rdar://36878038>

[Patrick Angle]

Created attachment 441312 [details]
Patch v1.0

[Patrick Angle]

Created attachment 441976 [details]
Patch v1.1

[Blaze Burg]

Comment on attachment 441976 [details]
Patch v1.1

View in context: https://bugs.webkit.org/attachment.cgi?id=441976&action=review

The code changes look good to me. Would be good to get signoff from Alex, John, or another NetworkProcess expert.

I think the changelog should explain why this is a one-way setting, as that differs from the other capabilities we handle in the same way (AllowInsecureMediaCaptureCapabilityKey).

> Source/WebKit/ChangeLog:12
> +        to allow the load to continue as if the certificate is trusted, regardless of the actual trustworthyness of the

Nit: trustworthiness

> Source/WebKit/NetworkProcess/NetworkProcess.h:462
> +    void acceptInsecureCertificates(PAL::SessionID);

(applies throughout) 

I would add a suffix like ForWebDriver or ForAutomation to make it clear that this is edge-case territory.

> Source/WebKit/UIProcess/Cocoa/AutomationClient.mm:81
> +    [configuration setAcceptInsecureCertificates:sessionCapabilities.acceptInsecureCertificates];

Nit: doing an if check would make this line up with the other capability forwarding below.

[Patrick Angle]

Comment on attachment 441976 [details]
Patch v1.1

View in context: https://bugs.webkit.org/attachment.cgi?id=441976&action=review

>> Source/WebKit/UIProcess/Cocoa/AutomationClient.mm:81
>> +    [configuration setAcceptInsecureCertificates:sessionCapabilities.acceptInsecureCertificates];
> 
> Nit: doing an if check would make this line up with the other capability forwarding below.

The other capability forwarding checks below actually have their `sessionCapabilities` properties implemented as optional booleans, but the existing `sessionCapabilities.acceptInsecureCertificates` is implemented a boolean, so the outer `if` check isn't necessary here.

[Patrick Angle]

Created attachment 442056 [details]
Patch v1.2 - Address BJ's review notes

[Chris Dumez]

Comment on attachment 442056 [details]
Patch v1.2 - Address BJ's review notes

View in context: https://bugs.webkit.org/attachment.cgi?id=442056&action=review

> Source/WebKit/NetworkProcess/NetworkSession.h:146
> +    void acceptInsecureCertificatesForAutomation() { m_acceptInsecureCertificates = true; }

The naming is a bit confusing. This looks a lot like a getter name.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:89
> +- (void)_acceptInsecureCertificatesForAutomation WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

shouldn't it be a flag on the _WKWebsiteDataStoreConfiguration so that you could only set it at creation time.

Seems weird to have a method on the WKWebsiteDataStore when you can only set it to YES (never back to NO).

[Alex Christensen]

Comment on attachment 442056 [details]
Patch v1.2 - Address BJ's review notes

View in context: https://bugs.webkit.org/attachment.cgi?id=442056&action=review

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2164
> +void NetworkProcess::acceptInsecureCertificatesForAutomation(PAL::SessionID sessionID)

This isn't needed, as discussed offline.  This state can be kept in the UI process.  Please remove.

[Patrick Angle]

Created attachment 442062 [details]
Patch v1.3 - Remove NetworkProcess changes

[Blaze Burg]

Comment on attachment 442062 [details]
Patch v1.3 - Remove NetworkProcess changes

r=me

[EWS]

Committed r285164 (243800@main): <https://commits.webkit.org/243800@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442062 [details].