Fish completion fails due to eval and subshell execution during completion

[aureq]

What happened?

I have a program (grafana-manager) that uses cobra 1.8.0 for command line handling and generating the necessary shell completion for fish-shell.

If I run my command as show below, then it work as expected (no error message) and the program receives 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 arguments to my command by pressing tab, the completion fails with an error (see below).
It appears the generation completion for fish tries to substitute/execute the command public. This results in fish showing errors on the terminal and the completion to fail.

I opened an issue (fish-shell/fish-shell#10194) with the fish-shell team but they recommended to follow upstream. They also provided additional information.

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

Details

• Fish-shell 3.7.0
• Cobra 1.8.0 (1.5.0 is also affected)

Generated 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 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'

Additional context

This feels like a 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 by the program.

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

Questions

• Is this something that could be altered directly within Cobra through programmatic means?
• Is there any guide to avoid/mitigate such risks?
• Or is this something that may require a patch?

[aureq]

Seems related to #2095

Thanks @krobelus