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

File chooser still mount under /run/user/<uid>/doc even if it has the permission to open it directly #1120

Closed
koplo199 opened this issue Oct 2, 2023 · 19 comments
Labels
portal: documents Issues with the documents portal

Comments

@koplo199
Copy link

koplo199 commented Oct 2, 2023

Even with the permission to open a file directly, the file chooser still mount it under /run/user/<uid>/doc/<hex_id_number>.
This is unexpected, and opening the exact same file but with the FileChooser portal disabled1 returns the correct path.

It for instance breaks Bottles, which uses Gtk.FileChooserNative.
Path returned without the FileChooser portal:

  • ~/.var/app/com.usebottles.bottles/data/bottles/bottles/my_bottle_name/my_executable_name.exe

Path returned with the FileChooser portal:

  • /run/user/1000/doc/d3ceb594/my_executable_name.exe

In the later case my_executable_name.exe won't be able to load its dll's, causing the breakage.

Footnotes

  1. Using: flatpak run --no-talk-name="org.freedesktop.portal.FileChooser"

@Mikenux
Copy link

Mikenux commented Oct 2, 2023

Does this work by launching the executable directly from Bottles (i.e. with a shortcut or something else from the UI)?

  1. Using: flatpak run --no-talk-name="org.freedesktop.portal.FileChooser"

I thought it didn't work...

@koplo199
Copy link
Author

koplo199 commented Oct 3, 2023

Does this work by launching the executable directly from Bottles (i.e. with a shortcut or something else from the UI)?

I'm not sure of what you mean exactly, but yes the affected buttons are both the "Run Executable" and the "Add shortcuts" buttons, shown in the screenshot below.

Bottles DarkBottles Light

  1. Using: flatpak run --no-talk-name="org.freedesktop.portal.FileChooser"

I thought it didn't work...

Again, I'm unsure of what you mean. The "good" path returned in my first comment was the result of an "Add shortcuts" and Bottles running with flatpak run --no-talk-name="org.freedesktop.portal.FileChooser" com.usebottles.bottles, while the "bad" path was returned by following the exact same steps, only differing by running Bottles normally.

EDIT: I precise that I only used Bottles as a real-world example, the result would be the same for any other flatpak application using the FileChooser portal.

@Mikenux
Copy link

Mikenux commented Oct 3, 2023

In other words, will a program installed successfully with Bottles launch? For example, referring to your screenshots, does Battle.net launch correctly?

Nothing to worry about in my comment on the command: I just thought that using --no-talk with the file chooser didn't work to disable it (but maybe it only has the effect of returning the correct paths?).

So for your bug this means that the portal needs to know that the executable was launched from the application directory and then use the correct path to access the other files.

@koplo199
Copy link
Author

koplo199 commented Oct 3, 2023

In other words, will a program installed successfully with Bottles launch? For example, referring to your screenshots, does Battle.net launch correctly?

No offense but I would like to keep this issue about the above bug, not about Bottles internals (unless it is absolutely necessary).

But to answer your question it depends: Bottles stores the returned executable path in the bottle configuration file. If a "good" path is stored from the beginning, it will work. Otherwise, it won't.

Nothing to worry about in my comment on the command: I just thought that using --no-talk with the file chooser didn't work to disable it (but maybe it only has the effect of returning the correct paths?).

This is correct.

So for your bug this means that the portal needs to know that the executable was launched from the application directory and then use the correct path to access the other files.

No, or at least this is incomplete.
The portal only needs to return the real path if the application has access to it without the portal. Simple as that.

@Mikenux
Copy link

Mikenux commented Oct 3, 2023

I would say that if the file to be launched belongs to the application and is located in the application directory, no problem. However, from a UX perspective, one could argue that something like this isn't necessary if launching from the program list or launching from a .desktop file works.

@koplo199
Copy link
Author

koplo199 commented Oct 4, 2023

Both of your assessments are false, but that's okay, you do you.

@Mikenux
Copy link

Mikenux commented Oct 4, 2023

I haven't done any assessment.

