diff --git a/.changeset/patch-add-git-helpers-safe-outputs.md b/.changeset/patch-add-git-helpers-safe-outputs.md new file mode 100644 index 0000000000..0ba972c610 --- /dev/null +++ b/.changeset/patch-add-git-helpers-safe-outputs.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Ensure `git_helpers.cjs` is included in `SAFE_OUTPUTS_FILES` so `generate_git_patch.cjs` and its dependencies load within the safe outputs MCP server. diff --git a/actions/setup/js/generate_git_patch.cjs b/actions/setup/js/generate_git_patch.cjs index 552e59311f..d7ec46a190 100644 --- a/actions/setup/js/generate_git_patch.cjs +++ b/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"); diff --git a/actions/setup/js/generate_git_patch.test.cjs b/actions/setup/js/generate_git_patch.test.cjs index 403838e4a8..9595bf92a4 100644 --- a/actions/setup/js/generate_git_patch.test.cjs +++ b/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); + }); }); diff --git a/actions/setup/js/git_helpers.cjs b/actions/setup/js/git_helpers.cjs new file mode 100644 index 0000000000..cff8eea239 --- /dev/null +++ b/actions/setup/js/git_helpers.cjs @@ -0,0 +1,66 @@ +// @ts-check +/// + +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, +}; diff --git a/actions/setup/js/git_helpers.test.cjs b/actions/setup/js/git_helpers.test.cjs new file mode 100644 index 0000000000..e5d2bd3d1a --- /dev/null +++ b/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; + } + }); + }); +}); diff --git a/actions/setup/js/push_repo_memory.cjs b/actions/setup/js/push_repo_memory.cjs index 055669ac25..1699f126c8 100644 --- a/actions/setup/js/push_repo_memory.cjs +++ b/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,7 +288,7 @@ 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; @@ -294,7 +296,7 @@ async function main() { // 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)}`); diff --git a/actions/setup/js/push_repo_memory.test.cjs b/actions/setup/js/push_repo_memory.test.cjs index e2a6d74b4c..5fc0f62be9 100644 --- a/actions/setup/js/push_repo_memory.test.cjs +++ b/actions/setup/js/push_repo_memory.test.cjs @@ -995,3 +995,124 @@ describe("push_repo_memory.cjs - glob pattern security tests", () => { }); }); }); + +describe("push_repo_memory.cjs - shell injection security tests", () => { + describe("safe git command execution", () => { + it("should use execGitSync helper from git_helpers.cjs", () => { + // This test verifies the security fix for shell injection vulnerability + // The file should use execGitSync helper which uses spawnSync with command-args array + + const fs = require("fs"); + const path = require("path"); + + const scriptPath = path.join(import.meta.dirname, "push_repo_memory.cjs"); + const scriptContent = fs.readFileSync(scriptPath, "utf8"); + + // Should import execGitSync from git_helpers, not use execSync or spawnSync directly + expect(scriptContent).toContain('const { execGitSync } = require("./git_helpers.cjs")'); + expect(scriptContent).not.toContain('const { execSync } = require("child_process")'); + expect(scriptContent).not.toContain('const { spawnSync } = require("child_process")'); + + // Should NOT have local execGitSync function (moved to git_helpers.cjs) + expect(scriptContent).not.toContain("function execGitSync(args, options = {})"); + + // Should use execGitSync function calls + expect(scriptContent).toContain("execGitSync(["); + }); + + it("should safely handle malicious branch names", () => { + // Test that malicious branch names would be rejected by git, not executed as shell commands + const maliciousBranchNames = [ + "feature; rm -rf /", // Command chaining + "feature && echo hacked", // Conditional execution + "feature | cat /etc/passwd", // Pipe redirection + "feature$(whoami)", // Command substitution + "feature`whoami`", // Backtick substitution + "feature\nrm -rf /", // Newline injection + 'feature"; curl evil.com', // Quote breaking + ]; + + // With spawnSync and args array, these would be treated as literal branch names + // Git would reject them as invalid branch names rather than executing them + for (const branchName of maliciousBranchNames) { + // Simulate the safe approach using spawnSync + const { spawnSync } = require("child_process"); + const result = spawnSync("git", ["checkout", branchName], { + encoding: "utf8", + stdio: "pipe", + }); + + // Command should fail (non-zero exit code) because branch doesn't exist + // Important: It should NOT execute the injected command + expect(result.status).not.toBe(0); + expect(result.error || result.stderr).toBeTruthy(); + } + }); + + it("should safely handle malicious repo URLs", () => { + // Test that malicious repo URLs would be treated as literals + const maliciousUrls = ["https://github.com/user/repo.git; rm -rf /", "https://github.com/user/repo.git && echo hacked", "https://github.com/user/repo.git | curl evil.com"]; + + // With spawnSync and args array, special characters are treated as part of the URL + // Git would fail to fetch from the malformed URL + for (const repoUrl of maliciousUrls) { + const { spawnSync } = require("child_process"); + const result = spawnSync("git", ["fetch", repoUrl, "main:main"], { + encoding: "utf8", + stdio: "pipe", + timeout: 1000, // Quick timeout since we expect failure + }); + + // Command should fail because URL is invalid + // Important: The injected command should NOT execute + expect(result.status).not.toBe(0); + } + }); + + it("should safely handle malicious commit messages", () => { + // Test that malicious commit messages would be treated as literals + const maliciousMessages = ["Update; rm -rf /", "Update && curl evil.com", "Update\nmalicious command", 'Update"; echo hacked']; + + // With spawnSync and args array, these would be literal commit messages + // No shell interpretation occurs + for (const message of maliciousMessages) { + const { spawnSync } = require("child_process"); + // Note: This would fail in actual use because there are no staged changes + // But it demonstrates that special characters are treated literally + const result = spawnSync("git", ["commit", "-m", message], { + encoding: "utf8", + stdio: "pipe", + }); + + // The command would fail (no staged changes), but importantly: + // The malicious part of the message should NOT be executed + // Special characters like ; && | should be part of the commit message, not shell operators + expect(result.status).not.toBe(0); + } + }); + + it("should verify no vulnerable execSync patterns remain", () => { + // Ensure no vulnerable patterns like execSync(`git ${variable}`) exist + const fs = require("fs"); + const path = require("path"); + + const scriptPath = path.join(import.meta.dirname, "push_repo_memory.cjs"); + const scriptContent = fs.readFileSync(scriptPath, "utf8"); + + // Should not have vulnerable patterns with template literals + // These patterns would indicate shell injection vulnerabilities: + const vulnerablePatterns = [ + /execSync\s*\(\s*`git.*\${/, // execSync(`git ... ${variable}`) + /execSync\s*\(\s*"git.*\${/, // execSync("git ... ${variable}") + /execSync\s*\(\s*'git.*\${/, // execSync('git ... ${variable}') + ]; + + for (const pattern of vulnerablePatterns) { + expect(scriptContent).not.toMatch(pattern); + } + + // Should use safe execGitSync with args array + expect(scriptContent).toContain("execGitSync(["); + }); + }); +}); diff --git a/actions/setup/setup.sh b/actions/setup/setup.sh index 78fe13e0a1..7d2fc12a2c 100755 --- a/actions/setup/setup.sh +++ b/actions/setup/setup.sh @@ -57,11 +57,16 @@ ls -1 "${JS_SOURCE_DIR}" | head -10 || echo "::warning::Failed to list files in FILE_COUNT_IN_DIR=$(ls -1 "${JS_SOURCE_DIR}" 2>/dev/null | wc -l) echo "Found ${FILE_COUNT_IN_DIR} files in ${JS_SOURCE_DIR}" -# Copy all .cjs files from js/ to destination +# Copy all .cjs files from js/ to destination (excluding test files) FILE_COUNT=0 for file in "${JS_SOURCE_DIR}"/*.cjs; do if [ -f "$file" ]; then filename=$(basename "$file") + # Skip test files + if [[ "$filename" == *.test.cjs ]]; then + echo "Skipping test file: ${filename}" + continue + fi cp "$file" "${DESTINATION}/${filename}" echo "Copied: ${filename}" FILE_COUNT=$((FILE_COUNT + 1)) @@ -207,6 +212,7 @@ SAFE_OUTPUTS_FILES=( "generate_compact_schema.cjs" "setup_globals.cjs" "error_helpers.cjs" + "git_helpers.cjs" "mcp_enhanced_errors.cjs" )