fix(@angular/cli): quote registry args on Windows and reject shell metacharacters - #33997
fix(@angular/cli): quote registry args on Windows and reject shell metacharacters#33997Tednoob17 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces registry validation to prevent shell metacharacters and invalid URLs, and attempts to fix Windows shell quoting issues by manually escaping arguments in host.ts. However, the review highlights a critical command injection vulnerability in the manual quoting implementation on Windows, as cmd.exe does not handle backslash-escaped quotes as expected. It is recommended to let Node.js's built-in spawn handle argument quoting safely and to remove the corresponding manual quoting tests.
| const childProcess = isWin32 | ||
| ? spawn(`${command} ${args.join(' ')}`, spawnOptions) | ||
| ? spawn( | ||
| `${command} ${args.map((a) => `"${String(a).replace(/"/g, '\\"')}"`).join(' ')}`, | ||
| spawnOptions, | ||
| ) | ||
| : spawn(command, args, spawnOptions); |
There was a problem hiding this comment.
Security Vulnerability: Command Injection via Manual Quoting on Windows
The manual quoting mechanism implemented here is vulnerable to command injection on Windows:
`${command} ${args.map((a) => `"${String(a).replace(/"/g, '\\"')}"`).join(' ')}`Why this happens:
cmd.exedoes not recognize\"as an escaped double quote. Incmd.exe, the backslash\is treated as a literal character, and the double quote"toggles the quoting state (on/off).- If an argument contains a double quote (e.g.,
https://example.com?key="&calc&"), the mapped string becomes:
"https://example.com?key=\"&calc&\"" - When parsed by
cmd.exe, the quoting state is evaluated as follows:"(opens quote)https://example.com?key=\(inside quotes)"(closes quote)&calc&(OUTSIDE QUOTES! This is interpreted as a command separator and executescalc)\(outside quotes)"(opens quote)"(closes quote)
Solution:
Instead of manually constructing a command string and passing it to spawn, you should pass the command and args array directly to spawn on Windows as well. Node.js's built-in spawn with shell: true automatically and safely handles argument quoting and escaping for cmd.exe under the hood.
This completely eliminates the need for manual quoting and avoids command injection vulnerabilities.
const childProcess = spawn(command, args, spawnOptions);| describe('Windows shell quoting', () => { | ||
| it('should wrap args in double quotes', () => { | ||
| const command = 'npm'; | ||
| const args = ['--registry', 'https://registry.example.com']; | ||
| const result = `${command} ${args | ||
| .map((a) => `"${String(a).replace(/"/g, '\\"')}"`) | ||
| .join(' ')}`; | ||
| expect(result).toBe( | ||
| 'npm "--registry" "https://registry.example.com"', | ||
| ); | ||
| }); | ||
|
|
||
| it('should escape inner double quotes', () => { | ||
| const command = 'npm'; | ||
| const args = ['--registry', 'https://example.com?key="value"']; | ||
| const result = `${command} ${args | ||
| .map((a) => `"${String(a).replace(/"/g, '\\"')}"`) | ||
| .join(' ')}`; | ||
| expect(result).toBe( | ||
| 'npm "--registry" "https://example.com?key=\\"value\\""', | ||
| ); | ||
| }); | ||
| }); |
c8410cd to
5ed1576
Compare
|
The Angular CLI is a developer-facing tool. Our threat model assumes that the environment where the CLI is executed (local terminal or CI/CD) is already a trusted context. If an adversary can influence the arguments passed to an |
PR Checklist
PR Type
What is the current behavior?
On Windows,
ng add --registry <url>passes the registry URL throughargs.join(" ")into acmd.exe /d /s /cshell string without quoting individual args. BecauseURL.canParseaccepts shell metacharacters like&,|,;,$,`,(,), a crafted registry value can break out and execute arbitrary commands.What is the new behavior?
packages/angular/cli/src/package-managers/host.ts.packages/angular/cli/src/commands/add/cli.tswithCommandModuleError.Does this PR introduce a breaking change?
Testing Plan
Added
packages/angular/cli/src/commands/add/registry-validation.spec.tscovering: