Skip to content

🛡️ Sentinel: [CRITICAL] Fix command injection and secret leak in github clone API #71

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 and secret leak in github clone API #71

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-03-09 - Sanitize Error Messages for Shell Commands with Tokens
**Vulnerability:** Command execution error messages (like `git clone`) can leak sensitive authentication tokens injected into the URL/command if the raw `error.message` is returned directly to the client.
**Learning:** Always sanitize output strings (using regex or redaction) before sending error messages derived from system processes to the frontend API responses, especially when tokens are involved.
**Prevention:** Use `.replace(/https:\/\/[^@]+@/g, 'https://***@')` or similar patterns in catch blocks when handling process errors containing URLs with embedded credentials.
5 changes: 5 additions & 0 deletions pr_body.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
🚨 **Severity:** CRITICAL
💡 **Vulnerability:** The API endpoint `github-clone.ts` used `exec` for cloning repositories, which parses inputs via the shell, exposing the application to Command Injection vulnerabilities. Furthermore, failure cases directly exposed the `error.message` to the client, leading to a potential leak of the Github OAuth token embedded within the clone URL.
🎯 **Impact:** An attacker could execute arbitrary code on the server by crafting malicious inputs in the `repoUrl` or `repoName` payload. Additionally, token leakage compromises the Github account's security.
🔧 **Fix:** Replaced `exec` with `execFile`, utilizing an argument array `['clone', '--', authUrl, repoName]` which isolates inputs strictly as arguments and prevents option injection via `--`. Intercepted the error payload in the catch block to sanitize output with `.replace(/https:\/\/[^@]+@/g, 'https://***@')`, preventing secret spillage. Added a journal entry to `.jules/sentinel.md` regarding proper process output sanitization.
✅ **Verification:** Ran `pnpm run check` and `pnpm test` (vitest) to confirm functional stability. The patch uses exact replacement methodologies to ensure identical behavior with strict shell isolation.
15 changes: 10 additions & 5 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 execFilePromise = util.promisify(execFile);

export const POST: APIRoute = async ({ request }) => {
try {
Expand All @@ -32,8 +32,9 @@ export const POST: APIRoute = async ({ request }) => {
// Assuming format https://github.com/user/repo.git
const authUrl = repoUrl.replace('https://', `https://oauth2:${githubToken}@`);

// Clone the repository
const { stdout, stderr } = await execPromise(`git clone ${authUrl} ${repoName}`, { cwd: reposRoot });
// Clone the repository securely using execFile to prevent command injection.
// We use '--' to prevent argument injection if the repo URL or name starts with '-'.
const { stdout, stderr } = await execFilePromise('git', ['clone', '--', authUrl, repoName], { cwd: reposRoot });
Comment on lines +35 to +37
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

security-high high

While using execFile with -- prevents command and argument injection, repoName is still vulnerable to path traversal. If repoName contains path traversal sequences (e.g., ../../), it can escape the intended reposRoot directory during cloning. Furthermore, on line 25, path.join(reposRoot, repoName) is used with fs.existsSync, which allows an attacker to probe the existence of arbitrary files on the server (File Existence Disclosure).

To prevent both issues, we should validate repoName to ensure it is a safe directory name (matching the isSafeRepoDirName helper regex). Additionally, we should validate that repoUrl starts with https:// to prevent cloning local paths or using unexpected protocols.

Note: Ideally, the repoName validation should be moved to the top of the handler (right after parsing the request body) to fully protect the fs.existsSync check on line 25.

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

    if (!repoUrl.startsWith('https://')) {
      return new Response(JSON.stringify({ error: 'URL de dépôt invalide (HTTPS uniquement)' }), { status: 400 });
    }

    // Clone the repository securely using execFile to prevent command injection.
    // We use '--' to prevent argument injection if the repo URL or name starts with '-'.
    const { stdout, stderr } = await execFilePromise('git', ['clone', '--', authUrl, repoName], { cwd: reposRoot });


// Try to auto-sync it into the database
try {
Expand All @@ -53,7 +54,11 @@ export const POST: APIRoute = async ({ request }) => {
});

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

return new Response(JSON.stringify({ error: safeError }), {
status: 500,
headers: { 'Content-Type': 'application/json' },
});
Comment on lines +57 to 64
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

medium

If error is not an object (e.g., if a promise rejects with null, undefined, or a string, or if a non-Error exception is thrown), accessing error.message directly will throw a TypeError, causing a secondary crash in the catch block. Using optional chaining (error?.message) and a fallback to String(error) prevents this.

Additionally, logging the sanitized error message to console.error is highly recommended for server-side observability and troubleshooting, while ensuring sensitive tokens remain redacted.

    // 🛡️ Sentinel: Prevent GitHub token from leaking in error messages
    const rawError = error?.message || (typeof error === 'string' ? error : 'Erreur lors du clonage');
    const safeError = rawError.replace(/https:\/\/[^@]+@/g, 'https://***@');

    // Log the sanitized error for server-side observability
    console.error('[github-clone] Error:', safeError);

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

Expand Down