fix: quote automation commands for the host shell

.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

.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

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,
 	}
 

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)
 			}

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,23 @@ 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):
+		// 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:
 		localViper.Set(key, value)
 	}
 	if err := localViper.WriteConfig(); err != nil {
@@ -91,6 +102,7 @@ func promptMissing(ask question.Asker, key string) (string, string, error) {
 		constants.ConfigOutputFormat,
 		constants.ConfigShowOctopus,
 		constants.ConfigEditor,
+		constants.ConfigShell,
 		// constants.ConfigProxyUrl,
 	}
 

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)
 

pkg/cmd/root/root.go

@@ -1,6 +1,10 @@
 package root
 
 import (
+	"fmt"
+	"os"
+	"strings"
+
 	"github.com/OctopusDeploy/cli/pkg/apiclient"
 	accountCmd "github.com/OctopusDeploy/cli/pkg/cmd/account"
 	apiCmd "github.com/OctopusDeploy/cli/pkg/cmd/api"
@@ -26,7 +30,9 @@ 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"
 	"github.com/spf13/viper"
 )
@@ -96,6 +102,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,11 +120,28 @@ 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
 	// environment after parsing but before execution.
-	cmd.PersistentPreRun = func(_ *cobra.Command, _ []string) {
+	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 {
+				fmt.Fprintf(cmd.ErrOrStderr(), "%s\n", output.Yellow(fmt.Sprintf("Warning: ignoring %s: %s", constants.EnvOctopusShell, err)))
+			}
+		}
+
 		// map flag alias values
 		for k, v := range flagAliases {
 			for _, aliasName := range v {
@@ -138,6 +163,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 {

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)
+}

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
 }
 

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,13 +39,15 @@ const (
 	ConfigEditor       = "Editor"
 	ConfigShowOctopus  = "ShowOctopus"
 	ConfigOutputFormat = "OutputFormat"
+	ConfigShell        = "Shell"
 )
 
 const (
 	EnvOctopusUrl         = "OCTOPUS_URL"
 	EnvOctopusApiKey      = "OCTOPUS_API_KEY"
 	EnvOctopusAccessToken = "OCTOPUS_ACCESS_TOKEN"
 	EnvOctopusSpace       = "OCTOPUS_SPACE"
+	EnvOctopusShell       = "OCTOPUS_SHELL"
 	EnvEditor             = "EDITOR"
 	EnvVisual             = "VISUAL"
 	EnvCI                 = "CI"

pkg/util/flag/flag.go

@@ -2,7 +2,9 @@ package flag
 
 import (
 	"fmt"
-	"strings"
+
+	"github.com/OctopusDeploy/cli/pkg/output"
+	"github.com/OctopusDeploy/cli/pkg/util/shell"
 )
 
 type Flag[T any] struct {
@@ -60,27 +62,44 @@ 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.
+//
+// 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 {
+	var values []string
+	quote := func(value string) string {
+		values = append(values, value)
+		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 {
@@ -96,5 +115,9 @@ func GenerateAutomationCmd(cmdPath string, space string, flags ...Generatable) s
 		}
 	}
 	autoCmd += " --no-prompt"
+
+	if warning := shell.PasteWarning(sh, values...); warning != "" {
+		autoCmd += "\n" + output.Yellow(warning)
+	}
 	return autoCmd
 }