Bug 193489 - [GTK][WPE] Add web extensions API to whitelist access to a security origin
Summary: [GTK][WPE] Add web extensions API to whitelist access to a security origin
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2019-01-16 06:17 PST by Carlos Garcia Campos
Modified: 2019-02-14 00:44 PST (History)
6 users (show)

See Also:


Attachments
Patch (37.56 KB, patch)
2019-01-16 06:26 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (38.33 KB, patch)
2019-01-16 06:36 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (39.30 KB, patch)
2019-01-17 03:50 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (63.26 KB, patch)
2019-01-25 03:24 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (67.88 KB, patch)
2019-01-25 07:12 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch for landing (67.87 KB, patch)
2019-01-28 01:06 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix mac builds (71.30 KB, patch)
2019-02-11 05:41 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2019-01-16 06:17:54 PST
Expose InjectedBundle::addOriginAccessWhitelistEntry(), InjectedBundle::removeOriginAccessWhitelistEntry() and InjectedBundle::resetOriginAccessWhitelists() in GLib API.
Comment 1 Carlos Garcia Campos 2019-01-16 06:26:26 PST
Created attachment 359265 [details]
Patch
Comment 2 Carlos Garcia Campos 2019-01-16 06:36:37 PST
Created attachment 359266 [details]
Patch
Comment 3 Adrian Perez 2019-01-16 13:13:29 PST
Comment on attachment 359266 [details]
Patch

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

Informally reviewing. Patch LGTM, with a small doubt about how the
function that adds a rule works :)

> Source/WebKit/Shared/API/glib/WebKitSecurityOriginInternal.h:2
> + * Copyright (C) 2017 Igalia S.L.

I am quite sure we are in 2019 at the moment ;-)

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:246
> + * be accessed from @origin.

Is it possible specify “any destination host” somehow? I don't know
how the WebKit code works internally, but from the description it looks
like using “.” as destination host with “allow_subdomains” set to “TRUE”
might work to achieve that. Maybe it would be worth it to explicitly
mention whether that's allowed or not in the documentation.
Comment 4 Carlos Garcia Campos 2019-01-17 03:50:11 PST
Created attachment 359366 [details]
Patch

Added support for any host and documented it.
Comment 5 Adrian Perez 2019-01-17 03:59:30 PST
Comment on attachment 359366 [details]
Patch

Thanks for the update to include the “match all hosts case”, I think using NULL to
mean “do not filter by host, allow all hosts” is coherent with other existing usages
in the public API (like the whitelists/blacklists for injected user content).

Informal r+ :)
Comment 6 Michael Catanzaro 2019-01-20 20:55:29 PST
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?

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>;
Comment 7 Carlos Garcia Campos 2019-01-25 03:16:18 PST
(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>;
Comment 8 Carlos Garcia Campos 2019-01-25 03:24:30 PST
Created attachment 360098 [details]
Patch
Comment 9 Carlos Garcia Campos 2019-01-25 07:12:06 PST
Created attachment 360112 [details]
Patch
Comment 10 Michael Catanzaro 2019-01-25 07:47:36 PST
Comment on attachment 360112 [details]
Patch

OK, thanks for updating this, it looks better. r=me if you get owner approval for the WK2 changes. If owners don't want to add port for some reason, then the original version of this patch is fine too I suppose.
Comment 11 Carlos Garcia Campos 2019-01-25 08:13:05 PST
(In reply to Michael Catanzaro from comment #10)
> Comment on attachment 360112 [details]
> Patch
> 
> OK, thanks for updating this, it looks better. r=me if you get owner
> approval for the WK2 changes. If owners don't want to add port for some
> reason, then the original version of this patch is fine too I suppose.

There's no change in behavior nor API in WebKit2 in any case.
Comment 12 Carlos Garcia Campos 2019-01-28 01:02:25 PST
A WebKit2 owner is required here too, please.
Comment 13 Carlos Garcia Campos 2019-01-28 01:06:50 PST
Created attachment 360325 [details]
Patch for landing
Comment 14 Carlos Garcia Campos 2019-01-31 00:35:30 PST
Please owners, we are in a hurry to land new API patches because of the release schedule.
Comment 15 Alex Christensen 2019-02-04 10:34:01 PST
Comment on attachment 360325 [details]
Patch for landing

The injected bundle is the wrong place for this API.  If you really want this API, please put it in a downstream branch, but I would advice against it.  We do not want this in upstream WebKit.  I give my explicit disapproval of it.

The optional port is suspicious and needs more explanation.
Comment 16 Michael Catanzaro 2019-02-04 12:55:36 PST
(In reply to Alex Christensen from comment #15)
> The injected bundle is the wrong place for this API.  If you really want
> this API, please put it in a downstream branch, but I would advice against
> it.  We do not want this in upstream WebKit.  I give my explicit disapproval
> of it.

OK, but the problem is where it's located rather than the functionality, right? We could e.g. create a UI process API for it?

> The optional port is suspicious and needs more explanation.

Hm, there's no strong reason for this. I just thought it was suspicious that the port was missing from Carlos's original patch, and figured it'd make most sense to work with security origins <protocol, host, port> rather than just protocol and host.
Comment 17 Alex Christensen 2019-02-04 13:27:03 PST
(In reply to Michael Catanzaro from comment #16)
> (In reply to Alex Christensen from comment #15)
> > The injected bundle is the wrong place for this API.  If you really want
> > this API, please put it in a downstream branch, but I would advice against
> > it.  We do not want this in upstream WebKit.  I give my explicit disapproval
> > of it.
> 
> OK, but the problem is where it's located rather than the functionality,
> right? We could e.g. create a UI process API for it?

That would be less problematic, but only if designed well.  It should, for example, not be anything similar to the SchemeRegistry, which tries to be multiprocess-global.
Comment 18 Michael Catanzaro 2019-02-04 17:12:04 PST
WebKitWebContext would be the most natural point, from the perspective of API users. That would correspond to a WebProcessPool. If the internal implementation were to be per-WebPage/WebPageProxy instead of per-WebProcessPool, our API could just call the internal function for every page controlled by the WebProcessPool.
Comment 19 Carlos Garcia Campos 2019-02-05 23:45:54 PST
(In reply to Michael Catanzaro from comment #18)
> WebKitWebContext would be the most natural point, from the perspective of
> API users. That would correspond to a WebProcessPool. If the internal
> implementation were to be per-WebPage/WebPageProxy instead of
> per-WebProcessPool, our API could just call the internal function for every
> page controlled by the WebProcessPool.

I think WebKitSecurityManager would be the most natural point, no?
Comment 20 Michael Catanzaro 2019-02-06 09:27:54 PST
Yeah sure. (Still corresponds to a WebProcessPool.)
Comment 21 Carlos Garcia Campos 2019-02-11 05:41:27 PST
Created attachment 361675 [details]
Try to fix mac builds
Comment 22 Carlos Garcia Campos 2019-02-11 05:51:46 PST
Oops, wrong bug
Comment 23 Carlos Garcia Campos 2019-02-13 03:42:07 PST
(In reply to Alex Christensen from comment #17)
> (In reply to Michael Catanzaro from comment #16)
> > (In reply to Alex Christensen from comment #15)
> > > The injected bundle is the wrong place for this API.  If you really want
> > > this API, please put it in a downstream branch, but I would advice against
> > > it.  We do not want this in upstream WebKit.  I give my explicit disapproval
> > > of it.
> > 
> > OK, but the problem is where it's located rather than the functionality,
> > right? We could e.g. create a UI process API for it?
> 
> That would be less problematic, but only if designed well.  It should, for
> example, not be anything similar to the SchemeRegistry, which tries to be
> multiprocess-global.

Moving the API to the UI process would mean we will keep the whitelist origin map in UI, web and network processes. Do you really want to move the API to the UI process?
Comment 24 Alex Christensen 2019-02-13 13:06:03 PST
It should certainly not be in the WebProcess.  Why would it be needed in the NetworkProcess?
Comment 25 Carlos Garcia Campos 2019-02-14 00:44:04 PST
(In reply to Alex Christensen from comment #24)
> It should certainly not be in the WebProcess.  Why would it be needed in the
> NetworkProcess?

See bug #184374