Skip to content

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

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 #34

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 @@
## 2025-05-15 - Command Injection in Docker Exec Sync
**Vulnerability:** Found a critical command injection vulnerability in `src/pages/api/docker-logs.ts` where unvalidated user query parameters (`id`, `tail`) were directly concatenated into a shell string and executed via `child_process.execSync`.
**Learning:** Node.js `execSync` combined with string interpolation of user parameters inherently leads to shell command injection and simultaneously blocks the entire single-threaded event loop, magnifying the risk with a Denial-of-Service vector.
**Prevention:** Always use `execFile` or `execFileAsync` with an explicit arguments array instead of executing raw shell strings. Additionally, strictly validate and sanitize any parameters (e.g., verifying parameters do not start with a hyphen to prevent flag injection, coercing types like strings to integers where applicable).
43 changes: 32 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,44 @@ export const GET: APIRoute = async ({ url }) => {
}

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

if (!containerId) {
if (!containerId || typeof containerId !== 'string') {
return new Response(JSON.stringify({ error: "ID du conteneur manquant" }), {
status: 400,
headers: { 'Content-Type': 'application/json' }
});
}

// Commande Docker pour récupérer les logs
const command = `docker logs --tail ${tail} ${containerId}`;
let logs = [];
// Prevent argument injection
if (containerId.startsWith('-')) {
return new Response(JSON.stringify({ error: "ID du conteneur invalide" }), {
status: 400,
headers: { 'Content-Type': 'application/json' }
});
}

const tail = parseInt(tailStr, 10);
if (isNaN(tail) || tail < 0) {
return new Response(JSON.stringify({ error: "Paramètre tail invalide" }), {
status: 400,
headers: { 'Content-Type': 'application/json' }
});
}
Comment on lines +35 to +41
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

security-medium medium

While the tail parameter is now parsed as an integer, it lacks an upper bound. A malicious user could provide an extremely large value, which could lead to excessive memory consumption or cause the execFile process to fail due to the default maxBuffer limit (1MB). Enforcing a reasonable maximum limit (e.g., 5000 lines) is recommended to prevent potential Denial-of-Service vectors.

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


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], { timeout: 10000 });
const output = stdout.trim() || stderr.trim();
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

medium

The use of the logical OR operator (||) here means that if stdout contains any data, the content of stderr is completely ignored. Docker containers often output log entries to both streams. It is better to concatenate both outputs to ensure that no log information is lost.

Suggested change
const output = stdout.trim() || stderr.trim();
const output = (stdout + 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');
const stderrStr = err.stderr.toString().trim();
logs = stderrStr ? stderrStr.split('\n') : [];
} else {
throw err;
// Sanitize error to prevent leaking secrets/internal state
throw new Error("Erreur lors de la récupération des logs Docker");
}
}
Comment on lines 48 to 57
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

security-medium medium

In the catch block, err.stderr typically contains the error message from the docker command itself (e.g., "Error: No such container") rather than the container's application logs. Returning this as valid log content with a 200 OK status is misleading and could leak internal system information. Since stderr from the container is already captured in the success case (when combined with stdout), the catch block should only handle actual execution failures by throwing a sanitized error.

    } catch (err: any) {
        // Sanitize error to prevent leaking secrets/internal state
        throw new Error("Erreur lors de la récupération des logs Docker");
    }


Expand All @@ -41,7 +61,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 }), {
// Return sanitized error
return new Response(JSON.stringify({ error: "Logs indisponibles: " + (error.message || "Erreur interne") }), {
status: 500,
headers: { 'Content-Type': 'application/json' }
});
Expand Down