fix: spawn EINVAL on Windows with script-shell configured

[gluxon]

Problem

Configuring script-shell in .npmrc on Windows to a .bat or .cmd file,

# .npmrc
script-shell = node_modules/.bin/shell-mock${isWindows() ? '.cmd'

will result in:

spawn EINVAL

  at spawn (../../node_modules/.pnpm/@[email protected][email protected]/node_modules/@pnpm/npm-lifecycle/lib/spawn.js:36:15)

This previously worked, but changed after the Node.js security release on April 10th, 2024.

What does npm do?

More modern versions of npm take the script-shell option and configure it as the shell option to pass to spawn directly.

spawn(cmd, args, { shell: scriptShell })

This repo currently executes script-shell directly instead of setting the shell spawn config:

spawn(scriptShell, [cmd])

Changes

I thought through a few options on how to fix this:

1. Re-fork the @npm/run-script package and use that in pnpm.
2. Match how @npm/run-script uses spawn and pass { shell: scriptShell } as an option.
3. Set { shell: true } on Windows when scriptShell is a batch file.

I went with option 3 for now since:

• It's the most minimal change that fixes the EINVAL issue.
• I don't believe it'll introduce new security problems since the purpose of NPM lifecycle scripts is to run shell scripts. NPM also defaults the shell option to true.

[gluxon]

Making sure this actually works and fixes pnpm's CI jobs on my fork here: https://github.com/gluxon/pnpm/actions/runs/8690981281

[KSXGitHub]

Oh, this is similar to the overrated Rust vulnerability.

Anyway, what are the possible ways a malicious actor could attack the user?

From the developer's POV. He already run arbitrary script when installing packages from npm. This means that npm packages can already inject malicious command without command line argument injection. It is the developer's responsibility to audit the package before installing them.

For the end user, it's hard to imagine an end user would use pnpm lifecycle to invoke a command.

[zkochan]

I'll update it in pnpm

[gluxon]

We might need to make more changes. 😞

My test above failed due to arguments parsing differently now.

00.3132048Z   ● pnpm run with custom shell
2024-04-15T14:46:00.3132311Z 
2024-04-15T14:46:00.3132552Z     expect(received).toStrictEqual(expected) // deep equality
2024-04-15T14:46:00.3132916Z 
2024-04-15T14:46:00.3133059Z     - Expected  - 1
2024-04-15T14:46:00.3133347Z     + Received  + 2
2024-04-15T14:46:00.3133508Z 
2024-04-15T14:46:00.3133602Z       Array [
2024-04-15T14:46:00.3133898Z         "-c",
2024-04-15T14:46:00.3134194Z     -   "foo bar",
2024-04-15T14:46:00.3134442Z     +   "foo",
2024-04-15T14:46:00.3134700Z     +   "bar",
2024-04-15T14:46:00.3134959Z       ]
2024-04-15T14:46:00.3135087Z 
2024-04-15T14:46:00.3135349Z     �[0m �[90m 463 |�[39m   }�[33m,�[39m [�[32m'build'�[39m])
2024-04-15T14:46:00.3135808Z      �[90m 464 |�[39m
2024-04-15T14:46:00.3138070Z     �[31m�[1m>�[22m�[39m�[90m 465 |�[39m   expect((�[36mawait�[39m �[36mimport�[39m(path�[33m.�[39mresolve(�[32m'shell-input.json'�[39m)))�[33m.�[39m�[36mdefault�[39m)�[33m.�[39mtoStrictEqual([�[32m'-c'�[39m�[33m,�[39m �[32m'foo bar'�[39m])
2024-04-15T14:46:00.3140132Z      �[90m     |�[39m                                                                    �[31m�[1m^�[22m�[39m
2024-04-15T14:46:00.3141158Z      �[90m 466 |�[39m })
2024-04-15T14:46:00.3141700Z      �[90m 467 |�[39m
2024-04-15T14:46:00.3143028Z      �[90m 468 |�[39m test(�[32m'pnpm run with RegExp script selector should work'�[39m�[33m,�[39m �[36masync�[39m () �[33m=>�[39m {�[0m
2024-04-15T14:46:00.3144002Z 
2024-04-15T14:46:00.3144648Z       at Object.<anonymous> (test/index.ts:465:68)

[gluxon]

I'll look at the problem above now. Sorry, I should have left this PR in draft while testing it.

[gluxon]

Oh, this is similar to the overrated Rust vulnerability.

Wow, I didn't see that. It looks like this vulnerability hit many programming languages. The Node.js fix mentioned that the underlying argument parsing logic on Windows isn't standardized and hard to get right.

Anyway, what are the possible ways a malicious actor could attack the user?

From the developer's POV. He already run arbitrary script when installing packages from npm. This means that npm packages can already inject malicious command without command line argument injection. It is the developer's responsibility to audit the package before installing them.

For the end user, it's hard to imagine an end user would use pnpm lifecycle to invoke a command.

This sounds right. Appreciate the second thoughts on attack vectors. I was worried about setting shell: true before seeing @npm/run-script does it too.