🛡️ Sentinel: [CRITICAL] Fix command injection in docker-logs#62
Conversation
Replaced `execSync` with `execFileAsync` and passed arguments as an array to prevent shell command injection when using `docker logs`. Added validation to reject container IDs that start with hyphens to prevent flag injection, and ensured the tail parameter is a safe integer. 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 execFileAsync and implementing strict validation for the containerId and tail parameters. It also includes documentation of the fix and an increased timeout for unit tests. Feedback focuses on removing temporary utility scripts from the repository and refining the log parsing logic to correctly handle empty output by avoiding single-element arrays containing empty strings.
| @@ -0,0 +1,61 @@ | |||
| import { readFileSync, writeFileSync } from 'fs'; | |||
There was a problem hiding this comment.
This file appears to be a temporary script used to apply changes to the codebase. Such artifacts should not be committed to the repository as they clutter the source tree and are not part of the application's runtime or build process.
| @@ -0,0 +1,9 @@ | |||
| import { readFileSync, writeFileSync } from 'fs'; | |||
There was a problem hiding this comment.
This file seems to be a scratchpad or utility script used during development to modify tests. It should be removed from the pull request to maintain a clean repository structure.
| { maxBuffer: 10 * 1024 * 1024 }, | ||
| ); | ||
| const output = stdout || stderr; // Docker logs often outputs to stderr | ||
| logs = output.trim().split("\n"); |
There was a problem hiding this comment.
When the output string is empty (after trimming), split("\n") will return an array containing a single empty string [""] instead of an empty array []. This might lead to unexpected empty lines being displayed in the UI.
| logs = output.trim().split("\n"); | |
| logs = output.trim() ? output.trim().split("\n") : []; |
| } | ||
| // Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur | ||
| if (err.stderr) { | ||
| logs = err.stderr.toString().trim().split("\n"); |
There was a problem hiding this comment.
Similar to the success case, if err.stderr results in an empty string after trimming, split("\n") will return [""]. It is better to return an empty array in such cases.
| logs = err.stderr.toString().trim().split("\n"); | |
| logs = err.stderr.toString().trim() ? err.stderr.toString().trim().split("\n") : []; |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Command injection in
src/pages/api/docker-logs.tsthrough un-sanitizedcontainerIdandtailquery parameters.🎯 Impact: Attackers could inject arbitrary shell commands, potentially gaining remote code execution on the server.
🔧 Fix: Replaced
execSyncwithexecFileAsyncand passed arguments as a strictly formatted array, avoiding shell interpolation. Added flag injection mitigation via explicit hyphen checking on inputs.✅ Verification: Ran
pnpm testsuccessfully. Prettier formatted. Journal entry created in.jules/sentinel.md.PR created automatically by Jules for task 13569164561529663768 started by @bobdivx