fix/batch-changes: validate files target paths in changeset hooks and executor#1352
Merged
cbrnrd merged 4 commits intoJul 9, 2026
Conversation
keegancsmith
approved these changes
Jul 9, 2026
| ))) | ||
| } | ||
| } | ||
| for name := range step.Files { |
Member
There was a problem hiding this comment.
FYI the code here is vendored from our monorepo.
Contributor
Author
There was a problem hiding this comment.
gah right, my previous attempts to re-vendor this did not go so well, but I will split this off into a separate PR.
Contributor
Author
There was a problem hiding this comment.
Comment on lines
+543
to
+552
| // 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) | ||
| } |
Member
There was a problem hiding this comment.
ok so this would of failed which is what we want, but now we fail earlier? And that is better from both a UX perspective + extra defense incase the other check for some reason doesn't run?
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.
…-validation-bypass-can-reach-executor
cbrnrd
deleted the
carterbrainerd-vuln-104-changesethooks-files-validation-bypass-can-reach-executor
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
src-clirejects commas in top-levelsteps.filestarget paths because a comma breaks out of Docker--mountsyntax and lets an attacker inject arbitrarysource=/target=pairs (e.g. mounting/var/run/docker.sock). That same validation was missing forchangesetHooks.*.steps[].files, so a malicious v3 batch spec could smuggle a dangerousfilestarget path through a hook and reach the executor-side Docker mount sink.The executor (
src batch exec) makes this worse: it loads already-parsedStepstructs from JSON and never re-runsParseBatchSpec/validateHooks, so parse-time validation alone is a coordinator-only guard.Solution
filestarget paths invalidateHooks(covers bothonCIFailureandonMergeConflict).createFilesToMount, so the check holds regardless of how a step reaches the executor.Verification Evidence