🛡️ Sentinel: [CRITICAL] Fix command injection in docker logs API#74
🛡️ Sentinel: [CRITICAL] Fix command injection in docker logs API#74bobdivx wants to merge 1 commit into
Conversation
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 Docker Logs API by replacing execSync with execFileSync, adding strict input validation for the container ID and tail parameters, and sanitizing error messages. The review feedback recommends replacing the synchronous process execution with an asynchronous execFile to avoid blocking the Node.js event loop, and updating the container ID validation regex to allow dots, which are valid characters in Docker container names.
| import type { APIRoute } from "astro"; | ||
| import { execFileSync } from "child_process"; |
There was a problem hiding this comment.
Using synchronous process execution (execFileSync) in an API route blocks the Node.js event loop. This can lead to severe performance degradation or Denial of Service (DoS) when multiple requests are handled concurrently, especially since container logs can be large or slow to retrieve. Switch to the asynchronous execFile wrapped in a Promise instead.
import type { APIRoute } from "astro";
import { execFile } from "child_process";
import { promisify } from "util";
const execFileAsync = promisify(execFile);| let logs = []; | ||
| try { | ||
| const output = execSync(command, { stdio: ['pipe', 'pipe', 'pipe'] }).toString(); | ||
| logs = output.trim().split('\n'); | ||
| // Using execFileSync with an argument array to prevent shell injection | ||
| const output = execFileSync( | ||
| "docker", | ||
| ["logs", "--tail", String(tail), "--", containerId], | ||
| { stdio: ["pipe", "pipe", "pipe"] }, | ||
| ).toString(); | ||
| logs = output.trim().split("\n"); | ||
| } catch (err: any) { | ||
| // Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur | ||
| if (err.stderr) { | ||
| logs = err.stderr.toString().trim().split('\n'); | ||
| } else { | ||
| throw err; | ||
| } | ||
| // Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur | ||
| if (err.stderr) { | ||
| logs = err.stderr.toString().trim().split("\n"); | ||
| } else { | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace execFileSync with the asynchronous execFileAsync to avoid blocking the Node.js event loop during process execution.
let logs = [];
try {
// Using execFileAsync with an argument array to prevent shell injection and avoid blocking the event loop
const { stdout } = await execFileAsync(
"docker",
["logs", "--tail", String(tail), "--", containerId]
);
logs = stdout.toString().trim().split("\n");
} catch (err: any) {
// Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur
if (err.stderr) {
logs = err.stderr.toString().trim().split("\n");
} else {
throw err;
}
}| } | ||
|
|
||
| // Input validation: prevent command argument injection (no flags/unsupported chars) | ||
| if (!/^[a-zA-Z0-9_-]+$/.test(containerId)) { |
There was a problem hiding this comment.
Docker container names can contain dots (.) (e.g., my.container.name or reverse-DNS style names). The current regex /^[a-zA-Z0-9_-]+$/ will reject these valid container names. Update the regex to allow dots.
| if (!/^[a-zA-Z0-9_-]+$/.test(containerId)) { | |
| if (!/^[a-zA-Z0-9_.-]+$/.test(containerId)) { |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Command injection in
src/pages/api/docker-logs.ts. User input foridandtailwere passed unvalidated toexecSyncvia string interpolation.🎯 Impact: High. An attacker could run arbitrary shell commands on the system by crafting malicious input in the
idparameter.🔧 Fix: Switched from
execSynctoexecFileSyncusing an argument array to bypass shell interpretation. Added robust input validation to ensure theidmatches an alphanumeric/dash regex andtailis an integer. Appended--before the container ID to prevent argument/flag injection. Sanitized the returned error string.✅ Verification: Ran
pnpm run checkandpnpm test. Executed a local injection test script to verify that shell commands embedded inidare now correctly passed as a single string to thedocker logsprocess rather than evaluated.PR created automatically by Jules for task 12882890723391972388 started by @bobdivx