fix(cli/mcp): prevent command injection via $EDITOR/$VISUAL#5288
Merged
Conversation
mcpEditConfigLaunchCommand concatenated the raw $VISUAL/$EDITOR value into a `sh -lc "<editor> <path>"` command string. Only the path was shell-quoted, so an editor value like `vim; rm -rf ~` (or any environment pollution) would execute arbitrary shell commands. Replace the sh -lc construction with editorLaunchCmd, which expands $VAR references and leading ~/~ prefixes without a shell, splits the result into argv via strings.Fields, and invokes the binary directly with exec.Command. This matches the safe pattern already used by the terminal-editor fallback (exec.Command(bin, path)) in the same function. Multi-token values such as `code --wait` are still honored. Add tests covering: direct invocation (no shell), argument splitting, $VAR expansion, tilde expansion, rejection of shell metacharacters, and proof that tilde expansion cannot be abused by an injection payload.
Parse EDITOR/VISUAL values into argv with shell-style quoting and backslash escaping while still avoiding shell execution. This preserves common editor commands such as emacsclient -a '' and quoted editor paths without reintroducing command injection risk. Co-authored-by: SivanCola <32437197+SivanCola@users.noreply.github.com>
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mcpEditConfigLaunchCommandconcatenated the raw$VISUAL/$EDITORvalueinto a
sh -lc "<editor> <path>"command string. Only the path wasshell-quoted, so an editor value like
vim; rm -rf ~(or any environmentpollution) would execute arbitrary shell commands.
Severity
Command-injection hardening for local environment-controlled editor values.
Exploitation requires the environment variable to be set to a malicious value;
this is not remote-input execution, but it is still a real local attack surface
for shared accounts, compromised tooling, or untrusted config sources. Removing
the shell interpretation layer makes the MCP config editor launch path safer.
Fix
Replace the
sh -lcconstruction witheditorLaunchCmd:$VARreferences viaos.ExpandEnv(no shell)for word splitting only (supports
code --wait,emacsclient -a '', andquoted editor paths)
~/~/viaos.UserHomeDir(no shell)exec.Command(args[0], append(args[1:], path)...)-- binary resolved by OSShell metacharacters become literal argv tokens and cannot be executed. This
matches the safe pattern already used by the terminal-editor fallback
(
exec.Command(bin, path)) in the same function.Shell operators, globbing, command substitution, and redirection are
intentionally not evaluated. The now-unused
shellQuotehelper is removed.Behavior preservation
sh -lc)exec)vimcode --waitemacsclient -c -a ''$HOME/bin/xExpandEnvexpands~/bin/x'/Applications/Visual Studio Code.app/Contents/Resources/app/bin/code' --waitvim; rm -rf ~rmTests
TestMCPEditConfigLaunchUsesVisualBeforeEditor-- direct invocationTestMCPEditConfigLaunchEditorWithArgs--code --waitarg splittingTestMCPEditConfigLaunchEditorParsesShellStyleQuotes-- shell-style quotes,empty quoted args, escaped whitespace, and literal backslashes
TestMCPEditConfigLaunchEditorRejectsUnterminatedQuote-- invalid quotededitor values fail cleanly
TestMCPEditConfigLaunchEditorRejectsShellMetachars-- injection defenseTestMCPEditConfigLaunchEditorExpandsEnvVar--$VARexpansionTestMCPEditConfigLaunchEditorExpandsTilde--~/~/expansionTestMCPEditConfigLaunchEditorTildeNotInPayload-- tilde not abusedTestMCPEditConfigLaunchFallsBackToTerminalEditor-- fallback unchangedVerification
go test ./internal/cli -run 'TestMCPEditConfigLaunch'-- passgo test ./internal/cli-- passgit diff --check-- passCache-impact
None. This only changes local CLI process construction for the MCP config editor
launch path.
Authorship note
@YoungDan-hero is the primary author of this PR. @SivanCola contributed a
maintainer follow-up that preserves shell-style quoted editor values while
keeping the no-shell launch path.