Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

documents: Add support for remote files #822

Closed

Conversation

sergio-costas
Copy link

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 #213
Fix #820

@sergio-costas sergio-costas force-pushed the add_support_for_remote_files branch from 6593b34 to eefe4c2 Compare June 27, 2022 09:37
@sergio-costas
Copy link
Author

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

@sergio-costas
Copy link
Author

@hadess What do you think about this proposal?

@sergio-costas sergio-costas marked this pull request as ready for review June 27, 2022 09:44
@barthalion barthalion force-pushed the add_support_for_remote_files branch from eefe4c2 to 0c2853e Compare June 27, 2022 13:18
@sergio-costas
Copy link
Author

If this patch isn't accepted, at least #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 sergio-costas force-pushed the add_support_for_remote_files branch 2 times, most recently from 638426c to bc1ca8e Compare June 28, 2022 07:44
@advancingu
Copy link

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

@lissyx
Copy link

lissyx commented Jul 20, 2022

Gentle ping?

@jhenstridge
Copy link
Contributor

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:

/* Avoid even loading gvfs to avoid accidental confusion */
g_setenv ("GIO_USE_VFS", "local", TRUE);

This code in documents.c:

file = g_file_new_for_uri (uri);
path = g_file_get_path (file);

... 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):

flatpak/xdg-desktop-portal-gtk@ce520f7

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.

src/xdp-utils.c Outdated Show resolved Hide resolved
src/xdp-utils.c Outdated Show resolved Hide resolved
src/xdp-utils.c Outdated Show resolved Hide resolved
@sergio-costas
Copy link
Author

@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 :-)

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 flatpak#213

Fix flatpak#820

Don't use Gvfs

Remove the need for Gvfs when accessing files mounted with
gvfsd-fuse.
Use g_autoptr instead of g_free et al.

Compare strings with g_strcmp0.

Simplify the code path.
@sergio-costas sergio-costas force-pushed the add_support_for_remote_files branch from 7de872a to 4c376bf Compare July 27, 2022 10:06
@jhenstridge
Copy link
Contributor

@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).

@sergio-costas
Copy link
Author

That's a good point, indeed.

@advancingu
Copy link

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

@sergio-costas
Copy link
Author

sergio-costas commented Oct 10, 2022

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants