[Webkit-unassigned] [Bug 193489] [GTK][WPE] Add web extensions API to whitelist access to a security origin

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=193489

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcatanzaro at igalia.com

--- Comment #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 359366
  --> https://bugs.webkit.org/attachment.cgi?id=359366
Patch

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

What is the use-case for this? An embedded application? Not open source?

I have one major reservation about the new API. We create a map of WebKitSecurityOrigin -- i.e. <protocol, host, port> -- to pair<protocol, host> to determine whether a security origin can access a <protocol, host> pair, with port notably lacking. I would expect that the port should be included as well, so we'd instead map from security origin to security origin. The policy would be more restrictive, but it intuitively makes more sense and the API exposed would be a bit nicer (one fewer parameter, and the parameters could have more meaningful names, e.g. WebKitSecurityOrigin *key and WebKitSecurityOrigin *value). So then the new APIs would allow you to decide whether a given security origin can access other security origins. I know that's not the functionality exposed by InjectedBundle::addOriginAccessWhitelistEntry and InjectedBundle::removeOriginAccessWhitelistEntry, but perhaps we should look into changing that at the InjectedBundle level before exposing it forever in the public API with this weird limitation. We could also use WebKitSecurityOrigin now in the public API, and just ignore the port, perhaps documenting that port will be ignored currently but might not be in the future.

> Source/WebKit/ChangeLog:10
> +        to Shared and make it availale from the web extensions API.

available

> Source/WebKit/ChangeLog:16
> +        * Shared/API/glib/WebKitSecurityOriginInternal.h: Added.
> +        * Shared/API/glib/WebKitSecurityOriginPrivate.h: Copied from Source/WebKit/UIProcess/API/glib/WebKitSecurityOriginPrivate.h.

I think you could just add the new API to WebKitSecurityOriginPrivate.h and include that from the test, like the Cocoa API tests do, instead of creating a new convention to separate APIs used only by API tests out into Internal.h files like this. Note that the non-glib API tests include lots of Private.h headers, but not any Internal.h headers, and there are many Internal.h headers in both the C and Cocoa APIs, so I would try to parallel that convention.

> Source/WebKit/Shared/API/glib/WebKitSecurityOrigin.cppSource/WebKit/UIProcess/API/glib/WebKitSecurityOrigin.cpp:72
> +// Internal API only used for testing.
> +bool webkitSecurityOriginCanRequest(WebKitSecurityOrigin* origin, const char* url)

The comment is pretty obvious, since it uses camelCase rather than underscores and the pattern is well-established that these are private functions.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:202
> +    extension->priv->bundle = bundle;
>      bundle->setClient(std::make_unique<WebExtensionInjectedBundleClient>(extension));

Can you explain the ownership semantics of WebKitWebExtension and InjectedBundle? I'm a bit confused why it is appropriate for the WebExtensionInjectedBundleClient to ref the InjectedBundle? I haven't investigated, but might have assumed the WebKitWebExtension is supposed to die when the InjectedBundle does, or something like that.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:241
> + * @protocol: the destination protocol
> + * @host: (nullable): the destination host, or %NULL for any host

Does the internal InjectedBundle API really accept null for host, but not for protocol?

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:245
> + * If @host is %NULL all hosts are allowed and @allow_subdomains parameter is ignored.

%NULL,

the @allow_subdomains

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:254
> +    g_return_if_fail(origin && !webkit_security_origin_is_opaque(origin));

Better on two separate lines (so you can tell from the critical which check is failing).

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:275
> +    g_return_if_fail(origin && !webkit_security_origin_is_opaque(origin));

Ditto.

> Source/WebKit/WebProcess/InjectedBundle/API/wpe/WebKitWebExtension.h:91
> +                                                           gboolean              allow_subdomains);

I just told Patrick to add a two-member enum instead of using gboolean parameters in the public API... should probably hold you to that, too. It will be better!

> Tools/TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.cpp:32
> -typedef HashMap<String, std::function<std::unique_ptr<WebProcessTest> ()>> TestsMap;
> +typedef HashMap<String, std::function<std::unique_ptr<WebProcessTest> (WebKitWebExtension*)>> TestsMap;

I think it's time for another typedef here, to simplify this typedef and the signature of WebProcessTest::add. Or convert it to using statements, since those are nicer than typedefs. I would try:

using TestFunction = std::function<std::unique_ptr<WebProcessTest> (WebKitWebExtension*)>;
using TestsMap = HashMap<String, TestFunction>;

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190121/6189ae4f/attachment.html>