Execute with an argument list about script

[NightMachinery]

@bitfield commented on Sep 1, 2020, 9:59 PM GMT+4:30:

Okay, please supply a short test program that uses ExecWithSlice in the way you want to use it, and that can't easily be refactored to work with Exec by calling strings.Join. We'll hash out the design a bit, and then we can go do it!

(I am not sure what you expect by a test program, so sorry if this is not that)

/// 2>/dev/null ; exec gorun "$0" "$@"

package main

import (
	"log"
	. "github.com/bitfield/script"
	"os"
)

func main() {
	url := os.Args[1]
	out := os.Args[2]

	_, err := Exec("youtube-dl --no-playlist --prefer-ffmpeg --extract-audio --audio-format mp3 " + url + " --output " + out ).Stdout()
	if err != nil {
		log.Fatalln(err.Error())
	}
}

~/tmp
❯ scripttest.go https://overcast.fm/+C7LmyTtQ test1.mp3
[generic] +C7LmyTtQ: Requesting header
WARNING: Falling back on generic information extractor.
[generic] +C7LmyTtQ: Downloading webpage
[generic] +C7LmyTtQ: Extracting information
[download] Downloading playlist: How to Destroy Surveillance Capitalism — Podcast – Cory Doctorow's craphound.com
[generic] playlist How to Destroy Surveillance Capitalism — Podcast – Cory Doctorow's craphound.com: Collected 1 video ids (downloading 1 of them)
[download] Downloading video 1 of 1
[download] Destination: test1.mp3
[download] 100% of 26.53MiB in 01:20.90KiB/s ETA 00:00nown ETA
[ffmpeg] Post-process file test1.mp3 exists, skipping
[download] Finished downloading playlist: How to Destroy Surveillance Capitalism — Podcast – Cory Doctorow's craphound.com

~/tmp
❯ scripttest.go https://overcast.fm/+C7LmyTtQ "Alice's test \"HAPPY\".mp3"
2020/09/01 22:19:04 unbalanced quotes or backslashes in [youtube-dl --no-playlist --prefer-ffmpeg --extract-audio --audio-format mp3 https://overcast.fm/+C7LmyTtQ --output Alice's test "HAPPY".mp3]

The same program with ExecWithSlice (Though this name is too lengthy and I suggest just using Cmd for it):

/// 2>/dev/null ; exec gorun "$0" "$@"

package main

import (
	"log"
	. "github.com/bitfield/script"
	"os"
)

func main() {
	url := os.Args[1]
	out := os.Args[2]

	_, err := ExecWithSlice("youtube-dl", "--no-playlist", "--prefer-ffmpeg", "--extract-audio", "--audio-format", "mp3", url, "--output", out).Stdout()
	if err != nil {
		log.Fatalln(err.Error())
	}
}

As I said, I am not at all proficient with go, so there might be better design choices. I solved a variant of this problem in Python using its metaprogramming API, achieving easy, safe interpolation of variables.

[bitfield]

Hey @NightMachinary, thanks for the question!

Is it invoking a shell, or is it parsing the string itself?

It uses Go's standard library exec.Command to run the external command directly (not via a shell). Though you can, of course, exec a shell if you like and give it a command to run.

https://github.com/bitfield/script/blob/master/filters.go#L120

how can we use an explicit argument list and not a string?

Can you clarify the question slightly for me? Are you asking if there's a way to execute a command with a slice of arguments, instead of a space-separated string? If so, something like this might work:

args := []string{"hello", "world"}
p := script.Exec("echo " + strings.Join(args, " "))

It's usually more convenient to start with a string, which is why Exec takes a string instead of a slice. If you have a use case where it's more convenient to pass a slice, I'd like to hear about it. We could provide that as an alternative.

Generally, I think a major shortcoming of the readme is that it does not clarify the security considerations of using this library.

Fair enough. What do you think needs to be mentioned in the README about this? I'm happy to accept PRs or to make the updates myself.

[NightMachinery]

@bitfield

Can you clarify the question slightly for me? Are you asking if there's a way to execute a command with a slice of arguments, instead of a space-separated string? If so, something like this might work:

args := []string{"hello", "world"}
p := script.Exec("echo " + strings.Join(args, " "))

It's usually more convenient to start with a string, which is why Exec takes a string instead of a slice. If you have a use case where it's more convenient to pass a slice, I'd like to hear about it. We could provide that as an alternative.

Yes, having a slice is sometimes more convenient; Generally, whenever the command is going to use some dynamic variable, there would be a danger of that variable containing spaces. For example, consider a fictional program get-movie-rating which takes exactly one argument, the name of a movie. script.Exec("get-movie-rating " + movie_name) would fail if the name contains whitespace.

Fair enough. What do you think needs to be mentioned in the README about this? I'm happy to accept PRs or to make the updates myself.

There are a lot of injection attack vectors in the shell; E.g., you get a string from the user to grep through the logs, and the user uses ... ; do-evil. It's not exactly clear that this library is or is not vulnerable to these kinds of attacks. It seems to me that the only bit of shell magic you have included is this splitting the command by whitespace into separate args (which doesn't seem that dangerous), but I am not sure. Anyhow, mentioning that this is the only bit of shell magic and that the library is safe, will reassure the potential users.

[bitfield]

script.Exec("get-movie-rating " + movie_name) would fail if the name contains whitespace.

Would it? Can you give a more detailed example which shows this?

It's not exactly clear that this library is or is not vulnerable to these kinds of attacks.

Fair point. Why don't you add an Exec test case which demonstrates this, one way or the other (I'm pretty sure a semicolon would simply be passed to the exec'ed binary, in which case there's no risk, but of course you can exec the shell as a binary, in which case there would be, but that's hardly script's fault, I think you'll agree.

[NightMachinery]

@bitfield commented on Aug 31, 2020, 5:12 PM GMT+4:30:

Would it? Can you give a more detailed example which shows this?

I'm new to Go, so I don't know how to write a test case for it, but testing this manually is very easy. Here I use it with a script of mine that just echoes each argument back in a new line:

package main

import (
	. "github.com/bitfield/script"
)

func main() {
	test2 := "test 2"
	_, _ = Exec("brishzq.zsh arger hi 'test 1' " + test2 ).Stdout()
}

The result of running this:

arg: hi
arg: test 1
arg: test
arg: 2

As you see, test 2 has gotten broken into two separate arguments. You might think we could do Exec("brishzq.zsh arger hi 'test 1' '" + test2 + "'"), but then we would need to escape ' in test2. In fact, the program completely breaks if test2 should contain ':

package main

import (
	"log"
	. "github.com/bitfield/script"
)

func main() {
	test2 := "Alice's pet"
	_, err := Exec("brishzq.zsh arger hi 'test 1' " + test2 ).Stdout()
	if err != nil {
		log.Fatalln(err.Error())
	}
}

2020/08/31 19:52:05 unbalanced quotes or backslashes in [brishzq.zsh arger hi 'test 1' Alice's pet]

Process finished with exit code 1

So the current design can only handle constant input, and anything dynamic can easily break it.

[bitfield]

As you see, test 2 has gotten broken into two separate arguments

Right, as it would with the shell. If you run:

cat filename with spaces

you'll find the shell thinks you're trying to cat three separate files. You'd need to put quotes round the name if you want the shell to interpret it as a single argument, and it's exactly the same here.

In fact, the program completely breaks if test2 should contain ':

As does the shell. Because quotes can be used to protect spaces, unbalanced quotes are not valid input.

If your issue report is that script is vulnerable to ... ; do-evil-type attacks, are you now satisfied that it's not? If so, I'll document this in the README.

[NightMachinery]

@bitfield commented on Sep 1, 2020, 1:51 PM GMT+4:30:

Right, as it would with the shell. If you run:

cat filename with spaces

you'll find the shell thinks you're trying to cat three separate files. You'd need to put quotes round the name if you want the shell to interpret it as a single argument, and it's exactly the same here.

First of all, I hope we agree that the library is to be better than the shell in something because the shell beats it handsomely in the syntax part.

Other than that, the shell's quoting is different from this library, and some newer shells (e.g., zsh) don't even require quoting. When you do echo "$sth", you do NOT need to escape anything in sth. That "$sth" is shell syntax for insert variable here quoted. However, this library has no concept of variables, and only works with strings, so you'll need to manually escape the inserted variable.

In fact, the program completely breaks if test2 should contain ':

As does the shell. Because quotes can be used to protect spaces, unbalanced quotes are not valid input.

The shell does not break under this circumstance:

❯ a="Alice's pet said \"Hello\!\"" # The escaping here is simply to feed the data into the shell. If we read this same string from a file, we do not need to escape it.
❯ echo "$a"
Alice's pet said "Hello!"

If your issue report is that script is vulnerable to ... ; do-evil-type attacks, are you now satisfied that it's not? If so, I'll document this in the README.

Yes, I am now satisfied that that attack vector doesn't exist (to the best of my very rudimentary knowledge).

---

While ExecWithSlice is so critical that I can't use the library without it, a nice feature to have is to have short aliases for the methods. For example, script.E that just calls script.Exec. These help make the boilerplate more palatable.

[bitfield]

ExecWithSlice is so critical that I can't use the library without it

Okay, please supply a short test program that uses ExecWithSlice in the way you want to use it, and that can't easily be refactored to work with Exec by calling strings.Join. We'll hash out the design a bit, and then we can go do it!

[bitfield]

Yes, that's just the sort of thing I had in mind. I find a very helpful way to design features is to write the code that you'd like to write, using the feature as though it exists, and then once you're happy with that, implementing the feature is a lot easier!

I'm not sure that being able to supply a slice is really your problem here, I think interpolating values containing quotes is the problem, and using a slice is one potential solution, is that fair to say? If there were an easy way to solve your use case without adding ExecWithSlice, would you be happy with that?

[NightMachinery]

Yes, that's just the sort of thing I had in mind. I find a very helpful way to design features is to write the code that you'd like to write, using the feature as though it exists, and then once you're happy with that, implementing the feature is a lot easier!

I'm not sure that being able to supply a slice is really your problem here, I think interpolating values containing quotes is the problem, and using a slice is one potential solution, is that fair to say? If there were an easy way to solve your use case without adding ExecWithSlice, would you be happy with that?

Your assessment is right on the money; Interpolating variables is the problem, and any way to do that that works reliably is fine by me.

[bitfield]

Great! Here's what I suggest. Fork the repo and create a work branch for this issue. Add a test case to TestExec, based on your demo program, that fails because it tries to supply a command argument containing quotes. We can then decide how best to solve it, and the test case will demonstrate that we've succeeded!

(If you don't have the time or inclination to do this, that's absolutely fine; I will do it sooner or later. But it will be a lot sooner if you can do it.)

[lalloni]

I would also find extremely useful to have something like ExecWithSlice (hopefuly with a nicer name).

Interpolation of variables is not necessarily the only problem because the sliced version would essentially avoid trying to implement the arguments list splitting based on white spaces only (which creates this kind of problems).

Specifically, I'm trying to replace this code (which uses Mage's sh package):

