sudo and commands only in /sbin #592

Raizo62 · 2018-12-18T10:24:52Z

Hi

Sorry, i come back for an other problem with sudo

With "sudo ls" , "ls" is colorized correctly with green

with "sudo ifup" where "ifup" is only in "/sbin", "ifup" is colorized in red

danielshahaf · 2018-12-18T12:22:44Z

with "sudo ifup" where "ifup" is only in "/sbin", "ifup" is colorized in red

Known issue. z-sy-h looks for commands in $PATH, but sudoers(5) can define an alternative path for root. Both the path value and the directories specified therein may be unreadable by non-root users, so there's no way to fix this in the general case. Ideas are welcome though.

Raizo62 · 2018-12-18T17:33:25Z

Can z-sy-h define a specific variable used only with "sudo" code of z-sy-h which contains default path of root's commands (as /sbin).
If an user defines an alternative path for root in sudoers, then he can modify this variable ?

danielshahaf · 2018-12-18T17:39:33Z

Can z-sy-h define a specific variable used only with "sudo" code of z-sy-h which contains default path of root's commands (as /sbin). If an user defines an alternative path for root in sudoers, then he can modify this variable ?

Such users would also need to send the command-path style in the :completion:*:sudo:* zstyle context. It would be nice to have completion and z-sy-h both obtain the value from the same place.

Raizo62 · 2018-12-18T18:25:21Z

On my zsh, the completion of "ifup" fails, and the completion of "sudo ifup" successes.
How the completion of zsh works ?

I want to install z-sy-h on OS for students in System Administration (Live Raizo), but it's very annoying if it shows a bad information : ifup is correct only behind sudo (if i am not root). And i can't add "/sbin" in the path of user (and it is not realistic)

danielshahaf · 2018-12-19T07:01:23Z

zstyle ":completion:*:sudo:*" command-path /bin /sbin .... Please ask further completion questions on zsh-users@ or #zsh on FreeNode to distribute the workload.

/sbin is in users' PATH on FreeBSD.

Good luck with your course!

Raizo62 · 2018-12-19T10:59:08Z

zstyle ":completion:*:sudo:*" command-path /bin /sbin .... Please ask further completion questions on zsh-users@ or #zsh on FreeNode to distribute the workload.

Sorry, i believe that i was not been clear : I didn't ask how to do the completion on zsh for /sbin.
I said that the completion of zsh can have different actions with the word "sudo" .
When i am user, the completion of zsh doesn't show "ifup" , but if i use the word "sudo", zsh shows "ifup"
Then my question was : why can z-sy-h not do the same ?

/sbin is in users' PATH on FreeBSD.

It is not in Debian, and i think that is a good thing ;-)

danielshahaf · 2018-12-19T11:32:14Z

Because that hasn't been implemented. Patches welcome.

Raizo62 · 2018-12-19T12:54:22Z

I can try....

danielshahaf · 2018-12-19T14:51:34Z

Great! Let's open a new issue to discuss possible approaches.

Raizo62 · 2018-12-19T15:32:31Z

I think to add in "_zsh_highlight_main__type" something as
if $(sudo which "$1" >/dev/null) ; then; REPLY=command; fi
But this command must be called only if i am in "sudo" command. Perhaps can i add a new parameter at the function _zsh_highlight_main__type ?
But do you have already a variable to know if we are in sudo command ?

danielshahaf · 2018-12-19T16:37:48Z

-1. It is unacceptable to run sudo from z-sy-h.

danielshahaf · 2018-12-19T16:38:47Z

Knowing whether the command word is sudo is easy.

Raizo62 · 2018-12-19T18:12:24Z

-1. It is unacceptable to run sudo from z-sy-h.

Ok, it's more complicated, but you're right.

And with this code ?

declare -a PATH_ROOT=("/sbin/" "/usr/sbin" "/usr/local/sbin")
if [ -n "$(find "$PATH_ROOT[@]" -maxdepth 1 -name "$1" -print -quit)" ]; then REPLY=command; fi

danielshahaf · 2018-12-19T19:10:43Z

This is plausible. We can't solve the general case, but a solution that's sound though not complete is worth consideration.

danielshahaf · 2018-12-19T19:14:37Z

@phy1729 @nicoulaj WDYT?

