🛡️ Sentinel: [CRITICAL] Fix command injection in pm2 API route#21
🛡️ Sentinel: [CRITICAL] Fix command injection in pm2 API route#21bobdivx wants to merge 1 commit into
Conversation
Replaced `exec` with `execFile` passing child process arguments as arrays, and validated that `appName` does not start with a hyphen to prevent flag injection, closing a shell command injection vector. Co-authored-by: bobdivx <6737167+bobdivx@users.noreply.github.com>
|
👋 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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a command injection vulnerability in the PM2 API by replacing shell execution with execFile, validating application names to prevent flag injection, and improving environment variable handling. Feedback identifies further security enhancements, such as sanitizing sensitive environment variables like NODE_OPTIONS and validating scriptPath against path traversal. Additionally, reviewers recommended using platform-specific command names for npm and npx to ensure compatibility with Windows environments.
| } | ||
|
|
||
| // Prepare secure environment dictionary instead of string concatenation | ||
| const childEnv = { ...process.env, ...(env || {}) }; |
There was a problem hiding this comment.
Merging user-provided environment variables directly into the child process environment is a security risk. Attackers can use variables like NODE_OPTIONS or LD_PRELOAD to execute arbitrary code or bypass security constraints. Sanitize the env object by removing sensitive keys.
| } else { | ||
| command = `cd ${scriptPath} && ${envString} npx -y pm2 start npm --name "${appName}" -- run ${npmScript}`; | ||
| // 🛡️ Sentinel: Using execFile with array of args to prevent shell injection | ||
| await execFilePromise('npm', ['run', 'build'], { cwd: scriptPath, env: childEnv }); |
There was a problem hiding this comment.
The scriptPath is used as the cwd for command execution without validation, which could lead to Path Traversal vulnerabilities. Ensure scriptPath is restricted to allowed project directories. Additionally, use the platform-aware npmCmd for Windows compatibility.
| import fs from 'node:fs'; | ||
|
|
||
| const execPromise = util.promisify(exec); | ||
| const execFilePromise = util.promisify(execFile); |
There was a problem hiding this comment.
To ensure cross-platform compatibility (especially on Windows), it's better to define platform-specific command names for npm and npx, as execFile cannot directly execute .cmd files without them.
const execFilePromise = util.promisify(execFile);
const npmCmd = process.platform === 'win32' ? 'npm.cmd' : 'npm';
const npxCmd = process.platform === 'win32' ? 'npx.cmd' : 'npx';| } else { | ||
| return new Response(JSON.stringify({ error: 'Unknown action' }), { status: 400 }); | ||
|
|
||
| const { stdout, stderr } = await execFilePromise('npx', ['-y', 'pm2', 'start', 'npm', '--name', appName, '--', 'run', npmScript], { cwd: scriptPath, env: childEnv }); |
There was a problem hiding this comment.
Use the platform-aware npxCmd defined earlier to ensure this works on Windows systems.
| const { stdout, stderr } = await execFilePromise('npx', ['-y', 'pm2', 'start', 'npm', '--name', appName, '--', 'run', npmScript], { cwd: scriptPath, env: childEnv }); | |
| const { stdout, stderr } = await execFilePromise(npxCmd, ['-y', 'pm2', 'start', 'npm', '--name', appName, '--', 'run', npmScript], { cwd: scriptPath, env: childEnv }); |
| else return new Response(JSON.stringify({ error: 'Unknown action' }), { status: 400 }); | ||
|
|
||
| // 🛡️ Sentinel: Using execFile with array of args to prevent shell injection | ||
| const { stdout, stderr } = await execFilePromise('npx', ['-y', 'pm2', pm2Action, appName]); |
There was a problem hiding this comment.
Use the platform-aware npxCmd defined earlier to ensure this works on Windows systems.
| const { stdout, stderr } = await execFilePromise('npx', ['-y', 'pm2', pm2Action, appName]); | |
| const { stdout, stderr } = await execFilePromise(npxCmd, ['-y', 'pm2', pm2Action, appName]); |
| export const GET: APIRoute = async () => { | ||
| try { | ||
| const { stdout } = await execPromise('npx -y pm2 jlist'); | ||
| const { stdout } = await execFilePromise('npx', ['-y', 'pm2', 'jlist']); |
There was a problem hiding this comment.
Use the platform-aware npxCmd defined earlier to ensure this works on Windows systems.
| const { stdout } = await execFilePromise('npx', ['-y', 'pm2', 'jlist']); | |
| const { stdout } = await execFilePromise(npxCmd, ['-y', 'pm2', 'jlist']); |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The
src/pages/api/pm2.tsroute concatenated user-provided values (likeappName,scriptPath, andenvparameters) directly into shell commands wrapped inexec. This allows trivial shell injection (e.g.; rm -rf /; #).🎯 Impact: An attacker with access to this endpoint could execute arbitrary commands on the host operating system with the privileges of the Node.js process.
🔧 Fix: Refactored the underlying command executions to use
execFile, utilizing arrays for arguments instead of a single string for the entire execution. Passed the directory path via thecwdoptions parameter and passed environment variables usingenvparameters. Explicitly rejectappNamethat start with-to guard against flag injections.✅ Verification: Ran
pnpm testandpnpm run checkand read through.jules/sentinel.mdjournal file. Tested logic flow on local instance.PR created automatically by Jules for task 6910222388751855960 started by @bobdivx