Bug 120160

Created attachment 212175 [details] WIP Patch v1

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Comment on attachment 212175 [details] WIP Patch v1 Attachment 212175 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1865193

Comment on attachment 212175 [details] WIP Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=212175&action=review > Source/WebKit2/Shared/APIObject.h:110 > + TypeSSLPermissionRequest, What do you need this for?

Comment on attachment 212175 [details] WIP Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=212175&action=review I think we should use TLS instead of SSL in the class name, for consistency with glib but also with our current API where we expose TLSErrorsPolicy. I think it should be better documented somewhere that when you set the policy to ASK, the permission-request signal is emitted with a TLS permission request object. I have doubts about the loading progress, but I think the current load should always finish, and a new load should be started when the request is allowed. > Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:39 > + * of an untrusted certificate. You should explain here that this is only used when the policy is ASK. > Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:40 > + */ Since: 2.4 > Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:42 > +static void webkit_permission_request_interface_init(WebKitPermissionRequestIface*); This should use WebKit coding style, since it's private and it's not generated. > Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:67 > + webkitWebContextGetContext(webkit_web_view_get_context(priv->webView))->allowSpecificHTTPSCertificateForHost(priv->certificateInfo, priv->host); Add a new private method to WebKitWebContext instead, webkitWebContextAllowSpecificHTTPSCertificateForHost(), it makes it easier to read. I think it's better to create the WebCertificateInfo here. > Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:68 > + webkitWebViewEmitLoadChanged(priv->webView, WEBKIT_LOAD_FINISHED); This should not change the load progress in my opinion. > Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:82 > + webkitWebViewLoadFailed(priv->webView, WEBKIT_LOAD_STARTED, priv->failingURI, priv->error); Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.h:61 > + We should add API to return TLSErrors and TLSCertificate at least, and probably also the host > Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:26 > +#include "WebKitSSLPermissionRequestPrivate.h" Why is this needed here if you are not adding new code to this file? > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:200 > - priv->tlsErrorsPolicy = WEBKIT_TLS_ERRORS_POLICY_IGNORE; > + priv->tlsErrorsPolicy = WEBKIT_TLS_ERRORS_POLICY_ASK; Why are you changing the default? How does the tls-errors-policy unit tests pass after this? > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:68 > + * @WEBKIT_TLS_ERRORS_POLICY_ASK: Ask for permission to load the page on a TLS error. Since 2.4 > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1507 > + } else if (tlsErrorsPolicy == WEBKIT_TLS_ERRORS_POLICY_ASK) { Do not use an else after a return. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1509 > + RefPtr<WebCertificateInfo> webCertificateInfo = WebCertificateInfo::create(platformCerticateInfo); > + GRefPtr<WebKitSSLPermissionRequest> request = adoptGRef(webkitSSLPermissionRequestCreate(webView, webCertificateInfo.get(), "", failingURI, error)); I don't understand this change, why not just creating the request with certificate flags + certificate instead of allocating this WebCertificateInfo here? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1511 > + webkitWebViewMakePermissionRequest(webView, WEBKIT_PERMISSION_REQUEST(request.get())); > + return; I think we should finish the load here. The difference is that when policy is ask, the user can show a SSL errors page. So a new load for the error page might happen. The error page would have a button to allow the user to ignore the errors and load the page anyway, but in this case, we call allow() to send the message to allow the certificate for the given host, and then we load again, so it's actually a new load. This is not like HTTS auth. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:31 > +static const char SSLExpectedSuccessTitle[] = "WebKit2Gtk+ Authentication test"; Authentication test? > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:152 > + g_signal_connect(m_webView, "permission-request", G_CALLBACK(permissionRequested), this); You should also disconnect the signal in the destructor. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:180 > + g_assert(test->m_loadFailed); I think the load should only fail when policy is FAIL, otherwise we would only have IGNORE and ASK. > Source/WebKit2/UIProcess/WebContext.cpp:1099 > + sendToAllProcesses(Messages::WebProcess::AllowSpecificHTTPSCertificateForHost(certificate->platformCertificateInfo(), host)); I think this should be soup specific, since you are implementing the message receiver in WebProcessSoup.cpp only > Source/WebKit2/WebProcess/WebProcess.messages.in:95 > +#if !ENABLE(NETWORK_PROCESS) > + AllowSpecificHTTPSCertificateForHost(WebKit::PlatformCertificateInfo certificate, WTF::String host) > +#endif Ditto.

Adding WebKit2 owners for the cross-platforms changes.

