[webkit-reviews] review denied: [Bug 90267] Handle SSL errors for SOUP : [Attachment 151088] New patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 90267: Handle SSL errors for SOUP
https://bugs.webkit.org/show_bug.cgi?id=90267

Attachment 151088: New patch
https://bugs.webkit.org/attachment.cgi?id=151088&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=151088&action=review


This looks good to me apart from a concern about how you save TLS certifictes
(indexed only on URL and not port). I think it would be good for Sergio and Dan
to take a look though.

> Source/WebCore/platform/network/soup/ResourceError.h:32
>  #include "ResourceErrorBase.h"
>  
> +#include <wtf/gobject/GRefPtr.h>
> +

Nit: This header could go directly under the first, I think.

> Source/WebCore/platform/network/soup/ResourceError.h:52
> +    ResourceError(const String& domain, int errorCode, const String&
failingURL, const String& localizedDescription, unsigned tlsErrors,
GTlsCertificate* certificate)
> +	   : ResourceErrorBase(domain, errorCode, failingURL,
localizedDescription)
> +	   , m_tlsErrors(tlsErrors)
> +	   , m_certificate(certificate)

m_tlsErrors is uninitialized for other constructors. Maybe it should be
initialized to 0 for completeness.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:172
> +static bool ignoreSSLErrors = false;

I like to prefix my global static variables with g. Which, I think, could be
useful in this case too.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:431
> +	   if (d->m_response.soupMessageTLSErrors() && !ignoreSSLErrors &&
!allowsAnyHTTPSCertificateHosts().contains(handle->firstRequest().url().host().
lower())) {

Does host() also include the port number? If I'm not mistaken you want
something like the security domain (scheme-host-scheme). The scheme may not be
important here, but perhaps it would be better to make this conditional on
protocolHostAndPortAreEqual.

I also wonder what happens when you make a request to http://foo.com and then
http://foo.com:80.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:432
> +	       CertificatesMap::iterator iter =
clientCertificates().find(handle->firstRequest().url().host().lower());

I think it makes sense to put this entire check into a helper method called
something like messageHasUnignoredTLSError.