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

Command run failed with error : spawn EINVAL #52554

Closed
GD-Hadi opened this issue Apr 16, 2024 · 14 comments
Closed

Command run failed with error : spawn EINVAL #52554

GD-Hadi opened this issue Apr 16, 2024 · 14 comments

Comments

@GD-Hadi
Copy link

GD-Hadi commented Apr 16, 2024

Version

v18.20.2

Platform

Microsoft Windows NT 10.0.19045.0 x64

Subsystem

No response

What steps will reproduce the bug?

  1. Download NodeJS Version v18.20.2, v20.12.2, v21.7.3
  2. Download Visual Studio Code
  3. Install Extension Pack SAP Fiori Extensions
  4. Generate a sample Project
  5. Use npm start
    Screenshot 2024-04-16 134718

How often does it reproduce? Is there a required condition?

everytime using npm start

What is the expected behavior? Why is that the expected behavior?

It should open the web browser with the started App

What do you see instead?

project1@0.0.1 start
fiori run --open "test/flpSandbox.html?sap-ui-xx-viewCache=false#project1-display"

Command run failed with error : spawn EINVAL

Additional information

Happened first time after updating the node version.
Seems like a new problem after the security patch.
After downgrade it works fine.
See also: Link

@RedYetiDev
Copy link
Member

Hi! This repo is mostly for issues regarding the NodeJS core, and we need further context/information to understand the problem.

If you have an issue with VSCode or Fiori, please report it to them.

Otherwise, please provide more context about the issue, such as stack traces and minimally reproducible examples.

Thank you!

@reduckted
Copy link

This is a problem with spawn and spawnSync on Windows.

Platform: Windows 10.0.19045 N/A Build 19045

Using Node.js v20.12.1:

> node
Welcome to Node.js v20.12.1.
Type ".help" for more information.
> child_process.spawnSync('npm.cmd', ['-v'])
{
  status: 0,
  signal: null,
  output: [ null, <Buffer 31 30 2e 35 2e 31 0a>, <Buffer > ],
  pid: 23324,
  stdout: <Buffer 31 30 2e 35 2e 31 0a>,
  stderr: <Buffer >
}

Using Node.js 20.12.2:

> node
Welcome to Node.js v20.12.2.
Type ".help" for more information.
> child_process.spawnSync('npm.cmd', ['-v'])
{
  error: Error: spawnSync npm.cmd EINVAL
      at Object.spawnSync (node:internal/child_process:1124:20)
      at Object.spawnSync (node:child_process:876:24)
      at REPL1:1:15
      at ContextifyScript.runInThisContext (node:vm:136:12)
      at REPLServer.defaultEval (node:repl:598:22)
      at bound (node:domain:432:15)
      at REPLServer.runBound [as eval] (node:domain:443:12)
      at REPLServer.onLine (node:repl:927:10)
      at REPLServer.emit (node:events:530:35)
      at REPLServer.emit (node:domain:488:12) {
    errno: -4071,
    code: 'EINVAL',
    syscall: 'spawnSync npm.cmd',
    path: 'npm.cmd',
    spawnargs: [ '-v' ]
  },
  status: null,
  signal: null,
  output: null,
  pid: 0,
  stdout: null,
  stderr: null
}

@cjihrig
Copy link
Contributor

cjihrig commented Apr 16, 2024

@reduckted
Copy link

Caused by 69ffc6d.

@bnoordhuis @RafaelGSS Now that spawning .cmd files is blocked, what is the alternative? Leaving the .cmd extension off doesn't work.

> node
Welcome to Node.js v20.12.2.
Type ".help" for more information.
> child_process.spawnSync('npm', ['-v'])
{
  error: Error: spawnSync npm ENOENT
      at Object.spawnSync (node:internal/child_process:1124:20)
      at Object.spawnSync (node:child_process:876:24)
      at REPL1:1:15
      at ContextifyScript.runInThisContext (node:vm:136:12)
      at REPLServer.defaultEval (node:repl:598:22)
      at bound (node:domain:432:15)
      at REPLServer.runBound [as eval] (node:domain:443:12)
      at REPLServer.onLine (node:repl:927:10)
      at REPLServer.emit (node:events:530:35)
      at REPLServer.emit (node:domain:488:12) {
    errno: -4058,
    code: 'ENOENT',
    syscall: 'spawnSync npm',
    path: 'npm',
    spawnargs: [ '-v' ]
  },
  status: null,
  signal: null,
  output: null,
  pid: 0,
  stdout: null,
  stderr: null
}

