go-debos / fakemachine

fake a machine
Apache License 2.0
34 stars 37 forks source link

Shellescape command line arguments #174

Closed sjoerdsimons closed 10 months ago

sjoerdsimons commented 10 months ago

The various fakemachine Run* functions end up running the provided commands in a shell, so add escaping to the arguments

sjoerdsimons commented 10 months ago

As a note this does mean some change in behaviour impacting users that already were, in a way, working around fakemachine not escaping. This does include debos, for which i've got some WIP patches prepped to push out

obbardc commented 10 months ago

@sjoerdsimons This works really well for me and also fixes #32. We can close that ticket once merged or add Fixes: #32 into commit description. Your choice.

Without series:

$ go run cmd/fakemachine/main.go -- python -c 'import sys;print(sys.argv)' "foo bar"
Running python -c import sys;print(sys.argv) foo bar using kvm backend
/wrapper: 15: Syntax error: word unexpected (expecting ")")
Powering off.
fakemachine: open /tmp/fakemachine-3416548428/result: no such file or directory

With series:

$ go run cmd/fakemachine/main.go -- python -c 'import sys;print(sys.argv)' "foo bar"
go: downloading github.com/alessio/shellescape v1.4.2
Running python -c 'import sys;print(sys.argv)' 'foo bar' using kvm backend
['-c', 'foo bar']

Also, do you have an idea of what the required debos changes are? I'd like to see that before merging really.

sjoerdsimons commented 10 months ago

@sjoerdsimons This works really well for me and also fixes #32. We can close that ticket once merged or add Fixes: #32 into commit description. Your choice.

The commit description of the first patch has that fixes already :)

Without series:

$ go run cmd/fakemachine/main.go -- python -c 'import sys;print(sys.argv)' "foo bar"
Running python -c import sys;print(sys.argv) foo bar using kvm backend
/wrapper: 15: Syntax error: word unexpected (expecting ")")
Powering off.
fakemachine: open /tmp/fakemachine-3416548428/result: no such file or directory

With series:

$ go run cmd/fakemachine/main.go -- python -c 'import sys;print(sys.argv)' "foo bar"
go: downloading github.com/alessio/shellescape v1.4.2
Running python -c 'import sys;print(sys.argv)' 'foo bar' using kvm backend
['-c', 'foo bar']

Also, do you have an idea of what the required debos changes are? I'd like to see that before merging really.

Just threw an ugly WIP into the template-var-escaping branch on debos; specifically the relevant changes are: https://github.com/go-debos/debos/commit/51f49938d967ed254f9d97ddb644ddd34ce4633c#diff-d5992fa9aeed09f40fed1dafb87719ff649162b19c57bb3aeacc9da264fb9d84L293

which is because debos formattted the arguments with fmt.Sprintf("%s:\"%s\", k, v) or in other words a template var always came out as -t snake:"badger" mostly to support values with a space. However now that we do proper escaping the " will be escaped and thus trickle through unintentially.

Fwiw overall fixing this probably may break some users (both fakemachine and debos) as it's a behaviour change in this corner but there isn't really a way around that i'm afraid.

obbardc commented 10 months ago

@sjoerdsimons This works really well for me and also fixes #32. We can close that ticket once merged or add Fixes: #32 into commit description. Your choice.

The commit description of the first patch has that fixes already :)

Sorry, I missed that entirely. All good!

Also, do you have an idea of what the required debos changes are? I'd like to see that before merging really.

Just threw an ugly WIP into the template-var-escaping branch on debos; specifically the relevant changes are: go-debos/debos@51f4993#diff-d5992fa9aeed09f40fed1dafb87719ff649162b19c57bb3aeacc9da264fb9d84L293

which is because debos formattted the arguments with fmt.Sprintf("%s:\"%s\", k, v) or in other words a template var always came out as -t snake:"badger" mostly to support values with a space. However now that we do proper escaping the " will be escaped and thus trickle through unintentially.

Fwiw overall fixing this probably may break some users (both fakemachine and debos) as it's a behaviour change in this corner but there isn't really a way around that i'm afraid.

I'm fine with that! Let's get this merged.