Fix shell injection in generate_git_patch.cjs and push_repo_memory.cjs via shared git_helpers.cjs

actions/setup/js/generate_git_patch.cjs

@@ -3,10 +3,10 @@
 
 const fs = require("fs");
 const path = require("path");
-const { execSync } = require("child_process");
 
 const { getBaseBranch } = require("./get_base_branch.cjs");
 const { getErrorMessage } = require("./error_helpers.cjs");
+const { execGitSync } = require("./git_helpers.cjs");
 
 /**
  * Generates a git patch file for the current changes
@@ -33,29 +33,26 @@ function generateGitPatch(branchName) {
     if (branchName) {
       // Check if the branch exists locally
       try {
-        execSync(`git show-ref --verify --quiet refs/heads/${branchName}`, { cwd, encoding: "utf8" });
+        execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd });
 
         // Determine base ref for patch generation
         let baseRef;
         try {
           // Check if origin/branchName exists
-          execSync(`git show-ref --verify --quiet refs/remotes/origin/${branchName}`, { cwd, encoding: "utf8" });
+          execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${branchName}`], { cwd });
           baseRef = `origin/${branchName}`;
         } catch {
           // Use merge-base with default branch
-          execSync(`git fetch origin ${defaultBranch}`, { cwd, encoding: "utf8" });
-          baseRef = execSync(`git merge-base origin/${defaultBranch} ${branchName}`, { cwd, encoding: "utf8" }).trim();
+          execGitSync(["fetch", "origin", defaultBranch], { cwd });
+          baseRef = execGitSync(["merge-base", `origin/${defaultBranch}`, branchName], { cwd }).trim();
         }
 
         // Count commits to be included
-        const commitCount = parseInt(execSync(`git rev-list --count ${baseRef}..${branchName}`, { cwd, encoding: "utf8" }).trim(), 10);
+        const commitCount = parseInt(execGitSync(["rev-list", "--count", `${baseRef}..${branchName}`], { cwd }).trim(), 10);
 
         if (commitCount > 0) {
           // Generate patch from the determined base to the branch
-          const patchContent = execSync(`git format-patch ${baseRef}..${branchName} --stdout`, {
-            cwd,
-            encoding: "utf8",
-          });
+          const patchContent = execGitSync(["format-patch", `${baseRef}..${branchName}`, "--stdout"], { cwd });
 
           if (patchContent && patchContent.trim()) {
             fs.writeFileSync(patchPath, patchContent, "utf8");
@@ -69,7 +66,7 @@ function generateGitPatch(branchName) {
 
     // Strategy 2: Check if commits were made to current HEAD since checkout
     if (!patchGenerated) {
-      const currentHead = execSync("git rev-parse HEAD", { cwd, encoding: "utf8" }).trim();
+      const currentHead = execGitSync(["rev-parse", "HEAD"], { cwd }).trim();
 
       if (!githubSha) {
         errorMessage = "GITHUB_SHA environment variable is not set";
@@ -78,17 +75,14 @@ function generateGitPatch(branchName) {
       } else {
         // Check if GITHUB_SHA is an ancestor of current HEAD
         try {
-          execSync(`git merge-base --is-ancestor ${githubSha} HEAD`, { cwd, encoding: "utf8" });
+          execGitSync(["merge-base", "--is-ancestor", githubSha, "HEAD"], { cwd });
 
           // Count commits between GITHUB_SHA and HEAD
-          const commitCount = parseInt(execSync(`git rev-list --count ${githubSha}..HEAD`, { cwd, encoding: "utf8" }).trim(), 10);
+          const commitCount = parseInt(execGitSync(["rev-list", "--count", `${githubSha}..HEAD`], { cwd }).trim(), 10);
 
           if (commitCount > 0) {
             // Generate patch from GITHUB_SHA to HEAD
-            const patchContent = execSync(`git format-patch ${githubSha}..HEAD --stdout`, {
-              cwd,
-              encoding: "utf8",
-            });
+            const patchContent = execGitSync(["format-patch", `${githubSha}..HEAD`, "--stdout"], { cwd });
 
             if (patchContent && patchContent.trim()) {
               fs.writeFileSync(patchPath, patchContent, "utf8");

actions/setup/js/generate_git_patch.test.cjs

@@ -126,4 +126,38 @@ describe("generateGitPatch", () => {
     expect(result).toHaveProperty("success");
     // Should attempt to use master as base branch
   });
+
+  it("should safely handle branch names with special characters", async () => {
+    const { generateGitPatch } = await import("./generate_git_patch.cjs");
+
+    process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo";
+    process.env.GITHUB_SHA = "abc123";
+
+    // Test with various special characters that could cause shell injection
+    const maliciousBranchNames = ["feature; rm -rf /", "feature && echo hacked", "feature | cat /etc/passwd", "feature$(whoami)", "feature`whoami`", "feature\nrm -rf /"];
+
+    for (const branchName of maliciousBranchNames) {
+      const result = generateGitPatch(branchName);
+
+      // Should not throw an error and should handle safely
+      expect(result).toHaveProperty("success");
+      expect(result.success).toBe(false);
+      // Should fail gracefully without executing injected commands
+    }
+  });
+
+  it("should safely handle GITHUB_SHA with special characters", async () => {
+    const { generateGitPatch } = await import("./generate_git_patch.cjs");
+
+    process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo";
+
+    // Test with malicious SHA that could cause shell injection
+    process.env.GITHUB_SHA = "abc123; echo hacked";
+
+    const result = generateGitPatch("test-branch");
+
+    // Should not throw an error and should handle safely
+    expect(result).toHaveProperty("success");
+    expect(result.success).toBe(false);
+  });
 });

actions/setup/js/git_helpers.cjs

@@ -0,0 +1,66 @@
+// @ts-check
+/// <reference types="@actions/github-script" />
+
+const { spawnSync } = require("child_process");
+
+/**
+ * Safely execute git command using spawnSync with args array to prevent shell injection
+ * @param {string[]} args - Git command arguments
+ * @param {Object} options - Spawn options
+ * @returns {string} Command output
+ * @throws {Error} If command fails
+ */
+function execGitSync(args, options = {}) {
+  // Log the git command being executed for debugging (but redact credentials)
+  const gitCommand = `git ${args
+    .map(arg => {
+      // Redact credentials in URLs
+      if (typeof arg === "string" && arg.includes("://") && arg.includes("@")) {
+        return arg.replace(/(https?:\/\/)[^@]+@/, "$1***@");
+      }
+      return arg;
+    })
+    .join(" ")}`;
+
+  if (typeof core !== "undefined" && core.debug) {
+    core.debug(`Executing git command: ${gitCommand}`);
+  }
+
+  const result = spawnSync("git", args, {
+    encoding: "utf8",
+    ...options,
+  });
+
+  if (result.error) {
+    if (typeof core !== "undefined" && core.error) {
+      core.error(`Git command failed with error: ${result.error.message}`);
+    }
+    throw result.error;
+  }
+
+  if (result.status !== 0) {
+    const errorMsg = result.stderr || `Git command failed with status ${result.status}`;
+    if (typeof core !== "undefined" && core.error) {
+      core.error(`Git command failed: ${gitCommand}`);
+      core.error(`Exit status: ${result.status}`);
+      if (result.stderr) {
+        core.error(`Stderr: ${result.stderr}`);
+      }
+    }
+    throw new Error(errorMsg);
+  }
+
+  if (typeof core !== "undefined" && core.debug) {
+    if (result.stdout) {
+      core.debug(`Git command output: ${result.stdout.substring(0, 200)}${result.stdout.length > 200 ? "..." : ""}`);
+    } else {
+      core.debug("Git command completed successfully with no output");
+    }
+  }
+
+  return result.stdout;
+}
+
+module.exports = {
+  execGitSync,
+};

actions/setup/js/git_helpers.test.cjs

@@ -0,0 +1,100 @@
+import { describe, it, expect } from "vitest";
+
+describe("git_helpers.cjs", () => {
+  describe("execGitSync", () => {
+    it("should export execGitSync function", async () => {
+      const { execGitSync } = await import("./git_helpers.cjs");
+      expect(typeof execGitSync).toBe("function");
+    });
+
+    it("should execute git commands safely", async () => {
+      const { execGitSync } = await import("./git_helpers.cjs");
+
+      // Test with a simple git command that should work
+      const result = execGitSync(["--version"]);
+      expect(result).toContain("git version");
+    });
+
+    it("should handle git command failures", async () => {
+      const { execGitSync } = await import("./git_helpers.cjs");
+
+      // Test with an invalid git command
+      expect(() => {
+        execGitSync(["invalid-command"]);
+      }).toThrow();
+    });
+
+    it("should prevent shell injection in branch names", async () => {
+      const { execGitSync } = await import("./git_helpers.cjs");
+
+      // Test with malicious branch name
+      const maliciousBranch = "feature; rm -rf /";
+
+      // This should fail because the branch doesn't exist,
+      // but importantly, it should NOT execute "rm -rf /"
+      expect(() => {
+        execGitSync(["rev-parse", maliciousBranch]);
+      }).toThrow();
+    });
+
+    it("should treat special characters as literals", async () => {
+      const { execGitSync } = await import("./git_helpers.cjs");
+
+      const specialBranches = ["feature && echo hacked", "feature | cat /etc/passwd", "feature$(whoami)", "feature`whoami`"];
+
+      for (const branch of specialBranches) {
+        // All should fail with git error, not execute shell commands
+        expect(() => {
+          execGitSync(["rev-parse", branch]);
+        }).toThrow();
+      }
+    });
+
+    it("should pass options to spawnSync", async () => {
+      const { execGitSync } = await import("./git_helpers.cjs");
+
+      // Test that options are properly passed through
+      const result = execGitSync(["--version"], { encoding: "utf8" });
+      expect(typeof result).toBe("string");
+      expect(result).toContain("git version");
+    });
+
+    it("should return stdout from successful commands", async () => {
+      const { execGitSync } = await import("./git_helpers.cjs");
+
+      // Use git --version which always succeeds
+      const result = execGitSync(["--version"]);
+      expect(typeof result).toBe("string");
+      expect(result).toContain("git version");
+    });
+
+    it("should redact credentials from logged commands", async () => {
+      const { execGitSync } = await import("./git_helpers.cjs");
+
+      // Mock core.debug to capture logged output
+      const debugLogs = [];
+      const originalCore = global.core;
+      global.core = {
+        debug: msg => debugLogs.push(msg),
+        error: () => {},
+      };
+
+      try {
+        // Try a git command with a URL containing credentials (will fail but that's ok)
+        try {
+          execGitSync(["fetch", "https://user:token@github.com/repo.git", "main"]);
+        } catch (e) {
+          // Expected to fail, we're just checking the logging
+        }
+
+        // Check that credentials were redacted in the log
+        const fetchLog = debugLogs.find(log => log.includes("git fetch"));
+        expect(fetchLog).toBeDefined();
+        expect(fetchLog).toContain("https://***@github.com/repo.git");
+        expect(fetchLog).not.toContain("user:token");
+      } finally {
+        global.core = originalCore;
+      }
+    });
+  });
+});

actions/setup/js/push_repo_memory.cjs

@@ -3,9 +3,10 @@
 
 const fs = require("fs");
 const path = require("path");
-const { execSync } = require("child_process");
+
 const { getErrorMessage } = require("./error_helpers.cjs");
 const { globPatternToRegex } = require("./glob_pattern_helpers.cjs");
+const { execGitSync } = require("./git_helpers.cjs");
 
 /**
  * Push repo-memory changes to git branch
@@ -95,7 +96,7 @@ async function main() {
   // This is necessary because checkout was configured with sparse-checkout
   core.info(`Disabling sparse checkout...`);
   try {
-    execSync("git sparse-checkout disable", { stdio: "pipe" });
+    execGitSync(["sparse-checkout", "disable"], { stdio: "pipe" });
   } catch (error) {
     // Ignore if sparse checkout wasn't enabled
     core.info("Sparse checkout was not enabled or already disabled");
@@ -108,14 +109,15 @@ async function main() {
 
     // Try to fetch the branch
     try {
-      execSync(`git fetch "${repoUrl}" "${branchName}:${branchName}"`, { stdio: "pipe" });
-      execSync(`git checkout "${branchName}"`, { stdio: "inherit" });
+      execGitSync(["fetch", repoUrl, `${branchName}:${branchName}`], { stdio: "pipe" });
+      execGitSync(["checkout", branchName], { stdio: "inherit" });
       core.info(`Checked out existing branch: ${branchName}`);
     } catch (fetchError) {
       // Branch doesn't exist, create orphan branch
       core.info(`Branch ${branchName} does not exist, creating orphan branch...`);
-      execSync(`git checkout --orphan "${branchName}"`, { stdio: "inherit" });
-      execSync("git rm -rf . || true", { stdio: "pipe" });
+      execGitSync(["checkout", "--orphan", branchName], { stdio: "inherit" });
+      // Use --ignore-unmatch to avoid failure when directory is empty
+      execGitSync(["rm", "-r", "-f", "--ignore-unmatch", "."], { stdio: "pipe" });
       core.info(`Created orphan branch: ${branchName}`);
     }
   } catch (error) {
@@ -270,7 +272,7 @@ async function main() {
   // Check if we have any changes to commit
   let hasChanges = false;
   try {
-    const status = execSync("git status --porcelain", { encoding: "utf8" });
+    const status = execGitSync(["status", "--porcelain"]);
     hasChanges = status.trim().length > 0;
   } catch (error) {
     core.setFailed(`Failed to check git status: ${getErrorMessage(error)}`);
@@ -286,15 +288,15 @@ async function main() {
 
   // Stage all changes
   try {
-    execSync("git add .", { stdio: "inherit" });
+    execGitSync(["add", "."], { stdio: "inherit" });
   } catch (error) {
     core.setFailed(`Failed to stage changes: ${getErrorMessage(error)}`);
     return;
   }
 
   // Commit changes
   try {
-    execSync(`git commit -m "Update repo memory from workflow run ${githubRunId}"`, { stdio: "inherit" });
+    execGitSync(["commit", "-m", `Update repo memory from workflow run ${githubRunId}`], { stdio: "inherit" });
   } catch (error) {
     core.setFailed(`Failed to commit changes: ${getErrorMessage(error)}`);
     return;
@@ -304,7 +306,7 @@ async function main() {
   core.info(`Pulling latest changes from ${branchName}...`);
   try {
     const repoUrl = `https://x-access-token:${ghToken}@github.com/${targetRepo}.git`;
-    execSync(`git pull --no-rebase -X ours "${repoUrl}" "${branchName}"`, { stdio: "inherit" });
+    execGitSync(["pull", "--no-rebase", "-X", "ours", repoUrl, branchName], { stdio: "inherit" });
   } catch (error) {
     // Pull might fail if branch doesn't exist yet or on conflicts - this is acceptable
     core.warning(`Pull failed (this may be expected): ${getErrorMessage(error)}`);
@@ -314,7 +316,7 @@ async function main() {
   core.info(`Pushing changes to ${branchName}...`);
   try {
     const repoUrl = `https://x-access-token:${ghToken}@github.com/${targetRepo}.git`;
-    execSync(`git push "${repoUrl}" HEAD:"${branchName}"`, { stdio: "inherit" });
+    execGitSync(["push", repoUrl, `HEAD:${branchName}`], { stdio: "inherit" });
     core.info(`Successfully pushed changes to ${branchName} branch`);
   } catch (error) {
     core.setFailed(`Failed to push changes: ${getErrorMessage(error)}`);