213174 2020-06-13 15:58:34 -0700 [GTK] MiniBrowser: can skip sandboxing without verbose warning 2023-02-12 05:41:14 -0800 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified NEW P2 Normal --- 1 fedora webkit-unassigned aperez bugs-noreply cgarcia mcatanzaro pgriffis oldest_to_newest 1662440 0 fedora 2020-06-13 15:58:34 -0700 One of the surprise points about problem described in [bug 213148] was a finding that the problem observed in WebKitGTK-under-Evolution arrangement with GDK_BACKEND=wayland was _not_ observed with WEBKIT_FORCE_SANDBOX=1 MiniBrowser -- fonts where always fine. Only the explicit sandbox via bwrap wrapping triggered the same type of issue, which indicates that the sandbox won't be established even if explicitly asked (only perhaps unless all prerequisites are satisifed). That means that sandboxing is not (at least under some situations) enabled! While I see that MiniBox is more for testing and minimal reproducers, some could have other expectations, and these could believe they are protected with sandboxing while they are not. I think this message logged in terminal indicates that no sandboxing is in place: > GApplication is required for xdg-desktop-portal access in the WebKit > sandbox. Actions that require xdg-desktop-portal will be broken. IMHO it should be double-checked, perhaps in sway WM without gnome-settings-daemon or respective xsettings schema installed. Also, at [bug 213148 comment 12], I proposed a visible distinguishing of when sandboxing is _provably_ enabled vs. when not (in titlebar) to make this status always crystal clear. 1662483 1 mcatanzaro 2020-06-14 06:41:45 -0700 I agree MiniBrowser should enable the sandbox if possible to do so without breaking tests, because otherwise it's a very different environment with very different bugs than the WebKit our users actually use. 1662560 2 cgarcia 2020-06-15 00:24:03 -0700 The sandbox is not enabled by default in WebKit. I'm fine with adding a command line option to enable it in MiniBrowser. 1662621 3 mcatanzaro 2020-06-15 07:08:54 -0700 (In reply to Carlos Garcia Campos from comment #2) > The sandbox is not enabled by default in WebKit. Thing is, that's no longer a good default. For GNOME 3.38 I think we have almost all applications using sandboxed WebKit. At least Epiphany, Evolution, Maps, devhelp, and captive portal helper. Hm, we might be missing gnome-online-accounts... I'll talk to rishi about that. Point is that MiniBrowser by default is no longer testing what users are actually getting, making it less useful for verifying whether a bug is an Ephy issue or a WebKit issue, for example. A CLI flag would be good, of course, but I would vote to have it disable the sandbox rather than enable it. 1662622 4 mcatanzaro 2020-06-15 07:10:29 -0700 (In reply to Michael Catanzaro from comment #3) > (In reply to Carlos Garcia Campos from comment #2) > > The sandbox is not enabled by default in WebKit. > > Thing is, that's no longer a good default. Sorry, I mean no longer a good default for MiniBrowser. Of course we can't change how our GTK 3 API works, so it needs to remain disabled by default there. (In GTK 4, the sandbox should be force-enabled and the webkit_web_context_set_sandbox_enabled() API should be removed.) 1662623 5 mcatanzaro 2020-06-15 07:13:14 -0700 (In reply to Michael Catanzaro from comment #3) > GNOME 3.38 Also missing: gnome-initial-setup privacy page. I'll handle that. 1663864 6 402204 cgarcia 2020-06-18 06:50:36 -0700 Created attachment 402204 Patch 1663867 7 402204 mcatanzaro 2020-06-18 07:19:14 -0700 Comment on attachment 402204 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402204&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:684 > + const char* appID = g_application_get_application_id(app); > + g_key_file_set_string(keyFile.get(), "Application", "name", appID ? appID : g_get_prgname()); This won't work (and can't be made to work)... look three lines up. :P 1663868 8 402204 mcatanzaro 2020-06-18 07:22:34 -0700 Comment on attachment 402204 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402204&action=review >> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:684 >> + g_key_file_set_string(keyFile.get(), "Application", "name", appID ? appID : g_get_prgname()); > > This won't work (and can't be made to work)... look three lines up. :P Sorry... wait... I'm confused. Here you have a GApplication, but it doesn't have an app ID? I didn't realize this was possible. We should probably add this case to the error path above, because the purpose of requiring GApplication is to ensure there is an app ID. 1663870 9 cgarcia 2020-06-18 07:32:31 -0700 Yes, MiniBrowser now uses GApplication, but without an ID to avoid the DBus registration. What is supposed to be broken or not supported? How can I test it? 1663875 10 mcatanzaro 2020-06-18 08:06:01 -0700 Patrick probably knows. 1664328 11 cgarcia 2020-06-19 06:22:12 -0700 I'm not going to enable the sandbox in MiniBrowser if it shows a warning always at startup or if I have to set a an application ID. 1664344 12 mcatanzaro 2020-06-19 07:15:53 -0700 Well we need to ask Patrick what all the app ID is used for, but I assume it's important. Why are you trying to avoid D-Bus registration? Is G_APPLICATION_NON_UNIQUE not good enough...? 1664351 13 fedora 2020-06-19 07:40:55 -0700 Sorry for stating something obvious to you/repeating myself or an utter non-sense, but I suspect that in my previous tests, MiniBrowser could not even be persuaded to apply the sandboxing with WEBKIT_FORCE_SANDBOX=1, and it might have been related exactly to "app ID" you are talking about, because `bwrap` could not get a valid `.flatpak-info` file (per stracing), and perhaps there's a link between the two. Anyway, explicit `bwrap` with `.flatpak-info` file injection worked for me: [bug 213148 comment 6] (and it wasn't a success just because it was made weaker for simplicity, I believe). 1664377 14 mcatanzaro 2020-06-19 08:52:20 -0700 Possibly the problem was missing app ID, indeed. Even if the sandbox seems to work without app ID, the portals are not designed to expect this, so basic actions are likely to be broken (saving, printing, notifications, etc.). I don't think we should try to make it work without app ID. (In reply to Michael Catanzaro from comment #12) > Is G_APPLICATION_NON_UNIQUE not good enough...? If G_APPLICATION_NON_UNIQUE is not sufficient for you, I would WONTFIX this issue, I suppose. I don't see why avoiding D-Bus registration is important, but I do understand we want MiniBrowser instances to be unique. 1664829 15 cgarcia 2020-06-21 02:04:59 -0700 DBus is slow. I can try to run all WebDriver tests using DBus registration and see if there's a difference. 1664845 16 mcatanzaro 2020-06-21 07:36:25 -0700 Ah that's true... if needed, we could add a command line flag to disable it for web driver tests. 1664863 17 aperez 2020-06-21 10:41:47 -0700 (In reply to Michael Catanzaro from comment #16) > Ah that's true... if needed, we could add a command line flag to disable it > for web driver tests. Also: disable setting an application id when the “--automation” command line flag is present, which is already being used to run in WebDriver mode ;-) 1664945 18 cgarcia 2020-06-22 00:08:05 -0700 I still would like to know what's exactly broken and why without the app id. 1664951 19 pgriffis 2020-06-22 00:47:56 -0700 So the actual portals used by the WebProcess seems very limited, maybe only `org.freedesktop.portal.Settings` at a glance. Passing any random ID would allow it to work while not being a security problem. Just the results of `g_get_prgname()` isn't really valid though. 1665001 20 mcatanzaro 2020-06-22 06:41:01 -0700 (In reply to Patrick Griffis from comment #19) > So the actual portals used by the WebProcess seems very limited, maybe only > `org.freedesktop.portal.Settings` at a glance. So without the settings portal, you lose access not just to your own settings (MiniBrowser doesn't have any gsettings), but also standard desktop settings. Antialiasing is probably broken? Off the top of my head: * No secrets portal, so credential storage won't work * Future: printer portal (I don't know why printing uses the web process, but bwrap sandbox breaks it, so it must) * Future: data transfer portal, for drag-and-drop? 1665044 21 cgarcia 2020-06-22 08:27:33 -0700 (In reply to Michael Catanzaro from comment #20) > (In reply to Patrick Griffis from comment #19) > > So the actual portals used by the WebProcess seems very limited, maybe only > > `org.freedesktop.portal.Settings` at a glance. > > So without the settings portal, you lose access not just to your own > settings (MiniBrowser doesn't have any gsettings), but also standard desktop > settings. Antialiasing is probably broken? It wasn't broken for sure when I tried. > Off the top of my head: > > * No secrets portal, so credential storage won't work Credential storage doesn't happen in the web process, but in the network process. > * Future: printer portal (I don't know why printing uses the web process, > but bwrap sandbox breaks it, so it must) Printing happens in the UI process. > * Future: data transfer portal, for drag-and-drop? Drag and drop and clipboard both happen in the UI process. 1665063 22 mcatanzaro 2020-06-22 09:21:30 -0700 I guess we can hardcode com.webkit.WebKit as the "app ID" for applications that use GtkApplication but do not set an app ID? I assume MiniBrowser is the only application that would ever consider doing that. > Printing happens in the UI process. That's what I thought too, but if it was true, bug #202363 would not exist.... 1665096 23 pgriffis 2020-06-22 11:16:49 -0700 (In reply to Michael Catanzaro from comment #22) > I guess we can hardcode com.webkit.WebKit as the "app ID" for applications > that use GtkApplication but do not set an app ID? I assume MiniBrowser is > the only application that would ever consider doing that. I expect lots of non-gapplication using applications use webkitgtk and would just get this by default. This isn't a major problem at the moment but it does mean all applications relying on this get the same permissions. Could throw a hash of `g_get_prgname()` at the end or something to just ensure its unique? 1665174 24 mcatanzaro 2020-06-22 14:03:30 -0700 (In reply to Patrick Griffis from comment #23) > I expect lots of non-gapplication using applications use webkitgtk and would > just get this by default. No, applications that don't use GApplication would get the warning that prints if you try to enable the sandbox without using GApplication. MiniBrowser is using GApplication but without setting an app ID, that's pretty special. I'm suggesting that maybe it's OK to run the sandbox with broken portals if you're doing that, on the assumption that the application is probably MiniBrowser. (I assume we don't want real-world applications running the sandbox without an app ID?) 1665306 25 cgarcia 2020-06-22 23:15:49 -0700 (In reply to Michael Catanzaro from comment #22) > I guess we can hardcode com.webkit.WebKit as the "app ID" for applications > that use GtkApplication but do not set an app ID? I assume MiniBrowser is > the only application that would ever consider doing that. > > > Printing happens in the UI process. > > That's what I thought too, but if it was true, bug #202363 would not > exist.... You are right, the dialog is shown in the UI process, but the actual printing happens in the web process. 1933004 26 mcatanzaro 2023-02-12 05:41:14 -0800 The problem here is unit tests. We don't want to force use of GApplication in unit tests. See e.g. https://gitlab.gnome.org/GNOME/geary/-/merge_requests/761#note_1666608 (I thought we were discussing a similar problem in a different bug report too, but I can't find it now.) 402204 2020-06-18 06:50:36 -0700 2020-06-18 07:19:14 -0700 Patch mb-sandbox.diff text/plain 3818 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggMGQ5NWY0ZjI3YWY5Li5hOGQ5YzUwYzM5ODUgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMg KzEsMTMgQEAKKzIwMjAtMDYtMTggIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2Fs aWEuY29tPgorCisgICAgICAgIFtHVEtdIE1pbmlCcm93c2VyOiBjYW4gc2tpcCBzYW5kYm94aW5n IHdpdGhvdXQgdmVyYm9zZSB3YXJuaW5nCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0yMTMxNzQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIFVJUHJvY2Vzcy9MYXVuY2hlci9nbGliL0J1YmJsZXdyYXBMYXVu Y2hlci5jcHA6CisgICAgICAgIChXZWJLaXQ6OmNyZWF0ZUZsYXRwYWtJbmZvKTogSGFuZGxlIG51 bGxwdHIgYXBwbGljYXRpb24gSUQsIHVzaW5nIGdfZ2V0X3ByZ25hbWUoKSBhcyBmYWxsYmFjay4K KwogMjAyMC0wNi0xNyAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+ CiAKICAgICAgICAgQWRkIHN1cHBvcnQgZm9yIGZldGNoaW5nIHJlZ2lzdHJhYmxlIGRvbWFpbnMg d2l0aCByZXNvdXJjZSBsb2FkIHN0YXRpc3RpY3MKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQv VUlQcm9jZXNzL0xhdW5jaGVyL2dsaWIvQnViYmxld3JhcExhdW5jaGVyLmNwcCBiL1NvdXJjZS9X ZWJLaXQvVUlQcm9jZXNzL0xhdW5jaGVyL2dsaWIvQnViYmxld3JhcExhdW5jaGVyLmNwcAppbmRl eCAwYTM0NTgxZTJlNTEuLjBmY2JhYzBiNmY1YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9V SVByb2Nlc3MvTGF1bmNoZXIvZ2xpYi9CdWJibGV3cmFwTGF1bmNoZXIuY3BwCisrKyBiL1NvdXJj ZS9XZWJLaXQvVUlQcm9jZXNzL0xhdW5jaGVyL2dsaWIvQnViYmxld3JhcExhdW5jaGVyLmNwcApA QCAtNjgwLDcgKzY4MCw4IEBAIHN0YXRpYyBpbnQgY3JlYXRlRmxhdHBha0luZm8oKQogICAgICAg ICBnX3dhcm5pbmcoIkdBcHBsaWNhdGlvbiBpcyByZXF1aXJlZCBmb3IgeGRnLWRlc2t0b3AtcG9y dGFsIGFjY2VzcyBpbiB0aGUgV2ViS2l0IHNhbmRib3guIEFjdGlvbnMgdGhhdCByZXF1aXJlIHhk Zy1kZXNrdG9wLXBvcnRhbCB3aWxsIGJlIGJyb2tlbi4iKTsKICAgICAgICAgcmV0dXJuIC0xOwog ICAgIH0KLSAgICBnX2tleV9maWxlX3NldF9zdHJpbmcoa2V5RmlsZS5nZXQoKSwgIkFwcGxpY2F0 aW9uIiwgIm5hbWUiLCBnX2FwcGxpY2F0aW9uX2dldF9hcHBsaWNhdGlvbl9pZChhcHApKTsKKyAg ICBjb25zdCBjaGFyKiBhcHBJRCA9IGdfYXBwbGljYXRpb25fZ2V0X2FwcGxpY2F0aW9uX2lkKGFw cCk7CisgICAgZ19rZXlfZmlsZV9zZXRfc3RyaW5nKGtleUZpbGUuZ2V0KCksICJBcHBsaWNhdGlv biIsICJuYW1lIiwgYXBwSUQgPyBhcHBJRCA6IGdfZ2V0X3ByZ25hbWUoKSk7CiAKICAgICBzaXpl X3Qgc2l6ZTsKICAgICBHVW5pcXVlT3V0UHRyPEdFcnJvcj4gZXJyb3I7CmRpZmYgLS1naXQgYS9U b29scy9DaGFuZ2VMb2cgYi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggYjQyNzUzM2NhY2FiLi41NjVj OWZjZWZiMDEgMTAwNjQ0Ci0tLSBhL1Rvb2xzL0NoYW5nZUxvZworKysgYi9Ub29scy9DaGFuZ2VM b2cKQEAgLTEsMyArMSwxNSBAQAorMjAyMC0wNi0xOCAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxj Z2FyY2lhQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dUS10gTWluaUJyb3dzZXI6IGNhbiBza2lw IHNhbmRib3hpbmcgd2l0aG91dCB2ZXJib3NlIHdhcm5pbmcKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIxMzE3NAorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFkZCAtLWRpc2FibGUtc2FuZGJveCBhbmQgZW5h YmxlIHRoZSB3ZWIgcHJvY2VzcyBzYW5kYm94IHVubGVzcyB0aGUgb3B0aW9uIGlzIHBhc3NlZC4K KworICAgICAgICAqIE1pbmlCcm93c2VyL2d0ay9tYWluLmM6CisgICAgICAgIChhY3RpdmF0ZSk6 CisKIDIwMjAtMDYtMTcgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgogCiAgICAgICAgIEFkZCBzdXBwb3J0IGZvciBmZXRjaGluZyByZWdpc3RyYWJsZSBkb21haW5z IHdpdGggcmVzb3VyY2UgbG9hZCBzdGF0aXN0aWNzCmRpZmYgLS1naXQgYS9Ub29scy9NaW5pQnJv d3Nlci9ndGsvbWFpbi5jIGIvVG9vbHMvTWluaUJyb3dzZXIvZ3RrL21haW4uYwppbmRleCBlYTVi MzBhMjM5YjAuLjVhZmE4OTgzNTRmNCAxMDA2NDQKLS0tIGEvVG9vbHMvTWluaUJyb3dzZXIvZ3Rr L21haW4uYworKysgYi9Ub29scy9NaW5pQnJvd3Nlci9ndGsvbWFpbi5jCkBAIC01NCw2ICs1NCw3 IEBAIHN0YXRpYyBjb25zdCBjaGFyICpjb29raWVzUG9saWN5Owogc3RhdGljIGNvbnN0IGNoYXIg KnByb3h5Owogc3RhdGljIGdib29sZWFuIGRhcmtNb2RlOwogc3RhdGljIGdib29sZWFuIGVuYWJs ZUlUUDsKK3N0YXRpYyBnYm9vbGVhbiBkaXNhYmxlU2FuZGJveDsKIHN0YXRpYyBnYm9vbGVhbiBw cmludFZlcnNpb247CiAKIHR5cGVkZWYgZW51bSB7CkBAIC0xMjIsNiArMTIzLDcgQEAgc3RhdGlj IGNvbnN0IEdPcHRpb25FbnRyeSBjb21tYW5kTGluZU9wdGlvbnNbXSA9CiAgICAgeyAiaWdub3Jl LXRscy1lcnJvcnMiLCAwLCAwLCBHX09QVElPTl9BUkdfTk9ORSwgJmlnbm9yZVRMU0Vycm9ycywg Iklnbm9yZSBUTFMgZXJyb3JzIiwgTlVMTCB9LAogICAgIHsgImNvbnRlbnQtZmlsdGVyIiwgMCwg MCwgR19PUFRJT05fQVJHX0ZJTEVOQU1FLCAmY29udGVudEZpbHRlciwgIkpTT04gd2l0aCBjb250 ZW50IGZpbHRlcmluZyBydWxlcyIsICJGSUxFIiB9LAogICAgIHsgImVuYWJsZS1pdHAiLCAwLCAw LCBHX09QVElPTl9BUkdfTk9ORSwgJmVuYWJsZUlUUCwgIkVuYWJsZSBJbnRlbGxpZ2VudCBUcmFj a2luZyBQcmV2ZW50aW9uIChJVFApIiwgTlVMTCB9LAorICAgIHsgImRpc2FibGUtc2FuZGJveCIs IDAsIDAsIEdfT1BUSU9OX0FSR19OT05FLCAmZGlzYWJsZVNhbmRib3gsICJEaXNhYmxlIHRoZSB3 ZWIgcHJvY2VzcyBzYW5kYm94IiwgTlVMTCB9LAogICAgIHsgInZlcnNpb24iLCAndicsIDAsIEdf T1BUSU9OX0FSR19OT05FLCAmcHJpbnRWZXJzaW9uLCAiUHJpbnQgdGhlIFdlYktpdEdUSyB2ZXJz aW9uIiwgTlVMTCB9LAogICAgIHsgR19PUFRJT05fUkVNQUlOSU5HLCAwLCAwLCBHX09QVElPTl9B UkdfRklMRU5BTUVfQVJSQVksICZ1cmlBcmd1bWVudHMsIDAsICJbVVJM4oCmXSIgfSwKICAgICB7 IDAsIDAsIDAsIDAsIDAsIDAsIDAgfQpAQCAtNTQ0LDYgKzU0Niw4IEBAIHN0YXRpYyB2b2lkIGFj dGl2YXRlKEdBcHBsaWNhdGlvbiAqYXBwbGljYXRpb24sIFdlYktpdFNldHRpbmdzICp3ZWJraXRT ZXR0aW5ncykKICAgICAgICAgTlVMTCk7CiAgICAgZ19vYmplY3RfdW5yZWYobWFuYWdlcik7CiAK KyAgICB3ZWJraXRfd2ViX2NvbnRleHRfc2V0X3NhbmRib3hfZW5hYmxlZCh3ZWJDb250ZXh0LCAh ZGlzYWJsZVNhbmRib3gpOworCiAgICAgaWYgKGNvb2tpZXNQb2xpY3kpIHsKICAgICAgICAgV2Vi S2l0Q29va2llTWFuYWdlciAqY29va2llTWFuYWdlciA9IHdlYmtpdF93ZWJfY29udGV4dF9nZXRf Y29va2llX21hbmFnZXIod2ViQ29udGV4dCk7CiAgICAgICAgIEdFbnVtQ2xhc3MgKmVudW1DbGFz cyA9IGdfdHlwZV9jbGFzc19yZWYoV0VCS0lUX1RZUEVfQ09PS0lFX0FDQ0VQVF9QT0xJQ1kpOwo=