fix(package-manager): prevent CWE-78 OS command injection across all Windows spawn paths#1
Open
fix(package-manager): prevent CWE-78 OS command injection across all Windows spawn paths#1
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)
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.
…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.
…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.
- 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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 angular#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