Bug 219995 - [GTK][WPE] Expose setCORSDisablingPatterns
Summary: [GTK][WPE] Expose setCORSDisablingPatterns
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 220366
Blocks:
  Show dependency treegraph
 
Reported: 2020-12-17 14:14 PST by Jan-Michael Brummer
Modified: 2021-06-04 07:22 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.32 KB, patch)
2020-12-17 14:21 PST, Jan-Michael Brummer
no flags Details | Formatted Diff | Diff
Patch (4.73 KB, patch)
2020-12-17 15:21 PST, Jan-Michael Brummer
no flags Details | Formatted Diff | Diff
Patch (6.81 KB, patch)
2020-12-18 06:11 PST, Jan-Michael Brummer
no flags Details | Formatted Diff | Diff
Patch (6.77 KB, patch)
2020-12-18 08:41 PST, Jan-Michael Brummer
no flags Details | Formatted Diff | Diff
Patch (10.07 KB, patch)
2021-06-03 11:31 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan-Michael Brummer 2020-12-17 14:14:12 PST
Expose setCORSDisablingPatterns within gtk part so that we can allow webextension:/// uri schemes in Epiphany.
Comment 1 Jan-Michael Brummer 2020-12-17 14:21:27 PST
Created attachment 416469 [details]
Patch
Comment 2 EWS Watchlist 2020-12-17 14:22:11 PST
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
Comment 3 Michael Catanzaro 2020-12-17 14:40:06 PST
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.
Comment 4 Michael Catanzaro 2020-12-17 14:54:39 PST
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.
Comment 5 Jan-Michael Brummer 2020-12-17 15:21:19 PST
Created attachment 416474 [details]
Patch
Comment 6 Jan-Michael Brummer 2020-12-17 15:21:48 PST
Updated version with fixed review findings, EXCEPT test.
Comment 7 Carlos Garcia Campos 2020-12-18 01:46:59 PST
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().
Comment 8 Jan-Michael Brummer 2020-12-18 06:11:39 PST
Created attachment 416511 [details]
Patch
Comment 9 Jan-Michael Brummer 2020-12-18 06:12:28 PST
Updated version with fixed review findings, again without test as earlyoom kills my test api enabled webkit build...
Comment 10 Carlos Garcia Campos 2020-12-18 06:25:43 PST
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.
Comment 11 Adrian Perez 2020-12-18 08:37:58 PST
(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.
Comment 12 Jan-Michael Brummer 2020-12-18 08:41:37 PST
Created attachment 416521 [details]
Patch
Comment 13 Adrian Perez 2020-12-18 08:41:43 PST
(By the way, you can take my previous comment as a second review
agreeing on the API addition, I think!)
Comment 14 Michael Catanzaro 2021-01-18 14:17:44 PST
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.)
Comment 15 Michael Catanzaro 2021-01-18 14:18:44 PST
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. :)
Comment 16 Michael Catanzaro 2021-06-01 05:01:50 PDT
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.
Comment 17 Michael Catanzaro 2021-06-03 07:56:18 PDT
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....
Comment 18 Michael Catanzaro 2021-06-03 11:31:25 PDT
Created attachment 430490 [details]
Patch
Comment 19 EWS 2021-06-04 07:22:49 PDT
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].