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

Check if we are on OpenBSD and set our dirname to /tmp/fish if we are. #6522

Merged
merged 4 commits into from
Jan 23, 2020

Conversation

qbit
Copy link
Contributor

@qbit qbit commented Jan 21, 2020

Description

OpenBSD uses unveil(2) in chromium and
firefox. This means that things outside of ~/Downloads are not visible to the
browsers.

…are.

OpenBSD uses [unveil(2)](https://man.openbsd.org/unveil) in chromium and
firefox. This means that things outside of ~/Downloads are not visible to the
browsers.
@zanchey
Copy link
Member

zanchey commented Jan 22, 2020

Is there really no way of passing a file system path into the browser? I worry about this approach causing problems down the line, as it seems like an implementation detail within OpenBSD that might change at any time.

@ammgws
Copy link
Contributor

ammgws commented Jan 22, 2020

Does this mean help.fish needs to address this too? Since it will try serve the help files locally if they exist.

@qbit qbit changed the title Check if we are on OpenBSD and set our dirname to ~/.Downloads if we are. Check if we are on OpenBSD and set our dirname to ~/Downloads if we are. Jan 22, 2020
@qbit
Copy link
Contributor Author

qbit commented Jan 22, 2020

I had a typo in the path. Fixed now.

@zanchey there is a way to add paths to the list of directories that the browser can access, that said, it requires root to modify them as they are in /etc. The other alternative is to disable unveil completely, which is not recommended.

Also the implementation is not likely to change any time soon. What gave you the impression it would?

@ammgws help stuff is fine because /usr/local/share is in the list of stuff exposed (icons and what not)

@faho faho added the bug Something that's not working as intended label Jan 22, 2020
@faho faho added this to the fish 3.2.0 milestone Jan 22, 2020
@faho
Copy link
Member

faho commented Jan 22, 2020

@qbit I don't suppose it'd be possible to convince OpenBSD to add ~/.cache (or $XDG_CACHE_HOME) to the list of allowed directories? I don't think we're entirely alone in using that.

Or, alternatively, is there a better way of figuring out we can't use it? Uname checks are a bit cheesy, and I prefer avoiding them.

Also is ~/Downloads really the best place? We want a directory that:

  • is writable by us
  • is readable, preferably only by the current user
  • isn't really "visible" to the user - we create weird fish-HEXSOUP.html files
  • can be cleaned periodically

Downloads seems to match the first two, but it won't be cleaned periodically, and it's quite visible. So users would see our files, and we might get reports that they exist.

@qbit
Copy link
Contributor Author

qbit commented Jan 22, 2020

Parts of ~/.cache are exposed (dconf, thumbnails) but not the entire directory. It's unlikely that it would be further opened up.

/tmp is available, given the permissions that are set, would that fit the bill? tmp is cleared on first boot on OpenBSD.

@faho
Copy link
Member

faho commented Jan 22, 2020

/tmp is available, given the permissions that are set, would that fit the bill? tmp is cleared on first boot on OpenBSD.

Definitely better, yes.

@qbit qbit changed the title Check if we are on OpenBSD and set our dirname to ~/Downloads if we are. Check if we are on OpenBSD and set our dirname to /tmp/fish if we are. Jan 22, 2020
@qbit
Copy link
Contributor Author

qbit commented Jan 22, 2020

Updated!

@qbit
Copy link
Contributor Author

qbit commented Jan 22, 2020

A good point was brought up, this will fail if multiple users run fish_config at the same time. There should be some unique value added to the fish part of the directory path to prevent this.

@faho
Copy link
Member

faho commented Jan 22, 2020

this will fail if multiple users run fish_config at the same time. There should be some unique value added to the fish part of the directory path to prevent this.

Wouldn't it work to use just /tmp, not /tmp/fish? We already mktemp the filename, so we shouldn't have collisions there. As long as the file isn't readable by other users (and we set umask prior) I don't see a problem with it, other than that other users get to see our files.

Or use /tmp/fish.$USER.

@qbit
Copy link
Contributor Author

qbit commented Jan 22, 2020

What about using the tempfile.TemporaryDirectory (or just NamedTemporaryFile)? That would let us remove the uname check, and gain this ability:

On completion of the context or destruction of the temporary directory object the newly created temporary directory and all its contents are removed from the filesystem.

edit I am proposing using tempfile for all OS, not just OpenBSD.

@faho
Copy link
Member

faho commented Jan 22, 2020

TemporaryDirectory is

New in version 3.2.

We do currently still support python 2.7, so we'd have to abandon that.

NamedTemporaryFile seems perfect to me, and I'm not sure why we're not using it.

@zanchey?

@qbit
Copy link
Contributor Author

qbit commented Jan 23, 2020

Updated to use tempfile.

Copy link
Member

@zanchey zanchey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok by me!

@faho, thanks for the good reasoning around the Downloads directory.

@faho faho merged commit 903fe9d into fish-shell:master Jan 23, 2020
@faho
Copy link
Member

faho commented Jan 23, 2020

Squash merged (with a more useful commit message) as 903fe9d. Thanks!

@zanchey If we want this in 3.1.0, we'd probably have to test it, especially in cygwin (because of windows' weirdness with having files open in multiple programs).

@zanchey
Copy link
Member

zanchey commented Jan 25, 2020

There's no better time to test it than the beta, so let's do it.

@zanchey zanchey modified the milestones: fish 3.2.0, fish 3.1.0 Jan 25, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants