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

Get rid of cross-spawn dependency #578

Open
sindresorhus opened this issue Oct 27, 2023 · 14 comments
Open

Get rid of cross-spawn dependency #578

sindresorhus opened this issue Oct 27, 2023 · 14 comments

Comments

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 27, 2023

It's pretty unmaintained.

We do a lot to the shell, so would be simpler to do that too in execa ourselves.

Maybe some of the things it fixes are already fixed in Node.js too.

https://github.com/moxystudio/node-cross-spawn/blob/master/lib/parse.js

@ehmicky
Copy link
Collaborator

ehmicky commented Oct 28, 2023

I agree that it not being maintained is a problem. Some of the bug reports seem rather important and are not currently looked into.

Breaking down cross-spawn feature by feature (which are all fixing problems with Windows):

  1. Executing files that have a shabang (since that's a Unix concept). Workaround for users would be to specify the interpreter explicitly instead. This would not work if the script location relies on the PATH environment variable, as opposed to being a file path. It does so by using which on the command, then reading the first bytes of the file to detect the shabang.
  2. Allows the command to a file path using the Unix path syntax. It does so by using path.normalize(), after having run which on the command.
  3. Some issues with escaping (commands with spaces, etc.) although I am not completely sure whether this is fixed or not in the latest version of Node. cross-spawn seems to fix this by wrapping the command in an additional cmd.exe call and manually escaping the command and arguments, using cmd.exe-specific syntax.
  4. It supports PATHEXT even when the shell option is false.

I am feeling conflicted.

  • On one hand, I am concerned that some of those fixes might be important to some users and any change might break their usage of Execa.
  • On the other hand, the current logic seems rather intricate and apparently buggy (according to the current GitHub issues on that repository), and therefore might be a high burden to maintain ourselves.

What are your thoughts?

@sindresorhus
Copy link
Owner Author

Relevant: bcoe/awesome-cross-platform-nodejs#26

@sindresorhus
Copy link
Owner Author

Maybe we could convince Bun to fix some of the things mention here to force Node.js' hand 🤣

@sindresorhus
Copy link
Owner Author

sindresorhus commented Oct 29, 2023

What are your thoughts?

I think it's worth doing it. cross-spawn is already buggy and unmaintained. We would release it as a major version to reduce the potential impact of any regressions.

@ehmicky
Copy link
Collaborator

ehmicky commented Oct 29, 2023

Yes, that sounds good.

We would also need to add automated tests for the features covered by cross-spawn, although our current tests already cover some of it.

That's some major undertaking, so maybe we should start by just copy/pasting the code as is in an initial PR, and then start refactoring it. For example, some of it can be:

  • Trimmed, e.g. we do not want the ENOENT logic
  • Simplified
  • Fixed, i.e. by looking into the GitHub issues of cross-spawn

@sindresorhus
Copy link
Owner Author

That's some major undertaking, so maybe we should start by just copy/pasting the code as is in an initial PR, and then start refactoring it.

👍

@ehmicky
Copy link
Collaborator

ehmicky commented Aug 23, 2024

I have spent some time figuring out what node-cross-spawn precisely does, and what are the problems with node:child_process on Windows it is fixing. This issue is a list of those problems.

Shebangs

Shebangs are a Unix concept. Windows does not support them.

node-cross-spawn emulates shebang support on Windows by:

Arguments splitting

In Unix, a subprocess file and arguments are specified as a file string and an arguments array of strings to the underlying C syscall. In Windows, those are passed as a single string, even when no shell is involved. The caller must escape the file and arguments with Windows-specific quoting rules so they are correctly split.

Node.js fixes this with the windowsVerbatimArguments option. When false, it performs that quoting automatically, which is basically, for each file or argument:

  • surround with double quotes, if contains ", \, tab or space
  • backslash escape double quotes and (if followed by double quotes) backslashes

It does not escape all shell-specific characters, so it is only meant for arguments splitting, not full-on shell escaping. In particular, it does not prevent shell injection.

windowsVerbatimArguments is false by default. Is is always true when the shell option is true, which is good, since it allows users that execute cmd.exe manually to performs that quoting themselves.

node-cross-spawn does not do anything there: the Node.js behavior is already good.

Node modules binaries

On Windows, *.cmd and *.bat files require a shell (like cmd.exe) to be run. This also includes other file extensions, basically anything but *.exe or *.com.

OS binaries are often .exe/.com, e.g. node itself, git, etc. However, Node modules binaries (installed either globally or locally) are *.cmd/*.ps1/*.sh files.

This means that, to run Node modules binaries on Windows, one must use the shell: true option. That option wraps the command with cmd.exe /d /s /c "file arg...". It does not escape file and arg, and just joins the arguments with spaces.

This has a few problems:

  • It requires users to remember to set shell: true. Or shell: process.platform === 'win32' if they don't want to spawn Bash on Unix.
  • It requires users to know whether the binary's file extension, which they might not know. For example, Execa does not easily know that.
  • Using a shell leads to potential command injection. This means the user must escape any user-provided input, using cmd.exe-specific quoting rules (which are very different from what developers are used to).
  • Whitespaces and quotes must be escaped. This includes spaces in the file path, which is common on Windows due to C:\\Program Files.

node-cross-spawn fixes this by emulating shell: true when shell: false is used. In addition, it performs some full escaping of the file and arguments.

PATHEXT

Windows has an environment variable PATHEXT, which is the list of file extensions which can be omitted by users when executing files. However, Windows only supports it when using a shell.

node-cross-spawn emulates its support without shells by taking it into account when resolving the full path of the executable file.

windowsHide

The windowsHide is left as is by node-cross-spawn.

@sindresorhus
Copy link
Owner Author

Great summary 👍

@wtto00

This comment has been minimized.

@ehmicky

This comment has been minimized.

@wtto00

This comment was marked as off-topic.

@ehmicky

This comment was marked as off-topic.

@wtto00

This comment was marked as off-topic.

@humam-nameer-10p

This comment has been minimized.

Repository owner locked and limited conversation to collaborators Nov 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants