🛡️ Sentinel: [CRITICAL] Fix command injection and secret leak in github clone API#71
🛡️ Sentinel: [CRITICAL] Fix command injection and secret leak in github clone API#71bobdivx wants to merge 1 commit into
Conversation
…ub clone API Co-authored-by: bobdivx <6737167+bobdivx@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request improves security in the github-clone.ts API endpoint by replacing exec with execFile to prevent command injection, and sanitizing error messages to avoid leaking GitHub OAuth tokens. The review feedback highlights that repoName remains vulnerable to path traversal and suggests validating both repoName and repoUrl. Additionally, it is recommended to use optional chaining on the error object in the catch block to prevent secondary crashes, and to log the sanitized error for server-side observability.
| // 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 }); |
There was a problem hiding this comment.
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 });| // 🛡️ 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' }, | ||
| }); |
There was a problem hiding this comment.
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' },
});
🚨 Severity: CRITICAL
💡 Vulnerability: The API endpoint
github-clone.tsusedexecfor cloning repositories, which parses inputs via the shell, exposing the application to Command Injection vulnerabilities. Furthermore, failure cases directly exposed theerror.messageto 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
repoUrlorrepoNamepayload. Additionally, token leakage compromises the Github account's security.🔧 Fix: Replaced
execwithexecFile, 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.mdregarding proper process output sanitization.✅ Verification: Ran
pnpm run checkandpnpm test(vitest) to confirm functional stability. The patch uses exact replacement methodologies to ensure identical behavior with strict shell isolation.PR created automatically by Jules for task 4931847268730697963 started by @bobdivx