Skip to content

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

Closed
Tednoob17 wants to merge 1 commit into
angular:mainfrom
Tednoob17:fix/security/registry-injection
Closed

fix(@angular/cli): quote registry args on Windows and reject shell metacharacters#33997
Tednoob17 wants to merge 1 commit 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 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.

Comment on lines 160 to 165
const childProcess = isWin32
? spawn(`${command} ${args.join(' ')}`, spawnOptions)
? spawn(
`${command} ${args.map((a) => `"${String(a).replace(/"/g, '\\"')}"`).join(' ')}`,
spawnOptions,
)
: 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.

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

Comment on lines +65 to +87
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\\""',
);
});
});

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.

@Tednoob17
Tednoob17 force-pushed the fix/security/registry-injection branch from c8410cd to 5ed1576 Compare September 2, 2026 18:43
@alan-agius4

Copy link
Copy Markdown
Collaborator

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 ng command, the execution environment is already effectively compromised.

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