🛡️ Sentinel: [CRITICAL] Fix command injection in github-clone api#26
🛡️ Sentinel: [CRITICAL] Fix command injection in github-clone api#26bobdivx wants to merge 1 commit into
Conversation
- Replaced `exec` with `execFile` to eliminate shell injection vulnerabilities. - Added input validation to explicitly block inputs starting with `-` to prevent argument/flag injection vulnerabilities to the `git` executable. - Added regex sanitization to the `catch` error block to prevent leaking the `githubToken` via the command execution `error.message`. - Updated Sentinel journal documenting the secret leakage learning via command error objects. Co-authored-by: bobdivx <6737167+bobdivx@users.noreply.github.com>
|
👋 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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the security of the GitHub cloning process by transitioning from exec to execFile, implementing checks against argument injection, and sanitizing error messages to prevent the leakage of sensitive tokens. Additionally, documentation was added to the sentinel log regarding these vulnerabilities. Feedback was provided regarding a potential path traversal vulnerability in the repoName parameter, suggesting the use of a strict regular expression to validate directory names and prevent unauthorized file system access.
| if (String(repoUrl).trim().startsWith('-') || String(repoName).trim().startsWith('-')) { | ||
| return new Response(JSON.stringify({ error: 'Paramètres invalides (injection suspectée)' }), { status: 400 }); | ||
| } |
There was a problem hiding this comment.
While checking for leading hyphens prevents argument injection, repoName is also used to construct a file path at line 29 and passed as a directory name to git clone at line 40. An attacker could provide a path like ../../some/path to perform a path traversal attack, potentially writing files outside the intended reposRoot. It is recommended to strictly validate repoName using a regular expression that only allows safe characters (alphanumeric, dots, underscores, and hyphens) and limits its length, similar to the isSafeRepoDirName utility defined in src/lib/forge-repos.ts.
| if (String(repoUrl).trim().startsWith('-') || String(repoName).trim().startsWith('-')) { | |
| return new Response(JSON.stringify({ error: 'Paramètres invalides (injection suspectée)' }), { status: 400 }); | |
| } | |
| const safeNameRegex = /^[a-zA-Z0-9._-]{1,128}$/; | |
| if (!safeNameRegex.test(repoName) || String(repoUrl).trim().startsWith('-')) { | |
| return new Response(JSON.stringify({ error: 'Paramètres invalides (nom de dépôt non sûr ou injection suspectée)' }), { status: 400 }); | |
| } |
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The
/api/github-cloneendpoint usedexec()with interpolatedrepoUrlandrepoNameparameters without sufficient sanitization. Additionally, errors thrown by command execution failures automatically concatenate arguments (such as thegithubToken) to theerror.message, which was then being returned verbatim to clients.🎯 Impact: An attacker could execute arbitrary OS commands on the server by supplying malicious
repoUrlorrepoNamepayload inputs. Alternatively, an attacker could attempt failing clones and read the server response to extract the instance's private GitHub Token.🔧 Fix: Refactored the command execution to use
execFile, verified that parameters do not start with hyphens to stop argument injection, and strictly sanitized the error text to redact tokens matchingoauth2:***@.✅ Verification: Ran test suite which passed, ran codebase checks which passed.
PR created automatically by Jules for task 3081547663990760482 started by @bobdivx