(Yes, it should be rewritten with a nullglob; I'm asking about the functionality and approach, not implementation.)

danielshahaf · 2018-12-19T19:18:31Z

@Raizo62 Thanks for putting up with my briefer replies that I sent from the mobile.

nicoulaj · 2018-12-20T17:22:59Z

Isn't this costly performance wise ? Also, not a big fan of hard coding the path like this, and subtle issues can occur because of the order of PATH entries, so it is seems tricky to get this right.

I think the zstyle approach is the best one (zstyle ":zsh-syntax-highlighting:*:sudo:*" command-path /bin /sbin ...). If you are in this situation you already have to hardcode root's PATH in your zshrc for completion anyway.

danielshahaf · 2018-12-20T18:03:51Z

Isn't this costly performance wise ? Also, not a big fan of hard coding the path like this, and subtle issues can occur because of the order of PATH entries, so it is seems tricky to get this right.

We could make it a bit fancier with a function that locally does path=( "${(@)path/\/bin/\/sbin}" ), runs 'rehash', and copies $commands to another name... but that's still guesswork and heuristics. How about a much KISSier approach: in 'sudo foo' always make 'foo' not highlighted at all. The rationale is that we can't know what sudoers(5) does: it could both remove and add $PATH entries.

I think the `zstyle` approach is the best one (`zstyle ":zsh-syntax-highlighting:*:sudo:*" command-path /bin /sbin ...`). If you are in this situation you already have to hardcode root's PATH in your zshrc for completion anyway.

(To clarify, z-sy-h does not currently honour any zstyles whatsoever, so no one should set that zstyle in their zshrc.) Sure, that makes sense as a larger / longer-term change.

Raizo62 · 2018-12-20T18:15:46Z

Isn't this costly performance wise ?

Not often costly, if we call this code only when we use "sudo"

The rationale is that we can't know what sudoers(5) does: it could both remove and add $PATH entries.

If user adds PATH entries in sudoes, he can also add them in config of z-sy-h

It is not a perfect solution, but often a good solution

danielshahaf · 2018-12-20T19:00:09Z

Not often costly, if we call this code only when we use "sudo"

You do realize that z-sy-h's main loop gets called for _every single character_ typed at the prompt? That's why we should avoid stat()s in the main loop.

Raizo62 · 2018-12-20T19:20:33Z

Not often costly, if we call this code only when we use "sudo"
You do realize that z-sy-h's main loop gets called for every single character typed at the prompt? That's why we should avoid stat()s in the main loop.

Yes. I have tested the code that i have proposed ;-)
Do you prefer a loop on $PATH_ROOT with test '-e' ?
And i suggest to do this test at the end of _zsh_highlight_main__type, and only on the word(letter) after a "sudo"

phy1729 · 2018-12-20T21:55:42Z

I'm not terribly fond of special casing sudo. Users could also be restricted to particular binaries even with particular arguments. Then there's doas and pkexec that could also be special cased.

Saying sudo will be treated just like every other precommand is a clear line, but adding this special case leads to more complexity to become more correct.

If we do special case sudo, we should probably use the completion zstyle, so users only have to configure it twice (once for sudo itself too).

danielshahaf · 2018-12-20T22:29:57Z

I'm not terribly fond of special casing sudo. Users could also be restricted to particular binaries even with particular arguments. Then there's doas and pkexec that could also be special cased. Saying sudo will be treated just like every other precommand is a clear line, but adding this special case leads to more complexity to become more correct.

Then what do you think about the other option I suggested --- making the 'foo' in 'sudo foo' not highlighted at all? This needn't be a special case for sudo; that rule could be applied to other precommands, such as doas(1) and jexec(1). On the other hand, the behaviour of master is correct for precommands such as nice(1) and moreutils' chronic(1), so perhaps we should have two classes of precommands: those that resolve their argv[0]-to-be argument in the same way this zsh process would, and those that don't --- but only two classes, not N. ?

Raizo62 · 2018-12-21T18:53:06Z

"sudo" is a command very used and z-sy-h will be a great help for users who write commands.

Other idea to code :

_zsh_highlight_main__type()
{
...
  local OLDPATH="${PATH}"
  PATH="${PATH}:${PATH_ROOT}"

  if zmodload -e zsh/parameter; then
...
  PATH="${OLDPATH}"
...
}

User must define : export ROOT_PATH="/sbin:/usr/sbin:/usr/local/sbin"

Raizo62 · 2018-12-22T08:07:36Z

Could you create a new branch where i can beginning to write a proposition of patch ?
I will can do on my own repository, but my second ask will be less easy :

But do you have already a variable to know if we are in sudo command ?

Knowing whether the command word is sudo is easy.

Could you show me how (and where) to do that ? (i can, but i think that will not be very tidy)

danielshahaf · 2018-12-22T19:24:23Z

For a branch please use one in your own repository and PR here for reviews. However, please remember we haven't yet agreed what we want the functionality to end up being — whether we'd not highlight the ls in sudo ls ever, or whether we'd accept some sort of "root's $PATH" input, and if so whether that'd be via zstyles or via parameters, and so on.

I don't want to discourage you, but I don't want you to burn out or be frustrated either; by all means do feel free to experiment and to sketch out proposals, but remember consensus hasn't yet emerged from this discussion.

To figure out whether sudo was the command word you'd use a state flag and reset it at the end of every simple command, when a | or ; or & or && (or one of their friends) is detected. It'd be somewhere in _zsh_highlight_main_highlighter_highlight_list; there's already code for detecting sudo (you'll see it add :sudo_opt: to the state) and for detecting the end of the simple command (that resets highlight_glob and some other state) that you'll just need to extend.

danielshahaf · 2018-12-22T19:25:08Z

... and I'm glad to see new people being interested in writing code for z-sy-h. :)

Raizo62 · 2018-12-24T09:00:29Z

I propose a code here : https://github.com/Raizo62/zsh-syntax-highlighting/tree/sudo

If you use z-sy-h as before (without define new varable), you have your old result :
sudo ifup # ifup stays red (if ifup is not in your path)

if you define the variable ZSH_HIGHLIGHT_SPECIAL_PATH

ZSH_HIGHLIGHT_SPECIAL_PATH="/sbin:/usr/sbin:/usr/local/sbin"
sudo ifup     # ifup is green

and i believe that the new code is very efficient :

  if [ "$this_word" = ":sudo_opt::start:" ]; then
    local PATH="${PATH}:${ZSH_HIGHLIGHT_SPECIAL_PATH}"
  fi

danielshahaf · 2018-12-24T10:09:57Z

Thanks for writing that! As I said before, we still have to achieve consensus on what to do before we decide how to do it, but having a proof of concept will be helpful.

Raizo62 · 2018-12-24T10:33:40Z

Yes, i understand.

In your thoughts, don't forget that :
sudo is very used and a big distribution like Debian doesn't add "/sbin" in the path of user.
With this code :

you show to user that his command has a bad syntax even behind sudo : "sudo ifpu"
your indications are less "bad" : "sudo ifup" is not a bad command (even if /sbin is not in your path)

This is a real need.

Also, perhaps that z-sy-h can initialize ZSH_HIGHLIGHT_SPECIAL_PATH if it doesn't detect sbin in the PATH of user ?

Raizo62 · 2019-03-25T07:43:29Z

I don't understand : If a user doesn't define the variable "ZSH_HIGHLIGHT_SPECIAL_PATH" (it is the default case), then zsh doesn't highlight commands behind "sudo". It is that you want, doesn't you ?

danielshahaf · 2019-03-25T16:16:29Z

I don't understand : If a user doesn't define the variable "ZSH_HIGHLIGHT_SPECIAL_PATH" (it is the default case), then zsh doesn't highlight commands behind "sudo". It is that you want, doesn't you ?

That is what I proposed, but it's not what your code does. With your code, if the variable is unset, commands after sudo would be highlighted according to the user's default $PATH.

As I said, the next step is for this thread to consense on what the new behaviour should be. You propose that there should be a new parameter that simply modifies PATH. There are a number of questions about that (summarizing from this thread):

What about precommands other than sudo?
What about sudo -u alice ifup? (which wouldn't use root's PATH)
What about sudoers(5) configurations that also restrict the allowed commands and/or arguments?
Can we interoperate with the zshcompsys(1) command-path style, that may be already set?
If we change $PATH, we have to rehash… but if we rehash too often, performance will suffer, and if we rehash too seldom, highlighting will be wrong. How to implement this both correctly and performantly?
What should the default behaviour be (when no custom paths are set)?

Also, your patch would cause /usr/sbin/grep to be called in preference to /usr/bin/grep, if _zsh_highlight_main__type started calling grep for some reason. I'm not really comfortable with that: the user-specified $PATH should not affect z-sy-h's own code in any way. (Sorry, I should have noticed this sooner.)

Raizo62 · 2019-03-25T18:18:43Z

I don't understand : If a user doesn't define the variable "ZSH_HIGHLIGHT_SPECIAL_PATH" (it is the default case), then zsh doesn't highlight commands behind "sudo". It is that you want, doesn't you ?

That is what I proposed, but it's not what your code does. With your code, if the variable is unset, commands after sudo would be highlighted according to the user's default $PATH.

This is also what your current code does. With this patch, it can do a little more.
And it can answer to the very recurrent problem : sudo ifpu
Then i am not agree with you and i don't prefer to not highlighting at all.

As I said, the next step is for this thread to consense on what the new behaviour should be.

Yes, but, after 3 months, there is no idea, proposition, arguments :-(

You can also have a variable with a list of the "special" commands. Zsh will show in green the commands in this list, if user uses "sudo".
But it is longer to create this list.

danielshahaf · 2019-03-25T18:33:08Z

With this patch, it can do a little more.

I don't mean to be rude, but I suppose I'd better say it plainly. Your code is not ready to be merged. It doesn't have docs, it doesn't have tests, it doesn't address the concern from the last paragraph of my previous comment, and it doesn't test the bitfield's value correctly. It could form the core of a solution, but it's not there yet.

Then i am not agree with you and i don't prefer to not highlighting at all.

Would you care to explain why you think that would be better?

You can also have a variable with a list of the "special" commands. Zsh will show in green the commands in this list, if user uses "sudo".

Yes, that could work. There's a possible problem here if root's path contains directories that are access(X_OK) but not access(R_OK) to the current user — that is, directories whose contents can be random-accessed but not enumerated — but I don't think that's a common setup.

danielshahaf · 2019-03-25T18:38:22Z

Yes, that could work. There's a possible problem here if root's path contains directories that are access(X_OK) but not access(R_OK) to the current user — that is, directories whose contents can be random-accessed but not enumerated — but I don't think that's a common setup.

Also, there's the complexity question. Enumerating the root's path is O(N) work where N is the number of binaries in root's path; testing existence of a single binary is O(M log N) where M is the number of directories in root's path… and that's without considering performance of network filesystems.

That is: obtaining the list of permitted commands ahead of time might not work performantly in some setups.

Raizo62 · 2019-03-25T19:11:21Z

With this patch, it can do a little more.
I don't mean to be rude, but I suppose I'd better say it plainly. Your code is not ready to be merged. It doesn't have docs, it doesn't have tests, it doesn't address the concern from the last paragraph of my previous comment, and it doesn't test the bitfield's value correctly. It could form the core of a solution, but it's not there yet.

I know. But i don't use my time to write docs or optimize code, if, already, you don't want this idea.
And I have not proposed any merge.
And this code works for a normal usage. I have tested it.

For your last paragraph, i see solution : When you will have the problem, you can save the original value of PATH before to modify it. After you can restore it before your "grep".

Then i am not agree with you and i don't prefer to not highlighting at all.
Would you care to explain why you think that would be better?

Because that will help very often (we do very often "sudo cmd"), and mislead very rarely ("sudo -u alice"). In the second case, we can remove the variable ZSH_HIGHLIGHT_SPECIAL_PATH, or change his value (local folder with files with good name)...

That is: obtaining the list of permitted commands ahead of time might not work performantly in some setups.

The second method doesn't enumerate path. It is only a list of "word". If the first parameter of sudo is in this list then zsh shows it with green.
This method can also completed this first ("ZSH_HIGHLIGHT_SPECIAL_PATH")

danielshahaf · 2019-03-25T20:39:45Z

I know. But i don't use my time to write docs or optimize code, if, already, you don't want this idea.

Sure.

And I have not proposed any merge.
And this code works for a normal usage. I have tested it.

Sure.

For your last paragraph, i see solution : When you will have the problem, you can save the original value of PATH before to modify it. After you can restore it before your "grep".

Sorry, but no. It's not good design to require hoop jumping for a common operation that usually requires none.

Because that will help very often (we do very often "sudo cmd"), and mislead very rarely ("sudo -u alice").

Are we talking about the same thing? I wasn't asking you to justify the user-settable knob. I was asking you to justify why sudo foo should highlight foo according to the default PATH when the knob is unset, as opposed to leaving the foo unhighlighted in that case. This behaviour is exactly why sudo ifup doesn't DTRT by default on Debian derivatives.

The second method doesn't enumerate path. It is only a list of "word".

Yes, I understood that. It's just that I anticipate people would set that list to /usr/local/sbin/*(:t) /usr/sbin/*(:t) /sbin/*(:t).

Raizo62 · 2019-03-26T13:10:14Z

Also, your patch would cause /usr/sbin/grep to be called in preference to /usr/bin/grep, if _zsh_highlight_main__type started calling grep for some reason. I'm not really comfortable with that: the user-specified $PATH should not affect z-sy-h's own code in any way. (Sorry, I should have noticed this sooner.)

You can also move the function which detects if a command is known in a new function. This function will do the local PATH.

I was asking you to justify why sudo foo should highlight foo according to the default PATH when the knob is unset, as opposed to leaving the foo unhighlighted in that case. This behaviour is exactly why sudo ifup doesn't DTRT by default on Debian derivatives.

(Sorry, i don't understand the word "DTRT" or found the correct spell)
But on Debian, ifconfig is not found without sudo, and works with sudo.
I think that will be a good thing if zsh helps when we use a bad spelling even when we use sudo. Zsh is do for help, doesn't it. If it can do it then why it would not do it?

danielshahaf · 2019-03-26T14:59:10Z

You can also move the function which detects if a command is known in a new function. This function will do the local PATH.

Yes, that has been suggested upthread. It could work, although thought will need to be put into the cache invalidation question (see the fifth bullet in my comment from yesterday).

(Sorry, i don't understand the word "DTRT" or found the correct spell)

It's an acronym of "do the right thing".

But on Debian, ifconfig is not found without sudo, and works with sudo.

That's neither here nor there; we aren't talking about how to highlight % ifconfig. We're talking about how to highlight % sudo ifconfig by default. You said you thought that should highlight ifconfig in red and I asked you to explain the thinking behind the design choice you proposed.

Raizo62 · 2019-03-26T18:47:12Z

That's neither here nor there; we aren't talking about how to highlight % ifconfig. We're talking about how to highlight % sudo ifconfig by default. You said you thought that should highlight ifconfig in red and I asked you to explain the thinking behind the design choice you proposed.

I wish to highlight ifconfig in green (behind sudo) because the command sudo ifconfig is correct (very often, without "rarely" case as sudo -u alice) : the shell command shows the good result.
But if I do a spelling error (sudo ficonfig), the command is uncorrect and zsh can alerte me (with red word) before the shell.
If i have no color behind sudo, i must wait the error of the shell to know that i have do an spelling error.

This behaviour is exactly why sudo ifup doesn't DTRT by default on Debian derivatives.

Why do you say that sudo ifup doesn't do the right thing ?

danielshahaf · 2019-03-26T19:09:50Z

> This behaviour is exactly why sudo ifup doesn't DTRT by default on Debian derivatives. Why do you say that `sudo ifup` doesn't do the right thing ?

Because it highlights `ifup` in red...

I wish to highlight `ifconfig` in *green* (behind `sudo`) because the command `sudo ifconfig` is correct (very often, without "rarely" case as `sudo -u alice`) : the shell command shows the good result. But if I do a spell error (`sudo ficonfig`), the command is uncorrect and zsh can alerte me (with red word) before the shell. If i have no color behind `sudo`, i must wait the error of the shell to know that i have do an spelling error.

You don't have to convince me that the green/red distinction is useful. I agree that it would be nice for 'sudo ifup' to highlight 'ifup' in green. However, I'm asking a different question: what algorithm should 'sudo foo' follow to decide what color to highlight 'foo' in when root's path is NOT configured.

Raizo62 · 2019-03-26T20:27:19Z

However, I'm asking a different question: what algorithm should 'sudo foo' follow to decide what color to highlight 'foo' in when root's path is NOT configured.

'sudo foo' must follow the same algorithm as sudo to know what color to use with 'foo'.
But zsh doesn't have the same right as the sudo command then it can't do the same thing, and you don't want tu use sudo (and you have right)

I am not the good person to answer to this question. I choose the solution which colors correctly more often foo.
With me, initialize ZSH_HIGHLIGHT_SPECIAL_PATH with root's path will be the default.
Then if an user removes the value of this variable, sudo ifconfig will be red.

Raizo62 · 2019-03-27T07:26:32Z

I can add also that unset ZSH_HIGHLIGHT_SPECIAL_PATH can is the case where the user is not in sudoers : then all commands behind sudo (even ls) will be red (This default choose will be do by the admin of the server)

danielshahaf · 2019-03-28T13:07:57Z

I can add also that unset ZSH_HIGHLIGHT_SPECIAL_PATH can is the case where the user is not in sudoers : then all commands behind sudo (even ls ) will be red

First, ls would be green when your variable is unset. (I agree that it would make sense for it to be red if the command would fail, but we can't determine that.)

Second, the common case, provided that the user has typed sudo, is that the user is in sudoers(5) and hasn't configured z-sy-h's "root's PATH" knob.

danielshahaf · 2019-03-28T13:27:21Z

You may have meant "set to empty" rather than "unset", but your code doesn't distinguish these two cases, and even if it did, I doubt zsh-syntax-highlighting is popular enough that admins will code their sudoers(5) configs into /usr/local/etc/zshrc's setting of your variable.

Raizo62 · 2019-03-28T16:38:04Z

You may have meant "set to empty" rather than "unset", but your code doesn't distinguish these two cases,

Yes, i know. I did think this idea when i was answer you. It is easy to modify the patch to do this.

I doubt zsh-syntax-highlighting is popular enough that admins will code their sudoers(5) configs into /usr/local/etc/zshrc's setting of your variable.

If an user is not in sudoers and the admin does nothing, then the command sudo ls will fail and ls will be red. It is correct.
If an user is in sudoers and the admin does nothing, then the command sudo ls will success but ls will be red. It is uncorrect. Perhaps the user can read the doc and configure the variable to have green.
If an user is in sudoers and the admin configures the variable, then the command sudo ls will success and ls will be green. It is correct.
If an user is not in sudoers and the admin configures the variable, then the command sudo ls will fail and ls will be green. It is uncorrect. Perhaps the user can read the doc and open a ticket or speak to the admin ;-)

I doubt zsh-syntax-highlighting is popular enough that admins will code their sudoers(5) configs into /usr/local/etc/zshrc's setting of your variable.

Perhaps, but if zsh-syntax-highlighting is enough popular to be installed by the "admin", the "admin" can configured it ;-)

I propose a boolean variable to color the command after sudo : if this variable is unset or true, the command behind sudo is always uncolored, otherwise zsh uses ZSH_HIGHLIGHT_SPECIAL_PATH.

danielshahaf · 2019-03-28T19:58:36Z

If an user is not in sudoers and the admin does nothing, then the command sudo ls will fail and ls will be red. It is correct.

No, it won't be red. It will be green, like I already said.

I propose a boolean variable to color the command after sudo : if this variable is unset or true, the command behind sudo is always uncolored, otherwise zsh uses ZSH_HIGHLIGHT_SPECIAL_PATH.

That's what I proposed back in December. I'm glad we agree on something :-)

Raizo62 · 2019-03-28T22:24:18Z

If an user is not in sudoers and the admin does nothing, then the command sudo ls will fail and ls will be red. It is correct.

No, it won't be red. It will be green, like I already said.

I speaked with a system with ZSH_HIGHLIGHT_SPECIAL_PATH (and with unset)

I propose a boolean variable to color the command after sudo : if this variable is unset or true, the command behind sudo is always uncolored, otherwise zsh uses ZSH_HIGHLIGHT_SPECIAL_PATH.

That's what I proposed back in December. I'm glad we agree on something :-)

I am not against if i have always a functionality to color the commands behind sudo ;-)

danielshahaf · 2019-03-29T09:35:24Z

I speaked with a system with ZSH_HIGHLIGHT_SPECIAL_PATH (and with unset)

Yes, and I already said —

Second, the common case, provided that the user has typed sudo, is that the user is in sudoers(5) and hasn't configured z-sy-h's "root's PATH" knob.

That means ls shouldn't be red by default.

I regret to say that I find this thread/discussion/collaboration challenging to participate in; moreover, I suspect the feeling's mutual. There are still unanswered design questions¹, but I fear that trying to discuss them would frustrate both of us. I'd like to avoid that outcome, if possible.

¹ For one, shouldn't the actual argument to the -u option be part of the zstyle context the path is looked up under? But this doesn't answer all open questions, not to mention that there isn't yet consensus on how much sudo should be special-cased in the first place...

Raizo62 · 2019-03-29T16:42:46Z

I believe that you want a miracle solution. Miracle solution because zsh-syntax-highlighting doesn't have access to files like sudoers(5), but zsh-syntax-highlighting must have access to this files to choose automatically the good color for foo behind sudo.

I tried to convince you to accept a partial solution where it is the user who explains to zsh-syntax-highlighting what is the good color.

I'm afraid that this functionality stays in standby for a longtime. Already 3 months without progress. I think that a miracle solution will need more more time.
It is too bad because we need it : When we tape a command, we have no interest for sudo but for foo.

At the end, i hoped that you accept to have a "beta-functionality" (beta because no perfect) disabled by default.

Ok. It is your code after all.

I will try to write a patch which applies our discussion. I will copy-paste it here.

danielshahaf · 2019-03-29T17:14:56Z

I believe that you want a miracle solution. Miracle solution because zsh-syntax-highlighting doesn't have access to files like sudoers(5), but zsh-syntax-highlighting must have access to this files to choose automatically the good color for foo behind sudo.

Then you misunderstood me. As far as I'm concerned, "z-sy-h doesn't have access to sudoers(5)" is a design constraint. What I'd like to do is to find a good solution, within the bounds of the effective constraints.

I tried to convince you to accept a partial solution where it is the user who explains to zsh-syntax-highlighting what is the good color.

That part I agree with.

At the end, i hoped that you accept to have a "beta-functionality" (beta because no perfect) disabled by default.

For avoidance of doubt, I never consider whether a fix is complete as a criterion to merging it; I only consider whether it is sound. In other words, I would happily merge a patch that implements only part of the desired behaviour. Your patch, however, fixes one use-case but regresses others.

I will try to write a patch which applies our discussion. I will copy-paste it here.

I look forward to the revised patch.

Raizo62 · 2019-03-31T09:14:30Z

Add funtion zsh_highlight_main__test_type_builtin also the change of PATH can't modify the behavior of the commands in zsh_highlight_main__type
The calls to builtin type -w -- $1 >/dev/null are replaced by call to zsh_highlight_main__test_type_builtin
If ZSH_HIGHLIGHT_SPECIAL_PATH exists but is empty, foo in sudo foo is always red
If ZSH_HIGHLIGHT_SPECIAL_PATH is not empty, zsh searchs in the PATH ZSH_HIGHLIGHT_SPECIAL_PATH to know if the command foo in sudo foo exists
If foo is in ZSH_HIGHLIGHT_SPECIAL_COMMAND then "Admin" says that user has access to this command : foo in sudo foo is green
I wanted that foo in sudo foo has no color if ZSH_HIGHLIGHT_SPECIAL_PATH is not exist, but i don't know how to do that.....

_zsh_highlight_main__test_type_builtin() {

	if [[ -n  ${ZSH_HIGHLIGHT_SPECIAL_PATH} ]]; then
		if [ "$this_word" = ":sudo_opt::start:" ]; then
			# ZSH_HIGHLIGHT_SPECIAL_PATH exist is not empty : "Admin" says that user has access to commands in these paths with sudo
			local PATH="${ZSH_HIGHLIGHT_SPECIAL_PATH}"
		fi
	fi

	builtin type -w -- $1 >/dev/null 2>&1
}

# Get the type of a command.
#
# Uses the zsh/parameter module if available to avoid forks, and a
# wrapper around 'type -w' as fallback.
#
# If $2 is 0, do not consider aliases.
#
# The result will be stored in REPLY.
_zsh_highlight_main__type() {
  integer -r aliases_allowed=${2-1}
  # We won't cache replies of anything that exists as an alias at all, to
  # ensure the cached value is correct regardless of $aliases_allowed.
  #
  # ### We probably _should_ cache them in a cache that's keyed on the value of
  # ### $aliases_allowed, on the assumption that aliases are the common case.
  integer may_cache=1

  # Cache lookup
  if (( $+_zsh_highlight_main__command_type_cache )); then
    REPLY=$_zsh_highlight_main__command_type_cache[(e)$1]
    if [[ -n "$REPLY" ]]; then
      return
    fi
  fi

	if [[ -v ZSH_HIGHLIGHT_SPECIAL_COMMAND ]]; then
		if (( ${ZSH_HIGHLIGHT_SPECIAL_COMMAND[(I)$1]} )); then
			# ZSH_HIGHLIGHT_SPECIAL_COMMAND exists : if foo of "sudo foo" is in ZSH_HIGHLIGHT_SPECIAL_COMMAND then "Admin" says that user has access to this command
			REPLY=command
			return 0
		fi
	fi

	if [[ -v ZSH_HIGHLIGHT_SPECIAL_PATH ]]; then
		if [ "$this_word" = ":sudo_opt::start:" ]; then
			if [ -z  "${ZSH_HIGHLIGHT_SPECIAL_PATH}" ]; then
				# ZSH_HIGHLIGHT_SPECIAL_PATH exist and is empty : "Admin" wants to say that user has no access to sudo : foo in "sudo foo" is red
				REPLY=none
				return 0
			fi
		fi
	fi

  # Main logic
  if (( $#options_to_set )); then
    setopt localoptions $options_to_set;
  fi
  unset REPLY
  if zmodload -e zsh/parameter; then
    if (( $+aliases[(e)$1] )); then
      may_cache=0
    fi
    if (( $+aliases[(e)$1] )) && (( aliases_allowed )); then
      REPLY=alias
    elif [[ $1 == *.* && -n ${1%.*} ]] && (( $+saliases[(e)${1##*.}] )); then
      REPLY='suffix alias'
    elif (( $reswords[(Ie)$1] )); then
      REPLY=reserved
    elif (( $+functions[(e)$1] )); then
      REPLY=function
    elif (( $+builtins[(e)$1] )); then
      REPLY=builtin
    elif (( $+commands[(e)$1] )); then
      REPLY=command
    # zsh 5.2 and older have a bug whereby running 'type -w ./sudo' implicitly
    # runs 'hash ./sudo=/usr/local/bin/./sudo' (assuming /usr/local/bin/sudo
    # exists and is in $PATH).  Avoid triggering the bug, at the expense of
    # falling through to the $() below, incurring a fork.  (Issue #354.)
    #
    # The first disjunct mimics the isrelative() C call from the zsh bug.
    elif {  [[ $1 != */* ]] || is-at-least 5.3 } &&
         ! _zsh_highlight_main__test_type_builtin; then
      REPLY=none
    fi
  fi
  if ! (( $+REPLY )); then
    # Note that 'type -w' will run 'rehash' implicitly.
    #
    # We 'unalias' in a subshell, so the parent shell is not affected.
    #
    # The colon command is there just to avoid a command substitution that
    # starts with an arithmetic expression [«((…))» as the first thing inside
    # «$(…)»], which is area that has had some parsing bugs before 5.6
    # (approximately).
    REPLY="${$(:; (( aliases_allowed )) || unalias -- $1 2>/dev/null; LC_ALL=C _zsh_highlight_main__test_type_builtin)##*: }"
    if [[ $REPLY == 'alias' ]]; then
      may_cache=0
    fi
  fi

  # Cache population
  if (( may_cache )) && (( $+_zsh_highlight_main__command_type_cache )); then
    _zsh_highlight_main__command_type_cache[(e)$1]=$REPLY
  fi

  [[ -n $REPLY ]]
  return $?
}

danielshahaf · 2019-03-31T13:30:18Z

- Add funtion `zsh_highlight_main__test_type_builtin` also the change of PATH can't modify the behavior of the commands in `zsh_highlight_main__type`

Thanks for the patch! Please send the same code as a pull request so it's easier to review.

- I wanted that foo in `sudo foo` has normal color if `ZSH_HIGHLIGHT_SPECIAL_PATH` is not exist, but i don't know how to do that.....

Good question. Right now, it gets highlighted as unknown-token because of line 840 in master, right? How about adding a new style, "indeterminate"? _zsh_highlight_main__type would return some new value, the loop would map that value to the style [indeterminate] (rather than [unknown-token], and we can make that style have no special highlighting by default. (@phy1729 what do you think?)

``` _zsh_highlight_main__test_type_builtin() { } _zsh_highlight_main__type() { }

So, to summarize, your overall approach is: - When falling back to the 'type' builtin, run it with a custom PATH; - Add two new knobs, ZSH_HIGHLIGHT_SPECIAL_PATH and ZSH_HIGHLIGHT_SPECIAL_COMMAND. Is that an accurate summary? Thanks again for the patch; I'll have to sleep on it.

Raizo62 · 2019-03-31T18:24:53Z

Please send the same code as a pull request so it's easier to review.

It is done.

Is that an accurate summary?

Yes it is.
ZSH_HIGHLIGHT_SPECIAL_PATH and ZSH_HIGHLIGHT_SPECIAL_COMMAND are not dependent on each other. These are 2 different approaches. ZSH_HIGHLIGHT_SPECIAL_PATH is more generic, and ZSH_HIGHLIGHT_SPECIAL_COMMAND is more precise

In the description of the pull request, i have put examples to use them.

phy1729 · 2019-04-02T03:51:42Z

How about adding a new style, "indeterminate"? _zsh_highlight_main__type would return some new value, the loop would map that value to the style [indeterminate] (rather than [unknown-token], and we can make that style have no special highlighting by default. (@phy1729 what do you think?)

I'm all for distinguishing the different meanings of unknown-token. Couple of questions,

When would indeterminate be used for precommands?
- Just when we might otherwise use unknown-token (i.e. not found in the user's PATH).
- Pedantically one could make the case for always (sudo or another precommand could restrict the allowed commands to a subset of those in the user's PATH.)
Would indeterminate also apply to a special parameter or command substitution in a command position? (Should continue this in another thread.)

I don't see it brought up before (apologies if I missed it), but the current approach would mishighlight other precomamnds (e.g. command ifconfig). This is partially an existing bug (since z-sy-h doesn't keep track of precommands), but the interface should allow for the user or admin to specify to which precommands the special path applies.

danielshahaf · 2019-04-02T09:05:37Z

This is partially an existing bug (since z-sy-h doesn't keep track of precommands), but the interface should allow for the user or admin to specify to which precommands the special path applies.

That's a textbook case for zstyle's, isn't it? (in order to simulate an associative array having plain arrays for values)

I wonder what to do with multiple precommands (e.g., builtin command doas -u alice sudo -u bob ssh foo ls). I suppose we can make them available in the style context (:…:builtin+command+…:…), or in a parameter for zstyle -e to access.

sudo and commands only in /sbin #592

sudo and commands only in /sbin #592

Comments

Raizo62 commented Dec 18, 2018

danielshahaf commented Dec 18, 2018 via email

Raizo62 commented Dec 18, 2018

danielshahaf commented Dec 18, 2018 via email

Raizo62 commented Dec 18, 2018

danielshahaf commented Dec 19, 2018

Raizo62 commented Dec 19, 2018

danielshahaf commented Dec 19, 2018

Raizo62 commented Dec 19, 2018

danielshahaf commented Dec 19, 2018

Raizo62 commented Dec 19, 2018

danielshahaf commented Dec 19, 2018

danielshahaf commented Dec 19, 2018

Raizo62 commented Dec 19, 2018 • edited Loading

danielshahaf commented Dec 19, 2018

danielshahaf commented Dec 19, 2018

danielshahaf commented Dec 19, 2018

nicoulaj commented Dec 20, 2018

danielshahaf commented Dec 20, 2018 via email

Raizo62 commented Dec 20, 2018

danielshahaf commented Dec 20, 2018 via email

Raizo62 commented Dec 20, 2018

phy1729 commented Dec 20, 2018

danielshahaf commented Dec 20, 2018 via email

Raizo62 commented Dec 21, 2018

Raizo62 commented Dec 22, 2018

danielshahaf commented Dec 22, 2018

danielshahaf commented Dec 22, 2018

Raizo62 commented Dec 24, 2018

danielshahaf commented Dec 24, 2018

Raizo62 commented Dec 24, 2018

Raizo62 commented Mar 25, 2019

danielshahaf commented Mar 25, 2019

Raizo62 commented Mar 25, 2019

danielshahaf commented Mar 25, 2019

danielshahaf commented Mar 25, 2019

Raizo62 commented Mar 25, 2019 • edited Loading

danielshahaf commented Mar 25, 2019

Raizo62 commented Mar 26, 2019

danielshahaf commented Mar 26, 2019

Raizo62 commented Mar 26, 2019 • edited Loading

danielshahaf commented Mar 26, 2019 via email

Raizo62 commented Mar 26, 2019

Raizo62 commented Mar 27, 2019 • edited Loading

danielshahaf commented Mar 28, 2019

danielshahaf commented Mar 28, 2019

Raizo62 commented Mar 28, 2019

danielshahaf commented Mar 28, 2019

Raizo62 commented Mar 28, 2019 • edited Loading

danielshahaf commented Mar 29, 2019

Raizo62 commented Mar 29, 2019

danielshahaf commented Mar 29, 2019

Raizo62 commented Mar 31, 2019 • edited Loading

danielshahaf commented Mar 31, 2019 via email

Raizo62 commented Mar 31, 2019

phy1729 commented Apr 2, 2019

danielshahaf commented Apr 2, 2019

danielshahaf commented Mar 15, 2020

danielshahaf commented Mar 16, 2020

danielshahaf commented Mar 16, 2020

Raizo62 commented Dec 19, 2018 •

edited

Loading

Raizo62 commented Mar 25, 2019 •

edited

Loading

Raizo62 commented Mar 26, 2019 •

edited

Loading

Raizo62 commented Mar 27, 2019 •

edited

Loading

Raizo62 commented Mar 28, 2019 •

edited

Loading

Raizo62 commented Mar 31, 2019 •

edited

Loading