From 4077ddc80354ae19200beedf66ff3d93c92a54f3 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 21 Oct 2026 22:16:41 -0600 Subject: [PATCH 01/10] test(shell): cover command variables whose path contains spaces --- pkg/shell/executable_path_test.go | 103 ++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 pkg/shell/executable_path_test.go diff --git a/pkg/shell/executable_path_test.go b/pkg/shell/executable_path_test.go new file mode 100644 index 0000000000..fe91693170 --- /dev/null +++ b/pkg/shell/executable_path_test.go @@ -0,0 +1,103 @@ +package shell + +import ( + "bytes" + "context" + "io" + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/suite" +) + +const helperProcessEnv = "DEVSY_SHELL_TEST_HELPER_PROCESS" + +func TestMain(m *testing.M) { + if os.Getenv(helperProcessEnv) == "1" { + _, _ = os.Stdout.WriteString("helper ran with " + os.Args[len(os.Args)-1]) + os.Exit(0) + } + os.Exit(m.Run()) +} + +type ExecutablePathTestSuite struct { + suite.Suite + executable string +} + +func TestExecutablePathTestSuite(t *testing.T) { + suite.Run(t, new(ExecutablePathTestSuite)) +} + +// SetupTest installs a copy of the test binary under a directory containing a +// space, like the binaries directory of a Windows profile such as "Jane Doe". +func (s *ExecutablePathTestSuite) SetupTest() { + dir := filepath.Join(s.T().TempDir(), "Jane Doe", "binaries") + s.Require().NoError(os.MkdirAll(dir, 0o750)) + + name := "provider" + if runtime.GOOS == "windows" { + name += ".exe" + } + s.executable = filepath.Join(dir, name) + + self, err := os.Executable() + s.Require().NoError(err) + source, err := os.Open(self) + s.Require().NoError(err) + defer func() { _ = source.Close() }() + target, err := os.OpenFile(s.executable, os.O_CREATE|os.O_WRONLY, 0o700) + s.Require().NoError(err) + _, err = io.Copy(target, source) + s.Require().NoError(err) + s.Require().NoError(target.Close()) +} + +func (s *ExecutablePathTestSuite) run(command string, environ ...string) (string, error) { + var stdout, stderr bytes.Buffer + err := RunEmulatedShell(context.Background(), &CommandRunner{ + Command: command, + Stdout: &stdout, + Stderr: &stderr, + Environ: append(os.Environ(), environ...), + }) + return stdout.String(), err +} + +func (s *ExecutablePathTestSuite) TestCommandVariableWithSpacesInPathIsNotSplit() { + output, err := s.run( + "${SSH_PROVIDER} init", + "SSH_PROVIDER="+s.executable, + helperProcessEnv+"=1", + ) + + s.Require().NoError(err) + s.Equal("helper ran with init", output) +} + +func (s *ExecutablePathTestSuite) TestShortVariableFormWithSpacesInPathIsNotSplit() { + output, err := s.run( + "$SSH_PROVIDER init", + "SSH_PROVIDER="+s.executable, + helperProcessEnv+"=1", + ) + + s.Require().NoError(err) + s.Equal("helper ran with init", output) +} + +func (s *ExecutablePathTestSuite) TestCommandVariableWithArgumentsStillSplits() { + output, err := s.run("${CMD} world", "CMD=echo hello") + + s.Require().NoError(err) + s.Equal("hello world\n", output) +} + +func (s *ExecutablePathTestSuite) TestVariableArgumentWithSpacesStillSplits() { + output, err := s.run("echo ${ARGS}", "ARGS=a b") + + s.Require().NoError(err) + s.Equal("a b\n", output) +} From 195b0eff74841f6ad02acbab15e90f1cf2f7e945 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 21 Oct 2026 22:31:49 -0600 Subject: [PATCH 02/10] ci: run emulated shell launch tests on Windows --- hack/ci/suites.yaml | 8 ++++++++ pkg/shell/executable_path_test.go | 28 +++++++++++++++------------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/hack/ci/suites.yaml b/hack/ci/suites.yaml index 337b874f9b..77a0b69cb1 100644 --- a/hack/ci/suites.yaml +++ b/hack/ci/suites.yaml @@ -80,6 +80,14 @@ unit: run: '^(TestInvocationSuite|TestProviderDaemonOverrides.*|TestStdioTransportSuite|TestLaunch.*|TestEnvBlock.*)$' args: [-race, -count=1, -v, -timeout=5m] + - name: windows-provider-launch + description: native Windows provider executable launch through the emulated shell + os: [windows] + packages: [./pkg/shell] + run: '^TestExecutablePathTestSuite$' + require_tests: [TestExecutablePathTestSuite] + args: [-race, -count=1, -v, -timeout=5m] + - name: ci-tooling-windows description: Go CI helpers that also run on Windows os: [windows] diff --git a/pkg/shell/executable_path_test.go b/pkg/shell/executable_path_test.go index fe91693170..97fda3c8ca 100644 --- a/pkg/shell/executable_path_test.go +++ b/pkg/shell/executable_path_test.go @@ -45,27 +45,18 @@ func (s *ExecutablePathTestSuite) SetupTest() { self, err := os.Executable() s.Require().NoError(err) - source, err := os.Open(self) + source, err := os.Open(self) //nolint:gosec // copies this test binary s.Require().NoError(err) defer func() { _ = source.Close() }() - target, err := os.OpenFile(s.executable, os.O_CREATE|os.O_WRONLY, 0o700) + target, err := os.OpenFile( //nolint:gosec // the copy must be executable + s.executable, os.O_CREATE|os.O_WRONLY, 0o700, + ) s.Require().NoError(err) _, err = io.Copy(target, source) s.Require().NoError(err) s.Require().NoError(target.Close()) } -func (s *ExecutablePathTestSuite) run(command string, environ ...string) (string, error) { - var stdout, stderr bytes.Buffer - err := RunEmulatedShell(context.Background(), &CommandRunner{ - Command: command, - Stdout: &stdout, - Stderr: &stderr, - Environ: append(os.Environ(), environ...), - }) - return stdout.String(), err -} - func (s *ExecutablePathTestSuite) TestCommandVariableWithSpacesInPathIsNotSplit() { output, err := s.run( "${SSH_PROVIDER} init", @@ -101,3 +92,14 @@ func (s *ExecutablePathTestSuite) TestVariableArgumentWithSpacesStillSplits() { s.Require().NoError(err) s.Equal("a b\n", output) } + +func (s *ExecutablePathTestSuite) run(command string, environ ...string) (string, error) { + var stdout, stderr bytes.Buffer + err := RunEmulatedShell(context.Background(), &CommandRunner{ + Command: command, + Stdout: &stdout, + Stderr: &stderr, + Environ: append(os.Environ(), environ...), + }) + return stdout.String(), err +} From c800de162edf15e7079fd04b41474bd9fb6acfe1 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 21 Oct 2026 22:43:01 -0600 Subject: [PATCH 03/10] fix(shell): keep spaced executable paths intact and explain Windows launch errors --- hack/ci/suites.yaml | 8 ++-- .../clientimplementation/process_other.go | 7 +++ .../clientimplementation/process_windows.go | 13 ++++++ .../provider_command_error_test.go | 45 +++++++++++++++++++ .../provider_command_error_windows_test.go | 30 +++++++++++++ .../clientimplementation/workspace_client.go | 30 ++++++++----- pkg/shell/shell.go | 45 +++++++++++++++++++ 7 files changed, 164 insertions(+), 14 deletions(-) create mode 100644 pkg/client/clientimplementation/process_other.go create mode 100644 pkg/client/clientimplementation/process_windows.go create mode 100644 pkg/client/clientimplementation/provider_command_error_test.go create mode 100644 pkg/client/clientimplementation/provider_command_error_windows_test.go diff --git a/hack/ci/suites.yaml b/hack/ci/suites.yaml index 77a0b69cb1..ea9f447be4 100644 --- a/hack/ci/suites.yaml +++ b/hack/ci/suites.yaml @@ -81,11 +81,11 @@ unit: args: [-race, -count=1, -v, -timeout=5m] - name: windows-provider-launch - description: native Windows provider executable launch through the emulated shell + description: native Windows provider executable launch and launch failure reporting os: [windows] - packages: [./pkg/shell] - run: '^TestExecutablePathTestSuite$' - require_tests: [TestExecutablePathTestSuite] + packages: [./pkg/shell, ./pkg/client/clientimplementation] + run: '^(TestExecutablePathTestSuite|TestProviderCommandErrorTestSuite)$' + require_tests: [TestExecutablePathTestSuite, TestProviderCommandErrorTestSuite] args: [-race, -count=1, -v, -timeout=5m] - name: ci-tooling-windows diff --git a/pkg/client/clientimplementation/process_other.go b/pkg/client/clientimplementation/process_other.go new file mode 100644 index 0000000000..f007d2a5c0 --- /dev/null +++ b/pkg/client/clientimplementation/process_other.go @@ -0,0 +1,7 @@ +//go:build !windows + +package clientimplementation + +func isInvalidProcessParameter(error) bool { + return false +} diff --git a/pkg/client/clientimplementation/process_windows.go b/pkg/client/clientimplementation/process_windows.go new file mode 100644 index 0000000000..f317d4cd02 --- /dev/null +++ b/pkg/client/clientimplementation/process_windows.go @@ -0,0 +1,13 @@ +//go:build windows + +package clientimplementation + +import ( + "errors" + + "golang.org/x/sys/windows" +) + +func isInvalidProcessParameter(err error) bool { + return errors.Is(err, windows.ERROR_INVALID_PARAMETER) +} diff --git a/pkg/client/clientimplementation/provider_command_error_test.go b/pkg/client/clientimplementation/provider_command_error_test.go new file mode 100644 index 0000000000..b265a50789 --- /dev/null +++ b/pkg/client/clientimplementation/provider_command_error_test.go @@ -0,0 +1,45 @@ +package clientimplementation + +import ( + "errors" + "io/fs" + "testing" + + "github.com/devsy-org/devsy/pkg/provider" + "github.com/stretchr/testify/suite" +) + +type ProviderCommandErrorTestSuite struct { + suite.Suite +} + +func TestProviderCommandErrorTestSuite(t *testing.T) { + suite.Run(t, new(ProviderCommandErrorTestSuite)) +} + +func (s *ProviderCommandErrorTestSuite) TestOmitsEmptyAction() { + cause := errors.New("exit status 255") + + err := providerCommandError("${SSH_PROVIDER} init", nil, cause) + + s.Equal(`run provider command "${SSH_PROVIDER} init": exit status 255`, err.Error()) + s.ErrorIs(err, cause) +} + +func (s *ProviderCommandErrorTestSuite) TestIncludesAction() { + cause := errors.New("exit status 1") + environ := []string{"UNRELATED=value", provider.CommandEnv + "=ls"} + + err := providerCommandError("${SSH_PROVIDER} command", environ, cause) + + s.Equal(`run provider command "${SSH_PROVIDER} command" (ls): exit status 1`, err.Error()) +} + +func (s *ProviderCommandErrorTestSuite) TestPlainStartFailureHasNoWindowsHint() { + cause := &fs.PathError{Op: "fork/exec", Path: "provider", Err: fs.ErrPermission} + + err := providerCommandError("provider", nil, cause) + + s.NotContains(err.Error(), "ERROR_INVALID_PARAMETER") + s.ErrorIs(err, fs.ErrPermission) +} diff --git a/pkg/client/clientimplementation/provider_command_error_windows_test.go b/pkg/client/clientimplementation/provider_command_error_windows_test.go new file mode 100644 index 0000000000..40a4a3d8dd --- /dev/null +++ b/pkg/client/clientimplementation/provider_command_error_windows_test.go @@ -0,0 +1,30 @@ +//go:build windows + +package clientimplementation + +import ( + "os" + "os/exec" + "syscall" + + "golang.org/x/sys/windows" +) + +// Combining DETACHED_PROCESS with CREATE_NEW_CONSOLE makes the real CreateProcess +// call fail with ERROR_INVALID_PARAMETER, the error reported in issue 1638. +func (s *ProviderCommandErrorTestSuite) TestRealInvalidParameterStartFailureGetsHint() { + self, err := os.Executable() + s.Require().NoError(err) + cmd := exec.Command(self) + cmd.SysProcAttr = &syscall.SysProcAttr{ + CreationFlags: windows.DETACHED_PROCESS | windows.CREATE_NEW_CONSOLE, + } + startErr := cmd.Start() + s.Require().ErrorIs(startErr, windows.ERROR_INVALID_PARAMETER) + + err = providerCommandError(self, nil, startErr) + + s.ErrorIs(err, windows.ERROR_INVALID_PARAMETER) + s.Contains(err.Error(), invalidProcessParameterHint) + s.Contains(err.Error(), "The parameter is incorrect") +} diff --git a/pkg/client/clientimplementation/workspace_client.go b/pkg/client/clientimplementation/workspace_client.go index 16f96e43aa..5c0a88750a 100644 --- a/pkg/client/clientimplementation/workspace_client.go +++ b/pkg/client/clientimplementation/workspace_client.go @@ -864,12 +864,7 @@ func RunCommand(ctx context.Context, opts RunCommandOptions) (retErr error) { Stderr: stderr, Environ: opts.Environ, }); err != nil { - return fmt.Errorf( - "run provider command %q (%s): %w", - opts.Command[0], - providerAction(opts.Environ), - err, - ) + return providerCommandError(opts.Command[0], opts.Environ, err) } return nil } @@ -880,10 +875,7 @@ func RunCommand(ctx context.Context, opts RunCommandOptions) (retErr error) { cmd.Stderr = stderr cmd.Env = opts.Environ if err := cmd.Run(); err != nil { - return fmt.Errorf( - "run provider command %q (%s): %w", - strings.Join(opts.Command, " "), providerAction(opts.Environ), err, - ) + return providerCommandError(strings.Join(opts.Command, " "), opts.Environ, err) } return nil } @@ -902,6 +894,24 @@ func flushCommandOutput(stdout, stderr *subprocess.StreamingRedactingWriter) err return errors.Join(stdoutErr, stderrErr) } +const invalidProcessParameterHint = "Windows rejected starting the provider executable " + + "(ERROR_INVALID_PARAMETER) before it ran, so this is a local process-launch failure " + + "rather than a remote host error; the cause is unconfirmed" + +func providerCommandError(command string, environ []string, err error) error { + action := "" + if name := providerAction(environ); name != "" { + action = fmt.Sprintf(" (%s)", name) + } + if isInvalidProcessParameter(err) { + return fmt.Errorf( + "run provider command %q%s: %w; %s", + command, action, err, invalidProcessParameterHint, + ) + } + return fmt.Errorf("run provider command %q%s: %w", command, action, err) +} + // providerAction extracts the provider action from the environment variables. func providerAction(environ []string) string { for _, kv := range environ { diff --git a/pkg/shell/shell.go b/pkg/shell/shell.go index babf371648..005e5ed999 100644 --- a/pkg/shell/shell.go +++ b/pkg/shell/shell.go @@ -46,6 +46,8 @@ func RunEmulatedShell( return err } + quoteExecutablePaths(parsed, expand.ListEnviron(runner.Environ...)) + r, err := interp.New(buildRunnerOptions(shellRunnerOptions{ stdin: runner.Stdin, stdout: runner.Stdout, @@ -70,6 +72,49 @@ func RunEmulatedShell( return nil } +// quoteExecutablePaths stops a command name such as ${PROVIDER_BINARY} from being +// split on whitespace when it expands to one existing file, e.g. a binary under +// C:\Users\Jane Doe. Values that are not a single file keep shell word splitting, +// so a variable like DOCKER_PATH="sudo docker" still expands to a command and arguments. +func quoteExecutablePaths(file *syntax.File, env expand.Environ) { + printer := syntax.NewPrinter() + syntax.Walk(file, func(node syntax.Node) bool { + call, ok := node.(*syntax.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + + word := call.Args[0] + if len(word.Parts) != 1 { + return true + } + exp, ok := word.Parts[0].(*syntax.ParamExp) + if !ok || exp.Param == nil || !isPlainExpansion(printer, word, exp.Param.Value) { + return true + } + if isExistingFile(env.Get(exp.Param.Value).String()) { + word.Parts[0] = &syntax.DblQuoted{Parts: []syntax.WordPart{exp}} + } + return true + }) +} + +func isPlainExpansion(printer *syntax.Printer, word *syntax.Word, name string) bool { + var sb strings.Builder + if err := printer.Print(&sb, word); err != nil { + return false + } + return sb.String() == "$"+name || sb.String() == "${"+name+"}" +} + +func isExistingFile(path string) bool { + if !strings.ContainsAny(path, " \t\n") { + return false + } + info, err := os.Stat(path) + return err == nil && !info.IsDir() +} + type shellRunnerOptions struct { stdin io.Reader stdout io.Writer From 0a495bd7defdfb54eea277e0315f9c344875ae3a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 21 Oct 2026 22:51:49 -0600 Subject: [PATCH 04/10] fix(client): ask for diagnostics when Windows rejects a provider launch --- pkg/client/clientimplementation/workspace_client.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/client/clientimplementation/workspace_client.go b/pkg/client/clientimplementation/workspace_client.go index 5c0a88750a..69cf56da6e 100644 --- a/pkg/client/clientimplementation/workspace_client.go +++ b/pkg/client/clientimplementation/workspace_client.go @@ -896,7 +896,8 @@ func flushCommandOutput(stdout, stderr *subprocess.StreamingRedactingWriter) err const invalidProcessParameterHint = "Windows rejected starting the provider executable " + "(ERROR_INVALID_PARAMETER) before it ran, so this is a local process-launch failure " + - "rather than a remote host error; the cause is unconfirmed" + "rather than a remote host error; the cause is unconfirmed, so please report the Windows " + + "version and whether running the executable directly from a terminal works" func providerCommandError(command string, environ []string, err error) error { action := "" From 9854510404349b6e030c5bf4561b2cbed64e76ee Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 22 Oct 2026 01:12:15 -0600 Subject: [PATCH 05/10] fix(shell): decide spaced executable joining when the command runs --- pkg/shell/executable_path_test.go | 17 +++++++++ pkg/shell/shell.go | 62 ++++++++++++++++++++----------- 2 files changed, 57 insertions(+), 22 deletions(-) diff --git a/pkg/shell/executable_path_test.go b/pkg/shell/executable_path_test.go index 97fda3c8ca..5ba6746f1c 100644 --- a/pkg/shell/executable_path_test.go +++ b/pkg/shell/executable_path_test.go @@ -86,6 +86,23 @@ func (s *ExecutablePathTestSuite) TestCommandVariableWithArgumentsStillSplits() s.Equal("hello world\n", output) } +func (s *ExecutablePathTestSuite) TestReassignedCommandVariableStillSplits() { + output, err := s.run("CMD='echo hello'; $CMD world", "CMD="+s.executable) + + s.Require().NoError(err) + s.Equal("hello world\n", output) +} + +func (s *ExecutablePathTestSuite) TestCommandVariableAssignedInScriptIsNotSplit() { + output, err := s.run( + "PROVIDER='"+s.executable+"'; ${PROVIDER} init", + helperProcessEnv+"=1", + ) + + s.Require().NoError(err) + s.Equal("helper ran with init", output) +} + func (s *ExecutablePathTestSuite) TestVariableArgumentWithSpacesStillSplits() { output, err := s.run("echo ${ARGS}", "ARGS=a b") diff --git a/pkg/shell/shell.go b/pkg/shell/shell.go index 005e5ed999..605c4e06f3 100644 --- a/pkg/shell/shell.go +++ b/pkg/shell/shell.go @@ -10,6 +10,7 @@ import ( "os/user" "path/filepath" "runtime" + "slices" "strings" "time" @@ -46,14 +47,13 @@ func RunEmulatedShell( return err } - quoteExecutablePaths(parsed, expand.ListEnviron(runner.Environ...)) - r, err := interp.New(buildRunnerOptions(shellRunnerOptions{ - stdin: runner.Stdin, - stdout: runner.Stdout, - stderr: runner.Stderr, - env: runner.Environ, - dir: dir, + stdin: runner.Stdin, + stdout: runner.Stdout, + stderr: runner.Stderr, + env: runner.Environ, + dir: dir, + commandVars: commandVariables(parsed), })...) if err != nil { return fmt.Errorf("create shell runner: %w", err) @@ -72,12 +72,11 @@ func RunEmulatedShell( return nil } -// quoteExecutablePaths stops a command name such as ${PROVIDER_BINARY} from being -// split on whitespace when it expands to one existing file, e.g. a binary under -// C:\Users\Jane Doe. Values that are not a single file keep shell word splitting, -// so a variable like DOCKER_PATH="sudo docker" still expands to a command and arguments. -func quoteExecutablePaths(file *syntax.File, env expand.Environ) { +// commandVariables returns the variables used alone as a command name, such as +// PROVIDER_BINARY in "${PROVIDER_BINARY} init". +func commandVariables(file *syntax.File) []string { printer := syntax.NewPrinter() + var names []string syntax.Walk(file, func(node syntax.Node) bool { call, ok := node.(*syntax.CallExpr) if !ok || len(call.Args) == 0 { @@ -89,14 +88,12 @@ func quoteExecutablePaths(file *syntax.File, env expand.Environ) { return true } exp, ok := word.Parts[0].(*syntax.ParamExp) - if !ok || exp.Param == nil || !isPlainExpansion(printer, word, exp.Param.Value) { - return true - } - if isExistingFile(env.Get(exp.Param.Value).String()) { - word.Parts[0] = &syntax.DblQuoted{Parts: []syntax.WordPart{exp}} + if ok && exp.Param != nil && isPlainExpansion(printer, word, exp.Param.Value) { + names = append(names, exp.Param.Value) } return true }) + return names } func isPlainExpansion(printer *syntax.Printer, word *syntax.Word, name string) bool { @@ -107,6 +104,25 @@ func isPlainExpansion(printer *syntax.Printer, word *syntax.Word, name string) b return sb.String() == "$"+name || sb.String() == "${"+name+"}" } +// joinExecutablePath undoes word splitting of a command variable that holds one existing +// file, e.g. a binary under C:\Users\Jane Doe. The value is read when the command runs, so a +// variable reassigned to a command with arguments, like DOCKER_PATH="sudo docker", keeps +// shell word splitting. +func joinExecutablePath(args []string, env expand.Environ, names []string) []string { + for _, name := range names { + value := env.Get(name).String() + if !isExistingFile(value) { + continue + } + + fields := strings.Fields(value) + if len(args) >= len(fields) && slices.Equal(args[:len(fields)], fields) { + return append([]string{value}, args[len(fields):]...) + } + } + return args +} + func isExistingFile(path string) bool { if !strings.ContainsAny(path, " \t\n") { return false @@ -116,11 +132,12 @@ func isExistingFile(path string) bool { } type shellRunnerOptions struct { - stdin io.Reader - stdout io.Writer - stderr io.Writer - env []string - dir string + stdin io.Reader + stdout io.Writer + stderr io.Writer + env []string + dir string + commandVars []string } func buildRunnerOptions(p shellRunnerOptions) []interp.RunnerOption { @@ -141,6 +158,7 @@ func buildRunnerOptions(p shellRunnerOptions) []interp.RunnerOption { // command to an absolute path so IsBuiltin returns false and the // real binary is executed via the exec handler. interp.CallHandler(func(ctx context.Context, args []string) ([]string, error) { + args = joinExecutablePath(args, interp.HandlerCtx(ctx).Env, p.commandVars) if args[0] == "kill" { args[0] = "/bin/kill" } From ebe36763faac35c2d25a536e935607f0e354d40f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 22 Oct 2026 03:51:06 -0600 Subject: [PATCH 06/10] fix(shell): tie spaced executable joining to the command that used the variable --- pkg/shell/executable_path_test.go | 30 +++++++++++++++++++++---- pkg/shell/shell.go | 37 ++++++++++++++++--------------- 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/pkg/shell/executable_path_test.go b/pkg/shell/executable_path_test.go index 5ba6746f1c..366087ad03 100644 --- a/pkg/shell/executable_path_test.go +++ b/pkg/shell/executable_path_test.go @@ -43,18 +43,22 @@ func (s *ExecutablePathTestSuite) SetupTest() { } s.executable = filepath.Join(dir, name) + s.copyTestBinary(s.executable) +} + +func (s *ExecutablePathTestSuite) copyTestBinary(target string) { self, err := os.Executable() s.Require().NoError(err) source, err := os.Open(self) //nolint:gosec // copies this test binary s.Require().NoError(err) defer func() { _ = source.Close() }() - target, err := os.OpenFile( //nolint:gosec // the copy must be executable - s.executable, os.O_CREATE|os.O_WRONLY, 0o700, + file, err := os.OpenFile( //nolint:gosec // the copy must be executable + target, os.O_CREATE|os.O_WRONLY, 0o700, ) s.Require().NoError(err) - _, err = io.Copy(target, source) + _, err = io.Copy(file, source) s.Require().NoError(err) - s.Require().NoError(target.Close()) + s.Require().NoError(file.Close()) } func (s *ExecutablePathTestSuite) TestCommandVariableWithSpacesInPathIsNotSplit() { @@ -103,6 +107,24 @@ func (s *ExecutablePathTestSuite) TestCommandVariableAssignedInScriptIsNotSplit( s.Equal("helper ran with init", output) } +func (s *ExecutablePathTestSuite) TestLiteralCommandIsNotJoinedWithUnrelatedVariable() { + dir := s.T().TempDir() + ext := filepath.Ext(s.executable) + tool := filepath.Join(dir, "tool"+ext) + spacedTool := filepath.Join(dir, "tool status"+ext) + s.copyTestBinary(tool) + s.copyTestBinary(spacedTool) + + output, err := s.run( + "'"+tool+"' status; $CMD", + "CMD="+spacedTool, + helperProcessEnv+"=1", + ) + + s.Require().NoError(err) + s.Equal("helper ran with status"+"helper ran with "+spacedTool, output) +} + func (s *ExecutablePathTestSuite) TestVariableArgumentWithSpacesStillSplits() { output, err := s.run("echo ${ARGS}", "ARGS=a b") diff --git a/pkg/shell/shell.go b/pkg/shell/shell.go index 605c4e06f3..92cd1d9421 100644 --- a/pkg/shell/shell.go +++ b/pkg/shell/shell.go @@ -72,11 +72,11 @@ func RunEmulatedShell( return nil } -// commandVariables returns the variables used alone as a command name, such as -// PROVIDER_BINARY in "${PROVIDER_BINARY} init". -func commandVariables(file *syntax.File) []string { +// commandVariables maps the position of each command whose name is a lone variable, +// such as "${PROVIDER_BINARY} init", to that variable's name. +func commandVariables(file *syntax.File) map[syntax.Pos]string { printer := syntax.NewPrinter() - var names []string + names := map[syntax.Pos]string{} syntax.Walk(file, func(node syntax.Node) bool { call, ok := node.(*syntax.CallExpr) if !ok || len(call.Args) == 0 { @@ -89,7 +89,7 @@ func commandVariables(file *syntax.File) []string { } exp, ok := word.Parts[0].(*syntax.ParamExp) if ok && exp.Param != nil && isPlainExpansion(printer, word, exp.Param.Value) { - names = append(names, exp.Param.Value) + names[word.Pos()] = exp.Param.Value } return true }) @@ -108,19 +108,17 @@ func isPlainExpansion(printer *syntax.Printer, word *syntax.Word, name string) b // file, e.g. a binary under C:\Users\Jane Doe. The value is read when the command runs, so a // variable reassigned to a command with arguments, like DOCKER_PATH="sudo docker", keeps // shell word splitting. -func joinExecutablePath(args []string, env expand.Environ, names []string) []string { - for _, name := range names { - value := env.Get(name).String() - if !isExistingFile(value) { - continue - } +func joinExecutablePath(args []string, env expand.Environ, name string) []string { + value := env.Get(name).String() + if !isExistingFile(value) { + return args + } - fields := strings.Fields(value) - if len(args) >= len(fields) && slices.Equal(args[:len(fields)], fields) { - return append([]string{value}, args[len(fields):]...) - } + fields := strings.Fields(value) + if len(args) < len(fields) || !slices.Equal(args[:len(fields)], fields) { + return args } - return args + return append([]string{value}, args[len(fields):]...) } func isExistingFile(path string) bool { @@ -137,7 +135,7 @@ type shellRunnerOptions struct { stderr io.Writer env []string dir string - commandVars []string + commandVars map[syntax.Pos]string } func buildRunnerOptions(p shellRunnerOptions) []interp.RunnerOption { @@ -158,7 +156,10 @@ func buildRunnerOptions(p shellRunnerOptions) []interp.RunnerOption { // command to an absolute path so IsBuiltin returns false and the // real binary is executed via the exec handler. interp.CallHandler(func(ctx context.Context, args []string) ([]string, error) { - args = joinExecutablePath(args, interp.HandlerCtx(ctx).Env, p.commandVars) + hc := interp.HandlerCtx(ctx) + if name, ok := p.commandVars[hc.Pos]; ok { + args = joinExecutablePath(args, hc.Env, name) + } if args[0] == "kill" { args[0] = "/bin/kill" } From 50518eaa871a34b8fbc4c67ad02d0c69f21a027c Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 22 Oct 2026 03:57:54 -0600 Subject: [PATCH 07/10] test(shell): order suite helpers after tests --- pkg/shell/executable_path_test.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/shell/executable_path_test.go b/pkg/shell/executable_path_test.go index 366087ad03..ee631fce66 100644 --- a/pkg/shell/executable_path_test.go +++ b/pkg/shell/executable_path_test.go @@ -46,21 +46,6 @@ func (s *ExecutablePathTestSuite) SetupTest() { s.copyTestBinary(s.executable) } -func (s *ExecutablePathTestSuite) copyTestBinary(target string) { - self, err := os.Executable() - s.Require().NoError(err) - source, err := os.Open(self) //nolint:gosec // copies this test binary - s.Require().NoError(err) - defer func() { _ = source.Close() }() - file, err := os.OpenFile( //nolint:gosec // the copy must be executable - target, os.O_CREATE|os.O_WRONLY, 0o700, - ) - s.Require().NoError(err) - _, err = io.Copy(file, source) - s.Require().NoError(err) - s.Require().NoError(file.Close()) -} - func (s *ExecutablePathTestSuite) TestCommandVariableWithSpacesInPathIsNotSplit() { output, err := s.run( "${SSH_PROVIDER} init", @@ -142,3 +127,18 @@ func (s *ExecutablePathTestSuite) run(command string, environ ...string) (string }) return stdout.String(), err } + +func (s *ExecutablePathTestSuite) copyTestBinary(target string) { + self, err := os.Executable() + s.Require().NoError(err) + source, err := os.Open(self) //nolint:gosec // copies this test binary + s.Require().NoError(err) + defer func() { _ = source.Close() }() + file, err := os.OpenFile( //nolint:gosec // the copy must be executable + target, os.O_CREATE|os.O_WRONLY, 0o700, + ) + s.Require().NoError(err) + _, err = io.Copy(file, source) + s.Require().NoError(err) + s.Require().NoError(file.Close()) +} From 666b9a54abb97552ab8a9cb3fafac906ce43b83a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 22 Oct 2026 06:20:15 -0600 Subject: [PATCH 08/10] fix(shell): resolve relative command variables against the shell directory --- pkg/shell/executable_path_test.go | 37 +++++++++++++++++++++++++++++++ pkg/shell/shell.go | 11 +++++---- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/pkg/shell/executable_path_test.go b/pkg/shell/executable_path_test.go index ee631fce66..6b48a5f42b 100644 --- a/pkg/shell/executable_path_test.go +++ b/pkg/shell/executable_path_test.go @@ -110,6 +110,43 @@ func (s *ExecutablePathTestSuite) TestLiteralCommandIsNotJoinedWithUnrelatedVari s.Equal("helper ran with status"+"helper ran with "+spacedTool, output) } +func (s *ExecutablePathTestSuite) TestRelativePathIsResolvedAgainstShellDirectory() { + shellDir := s.T().TempDir() + relative := filepath.Join("spaced dir", filepath.Base(s.executable)) + s.Require().NoError(os.MkdirAll(filepath.Join(shellDir, "spaced dir"), 0o750)) + s.copyTestBinary(filepath.Join(shellDir, relative)) + + output, err := s.run( + "cd '"+shellDir+"'; ${PROVIDER} init", + "PROVIDER="+relative, + helperProcessEnv+"=1", + ) + + s.Require().NoError(err) + s.Equal("helper ran with init", output) +} + +func (s *ExecutablePathTestSuite) TestRelativePathExistingOnlyInProcessDirectoryIsNotJoined() { + processDir := s.T().TempDir() + shellDir := s.T().TempDir() + name := "spaced" + filepath.Ext(s.executable) + relative := "." + string(filepath.Separator) + name + " dir" + + string(filepath.Separator) + "provider" + s.Require().NoError(os.MkdirAll(filepath.Join(processDir, name+" dir"), 0o750)) + s.copyTestBinary(filepath.Join(processDir, name+" dir", "provider")) + s.copyTestBinary(filepath.Join(shellDir, name)) + s.T().Chdir(processDir) + + output, err := s.run( + "cd '"+shellDir+"'; ${PROVIDER} init", + "PROVIDER="+relative, + helperProcessEnv+"=1", + ) + + s.Require().NoError(err) + s.Equal("helper ran with init", output) +} + func (s *ExecutablePathTestSuite) TestVariableArgumentWithSpacesStillSplits() { output, err := s.run("echo ${ARGS}", "ARGS=a b") diff --git a/pkg/shell/shell.go b/pkg/shell/shell.go index 92cd1d9421..c276cf6f98 100644 --- a/pkg/shell/shell.go +++ b/pkg/shell/shell.go @@ -108,9 +108,9 @@ func isPlainExpansion(printer *syntax.Printer, word *syntax.Word, name string) b // file, e.g. a binary under C:\Users\Jane Doe. The value is read when the command runs, so a // variable reassigned to a command with arguments, like DOCKER_PATH="sudo docker", keeps // shell word splitting. -func joinExecutablePath(args []string, env expand.Environ, name string) []string { +func joinExecutablePath(args []string, env expand.Environ, dir, name string) []string { value := env.Get(name).String() - if !isExistingFile(value) { + if !isExistingFile(dir, value) { return args } @@ -121,10 +121,13 @@ func joinExecutablePath(args []string, env expand.Environ, name string) []string return append([]string{value}, args[len(fields):]...) } -func isExistingFile(path string) bool { +func isExistingFile(dir, path string) bool { if !strings.ContainsAny(path, " \t\n") { return false } + if !filepath.IsAbs(path) { + path = filepath.Join(dir, path) + } info, err := os.Stat(path) return err == nil && !info.IsDir() } @@ -158,7 +161,7 @@ func buildRunnerOptions(p shellRunnerOptions) []interp.RunnerOption { interp.CallHandler(func(ctx context.Context, args []string) ([]string, error) { hc := interp.HandlerCtx(ctx) if name, ok := p.commandVars[hc.Pos]; ok { - args = joinExecutablePath(args, hc.Env, name) + args = joinExecutablePath(args, hc.Env, hc.Dir, name) } if args[0] == "kill" { args[0] = "/bin/kill" From 485ac34f9fed63d66652e1005f4dc7d3ae92d77d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 25 Oct 2026 09:16:43 -0600 Subject: [PATCH 09/10] fix(client): limit the Windows launch hint to process-start errors --- pkg/client/clientimplementation/process_windows.go | 8 +++++++- .../provider_command_error_windows_test.go | 10 ++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/pkg/client/clientimplementation/process_windows.go b/pkg/client/clientimplementation/process_windows.go index f317d4cd02..9761df382d 100644 --- a/pkg/client/clientimplementation/process_windows.go +++ b/pkg/client/clientimplementation/process_windows.go @@ -4,10 +4,16 @@ package clientimplementation import ( "errors" + "io/fs" "golang.org/x/sys/windows" ) +// isInvalidProcessParameter reports only process-creation failures, which os/exec wraps as +// "fork/exec"; the same errno from an output writer after the process started is not one. func isInvalidProcessParameter(err error) bool { - return errors.Is(err, windows.ERROR_INVALID_PARAMETER) + var pathErr *fs.PathError + return errors.As(err, &pathErr) && + pathErr.Op == "fork/exec" && + errors.Is(pathErr.Err, windows.ERROR_INVALID_PARAMETER) } diff --git a/pkg/client/clientimplementation/provider_command_error_windows_test.go b/pkg/client/clientimplementation/provider_command_error_windows_test.go index 40a4a3d8dd..175acd02c3 100644 --- a/pkg/client/clientimplementation/provider_command_error_windows_test.go +++ b/pkg/client/clientimplementation/provider_command_error_windows_test.go @@ -3,6 +3,7 @@ package clientimplementation import ( + "io/fs" "os" "os/exec" "syscall" @@ -28,3 +29,12 @@ func (s *ProviderCommandErrorTestSuite) TestRealInvalidParameterStartFailureGets s.Contains(err.Error(), invalidProcessParameterHint) s.Contains(err.Error(), "The parameter is incorrect") } + +func (s *ProviderCommandErrorTestSuite) TestInvalidParameterFromOutputWriterGetsNoHint() { + writeErr := &fs.PathError{Op: "write", Path: "stdout", Err: windows.ERROR_INVALID_PARAMETER} + + err := providerCommandError("provider", nil, writeErr) + + s.ErrorIs(err, windows.ERROR_INVALID_PARAMETER) + s.NotContains(err.Error(), invalidProcessParameterHint) +} From ee478508ccc9c562bd83bd8ddf104461ac62b38b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 25 Oct 2026 10:33:46 -0600 Subject: [PATCH 10/10] fix(shell): split command variables only on shell IFS characters --- pkg/shell/executable_path_test.go | 16 ++++++++++++++++ pkg/shell/shell.go | 8 +++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/pkg/shell/executable_path_test.go b/pkg/shell/executable_path_test.go index 6b48a5f42b..749799a661 100644 --- a/pkg/shell/executable_path_test.go +++ b/pkg/shell/executable_path_test.go @@ -147,6 +147,22 @@ func (s *ExecutablePathTestSuite) TestRelativePathExistingOnlyInProcessDirectory s.Equal("helper ran with init", output) } +func (s *ExecutablePathTestSuite) TestNonBreakingSpaceStaysInsideTheJoinedPath() { + dir := filepath.Join(s.T().TempDir(), "Jane\u00a0Doe and co") + s.Require().NoError(os.MkdirAll(dir, 0o750)) + executable := filepath.Join(dir, filepath.Base(s.executable)) + s.copyTestBinary(executable) + + output, err := s.run( + "${PROVIDER} init", + "PROVIDER="+executable, + helperProcessEnv+"=1", + ) + + s.Require().NoError(err) + s.Equal("helper ran with init", output) +} + func (s *ExecutablePathTestSuite) TestVariableArgumentWithSpacesStillSplits() { output, err := s.run("echo ${ARGS}", "ARGS=a b") diff --git a/pkg/shell/shell.go b/pkg/shell/shell.go index c276cf6f98..0ba0e07034 100644 --- a/pkg/shell/shell.go +++ b/pkg/shell/shell.go @@ -114,13 +114,19 @@ func joinExecutablePath(args []string, env expand.Environ, dir, name string) []s return args } - fields := strings.Fields(value) + fields := strings.FieldsFunc(value, isIFSDelimiter) if len(args) < len(fields) || !slices.Equal(args[:len(fields)], fields) { return args } return append([]string{value}, args[len(fields):]...) } +// isIFSDelimiter matches the default IFS, so non-ASCII whitespace stays inside a field +// exactly as the shell leaves it. +func isIFSDelimiter(r rune) bool { + return r == ' ' || r == '\t' || r == '\n' +} + func isExistingFile(dir, path string) bool { if !strings.ContainsAny(path, " \t\n") { return false