fix/security: shell-escape attacker-controlled filenames in batch change templates

[cbrnrd]

Note: this issue was brought to our attention via a HackerOne report.

Problem

src-cli renders batch spec run: fields through Go's text/template, which performs no output escaping, and the join builtin is a plain strings.Join. The rendered string is written to a script file and executed under /bin/sh inside the batch-change container.

Several template variables expose raw filenames parsed from git diff output and from Sourcegraph search results:

• repository.search_result_paths
• steps.{modified,added,deleted,renamed}_files
• previous_step.{modified,added,deleted,renamed}_files
• step.{modified,added,deleted,renamed}_files

Because git permits filenames containing shell metacharacters (`, $, ;, |, &, >, <, newlines, …), an attacker who controls filenames in a target repository can inject arbitrary shell commands into the rendered script. The reproducer in the report uses the canonical gofmt -w ${{ join repository.search_result_paths " " }} example from the Batch Changes docs to exfiltrate SRC_ACCESS_TOKEN and tamper with the workspace.

This is a supply-chain attack vector: planting a maliciously named file in a public repo is enough to trigger code execution in any organization that runs a batch change matching it. The container has the workspace mounted read-write plus access to SRC_ACCESS_TOKEN and any env: secrets.

Solution

Shell-escape every filename-bearing value at the template FuncMap boundary using github.com/kballard/go-shellquote, so the rendered text is always safe to splat into /bin/sh regardless of what the underlying spec author wrote.

Changes in lib/batches/template/templating.go:

• Added a shellEscapeAll([]string) []string helper that runs shellquote.Join over each element. Elements without metacharacters pass through unmodified, so existing specs targeting benign filenames keep producing identical output.

• Applied it to the four *_files fields in both StepContext.ToFuncMap (covers step, previous_step, steps) and ChangesetTemplateContext.ToFuncMap (covers changeset templates).

• Pre-escaped each element of Repository.SearchResultPaths() so ${{ repository.search_result_paths }} and ${{ join repository.search_result_paths " " }} are both safe.

• Kept the slice shape ([]string) rather than collapsing into a pre-joined string, so ${{ join … " " }} and ${{ range … }} keep working unchanged for spec authors.

• Left stdout / stderr raw. They are routinely captured into outputs and reused as plain values (e.g. a filename written by echo); pre-quoting silently changes those semantics. Spec authors who splat stdio against untrusted data should opt in with the new shellquote_join template builtin:

  run: do-something ${{ shellquote_join previous_step.stdout }}

  This trade-off is documented inline next to the assignment.

The fix is purely at the template layer; the git diff parser in lib/batches/git/changes.go is intentionally left alone so unusual-but-legitimate filenames aren't silently dropped.

Verification Evidence

Regression test

Added lib/batches/template/templating_security_test.go (TestVULN91_NoShellInjectionFromFilenames). It feeds the report's exact PoC filenames

`id > /tmp/PWNED && cat /tmp/PWNED`.go
foo.go; echo INJECTED_$(whoami) > /tmp/PWNED2; #.go
with space.go
with'quote.go
with<newline>.go

through every previously vulnerable variable in both StepContext and ChangesetTemplateContext, and asserts that none of `id, $(, ; echo, > /tmp/PWNED, or PWNED2 appear outside a single-quoted shell region in the rendered output.

Regression test catches the unpatched code

Reverting just the shellEscapeAll(res.ChangedFiles.Modified) call to its original res.ChangedFiles.Modified and re-running:

--- FAIL: TestVULN91_NoShellInjectionFromFilenames/step_run_via_previous_step.modified_files
    rendered output contains unescaped shell metasequence "`id"
    rendered: gofmt -w main.go `id > /tmp/PWNED && cat /tmp/PWNED`.go foo.go; echo INJECTED_$(whoami) > /tmp/PWNED2; #.go …
    rendered output contains unescaped shell metasequence "$("
    rendered output contains unescaped shell metasequence "; echo"
    rendered output contains unescaped shell metasequence "> /tmp/PWNED"
    rendered output contains unescaped shell metasequence "PWNED2"

i.e. the test loudly reproduces the exact injection from the HackerOne report on unpatched code, and passes on the patched code.

Full test suite

$ go test ./...                      # src-cli module
ok      github.com/sourcegraph/src-cli/cmd/src
ok      github.com/sourcegraph/src-cli/internal/batches/docker
ok      github.com/sourcegraph/src-cli/internal/batches/executor
ok      github.com/sourcegraph/src-cli/internal/batches/service
…        (all packages pass)

$ go test ./...                      # lib/ module
ok      github.com/sourcegraph/sourcegraph/lib/batches/template
…        (all packages pass)

Pre-existing executor tests that exercise ${{ join previous_step.modified_files " " }} and ${{ previous_step.modified_files }} continue to render the same output for benign filenames (shellquote.Join("modified.txt") → modified.txt), so no batch spec authored against the documented examples sees a behavior change.

Test Plan

• New unit test TestVULN91_NoShellInjectionFromFilenames covers every affected variable on both StepContext and ChangesetTemplateContext.
• Verified the test fails against the pre-fix code path with the report's exact PoC strings.
• Full go test ./... passes for both modules.