Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion packages/angular/cli/src/commands/add/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,18 @@ import { VERSION } from '../../utilities/version';

class CommandError extends Error {}

export const SHELL_METACHARACTERS = /[&|;$`()]/;

export function validateRegistry(registry: string): void {
if (!URL.canParse(registry)) {
throw new CommandModuleError('Option --registry must be a valid URL.');
}

if (SHELL_METACHARACTERS.test(registry)) {
throw new CommandModuleError('Option --registry contains invalid characters.');
}
}

interface AddCommandArgs extends SchematicsCommandArgs {
collection: string;
verbose?: boolean;
Expand Down Expand Up @@ -132,7 +144,9 @@ export default class AddCommandModule
return true;
}

if (typeof registry === 'string' && URL.canParse(registry)) {
if (typeof registry === 'string') {
validateRegistry(registry);

return true;
}

Expand Down
88 changes: 88 additions & 0 deletions packages/angular/cli/src/commands/add/registry-validation.spec.ts
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\\""',
);
});
});
Comment on lines +65 to +87

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Remove Obsolete Quoting Tests

Since the manual quoting implementation in host.ts is being replaced with Node.js's built-in spawn argument handling, these manual quoting tests are no longer necessary and should be removed.

});
5 changes: 4 additions & 1 deletion packages/angular/cli/src/package-managers/host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

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:

  1. cmd.exe does not recognize \" as an escaped double quote. In cmd.exe, the backslash \ is treated as a literal character, and the double quote " toggles the quoting state (on/off).
  2. If an argument contains a double quote (e.g., https://example.com?key="&calc&"), the mapped string becomes:
    "https://example.com?key=\"&calc&\""
  3. 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 executes calc)
    • \ (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);


let stdout = '';
Expand Down