Fix: Pager variables are not evaluated

[sandr01d]

Check list

• I have performed a self-review of my code
• I have commented my code in hard-to-understand areas
• I have made corresponding changes to the documentation

Description

Our pager variables, e.g. $_forgit_blame_pager used to be in deferred code blocks that were evaluated with eval. Since we've removed the usage of eval in #326, variables in the pagers no longer get expanded, as is the case with #380.
In this specific case, evaluation is what we want and deferred code is unavoidable if we do not want to cut down on existing functionality, since the pagers are defined outside forgit (either in the git config or in an environment variable). The most straight forward approach to solving this would probably be to introduce eval everywhere we use the pager variables, e.g.

- "$FORGIT" exec_diff "${commits[@]}" -U"$diff_context" -- | $_forgit_diff_pager
+ "$FORGIT" exec_diff "${commits[@]}" -U"$diff_context" -- | eval "$_forgit_diff_pager"

However, this solution is ugly in my eyes and I'm concerned it reintroduces the anti-pattern of eval/deferred code that we got rid of in #326.
So here is an attempt to solve this issue while keeping the amount of deferred code and usage of eval as low as possible.

The code here is not final by any means and not tested thoroughly. This is a draft to start a discussion on how we want to solve this issue. Would highly appreciate your thoughts on this @wfxr, @cjappl & @carlfriedrich.

Type of change

• Bug fix
• New feature
• Refactor
• Breaking change
• Documentation change

Test environment

• Shell
  • bash
  • zsh
  • fish
• OS
  • Linux
  • Mac OS X
  • Windows
  • Others:

[cjappl]

Remind me again what the initial goal of striking eval from the program was?

I think this approach is fine, especially if we can say that we didn't "give up" that initial goal by re-introducing it here. Code seems good and encapsulated, and I'm generally for this change 👍 (interested to hear what others think)

[sandr01d]

@cjappl

Remind me again what the initial goal of striking eval from the program was?

eval takes deferred code as a string and interprets it as a commandline while applying all the nasty word splitting, globbing etc. that tends to introduce all kinds of bugs. While doing so, eval effectively removes the outer quotes, leaving variables unquoted when you're not careful. Working around this lead us to introduce all the weird "double escaping", like we did in #323. To make things worse, the deferred code snippets do not get checked by shellcheck, making it even harder to find issues. eval "blindly" executes any string you pass it - combine that with untrusted input (in our case that could be a repository with a specially crafted commit message) and you gain a huge attack surface for code injection.
To sum things up, our effort wasn't about eval specifically but about deferred code in general - eval is just the most common way to execute deferred code.

[wfxr]

@sandr01d Thank you for your recent series of outstanding contributions to forgit.

I see your points, and agree with most of them. My only concern is, for a terminal utils like forgit, do we need to pursue extreme security to the point of giving up the simplicity and efficiency of the code? eval is present in every corner of fzf, including environment variables, various --bind parameters, etc. we will inevitably use eval as long as we are using fzf. I think one of the main reasons why fzf has been so successful is also because of the flexibility brought by the eval mechanism. So maybe we don't need to consider security more important than fzf since security depends on the weakest link in the system. But I'm still 100% in favor of avoiding using eval as long as there are good alternative implementations.

[sandr01d]

@wfxr

My only concern is, for a terminal utils like forgit, do we need to pursue extreme security to the point of giving up the simplicity and efficiency of the code?

My comment regarding security was meant as one of the reasons why we've removed eval in #326 and not related to this PR. I do not think the changes we've made in #326 have made the code more complex or less efficient. In fact, I personally find it easier to work with the code base since we've made these changes. Does your concern regard the changes in #326 or the changes proposed here?
In this particular case eval is obviously unavoidable, since we need to execute code that is stored as a string outside of forgit. Since these strings are defined in the users environment (git config or environment variables), I wouldn't consider them as coming from an untrusted source, so this should be fine.
Do I understand correctly, that you would prefer not go in the direction this PR suggests? If so, what would be your preferred way to solve #318? Simply add eval before any usage of the pager variables as outlined in the description or do you have something different in mind? Would be interested in reading your thoughts on the matter.

[wfxr]

@sandr01d

Does your concern regard the changes in #326 or the changes proposed here?

No, I'm referring to the changes made in this PR. #326 looks good to me. As you said, solving this problem is difficult to bypass eval, even if we rewrite these pagers into functions in a verbose way. I personally think it is more concise to use eval in the place where the pager need to be lazy evaluated, if that's necessary.

When I talk about efficiency, I don’t just mean this part of the function, but also the loading speed of the git-forgit script. After all, we need to completely parse this script every time we use a forgit function. I'm worried that its continued expansion will make users experience more obvious delays. Of course, just this one commit will not immediately cause a problem, but we should still be aware of it.

Another point I think of is lazy evaluation should already be achieved every time a forgit command calls the git-forgit script. Do we need to lazy evaluate each pager again inside git-forgit?

[carlfriedrich]

Just adding my two cents here: I am more conservative in this case. I like the previous implementation better, since it is really easy to read. The change adds complexitiy to the code that is not needed IMO.

What about calling eval directly when assigning the _forgit_*_pager variables? Then at least we have them all in one place instead of scattered all over the file.

[sandr01d]

What about calling eval directly when assigning the forgit*_pager variables? Then at least we have them all in one place instead of scattered all over the file.

That sounds like a good approach to me @carlfriedrich, not sure exactly what it would look like though. Would you be open to create a PR for this? In this case we could close this one.

[wfxr]

Another option is moving all the pager logic into another script named like forgit-pager:

declare -A PAGERS

PAGERS["core"]=${FORGIT_PAGER:-$(git config core.pager || echo cat)}
PAGERS["show"]=${FORGIT_SHOW_PAGER:-$(git config pager.show || echo "${PAGERS[core]}")}
PAGERS["diff"]=${FORGIT_DIFF_PAGER:-$(git config pager.diff || echo "${PAGERS[core]}")}
PAGERS["blame"]=${FORGIT_BLAME_PAGER:-$(git config pager.blame || echo "${PAGERS[core]}")}
PAGERS["ignore"]=${FORGIT_IGNORE_PAGER:-$(hash bat &>/dev/null && echo 'bat -l gitignore --color=always' || echo cat)}
PAGERS["enter"]=${FORGIT_ENTER_PAGER:-"LESS='-r' less"}

pager="${1:core}"

if [[ -z "${PAGERS[$pager]}" ]]; then
  echo "pager not found: $pager" >&2
  exit 1
fi

eval "${PAGERS[$pager]}"

Defining a function in git-forgit should also be able to implement the above logic, but I think using a separate script can help keep the main code lean.

Then we use forgit-pager in git-forgit like this:

++FORGIT_PAGER="${FORGIT%/*}/forgit-pager"
...
--preview_cmd="echo {} | ... | $_forgit_show_pager"
++preview_cmd="echo {} | ... | $FORGIT_PAGER show"
...

[carlfriedrich]

@wfxr I like that approach, we would even have only one single eval there. I do not see any advantage of putting that into a dedicated script, though, and would prefer a function.

[wfxr]

@wfxr I like that approach, we would even have only one single eval there. I do not see any advantage of putting that into a dedicated script, though, and would prefer a function.

@carlfriedrich Yes, it can be implemented in the existing git-forgit script. Perhaps one advantage of creating a dedicate script is that it can reduce some lines in git-forgit. But it's not that important, both ways are OK for me.

[cjappl]

I would prefer keeping this in a single script, if possible. I think it simplifies things especially with regards to our packaging and making sure everything works when installed. Not the end of the world if we split it though, just my 2cents!

…

On Sat, Apr 6, 2024 at 1:59 AM, Wenxuan ***@***.***(mailto:On Sat, Apr 6, 2024 at 1:59 AM, Wenxuan <<a href=)> wrote: > ***@***.***(https://github.com/wfxr) I like that approach, we would even have only one single eval there. I do not see any advantage of putting that into a dedicated script, though, and would prefer a function. ***@***.***(https://github.com/carlfriedrich) Yes, it can be implemented in the existing git-forgit script. Perhaps one advantage of creating a dedicate script is that it can reduce some lines in git-forgit. But it's not that important, both ways are OK for me. — Reply to this email directly, [view it on GitHub](#381 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADMDXY4MJL4PBOIXGRGKQ3TY362QNAVCNFSM6AAAAABFUFA7ICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBRGAZDENZQHE). You are receiving this because you were mentioned.Message ID: ***@***.***>

[carlfriedrich]

Great, @sandr01d can you pick @wfxr 's code from above and put it in a function?

[sandr01d]

I definitively like @wfxr's approach. Unfortunately associative arrays are only available in bash 4.0+, so the code won't work on macOS as is. I've pushed an approach that follows the same idea without using associative arrays. Also had to adjust it a bit to make it work with variables being passed along to the ignore pager. WDYT?

[wfxr]

LGTM！

[carlfriedrich]

Great solution, thanks a lot @sandr01d. Looks good to me code-wise, just a very tiny comment.

[carlfriedrich]

Can we add a local here as well please?

[sandr01d]

Sure, updated accordingly. Let me know if it works for you like this.

[carlfriedrich]

Great, thanks a lot!