diff --git a/.changeset/patch-fix-patch-parser-file-protection-bypass.md b/.changeset/patch-fix-patch-parser-file-protection-bypass.md new file mode 100644 index 00000000000..379704b55e9 --- /dev/null +++ b/.changeset/patch-fix-patch-parser-file-protection-bypass.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fixed a security issue where crafted patches could bypass file protection checks by exploiting parser differences between the patch parser and `git am`. diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 623b3db448e..84c905c3877 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -27,6 +27,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/github-agentic-workflows.md` - `.github/aw/github-mcp-server.md` - `.github/aw/llms.md` +- `.github/aw/mcp-clis.md` - `.github/aw/memory.md` - `.github/aw/messages.md` - `.github/aw/network.md` diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 8f7b3991416..6b1ab0ad165 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -28,7 +28,7 @@ const { findRepoCheckout } = require("./find_repo_checkout.cjs"); const { getBaseBranch } = require("./get_base_branch.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); -const { checkFileProtection } = require("./manifest_file_helpers.cjs"); +const { checkFileProtection, checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs"); const { renderTemplateFromFile, renderFilesList, buildProtectedFileList, getPromptPath } = require("./messages_core.cjs"); const { COPILOT_REVIEWER_BOT, FAQ_CREATE_PR_PERMISSIONS_URL } = require("./constants.cjs"); const { isStagedMode } = require("./safe_output_helpers.cjs"); @@ -1666,6 +1666,15 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo // Apply the patch using git CLI (skip if empty) if (!isEmpty) { + let postApplyBaseRef = null; + const capturePostApplyBaseRef = async () => { + const headResult = await exec.getExecOutput("git", ["rev-parse", "HEAD"]); + const resolvedRef = headResult.stdout.trim(); + if (resolvedRef) { + postApplyBaseRef = resolvedRef; + } + }; + // Resolve temporary ID references in patch content before applying // This handles references like #aw_XXX in committed source code if (resolvedTemporaryIds && Object.keys(resolvedTemporaryIds).length > 0) { @@ -1691,6 +1700,7 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo // This allows git to resolve create-vs-modify mismatches when a file exists in target but not source let patchApplied = false; try { + await capturePostApplyBaseRef(); await exec.exec("git", ["am", "--3way", patchFilePath]); core.info("Patch applied successfully"); patchApplied = true; @@ -1771,6 +1781,7 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo // Try --3way first to maximize repair opportunities even on fallback branches. // If that still fails with add/add conflicts, recover and continue git am. try { + await capturePostApplyBaseRef(); await exec.exec("git", ["am", "--3way", patchFilePath]); } catch (fallbackPatchError) { core.warning(`Fallback git am --3way failed: ${getErrorMessage(fallbackPatchError)}`); @@ -1802,58 +1813,92 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo } } - // Push the applied commits to the branch (with fallback to issue creation on failure) - // Note: when manifestProtectionFallback is set we still push the branch so the - // fallback issue can include a compare URL. Genuine push failures are handled in - // the catch block below. + // POST-APPLY FILE PROTECTION: Verify actual files written match policy. + // This is the primary defense against parser-differential attacks where the JS + // patch parser and git am disagree on which files a patch contains. + // (see github/agentic-workflows#539) { - try { - branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, { recreateRef, githubClient, owner: repoParts.owner, repo: repoParts.repo }); - - await pushSignedCommits({ - githubClient, - owner: repoParts.owner, - repo: repoParts.repo, - branch: branchName, - baseRef: `origin/${baseBranch}`, - cwd: process.cwd(), - signedCommits, - resolvedTemporaryIds, - currentRepo: itemRepo, + const diffBaseRef = postApplyBaseRef || `origin/${baseBranch}`; + const diffResult = await exec.getExecOutput("git", ["diff", "--name-only", "--no-renames", `${diffBaseRef}..HEAD`]); + const actualFiles = diffResult.stdout + .split("\n") + .map(f => f.trim()) + .filter(Boolean); + if (actualFiles.length > 0) { + core.info(`Post-apply verification: ${actualFiles.length} file(s) actually modified`); + const postApplyProtection = checkFileProtectionPostApply(actualFiles, { + ...config, + protected_files_policy: config.protected_files_policy ?? "request_review", }); - core.info("Changes pushed to branch"); - - // Count new commits on PR branch relative to base, used to restrict - // the extra empty CI-trigger commit to exactly 1 new commit. - try { - const { stdout: countStr } = await exec.getExecOutput("git", ["rev-list", "--count", `origin/${baseBranch}..HEAD`]); - newCommitCount = parseInt(countStr.trim(), 10); - core.info(`${newCommitCount} new commit(s) on branch relative to origin/${baseBranch}`); - } catch { - // Non-fatal - newCommitCount stays 0, extra empty commit will be skipped - core.info("Could not count new commits - extra empty commit will be skipped"); + if (postApplyProtection.action === "deny") { + const filesStr = postApplyProtection.files.join(", "); + const msg = `SECURITY: Post-apply file-protection check failed. ` + `The patch applied files that were not detected by the pre-apply parser: ${filesStr}. ` + `This may indicate a patch-parser bypass attempt. Aborting.`; + core.error(msg); + return { success: false, error: msg }; } - } catch (pushError) { - // Push failed - create fallback issue instead of PR (if fallback is enabled) - core.error(`Git push failed: ${pushError instanceof Error ? pushError.message : String(pushError)}`); + if (postApplyProtection.action === "fallback") { + manifestProtectionFallback = postApplyProtection.files; + core.warning(`Post-apply: Protected file protection triggered (fallback-to-issue): ${postApplyProtection.files.join(", ")}`); + } + if (postApplyProtection.action === "request_review") { + manifestProtectionRequestReview = postApplyProtection.files; + core.warning(`Post-apply: Protected file protection triggered (request_review): ${postApplyProtection.files.join(", ")}`); + } + } + } - if (manifestProtectionFallback) { - // Push failed specifically for a protected-file modification. Don't create - // a generic push-failed issue — fall through to the manifestProtectionFallback - // block below, which will create the proper protected-file review issue with - // patch artifact download instructions (since the branch was not pushed). - core.warning("Git push failed for protected-file modification - deferring to protected-file review issue"); - manifestProtectionPushFailedError = pushError; - } else if (!fallbackAsIssue) { - // Fallback is disabled - return error without creating issue - core.error("fallback-as-issue is disabled - not creating fallback issue"); - const error = `Failed to push changes: ${pushError instanceof Error ? pushError.message : String(pushError)}`; - return { - success: false, - error, - error_type: "push_failed", - }; - } else { + // Push the applied commits to the branch (with fallback to issue creation on failure) + // Note: when manifestProtectionFallback is set we still push the branch so the + // fallback issue can include a compare URL. Genuine push failures are handled in + // the catch block below. + { + try { + branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, { recreateRef, githubClient, owner: repoParts.owner, repo: repoParts.repo }); + + await pushSignedCommits({ + githubClient, + owner: repoParts.owner, + repo: repoParts.repo, + branch: branchName, + baseRef: `origin/${baseBranch}`, + cwd: process.cwd(), + signedCommits, + resolvedTemporaryIds, + currentRepo: itemRepo, + }); + core.info("Changes pushed to branch"); + + // Count new commits on PR branch relative to base, used to restrict + // the extra empty CI-trigger commit to exactly 1 new commit. + try { + const { stdout: countStr } = await exec.getExecOutput("git", ["rev-list", "--count", `origin/${baseBranch}..HEAD`]); + newCommitCount = parseInt(countStr.trim(), 10); + core.info(`${newCommitCount} new commit(s) on branch relative to origin/${baseBranch}`); + } catch { + // Non-fatal - newCommitCount stays 0, extra empty commit will be skipped + core.info("Could not count new commits - extra empty commit will be skipped"); + } + } catch (pushError) { + // Push failed - create fallback issue instead of PR (if fallback is enabled) + core.error(`Git push failed: ${pushError instanceof Error ? pushError.message : String(pushError)}`); + + if (manifestProtectionFallback) { + // Push failed specifically for a protected-file modification. Don't create + // a generic push-failed issue — fall through to the manifestProtectionFallback + // block below, which will create the proper protected-file review issue with + // patch artifact download instructions (since the branch was not pushed). + core.warning("Git push failed for protected-file modification - deferring to protected-file review issue"); + manifestProtectionPushFailedError = pushError; + } else if (!fallbackAsIssue) { + // Fallback is disabled - return error without creating issue + core.error("fallback-as-issue is disabled - not creating fallback issue"); + const error = `Failed to push changes: ${pushError instanceof Error ? pushError.message : String(pushError)}`; + return { + success: false, + error, + error_type: "push_failed", + }; + } else { core.warning("Git push operation failed - creating fallback issue instead of pull request"); const runUrl = buildWorkflowRunUrl(context, context.repo); diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index a483b0a80e1..fee9bdf0755 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -2510,6 +2510,65 @@ describe("create_pull_request - patch apply fallback to original base commit", ( expect(global.core.warning).toHaveBeenCalledWith(expect.stringContaining("merge conflicts")); }); + it("should diff against the fallback pre-apply HEAD for post-apply protection checks", async () => { + let primaryAmAttempted = false; + const promptsDir = path.join(tempDir, "prompts"); + fs.mkdirSync(promptsDir, { recursive: true }); + fs.writeFileSync(path.join(promptsDir, "manifest_protection_request_review.md"), "Protected files: {{files}}", "utf8"); + fs.writeFileSync(path.join(promptsDir, "manifest_protection_request_changes_review.md"), "Protected files: {{files}}", "utf8"); + process.env.GH_AW_PROMPTS_DIR = promptsDir; + global.exec = { + exec: vi.fn().mockImplementation((cmd, args) => { + if (isGitAm3Way(cmd, args) && !primaryAmAttempted) { + primaryAmAttempted = true; + throw new Error("CONFLICT (content): Merge conflict in file.txt"); + } + return Promise.resolve(0); + }), + getExecOutput: vi.fn().mockImplementation((cmd, args) => { + if (cmd === "git" && Array.isArray(args) && args[0] === "merge-base") { + return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }); + } + if (cmd === "git" && Array.isArray(args) && args[0] === "rev-parse" && args[1] === "HEAD") { + return Promise.resolve({ exitCode: 0, stdout: primaryAmAttempted ? `${MOCK_BASE_COMMIT_SHA}\n` : "origin-main-head\n", stderr: "" }); + } + if (cmd === "git" && Array.isArray(args) && args[0] === "diff" && args[1] === "--name-only" && args[2] === "--no-renames") { + expect(args[3]).toBe(`${MOCK_BASE_COMMIT_SHA}..HEAD`); + return Promise.resolve({ exitCode: 0, stdout: ".github/CODEOWNERS\n", stderr: "" }); + } + if (cmd === "git" && Array.isArray(args) && args[0] === "diff" && args.includes("--diff-filter=U")) { + return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }); + } + if (cmd === "git" && Array.isArray(args) && args[0] === "status") { + return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }); + } + if (cmd === "git" && Array.isArray(args) && args[0] === "am" && args[1] === "--show-current-patch=diff") { + return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }); + } + return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }); + }), + }; + + const { main } = require("./create_pull_request.cjs"); + const handler = await main({ + protected_path_prefixes: [".github/"], + }); + const result = await handler( + { + title: "Test PR", + body: "Test body", + patch_path: patchFilePath, + branch: "test-branch", + base_commit: MOCK_BASE_COMMIT_SHA, + }, + {} + ); + + expect(result.success).toBe(true); + expect(global.github.rest.pulls.createReview).toHaveBeenCalled(); + expect(global.exec.getExecOutput).toHaveBeenCalledWith("git", ["diff", "--name-only", "--no-renames", `${MOCK_BASE_COMMIT_SHA}..HEAD`]); + }); + it("should return error when both git am --3way and the fallback git am fail", async () => { global.exec = { exec: vi.fn().mockImplementation((cmd, args) => { diff --git a/actions/setup/js/manifest_file_helpers.cjs b/actions/setup/js/manifest_file_helpers.cjs index 8098ed1a012..68c9515c2d1 100644 --- a/actions/setup/js/manifest_file_helpers.cjs +++ b/actions/setup/js/manifest_file_helpers.cjs @@ -3,6 +3,16 @@ /** @typedef {import('./types/handler-factory').HandlerConfig} HandlerConfig */ const { extractDiffGitHeaderEntries } = require("./patch_path_helpers.cjs"); +function normalizeDotFolderExcludes(excludes) { + return new Set((Array.isArray(excludes) ? excludes : []).map(exclude => String(exclude || "").replace(/\/+$/, "")).filter(Boolean)); +} + +function formatHeaderLineForError(headerLine) { + const value = typeof headerLine === "string" ? headerLine : ""; + const truncated = value.length > 200 ? `${value.slice(0, 200)}…` : value; + return JSON.stringify(truncated); +} + /** * Extracts the unique set of file basenames (filename without directory path) changed in a git patch. * Parses "diff --git a/ b/" headers to determine which files were modified. @@ -58,7 +68,10 @@ function extractPathsFromPatch(patchContent) { const entries = extractDiffGitHeaderEntries(patchContent); for (const entry of entries) { if (!entry.parseable) { - continue; + // Fail-closed: unparseable diff --git headers are security-relevant because + // git am may still apply the hunk via ---/+++ fallback. Throwing here prevents + // silent policy bypass (see github/agentic-workflows#539). + throw new Error(`Patch contains unparseable diff --git header (fail-closed): ${formatHeaderLineForError(entry.headerLine)}. ` + `Cannot verify file-protection policy. Rejecting patch.`); } for (const filePath of [entry.oldPath, entry.newPath]) { if (filePath && filePath !== "dev/null") { @@ -175,13 +188,13 @@ function checkExcludedFiles(patchContent, excludedFilePatterns) { */ function checkForTopLevelDotFolders(patchContent, excludes) { const changedPaths = extractPathsFromPatch(patchContent); - const excludeSet = new Set(Array.isArray(excludes) ? excludes : []); + const excludeSet = normalizeDotFolderExcludes(excludes); const found = changedPaths.filter(p => { const slashIdx = p.indexOf("/"); if (slashIdx === -1) return false; // root-level file, not inside a folder const firstComponent = p.substring(0, slashIdx); if (!firstComponent.startsWith(".") || firstComponent.length < 2) return false; - return !excludeSet.has(firstComponent + "/"); + return !excludeSet.has(firstComponent); }); return { hasTopLevelDotFolders: found.length > 0, topLevelDotFoldersFound: found }; } @@ -226,8 +239,7 @@ function checkFileProtection(patchContent, config) { const prefixes = Array.isArray(config.protected_path_prefixes) ? config.protected_path_prefixes : []; const { manifestFilesFound } = checkForManifestFiles(patchContent, manifestFiles); const { protectedPathsFound } = checkForProtectedPaths(patchContent, prefixes); - const dotFolderExcludes = Array.isArray(config.protected_dot_folder_excludes) ? config.protected_dot_folder_excludes : []; - const { topLevelDotFoldersFound } = config.protect_top_level_dot_folders ? checkForTopLevelDotFolders(patchContent, dotFolderExcludes) : { topLevelDotFoldersFound: [] }; + const { topLevelDotFoldersFound } = config.protect_top_level_dot_folders ? checkForTopLevelDotFolders(patchContent, config.protected_dot_folder_excludes) : { topLevelDotFoldersFound: [] }; const allFound = [...new Set([...manifestFilesFound, ...protectedPathsFound, ...topLevelDotFoldersFound])]; if (allFound.length === 0) { @@ -243,4 +255,89 @@ function checkFileProtection(patchContent, config) { return { action: "deny", source: "protected", files: allFound }; } -module.exports = { extractFilenamesFromPatch, extractPathsFromPatch, checkForManifestFiles, checkForProtectedPaths, checkForTopLevelDotFolders, checkAllowedFiles, checkExcludedFiles, checkFileProtection }; +/** + * Post-apply file-protection verification. + * + * After `git am` applies a patch, this function runs `git diff --name-only` + * against the pre-apply commit to get the *actual* files written to the working + * tree. It then re-checks file protection against that ground-truth file list. + * + * This eliminates parser-differential attacks where the JS patch parser and + * `git am` disagree on which files a patch contains (see github/agentic-workflows#539). + * + * @param {string[]} actualFiles - List of files actually modified (from `git diff --name-only`) + * @param {HandlerConfig} config - The handler configuration with file protection rules + * @returns {{ action: 'allow' } | { action: 'deny', source: 'allowlist'|'protected'|'post-apply', files: string[] } | { action: 'fallback', files: string[] } | { action: 'request_review', files: string[] }} + */ +function checkFileProtectionPostApply(actualFiles, config) { + if (!actualFiles || actualFiles.length === 0) { + return { action: "allow" }; + } + + // Step 1: allowlist check + const allowedFilePatterns = Array.isArray(config.allowed_files) ? config.allowed_files : []; + if (allowedFilePatterns.length > 0) { + const { globPatternToRegex } = require("./glob_pattern_helpers.cjs"); + const compiledPatterns = allowedFilePatterns.map(p => globPatternToRegex(p)); + const disallowed = actualFiles.filter(file => { + return !compiledPatterns.some(re => re.test(file)); + }); + if (disallowed.length > 0) { + return { action: "deny", source: "post-apply", files: disallowed }; + } + } + + // Step 2: protected-files check + if (config.protected_files_policy === "allowed") { + return { action: "allow" }; + } + + const manifestFiles = Array.isArray(config.protected_files) ? config.protected_files : []; + const prefixes = Array.isArray(config.protected_path_prefixes) ? config.protected_path_prefixes : []; + const dotFolderExcludes = normalizeDotFolderExcludes(config.protected_dot_folder_excludes); + + const allProtected = []; + + // Check manifest files (basename match) + for (const file of actualFiles) { + const basename = file.split("/").pop() || ""; + if (manifestFiles.includes(basename) || manifestFiles.includes(file)) { + allProtected.push(file); + } + } + + // Check path prefixes + for (const file of actualFiles) { + if (prefixes.some(prefix => file.startsWith(prefix))) { + if (!allProtected.includes(file)) { + allProtected.push(file); + } + } + } + + // Check top-level dot folders + if (config.protect_top_level_dot_folders) { + for (const file of actualFiles) { + if (file.startsWith(".") && file.includes("/")) { + const topFolder = file.split("/")[0]; + if (!dotFolderExcludes.has(topFolder) && !allProtected.includes(file)) { + allProtected.push(file); + } + } + } + } + + if (allProtected.length === 0) { + return { action: "allow" }; + } + + if (config.protected_files_policy === "fallback-to-issue") { + return { action: "fallback", files: allProtected }; + } + if (config.protected_files_policy === "request_review") { + return { action: "request_review", files: allProtected }; + } + return { action: "deny", source: "protected", files: allProtected }; +} + +module.exports = { extractFilenamesFromPatch, extractPathsFromPatch, checkForManifestFiles, checkForProtectedPaths, checkForTopLevelDotFolders, checkAllowedFiles, checkExcludedFiles, checkFileProtection, checkFileProtectionPostApply }; diff --git a/actions/setup/js/manifest_file_helpers.test.cjs b/actions/setup/js/manifest_file_helpers.test.cjs index 31a6a642d2b..537c113b9f6 100644 --- a/actions/setup/js/manifest_file_helpers.test.cjs +++ b/actions/setup/js/manifest_file_helpers.test.cjs @@ -255,8 +255,13 @@ index 0000000..abc expect(result).toEqual([".github/workflows/ci.yml"]); }); - it("should parse quoted headers and ignore malformed headers", () => { + it("should parse quoted headers and throw on malformed headers (fail-closed)", () => { const patch = [`diff --git "a/.github/workflows/ci file.yml" "b/.github/workflows/ci file.yml"`, "index abc..def 100644", "diff --git ", "index abc..def 100644"].join("\n"); + expect(() => extractPathsFromPatch(patch)).toThrow(/unparseable diff --git header/); + }); + + it("should parse quoted headers when no malformed headers present", () => { + const patch = [`diff --git "a/.github/workflows/ci file.yml" "b/.github/workflows/ci file.yml"`, "index abc..def 100644"].join("\n"); const result = extractPathsFromPatch(patch); expect(result).toContain(".github/workflows/ci file.yml"); expect(result).toHaveLength(1); @@ -627,4 +632,115 @@ index abc..def 100644 expect(result.source).toBe("protected"); }); }); + + // =================================================================== + // Security regression tests for patch-parser differential bypass + // (see github/agentic-workflows#539) + // =================================================================== + describe("extractPathsFromPatch - fail-closed on unparseable headers (security)", () => { + const { extractPathsFromPatch } = require("./manifest_file_helpers.cjs"); + + it("should throw on unterminated quoted path (parseable: false)", () => { + const patch = `diff --git "a/x b/y\nindex abc..def 100644\n`; + expect(() => extractPathsFromPatch(patch)).toThrow(/unparseable diff --git header/); + }); + + it("should throw on diff --git with no paths", () => { + const patch = `diff --git \nindex abc..def 100644\n`; + expect(() => extractPathsFromPatch(patch)).toThrow(/unparseable diff --git header/); + }); + + it("should throw on diff --git with only one token", () => { + const patch = `diff --git a/only-one-token\nindex abc..def 100644\n`; + expect(() => extractPathsFromPatch(patch)).toThrow(/unparseable diff --git header/); + }); + + it("should escape unparseable diff --git headers in the error message", () => { + const patch = 'diff --git "a/\u001b[31mevil b/file.txt\nindex abc..def 100644\n'; + let thrown; + try { + extractPathsFromPatch(patch); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(Error); + expect(thrown.message).toContain("\\u001b"); + expect(thrown.message).not.toContain("\u001b"); + }); + + it("should NOT throw on valid diff --git headers", () => { + const patch = `diff --git a/README.md b/README.md\nindex abc..def 100644\n`; + expect(() => extractPathsFromPatch(patch)).not.toThrow(); + expect(extractPathsFromPatch(patch)).toEqual(["README.md"]); + }); + }); + + describe("checkFileProtectionPostApply", () => { + const { checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs"); + + it("should allow when no files modified", () => { + const result = checkFileProtectionPostApply([], {}); + expect(result.action).toBe("allow"); + }); + + it("should allow when files are within allowed-files list", () => { + const result = checkFileProtectionPostApply(["src/index.js", "src/utils.js"], { allowed_files: ["src/**"] }); + expect(result.action).toBe("allow"); + }); + + it("should deny when files violate allowed-files (post-apply detection)", () => { + const result = checkFileProtectionPostApply(["src/index.js", ".github/workflows/evil.yml"], { allowed_files: ["src/**"] }); + expect(result.action).toBe("deny"); + expect(result.source).toBe("post-apply"); + expect(result.files).toContain(".github/workflows/evil.yml"); + }); + + it("should detect protected path prefixes in actual files", () => { + const result = checkFileProtectionPostApply(["README.md", ".github/CODEOWNERS"], { protected_path_prefixes: [".github/"], protected_files_policy: "deny" }); + expect(result.action).toBe("deny"); + expect(result.files).toContain(".github/CODEOWNERS"); + }); + + it("should detect top-level dot folders", () => { + const result = checkFileProtectionPostApply(["src/app.js", ".husky/pre-commit"], { protect_top_level_dot_folders: true, protected_dot_folder_excludes: [], protected_files_policy: "deny" }); + expect(result.action).toBe("deny"); + expect(result.files).toContain(".husky/pre-commit"); + }); + + it("should respect dot folder excludes", () => { + const result = checkFileProtectionPostApply([".changeset/fix.md"], { protect_top_level_dot_folders: true, protected_dot_folder_excludes: [".changeset/"], protected_files_policy: "deny" }); + expect(result.action).toBe("allow"); + }); + + it("should return fallback when policy is fallback-to-issue", () => { + const result = checkFileProtectionPostApply([".github/CODEOWNERS"], { protected_path_prefixes: [".github/"], protected_files_policy: "fallback-to-issue" }); + expect(result.action).toBe("fallback"); + expect(result.files).toContain(".github/CODEOWNERS"); + }); + + it("should return request_review when policy is request_review", () => { + const result = checkFileProtectionPostApply(["package.json"], { protected_files: ["package.json"], protected_files_policy: "request_review" }); + expect(result.action).toBe("request_review"); + expect(result.files).toContain("package.json"); + }); + + // Regression: variant 1 from the issue — hunk with no diff --git header + it("should catch files that bypass the pre-apply parser (variant 1: no diff --git header)", () => { + // Simulates what git am actually applies vs what the parser sees + const actualFilesFromGitDiff = ["README.md", ".github/workflows/evil.yml"]; + const result = checkFileProtectionPostApply(actualFilesFromGitDiff, { allowed_files: ["README.md", "src/**"] }); + expect(result.action).toBe("deny"); + expect(result.source).toBe("post-apply"); + expect(result.files).toContain(".github/workflows/evil.yml"); + }); + + // Regression: variant 2 — rename to overrides diff --git line + it("should catch files that bypass via rename-to extended header (variant 2)", () => { + const actualFilesFromGitDiff = [".github/CODEOWNERS"]; + const result = checkFileProtectionPostApply(actualFilesFromGitDiff, { allowed_files: ["README.md", "docs/**"] }); + expect(result.action).toBe("deny"); + expect(result.source).toBe("post-apply"); + expect(result.files).toContain(".github/CODEOWNERS"); + }); + }); }); diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 15f247ef5bb..85e91339e32 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -14,7 +14,7 @@ const { pushExtraEmptyCommit } = require("./extra_empty_commit.cjs"); const { detectForkPR, checkBranchPushable } = require("./pr_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); -const { checkFileProtection } = require("./manifest_file_helpers.cjs"); +const { checkFileProtection, checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs"); const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); const { renderTemplateFromFile, buildProtectedFileList, getPromptPath } = require("./messages_core.cjs"); const { ensureFullHistoryForBundle, getGitAuthEnv, extractBundlePrerequisiteCommits, isShallowOrSparseCheckout, linearizeRangeAsCommit } = require("./git_helpers.cjs"); @@ -540,16 +540,14 @@ async function main(config = {}) { core.info(`✓ Labels validation passed: ${envLabels.join(", ")}`); } - // Deferred protected file protection – fallback-to-issue path. - // Create a review issue now that we have repoParts, pullNumber, and prTitle available. - if (protectedFilesForFallback && protectedFilesForFallback.length > 0) { + const createProtectedFilesFallbackIssue = async files => { const runUrl = buildWorkflowRunUrl(context, context.repo); const runId = context.runId; const patchFileName = patchFilePath ? patchFilePath.replace("/tmp/gh-aw/", "") : "aw-unknown.patch"; const githubServer = process.env.GITHUB_SERVER_URL || "https://github.com"; const prUrl = `${githubServer}/${repoParts.owner}/${repoParts.repo}/pull/${pullNumber}`; const issueTitle = `[gh-aw] Protected Files: ${prTitle || `PR #${pullNumber}`}`; - const fileList = buildProtectedFileList(protectedFilesForFallback, githubServer, repoParts.owner, repoParts.repo, branchName); + const fileList = buildProtectedFileList(files, githubServer, repoParts.owner, repoParts.repo, branchName); const templatePath = getPromptPath("manifest_protection_push_to_pr_fallback.md"); const issueBody = renderTemplateFromFile(templatePath, { files: fileList, @@ -591,6 +589,12 @@ async function main(config = {}) { core.error(error); return { success: false, error }; } + }; + + // Deferred protected file protection – fallback-to-issue path. + // Create a review issue now that we have repoParts, pullNumber, and prTitle available. + if (protectedFilesForFallback && protectedFilesForFallback.length > 0) { + return await createProtectedFilesFallbackIssue(protectedFilesForFallback); } const hasChanges = !isEmpty; @@ -912,6 +916,35 @@ async function main(config = {}) { } // end else (patch path) core.info(`Apply transport completed; signed-push base ref: ${rangeBaseRef}`); + // POST-APPLY FILE PROTECTION: Verify actual files written match policy. + // This is the primary defense against parser-differential attacks where the JS + // patch parser and git am disagree on which files a patch contains. + // (see github/agentic-workflows#539) + { + const diffResult = await exec.getExecOutput("git", ["diff", "--name-only", "--no-renames", `${rangeBaseRef}..HEAD`], baseGitOpts); + const actualFiles = diffResult.stdout + .split("\n") + .map(f => f.trim()) + .filter(Boolean); + if (actualFiles.length > 0) { + core.info(`Post-apply verification: ${actualFiles.length} file(s) actually modified`); + const postApplyProtection = checkFileProtectionPostApply(actualFiles, config); + if (postApplyProtection.action === "deny") { + const filesStr = postApplyProtection.files.join(", "); + const msg = `SECURITY: Post-apply file-protection check failed. ` + `The patch applied files that were not detected by the pre-apply parser: ${filesStr}. ` + `This may indicate a patch-parser bypass attempt. Aborting push.`; + core.error(msg); + // Reset to pre-apply state + await exec.exec("git", ["reset", "--hard", rangeBaseRef], baseGitOpts); + return { success: false, error: msg }; + } + if (postApplyProtection.action === "fallback") { + core.warning(`Post-apply: Protected file protection triggered (fallback-to-issue): ${postApplyProtection.files.join(", ")}`); + await exec.exec("git", ["reset", "--hard", rangeBaseRef], baseGitOpts); + return await createProtectedFilesFallbackIssue(postApplyProtection.files); + } + } + } + // When threat detection produced a warning, create a review PR instead of pushing // directly to the existing PR branch. This allows manual review of the changes // before they are merged into the target PR. diff --git a/actions/setup/js/push_to_pull_request_branch.test.cjs b/actions/setup/js/push_to_pull_request_branch.test.cjs index 56b95bc49d2..08a53eb2d8a 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -157,6 +157,14 @@ describe("push_to_pull_request_branch.cjs", () => { }), getBranchProtection: vi.fn().mockRejectedValue(Object.assign(new Error("Branch not protected"), { status: 404 })), }, + issues: { + create: vi.fn().mockResolvedValue({ + data: { + number: 456, + html_url: "https://github.com/test-owner/test-repo/issues/456", + }, + }), + }, }, graphql: vi.fn(), }; @@ -783,6 +791,7 @@ index 0000000..abc1234 mockExec.getExecOutput .mockResolvedValueOnce({ exitCode: 0, stdout: "preflight-sha\trefs/heads/feature-branch\n", stderr: "" }) // preflight ls-remote .mockResolvedValueOnce({ exitCode: 0, stdout: "local-head-before\n", stderr: "" }) // rev-parse HEAD before patch + .mockResolvedValueOnce({ exitCode: 0, stdout: "test.txt\n", stderr: "" }) // post-apply git diff --name-only --no-renames .mockResolvedValueOnce({ exitCode: 0, stdout: "0\n", stderr: "" }) // rev-list --merges --count (no merge commits) .mockResolvedValueOnce({ exitCode: 0, stdout: "1\n", stderr: "" }) // rev-list --count (new commits) .mockResolvedValueOnce({ exitCode: 0, stdout: " file.txt | 1 +\n 1 file changed, 1 insertion(+)\n", stderr: "" }); // git diff --stat (non-empty = has file changes) @@ -2141,7 +2150,12 @@ ${diffs} it("should accept files that match the allowed-files pattern", async () => { const patchPath = createPatchFile(createPatchWithFiles(".changeset/my-feature-fix.md")); - mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (cmd, args) => { + if (cmd === "git" && Array.isArray(args) && args[0] === "diff" && args[1] === "--name-only" && args[2] === "--no-renames") { + return { exitCode: 0, stdout: ".changeset/my-feature-fix.md\n", stderr: "" }; + } + return { exitCode: 0, stdout: "abc123\n", stderr: "" }; + }); const module = await loadModule(); const handler = await module.main({ allowed_files: [".changeset/**"] }); @@ -2171,7 +2185,12 @@ ${diffs} it("should allow a protected file when both allowed-files matches and protected-files: allowed is set", async () => { // Both checks are satisfied explicitly: allowlist scope + protected-files permission. const patchPath = createPatchFile(createPatchWithFiles("package.json")); - mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + mockExec.getExecOutput.mockImplementation(async (cmd, args) => { + if (cmd === "git" && Array.isArray(args) && args[0] === "diff" && args[1] === "--name-only" && args[2] === "--no-renames") { + return { exitCode: 0, stdout: "package.json\n", stderr: "" }; + } + return { exitCode: 0, stdout: "abc123\n", stderr: "" }; + }); const module = await loadModule(); const handler = await module.main({ @@ -2184,6 +2203,46 @@ ${diffs} expect(result.success).toBe(true); }); + it("should create a fallback issue instead of pushing when post-apply detects protected files", async () => { + const patchPath = createPatchFile(createPatchWithFiles("README.md")); + const pushSignedCommitsModule = require("./push_signed_commits.cjs"); + const pushSignedSpy = vi.spyOn(pushSignedCommitsModule, "pushSignedCommits").mockResolvedValue("remote-head-after"); + const promptsDir = path.join(tempDir, "prompts"); + fs.mkdirSync(promptsDir, { recursive: true }); + fs.writeFileSync(path.join(promptsDir, "manifest_protection_push_to_pr_fallback.md"), "Protected files: {{files}}", "utf8"); + process.env.GH_AW_PROMPTS_DIR = promptsDir; + mockExec.getExecOutput.mockImplementation(async (cmd, args) => { + if (cmd === "git" && Array.isArray(args) && args[0] === "ls-remote") { + return { exitCode: 0, stdout: "preflight-sha\trefs/heads/feature-branch\n", stderr: "" }; + } + if (cmd === "git" && Array.isArray(args) && args[0] === "rev-parse") { + return { exitCode: 0, stdout: "local-head-before\n", stderr: "" }; + } + if (cmd === "git" && Array.isArray(args) && args[0] === "diff" && args[1] === "--name-only" && args[2] === "--no-renames") { + return { exitCode: 0, stdout: ".github/CODEOWNERS\n", stderr: "" }; + } + return { exitCode: 0, stdout: "", stderr: "" }; + }); + + const module = await loadModule(); + const handler = await module.main({ + protected_path_prefixes: [".github/"], + protected_files_policy: "fallback-to-issue", + }); + try { + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(result.fallback_used).toBe(true); + expect(result.issue_number).toBe(456); + expect(mockExec.exec).toHaveBeenCalledWith("git", ["reset", "--hard", "local-head-before"], expect.any(Object)); + expect(mockGithub.rest.issues.create).toHaveBeenCalledTimes(1); + expect(pushSignedSpy).not.toHaveBeenCalled(); + } finally { + pushSignedSpy.mockRestore(); + } + }); + it("should block a protected file when no allowed-files list is configured", async () => { const patchPath = createPatchFile(createPatchWithFiles("package.json"));