When not ignoring TLS errors we should fail before showing the HTTP auth dialog in case of invalid certificate. See also https://bugzilla.gnome.org/show_bug.cgi?id=633366
Created attachment 239022 [details] Patch
I'm not sure if your test works. The server isn't set up to use HTTP authentication; I think you need to add a SoupAuthDomain for "/auth" using soup_server_add_auth_domain().
(In reply to comment #2) > I'm not sure if your test works. The server isn't set up to use HTTP authentication; I think you need to add a SoupAuthDomain for "/auth" using soup_server_add_auth_domain(). I wrote the test before the patch, the test does the same than the auth tests
OK, I guess the WWW-Authenticate header is sufficient.
Comment on attachment 239022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239022&action=review The change looks conceptually good to me. I was wondering if it wouldn't be better to listen to tls-errors notifications from the SoupMessage instead of having to handle these things in different parts of our code... > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:-577 > - if (!response.soupMessageTLSErrors() || gIgnoreSSLErrors) I guess this deserves a comment explaining that in the case of authentication challenge, the response has not been updated with the SoupMessage contents yet. > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:593 > + handle->client()->didFail(handle, ResourceError::tlsError(d->m_soupRequest.get(), tlsErrors, certificate)); So I guess both soupMessageCertificate() and soupMessageTLSErrors() became unused methods with these changes. Time to remove them?
(In reply to comment #5) > (From update of attachment 239022 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239022&action=review > > The change looks conceptually good to me. I was wondering if it wouldn't be better to listen to tls-errors notifications from the SoupMessage instead of having to handle these things in different parts of our code... Yes, but unfortunately libsoup doesn't emit notify for tls-errors nor flags properties. > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:-577 > > - if (!response.soupMessageTLSErrors() || gIgnoreSSLErrors) > > I guess this deserves a comment explaining that in the case of authentication challenge, the response has not been updated with the SoupMessage contents yet. Ok. > > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:593 > > + handle->client()->didFail(handle, ResourceError::tlsError(d->m_soupRequest.get(), tlsErrors, certificate)); > > So I guess both soupMessageCertificate() and soupMessageTLSErrors() became unused methods with these changes. Time to remove them? No, those are used by CertificateInfo when created for a ResourceResponse
Comment on attachment 239022 [details] Patch I'll r+ it now but I'd like to hear Dan's opinion on the matter before landing.
(In reply to comment #6) > (In reply to comment #5) > > The change looks conceptually good to me. I was wondering if it wouldn't be better to listen to tls-errors notifications from the SoupMessage instead of having to handle these things in different parts of our code... > > Yes, but unfortunately libsoup doesn't emit notify for tls-errors nor flags properties. Hm... it's a bug that :flags doesn't get notified when CERTIFICATE_TRUSTED changes. But :tls-errors definitely should be getting notified (and the flags will have already changed at the point when :tls-errors is notified).
So, Dan confirmed I was wrong, I was not getting the tls-errors notify signal because I was connecting in request-started and it's too late. But I realized we are connecting to got-headers, that is also emitted before authenticate and tls errors are already set at that point so it looks like a good place to handle TLS errors.
Created attachment 239338 [details] Patch Updated patch, now TLS errors are handled in a single place.
Comment on attachment 239338 [details] Patch Much better now.
Committed r174394: <http://trac.webkit.org/changeset/174394>