Skip to content

🛡️ Sentinel: [CRITICAL] Fix command injection in reconcile #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
connerkup wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🛡️ Sentinel: [CRITICAL] Fix command injection in reconcile #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## 2024-05-14 - Fix command injection in Reconciler
**Vulnerability:** A command injection vulnerability was present in the reconciler (`packages/core/src/reconcile.ts`). The `execSync` function was used to perform syntax checking on a temporary file path with string interpolation (`node --check "${tempFile}"`). While `tempFile` itself was relatively constrained, an attacker could potentially manipulate the workspace path to inject arbitrary shell commands.
**Learning:** This codebase previously relied on `execSync` which invokes a shell interpreter, making it vulnerable to command injection if paths are not completely sanitized. Always avoid string-based command execution when user input or variable paths are involved. Furthermore, when refactoring to avoid shell usage, `execFileSync` is often a safer drop-in replacement than `spawnSync` because it preserves the exact Error object structure that existing `try...catch` blocks expect.
**Prevention:** Replaced `execSync` with `execFileSync(process.execPath, ["--check", tempFile])` which avoids shell interpretation altogether by passing arguments as an array directly to the executable. Prefer `execFileSync` over `execSync` to prevent command injection while maintaining error handling compatibility.
4 changes: 2 additions & 2 deletions packages/core/src/reconcile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { execSync } from "node:child_process";
import { execFileSync } from "node:child_process";
import crypto from "node:crypto";
import fs from "node:fs";
import path from "node:path";
Expand Down Expand Up @@ -139,7 +139,7 @@ export function reconcile(options: ReconcileOptions): ReconcileResult {
fs.writeFileSync(tempFile, extractedCode, "utf8");

try {
execSync(`node --check "${tempFile}"`, { stdio: "pipe" });
execFileSync(process.execPath, ["--check", tempFile], { stdio: "pipe" });
log.audit("Syntax check passed");
} catch (err) {
const stderr =
Expand Down
Loading