Potential fix for code scanning alert no. 28: Unsafe shell command constructed from library input#75
Potential fix for code scanning alert no. 28: Unsafe shell command constructed from library input#75
Conversation
…nstructed from library input Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try { | ||
| // Clone with no checkout | ||
| await execAsync(`git clone --no-checkout --progress ${repoURL} ${targetDir}`, { | ||
| await execFileAsync("git", ["clone", "--no-checkout", "--progress", repoURL, targetDir], { |
Check failure
Code scanning / CodeQL
Second order command injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 29 days ago
In general, to fix second‑order git command injection you must ensure any user‑controlled values that become git repository URLs or clone destinations cannot be interpreted as git options. At a minimum, reject inputs that start with -, and ideally constrain them to known‑good schemes (e.g. https://, git@, ssh://, file://, or local paths). This validation must happen before passing values into execFile("git", [...]) (or equivalent), and must be applied consistently to all code paths that can receive tainted inputs.
For this codebase, the best targeted fix is:
-
Strengthen
validateRepoURLinpackages/sdk/src/client/common/templates/git.tsso it:- Rejects strings starting with
-. - Optionally enforces that if the value looks like a URL, it uses an allowed scheme.
- Leaves existing behavior (accepting normal git/ssh/https URLs and local paths) intact.
- Rejects strings starting with
-
Ensure this validation also covers other git operations performed in this file (e.g. subdirectory/template fetch helpers, which are in the omitted
[...]), by reusingvalidateRepoURL/validateTargetDiranywhere a potentially tainted repo URL or directory is passed toexecFileAsync("git", ...). Since you’ve provided only thefetchTemplatesnippet, we will confine edits to strengthening the validators there, which addresses the flagged sink at line 76 and any other code in this file that already calls these validators.
Concretely, in packages/sdk/src/client/common/templates/git.ts we will modify validateRepoURL to add:
- A simple URL parse using the standard
URLconstructor when the string contains://, and a check to allow onlyhttp:,https:,ssh:, orgit:schemes. - A comment describing the security rationale, to clarify intent for future maintainers.
No other files (create.ts, index.ts) require code changes to address this specific injection risk; they will continue to pass cfg.repoURL down as before, but that value will now be validated in fetchTemplate.
| @@ -23,17 +23,32 @@ | ||
| * interpreted as an option such as `--upload-pack`. | ||
| * | ||
| * This allows typical git/ssh/https URLs and local paths, but rejects | ||
| * values starting with a dash. | ||
| * values starting with a dash and disallows unexpected URL schemes. | ||
| */ | ||
| function validateRepoURL(repoURL: string): void { | ||
| if (!repoURL || typeof repoURL !== "string") { | ||
| throw new Error("Invalid repository URL"); | ||
| } | ||
|
|
||
| // Disallow anything that looks like a git option | ||
| // Disallow anything that looks like a git option (e.g. "--upload-pack") | ||
| if (repoURL.startsWith("-")) { | ||
| throw new Error("Repository URL must not start with '-'"); | ||
| } | ||
|
|
||
| // If the value looks like a URL, restrict it to common git URL schemes. | ||
| // Local paths (no "://") are allowed as-is. | ||
| if (repoURL.includes("://")) { | ||
| try { | ||
| const parsed = new URL(repoURL); | ||
| const allowedProtocols = new Set(["http:", "https:", "ssh:", "git:"]); | ||
| if (!allowedProtocols.has(parsed.protocol)) { | ||
| throw new Error("Unsupported repository URL protocol"); | ||
| } | ||
| } catch (err) { | ||
| // Re-throw with a generic validation message to avoid leaking details | ||
| throw new Error("Invalid repository URL"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** |
| try { | ||
| // Clone with no checkout | ||
| await execAsync(`git clone --no-checkout --progress ${repoURL} ${targetDir}`, { | ||
| await execFileAsync("git", ["clone", "--no-checkout", "--progress", repoURL, targetDir], { |
Check failure
Code scanning / CodeQL
Second order command injection High
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 29 days ago
In general, to fix this kind of issue you must ensure that any value that can affect git’s interpretation of command-line arguments (repository URL, extra options) is validated or constrained before being passed to execFile. For git, this means ensuring repository URLs are well-formed (e.g., starting with git@, ssh://, https://, or being a safe local path), and that no argument intended to be a path can be interpreted as a git option like --upload-pack.
In this codebase the best targeted fix is:
- Add a small validation helper in
packages/sdk/src/client/common/templates/git.tsthat:- Ensures
repoURLis either a safe “URL-like” string (e.g., starts withgit@,ssh://,https://, orfile:) or a relative/local path without leading dashes. - Ensures
targetDiris a safe filesystem path (non-empty, not starting with-, no embedded NUL, etc.).
- Ensures
- Call this helper at the beginning of
fetchTemplateso that any attempt to pass in a maliciousrepoURLortargetDir(fromoptions.template, template catalog, or project name) will be rejected beforeexecFileAsyncruns.
This approach:
- Keeps functionality the same for legitimate inputs.
- Centralizes validation at the point of interaction with git, so all callers (current and future) are covered.
- Addresses both CodeQL alert variants (tainted
targetDirand underlying taintedrepoURL) with a single change. - Uses only standard Node APIs; no new external dependencies are needed.
No changes are required in create.ts or index.ts for the fix to be effective; those files will continue to pass through options as before, but now invalid values will be rejected by fetchTemplate.
| @@ -19,6 +19,39 @@ | ||
| } | ||
|
|
||
| /** | ||
| * Basic validation to ensure the git repository URL or path cannot be | ||
| * interpreted as an option such as `--upload-pack`. | ||
| * | ||
| * This allows typical git/ssh/https URLs and local paths, but rejects | ||
| * values starting with a dash. | ||
| */ | ||
| function validateRepoURL(repoURL: string): void { | ||
| if (!repoURL || typeof repoURL !== "string") { | ||
| throw new Error("Invalid repository URL"); | ||
| } | ||
|
|
||
| // Disallow anything that looks like a git option | ||
| if (repoURL.startsWith("-")) { | ||
| throw new Error("Repository URL must not start with '-'"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Validate the target directory used for cloning to ensure it cannot be | ||
| * interpreted by git as an option. | ||
| */ | ||
| function validateTargetDir(targetDir: string): void { | ||
| if (!targetDir || typeof targetDir !== "string") { | ||
| throw new Error("Invalid target directory"); | ||
| } | ||
|
|
||
| // Disallow leading dashes so it cannot be parsed as an option | ||
| if (targetDir.startsWith("-")) { | ||
| throw new Error("Target directory must not start with '-'"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Fetch full template repository | ||
| */ | ||
| export async function fetchTemplate( | ||
| @@ -32,6 +65,10 @@ | ||
| throw new Error("repoURL is required"); | ||
| } | ||
|
|
||
| // Validate untrusted inputs before passing to git | ||
| validateRepoURL(repoURL); | ||
| validateTargetDir(targetDir); | ||
|
|
||
| logger.info(`\nCloning repo: ${repoURL} → ${targetDir}\n`); | ||
|
|
||
| try { |
anupsv
committed this autofix suggestion 29 days ago.
…jection Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
1 participant
Potential fix for https://github.com/Layr-Labs/ecloud/security/code-scanning/28
In general, the fix is to stop passing a shell-constructed string to
child_process.execand instead invokegitviaexecFile(orspawn) with an argument array. This way, therepoURLandtargetDirvalues are passed as separate arguments, and the shell never interprets them. We should mirror the existing safe usage already present in this file forcheckoutandsubmodule update, which useexecFileAsync("git", [...]).Concretely, in
packages/sdk/src/client/common/templates/git.ts, we will change the clone step infetchTemplate(currently usingexecAsyncwith a single string) to useexecFileAsync("git", ["clone", "--no-checkout", "--progress", repoURL, targetDir], ...). SinceexecFileAsyncis already defined and imported, no new imports are required. This preserves existing behavior (same git command and options, samemaxBuffer) while eliminating the unsafe shell string interpolation. No changes are needed increate.tsorindex.ts; they will continue to callfetchTemplatewithrepoURL, but that value will now only be used as a process argument, not as part of a shell command.Suggested fixes powered by Copilot Autofix. Review carefully before merging.