Repository navigation
fix(platform): run commands as argv, not as strings handed to a shell - #567
Merged
Merged
Conversation
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.
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
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.
Every command in
pkg/platformwas assembled by concatenation and handed tosudo -n bash -c, unquoted:ParsePURLapplies no charset validation to a name and decodes the version throughurl.PathUnescape, so%3Barrives as;. A name carrying shell metacharacters ran as root.The change
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:
dpkg-query -W -f='${Version}'${…}shrpm -q --queryformat '%{VERSION}'%{…}brewinstall--casklaunchctl list | grep -qstrings.Containsover captured stdoutport installed | grep -qThe two pipes are also more precise as Go:
strings.Containssearches what we mean, wheregrepmatched apattern.
What the wrapper keeps
Everything that concerns the child process rather than the shell:
commandTimeout, the endlessy\nstdinthat auto-confirms prompts a tool raises despite its non-interactive flags,
DEBIAN_FRONTEND=noninteractive,and
sudo -nso elevation fails fast instead of prompting on a tty.captureRefreshrecords argv rather than a command string; the three refresh tests compare withslices.Equal.Deliberately not done
No
pkg/processconsolidation.pkg/platformimports onlypkg/ioxfrom this module, and every commanduses
context.Background(). Routing it throughpkg/process.Runnerwould add a dependency onpkg/resultandpkg/statusand thread a runner through the manager constructors — a layering decision, charted by6.2and6.3, not amechanical 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, zeroFAIL, 103 packagesmake build-all,make vet-all— linux, darwin, windows. 46 of the conversions are in linux-tagged filesthat a darwin-only build never compiles, so the per-GOOS sweep is doing real work here.
"/etc/init.d/" + namestays one argument,service <name> startkeeps the name in the middle, andflatpak install -y flathubkeepsflathubas a fixedargument rather than a package.
Fixes #561.