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.
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.
The sandbox is not enabled by default in WebKit. I'm fine with adding a command line option to enable it in MiniBrowser.
(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.
(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.)
(In reply to Michael Catanzaro from comment #3) > GNOME 3.38 Also missing: gnome-initial-setup privacy page. I'll handle that.
Created attachment 402204 [details] Patch
Comment on attachment 402204 [details] 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
Comment on attachment 402204 [details] 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.
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?
Patrick probably knows.
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.
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...?
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).
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.
DBus is slow. I can try to run all WebDriver tests using DBus registration and see if there's a difference.
Ah that's true... if needed, we could add a command line flag to disable it for web driver tests.
(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 ;-)
I still would like to know what's exactly broken and why without the app id.
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.
(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?
(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.
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....
(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?
(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?)
(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.
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.)