-
Notifications
You must be signed in to change notification settings - Fork 13
chore(node): Fix DEP0190 #506
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.
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
Conversation
✅ Deploy Preview for cedarjs canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR addresses Node.js DEP0190 deprecation by converting execa(command, args[], {shell: true}) calls to execa(command + args.join(' '), {shell: true}) across the codebase.
Critical Issues Found
checkNodeVersion.js:12: Version check logic is inverted - usessemver.gt()instead ofsemver.gte(), which blocks Node v24.0.0 itselftelemetry.ts:31-40: Shell injection vulnerability -spawn()signature changed from array to string, but args aren't escaped- Multiple CLI handlers: Shell injection risks in
lint.js,prismaHandler.js,testHandler.js,testHandlerEsm.js, andrwfw.jswhere user-provided arguments are concatenated into shell commands without sanitization
Root Cause
While Node 24's DEP0190 prevents passing separate args with shell: true, the proper fix is to either:
- Remove
shell: trueand pass args as array (preferred) - If shell is required, properly escape/quote all dynamic values
The current implementation concatenates unsanitized user input directly into shell commands, creating security vulnerabilities.
Confidence Score: 0/5
- This PR introduces critical security vulnerabilities and a blocking bug that prevents Node v24.0.0 usage
- Score reflects two critical blocking issues: (1) inverted version check logic that blocks the target Node version, and (2) multiple shell injection vulnerabilities introduced by concatenating unsanitized user input into shell commands
- Critical attention needed:
packages/cli/src/middleware/checkNodeVersion.js(blocks Node 24),packages/telemetry/src/telemetry.ts, and all CLI command handlers that concatenate user input
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/telemetry/src/telemetry.ts | 1/5 | Critical shell injection vulnerability - spawn now concatenates unsanitized args with shell: true |
| packages/cli/src/middleware/checkNodeVersion.js | 0/5 | Critical logic bug - version check uses gt instead of gte, blocks Node v24.0.0 itself |
| packages/cli/src/commands/lint.js | 1/5 | Shell injection risk - user-provided path and format options concatenated without escaping |
| packages/cli/src/commands/prismaHandler.js | 1/5 | Shell injection risk - user options concatenated into shell command without sanitization |
| packages/cli/src/commands/testHandler.js | 1/5 | Shell injection risk - jestArgs built from user input and concatenated without escaping |
| packages/cli/src/commands/testHandlerEsm.js | 1/5 | Shell injection risk - vitestArgs concatenated without escaping |
| packages/cli/src/rwfw.js | 1/5 | Shell injection risk - process.argv concatenated directly into shell command |
| packages/testing/src/api/vitest/CedarApiVitestEnv.ts | 2/5 | Shell injection risk - command array concatenated without validation |
Sequence Diagram
sequenceDiagram
participant User
participant CLI
participant Execa
participant Shell
participant Process
Note over User,Process: Before Node 24: Array args passed separately
User->>CLI: rw test filter foo
CLI->>Execa: execa with array args
Execa->>Shell: spawn with separate args
Shell->>Process: yarn test filter foo
Note over User,Process: After Node 24: DEP0190 requires string concat
User->>CLI: rw test filter foo
CLI->>Execa: execa with concatenated string
Execa->>Shell: spawn with single string
Shell->>Process: yarn test filter foo
Note over CLI,Shell: SECURITY ISSUE
User->>CLI: rw test filter malicious input
CLI->>Execa: concatenated without escaping
Shell->>Process: Multiple commands may execute
Note over Shell,Process: Shell injection vulnerability
Additional Comments (2)
-
packages/cli/src/commands/prismaHandler.js, line 54-74 (link)logic: Shell injection risk: the
argsarray is built from user-providedoptions(line 55-65). String values are directly concatenated into the shell command. If a user provides malicious input like--name "foo; rm -rf /", it could execute arbitrary commands. -
packages/cli/src/commands/testHandler.js, line 112-154 (link)logic: Shell injection risk:
jestArgsincludes user-provided filter params and forward flags fromothersobject. These are concatenated into a shell command without escaping. Malicious input could execute arbitrary commands.
18 files reviewed, 7 comments
| const LOWER_BOUND = 'v24.0.0' | ||
|
|
||
| if (semver.gt(pVersionC, LOWER_BOUND_C)) { | ||
| if (semver.gt(pVersionC, semver.clean(LOWER_BOUND))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Logic inverted: semver.gt(pVersionC, LOWER_BOUND_C) returns true when version is greater than v24.0.0, making the check pass only for versions above v24, blocking v24 itself
| if (semver.gt(pVersionC, semver.clean(LOWER_BOUND))) { | |
| if (semver.gte(pVersionC, semver.clean(LOWER_BOUND))) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/src/middleware/checkNodeVersion.js
Line: 12:12
Comment:
**logic:** Logic inverted: `semver.gt(pVersionC, LOWER_BOUND_C)` returns true when version is *greater than* v24.0.0, making the check pass only for versions above v24, blocking v24 itself
```suggestion
if (semver.gte(pVersionC, semver.clean(LOWER_BOUND))) {
```
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| // TODO: Run vitest programmatically. See https://vitest.dev/advanced/api/ | ||
| const runCommand = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Shell injection risk: vitestArgs includes user-provided arguments that are concatenated into a shell command without escaping
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/src/commands/testHandlerEsm.js
Line: 87:87
Comment:
**logic:** Shell injection risk: `vitestArgs` includes user-provided arguments that are concatenated into a shell command without escaping
How can I resolve this? If you propose a fix, please make it concise.
packages/cli/src/rwfw.js
Outdated
| // deprecated passing args in that way. | ||
| // See https://nodejs.org/api/deprecations.html#DEP0190 | ||
| // TODO: The real fix is being able to run without `shell: true` | ||
| execa.sync('yarn ' + [...command].join(' '), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Shell injection risk: command comes from process.argv.slice(2) without validation or escaping. Malicious arguments could execute arbitrary shell commands
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/src/rwfw.js
Line: 54:54
Comment:
**logic:** Shell injection risk: `command` comes from `process.argv.slice(2)` without validation or escaping. Malicious arguments could execute arbitrary shell commands
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const scriptArgs = [ | ||
| path.join(__dirname, 'scripts', 'invoke.js'), | ||
| ...args, |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
library input
shell command
This shell argument which depends on
library input
shell command
This shell argument which depends on
library input
shell command
This shell argument which depends on
library input
shell command
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 17 days ago
General approach:
Prevent user input from being interpreted as part of a shell command when spawning subprocesses. Specifically, on Windows with shell: true, do not build a command string with direct input interpolation. Instead, pass the command and its arguments as separate elements in the spawn function, or if a single command string must be used, safely quote all user inputs.
Detailed fix:
- Instead of constructing the command string as
[execPath, ...scriptArgs].join(' ')on Windows, use the argument array form forspawn. Whileshell: truegenerally expects a string, on newer versions of Node, it is safer to avoidshell: trueunless absolutely necessary. - If you must use
shell: true, ensure every user-controlled argument is safely quoted for the shell (shell-quoteis the standard tool for this). - Ideally, use
shell: false(the default) and always pass arguments as an array, as is already done for non-Windows. - If
shell: trueis only set on Windows to hide the console window or fordetachedsupport, carefully review if that's still needed, or replicate the Windows hiding logic usingwindowsHide: truewithoutshell: true.
Specific file/regions to change:
- In
packages/telemetry/src/telemetry.ts, in thespawnProcessfunction, change the Windows-specific branch to either:- Remove
shell: trueif it's not required, and use the safe argument array (like on other platforms). - If you truly need
shell: true, properly quote all arguments usingshell-quote.
- Remove
Imports and definitions needed:
- If using
shell-quote, import it at the top:import * as shellQuote from 'shell-quote'(for TypeScript, or justimport shellQuote from 'shell-quote'if using ES modules or default import).
-
Copy modified lines R22-R23 -
Copy modified lines R41-R42
| @@ -19,8 +19,8 @@ | ||
| // The following options run the process in the background without a console window, even though they don't look like they would. | ||
| // See https://github.com/nodejs/node/issues/21825#issuecomment-503766781 for information | ||
| detached: false, | ||
| windowsHide: false, | ||
| shell: true, | ||
| windowsHide: true, | ||
| // shell: false (default), so just remove shell: true for safety | ||
| } | ||
| : { | ||
| stdio: process.env.REDWOOD_VERBOSE_TELEMETRY | ||
| @@ -38,9 +38,8 @@ | ||
| ] | ||
|
|
||
| if (isWindows) { | ||
| // Use command string with empty args array to avoid DEP0190 warning when | ||
| // `shell: true` | ||
| spawn([execPath, ...scriptArgs].join(' '), [], spawnOptions).unref() | ||
| // Use proper args array and avoid using shell for safety | ||
| spawn(process.execPath, scriptArgs, spawnOptions).unref() | ||
| } else { | ||
| // Use proper args array when no shell needed | ||
| spawn(process.execPath, scriptArgs, spawnOptions).unref() |
| path.join(__dirname, 'scripts', 'invoke.js'), | ||
| ...args, | ||
| '--root', | ||
| getPaths().base, |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
library input
shell command
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 17 days ago
The problem lies in dynamically constructing a shell command string via .join(' ') for Windows, with potentially untrusted input passed directly into the shell. This construction should be avoided. The best fix is:
- Always pass arguments as an array to
spawn, even on Windows, unless shell metacharacters are strictly necessary. - If
{ shell: true }is strictly needed (e.g., for backgrounding, which is not actually needed here), arguments must be escaped using a trusted library such asshell-quote. - Otherwise, avoid
shell: trueand always supply arguments as an array.
The code should be changed so that:
- On both Windows and non-Windows, the process is spawned using an argument array (
spawn(process.execPath, scriptArgs, spawnOptions)), eliminating the need to join arguments into a string. - Remove the use of the command string with
shell: truefor the Windows case. - If
shell: trueis mandatory for deeper reasons, useshell-quoteto properly escape all embedded arguments, especially from untrusted sources.
Required changes:
packages/telemetry/src/telemetry.ts:- Remove the special case for Windows using
.join(' ')and empty args array. - Use argument arrays in all cases.
- Potentially set
shell: falsefor Windows.
- Remove the special case for Windows using
- No need to change anything within
packages/project-config/src/paths.ts. - No new methods required.
- No new imports required unless you decide you must use
shell-quote(but it's not strictly necessary here).
-
Copy modified lines R40-R41
| @@ -37,14 +37,8 @@ | ||
| getPaths().base, | ||
| ] | ||
|
|
||
| if (isWindows) { | ||
| // Use command string with empty args array to avoid DEP0190 warning when | ||
| // `shell: true` | ||
| spawn([execPath, ...scriptArgs].join(' '), [], spawnOptions).unref() | ||
| } else { | ||
| // Use proper args array when no shell needed | ||
| spawn(process.execPath, scriptArgs, spawnOptions).unref() | ||
| } | ||
| // Use argument array in all cases | ||
| spawn(process.execPath, scriptArgs, spawnOptions).unref() | ||
| } | ||
|
|
||
| // wrap a function in this call to get a telemetry hit including how long it took |
| spawn([execPath, ...scriptArgs].join(' '), [], spawnOptions).unref() | ||
| } else { | ||
| // Use proper args array when no shell needed | ||
| spawn(process.execPath, scriptArgs, spawnOptions).unref() |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
library input
shell command
This shell argument which depends on
library input
shell command
This shell argument which depends on
library input
shell command
This shell argument which depends on
library input
shell command
This shell argument which depends on
library input
shell command
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 17 days ago
To fix this vulnerability, we need to ensure that all arguments which can be influenced by external input (e.g., those sourced from getPaths().base and the ...args array) are properly escaped or quoted before assembly into a command string that is interpreted by the shell. On Windows, since we must use shell: true and the string form for suppression of the console window, we should construct the command as a string where each argument is safely quoted. The established best practice is to use the shell-quote library to quote each argument. Thus, we should import shell-quote, and, when constructing the shell command string (used only on Windows), use shellQuote.quote([...argv]) instead of simply joining the arguments with spaces. This will prevent any shell metacharacters from being interpreted and protect the invocation from unsafe injection. No change is required for the non-Windows (Unix-like) branch since the argument array form is already safe.
Required changes:
- Add a
shell-quoteimport topackages/telemetry/src/telemetry.ts. - On Windows, when building the command string passed to
spawn, useshellQuote.quote([execPath, ...scriptArgs])instead of[execPath, ...scriptArgs].join(' ').
-
Copy modified line R5 -
Copy modified line R43
| @@ -2,7 +2,7 @@ | ||
| import type { SpawnOptions } from 'child_process' | ||
| import os from 'os' | ||
| import path from 'path' | ||
|
|
||
| import shellQuote from 'shell-quote' | ||
| import { getPaths } from '@cedarjs/project-config' | ||
|
|
||
| const spawnProcess = (...args: string[]) => { | ||
| @@ -40,7 +40,7 @@ | ||
| if (isWindows) { | ||
| // Use command string with empty args array to avoid DEP0190 warning when | ||
| // `shell: true` | ||
| spawn([execPath, ...scriptArgs].join(' '), [], spawnOptions).unref() | ||
| spawn(shellQuote.quote([execPath, ...scriptArgs]), [], spawnOptions).unref() | ||
| } else { | ||
| // Use proper args array when no shell needed | ||
| spawn(process.execPath, scriptArgs, spawnOptions).unref() |
-
Copy modified lines R31-R32
| @@ -28,7 +28,8 @@ | ||
| "envinfo": "7.14.0", | ||
| "systeminformation": "5.23.8", | ||
| "uuid": "10.0.0", | ||
| "yargs": "17.7.2" | ||
| "yargs": "17.7.2", | ||
| "shell-quote": "^1.8.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/envinfo": "7.8.4", |
| Package | Version | Security advisories |
| shell-quote (npm) | 1.8.3 | None |
With Node 24 you're no longer allowed to pass separate args to child processes when you also have
shell: truespecified.https://nodejs.org/api/deprecations.html#DEP0190