fix(@angular/cli): quote registry args on Windows and reject shell metacharacters - #33998
fix(@angular/cli): quote registry args on Windows and reject shell metacharacters#33998Tednoob17 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces registry URL validation to prevent command injection by checking for shell metacharacters and simplifies the process spawning logic on Windows by using standard array-based arguments. The review feedback highlights that the SHELL_METACHARACTERS regex should be expanded to cover additional critical characters (such as <, >, quotes, and newlines) and that the corresponding unit tests should be updated. Additionally, because of the updated spawning logic on Windows, the manual quoting configuration (requiresQuoting) in host.ts must be disabled to prevent execution failures.
|
|
||
| class CommandError extends Error {} | ||
|
|
||
| export const SHELL_METACHARACTERS = /[&|;$`()]/; |
There was a problem hiding this comment.
The SHELL_METACHARACTERS regular expression is missing several critical shell metacharacters that can be used for command injection or argument breakout, especially on Windows cmd.exe.
Specifically, the following characters are not covered:
<and>(used for input/output redirection)"(double quote) and'(single quote) (used for string/argument breakout)\nand\r(used for command termination/newline injection)
Since URL.canParse can be permissive and allow these characters in certain parts of the URL (such as query parameters or paths), an attacker could bypass the validation. For example, a URL like https://registry.example.com?q=">whoami would be considered valid by URL.canParse and would not be caught by the current regex, potentially leading to arbitrary file writes or command execution when spawned via a shell on Windows.
We should expand the regex to include these characters.
| export const SHELL_METACHARACTERS = /[&|;$`()]/; | |
| export const SHELL_METACHARACTERS = /[&|;$()\x60<>'"\n\r]/; |
| const childProcess = isWin32 | ||
| ? spawn(`${command} ${args.join(' ')}`, spawnOptions) | ||
| : spawn(command, args, spawnOptions); | ||
| const childProcess = spawn(command, args, spawnOptions); |
There was a problem hiding this comment.
Since the spawning logic on Windows has been changed from manually joining arguments into a single string to passing them as an array (spawn(command, args, spawnOptions)), Node.js will now automatically handle the quoting of arguments on Windows.
Because of this, the manual quoting enabled by requiresQuoting: platform() === 'win32' (defined on line 110 of this file) is no longer necessary and will actually cause command execution to fail on Windows. When requiresQuoting is true, package specifiers are wrapped in double quotes (e.g., "@angular/core"), which Node.js will then escape again, passing literal quotes to the package manager and resulting in EINVALIDPACKAGENAME errors.
Please update requiresQuoting on line 110 to be false (or remove it entirely if no longer needed).
| it('should match shell metacharacters', () => { | ||
| expect(SHELL_METACHARACTERS.test('&')).toBe(true); | ||
| expect(SHELL_METACHARACTERS.test('|')).toBe(true); | ||
| expect(SHELL_METACHARACTERS.test(';')).toBe(true); | ||
| expect(SHELL_METACHARACTERS.test('$')).toBe(true); | ||
| expect(SHELL_METACHARACTERS.test('`')).toBe(true); | ||
| expect(SHELL_METACHARACTERS.test('(')).toBe(true); | ||
| expect(SHELL_METACHARACTERS.test(')')).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Update the test suite to verify that the newly added shell metacharacters (such as <, >, ", ', \n, and \r) are correctly matched by SHELL_METACHARACTERS.
| it('should match shell metacharacters', () => { | |
| expect(SHELL_METACHARACTERS.test('&')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('|')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test(';')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('$')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('`')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('(')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test(')')).toBe(true); | |
| }); | |
| it('should match shell metacharacters', () => { | |
| expect(SHELL_METACHARACTERS.test('&')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('|')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test(';')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('$')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test(String.fromCharCode(96))).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('(')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test(')')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('<')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('>')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('"')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test("'")).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('\\n')).toBe(true); | |
| expect(SHELL_METACHARACTERS.test('\\r')).toBe(true); | |
| }); |
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: