flatpak / xdg-desktop-portal

Desktop integration portal
https://flatpak.github.io/xdg-desktop-portal/
GNU Lesser General Public License v2.1
603 stars 194 forks source link

parse_app_info_from_snap() sets the app id to "snap.$AppName", but gnome-shell's access dialog compares it to $DesktopFile #769

Open oSoMoN opened 2 years ago

oSoMoN commented 2 years ago

(originally incorrectly reported against gnome-shell)

When testing the proposed WebExtensions portal with Firefox packaged as a snap, the access dialog isn't displayed because the caller's appId is "snap.firefox", whereas the desktop id that gnome-shell compares it to is "firefox_firefox".

Other portals also use the app id when calling into xdp_impl_access_call_access_dialog_sync(), so it looks like these calls will consistently be denied for applications running as snaps.

The implementation of parse_app_info_from_snap() parses the output of snap routine portal-info $PID, which for firefox looks like this:

[Snap Info]
InstanceName=firefox
AppName=firefox
DesktopFile=firefox_firefox.desktop
HasNetworkStatus=false

So instead of setting the app id to "snap." + $AppName, it looks like the value of $DesktopFile (trimmed of the ".desktop" suffix) should be used. Not sure what assumptions this would break elsewhere though.

oSoMoN commented 2 years ago

One of these assumptions is in app_has_file_access() in document-portal/document-portal.c:

if (g_str_has_prefix (target_app_id, "snap."))
  {
    res = get_output (&error, "snap", "routine", "file-access",
                      target_app_id + strlen ("snap."), path, NULL);
  }
jhenstridge commented 2 years ago

There is some background on the selection of the ID in PR https://github.com/flatpak/xdg-desktop-portal/pull/155. The main goals were to pick something that (a) would not conflict with IDs used by Flatpak apps, and (b) would represent the security domain of the client.

A snap package may contain multiple command entry points, each potentially having its own desktop file (e.g. the LibreOffice snap is like this, having desktop files for Writer, Calc, Impress,etc). But all commands for a snap are effectively in the same security domain and can pass information between each other with no restriction. So we settled on snap.${pkg_name} as the ID for the XdpAppInfo struct. On the downside, this is not a valid desktop file ID like the Flatpak package names.

We can get a desktop file in a confinement-neutral method via xdp_app_info_load_app_info:

g_autoptr(GAppInfo) info = xdp_app_info_load_app_info (app_info);
g_autofree char *desktop_id = g_app_info_get_id (info);

(we could also provide a method to get the desktop file ID directly without going via GAppInfo).

So one option here would be to pass the desktop file ID to the backend UI service rather than the ID used to make security decisions. That's clearly what the UI service wants if it wants to extract translated display names and icons. But if the backend UI service is making security decisions of its own, perhaps it should have the same ID that xdg-desktop-portal is using to make its security decisions.

With that said, for the simple security policy implemented by gnome-shell's AccessDialog implementation doing a focused window match, it would seem to be more interested in the desktop ID than the snap.${package_name} IDs.

3v1n0 commented 2 years ago

We could change a bit the way gnome-shell computes the parent app, but so far we're using this strategy: https://gitlab.gnome.org/GNOME/mutter/-/blob/main/src/core/window.c#L838-886

oSoMoN commented 2 years ago

@jhenstridge I tried your suggestion to pass the desktop file ID to the backend UI service, and I can confirm this works as expected, without changes to gnome-shell. This is my patch on top of https://github.com/flatpak/xdg-desktop-portal/pull/705:

