Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

GuillaumeRx
Copy link

@GuillaumeRx GuillaumeRx commented Dec 7, 2022

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

@GuillaumeRx GuillaumeRx changed the title ignore stdio in execSync Ignore STDIO in execSync when running the post-install hook of truffle unbox Dec 7, 2022
@cds-amal
Copy link
Member

cds-amal commented Dec 7, 2022

Thanks, @GuillaumeRx!

@cds-amal cds-amal changed the title Ignore STDIO in execSync when running the post-install hook of truffle unbox Fix: truffle unbox Ignore STDIO in execSync when running the post-install hook Dec 7, 2022
cds-amal
cds-amal previously approved these changes Dec 7, 2022
Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

Tested locally and LGTM! Thanks, @GuillaumeRx!

We need another approval, and CI passing ofc.

Copy link
Contributor

@cliffoo cliffoo left a comment

Choose a reason for hiding this comment

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

Thanks @GuillaumeRx !

@@ -148,7 +148,7 @@ function installBoxDependencies({ hooks }: boxConfig, destination: string) {
const postUnpack = hooks["post-unpack"];

if (postUnpack.length === 0) return;
execSync(postUnpack, { cwd: destination });
execSync(postUnpack, { cwd: destination, stdio: "ignore" });
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Or use spawn instead of exec.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

sukanyaparashar
sukanyaparashar previously approved these changes Dec 7, 2022
Copy link
Contributor

@sukanyaparashar sukanyaparashar left a comment

Choose a reason for hiding this comment

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

Works locally! Thanks @GuillaumeRx.

@cds-amal cds-amal dismissed their stale review December 7, 2022 19:46

This implementation removes child process' stderr/stdout


spawnSync(postUnpack, {
cwd: destination,
shell: true,
Copy link
Author

Choose a reason for hiding this comment

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

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

spawnSync(postUnpack, {
cwd: destination,
shell: true,
stdio: ["ignore", process.stdout, process.stderr]
Copy link
Author

Choose a reason for hiding this comment

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

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.

@GuillaumeRx GuillaumeRx requested review from cliffoo and cds-amal and removed request for eggplantzzz and cliffoo December 8, 2022 11:50
@GuillaumeRx GuillaumeRx changed the title Fix: truffle unbox Ignore STDIO in execSync when running the post-install hook Fix: truffle unbox use spawnSync to run the post-install hook Dec 8, 2022
@cliffoo
Copy link
Contributor

cliffoo commented Dec 8, 2022

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 👍

@cds-amal cds-amal dismissed sukanyaparashar’s stale review December 9, 2022 11:31

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

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

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.


spawnSync(postUnpack, {
cwd: destination,
shell: true,
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link

OnlyOneJMJQ commented Mar 6, 2023

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants