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

Add support for ZSH_DISABLE_COMPFIX flag. #1911

Merged
merged 1 commit into from
Oct 1, 2018

Conversation

NathanSMB
Copy link
Contributor

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:https://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

When using Oh My Zsh the ZSH_DISABLE_COMPFIX flag allows the zsh completion system to use files it deems to be insecure.
@ljharb
Copy link
Member

ljharb commented Sep 19, 2018

I'm a bit confused (i don't use zsh); it seems like -i is an insecure mode, which we wouldn't want by default - what is the -u flag?

@NathanSMB
Copy link
Contributor Author

-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.

@NathanSMB
Copy link
Contributor Author

This is the most relevant part of the compinit documentation.

For security reasons compinit also checks if the completion system would use files not owned by root or by the current user, or files in directories that are world- or group-writable or that are not owned by root or by the current user. If such files or directories are found, compinit will ask if the completion system should really be used. To avoid these tests and make all files found be used without asking, use the option -u, and to make compinit silently ignore all insecure files and directories use the option -i. This security check is skipped entirely when the -C option is given.

Copy link
Member

@ljharb ljharb left a 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
Copy link
Member

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

Copy link
Contributor Author

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.

@NathanSMB
Copy link
Contributor Author

NathanSMB commented Sep 21, 2018

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.

screen shot 2018-09-21 at 5 39 05 pm

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.

screen shot 2018-09-21 at 5 39 34 pm

It prints out the following error.

_main_complete:163: permission denied:
_complete:96: bad math expression: operand expected at end of string

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.

@ljharb
Copy link
Member

ljharb commented Sep 22, 2018

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.

@NathanSMB
Copy link
Contributor Author

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 -i flag. Something like NVM_SILENCE_COMPINIT. I suggest prefixing the flag with NVM since it isn't shared with another application. I can add this but just want to make sure it's the direction you want to go first.

@ljharb
Copy link
Member

ljharb commented Sep 23, 2018

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.

@NathanSMB
Copy link
Contributor Author

NathanSMB commented Sep 23, 2018

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?

@ljharb
Copy link
Member

ljharb commented Sep 23, 2018

Let’s just restore the default case to its original behavior.

@NathanSMB
Copy link
Contributor Author

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 -u or not.

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
Copy link
Member

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.

@ljharb
Copy link
Member

ljharb commented Sep 26, 2018

I think it's OK to skip the tests since autocompletion has none right now.

@NathanSMB
Copy link
Contributor Author

Added the change you requested.

@ljharb ljharb force-pushed the feature/omz-compfix-flag-support branch from 06c55d1 to 0c2efed Compare October 1, 2018 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants