[webkit-reviews] review denied: [Bug 120160] [GTK] Add WebKit2 API for SSL errors : [Attachment 212175] WIP Patch v1

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Brian Holt
<brian.holt at samsung.com>'s request for review:
Bug 120160: [GTK] Add WebKit2 API for SSL errors
https://bugs.webkit.org/show_bug.cgi?id=120160

Attachment 212175: WIP Patch v1
https://bugs.webkit.org/attachment.cgi?id=212175&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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))->allowSp
ecificHTTPSCertificateForHost(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(c
ertificate->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.