-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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 support for ZSH_DISABLE_COMPFIX flag. #1911
Add support for ZSH_DISABLE_COMPFIX flag. #1911
Conversation
When using Oh My Zsh the ZSH_DISABLE_COMPFIX flag allows the zsh completion system to use files it deems to be insecure.
I'm a bit confused (i don't use zsh); it seems like |
-i silently ignores insecure files. So it would not use insecure files when initializing the autocompletion. -u uses insecure files and would be the insecure option. Which is why oh my zsh has it behind a flag. |
This is the most relevant part of the compinit documentation.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current behavior is to not use -i
or -u
- why would we want to silently ignore insecure files in the default case?
bash_completion
Outdated
@@ -85,7 +85,12 @@ __nvm() { | |||
# ZSH, load and run bashcompinit before calling the complete function. | |||
if [[ -n ${ZSH_VERSION-} ]]; then | |||
autoload -U +X bashcompinit && bashcompinit | |||
autoload -U +X compinit && compinit | |||
autoload -U +X compinit | |||
if [[ $ZSH_DISABLE_COMPFIX = true ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this loses the semantic that compinit
only runs if autoload
succeeds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed a change to fix that.
I don't think nvm needs to tell people about a potential issue with their shell every time it launches. NVM is an application to manage node versions and I believe it to be a negative user experience if I'm notified every time I launch the shell that I have insecure directories. Of course for the vast majority of the users this won't happen unless their is an issue. This is what it looks like if you have insecure directories and run compinit without a flag. If you respond with "y" then it is the equivalent of running compinit -i. If you respond with "n" and then try to autocomplete a command then the following happens. In this case I just typed "nvm" and hit the tab key. It prints out the following error.
Now if nvm was suppose to help you manage your shell I would think this was fine. But I don't want it to help me manage my shell. Oh my zsh already told me I had a permission issue, I figured out what caused it and determined it wasn't a security risk to have the permissions the way they were. And I was not able to change the permissions without breaking functionality of another application. So I set the ZSH_DISABLE_COMPFIX flag. But I still had the message in the first screenshot popping up when I launched my shell. I was surprised when I figured out it was nvm because it is not behavior I expected. I think the best option is to ignore the insecure directories to avoid inadvertently causing a security issue on a users machine and to support the ZSH_DISABLE_COMPFIX flag that is already supported by oh my zsh. |
That explanation makes sense - what do you think about continuing to offer the prompt when the env var isn't set? That would address your use case, but not change the experience for the typical user. |
I think that would be okay though I still don't think that is really the duty of nvm. If you think keeping the prompt by default is the correct path I would recommend adding another flag to use the |
Whether it's nvm's duty or not isn't really the point; it's the behavior nvm has had for many years, and nobody's noticed or complained before now :-) I'm fine adding support for this ZSH env var, but I'm not yet convinced there's value in altering the current behavior for everyone else. |
That's a fair point. Others may have experienced this behavior and come to rely on it. Especially in cases where they don't use oh my zsh and never run compinit themselves. Would you like me to just remove the -i flag in the default case or should I add a second flag to give users the option to silence without using insecure files as well? |
Let’s just restore the default case to its original behavior. |
Okay. I have reverted the default behavior so it will behave the same as before my change. Will I need to add tests for this? I don't really know how I would confirm whether compinit was run with a |
bash_completion
Outdated
@@ -85,7 +85,11 @@ __nvm() { | |||
# ZSH, load and run bashcompinit before calling the complete function. | |||
if [[ -n ${ZSH_VERSION-} ]]; then | |||
autoload -U +X bashcompinit && bashcompinit | |||
autoload -U +X compinit && compinit | |||
autoload -U +X compinit && if [[ $ZSH_DISABLE_COMPFIX = true ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use ${ZSH_DISABLE_COMPFIX-}
here, in case someone has -o
set.
I think it's OK to skip the tests since autocompletion has none right now. |
Added the change you requested. |
06c55d1
to
0c2efed
Compare
When using Oh My Zsh the ZSH_DISABLE_COMPFIX flag allows the zsh completion system to use files in directories it deems to be insecure.
In the case of the ZSH_DISABLE_COMPFIX flag not being true I added the -i flag to compinit. This flag will cause the zsh completion system to ignore files in directories it deems to be insecure. If this flag is not used the user will be prompted to either ignore insecure directories or abort compinit. Ignoring the insecure directories manually is the same as using the -i flag and aborting will cause autocompletion in zsh to fail. I believe -i would be the sane default here and this would be shared with Oh My Zsh.
Here is the relevant documentation on compinit: http://zsh.sourceforge.net/Doc/Release/Completion-System.html#Use-of-compinit
And here is an example on how Oh My Zsh handles this problem: https://github.com/robbyrussell/oh-my-zsh/blob/e96a7b728a9c15f5615f4e41a79a5f0fe2b97712/oh-my-zsh.sh#L70