diff --git a/src/webextensions.c b/src/webextensions.c
index 340a9b1..7f20eda 100644
--- a/src/webextensions.c
+++ b/src/webextensions.c
@@ -399,12 +399,14 @@ handle_start_in_thread (GTask *task,
       g_autoptr(GVariant) access_results = NULL;
       GVariantBuilder opt_builder;
       g_autoptr(GAppInfo) info = NULL;
+      g_auto(GStrv) app_id_components;
       const char *display_name;
       g_autofree gchar *title = NULL;
       g_autofree gchar *subtitle = NULL;
       g_autofree gchar *body = NULL;

       info = xdp_app_info_load_app_info (request->app_info);
+      app_id_components = g_strsplit (g_app_info_get_id (info), ".desktop", 0);
       display_name = info ? g_app_info_get_display_name (info) : app_id;
       title = g_strdup_printf (_("Allow %s to start WebExtension backend?"), display_name);
       subtitle = g_strdup_printf (_("%s is requesting to launch \"%s\" (%s)."), display_name, server_description, arg_name);
@@ -416,7 +418,7 @@ handle_start_in_thread (GTask *task,
       g_variant_builder_add (&opt_builder, "{sv}", "grant_label", g_variant_new_string (_("Allow")));
       if (!xdp_impl_access_call_access_dialog_sync (access_impl,
                                                          request->id,
-                                                         app_id,
+                                                         app_id_components[0],
                                                          "",
                                                          title,
                                                          subtitle,
3v1n0 commented 2 years ago

g_auto(GStrv) app_id_components = NULL, and probably can set max_tokens.

3v1n0 commented 2 years ago

I feel we've to handle with this in way more places than just here.

For some reason the portal used an ID that isn't matching the snap app ID, so this is even wronger when it comes to stuff like notifications (note that this is ran from snap.acestream.mpv, but that part is ignored):

❯ dbus-monitor --session --monitor "interface='org.freedesktop.impl.portal.Notification'"
signal time=1652359757.973630 sender=org.freedesktop.DBus -> destination=:1.11630 serial=2 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.11630"
signal time=1652359757.973687 sender=org.freedesktop.DBus -> destination=:1.11630 serial=4 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameLost
   string ":1.11630"
method call time=1652359761.906632 sender=:1.92 -> destination=:1.93 serial=2103 path=/org/freedesktop/portal/desktop; interface=org.freedesktop.impl.portal.Notification; member=AddNotification
   string "snap.acestreamplayer"
   string "fooooo"
   array [
      dict entry(
         string "title"
         variant             string "test notification 2"
      )
      dict entry(
         string "body"
         variant             string "test notification body 2"
      )
      dict entry(
         string "priority"
         variant             string "low"
      )
   ]

Wich get discarded by the shell, as snap.acestreamplayer is not matching any desktop ID (while in this case acestreamplayer_mpv - as an example of app that is not matching snap name).

Also it's definitely not per app when it comes to snaps with multiple applications embedded.

This means that for example, we don't set / get permissions on a snap that contains multiple applications that may indeed have different permissions.

While that can be changed by switching to use snap.${snap_name}_${snap_app} (at least when snap_name != snap_app), removing the snap. part is IMHO the thing to do, but that would imply breaking the user settings on permissions, unless we tune the store for the snap case, so that already-set permissions on "base app" are accepted.

I think that's safe anyways because applications won't have ID's such as snap-name_snap-instance anywhere else, so there won't never be a clash. Having the snap. prefix removed is indeed something that we can also handle at shell levels, but

Another option is to use more consistently a xdp_app_info_get_desktop_id allover around the places.

3v1n0 commented 2 years ago

Also worth mentioning here (as it's related in the Gtk notifications case), that the fact we're using a different desktop-id for SNAPs is breaking many GApplication's (or in general, Freedesktop applications) under snap, as portals and shells are expected to be able to activate them or activate their actions through their well-known name, that doesn't exist in in our case.

Because a snap app with desktop ID foo_bar may still be exposing a DBus org.freedesktop.Application interface as org.Foo.Bar in /org/Foo/Bar, and we've no way to figure that out.

This is crucial in the notifications case (and probably other cases) as we need this to be able to activate the application or application's actions through this. And GNOME Shell won't either show notifications if no app with such DBus name/path is found.