Fish tries to substitute (execute) a subshell command during completion (tab) even when surrounded with double quotes #10194

aureq · 2024-01-06T04:52:43Z

What happened?

I have a command that I have a shell completion for (grafana-manager) and for which the shell completion is generated by github.com/spf13/cobra (Golang).

If I run my command as show below, then it work as expected (no error message) and the program receives the the --dashboard-name value as written on the shell prompt.

./grafana-manager public-dashboards delete --organization-id 3 --dashboard-name "k8s (public)"

However, if I attempt to add further complete my command by pressing tab, fish appears to try to execute the command public. This results in fish showing errors on the terminal and the completion to fail.

fish: Unknown command: public
in command substitution
        called on line 1 of file -
in command substitution
        called on line 25 of file -
in function '__grafana_manager_perform_completion'
        called on line 1 of file -
in command substitution
        called on line 68 of file -
in function '__grafana_manager_prepare_completions'
in command substitution
- (line 1): Unknown command
GRAFANA_MANAGER_ACTIVE_HELP=0 ./grafana-manager __complete public-dashboards delete --organization-id 3 --dashboard-name k8s (public) --
                                                                                                                             ^~~~~~~^
in command substitution
        called on line 25 of file -
in function '__grafana_manager_perform_completion'
        called on line 1 of file -
in command substitution
        called on line 68 of file -
in function '__grafana_manager_prepare_completions'
in command substitution

Output of `fish --version`

3.7.0

Additional context

This should quite dangerous situation if the provided value between the parenthesis was to do something nasty.

If I escape the parenthesis, then the shell completion works as expected ✅ however, running the command fails ❌ because the (escaped) value supplied to the program is not as expected.

./grafana-manager public-dashboards delete --organization-id 3 --dashboard-name "k8s \(public\)" --[tab]

The text was updated successfully, but these errors were encountered:

zanchey · 2024-01-06T05:35:37Z

Can you post the output of the completions file that cobra is generating?

aureq · 2024-01-06T05:41:50Z

So, I'm using github.com/spf13/cobra v1.5.0 (upgrade to 1.8.0 is planned)

I usually inject the completion via ./grafana-manager completion fish | source.

And below is the actual completion.

# fish completion for grafana-manager                      -*- shell-script -*-

function __grafana_manager_debug
    set -l file "$BASH_COMP_DEBUG_FILE"
    if test -n "$file"
        echo "$argv" >> $file
    end
end

function __grafana_manager_perform_completion
    __grafana_manager_debug "Starting __grafana_manager_perform_completion"

    # Extract all args except the last one
    set -l args (commandline -opc)
    # Extract the last arg and escape it in case it is a space
    set -l lastArg (string escape -- (commandline -ct))

    __grafana_manager_debug "args: $args"
    __grafana_manager_debug "last arg: $lastArg"

    # Disable ActiveHelp which is not supported for fish shell
    set -l requestComp "GRAFANA_MANAGER_ACTIVE_HELP=0 $args[1] __complete $args[2..-1] $lastArg"

    __grafana_manager_debug "Calling $requestComp"
    set -l results (eval $requestComp 2> /dev/null)

    # Some programs may output extra empty lines after the directive.
    # Let's ignore them or else it will break completion.
    # Ref: https://github.com/spf13/cobra/issues/1279
    for line in $results[-1..1]
        if test (string trim -- $line) = ""
            # Found an empty line, remove it
            set results $results[1..-2]
        else
            # Found non-empty line, we have our proper output
            break
        end
    end

    set -l comps $results[1..-2]
    set -l directiveLine $results[-1]

    # For Fish, when completing a flag with an = (e.g., <program> -n=<TAB>)
    # completions must be prefixed with the flag
    set -l flagPrefix (string match -r -- '-.*=' "$lastArg")

    __grafana_manager_debug "Comps: $comps"
    __grafana_manager_debug "DirectiveLine: $directiveLine"
    __grafana_manager_debug "flagPrefix: $flagPrefix"

    for comp in $comps
        printf "%s%s\n" "$flagPrefix" "$comp"
    end

    printf "%s\n" "$directiveLine"
