π‘οΈ Sentinel: [HIGH] Fix command injection risks by removing execSync#272
π‘οΈ Sentinel: [HIGH] Fix command injection risks by removing execSync#272google-labs-jules[bot] wants to merge 1 commit into
Conversation
Replaced multiple instances of `execSync` with `execFileSync` across the codebase (including `cli/commands/score.mjs`, `cli/commands/setup.mjs`, `cli/ensure-skills.mjs`, `cli/validators/freshness.mjs`, and `cli/shared-git.mjs`) to eliminate shell execution and prevent potential command injection vulnerabilities from dynamic or uncontrolled inputs. - Migrated shell pipes and redirects to native JS equivalents and `stdio` configurations. - explicitly set `maxBuffer` for heavy git operations to avoid `ENOBUFS`. - Kept zero-dependency constraint.
|
π Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a π emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
π¨ Severity: HIGH
π‘ Vulnerability:
Multiple instances of
execSyncwere used throughout the CLI to execute system commands (git,where,which).execSyncspawns a shell to interpret the command string, which is vulnerable to command injection if any interpolated variables (like paths or filenames) contain shell metacharacters (e.g.,;,&&,|,$()). While some uses were static, the pattern itself is unsafe and risks injection if inputs are ever derived from user-controlled sources or external config files (like.specify/init-options.jsonpreviously).π― Impact:
An attacker who can control input passed into an
execSynccommand (such as via a malformed repository name, directory path, or config file read during initialization) could execute arbitrary shell commands on the user's machine with the privileges of the Node.js process.π§ Fix:
execSyncwithexecFileSyncincli/commands/score.mjs,cli/commands/setup.mjs,cli/ensure-skills.mjs,cli/validators/freshness.mjs, andcli/shared-git.mjs.execFileSyncto bypass shell interpolation completely.| wc -l) natively in JavaScript (.split('\n').filter(Boolean).length).2>/dev/null) using Node's nativestdio: ['pipe', 'pipe', 'ignore']option.maxBuffer(1024 * 1024 * 5) forgit logoperations to preventENOBUFS(buffer exhaustion) errors that can occur when reading large histories natively without a shell pipe.β Verification:
node --test tests/*.test.mjs), verifying that CLI logic, git detection (isGitRepo), and system checking (isCliAvailable) remain functional.execFileSyncwas correctly imported in all modified files.PR created automatically by Jules for task 2584548517710371358 started by @raccioly