fix(inject): quote bootstrap inputs and preserve installed agents

.github/workflows/pr-ci.yml

@@ -26,6 +26,7 @@ jobs:
           filters: |
             go:
               - '**/*.go'
+              - 'pkg/inject/**'
               - 'go.mod'
               - 'go.sum'
               - '.goreleaser.yml'
@@ -121,6 +122,18 @@ jobs:
 
           printf '%s\n' "$packages" | xargs go test -race -short
 
+      - name: test agent bootstrap with BusyBox sh
+        if: runner.os == 'Linux'
+        shell: bash
+        run: |
+          set -euo pipefail
+          sudo apt-get update -qq
+          sudo apt-get install -y busybox-static
+          original_sh="$(readlink /bin/sh)"
+          trap 'sudo ln -sf "$original_sh" /bin/sh' EXIT
+          sudo ln -sf /bin/busybox /bin/sh
+          go test -race -count=1 -timeout 90s ./pkg/inject -run '^(TestBootstrapSuite|TestInjectScript_RealShell.*)$'
+
       - name: test E2E runtime helpers
         run: go test -race -count=1 ./e2e/framework/runenv/... ./e2e/framework/dockerwsl ./e2e/cmd/devsy-ci
 
@@ -216,6 +229,7 @@ jobs:
       matrix:
         include:
           - runner: ubuntu-latest
+            job_timeout_minutes: 45
           - runner: macos-latest
           - runner: windows-latest
             # Cold builds need room to save the Go cache after artifact uploads.
@@ -265,14 +279,28 @@ jobs:
           name: devsy-ci-podman-${{ steps.os.outputs.runner_os }}
           path: ${{ runner.temp }}/devsy-ci-podman*
 
+      - name: disk usage before Linux snapshot
+        if: runner.os == 'Linux'
+        run: |
+          df -h /tmp "$RUNNER_TEMP" "$GITHUB_WORKSPACE"
+          du -sh "$(go env GOCACHE)" "$(go env GOMODCACHE)"
+
       - uses: goreleaser/goreleaser-action@f06c13b6b1a9625abc9e6e439d9c05a8f2190e94 # v7
         with:
           distribution: goreleaser
           version: "~> v2"
-          args: build --id devsy-${{ steps.os.outputs.runner_os }} --snapshot
+          args: >-
+            build --id devsy-${{ steps.os.outputs.runner_os }} --snapshot
+            ${{ runner.os == 'Linux' && '--parallelism=1' || '' }}
         env:
           DEVSY_CLI_VERSION: v0.0.0
 
+      - name: disk usage after Linux snapshot
+        if: always() && runner.os == 'Linux'
+        run: |
+          df -h /tmp "$RUNNER_TEMP" "$GITHUB_WORKSPACE"
+          du -sh "$(go env GOCACHE)" "$(go env GOMODCACHE)"
+
       - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7
         with:
           name: devsy-${{ steps.os.outputs.runner_os }}

pkg/agent/delivery/exec_stream_cancellation_unix_test.go

@@ -129,27 +129,17 @@ func cancellationDeliveryExecutor(
 	t *testing.T, runtime driver.ArgvExecDriver, canceled chan<- error,
 ) func(context.Context, []string, driver.Streams) error {
 	return func(ctx context.Context, argv []string, streams driver.Streams) error {
+		err := runtime.CommandContainerArgv(ctx, "owned-delivery-test", argv, streams)
 		if streams.Stdin != nil {
-			callbackDone := make(chan struct{})
-			stop := context.AfterFunc(ctx, func() {
-				defer close(callbackDone)
+			if cause := context.Cause(ctx); cause != nil {
 				select {
-				case canceled <- context.Cause(ctx):
+				case canceled <- cause:
 				default:
 					t.Error("unexpected extra attempt cancellation")
 				}
-			})
-			defer func() {
-				if !stop() {
-					select {
-					case <-callbackDone:
-					case <-time.After(execStreamCancellationTestBound):
-						t.Error("attempt observer callback did not join")
-					}
-				}
-			}()
+			}
 		}
-		return runtime.CommandContainerArgv(ctx, "owned-delivery-test", argv, streams)
+		return err
 	}
 }
 

pkg/agent/inject.go

@@ -82,6 +82,11 @@ func (o *InjectOptions) Validate() error {
 	if o.Exec == nil {
 		return fmt.Errorf("exec function is required")
 	}
+	for _, value := range []string{o.RemoteAgentPath, o.RemoteVersion, o.DownloadURL, o.Command} {
+		if strings.ContainsRune(value, 0) {
+			return fmt.Errorf("agent injection parameter contains NUL")
+		}
+	}
 	return nil
 }
 
@@ -277,12 +282,16 @@ type injectConfig struct {
 }
 
 func injectAgent(ctx context.Context, cfg *injectConfig) error {
+	params, err := buildScriptParams(cfg)
+	if err != nil {
+		return err
+	}
 	buf := &bytes.Buffer{}
 	//nolint:staticcheck
 	wasExecuted, err := inject.Inject(ctx, inject.InjectOptions{
 		Exec:         cfg.opts.Exec,
 		LocalFile:    createBinaryLoader(ctx, cfg),
-		ScriptParams: buildScriptParams(cfg),
+		ScriptParams: params,
 		Stdin:        cfg.opts.Stdin,
 		Stdout:       cfg.opts.Stdout,
 		Stderr:       setupStderr(cfg.opts, buf),
@@ -312,16 +321,20 @@ func createBinaryLoader(ctx context.Context, cfg *injectConfig) func(bool) (io.R
 	}
 }
 
-func buildScriptParams(cfg *injectConfig) *inject.Params {
+func buildScriptParams(cfg *injectConfig) (*inject.Params, error) {
 	opts := cfg.opts
+	existsCheck, err := cfg.vc.buildExistsCheck(opts.RemoteAgentPath)
+	if err != nil {
+		return nil, err
+	}
 	return &inject.Params{
 		Command:             opts.Command,
 		AgentRemotePath:     opts.RemoteAgentPath,
 		DownloadURLs:        inject.NewDownloadURLs(opts.DownloadURL),
-		ExistsCheck:         cfg.vc.buildExistsCheck(opts.RemoteAgentPath),
+		ExistsCheck:         existsCheck,
 		PreferAgentDownload: *opts.PreferDownloadFromRemoteUrl,
 		ShouldChmodPath:     true,
-	}
+	}, nil
 }
 
 func handleInjectError(err error, wasExecuted bool, buf *bytes.Buffer) error {
@@ -394,12 +407,20 @@ func newVersionChecker(opts *InjectOptions) *versionChecker {
 	}
 }
 
-func (vc *versionChecker) buildExistsCheck(agentPath string) string {
+func (vc *versionChecker) buildExistsCheck(agentPath string) (string, error) {
+	quotedPath, err := inject.QuotePOSIXLiteral(agentPath)
+	if err != nil {
+		return "", err
+	}
 	if vc.skipCheck {
-		return fmt.Sprintf(`! [ -x "%s" ]`, agentPath)
+		return fmt.Sprintf(`! [ -x %s ]`, quotedPath), nil
 	}
-	return fmt.Sprintf(`! { [ -x "%s" ] && [ "$("%s" --version 2>/dev/null)" = "%s" ]; }`,
-		agentPath, agentPath, vc.remoteVersion)
+	quotedVersion, err := inject.QuotePOSIXLiteral(vc.remoteVersion)
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprintf(`! { [ -x %s ] && [ "$(%s --version 2>/dev/null)" = %s ]; }`,
+		quotedPath, quotedPath, quotedVersion), nil
 }
 
 // session encapsulates the state and resources for an injection session.
@@ -409,12 +430,16 @@ func (vc *versionChecker) detectRemoteAgentVersion(
 	agentPath string,
 ) (string, error) {
 	buf := &bytes.Buffer{}
-	versionCmd := fmt.Sprintf("%s --version", agentPath)
+	quotedPath, err := inject.QuotePOSIXLiteral(agentPath)
+	if err != nil {
+		return "", err
+	}
+	versionCmd := fmt.Sprintf("%s --version", quotedPath)
 
 	checkCtx, cancel := context.WithTimeout(ctx, versionCheckTimeout)
 	defer cancel()
-	log.Debugf("checking remote agent version: %s", versionCmd)
-	err := exec(checkCtx, versionCmd, nil, buf, io.Discard)
+	log.Debug("checking remote agent version")
+	err = exec(checkCtx, versionCmd, nil, buf, io.Discard)
 	if err != nil {
 		if errors.Is(checkCtx.Err(), context.DeadlineExceeded) && ctx.Err() == nil {
 			return "", fmt.Errorf(

pkg/agent/inject_literal_test.go

@@ -0,0 +1,95 @@
+//nolint:gosec // These regressions execute generated shell checks and a private executable fixture.
+package agent
+
+import (
+	"context"
+	"io"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/devsy-org/devsy/pkg/inject"
+	"github.com/stretchr/testify/require"
+)
+
+func TestVersionCheckerLiteralPathAndVersion(t *testing.T) {
+	if _, err := exec.LookPath("/bin/sh"); err != nil {
+		t.Skip("POSIX /bin/sh is unavailable")
+	}
+	root := t.TempDir()
+	agentPath := filepath.Join(root, "agent '\"\\\n雪 $(touch sentinel)`touch sentinel`")
+	version := "v1 ' \" \\ 雪 $(touch sentinel)`touch sentinel`"
+	literalVersion, err := inject.QuotePOSIXLiteral(version)
+	require.NoError(t, err)
+	require.NoError(
+		t,
+		os.WriteFile(agentPath, []byte("#!/bin/sh\nprintf '%s\\n' "+literalVersion+"\n"), 0o700),
+	)
+	vc := &versionChecker{remoteVersion: version}
+	predicate, err := vc.buildExistsCheck(agentPath)
+	require.NoError(t, err)
+	cmd := exec.CommandContext(
+		t.Context(),
+		"/bin/sh",
+		"-c",
+		"if "+predicate+"; then printf install; else printf reuse; fi",
+	)
+	cmd.Dir = root
+	output, err := cmd.CombinedOutput()
+	require.NoError(t, err, string(output))
+	require.Equal(t, "reuse", string(output))
+
+	run := func(ctx context.Context, code string, stdin io.Reader, stdout, stderr io.Writer) error {
+		command := exec.CommandContext(ctx, "/bin/sh", "-c", code)
+		command.Dir = root
+		command.Stdin, command.Stdout, command.Stderr = stdin, stdout, stderr
+		return command.Run()
+	}
+	detected, err := vc.detectRemoteAgentVersion(t.Context(), run, agentPath)
+	require.NoError(t, err)
+	require.Equal(t, version, detected)
+	_, err = os.Stat(filepath.Join(root, "sentinel"))
+	require.True(t, os.IsNotExist(err))
+
+	vc.remoteVersion = "mismatch"
+	predicate, err = vc.buildExistsCheck(agentPath)
+	require.NoError(t, err)
+	require.NoError(t, exec.CommandContext(t.Context(), "/bin/sh", "-c", predicate).Run())
+}
+
+func TestVersionCheckerRejectsNULBeforeExecution(t *testing.T) {
+	vc := &versionChecker{remoteVersion: "v1"}
+	_, err := vc.buildExistsCheck("bad\x00path")
+	require.Error(t, err)
+	vc.remoteVersion = "v1\x00bad"
+	_, err = vc.buildExistsCheck("/tmp/agent")
+	require.Error(t, err)
+	_, err = vc.detectRemoteAgentVersion(
+		t.Context(),
+		func(context.Context, string, io.Reader, io.Writer, io.Writer) error {
+			t.Fatal("must not execute an invalid path")
+			return nil
+		},
+		"bad\x00path",
+	)
+	require.Error(t, err)
+
+	for _, field := range []string{"path", "version", "url", "command"} {
+		opts := &InjectOptions{
+			Exec: func(context.Context, string, io.Reader, io.Writer, io.Writer) error { return nil },
+		}
+		switch field {
+		case "path":
+			opts.RemoteAgentPath = "bad\x00path"
+		case "version":
+			opts.RemoteVersion = "bad\x00version"
+		case "url":
+			opts.DownloadURL = "bad\x00url"
+		case "command":
+			opts.Command = "bad\x00command"
+		}
+		require.Error(t, opts.Validate(), strings.ToUpper(field))
+	}
+}