Skip to content

fix: prevent command injection in isRepoPrivate()#973

Open
Aakeeo wants to merge 2 commits intoNVIDIA:mainfrom
Aakeeo:fix/command-injection-isRepoPrivate
Open

fix: prevent command injection in isRepoPrivate()#973
Aakeeo wants to merge 2 commits intoNVIDIA:mainfrom
Aakeeo:fix/command-injection-isRepoPrivate

Conversation

@Aakeeo
Copy link
Copy Markdown

@Aakeeo Aakeeo commented Mar 26, 2026

Replace execSync (shell string interpolation) with execFileSync (argument array) in isRepoPrivate() to prevent shell metacharacter injection via the repo parameter.

Summary

isRepoPrivate() in bin/lib/credentials.js passes the repo parameter directly into an execSync() template literal, which spawns a shell. Although the only current call site uses a hardcoded string, the function is
exported and accepts arbitrary input — any future caller passing user-derived data (e.g. a git remote, CLI argument) would allow arbitrary command execution. This replaces it with execFileSync() which bypasses the shell
entirely.

Related Issue

N/A — discovered during a security audit of the codebase.

Changes

  • Replace execSync with execFileSync in isRepoPrivate() to pass arguments as an array instead of interpolating into a shell string
  • Add execFileSync to the child_process import (retain execSync for the existing gh auth token call in ensureGithubToken())

Type of Change

  • Code change for a new feature, bug fix, or refactor.
  • Code change with doc updates.
  • Doc only. Prose changes without code sample modifications.
  • Doc only. Includes code sample changes.

Testing

  • npx prek run --all-files passes (or equivalently make check).
  • npm test passes.
  • make docs builds without warnings. (for doc-only changes)

Checklist

General

Code Changes

  • Formatters applied — npx prek run --all-files auto-fixes formatting (or make format for targeted runs).
  • Tests added or updated for new or changed behavior.
  • No secrets, API keys, or credentials committed.
  • Doc pages updated for any user-facing behavior changes (new commands, changed defaults, new features, bug fixes that contradict existing docs).

Doc Changes

N/A

Summary by CodeRabbit

  • Refactor
    • Improved reliability of repository authentication and privacy checks performed by the tool.
    • Made CLI interactions more robust and quieter, reducing spurious console output during background checks.
    • No changes to public interfaces or user-facing configuration; behavior and control flow remain unchanged.

Replace execSync (shell string interpolation) with execFileSync
(argument array) in isRepoPrivate() to prevent shell metacharacter
injection via the repo parameter.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 683f0e13-0d9c-4674-9327-f668f0de4bce

📥 Commits

Reviewing files that changed from the base of the PR and between 9b52213 and 0f0f8b1.

📒 Files selected for processing (1)
  • bin/lib/credentials.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • bin/lib/credentials.js

📝 Walkthrough

Walkthrough

bin/lib/credentials.js updates subprocess calls in isRepoPrivate() and ensureGithubToken() to use execFileSync with argument arrays and explicit stdio configuration instead of shell-invoked execSync with inline redirection.

Changes

Cohort / File(s) Summary
Credentials subprocess invocation
bin/lib/credentials.js
Replaced execSync with execFileSync for GitHub CLI calls in isRepoPrivate() and ensureGithubToken(); converted command strings to argument arrays (e.g., ["api", "repos/${repo}", "--jq", ".private"], ["auth", "token"]); replaced shell stderr redirection with explicit stdio: ["ignore","pipe","ignore"].

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I nibbled the shell, then hopped to a file,
Arguments tidy in neat little pile,
No wild redirects, just pipes running clean,
A rabbit-approved, quieter machine. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: prevent command injection in isRepoPrivate()' accurately and specifically summarizes the main security fix in the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
bin/lib/credentials.js (1)

195-195: Consider updating to execFileSync for consistency.

While this line has no user input and isn't vulnerable, switching to execFileSync would align with the pattern established in isRepoPrivate() and provide defense-in-depth.

♻️ Optional consistency refactor
-    token = execSync("gh auth token 2>/dev/null", { encoding: "utf-8" }).trim();
+    token = execFileSync("gh", ["auth", "token"], {
+      encoding: "utf-8",
+      stdio: ["ignore", "pipe", "ignore"],
+    }).trim();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bin/lib/credentials.js` at line 195, Replace the use of execSync when
populating the token variable with execFileSync for consistency and
defense-in-depth: call execFileSync with the program "gh" and arguments
["auth","token"] (preserving the { encoding: "utf-8" } and redirection behavior)
and ensure the module imports/uses execFileSync from child_process instead of
execSync; locate the token = execSync(...) assignment in bin/lib/credentials.js
and update it to use execFileSync similarly to the pattern used in
isRepoPrivate().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@bin/lib/credentials.js`:
- Line 195: Replace the use of execSync when populating the token variable with
execFileSync for consistency and defense-in-depth: call execFileSync with the
program "gh" and arguments ["auth","token"] (preserving the { encoding: "utf-8"
} and redirection behavior) and ensure the module imports/uses execFileSync from
child_process instead of execSync; locate the token = execSync(...) assignment
in bin/lib/credentials.js and update it to use execFileSync similarly to the
pattern used in isRepoPrivate().

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2c55a4e3-6815-43c6-a5f6-d4fa49c82b75

📥 Commits

Reviewing files that changed from the base of the PR and between f0f53e4 and 9b52213.

📒 Files selected for processing (1)
  • bin/lib/credentials.js

Address CodeRabbit review feedback — switch the remaining execSync
call in ensureGithubToken() to execFileSync and drop the now-unused
execSync import.

Co-Authored-By: Claude Opus 4.6 (1M context) <[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.

1 participant