Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid Wrapping Script Code In Quotes #134

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

hostilefork
Copy link
Contributor

The exec() command only runs a program with literal argument strings.
It does not know how to do things like expanding environment variables,
or piping/redirection. Hence the way to get "shell intelligence"
is to use exec() to run the sh process with the command line string
as a parameter.

But the method being used to do this was:

exec.exec(`sh -c \\"${script}"`)

This has problems, because wrapping the script in quotes creates
issues when the script itself contains quotes. Escaping the string
correctly is a non-trivial problem, which also would create "noise"
in the debug output which would add confusion.

http://mywiki.wooledge.org/BashFAQ/050

The easiest way to work around this here is to use the array form of
exec() to pass exactly two parameters to sh. No manipulation of the
script string is needed with this approach:

exec.exec("sh", ["-c", script])

There are more cases in the project of sh -c usage which should also
be changed, and likely abstracted (shellExec()?) But this small patch
just fixes the most important case for the user-provided script.


Additionally, this removes the escaped backslash (\\) from the
start of the executed command. That was presumably to suppress the
use of aliases:

https://unix.stackexchange.com/questions/524254/why-are-backslashes-included-in-this-shell-script

But because this is invoking a non-interactive shell session, aliases
would not apply. Hence the extra backslash shouldn't be needed.

The exec() command only runs a program with literal argument strings.
It does not know how to do things like expanding environment variables,
or piping/redirection.  Hence the way to get "shell intelligence"
is to use exec() to run the `sh` process with the command line string
as a parameter.

But the method being used to do this was:

    exec.exec(`sh -c \\"${script}"`)

This has problems, because wrapping the script in quotes creates
issues when the script itself contains quotes.  Escaping the string
correctly is a non-trivial problem, which also would create "noise"
in the debug output which would add confusion.

http://mywiki.wooledge.org/BashFAQ/050

The easiest way to work around this here is to use the array form of
exec() to pass exactly two parameters to `sh`.  No manipulation of the
script string is needed with this approach:

    exec.exec("sh", ["-c", script])

There are more cases in the project of `sh -c` usage which should also
be changed, and likely abstracted (shellExec()?)  But this small patch
just fixes the most important case for the user-provided script.

---

Additionally, this removes the escaped backslash (`\\`) from the
start of the executed command.  That was presumably to suppress the
use of aliases:

https://unix.stackexchange.com/questions/524254/why-are-backslashes-included-in-this-shell-script

But because this is invoking a non-interactive shell session, aliases
would not apply.  Hence the extra backslash shouldn't be needed.
Copy link
Member

@ychescale9 ychescale9 left a comment

Choose a reason for hiding this comment

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

Thanks!

@ychescale9 ychescale9 merged commit 0d47f08 into ReactiveCircus:main Mar 9, 2021
@hostilefork
Copy link
Contributor Author

hostilefork commented Mar 10, 2021

@ychescale9 I notice that GitHub implements the custom shell: option by just writing the fragment of Yaml out to a temporary file and then running that.

This could offer benefits of wrapping long lines with backslash, handling #comments, etc.

Is there a reason not to do this here? You get the feature of erroring out on any line with -e, so that the script will stop if any step fails. The settings GitHub uses are:

bash --noprofile --norc -eo pipefail {0}

Where {0} is substituted with the temporary file created.

Offering the option of specifying a shell: would be ideal, but the default having -e is important.

@ychescale9
Copy link
Member

@hostilefork Thanks for the input. I haven't looked into better backslash support, but if you can come up with something without breaking change feel free to send a PR:)

Thanks again for the contributions!

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.

2 participants