Bug 219325 – [WPE][GTK] flatpak-spawn subsandbox clears environment variables, breaks EphyPermissionsManager

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

When logging into a certain website in Ephy Tech Preview, I notice that I am prompted to choose whether to save my password every time, despite selecting "Never Save" the password each time. I should not be prompted again when I select "Never Save." But this works perfectly fine outside flatpak. I wound up debugging EphyPermissionsManager and determined that its GKeyfileSettingsBackend is returning bogus results when the EphyPermissionsManager is created in the web process (but it works fine in the UI process). Turns out the GKeyfileSettingsBackend is created for the wrong filename: ~/.local/share/epiphany/permissions.ini (wrong) instead of ~/.var/app/org.gnome.Epiphany.Devel/data/epiphany/permissions.ini. Turns out ephy_default_profile_dir() is wrong. But that is set from g_get_user_data_dir(), which is set from $XDG_DATA_HOME. flatpak sets that to /home/mcatanzaro/.var/app/org.gnome.Epiphany.Devel/data, and that's what it is set to in the UI process. But in the web process, it is unset, so g_get_user_data_dir() falls back to ~/.local/share/epiphany, which is not persistent, and we have a disaster.

So somehow the environment variable is being filtered before it reaches the web process, presumably by flatpak-spawn. I'm very confused, because FlatpakLauncher.cpp does not pass --clear-env to flatpak-spawn, implying that environment variables should not be cleared. We should either fix it somehow, or disable the flatpak-spawn subsandbox.

[Michael Catanzaro]

I think the source code for this is in flatpak/portal/flatpak-portal.c? And the problem isn't that the environment is cleared, but that the environment is inherited from the host rather than from the sandboxed UI process?

[Michael Catanzaro]

Hi Patrick, do you think it would be safe for us to manually forward the entire environment using 'flatpak-spawn --env'? I think that's the most plausible solution here. If we forward only a subset of environment variables, who knows what else might break, right? It would also be super confusing for environment variables to not affect the web process.

[Alexander Larsson]

I don't quite understand this. The spawned process will start with either an empty env (--clear-env), or the host env, and on top of that will be whatever was passed via --env (although, it might be nice to have a --forward-env options that just took the env from the parent of the flatpak-spawn call). 

However, it should still set the $XDG_DATA_HOME env var. That is set irregardless of what env the process starts from. And, in my testing this seems to work:

[alex@greebo ~]$ flatpak run --command=bash org.gnome.gedit
[📦 org.gnome.gedit ~]$ export XDG_DATA_FOO=FOO
[📦 org.gnome.gedit ~]$ export | grep XDG_DATA_
declare -x XDG_DATA_DIRS="/app/share:/usr/share:/usr/share/runtime/share:/run/host/user-share:/run/host/share"
declare -x XDG_DATA_FOO="FOO"
declare -x XDG_DATA_HOME="/home/alex/.var/app/org.gnome.gedit/data"
[📦 org.gnome.gedit ~]$ flatpak-spawn --env=XDG_DATA_BAR=BAR bash -c export | grep XDG_DATA_
declare -x XDG_DATA_BAR="BAR"
declare -x XDG_DATA_DIRS="/app/share:/usr/share:/usr/share/runtime/share:/run/host/user-share:/run/host/share"
declare -x XDG_DATA_HOME="/home/alex/.var/app/org.gnome.gedit/data"
[📦 org.gnome.gedit ~]$ export XDG_DATA_HOME=/foo/bar
[📦 org.gnome.gedit ~]$ flatpak-spawn --env=XDG_DATA_BAR=BAR bash -c export | grep XDG_DATA_
declare -x XDG_DATA_BAR="BAR"
declare -x XDG_DATA_DIRS="/app/share:/usr/share:/usr/share/runtime/share:/run/host/user-share:/run/host/share"
declare -x XDG_DATA_HOME="/home/alex/.var/app/org.gnome.gedit/data"

[Michael Catanzaro]

Hm, interesting. That is indeed the behavior we *want* to get for XDG_DATA_HOME, but it's not the behavior we are actually getting. I have no clue why not.

(In reply to Alexander Larsson from comment #3)
> (although, it might be nice to have a
> --forward-env options that just took the env from the parent of the
> flatpak-spawn call). 

Yeah that would be nice indeed, because otherwise simple things like debug environment variables will not work. E.g. you expect setting GST_DEBUG will result in a gstreamer debug log, but it will get stripped out. So I think we must propagate all environment variables regardless of whether XDG dirs are handled specially.

[Michael Catanzaro]

OK, something weird is going on. Inside the UI process sandbox, we have:

$ env | grep XDG
XDG_CONFIG_DIRS=/app/etc/xdg:/etc/xdg
XDG_MENU_PREFIX=gnome-
XDG_DATA_HOME=/home/mcatanzaro/.var/app/org.gnome.Epiphany.Devel/data
XDG_CONFIG_HOME=/home/mcatanzaro/.var/app/org.gnome.Epiphany.Devel/config
XDG_SESSION_DESKTOP=gnome
XDG_SESSION_TYPE=wayland
XDG_CURRENT_DESKTOP=GNOME
XDG_CACHE_HOME=/home/mcatanzaro/.var/app/org.gnome.Epiphany.Devel/cache
XDG_SESSION_CLASS=user
XDG_RUNTIME_DIR=/run/user/1000
XDG_DATA_DIRS=/app/share:/usr/share:/usr/share/runtime/share:/run/host/user-share:/run/host/share

But in the web process sandbox:

$ env | grep XDG
XDG_CONFIG_DIRS=/app/etc/xdg:/etc/xdg
XDG_MENU_PREFIX=gnome-
XDG_SESSION_DESKTOP=gnome
XDG_SESSION_TYPE=wayland
XDG_CURRENT_DESKTOP=GNOME
XDG_SESSION_CLASS=user
XDG_RUNTIME_DIR=/run/user/1000
XDG_DATA_DIRS=/app/share:/usr/share:/usr/share/runtime/share:/run/host/user-share:/run/host/share

So the web process sandbox is getting some things from flatpak, because we see /app in the web process. Comparing the full differences (not copied here), I see the web process is missing a few things set by gnome-shell when launching apps: GJS_DEBUG_OUTPUT, DESTKOP_STARTUP_ID, GIO_LAUNCHED_DESKTOP_FILE_PID, and GIO_LAUNCHED_DESKTOP_FILE. The web process is also missing DBUS_SYSTEM_BUS_ADDRESS, which means geolocation is probably broken (and indeed, I see a comment in FlatpakLauncher.cpp "Note that this only allows portals and $appid.Sandbox.* access" that indicates this is intentional). Finally, the web process is missing XDG_DATA_HOME, XDG_CONFIG_HOME, and XDG_CACHE_HOME.

Since that is inconsistent from what Alex's test shows, I entered the UI process sandbox and ran:

$ flatpak-spawn bash -c env | grep XDG_DATA
XDG_DATA_HOME=/home/mcatanzaro/.var/app/org.gnome.Epiphany/data
XDG_DATA_DIRS=/app/share:/usr/share:/usr/share/runtime/share:/run/host/share

which confirms Alex's test. That's hard to explain. Why is our web process getting different behavior than when we manually call flatpak-spawn?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> which confirms Alex's test. That's hard to explain. Why is our web process
> getting different behavior than when we manually call flatpak-spawn?

Ah sorry, the difference is the --sandbox flag. Passing --sandbox makes the XDG dirs go away. So that's what breaks EphyPermissionsManager.

It does not matter, because we really do need to propagate the entire environment regardless for the reasons mentioned above. This is very frustrating to test, though, because flatpak-spawn just refuses to do anything when run outside a flatpak sandbox, which makes development pretty difficult. The best I can do is submit an untested patch and add it to gnome-build-meta to see if it works. If we have instructions for building modified flatpak runtimes anywhere, that would be wonderful, but I suspect that's one of those things we just never got around to making easy....

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> This is very
> frustrating to test, though, because flatpak-spawn just refuses to do
> anything when run outside a flatpak sandbox, which makes development pretty
> difficult.

OK, solution is obvious: WebKit has an entire flatpak dev environment set up... all I have to do is figure out how to use the wrapper scripts. I can do this. :P

For now, I'll upload my patch, without r? for now until I've tested it properly.

[Michael Catanzaro]

Created attachment 418591 [details]
Patch

[Michael Catanzaro]

Comment on attachment 418591 [details]
Patch

OK, I can't actually test it with run-minibrowser because it doesn't run a real flatpak environment. It just does this:

$ run-minibrowser --gtk
Portal call failed: Authorization error: Key file does not have group “Application”
Portal call failed: Authorization error: Key file does not have group “Application”

** (MiniBrowser:21): WARNING **: 08:52:21.529: WebProcess CRASHED


This shouldn't happen in a real flatpak though. I think this is safe to land.

[Michael Catanzaro]

Comment on attachment 418591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=418591&action=review

> Source/WebKit/UIProcess/Launcher/glib/FlatpakLauncher.cpp:71
> +    // One hopes. Anything that contains = might break, but best we can do is pass it along.

I realized this is actually quite easy to test inside a flatpak sandbox:

$ flatpak-spawn --sandbox --env=G_DEBUG=fatal-criticals env | grep G_DEBUG
G_DEBUG=fatal-criticals

So it works as expected. We're sure to have problems if any environment variable contains anything that's not suitable for the shell, though. So let's add a call to g_shell_quote().

[Michael Catanzaro]

Created attachment 418876 [details]
Patch

[Adrian Perez]

Comment on attachment 418876 [details]
Patch

Patch idea LGTM, but please fix the issue below before landing :]


View in context: https://bugs.webkit.org/attachment.cgi?id=418876&action=review

> Source/WebKit/UIProcess/Launcher/glib/FlatpakLauncher.cpp:72
> +        GUniquePtr<char> quotedVariable(g_shell_quote(*variable));

Quoting the variable here is wrong because g_subprocess_launcher_spawnv()
below will result in the executable being invoked directly *without* the
shell intervening — either through a call to posix_spawn(), or a fork()+execv*()
combo inside GLib.

[Michael Catanzaro]

You are right. Good catch.

[Michael Catanzaro]

Created attachment 418919 [details]
Patch for landing

[EWS]

Committed r272179: <https://trac.webkit.org/changeset/272179>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418919 [details].

[Michael Catanzaro]

Comment on attachment 418919 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=418919&action=review

> Source/WebKit/ChangeLog:8
> +        Manually forward all environment variables from the web process to the UI process.

Oops, it landed with this typo. Of course it's supposed to say: "Manually forward all environment variables from the UI process to the web process."