end

# This function does two things:
# - Obtain the completions and store them in the global __grafana_manager_comp_results
# - Return false if file completion should be performed
function __grafana_manager_prepare_completions
    __grafana_manager_debug ""
    __grafana_manager_debug "========= starting completion logic =========="

    # Start fresh
    set --erase __grafana_manager_comp_results

    set -l results (__grafana_manager_perform_completion)
    __grafana_manager_debug "Completion results: $results"

    if test -z "$results"
        __grafana_manager_debug "No completion, probably due to a failure"
        # Might as well do file completion, in case it helps
        return 1
    end

    set -l directive (string sub --start 2 $results[-1])
    set --global __grafana_manager_comp_results $results[1..-2]

    __grafana_manager_debug "Completions are: $__grafana_manager_comp_results"
    __grafana_manager_debug "Directive is: $directive"

    set -l shellCompDirectiveError 1
    set -l shellCompDirectiveNoSpace 2
    set -l shellCompDirectiveNoFileComp 4
    set -l shellCompDirectiveFilterFileExt 8
    set -l shellCompDirectiveFilterDirs 16

    if test -z "$directive"
        set directive 0
    end

    set -l compErr (math (math --scale 0 $directive / $shellCompDirectiveError) % 2)
    if test $compErr -eq 1
        __grafana_manager_debug "Received error directive: aborting."
        # Might as well do file completion, in case it helps
        return 1
    end

    set -l filefilter (math (math --scale 0 $directive / $shellCompDirectiveFilterFileExt) % 2)
    set -l dirfilter (math (math --scale 0 $directive / $shellCompDirectiveFilterDirs) % 2)
    if test $filefilter -eq 1; or test $dirfilter -eq 1
        __grafana_manager_debug "File extension filtering or directory filtering not supported"
        # Do full file completion instead
        return 1
    end

    set -l nospace (math (math --scale 0 $directive / $shellCompDirectiveNoSpace) % 2)
    set -l nofiles (math (math --scale 0 $directive / $shellCompDirectiveNoFileComp) % 2)

    __grafana_manager_debug "nospace: $nospace, nofiles: $nofiles"

    # If we want to prevent a space, or if file completion is NOT disabled,
    # we need to count the number of valid completions.
    # To do so, we will filter on prefix as the completions we have received
    # may not already be filtered so as to allow fish to match on different
    # criteria than the prefix.
    if test $nospace -ne 0; or test $nofiles -eq 0
        set -l prefix (commandline -t | string escape --style=regex)
        __grafana_manager_debug "prefix: $prefix"

        set -l completions (string match -r -- "^$prefix.*" $__grafana_manager_comp_results)
        set --global __grafana_manager_comp_results $completions
        __grafana_manager_debug "Filtered completions are: $__grafana_manager_comp_results"

        # Important not to quote the variable for count to work
        set -l numComps (count $__grafana_manager_comp_results)
        __grafana_manager_debug "numComps: $numComps"

        if test $numComps -eq 1; and test $nospace -ne 0
            # We must first split on \t to get rid of the descriptions to be
            # able to check what the actual completion will be.
            # We don't need descriptions anyway since there is only a single
            # real completion which the shell will expand immediately.
            set -l split (string split --max 1 \t $__grafana_manager_comp_results[1])

            # Fish won't add a space if the completion ends with any
            # of the following characters: @=/:.,
            set -l lastChar (string sub -s -1 -- $split)
            if not string match -r -q "[@=/:.,]" -- "$lastChar"
                # In other cases, to support the "nospace" directive we trick the shell
                # by outputting an extra, longer completion.
                __grafana_manager_debug "Adding second completion to perform nospace directive"
                set --global __grafana_manager_comp_results $split[1] $split[1].
                __grafana_manager_debug "Completions are now: $__grafana_manager_comp_results"
            end
        end

        if test $numComps -eq 0; and test $nofiles -eq 0
            # To be consistent with bash and zsh, we only trigger file
            # completion when there are no other completions
            __grafana_manager_debug "Requesting file completion"
            return 1
        end
    end

    return 0