If you want to launch an executable from the application directory (i.e. ~/.var/app/com.usebottles.bottles/), then I think it will be better to get things working (if this is not the case) to launch it from the program list and from a launcher, so nothing to do with the file chooser. I don't think anyone will add a method for just one case. In this case, giving more cases if you have any in mind is welcome.

@koplo199
Copy link
Author

koplo199 commented Oct 4, 2023

If you want to launch an executable from the application directory (i.e. ~/.var/app/com.usebottles.bottles/), then I think it will be better to [...]

Again, this is not an issue about Bottles internals.

I don't think anyone will add a method for just one case. In this case, giving more cases if you have any in mind is welcome.

This is not a request to "add a method for just one case": any flatpak application using Gtk.FileChooserNative will have the same issue, and no new method need to be added. The /run/user/<uid>/doc mounting alone is known to cause plenty of issues, what I'm proposing is simply to avoid doing it where it's absolutely not necessary.

You seem to want to help, and I thank you for that. I'm however no broken record and thus not interested in pursuing the exact same discussion over and over. I'll have to stop replying to your comments, but I'm grateful for the time you gave to me on this matter.

@GeorgesStavracas
Copy link
Member

Reading the description, I think the underlying use case is "opening a file and grant access to neighboring files", not necessarily how the file path is formatted. So from a feature perspective, this is just a consequence of not having #463.

@koplo199
Copy link
Author

koplo199 commented Oct 4, 2023

Reading the description, I think the underlying use case is "opening a file and grant access to neighboring files", not necessarily how the file path is formatted. So from a feature perspective, this is just a consequence of not having #463.

It's not an accurate description of the issue. To reuse the example given in the description, the executable could open any file it wants on disk, granting Bottles has the permission to access it. It has hardly something to do with neighboring files.

@GeorgesStavracas
Copy link
Member

In the later case my_executable_name.exe won't be able to load its dll's, causing the breakage.

This is what was mentioned. Which programs load random DLLs in random locations?

In any case, this sounds fundamentally incompatible with any sandboxing mechanism.

@Mikenux
Copy link

Mikenux commented Oct 4, 2023

@koplo199: Could you give a way to reproduce your bug? (which program, how it was installed with Bottles, etc.)

@koplo199
Copy link
Author

koplo199 commented Oct 4, 2023

In the later case my_executable_name.exe won't be able to load its dll's, causing the breakage.

This is what was mentioned. Which programs load random DLLs in random locations?

This was merely an example to illustrate the bug in case my wording for it was unclear. And I feel it had the inverse effect: the wording for my examples made the bug unclear. I could remove the DLL mention if that is what confuse you.

In any case, this sounds fundamentally incompatible with any sandboxing mechanism.

No it's not, and I'm now sure there is a misunderstanding. This is not a sandbox issue. The issue is caused by, and only by, the FileChooser copying the file over when it don't have to. Why does the FileChooser not have to copy it over? Because the file is already inside the sandbox. The application already have access to it. The real path should be returned instead. English is not my native language, and I genuinely can't tell if I'm being unclear or not. From my point of view I repeat the same thing over and over again, and to be honest, I begin to feel a bit irritated because as you can imagine this is far from pleasant.

To me, the simple wording I used was:

The FileChooser portal needs to return the real path if the application has access to it without the portal.

Honestly I feel like closing this issue and reopening one without any mentions of Bottles, since I only have to answer question about Bottles internals instead of clarification regarding the bug.

Something I also hadn't mentioned is that the effect stack. Trying to open the same file multiple time will each time copy it inside /run/user/<uid>/doc. You could literally fill your entire drive by opening a big enough file multiple times. This is not expected, especially when, for the last time, the file does not have to be copied even once.

@GeorgesStavracas
Copy link
Member

That's the challenge of communication - you have a lot of context in your head that makes this issue obvious to you, but it's not a context shared with anybody else in this discussion. I think I understand the problem now.

It wasn't truly random access after all - it's random access within the sandboxed environment. This detail escaped me all this time. If the location is outside the sandbox, it of course has to return a document portal path; otherwise... what? return the real path? Not make Bottles or GTK go through the documents portal when the path is already available? I'm not sure

@GeorgesStavracas
Copy link
Member

