fix: error when script-shell is configured to a .bat or .cmd file
#7945
Conversation
| // 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 () => { |
There was a problem hiding this comment.
Note that we're no longer testing custom script-shell setups on Windows on CI. This has been broken for the last week due to this issue.
There was a problem hiding this comment.
If we do want to support this test, I can compile a .exe file to @pnpm.e2e/shell-mock that matches the behavior of shell-mock.js. Would that be useful, or overkill?
(I actually think we shouldn't do this since it's hard to audit .exe files on CI and it would run locally for pnpm contributors using Windows.)
| @@ -441,3 +443,14 @@ Run ${chalk.yellow('pnpm dedupe')} to apply the changes above. | |||
| `, | |||
| } | |||
| } | |||
|
|
|||
| function reportInvalidScriptShellWindowsError (_err: unknown): ErrorInfo { | |||
There was a problem hiding this comment.
_err isn't used. Why declare it?
There was a problem hiding this comment.
Intention was to be explicit that the original error is unused and a new one with a more detailed explanation is created.
Happy to just remove the argument if you think that's clearer.
There was a problem hiding this comment.
Well, it's kinda subjective. I do personally think that not passing the error as function argument would communicate the intention of ignoring the error object in a more explicit manner.
There was a problem hiding this comment.
Appreciate the thoughts nonetheless! I think I'm going to inline the hint instead of throwing a generic error and adding pnpm-specific reporting afterward, which should remove the changes in the cli/default-reporter package.
pnpm/exec/lifecycle/src/runLifecycleHook.ts
Lines 60 to 66 in 6d71b29
I originally didn't want to assume runLifecycleHook's scriptShell option was always configured from .npmrc, but the changes in this PR can be a lot simpler under that assumption.
gluxon
force-pushed
the
show-error-if-executing-cmd-or-bat
branch
from
1ecde86 to
6d71b29
Compare
Problem
Configuring
script-shellin.npmrcon Windows to a.bator.cmdfile,will result in:
This previously worked, but changed after the Node.js security release on April 10th, 2024.
Prior Attempt
Yesterday, I attempted to update
@pnpm/npm-lifecycleto support.bat/.cmdfiles, but this change was reverted after discovering behavior differences with arguments passed to these.bat/.cmdfiles.pnpm/npm-lifecycle#42
What does npm do?
As of version 10.5.0, npm doesn't handle
.cmdfiles forscript-shelleither on Node.js 21.7.3.Proposal
Let's show an error if
.bat/.cmdfiles are set as thescript-shellfor now.I think there's several strong reasons for this:
To elaborate on the first point, I haven't found an easy way to support
.bat/.cmdfiles as shells on newer versions of Node.js without making.bator.cmdfiles behave differently than.exefiles or shells on Linux or macOS. Arguments are parsed regardless ofspawn,exec,execFile, orwindowsVerbatimArgumentsoptions. I believe the correct behavior would involve manually escaping variables like%PATH%so they're passed verbatim to thescript-shell. The Rust team attempted to do something similar so arguments are passed verbatim, but their implementation was incorrect. I don't think we should try to do the same given that. From their blog post (emphasis mine):Changes
Here's a screenshot of the new error users will see under this unsupported configuration.