fix/batch-changes: validate files target paths in changeset hooks and executor

Author: cbrnrd

internal/batches/executor/run_steps.go

@@ -540,6 +540,16 @@ func createFilesToMount(tempDir string, step batcheslib.Step, stepContext *templ
 	// can mount them into the container.
 	filesToMount := make(map[string]*os.File, len(files))
 	for name, content := range files {
+		// Defense-in-depth: the mount target name is concatenated directly
+		// into a Docker `--mount type=bind,...,target=<name>` argument. A comma
+		// in the name would let an attacker break out of the mount spec and
+		// inject arbitrary source=/target= pairs (e.g. /var/run/docker.sock).
+		// Parse-time validation rejects this, but the executor receives
+		// pre-parsed steps via JSON and never re-runs that validation, so we
+		// re-check here at the sink.
+		if strings.Contains(name, ",") {
+			return nil, cleanup, errors.Newf("files target path %q contains invalid characters", name)
+		}
 		fp, err := os.CreateTemp(tempDir, "")
 		if err != nil {
 			return nil, cleanup, errors.Wrap(err, "creating temporary file")

internal/batches/executor/run_steps_test.go

@@ -5,9 +5,25 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
 	"github.com/sourcegraph/sourcegraph/lib/batches/template"
 )
 
+func TestCreateFilesToMount_RejectsCommaInTargetPath(t *testing.T) {
+	step := batcheslib.Step{
+		Files: map[string]string{
+			"/tmp/x,source=/var/run/docker.sock,target=/var/run/docker.sock": "IGNORED",
+		},
+	}
+
+	_, cleanup, err := createFilesToMount(t.TempDir(), step, &template.StepContext{})
+	if cleanup != nil {
+		cleanup()
+	}
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "contains invalid characters")
+}
+
 func TestRenderStepContainer(t *testing.T) {
 	t.Run("static image", func(t *testing.T) {
 		got, err := renderStepContainer("alpine:3", &template.StepContext{})

internal/batches/service/service_test.go

@@ -615,6 +615,31 @@ changesetTemplate:
 `,
 			expectedErr: errors.New("parsing batch spec: step 1 files target path contains invalid characters"),
 		},
+		{
+			name:         "hook files target path with comma",
+			batchSpecDir: tempDir,
+			rawSpec: `
+version: 3
+name: test-spec
+description: A test spec
+on:
+  - repository: github.com/sourcegraph/src-cli
+changesetTemplate:
+  title: Test Hook Files
+  body: Test hook files target path with comma
+  branch: test
+  commit:
+    message: Test
+changesetHooks:
+  onCIFailure:
+    steps:
+      - run: echo test
+        image: alpine:3
+        files:
+          "/tmp/x,source=/var/run/docker.sock,target=/var/run/docker.sock": "IGNORED"
+`,
+			expectedErr: errors.New("parsing batch spec: hooks.onCIFailure step 1 files target path contains invalid characters"),
+		},
 		{
 			name:         "mount path dot-dot traversal",
 			batchSpecDir: tempDir,

lib/batches/batch_spec.go

@@ -299,6 +299,13 @@ func validateHooks(spec *BatchSpec) error {
 					)))
 				}
 			}
+			for name := range step.Files {
+				if strings.Contains(name, invalidMountCharacters) {
+					errs = errors.Append(errs, NewValidationError(errors.Newf(
+						"hooks.%s step %d files target path contains invalid characters", event, i+1,
+					)))
+				}
+			}
 		}
 	}