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

Add better protection to setting $PATH #2969

Closed
simotek opened this Issue Apr 24, 2016 · 10 comments

Comments

Projects
None yet
6 participants
@simotek
Copy link

simotek commented Apr 24, 2016

As $PATH is special in fish someone at some point decided to add some protection so that if for example you do this set -x PATH "$PATH /opt/bin" Fish will suggest that you are doing this wrong and will not update the variable.

Below are some other creative ways of breaking the path in fish, it would probably be nice to warn against them as well. I guess as a rule of thumb you could always check if at least one entry in the array is a valid path (oddly enough in openSUSE the first entry in $PATH normally doesn't exist)

'# Presume $TMP contains "$PATH /usr/local/bin"echo $TMP | read -x PATH`
or
'set -x PATH $TMP'

set -x PATH FOO

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Apr 24, 2016

See also #199.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Apr 28, 2016

The fix referenced in issue #199 only partially addresses the problem and does so inconsistently. For example, this is caught and rejected:

set TMP "$PATH /usr/local/bin"
set -x PATH $TMP

As is:

set -x PATH "$PATH /usr/local/bin"

But this succeeds when it should fail according to issue #199:

set -x PATH FOO

I'm not convinced the echo FOO | read -x PATH case should have the same checks. It's a very unusual way to set PATH. I would propose simply rejecting any attempt to set PATH via the read command.

@simotek

This comment has been minimized.

Copy link

simotek commented Apr 28, 2016

If echo FOO | read -x PATH was to be rejected please make sure its clearly documented one could always do

echo STUFF | read -x FOO
set -x PATH (string split ' ' FOO)

Either way scripts generically reading environment vars are going to have issues with PATH and handle it specially.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Apr 28, 2016

Note that the something | read -x PATH form only makes sense if the -a switch is also provided. Without that switch you can only set PATH to a single directory which isn't useful and almost certainly not what the writer intended. I'm ambivalent about allowing read to set PATH even in the case of something | read -a -x PATH but it definitely should be an error if the -a switch is not present. And the argument for disallowing it even if -a is used is that someone could be tempted to do

echo /dir1:/dir2:/dir3 | read -a -x PATH

And that is obviously wrong for fish as you end up with the single element "/dir1:/dir2:/dir3". If we allow the ... | read -a -x PATH form then we have to do all the sanity checks that are done for the set -x PATH ... case. Maybe we should do that but I'm more inclined to just require people set PATH using the canonical set -x PATH ....

@simotek

This comment has been minimized.

Copy link

simotek commented Apr 28, 2016

You learn something every day :) echo '/usr/bin:/usr/sbin' | string replace ':' ' ' | read -a -x PATH is much cleaner then what I had before, but if you think it will get cut I won't update my code.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Apr 29, 2016

I'm certainly not going to be the sole decider on this issue and I may very well change my mind after further reflection. Note that you can do the same thing as your last example in a more idiomatic fashion like this:

set -x PATH (echo '/usr/bin:/usr/sbin' | string replace ':' ' ')

Which also makes it trivial to prepend or append to the existing path (appending in this case):

set -x PATH $PATH (echo '/usr/bin:/usr/sbin' | string replace ':' ' ')

Something that isn't possible with the read pipeline construction. So, yes, I'm still leaning towards disallowing modifying PATH with the read command.

@faho

This comment has been minimized.

Copy link
Member

faho commented Apr 29, 2016

There's actually no need for the pipe: set -x PATH (string replace ':' ' ' -- '/usr/bin:/usr/sbin').

Regarding read PATH, I think it's conceptually simpler to have all checks apply to all (both?) ways of setting $PATH.

@simotek: "in openSUSE the first entry in $PATH normally doesn't exist"? Which entry is that?

It's possible this is a valid use of an invalid element of $PATH - something that should override everything if it exists, so the user/admin can create it and instantly all shells will use it.

That would point towards a slightly different kind of protection: Make it easy to rollback $PATH. We could just store the $PATH value e.g. after we've processed fish_user_paths, and allow the user to get back to that.

@nanohard

This comment has been minimized.

Copy link

nanohard commented Jul 23, 2017

Yes, better protection is needed when one follows the docs.
Doing this:
set -U fish_user_paths /code/go/bin $fish_user_paths
results in this:

⟩ echo $fish_user_paths | tr " " "\n" | nl
     1  /code/go/bin
     2  /usr/local/bin

Notice how /usr/bin is missing, where before, at default, it wasn't. I took notice of it because after adding to PATH, whenever I ran ls, la, or ll it would return an error about aplay not being found... dafuq? It took me longer than it should have to figure out that /usr/bin was missing from PATH, but I'm new to fish and had to Google the answer to list the fish PATH as set by fish_user_paths.

I had also tried to set it in config.fish using some form of export, or maybe set, I can't remember. I just want to move past this experience and try to forget that a list command was looking for a sound player that I don't have on my system.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented Aug 9, 2017

@nanohard, I'm not actually sure how you ended up in that situation. Editing fish_user_paths adds to PATH, but does not replace it - so $PATH should have still contained the usual directories. Have you been able to reproduce this at all?

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 9, 2018

I think we want to go in the other direction - it's apparently common to set PATH to non-existent directories in dot files, and it's frustrating to have to workaround the error messages. We're going to be more trusting and less "helpful" here. Closing. Thanks for filing!

kuwata0037 added a commit to kuwata0037/dotfiles that referenced this issue Jan 5, 2019

Don't check whether a path exists
Setting $PATH no longer warns on non-existent directories in fish3.0.
- (fish-shell/fish-shell#2969)

kuwata0037 added a commit to kuwata0037/dotfiles that referenced this issue Jan 6, 2019

Don't check whether a path exists
Setting $PATH no longer warns on non-existent directories in fish3.0.
- (fish-shell/fish-shell#2969)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment