lmorg / murex

A smarter shell and scripting environment with advanced features designed for usability, safety and productivity (eg smarter DevOps tooling)
https://murex.rocks
GNU General Public License v2.0
1.5k stars 27 forks source link

Hint Text Expands Tilde Character Unnecessarily #663

Closed tiymat closed 1 year ago

tiymat commented 1 year ago

Describe the bug: I initially encountered this problem when trying to run git reset HEAD~ and noticed that it failed because the ~ character was being expanded to the value of my home directory.

This led me to the murex documentation, which said that tildes would not be expanded within single quotes. So I tried git reset 'HEAD~' and saw that, although the tilde character appears to get passed to git correctly and the command runs as expected, the hint text still expands the tilde.

Then again, the hint text starts with "(example only)" so it could be that the hint text isn't meant to be doing that much work and I'm just misunderstanding its purpose.

Expected behaviour: Ideally, the hint text would check whether tilde characters are within single quotes prior to expanding them. If that's not in scope for the status line then maybe the documentation could be updated to explain the meaning of the (example only) hint text.

Screenshots: Tilde expansion

Platform (please complete the following information):

Additional context Given that the command runs as expected once the tilde is between ' characters this isn't some huge problem and I was on the fence about making an issue at all but thought I should for documentation if nothing else.

Also, at first I was a bit surprised by tildes outside of single quotes always being expanded. When you run e.g. echo 123~456 I believe bash, fish, and zsh all output 123~456 while out 123~456 in murex outputs 123/ when I run it.

I guess the positive tradeoff is it makes determining what a command containing a ~ will do simpler? If I remember correctly, bash decides whether or not to expand a tilde based on whether it immediately follows a : or the first =, among other things, so when composing a command in bash I guess there's more logic you have to keep in your mind.

lmorg commented 1 year ago

Given that the command runs as expected once the tilde is between ' characters this isn't some huge problem and I was on the fence about making an issue at all but thought I should for documentation if nothing else.

I always value feedback :)


Also, at first I was a bit surprised by tildes outside of single quotes always being expanded. When you run e.g. echo 123~456 I believe bash, fish, and zsh all output 123~456 while out 123~456 in murex outputs 123/ when I run it.

This is definitely a bug albeit an intention one. Intentional because:

Your point about (example only) is absolutely right. But it always felt like a weak solution.

I guess the positive tradeoff is it makes determining what a command containing a ~ will do simpler? If I remember correctly, bash decides whether or not to expand a tilde based on whether it immediately follows a : or the first =, among other things, so when composing a command in bash I guess there's more logic you have to keep in your mind.

I did consider whether to add some extra logic around how ~ might get expanded but ended up against it. Partly for simplicity but also partly because I like the explicitness of having ~ inside single quotes even if it seems jarring at first when coming from Bash. After all, can I accurately assume to understand every instance when someone does or does not want ~ expanded?


That's the background. Now lets look at how I might resolve this bug...

I do agree that (example only) should be documented. That's an easy fix. However I do also think the code could be improved as well. While one of the two original concerns was that there are edge cases where a parse of the command line would lead to incorrect results (eg the source example above), the current solution is wrong more often than a parser would be. So the simplified solution isn't working either. I've also completely rewritten Murex's parsers since the hint text code was written, so it's much easier (and quicker) now for me to generate an example. I think even if code like bg { out $example } didn't expand in the hint text whereas out "$example" did, that would still be a massive step up from where we are at today.

tiymat commented 1 year ago

After reading the context you gave in your response I'd agree that the (example only) documentation would be the first step, and maybe all that's required. Had there been something explaining that in the documentation I might have had a better understanding of the status text's role and probably not thought there was a bug at all.

lmorg commented 1 year ago

@tiymat I've been thinking about this a lot and I'm very tempted to remove this feature altogether. The reasons being:

  1. updating it to accurately parse the command line is going to be harder than expected (simply put, i can either expand variables AND subshells, or neither variables and subshells. I can't do one without the other -- at least not easily. The problem is unexpectedly expanding subshells is a dangerous idea because they could contain code that modify the system state)
  2. The current status quo, with the (example only) feels very amateurish. This was fine for one of the earlier, more beta, versions of the shell but I feel Murex has moved passed that point now
  3. It's not uncommon for variables to hold secrets. Thus displaying variables could be argued as a security risk due to "shoulder surfers" (passing secrets in the command line has its own security problems due to those secrets being visible in ps but we cannot prevent how people manage secrets but we can prevent those secrets from being unexpected printed to screen)

Point 1 is more due to my own laziness, I'd rather not fix something that's already working just to make a niche feature work. Point 2 would be solved automatically by solving point 1. And arguably point 3 is a little paranoid and could be solved with an enable/disable option in config and maybe blacklisting variables with names like TOKEN. However I'm not convinced this feature, even if it were fixed, would be valuable enough to warrant the effort to get it there.

What's your thoughts?

tiymat commented 1 year ago
  1. I definitely don't think of the hint text as one of Murex's flagship features, and while it can be nice to have in a few specific cases, it seems from what you said it would be an expensive feature (time-wise) to keep developing.

  2. I agree the (example only) feels awkward, and it definitely confused me when I was first starting out with Murex.

  3. Personally I'm less sold on the security concern -- to me the likelihood of a scenario where an attacker is close enough for long enough to see a secret, in a context that makes it apparent that it is a secret and the information needed to exploit the secret, without the Murex user noticing in time to hit e.g. Ctrl+C to remove the hint text seems remote. And if it were to occur, and the attacker had that level of visibility of the screen for that amount of time, I can't help but suspect that they would have many other opportunities to see compromising information, unrelated to the hint text. I agree that in theory the hint text does pose a miniscule risk, but I personally don't think it's big enough to be relevant.

My initial thought was to make it configurable like the F1 preview box, so you could hit some key combination to bring it up for the current command(s) you have in the prompt.

If it does become something that requires action on the user's part to invoke, rather than just being visible at all times, then if it isn't very reliable (in the sense of being guaranteed to be able to 'predict' the commands' output) I could imagine users wouldn't go to the trouble of using it very often.

Personally I don't look at it frequently, and wouldn't notice its absence much, but I can only speak for myself.

lmorg commented 1 year ago

Just to be clear, I definitely want to keep the hint text -- as a general feature -- around. I think it's context aware tips are generally useful and I like the way how it disappears after readline has been closed so your terminal emulators scrollback doesn't get cluttered with meta-information (like what happens with similar extensions to Bash et al).

I just think specifically the $VARIABLE hints are poorly built.

I'm going to remove that $VARIABLE hints. :)

As an aside, you can disable the hint text completely if you don't like it:

config set shell hint-text-enabled false