xdg-desktop-portal icon indicating copy to clipboard operation
xdg-desktop-portal copied to clipboard

documents: Add support for remote files

Open sergio-costas opened this issue 2 years ago • 11 comments

By default, xdg-desktop-portal runs with "GIO_USE_VFS=local" to guarantee that it only sends local URIs, but that also means that the user can't access files that are mounted locally using GVfs.

This patch filters all the URIs sent and replaces, wherever possible, any remote URI (like sftp://, smb://...) with the corresponding one in the local filesystem created by GVfs.

Fix https://github.com/flatpak/xdg-desktop-portal/issues/213 Fix https://github.com/flatpak/xdg-desktop-portal/issues/820

sergio-costas avatar Jun 27 '22 09:06 sergio-costas

I think that this patch is much better than the original.

sergio-costas avatar Jun 27 '22 09:06 sergio-costas

@hadess What do you think about this proposal?

sergio-costas avatar Jun 27 '22 09:06 sergio-costas

If this patch isn't accepted, at least https://github.com/flatpak/xdg-desktop-portal/pull/823 should, to correctly detect the case where a remote URI was selected by the user in a case where they aren't supported.

sergio-costas avatar Jun 27 '22 13:06 sergio-costas

So what's the next step here? What needs to be done to get a merge decision?

advancingu avatar Jul 12 '22 09:07 advancingu

Gentle ping?

lissyx avatar Jul 20 '22 15:07 lissyx

I think this is something we want to support, but the implementation is the wrong approach. The better fix is probably to remove this code:

https://github.com/flatpak/xdg-desktop-portal/blob/750b9e5d6d7cdf451bb4f9f4fc97682fafa1a364/src/xdg-desktop-portal.c#L380-L381

This code in documents.c:

https://github.com/flatpak/xdg-desktop-portal/blob/750b9e5d6d7cdf451bb4f9f4fc97682fafa1a364/src/documents.c#L79-L80

... will already rewrite GVFS URIs to the corresponding FUSE file paths if we're not forcing GIO_USE_VFS=local:

>>> file = Gio.File.new_for_uri('smb://elzar/media/foo.txt')
>>> file.get_path()
'/run/user/1000/gvfs/smb-share:server=elzar,share=media/foo.txt'

Outside of the register_document code, the only other place that uses GIO to with non-file URIs is the OpenURI portal. I think the code is fairly safe, since it codes its own lookup of non-local URIs to only search for scheme handlers. There is however a g_file_new_for_uri call within the g_app_info_launch_uris implementation. I think this is probably also safe, since it just seems to be formatting the URI to pass it on the command line to the application.

I think the root cause for the bug reports was this change in xdg-desktop-portal-gtk (which was then inherited by x-d-p-gnome):

https://github.com/flatpak/xdg-desktop-portal-gtk/commit/ce520f7bb4a60e669669402c9ffae9aeb4799943

The impl service started passing us GVFS URIs without us ever testing whether it worked. Given that change is now 4 years old, it's probably not worth trying to update the contract between x-d-p and the impl services to indicate whether VFS URIs are supported or not.

jhenstridge avatar Jul 27 '22 03:07 jhenstridge

@jhenstridge I fully agree in that the right solution was to remove that. In fact, I proposed that too, but they were reluctant to remove it. That's why I prepared this other patch, at least as a temporary solution. But of course, if you think that it is safe to remove that line and fully enable GVFS, I will be very happy to burn this MR and bury the remains very, very deep :-)

sergio-costas avatar Jul 27 '22 09:07 sergio-costas

@sergio-costas: I think it is going to be hard to reproduce the GVFS logic, and even if we do it'd risk getting out of sync.

It also seems the path translation doesn't always match the logic in this PR. While it seems to be correct for e.g. accessing my camera via a gphoto2:// URI, the smb URI I mentioned above is a bit different:

>>> file = Gio.File.new_for_uri('smb://elzar/media/foo.txt')
>>> file.get_path()
'/run/user/1000/gvfs/smb-share:server=elzar,share=media/foo.txt'

Your code looks like it'd instead generate the file path /run/user/1000/gvfs/smb:host=elzar/media/foo.txt. This second path doesn't actually work to access the files, so it's not just a case of having multiple paths representing the same resource.

Even if we add special case logic for SMB, it's quite possible that there are yet more GVFS backends that don't follow the common pattern. At some point it must be easier to just turn gvfs back on, and ensure we are careful with URIs supplied by sandboxed applications (which it already looks like we do).

jhenstridge avatar Jul 27 '22 10:07 jhenstridge

That's a good point, indeed.

sergio-costas avatar Jul 27 '22 10:07 sergio-costas

Friendly ping to ask what needs to be done to get this moving again? The PR has been open for three months.

advancingu avatar Oct 07 '22 19:10 advancingu

So... I see two possible ways of fixing this:

  • we can finally enable using GVFS by removing g_setenv ("GIO_USE_VFS", "local", TRUE);

  • or we can create a very simple daemon that allows to transform remote URIs into local ones using GVFS using DBus, running outside the flatpak/snap, and let the container security manage whether the contained program can or cannot access that local file. That way, the GVFS code that has access to remote places is isolated and has a very specific interface. A call to this DBus interface would replace the code in this MR.

Obviously, the most elegant option is the first one, but the second one can be used temporarily while there are still doubts about the implications of enabling the whole GVFS inside the portal.

But, as usual, is just an idea... If you say "no way", I'll completely scrap it, but if you say "well... show us the code and let's see" I can prepare a MR.

sergio-costas avatar Oct 10 '22 10:10 sergio-costas

I discovered a simpler way of fixing this: https://gitlab.gnome.org/GNOME/xdg-desktop-portal-gnome/-/merge_requests/67

The point is that xdg-desktop-portal-gtk does the translation from SMB/SFTP/other remote file systems into a local URI using FUSE before sending the list to xdg-desktop-portal, but xdg-desktop-portal-gnome doesn't.

sergio-costas avatar Jan 30 '23 09:01 sergio-costas