Bug 193489

Created attachment 359265 [details] Patch

Created attachment 359266 [details] Patch

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.

Created attachment 359366 [details] Patch Added support for any host and documented it.

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

(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>;

Created attachment 360098 [details] Patch

Created attachment 360112 [details] Patch

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.

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

A WebKit2 owner is required here too, please.

Created attachment 360325 [details] Patch for landing

Please owners, we are in a hurry to land new API patches because of the release schedule.

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.

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

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

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.

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

Yeah sure. (Still corresponds to a WebProcessPool.)

Created attachment 361675 [details] Try to fix mac builds

Oops, wrong bug

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

It should certainly not be in the WebProcess. Why would it be needed in the NetworkProcess?

(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

Hi Alex, can we look at this again? Looks like you quite firmly do not want this API in the web process. Carlos suggested putting the public API in WebKitSecurityManager instead. That would mean all the *internal* InjectedBundle API changes in this patch remain, but the public GTK APIs would be removed. (The public APIs are what you are objecting to, right, Alex?) Then we would add internal IPC to allow WebKitSecurityManager in the UI process to use the internal InjectedBundle APIs. Does that sound OK? We could also drop the internal InjectedBundle API changes if we don't check ports, following my earlier suggestion in comment #6: "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." I think it's better if we check ports, though, so I would only do this if Alex really doesn't like checking the port. Finally, the public APIs would need to change "whitelist" to "allowlist," of course. Motivation: Jan-Michael needs this to implement WebExtensions. He can't get lasercat working because the extension tries to make an XMLHTTPRequest from the website's origin (say, http://example.com) to a webextension:// URI, and that gets blocked by CORS. I assume that needs to be exempted from CORS. This would also allow us to remove our hacks to PDF.js: currently we patch PDF.js to feed it PDF content using g_strdup_printf() in order to avoid it trying to load the PDF via HTTP and getting blocked by CORS.

Note this proposal does imply the origin map has to be duplicated in every process. It's already in web process and network process, so now it would be in the UI process as well: (In reply to Carlos Garcia Campos from comment #23) > 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?

We successfully implemented web extensions without this. I think we used WKWebView._setCORSDisablingPatterns for this particular case. That's not a perfect interface, but it's in the UI process.

(In reply to Alex Christensen from comment #28) > We successfully implemented web extensions without this. I think we used > WKWebView._setCORSDisablingPatterns for this particular case. That's not a > perfect interface, but it's in the UI process. Looks like Jan-Michael really liked this idea, because he implemented it in bug #219995. I notice we would be the first port to expose this as public API, so please be sure you really like it.

I understand that this won't be needed anymore now that the patch from bug #219995 has landed. Let's close this; if needed we can reopen it in the future :)