Fix: truffle unbox use spawnSync to run the post-install hook

[GuillaumeRx]

PR description

This replace execSync in favor of spawnSync to run the post-install hook avoiding a ENOBUFS error due to Yarn 3 filling the 200 ko Buffer limit of the spawned process.

Testing instructions

• Run the truffle unbox metamask/snap-box command
• Observe that the command succeeds.

Demo

With a npm box:

_g_t_box-test.2022-12-08.13-29-36.mp4

With a yarn box

yarn.truffle.unbox.mp4

[cds-amal]

Thanks, @GuillaumeRx!

[cds-amal]

Tested locally and LGTM! Thanks, @GuillaumeRx!

We need another approval, and CI passing ofc.

[cliffoo]

Thanks @GuillaumeRx !

[cliffoo]

Would increasing maxBuffer be better? Ignoring stdio altogether hides warnings.

Or use spawn instead of exec.

[cds-amal]

Would prefer using spawn to get around the buffer issue. Thanks, @cliffoo

[GuillaumeRx]

Thanks for the review ! I'll push an update using spawn tomorrow 😃

[cds-amal]

Hey @GuillaumeRx, the move to spawn has to make sure we don't break other boxes that use complex commands as well as parsing hooks.post-unpack because spawn and execSync APIs describe a utility's command and its arguments differently.

[cliffoo]

Looks like parsing can be avoided by setting shell to true (gist)

[GuillaumeRx]

execSync will only output stderr to the parent process stderr. Should I replicate this behaviour with spawn ?

[sukanyaparashar]

Works locally! Thanks @GuillaumeRx.

This implementation removes child process' stderr/stdout

[GuillaumeRx]

Allow to run complex commands without parsing. ref: #5765 (comment)

[GuillaumeRx]

Ignore stdin and pipe stdout / stderr to parent process. I also piped stdout because - after looking at some boxes - most of them use --loglevel=error to elevate the logging to stderr.

execSync was previously left at its default config that didn't pipe stdout, now that it is with spawnSync the log level flag is not required anymore.

[cliffoo]

hhmvvsmmmmm so this prompted us to revisit the post-install hook workflow as a whole: the security implications of allowing box creators to execute arbitrary commands on the user end (e.g. rm <path-to-ur-favorite-file>) in a (sub)shell are grave. I think this PR should brew while we decide on a solution that thwarts malicious commands (with regex perhaps to define what's allowed).

And for metamask/snap-box specifically, yarn install --silent should get around this problem for now.

Thanks again @GuillaumeRx 👍

Team is revisiting the post-install hook workflow due to security concerns

[cds-amal]

Hi @GuillaumeRx,

We decided that since this PR doesn't affect the vulnerability issue, we could merge it. However, not sanitizing inputs when using spawnSync and shell is a new concern, e.g. expanding * and $ in the shell. To be honest, I'm unsure how to proceed and need some time to research this.

[cds-amal]

I'm concerned there's no sanitization checks on this arbitrary command as noted in the docs

If the shell option is enabled, do not pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution.

[OnlyOneJMJQ]

This is blocked by #5952

Update since we've discussed this internally: This PR can be merged once we restrict use of the post-install hook to only those boxes we control (those in the truffle-box repo). That way we can be sure no malicious code is being executed, but leave flexibility for our trusted first-party boxes.

Tracking this work in a GitHub project here.