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

Deactivated security check at compinit #396

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

toadjaune
Copy link

No description provided.

@toadjaune
Copy link
Author

See #397 for explanations.

@@ -31,7 +31,12 @@ __zplug::core::load::from_cache()

# Plugins with defer-level set
source "$_zplug_cache[defer_1_plugin]"
compinit -d "$ZPLUG_HOME/zcompdump"
if [ ${UID} ] && [ -n ${SUDO_USER} ] ; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[[ is better, and { is unneeded

-        if [ ${UID} ] && [ -n ${SUDO_USER} ] ; then
+        if [[ -n $UID ] && [[ -n $SUDO_USER ]] ; then

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one [ to much in front of SUDO_USER. Variables in {} is in this case unneeded but general good style, this way variables can be chained and/or substituted.

@babarot
Copy link
Member

babarot commented May 28, 2017

Well, there is WIP in this P-R titile. What do you mean?

@toadjaune
Copy link
Author

I had put WIP in the title because in the initial PR, I had just completely disabled the security. I guess it doesn't make sense anymore since we seem to have a proper fix.

However, I'm a bit confused by the compinit line in core.zsh, which alwas has the security disabled. Is this because it's supposed to load a dump file already generated by load.zsh ?

@toadjaune toadjaune changed the title WIP : Deactivated security check at compinit Deactivated security check at compinit Jun 3, 2017
@toadjaune
Copy link
Author

After re-reading the change request, I'm not sure what you guys are asking for :
if [[ -n $UID ] && [ -n $SUDO_USER ]] ; then or
if [[ -n $UID ]] && [[ -n $SUDO_USER ]] ; then ?

@NemesisRE
Copy link

I would say this:
if [[ ${UID} -eq 0 ]] && [[ -n ${SUDO_USER} ]]; then
If you are root "[[ ${UID} == 0 ]]" and used sudo to be it "[[ -n ${SUDO_USER} ]]" then disable security check

This will not work because syntax is wrong:
if [[ -n $UID ] && [ -n $SUDO_USER ]] ; then

And both check a condition that doesn't matter:
if [[ -n $UID ]] && [[ -n $SUDO_USER ]] ; then
$UID is always set

@toadjaune
Copy link
Author

toadjaune commented Jul 24, 2017

Changed according to your latest suggestion, @NemesisRE. Everybody happy with this ?

@qrevel
Copy link

qrevel commented Sep 18, 2017

LGTM

@babarot
Copy link
Member

babarot commented Sep 19, 2017

I'll check it later

@toadjaune
Copy link
Author

@b4b4r07, any news ?

boazy added a commit to boazy/zplug that referenced this pull request Dec 23, 2017
Squashed commit of the following Pull Request:
zplug#396

commit 8d08dc1
Author: Arnaud Venturi <arnaud.venturi@rez-gif.supelec.fr>
Date:   Mon Jul 24 18:06:27 2017 +1000

    Update load.zsh

commit 05130e1
Author: Arnaud Venturi <arnaud.venturi@rez-gif.supelec.fr>
Date:   Sat Jun 3 22:34:17 2017 +1000

    Included the style change requested in PR

commit 75fbff9
Author: Arnaud Venturi <arnaud.venturi@rez-gif.supelec.fr>
Date:   Thu Apr 27 16:26:00 2017 +1000

    Allowed insecure compinit when using sudo

commit 467075d
Author: Arnaud Venturi <arnaud.venturi@rez-gif.supelec.fr>
Date:   Fri Apr 21 12:52:18 2017 +1000

    Deactivated security check at compinit
@g0t4
Copy link

g0t4 commented May 7, 2018

How about a more generic COMPINIT_ARGS or otherwise open this up to more use cases?

I'd like to use this to solve the issue of multiple users + home-brew #428

@toadjaune
Copy link
Author

@g0t4 I'm not sure about that.
Providing a way to manually skip the security check seems sensible (since in edge cases such as yours, it's a blocking issue).

I feel however like the simple use of sudo is way more common, and should work out-of-the box, both in terms of usability and security (we're not talking about deleting the security check, but only remove the second execution)

Don't get me wrong, I agree that we need a manual override, I would just prefer to treat it as a separate issue.

@k0lter
Copy link

k0lter commented Jan 4, 2019

This is still an annoying issue while using sudo.

How about a specific variable (maybe ZPLUG_COMPINIT_UNSECURE or something else) well documented and disabled by default.

Something like this:

if [ ${UID} ] && [ -n ${SUDO_USER} ] && [ "${ZPLUG_COMPINIT_UNSECURE}" = "1" ]

What do you think?

@babarot babarot added this to the v2.5.0 milestone Jan 30, 2020
@k0lter
Copy link

k0lter commented May 16, 2020

any news on this issue?

The environment variable (either ZPLUG_COMPINIT_UNSECURE, ZSH_DISABLE_COMPFIX or COMPINIT_ARGS) diabled by default seems reasonable? what to you think?

@TiiFuchs
Copy link

This is still something I always have to patch myself when using my zsh with zplug. :/

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

Successfully merging this pull request may close these issues.

None yet

7 participants