Skip to content

harden: sanitize child_process call in init.mjs... - #53

Merged
arozumenko merged 2 commits into
arozumenko:mainfrom
anupamme:fix-repo-sdlc-skills-child-process-command-injection-init-mjs
Aug 9, 2026
Merged

harden: sanitize child_process call in init.mjs...#53
arozumenko merged 2 commits into
arozumenko:mainfrom
anupamme:fix-repo-sdlc-skills-child-process-command-injection-init-mjs

Conversation

@anupamme

@anupamme anupamme commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Harden input handling in bin/init.mjs (flagged by semgrep).

Vulnerability

Field Value
ID javascript.lang.security.detect-child-process.detect-child-process
Severity HIGH
Scanner semgrep
Rule javascript.lang.security.detect-child-process.detect-child-process
File bin/init.mjs:945
Assessment Defensive hardening

Description: Detected calls to child_process from a function argument ref. This could lead to a command injection if the input is user controllable. Try to avoid calls to child_process, and if it is needed ensure user input is correctly sanitized or sandboxed.

Threat Model Context

This is a Node.js library - vulnerabilities affect downstream consumers who use this package.

Changes

  • bin/init.mjs

Behavior Preservation

The change is scoped to 1 file on the vulnerable path; it only tightens handling of untrusted input and leaves valid inputs unaffected.


This patch removes an exploit primitive — a code pattern that, while not independently exploitable today, could be chained with other weaknesses by automated exploit-development tooling. Proactive removal of such primitives raises the bar against increasingly capable automated attack tools.


Automated security fix by OrbisAI Security

…ss security vulnerability

Automated security fix generated by OrbisAI Security
@arozumenko

Copy link
Copy Markdown
Owner

Thanks — the execSyncexecFileSync swap is the right fix and I'm happy to merge it. One tightening request before it goes in:

The ref validator regex ^[a-zA-Z0-9._\-\/]+$ allows a leading dash. Since execFileSync passes argv directly to git (no shell), a ref like --upload-pack or --output would be interpreted by git as an option rather than a ref — argument injection rather than command injection. Impact is low (no shell reachable), but it's a cheap gap to close.

Two options, either is fine:

  1. Reject leading dashes in the guard:
    if (!/^[a-zA-Z0-9._\/]+$/.test(ref) || ref.startsWith('-')) {
  2. Or use -- end-of-options before the ref in each git invocation, e.g.:
    execFileSync('git', ['-C', dest, 'fetch', '--quiet', '--depth', '1', 'origin', '--', ref], ...)
    execFileSync('git', ['clone', '--quiet', '--depth', '1', '--branch', ref, '--', `https://github.com/${repo}`, dest], ...)
    (note --branch still consumes ref as its value, so option 1 is the cleaner guard here).

Prefer option 1 — a single ref.startsWith('-') check in the existing guard. Once that's in, LGTM.

@arozumenko arozumenko left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

commented

…lowClone

The ref validator regex allowed a leading dash, so a ref like
--upload-pack would pass the guard and be forwarded to git as an
option flag (argument injection, no shell required). Add a
ref.startsWith('-') check alongside the regex and clean up the
redundant \- escape in the character class.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@anupamme

anupamme commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Review comments addressed. Pls review.

@arozumenko

Copy link
Copy Markdown
Owner

Confirmed — commit 952bbcb closes it. The ref.startsWith('-') guard now rejects leading-dash refs (--upload-pack etc.), and the redundant \- escape cleanup is a nice touch. Argument-injection gap is closed and there's no shell on the path anymore. LGTM 👍

@arozumenko
arozumenko merged commit 48a0ea1 into arozumenko:main Aug 9, 2026
3 checks passed
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