[webkit-reviews] review denied: [Bug 240221] [GLIB] Add API to set WebView's Content-Security-Policy : [Attachment 459019] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has denied Patrick Griffis
<pgriffis at igalia.com>'s request for review:
Bug 240221: [GLIB] Add API to set WebView's Content-Security-Policy
https://bugs.webkit.org/show_bug.cgi?id=240221

Attachment 459019: Patch

https://bugs.webkit.org/attachment.cgi?id=459019&action=review




--- Comment #2 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 459019
  --> https://bugs.webkit.org/attachment.cgi?id=459019
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459019&action=review

Let's see if Carlos Garcia or Adrian have any alternative proposals for how the
API should look. I was quite tempted to suggest using WebKitWebsitePolicies,
but that is really designed around the decide-policy API, which this is
unrelated to.

> Source/WebKit/ChangeLog:13
> +	   Both of these are required for a complete WebExtension
implementation in
> +	   browsers such as Epiphany.

For posterity: https://gitlab.gnome.org/GNOME/epiphany/-/issues/1777

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:29
> +#include <WebCore/ContentSecurityPolicy.h>

I'm surprised style checker didn't complain about this. <> includes sort
farther down, below all the "" includes.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1476
> +	* In practice this limits the Content-Security-Policies that are
allowed to be set.

Would be interesting to see a little more information here. You know all about
it because you are a CSP expert, but I had to look into
ContentSecurityPolicySourceList.cpp to see that it was doing anything at all.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:5148
> +/**
> + * webkit_web_view_set_default_content_security_policy:
> + * @web_view: a #WebKitWebView
> + * @policy: (nullable): a Content-Security-Policy
> + *
> + * This configures created #WebKitWebView's to use @policy as its initial
> + * Content-Security-Policy as if it were set by an HTTP header.

OK, but the GObject property is construct only, so why does this exist? Either
the property should not be construct only, or there should be no setter.

I think construct-only is probably the way to go, because potential for error
is high otherwise. The new policy will only take effect for a new WebPage, but
page swapping is opaque to API users and very confusing. I bet 95% of
developers using WebKit don't understand this. And it would probably be
extremely unusual that you would ever want to call this function, right?

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:225
> + * WebKitWebExtensionMode:

Sadly we hit terminology hell here. WebKitWebExtension is totally unrelated to
WebExtensions. And here you have the first WebKit API for WebExtensions.
Calling it anything other than WebKitWebExtensionMode would be weird, but it's
also absurdly confusing because it's not related to WebKitWebExtension. Maybe
it is OK because bug #226665 proposes to fix this in the GTK 4 and new WPE
APIs. Will be really confusing in the old APIs, though. At least add a sentence
to document that this is for WebExtensions and not WebKitWebExtensions.

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:228
> + * @WEBKIT_WEB_EXTENSION_MODE_MANIFESTV2: For a ManifestV2 extension.
> + * @WEBKIT_WEB_EXTENSION_MODE_MANIFESTV3: For a ManifestV3 extension.

I was hoping we could do without this enum entirely if we limit ourselves to
only supporting Manifest v3, but I see that NONE really needs to be the
default? I suppose enum is best, that way we're covered whenever manifest v4
rolls around.

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:638
> +WEBKIT_API void
> +webkit_web_view_set_web_extension_mode		(WebKitWebView	       
   *web_view,
> +							WebKitWebExtensionMode 
    mode);

Indentation error here

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1720
> +    // Create a new web view with a policy that blocks eval().
> +    auto webView = Test::adoptView(g_object_new(WEBKIT_TYPE_WEB_VIEW,
> +	   "default-content-security-policy", "script-src 'self'", nullptr));
> +
> +    // Ensure JavaScript still functions.
> +    javascriptResult = test->runJavaScriptAndWaitUntilFinished("'allowed'",
&error.outPtr(), webView.get());
> +    g_assert_nonnull(javascriptResult);
> +    g_assert_no_error(error.get());
> +    GUniquePtr<char>
value(WebViewTest::javascriptResultToCString(javascriptResult));
> +    g_assert_cmpstr(value.get(), ==, "allowed");
> +    webkit_javascript_result_unref(javascriptResult);
> +
> +    // Then ensure eval is blocked.
> +    javascriptResult =
test->runJavaScriptAndWaitUntilFinished("eval('allowed')", &error.outPtr(),
webView.get());
> +    g_assert_null(javascriptResult);
> +    g_assert_error(error.get(), WEBKIT_JAVASCRIPT_ERROR,
WEBKIT_JAVASCRIPT_ERROR_SCRIPT_FAILED);

Please also test to ensure eval works when you *don't* use
default-content-security-policy. And test
webkit_web_view_set_default_content_security_policy()/webkit_web_view_get_defau
lt_content_security_policy().

If you retain webkit_web_view_set_default_content_security_policy(), you should
test to see what happens when you change it after the web view is created.

We need tests for WebKitWebExtensionMode too.