out, err := sh.Output("go", "build", "-ldflags", "-X main.version="+version, "-o", filepath.Join(BuildDir, binary), filepath.Join(".", binary, "main.go"))

With something like this:

out, err := script.Exec("go", "build", "-ldflags", "-X main.version="+version, "-o", filepath.Join(BuildDir, binary), filepath.Join(".", binary, "main.go")).String()

Being able to specify the arguments as a slice saves us from dealing and worrying about quoting, shell syntax, etc.

That use case, using the suggested strings.Join() would look much worse because it would no only be more verbose but also the coder would need to be careful about what needs to be quoted and quote it appropriately, etc:

out, err := script.Exec("go build -ldflags '-X main.version="+version+"' -o '"+ filepath.Join(BuildDir, binary) +"' '" + filepath.Join(".", binary, "main.go")).String()

Which is not only harder to write but also to read!

I will gladly send a PR if this makes sense.

[bitfield]

I'm not sure I follow this, @lalloni. What code is it you want to write? The example you gave doesn't use a slice, but variadic arguments.

[lalloni]

Hi @bitfield! Well... you are right about the slice vs varargs... I joined late into this conversation and if you check the previous discussion it was already called "slice" while the example ExecWithSlice actually received varargs. Nevertheless I feel the varargs approach is the most ergonomic, that's why I naturally used that.

Generally speaking, I think the fact that variadic arguments are received as slices is also contributing to the terminology mix up.

One more data important data point for me, coming from abundant previous experience using other command execution APIs, is that receiving the command line as a string array/slice (and varargs variants to produce nicer API) is pretty ubiquitous. Kubernetes, Docker, os/exec, juju, cilium, etc.

I get that an alternative could be to use yet another library (like github.com/keegancsmith/shell) to render shell command strings before calling script... but that seems suboptimal since this design would mean to render data we may have on diverse structures into a shell syntax string (which can be tricky) to pass into shell.Exec for it to parse the shell syntax and re-split into the constituent parts to finally call exec... I think it would more direct if we could simply pass the strings to shell.Exec.

[lalloni]

BTW, as you can see on my PR #76 I went ahead and implemented this using ExecArgs(cmd string, args ...string) and I used that for the purposes I was needing.

I am now not so sure about the cmd-args separation. In many cases it got in the way, making the API slightly harder to use compared with how it would have been if having just ExecArgs(args ...string).

So possibly receiving only the variadic form would be better from a usage perspective.