fix: prevent action execution during shell completion after double dash#2342
fix: prevent action execution during shell completion after double dash#2342toller892 wants to merge 2 commits into
Conversation
When a shell autocomplete script appends --generate-bash-completion to a command line that already contains "--", the completion flag was passed through to the action as a positional argument, causing the action to execute during a completion attempt. This is particularly dangerous because tab-completion could trigger side effects (e.g., file creation, network calls) without the user pressing Enter. Changes: - checkShellCompleteFlag now strips --generate-bash-completion from arguments even when "--" is present, and returns a new boolean indicating whether the completion flag was detected - RunContext returns early (without executing the action) when the completion flag was detected but shell completion is disabled - Updated tests to verify the new behavior Fixes urfave#1993
dearchap
left a comment
There was a problem hiding this comment.
The fix looks correct. The key insight is that stripping --generate-bash-completion from args before the -- loop, combined with the early return in RunContext, prevents the flag from leaking into the action.
One observation: the stripped slice is constructed unconditionally even in the fast path where lastArg != "--generate-bash-completion". This is fine since that path returns early at line 443, but the allocation on line 447-448 is unnecessary for that case. Not a real concern though.
The test coverage is thorough — verifies both the new return value and the stripped args for the -- case.
dearchap
left a comment
There was a problem hiding this comment.
The fix looks correct. The key change — stripping --generate-bash-completion before iterating for --, then returning early when completionDetected && !shellComplete — cleanly prevents action execution during completion attempts after --.
Description
When a shell autocomplete script appends
--generate-bash-completionto a command line that already contains--, the completion flag is passed through to the action as a positional argument, causing the action to execute during a completion attempt.This is particularly dangerous because tab-completion could trigger side effects (e.g., file creation, network calls) without the user pressing Enter.
Fixes #1993
Root Cause
PR #1938 added a check in
checkShellCompleteFlagto disable shell completion when--is present (correct behavior — after--, only positional args should be accepted). However, when this check fires, the function returns the unmodified arguments including--generate-bash-completion. The app then runs normally with--generate-bash-completionas a positional argument, executing the action.Changes
help.go—checkShellCompleteFlag--generate-bash-completionfrom arguments when it's detected (regardless of--presence), so it never leaks into the action's positional argumentscompletionDetected boolto indicate whether the completion flag was present(false, true, stripped)when--is present (completion disabled, but flag was detected and stripped)(true, true, stripped)in the normal case (completion enabled, flag stripped)app.go—RunContextnil) whencompletionDetected && !shellComplete— the completion flag was present but shell completion is disabled (due to--). This prevents the action from executing during a completion attempt.help_test.goTest_checkShellCompleteFlagto verify the newcompletionDetectedreturn value["--", "foo", "--generate-bash-completion"]to["--", "foo"]Behavior Change
app --<TAB>--generate-bash-completionas argapp -- foo <TAB>--generate-bash-completionas argapp foo <TAB>app <TAB>