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

[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 344881: WIP Patch

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




--- Comment #7 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 344881
  --> https://bugs.webkit.org/attachment.cgi?id=344881
WIP Patch

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

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:95
> +void
CurlSSLHandle::allowSpecificHTTPSCertificateForHost(CertificateInfo::Certificat
eChain&& certificates, String&& host)

I don't think you should pass the lifetime of 'host' by calling this method.
'host' should be a lvalue reference.

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:99
> +    auto found = m_allowedCertificatesForHost.find(host);

This is an iterator. found → iterator or iter

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:101
> +	   m_allowedCertificatesForHost.set(WTFMove(host),
Vector<CertificateInfo::CertificateChain> { WTFMove(certificates) });

Do you need to specify the type "Vector<CertificateInfo::CertificateChain>"  in
this case?
m_allowedCertificatesForHost.set(WTFMove(host), { WTFMove(certificates) });

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:110
> +    auto found = m_allowedCertificatesForHost.find(host);

This is an iterator. found → iterator or iter

> Source/WebCore/platform/network/curl/CurlSSLHandle.cpp:115
> +    return allowedCertificates.contains(certificates);

return found->contains(certificates);

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:51
> +    if (auto data =
WTF::get_if<CertificateInfo::Certificate>(sslHandle.getCACertInfo()))

'auto data' should be "auto& data" to avoid copy.

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:71
> +    if (auto error = m_certificateInfo.verificationError())

Do you need to define 'error' twice?
if (error)

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:78
> +    if (!m_certificateInfo.verificationError())

Why don't use 'error' you defined above?
if (!error)
This change is start to check 'preverify_ok'. Do you really need this check?

> Source/WebCore/platform/network/curl/CurlSSLVerifier.cpp:85
>	       m_curlHandle->setSslVerifyPeer(CurlHandle::VerifyPeer::Disable);

Just out of curiosity, Why is this code needed?
When is m_curlHandle null?

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:173
> +   
CurlContext::singleton().sslHandle().allowAnyHTTPSCertificatesForHost(WTFMove(h
ostCopy));

This code doesn't look good.
In this case, allowAnyHTTPSCertificatesForHost should take a value instead of a
rvalue reference.
allowAnyHTTPSCertificatesForHost(String);
Then, only one copy is created by calling the method.