Skip to content

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

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

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-06-01 - Fix Command Injection in API endpoints using Docker logs
**Vulnerability:** The API route `src/pages/api/docker-logs.ts` constructed shell commands using unvalidated, unsanitized user input (`id` and `tail` query parameters) directly into `execSync`, creating a severe command injection vulnerability.
**Learning:** Shell-based execution (`execSync` or `exec`) implicitly runs commands within a shell environment where operators like `;` and `&&` evaluate, allowing an attacker to inject arbitrary system commands via user-supplied parameters if not strictly validated or escaped.
**Prevention:** Always use `execFile` or `execFileAsync` which execute specific binaries directly and accept an array of arguments, bypassing shell parsing. Additionally, strictly validate all query parameters (e.g., using regex `^[a-zA-Z0-9_-]+$`) to prevent flag/argument injection.
26 changes: 21 additions & 5 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 @@ -21,12 +24,25 @@ export const GET: APIRoute = async ({ url }) => {
});
}

// Commande Docker pour récupérer les logs
const command = `docker logs --tail ${tail} ${containerId}`;
// Validation stricte pour éviter l'injection de commandes/flags
if (!/^[a-zA-Z0-9_-]+$/.test(containerId) || !/^\d+$/.test(tail)) {
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 1, 2026

Choose a reason for hiding this comment

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

high

Issue: Overly Restrictive Regex Validation

Docker container names can contain dots (.) and often start with a leading slash (/) when retrieved from the Docker API (e.g., /my-container). The current regex /^[a-zA-Z0-9_-]+$/ will reject these valid names, causing the API to return a 400 error and breaking the logs view for those containers.

Additionally, the tail parameter in Docker logs can accept "all" to retrieve all logs. Restricting it to only digits /^\d+$/ prevents this valid use case.

Recommendation

Update the regex to:

  1. Allow an optional leading slash and dots in the container ID/name.
  2. Ensure the container ID/name starts with an alphanumeric character (or a slash followed by one) to prevent flag injection (e.g., passing --help as a container ID).
  3. Allow "all" as a valid value for tail.
Suggested change
if (!/^[a-zA-Z0-9_-]+$/.test(containerId) || !/^\d+$/.test(tail)) {
if (!/^\/?[a-zA-Z0-9][a-zA-Z0-9_.-]*$/.test(containerId) || !/^(?:\d+|all)$/.test(tail)) {

return new Response(JSON.stringify({ error: "Paramètres invalides" }), {
status: 400,
headers: { 'Content-Type': 'application/json' }
});
}

// Utilisation d'un tableau d'arguments pour empêcher l'injection via shell
let logs = [];
try {
const output = execSync(command, { stdio: ['pipe', 'pipe', 'pipe'] }).toString();
logs = output.trim().split('\n');
const { stdout, stderr } = await execFileAsync('docker', ['logs', '--tail', tail, containerId]);
// Docker logs output can be stdout or stderr
logs = stdout ? stdout.trim().split('\n') : [];
if (stderr && stderr.trim().length > 0 && logs.length === 0) {
logs = stderr.trim().split('\n');
} else if (stderr && stderr.trim().length > 0) {
logs.push(...stderr.trim().split('\n'));
}
Comment on lines +40 to +45
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jun 1, 2026

Choose a reason for hiding this comment

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

medium

Improvement: Simplify stdout/stderr Merging

The current logic for merging stdout and stderr is verbose and can introduce empty strings into the logs array if stdout or stderr contains only whitespace.

Using filter(Boolean) simplifies the merging logic, improves readability, and ensures no empty lines are added.

        const stdoutLines = stdout ? stdout.trim().split('\n').filter(Boolean) : [];
        const stderrLines = stderr ? stderr.trim().split('\n').filter(Boolean) : [];
        logs = [...stdoutLines, ...stderrLines];

} catch (err: any) {
// Certains logs sortent sur stderr, checkons stderr si stdout est vide ou si erreur
if (err.stderr) {
Expand Down