end

# Since Fish completions are only loaded once the user triggers them, we trigger them ourselves
# so we can properly delete any completions provided by another script.
# Only do this if the program can be found, or else fish may print some errors; besides,
# the existing completions will only be loaded if the program can be found.
if type -q "grafana-manager"
    # The space after the program name is essential to trigger completion for the program
    # and not completion of the program name itself.
    # Also, we use '> /dev/null 2>&1' since '&>' is not supported in older versions of fish.
    complete --do-complete "grafana-manager " > /dev/null 2>&1
end

# Remove any pre-existing completions for the program since we will be handling all of them.
complete -c grafana-manager -e

# The call to __grafana_manager_prepare_completions will setup __grafana_manager_comp_results
# which provides the program's completion choices.
complete -c grafana-manager -n '__grafana_manager_prepare_completions' -f -a '$__grafana_manager_comp_results'

aureq · 2024-01-06T05:50:23Z

I just upgraded to github.com/spf13/cobra v1.8.0 and I get a similar error message (same but duplicated).

fish: Unknown command: public
in command substitution
        called on line 1 of file -
in command substitution
        called on line 25 of file -
in function '__grafana_manager_perform_completion'
        called on line 1 of file -
in command substitution
        called on line 67 of file -
in function '__grafana_manager_perform_completion_once'
        called on line 89 of file -
in function '__grafana_manager_requires_order_preservation'
in command substitution
- (line 1): Unknown command
GRAFANA_MANAGER_ACTIVE_HELP=0 ./grafana-manager __complete public-dashboards delete --organization-id 3 --dashboard-name k8s (public) --
                                                                                                                             ^~~~~~~^
in command substitution
        called on line 25 of file -
in function '__grafana_manager_perform_completion'
        called on line 1 of file -
in command substitution
        called on line 67 of file -
in function '__grafana_manager_perform_completion_once'
        called on line 89 of file -
in function '__grafana_manager_requires_order_preservation'
in command substitution
fish: Unknown command: public
in command substitution
        called on line 1 of file -
in command substitution
        called on line 25 of file -
in function '__grafana_manager_perform_completion'
        called on line 1 of file -
in command substitution
        called on line 67 of file -
in function '__grafana_manager_perform_completion_once'
        called on line 89 of file -
in function '__grafana_manager_requires_order_preservation'
in command substitution
- (line 1): Unknown command
GRAFANA_MANAGER_ACTIVE_HELP=0 ./grafana-manager __complete public-dashboards delete --organization-id 3 --dashboard-name k8s (public) --
                                                                                                                             ^~~~~~~^
in command substitution
        called on line 25 of file -
in function '__grafana_manager_perform_completion'
        called on line 1 of file -
in command substitution
        called on line 67 of file -
in function '__grafana_manager_perform_completion_once'
        called on line 89 of file -
in function '__grafana_manager_requires_order_preservation'
in command substitution
fish: Unknown command: public
in command substitution
        called on line 1 of file -
in command substitution
        called on line 25 of file -
in function '__grafana_manager_perform_completion'
        called on line 1 of file -
in command substitution
        called on line 67 of file -
in function '__grafana_manager_perform_completion_once'
        called on line 122 of file -
in function '__grafana_manager_prepare_completions'
in command substitution
- (line 1): Unknown command
GRAFANA_MANAGER_ACTIVE_HELP=0 ./grafana-manager __complete public-dashboards delete --organization-id 3 --dashboard-name k8s (public) --
                                                                                                                             ^~~~~~~^
in command substitution
        called on line 25 of file -
in function '__grafana_manager_perform_completion'
        called on line 1 of file -
in command substitution
        called on line 67 of file -
in function '__grafana_manager_perform_completion_once'
        called on line 122 of file -
in function '__grafana_manager_prepare_completions'
in command substitution

The shell completion is as follows

# fish completion for grafana-manager                      -*- shell-script -*-

function __grafana_manager_debug
    set -l file "$BASH_COMP_DEBUG_FILE"
    if test -n "$file"
        echo "$argv" >> $file
    end
end

function __grafana_manager_perform_completion
    __grafana_manager_debug "Starting __grafana_manager_perform_completion"

    # Extract all args except the last one
    set -l args (commandline -opc)
    # Extract the last arg and escape it in case it is a space
    set -l lastArg (string escape -- (commandline -ct))

    __grafana_manager_debug "args: $args"
    __grafana_manager_debug "last arg: $lastArg"

    # Disable ActiveHelp which is not supported for fish shell
    set -l requestComp "GRAFANA_MANAGER_ACTIVE_HELP=0 $args[1] __complete $args[2..-1] $lastArg"

    __grafana_manager_debug "Calling $requestComp"
    set -l results (eval $requestComp 2> /dev/null)

    # Some programs may output extra empty lines after the directive.
    # Let's ignore them or else it will break completion.
    # Ref: https://github.com/spf13/cobra/issues/1279
    for line in $results[-1..1]
        if test (string trim -- $line) = ""
            # Found an empty line, remove it
            set results $results[1..-2]
        else
            # Found non-empty line, we have our proper output
            break
        end
    end

    set -l comps $results[1..-2]
    set -l directiveLine $results[-1]

    # For Fish, when completing a flag with an = (e.g., <program> -n=<TAB>)
    # completions must be prefixed with the flag
    set -l flagPrefix (string match -r -- '-.*=' "$lastArg")

    __grafana_manager_debug "Comps: $comps"
    __grafana_manager_debug "DirectiveLine: $directiveLine"
    __grafana_manager_debug "flagPrefix: $flagPrefix"

    for comp in $comps
        printf "%s%s\n" "$flagPrefix" "$comp"
    end

    printf "%s\n" "$directiveLine"
end

# this function limits calls to __grafana_manager_perform_completion, by caching the result behind $__grafana_manager_perform_completion_once_result
function __grafana_manager_perform_completion_once
    __grafana_manager_debug "Starting __grafana_manager_perform_completion_once"

    if test -n "$__grafana_manager_perform_completion_once_result"
        __grafana_manager_debug "Seems like a valid result already exists, skipping __grafana_manager_perform_completion"
        return 0
    end

    set --global __grafana_manager_perform_completion_once_result (__grafana_manager_perform_completion)
    if test -z "$__grafana_manager_perform_completion_once_result"
        __grafana_manager_debug "No completions, probably due to a failure"
        return 1
    end

    __grafana_manager_debug "Performed completions and set __grafana_manager_perform_completion_once_result"
    return 0
end

# this function is used to clear the $__grafana_manager_perform_completion_once_result variable after completions are run
function __grafana_manager_clear_perform_completion_once_result
    __grafana_manager_debug ""
    __grafana_manager_debug "========= clearing previously set __grafana_manager_perform_completion_once_result variable =========="
    set --erase __grafana_manager_perform_completion_once_result
    __grafana_manager_debug "Successfully erased the variable __grafana_manager_perform_completion_once_result"
end

function __grafana_manager_requires_order_preservation
    __grafana_manager_debug ""
    __grafana_manager_debug "========= checking if order preservation is required =========="

    __grafana_manager_perform_completion_once
    if test -z "$__grafana_manager_perform_completion_once_result"
        __grafana_manager_debug "Error determining if order preservation is required"
        return 1
    end

    set -l directive (string sub --start 2 $__grafana_manager_perform_completion_once_result[-1])
    __grafana_manager_debug "Directive is: $directive"

    set -l shellCompDirectiveKeepOrder 32
    set -l keeporder (math (math --scale 0 $directive / $shellCompDirectiveKeepOrder) % 2)
    __grafana_manager_debug "Keeporder is: $keeporder"

    if test $keeporder -ne 0
        __grafana_manager_debug "This does require order preservation"
        return 0
    end

    __grafana_manager_debug "This doesn't require order preservation"
    return 1
end


# This function does two things:
# - Obtain the completions and store them in the global __grafana_manager_comp_results
# - Return false if file completion should be performed
function __grafana_manager_prepare_completions
    __grafana_manager_debug ""
    __grafana_manager_debug "========= starting completion logic =========="

    # Start fresh
    set --erase __grafana_manager_comp_results

    __grafana_manager_perform_completion_once
    __grafana_manager_debug "Completion results: $__grafana_manager_perform_completion_once_result"

    if test -z "$__grafana_manager_perform_completion_once_result"
        __grafana_manager_debug "No completion, probably due to a failure"
        # Might as well do file completion, in case it helps
        return 1
    end

    set -l directive (string sub --start 2 $__grafana_manager_perform_completion_once_result[-1])
    set --global __grafana_manager_comp_results $__grafana_manager_perform_completion_once_result[1..-2]

    __grafana_manager_debug "Completions are: $__grafana_manager_comp_results"
    __grafana_manager_debug "Directive is: $directive"

    set -l shellCompDirectiveError 1
    set -l shellCompDirectiveNoSpace 2
    set -l shellCompDirectiveNoFileComp 4
    set -l shellCompDirectiveFilterFileExt 8
    set -l shellCompDirectiveFilterDirs 16

    if test -z "$directive"
        set directive 0
    end

    set -l compErr (math (math --scale 0 $directive / $shellCompDirectiveError) % 2)
    if test $compErr -eq 1
        __grafana_manager_debug "Received error directive: aborting."
        # Might as well do file completion, in case it helps
        return 1
    end

    set -l filefilter (math (math --scale 0 $directive / $shellCompDirectiveFilterFileExt) % 2)
    set -l dirfilter (math (math --scale 0 $directive / $shellCompDirectiveFilterDirs) % 2)
    if test $filefilter -eq 1; or test $dirfilter -eq 1
        __grafana_manager_debug "File extension filtering or directory filtering not supported"
        # Do full file completion instead
        return 1
    end

    set -l nospace (math (math --scale 0 $directive / $shellCompDirectiveNoSpace) % 2)
    set -l nofiles (math (math --scale 0 $directive / $shellCompDirectiveNoFileComp) % 2)

    __grafana_manager_debug "nospace: $nospace, nofiles: $nofiles"

    # If we want to prevent a space, or if file completion is NOT disabled,
    # we need to count the number of valid completions.
    # To do so, we will filter on prefix as the completions we have received
    # may not already be filtered so as to allow fish to match on different
    # criteria than the prefix.
    if test $nospace -ne 0; or test $nofiles -eq 0
        set -l prefix (commandline -t | string escape --style=regex)
        __grafana_manager_debug "prefix: $prefix"

        set -l completions (string match -r -- "^$prefix.*" $__grafana_manager_comp_results)
        set --global __grafana_manager_comp_results $completions
        __grafana_manager_debug "Filtered completions are: $__grafana_manager_comp_results"

        # Important not to quote the variable for count to work
        set -l numComps (count $__grafana_manager_comp_results)
        __grafana_manager_debug "numComps: $numComps"

        if test $numComps -eq 1; and test $nospace -ne 0
            # We must first split on \t to get rid of the descriptions to be
            # able to check what the actual completion will be.
            # We don't need descriptions anyway since there is only a single
            # real completion which the shell will expand immediately.
            set -l split (string split --max 1 \t $__grafana_manager_comp_results[1])

            # Fish won't add a space if the completion ends with any
            # of the following characters: @=/:.,
            set -l lastChar (string sub -s -1 -- $split)
            if not string match -r -q "[@=/:.,]" -- "$lastChar"
                # In other cases, to support the "nospace" directive we trick the shell
                # by outputting an extra, longer completion.
                __grafana_manager_debug "Adding second completion to perform nospace directive"
                set --global __grafana_manager_comp_results $split[1] $split[1].
                __grafana_manager_debug "Completions are now: $__grafana_manager_comp_results"
            end
        end

        if test $numComps -eq 0; and test $nofiles -eq 0
            # To be consistent with bash and zsh, we only trigger file
            # completion when there are no other completions
            __grafana_manager_debug "Requesting file completion"
            return 1
        end
    end

    return 0
end

# Since Fish completions are only loaded once the user triggers them, we trigger them ourselves
# so we can properly delete any completions provided by another script.
# Only do this if the program can be found, or else fish may print some errors; besides,
# the existing completions will only be loaded if the program can be found.
if type -q "grafana-manager"
    # The space after the program name is essential to trigger completion for the program
    # and not completion of the program name itself.
    # Also, we use '> /dev/null 2>&1' since '&>' is not supported in older versions of fish.
    complete --do-complete "grafana-manager " > /dev/null 2>&1
end

# Remove any pre-existing completions for the program since we will be handling all of them.
complete -c grafana-manager -e

# this will get called after the two calls below and clear the $__grafana_manager_perform_completion_once_result global
complete -c grafana-manager -n '__grafana_manager_clear_perform_completion_once_result'
# The call to __grafana_manager_prepare_completions will setup __grafana_manager_comp_results
# which provides the program's completion choices.
# If this doesn't require order preservation, we don't use the -k flag
complete -c grafana-manager -n 'not __grafana_manager_requires_order_preservation && __grafana_manager_prepare_completions' -f -a '$__grafana_manager_comp_results'
# otherwise we use the -k flag
complete -k -c grafana-manager -n '__grafana_manager_requires_order_preservation && __grafana_manager_prepare_completions' -f -a '$__grafana_manager_comp_results'

faho · 2024-01-06T08:21:55Z

Isn't the issue simply:

set -l results (eval $requestComp 2> /dev/null)

This runs eval. eval will evaluate tokens, including running command substitutions.

faho · 2024-01-06T08:27:16Z

Yup, that's exactly it. Your failing command is

GRAFANA_MANAGER_ACTIVE_HELP=0 ./grafana-manager __complete public-dashboards delete --organization-id 3 --dashboard-name k8s (public) --

which lines up with

set -l requestComp "GRAFANA_MANAGER_ACTIVE_HELP=0 $args[1] __complete $args[2..-1] $lastArg"

which is then evald. That means this is an issue in cobra upstream, not fish.

aureq · 2024-01-06T09:23:09Z

@faho Thanks, do you mind explaining what's happening here please? And ideally, what should be explained to upstream?

And maybe if you have an example on how this should be fixed that be great.

faho · 2024-01-06T09:46:07Z

Thanks, do you mind explaining what's happening here please?

They are running eval on an argument that is taken from your commandline. eval will interpret a string as fish code again, perform any expansions and run any commands, including command substitutions (what you refer to as a "subshell" - I prefer not to use that term because it's a specific and different thing in posixy shells).

Basically they are doing the equivalent of this:

set -l token (commandline --current-token)
eval "echo $token"

This will get the current token (e.g. (public)), and end up running eval "echo (public)", which will eval echo (public), which will try to run public.

That is what eval does, so if you want to not have command substitutions run, you need to either not use eval or use string escape on the parts you don't want run. And that would have to be a change to cobra.

We capture the commandline tokens using fish's "commandline --tokenize" (-o). That function turns echo 'some argument $(123)' into two arguments while removing the quotes echo some argument $(123) Later we pass "some argument $(123)" without quotes to the shell's "eval". This is wrong and causes spurious evaluation of the parenthesis as command substitution. Fix this by escaping the arguments. The downside of this change is that things like "$HOME" or "~" will no longer be escaped. Changing this requires changes in fish, which I'm working on. Reproduce the issue by pasting the completion script at fish-shell/fish-shell#10194 (comment) to a file "grafana-manager.fish" and running function grafana-manager; end source grafana-manager.fish Then type (without pressing Enter) grafana-manager public-dashboards delete --organization-id 3 --dashboard-name "k8s (public)" <TAB> Fixes fish-shell/fish-shell#10194

aureq · 2024-01-06T09:48:13Z

ok, That makes sense. Thanks a lot for your explanation.

Issue fish-shell#10194 reports Cobra completions do set -l args (commandline -opc) eval $args[1] __complete $args[2..] (commandline -ct | string escape) The intent behind "eval" is to expand variables and tildes in "$args". Fair enough. Several of our own completions do the same, see below. The problem with "commandline -o" + "eval" is that the former removes quotes that are significant for "eval". This becomes a problem if $args contains quoted () or {}, for example this command will wrongly execute a command substituion: git --work-tree='(launch-missiles)' <TAB> It is possible to escape the string the tokens before running eval, but then there will be no expansion of variables etc. The problem is that "commandline -o" only unescapes tokens so they end up in a weird state in-between source and expanded version. Remove the need for "eval" by making "commandline -o" expand things like variables and braces I don't think there is any use case where not expanding is better. This is a mild breaking change: if a variable expansion contains spaces or other shell metacharacters, "eval" will do the wrong thing. I expect the risk to be minor. There are some third-party uses of "eval (commandline -opc)". Besides Cobra that's at least [pip](pypa/pip#9727). The new expansion skips command substituions, in the hope that this matches user expectations better.

…eval Issue fish-shell#10194 reports Cobra completions do set -l args (commandline -opc) eval $args[1] __complete $args[2..] (commandline -ct | string escape) The intent behind "eval" is to expand variables and tildes in "$args". Fair enough. Several of our own completions do the same, see below. The problem with "commandline -o" + "eval" is that the former removes quotes that are significant for "eval". This becomes a problem if $args contains quoted () or {}, for example this command will wrongly execute a command substituion: git --work-tree='(launch-missiles)' <TAB> It is possible to escape the string the tokens before running eval, but then there will be no expansion of variables etc. The problem is that "commandline -o" only unescapes tokens so they end up in a weird state in-between source and expanded version. Remove the need for "eval" by making "commandline -o" expand things like variables and braces I don't think there is any use case where not expanding is better. This is a mild breaking change: if a variable expansion contains spaces or other shell metacharacters, "eval" will do the wrong thing. I expect the risk to be minor. There are some third-party uses of "eval (commandline -opc)". Besides Cobra that's at least [pip](pypa/pip#9727). The new expansion skips command substituions, in the hope that this matches user expectations better.

…eval Issue fish-shell#10194 reports Cobra completions do set -l args (commandline -opc) eval $args[1] __complete $args[2..] (commandline -ct | string escape) The intent behind "eval" is to expand variables and tildes in "$args". Fair enough. Several of our own completions do the same, see below. The problem with "commandline -o" + "eval" is that the former already removes quotes that are significant for "eval". This becomes a problem if $args contains quoted () or {}, for example this command will wrongly execute a command substituion: git --work-tree='(launch-missiles)' <TAB> It is possible to escape the string the tokens before running eval, but then there will be no expansion of variables etc. The problem is that "commandline -o" only unescapes tokens so they end up in a weird state in-between source and expanded version. Remove the need for "eval" by making "commandline -o" expand things like variables and braces I don't think there is any use case where not expanding is better. This enables custom completion scripts to be aware of shell variables without eval, see the added test for completions to "make -C $var/some/dir ". Some completion scripts use eval without escaping the tokens. This is already broken in some ways but this changes adds more cases of breakage : when a variable expansion contains spaces or other shell metacharacters, "eval" will do the wrong thing. There are some third-party uses of "eval (commandline -opc)". Besides Cobra that's at least [pip](pypa/pip#9727). The new expansion skips command substitutions, in the hope that this matches user expectation better than running them. They are passed through as-is (instead of cancelling or expanding to nothing) to make custom completion scripts work well in the common case.

Issue fish-shell#10194 reports Cobra completions do set -l args (commandline -opc) eval $args[1] __complete $args[2..] (commandline -ct | string escape) The intent behind "eval" is to expand variables and tildes in "$args". Fair enough. Several of our own completions do the same, see the next commit. The problem with "commandline -o" + "eval" is that the former already removes quotes that are relevant for "eval". This becomes a problem if $args contains quoted () or {}, for example this command will wrongly execute a command substituion: git --work-tree='(launch-missiles)' <TAB> It is possible to escape the string the tokens before running eval, but then there will be no expansion of variables etc. The problem is that "commandline -o" only unescapes tokens so they end up in a weird state somewhere in-between what the user typed and the expanded version. Remove the need for "eval" by introducing "commandline -x" which expands things like variables and braces. This enables custom completion scripts to be aware of shell variables without eval, see the added test for completions to "make -C $var/some/dir ". This means that essentially all third party scripts should migrate from "commandline -o" to "commandline -x". For example set -l tokens if commandline -x >/dev/null 2>&1 set tokens (commandline -xpc) else set tokens (commandline -opc) end Since this is mainly used for completions, the expansion skips command substitutions. They are passed through as-is (instead of cancelling or expanding to nothing) to make custom completion scripts work reasonably well in the common case. Of course there are cases where we would want to expand command substitutions here, so I'm not sure.

We capture the commandline tokens using fish's "commandline --tokenize" (-o). Given a commandline of some-cobra-command 'some argument $(123)' "$HOME" into three arguments while removing the quotes some-cobra-command some argument $(123) $HOME Later we pass "some argument $(123)" without quotes to the shell's "eval". This is wrong and causes spurious evaluation of the parenthesis as command substitution. Fix this by escaping the arguments. The upcoming fish 3.8 has a new flag, "commandline -x" which will expand variables like $HOME properly, see fish-shell/fish-shell#10212 Use that if available. Reproduce the issue by pasting the completion script at fish-shell/fish-shell#10194 (comment) to a file "grafana-manager.fish" and running function grafana-manager; end source grafana-manager.fish Then type (without pressing Enter) grafana-manager public-dashboards delete --organization-id 3 --dashboard-name "k8s (public)" <TAB> Fixes fish-shell/fish-shell#10194

aureq changed the title ~~Fish tries to execute a subshell command during completion (tab) even when surrounded with double quotes~~ Fish tries to substitute (execute) a subshell command during completion (tab) even when surrounded with double quotes Jan 6, 2024

krobelus self-assigned this Jan 6, 2024

faho closed this as completed Jan 6, 2024

faho added the upstream we can't fix it label Jan 6, 2024

krobelus mentioned this issue Jan 6, 2024

fish completions: fix double-evaluation of commandline spf13/cobra#2095

Open

aureq mentioned this issue Jan 6, 2024

Fish completion fails due to eval and subshell execution during completion spf13/cobra#2096

Open

krobelus mentioned this issue Jan 14, 2024

builtin commandline: --tokenize to expand strings, to avoid need for eval #10212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fish tries to substitute (execute) a subshell command during completion (tab) even when surrounded with double quotes #10194

Fish tries to substitute (execute) a subshell command during completion (tab) even when surrounded with double quotes #10194

aureq commented Jan 6, 2024 •

edited by zanchey

zanchey commented Jan 6, 2024

aureq commented Jan 6, 2024

aureq commented Jan 6, 2024 •

edited by faho

faho commented Jan 6, 2024 •

edited

faho commented Jan 6, 2024

aureq commented Jan 6, 2024 •

edited

faho commented Jan 6, 2024

aureq commented Jan 6, 2024

Fish tries to substitute (execute) a subshell command during completion (tab) even when surrounded with double quotes #10194

Fish tries to substitute (execute) a subshell command during completion (tab) even when surrounded with double quotes #10194

Comments

aureq commented Jan 6, 2024 • edited by zanchey

What happened?

Output of fish --version

Additional context

zanchey commented Jan 6, 2024

aureq commented Jan 6, 2024

aureq commented Jan 6, 2024 • edited by faho

faho commented Jan 6, 2024 • edited

faho commented Jan 6, 2024

aureq commented Jan 6, 2024 • edited

faho commented Jan 6, 2024

aureq commented Jan 6, 2024

aureq commented Jan 6, 2024 •

edited by zanchey

Output of `fish --version`

aureq commented Jan 6, 2024 •

edited by faho

faho commented Jan 6, 2024 •

edited

aureq commented Jan 6, 2024 •

edited