Skip to content

fix(platform): run commands as argv, not as strings handed to a shell - #567

Merged
David-Noble-at-work merged 1 commit into
developfrom
platform-argv
Aug 19, 2026
Merged

David-Noble-at-work merged 1 commit into
developfrom
platform-argv

Conversation

@David-Noble-at-work

Copy link
Copy Markdown
Contributor

Every command in pkg/platform was assembled by concatenation and handed to sudo -n bash -c, unquoted:

runShellCommand("apt-get install -y "+strings.Join(names, " "), true)   // true = sudo

ParsePURL applies no charset validation to a name and decodes the version through url.PathUnescape, so
%3B arrives as ;. A name carrying shell metacharacters ran as root.

The change

var runShellCommand = func(command string, sudo bool) Result   // before
var runCommand      = func(argv []string, sudo bool) Result    // after

The vector reaches the kernel as a vector. No shell parses it, nothing needs quoting, and no argument can
inject — a package name containing a semicolon is a package name containing a semicolon.

70 call sites: 64 mechanical, 5 by hand. The five are the ones that looked like they needed a shell and
didn't:

Site Why it looked shell-shaped Resolution
dpkg-query -W -f='${Version}' ${…} dpkg parses the format itself; the quotes only hid it from sh
rpm -q --queryformat '%{VERSION}' %{…} same, for rpm
brew install command built as a string to vary on --cask appends to an argv slice
launchctl list | grep -q a pipe strings.Contains over captured stdout
port installed | grep -q a pipe same

The two pipes are also more precise as Go: strings.Contains searches what we mean, where grep matched a
pattern.

What the wrapper keeps

Everything that concerns the child process rather than the shell: commandTimeout, the endless y\n stdin
that auto-confirms prompts a tool raises despite its non-interactive flags, DEBIAN_FRONTEND=noninteractive,
and sudo -n so elevation fails fast instead of prompting on a tty.

captureRefresh records argv rather than a command string; the three refresh tests compare with
slices.Equal.

Deliberately not done

No pkg/process consolidation. pkg/platform imports only pkg/iox from this module, and every command
uses context.Background(). Routing it through pkg/process.Runner would add a dependency on pkg/result and
pkg/status and thread a runner through the manager constructors — a layering decision, charted by
6.2 and 6.3, not a
mechanical rewrite.

No script tier. All 70 commands are argv-shaped, so 6.3's tier 2 has no consumer here. Building an unused
fallback would be speculative.

Verified

  • make check — exit 0, zero FAIL, 103 packages
  • make build-all, make vet-all — linux, darwin, windows. 46 of the conversions are in linux-tagged files
    that a darwin-only build never compiles
    , so the per-GOOS sweep is doing real work here.
  • Spot-checked the shapes a naive split would corrupt: "/etc/init.d/" + name stays one argument, service <name> start keeps the name in the middle, and flatpak install -y flathub keeps flathub as a fixed
    argument rather than a package.

Fixes #561.

Every command in this package was assembled by concatenation and handed to `sudo -n bash -c`, and nothing was
quoted. ParsePURL applies no charset validation to a package name and decodes the version through
url.PathUnescape, so %3B arrives as a semicolon. A name carrying shell metacharacters executed as root.

runShellCommand(command string, sudo bool) becomes runCommand(argv []string, sudo bool). The vector reaches the
kernel as a vector, so no shell parses it, nothing needs quoting, and no argument can inject: a package name
containing a semicolon is a package name containing a semicolon.

All seventy call sites converted -- sixty-four mechanically, five by hand, one already argv-shaped. The five are
the ones that looked like they needed a shell and did not:

dpkg-query -W -f='${Version}' and rpm -q --queryformat '%{VERSION}' carried single quotes whose only job was
hiding the format from a shell. dpkg and rpm parse those themselves, so as argv elements they need no quoting
at all.

brew's installRaw built its command as a string so it could vary on the cask flag; it now appends to an argv
slice.

launchctl list | grep -q and port installed | grep -q were pipes whose right-hand side was a substring test.
They are strings.Contains over captured stdout, which is also more precise than grep -- it searches what we
mean rather than what a pattern matches.

The wrapper keeps everything that concerns the child rather than the shell: commandTimeout, the endless y\n
stdin that auto-confirms prompts a tool raises despite its non-interactive flags, DEBIAN_FRONTEND, and sudo -n
so elevation fails fast rather than prompting on a tty.

captureRefresh records argv instead of a command string, and the three refresh tests compare with slices.Equal.

Not done here, deliberately. pkg/platform imports only pkg/iox from this module and every command uses
context.Background(); routing it through pkg/process.Runner would add a dependency on pkg/result and pkg/status
and thread a runner through the manager constructors. That is a layering decision, charted by 6.2 and 6.3, not
a mechanical rewrite. And no script tier is added, because all seventy commands are argv-shaped -- building an
unused fallback here would be speculative.

Verified with make check, and with build-all and vet-all across linux, darwin and windows: forty-six of the
conversions are in linux-tagged files that darwin alone never compiles.

Fixes #561.
@David-Noble-at-work David-Noble-at-work self-assigned this Aug 19, 2026
@David-Noble-at-work
David-Noble-at-work merged commit 7988c1d into develop Aug 19, 2026
7 of 8 checks passed
@David-Noble-at-work
David-Noble-at-work deleted the platform-argv branch August 19, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pkg/platform runs every command through sh -c, unquoted, some of it under sudo

1 participant