Skip to content

fix(powershell): avoid evaluating CLI paths as source #10058

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
martinrrm wants to merge 1 commit into
base: latest
Choose a base branch
from
Draft

fix(powershell): avoid evaluating CLI paths as source #10058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions bin/npm.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
14 changes: 10 additions & 4 deletions bin/npx.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
72 changes: 39 additions & 33 deletions test/bin/windows-shims.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})
})
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion workspaces/config/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
},
Expand Down
10 changes: 9 additions & 1 deletion workspaces/config/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 => {
Expand Down
Loading