Bug 213174 – [GTK] MiniBrowser: can skip sandboxing without verbose warning

• Component: WebKitGTK
• Status: NEW

[Jan Pokorný [poki]]

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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

The sandbox is not enabled by default in WebKit. I'm fine with adding a command line option to enable it in MiniBrowser.

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #3)
> GNOME 3.38

Also missing: gnome-initial-setup privacy page. I'll handle that.

[Carlos Garcia Campos]

Created attachment 402204 [details]
Patch

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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?

[Michael Catanzaro]

Patrick probably knows.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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

[Jan Pokorný [poki]]

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

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

DBus is slow. I can try to run all WebDriver tests using DBus registration and see if there's a difference.

[Michael Catanzaro]

Ah that's true... if needed, we could add a command line flag to disable it for web driver tests.

[Adrian Perez]

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

[Carlos Garcia Campos]

I still would like to know what's exactly broken and why without the app id.

[Patrick Griffis]

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.

[Michael Catanzaro]

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

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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

[Patrick Griffis]

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

[Michael Catanzaro]

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

[Carlos Garcia Campos]

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

[Michael Catanzaro]

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