Summary: | [WPE][GTK] flatpak-spawn subsandbox clears environment variables, breaks EphyPermissionsManager | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
Component: | WebKitGTK | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | alexl, aperez, bugs-noreply, mcatanzaro, pgriffis | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 218121 | ||||||||||
Attachments: |
|
Description
Michael Catanzaro
2020-11-28 09:12:32 PST
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? 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. 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" 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. 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? (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.... (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. Created attachment 418591 [details]
Patch
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.
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(). Created attachment 418876 [details]
Patch
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. You are right. Good catch. Created attachment 418919 [details]
Patch for landing
Committed r272179: <https://trac.webkit.org/changeset/272179> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418919 [details]. 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." |