fix: show clearer error when script-shell is a .bat/.cmd file

pnpm · zkochan · Apr 18, 2024 · Apr 16, 2024 · Apr 18, 2024 · Apr 17, 2024
commit 6d71b29f80b003237d63ea3cf59864dd09a15721
@@ -0,0 +1,6 @@
+---
+"@pnpm/lifecycle": patch
+pnpm: patch
+---
+
+If the `script-shell` option is configured to a `.bat`/`.cmd` file on Windows, pnpm will now error with `ERR_PNPM_INVALID_SCRIPT_SHELL_WINDOWS`. Newer [versions of Node.js released in April 2024](https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2) do not support executing these files directly without behavior differences. If the `script-shell` option is necessary for your use-case, please set a `.exe` file instead.
@@ -43,13 +43,15 @@
     "@pnpm/read-package-json": "workspace:*",
     "@pnpm/store-controller-types": "workspace:*",
     "@pnpm/types": "workspace:*",
+    "is-windows": "^1.0.2",
     "path-exists": "^4.0.0",
     "run-groups": "^3.0.1"
   },
   "devDependencies": {
     "@pnpm/lifecycle": "workspace:*",
     "@pnpm/test-fixtures": "workspace:*",
     "@pnpm/test-ipc-server": "workspace:*",
+    "@types/is-windows": "^1.0.2",
     "@types/rimraf": "^3.0.2",
     "@zkochan/rimraf": "^2.1.3",
     "load-json-file": "^6.2.0"

@@ -4,6 +4,7 @@ import lifecycle from '@pnpm/npm-lifecycle'
 import { type DependencyManifest, type ProjectManifest } from '@pnpm/types'
 import { PnpmError } from '@pnpm/error'
 import { existsSync } from 'fs'
+import isWindows from 'is-windows'
 
 function noop () {} // eslint-disable-line:no-empty
 
@@ -32,6 +33,39 @@ export async function runLifecycleHook (
 ): Promise<void> {
   const optional = opts.optional === true
 
+  // To remediate CVE_2024_27980, Node.js does not allow .bat or .cmd files to
+  // be spawned without the "shell: true" option.
+  //
+  // https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows
+  //
+  // Unfortunately, setting spawn's shell option also causes arguments to be
+  // evaluated before they're passed to the shell, resulting in a surprising
+  // behavior difference only with .bat/.cmd files.
+  //
+  // Instead of showing a "spawn EINVAL" error, let's throw a clearer error that
+  // this isn't supported.
+  //
+  // If this behavior needs to be supported in the future, the arguments would
+  // need to be escaped before they're passed to the .bat/.cmd file. For
+  // example, scripts such as "echo %PATH%" should be passed verbatim rather
+  // than expanded. This is difficult to do correctly. Other open source tools
+  // (e.g. Rust) attempted and introduced bugs. The Rust blog has a good
+  // high-level explanation of the same security vulnerability Node.js patched.
+  //
+  // https://blog.rust-lang.org/2024/04/09/cve-2024-24576.html#overview
+  //
+  // Note that npm (as of version 10.5.0) doesn't support setting script-shell
+  // to a .bat or .cmd file either.
+  if (opts.scriptShell != null && isWindowsBatchFile(opts.scriptShell)) {
+    throw new PnpmError('ERR_PNPM_INVALID_SCRIPT_SHELL_WINDOWS', 'Cannot spawn .bat or .cmd as a script shell.', {
+      hint: `\
+The .npmrc script-shell option was configured to a .bat or .cmd file. These cannot be used as a script shell reliably.
+
+Please unset the script-shell option, or configure it to a .exe instead.
+`,
+    })
+  }
+
   const m = { _id: getId(manifest), ...manifest }
   m.scripts = { ...m.scripts }
 
@@ -126,3 +160,12 @@ export async function runLifecycleHook (
 function getId (manifest: ProjectManifest | DependencyManifest): string {
   return `${manifest.name ?? ''}@${manifest.version ?? ''}`
 }
+
+function isWindowsBatchFile (scriptShell: string) {
+  // Node.js performs a similar check to determine whether it should throw
+  // EINVAL when spawning a .cmd/.bat file.
+  //
+  // https://github.com/nodejs/node/commit/6627222409#diff-1e725bfa950eda4d4b5c0c00a2bb6be3e5b83d819872a1adf2ef87c658273903
+  const scriptShellLower = scriptShell.toLowerCase()
+  return (scriptShellLower.endsWith('.cmd') || scriptShellLower.endsWith('.bat')) && isWindows()
+}
@@ -17,6 +17,9 @@ import { DEFAULT_OPTS, REGISTRY_URL } from './utils'
 
 const pnpmBin = path.join(__dirname, '../../../pnpm/bin/pnpm.cjs')
 
+const skipOnWindows = isWindows() ? test.skip : test
+const onlyOnWindows = !isWindows() ? test.skip : test
+
 test('pnpm run: returns correct exit code', async () => {
   prepare({
     scripts: {
@@ -437,7 +440,10 @@ test('scripts work with PnP', async () => {
   expect(fooOutput).toStrictEqual(helloWorldJsBinOutput)
 })
 
-test('pnpm run with custom shell', async () => {
+// A .exe file to configure as the scriptShell option would be necessary to test
+// this behavior on Windows. Skip this test for now since compiling a custom
+// .exe would be quite involved and hard to maintain.
+skipOnWindows('pnpm run with custom shell', async () => {
   prepare({
     scripts: {
       build: 'foo bar',
@@ -459,12 +465,44 @@ test('pnpm run with custom shell', async () => {
     extraBinPaths: [],
     extraEnv: {},
     rawConfig: {},
-    scriptShell: path.resolve(`node_modules/.bin/shell-mock${isWindows() ? '.cmd' : ''}`),
+    scriptShell: path.resolve('node_modules/.bin/shell-mock'),
   }, ['build'])
 
   expect((await import(path.resolve('shell-input.json'))).default).toStrictEqual(['-c', 'foo bar'])
 })
 
+onlyOnWindows('pnpm shows error if script-shell is .cmd', async () => {
+  prepare({
+    scripts: {
+      build: 'foo bar',
+    },
+    dependencies: {
+      '@pnpm.e2e/shell-mock': '0.0.0',
+    },
+  })
+
+  await execa(pnpmBin, [
+    'install',
+    `--registry=${REGISTRY_URL}`,
+    '--store-dir',
+    path.resolve(DEFAULT_OPTS.storeDir),
+  ])
+
+  async function runScript () {
+    await run.handler({
+      dir: process.cwd(),
+      extraBinPaths: [],
+      extraEnv: {},
+      rawConfig: {},
+      scriptShell: path.resolve('node_modules/.bin/shell-mock.cmd'),
+    }, ['build'])
+  }
+
+  await expect(runScript).rejects.toEqual(expect.objectContaining({
+    code: 'ERR_PNPM_INVALID_SCRIPT_SHELL_WINDOWS',
+  }))
+})
+
 test('pnpm run with RegExp script selector should work', async () => {
   prepare({
     scripts: {