Comment on attachment 212175 [details] WIP Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=212175&action=review Thanks for the comments, its been very helpful. This was a first WIP patch to check whether my proposal was broadly correct and it seems that with the exception of the load handling it was, so I will shortly be uploading a new patch to address the comments. >> Source/WebKit2/Shared/APIObject.h:110 >> + TypeSSLPermissionRequest, > > What do you need this for? I don't, good catch. >> Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:39 >> + * of an untrusted certificate. > > You should explain here that this is only used when the policy is ASK. Sure. >> Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:42 >> +static void webkit_permission_request_interface_init(WebKitPermissionRequestIface*); > > This should use WebKit coding style, since it's private and it's not generated. Ok. FWIW I copied this from GeolocationPermissionRequest. >> Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:67 >> + webkitWebContextGetContext(webkit_web_view_get_context(priv->webView))->allowSpecificHTTPSCertificateForHost(priv->certificateInfo, priv->host); > > Add a new private method to WebKitWebContext instead, webkitWebContextAllowSpecificHTTPSCertificateForHost(), it makes it easier to read. I think it's better to create the WebCertificateInfo here. ok >> Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.cpp:68 >> + webkitWebViewEmitLoadChanged(priv->webView, WEBKIT_LOAD_FINISHED); > > This should not change the load progress in my opinion. Ok. >> Source/WebKit2/UIProcess/API/gtk/WebKitSSLPermissionRequest.h:61 >> + > > We should add API to return TLSErrors and TLSCertificate at least, and probably also the host Sure, will do that in the next round. >> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:26 >> +#include "WebKitSSLPermissionRequestPrivate.h" > > Why is this needed here if you are not adding new code to this file? You're right, it's not necessary. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:200 >> + priv->tlsErrorsPolicy = WEBKIT_TLS_ERRORS_POLICY_ASK; > > Why are you changing the default? How does the tls-errors-policy unit tests pass after this? Oops, that was test code (while I was developing) that slipped in. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1507 >> + } else if (tlsErrorsPolicy == WEBKIT_TLS_ERRORS_POLICY_ASK) { > > Do not use an else after a return. Ok, will refactor that. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1509 >> + GRefPtr<WebKitSSLPermissionRequest> request = adoptGRef(webkitSSLPermissionRequestCreate(webView, webCertificateInfo.get(), "", failingURI, error)); > > I don't understand this change, why not just creating the request with certificate flags + certificate instead of allocating this WebCertificateInfo here? It definitely wasn't my first choice, I would have done exactly what you suggested except that there is no way of constructing a WebKit::PlatformCertificateInfo (which is part of the message passed to the WebProcess) from the certificate and the flags, but it is possible to get both certificate and flags, and also the PlatformCertificateInfo from WebCertificateInfo. >> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:31 >> +static const char SSLExpectedSuccessTitle[] = "WebKit2Gtk+ Authentication test"; > > Authentication test? Copy/paste :/ >> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:152 >> + g_signal_connect(m_webView, "permission-request", G_CALLBACK(permissionRequested), this); > > You should also disconnect the signal in the destructor. Ok. >> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:180 >> + g_assert(test->m_loadFailed); > > I think the load should only fail when policy is FAIL, otherwise we would only have IGNORE and ASK. Fair point. >> Source/WebKit2/UIProcess/WebContext.cpp:1099 >> + sendToAllProcesses(Messages::WebProcess::AllowSpecificHTTPSCertificateForHost(certificate->platformCertificateInfo(), host)); > > I think this should be soup specific, since you are implementing the message receiver in WebProcessSoup.cpp only Sure.

Created attachment 213171 [details] Patch

Comment on attachment 213171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213171&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:61 > + PlatformCertificateInfo certificateInfo(resourceError); > + webkitWebViewLoadFailedWithTLSErrors(WEBKIT_WEB_VIEW(clientInfo), resourceError.failingURL().utf8().data(), webError.get(), certificateInfo); I still don't see the point of using this PlatformCertificateInfo object here instead of passing certificate + flags like we currently do. If we need to add new API to PlatformCertificateInfo to create it with certificate + flags we can always add it. In any case, hopefully this is going to change soon, see bug #118520. > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:44 > + * the permission-request signal is emitted with this WebKitTLSPermissionRequest. #WebKitWebView::permission-request > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:99 > + request->priv->webView = webView; It seems we only want the view to get its web context, why not passing the context instead since it's what we actually want? > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:118 > +GTlsCertificate* webkit_tls_permission_request_get_certificate(WebKitTLSPermissionRequest *request) This should be documented. > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:125 > +GTlsCertificateFlags webkit_tls_permission_request_get_tls_errors(WebKitTLSPermissionRequest *request) Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:132 > +const gchar* webkit_tls_permission_request_get_host(WebKitTLSPermissionRequest *request) Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.h:61 > +webkit_tls_permission_request_get_type (void); Parameters should be lined up with all other methods. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:908 > + context->priv->context->allowSpecificHTTPSCertificateForHost(certificate, String(host)); I think we should allocate the WebCertificateInfo here, right before using it. host is utf8, you should use String::fromUTF8() here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1522 > + WebCore::URL uri(ParsedURLString, String(failingURI)); failingURI is UTF8, you should use String::fromUTF8(). Or you could create a SoupURI for the given failingURI and pass soup_uri->host directly to avoid charset conversions. > Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:869 > webkit_uri_scheme_request_get_scheme You should also add the new type (webkit_tls_permission_request_get_type) to the file webkit2gtk.types

Comment on attachment 213171 [details] Patch Thanks for the review!

Created attachment 213272 [details] Patch

Comment on attachment 213272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213272&action=review I'm sorry I was not clear enough in previous reviews about the use of WebCertificateInfo and PlatformCertificateInfo. May only concern was about unnecessary allocating a WebCertificate object. Patch looks good to me, except for the minor details I commented and the lack of WebKitTLSRequest API test. It would be great if other GTK+ reviewers could review the public API and a WebKit2 owner the cross-platform changes. > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:55 > + GTlsCertificateFlags tlsErrors; > + GRefPtr<GTlsCertificate> certificate; We could use a stack allocated PlatformCertificateInfo here and set the certificate/flags in the init, that way we don't need the new PlatformCertificateInfo constructor, but only the set methods added in bug #118520. May main concern was allocating the WebCertificate object here, because it's only used internally > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:74 > + webkitWebContextAllowSpecificHTTPSCertificateForHost(priv->context, priv->tlsErrors, priv->certificate.get(), priv->host.data()); Then you could pass the internal PlatformCertificateInfo here as a const reference > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:103 > + request->priv->tlsErrors = tlsErrors; > + request->priv->certificate = certificate; Here you would call certificateInfo.setCertificate and certificateInfo.setTLSErrors > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:125 > + * Get the #GTlsCertificate certificate associated with this the #GTlsCertificate certificate sounds a bit redundant > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequestPrivate.h:23 > +#include "WebCertificateInfo.h" You don't need this here anymore. > Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequestPrivate.h:26 > +#include "WebKitWebView.h" Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:24 > +#include "PlatformCertificateInfo.h" > +#include "WebCertificateInfo.h" WebCertificateInfo.h already includes PlatformCertificateInfo.h > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1521 > + GOwnPtr<SoupURI> soup_uri(soup_uri_new(failingURI)); soup_uri -> soupURI > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:206 > +} I have just realized we are not testing the WebKitPermissionRequest API here, you should check that get_certificate returns a valid certificate (and even that it's deleted when test finishes), that tls_errors returns the expected errors and get_host returns the expected host. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:273 > + // In this case the order of the tests does matter because tls-errors-policy tests the default policy. > + SSLTest::add("WebKitWebView", "tls-errors-policy", testTLSErrorsPolicy); If any other test changes the default value, it should ensure that when the test finishes the value is set to the default again, so that it doesn't affect to other tests.

Comment on attachment 213272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213272&action=review Thanks again for the review. >> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:55 >> + GRefPtr<GTlsCertificate> certificate; > > We could use a stack allocated PlatformCertificateInfo here and set the certificate/flags in the init, that way we don't need the new PlatformCertificateInfo constructor, but only the set methods added in bug #118520. May main concern was allocating the WebCertificate object here, because it's only used internally Aha. That makes sense, and I'll update the patch to that (and make it depend on #118520). >> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:103 >> + request->priv->certificate = certificate; > > Here you would call certificateInfo.setCertificate and certificateInfo.setTLSErrors Ok, will do that >> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequest.cpp:125 >> + * Get the #GTlsCertificate certificate associated with this > > the #GTlsCertificate certificate sounds a bit redundant sure >> Source/WebKit2/UIProcess/API/gtk/WebKitTLSPermissionRequestPrivate.h:26 >> +#include "WebKitWebView.h" > > Ditto. Good catch >> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:24 >> +#include "WebCertificateInfo.h" > > WebCertificateInfo.h already includes PlatformCertificateInfo.h ok >> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:206 >> +} > > I have just realized we are not testing the WebKitPermissionRequest API here, you should check that get_certificate returns a valid certificate (and even that it's deleted when test finishes), that tls_errors returns the expected errors and get_host returns the expected host. Good point, sorry about that! >> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:273 >> + SSLTest::add("WebKitWebView", "tls-errors-policy", testTLSErrorsPolicy); > > If any other test changes the default value, it should ensure that when the test finishes the value is set to the default again, so that it doesn't affect to other tests. Hrm. Maybe I should improve on that comment by stating that we need to check the default policy first because once we've allowed an exception for this host (127.0.0.1) it won't return a TLS error. So the order will still matter (a bit like authentication success).

Comment on attachment 213272 [details] Patch So, here's my opinion after understanding how this works: the permission request APIs we currently have are also in a way actions - when you allow a geolocation request webcore will go ahead and provide proceed with obtaining the location, when you deny it, webcore will tell the backend it's denied, and the content will be able to show an error. I think it would be bad to reuse these methods with such a different meaning and requiring the API user to take a second action after using them. So I think I would prefer if we add public API to WebContext to let the browser add specific certificates for a host (so the method allow is currently calling) and use a model that's more a notification than a permission request, which is more consistent with how this works.

Cross platform WK2 parts look fine.

Created attachment 213695 [details] New Patch

Comment on attachment 213695 [details] New Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213695&action=review > Source/WebKit2/Shared/soup/PlatformCertificateInfo.h:52 > + void setCertificate(GTlsCertificate* certificate) {m_certificate = certificate; } This is only while we're waiting for #118520 to land.

(In reply to comment #17) > (From update of attachment 213695 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=213695&action=review > > > Source/WebKit2/Shared/soup/PlatformCertificateInfo.h:52 > > + void setCertificate(GTlsCertificate* certificate) {m_certificate = certificate; } > > This is only while we're waiting for #118520 to land. Also, I only removed "r?" because of this, but I'm still very interested in feedback and comments!

Comment on attachment 213695 [details] New Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213695&action=review > Source/WebKit2/Shared/soup/PlatformCertificateInfo.h:53 > + void setCertificate(GTlsCertificate* certificate) {m_certificate = certificate; } > + void setTLSErrors(GTlsCertificateFlags tlsErrors) {m_tlsErrors = tlsErrors; } In case it's not obvious, Brian just explained to me that all the changes to this file will go away after that patch for bug 118520 land. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:125 > + LOAD_FAILED_WITH_TLS_ERRORS, I would move this up, together with LOAD_FAILED > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:631 > + webViewClass->load_failed_with_tls_errors = webkitWebViewLoadFailedWithTLSErrorsDefaultHandler; I would probably move this up too together with the default handler of load-failed. Also, I would rename the default handler to webkitWebViewLoadFailWithTLSErrors, for consistency with the naming of the default handler for that other signal (and also because introducing a new suffix here will be IMHO more confusing than helpful). BUT, this is probably not needed anyway if you get rid of this default handler (see below) > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1435 > + * GTlsCertificateFlags and the host. To allow an exception for this GTlsCertificateFlags -> #GTlsCertificateFlags > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1439 > + * The default signal handler frees the WebKitCertificateInfo object. WebKitCertificateInfo -> #WebKitCertificateInfo > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1446 > + signals[LOAD_FAILED_WITH_TLS_ERRORS] = For consistency with my previous comments, I would move this up. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1563 > + WebKitCertificateInfo* info = webkit_certificate_info_new(tlsErrors, certificate, soupURI->host); > + gboolean returnValue; > + g_signal_emit(webView, signals[LOAD_FAILED_WITH_TLS_ERRORS], 0, info, &returnValue); I think you should create the WebKitCertificateInfo instance in the stack instead of the heap and then pass the reference to it, since that way neither the default handler that you have now (and that would disappear anyway) nor the specific handler an app might be providing would need to free the memory themselves. It will just get released after the call to g_signal_emit() finishes, when leaving the scope. You probably have to move the definition of the struct from the cpp file to the private header. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:35 > +static const char TLSSuccessHTMLString[] = > + "<html>" > + "<head><title>WebKit2Gtk+ TLS permission test</title></head>" > + "<body></body></html>"; You can put this in a single line if you want.

Comment on attachment 213695 [details] New Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213695&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:23 > +#include "PlatformCertificateInfo.h" This is already included by WebKitCertificateInfoPrivate.h > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:32 > + _WebKitCertificateInfo(const PlatformCertificateInfo& certificateInfo, const gchar* host) > + : certificateInfo(certificateInfo) > + , host(host) I think WebKitCertificateInfo should simply wrap PlatformCertificateInfo, do we really need to keep the host in the WebKitCertificateInfo object? > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:67 > +WebKitCertificateInfo* webkit_certificate_info_new(GTlsCertificateFlags tlsErrors, GTlsCertificate *certificate, const gchar *host) Why is this public? I don't think the user needs to create a WebKitCertificateInfo > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:113 > + * webkit_certificate_info_get_certificate: get_tls_certificate, maybe? > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:819 > +void webkit_web_context_allow_specific_tls_certificate(WebKitWebContext* context, WebKitCertificateInfo* info) I would pass the host as a parameter here, and possibly rename it as something like webkit_web_context_allow_tls_certificate_for_host > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:69 > + * @WEBKIT_TLS_ERRORS_POLICY_ASK: Ask for permission to load the page on a TLS error. Since 2.4 It's not exactly asking permission to load the page, because the page load finishes in any case. Maybe we should rename this as NOTIFY. Or maybe we don't even need a new policy for this, if we always emit the signal load-failed-with-tls-errors when load-failed, and if not handled we fallback to load-failed making the load fail. If handled, we just finish the load. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:631 >> + webViewClass->load_failed_with_tls_errors = webkitWebViewLoadFailedWithTLSErrorsDefaultHandler; > > I would probably move this up too together with the default handler of load-failed. > > Also, I would rename the default handler to webkitWebViewLoadFailWithTLSErrors, for consistency with the naming of the default handler for that other signal (and also because introducing a new suffix here will be IMHO more confusing than helpful). BUT, this is probably not needed anyway if you get rid of this default handler (see below) You can also get rid of the default handler by freeing the certificate info after emitting the signal. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1430 > + * @info: a #WebKitCertificateInfo We could pass the host here as a signal argument. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1432 > + * Emitted when a tls error occurs during a load operation and the tls tls -> TLS > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1433 > + * policy is WEBKIT_TLS_ERRORS_POLICY_ASK. The @info object contains WEBKIT_TLS_ERRORS_POLICY_ASK -> %WEBKIT_TLS_ERRORS_POLICY_ASK > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1559 > + } else if (tlsErrorsPolicy == WEBKIT_TLS_ERRORS_POLICY_ASK) { Do not use an else after a return; > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1561 > + WebKitCertificateInfo* info = webkit_certificate_info_new(tlsErrors, certificate, soupURI->host); You should use webkitCertificateInfoCreate here. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1563 >> + g_signal_emit(webView, signals[LOAD_FAILED_WITH_TLS_ERRORS], 0, info, &returnValue); > > I think you should create the WebKitCertificateInfo instance in the stack instead of the heap and then pass the reference to it, since that way neither the default handler that you have now (and that would disappear anyway) nor the specific handler an app might be providing would need to free the memory themselves. It will just get released after the call to g_signal_emit() finishes, when leaving the scope. > > You probably have to move the definition of the struct from the cpp file to the private header. We should document in the signal api doc that to handle this asynchronously you should copy the given WebKitCertificateInfo (with webkit_certificate_info_copy()), we should not expect the user have to free the object unless it's copied. Using a stack allocated certificate info as Mario suggests, in case of handling this synchronously we would avoid a heap allocation here. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:154 > + static gboolean runLoadFailedWithTlsErrorsCallback(WebKitWebView*, WebKitCertificateInfo* info, TLSErrorsTest* test) Tls -> TLS > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:160 > + void runLoadFailedWithTlsErrors(WebKitCertificateInfo* info) Ditto. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:166 > + WebKitCertificateInfo* waitForLoadFailedWithTlsErrors() Ditto. waitUntilLoadFailedWithTlsErrors? for consistency with other waitUntil methods that we have. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:179 > + WebKitWebContext* context = webkit_web_view_get_context(test->m_webView); > + webkit_web_context_set_tls_errors_policy(context, WEBKIT_TLS_ERRORS_POLICY_ASK); Before changing the policy, we might also test that load-failed-with-tls-errors is not emitted. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:181 > + // Test the WebKitTLSPermissionRequest API Nit: Finish the comment with a period. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:184 > + g_assert(webkit_certificate_info_get_certificate(info)); You could use G_IS_TLS_CERTIFICATE

Thanks for all the comments, I'll do another patch shortly.

Created attachment 213777 [details] Patch

Comment on attachment 213777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213777&action=review I'm mostly ok with this patch (regardless of the nitpicking), but at least Carlos should take a look to it anyway > Source/WebKit2/Shared/soup/PlatformCertificateInfo.h:53 > + void setCertificate(GTlsCertificate* certificate) {m_certificate = certificate; } > + void setTLSErrors(GTlsCertificateFlags tlsErrors) {m_tlsErrors = tlsErrors; } Missing blank after { (Why doesn't the style checker pick this up?) > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.h:28 > +#include <glib-object.h> > +#include <gio/gio.h> Wrong alphabetical order > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:854 > + * @host: the host on whch the error occurred whch -> which > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:858 > + * and the #GTlsCertificateFlags.To allow an exception for this certificate .To -> ". To" > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:35 > +static const char TLSSuccessHTMLString[] = > + "<html>" > + "<head><title>WebKit2Gtk+ TLS permission test</title></head>" > + "<body></body></html>"; You like multiple lines, huh? :) > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:165 > + if (m_certificateInfo) > + webkit_certificate_info_free(info); free m_certificateInfo here > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:182 > +static void testLoadFailedWithTlsErrors(TLSErrorsTest* test, gconstpointer) testLoadFailedWithTlsErrors -> testLoadFailedWithTLSErrors > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:278 > + Extra line unneeded > Source/WebKit2/WebProcess/WebProcess.messages.in:87 > +#if !ENABLE(NETWORK_PROCESS) > +#if USE(SOUP) #if !ENABLE(NETWORK_PROCESS) && USE(SOUP) ?

