From 8870df67821dab58a0b0beb2e750480befe34964 Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Tue, 7 Jul 2026 13:25:18 -0400 Subject: [PATCH 1/3] fix/batch-changes: validate files target paths in changeset hooks and executor --- internal/batches/executor/run_steps.go | 10 +++++++++ internal/batches/executor/run_steps_test.go | 16 +++++++++++++ internal/batches/service/service_test.go | 25 +++++++++++++++++++++ lib/batches/batch_spec.go | 7 ++++++ 4 files changed, 58 insertions(+) diff --git a/internal/batches/executor/run_steps.go b/internal/batches/executor/run_steps.go index 67ccbfcb0b..543a2b591c 100644 --- a/internal/batches/executor/run_steps.go +++ b/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=` 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") diff --git a/internal/batches/executor/run_steps_test.go b/internal/batches/executor/run_steps_test.go index 7b8dc6e356..9af78eea0e 100644 --- a/internal/batches/executor/run_steps_test.go +++ b/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{}) diff --git a/internal/batches/service/service_test.go b/internal/batches/service/service_test.go index b341ab8aeb..619f7947c9 100644 --- a/internal/batches/service/service_test.go +++ b/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, diff --git a/lib/batches/batch_spec.go b/lib/batches/batch_spec.go index 30ef50d4a9..3f87f68e62 100644 --- a/lib/batches/batch_spec.go +++ b/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, + ))) + } + } } } From 996bcec902ba79a395c79eb3e48f1bb031c90d3f Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Thu, 9 Jul 2026 10:47:20 -0400 Subject: [PATCH 2/3] remove lib/ changes --- lib/batches/batch_spec.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/batches/batch_spec.go b/lib/batches/batch_spec.go index 3f87f68e62..30ef50d4a9 100644 --- a/lib/batches/batch_spec.go +++ b/lib/batches/batch_spec.go @@ -299,13 +299,6 @@ 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, - ))) - } - } } } From dfcb3367d7d2494824c05ab82a208a6722f43d1a Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Thu, 9 Jul 2026 11:23:15 -0400 Subject: [PATCH 3/3] fix/batch-changes: drop orphaned hook files parse test The hook-files parse-time validation lives in lib/batches (synced from the sourcegraph monorepo) and was intentionally removed from this PR in 996bcec. The service_test case for it remained, so ParseBatchSpec returned no error and the test nil-dereferenced err.Error(). The executor sink check in createFilesToMount (and its test) remains as src-cli's defense. --- internal/batches/service/service_test.go | 25 ------------------------ 1 file changed, 25 deletions(-) diff --git a/internal/batches/service/service_test.go b/internal/batches/service/service_test.go index 619f7947c9..b341ab8aeb 100644 --- a/internal/batches/service/service_test.go +++ b/internal/batches/service/service_test.go @@ -615,31 +615,6 @@ 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,