[Webkit-unassigned] [Bug 90267] Handle SSL errors for SOUP

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90267


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #151088|review?                     |review-
               Flag|                            |




--- Comment #18 from Martin Robinson <mrobinson at webkit.org>  2012-07-06 09:23:46 PST ---
(From update of attachment 151088)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.