From b2fca967fd221046e341d79d61802f2d02d1e6fa Mon Sep 17 00:00:00 2001 From: Martin Ruiz Date: Thu, 1 Oct 2026 17:53:22 +0000 Subject: [PATCH] fix(powershell): avoid evaluating CLI paths as source Launch the CLI through a native process instead of evaluating executable and CLI paths as PowerShell source, and reject project-level prefix settings so repository config cannot redirect CLI selection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- bin/npm.ps1 | 14 +++++-- bin/npx.ps1 | 14 +++++-- test/bin/windows-shims.js | 72 ++++++++++++++++++--------------- workspaces/config/lib/index.js | 3 +- workspaces/config/test/index.js | 10 ++++- 5 files changed, 70 insertions(+), 43 deletions(-) diff --git a/bin/npm.ps1 b/bin/npm.ps1 index efed03fe5655e..a0b8de174e341 100644 --- a/bin/npm.ps1 +++ b/bin/npm.ps1 @@ -37,14 +37,20 @@ if ($MyInvocation.ExpectingInput) { # takes pipeline input ).GetValue($MyInvocation).Text } - $NODE_EXE = $NODE_EXE.Replace("``", "````") - $NPM_CLI_JS = $NPM_CLI_JS.Replace("``", "````") - $NPM_COMMAND_ARRAY = [Management.Automation.Language.Parser]::ParseInput($NPM_ORIGINAL_COMMAND, [ref] $null, [ref] $null). EndBlock.Statements.PipelineElements.CommandElements.Extent.Text $NPM_ARGS = ($NPM_COMMAND_ARRAY | Select-Object -Skip 1) -join ' ' - Invoke-Expression "& `"$NODE_EXE`" `"$NPM_CLI_JS`" $NPM_ARGS" + $NPM_PROCESS = New-Object System.Diagnostics.Process + $NPM_PROCESS.StartInfo = New-Object System.Diagnostics.ProcessStartInfo + $NPM_PROCESS.StartInfo.FileName = $NODE_EXE + $NPM_PROCESS.StartInfo.UseShellExecute = $false + $NPM_PROCESS.StartInfo.Arguments = "`"$NPM_CLI_JS`" $NPM_ARGS" + $NPM_PROCESS.Start() | Out-Null + $NPM_PROCESS.WaitForExit() + $NPM_EXIT_CODE = $NPM_PROCESS.ExitCode + $NPM_PROCESS.Dispose() + exit $NPM_EXIT_CODE } exit $LASTEXITCODE diff --git a/bin/npx.ps1 b/bin/npx.ps1 index 3fe7b5435763a..ff9dbfd3f48b4 100644 --- a/bin/npx.ps1 +++ b/bin/npx.ps1 @@ -37,14 +37,20 @@ if ($MyInvocation.ExpectingInput) { # takes pipeline input ).GetValue($MyInvocation).Text } - $NODE_EXE = $NODE_EXE.Replace("``", "````") - $NPX_CLI_JS = $NPX_CLI_JS.Replace("``", "````") - $NPX_COMMAND_ARRAY = [Management.Automation.Language.Parser]::ParseInput($NPX_ORIGINAL_COMMAND, [ref] $null, [ref] $null). EndBlock.Statements.PipelineElements.CommandElements.Extent.Text $NPX_ARGS = ($NPX_COMMAND_ARRAY | Select-Object -Skip 1) -join ' ' - Invoke-Expression "& `"$NODE_EXE`" `"$NPX_CLI_JS`" $NPX_ARGS" + $NPX_PROCESS = New-Object System.Diagnostics.Process + $NPX_PROCESS.StartInfo = New-Object System.Diagnostics.ProcessStartInfo + $NPX_PROCESS.StartInfo.FileName = $NODE_EXE + $NPX_PROCESS.StartInfo.UseShellExecute = $false + $NPX_PROCESS.StartInfo.Arguments = "`"$NPX_CLI_JS`" $NPX_ARGS" + $NPX_PROCESS.Start() | Out-Null + $NPX_PROCESS.WaitForExit() + $NPX_EXIT_CODE = $NPX_PROCESS.ExitCode + $NPX_PROCESS.Dispose() + exit $NPX_EXIT_CODE } exit $LASTEXITCODE diff --git a/test/bin/windows-shims.js b/test/bin/windows-shims.js index d785fbe7b1c55..3e6d284651a5c 100644 --- a/test/bin/windows-shims.js +++ b/test/bin/windows-shims.js @@ -58,6 +58,10 @@ t.test('shim contents', t => { const { diff, letters } = diffFiles(SHIMS['npm.ps1'], SHIMS['npx.ps1']) t.strictSame(diff, []) t.strictSame([...letters], ['M', 'X'], 'all other changes are m->x') + t.notMatch(SHIMS['npm.ps1'], /Invoke-Expression/, 'does not evaluate commands') + t.notMatch(SHIMS['npx.ps1'], /Invoke-Expression/, 'does not evaluate commands') + t.match(SHIMS['npm.ps1'], /ProcessStartInfo/, 'starts node without reparsing the command') + t.match(SHIMS['npx.ps1'], /ProcessStartInfo/, 'starts node without reparsing the command') t.end() }) }) @@ -78,43 +82,45 @@ t.test('node-gyp', t => { }) t.test('run shims', t => { - const path = t.testdir({ - ...SHIMS, - 'node.exe': readFileSync(process.execPath), - // simulate the state where one version of npm is installed - // with node, but we should load the globally installed one - 'global-prefix': { - node_modules: { - npm: t.fixture('symlink', ROOT), + const path = join(t.testdir({ + "$(throw 'shim path evaluated')": { + ...SHIMS, + 'node.exe': readFileSync(process.execPath), + // simulate the state where one version of npm is installed + // with node, but we should load the globally installed one + 'global-prefix': { + node_modules: { + npm: t.fixture('symlink', ROOT), + }, }, - }, - // put in a shim that ONLY prints the intended global prefix, - // and should not be used for anything else. - node_modules: { - npm: { - bin: { - 'npm-prefix.js': ` - const { resolve } = require('path') - console.log(resolve(__dirname, '../../../global-prefix')) - `, - 'npx-cli.js': `throw new Error('local npx should not be called')`, - 'npm-cli.js': `throw new Error('local npm should not be called')`, + // put in a shim that ONLY prints the intended global prefix, + // and should not be used for anything else. + node_modules: { + npm: { + bin: { + 'npm-prefix.js': ` + const { resolve } = require('path') + console.log(resolve(__dirname, '../../../global-prefix')) + `, + 'npx-cli.js': `throw new Error('local npx should not be called')`, + 'npm-cli.js': `throw new Error('local npm should not be called')`, + }, }, }, + // test script returning all command line arguments + [SCRIPT_NAME]: `#!/usr/bin/env node\n\nprocess.argv.slice(2).forEach((arg) => console.log(arg))`, + // package.json for the test script + 'package.json': ` + { + "name": "${PACKAGE_NAME}", + "version": "${PACKAGE_VERSION}", + "scripts": { + "test": "node ${SCRIPT_NAME}" + }, + "bin": "${SCRIPT_NAME}" + }`, }, - // test script returning all command line arguments - [SCRIPT_NAME]: `#!/usr/bin/env node\n\nprocess.argv.slice(2).forEach((arg) => console.log(arg))`, - // package.json for the test script - 'package.json': ` - { - "name": "${PACKAGE_NAME}", - "version": "${PACKAGE_VERSION}", - "scripts": { - "test": "node ${SCRIPT_NAME}" - }, - "bin": "${SCRIPT_NAME}" - }`, - }) + }), "$(throw 'shim path evaluated')") // The removal of this fixture causes this test to fail when done with // the default tap removal. Using rimraf's `moveRemove` seems to make this diff --git a/workspaces/config/lib/index.js b/workspaces/config/lib/index.js index 4121c2a7a3840..7a978b0e0713b 100644 --- a/workspaces/config/lib/index.js +++ b/workspaces/config/lib/index.js @@ -739,10 +739,11 @@ class Config { await readFile(file, 'utf8').then( data => { const parsedConfig = ini.parse(data) - if (type === 'project' && parsedConfig.prefix) { + if (type === 'project' && hasOwnProperty(parsedConfig, 'prefix')) { // Log error if prefix is mentioned in project .npmrc /* eslint-disable-next-line max-len */ log.error('config', `prefix cannot be changed from project config: ${file}.`) + delete parsedConfig.prefix } return this.#loadObject(parsedConfig, type, file) }, diff --git a/workspaces/config/test/index.js b/workspaces/config/test/index.js index ad0355000df47..d9c9b2ab059c1 100644 --- a/workspaces/config/test/index.js +++ b/workspaces/config/test/index.js @@ -1848,18 +1848,23 @@ t.test('umask', async t => { t.test('catch project config prefix error', async t => { const path = t.testdir() + const defaultPrefix = resolve(path, 'default-prefix') + const projectPrefix = "$(throw 'project prefix evaluated')" t.testdir({ project: { node_modules: {}, '.npmrc': ` project-config = true foo = from-project-config - prefix=./lib + prefix=${projectPrefix} `, }, }) const config = new Config({ npmPath: `${path}/npm`, + env: { + PREFIX: defaultPrefix, + }, argv: [process.execPath, __filename], cwd: join(`${path}/project`), shorthands, @@ -1877,6 +1882,9 @@ t.test('catch project config prefix error', async t => { t.match(filtered, [[ 'error', 'config', `prefix cannot be changed from project config: ${path}`, ]], 'Expected error logged') + t.equal(config.get('prefix', 'project'), undefined, 'project prefix is ignored') + t.equal(config.find('prefix'), 'default', 'project prefix does not win config precedence') + t.equal(config.globalPrefix, defaultPrefix, 'project prefix is not effective') }) t.test('invalid single hyphen errors', async t => {