[GTK][WPE] Expose setCORSDisablingPatterns

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jan-Michael Brummer]

Expose setCORSDisablingPatterns within gtk part so that we can allow webextension:/// uri schemes in Epiphany.

[Jan-Michael Brummer]

Created attachment 416469 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

Comment on attachment 416469 [details]
Patch

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

I like it!

> Source/WebKit/ChangeLog:8
> +        No new tests (OOPS!).

Constructing a test for this will be a lot harder than just implementing the API, but it's required to land new WebKit API.

Something very simple in WebViewTest.cpp should suffice.

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:594
> +                                                      const gchar * const       *patternList);

gchar* (the * hangs on the type)

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:469
> +void webkit_web_view_set_cors_disabling_patterns(WebKitWebView* webView, const gchar * const *patternList)

Needs a doc comment with introspection annotations and a Since: 2.32 tag.

Also, remember that in WebKit (and most C++ projects) the * hangs on the type, not the variable name. So it's: const gchar* const * patternList

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:477
> +    for (size_t i = 0; patternList[i]; ++i)

We normally use: i++

(Unless there is really some good reason to use prefix increment, which in this case, there is not.)

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:480
> +    auto& page = *webkitWebViewBaseGetPage(reinterpret_cast<WebKitWebViewBase*>(webView));

I was going to say: you can use a GObject cast: WEBKIT_WEB_VIEW_BASE(webView)

But: actually you can't, because WebKitWebViewBase doesn't exist for WPE port. So you'd need an #if PLATFORM(GTK) condition. Then I got confused: why is the WPE EWS green? Finally, I noticed you added this to WebKitWebViewGtk.cpp.

OK, well, there is nothing GTK-specific here. So move this to Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp instead. Then, use getPage(webView) to get the page without having to type WebKitWebViewBase directly, and now this will work for WPE. And lastly, modify Source/WebKit/UIProcess/API/wpe/WebKitWebView.h.

> Source/WebKit/UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt:293
> +webkit_web_view_set_cors_disabling_patterns

Needs to be added to WPE docs as well.

[Michael Catanzaro]

Comment on attachment 416469 [details]
Patch

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

>> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:594
>> +                                                      const gchar * const       *patternList);
> 
> gchar* (the * hangs on the type)

Oops, that's totally wrong because WebKit code style does not apply in the public headers. You had this correct already.

[Jan-Michael Brummer]

Created attachment 416474 [details]
Patch

[Jan-Michael Brummer]

Updated version with fixed review findings, EXCEPT test.

[Carlos Garcia Campos]

Comment on attachment 416474 [details]
Patch

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

r- because we need an API test. Thanks Jan-Michael!

> Source/WebKit/ChangeLog:8
> +        No new tests (OOPS!).

Remove this line.

> Source/WebKit/ChangeLog:13
> +        * UIProcess/API/gtk/WebKitWebView.h:
> +        * UIProcess/API/gtk/WebKitWebViewGtk.cpp:
> +        (webkit_web_view_set_cors_disabling_patterns):
> +        * UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt:

You should regenerate the changelog, since there are more files modified.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4717
> + * webkit_web_view_set_cors_disabling_patterns:

what about webkit_web_view_set_cors_allow_list()?

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4719
> + * @patterns: a NULL-terminated list of patterns.

(array zero-terminated=1) (element-type utf8) (transfer none) (nullable): a %NULL-terminated list of patterns

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4721
> + * Sets @pattern for which CORS checks are disabled in @web_view.

I think we need more documentation here. It's not clear to me what a valid pattern is (check UserContentURLPattern::parse()).

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4729
> +    if (!patterns || !g_strv_length(const_cast<char**>(patterns)))

I don't think we should return early in this case. Setting an empty list is the only way to reset the patterns. That's what updateCORSDisablingPatterns() does. I think we should mark patterns as nullable and document that if it's NULL or empty the patterns will be reset. I think this is consistent with webkit_user_script_new().

[Jan-Michael Brummer]

Created attachment 416511 [details]
Patch

[Jan-Michael Brummer]

Updated version with fixed review findings, again without test as earlyoom kills my test api enabled webkit build...

[Carlos Garcia Campos]

Comment on attachment 416511 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4735
> +    if (!allowList)
> +        allowListVector = Vector<String>();

This is not needed, the vector has already been constructed in previous line.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #7)
> Comment on attachment 416474 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416474&action=review
>
> [...]
> 
> > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4717
> > + * webkit_web_view_set_cors_disabling_patterns:
> 
> what about webkit_web_view_set_cors_allow_list()?

I also like _set_cors_allow_list() better, as it is more consistent having
a name used elsewhere in the public API =) 

> > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4729
> > +    if (!patterns || !g_strv_length(const_cast<char**>(patterns)))
> 
> I don't think we should return early in this case. Setting an empty list is
> the only way to reset the patterns. That's what
> updateCORSDisablingPatterns() does. I think we should mark patterns as
> nullable and document that if it's NULL or empty the patterns will be reset.
> I think this is consistent with webkit_user_script_new().

Agreed as well.

[Jan-Michael Brummer]

Created attachment 416521 [details]
Patch

[Adrian Perez]

(By the way, you can take my previous comment as a second review
agreeing on the API addition, I think!)

[Michael Catanzaro]

Comment on attachment 416521 [details]
Patch

r- for now so we don't forget that we need to implement the test. (The API itself looks good.)

[Michael Catanzaro]

Comment on attachment 416521 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4725
> + * Sets @allow_list for which CORS (Cross-Origin Resource Sharing) checks are disabled in @web_view.
> + * Passing a %NULL as @allow_list implies that no URIs are disabled for CORS checks.
> + * URI patterns must be of the form `[protocol]://[host]/[path]`, where the
> + * *host* and *path* components can contain the wildcard character (`*`) to
> + * represent zero or more other characters.

We should clarify that requests *to* resources that match the allowed patterns bypass CORS, not requests *from* resources that match the allowed patterns. Otherwise, it's not clear which. :)

[Michael Catanzaro]

Comment on attachment 416521 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4727
> + * Since: 2.32

Let's not forget to update this to 2.34 now

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1488
> +static void testWebViewCORSAllowList(WebViewTest* test, gconstpointer)
> +{
> +}

I will think about how to write a test for this.

[Michael Catanzaro]

Comment on attachment 416521 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4717
> + * webkit_web_view_set_cors_allow_list:

I'm also going to change this to allowlist rather than allow_list, since it recently became common to use it as one word.

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:4725
>> + * represent zero or more other characters.
> 
> We should clarify that requests *to* resources that match the allowed patterns bypass CORS, not requests *from* resources that match the allowed patterns. Otherwise, it's not clear which. :)

I've expanded this documentation a bit:

 * Sets the @allowlist for which
 * [Cross-Origin Resource Sharing](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS)
 * checks are disabled in @web_view. Passing a %NULL as @allowlist
 * implies that no URIs are disabled for CORS checks. URI patterns must
 * be of the form `[protocol]://[host]/[path]`, where the *host* and
 * *path* components may contain the wildcard character (`*`) to
 * represent zero or more other characters.
 *
 * Disabling CORS checks permits resources from other origins to load
 * allowlisted resources. It does not permit the allowlisted resources
 * to load resources from other origins.
 *
 * If this function is called multiple times, only the allowlist set by
 * the most recent call will be effective.

>> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:1488
>> +}
> 
> I will think about how to write a test for this.

Going to try to write a test now....

[Michael Catanzaro]

Created attachment 430490 [details]
Patch

[EWS]

Committed r278456 (238476@main): <https://commits.webkit.org/238476@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430490 [details].