[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

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 359366 [details]
> 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?

This is used by WPE applications still using the C API, this is needed to migrate them to the glib API.

> 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.

Ok, but we can't use SecurityOrigin anyway for the destination, as we still want the special values for all hosts (and now all ports).

> > 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.

We can't, because WebKitSecurityOriginPrivate.h uses WEbCore types that make the tests fail to link.

> > 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.

Not so obvious considering your previous comment. Private.h means private API *required* by the public API implementation and Internal.h means public API we don't want to expose, but it's used *only* by tests. Maybe Internal is not the best name for this, though, but I don't think it matters.

> > 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.

The web extension is owned by the extension manager which is a singleton, so it will never be destroyed. The InjectedBundle is owned by the WebProcess, which is a singleton, so it will never be destroyed.

> > 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?

Not exactly, OriginAccessEntry API handles an empty host as any host. That's the only special case. We don't want to use empty strings for this in the public API, so we use NULL instead. String::fromUTF8(nullptr).isEmpty() will be true.

> > 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/20190125/99a74647/attachment.html>