This repository was archived by the owner on Aug 30, 2024. It is now read-only.

coder/coder-v1-cliPublic archive

NotificationsYou must be signed in to change notification settings
Fork18
Star71

fix: Escape shell arguments#227

Merged

jawnsy merged 1 commit intomasterfromjawnsy/ch6190

Jan 25, 2021

Merged

fix: Escape shell arguments#227

jawnsy merged 1 commit intomasterfromjawnsy/ch6190

Jan 25, 2021

Conversation

Copy link

Contributor

jawnsy commentedJan 24, 2021•
edited
Loading

The commands passed to "coder sh" are passed as a single argument to "sh -c", so we need to shell-escape the command we pass. This change escapes spaces, backslash, and quotes.

I'm not 100% sure if this is a good idea, since it is a breaking change in case people have already been escaping their commands.

Note: this will need to be rebased once#226 is merged

Tested as follows:

[coder@jawnsy-m coder-cli]$go install ./cmd/coder[coder@jawnsy-m coder-cli]$which coder/home/coder/go/bin/coder[coder@jawnsy-m coder-cli]$coder sh jawnsy-m go run~/test.go 1 2"3 4"'"abc def" \\abc' 5 6"7 8 9"warning: version mismatch detected  | Coder CLI version: unknown  | Coder API version: 1.14.0+340-gd64a44cb3-20210123  |  | tip: download the appropriate version here: https://github.com/cdr/coder-cli/releasesargv[0] = /tmp/go-build627553793/b001/exe/testargv[1] = 1argv[2] = 2argv[3] = 3 4argv[4] = "abc def" \\abcargv[5] = 5argv[6] = 6argv[7] = 7 8 9[coder@jawnsy-m coder-cli]$go run~/test.go 1 2"3 4"'"abc def" \\abc' 5 6"7 8 9"argv[0] = /tmp/go-build909559834/b001/exe/testargv[1] = 1argv[2] = 2argv[3] = 3 4argv[4] = "abc def" \\abcargv[5] = 5argv[6] = 6argv[7] = 7 8 9

For comparison, this is whatsh -c andexec would look like:

[coder@jawnsy-m coder-cli]$sh -c"go run ~/test.go 1 2\"3 4\" '\"abc def\"\\abc' 5 6\"7 8 9\""argv[0] = /tmp/go-build754337722/b001/exe/testargv[1] = 1argv[2] = 2argv[3] = 3 4argv[4] = "abc def" \abcargv[5] = 5argv[6] = 6argv[7] = 7 8 9[coder@jawnsy-m coder-cli]$bash[coder@jawnsy-m coder-cli]$exec go run~/test.go 1 2"3 4"'"abc def" \\abc' 5 6"7 8 9"argv[0] = /tmp/go-build905411501/b001/exe/testargv[1] = 1argv[2] = 2argv[3] = 3 4argv[4] = "abc def" \\abcargv[5] = 5argv[6] = 6argv[7] = 7 8 9

And this was the previous behavior:

[coder@jawnsy-m coder-cli]$which coder/opt/coder/coder-cli/coder[coder@jawnsy-m coder-cli]$coder sh jawnsy-m go run~/test.go 1 2"3 4"'"abc def" \\abc' 5 6"7 8 9"warning: version mismatch detected  | Coder CLI version: v1.15.0-8-gae35620  | Coder API version: 1.14.0+340-gd64a44cb3-20210123  |  | tip: download the appropriate version here: https://github.com/cdr/coder-cli/releasesargv[0] = /tmp/go-build394408240/b001/exe/testargv[1] = 1argv[2] = 2argv[3] = 3argv[4] = 4argv[5] = abc defargv[6] = \abcargv[7] = 5argv[8] = 6argv[9] = 7argv[10] = 8argv[11] = 9

Copy link

shortcut-integrationbot commentedJan 24, 2021

This pull request has been linked toClubhouse Story #6190: Subtle bug(?) with coder sh handling of spaces.

jawnsy self-assigned this

Jan 24, 2021

jawnsy requested review fromcmoog,tychoish andkylecarbs

January 24, 2021 01:51

jawnsy force-pushed thejawnsy/ch6190 branch from259876f to761c30dCompare

January 24, 2021 01:53

tychoish approved these changes

Jan 25, 2021

View reviewed changes

internal/cmd/shell.go

		// $ coder sh go run ~/test.go 1 2 "3 4" '"abc def" \\abc' 5 6 "7 8 9"
		func shellEscape(arg string) string {
		r := strings.NewReplacer(`\`, `\\`, `"`, `\"`, `'`, `\'`, ` `, `\ `)
		return r.Replace(arg)

Copy link

tychoishJan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think eventually we should be relying on shlex (or similar) to be parsing and escaping shell commands from strings into arg arrays, and avoid rolling our own for things like this.

Copy link

ContributorAuthor

jawnsyJan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@tychoish Agree, I did a quick search bit didn't find any good ones. Happy to switch over now, or we can do it later, WDYT?

Copy link

tychoishJan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

https://github.com/google/shlex is functional.

I think this might also solve the %q question in the other PR

Copy link

ContributorAuthor

jawnsyJan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@tychoish Oh yeah, that looks much better. Looks like there were a few cases that I missed.

I don't think this will work for#226 because we need the shell to interpret the subshell calls for getent/etc, but I will give it a try

Copy link

ContributorAuthor

jawnsyJan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hmm it looks like shlex is for parsing shell expressions, how do I use it to escape things?

fix: Escape shell arguments

0df7ec5

The commands passed to "coder sh" are passed as a single argumentto "sh -c", so we need to shell-escape the command we pass.This change escapes spaces, backslash, and quotes.

jawnsy force-pushed thejawnsy/ch6190 branch from761c30d to0df7ec5Compare

January 25, 2021 15:47

jawnsy marked this pull request as ready for review

January 25, 2021 16:21