From fa43c21bd7b77e9a9ad77b7fd0b89b6dba69162d Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Wed, 19 Aug 2026 11:53:17 +1000 Subject: [PATCH 01/13] fix: quote automation commands for the host shell (#72) Generated automation commands always used single quotes, which cmd.exe passes through verbatim, so the command fails to find the entity. Values are now quoted using the rules of the shell the CLI is running under, and values needing no quoting are emitted bare. The shell can be forced with `octopus config set Shell cmd`, OCTOPUS_SHELL, or --shell. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/config/list/list.go | 3 + pkg/cmd/config/set/set.go | 12 +- pkg/cmd/release/deploy/deploy_test.go | 6 +- pkg/cmd/root/root.go | 7 + pkg/config/config.go | 4 + pkg/constants/constants.go | 3 + pkg/util/flag/flag.go | 21 ++- pkg/util/flag/flag_test.go | 62 +++++++ pkg/util/shell/parent_other.go | 9 + pkg/util/shell/parent_windows.go | 29 ++++ pkg/util/shell/quote.go | 100 +++++++++++ pkg/util/shell/quote_test.go | 234 ++++++++++++++++++++++++++ pkg/util/shell/shell.go | 89 ++++++++++ pkg/util/shell/shell_test.go | 76 +++++++++ 14 files changed, 646 insertions(+), 9 deletions(-) create mode 100644 pkg/util/flag/flag_test.go create mode 100644 pkg/util/shell/parent_other.go create mode 100644 pkg/util/shell/parent_windows.go create mode 100644 pkg/util/shell/quote.go create mode 100644 pkg/util/shell/quote_test.go create mode 100644 pkg/util/shell/shell.go create mode 100644 pkg/util/shell/shell_test.go diff --git a/pkg/cmd/config/list/list.go b/pkg/cmd/config/list/list.go index 51a1d630..a58a395c 100644 --- a/pkg/cmd/config/list/list.go +++ b/pkg/cmd/config/list/list.go @@ -49,6 +49,7 @@ func listRun(cmd *cobra.Command) error { Host string `json:"host"` NoPrompt string `json:"noprompt"` OutputFormat string `json:"outputformat"` + Shell string `json:"shell"` Space string `json:"space"` } @@ -74,6 +75,8 @@ func listRun(cmd *cobra.Command) error { configData.Space = configFile.GetString(key) case strings.ToLower(constants.ConfigOutputFormat): configData.OutputFormat = configFile.GetString(key) + case strings.ToLower(constants.ConfigShell): + configData.Shell = configFile.GetString(key) default: return fmt.Errorf("the key '%s' is not a supported config option", key) } diff --git a/pkg/cmd/config/set/set.go b/pkg/cmd/config/set/set.go index e27381af..f01a12ea 100644 --- a/pkg/cmd/config/set/set.go +++ b/pkg/cmd/config/set/set.go @@ -10,6 +10,7 @@ import ( "github.com/OctopusDeploy/cli/pkg/constants" "github.com/OctopusDeploy/cli/pkg/factory" "github.com/OctopusDeploy/cli/pkg/question" + "github.com/OctopusDeploy/cli/pkg/util/shell" "github.com/spf13/cobra" "github.com/spf13/viper" ) @@ -67,13 +68,19 @@ func setRun(isPromptEnabled bool, ask question.Asker, key string, value string) key = k } key = strings.ToLower(key) - if key == strings.ToLower(constants.ConfigNoPrompt) { + switch key { + case strings.ToLower(constants.ConfigNoPrompt): boolValue, err := strconv.ParseBool(value) if err != nil { return fmt.Errorf("the provided value %s is not valid for NoPrompt, please use true of false", value) } localViper.Set(key, boolValue) - } else { + case strings.ToLower(constants.ConfigShell): + if err := shell.Validate(value); err != nil { + return err + } + localViper.Set(key, value) + default: localViper.Set(key, value) } if err := localViper.WriteConfig(); err != nil { @@ -91,6 +98,7 @@ func promptMissing(ask question.Asker, key string) (string, string, error) { constants.ConfigOutputFormat, constants.ConfigShowOctopus, constants.ConfigEditor, + constants.ConfigShell, // constants.ConfigProxyUrl, } diff --git a/pkg/cmd/release/deploy/deploy_test.go b/pkg/cmd/release/deploy/deploy_test.go index 24d8d238..a85fc45b 100644 --- a/pkg/cmd/release/deploy/deploy_test.go +++ b/pkg/cmd/release/deploy/deploy_test.go @@ -18,6 +18,7 @@ import ( "github.com/OctopusDeploy/cli/pkg/executor" "github.com/OctopusDeploy/cli/pkg/question" "github.com/OctopusDeploy/cli/pkg/surveyext" + "github.com/OctopusDeploy/cli/pkg/util/shell" "github.com/OctopusDeploy/cli/test/fixtures" "github.com/OctopusDeploy/cli/test/testutil" "github.com/OctopusDeploy/go-octopusdeploy/v2/pkg/channels" @@ -2312,6 +2313,9 @@ func TestDeployCreate_AutomationMode(t *testing.T) { // this happens outside the scope of the normal AskQuestions flow so warrants its own integration-style test func TestDeployCreate_GenerationOfAutomationCommand_MasksSensitiveVariables(t *testing.T) { + // pin the shell so the expected command doesn't depend on where the tests are run + t.Setenv(constants.EnvOctopusShell, string(shell.Bash)) + const spaceID = "Spaces-1" const fireProjectID = "Projects-22" @@ -2492,7 +2496,7 @@ func TestDeployCreate_GenerationOfAutomationCommand_MasksSensitiveVariables(t *t Deployment Targets: All included Target Tags: All included - Automation Command: octopus release deploy --space 'Default Space' --project 'Fire Project' --version '2.0' --environment 'dev' --priority 'true' --variable 'Boring Variable:BORING' --variable 'Nuclear Launch Codes:*****' --variable 'Secret Password:*****' --no-prompt + Automation Command: octopus release deploy --space 'Default Space' --project 'Fire Project' --version 2.0 --environment dev --priority true --variable 'Boring Variable:BORING' --variable 'Nuclear Launch Codes:*****' --variable 'Secret Password:*****' --no-prompt Warning: Command includes some sensitive variable values which have been replaced with placeholders. Successfully started 2 deployment(s) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 05106062..d7a44792 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -1,6 +1,9 @@ package root import ( + "fmt" + "strings" + "github.com/OctopusDeploy/cli/pkg/apiclient" accountCmd "github.com/OctopusDeploy/cli/pkg/cmd/account" apiCmd "github.com/OctopusDeploy/cli/pkg/cmd/api" @@ -27,6 +30,7 @@ import ( "github.com/OctopusDeploy/cli/pkg/constants" "github.com/OctopusDeploy/cli/pkg/factory" "github.com/OctopusDeploy/cli/pkg/question" + "github.com/OctopusDeploy/cli/pkg/util/shell" "github.com/spf13/cobra" "github.com/spf13/viper" ) @@ -96,6 +100,8 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro cmdPFlags.BoolP(constants.FlagNoPrompt, "", false, "Disable prompting in interactive mode") + cmdPFlags.String(constants.FlagShell, "", fmt.Sprintf(`Specify the shell that generated automation commands are quoted for (%s); defaults to the shell the CLI is running under`, strings.Join(shell.Names, ", "))) + // Enable service messages flag is hidden as it's intended for internal CI/CD use only cmdPFlags.BoolP(constants.FlagEnableServiceMessages, "", false, "Enable service messages for integration with Octopus CI/CD") cmdPFlags.MarkHidden(constants.FlagEnableServiceMessages) @@ -112,6 +118,7 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro _ = viper.BindPFlag(constants.ConfigNoPrompt, cmdPFlags.Lookup(constants.FlagNoPrompt)) _ = viper.BindPFlag(constants.ConfigSpace, cmdPFlags.Lookup(constants.FlagSpace)) + _ = viper.BindPFlag(constants.ConfigShell, cmdPFlags.Lookup(constants.FlagShell)) _ = viper.BindPFlag(constants.FlagEnableServiceMessages, cmdPFlags.Lookup(constants.FlagEnableServiceMessages)) // if we attempt to check the flags before Execute is called, cobra hasn't parsed anything yet, // so we'll get bad values. PersistentPreRun is a convenient callback for setting up our diff --git a/pkg/config/config.go b/pkg/config/config.go index 7b2b7999..8fcd5338 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -30,6 +30,7 @@ func setDefaults(v *viper.Viper) { // v.SetDefault(constants.ConfigProxyUrl, "") v.SetDefault(constants.ConfigShowOctopus, true) v.SetDefault(constants.ConfigOutputFormat, "table") + v.SetDefault(constants.ConfigShell, "") if runtime.GOOS == "windows" { v.SetDefault(constants.ConfigEditor, "notepad") @@ -58,6 +59,9 @@ func bindEnvironment(v *viper.Viper) error { if err := v.BindEnv(constants.ConfigNoPrompt, constants.EnvCI); err != nil { return err } + if err := v.BindEnv(constants.ConfigShell, constants.EnvOctopusShell); err != nil { + return err + } return nil } diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 39b2ccf0..39f95e95 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -12,6 +12,7 @@ const ( FlagOutputFormatLegacy = "outputFormat" FlagNoPrompt = "no-prompt" FlagEnableServiceMessages = "enable-service-messages" + FlagShell = "shell" ) // flags for storing things in the go context @@ -38,6 +39,7 @@ const ( ConfigEditor = "Editor" ConfigShowOctopus = "ShowOctopus" ConfigOutputFormat = "OutputFormat" + ConfigShell = "Shell" ) const ( @@ -45,6 +47,7 @@ const ( EnvOctopusApiKey = "OCTOPUS_API_KEY" EnvOctopusAccessToken = "OCTOPUS_ACCESS_TOKEN" EnvOctopusSpace = "OCTOPUS_SPACE" + EnvOctopusShell = "OCTOPUS_SHELL" EnvEditor = "EDITOR" EnvVisual = "VISUAL" EnvCI = "CI" diff --git a/pkg/util/flag/flag.go b/pkg/util/flag/flag.go index 35da9467..483eb0c2 100644 --- a/pkg/util/flag/flag.go +++ b/pkg/util/flag/flag.go @@ -2,7 +2,8 @@ package flag import ( "fmt" - "strings" + + "github.com/OctopusDeploy/cli/pkg/util/shell" ) type Flag[T any] struct { @@ -60,27 +61,35 @@ func New[T any](name string, secure bool) *Flag[T] { // GenerateAutomationCmd generates the command that can be used to achive // the same results with the CLI in automation mode. func GenerateAutomationCmd(cmdPath string, space string, flags ...Generatable) string { + return GenerateAutomationCmdForShell(shell.Current(), cmdPath, space, flags...) +} + +// GenerateAutomationCmdForShell generates the automation command, quoting values using +// the rules of the given shell. +func GenerateAutomationCmdForShell(sh shell.Shell, cmdPath string, space string, flags ...Generatable) string { + quote := func(value string) string { return shell.Quote(sh, value) } + autoCmd := cmdPath if space != "" { - autoCmd += fmt.Sprintf(" --space '%s'", strings.ReplaceAll(space, "'", "'\\''")) + autoCmd += fmt.Sprintf(" --space %s", quote(space)) } for _, flag := range flags { switch value := flag.GetValue().(type) { case string: if value != "" { if flag.IsSecure() { - autoCmd += fmt.Sprintf(" --%s '***'", flag.GetName()) + autoCmd += fmt.Sprintf(" --%s %s", flag.GetName(), quote("***")) continue } - autoCmd += fmt.Sprintf(" --%s '%s'", flag.GetName(), strings.ReplaceAll(value, "'", "'\\''")) + autoCmd += fmt.Sprintf(" --%s %s", flag.GetName(), quote(value)) } case []string: for _, val := range value { if flag.IsSecure() { - autoCmd += fmt.Sprintf(" --%s '***'", flag.GetName()) + autoCmd += fmt.Sprintf(" --%s %s", flag.GetName(), quote("***")) continue } - autoCmd += fmt.Sprintf(" --%s '%s'", flag.GetName(), strings.ReplaceAll(val, "'", "'\\''")) + autoCmd += fmt.Sprintf(" --%s %s", flag.GetName(), quote(val)) } case bool: if value { diff --git a/pkg/util/flag/flag_test.go b/pkg/util/flag/flag_test.go new file mode 100644 index 00000000..05fa5aff --- /dev/null +++ b/pkg/util/flag/flag_test.go @@ -0,0 +1,62 @@ +package flag_test + +import ( + "testing" + + "github.com/OctopusDeploy/cli/pkg/util/flag" + "github.com/OctopusDeploy/cli/pkg/util/shell" + "github.com/stretchr/testify/assert" +) + +func TestGenerateAutomationCmdForShell(t *testing.T) { + project := flag.New[string]("project", false) + project.Value = "Soft Drinks" + version := flag.New[string]("version", false) + version.Value = "0.0.3" + environments := flag.New[[]string]("environment", false) + environments.Value = []string{"Dev", "Test Environment"} + tenantTag := flag.New[string]("tenant-tag", false) + tenantTag.Value = "Regions/us-east" + force := flag.New[bool]("force-package-download", false) + force.Value = true + timeout := flag.New[int]("timeout", false) + timeout.Value = 30 + password := flag.New[string]("password", true) + password.Value = "hunter2" + empty := flag.New[string]("description", false) + + flags := []flag.Generatable{project, version, environments, tenantTag, force, timeout, password, empty} + + tests := []struct { + shell shell.Shell + expected string + }{ + { + shell.Bash, + `octopus release deploy --space 'Default Space' --project 'Soft Drinks' --version 0.0.3 --environment Dev --environment 'Test Environment' --tenant-tag Regions/us-east --force-package-download --timeout 30 --password '***' --no-prompt`, + }, + { + shell.PowerShell, + `octopus release deploy --space 'Default Space' --project 'Soft Drinks' --version 0.0.3 --environment Dev --environment 'Test Environment' --tenant-tag Regions/us-east --force-package-download --timeout 30 --password '***' --no-prompt`, + }, + { + shell.Cmd, + `octopus release deploy --space "Default Space" --project "Soft Drinks" --version 0.0.3 --environment Dev --environment "Test Environment" --tenant-tag Regions/us-east --force-package-download --timeout 30 --password "***" --no-prompt`, + }, + } + + for _, test := range tests { + t.Run(string(test.shell), func(t *testing.T) { + actual := flag.GenerateAutomationCmdForShell(test.shell, "octopus release deploy", "Default Space", flags...) + assert.Equal(t, test.expected, actual) + }) + } +} + +func TestGenerateAutomationCmdForShell_NoSpace(t *testing.T) { + name := flag.New[string]("name", false) + name.Value = "Dev" + + actual := flag.GenerateAutomationCmdForShell(shell.Bash, "octopus environment create", "", name) + assert.Equal(t, "octopus environment create --name Dev --no-prompt", actual) +} diff --git a/pkg/util/shell/parent_other.go b/pkg/util/shell/parent_other.go new file mode 100644 index 00000000..01387ba0 --- /dev/null +++ b/pkg/util/shell/parent_other.go @@ -0,0 +1,9 @@ +//go:build !windows + +package shell + +// parentProcessName is only used to tell cmd and PowerShell apart, so there is +// nothing to look up on unix. +func parentProcessName() string { + return "" +} diff --git a/pkg/util/shell/parent_windows.go b/pkg/util/shell/parent_windows.go new file mode 100644 index 00000000..56fbf9ca --- /dev/null +++ b/pkg/util/shell/parent_windows.go @@ -0,0 +1,29 @@ +//go:build windows + +package shell + +import ( + "os" + "syscall" + "unsafe" +) + +// parentProcessName returns the file name of the executable that launched us. +func parentProcessName() string { + snapshot, err := syscall.CreateToolhelp32Snapshot(syscall.TH32CS_SNAPPROCESS, 0) + if err != nil { + return "" + } + defer syscall.CloseHandle(snapshot) + + entry := syscall.ProcessEntry32{} + entry.Size = uint32(unsafe.Sizeof(entry)) + ppid := uint32(os.Getppid()) + + for err = syscall.Process32First(snapshot, &entry); err == nil; err = syscall.Process32Next(snapshot, &entry) { + if entry.ProcessID == ppid { + return syscall.UTF16ToString(entry.ExeFile[:]) + } + } + return "" +} diff --git a/pkg/util/shell/quote.go b/pkg/util/shell/quote.go new file mode 100644 index 00000000..f41678be --- /dev/null +++ b/pkg/util/shell/quote.go @@ -0,0 +1,100 @@ +package shell + +import "strings" + +// Characters which carry no special meaning to the shell and so never need quoting. +// Letters and digits are always safe and aren't repeated here. +const ( + posixSafeChars = `@%+=:,./-_` + powerShellSafeChars = `+=:./\-_` + cmdSafeChars = `+=:./\-_` +) + +// Quote renders value so that sh passes it to the CLI as a single argument, unchanged. +// Values which don't need quoting are returned as they are. +func Quote(sh Shell, value string) string { + switch sh { + case PowerShell: + return quotePowerShell(value) + case Cmd: + return quoteCmd(value) + default: + return quotePosix(value) + } +} + +func isBare(value string, safeChars string) bool { + if value == "" { + return false + } + for _, r := range value { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + case strings.ContainsRune(safeChars, r): + default: + return false + } + } + return true +} + +// quotePosix quotes for sh, bash, zsh and friends. Everything inside single quotes is +// literal, so only the single quote itself needs handling; it is closed, escaped with a +// backslash, and reopened. +func quotePosix(value string) string { + if isBare(value, posixSafeChars) { + return value + } + return "'" + strings.ReplaceAll(value, "'", `'\''`) + "'" +} + +// quotePowerShell quotes for PowerShell. Single quoted strings are literal, and a single +// quote is escaped by doubling it. +func quotePowerShell(value string) string { + if isBare(value, powerShellSafeChars) { + return value + } + return "'" + strings.ReplaceAll(value, "'", "''") + "'" +} + +// quoteCmd quotes for cmd.exe, which has to survive two passes: cmd's own parsing, and +// then the argv parsing the CLI does as it starts up. +// - a literal double quote is `\"` for argv; the surrounding quotes are closed around it +// and the quote is caret escaped so cmd's own quoting stays balanced +// - backslashes only matter to argv where they run into a double quote, in which case +// the whole run has to be doubled +// - % is expanded even inside double quotes and can't be escaped there, so it is emitted +// outside them as ^% +// +// Two things can't be fixed here: a newline can't be represented in cmd at all, and ! is +// expanded when delayed expansion has been switched on. +func quoteCmd(value string) string { + if isBare(value, cmdSafeChars) { + return value + } + + var sb strings.Builder + sb.WriteByte('"') + backslashes := 0 + for i := 0; i < len(value); i++ { + switch c := value[i]; c { + case '\\': + backslashes++ + case '"': + sb.WriteString(strings.Repeat(`\`, backslashes*2)) + backslashes = 0 + sb.WriteString(`"\^""`) + case '%': + sb.WriteString(strings.Repeat(`\`, backslashes*2)) + backslashes = 0 + sb.WriteString(`"^%"`) + default: + sb.WriteString(strings.Repeat(`\`, backslashes)) + backslashes = 0 + sb.WriteByte(c) + } + } + sb.WriteString(strings.Repeat(`\`, backslashes*2)) + sb.WriteByte('"') + return sb.String() +} diff --git a/pkg/util/shell/quote_test.go b/pkg/util/shell/quote_test.go new file mode 100644 index 00000000..c8004754 --- /dev/null +++ b/pkg/util/shell/quote_test.go @@ -0,0 +1,234 @@ +package shell_test + +import ( + "fmt" + "os/exec" + "strings" + "testing" + + "github.com/OctopusDeploy/cli/pkg/util/shell" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestQuote(t *testing.T) { + tests := []struct { + name string + value string + posix string + powerShell string + cmd string + }{ + {"plain word", "Dev", "Dev", "Dev", "Dev"}, + {"version number", "0.0.3", "0.0.3", "0.0.3", "0.0.3"}, + {"tenant tag", "Regions/us-east", "Regions/us-east", "Regions/us-east", "Regions/us-east"}, + {"variable assignment", "Name:Value", "Name:Value", "Name:Value", "Name:Value"}, + {"empty", "", `''`, `''`, `""`}, + {"space", "Soft Drinks", `'Soft Drinks'`, `'Soft Drinks'`, `"Soft Drinks"`}, + {"single quote", "it's", `'it'\''s'`, `'it''s'`, `"it's"`}, + {"single quote and space", "it's here", `'it'\''s here'`, `'it''s here'`, `"it's here"`}, + {"double quote", `say "hi"`, `'say "hi"'`, `'say "hi"'`, `"say "\^""hi"\^"""`}, + {"backtick", "a`b", "'a`b'", "'a`b'", "\"a`b\""}, + {"dollar variable", "$HOME", `'$HOME'`, `'$HOME'`, `"$HOME"`}, + {"percent variable", "%PATH%", `%PATH%`, `'%PATH%'`, `""^%"PATH"^%""`}, + {"newline", "line1\nline2", "'line1\nline2'", "'line1\nline2'", "\"line1\nline2\""}, + {"comma", "a,b", "a,b", `'a,b'`, `"a,b"`}, + {"tilde", "~/tmp", `'~/tmp'`, `'~/tmp'`, `"~/tmp"`}, + {"masked secret", "*****", `'*****'`, `'*****'`, `"*****"`}, + {"ampersand", "A & B", `'A & B'`, `'A & B'`, `"A & B"`}, + {"windows path", `C:\Program Files\Octopus\`, `'C:\Program Files\Octopus\'`, `'C:\Program Files\Octopus\'`, `"C:\Program Files\Octopus\\"`}, + {"backslash then quote", `a\"b`, `'a\"b'`, `'a\"b'`, `"a\\"\^""b"`}, + {"non ascii", "Café", `'Café'`, `'Café'`, `"Café"`}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.posix, shell.Quote(shell.Bash, test.value), "bash") + assert.Equal(t, test.powerShell, shell.Quote(shell.PowerShell, test.value), "powershell") + assert.Equal(t, test.cmd, shell.Quote(shell.Cmd, test.value), "cmd") + }) + } +} + +// roundTripValues are fed through the real quoting and then back out again by the +// round trip tests below. +var roundTripValues = []string{ + "Dev", + "0.0.3", + "Regions/us-east", + "Soft Drinks", + "", + "it's", + "it's here", + `say "hi"`, + "a`b", + "$HOME", + "%PATH%", + "a,b", + "~/tmp", + "*****", + "A & B", + "a|b", + "a>b= len(line) { + return "", fmt.Errorf("trailing caret in %q", line) + } + sb.WriteByte(line[i]) + case c == '%': + return "", fmt.Errorf("unescaped %% in %q; cmd expands it even inside quotes", line) + case !inQuote && strings.ContainsRune(`&|<>()^`, rune(c)): + return "", fmt.Errorf("unquoted metacharacter %q in %q", c, line) + default: + sb.WriteByte(c) + } + } + if inQuote { + return "", fmt.Errorf("unbalanced quotes in %q", line) + } + return sb.String(), nil +} + +// parseArgv splits a windows command line into arguments the same way the go runtime +// does when it populates os.Args. The rules are documented at +// http://daviddeley.com/autohotkey/parameters/parameters.htm#WINARGV, including the +// "prior to 2008" handling of a doubled quote inside a quoted run. +func parseArgv(line string) []string { + var args []string + var current []byte + var backslashes int + started := false + inQuote := false + + appendBackslashes := func(n int) { + for ; n > 0; n-- { + current = append(current, '\\') + } + } + + for i := 0; i < len(line); i++ { + c := line[i] + switch c { + case '\\': + backslashes++ + continue + case '"': + appendBackslashes(backslashes / 2) + if backslashes%2 == 0 { + if inQuote && i+1 < len(line) && line[i+1] == '"' { + current = append(current, '"') + i++ + } + inQuote = !inQuote + } else { + current = append(current, '"') + } + backslashes = 0 + started = true + continue + case ' ', '\t': + if !inQuote { + appendBackslashes(backslashes) + backslashes = 0 + if started { + args = append(args, string(current)) + current = nil + started = false + } + continue + } + } + appendBackslashes(backslashes) + backslashes = 0 + current = append(current, c) + started = true + } + + appendBackslashes(backslashes) + if started { + args = append(args, string(current)) + } + return args +} diff --git a/pkg/util/shell/shell.go b/pkg/util/shell/shell.go new file mode 100644 index 00000000..d9f342c7 --- /dev/null +++ b/pkg/util/shell/shell.go @@ -0,0 +1,89 @@ +package shell + +import ( + "fmt" + "os" + "path/filepath" + "runtime" + "strings" + + "github.com/OctopusDeploy/cli/pkg/constants" + "github.com/spf13/viper" +) + +// Shell identifies the command line interpreter that generated automation commands +// are quoted for. Each shell has its own quoting and escaping rules. +type Shell string + +const ( + // Bash covers the POSIX shell family; sh, bash, zsh and friends all quote the same way. + Bash Shell = "bash" + PowerShell Shell = "powershell" + Cmd Shell = "cmd" +) + +// Names lists the values accepted by Parse, for help text and error messages. +var Names = []string{string(Bash), string(PowerShell), string(Cmd)} + +// Parse converts a shell name, such as the value of a config setting or the name of +// an executable, into a Shell. +func Parse(name string) (Shell, bool) { + name = strings.ToLower(strings.TrimSpace(name)) + name = strings.TrimSuffix(name, ".exe") + switch name { + case "sh", "bash", "zsh", "ksh", "dash", "ash": + return Bash, true + case "powershell", "pwsh": + return PowerShell, true + case "cmd", "command": + return Cmd, true + } + return "", false +} + +// Validate returns an error if name isn't a shell we can generate commands for. +func Validate(name string) error { + if _, ok := Parse(name); !ok { + return fmt.Errorf("the provided value %s is not a valid shell, please use one of %s", name, strings.Join(Names, ", ")) + } + return nil +} + +// Current returns the shell to generate automation commands for; the explicitly +// configured shell if there is one, otherwise the detected host shell. +func Current() Shell { + if s, ok := Parse(viper.GetString(constants.ConfigShell)); ok { + return s + } + return Detect(runtime.GOOS, os.Getenv) +} + +// Detect works out which shell the CLI is being run from. goos is a runtime.GOOS value +// and getenv looks up environment variables; both are parameters so this can be tested. +func Detect(goos string, getenv func(string) string) Shell { + // checked here as well as through viper so the override still works if the + // config system hasn't been set up, such as in tests + if s, ok := Parse(getenv(constants.EnvOctopusShell)); ok { + return s + } + + if goos == "windows" { + // the parent process is the only reliable signal on windows; PSModulePath is + // a machine wide variable that cmd.exe inherits too, so it tells us nothing. + // note this always misses when cross-compiled elsewhere, which is why it can't + // be the only check. + if s, ok := Parse(parentProcessName()); ok { + return s + } + // cmd is the safer guess: double quoted output also works in PowerShell, + // whereas single quoted output doesn't work in cmd at all. + return Cmd + } + + if sh := getenv("SHELL"); sh != "" { + if s, ok := Parse(filepath.Base(sh)); ok { + return s + } + } + return Bash +} diff --git a/pkg/util/shell/shell_test.go b/pkg/util/shell/shell_test.go new file mode 100644 index 00000000..395af9c5 --- /dev/null +++ b/pkg/util/shell/shell_test.go @@ -0,0 +1,76 @@ +package shell_test + +import ( + "runtime" + "testing" + + "github.com/OctopusDeploy/cli/pkg/util/shell" + "github.com/stretchr/testify/assert" +) + +func TestParse(t *testing.T) { + tests := []struct { + name string + expected shell.Shell + ok bool + }{ + {"bash", shell.Bash, true}, + {"BASH", shell.Bash, true}, + {" zsh ", shell.Bash, true}, + {"sh", shell.Bash, true}, + {"ksh", shell.Bash, true}, + {"dash", shell.Bash, true}, + {"powershell", shell.PowerShell, true}, + {"powershell.exe", shell.PowerShell, true}, + {"pwsh", shell.PowerShell, true}, + {"pwsh.exe", shell.PowerShell, true}, + {"cmd", shell.Cmd, true}, + {"cmd.exe", shell.Cmd, true}, + {"", "", false}, + {"fish", "", false}, + {"nushell", "", false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual, ok := shell.Parse(test.name) + assert.Equal(t, test.ok, ok) + assert.Equal(t, test.expected, actual) + }) + } +} + +func TestValidate(t *testing.T) { + assert.NoError(t, shell.Validate("cmd")) + assert.EqualError(t, shell.Validate("fish"), "the provided value fish is not a valid shell, please use one of bash, powershell, cmd") +} + +func TestDetect(t *testing.T) { + tests := []struct { + name string + goos string + env map[string]string + expected shell.Shell + usesParentProcess bool + }{ + {"unix with SHELL", "linux", map[string]string{"SHELL": "/bin/zsh"}, shell.Bash, false}, + {"unix with unknown SHELL", "linux", map[string]string{"SHELL": "/usr/bin/fish"}, shell.Bash, false}, + {"unix without SHELL", "darwin", nil, shell.Bash, false}, + {"unix with pwsh as SHELL", "linux", map[string]string{"SHELL": "/usr/local/bin/pwsh"}, shell.PowerShell, false}, + {"windows falls back to cmd", "windows", nil, shell.Cmd, true}, + {"windows ignores PSModulePath", "windows", map[string]string{"PSModulePath": `C:\Program Files\WindowsPowerShell\Modules`}, shell.Cmd, true}, + {"override wins on unix", "linux", map[string]string{"SHELL": "/bin/bash", "OCTOPUS_SHELL": "cmd"}, shell.Cmd, false}, + {"override wins on windows", "windows", map[string]string{"OCTOPUS_SHELL": "pwsh"}, shell.PowerShell, false}, + {"invalid override is ignored", "linux", map[string]string{"SHELL": "/bin/bash", "OCTOPUS_SHELL": "fish"}, shell.Bash, false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if test.usesParentProcess && runtime.GOOS == "windows" { + t.Skip("the real parent process is inspected when actually running on windows") + } + getenv := func(key string) string { return test.env[key] } + assert.Equal(t, test.expected, shell.Detect(test.goos, getenv)) + }) + } +} From 1491852177123583c91747342ad6f8b14a1e8bcb Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:07:37 +1000 Subject: [PATCH 02/13] fix: quote posix values starting with = for zsh zsh's EQUALS option, on by default, expands a word beginning with = to the path of the named command, so a bare `=foo` aborts the whole command with "foo not found" rather than passing the value through. The bash quoting covers zsh, so a leading = now forces quoting the same way ~ already does. Adds a zsh round trip test alongside the sh one; zsh is the stricter of the two so it catches this class of expansion. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/util/shell/quote.go | 4 +++- pkg/util/shell/quote_test.go | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/pkg/util/shell/quote.go b/pkg/util/shell/quote.go index f41678be..c116d527 100644 --- a/pkg/util/shell/quote.go +++ b/pkg/util/shell/quote.go @@ -42,7 +42,9 @@ func isBare(value string, safeChars string) bool { // literal, so only the single quote itself needs handling; it is closed, escaped with a // backslash, and reopened. func quotePosix(value string) string { - if isBare(value, posixSafeChars) { + // = is harmless in the middle of a word but a word which starts with one is subject + // to zsh's =cmd expansion, so `=foo` aborts the whole command with "foo not found". + if !strings.HasPrefix(value, "=") && isBare(value, posixSafeChars) { return value } return "'" + strings.ReplaceAll(value, "'", `'\''`) + "'" diff --git a/pkg/util/shell/quote_test.go b/pkg/util/shell/quote_test.go index c8004754..7303e90f 100644 --- a/pkg/util/shell/quote_test.go +++ b/pkg/util/shell/quote_test.go @@ -23,6 +23,8 @@ func TestQuote(t *testing.T) { {"version number", "0.0.3", "0.0.3", "0.0.3", "0.0.3"}, {"tenant tag", "Regions/us-east", "Regions/us-east", "Regions/us-east", "Regions/us-east"}, {"variable assignment", "Name:Value", "Name:Value", "Name:Value", "Name:Value"}, + {"embedded equals", "a=b", "a=b", "a=b", "a=b"}, + {"leading equals", "=foo", `'=foo'`, "=foo", "=foo"}, {"empty", "", `''`, `''`, `""`}, {"space", "Soft Drinks", `'Soft Drinks'`, `'Soft Drinks'`, `"Soft Drinks"`}, {"single quote", "it's", `'it'\''s'`, `'it''s'`, `"it's"`}, @@ -65,6 +67,8 @@ var roundTripValues = []string{ "$HOME", "%PATH%", "a,b", + "a=b", + "=foo", "~/tmp", "*****", "A & B", @@ -119,6 +123,25 @@ func TestQuotePosix_RoundTrip(t *testing.T) { } } +// TestQuoteZsh_RoundTrip runs the quoted values through a real zsh, which the bash +// quoting also covers. zsh expands a leading = and a leading ~ where the other posix +// shells don't, so it is the stricter test of the two. +func TestQuoteZsh_RoundTrip(t *testing.T) { + zsh, err := exec.LookPath("zsh") + if err != nil { + t.Skip("zsh is not available") + } + + for _, value := range append(roundTripValues, "line1\nline2") { + t.Run(fmt.Sprintf("%q", value), func(t *testing.T) { + script := "printf '%s' " + shell.Quote(shell.Bash, value) + out, err := exec.Command(zsh, "--no-rcs", "-c", script).Output() + require.NoError(t, err) + assert.Equal(t, value, string(out)) + }) + } +} + // TestQuotePowerShell_RoundTrip runs the quoted values through pwsh when it happens to // be installed; it isn't on CI, so this usually skips. func TestQuotePowerShell_RoundTrip(t *testing.T) { From 8a88e5fc2dc98d0e56863ec2af67a326f27295be Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:08:10 +1000 Subject: [PATCH 03/13] fix: escape the unicode single quote variants for PowerShell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PowerShell's tokenizer accepts U+2018, U+2019, U+201A and U+201B as single quotes, so any of them closes a single quoted string in the same way ' does. A value carrying a curly apostrophe, which is easy to pick up from a web UI or Word, produced 'Bob’s Project' and PowerShell rejected it as a parse error. All four are now doubled alongside the ascii quote; doubling the same character is how PowerShell escapes it, so the value round trips unchanged. bash and cmd don't treat these characters specially, so this is PowerShell only. isBare already forced quoting for them since they aren't ascii; the gap was in the escaping. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/util/shell/quote.go | 15 ++++++++++++++- pkg/util/shell/quote_test.go | 3 +++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pkg/util/shell/quote.go b/pkg/util/shell/quote.go index c116d527..5c03abf6 100644 --- a/pkg/util/shell/quote.go +++ b/pkg/util/shell/quote.go @@ -50,13 +50,26 @@ func quotePosix(value string) string { return "'" + strings.ReplaceAll(value, "'", `'\''`) + "'" } +// powerShellQuoteEscaper doubles every character PowerShell's tokenizer accepts as a +// single quote. As well as the ascii one those are the unicode "smart" variants, which +// close a single quoted string just like ' does; a value carrying a curly apostrophe +// from a web UI (Bob’s Project) would otherwise be a parse error. Doubling the same +// character is what escapes it, so the value still comes back byte for byte. +var powerShellQuoteEscaper = strings.NewReplacer( + `'`, `''`, + "‘", "‘‘", // left single quotation mark + "’", "’’", // right single quotation mark + "‚", "‚‚", // single low-9 quotation mark + "‛", "‛‛", // single high-reversed-9 quotation mark +) + // quotePowerShell quotes for PowerShell. Single quoted strings are literal, and a single // quote is escaped by doubling it. func quotePowerShell(value string) string { if isBare(value, powerShellSafeChars) { return value } - return "'" + strings.ReplaceAll(value, "'", "''") + "'" + return "'" + powerShellQuoteEscaper.Replace(value) + "'" } // quoteCmd quotes for cmd.exe, which has to survive two passes: cmd's own parsing, and diff --git a/pkg/util/shell/quote_test.go b/pkg/util/shell/quote_test.go index 7303e90f..d5a5bd89 100644 --- a/pkg/util/shell/quote_test.go +++ b/pkg/util/shell/quote_test.go @@ -41,6 +41,8 @@ func TestQuote(t *testing.T) { {"windows path", `C:\Program Files\Octopus\`, `'C:\Program Files\Octopus\'`, `'C:\Program Files\Octopus\'`, `"C:\Program Files\Octopus\\"`}, {"backslash then quote", `a\"b`, `'a\"b'`, `'a\"b'`, `"a\\"\^""b"`}, {"non ascii", "Café", `'Café'`, `'Café'`, `"Café"`}, + {"curly apostrophe", "Bob’s Project", `'Bob’s Project'`, `'Bob’’s Project'`, `"Bob’s Project"`}, + {"other smart single quotes", "a‘b‚c‛d", `'a‘b‚c‛d'`, `'a‘‘b‚‚c‛‛d'`, `"a‘b‚c‛d"`}, } for _, test := range tests { @@ -81,6 +83,7 @@ var roundTripValues = []string{ `a\"b`, `\\server\share\`, "Café", + "Bob’s Project", "trailing space ", "#comment", "a;b", From 102520c9b31475babec744d1ef96ea8a61b042ce Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:08:53 +1000 Subject: [PATCH 04/13] docs: record that % in a cmd value only survives the interactive prompt The caret doesn't escape %; percent expansion is an earlier parsing phase than caret processing. "^%" works at the prompt only because an unmatched % and an undefined %var% are left alone there. A batch file drops the first and deletes the second, so `100% Done` and `%PATH%` are both mangled when the command is pasted into a .bat or .cmd file. The batch escape is %%, which in turn doesn't collapse at the prompt, so no single encoding suits both and % joins newlines and delayed expansion in the can't-be-fixed list. The round trip test simulates the interactive prompt, which is noted on the simulator so it isn't read as evidence for batch files. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/util/shell/quote.go | 15 +++++++++++---- pkg/util/shell/quote_test.go | 5 ++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/pkg/util/shell/quote.go b/pkg/util/shell/quote.go index 5c03abf6..e94a14c0 100644 --- a/pkg/util/shell/quote.go +++ b/pkg/util/shell/quote.go @@ -78,11 +78,18 @@ func quotePowerShell(value string) string { // and the quote is caret escaped so cmd's own quoting stays balanced // - backslashes only matter to argv where they run into a double quote, in which case // the whole run has to be doubled -// - % is expanded even inside double quotes and can't be escaped there, so it is emitted -// outside them as ^% +// - % is expanded even inside double quotes, so it is emitted outside them as ^% // -// Two things can't be fixed here: a newline can't be represented in cmd at all, and ! is -// expanded when delayed expansion has been switched on. +// Three things can't be fixed here: +// - a newline can't be represented in cmd at all +// - ! is expanded when delayed expansion has been switched on +// - % only survives at the interactive prompt. The caret doesn't really escape it, +// because percent expansion is an earlier parsing phase than caret processing; what +// saves us is that the prompt leaves an unmatched % and an undefined %var% alone. +// A batch file doesn't: it drops an unmatched % and deletes an undefined %var% +// outright, so `100% Done` and `%PATH%` both come out mangled there. The batch +// escape is %%, which in turn doesn't collapse at the prompt, so no single encoding +// works for both and the interactive one is the one worth having. func quoteCmd(value string) string { if isBare(value, cmdSafeChars) { return value diff --git a/pkg/util/shell/quote_test.go b/pkg/util/shell/quote_test.go index d5a5bd89..7fea7b13 100644 --- a/pkg/util/shell/quote_test.go +++ b/pkg/util/shell/quote_test.go @@ -164,7 +164,10 @@ func TestQuotePowerShell_RoundTrip(t *testing.T) { } // simulateCmd applies cmd.exe's own processing to a command line and returns what cmd -// would hand to the program. Outside double quotes a caret escapes the next character; +// would hand to the program. It models the interactive prompt, which is where a copied +// command gets pasted; percent expansion in a batch file follows different rules and a +// value containing % doesn't survive there, as quoteCmd's comment explains. +// Outside double quotes a caret escapes the next character; // quotes themselves are passed through for the program to deal with. An unescaped % or // metacharacter outside quotes is reported as an error rather than simulated, because // either one means the generated command is broken. From e89ef0d5c955adcb681b6d2f3e1a3d1bc20cfa06 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:09:58 +1000 Subject: [PATCH 05/13] fix: reject an invalid --shell or OCTOPUS_SHELL instead of ignoring it Current falls back to detection when the configured value doesn't parse, so `--shell powershel` quietly produced detected-shell quoting while `config set Shell powershel` rejected the same value. The flag and the environment variable are now validated in the root PersistentPreRun, which becomes PersistentPreRunE so it can fail. The config file value is deliberately not validated there: a bad value hand-edited into the file would otherwise fail every command including the config commands needed to correct it. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/root/root.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index d7a44792..c7170373 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -2,6 +2,7 @@ package root import ( "fmt" + "os" "strings" "github.com/OctopusDeploy/cli/pkg/apiclient" @@ -123,7 +124,20 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro // if we attempt to check the flags before Execute is called, cobra hasn't parsed anything yet, // so we'll get bad values. PersistentPreRun is a convenient callback for setting up our // environment after parsing but before execution. - cmd.PersistentPreRun = func(_ *cobra.Command, _ []string) { + cmd.PersistentPreRunE = func(_ *cobra.Command, _ []string) error { + // an explicitly asked for shell is validated here rather than silently ignored; + // the config file value isn't, because a bad one there would lock the user out + // of the config commands they need to fix it. + if v, _ := cmdPFlags.GetString(constants.FlagShell); v != "" { + if err := shell.Validate(v); err != nil { + return fmt.Errorf("--%s: %w", constants.FlagShell, err) + } + } else if v := os.Getenv(constants.EnvOctopusShell); v != "" { + if err := shell.Validate(v); err != nil { + return fmt.Errorf("%s: %w", constants.EnvOctopusShell, err) + } + } + // map flag alias values for k, v := range flagAliases { for _, aliasName := range v { @@ -145,6 +159,8 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro if spaceNameOrId := viper.GetString(constants.ConfigSpace); spaceNameOrId != "" { clientFactory.SetSpaceNameOrId(spaceNameOrId) } + + return nil } cmd.RunE = func(cmd *cobra.Command, args []string) error { From 144f46bf82384dd70f1d73fdd903cca6c2194b41 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Mon, 31 Aug 2026 12:10:18 +1000 Subject: [PATCH 06/13] fix: let config set Shell "" clear the setting and offer Shell in config get Two gaps in the config surface for the new key: - an empty value was rejected by validation, so once Shell was set the only way back to auto detection was hand editing the config file. Empty is the documented default, so it now clears the key. - config get's interactive key picker never listed Shell, though set's did. Getting it by name already worked, since IsValidKey goes through viper. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cmd/config/get/get.go | 1 + pkg/cmd/config/set/set.go | 8 ++++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/config/get/get.go b/pkg/cmd/config/get/get.go index e76b11b0..38c2d537 100644 --- a/pkg/cmd/config/get/get.go +++ b/pkg/cmd/config/get/get.go @@ -65,6 +65,7 @@ func promptMissing(ask question.Asker) (string, error) { constants.ConfigOutputFormat, constants.ConfigShowOctopus, constants.ConfigEditor, + constants.ConfigShell, // constants.ConfigProxyUrl, } diff --git a/pkg/cmd/config/set/set.go b/pkg/cmd/config/set/set.go index f01a12ea..91eec84e 100644 --- a/pkg/cmd/config/set/set.go +++ b/pkg/cmd/config/set/set.go @@ -76,8 +76,12 @@ func setRun(isPromptEnabled bool, ask question.Asker, key string, value string) } localViper.Set(key, boolValue) case strings.ToLower(constants.ConfigShell): - if err := shell.Validate(value); err != nil { - return err + // empty is the documented default and clears the setting, putting the CLI back + // to detecting the shell it is running under + if value != "" { + if err := shell.Validate(value); err != nil { + return err + } } localViper.Set(key, value) default: From 30e63a0a0c3b02aa182558ecb9f01accd71a2fb6 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 4 Sep 2026 14:06:28 +1000 Subject: [PATCH 07/13] fix: warn instead of failing on an invalid OCTOPUS_SHELL Validating OCTOPUS_SHELL in PersistentPreRunE aborted every command, so a stale or mistaken value exported in a shell profile locked the user out of the whole CLI, including the `octopus config set Shell` needed to fix it. --shell keeps failing: it is typed for a single command, so an error is what the user expects and retyping it is the fix. The environment variable and the config file value are set once and apply to everything afterwards, so those now warn and fall back to detecting the host shell. --- pkg/cmd/root/root.go | 14 +++++--- pkg/cmd/root/root_test.go | 69 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 5 deletions(-) create mode 100644 pkg/cmd/root/root_test.go diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index c7170373..be74716d 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -30,6 +30,7 @@ import ( workerPoolCmd "github.com/OctopusDeploy/cli/pkg/cmd/workerpool" "github.com/OctopusDeploy/cli/pkg/constants" "github.com/OctopusDeploy/cli/pkg/factory" + "github.com/OctopusDeploy/cli/pkg/output" "github.com/OctopusDeploy/cli/pkg/question" "github.com/OctopusDeploy/cli/pkg/util/shell" "github.com/spf13/cobra" @@ -124,17 +125,20 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro // if we attempt to check the flags before Execute is called, cobra hasn't parsed anything yet, // so we'll get bad values. PersistentPreRun is a convenient callback for setting up our // environment after parsing but before execution. - cmd.PersistentPreRunE = func(_ *cobra.Command, _ []string) error { - // an explicitly asked for shell is validated here rather than silently ignored; - // the config file value isn't, because a bad one there would lock the user out - // of the config commands they need to fix it. + cmd.PersistentPreRunE = func(cmd *cobra.Command, _ []string) error { + // --shell is validated because it was typed for this one command, so failing is + // what the user expects and they can just retype it. OCTOPUS_SHELL and the config + // file value are only warned about: both are set once and apply to every command + // afterwards, so rejecting them would lock the user out of the whole CLI, + // including the `config set Shell` needed to fix it. Detect falls back to the + // host shell in that case. if v, _ := cmdPFlags.GetString(constants.FlagShell); v != "" { if err := shell.Validate(v); err != nil { return fmt.Errorf("--%s: %w", constants.FlagShell, err) } } else if v := os.Getenv(constants.EnvOctopusShell); v != "" { if err := shell.Validate(v); err != nil { - return fmt.Errorf("%s: %w", constants.EnvOctopusShell, err) + fmt.Fprintf(cmd.ErrOrStderr(), "%s\n", output.Yellow(fmt.Sprintf("Warning: ignoring %s: %s", constants.EnvOctopusShell, err))) } } diff --git a/pkg/cmd/root/root_test.go b/pkg/cmd/root/root_test.go new file mode 100644 index 00000000..0eacccae --- /dev/null +++ b/pkg/cmd/root/root_test.go @@ -0,0 +1,69 @@ +package root_test + +import ( + "bytes" + "testing" + + cmdRoot "github.com/OctopusDeploy/cli/pkg/cmd/root" + "github.com/OctopusDeploy/cli/pkg/constants" + "github.com/OctopusDeploy/cli/test/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// runVersion executes a command that needs neither a server nor prompting, so the only +// thing under test is the shell validation in PersistentPreRunE. +func runVersion(t *testing.T, args ...string) (string, error) { + t.Helper() + + stdout := &bytes.Buffer{} + stderr := &bytes.Buffer{} + + rootCmd := cmdRoot.NewCmdRoot(testutil.NewMockFactory(testutil.NewMockHttpServer()), nil, nil) + rootCmd.SetOut(stdout) + rootCmd.SetErr(stderr) + rootCmd.SetArgs(append([]string{"version"}, args...)) + + err := rootCmd.Execute() + return stderr.String(), err +} + +func TestRoot_InvalidShellFlagIsRejected(t *testing.T) { + _, err := runVersion(t, "--shell", "fish") + require.Error(t, err) + assert.EqualError(t, err, "--shell: the provided value fish is not a valid shell, please use one of bash, powershell, cmd") +} + +func TestRoot_ValidShellFlagIsAccepted(t *testing.T) { + _, err := runVersion(t, "--shell", "pwsh") + assert.NoError(t, err) +} + +// an invalid OCTOPUS_SHELL is warned about rather than rejected; it is set once and +// applies to every command afterwards, so failing would lock the user out of the CLI +// entirely, including the config command needed to fix it. +func TestRoot_InvalidShellEnvIsWarnedAboutbutNotFatal(t *testing.T) { + t.Setenv(constants.EnvOctopusShell, "fish") + + stderr, err := runVersion(t) + assert.NoError(t, err) + assert.Contains(t, stderr, "Warning: ignoring OCTOPUS_SHELL: the provided value fish is not a valid shell") +} + +func TestRoot_ValidShellEnvIsSilent(t *testing.T) { + t.Setenv(constants.EnvOctopusShell, "pwsh") + + stderr, err := runVersion(t) + assert.NoError(t, err) + assert.Equal(t, "", stderr) +} + +// the flag is the more specific signal, so a bad env var alongside a good flag is not +// worth complaining about +func TestRoot_ShellFlagSupersedesInvalidEnv(t *testing.T) { + t.Setenv(constants.EnvOctopusShell, "fish") + + stderr, err := runVersion(t, "--shell", "bash") + assert.NoError(t, err) + assert.Equal(t, "", stderr) +} From fd22bc50337a04b0055ba14272c17f8860609523 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 4 Sep 2026 14:07:10 +1000 Subject: [PATCH 08/13] test: run the zsh quoting round trip on CI zsh isn't on the ubuntu runner image, so TestQuoteZsh_RoundTrip skipped on every build. It is the strict posix test, and the only executable guard for the leading = fix, so quoting could have regressed with CI still green. Installs zsh in the workflow, and makes a missing shell fail rather than skip when CI is set so the coverage can't quietly disappear again. --- .github/workflows/integration-test.yml | 5 ++++ .github/workflows/pr-validation.yml | 6 +++++ pkg/util/shell/quote_test.go | 33 ++++++++++++++++---------- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/.github/workflows/integration-test.yml b/.github/workflows/integration-test.yml index 018d6bf5..6e18d2cf 100644 --- a/.github/workflows/integration-test.yml +++ b/.github/workflows/integration-test.yml @@ -62,6 +62,11 @@ jobs: - name: Setup gotestsum run: go install gotest.tools/gotestsum@latest + # the unit tests below include the shell quoting round trips, which need a real + # zsh; it isn't on the runner image + - name: Install zsh + run: sudo apt-get update && sudo apt-get install -y zsh + # we don't technically need to run the unit tests but they're fast so why not - name: Unit Tests run: gotestsum --format testname --junitfile ../unit-tests.xml diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index 05947d79..fff48562 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -28,6 +28,12 @@ jobs: - name: Setup gotestsum run: go install gotest.tools/gotestsum@latest + # the shell quoting round trip tests run the generated commands through the real + # shells; pwsh is already on the runner image but zsh isn't, and zsh is the strict + # one because it expands a leading = and ~ where the other posix shells don't + - name: Install zsh + run: sudo apt-get update && sudo apt-get install -y zsh + - name: Unit Tests run: gotestsum --format testname --junitfile ../unit-tests.xml working-directory: ./pkg diff --git a/pkg/util/shell/quote_test.go b/pkg/util/shell/quote_test.go index 7fea7b13..c2e60897 100644 --- a/pkg/util/shell/quote_test.go +++ b/pkg/util/shell/quote_test.go @@ -2,6 +2,7 @@ package shell_test import ( "fmt" + "os" "os/exec" "strings" "testing" @@ -109,12 +110,26 @@ func TestQuoteCmd_RoundTrip(t *testing.T) { } } -// TestQuotePosix_RoundTrip runs the quoted values through a real /bin/sh. -func TestQuotePosix_RoundTrip(t *testing.T) { - sh, err := exec.LookPath("sh") +// lookShell finds a shell to round trip through. Locally a missing shell just skips the +// test, but on CI it fails: these tests are the only thing that checks the generated +// quoting against a real parser, and a silent skip there means the coverage quietly +// disappears the day the runner image or the workflow changes. +func lookShell(t *testing.T, name string) string { + t.Helper() + + path, err := exec.LookPath(name) if err != nil { - t.Skip("sh is not available") + if os.Getenv("CI") != "" { + t.Fatalf("%s is not installed; it is needed to round trip the generated quoting", name) + } + t.Skipf("%s is not available", name) } + return path +} + +// TestQuotePosix_RoundTrip runs the quoted values through a real /bin/sh. +func TestQuotePosix_RoundTrip(t *testing.T) { + sh := lookShell(t, "sh") for _, value := range append(roundTripValues, "line1\nline2") { t.Run(fmt.Sprintf("%q", value), func(t *testing.T) { @@ -130,10 +145,7 @@ func TestQuotePosix_RoundTrip(t *testing.T) { // quoting also covers. zsh expands a leading = and a leading ~ where the other posix // shells don't, so it is the stricter test of the two. func TestQuoteZsh_RoundTrip(t *testing.T) { - zsh, err := exec.LookPath("zsh") - if err != nil { - t.Skip("zsh is not available") - } + zsh := lookShell(t, "zsh") for _, value := range append(roundTripValues, "line1\nline2") { t.Run(fmt.Sprintf("%q", value), func(t *testing.T) { @@ -148,10 +160,7 @@ func TestQuoteZsh_RoundTrip(t *testing.T) { // TestQuotePowerShell_RoundTrip runs the quoted values through pwsh when it happens to // be installed; it isn't on CI, so this usually skips. func TestQuotePowerShell_RoundTrip(t *testing.T) { - pwsh, err := exec.LookPath("pwsh") - if err != nil { - t.Skip("pwsh is not available") - } + pwsh := lookShell(t, "pwsh") for _, value := range append(roundTripValues, "line1\nline2") { t.Run(fmt.Sprintf("%q", value), func(t *testing.T) { From d5b0f6a3f3d837800678b4d8afa55400920da85d Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 4 Sep 2026 14:08:26 +1000 Subject: [PATCH 09/13] docs: record what PowerShell 5.1 does to a generated command quotePowerShell gets a value through PowerShell's parser, but handing it on to a native executable is a second step, and Windows PowerShell 5.1 rebuilds the command line without escaping. A trailing backslash and an embedded double quote both arrive corrupted there however they are quoted, which is a limitation worth stating next to the code rather than leaving to be rediscovered. PowerShell 7 is unaffected. The existing round trip test uses Write-Host, a cmdlet, so it only proves PowerShell parsed the value; it never reaches the native argument step where this goes wrong. Adds a second round trip through printf that does. --- pkg/util/shell/quote.go | 12 ++++++++++++ pkg/util/shell/quote_test.go | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/pkg/util/shell/quote.go b/pkg/util/shell/quote.go index e94a14c0..b28d31c5 100644 --- a/pkg/util/shell/quote.go +++ b/pkg/util/shell/quote.go @@ -65,6 +65,18 @@ var powerShellQuoteEscaper = strings.NewReplacer( // quotePowerShell quotes for PowerShell. Single quoted strings are literal, and a single // quote is escaped by doubling it. +// +// This gets the value through PowerShell's own parser intact, which is as far as we can +// go. Handing it on to a native executable is PowerShell's job, and Windows PowerShell +// 5.1 does it badly: it rebuilds the command line without escaping, so two shapes of +// value still arrive corrupted no matter how they are quoted here. +// - a trailing backslash escapes the closing quote 5.1 generates, so +// C:\Program Files\Octopus\ arrives as C:\Program Files\Octopus" +// - an embedded double quote isn't escaped either, so say "hi" loses its quotes +// +// PowerShell 7 fixed both. There is nothing to do about 5.1 short of emitting the +// argument-by-argument syntax, which is unreadable for a command meant to be copied, +// so this is a documented limitation rather than a bug in the quoting. func quotePowerShell(value string) string { if isBare(value, powerShellSafeChars) { return value diff --git a/pkg/util/shell/quote_test.go b/pkg/util/shell/quote_test.go index c2e60897..447ab1a5 100644 --- a/pkg/util/shell/quote_test.go +++ b/pkg/util/shell/quote_test.go @@ -172,6 +172,27 @@ func TestQuotePowerShell_RoundTrip(t *testing.T) { } } +// TestQuotePowerShell_RoundTripToNativeCommand goes a step further than the test above: +// Write-Host is a cmdlet, so it only proves PowerShell parsed the value into one string. +// The CLI is a native executable, and handing an argument to one of those is a separate +// step with its own escaping. Running printf, which echoes its argument back verbatim, +// covers that step too. It is the step Windows PowerShell 5.1 gets wrong for a trailing +// backslash or an embedded quote, as quotePowerShell describes; 5.1 is Windows only and +// can't be exercised here, so this guards the PowerShell 7 behaviour we can reach. +func TestQuotePowerShell_RoundTripToNativeCommand(t *testing.T) { + pwsh := lookShell(t, "pwsh") + printf := lookShell(t, "printf") + + for _, value := range append(roundTripValues, "line1\nline2") { + t.Run(fmt.Sprintf("%q", value), func(t *testing.T) { + script := "& '" + printf + "' '%s' " + shell.Quote(shell.PowerShell, value) + out, err := exec.Command(pwsh, "-NoProfile", "-Command", script).Output() + require.NoError(t, err) + assert.Equal(t, value, string(out)) + }) + } +} + // simulateCmd applies cmd.exe's own processing to a command line and returns what cmd // would hand to the program. It models the interactive prompt, which is where a copied // command gets pasted; percent expansion in a batch file follows different rules and a From c75c6a059cef7e8dcbc880ba6be5b941d056065c Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 4 Sep 2026 14:08:39 +1000 Subject: [PATCH 10/13] docs: correct why cmd is the windows detection fallback The comment claimed double quoted output also works in PowerShell, which holds for an ordinary quoted value but not for one containing a double quote, where the ^ escapes mean nothing to PowerShell, nor for a trailing backslash, which cmd doubles for argv. The fallback is still the better of the two, but it isn't the clean degradation the comment promised, and it matters because this is the branch taken whenever the parent process can't be identified. --- pkg/util/shell/shell.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/util/shell/shell.go b/pkg/util/shell/shell.go index d9f342c7..bc43de05 100644 --- a/pkg/util/shell/shell.go +++ b/pkg/util/shell/shell.go @@ -75,8 +75,13 @@ func Detect(goos string, getenv func(string) string) Shell { if s, ok := Parse(parentProcessName()); ok { return s } - // cmd is the safer guess: double quoted output also works in PowerShell, - // whereas single quoted output doesn't work in cmd at all. + // cmd is the guess with the better failure mode, though neither is safe. Single + // quoted output is useless in cmd for any value that needed quoting, whereas cmd + // output is mostly readable in PowerShell: a plain quoted value like + // "Soft Drinks" works in both. It only diverges for a value carrying a double + // quote, where the ^ escapes are meaningless to PowerShell, or a trailing + // backslash, which cmd doubles for argv and PowerShell leaves alone. Setting + // Shell in config is the fix when detection can't see the parent process. return Cmd } From 45921f0a2f3f71ce9a15672a0f1c9030d9d045c8 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 4 Sep 2026 14:11:09 +1000 Subject: [PATCH 11/13] fix: detect the shell from the parent process on unix Detection read $SHELL, which names the login shell rather than the shell the command was typed into. Someone whose login shell is bash but who is working in pwsh got posix quoting, and 'Bob'\''s Project' is not something pwsh can parse, so the generated command was broken for exactly the case the shell support was added for. parentProcessName was stubbed out on unix as only being needed to tell cmd and PowerShell apart, but pwsh on unix is that case. Reads /proc on linux and asks ps elsewhere, falling back to $SHELL when the parent isn't a shell we know, which is what happens under make, a CI runner or an IDE. Detect now takes the lookup as a parameter alongside goos and getenv so the unix and windows branches are both testable, which also removes the skip the windows cases needed. --- pkg/util/shell/parent_other.go | 9 -------- pkg/util/shell/parent_unix.go | 40 ++++++++++++++++++++++++++++++++ pkg/util/shell/shell.go | 18 +++++++++++---- pkg/util/shell/shell_test.go | 42 +++++++++++++++++++--------------- 4 files changed, 76 insertions(+), 33 deletions(-) delete mode 100644 pkg/util/shell/parent_other.go create mode 100644 pkg/util/shell/parent_unix.go diff --git a/pkg/util/shell/parent_other.go b/pkg/util/shell/parent_other.go deleted file mode 100644 index 01387ba0..00000000 --- a/pkg/util/shell/parent_other.go +++ /dev/null @@ -1,9 +0,0 @@ -//go:build !windows - -package shell - -// parentProcessName is only used to tell cmd and PowerShell apart, so there is -// nothing to look up on unix. -func parentProcessName() string { - return "" -} diff --git a/pkg/util/shell/parent_unix.go b/pkg/util/shell/parent_unix.go new file mode 100644 index 00000000..9c6cd651 --- /dev/null +++ b/pkg/util/shell/parent_unix.go @@ -0,0 +1,40 @@ +//go:build !windows + +package shell + +import ( + "os" + "os/exec" + "path/filepath" + "runtime" + "strconv" + "strings" +) + +// parentProcessName returns the file name of the executable that launched us, or "" +// when it can't be worked out. $SHELL names the login shell, not the shell the command +// was actually typed into, so this is what tells us the user is sitting in pwsh on a +// machine whose login shell is something else. +func parentProcessName() string { + ppid := os.Getppid() + if ppid <= 1 { + return "" + } + + if runtime.GOOS == "linux" { + // /proc//comm is truncated to 15 characters, which is fine for every name + // Parse recognises + if b, err := os.ReadFile("/proc/" + strconv.Itoa(ppid) + "/comm"); err == nil { + return strings.TrimSpace(string(b)) + } + return "" + } + + // darwin and the bsds have no /proc, so ask ps. -o comm= prints the executable with + // no header, as a full path on darwin, so take the base of it + out, err := exec.Command("ps", "-o", "comm=", "-p", strconv.Itoa(ppid)).Output() + if err != nil { + return "" + } + return filepath.Base(strings.TrimSpace(string(out))) +} diff --git a/pkg/util/shell/shell.go b/pkg/util/shell/shell.go index bc43de05..d4572ae7 100644 --- a/pkg/util/shell/shell.go +++ b/pkg/util/shell/shell.go @@ -55,12 +55,13 @@ func Current() Shell { if s, ok := Parse(viper.GetString(constants.ConfigShell)); ok { return s } - return Detect(runtime.GOOS, os.Getenv) + return Detect(runtime.GOOS, os.Getenv, parentProcessName) } -// Detect works out which shell the CLI is being run from. goos is a runtime.GOOS value -// and getenv looks up environment variables; both are parameters so this can be tested. -func Detect(goos string, getenv func(string) string) Shell { +// Detect works out which shell the CLI is being run from. goos is a runtime.GOOS value, +// getenv looks up environment variables and parentProcess names the executable that +// launched us; all three are parameters so this can be tested. +func Detect(goos string, getenv func(string) string, parentProcess func() string) Shell { // checked here as well as through viper so the override still works if the // config system hasn't been set up, such as in tests if s, ok := Parse(getenv(constants.EnvOctopusShell)); ok { @@ -72,7 +73,7 @@ func Detect(goos string, getenv func(string) string) Shell { // a machine wide variable that cmd.exe inherits too, so it tells us nothing. // note this always misses when cross-compiled elsewhere, which is why it can't // be the only check. - if s, ok := Parse(parentProcessName()); ok { + if s, ok := Parse(parentProcess()); ok { return s } // cmd is the guess with the better failure mode, though neither is safe. Single @@ -85,6 +86,13 @@ func Detect(goos string, getenv func(string) string) Shell { return Cmd } + // the parent process is asked first because it names the shell the command was + // actually typed into. $SHELL is only the login shell, so someone who runs pwsh from + // a bash login would otherwise get posix quoting, which pwsh can't parse. + if s, ok := Parse(parentProcess()); ok { + return s + } + if sh := getenv("SHELL"); sh != "" { if s, ok := Parse(filepath.Base(sh)); ok { return s diff --git a/pkg/util/shell/shell_test.go b/pkg/util/shell/shell_test.go index 395af9c5..d936953f 100644 --- a/pkg/util/shell/shell_test.go +++ b/pkg/util/shell/shell_test.go @@ -1,7 +1,6 @@ package shell_test import ( - "runtime" "testing" "github.com/OctopusDeploy/cli/pkg/util/shell" @@ -47,30 +46,35 @@ func TestValidate(t *testing.T) { func TestDetect(t *testing.T) { tests := []struct { - name string - goos string - env map[string]string - expected shell.Shell - usesParentProcess bool + name string + goos string + env map[string]string + parent string + expected shell.Shell }{ - {"unix with SHELL", "linux", map[string]string{"SHELL": "/bin/zsh"}, shell.Bash, false}, - {"unix with unknown SHELL", "linux", map[string]string{"SHELL": "/usr/bin/fish"}, shell.Bash, false}, - {"unix without SHELL", "darwin", nil, shell.Bash, false}, - {"unix with pwsh as SHELL", "linux", map[string]string{"SHELL": "/usr/local/bin/pwsh"}, shell.PowerShell, false}, - {"windows falls back to cmd", "windows", nil, shell.Cmd, true}, - {"windows ignores PSModulePath", "windows", map[string]string{"PSModulePath": `C:\Program Files\WindowsPowerShell\Modules`}, shell.Cmd, true}, - {"override wins on unix", "linux", map[string]string{"SHELL": "/bin/bash", "OCTOPUS_SHELL": "cmd"}, shell.Cmd, false}, - {"override wins on windows", "windows", map[string]string{"OCTOPUS_SHELL": "pwsh"}, shell.PowerShell, false}, - {"invalid override is ignored", "linux", map[string]string{"SHELL": "/bin/bash", "OCTOPUS_SHELL": "fish"}, shell.Bash, false}, + {"unix with SHELL", "linux", map[string]string{"SHELL": "/bin/zsh"}, "", shell.Bash}, + {"unix with unknown SHELL", "linux", map[string]string{"SHELL": "/usr/bin/fish"}, "", shell.Bash}, + {"unix without SHELL", "darwin", nil, "", shell.Bash}, + {"unix with pwsh as SHELL", "linux", map[string]string{"SHELL": "/usr/local/bin/pwsh"}, "", shell.PowerShell}, + {"windows falls back to cmd", "windows", nil, "", shell.Cmd}, + {"windows ignores PSModulePath", "windows", map[string]string{"PSModulePath": `C:\Program Files\WindowsPowerShell\Modules`}, "", shell.Cmd}, + {"windows reads the parent process", "windows", nil, "powershell.exe", shell.PowerShell}, + {"override wins on unix", "linux", map[string]string{"SHELL": "/bin/bash", "OCTOPUS_SHELL": "cmd"}, "", shell.Cmd}, + {"override wins on windows", "windows", map[string]string{"OCTOPUS_SHELL": "pwsh"}, "", shell.PowerShell}, + {"invalid override is ignored", "linux", map[string]string{"SHELL": "/bin/bash", "OCTOPUS_SHELL": "fish"}, "", shell.Bash}, + // $SHELL names the login shell, so it keeps saying bash while the user is sat in + // pwsh; the parent process is the one that knows + {"unix in pwsh started from bash", "linux", map[string]string{"SHELL": "/bin/bash"}, "pwsh", shell.PowerShell}, + {"unix parent beats SHELL", "darwin", map[string]string{"SHELL": "/bin/bash"}, "zsh", shell.Bash}, + {"unix falls back to SHELL for an unknown parent", "linux", map[string]string{"SHELL": "/usr/local/bin/pwsh"}, "make", shell.PowerShell}, + {"unix override beats the parent process", "linux", map[string]string{"OCTOPUS_SHELL": "cmd"}, "pwsh", shell.Cmd}, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if test.usesParentProcess && runtime.GOOS == "windows" { - t.Skip("the real parent process is inspected when actually running on windows") - } getenv := func(key string) string { return test.env[key] } - assert.Equal(t, test.expected, shell.Detect(test.goos, getenv)) + parent := func() string { return test.parent } + assert.Equal(t, test.expected, shell.Detect(test.goos, getenv, parent)) }) } } From ad69548ceb03fef186835c24226cae9ad5966a65 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 4 Sep 2026 14:13:39 +1000 Subject: [PATCH 12/13] feat: warn when a generated cmd command can't be pasted into a script quoteCmd's encoding for % only survives the interactive prompt; a .bat or .cmd file drops an unmatched % and substitutes %var%, so a project named "100% Cotton" is silently wrong when the generated command is pasted into a script, which is what the command is for. ! has the same problem under delayed expansion, and a line break can't be quoted in cmd at all. None of that is fixable in the quoting, so the command now says so. The warning is appended to the returned string rather than printed by each of the 49 call sites, which all print the result verbatim; that keeps the warning attached to the command it describes without a mechanical change across the whole tree. Posix and PowerShell can quote anything, so they never warn. --- pkg/util/flag/flag.go | 16 ++++++++++++- pkg/util/flag/flag_test.go | 31 ++++++++++++++++++++++++ pkg/util/shell/warn.go | 47 ++++++++++++++++++++++++++++++++++++ pkg/util/shell/warn_test.go | 48 +++++++++++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 pkg/util/shell/warn.go create mode 100644 pkg/util/shell/warn_test.go diff --git a/pkg/util/flag/flag.go b/pkg/util/flag/flag.go index 483eb0c2..7886a67d 100644 --- a/pkg/util/flag/flag.go +++ b/pkg/util/flag/flag.go @@ -3,6 +3,7 @@ package flag import ( "fmt" + "github.com/OctopusDeploy/cli/pkg/output" "github.com/OctopusDeploy/cli/pkg/util/shell" ) @@ -66,8 +67,17 @@ func GenerateAutomationCmd(cmdPath string, space string, flags ...Generatable) s // GenerateAutomationCmdForShell generates the automation command, quoting values using // the rules of the given shell. +// +// Where the shell can't carry a value at all the warning is appended on its own line, +// because the command exists to be copied and the user would otherwise paste something +// that is silently wrong. Every caller prints the result straight out, so returning it +// as part of the string keeps the warning next to the command it is about. func GenerateAutomationCmdForShell(sh shell.Shell, cmdPath string, space string, flags ...Generatable) string { - quote := func(value string) string { return shell.Quote(sh, value) } + var values []string + quote := func(value string) string { + values = append(values, value) + return shell.Quote(sh, value) + } autoCmd := cmdPath if space != "" { @@ -105,5 +115,9 @@ func GenerateAutomationCmdForShell(sh shell.Shell, cmdPath string, space string, } } autoCmd += " --no-prompt" + + if warning := shell.PasteWarning(sh, values...); warning != "" { + autoCmd += "\n" + output.Yellow(warning) + } return autoCmd } diff --git a/pkg/util/flag/flag_test.go b/pkg/util/flag/flag_test.go index 05fa5aff..62ce176f 100644 --- a/pkg/util/flag/flag_test.go +++ b/pkg/util/flag/flag_test.go @@ -60,3 +60,34 @@ func TestGenerateAutomationCmdForShell_NoSpace(t *testing.T) { actual := flag.GenerateAutomationCmdForShell(shell.Bash, "octopus environment create", "", name) assert.Equal(t, "octopus environment create --name Dev --no-prompt", actual) } + +func TestGenerateAutomationCmdForShell_WarnsWhenCmdCannotCarryAValue(t *testing.T) { + project := flag.New[string]("project", false) + project.Value = "100% Cotton" + + actual := flag.GenerateAutomationCmdForShell(shell.Cmd, "octopus release deploy", "", project) + + assert.Contains(t, actual, `--project "100"^%" Cotton"`) + assert.Contains(t, actual, "\nWarning: this command can't be pasted into a script as it is:") + assert.Contains(t, actual, "%") +} + +// the same value is fine in a posix shell, so nothing is appended +func TestGenerateAutomationCmdForShell_DoesNotWarnForPosix(t *testing.T) { + project := flag.New[string]("project", false) + project.Value = "100% Cotton" + + actual := flag.GenerateAutomationCmdForShell(shell.Bash, "octopus release deploy", "", project) + + assert.Equal(t, `octopus release deploy --project '100% Cotton' --no-prompt`, actual) +} + +// the space is quoted like any other value, so it has to be checked too +func TestGenerateAutomationCmdForShell_WarnsAboutTheSpaceName(t *testing.T) { + name := flag.New[string]("name", false) + name.Value = "Dev" + + actual := flag.GenerateAutomationCmdForShell(shell.Cmd, "octopus environment create", "100% Cotton", name) + + assert.Contains(t, actual, "Warning: this command can't be pasted into a script as it is:") +} diff --git a/pkg/util/shell/warn.go b/pkg/util/shell/warn.go new file mode 100644 index 00000000..1759919b --- /dev/null +++ b/pkg/util/shell/warn.go @@ -0,0 +1,47 @@ +package shell + +import ( + "fmt" + "strings" +) + +// unsupported lists, per shell, the characters a quoted value can't reliably carry, and +// what goes wrong. Only cmd has any: posix shells and PowerShell can quote anything. +var unsupported = map[Shell][]struct { + char string + name string + effect string +}{ + Cmd: { + {"%", "%", "is expanded before any escaping is applied, so it only survives at the interactive prompt; a .bat or .cmd script drops an unmatched % and replaces %var%"}, + {"!", "!", "is expanded when delayed expansion is switched on, which strips it and anything it encloses"}, + {"\n", "a line break", "ends the command in cmd, and nothing can quote it"}, + }, +} + +// PasteWarning returns a warning for the values that can't make it through the shell +// intact, or "" when they all can. The generated command is meant to be copied and +// pasted, and quoting alone can't tell the user that what they're about to paste is +// going to be silently mangled. +func PasteWarning(sh Shell, values ...string) string { + problems := unsupported[sh] + if len(problems) == 0 { + return "" + } + + var found []string + seen := map[string]bool{} + for _, p := range problems { + for _, v := range values { + if strings.Contains(v, p.char) && !seen[p.name] { + seen[p.name] = true + found = append(found, fmt.Sprintf("%s %s", p.name, p.effect)) + } + } + } + if len(found) == 0 { + return "" + } + + return fmt.Sprintf("Warning: this command can't be pasted into a script as it is: %s.", strings.Join(found, "; and ")) +} diff --git a/pkg/util/shell/warn_test.go b/pkg/util/shell/warn_test.go new file mode 100644 index 00000000..f784749a --- /dev/null +++ b/pkg/util/shell/warn_test.go @@ -0,0 +1,48 @@ +package shell_test + +import ( + "testing" + + "github.com/OctopusDeploy/cli/pkg/util/shell" + "github.com/stretchr/testify/assert" +) + +func TestPasteWarning(t *testing.T) { + tests := []struct { + name string + shell shell.Shell + values []string + contains []string + }{ + {"cmd is fine with an ordinary value", shell.Cmd, []string{"Soft Drinks", "0.0.3"}, nil}, + {"cmd warns about a percent", shell.Cmd, []string{"100% Cotton"}, []string{"%"}}, + {"cmd warns about an environment variable", shell.Cmd, []string{"%PATH%"}, []string{"%"}}, + {"cmd warns about a bang", shell.Cmd, []string{"Ship it!"}, []string{"!"}}, + {"cmd warns about a line break", shell.Cmd, []string{"line1\nline2"}, []string{"a line break"}}, + {"cmd reports every problem it finds", shell.Cmd, []string{"100%", "Ship it!"}, []string{"%", "!"}}, + {"cmd only reports each problem once", shell.Cmd, []string{"100%", "50%"}, []string{"%"}}, + {"cmd checks every value", shell.Cmd, []string{"fine", "100% Cotton"}, []string{"%"}}, + // posix shells and PowerShell can quote anything, so there is never a warning + {"bash is always fine", shell.Bash, []string{"100% Cotton", "Ship it!", "line1\nline2"}, nil}, + {"powershell is always fine", shell.PowerShell, []string{"100% Cotton", "Ship it!", "line1\nline2"}, nil}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := shell.PasteWarning(test.shell, test.values...) + + if test.contains == nil { + assert.Equal(t, "", actual) + return + } + assert.Contains(t, actual, "Warning:") + for _, want := range test.contains { + assert.Contains(t, actual, want) + } + }) + } +} + +func TestPasteWarning_NoValues(t *testing.T) { + assert.Equal(t, "", shell.PasteWarning(shell.Cmd)) +} From cf7e48971912a8b9c431beffaa9b8cbc4c9a9ae2 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Fri, 4 Sep 2026 14:13:51 +1000 Subject: [PATCH 13/13] docs: correct which round trip tests run on CI The comment had the two environments the wrong way round: pwsh ships on the ubuntu runner image, so the PowerShell round trip runs on every build, and it is a local machine without pwsh that skips it. Reading it the other way makes the PowerShell quoting look unverified and invites a change to it being under-tested. --- pkg/util/shell/quote_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/util/shell/quote_test.go b/pkg/util/shell/quote_test.go index 447ab1a5..c58b55d4 100644 --- a/pkg/util/shell/quote_test.go +++ b/pkg/util/shell/quote_test.go @@ -157,8 +157,9 @@ func TestQuoteZsh_RoundTrip(t *testing.T) { } } -// TestQuotePowerShell_RoundTrip runs the quoted values through pwsh when it happens to -// be installed; it isn't on CI, so this usually skips. +// TestQuotePowerShell_RoundTrip checks the quoted values survive PowerShell's own +// parser. pwsh is on the ubuntu runner image, so this does run on CI; it is a local +// machine without pwsh installed where it skips. func TestQuotePowerShell_RoundTrip(t *testing.T) { pwsh := lookShell(t, "pwsh")