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

which() should check executability before returning a value #657

Closed
nfischer opened this issue Feb 10, 2017 · 4 comments
Closed

which() should check executability before returning a value #657

nfischer opened this issue Feb 10, 2017 · 4 comments
Labels
fix Bug/defect, or a fix for such a problem help wanted

Comments

@nfischer
Copy link
Member

which() should only return a value if it finds a matching file in the PATH and that file is executable.

I'm not sure if it's possible to reliably check executability on Windows, but this should at least be done for unix. This section of the docs suggests a way of using fs.statSync() to check this info.

The right way would be to also check if the current user matches the user, group, or all fields as well, but I think this might be tricky. It should be sufficient to just assume that we do have a match, and just do an OR of all three (ex. userCanExecute() || groupCanExecute() || allCanExecute()).

@nfischer nfischer added fix Bug/defect, or a fix for such a problem help wanted labels Feb 10, 2017
@nfischer
Copy link
Member Author

@termosa, if you're interested in taking this up in a new PR after #655 is merged, just ping this thread to let me know you're working on it. Otherwise I'll take it up.

@termosa
Copy link
Contributor

termosa commented Feb 10, 2017

@nfischer I updated #655 with fixes.
This issue is interesting and can be incredibly useful in specific cases. But still I want to ask you: Are you sure you want to apply it to the which function and change its default behavior?

@nfischer
Copy link
Member Author

Are you sure you want to apply it to the which function and change its default behavior?

From the docs:

Searches for command in the system's PATH.

To me, "command" implies something which can be executed. Therefore, it's a bug to point to something which cannot be executed (but, as noted above, Windows and Unix have different definitions of "executability", and we need to make the right decision for each OS).


To examine this another way, imagine your $PATH is ~/first-folder:~/second-folder, and you have the following:

$ ls -l ~/first-folder/foo ~/second-folder/foo
-rw-r----- 1 [...] ~/first-folder/foo
-rwxr-x--- 1 [...] ~/second-folder/foo
$ foo # executes second-folder/foo, because first-folder/foo lacks exec bit
$ which foo # if this returns first-folder/foo, that would be a bug
~/second-folder/foo

@termosa
Copy link
Contributor

termosa commented Jul 12, 2018

@nfischer OK. I'll send you PR soon.

termosa added a commit to termosa/shelljs that referenced this issue Jul 12, 2018
termosa added a commit to termosa/shelljs that referenced this issue Jul 14, 2018
termosa added a commit to termosa/shelljs that referenced this issue Jul 14, 2018
termosa added a commit to termosa/shelljs that referenced this issue Jul 15, 2018
nfischer pushed a commit that referenced this issue Jul 18, 2018
On Unix, this only matches files with the exec bit set. On Windows, this only
matches files which are readable (since Windows has different rules for
execution).

Fixes #657.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug/defect, or a fix for such a problem help wanted
Projects
None yet
Development

No branches or pull requests

2 participants