Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions cli/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,17 @@ import { serve } from 'srvx'
import { createConnectorApp, generateToken, CONNECTOR_VERSION } from './server.ts'
import { getNpmUser, NPM_REGISTRY_URL } from './npm-client.ts'
import { initLogger, showToken, logInfo, logWarning, logError } from './logger.ts'
import { resolveNpmProcessCommand } from './npm-process.ts'

const DEFAULT_PORT = 31415
const DEFAULT_FRONTEND_URL = 'https://npmx.dev/'
const DEV_FRONTEND_URL = 'http://127.0.0.1:3000/'

async function runNpmLogin(): Promise<boolean> {
return new Promise(resolve => {
const child = spawn('npm', ['login', `--registry=${NPM_REGISTRY_URL}`], {
const { command, args } = resolveNpmProcessCommand(['login', `--registry=${NPM_REGISTRY_URL}`])

const child = spawn(command, args, {
stdio: 'inherit',
shell: true,
})

child.on('close', code => {
Expand Down
12 changes: 5 additions & 7 deletions cli/src/npm-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { join } from 'node:path'
import * as v from 'valibot'
import { PackageNameSchema, UsernameSchema, OrgNameSchema, ScopeTeamSchema } from './schemas.ts'
import { logCommand, logSuccess, logError, logDebug } from './logger.ts'
import { resolveNpmProcessCommand } from './npm-process.ts'

const execFileAsync = promisify(execFile)
export const NPM_REGISTRY_URL = 'https://registry.npmjs.org/'
Expand Down Expand Up @@ -332,13 +333,10 @@ async function execNpm(args: string[], options: ExecNpmOptions = {}): Promise<Np

try {
logDebug('Executing npm command:', { command: 'npm', args: npmArgs })
// Use execFile instead of exec to avoid shell injection vulnerabilities
// On Windows, shell: true is required to execute .cmd files (like npm.cmd)
// On Unix, we keep it false for better security and performance
const { stdout, stderr } = await execFileAsync('npm', npmArgs, {
const { command, args: processArgs } = resolveNpmProcessCommand(npmArgs)
const { stdout, stderr } = await execFileAsync(command, processArgs, {
timeout: 60000,
env: createNpmEnv(),
shell: process.platform === 'win32',
})

logDebug('Command succeeded:', { stdout, stderr })
Expand Down Expand Up @@ -610,11 +608,11 @@ export async function packageInit(
logCommand(`${displayCmd} (in temp dir for ${name})`)

try {
const { stdout, stderr } = await execFileAsync('npm', npmArgs, {
const { command, args: processArgs } = resolveNpmProcessCommand(npmArgs)
const { stdout, stderr } = await execFileAsync(command, processArgs, {
timeout: 60000,
cwd: tempDir,
env: createNpmEnv(),
shell: process.platform === 'win32',
})

logSuccess(`Published ${name}@0.0.0`)
Expand Down
24 changes: 24 additions & 0 deletions cli/src/npm-process.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import process from 'node:process'

interface NpmProcessCommand {
command: string
args: string[]
}

export function resolveNpmProcessCommand(
npmArgs: string[],
platform = process.platform,
comSpec = process.env.ComSpec,
): NpmProcessCommand {
if (platform === 'win32') {
return {
command: comSpec || 'cmd.exe',
args: ['/d', '/s', '/c', 'npm', ...npmArgs],
}
}
Comment on lines +13 to +18
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any examples anyone is aware of, of other projects doing this? Just want to make sure, as not a windows user, this is the safest best way 😅

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure if it's the safest way, however am aware that it works as intended to minimise the DEP1090 Issue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this a bit more.

The two approaches I found were either:

  • keep using a shell, but pass a single command string
  • explicitly use cmd.exe /d /s /c ... on Windows

Node-RED took the first approach for the same deprecation, with the main change in b364f8f. For this case though, I think the second one is the better fit. It is still consistent with Node’s Windows guidance for spawning .bat and .cmd files, while avoiding shell: true with args[], which is what DEP0190 warns about.

So this seems like a solid approach to me.


return {
command: 'npm',
args: npmArgs,
}
}
Loading