Skip to content

fix(@angular/cli): quote registry args on Windows and reject shell metacharacters - #33998

Closed
Tednoob17 wants to merge 3 commits into
angular:mainfrom
Tednoob17:fix/security/registry-injection
Closed

fix(@angular/cli): quote registry args on Windows and reject shell metacharacters#33998
Tednoob17 wants to merge 3 commits into
angular:mainfrom
Tednoob17:fix/security/registry-injection

Conversation

@Tednoob17

Copy link
Copy Markdown

PR Checklist

  • The commit message follows our guidelines
  • Tests for the changes have been added

PR Type

  • Bugfix

What is the current behavior?

On Windows, ng add --registry <url> passes the registry URL through args.join(" ") into a cmd.exe /d /s /c shell string without quoting individual args. Because URL.canParse accepts shell metacharacters like &, |, ;, $, `, (, ), a crafted registry value can break out and execute arbitrary commands.

What is the new behavior?

  • Each arg is quoted individually on Windows in packages/angular/cli/src/package-managers/host.ts.
  • Registry URLs containing shell metacharacters are rejected early in packages/angular/cli/src/commands/add/cli.ts with CommandModuleError.

Does this PR introduce a breaking change?

  • No

Testing Plan

Added packages/angular/cli/src/commands/add/registry-validation.spec.ts covering:

  • rejection of shell metacharacters in registry URLs
  • acceptance of valid registry URLs
  • Windows quoting behavior

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 = /[&|;$`()]/;

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-high high

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)
  • \n and \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.

Suggested change
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

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).

Comment on lines +13 to +21
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);
});

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

Update the test suite to verify that the newly added shell metacharacters (such as <, >, ", ', \n, and \r) are correctly matched by SHELL_METACHARACTERS.

Suggested change
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);
});

@alan-agius4

Copy link
Copy Markdown
Collaborator

Duplicate of #33997

@alan-agius4 alan-agius4 marked this as a duplicate of #33997 Sep 3, 2026
@alan-agius4 alan-agius4 closed this Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants