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

Handle directories in the OpenFile method #128

Closed
mariospr opened this issue Nov 24, 2017 · 4 comments
Closed

Handle directories in the OpenFile method #128

mariospr opened this issue Nov 24, 2017 · 4 comments

Comments

@mariospr
Copy link
Member

As mentioned on IRC today, I found that the OpenFile method won't handle FDs pointing to directories even in the case where such directories are reachable both inside and outside the sandbox, which is kind of a problem for situation like one we have on Endless, where the Dropbox app (which has filesystem=home permissions granted) tries to open the file manager in the "Dropbox directory".

I've discussed this with @alexlarsson and he seems to agree on that we can add an additional check in https://github.com/flatpak/xdg-desktop-portal/blob/master/src/xdp-utils.c#L340, so that fail when the FD passed points to something that is neither a regular file nor a directory (instead of checking just for regular files).

This, of course, won't be useful in cases where such directory is not reachable inside the sandbox, since those kind of paths would need to be handled via the documents portal, and FUSE does not support directories, but it can be useful nonetheless in cases like the mentioned here.

I'll be linking to a PR soon later today.

Now see the full IRC log, for reference:

<msanchez> alexlarsson: hi, quick question about the OpenFile method if you don't mind me asking...
<alexlarsson> sure
<msanchez> only let me find a link first
<msanchez> https://github.com/flatpak/xdg-desktop-portal/blob/master/src/xdp-utils.c#L340
<msanchez> can you think of any reason why we could not extend that to (mode != S_IFREG && mode != S_IFDIR)? 
<msanchez> the reason I ask is because it would be nice to keep being able to open not just regular files but also directories (assuming you have something to handle that such as Nautilus, of course)
<alexlarsson> Well, at a minimum the fuse filesystem for documents don't handle directories
<alexlarsson> But also, documents are made for "document level" files
<msanchez> I'm not 100% sure that's a problem here. I added that extra check in there to see if it would work and that did the trick for me
<msanchez> when passing the fd for /home/mario (in a sandbox with read permission to my home) I could see the mode matching S_IFDIR
<alexlarsson> That is not the issue though
<msanchez> and just doing that change was the difference between my Dropbox flatpak being able to open the Dropbox directory or not 
<msanchez> hmm... I guess I'm missing things then
<alexlarsson> I mean, you could register one
<alexlarsson> but, could you read the directory in //run/user/1000/doc ?
<alexlarsson> I'd be surprised if it worked at all
<msanchez> ah, right, I see what you mean
<alexlarsson> and its definately not designed to be safe to do that
<msanchez> in this case I suppose it works because I have filesystem=home permissions and the /home/mario dir is reachable both inside and outside the sandbox
<msanchez> still, I'd like to understand this better, since being able to open directories via the OpenFile method is kind of important to us
<alexlarsson> Wait a minute
<alexlarsson> does the OpenFile portal even create document portal objects
<alexlarsson> I guess it does if the destination app doesn't have access
<msanchez> no, it doesn't
<alexlarsson> But, i guess for other cases it would make sense for it to work.
<msanchez> https://github.com/flatpak/xdg-desktop-portal/commit/fa008ea5ece0785ac30165c4d416cf255b02252f
<alexlarsson> msanchez: well, flatpak itself does when you spawn a desktop file handler for it
<alexlarsson> But, i guess if the handler is not sandboxed
<alexlarsson> then it makes sense to allow this
<msanchez> hmm.. .I checked this with gdb and I saw clearly that, after me adding the additional check for S_IFDIR there, the path passed to Nautilus was the one I'd expect too: `file:///home/mario`
<alexlarsson> So, yeah, i think allowing dirs make sense here
<msanchez> but again, I understand that's because the path was reachable inside the sandbox in the first place
<msanchez> cool, thanks
<alexlarsson> well, thats sort of the point of using fds
<alexlarsson> that you can pass and fd is proof that your sandbox had access to it
<msanchez> yes, and I like it
<alexlarsson> the only problem is if the target thing of the openfile doesn't have access
<msanchez> well, that's not a problem in the case of the dropbox app, at least for now 
<alexlarsson> For files this is handled by flatpak rewriting the urls to documents as needed
<alexlarsson> which *doesnt* work for dirs
<msanchez> yes, the FUSE system
<alexlarsson> But, thats such an uncommon case, so lets just have that fail
@mariospr
Copy link
Member Author

Ready for review at #130

@matthiasclasen @alexlarsson Please let me know what you think, and merge if needed. Thanks!

@TingPing
Copy link
Member

TingPing commented Jan 20, 2018

@mariospr So does this mean GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER can be supported by GtkFileChooserNativePortal now?

EDIT: I guess from reading that conversation that directories still won't work w/o permissions to them.

@mariospr
Copy link
Member Author

@mariospr So does this mean GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER can be supported by GtkFileChooserNativePortal now?

I don't think so, this is used for now only from the email and openURI portals, don't think would make a difference to GTK+'s implementation of the file chooser inside the sandbox

EDIT: I guess from reading that conversation that directories still won't work w/o permissions to them.

Correct

@jcaesar
Copy link

jcaesar commented Apr 2, 2018

I just wanted to mention that I could use this for org.openclonk.OpenClonk (which is a game, and as such gets sandboxed, but comes with an scenario editor, that needs access to project folders).

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

No branches or pull requests

3 participants