@RafaelGSS
Copy link
Member

@bnoordhuis @RafaelGSS Now that spawning .cmd files is blocked, what is the alternative? Leaving the .cmd extension off doesn't work.

Use { shell: true } when spawning if the input is sanitized. @bnoordhuis is there any other suggested way?

@reduckted
Copy link

Awesome, that solves the problem for me. Thanks for the quick response @RafaelGSS ❤️

@RedYetiDev
Copy link
Member

Thanks for the info! Sorry if I misunderstood the initial request!

@ecraig12345
Copy link

This seems like a very significant breaking behavior change to make in a patch version? My team observed this same issue from a different tool which spawns npm.cmd to run scripts on Windows, and it seems like it would break any tool using this approach unless they already use { shell: true }.

@RedYetiDev
Copy link
Member

RedYetiDev commented Apr 16, 2024

@ecraig12345, while the change maybe is a bit more than a patch, it has been released, so there's really nothing we can do about that specifically, but maybe we can make a sort-of note that this change occurred, so that users will understand how to continue using their projects, WDYT @RafaelGSS?

I know we have the blog posts, but TBH not many end users will read all the way through the blogs to figure out this one issue, as not everyone reads the security updates

Maybe a notice in the documentation?

NickGerleman added a commit to NickGerleman/yoga that referenced this issue Apr 17, 2024
Summary: Node made a breaking change in a security release for 18/20 where `spawn()` no longer loads `.cmd` files by default. nodejs/node#52554. Execute command in shell.

Differential Revision: D56230965
kpanot added a commit to AmadeusITGroup/otter that referenced this issue Apr 17, 2024
kpanot added a commit to AmadeusITGroup/otter that referenced this issue Apr 17, 2024
kpanot added a commit to AmadeusITGroup/otter that referenced this issue Apr 17, 2024
kpanot added a commit to AmadeusITGroup/otter that referenced this issue Apr 17, 2024
kpanot added a commit to AmadeusITGroup/otter that referenced this issue Apr 17, 2024
kpanot added a commit to AmadeusITGroup/otter that referenced this issue Apr 17, 2024
@RafaelGSS
Copy link
Member

RafaelGSS commented Apr 17, 2024

I know we have the blog posts, but TBH not many end users will read all the way through the blogs to figure out this one issue, as not everyone reads the security updates

Maybe a notice in the documentation?

I think we can update the security release blog post to provide alternatives, then I can comment in the commit link to point to the blog post

@RedYetiDev
Copy link
Member

Great!

@RafaelGSS
Copy link
Member

This seems like a very significant breaking behavior change to make in a patch version? My team observed this same issue from a different tool which spawns npm.cmd to run scripts on Windows, and it seems like it would break any tool using this approach unless they already use { shell: true }.

According to our policy. We can only perform a breaking change in a semver-patch release if it's a security vulnerability is found. That was the case.

We also provide a mechanism to revert this behaviour such as --security-revert=CVE-2024-27980. Note, that this will disable the security patch, which is not recommended.

@JoostK
Copy link
Contributor

JoostK commented Apr 18, 2024

This is quite an invasisve change and because of #52017 I cannot propagate the --security-revert to usages of spawn in libraries and/or subprocesses. Is there any approach to making this work?

@RafaelGSS
Copy link
Member

This is quite an invasisve change and because of #52017 I cannot propagate the --security-revert to usages of spawn in libraries and/or subprocesses. Is there any approach to making this work?

If you don't have control of the process to pass the --security-revert (on its child processes too), then there's no official workaround for that. The #52365 should solve it, but it hasn't been released yet. You can try monkey-patching .spawnSync/spawn to bind { shell: true } by default, but monkey-patching is not really a recommended option, it might work until new PRs arrises.

Please note, that this was a coordinated security release with other teams (Rust, PHP, ...) and per policy, we must fix it -- even if it breaks people, security is a must. We always attempt to create revert options (such as --security-revert, but sometimes it might not apply to all environments).

@nodejs nodejs locked as resolved and limited conversation to collaborators Apr 18, 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

7 participants