Comment on attachment 213777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213777&action=review >> Source/WebKit2/WebProcess/WebProcess.messages.in:87 >> +#if USE(SOUP) > > #if !ENABLE(NETWORK_PROCESS) && USE(SOUP) ? It should be one because currently nested #if-s are not supported by the generate-message-receiver.py. There is patch waiting to signal such error: https://bugs.webkit.org/show_bug.cgi?id=121877 till correct handling of nested #if-s are implemented.

(In reply to comment #24) > [...] > It should be one because currently nested #if-s are not supported by the generate-message-receiver.py. There is patch waiting to signal such error: https://bugs.webkit.org/show_bug.cgi?id=121877 till correct handling of nested #if-s are implemented. Thanks for the link. Strangely, the patch provided for TLS errors works and the unit test passes, so I'm a bit surprised to hear that nested #if-s are not supported.

Comment on attachment 213777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213777&action=review API looks good to me, except what I comment below about the load-failed and load-failed-with-tls-errors signals. Gustavo? > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:32 > + * @See_also: #WebKitWebView Add #WebKitWebContext here too, since it uses the certificate info. > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:35 > + * WebKit will fire a #WebKitWebView::load-failed-with-tls-errors with a a #WebKitWebView::load-failed-with-tls-errors signal > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfo.cpp:39 > + * To handle this signal asynchronously you should copy the #WebKitCertificateInfo > + * with #WebKitCertificateInfo::webkit_certificate_info_copy. #WebKitCertificateInfo::webkit_certificate_info_copy -> webkit_certificate_info_copy() > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:862 > + * with #webkit_certificate_info_copy. #webkit_certificate_info_copy -> webkit_certificate_info_copy() and return %TRUE. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1550 > webkitWebViewLoadFailed(webView, WEBKIT_LOAD_STARTED, failingURI, error); This will emit the failed signal. I think this should be the fallback when the new signal is not handled. Imagine an application handling both failed and filed-with-tls-errors, if we emit failed first, the app doesn't know that with-tls will be mitted later, and will show an error page with the error. And apps handling with-tls-errors, are not interested in handling the error again in load-failed. This is the way we avoid the new policy. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1555 > + PlatformCertificateInfo certificateInfo; > + certificateInfo.setCertificate(certificate); > + certificateInfo.setTLSErrors(tlsErrors); For this particular case I think it makes sense to add a PlatformCertificateInfo constructor taking certificate + tls errors. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1556 > + WebKitCertificateInfo info(certificateInfo); Or maybe it would be better to add a WebKitCertificateInfo taking the certificate + flags instead. That way we avoid creating the temporary PlatformCertificateInfo here. The constructor would call setCertificate and setTLSErrors on its PlatformCertificateInfo > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:249 > + gboolean (* load_failed_with_tls_errors) (WebKitWebView *web_view, > + WebKitCertificateInfo *info); The host parameter is missing here. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:174 > + return; Useless return here. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:179 > + Extra line here. > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:191 > + g_assert(G_IS_TLS_CERTIFICATE(webkit_certificate_info_get_tls_certificate(test->m_certificateInfo))); Better add getters for certificate and host

Created attachment 213877 [details] Patch

Comment on attachment 213877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213877&action=review This looks much better, there's just still a minor issue with the signal emission. Note also that PlatformCertificateInfo will be renamed as CertificateInfo in bug #118520. > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfoPrivate.h:31 > + : certificateInfo(tlsErrors, certificate) > + { > + } I actually meant calling setCertificate and setTLSErrors here so that we don't need the new PlatformCertificateInfo constructor, but in any case the new constructor makes sense to me. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:859 > + * and this host use #webkit_web_context_allow_tls_certificate_for_host. #webkit_web_context_allow_tls_certificate_for_host -> webkit_web_context_allow_tls_certificate_for_host() I guess missed this one in previous review, but the rule is the same, simply use foo() for functions. #Foo for classes and %FOO for enum values > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1556 > + gboolean returnValue; > + if (g_signal_has_handler_pending(webView, signals[LOAD_FAILED_WITH_TLS_ERRORS], 0, TRUE)) { > + GOwnPtr<SoupURI> soupURI(soup_uri_new(failingURI)); > + WebKitCertificateInfo info(tlsErrors, certificate); > + g_signal_emit(webView, signals[LOAD_FAILED_WITH_TLS_ERRORS], 0, &info, soupURI->host, &returnValue); > + } else > + g_signal_emit(webView, signals[LOAD_FAILED], 0, WEBKIT_LOAD_STARTED, failingURI, error, &returnValue); Emit the signal and check the return value. If return value is FALSE (unhandled) you emit the load-failed signal. I think the default value is FALSE in case nobody connects to the signal, but you can always add a default handler that simply returns FALSE to be extra sure. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:250 > + const gchar host); Nit: host should be lined up with info, not with the *

Thanks for the heads up here. I'll take a whack at implementing this in uzbl tonight to see how it works out.

Created attachment 213881 [details] Patch

Comment on attachment 213881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213881&action=review I'm happy with the API, bit worried with the slight change in behaviour at the emission site. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1555 > + g_signal_emit(webView, signals[LOAD_FAILED], 0, WEBKIT_LOAD_STARTED, failingURI, error, &returnValue); Why not keep using webkitWebViewLoadFailed here? It seems to do some useful work cancelling auth and setting IsLoading to false.

Comment on attachment 213881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=213881&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1555 >> + g_signal_emit(webView, signals[LOAD_FAILED], 0, WEBKIT_LOAD_STARTED, failingURI, error, &returnValue); > > Why not keep using webkitWebViewLoadFailed here? It seems to do some useful work cancelling auth and setting IsLoading to false. Because the same work is already done at the beginning of this function.

Created attachment 214254 [details] Rebased patch

Comment on attachment 214254 [details] Rebased patch Attachment 214254 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4097015

Comment on attachment 214254 [details] Rebased patch Attachment 214254 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4097016

Created attachment 214258 [details] Patch

(In reply to comment #29) > Thanks for the heads up here. I'll take a whack at implementing this in uzbl tonight to see how it works out. Did you give it a try? Any thoughts?

Comment on attachment 214258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214258&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:68 > + * @WEBKIT_TLS_ERRORS_POLICY_FAIL: TLS errors make the load finish with an error and a notification signal. I think you should add a cross-rerefence to the signal here. #WebKitWebView::load-failed-with-tls-errors and make it explicit it's emitted *before* the load is finished. How about: TLS errors will emit #WebKitWebView::load-failed-with-tls-errors and, if the signal is handled, finish the load. In case the signal is not handled, #WebKitWebView::load-failed is emitted before the load finishes.

Comment on attachment 214258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214258&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:68 >> + * @WEBKIT_TLS_ERRORS_POLICY_FAIL: TLS errors make the load finish with an error and a notification signal. > > I think you should add a cross-rerefence to the signal here. #WebKitWebView::load-failed-with-tls-errors and make it explicit it's emitted *before* the load is finished. How about: > > TLS errors will emit #WebKitWebView::load-failed-with-tls-errors and, if the signal is handled, finish the load. In case the signal is not handled, #WebKitWebView::load-failed is emitted before the load finishes. Good idea, I'll be happy to do that before landing.

Comment on attachment 214258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214258&action=review > Source/WebCore/platform/network/CertificateInfo.h:47 > + explicit CertificateInfo(GTlsCertificateFlags, GTlsCertificate*); I think it would be better to pass the certificate as a first argument instead. That way if we eventually use the CertificateInfo to wrap a certificate with no tls errors, we could make the flags argument optional by giving it a default value of 0 (which means no errors) > Source/WebKit2/UIProcess/API/gtk/WebKitCertificateInfoPrivate.h:28 > + _WebKitCertificateInfo(GTlsCertificateFlags tlsErrors, GTlsCertificate* certificate) And we might change the order here too > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:861 > + * We should also document here what happens when the user returns TRUE/FALSE (load finishes/load-failed is emitted and load finishes)

Created attachment 214557 [details] Patch for landing

Comment on attachment 214557 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=214557&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:863 > + * To handle this signal asynchronously you should copy the #WebKitCertificateInfo > + * with webkit_certificate_info_copy() and return %TRUE in which case the load I would use a different paragraph, because the return value has nothing to do with whether the signal is handled synchronously or asynchronously. I mean TRUE and FALSE will do the same in both cases. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:864 > + * will finish. If %FALSE is returned, #load-failed is emitted and and the #load-failed -> #WebKitWebView::load-failed > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:866 > + * Remove this extra line.

Comment on attachment 214557 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=214557&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:863 >> + * with webkit_certificate_info_copy() and return %TRUE in which case the load > > I would use a different paragraph, because the return value has nothing to do with whether the signal is handled synchronously or asynchronously. I mean TRUE and FALSE will do the same in both cases. What about: * To handle this signal asynchronously you should copy the #WebKitCertificateInfo * with webkit_certificate_info_copy() and return %TRUE. * * If %FALSE is returned, #WebKitWebView::load-failed will be emitted. The load * will finish regardless of the returned value. >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:864 >> + * will finish. If %FALSE is returned, #load-failed is emitted and and the > > #load-failed -> #WebKitWebView::load-failed Ah, I didn't realise that

Created attachment 214720 [details] Patch for landing

(In reply to comment #43) > (From update of attachment 214557 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=214557&action=review > > >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:863 > [...] > > What about: > > * To handle this signal asynchronously you should copy the #WebKitCertificateInfo > * with webkit_certificate_info_copy() and return %TRUE. > * > * If %FALSE is returned, #WebKitWebView::load-failed will be emitted. The load > * will finish regardless of the returned value. > I didn't properly address the point that handling this signal asynchronously has nothing to do with the load. This proposed text should be clearer: * To handle this signal asynchronously you should copy the #WebKitCertificateInfo * with webkit_certificate_info_copy(). * * If %FALSE is returned, #WebKitWebView::load-failed will be emitted. The load * will finish regardless of the returned value, emitting #WebKitWebView::load-changed * with %WEBKIT_LOAD_FINISHED.

Comment on attachment 214557 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=214557&action=review >>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:863 >>>> + * with webkit_certificate_info_copy() and return %TRUE in which case the load >>> >>> I would use a different paragraph, because the return value has nothing to do with whether the signal is handled synchronously or asynchronously. I mean TRUE and FALSE will do the same in both cases. >> >> What about: >> >> * To handle this signal asynchronously you should copy the #WebKitCertificateInfo >> * with webkit_certificate_info_copy() and return %TRUE. >> * >> * If %FALSE is returned, #WebKitWebView::load-failed will be emitted. The load >> * will finish regardless of the returned value. > > I didn't properly address the point that handling this signal asynchronously has nothing to do with the load. This proposed text should be clearer: > > * To handle this signal asynchronously you should copy the #WebKitCertificateInfo > * with webkit_certificate_info_copy(). > * > * If %FALSE is returned, #WebKitWebView::load-failed will be emitted. The load > * will finish regardless of the returned value, emitting #WebKitWebView::load-changed > * with %WEBKIT_LOAD_FINISHED. What about this? (first part is the same): * To handle this signal asynchronously you should copy the #WebKitCertificateInfo * with webkit_certificate_info_copy(). * * If the signal is not handled or %FALSE is returned in the callback, * #WebKitWebView::load-failed will be emitted too. * * Finally, since a load error causes the load operation to finish, the signal * WebKitWebView::load-changed will always be emitted with * %WEBKIT_LOAD_FINISHED event right after this one.

Comment on attachment 214720 [details] Patch for landing I think we have iterated enough here. Feel free to improve the documentation in a follow up patch.

Comment on attachment 214720 [details] Patch for landing Clearing flags on attachment: 214720 Committed r157781: <http://trac.webkit.org/changeset/157781>

All reviewed patches have been landed. Closing bug.

(In reply to comment #48) > (From update of attachment 214720 [details]) > Clearing flags on attachment: 214720 > > Committed r157781: <http://trac.webkit.org/changeset/157781> Reopen, because the base of this patch was rolled out by Anders and the GTK build is now broken. :( See https://bugs.webkit.org/show_bug.cgi?id=118520 for details.

Created attachment 214939 [details] Revert CertificateInfo

Comment on attachment 214939 [details] Revert CertificateInfo View in context: https://bugs.webkit.org/attachment.cgi?id=214939&action=review Looks good to me. I'll land it manually so I can fix the small typo in the ChangeLog > Source/WebKit2/ChangeLog:8 > + Revert back to using PlatformCertificateInfo folloing the rollout folloing -> following

Committed r157853: <http://trac.webkit.org/changeset/157853>

One question: shouldn't the 'host' parameter on the "load-failed-with-tls-errors" signal be a const gchar *? I'm also seeing that GTlsCertificate is kind of bare and only gives a PEM string that needs to be manually pulled apart, but that isn't webkit's problem.

Also, it looks like webkit_web_context_allow_tls_certificate_for_host is missing @since 2.4.

(In reply to comment #54) > One question: shouldn't the 'host' parameter on the "load-failed-with-tls-errors" signal be a const gchar *? It's const gchar *. It's gtk-doc that doesn't use the const in the generated doc, probably because it can't know it. When defining the signal you pass a GType, G_TYPE_STRING in this case. > I'm also seeing that GTlsCertificate is kind of bare and only gives a PEM string that needs to be manually pulled apart, but that isn't webkit's problem. Nothing we can do in WebKit, fee free to file a bug report in GNOME bugzilla.

(In reply to comment #55) > Also, it looks like webkit_web_context_allow_tls_certificate_for_host is missing @since 2.4. Please, don't reuse this bug for new issues, I've already opened bug #123499 with the trivial patch.