feat: Account for Node security patch

[kant2002]

As of https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2#command-injection-via-args-parameter-of-child_processspawn-without-shell-option-enabled-on-windows-cve-2024-27980---high Cordova produce unrecognized error on Windows.

Fixes: apache/cordova-cli#456

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.50%. Comparing base (eb0f002) to head (431497d).

Files with missing lines	Patch %	Lines
lib/check_reqs.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1778   +/-   ##
=======================================
  Coverage   72.50%   72.50%           
=======================================
  Files          23       23           
  Lines        1837     1837           
=======================================
  Hits         1332     1332           
  Misses        505      505           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[erisu]

Can you confirm if this change is valid?

The article you're linking is about Node.js spawn while the code appears to be using the npm package execa and the README for version 5.1.1 of this package does not to mention spawn as a valid option.

[erisu]

The article is talking about the shell option and the execa package does have this flag.

https://www.npmjs.com/package/execa/v/5.1.1#shell

Bit curious, since the dependency suggests setting it is

unsafe, potentially allowing command injection.

But since we are defining the bat file and not taking in user arg, it should be OK.

[breautek]

The article is talking about the shell option and the execa package does have this flag.

https://www.npmjs.com/package/execa/v/5.1.1#shell

Bit curious, since the dependency suggests setting it is

unsafe, potentially allowing command injection.

But since we are defining the bat file and not taking in user arg, it should be OK.

I assume execa is using spawn behind-the-scenes, and it's generally recommended to not enable shell because it can be a security concern:

This is from the NodeJS 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.

The security concern arises if we are passing in user supplied values, which we are not:

path.join(__dirname, 'getASPath.bat')

path is a NodeJS API, and __dirname is a module-level NodeJS variable, concating
hard-coded string getASPath.bat. So I think using shell option here is safe.

I do think we should comment why we are using shell though so that someone else doesn't remove it for optimization or to "harden" reasons.

[breautek]

Suggested change

	// "spawn" option enabled for CVE-2024-27980 (Windows) Mitigation
	// "shell" option enabled for CVE-2024-27980 (Windows) Mitigation

I wrote in "spawn" because that was in the original PR but since we determine the "shell" option is correct, that should be reflected in the comment too.

[erisu]

Changes looks good to me.