Skip to content

🛡️ Sentinel: [CRITICAL] Fix command injection in multiple API routes #81

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 multiple API routes #81

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-02-27 - [Fix Command Injection in API Routes]
**Vulnerability:** Found multiple API routes (`docker-logs.ts`, `docker-health.ts`, `github-clone.ts`) using `execSync` and `exec` with string interpolation for executing shell commands involving user inputs, leading to potential command injection.
**Learning:** External processes should be spawned safely using explicit arrays of arguments, preventing argument injections.
**Prevention:** Avoid `exec` and `execSync` altogether when running commands with variable parameters. Use `execFile` or `execFileAsync` (`promisify(execFile)`) providing an explicit array for arguments. For Git clone, make sure to separate URL arguments with `--`. Check and validate user inputs with regex.
9 changes: 7 additions & 2 deletions src/pages/api/docker-health.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
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 () => {
try {
const format = '{"ID":"{{.ID}}","Names":"{{.Names}}","Image":"{{.Image}}","Status":"{{.Status}}","State":"{{.State}}","Ports":"{{.Ports}}"}';
const output = execSync(`docker ps -a --format '${format}'`).toString().trim();
// 🛡️ Sentinel: Use execFileAsync with argument arrays instead of execSync to prevent command injection
const { stdout } = await execFileAsync('docker', ['ps', '-a', '--format', format]);
const output = stdout.trim();

if (!output) {
return new Response(JSON.stringify({ containers: [] }), {
Expand Down
28 changes: 22 additions & 6 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,14 +24,26 @@ export const GET: APIRoute = async ({ url }) => {
});
}

// Commande Docker pour récupérer les logs
const command = `docker logs --tail ${tail} ${containerId}`;
// 🛡️ Sentinel: Validate containerId to prevent argument injection
if (!/^[a-zA-Z0-9_.-]+$/.test(containerId)) {
return new Response(JSON.stringify({ error: "ID de conteneur invalide" }), {
status: 400,
headers: { 'Content-Type': 'application/json' }
});
}

const tailNum = parseInt(tail, 10);
const validTail = isNaN(tailNum) ? '100' : String(tailNum);
Comment on lines +35 to +36
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

high

The current validation of tail using parseInt breaks the tail=all functionality, which is a valid and common option in Docker logs. Additionally, it does not handle negative numbers gracefully. We can simplify and secure this by validating that tail is either exactly 'all' or a non-negative integer using a regular expression.

Suggested change
const tailNum = parseInt(tail, 10);
const validTail = isNaN(tailNum) ? '100' : String(tailNum);
const validTail = (tail === 'all' || /^\d+$/.test(tail)) ? tail : '100';


let logs = [];
try {
const output = execSync(command, { stdio: ['pipe', 'pipe', 'pipe'] }).toString();
// 🛡️ Sentinel: Use execFileAsync with argument arrays instead of execSync with string concatenation
// to prevent command injection via shell operators.
const { stdout, stderr } = await execFileAsync('docker', ['logs', '--tail', validTail, containerId]);
// Docker logs may output to stdout or stderr depending on the container
const output = stdout || stderr;
logs = 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 {
Expand All @@ -41,7 +56,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 }), {
// 🛡️ Sentinel: Don't leak raw error details
return new Response(JSON.stringify({ error: "Logs indisponibles" }), {
status: 500,
headers: { 'Content-Type': 'application/json' }
});
Expand Down
32 changes: 19 additions & 13 deletions src/pages/api/github-clone.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import type { APIRoute } from 'astro';
import { exec } from 'node:child_process';
import { execFile } from 'node:child_process';
import util from 'node:util';
import fs from 'node:fs';
import path from 'node:path';
import { getReposRootResolved } from '../../lib/forge-repos';
import { getConfig } from '../../lib/config-db';

const execPromise = util.promisify(exec);
const execFileAsync = util.promisify(execFile);

export const POST: APIRoute = async ({ request }) => {
try {
Expand All @@ -16,6 +16,11 @@ export const POST: APIRoute = async ({ request }) => {
return new Response(JSON.stringify({ error: 'repoUrl et repoName requis' }), { status: 400 });
}

// 🛡️ Sentinel: Validate repoName to prevent directory traversal and command injection
if (!/^[a-zA-Z0-9_.-]+$/.test(repoName) || repoName.includes('..')) {
return new Response(JSON.stringify({ error: 'Nom de dépôt invalide' }), { status: 400 });
}
Comment on lines +20 to +22
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

medium

Instead of writing a custom regex to validate the repository name, you can reuse the existing isSafeRepoDirName helper function from src/lib/forge-repos.ts to ensure consistency and maintainability across the codebase. Note that you will also need to add isSafeRepoDirName to the imports from ../../lib/forge-repos.

Suggested change
if (!/^[a-zA-Z0-9_.-]+$/.test(repoName) || repoName.includes('..')) {
return new Response(JSON.stringify({ error: 'Nom de dépôt invalide' }), { status: 400 });
}
if (!isSafeRepoDirName(repoName) || repoName === '.' || repoName === '..') {
return new Response(JSON.stringify({ error: 'Nom de dépôt invalide' }), { status: 400 });
}


const githubToken = await getConfig('githubToken', true);
if (!githubToken || githubToken.trim() === '') {
return new Response(JSON.stringify({ error: 'Jeton GitHub manquant' }), { status: 400 });
Expand All @@ -30,19 +35,16 @@ export const POST: APIRoute = async ({ request }) => {

// Inject token into URL for private repo cloning
// Assuming format https://github.com/user/repo.git
// 🛡️ Sentinel: Make sure repoUrl is basically valid before injecting
if (!repoUrl.startsWith('https://github.com/')) {
return new Response(JSON.stringify({ error: 'URL Github invalide' }), { status: 400 });
}
const authUrl = repoUrl.replace('https://', `https://oauth2:${githubToken}@`);
Comment on lines +39 to 42
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

security-high high

Using simple string replacement (replace('https://', ...)) and startsWith for URL validation and credential injection can be fragile and prone to parsing discrepancies. Utilizing the built-in URL API is much more robust and secure, ensuring that the protocol is strictly https: and the hostname is strictly github.com, while safely injecting the credentials without manual string manipulation.

    let parsedUrl: URL;
    try {
      parsedUrl = new URL(repoUrl);
      if (parsedUrl.protocol !== 'https:' || parsedUrl.hostname !== 'github.com') {
        return new Response(JSON.stringify({ error: 'URL Github invalide' }), { status: 400 });
      }
    } catch {
      return new Response(JSON.stringify({ error: 'URL Github invalide' }), { status: 400 });
    }
    parsedUrl.username = 'oauth2';
    parsedUrl.password = githubToken;
    const authUrl = parsedUrl.toString();


// Clone the repository
const { stdout, stderr } = await execPromise(`git clone ${authUrl} ${repoName}`, { cwd: reposRoot });

// Try to auto-sync it into the database
try {
const syncScript = path.join(process.cwd(), 'src/pages/api/sync-projects.ts');
// Just ping the sync endpoint internally or rely on the user clicking "Sync"
// For simplicity, we just clone here. The user can click "Synchroniser" on the dashboard.
} catch(e) {
// Ignore sync error
}
// 🛡️ Sentinel: Prevent command injection using execFile instead of exec with string interpolation,
// and use `--` to indicate end of options.
const { stdout, stderr } = await execFileAsync('git', ['clone', '--', authUrl, repoName], { cwd: reposRoot });

return new Response(JSON.stringify({
status: 'ok',
Expand All @@ -53,7 +55,11 @@ export const POST: APIRoute = async ({ request }) => {
});

} catch (error: any) {
return new Response(JSON.stringify({ error: error.message || 'Erreur lors du clonage' }), {
// 🛡️ Sentinel: Prevent token leak in error messages
let msg = error.message || 'Erreur lors du clonage';
msg = msg.replace(/https:\/\/[^@]+@/g, 'https://***@');

return new Response(JSON.stringify({ error: msg }), {
status: 500,
headers: { 'Content-Type': 'application/json' },
});
Expand Down