Which version of xdg-desktop-portal are you using?

@Mikenux
Copy link

Mikenux commented Oct 4, 2023

I tested with Bottles 51.9 (from flathub) with default permission set. I installed Steam and Foxit Reader (from an installer downloaded from their websites) and launching them with "Run Executable..." works. I installed Pokemon Infinity from Bottles, but then I removed it by deleting files. I downloaded it from website, then decompressed it into a "Program Files (x86)" of a bottle, and launching it works. Version of xdg-desktop-portal is 1.16.0 (-3 from Fedora 38).

So what am I missing to reproduce the bug, in case the version of xdg-desktop-portal you (@koplo199) are using is 1.16.0?

@GeorgesStavracas GeorgesStavracas added portal: documents Issues with the documents portal need more info labels Oct 5, 2023
@GeorgesStavracas
Copy link
Member

GeorgesStavracas commented Oct 5, 2023

So yesterday I took a look at the Document portal code, and apparently it does what this issue expects: it checks if the file is accessible to the app, and if it is, it doesn't go through the FUSE code paths:

if (as_needed_by_app &&
app_has_file_access (target_app_id, target_perms, path))
{
id = g_strdup ("");
}

On Flatpak, this function spawns flatpak info --file-access=PATH APP_ID:

{
/* First we try flatpak info --file-access=PATH APPID, which is supported on new versions */
arg = g_strdup_printf ("--file-access=%s", path);
res = get_output (&error, "flatpak", "info", arg, target_app_id, NULL);
}

If that fails, the Document portal tries a fallback path that may produce false negatives, but no false positives:

/* Secondly we fall back to a simple check that will not be perfect but should not
cause false positives. */
return app_has_file_access_fallback (target_app_id, target_perms, path);
}

So now the question is: under which circumstances specifically you can reproduce this issue; and if you get a reliable reproducer, and run flatpak info --file-access=PATH APP_ID directly, what does it return?

Another question that may or may not be important is: what system are you running this on? Is it a locked down OSTree image? Regular distro?

@GeorgesStavracas GeorgesStavracas added the needs diagnosis Root cause of the issue needs to be diagnosed label Oct 5, 2023
@koplo199
Copy link
Author

koplo199 commented Oct 6, 2023

That's the challenge of communication - you have a lot of context in your head that makes this issue obvious to you, but it's not a context shared with anybody else in this discussion. I think I understand the problem now.

It wasn't truly random access after all - it's random access within the sandboxed environment. This detail escaped me all this time.

That's exactly the issue!

If the location is outside the sandbox, it of course has to return a document portal path; otherwise... what? return the real path?

No the current behaviour is totally fine, it should indeed return a document portal path (otherwise the application wouldn't be able to access it). The only difference is that for the same file, the same path should be returned, to avoid useless duplications.

GeorgesStavracas: Which version of xdg-desktop-portal are you using?
Mikenux: So what am I missing to reproduce the bug, in case the version of xdg-desktop-portal you (@koplo199) are using is 1.16.0?

I confirm I was using 1.16.0. I upgraded to 1.18.0 just a few hours ago and... it fixed EVERYTHING! Literally every aspect of the bug described in this issue got fixed with this update. No more duplication for files outside of the sandbox (now the same path is returned for the same file, as described above). For files inside the sandbox, the real path is now returned. It behave exactly as one would expect.

Mikenux: Version of xdg-desktop-portal is 1.16.0 (-3 from Fedora 38).
GeorgesStavracas: Another question that may or may not be important is: what system are you running this on? Is it a locked down OSTree image? Regular distro?

To provide more informations regardless: I'm using a regular Arch Linux install. No quirks, just a plain regular install. I run pacdiff fairly regularly so I know I also don't have outdated configuration files.

@GeorgesStavracas
Copy link
Member

Alright, thanks for testing it with a newer version. I'll close this as resolved. The PR that fixed this was #1007.

@GeorgesStavracas GeorgesStavracas removed need more info needs diagnosis Root cause of the issue needs to be diagnosed labels Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
portal: documents Issues with the documents portal
Projects
Status: Triaged
Development

No branches or pull requests

3 participants