diff --git a/backend/sftp/sftp.go b/backend/sftp/sftp.go index e62dbd220..6d639ee6f 100644 --- a/backend/sftp/sftp.go +++ b/backend/sftp/sftp.go @@ -2470,9 +2470,16 @@ func quoteOrEscapeShellPath(shellType string, shellPath string) (string, error) return "'" + powerShellQuoteEscaper.Replace(shellPath) + "'", nil } // Windows Command Prompt + // + // cmd has no reliable command-line escaping for the double quote used + // as the delimiter, while % expands environment variables and ! may + // expand them when delayed expansion is enabled, even inside double + // quotes. A newline or carriage return ends the command. None of these + // can be neutralised safely, so reject any path containing them rather + // than risk altering the command. if shellType == "cmd" { - if strings.Contains(shellPath, "\"") { - return "", fmt.Errorf("path is not valid in shell type %s: %s", shellType, shellPath) + if strings.ContainsAny(shellPath, "\"%!\r\n") { + return "", fmt.Errorf("path is not valid in shell type %s: %q", shellType, shellPath) } return "\"" + shellPath + "\"", nil } diff --git a/backend/sftp/sftp_internal_test.go b/backend/sftp/sftp_internal_test.go index 322c38eef..9dd8ae360 100644 --- a/backend/sftp/sftp_internal_test.go +++ b/backend/sftp/sftp_internal_test.go @@ -36,6 +36,10 @@ func TestShellEscapeUnix(t *testing.T) { {"$(rm -rf /)", "\\$\\(rm\\ -rf\\ /\\)"}, {"/test/\n", "/test/'\n'"}, {":\"'", ":\\\"\\'"}, + // a backslash must be escaped so it cannot neutralise the escape + // of the metacharacter that follows it + {"a\\;id", "a\\\\\\;id"}, + {"`id`", "\\`id\\`"}, } { got, err := quoteOrEscapeShellPath("unix", test.unescaped) assert.NoError(t, err) @@ -50,8 +54,15 @@ func TestShellEscapeCmd(t *testing.T) { }{ {"", "\"\"", true}, {"c:/this/is/harmless", "\"c:/this/is/harmless\"", true}, + // & < > | ^ are not special inside cmd double quotes so are allowed {"c:/test¬epad", "\"c:/test¬epad\"", true}, {"c:/test\"&\"notepad", "", false}, + // % and ! expand environment variables even inside double quotes + {"c:/test%PATH%notepad", "", false}, + {"c:/test!PATH!notepad", "", false}, + // a newline or carriage return ends the command + {"c:/test\nnotepad", "", false}, + {"c:/test\rnotepad", "", false}, } { got, err := quoteOrEscapeShellPath("cmd", test.unescaped) if test.ok {