GNOME Bugzilla – Bug 783130
Make dbus activation sandbox-aware
Last modified: 2017-05-29 21:58:53 UTC
When launching a url handler by D-Bus, we need to make sure the launched application can actually see the file we are passing. With sandboxed applications, this is no longer automatically the case. We fix this by calling the document portal to exported the files as-needed. See the https://git.gnome.org/browse/glib/log/?h=sandboxed-dbus-activation branch. Not that this code requires version 2 of the document portal API. If we talk to an older version, the AddFull call will fail, and the activate the application with the original uris, as before.
Created attachment 352761 [details] [review] Update flatpak document portal interface This api has been changed upstream, recently.
Created attachment 352762 [details] [review] Add a wrapper for the AddFull document portal api This is a wrapper which takes a list of uris and rewrites them by calling AddFull with the file:// uris.
Created attachment 352763 [details] [review] Make dbus activation sandbox-aware When we call org.freedesktop.Application.Open to activate an application and pass file uris, the application may not be able to see the files due to a flatpak sandbox. Flatpak puts the flatpak app-id in the X-Flatpak key in desktop files that it exports, so we can easily recognize applications that may be affected by this. In this case, call the document portal to export the files and pass the resulting uri's instead of the original ones.
Review of attachment 352761 [details] [review]: > This api has been changed upstream, recently. Would be good to include a link to upstream.
Review of attachment 352762 [details] [review]: ::: gio/gdocumentportal.c @@ +173,3 @@ + GUnixFDList *fd_list = NULL; + GList *l; + int i, j; Technically these should be gsize (or at least unsigned). @@ +180,3 @@ + if (!init_document_portal ()) + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, Maybe an error code like G_IO_ERROR_NOT_INITIALIZED would be more appropriate? @@ +185,3 @@ + } + + length = g_list_length (uris); If (length == 0) this function will end up returning NULL without setting an error. Is that desired, or do we want to add a precondition check to avoid (length == 0)? @@ +202,3 @@ + int fd; + + fd = open (path, O_CLOEXEC | O_PATH); s/open/g_open/ for consistency and portability. @@ +216,3 @@ + } + + g_variant_builder_add (&builder, "u", 1 << 2); /* as-needed-by-app */ Move the flag definition out to a constant/enum somewhere, since it’s used in more than one place here. @@ +237,3 @@ + for (l = uris, i = 0, j = 0; l; l = l->next, i++) + { + char *uri = l->data; const @@ +242,3 @@ + if (as_is[i]) /* use as-is, not a file uri */ + { + ruri = g_strdup (uri); Indentation problem. @@ +259,3 @@ + } + + ruris = g_list_append (ruris, ruri); Would be more efficient to prepend here, then reverse the list before returning.
Review of attachment 352763 [details] [review]: ::: gio/gdocumentportal.c @@ -218,3 @@ - g_variant_builder_add (&builder, "u", 1 << 2); /* as-needed-by-app */ - g_variant_builder_add (&builder, "s", app_id); - g_variant_builder_add (&builder, "^as", permissions); Why has this lot been removed?
(In reply to Philip Withnall from comment #5) > + > + length = g_list_length (uris); > > If (length == 0) this function will end up returning NULL without setting an > error. Is that desired, or do we want to add a precondition check to avoid > (length == 0)? > NULL-in, NULL-out seems just right to me. Not an error, it properly handled all the documents in the list (ie: none).
(In reply to Philip Withnall from comment #6) > Review of attachment 352763 [details] [review] [review]: > > ::: gio/gdocumentportal.c > @@ -218,3 @@ > - g_variant_builder_add (&builder, "u", 1 << 2); /* as-needed-by-app */ > - g_variant_builder_add (&builder, "s", app_id); > - g_variant_builder_add (&builder, "^as", permissions); > > Why has this lot been removed? This was an accidental leftover from a version of the branch that didn't use dbus-codegen
(In reply to Matthias Clasen from comment #7) > (In reply to Philip Withnall from comment #5) > > > + > > + length = g_list_length (uris); > > > > If (length == 0) this function will end up returning NULL without setting an > > error. Is that desired, or do we want to add a precondition check to avoid > > (length == 0)? > > > > NULL-in, NULL-out seems just right to me. Not an error, it properly handled > all the documents in the list (ie: none). Agreed. I just wanted to make sure it was deliberate.
Created attachment 352784 [details] [review] Update flatpak document portal interface This api has been changed upstream, recently. A new AddFull method has been added in this commit: https://github.com/flatpak/flatpak/commit/6ce8521b640c7a69f97a2fd7c96de94eb9a83125
Created attachment 352785 [details] [review] Add a wrapper for the AddFull document portal api This is a wrapper which takes a list of uris and rewrites them by calling AddFull with the file:// uris.
Created attachment 352787 [details] [review] Make dbus activation sandbox-aware When we call org.freedesktop.Application.Open to activate an application and pass file uris, the application may not be able to see the files due to a flatpak sandbox. Flatpak puts the flatpak app-id in the X-Flatpak key in desktop files that it exports, so we can easily recognize applications that may be affected by this. In this case, call the document portal to export the files and pass the resulting uri's instead of the original ones.
Review of attachment 352787 [details] [review]: ::: gio/gdesktopappinfo.c @@ +37,3 @@ #ifdef G_OS_UNIX #include "glib-unix.h" +#include "gunixfdlist.h" This one is unnecessary and should be dropped @@ +2873,3 @@ uris ? "Open" : "Activate", g_variant_builder_end (&builder), NULL, G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL); + Some redundant whitespace changes here
Created attachment 352788 [details] [review] Make dbus activation sandbox-aware When we call org.freedesktop.Application.Open to activate an application and pass file uris, the application may not be able to see the files due to a flatpak sandbox. Flatpak puts the flatpak app-id in the X-Flatpak key in desktop files that it exports, so we can easily recognize applications that may be affected by this. In this case, call the document portal to export the files and pass the resulting uri's instead of the original ones.
Review of attachment 352784 [details] [review]: ++
Review of attachment 352785 [details] [review]: Looks good too me.
Review of attachment 352788 [details] [review]: ++
Attachment 352784 [details] pushed as a76fc7f - Update flatpak document portal interface Attachment 352785 [details] pushed as 60a1cc9 - Add a wrapper for the AddFull document portal api Attachment 352788 [details] pushed as d3b4f7c - Make dbus activation sandbox-aware