🛡️ Sentinel: [CRITICAL] Fix command injection in docker logs endpoint#58
🛡️ Sentinel: [CRITICAL] Fix command injection in docker logs endpoint#58bobdivx wants to merge 1 commit into
Conversation
- Replaced vulnerable `execSync` with `execFileAsync` to prevent shell evaluation. - Added explicit parameter validation against flag injection (`-`). - Sanitized `error.message` in catch block to prevent data leakage. - Logged critical learning in `.jules/sentinel.md`. Co-authored-by: bobdivx <6737167+bobdivx@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 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. |
![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 endpoint by replacing execSync with execFileAsync and implementing input validation for the container ID. It also adds a sentinel file documenting the vulnerability and prevention strategies. Feedback focuses on further hardening the tail parameter validation, refining the concatenation of stdout and stderr to avoid empty lines, and improving error handling to return appropriate HTTP status codes when the Docker command fails.
| const containerId = url.searchParams.get('id'); | ||
| const tail = url.searchParams.get('tail') || '100'; | ||
| const containerId = url.searchParams.get("id"); | ||
| const tail = url.searchParams.get("tail") || "100"; |
There was a problem hiding this comment.
While execFileAsync prevents shell injection, the tail parameter should still be validated to ensure it is either a positive integer or the string 'all'. This prevents potential argument injection or unexpected behavior if a malicious flag is passed as the value for the --tail argument.
const tail = url.searchParams.get("tail") || "100";
if (tail !== "all" && !/^\d+$/.test(tail)) {
return new Response(
JSON.stringify({ error: "Paramètre tail invalide" }),
{
status: 400,
headers: { "Content-Type": "application/json" },
},
);
}| const output = | ||
| stdout.toString() + (stderr ? "\n" + stderr.toString() : ""); | ||
| logs = output.trim() ? output.trim().split("\n") : []; |
There was a problem hiding this comment.
The current logic for combining stdout and stderr manually adds a newline, which can result in empty lines in the output if both streams already contain trailing newlines (which is common for Docker logs). Simply concatenating the outputs and trimming the result is more robust.
| const output = | |
| stdout.toString() + (stderr ? "\n" + stderr.toString() : ""); | |
| logs = output.trim() ? output.trim().split("\n") : []; | |
| const output = stdout.toString() + stderr.toString(); | |
| logs = output.trim() ? 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; | ||
| } | ||
| if (err.stderr) { | ||
| logs = err.stderr.toString().trim().split("\n"); | ||
| } else { | ||
| throw new Error("Erreur d'exécution Docker"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current error handling returns a 200 OK status even when the Docker command fails (e.g., if the container does not exist), including the error message in the logs array. It would be more appropriate to return a proper error status code (e.g., 404 or 400) when the command fails to execute correctly.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The
src/pages/api/docker-logs.tsendpoint was vulnerable to Remote Code Execution (RCE) via command injection. ThecontainerIdandtailquery parameters were interpolated directly into a shell string passed toexecSync().🎯 Impact: An attacker could execute arbitrary commands on the host by supplying a malicious
id(e.g.,id=123; rm -rf /;).🔧 Fix: Refactored the endpoint to use
execFileAsync(withutil.promisify), passing arguments as a strictly formatted array, eliminating shell evaluation entirely. Added validation to prevent argument/flag injection (rejectingcontainerIdstarting with-). Sanitized error outputs to avoid exposing internals.✅ Verification: Ran
pnpm run checkand vitest. Validated code format with Prettier.Additionally, recorded this critical vulnerability, the learning, and prevention strategies in
.jules/sentinel.md.PR created automatically by Jules for task 16260930353450927475 started by @bobdivx