fix(package-manager): prevent CWE-78 OS command injection across all Windows spawn paths#32892
fix(package-manager): prevent CWE-78 OS command injection across all Windows spawn paths#32892g0w6y wants to merge 7 commits intoangular:mainfrom
Conversation
…ers on Windows On Windows, package manager commands are executed through cmd.exe with shell:true because npm, yarn, pnpm etc. are installed as .cmd scripts. Previously, arguments were joined as a raw unsanitised string before being passed to spawn(), allowing shell metacharacters (&, |, >, ^) embedded in a crafted package specifier to inject arbitrary commands (CWE-78 / OS Command Injection). Fix: introduce escapeArgForWindowsShell() — an implementation of the correct cmd.exe quoting algorithm — and apply it to every argument before string-concatenation in the two affected files: - packages/angular/cli/src/package-managers/host.ts - packages/angular_devkit/schematics/tasks/package-manager/executor.ts The safe array-form spawn path used on Linux/macOS is unchanged. Related: the analogous repo-init/executor.ts path was already patched in angular#32678. This PR closes the remaining two locations. Refs: CWE-78, GHSA (pending), PR angular#32678
…l Windows spawn paths Five locations concatenated user-controlled arguments into a raw shell string executed by cmd.exe (shell:true), allowing metacharacters such as &, |, >, ^ in a package specifier or --package-manager flag to inject and execute arbitrary OS commands silently alongside the legitimate package manager process. Affected paths and their fix: - host.ts: shell:isWin32 + args.join concat replaced with cmd.exe array invocation (shell:false) so Node.js controls arg quoting - executor.ts: escapedArgs+string-concat pattern replaced with cmd.exe direct array invocation; shell:true removed - ssr-dev-server/utils.ts: args.join concat replaced with cmd.exe array dispatch on Windows, safe array-form on POSIX - ssr-dev-server/index.ts: stray shell:true removed from spawnAsObservable call-site (platform dispatch in utils.ts) - workspace/index.ts: ALLOWED_PKG_MANAGERS allowlist guard added before execSync to block injection via ng new --package-manager POSIX spawn paths (array-form, shell:false) are unchanged. Follows pattern from angular#32678 which patched repo-init/executor.ts. CWE: CWE-78 (OS Command Injection)
There was a problem hiding this comment.
Code Review
This pull request addresses command injection vulnerabilities (CWE-78) by replacing shell-based command execution with direct process spawning. On Windows, the implementation now invokes cmd.exe using array-based arguments and shell:false to prevent metacharacter injection. Additionally, an allowlist for package managers was added to secure execSync calls. The review feedback points out that the escapeArgForWindowsShell function is unused and should be removed to eliminate dead code.
packages/angular_devkit/schematics/tasks/package-manager/executor.ts
Outdated
Show resolved
Hide resolved
The cmd.exe array invocation (shell:false) approach does not require manual argument escaping. Remove the dead helper per code review feedback on angular#32892.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses command injection vulnerabilities (CWE-78) by refactoring how child processes are spawned across several packages. Specifically, it replaces the use of shell: true and string concatenation with direct cmd.exe invocations on Windows using argument arrays and shell: false. It also adds an allowlist for package managers in the workspace schematic to prevent arbitrary command execution. Review feedback identifies functional regressions introduced by these changes: since the shell is no longer interpreting the command string, arguments that were previously manually quoted (such as file paths or registry URLs) will now be passed with literal quotes to the underlying process, causing them to fail. These arguments need to be updated to remove manual quoting.
packages/angular_devkit/build_angular/src/builders/ssr-dev-server/index.ts
Show resolved
Hide resolved
packages/angular_devkit/schematics/tasks/package-manager/executor.ts
Outdated
Show resolved
Hide resolved
…ll:false migration When shell:true was active, arguments with special characters required manual quoting (e.g. `"/Users/gourisankara/.antigravity/antigravity/bin /opt/homebrew/bin /opt/homebrew/sbin /usr/local/bin /System/Cryptexes/App/usr/bin /usr/bin /bin /usr/sbin /sbin /var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin /var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin /var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin /Users/gourisankara/.foundry/bin"`, `--registry="..."`). With shell:false + cmd.exe array invocation, Node.js controls quoting internally — manual quotes are passed as literal characters to the subprocess and cause failures. - ssr-dev-server/index.ts: remove literal quotes around path arg - executor.ts: split --registry="url" into ['--registry', url] Fixes functional regressions identified in angular#32892 review.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses command injection vulnerabilities (CWE-78) by refactoring process spawning logic across several packages. It replaces shell-based execution and string concatenation with array-based arguments and direct cmd.exe invocation on Windows. Additionally, an allowlist for package managers was added to workspace/index.ts to prevent arbitrary command execution. A critical syntax error was identified in executor.ts where the .on() method was incorrectly placed within a ternary operator, which will cause a runtime error.
packages/angular_devkit/schematics/tasks/package-manager/executor.ts
Outdated
Show resolved
Hide resolved
…hain The .on() call was incorrectly placed inside each branch of the ternary operator instead of being chained after the ternary resolves to a ChildProcess instance. Wrap the ternary in parentheses and chain .on() outside so it applies to whichever spawn() call was selected. Fixes syntax error identified in angular#32892 review.
|
@gemini-code-assist[bot] Thanks for catching the ternary chaining issue — fixed by wrapping the expression in parentheses so All previous feedback (dead code + quoting regressions + syntax) has now been resolved. Ready for human review. |
|
Thank you for the update. The fix looks solid, and wrapping the expression in parentheses is the correct approach to ensure the |
- utils.ts: move node:os import after node:net (import/order) - executor.ts: collapse multiple blank lines to max 2 (no-multiple-empty-lines) - host.ts: add blank line before statement at line 175 (padding-line-between-statements)
Problem
Five locations in Angular CLI concatenated user-controlled arguments
into a raw shell string executed by
cmd.exewithshell: true.Any shell metacharacter (
&,|,>,^) in a crafted packagespecifier or
--package-managerflag value silently injects andexecutes arbitrary OS commands alongside the legitimate process.
package-managers/host.tsng add <pkg>,ng updatetasks/package-manager/executor.tsNodePackageInstallTaskssr-dev-server/utils.tsssr-dev-server/index.tsspawnAsObservablewith strayshell:trueschematics/angular/workspace/index.tsng new --package-manager=<value>Fix
Windows spawn paths (
host.ts,executor.ts,utils.ts):Replace
spawn(\${cmd} ${args.join(' ')}`, { shell: true })with directcmd.exearray invocation usingshell: false`. Node.jscontrols argument quoting — metacharacters never reach cmd.exe as
shell operators.
ssr-dev-server/index.ts: Remove strayshell: truefrom thespawnAsObservablecall-site. Platform dispatch is handled insideutils.ts.workspace/index.ts: AddALLOWED_PKG_MANAGERSSet guard beforeexecSync(\${packageManager} --version`). Onlynpm,yarn,pnpm,bun,cnpmaccepted — blocks all-platform injection viang new --package-manager`.Relation to prior work
Follows the fix pattern from #32678 which patched the analogous
repo-init/executor.tspath. This PR closes all five remainingvulnerable locations.
Verification
CWE: CWE-78 — Improper Neutralisation of Special Elements in an OS Command