Security: Shell command injection risk from package directory name in execSync

[tomaioo]

Summary

Security: Shell command injection risk from package directory name in execSync

Problem

Severity: High | File: tools/bump-packages-minor.js:L54

The script interpolates type into a shell command: execSync(`git ls-files --others --deleted --exclude-standard --directory ed/${type}`). type comes from directory names under packages, which can be influenced by repository content. A crafted folder name containing shell metacharacters could execute arbitrary commands when this script runs.

Solution

Use execFileSync('git', ['ls-files', '--others', '--deleted', '--exclude-standard', '--directory', ed/${type}]) instead of shell strings. Also validate type against expected package names (e.g., ['css','css6','elements','events','idl']).

Changes

• tools/bump-packages-minor.js (modified)

[nberlette]

While this is indeed a security risk, is there any real viability to it, given that the file in question is not a part of any public API and is never exposed to userland code or input?

Maybe a maintainer could chime in here (and please correct me if I'm mistaken!), but my initial read on this is that code in the tools/ folder is only ever going to be run intentionally, without any untrusted inputs... I can't visualize a scenario where the folder names that are interpolated in the execSync call site are able to be manipulated by a bad actor ahead of time. However, if this code is executed automatically in, for example, response to any new PR being created or updated, that would introduce a potential attack vector as you've described.

[tomaioo]

Great question—you're right that this isn’t a public API/input path, so the risk depends on who can influence repo contents where the script runs. The viable scenario is CI or a maintainer running the tool on a branch/PR (including forks) where a crafted package directory name is present; in that context, repo files are effectively untrusted input. So this is mostly a hardening/supply-chain fix: low likelihood in normal use, but high impact if triggered, and execFileSync + allowlisting removes the class of issue with minimal cost.

[tomaioo]

Great question—you're right that this isn’t a public API/input path, so the risk depends on who can influence repo contents where the script runs. The viable scenario is CI or a maintainer running the tool on a branch/PR (including forks) where a crafted package directory name is present; in that context, repo files are effectively untrusted input. So this is mostly a hardening/supply-chain fix: low likelihood in normal use, but high impact if triggered, and execFileSync + allowlisting removes the class of issue with minimal cost.

[tomaioo]

Great question—you're right that this isn’t a public API/input path, so the risk depends on who can influence repo contents where the script runs. The viable scenario is CI or a maintainer running the tool on a branch/PR (including forks) where a crafted package directory name is present; in that context, repo files are effectively untrusted input. So this is mostly a hardening/supply-chain fix: low likelihood in normal use, but high impact if triggered, and execFileSync + allowlisting removes the class of issue with minimal cost.