New function __fish_preview_current_file to open current file in pager

[exploide]

Description

New function __fish_preview_current_file to open current file in pager. Bound to Alt+O by shared key bindings.

Fixes issue #6838

TODOs:

• Changes to fish usage are reflected in user documentation/manpages.
• Tests have been added for regressions fixed
• User-visible changes noted in CHANGELOG.md

[krobelus]

This is really useful! Let's add a changelog entry and docs under Shared bindings in index.rst, so users will notice.

[exploide]

I incorporated most of your suggestions. Please have a second look.

However, I encountered another problem, so I would not merge this now:

This use of eval, at least the second and probably also the third use, poses a funny security vulnerability. Imagine you want to examine files originating from someone else. If the person prepared a file with the name hello; touch pwned and you do somethink like >_ your-command 'hello; touch pwned' <Alt+O>, then the eval executes that.
It's a hilarious attack vector, but still it is one 😄

Some existing functions or completions might have the same problem.

Can this easily be solved by using commandline and eval in a different way? Or do we need another way of expanding arguments? @faho said on Gitter it would need a new shell builtin when not using eval for expansion.

The problem with (eval echo (commandline -oc)[-1]) is that after accessing index [-1], the result is no longer escaped.

[krobelus]

I think your solution works fine for most use cases, but it is surprisingly tricky to fix the issues.
I tried to do that with below example that also supports multiple files.

The problem with (eval echo (commandline -oc)[-1]) is that after
accessing index [-1], the result is no longer escaped.

Yeah I just realized that. commandline -o does tokenize, but not
expand the likes of $, ~, {} and []. So passing it to eval is
wrong because eval tokenizes again.
My fix is to escape all characters that are relevant for tokenization, but
not the ones that will be expanded later. We should have better support
for use cases like this. Here we could either

1. Have a variant of eval that does only expansion, not tokenization.
   This is RFC: expand and safer alternatives to eval #5811.

2. Add a new flag to commandline -o to make it return an escaped string,
   that can be passed to eval safely. This is similar to my manual escaping.

This use of eval, at least the second and probably also the third use,
poses a funny security vulnerability.

Yeah, the third eval evaluates the result of a former evaluation. We
should not do that, since that's not what fish would do when executing
the command. To fix that I do the replacement of a "-option=" prefix
before evaluating. This will fail to work for files that literally start
like that but I don't think we care.

The second eval suffers from the double-tokenization issue; that's
hopefully fixed by escaping.

When the cursor is on something that does not tokenize (for example
unclosed quotes "asdf, pressing Control+O prints the tokenizer
error message. I think that's better than silencing the error.
This message comes from eval, which fails, so I don't run less, or repaint if that happens.

I got rid of some variable quoting, instead I explicitly check set -q foo[1]. Of course that's just a matter of style.

---

function __fish_preview_current_file --description "Open the file at the cursor in a pager"
    set -l pager less --
    set -q PAGER && echo $PAGER | read -at pager

    # commandline -t will never return an empty list. However, the token
    # could comprise multiple lines, so join them into a single string.
    set -l file (commandline -t | string collect)
    # count $file is 1
    # $backslash will parsed as regex which may need additional escaping.
    set -l backslash '\\\\'
    not status test-feature regex-easyesc && set backslash $backslash$backslash
    test -z $file && set file (string replace -ra -- '([ ;#^<>&|()"\'])' "$backslash\$1" (commandline -oc)[-1])
    # count $file is 0 or 1

    set -q file[1] || return
    # Now file contains exactly one element, the unexpanded token.

    # strip -option= from token if necessary
    set file (string replace -r -- '^-[^\s=]*=(.*)' '$1' $file)

    true # Clear $status from string replace, we don't want to return because of it.
    eval set -l files $file || return # Return when eval fails.
    
    if set -q files[1] && test -f $files[1]
        $pager $files
        commandline -f repaint
    end
end

[exploide]

Thanks for continuing this. I will test that tomorrow and come back.

And thanks for linking the issue related to an expand builtin. I think that would be the way to go. Good that @faho already has something like that. The problem with evaluating harmful file names is already present for example in the completions of unzip.

[exploide]

Seems to work so far, thanks. I pushed that version now. It's ready from my side. At least until we have a safe alternative to eval.

When happens the following? When a filename contains line breaks?

However, the token could comprise multiple lines, so join them into a single string

[krobelus]

When happens the following? When a filename contains line breaks?

Exactly, you can create a file like touch a\nb.
Opening such files with line breaks works if the cursor is directly on the token (though only after I added another string collect). If the cursor is past the token, then (commandline -o)[-1] is used, so this tries to open only the last line in the last token.

Merged to master as 8025e80, thank you!