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

Don't trust process.env.SHELL #2

Closed
silverwind opened this issue Feb 15, 2016 · 8 comments
Closed

Don't trust process.env.SHELL #2

silverwind opened this issue Feb 15, 2016 · 8 comments

Comments

@silverwind
Copy link

As seen in sindresorhus/shell-path#10 (comment), process.env.SHELL can't be trusted to work in certain situations, leading to hard to debug errors.

I think we should first stat and check for the executable bit, and possibly fallback to well-known shell paths like /bin/bash or /bin/zsh on error.

@sindresorhus
Copy link
Owner

Don't trust process.env.SHELL

👎 I don't think we should distrust it just because one user has a misconfigured system. With that logic nothing can be trusted. I would be open to it if it were a common problem, but that's not the case.

@silverwind
Copy link
Author

I tend to agree, but I think it's at least worth investigating if there are more reliable ways find the user's shell, a environment variable just seems too brittle. On Linux, I'd probably put more trust into the passwd entry.

@sindresorhus
Copy link
Owner

Alright. I did research doing more extensive checks initially, including using passwd, but didn't want to complicate as I discovered the current solution would work in 99% of cases.

@silverwind
Copy link
Author

Okay let's leave it simple. Doing passwd parsing cross-platform would be too messy.

@sindresorhus
Copy link
Owner

Doing passwd parsing cross-platform would be too messy.

Not even possible. OS X no longer uses passwd. https://github.com/sindresorhus/passwd-user/blob/09cc55af4d8d0a94885ce318cd3993d44ef7aa44/index.js#L81-L83

@rickgregory
Copy link

Since I started this little issue in the linked (now closed) bug... let me know if you need me to test out anything. While I've done a clean install on my Mac, I have a cloned, bootable drive of the disk as it was when it showed the problem.

@silverwind
Copy link
Author

@rickgregory one think I'm still wondering about is how your regular shell was still working. I wouldn't totally exlude the possibilty of a bug in electron.

Maybe the output of these from your terminal may help us track this further:

/bin/zsh --version
which zsh
echo $SHELL
/usr/bin/id -P $(whoami)

@rickgregory
Copy link

My standard shell is bash, not zsh. I'll boot from the external clone
tonight and try out the commands above. For now, here's what a clean 10.11.3 install gives:

$ /bin/zsh --version
zsh 5.0.8 (x86_64-apple-darwin15.0)
$ which zsh
/bin/zsh
$ echo $SHELL
/bin/bash
$ /usr/bin/id -P $(whoami)
rickg:********:501:20::0:0:richard gregory:/Users/rickg:/bin/bash

On Tue, Feb 16, 2016 at 1:45 PM, silverwind notifications@github.com
wrote:

@rickgregory https://github.com/rickgregory one think I'm still
wondering about is how your regular shell was still working. I wouldn't
totally exlude the possibilty of a bug in electron.

Maybe the output of these from your terminal may help us track this
further:

/bin/zsh --version
which zsh
echo $SHELL
/usr/bin/id -P $(whoami)


Reply to this email directly or view it on GitHub
#2 (comment)
.

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