Bug 213174

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