Repository navigation
pkg/platform runs every command through sh -c, unquoted, some of it under sudo #561
Description
Epic: #442
Every command in pkg/platform is assembled by string concatenation and handed to a shell. Nothing is quoted:
runShellCommand("apt-get install -y "+strings.Join(names, " "), true) // true = sudo
runShellCommand("apt-cache show "+name, false)
runShellCommand("dpkg-query -W "+name, false)
runShellCommand("apt-get remove -y "+strings.Join(names, " "), true)runShellCommand runs sudo -n bash -c <command> when its second argument is true, so a package name carrying
shell metacharacters executes as root.
The names are not validated
ParsePURL (pkg/platform/purl.go:86) takes the name verbatim. It splits on /, @, ? and #, rejects an
empty name, and applies no charset check:
if i := strings.LastIndexByte(rest, '/'); i >= 0 {
namespace = rest[:i]
name = rest[i+1:]
} else {
name = rest
}The version additionally passes through url.PathUnescape, so %3B decodes to ; — percent-encoding becomes
shell metacharacters on the way through.
Confirm reachability first. A name originating in a .star file is uninteresting: its author can already
run shell. A name arriving in a lore package manifest fetched from a registry is data, not code, and that is
the vector worth establishing or ruling out. Severity assumes it is reachable; lower it if tracing says
otherwise.
The fix is to stop using a shell, not to escape better
The instinct is to add a quoting function. That is the lesser half. Go's standard library deliberately ships no
shell quoter, because exec.Command(name, args...) runs the binary directly and there is nothing to escape:
exec.CommandContext(ctx, "sudo", "-n", "apt-get", "install", "-y", name)All 57 distinct command strings in pkg/platform are argv-able. Enumerated, only three even appear
otherwise:
| Command | Looks shell-dependent because | Actually |
|---|---|---|
dpkg-query -W -f='${Version}' |
${...} |
dpkg's own format syntax; the quotes exist only to hide it from the shell. As argv, -f=${Version} is a plain argument |
rpm -q --queryformat '%{VERSION}' |
%{...} |
rpm's format syntax, same story |
launchctl list | grep -q |
a genuine pipe | run launchctl list via argv and search the captured output in Go |
Two of the three need quoting because a shell is present. Remove the shell and the need vanishes with it.
Dropping the shell also removes a process per command and ends the platform layer's dependency on sh being on
PATH. runShellCommand's auto-confirm stdin stream and DEBIAN_FRONTEND=noninteractive still apply — those
concern the child process, not the shell.
The other half: shell.exec needs a quoter
plan.shell.exec(command=...) takes a command string an author assembles, and shell syntax is the point there —
foo | bar > baz cannot be expressed in argv. So authors need a safe way to interpolate a path or a name.
POSIX single-quoting is complete and trivial; there is no input it cannot represent:
func Quote(s string) string {
return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
}Single quotes preserve everything literally — backslashes, newlines, $, globs. Only ' needs care, handled by
closing, escaping, reopening. Ruby's Shellwords.escape is longer only because it tries to avoid quoting where
unnecessary, which is cosmetic.
Alternatives, for the record: mvdan.cc/sh/v3/syntax.Quote is best in class and dialect-aware but a sizable new
module dependency (mvdan.cc/sh is currently a CI tool only, not a module dependency);
al.essio.dev/pkg/shellescape is a small single-function package. github.com/google/shlex is already an
indirect dependency but only splits — it has no quoter.
Where the function lives is a decision, not a detail: exposed to Starlark as something like shell.quote(), it
wants a real home rather than an unexported helper.
On Windows this is an escaping bug and nothing else
shell.Provider.Exec runs sh -c on every platform (pkg/op/provider/shell/provider.go:55), so on Windows
it depends on Git-for-Windows' sh, the MSYS2 runtime. Quoting is sufficient there, and it is worth recording
why, because the obvious suspicion — that MSYS path translation is also involved — is wrong.
MSYS2 presents a POSIX tree in which drives mount under a distinct syntax, /c/foo for C:\foo. It does not
require that form: "Cygwin which provides the POSIX API will just forward the paths to the Windows API as is",
so ls C:/ works unconverted. Its automatic conversion applies only to "all the arguments that look like Unix
paths" when "calling native executables from the context of Cygwin" — not to shell operations. A redirect target
is resolved by sh itself and never reaches argument conversion.
What breaks is one step earlier: unquoted, sh consumes the backslashes as escapes, so C:\Users\RUNNER~1\...
becomes C:UsersRUNNER~1AppData..., a drive-relative path, before any filesystem layer sees it. Single quotes
preserve the backslashes, MSYS forwards the Windows path unchanged, and the write lands where intended. Forward
slashes work too, as belt-and-braces rather than a second required transform.
Sources: https://www.msys2.org/docs/filesystem-paths/ and
https://github.com/msys2/msys2/wiki/Porting/f87a222118b1008ebc166ad237f04edb759c8f4c
What surfaced it
test_shell_exec.star and test_source.star interpolate t.tmp(...) — an OS-native path, since t.tmp
returns filepath.Join(...) — straight into an sh -c string. On Windows the backslashes are eaten, the
redirect writes somewhere unintended, and the expectation reports the file missing. Both are in the windows
known-failure set, and both are fixed by quoting at the call site.
Activity
test: stop four fixtures asserting Unix-shaped assumptions (#562)
docs: specify command execution as two tiers (#565)