Skip to content

Conversation

@kim-sung-jee
Copy link

@kim-sung-jee kim-sung-jee commented Nov 19, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Passing an args array together with shell: true triggers Node.js deprecation warning DEP0190

Issue Number: nestjs/nest#15943

What is the new behavior?

When shell is enabled, the CLI now passes a single command string to spawn instead of using the args array

This matches the pattern recommended by a Node.js member in
nodejs/help#5063 (comment)

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

This change follows Node.js API guidance, rather than introducing a new escaping strategy.

Node.js deprecated the args + shell: true combination because the API shape gives a false impression that arguments are safely escaped, while they are in fact just concatenated into a single shell command string (see the DEP0190 change in normalizeSpawnArguments).

This PR:

  • Aligns the CLI with the recommended usage by passing a single command string when shell: true, and keeping command + args only when shell is false ( the same pattern Node.js itself uses internally ).
  • Avoids implying any “magic escaping” from the CLI side — we simply stop using the misleading args + shell: true combination.

Proper shell escaping / input hardening is intentionally out of scope for this PR and should be handled separately in a follow-up change, closer to where potentially untrusted input enters the system.

@kim-sung-jee kim-sung-jee force-pushed the fix/nodejs-shell-deprecate branch from 9c8c25c to 367c29b Compare November 19, 2025 15:25
Copy link
Member

@micalevisk micalevisk left a comment

Choose a reason for hiding this comment

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

FYI this is not a fix but a refactor

@kamilmysliwiec
Copy link
Member

#3192

This introduces another issue

@kim-sung-jee
Copy link
Author

@kamilmysliwiec @micalevisk Thanks for the heads-up!

  1. On Unix, shell used to split a single “command” string into proper argv tokens. After fix(@nestjs/cli): disable shell execution on unix to prevent command injection #3128, shell became false while the code still relied on the shell to do that splitting. For example, in AbstractRunner:

    // this.binary: 'node'
    // this.args: ['node_modules/@nestjs/schematics/bin/nest.js']
    // command: '@nestjs/schematics:application --name=practice --directory=... --package-manager="npm" ...'
    
    const args = [command];
    
    spawn(this.binary, [...this.args, ...args], {
      shell: false, // after #3128 on Unix
    });

    This results in:

    spawn('node', [
      'node_modules/@nestjs/schematics/bin/nest.js',
      '@nestjs/schematics:application --name=practice --directory=... --package-manager="npm" ...',
    ], { shell: false });

    Expected:

    spawn('node', [
      'node_modules/@nestjs/schematics/bin/nest.js',
      '@nestjs/schematics:application',
      '--name=practice',
      '--directory=...',
      '--package-manager=npm',
      ...
    ], { shell: false });

    With shell: true, the shell split that long string into @nestjs/schematics:application, --name=..., --directory=..., etc. With shell: false, it’s passed as a single argv element, and schematics can’t parse it correctly.

    In addition, some arguments were already in a shell-oriented format (for example a quoted CLI path like "path/to/schematics.js" and a single command string with quoted options). With shell: true, the shell strips those quotes and does word splitting for us. Once shell is turned off but those formats stay the same, Node forwards the raw strings (including quotes) as argv elements, which is why nest new starts failing even though the underlying files and commands are correct.

  2. This PR only adapts to the Node.js API change for shell: true
    This PR doesn’t change when shell is enabled or disabled. It simply updates the shell: true call sites to match Node’s DEP0190 guidance: instead of spawn(binary, args, { shell: true }), the CLI now uses a single command string (spawn(commandString, { shell: true })), while the shell: false path stays as spawn(binary, args, { shell: false }).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants