[webkit-reviews] review denied: [Bug 187611] [Curl] Fix implementation error in handling Certificate exceptions. : [Attachment 345038] FIX

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 187611: [Curl] Fix implementation error in handling Certificate exceptions.
https://bugs.webkit.org/show_bug.cgi?id=187611

Attachment 345038: FIX

https://bugs.webkit.org/attachment.cgi?id=345038&action=review




--- Comment #15 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 345038
  --> https://bugs.webkit.org/attachment.cgi?id=345038
FIX

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

> Source/WebCore/ChangeLog:3
> +	   [Curl] Fix implementation error in handling Certificate exceptions.

In my understanding, this patch doesn't solve any problem.

isAllowedHTTPSCertificateHost is used if PLATFORM(WIN),
canIgnoredHTTPSCertificate is used otherwise.
But, Curl support is used only in WinCairo port in upstream WebKit.
This patch changes two things:
1. How isAllowedHTTPSCertificateHost is used.
2. Remove canIgnoredHTTPSCertificate.
#2 is a trivial change because this is an unused function in upstream ports.

I'd like to propose a following summaries:

[Curl] Disable CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST for hosts
specified by setAllowsAnyHTTPSCertificate

or

[Curl] Change the implementation of setAllowsAnyHTTPSCertificate by disabling
CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST

> Source/WebCore/ChangeLog:32
> +	   No new tests. Covered by existing tests.

AFAIK, there is no test cases of
ResourceHandle::setHostAllowsAnyHTTPSCertificate.
It's impossible to test unused a function even by manual testing.
It should be testable by using Windows MiniBrowser because Windows WK1 has an
API WebMutableURLRequest::setAllowsAnyHTTPSCertificate.

> Source/WebCore/ChangeLog:34
> +	   * platform/LocalizedStrings.cpp:

This patch doesn't change LocalizedStrings.cpp. Remove this.

> Source/WebCore/platform/network/curl/CertificateInfo.h:36
> +    using CertificateChain = Vector<Certificate>;

Remove "using CertificateChain = Vector<Certificate>;". This is not used yet.