From 038c8ff64fb6ba169d4e09fa622b337627967eb3 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Wed, 3 Jun 2026 16:24:38 -0700 Subject: [PATCH 1/4] fix: patch-parser file-protection bypass via post-apply enforcement Addresses a security vulnerability where the JS patch parser and git am can disagree on which files a patch contains, allowing an attacker to bypass allowed-files/protected-files policies. Three fixes: 1. Post-apply enforcement (primary): After git am applies a patch and BEFORE push, run `git diff --name-only` against the pre-apply commit to get the actual files written. Re-check file protection against this ground-truth list. This eliminates all parser-differential attacks regardless of variant. 2. Fail-closed on unparseable headers: extractPathsFromPatch() now throws on any diff --git header where parseable === false, instead of silently skipping it. This prevents crafted headers from evading policy checks. 3. Regression tests: Added test cases for unparseable headers, post-apply detection of files outside allowed-files, protected paths, and top-level dot folders. Fixes github/agentic-workflows#539 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- actions/setup/js/create_pull_request.cjs | 34 ++++- actions/setup/js/manifest_file_helpers.cjs | 95 ++++++++++++- .../setup/js/manifest_file_helpers.test.cjs | 132 +++++++++++++++++- .../setup/js/push_to_pull_request_branch.cjs | 29 +++- 4 files changed, 285 insertions(+), 5 deletions(-) diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 43adbcfbe32..8fd5d78efa3 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -27,7 +27,7 @@ const { closeOlderPullRequests } = require("./close_older_pull_requests.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"); @@ -1740,6 +1740,38 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo } } + // 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", `origin/${baseBranch}..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", + }); + 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 }; + } + 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(", ")}`); + } + } + } + // 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 diff --git a/actions/setup/js/manifest_file_helpers.cjs b/actions/setup/js/manifest_file_helpers.cjs index 8098ed1a012..30008df0f39 100644 --- a/actions/setup/js/manifest_file_helpers.cjs +++ b/actions/setup/js/manifest_file_helpers.cjs @@ -58,7 +58,13 @@ 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): "${entry.headerLine}". ` + + `Cannot verify file-protection policy. Rejecting patch.` + ); } for (const filePath of [entry.oldPath, entry.newPath]) { if (filePath && filePath !== "dev/null") { @@ -243,4 +249,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 = Array.isArray(config.protected_dot_folder_excludes) ? 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.includes(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..2d34ebe0bdd 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,129 @@ 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 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 b99e1d28e79..5f6bd7eaf78 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"); @@ -898,6 +898,33 @@ 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") { + protectedFilesForFallback = postApplyProtection.files; + core.warning(`Post-apply: Protected file protection triggered (fallback-to-issue): ${postApplyProtection.files.join(", ")}`); + } + } + } + // 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. From 401b8cac106243876cbae43bbbc08bc096e13de8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 3 Jun 2026 23:49:47 +0000 Subject: [PATCH 2/4] fix: address safe-output review feedback --- actions/setup/js/create_pull_request.cjs | 23 +++++-- actions/setup/js/create_pull_request.test.cjs | 59 +++++++++++++++++ actions/setup/js/manifest_file_helpers.cjs | 28 +++++---- .../setup/js/manifest_file_helpers.test.cjs | 58 +++++++---------- .../setup/js/push_to_pull_request_branch.cjs | 24 ++++--- .../js/push_to_pull_request_branch.test.cjs | 63 ++++++++++++++++++- 6 files changed, 192 insertions(+), 63 deletions(-) diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 8fd5d78efa3..102577fac39 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -1604,6 +1604,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) { @@ -1629,6 +1638,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; @@ -1709,6 +1719,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)}`); @@ -1745,8 +1756,12 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo // 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", `origin/${baseBranch}..HEAD`]); - const actualFiles = diffResult.stdout.split("\n").map(f => f.trim()).filter(Boolean); + 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, { @@ -1755,9 +1770,7 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo }); 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.`; + 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 }; } 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 30008df0f39..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. @@ -61,10 +71,7 @@ function extractPathsFromPatch(patchContent) { // 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): "${entry.headerLine}". ` + - `Cannot verify file-protection policy. Rejecting patch.` - ); + 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") { @@ -181,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 }; } @@ -232,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) { @@ -288,13 +294,13 @@ function checkFileProtectionPostApply(actualFiles, config) { const manifestFiles = Array.isArray(config.protected_files) ? config.protected_files : []; const prefixes = Array.isArray(config.protected_path_prefixes) ? config.protected_path_prefixes : []; - const dotFolderExcludes = Array.isArray(config.protected_dot_folder_excludes) ? config.protected_dot_folder_excludes : []; + 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(); + const basename = file.split("/").pop() || ""; if (manifestFiles.includes(basename) || manifestFiles.includes(file)) { allProtected.push(file); } @@ -314,7 +320,7 @@ function checkFileProtectionPostApply(actualFiles, config) { for (const file of actualFiles) { if (file.startsWith(".") && file.includes("/")) { const topFolder = file.split("/")[0]; - if (!dotFolderExcludes.includes(topFolder) && !allProtected.includes(file)) { + if (!dotFolderExcludes.has(topFolder) && !allProtected.includes(file)) { allProtected.push(file); } } diff --git a/actions/setup/js/manifest_file_helpers.test.cjs b/actions/setup/js/manifest_file_helpers.test.cjs index 2d34ebe0bdd..537c113b9f6 100644 --- a/actions/setup/js/manifest_file_helpers.test.cjs +++ b/actions/setup/js/manifest_file_helpers.test.cjs @@ -655,6 +655,19 @@ index abc..def 100644 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(); @@ -671,63 +684,42 @@ index abc..def 100644 }); it("should allow when files are within allowed-files list", () => { - const result = checkFileProtectionPostApply( - ["src/index.js", "src/utils.js"], - { allowed_files: ["src/**"] } - ); + 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/**"] } - ); + 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" } - ); + 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" } - ); + 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" } - ); + 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" } - ); + 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" } - ); + 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"); }); @@ -736,10 +728,7 @@ index abc..def 100644 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/**"] } - ); + 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"); @@ -748,10 +737,7 @@ index abc..def 100644 // 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/**"] } - ); + 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 5f6bd7eaf78..3b9e8684db0 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -526,16 +526,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, @@ -577,6 +575,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; @@ -904,23 +908,25 @@ async function main(config = {}) { // (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); + 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.`; + 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") { - protectedFilesForFallback = postApplyProtection.files; 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); } } } 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")); From 421cfb46beb6654187ef820b8683be6f68d10004 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 4 Jun 2026 02:08:27 +0000 Subject: [PATCH 3/4] chore: plan review feedback Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 1 + 1 file changed, 1 insertion(+) 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` From 32a466c591a5170bbdae38d23803afde9c638f00 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 4 Jun 2026 03:14:45 +0000 Subject: [PATCH 4/4] Add changeset --- .changeset/patch-fix-patch-parser-file-protection-bypass.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/patch-fix-patch-parser-file-protection-bypass.md 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`.