-
Notifications
You must be signed in to change notification settings - Fork 11.8k
fix(@angular/cli): quote registry args on Windows and reject shell metacharacters #33997
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| /** | ||
| * @license | ||
| * Copyright Google LLC All Rights Reserved. | ||
| * | ||
| * Use of this source code is governed by an MIT-style license that can be | ||
| * found in the LICENSE file at https://angular.dev/license | ||
| */ | ||
|
|
||
| import { SHELL_METACHARACTERS, validateRegistry } from './cli'; | ||
|
|
||
| describe('registry validation', () => { | ||
| describe('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 not match safe URL characters', () => { | ||
| expect(SHELL_METACHARACTERS.test('https://registry.example.com')).toBe(false); | ||
| expect(SHELL_METACHARACTERS.test('http://registry.example.com/path')).toBe(false); | ||
| expect(SHELL_METACHARACTERS.test('https://registry.example.com:8080')).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('validateRegistry', () => { | ||
| it('should reject URLs with shell metacharacters', () => { | ||
| expect(() => validateRegistry('https://example.com&cmd')).toThrow( | ||
| 'Option --registry contains invalid characters.', | ||
| ); | ||
| expect(() => validateRegistry('https://example.com|cmd')).toThrow( | ||
| 'Option --registry contains invalid characters.', | ||
| ); | ||
| expect(() => validateRegistry('https://example.com;cmd')).toThrow( | ||
| 'Option --registry contains invalid characters.', | ||
| ); | ||
| expect(() => validateRegistry('https://example.com$cmd')).toThrow( | ||
| 'Option --registry contains invalid characters.', | ||
| ); | ||
| expect(() => validateRegistry('https://example.com`cmd`')).toThrow( | ||
| 'Option --registry contains invalid characters.', | ||
| ); | ||
| expect(() => validateRegistry('https://example.com(cmd)')).toThrow( | ||
| 'Option --registry contains invalid characters.', | ||
| ); | ||
| }); | ||
|
|
||
| it('should accept valid URLs', () => { | ||
| expect(() => validateRegistry('https://registry.example.com')).not.toThrow(); | ||
| expect(() => validateRegistry('http://registry.example.com:8080')).not.toThrow(); | ||
| expect(() => validateRegistry('https://registry.example.com/path')).not.toThrow(); | ||
| }); | ||
|
|
||
| it('should reject invalid URLs', () => { | ||
| expect(() => validateRegistry('not-a-url')).toThrow( | ||
| 'Option --registry must be a valid URL.', | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| 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\\""', | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -158,7 +158,10 @@ export const NodeJS_HOST: Host = { | |
| env, | ||
| } satisfies SpawnOptions; | ||
| const childProcess = isWin32 | ||
| ? spawn(`${command} ${args.join(' ')}`, spawnOptions) | ||
| ? spawn( | ||
| `${command} ${args.map((a) => `"${String(a).replace(/"/g, '\\"')}"`).join(' ')}`, | ||
| spawnOptions, | ||
| ) | ||
| : spawn(command, args, spawnOptions); | ||
|
Comment on lines
160
to
165
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Vulnerability: Command Injection via Manual Quoting on WindowsThe manual quoting mechanism implemented here is vulnerable to command injection on Windows: `${command} ${args.map((a) => `"${String(a).replace(/"/g, '\\"')}"`).join(' ')}`Why this happens:
Solution:Instead of manually constructing a command string and passing it to This completely eliminates the need for manual quoting and avoids command injection vulnerabilities. const childProcess = spawn(command, args, spawnOptions); |
||
|
|
||
| let stdout = ''; | ||
|
|
||
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.
Remove Obsolete Quoting Tests
Since the manual quoting implementation in
host.tsis being replaced with Node.js's built-inspawnargument handling, these manual quoting tests are no longer necessary and should be removed.