Skip to content

🛡️ Sentinel: [CRITICAL] Fix command injection in docker logs API #65

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
bobdivx wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

🛡️ Sentinel: [CRITICAL] Fix command injection in docker logs API #65

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-24 - [CRITICAL] Command Injection in docker logs API
**Vulnerability:** The API endpoint `src/pages/api/docker-logs.ts` utilized `execSync` with unsanitized user inputs (`id` and `tail` query parameters) concatenated directly into a shell command string, creating a critical risk of arbitrary command execution.
**Learning:** Shell strings generated with external user input inherently trust that input to not contain shell metacharacters or secondary commands (e.g., using `;` or `&&`). Relying solely on `execSync` without passing an arguments array bypasses proper encoding.
**Prevention:** Always use `execFile` (or its async wrapper) with an array of arguments for external commands instead of string concatenation. Validate and strictly cast types (e.g., parsing integers for numeric limits), sanitize error outputs to avoid stack trace leaks, and block flags from being injected by rejecting inputs starting with hyphens (`-`).
35 changes: 24 additions & 11 deletions src/pages/api/docker-logs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import type { APIRoute } from 'astro';
import { execSync } from 'child_process';
import { execFile } from 'child_process';
import { promisify } from 'util';

const execFileAsync = promisify(execFile);

export const GET: APIRoute = async ({ url }) => {
try {
Expand All @@ -12,27 +15,36 @@ export const GET: APIRoute = async ({ url }) => {
}

const containerId = url.searchParams.get('id');
const tail = url.searchParams.get('tail') || '100';
const tailParam = url.searchParams.get('tail') || '100';

// 🛡️ Security: Validate input and prevent flag injection
if (!containerId || containerId.startsWith('-')) {
return new Response(JSON.stringify({ error: "ID du conteneur invalide ou manquant" }), {
status: 400,
headers: { 'Content-Type': 'application/json' }
});
}

if (!containerId) {
return new Response(JSON.stringify({ error: "ID du conteneur manquant" }), {
const tail = parseInt(tailParam, 10);
if (isNaN(tail)) {
return new Response(JSON.stringify({ error: "Paramètre tail invalide" }), {
status: 400,
headers: { 'Content-Type': 'application/json' }
});
}
Comment on lines +28 to 34
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Validate that the tail parameter is a non-negative integer. If a negative value is passed, it could lead to unexpected behavior or errors when passed to the docker logs command.

Suggested change
const tail = parseInt(tailParam, 10);
if (isNaN(tail)) {
return new Response(JSON.stringify({ error: "Paramètre tail invalide" }), {
status: 400,
headers: { 'Content-Type': 'application/json' }
});
}
const tail = parseInt(tailParam, 10);
if (isNaN(tail) || tail < 0) {
return new Response(JSON.stringify({ error: "Paramètre tail invalide" }), {
status: 400,
headers: { 'Content-Type': 'application/json' }
});
}


// Commande Docker pour récupérer les logs
const command = `docker logs --tail ${tail} ${containerId}`;
let logs = [];
// 🛡️ Security: Use execFile to prevent command injection
let logs: string[] = [];
try {
const output = execSync(command, { stdio: ['pipe', 'pipe', 'pipe'] }).toString();
logs = output.trim().split('\n');
const { stdout, stderr } = await execFileAsync('docker', ['logs', '--tail', tail.toString(), '--', containerId]);
const output = stdout.trim() || stderr.trim();
logs = output ? output.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;
throw new Error("Execution failed");
}
}
Comment on lines +37 to 49
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There are two issues with the current implementation of log retrieval and error handling:

  1. Data Loss on Success: If a container writes to both stdout and stderr, stdout.trim() || stderr.trim() will only return stdout.trim(), completely discarding all stderr logs.
  2. Incorrect Error Handling: Since execFileAsync resolves when the process exits with 0, any logs written to stderr during a successful run are already present in stderr of the resolved promise. If the process exits with a non-zero code (e.g., container not found), execFileAsync throws an error, and err.stderr contains the CLI error message (e.g., "Error: No such container: ..."). Returning err.stderr as logs with a 200 OK status is incorrect and leaks system errors. Instead, we should propagate the error so the outer catch block can return a clean 500 response.
    let logs: string[] = [];
    try {
        const { stdout, stderr } = await execFileAsync('docker', ['logs', '--tail', tail.toString(), '--', containerId]);
        const output = [stdout.trim(), stderr.trim()].filter(Boolean).join('
');
        logs = output ? output.split('
') : [];
    } catch (err: any) {
        throw new Error("Execution failed");
    }


Expand All @@ -41,7 +53,8 @@ export const GET: APIRoute = async ({ url }) => {
headers: { 'Content-Type': 'application/json' }
});
} catch (error: any) {
return new Response(JSON.stringify({ error: "Logs indisponibles: " + error.message }), {
// 🛡️ Security: Sanitize error message to prevent leaking system details
return new Response(JSON.stringify({ error: "Logs indisponibles" }), {
status: 500,
headers: { 'Content-Type': 'application/json' }
});
Expand Down