Skip to content

Conversation

@LukeAbby
Copy link
Contributor

exec and execSync both document:

Never pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution.

While I'm not particularly worried about inputs being malicious (they are from the user provided paths) it's still a problem because shell metacharacters like *, ", ', (space), etc. would all need handling since they are legal in paths.

However this is a breaking change because formatCmd changed from a string to a string[]. If that's unacceptable I can instead do this:

  1. Add a deprecation warning for formatCwd: string.
  2. Run using execSync when formatCwd is a string and spawnSync otherwise.
  3. In a future major version remove formatCwd: string.

Or I could try to find a package that implements quoting of user input to the shell.

Copy link
Contributor Author

@LukeAbby LukeAbby left a comment

Choose a reason for hiding this comment

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

Looks like the windows failure is most likely due to the fact that I'd need to suffix pnpm with pnpm.cmd or something.

I think the better approach would be something like https://www.npmjs.com/package/cross-spawn though. It has the same API but fixes Node's deficiencies. Let me know what you think.

shell(`${formatter} ${updatedPaths.join(" ")}`)
const command = formatter[0]
const args = formatter.slice(1)
shell(command, [...args, "--", ...updatedPaths])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extra -- is to prevent a filename named like --help from being interpreted as a flag. Arguably needs to be applied elsewhere but it gets increasingly difficult elsewhere since shell commands rarely are prepared to actually deal with dashes at the beginning of filenames in arbitrary locations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To do

Development

Successfully merging this pull request may close these issues.

1 participant