From ad6582c367f72ef4184b8dc106b60c2c0ec06514 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:43:10 +0000 Subject: [PATCH 1/7] Initial plan From 36b031cb2106c725e401f5f4f3d3c05e26404679 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 18 Oct 2026 20:54:45 +0000 Subject: [PATCH 2/7] Recognize merged base ancestors behind the trusted branch tip Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../js/bundle_transport.integration.test.cjs | 42 ++++- .../setup/js/push_to_pull_request_branch.cjs | 57 +++++- ...o_pull_request_branch.integration.test.cjs | 26 ++- .../js/push_to_pull_request_branch.test.cjs | 178 ++++++++++++------ 4 files changed, 238 insertions(+), 65 deletions(-) diff --git a/actions/setup/js/bundle_transport.integration.test.cjs b/actions/setup/js/bundle_transport.integration.test.cjs index 97943f006b2..c7749f0fb23 100644 --- a/actions/setup/js/bundle_transport.integration.test.cjs +++ b/actions/setup/js/bundle_transport.integration.test.cjs @@ -6,7 +6,7 @@ import os from "os"; import path from "path"; const { ensureFullHistoryForBundle, importLocalBundle, offlineGitOptions } = require("./git_helpers.cjs"); -const { resolveTrustedBaseTip, getBundleValidationBase, getBundlePreApplyFiles } = require("./push_to_pull_request_branch.cjs"); +const { resolveTrustedBaseTip, resolveBundleTrustedBase, getBundleValidationBase, getBundlePreApplyFiles } = require("./push_to_pull_request_branch.cjs"); const { checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs"); const gitEnv = () => { @@ -109,6 +109,46 @@ describe("offline bundle transport with real Git", () => { return { oldHead, trustedBase, candidate }; } + it("recognizes the newest merged base without fetching an advanced trusted tip", async () => { + git(source, ["checkout", "-b", "feature"]); + const oldHead = addCommit("existing.txt", "PR work\n"); + git(source, ["checkout", "main"]); + const olderBase = addCommit("older.txt", "older upstream\n"); + git(source, ["checkout", "feature"]); + git(source, ["merge", "--no-ff", "main", "-m", "merge older base"]); + git(source, ["checkout", "main"]); + const mergedBase = addCommit("package.json", '{"upstream":true}\n'); + git(source, ["checkout", "feature"]); + addCommit("agent.txt", "agent work\n"); + git(source, ["merge", "--no-ff", "main", "-m", "merge newer base"]); + const bundle = path.join(root, "agent.bundle"); + git(source, ["bundle", "create", bundle, "feature", `^${oldHead}`]); + const target = path.join(root, "target"); + git(root, ["clone", source, target]); + git(source, ["checkout", "main"]); + const trustedTip = addCommit("later.txt", "base advances after agent finishes\n"); + const exec = execApi(target); + await importLocalBundle(exec, bundle, "refs/heads/feature", "refs/bundles/test"); + const before = packInventory(target); + expect(await resolveTrustedBaseTip(exec, {}, "main")).toBe(trustedTip); + const compareCommits = vi.fn(async ({ base, head }) => { + expect(base).toBe(trustedTip); + const result = spawnSync("git", ["merge-base", "--is-ancestor", head, base], { cwd: source, env: gitEnv() }); + return { data: { status: result.status === 0 ? "behind" : "diverged" } }; + }); + const recognized = await resolveBundleTrustedBase(exec, {}, oldHead, "refs/bundles/test", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }); + expect(recognized).toBe(mergedBase); + expect(compareCommits.mock.calls.some(([args]) => args.head === olderBase)).toBe(false); + const baseline = await getBundleValidationBase(exec, {}, oldHead, "refs/bundles/test", recognized); + const files = await getBundlePreApplyFiles(exec, {}, baseline.ref, "refs/bundles/test", baseline.conflictedFiles); + expect(files).toEqual(["agent.txt"]); + expect(checkFileProtectionPostApply(files, { protected_files: ["package.json"] }).action).toBe("allow"); + const absent = await exec.getExecOutput("git", ["cat-file", "-e", trustedTip], { ...offlineGitOptions(), ignoreReturnCode: true }); + expect(absent.exitCode).not.toBe(0); + expect(packInventory(target)).toEqual(before); + expect(exec.calls.some(args => args[0] === "fetch")).toBe(false); + }); + it.each([false, true])("does not lazily transfer blobs while inspecting a partial clone (old Git guard: %s)", async emulateOldGit => { const { oldHead, trustedBase, candidate } = mergeFixture(); git(source, ["config", "uploadpack.allowFilter", "true"]); diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index ae364d58d42..3861ac3bfad 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -201,6 +201,58 @@ async function resolveTrustedBaseTip(exec, gitOptions, baseRef) { return ""; } +/** + * Recognize a newly merged base ancestor using only the pinned remote tip as + * provenance. Topological order prefers newer parents regardless of merge order + * or commit timestamps. All object inspection stays offline. + * + * @param {{ getExecOutput: Function }} exec + * @param {Record} gitOptions + * @param {string} oldHead + * @param {string} candidate + * @param {string} trustedTip + * @param {typeof github} githubClient + * @param {{ owner: string, repo: string }} repoParts + * @returns {Promise} + */ +async function resolveBundleTrustedBase(exec, gitOptions, oldHead, candidate, trustedTip, githubClient, repoParts) { + if (!trustedTip) return ""; + const opts = { ...offlineGitOptions(gitOptions), ignoreReturnCode: true }; + try { + const tipAncestor = await exec.getExecOutput("git", ["merge-base", "--is-ancestor", trustedTip, candidate], opts); + if (tipAncestor.exitCode === 0) return trustedTip; + + const merges = await exec.getExecOutput("git", ["rev-list", "--merges", "--parents", `${oldHead}..${candidate}`], opts); + if (merges.exitCode !== 0) return ""; + const parents = new Set( + merges.stdout + .trim() + .split("\n") + .flatMap(line => line.split(" ").slice(1)) + .filter(sha => /^[0-9a-f]{40}$/.test(sha)) + ); + if (!parents.size) return ""; + const ordered = await exec.getExecOutput("git", ["rev-list", "--topo-order", ...parents, "--not", oldHead], opts); + if (ordered.exitCode !== 0) return ""; + for (const parent of ordered.stdout.trim().split("\n")) { + if (!parents.has(parent)) continue; + try { + // Comparing tip...parent reports "behind" when parent is an ancestor. + const { data } = await githubClient.rest.repos.compareCommits({ ...repoParts, base: trustedTip, head: parent, per_page: 1 }); + if (data.status === "behind" || data.status === "identical") return parent; + } catch { + // A parent may exist only in the bundle, or comparison may be unavailable. + core.debug("Bundle validation: merge parent comparison unavailable"); + } + } + } catch { + // Missing local history must not trigger a fetch or expose remote diagnostics. + core.debug("Bundle validation: merged base history unavailable"); + } + core.debug("Bundle validation: no trusted merged base ancestor confirmed; using PR-head validation"); + return ""; +} + /** * Measure the expanded blob size of files changed by the applied agent commits. * Deleted files contribute zero bytes because no new content is being introduced. @@ -1122,7 +1174,7 @@ async function main(config = {}) { core.info(`Applying changes from bundle: ${bundleFilePath}`); const bundleRef = `refs/bundles/push-${branchName.replace(/[^a-zA-Z0-9-]/g, "-")}`; try { - const trustedBase = signedCommits ? "" : await resolveTrustedBaseTip(exec, baseGitOpts, pullRequest.base?.ref); + const trustedTip = signedCommits ? "" : await resolveTrustedBaseTip(exec, baseGitOpts, pullRequest.base?.ref); const bundleGitOpts = offlineGitOptions(baseGitOpts); await ensureFullHistoryForBundle( exec, @@ -1140,6 +1192,7 @@ async function main(config = {}) { await importLocalBundle(exec, bundleFilePath, `refs/heads/${branchName}`, bundleRef, baseGitOpts); core.info(`Imported local bundle to ${bundleRef}`); + const trustedBase = await resolveBundleTrustedBase(exec, baseGitOpts, rangeBaseRef, bundleRef, trustedTip, githubClient, repoParts); const validationBase = await getBundleValidationBase(exec, baseGitOpts, rangeBaseRef, bundleRef, trustedBase); validationBaseRef = validationBase.ref; validationConflictFiles = validationBase.conflictedFiles; @@ -1821,4 +1874,4 @@ async function main(config = {}) { }); } -module.exports = { main, HANDLER_TYPE, getBundlePreApplyFiles, getBundleValidationBase, resolveTrustedBaseTip }; +module.exports = { main, HANDLER_TYPE, getBundlePreApplyFiles, getBundleValidationBase, resolveTrustedBaseTip, resolveBundleTrustedBase }; diff --git a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs index 8529dbe467f..3fe34ed9d9f 100644 --- a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs @@ -6,7 +6,7 @@ import path from "path"; import { spawnSync } from "child_process"; const require = createRequire(import.meta.url); -const { getBundlePreApplyFiles, getBundleValidationBase } = require("./push_to_pull_request_branch.cjs"); +const { getBundlePreApplyFiles, getBundleValidationBase, resolveBundleTrustedBase } = require("./push_to_pull_request_branch.cjs"); const { checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs"); global.core = { @@ -79,7 +79,10 @@ describe("push_to_pull_request_branch bundle integration", () => { [false, false], [true, false], [false, true], - ])("excludes trusted base changes while counting agent changes (conflict: %s, reverted change: %s)", async (conflict, revertedChange) => { + [false, false, true], + [true, false, true], + [false, true, true], + ])("excludes trusted base changes while counting agent changes (conflict: %s, reverted change: %s, advanced base: %s)", async (conflict, revertedChange, baseAdvanced = false) => { const repo = createRepo("push-pr-validation-base-"); tempDirs.push(repo); const git = args => execGit(args, { cwd: repo }); @@ -119,7 +122,24 @@ describe("push_to_pull_request_branch bundle integration", () => { git(["commit", "-m", "agent work"]); const exec = createExecApi(repo); - const { ref: baseline, conflictedFiles } = await getBundleValidationBase(exec, {}, oldHead, "HEAD", trustedBase); + let trustedTip = trustedBase; + if (baseAdvanced) { + git(["checkout", "main"]); + writeRepoFile(repo, "later.txt", "later base work\n"); + git(["add", "."]); + git(["commit", "-m", "base advances after agent merge"]); + trustedTip = git(["rev-parse", "HEAD"]).stdout.trim(); + git(["checkout", "feature"]); + } + const compareCommits = vi.fn(async ({ base, head }) => { + expect(base).toBe(trustedTip); + const ancestor = execGit(["merge-base", "--is-ancestor", head, base], { cwd: repo, allowFailure: true }); + return { data: { status: ancestor.status === 0 ? "behind" : "diverged" } }; + }); + const recognizedBase = await resolveBundleTrustedBase(exec, {}, oldHead, "HEAD", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }); + expect(recognizedBase).toBe(trustedBase); + expect(compareCommits).toHaveBeenCalledTimes(baseAdvanced ? 1 : 0); + const { ref: baseline, conflictedFiles } = await getBundleValidationBase(exec, {}, oldHead, "HEAD", recognizedBase); expect(baseline).not.toBe(oldHead); const files = await getBundlePreApplyFiles(exec, {}, baseline, "HEAD", conflictedFiles); expect(files.sort()).toEqual(conflict ? ["agent.txt", "conflict.txt"] : ["agent.txt"]); 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 17da0204bdb..f2022395f46 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -346,6 +346,46 @@ index 0000000..abc1234 return patchPath; } + describe("merged base provenance", () => { + const trustedTip = "a".repeat(40); + const mergedBase = "b".repeat(40); + + it.each(["behind", "identical", "ahead", "diverged", "unknown", "error"])("uses only confirmed ancestors of the pinned tip (compare status: %s)", async status => { + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[0] === "merge-base") return { exitCode: 128, stdout: "", stderr: "tip object unavailable" }; + if (args[1] === "--merges") return { exitCode: 0, stdout: `${"c".repeat(40)} ${mergedBase} ${mergedBase}\n`, stderr: "" }; + return { exitCode: 0, stdout: `${mergedBase}\n`, stderr: "" }; + }); + mockGithub.rest.repos.compareCommits = vi.fn(async () => { + if (status === "error") throw new Error("synthetic secret-bearing API diagnostic"); + return { data: { status } }; + }); + const module = await loadModule(); + const confirmed = status === "behind" || status === "identical"; + const base = await module.resolveBundleTrustedBase(mockExec, { cwd: tempDir }, "old-head", "candidate", trustedTip, mockGithub, mockContext.repo); + expect(base).toBe(confirmed ? mergedBase : ""); + expect(mockGithub.rest.repos.compareCommits).toHaveBeenCalledExactlyOnceWith({ ...mockContext.repo, base: trustedTip, head: mergedBase, per_page: 1 }); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["rev-list", "--topo-order", mergedBase, "--not", "old-head"], expect.any(Object)); + for (const [, , options] of mockExec.getExecOutput.mock.calls) { + expect(options).toEqual(expect.objectContaining({ cwd: tempDir, silent: true, env: expect.objectContaining({ GIT_ALLOW_PROTOCOL: "", GIT_NO_LAZY_FETCH: "1" }) })); + } + expect(JSON.stringify([mockCore.debug.mock.calls, mockCore.warning.mock.calls])).not.toContain("synthetic"); + }); + + it.each(["no tip", "no merges", "already merged", "missing history"])("keeps PR-head validation without remote comparisons when there is %s", async scenario => { + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[0] === "merge-base" || scenario === "missing history") return { exitCode: 128, stdout: "", stderr: "" }; + return { exitCode: 0, stdout: scenario === "already merged" && args[1] === "--merges" ? `${"c".repeat(40)} ${mergedBase}\n` : "", stderr: "" }; + }); + mockGithub.rest.repos.compareCommits = vi.fn(); + const module = await loadModule(); + const base = await module.resolveBundleTrustedBase(mockExec, {}, "old-head", "candidate", scenario === "no tip" ? "" : trustedTip, mockGithub, mockContext.repo); + expect(base).toBe(""); + expect(mockGithub.rest.repos.compareCommits).not.toHaveBeenCalled(); + expect(await module.getBundleValidationBase(mockExec, {}, "old-head", "candidate", base)).toEqual({ ref: "old-head", conflictedFiles: [] }); + }); + }); + // ────────────────────────────────────────────────────── // Configuration Parsing Tests // ────────────────────────────────────────────────────── @@ -2330,7 +2370,7 @@ index 0000000..abc1234 describe("patch size validation", () => { const protectedPatch = "diff --git a/package.json b/package.json\n--- a/package.json\n+++ b/package.json\n@@ -1 +1 @@\n-old\n+new\n"; - it.each([ + const scenarios = [ ["small agent change", "small patch\n", 16, "merge-agent.txt", null, false], ["oversized agent patch", "x".repeat(2048), 16, "merge-agent.txt", "Patch size", false], ["oversized transient agent commit patch", "small patch\n", 16, "merge-agent.txt", "Patch size", false, "", false, false, "x".repeat(2048)], @@ -2341,66 +2381,86 @@ index 0000000..abc1234 ["reverted protected-file change after trusted-base fallback", "small patch\n", 16, "merge-agent.txt", "bundle modifies protected files", false, "package.json", false, true], ["agent-created octopus merge", "small patch\n", 16, "merge-agent.txt", "octopus merge", false, "", true], ["agent-created octopus merge after trusted-base fallback", "small patch\n", 16, "merge-agent.txt", "octopus merge", false, "", true, true], - ])("validates %s independently of merged base changes", async (_name, agentPatch, contentSize, changedFile, expectedError, binaryConflict, historyFile = "", octopusMerge = false, mergeTreeFailure = false, historyPatch = agentPatch) => { - const branch = "merge-size-validation"; - const trustedBase = "b".repeat(40); - const validationTree = "a".repeat(40); - createPatchFile(branch, protectedPatch + "x".repeat(4096)); - fs.writeFileSync(canonicalBundlePath(branch), "bundle content"); - const changedPath = path.join(process.cwd(), "merge-agent.txt"); - fs.writeFileSync(changedPath, Buffer.alloc(contentSize)); - const pr = (await mockGithub.rest.pulls.get()).data; - pr.base.sha = trustedBase; - pr.base.ref = "main"; - mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { - if (args[0] === "ls-remote" && args.includes("--refs")) return { exitCode: 0, stdout: `${trustedBase}\trefs/heads/main\n`, stderr: "" }; - if (args[0] === "merge-base") { - return { exitCode: args[3] === "remote-head" ? 1 : 0, stdout: "", stderr: "" }; - } - if (args[0] === "merge-tree") { - if (mergeTreeFailure) return { exitCode: 128, stdout: "", stderr: "unsupported merge-tree" }; - return { exitCode: binaryConflict ? 1 : 0, stdout: validationTree + (binaryConflict ? "\0merge-agent.txt\0\0" : "\0"), stderr: "" }; - } - if (args[0] === "diff" && args[1] === "--binary") return { exitCode: 0, stdout: agentPatch, stderr: "" }; - if (args[0] === "diff" && args[1] === "--name-only") return { exitCode: 0, stdout: changedFile ? changedFile + "\0" : "", stderr: "" }; - if (args[0] === "log" && args.includes("--patch")) return { exitCode: 0, stdout: historyPatch, stderr: "" }; - if (args[0] === "log" && args[1] === "--format=") return { exitCode: 0, stdout: historyFile ? historyFile + "\0" : "", stderr: "" }; - if (args[0] === "rev-list" && args[1] === "--min-parents=3") return { exitCode: 0, stdout: octopusMerge ? "c".repeat(40) + "\n" : "", stderr: "" }; - return { exitCode: 0, stdout: args[0] === "bundle" && args[1] === "unbundle" ? "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa HEAD\n" : "remote-head\n", stderr: "" }; - }); - const pushModule = require("./push_signed_commits.cjs"); - const pushSpy = vi.spyOn(pushModule, "pushSignedCommits").mockResolvedValue("candidate-head"); - try { - const module = await loadModule(); - const handler = await module.main({ max_patch_size: 1, signed_commits: false, protected_files: ["package.json"] }); - const result = await handler({ branch, diff_size: 0 }, {}); - if (expectedError) { - expect(result.success).toBe(false); - expect(result.error).toContain(expectedError); - expect(pushSpy).not.toHaveBeenCalled(); - } else { - expect(result.success).toBe(true); - expect(pushSpy).toHaveBeenCalledWith(expect.objectContaining({ baseRef: "remote-head", signedCommits: false })); - expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["diff", "--name-only", "--no-renames", "-z", `${validationTree}..HEAD`], expect.any(Object)); - } - expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["ls-remote", "--refs", "--exit-code", "origin", "refs/heads/main"], expect.objectContaining({ silent: true })); - expect(mockExec.exec).not.toHaveBeenCalledWith("git", ["fetch", "origin", trustedBase], expect.any(Object)); - if (expectedError === "bundle modifies protected files") { - const historyExclusions = mergeTreeFailure ? ["remote-head"] : ["remote-head", trustedBase]; - expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["log", "--format=", "--name-only", "--no-renames", "--diff-merges=remerge", "-z", "refs/bundles/push-feature-branch", "--not", ...historyExclusions], expect.any(Object)); - } - if (expectedError === "octopus merge") { - const historyExclusions = mergeTreeFailure ? ["remote-head"] : ["remote-head", trustedBase]; - expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["rev-list", "--min-parents=3", "refs/bundles/push-feature-branch", "--not", ...historyExclusions], expect.any(Object)); - } - if (expectedError === "Changed content size") { - expect(mockExec.exec).toHaveBeenCalledWith("git", ["reset", "--hard", "remote-head"], expect.any(Object)); + ]; + const cases = scenarios.flatMap(scenario => [false, true].map(baseAdvanced => [baseAdvanced, ...scenario])); + it.each(cases)( + "validates with advanced base %s: %s independently of merged base changes", + async (baseAdvanced, _name, agentPatch, contentSize, changedFile, expectedError, binaryConflict, historyFile = "", octopusMerge = false, mergeTreeFailure = false, historyPatch = agentPatch) => { + const branch = "merge-size-validation"; + const trustedBase = "b".repeat(40); + const trustedTip = baseAdvanced ? "d".repeat(40) : trustedBase; + const validationTree = "a".repeat(40); + createPatchFile(branch, protectedPatch + "x".repeat(4096)); + fs.writeFileSync(canonicalBundlePath(branch), "bundle content"); + const changedPath = path.join(process.cwd(), "merge-agent.txt"); + fs.writeFileSync(changedPath, Buffer.alloc(contentSize)); + const pr = (await mockGithub.rest.pulls.get()).data; + pr.base.sha = trustedBase; + pr.base.ref = "main"; + mockGithub.rest.repos.compareCommits = vi.fn().mockResolvedValue({ data: { status: "behind" } }); + mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { + if (args[0] === "ls-remote" && args.includes("--refs")) return { exitCode: 0, stdout: `${trustedTip}\trefs/heads/main\n`, stderr: "" }; + if (args[0] === "merge-base") { + if (baseAdvanced && args[2] === trustedTip) return { exitCode: 128, stdout: "", stderr: "tip object unavailable" }; + return { exitCode: args[3] === "remote-head" ? 1 : 0, stdout: "", stderr: "" }; + } + if (args[0] === "rev-list" && args[1] === "--merges") return { exitCode: 0, stdout: `${"c".repeat(40)} ${trustedBase}\n`, stderr: "" }; + if (args[0] === "rev-list" && args[1] === "--topo-order") return { exitCode: 0, stdout: `${trustedBase}\n`, stderr: "" }; + if (args[0] === "merge-tree") { + if (mergeTreeFailure) return { exitCode: 128, stdout: "", stderr: "unsupported merge-tree" }; + return { exitCode: binaryConflict ? 1 : 0, stdout: validationTree + (binaryConflict ? "\0merge-agent.txt\0\0" : "\0"), stderr: "" }; + } + if (args[0] === "diff" && args[1] === "--binary") return { exitCode: 0, stdout: agentPatch, stderr: "" }; + if (args[0] === "diff" && args[1] === "--name-only") return { exitCode: 0, stdout: changedFile ? changedFile + "\0" : "", stderr: "" }; + if (args[0] === "log" && args.includes("--patch")) return { exitCode: 0, stdout: historyPatch, stderr: "" }; + if (args[0] === "log" && args[1] === "--format=") return { exitCode: 0, stdout: historyFile ? historyFile + "\0" : "", stderr: "" }; + if (args[0] === "rev-list" && args[1] === "--min-parents=3") return { exitCode: 0, stdout: octopusMerge ? "c".repeat(40) + "\n" : "", stderr: "" }; + return { exitCode: 0, stdout: args[0] === "bundle" && args[1] === "unbundle" ? "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa HEAD\n" : "remote-head\n", stderr: "" }; + }); + const pushModule = require("./push_signed_commits.cjs"); + const pushSpy = vi.spyOn(pushModule, "pushSignedCommits").mockResolvedValue("candidate-head"); + try { + const module = await loadModule(); + const handler = await module.main({ max_patch_size: 1, signed_commits: false, protected_files: ["package.json"] }); + const result = await handler({ branch, diff_size: 0 }, {}); + if (expectedError) { + expect(result.success).toBe(false); + expect(result.error).toContain(expectedError); + expect(pushSpy).not.toHaveBeenCalled(); + } else { + expect(result.success).toBe(true); + expect(pushSpy).toHaveBeenCalledWith(expect.objectContaining({ baseRef: "remote-head", signedCommits: false })); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["diff", "--name-only", "--no-renames", "-z", `${validationTree}..HEAD`], expect.any(Object)); + } + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["ls-remote", "--refs", "--exit-code", "origin", "refs/heads/main"], expect.objectContaining({ silent: true })); + if (baseAdvanced) { + expect(mockGithub.rest.repos.compareCommits).toHaveBeenCalledWith({ owner: "test-owner", repo: "test-repo", base: trustedTip, head: trustedBase, per_page: 1 }); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["merge-tree", "--write-tree", "--name-only", "-z", "remote-head", trustedBase], expect.any(Object)); + } else { + expect(mockGithub.rest.repos.compareCommits).not.toHaveBeenCalled(); + } + expect(mockExec.exec).not.toHaveBeenCalledWith("git", ["fetch", "origin", trustedBase], expect.any(Object)); + if (expectedError === "bundle modifies protected files") { + const historyExclusions = mergeTreeFailure ? ["remote-head"] : ["remote-head", trustedBase]; + expect(mockExec.getExecOutput).toHaveBeenCalledWith( + "git", + ["log", "--format=", "--name-only", "--no-renames", "--diff-merges=remerge", "-z", "refs/bundles/push-feature-branch", "--not", ...historyExclusions], + expect.any(Object) + ); + } + if (expectedError === "octopus merge") { + const historyExclusions = mergeTreeFailure ? ["remote-head"] : ["remote-head", trustedBase]; + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["rev-list", "--min-parents=3", "refs/bundles/push-feature-branch", "--not", ...historyExclusions], expect.any(Object)); + } + if (expectedError === "Changed content size") { + expect(mockExec.exec).toHaveBeenCalledWith("git", ["reset", "--hard", "remote-head"], expect.any(Object)); + } + } finally { + pushSpy.mockRestore(); + fs.rmSync(changedPath, { force: true }); } - } finally { - pushSpy.mockRestore(); - fs.rmSync(changedPath, { force: true }); } - }); + ); it.each([128, "throw"])("retains the PR-head baseline when merge-tree fails (%s)", async failure => { mockExec.getExecOutput.mockImplementation(async (_cmd, args) => { From 713c4208f04262cf08f2a65ed9e7e9638e9ad3f7 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Sun, 18 Oct 2026 15:20:38 -0700 Subject: [PATCH 3/7] Harden merged-base validation and bound ancestor comparisons Keep protected-file and history checks relative to the PR head for non-tip ancestors while retaining their size baseline. Limit ancestor comparisons to four and stop on errors other than missing parents. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../js/bundle_transport.integration.test.cjs | 3 + .../setup/js/push_to_pull_request_branch.cjs | 53 ++++--- ...o_pull_request_branch.integration.test.cjs | 133 +++++++++++++++++- .../js/push_to_pull_request_branch.test.cjs | 10 +- ...o_pull_request_branch_comparisons.test.cjs | 101 +++++++++++++ 5 files changed, 276 insertions(+), 24 deletions(-) create mode 100644 actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs diff --git a/actions/setup/js/bundle_transport.integration.test.cjs b/actions/setup/js/bundle_transport.integration.test.cjs index c7749f0fb23..c97fa7035dc 100644 --- a/actions/setup/js/bundle_transport.integration.test.cjs +++ b/actions/setup/js/bundle_transport.integration.test.cjs @@ -143,6 +143,9 @@ describe("offline bundle transport with real Git", () => { const files = await getBundlePreApplyFiles(exec, {}, baseline.ref, "refs/bundles/test", baseline.conflictedFiles); expect(files).toEqual(["agent.txt"]); expect(checkFileProtectionPostApply(files, { protected_files: ["package.json"] }).action).toBe("allow"); + // Reachability permits the size baseline, not a protected-file exemption. + const protectionFiles = await getBundlePreApplyFiles(exec, {}, oldHead, "refs/bundles/test", baseline.conflictedFiles); + expect(checkFileProtectionPostApply(protectionFiles, { protected_files: ["package.json"] }).action).toBe("deny"); const absent = await exec.getExecOutput("git", ["cat-file", "-e", trustedTip], { ...offlineGitOptions(), ignoreReturnCode: true }); expect(absent.exitCode).not.toBe(0); expect(packInventory(target)).toEqual(before); diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 3861ac3bfad..97d962b0918 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -37,6 +37,7 @@ const { buildManualBranchApplyCommands, withTransientPushRetry } = require("./cr /** @type {string} Safe output type handled by this module */ const HANDLER_TYPE = "push_to_pull_request_branch"; +const MAX_BUNDLE_PARENT_COMPARISONS = 4; const MISSING_BRANCH_ERROR_TEMPLATE = branchName => `Branch ${branchName} no longer exists on origin (it may have been deleted), can't push to it.`; const MISSING_REMOTE_REF_PATTERNS = [ "couldn't find remote ref", @@ -202,9 +203,10 @@ async function resolveTrustedBaseTip(exec, gitOptions, baseRef) { } /** - * Recognize a newly merged base ancestor using only the pinned remote tip as - * provenance. Topological order prefers newer parents regardless of merge order - * or commit timestamps. All object inspection stays offline. + * Recognize a newly merged ancestor of the pinned remote tip for size checks. + * Reachability does not prove a historical base-branch tip: only the exact pinned + * tip can exempt protected files or history. Topological order prefers newer + * parents without relying on timestamps. All object inspection stays offline. * * @param {{ getExecOutput: Function }} exec * @param {Record} gitOptions @@ -234,15 +236,23 @@ async function resolveBundleTrustedBase(exec, gitOptions, oldHead, candidate, tr if (!parents.size) return ""; const ordered = await exec.getExecOutput("git", ["rev-list", "--topo-order", ...parents, "--not", oldHead], opts); if (ordered.exitCode !== 0) return ""; + let comparisonAttempts = 0; for (const parent of ordered.stdout.trim().split("\n")) { if (!parents.has(parent)) continue; + if (comparisonAttempts >= MAX_BUNDLE_PARENT_COMPARISONS) { + core.debug("Bundle validation: merge parent comparison budget exhausted"); + break; + } + comparisonAttempts++; try { // Comparing tip...parent reports "behind" when parent is an ancestor. const { data } = await githubClient.rest.repos.compareCommits({ ...repoParts, base: trustedTip, head: parent, per_page: 1 }); if (data.status === "behind" || data.status === "identical") return parent; - } catch { - // A parent may exist only in the bundle, or comparison may be unavailable. + } catch (error) { core.debug("Bundle validation: merge parent comparison unavailable"); + // Only a missing parent is candidate-specific; other failures stop this optional check. + if (typeof error === "object" && error !== null && "status" in error && error.status === 404) continue; + break; } } } catch { @@ -1121,6 +1131,7 @@ async function main(config = {}) { let pushedCommitSha = ""; let rangeBaseRef = branchRemoteRef; let validationBaseRef = branchRemoteRef; + let protectionBaseRef = branchRemoteRef; let validationConflictFiles = []; if (hasChanges) { // Capture HEAD before applying changes to compute new-commit count later @@ -1202,10 +1213,18 @@ async function main(config = {}) { } else { core.info("Bundle validation baseline uses PR head"); } - const agentHistoryExclusions = [rangeBaseRef]; - if (trustedBaseInValidationBase) agentHistoryExclusions.push(trustedBase); + const sizeHistoryExclusions = [rangeBaseRef]; + if (trustedBaseInValidationBase) sizeHistoryExclusions.push(trustedBase); + // An ancestor may be an intermediate side-branch state that was reverted + // before reaching the base branch. Only the pinned tip has state provenance. + const hasTrustedBaseState = trustedBaseInValidationBase && trustedBase === trustedTip; + protectionBaseRef = hasTrustedBaseState ? validationBaseRef : rangeBaseRef; + const agentHistoryExclusions = hasTrustedBaseState ? sizeHistoryExclusions : [rangeBaseRef]; + if (trustedBaseInValidationBase && !hasTrustedBaseState) { + core.info("Bundle file protection uses PR head: merged ancestor is not the pinned base tip"); + } if (deferBundleValidation) { - const agentPatchStream = await exec.getExecOutput("git", ["log", "--format=", "--patch", "--binary", "--no-ext-diff", "--no-textconv", "--diff-merges=remerge", bundleRef, "--not", ...agentHistoryExclusions], bundleGitOpts); + const agentPatchStream = await exec.getExecOutput("git", ["log", "--format=", "--patch", "--binary", "--no-ext-diff", "--no-textconv", "--diff-merges=remerge", bundleRef, "--not", ...sizeHistoryExclusions], bundleGitOpts); const validation = validatePatch(agentPatchStream.stdout); if (validation.error) return validation.error; if (validation.fallbackFiles.length) return await createProtectedFilesFallbackIssue(validation.fallbackFiles); @@ -1217,14 +1236,14 @@ async function main(config = {}) { // patch artifact under-detects files (for example, merge-resolution // content preserved only by the bundle transport). { - let bundleFiles = await getBundlePreApplyFiles(exec, bundleGitOpts, validationBaseRef, bundleRef, validationConflictFiles); + let bundleFiles = await getBundlePreApplyFiles(exec, bundleGitOpts, protectionBaseRef, bundleRef, validationConflictFiles); if (deferBundleValidation) { const octopus = await exec.getExecOutput("git", ["rev-list", "--min-parents=3", bundleRef, "--not", ...agentHistoryExclusions], bundleGitOpts); if (octopus.stdout.trim()) { return { success: false, error: "Cannot validate agent-created octopus merge commits in the bundle. Use two-parent merges instead." }; } - // Preserve protection for agent commits that change then revert a - // file, without attributing trusted base-branch commits to the agent. + // Preserve changes that were later reverted. Ancestor reachability + // alone must not exempt side-branch commits from file protection. const history = await exec.getExecOutput("git", ["log", "--format=", "--name-only", "--no-renames", "--diff-merges=remerge", "-z", bundleRef, "--not", ...agentHistoryExclusions], bundleGitOpts); bundleFiles = [...new Set([...bundleFiles, ...history.stdout.split("\0").filter(Boolean)])]; } @@ -1384,7 +1403,7 @@ async function main(config = {}) { } } } // end else (patch path) - if (!hasBundleFile) validationBaseRef = rangeBaseRef; + if (!hasBundleFile) validationBaseRef = protectionBaseRef = rangeBaseRef; core.info(`Apply transport completed; signed-push base ref: ${rangeBaseRef}`); // POST-APPLY FILE PROTECTION: Verify actual files written match policy. @@ -1396,9 +1415,10 @@ async function main(config = {}) { const diffResult = await exec.getExecOutput("git", ["diff", "--name-only", "--no-renames", "-z", `${validationBaseRef}..HEAD`], hasBundleFile ? offlineGitOptions(baseGitOpts) : baseGitOpts); const actualFiles = [...new Set([...diffResult.stdout.split("\0").filter(Boolean), ...validationConflictFiles])]; agentChangedFiles = actualFiles; - if (actualFiles.length > 0) { - core.info(`Post-apply verification: ${actualFiles.length} file(s) actually modified`); - const postApplyProtection = checkFileProtectionPostApply(actualFiles, config); + const protectionFiles = protectionBaseRef === validationBaseRef ? actualFiles : await getBundlePreApplyFiles(exec, baseGitOpts, protectionBaseRef, "HEAD", validationConflictFiles); + if (protectionFiles.length > 0) { + core.info(`Post-apply verification: ${protectionFiles.length} file(s) actually modified`); + const postApplyProtection = checkFileProtectionPostApply(protectionFiles, 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.`; @@ -1412,7 +1432,8 @@ async function main(config = {}) { await exec.exec("git", ["reset", "--hard", rangeBaseRef], baseGitOpts); return await createProtectedFilesFallbackIssue(postApplyProtection.files); } - + } + if (actualFiles.length > 0) { const changedBlobSizeBytes = getChangedBlobSizeBytes(baseGitOpts, actualFiles); const changedBlobSizeKb = Math.ceil(changedBlobSizeBytes / 1024); core.info(`Changed content size: ${changedBlobSizeKb} KB (maximum allowed: ${maxSizeKb} KB)`); diff --git a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs index 3fe34ed9d9f..71ef6a69c35 100644 --- a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs @@ -45,18 +45,22 @@ function writeRepoFile(repoDir, relativePath, content) { } function createExecApi(cwd) { - return { + const api = { async getExecOutput(command, args = [], options = {}) { if (command !== "git") { throw new Error(`unexpected command: ${command}`); } - const result = execGit(args, { cwd, allowFailure: true }); + const result = execGit(args, { cwd, env: { ...process.env, ...options.env }, allowFailure: true }); if (result.status !== 0 && !options.ignoreReturnCode) { throw new Error(result.stderr || result.stdout); } return { exitCode: result.status, stdout: result.stdout, stderr: result.stderr }; }, + async exec(command, args = [], options = {}) { + return (await api.getExecOutput(command, args, options)).exitCode; + }, }; + return api; } function fetchBaseCommit(targetRepo, sourceRepo, baseSha, branchName) { @@ -82,7 +86,7 @@ describe("push_to_pull_request_branch bundle integration", () => { [false, false, true], [true, false, true], [false, true, true], - ])("excludes trusted base changes while counting agent changes (conflict: %s, reverted change: %s, advanced base: %s)", async (conflict, revertedChange, baseAdvanced = false) => { + ])("excludes base changes from size but requires tip provenance for protection (conflict: %s, reverted change: %s, advanced base: %s)", async (conflict, revertedChange, baseAdvanced = false) => { const repo = createRepo("push-pr-validation-base-"); tempDirs.push(repo); const git = args => execGit(args, { cwd: repo }); @@ -150,9 +154,22 @@ describe("push_to_pull_request_branch bundle integration", () => { expect(Buffer.byteLength(agentPatch)).toBeLessThan(1024); expect(files.reduce((size, file) => size + fs.statSync(path.join(repo, file)).size, 0)).toBeLessThan(1024); expect(git(["rev-list", "--count", "--merges", `${oldHead}..HEAD`]).stdout.trim()).toBe("1"); - const historyFiles = git(["log", "--format=", "--name-only", "--no-renames", "--diff-merges=remerge", "-z", "HEAD", "--not", oldHead, trustedBase]).stdout.split("\0").filter(Boolean); - expect([...new Set(historyFiles)].sort()).toEqual(revertedChange ? ["agent.txt", "package.json"] : conflict ? ["agent.txt", "conflict.txt"] : ["agent.txt"]); - expect(checkFileProtectionPostApply([...files, ...historyFiles], { protected_files: ["package.json"] }).action).toBe(revertedChange ? "deny" : "allow"); + const historyExclusions = baseAdvanced ? [oldHead] : [oldHead, trustedBase]; + const historyFiles = git(["log", "--format=", "--name-only", "--no-renames", "--diff-merges=remerge", "-z", "HEAD", "--not", ...historyExclusions]) + .stdout.split("\0") + .filter(Boolean); + expect([...new Set(historyFiles)].sort()).toEqual( + baseAdvanced + ? conflict + ? ["agent.txt", "conflict.txt", "package.json", "upstream.txt"] + : ["agent.txt", "package.json", "upstream.txt"] + : revertedChange + ? ["agent.txt", "package.json"] + : conflict + ? ["agent.txt", "conflict.txt"] + : ["agent.txt"] + ); + expect(checkFileProtectionPostApply([...files, ...historyFiles], { protected_files: ["package.json"] }).action).toBe(revertedChange || baseAdvanced ? "deny" : "allow"); }); it.each([false, true])("detects protected changes made and restored only in agent merge commits (octopus: %s)", async octopus => { @@ -214,6 +231,110 @@ describe("push_to_pull_request_branch bundle integration", () => { expect(checkFileProtectionPostApply([...files, ...historyFiles], { protected_files: ["package.json"] }).action).toBe("deny"); }); + it.each([ + [false, false], + [true, false], + [false, true], + ])("rejects unsigned bundles importing a non-tip side ancestor (side reverted: %s, agent reverted: %s)", async (sideReverted, agentReverted) => { + const sourceRepo = createRepo("push-pr-side-ancestor-source-"); + const targetRepo = createRepo("push-pr-side-ancestor-target-"); + tempDirs.push(sourceRepo, targetRepo); + const git = args => execGit(args, { cwd: sourceRepo }); + writeRepoFile(sourceRepo, "package.json", "{}\n"); + git(["add", "."]); + git(["commit", "-m", "common base"]); + git(["branch", "-M", "main"]); + git(["checkout", "-b", "feature"]); + writeRepoFile(sourceRepo, "existing.txt", "existing PR\n"); + git(["add", "."]); + git(["commit", "-m", "existing PR"]); + const oldHead = git(["rev-parse", "HEAD"]).stdout.trim(); + git(["checkout", "-b", "side", "main"]); + writeRepoFile(sourceRepo, "package.json", '{"scripts":{"review-regression":"echo regression"}}\n'); + git(["commit", "-am", "A adds protected script"]); + const added = git(["rev-parse", "HEAD"]).stdout.trim(); + writeRepoFile(sourceRepo, "package.json", "{}\n"); + git(["commit", "-am", "B removes protected script"]); + const removed = git(["rev-parse", "HEAD"]).stdout.trim(); + git(["checkout", "main"]); + git(["merge", "--no-ff", "side", "-m", "merge side after script removal"]); + const trustedTip = git(["rev-parse", "HEAD"]).stdout.trim(); + expect(git(["show", "main:package.json"]).stdout).toBe("{}\n"); + expect(git(["rev-list", "--first-parent", "main"]).stdout).not.toContain(added); + const mergedAncestor = sideReverted ? removed : added; + git(["checkout", "-b", "candidate", "feature"]); + git(["merge", "--no-ff", mergedAncestor, "-m", "merge intermediate side state"]); + if (agentReverted) { + writeRepoFile(sourceRepo, "package.json", "{}\n"); + git(["commit", "-am", "agent restores protected file"]); + } + writeRepoFile(sourceRepo, "agent.txt", "small agent change\n"); + git(["add", "."]); + git(["commit", "-m", "agent change"]); + const candidate = git(["rev-parse", "HEAD"]).stdout.trim(); + const bundlePath = path.join(sourceRepo, ".git", "candidate.bundle"); + const patchPath = path.join(sourceRepo, ".git", "candidate.patch"); + git(["bundle", "create", bundlePath, "HEAD", `^${oldHead}`]); + fs.writeFileSync(patchPath, git(["format-patch", "--stdout", `${oldHead}..HEAD`]).stdout); + execGit(["fetch", sourceRepo, "main:refs/remotes/origin/main", "feature:refs/remotes/origin/feature"], { cwd: targetRepo }); + execGit(["remote", "add", "origin", sourceRepo], { cwd: targetRepo }); + const exec = createExecApi(targetRepo); + exec.getExecOutput = vi.fn(exec.getExecOutput); + const originalEnv = { ...process.env }; + const originalGlobals = { exec: global.exec, github: global.github, context: global.context }; + const paths = require("./resolve_transport_paths.cjs"); + const transportPaths = vi.spyOn(paths, "resolveTransportPaths").mockReturnValue({ patchPath, bundlePath }); + const push = vi.spyOn(require("./push_signed_commits.cjs"), "pushSignedCommits").mockRejectedValue(new Error("Unexpected push")); + try { + process.env.GITHUB_REPOSITORY = "test-owner/test-repo"; + process.env.GITHUB_WORKSPACE = targetRepo; + global.exec = exec; + global.context = { repo: { owner: "test-owner", repo: "test-repo" }, payload: { pull_request: { number: 123 } } }; + const compareCommits = vi.fn(async ({ base, head }) => { + expect(base).toBe(trustedTip); + const result = execGit(["merge-base", "--is-ancestor", head, base], { cwd: sourceRepo, allowFailure: true }); + return { data: { status: result.status === 0 ? "behind" : "diverged" } }; + }); + global.github = { + rest: { + pulls: { + get: vi.fn().mockResolvedValue({ + data: { + head: { ref: "feature", sha: oldHead, repo: { full_name: "test-owner/test-repo", fork: false } }, + base: { ref: "main", repo: { full_name: "test-owner/test-repo" } }, + labels: [], + }, + }), + }, + repos: { get: vi.fn().mockResolvedValue({ data: { default_branch: "main" } }), compareCommits }, + }, + }; + delete require.cache[require.resolve("./push_to_pull_request_branch.cjs")]; + const handler = await require("./push_to_pull_request_branch.cjs").main({ signed_commits: false, check_branch_protection: false, protected_files: ["package.json"] }); + const result = await handler({ branch: "feature" }, {}); + expect(result).toMatchObject({ success: false, skipped: true }); + expect(result.error).toContain(`${agentReverted ? "patch" : "bundle"} modifies protected files (package.json)`); + expect(compareCommits).toHaveBeenCalledWith({ owner: "test-owner", repo: "test-repo", base: trustedTip, head: mergedAncestor, per_page: 1 }); + if (!agentReverted) { + expect(exec.getExecOutput).toHaveBeenCalledWith("git", ["log", "--format=", "--name-only", "--no-renames", "--diff-merges=remerge", "-z", "refs/bundles/push-feature", "--not", oldHead], expect.any(Object)); + } + expect(execGit(["rev-parse", "feature"], { cwd: targetRepo }).stdout.trim()).toBe(oldHead); + expect(execGit(["rev-parse", "origin/feature"], { cwd: targetRepo }).stdout.trim()).toBe(oldHead); + expect(execGit(["show", `${candidate}:package.json`], { cwd: targetRepo }).stdout).toBe(sideReverted || agentReverted ? "{}\n" : '{"scripts":{"review-regression":"echo regression"}}\n'); + expect(exec.getExecOutput.mock.calls.filter(([, args]) => args[0] === "fetch").map(([, args]) => args)).toEqual([["fetch", "origin", "feature:refs/remotes/origin/feature"]]); + expect(push).not.toHaveBeenCalled(); + } finally { + transportPaths.mockRestore(); + push.mockRestore(); + Object.assign(global, originalGlobals); + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) delete process.env[key]; + } + Object.assign(process.env, originalEnv); + delete require.cache[require.resolve("./push_to_pull_request_branch.cjs")]; + } + }); + it.each(["text", "binary", "modify/delete"])("keeps protected %s conflict resolutions in the validation file set", async kind => { const repo = createRepo("push-pr-protected-resolution-"); tempDirs.push(repo); 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 f2022395f46..329513812c9 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -2431,6 +2431,9 @@ index 0000000..abc1234 expect(result.success).toBe(true); expect(pushSpy).toHaveBeenCalledWith(expect.objectContaining({ baseRef: "remote-head", signedCommits: false })); expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["diff", "--name-only", "--no-renames", "-z", `${validationTree}..HEAD`], expect.any(Object)); + if (baseAdvanced) { + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["diff", "--name-only", "--no-renames", "-z", "remote-head..HEAD"], expect.any(Object)); + } } expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["ls-remote", "--refs", "--exit-code", "origin", "refs/heads/main"], expect.objectContaining({ silent: true })); if (baseAdvanced) { @@ -2440,8 +2443,11 @@ index 0000000..abc1234 expect(mockGithub.rest.repos.compareCommits).not.toHaveBeenCalled(); } expect(mockExec.exec).not.toHaveBeenCalledWith("git", ["fetch", "origin", trustedBase], expect.any(Object)); + if (!mergeTreeFailure && expectedError !== "protected files" && expectedError !== "Patch size") { + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["diff", "--name-only", "--no-renames", "-z", `${baseAdvanced ? "remote-head" : validationTree}..refs/bundles/push-feature-branch`], expect.any(Object)); + } if (expectedError === "bundle modifies protected files") { - const historyExclusions = mergeTreeFailure ? ["remote-head"] : ["remote-head", trustedBase]; + const historyExclusions = mergeTreeFailure || baseAdvanced ? ["remote-head"] : ["remote-head", trustedBase]; expect(mockExec.getExecOutput).toHaveBeenCalledWith( "git", ["log", "--format=", "--name-only", "--no-renames", "--diff-merges=remerge", "-z", "refs/bundles/push-feature-branch", "--not", ...historyExclusions], @@ -2449,7 +2455,7 @@ index 0000000..abc1234 ); } if (expectedError === "octopus merge") { - const historyExclusions = mergeTreeFailure ? ["remote-head"] : ["remote-head", trustedBase]; + const historyExclusions = mergeTreeFailure || baseAdvanced ? ["remote-head"] : ["remote-head", trustedBase]; expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["rev-list", "--min-parents=3", "refs/bundles/push-feature-branch", "--not", ...historyExclusions], expect.any(Object)); } if (expectedError === "Changed content size") { diff --git a/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs b/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs new file mode 100644 index 00000000000..72a8305a55f --- /dev/null +++ b/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs @@ -0,0 +1,101 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; + +describe("bundle merge-parent comparison budget", () => { + const comparisonBudget = 4; + const trustedTip = "a".repeat(40); + const oldHead = "b".repeat(40); + const candidate = "c".repeat(40); + const repoParts = { owner: "test-owner", repo: "test-repo" }; + const secretDiagnostic = "synthetic secret-bearing API diagnostic"; + const parents = Array.from({ length: 24 }, (_, index) => (index + 1).toString(16).padStart(40, "0")); + let mockCore; + let mockExec; + let compareCommits; + let resolveBundleTrustedBase; + + beforeEach(() => { + mockCore = { debug: vi.fn(), warning: vi.fn() }; + vi.stubGlobal("core", mockCore); + mockExec = { + exec: vi.fn(), + getExecOutput: vi.fn(async (_command, args) => { + if (args[0] === "merge-base") return { exitCode: 128, stdout: "", stderr: secretDiagnostic }; + if (args[1] === "--merges") { + const merges = parents.map((parent, index) => `${(index + 100).toString(16).padStart(40, "0")} ${oldHead} ${parent}`); + return { exitCode: 0, stdout: `${merges.join("\n")}\n`, stderr: "" }; + } + if (args[1] === "--topo-order") return { exitCode: 0, stdout: `${parents.join("\n")}\n`, stderr: "" }; + throw new Error("Unexpected git operation"); + }), + }; + compareCommits = vi.fn(); + ({ resolveBundleTrustedBase } = require("./push_to_pull_request_branch.cjs")); + }); + + afterEach(() => { + expect(mockExec.exec).not.toHaveBeenCalled(); + for (const [command, args, options] of mockExec.getExecOutput.mock.calls) { + expect(command).toBe("git"); + expect(["merge-base", "rev-list"]).toContain(args[0]); + expect(options).toEqual( + expect.objectContaining({ + cwd: process.cwd(), + silent: true, + ignoreReturnCode: true, + env: expect.objectContaining({ GIT_ALLOW_PROTOCOL: "", GIT_NO_LAZY_FETCH: "1", GIT_TERMINAL_PROMPT: "0" }), + }) + ); + } + expect(JSON.stringify([mockCore.debug.mock.calls, mockCore.warning.mock.calls])).not.toContain(secretDiagnostic); + vi.unstubAllGlobals(); + }); + + async function resolve() { + return resolveBundleTrustedBase(mockExec, { cwd: process.cwd(), env: { GIT_ALLOW_PROTOCOL: "https", GIT_NO_LAZY_FETCH: "0" } }, oldHead, candidate, trustedTip, { rest: { repos: { compareCommits } } }, repoParts); + } + + it.each(["missing", "ahead", "diverged", "unknown"])("caps 24 attacker-controlled parents at four comparisons after %s results", async status => { + compareCommits.mockImplementation(async () => { + if (compareCommits.mock.calls.length > comparisonBudget) return { data: { status: "behind" } }; + if (status === "missing") throw Object.assign(new Error(secretDiagnostic), { status: 404 }); + return { data: { status } }; + }); + + expect(await resolve()).toBe(""); + expect(compareCommits).toHaveBeenCalledTimes(comparisonBudget); + expect(compareCommits.mock.calls).toEqual(parents.slice(0, comparisonBudget).map(head => [{ ...repoParts, base: trustedTip, head, per_page: 1 }])); + expect(mockCore.debug).toHaveBeenCalledWith("Bundle validation: merge parent comparison budget exhausted"); + }); + + it.each([401, 403, 429, 500, 502, 503, 504])("stops immediately on HTTP %s rather than querying another parent", async status => { + compareCommits.mockRejectedValueOnce(Object.assign(new Error(secretDiagnostic), { status })).mockResolvedValue({ data: { status: "behind" } }); + + expect(await resolve()).toBe(""); + expect(compareCommits).toHaveBeenCalledTimes(1); + }); + + it.each([ + ["unknown error", new Error(secretDiagnostic)], + ["transport error", Object.assign(new Error(secretDiagnostic), { code: "ECONNRESET" })], + ["string error", secretDiagnostic], + ["null error", null], + ["undefined error", undefined], + ["unrecognized status", Object.assign(new Error(secretDiagnostic), { status: "404" })], + ])("stops immediately on %s", async (_label, error) => { + compareCommits.mockRejectedValueOnce(error).mockResolvedValue({ data: { status: "behind" } }); + + expect(await resolve()).toBe(""); + expect(compareCommits).toHaveBeenCalledTimes(1); + }); + + it.each(["behind", "identical"])("accepts a confirmed %s ancestor after missing parents within the budget", async status => { + compareCommits.mockImplementation(async () => { + if (compareCommits.mock.calls.length < comparisonBudget) throw Object.assign(new Error(secretDiagnostic), { status: 404 }); + return { data: { status } }; + }); + + expect(await resolve()).toBe(parents[comparisonBudget - 1]); + expect(compareCommits).toHaveBeenCalledTimes(comparisonBudget); + expect(compareCommits).toHaveBeenLastCalledWith({ ...repoParts, base: trustedTip, head: parents[comparisonBudget - 1], per_page: 1 }); + }); +}); From 5439cf3f0b40b931c0f348a55c9b689be65a710f Mon Sep 17 00:00:00 2001 From: pelikhan Date: Sun, 18 Oct 2026 16:09:28 -0700 Subject: [PATCH 4/7] Bound optional bundle baseline discovery without expanding shallow checkouts Cap Git execution time, output, and history scans; abort REST comparisons after five seconds without retries. Keep optional discovery offline and cover unchanged shallow/partial checkout state plus explicit history recovery gating. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../js/bundle_transport.integration.test.cjs | 46 ++++++- .../setup/js/push_to_pull_request_branch.cjs | 73 ++++++++--- ...o_pull_request_branch.integration.test.cjs | 10 +- .../js/push_to_pull_request_branch.test.cjs | 15 ++- ...o_pull_request_branch_comparisons.test.cjs | 114 +++++++++++++++++- .../reference/safe-outputs-pull-requests.md | 6 +- 6 files changed, 238 insertions(+), 26 deletions(-) diff --git a/actions/setup/js/bundle_transport.integration.test.cjs b/actions/setup/js/bundle_transport.integration.test.cjs index c97fa7035dc..ba752775f87 100644 --- a/actions/setup/js/bundle_transport.integration.test.cjs +++ b/actions/setup/js/bundle_transport.integration.test.cjs @@ -6,7 +6,7 @@ import os from "os"; import path from "path"; const { ensureFullHistoryForBundle, importLocalBundle, offlineGitOptions } = require("./git_helpers.cjs"); -const { resolveTrustedBaseTip, resolveBundleTrustedBase, getBundleValidationBase, getBundlePreApplyFiles } = require("./push_to_pull_request_branch.cjs"); +const { resolveTrustedBaseTip, resolveBundleTrustedBase, getBundleValidationBase, getBundlePreApplyFiles, boundedValidationExec } = require("./push_to_pull_request_branch.cjs"); const { checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs"); const gitEnv = () => { @@ -188,6 +188,50 @@ describe("offline bundle transport with real Git", () => { expect(fs.readFileSync(path.join(shallow, ".git/shallow"), "utf8")).toBe(shallowFile); }); + it.each([false, true])("keeps advanced-base discovery offline in a shallow checkout (filter support: %s)", async allowFilter => { + const oldHead = git(source, ["rev-parse", "HEAD"]); + git(source, ["checkout", "-b", "feature"]); + git(source, ["config", "uploadpack.allowFilter", String(allowFilter)]); + const shallow = path.join(root, "shallow"); + git(root, ["clone", "--depth=1", "--filter=blob:none", "--no-checkout", "--branch", "feature", `file://${source}`, shallow]); + const shallowPath = path.join(shallow, ".git/shallow"); + const shallowFile = fs.readFileSync(shallowPath, "utf8"); + git(source, ["checkout", "main"]); + const mergedBase = addCommit("upstream.txt", "upstream\n"); + git(source, ["checkout", "feature"]); + git(source, ["merge", "--no-ff", "main", "-m", "merge provisioned base"]); + addCommit("agent.txt", "agent\n"); + const bundle = path.join(root, "shallow-agent.bundle"); + git(source, ["bundle", "create", bundle, "feature", `^${oldHead}`]); + git(source, ["checkout", "main"]); + const trustedTip = addCommit("later.txt", "advanced base tip\n"); + + const exec = execApi(shallow); + await ensureFullHistoryForBundle(exec, {}, { bundleFilePath: bundle }); + await importLocalBundle(exec, bundle, "refs/heads/feature", "refs/bundles/test"); + const before = packInventory(shallow); + const compareCommits = vi.fn(async () => ({ data: { status: "behind" } })); + const options = { cwd: shallow, env: gitEnv() }; + expect(await resolveTrustedBaseTip(boundedValidationExec, options, "main")).toBe(trustedTip); + const recognized = await resolveBundleTrustedBase(boundedValidationExec, options, oldHead, "refs/bundles/test", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }); + expect(recognized).toBe(mergedBase); + await getBundleValidationBase(boundedValidationExec, options, oldHead, "refs/bundles/test", recognized); + + expect(compareCommits).toHaveBeenCalledTimes(1); + expect(exec.calls.some(args => args[0] === "fetch")).toBe(false); + expect(git(shallow, ["rev-parse", "--is-shallow-repository"])).toBe("true"); + expect(fs.readFileSync(shallowPath, "utf8")).toBe(shallowFile); + expect(packInventory(shallow)).toEqual(before); + expect(fs.readdirSync(shallow)).toEqual([".git"]); + const absent = await boundedValidationExec.getExecOutput("git", ["cat-file", "-e", trustedTip], offlineGitOptions(options)); + expect(absent.exitCode).not.toBe(0); + if (allowFilter) { + const blob = git(source, ["rev-parse", `${oldHead}:shared.txt`]); + expect((await boundedValidationExec.getExecOutput("git", ["cat-file", "-e", blob], offlineGitOptions(options))).exitCode).not.toBe(0); + } + expect(packInventory(shallow)).toEqual(before); + }); + it("imports an incremental bundle into a shallow checkout with locally provisioned prerequisites", async () => { const prerequisite = git(source, ["rev-parse", "HEAD"]); const shallow = path.join(root, "shallow"); diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 97d962b0918..56dfd36cb98 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -7,6 +7,7 @@ const { createReportIssue } = require("./issue_report.cjs"); const fs = require("fs"); const { withTransportIntegrity } = require("./transport_integrity.cjs"); const path = require("path"); +const childProcess = require("child_process"); const { generateStagedPreview } = require("./staged_preview.cjs"); const { isStagedMode } = require("./safe_output_helpers.cjs"); const { pushSignedCommits } = require("./push_signed_commits.cjs"); @@ -19,7 +20,7 @@ const { detectForkPR, checkBranchPushable } = require("./pr_helpers.cjs"); const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { checkFileProtection, checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs"); -const { POLICY_FILE_PROTECTION_DENIED_REASON_CODE } = require("./error_codes.cjs"); +const { ERR_SYSTEM, POLICY_FILE_PROTECTION_DENIED_REASON_CODE } = require("./error_codes.cjs"); const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); const { renderTemplateFromFile, buildProtectedFileList, getPromptPath } = require("./messages_core.cjs"); const { withGitHubHostToken } = require("./git_auth_helpers.cjs"); @@ -38,6 +39,36 @@ const { buildManualBranchApplyCommands, withTransientPushRetry } = require("./cr /** @type {string} Safe output type handled by this module */ const HANDLER_TYPE = "push_to_pull_request_branch"; const MAX_BUNDLE_PARENT_COMPARISONS = 4; +const MAX_BUNDLE_HISTORY_COMMITS = 100; +const BUNDLE_VALIDATION_TIMEOUT_MS = 5000; +const BUNDLE_VALIDATION_MAX_OUTPUT = 1024 * 1024; + +// @actions/exec has no process timeout or output limit. Keep optional baseline +// discovery separate from bundle recovery, and discard failed command diagnostics. +const boundedValidationExec = { + /** + * @param {string} command + * @param {string[]} args + * @param {import("@actions/exec").ExecOptions} [options] + */ + async getExecOutput(command, args, options = {}) { + if (command !== "git") throw new Error(`${ERR_SYSTEM}: Bundle validation only supports git commands`); + const result = childProcess.spawnSync(command, args, { + cwd: options.cwd, + env: options.env, + encoding: "utf8", + timeout: BUNDLE_VALIDATION_TIMEOUT_MS, + maxBuffer: BUNDLE_VALIDATION_MAX_OUTPUT, + killSignal: "SIGKILL", + stdio: "pipe", + }); + if (result.error || result.signal || result.status === null) { + core.debug("Bundle validation: bounded Git command failed or exceeded its execution/output limit"); + return { exitCode: 128, stdout: "", stderr: "" }; + } + return { exitCode: result.status, stdout: result.stdout, stderr: "" }; + }, +}; const MISSING_BRANCH_ERROR_TEMPLATE = branchName => `Branch ${branchName} no longer exists on origin (it may have been deleted), can't push to it.`; const MISSING_REMOTE_REF_PATTERNS = [ "couldn't find remote ref", @@ -224,20 +255,24 @@ async function resolveBundleTrustedBase(exec, gitOptions, oldHead, candidate, tr const tipAncestor = await exec.getExecOutput("git", ["merge-base", "--is-ancestor", trustedTip, candidate], opts); if (tipAncestor.exitCode === 0) return trustedTip; - const merges = await exec.getExecOutput("git", ["rev-list", "--merges", "--parents", `${oldHead}..${candidate}`], opts); + const merges = await exec.getExecOutput("git", ["rev-list", "--merges", "--parents", `--max-count=${MAX_BUNDLE_HISTORY_COMMITS + 1}`, `${oldHead}..${candidate}`], opts); if (merges.exitCode !== 0) return ""; - const parents = new Set( - merges.stdout - .trim() - .split("\n") - .flatMap(line => line.split(" ").slice(1)) - .filter(sha => /^[0-9a-f]{40}$/.test(sha)) - ); + const mergeLines = merges.stdout.trim().split("\n").filter(Boolean); + if (mergeLines.length > MAX_BUNDLE_HISTORY_COMMITS || mergeLines.some(line => line.split(" ").length > 3)) { + core.debug("Bundle validation: merge history exceeds the discovery budget"); + return ""; + } + const parents = new Set(mergeLines.flatMap(line => line.split(" ").slice(1)).filter(sha => /^[0-9a-f]{40}$/.test(sha))); if (!parents.size) return ""; - const ordered = await exec.getExecOutput("git", ["rev-list", "--topo-order", ...parents, "--not", oldHead], opts); + const ordered = await exec.getExecOutput("git", ["rev-list", "--topo-order", `--max-count=${MAX_BUNDLE_HISTORY_COMMITS + 1}`, ...parents, "--not", oldHead], opts); if (ordered.exitCode !== 0) return ""; + const orderedParents = ordered.stdout.trim().split("\n").filter(Boolean); + if (orderedParents.length > MAX_BUNDLE_HISTORY_COMMITS) { + core.debug("Bundle validation: parent history exceeds the discovery budget"); + return ""; + } let comparisonAttempts = 0; - for (const parent of ordered.stdout.trim().split("\n")) { + for (const parent of orderedParents) { if (!parents.has(parent)) continue; if (comparisonAttempts >= MAX_BUNDLE_PARENT_COMPARISONS) { core.debug("Bundle validation: merge parent comparison budget exhausted"); @@ -246,7 +281,13 @@ async function resolveBundleTrustedBase(exec, gitOptions, oldHead, candidate, tr comparisonAttempts++; try { // Comparing tip...parent reports "behind" when parent is an ancestor. - const { data } = await githubClient.rest.repos.compareCommits({ ...repoParts, base: trustedTip, head: parent, per_page: 1 }); + const { data } = await githubClient.rest.repos.compareCommits({ + ...repoParts, + base: trustedTip, + head: parent, + per_page: 1, + request: { timeout: BUNDLE_VALIDATION_TIMEOUT_MS, retries: 0, signal: AbortSignal.timeout(BUNDLE_VALIDATION_TIMEOUT_MS) }, + }); if (data.status === "behind" || data.status === "identical") return parent; } catch (error) { core.debug("Bundle validation: merge parent comparison unavailable"); @@ -1185,7 +1226,7 @@ async function main(config = {}) { core.info(`Applying changes from bundle: ${bundleFilePath}`); const bundleRef = `refs/bundles/push-${branchName.replace(/[^a-zA-Z0-9-]/g, "-")}`; try { - const trustedTip = signedCommits ? "" : await resolveTrustedBaseTip(exec, baseGitOpts, pullRequest.base?.ref); + const trustedTip = signedCommits ? "" : await resolveTrustedBaseTip(boundedValidationExec, baseGitOpts, pullRequest.base?.ref); const bundleGitOpts = offlineGitOptions(baseGitOpts); await ensureFullHistoryForBundle( exec, @@ -1203,8 +1244,8 @@ async function main(config = {}) { await importLocalBundle(exec, bundleFilePath, `refs/heads/${branchName}`, bundleRef, baseGitOpts); core.info(`Imported local bundle to ${bundleRef}`); - const trustedBase = await resolveBundleTrustedBase(exec, baseGitOpts, rangeBaseRef, bundleRef, trustedTip, githubClient, repoParts); - const validationBase = await getBundleValidationBase(exec, baseGitOpts, rangeBaseRef, bundleRef, trustedBase); + const trustedBase = await resolveBundleTrustedBase(boundedValidationExec, baseGitOpts, rangeBaseRef, bundleRef, trustedTip, githubClient, repoParts); + const validationBase = await getBundleValidationBase(boundedValidationExec, baseGitOpts, rangeBaseRef, bundleRef, trustedBase); validationBaseRef = validationBase.ref; validationConflictFiles = validationBase.conflictedFiles; const trustedBaseInValidationBase = validationBaseRef !== rangeBaseRef; @@ -1895,4 +1936,4 @@ async function main(config = {}) { }); } -module.exports = { main, HANDLER_TYPE, getBundlePreApplyFiles, getBundleValidationBase, resolveTrustedBaseTip, resolveBundleTrustedBase }; +module.exports = { main, HANDLER_TYPE, getBundlePreApplyFiles, getBundleValidationBase, resolveTrustedBaseTip, resolveBundleTrustedBase, boundedValidationExec }; diff --git a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs index 71ef6a69c35..f456f7d27f5 100644 --- a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs @@ -285,6 +285,7 @@ describe("push_to_pull_request_branch bundle integration", () => { const paths = require("./resolve_transport_paths.cjs"); const transportPaths = vi.spyOn(paths, "resolveTransportPaths").mockReturnValue({ patchPath, bundlePath }); const push = vi.spyOn(require("./push_signed_commits.cjs"), "pushSignedCommits").mockRejectedValue(new Error("Unexpected push")); + let boundedOutputSpy; try { process.env.GITHUB_REPOSITORY = "test-owner/test-repo"; process.env.GITHUB_WORKSPACE = targetRepo; @@ -310,11 +311,15 @@ describe("push_to_pull_request_branch bundle integration", () => { }, }; delete require.cache[require.resolve("./push_to_pull_request_branch.cjs")]; - const handler = await require("./push_to_pull_request_branch.cjs").main({ signed_commits: false, check_branch_protection: false, protected_files: ["package.json"] }); + const module = require("./push_to_pull_request_branch.cjs"); + const boundedOutput = module.boundedValidationExec.getExecOutput; + // Match the fixture's exec API, which defaults all Git calls to targetRepo. + boundedOutputSpy = vi.spyOn(module.boundedValidationExec, "getExecOutput").mockImplementation((command, args, options) => boundedOutput(command, args, { ...options, cwd: targetRepo })); + const handler = await module.main({ signed_commits: false, check_branch_protection: false, protected_files: ["package.json"] }); const result = await handler({ branch: "feature" }, {}); expect(result).toMatchObject({ success: false, skipped: true }); expect(result.error).toContain(`${agentReverted ? "patch" : "bundle"} modifies protected files (package.json)`); - expect(compareCommits).toHaveBeenCalledWith({ owner: "test-owner", repo: "test-repo", base: trustedTip, head: mergedAncestor, per_page: 1 }); + expect(compareCommits).toHaveBeenCalledWith({ owner: "test-owner", repo: "test-repo", base: trustedTip, head: mergedAncestor, per_page: 1, request: { timeout: 5000, retries: 0, signal: expect.any(AbortSignal) } }); if (!agentReverted) { expect(exec.getExecOutput).toHaveBeenCalledWith("git", ["log", "--format=", "--name-only", "--no-renames", "--diff-merges=remerge", "-z", "refs/bundles/push-feature", "--not", oldHead], expect.any(Object)); } @@ -326,6 +331,7 @@ describe("push_to_pull_request_branch bundle integration", () => { } finally { transportPaths.mockRestore(); push.mockRestore(); + boundedOutputSpy?.mockRestore(); Object.assign(global, originalGlobals); for (const key of Object.keys(process.env)) { if (!(key in originalEnv)) delete process.env[key]; 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 329513812c9..c2c0c16519c 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -248,6 +248,7 @@ describe("push_to_pull_request_branch.cjs", () => { delete global.exec; delete global.context; delete global.github; + vi.restoreAllMocks(); vi.clearAllMocks(); }); @@ -257,6 +258,7 @@ describe("push_to_pull_request_branch.cjs", () => { async function loadModule() { // Module loads when imported const module = require("./push_to_pull_request_branch.cjs"); + vi.spyOn(module.boundedValidationExec, "getExecOutput").mockImplementation((command, args, options) => mockExec.getExecOutput(command, args, options)); return module; } @@ -364,8 +366,8 @@ index 0000000..abc1234 const confirmed = status === "behind" || status === "identical"; const base = await module.resolveBundleTrustedBase(mockExec, { cwd: tempDir }, "old-head", "candidate", trustedTip, mockGithub, mockContext.repo); expect(base).toBe(confirmed ? mergedBase : ""); - expect(mockGithub.rest.repos.compareCommits).toHaveBeenCalledExactlyOnceWith({ ...mockContext.repo, base: trustedTip, head: mergedBase, per_page: 1 }); - expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["rev-list", "--topo-order", mergedBase, "--not", "old-head"], expect.any(Object)); + expect(mockGithub.rest.repos.compareCommits).toHaveBeenCalledExactlyOnceWith({ ...mockContext.repo, base: trustedTip, head: mergedBase, per_page: 1, request: { timeout: 5000, retries: 0, signal: expect.any(AbortSignal) } }); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["rev-list", "--topo-order", "--max-count=101", mergedBase, "--not", "old-head"], expect.any(Object)); for (const [, , options] of mockExec.getExecOutput.mock.calls) { expect(options).toEqual(expect.objectContaining({ cwd: tempDir, silent: true, env: expect.objectContaining({ GIT_ALLOW_PROTOCOL: "", GIT_NO_LAZY_FETCH: "1" }) })); } @@ -2437,7 +2439,14 @@ index 0000000..abc1234 } expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["ls-remote", "--refs", "--exit-code", "origin", "refs/heads/main"], expect.objectContaining({ silent: true })); if (baseAdvanced) { - expect(mockGithub.rest.repos.compareCommits).toHaveBeenCalledWith({ owner: "test-owner", repo: "test-repo", base: trustedTip, head: trustedBase, per_page: 1 }); + expect(mockGithub.rest.repos.compareCommits).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + base: trustedTip, + head: trustedBase, + per_page: 1, + request: { timeout: 5000, retries: 0, signal: expect.any(AbortSignal) }, + }); expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["merge-tree", "--write-tree", "--name-only", "-z", "remote-head", trustedBase], expect.any(Object)); } else { expect(mockGithub.rest.repos.compareCommits).not.toHaveBeenCalled(); diff --git a/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs b/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs index 72a8305a55f..e3a746f66b6 100644 --- a/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs @@ -7,6 +7,7 @@ describe("bundle merge-parent comparison budget", () => { const candidate = "c".repeat(40); const repoParts = { owner: "test-owner", repo: "test-repo" }; const secretDiagnostic = "synthetic secret-bearing API diagnostic"; + const request = { timeout: 5000, retries: 0, signal: expect.any(AbortSignal) }; const parents = Array.from({ length: 24 }, (_, index) => (index + 1).toString(16).padStart(40, "0")); let mockCore; let mockExec; @@ -63,7 +64,7 @@ describe("bundle merge-parent comparison budget", () => { expect(await resolve()).toBe(""); expect(compareCommits).toHaveBeenCalledTimes(comparisonBudget); - expect(compareCommits.mock.calls).toEqual(parents.slice(0, comparisonBudget).map(head => [{ ...repoParts, base: trustedTip, head, per_page: 1 }])); + expect(compareCommits.mock.calls).toEqual(parents.slice(0, comparisonBudget).map(head => [{ ...repoParts, base: trustedTip, head, per_page: 1, request }])); expect(mockCore.debug).toHaveBeenCalledWith("Bundle validation: merge parent comparison budget exhausted"); }); @@ -88,6 +89,18 @@ describe("bundle merge-parent comparison budget", () => { expect(compareCommits).toHaveBeenCalledTimes(1); }); + it("aborts a stalled comparison after five seconds and stops discovery", async () => { + compareCommits.mockImplementation( + ({ request }) => + new Promise((_resolve, reject) => { + request.signal.addEventListener("abort", () => reject(request.signal.reason), { once: true }); + }) + ); + expect(await resolve()).toBe(""); + expect(compareCommits).toHaveBeenCalledTimes(1); + expect(compareCommits.mock.calls[0][0].request.signal.aborted).toBe(true); + }); + it.each(["behind", "identical"])("accepts a confirmed %s ancestor after missing parents within the budget", async status => { compareCommits.mockImplementation(async () => { if (compareCommits.mock.calls.length < comparisonBudget) throw Object.assign(new Error(secretDiagnostic), { status: 404 }); @@ -96,6 +109,103 @@ describe("bundle merge-parent comparison budget", () => { expect(await resolve()).toBe(parents[comparisonBudget - 1]); expect(compareCommits).toHaveBeenCalledTimes(comparisonBudget); - expect(compareCommits).toHaveBeenLastCalledWith({ ...repoParts, base: trustedTip, head: parents[comparisonBudget - 1], per_page: 1 }); + expect(compareCommits).toHaveBeenLastCalledWith({ ...repoParts, base: trustedTip, head: parents[comparisonBudget - 1], per_page: 1, request }); + }); + + it.each(["merge history", "parent history", "octopus merge"])("falls back without API comparisons when %s exceeds the discovery budget", async scenario => { + const original = mockExec.getExecOutput.getMockImplementation(); + mockExec.getExecOutput.mockImplementation(async (command, args, options) => { + if ((scenario === "merge history" && args[1] === "--merges") || (scenario === "parent history" && args[1] === "--topo-order")) { + return { exitCode: 0, stdout: Array.from({ length: 101 }, () => `${candidate} ${oldHead} ${parents[0]}`).join("\n"), stderr: "" }; + } + if (scenario === "octopus merge" && args[1] === "--merges") { + return { exitCode: 0, stdout: `${candidate} ${oldHead} ${parents[0]} ${parents[1]}\n`, stderr: "" }; + } + return original(command, args, options); + }); + + expect(await resolve()).toBe(""); + expect(compareCommits).not.toHaveBeenCalled(); + for (const [, args] of mockExec.getExecOutput.mock.calls) { + if (args[0] === "rev-list") expect(args).toContain("--max-count=101"); + } + }); + + it.each(["merge history", "parent history"])("still recognizes an ancestor at the 100-record %s boundary", async scenario => { + const original = mockExec.getExecOutput.getMockImplementation(); + mockExec.getExecOutput.mockImplementation(async (command, args, options) => { + if (scenario === "merge history" && args[1] === "--merges") { + return { exitCode: 0, stdout: Array.from({ length: 100 }, () => `${candidate} ${oldHead} ${parents[0]}`).join("\n"), stderr: "" }; + } + if (scenario === "parent history" && args[1] === "--topo-order") { + return { exitCode: 0, stdout: [...Array.from({ length: 99 }, () => candidate), parents[0]].join("\n"), stderr: "" }; + } + return original(command, args, options); + }); + compareCommits.mockResolvedValue({ data: { status: "behind" } }); + expect(await resolve()).toBe(parents[0]); + expect(compareCommits).toHaveBeenCalledTimes(1); + }); +}); + +describe("bounded optional validation Git", () => { + const childProcess = require("child_process"); + const { boundedValidationExec, resolveTrustedBaseTip, resolveBundleTrustedBase, getBundleValidationBase } = require("./push_to_pull_request_branch.cjs"); + let spawn; + let realSpawn; + let debug; + + beforeEach(() => { + debug = vi.fn(); + vi.stubGlobal("core", { debug, warning: vi.fn() }); + realSpawn = childProcess.spawnSync; + spawn = vi.spyOn(childProcess, "spawnSync"); + }); + + afterEach(() => { + spawn.mockRestore(); + vi.unstubAllGlobals(); + }); + + it("enforces process and output bounds that caller options cannot relax", async () => { + spawn.mockReturnValue({ status: 0, stdout: "git output", stderr: "private diagnostic" }); + expect(await boundedValidationExec.getExecOutput("git", ["rev-list", "HEAD"], { cwd: "/checkout", env: { GIT_ALLOW_PROTOCOL: "" }, timeout: 0, maxBuffer: Infinity })).toEqual({ exitCode: 0, stdout: "git output", stderr: "" }); + expect(spawn).toHaveBeenCalledWith("git", ["rev-list", "HEAD"], expect.objectContaining({ cwd: "/checkout", env: { GIT_ALLOW_PROTOCOL: "" }, timeout: 5000, maxBuffer: 1024 * 1024, killSignal: "SIGKILL", stdio: "pipe" })); + }); + + it.each(["ETIMEDOUT", "ENOBUFS", "ENOENT", "SIGKILL"])("discards incomplete output and falls back after %s", async failure => { + spawn.mockReturnValue({ + status: failure === "SIGKILL" ? null : 0, + signal: failure === "SIGKILL" ? failure : null, + error: failure === "SIGKILL" ? undefined : Object.assign(new Error("private diagnostic"), { code: failure }), + stdout: "partial private output", + stderr: "private diagnostic", + }); + const compareCommits = vi.fn(); + const client = { rest: { repos: { compareCommits } } }; + expect(await resolveTrustedBaseTip(boundedValidationExec, {}, "main")).toBe(""); + expect(await resolveBundleTrustedBase(boundedValidationExec, {}, "old", "candidate", "a".repeat(40), client, { owner: "owner", repo: "repo" })).toBe(""); + expect(await getBundleValidationBase(boundedValidationExec, {}, "old", "candidate", "a".repeat(40))).toEqual({ ref: "old", conflictedFiles: [] }); + expect(compareCommits).not.toHaveBeenCalled(); + expect(JSON.stringify(debug.mock.calls)).not.toContain("private"); + }); + + it("preserves merge-tree conflict output for conservative file protection", async () => { + spawn.mockReturnValue({ status: 1, stdout: `${"a".repeat(40)}\0package.json\0\0`, stderr: "private diagnostic" }); + expect(await boundedValidationExec.getExecOutput("git", ["merge-tree"])).toEqual({ exitCode: 1, stdout: `${"a".repeat(40)}\0package.json\0\0`, stderr: "" }); + }); + + it.each([ + ["ETIMEDOUT", "setInterval(() => {}, 1000)"], + ["ENOBUFS", "process.stdout.write(Buffer.alloc(2 * 1024 * 1024)); setInterval(() => {}, 1000)"], + ])("terminates a real subprocess on %s without returning partial output", async (code, script) => { + let result; + spawn.mockImplementation((_command, _args, options) => { + result = realSpawn(process.execPath, ["-e", script], options); + return result; + }); + expect(await boundedValidationExec.getExecOutput("git", ["rev-list", "HEAD"])).toEqual({ exitCode: 128, stdout: "", stderr: "" }); + expect(result.error.code).toBe(code); + expect(result.signal).toBe("SIGKILL"); }); }); diff --git a/docs/src/content/docs/reference/safe-outputs-pull-requests.md b/docs/src/content/docs/reference/safe-outputs-pull-requests.md index c8c774d8a11..b009683125b 100644 --- a/docs/src/content/docs/reference/safe-outputs-pull-requests.md +++ b/docs/src/content/docs/reference/safe-outputs-pull-requests.md @@ -437,9 +437,11 @@ By default, pushes are replayed through GitHub's signed commit API because `sign Signed replay checks every commit's synthesized file changes before creating a remote branch or publishing commits. Each commit supports at most 4096 additions and deletions combined, subject to configured patch limits; exceeding this bound fails without unsigned fallback. Queue Claims retain per-path authorization, including the first commit's paths when creating a new branch. -For unsigned bundle pushes, the handler pins the current PR base branch tip using an exact-ref, metadata-only `git ls-remote` against the target checkout's origin, with that checkout's credentials rather than a fork-head token. It does not fetch objects to establish this optional baseline. After local bundle import, patch-size, changed-content-size, and protected-file checks can compare against a validation-only merge of the old PR head and that trusted tip when the necessary objects and ancestry are locally available. Unmodified base-branch changes are excluded; agent changes and conflict resolutions remain subject to the checks. Agent-created octopus merges (three or more parents) are rejected because their resolution changes cannot be validated with Git's remerge diffs. +For unsigned bundle pushes, the handler pins the current PR base branch tip using an exact-ref, metadata-only `git ls-remote` against the target checkout's origin, with that checkout's credentials rather than a fork-head token. It does not fetch objects to establish this optional baseline. After local bundle import, patch-size and changed-content-size checks can compare against a validation-only merge of the old PR head and a locally available ancestor confirmed against the pinned tip. Only the exact pinned tip exempts base changes from protected-file and history checks; other ancestors retain PR-head protection because reachability does not prove a historical base-branch state. Agent changes and conflict resolutions remain subject to the checks. Agent-created octopus merges (three or more parents) are rejected because their resolution changes cannot be validated with Git's remerge diffs. -Local bundle import and optional ancestry/tree inspection deny all Git transports, including promisor lazy downloads. This uses `GIT_ALLOW_PROTOCOL` as well as `GIT_NO_LAZY_FETCH`, so it does not depend on newer Git versions recognizing the latter. Missing credentials, unavailable history or blobs, and unsupported `merge-tree` retain conservative PR-head validation; they never cause an optional baseline fetch. A newer base tip not merged into the candidate also retains PR-head validation; workflow-start SHAs and agent metadata are not alternate provenance sources. Staged mode retains offline patch-preview checks. The push remains append-only: merge an already-provisioned base ref rather than rebasing existing PR commits. +Optional baseline discovery limits each Git command to 5 seconds and 1 MiB per output stream, each history scan to 100 records, and REST comparisons to four requests with a 5-second abort deadline and no retries. Reaching a limit retains conservative PR-head validation. Only missing-parent (404) responses permit another comparison within that budget; other API failures stop discovery. + +Local bundle import and optional ancestry/tree inspection deny all Git transports, including promisor lazy downloads. This uses `GIT_ALLOW_PROTOCOL` as well as `GIT_NO_LAZY_FETCH`, so it does not depend on newer Git versions recognizing the latter. Missing credentials, unavailable history or blobs, and unsupported `merge-tree` retain conservative PR-head validation; they never cause an optional baseline fetch, deepen a shallow checkout, or populate missing blobs. A newer base tip may confirm an already-provisioned ancestor through metadata-only REST comparisons, but is never downloaded. Workflow-start SHAs and agent metadata are not alternate provenance sources. Staged mode retains offline patch-preview checks. The push remains append-only: merge an already-provisioned base ref rather than rebasing existing PR commits. ### Bundle prerequisites and history downloads From 40e84dd2d078ab1d7278747cdfd012c567d03aca Mon Sep 17 00:00:00 2001 From: pelikhan Date: Sun, 18 Oct 2026 16:52:28 -0700 Subject: [PATCH 5/7] Specify bounded safe-output Git operations Formalize offline baseline discovery, explicit prerequisite recovery, credential roles, and separate size/protection provenance. Add a bounded TLA+ Git-operation model with executable negative controls and reachability witnesses. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../docs/specs/safe-outputs-specification.md | 42 +++- specs/workflow-security/README.md | 88 +++++++- specs/workflow-security/SafeOutputGit.cfg | 26 +++ specs/workflow-security/SafeOutputGit.tla | 208 ++++++++++++++++++ specs/workflow-security/check_test.py | 61 ++++- specs/workflow-security/check_transport.py | 103 ++++++--- 6 files changed, 495 insertions(+), 33 deletions(-) create mode 100644 specs/workflow-security/SafeOutputGit.cfg create mode 100644 specs/workflow-security/SafeOutputGit.tla diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index 7aa0752ec76..bac5c60b645 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -7,9 +7,9 @@ sidebar: # Safe Outputs MCP Gateway Specification -**Version**: 1.29.12
+**Version**: 1.29.13
**Status**: Working Draft
-**Publication Date**: 2026-10-14
+**Publication Date**: 2026-10-18
**Editor**: GitHub Agentic Workflows Team
**This Version**: [safe-outputs-specification](/gh-aw/specs/safe-outputs-specification/)
**Latest Published Version**: This document @@ -2572,6 +2572,7 @@ safe-outputs: 12. **`head_repo` Required for Same-Organization Forks**: The owner-qualified head reference (requirement 9) alone is insufficient when `head-repo` and `target-repo` share the same owner/organization, because multiple repositories under that owner can define the same branch name. Whenever `head-repo` differs from `target-repo` — including when both share the same owner/organization — every pull request creation or update API call MUST include the `head_repo` parameter naming the head repository, in addition to the owner-qualified `head` reference. 13. **Signed Commit Delivery**: Branch delivery MUST satisfy the [signed commit delivery requirements](#signed-commit-delivery-requirements), shared with `push_to_pull_request_branch`. 14. **Transport Integrity**: Patch/bundle application and publication MUST satisfy the shared [transport integrity requirements](#patch-and-bundle-transport-integrity-requirements). +15. **Bundle Git Operations**: Bundle inspection and prerequisite recovery MUST satisfy [BGO-001–003 and BGO-008](#bundle-git-operations-and-validation-baselines). Optional unsigned push baselines (BGO-004–007) apply only to `push_to_pull_request_branch`. **Configuration Parameters**: @@ -2591,6 +2592,7 @@ safe-outputs: - `github-token`: Optional credential for upstream pull request creation and related upstream metadata operations - `head-github-token`: Optional credential for `head-repo` branch writes. When specified, it SHOULD be limited to `contents: write` on `head-repo` - `head-github-app`: Optional GitHub App configuration to mint an ephemeral credential for `head-repo` branch writes at runtime. When `head-github-app` is configured, the minted token takes precedence over `head-github-token`. The app installation MUST have `contents: write` on `head-repo` +- `allow-full-history-fetch`: Literal boolean, default `false`, independently scoped to this handler. Permits potentially unbounded bundle prerequisite recovery; does not enable downloads for optional validation baselines. - `preserve-branch-name`: When `true`, use the agent-supplied branch name verbatim without appending a random salt suffix (default: `false`) - `recreate-ref`: When `true` (and `preserve-branch-name: true`), allows the handler to force-delete an existing remote branch ref and recreate it from the agent's local HEAD on collision. When `false` (default), an existing remote branch under `preserve-branch-name: true` causes a fallback rather than overwriting the remote ref. Has no effect when `preserve-branch-name: false`. (default: `false`) @@ -2635,6 +2637,34 @@ These requirements apply to `create_pull_request` and `push_to_pull_request_bran All implementation and test paths in this table are under `actions/setup/js/`. The bounded [transport lifecycle model](https://github.com/github/gh-aw/blob/main/specs/workflow-security/SafeOutputTransport.tla) specifies stable consumption, failure, retry, cleanup, and diagnostic invariants; it does not prove filesystem isolation or cryptographic collision resistance. +##### Bundle Git Operations and Validation Baselines + +**BGO-001 — Offline object inspection**: Bundle prerequisite checks, local import, and optional baseline ancestry/tree inspection MUST deny all Git transports, including promisor lazy downloads, using both `GIT_ALLOW_PROTOCOL=""` and `GIT_NO_LAZY_FETCH=1`. Missing local history or blobs MUST NOT implicitly deepen a shallow checkout, expand sparse-checkout patterns, or materialize missing remote objects. A local bundle MAY supply its own objects without changing this rule. + +**BGO-002 — Explicit recovery authority**: `create_pull_request` and `push_to_pull_request_branch` MUST default `allow-full-history-fetch` to `false` and MUST accept only a literal boolean. Missing prerequisites MUST fail with checkout-provisioning guidance unless this handler's trusted configuration is exactly `true`. An agent request MUST NOT grant recovery authority. Opted-in recovery MAY use `--unshallow` or fetch-by-SHA and MUST warn that downloads have no byte limit. Depth, a SHA, and `blob:none` MUST NOT be described as byte bounds. A locally sufficient bundle MUST NOT trigger recovery. + +**BGO-003 — Credential roles**: Recovery MUST use the handler-selected authorized repository and its credentials. For `push_to_pull_request_branch`, this is the validated PR head repository, including a configured automation fork; head credentials MUST be restored before baseline discovery and validation. For `create_pull_request`, recovery uses the target checkout's `origin`. Optional base metadata discovery MUST query only the target checkout's `origin` and use its credentials, not a temporarily installed fork-head token. REST comparisons MUST use the upstream repository client; Git credentials MUST NOT be assumed to authorize REST. + +**BGO-004 — Pinned provenance**: For unsigned bundle pushes, optional baseline discovery MUST pin exactly one current base-branch SHA from an exact-ref, metadata-only `ls-remote`. It MUST NOT fetch the pinned object. A locally proven merge of the exact pinned tip MUST bypass REST comparison discovery. Otherwise, a merge parent MAY be selected for size validation only when REST comparison with the pinned SHA as `base` and that parent as `head` reports `behind` or `identical`. Workflow-start SHAs, commit timestamps, and agent metadata MUST NOT substitute for pinned provenance. Parent preference MUST use topological ordering. + +**BGO-005 — Bounded discovery**: Optional base metadata, ancestry, and merge-tree commands MUST have a 5-second process timeout, a 1 MiB limit per output stream, noninteractive authentication, and no shell interpolation. Each merge-parent/history scan MUST accept at most 100 records, using a 101st record to detect overflow before trusting a partial scan. Octopus-parent discovery MUST fall back rather than expand an unbounded argument list. REST comparison discovery MUST issue at most four requests, each with a 5-second abort deadline and automatic retries disabled. Only candidate-specific missing-parent responses (404) MAY continue within the remaining budget. Authorization, rate-limit, service, transport, timeout, and unknown failures MUST stop comparison discovery immediately. + +**BGO-006 — Conservative fallback**: Timeouts, output overflow, missing required local objects, and unsupported merge-tree operations MUST NOT be treated as successful evidence; partial command output MUST NOT establish a baseline. An inconclusive exact-tip ancestry probe MAY proceed to independently bounded parent discovery without downloading the tip. Unavailable pinned metadata, history overflow, an unconfirmed ancestor, or inability to construct its validation tree MUST retain PR-head validation. An optional baseline failure MUST NOT enable recovery downloads even when `allow-full-history-fetch` is true; that flag applies only to prerequisite recovery. Required file/history checks MUST NOT be skipped because optional discovery failed. + +**BGO-007 — Separate size and protection provenance**: A validation-only merge tree MAY exclude confirmed ancestor content from patch-size and changed-content-size calculations when the required objects and ancestry are available locally. Only the exact pinned base tip MAY exempt its history and unmodified files from protected-file checks. A non-tip ancestor MUST retain PR-head-based protected-file and history checks, including changes later reverted in side-branch or agent commits. Reachability alone does not prove a historical base-branch state. Agent changes and conflict resolutions MUST remain subject to policy checks; unvalidated octopus merge resolutions MUST be rejected. + +**BGO-008 — Safe failures and append-only delivery**: Optional discovery and recovery diagnostics MUST NOT expose raw remote errors, artifact contents, or credentials. Validation-only trees MUST NOT rebase or rewrite existing PR commits. Baseline selection MUST NOT change append-only push policy, signing defaults, or authorization of the destination repository. + +**Conformance Evidence**: + +| Requirements | Implementation / regression evidence | +|---|---| +| BGO-001–003 | `git_helpers.cjs`, `git_helpers.test.cjs`, and `bundle_transport.integration.test.cjs`: literal opt-in, scoped fork recovery, unchanged shallow markers and pack inventory, missing blobs still absent, and no optional fetches | +| BGO-004–006 | `push_to_pull_request_branch.cjs` and `push_to_pull_request_branch_comparisons.test.cjs`: 100/101-record boundaries, 24-parent request caps, abort signals, real subprocess time/output limits, fixed diagnostics, and conservative fallback | +| BGO-007–008 | `push_to_pull_request_branch.test.cjs` and `push_to_pull_request_branch.integration.test.cjs`: exact-tip no-network path, advanced-base sizing, strict non-tip protection, A-add/B-remove side-branch imports, reverted changes, and unchanged PR refs on rejection | + +Paths in this table are under `actions/setup/js/`. The bounded [Git-operation model](https://github.com/github/gh-aw/blob/main/specs/workflow-security/SafeOutputGit.tla) separates prerequisite recovery from optional discovery and size trust from protection trust. Synthetic negative controls and reachability witnesses check the abstraction, not Git internals, wall-clock guarantees, credential grants, or implementation refinement. + **Security Requirements**: - Branch name sanitization (prevent injection). Producers and consumers MUST reject dash-only branch names before deriving transport paths or publishing, including in allow-empty mode. Adding a salt MUST NOT turn an empty normalized branch into a valid branch name. @@ -3689,6 +3719,7 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement - `allowed-repos`: Cross-repository allowlist applied to both `target-repo` and `head-repo` - `head-github-token`: Optional credential for `head-repo` branch writes - `head-github-app`: Optional GitHub App configuration to mint an ephemeral credential for `head-repo` branch writes at runtime; takes precedence over `head-github-token` when both are specified +- `allow-full-history-fetch`: Literal boolean, default `false`, independently scoped to this handler. Permits potentially unbounded bundle prerequisite recovery; does not enable downloads for optional validation baselines. **Notes**: @@ -3705,6 +3736,7 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement - When the Convertible fallback (WTD2) opens a review or fallback pull request from `head-repo`, the `head_repo` parameter MUST be included on the pull request creation API call whenever `head-repo` differs from `target-repo`, including when both share the same owner/organization - Signed branch delivery MUST satisfy the shared [signed commit delivery requirements](#signed-commit-delivery-requirements). - Patch/bundle handling and publication MUST satisfy the shared [transport integrity requirements](#patch-and-bundle-transport-integrity-requirements). +- Bundle inspection, recovery, and unsigned validation baselines MUST satisfy [BGO-001–008](#bundle-git-operations-and-validation-baselines). --- @@ -6197,6 +6229,12 @@ This specification revision aligns with directly relevant `CHANGELOG.md` entries - **Earlier changelog entry**: status comments were decoupled from default AI reaction behavior; explicit `on.status-comment` configuration is required when status comments are desired. - **Earlier changelog entry**: `command` trigger was renamed to `slash_command` with deprecation compatibility. +**Version 1.29.13** (2026-10-18): + +- **Added**: BGO-001–008 for offline bundle inspection, literal history-recovery opt-in, repository-specific credential roles, and bounded optional validation-baseline discovery. +- **Clarified**: Confirmed non-tip ancestors may reduce size accounting but cannot exempt protected files/history; only the exact pinned base tip grants that exemption. +- **Added**: A bounded Git-operation TLA+ model with shallow/full and opt-in/default-deny profiles, negative controls, and reachability witnesses in the pinned transport checker. + **Version 1.29.12** (2026-10-14): - **Added**: PTI-001–007 for captured patch/bundle bytes, nonblocking regular-file checks, private snapshots, bounded integrity-check cost, trusted transformations, permanent retry classification, restricted Git cleanup, and safe diagnostics. diff --git a/specs/workflow-security/README.md b/specs/workflow-security/README.md index 0cd78daf8d4..97379f163a1 100644 --- a/specs/workflow-security/README.md +++ b/specs/workflow-security/README.md @@ -45,6 +45,7 @@ sandboxed agent principal, even when GitHub Actions runs both in the same job. | `pull_request_target` checkout policy | [`PullRequestTargetCheckout.tla`](PullRequestTargetCheckout.tla) | Strict warning acknowledgment, explicit checkout disablement, trusted base checkout, exact allowlisted external checkout, and fail-closed `fetch` handling. | | PR checkout authorization | [`CommentTriggeredPRCheckout.tla`](CommentTriggeredPRCheckout.tla) | Same-repository Bot branch-event authorization for supported PR actions plus the stricter allow-listed comment/review exception. | | Patch/bundle application | [`SafeOutputTransport.tla`](SafeOutputTransport.tla) | Captured bytes, trusted transformations, private snapshots, publication retries, sticky failures, bounded local checking cost, and finally cleanup. | +| Bundle Git operations | [`SafeOutputGit.tla`](SafeOutputGit.tla) | Explicit prerequisite recovery, offline optional discovery, bounded comparison/history work, credential roles, and separate size/protection provenance. | | Host-side MCP compilation | [`MCPCompile.tla`](MCPCompile.tla) | Startup write capability, actor authorization, early option rejection, filesystem isolation, cached-only imports, repository-root cache selection, validation, and write-enabled emission. | The `issue` effect creates at most one validated issue in the main public @@ -152,7 +153,8 @@ reported. An already completed remote effect is not rolled back if a later completion check fails. Diagnostic safety and zero full-artifact hashes around an unrelated local command are abstract obligations, not measurements of Node. -Run both baselines, all negative controls, and all reachability witnesses with +Run both transport baselines and the Git-operation profiles below, including +all negative controls and reachability witnesses, with the pinned TLA+ runtime described below. The runner rejects incorrect tooling status or invariant names and automatically removes temporary configurations, TLC states, and traces: @@ -198,7 +200,86 @@ rollback remain implementation obligations covered by `transport_integrity_git.test.cjs`, `error_recovery.test.cjs`, and `create_or_update_pull_request.test.cjs` under `actions/setup/js/`. The existing `check.py` and its checked-in examples cover the job and runtime -repository-list checkout models; `check_transport.py` checks transport separately. +repository-list checkout models; `check_transport.py` checks transport and Git +operations separately. Use `--model transport` or `--model git` to select one. + +## Safe-output bundle Git operations + +[`SafeOutputGit.tla`](SafeOutputGit.tla) models the unsigned push-to-PR bundle +path in Safe Outputs +[BGO-001–008](../../docs/src/content/docs/specs/safe-outputs-specification.md#bundle-git-operations-and-validation-baselines). +Prerequisite recovery and optional baseline discovery are distinct stages. +Missing prerequisites fail by default; only trusted literal opt-in permits +recovery with the validated head-repository credential. Metadata discovery uses +the target checkout's credential instead, and never imports remote objects. +The recovery flag never authorizes downloads during optional discovery. + +An exact pinned tip merged locally bypasses REST. Otherwise, bounded parent +discovery compares against that pinned tip. Only `behind`/`identical` establishes +an ancestor size baseline; `ahead` and candidate-specific 404 can continue +within the request budget. Authorization, rate-limit, service, transport, timeout, +and unknown failures stop comparison discovery. An inconclusive exact-tip +ancestry probe can still enter bounded parent discovery without downloading +the tip. Missing required local data, unsupported operations, time/output +limits, history overflow, and budget exhaustion fall back to PR-head validation +when the remaining checks cannot establish a baseline. +A successful validation tree is required before an exemption is used. A +non-tip ancestor never exempts protected history, including A-add/B-remove or +reverted changes, even when it reduces size accounting. Exact-tip exemptions +still do not excuse agent-authored protected changes. + +The runner exhausts all combinations of `AllowFullHistoryFetch` and `Shallow`. +It requires exhaustive success for these four secure profiles, then the exact +named counterexample for each synthetic fault: + +| Fault | Expected invariant | +|---|---| +| `implicit-recovery` | `RecoveryOptIn` | +| `optional-fetch` / `lazy-fetch` | `NoOptionalMaterialization` | +| `wrong-recovery-token` | `RecoveryAuthentication` | +| `wrong-metadata-token` | `MetadataAuthentication` | +| `unbounded-local` | `BoundedLocalInspection` | +| `unbounded-history` | `BoundedHistory` | +| `unbounded-comparisons` | `BoundedComparisons` | +| `retry-global-error` | `NoGlobalFailureRetry` | +| `comparison-retry` | `NoComparisonRetries` | +| `unbounded-request` | `BoundedRequests` | +| `ancestor-exemption` | `ProtectionProvenance` | +| `unsafe-diagnostics` | `SafeDiagnostics` | + +The wrong-recovery-token control runs with opt-in enabled; the others use the +default-deny shallow profile. Secure profiles additionally check shallow +preservation, exact-tip zero-comparison behavior, conservative fallback, and +protected-change rejection before publication. With `Fault = "none"`, the runner +appends each of `NoAncestorBaseline`, `NoExactTipBaseline`, `NoRecovery`, +`NoConservativeFallback`, `NoSafePublication`, `NoProtectedRejection`, and +`NoMissingParentContinuation`. These false invariants witness effective ancestor +and exact-tip baselines, successful opted-in recovery, fallback, valid +publication, protected-history rejection, and a request following a 404, +respectively. They prevent success from consisting only of rejected operations. + +```bash +TLA2TOOLS_JAR=/path/to/tla2tools.jar JAVA_BIN=/path/to/java \ + python3 specs/workflow-security/check_transport.py --model git +``` + +`SafeOutputGit.cfg` matches the implementation's four-comparison budget and +100-record history limit. Finite boundary classes exercise both the 100th and +101st records, and the over-budget fault attempts a fifth comparison. +The implementation also uses a 5-second deadline per optional Git +command/request and 1 MiB per process output stream. +The model abstracts elapsed time/output into finite success, missing-data, +timeout, and overflow outcomes. It checks decision boundaries, not real time, +process termination, byte counts, Git parsing/DAG correctness, merge correctness, +actual credential grants or restoration, signed delivery, ordinary PR-head +fetch/push operations, or implementation refinement. Pinned refs, local ancestry +answers, REST reachability responses, and protected-history classifications are +assumed accurate inputs to the abstraction. The job-level model covers +credential lifetime; runtime conformance for exact thresholds, subprocess +termination, abort signals, and shallow/partial-clone preservation remains in +`push_to_pull_request_branch_comparisons.test.cjs`, +`push_to_pull_request_branch.integration.test.cjs`, and +`bundle_transport.integration.test.cjs` under `actions/setup/js/`. ## Runtime repository-list checkout @@ -345,6 +426,9 @@ parsing, symlink confinement, `.git` indirection, submodules, object graph ancestry, merge correctness, signed pushes, or concurrent checkout discovery. These require separate refinement of [`gitutil`](../../pkg/gitutil/gitutil.go) and [`findRepoCheckout`](../../actions/setup/js/find_repo_checkout.cjs). +The separate [bundle Git-operation model](#safe-output-bundle-git-operations) +specifies privileged recovery and optional validation-baseline decisions without +adding remote access to the credential-free agent's `GitOperation` transition. ## Host-side MCP compilation diff --git a/specs/workflow-security/SafeOutputGit.cfg b/specs/workflow-security/SafeOutputGit.cfg new file mode 100644 index 00000000000..30d37af5b87 --- /dev/null +++ b/specs/workflow-security/SafeOutputGit.cfg @@ -0,0 +1,26 @@ +SPECIFICATION Spec +CONSTANTS + Fault = "none" + AllowFullHistoryFetch = FALSE + Shallow = TRUE + ComparisonBudget = 4 + HistoryLimit = 100 +INVARIANTS + TypeOK + RecoveryOptIn + NoOptionalMaterialization + ShallowPreserved + RecoveryAuthentication + MetadataAuthentication + BoundedLocalInspection + BoundedHistory + BoundedComparisons + ExactTipFastPath + ConservativeFallback + NoGlobalFailureRetry + NoComparisonRetries + BoundedRequests + ProtectionProvenance + ProtectedPublication + SafeDiagnostics +CHECK_DEADLOCK FALSE diff --git a/specs/workflow-security/SafeOutputGit.tla b/specs/workflow-security/SafeOutputGit.tla new file mode 100644 index 00000000000..94efbd2dc3e --- /dev/null +++ b/specs/workflow-security/SafeOutputGit.tla @@ -0,0 +1,208 @@ +---------------------------- MODULE SafeOutputGit ---------------------------- +EXTENDS Naturals, Sequences + +CONSTANTS Fault, AllowFullHistoryFetch, Shallow, ComparisonBudget, HistoryLimit +ASSUME AllowFullHistoryFetch \in BOOLEAN /\ Shallow \in BOOLEAN +ASSUME ComparisonBudget \in Nat \ {0} /\ HistoryLimit \in Nat \ {0} +ASSUME Fault \in {"none", "implicit-recovery", "optional-fetch", "lazy-fetch", + "wrong-recovery-token", "wrong-metadata-token", + "unbounded-local", "unbounded-history", "unbounded-comparisons", + "retry-global-error", "comparison-retry", "unbounded-request", + "ancestor-exemption", "unsafe-diagnostics"} + +\* Finite outcome classes abstract process deadlines and output limits. +\* This is the push-to-PR bundle path, not an implementation of Git's DAG. +VARIABLE s +vars == <> +Stages == {"prerequisites", "recover", "metadata", "inspect", "scan", "compare", + "tree", "protect", "publish", "done"} +LocalOutcomes == {"ok", "missing", "unsupported", "timeout", "overflow"} +CompareOutcomes == {"behind", "identical", "ahead", "missing", + "authorization", "rate-limit", "service", "transport", + "timeout", "unknown"} +GlobalFailures == {"authorization", "rate-limit", "service", "transport", + "timeout", "unknown"} + +Init == + \E missing, tipMerged, ancestorProtected, agentProtected \in BOOLEAN: + s = [stage |-> "prerequisites", missing |-> missing, tipMerged |-> tipMerged, + ancestorProtected |-> ancestorProtected, agentProtected |-> agentProtected, + shallow |-> Shallow, recovered |-> FALSE, + recoveryToken |-> "none", metadataToken |-> "none", + optionalDownloaded |-> FALSE, sparseExpanded |-> FALSE, + localBounded |-> TRUE, historyBounded |-> TRUE, + requestBounded |-> TRUE, retries |-> 0, comparisons |-> 0, + retriedGlobalFailure |-> FALSE, diagnosticsSafe |-> TRUE, + baseline |-> "pr-head", protectionExempt |-> FALSE, + fallback |-> FALSE, tipSeen |-> FALSE, + continuedMissing |-> FALSE, published |-> FALSE, rejected |-> FALSE, + lastEvent |-> "Init"] + +CheckPrerequisites == + /\ s.stage = "prerequisites" + /\ s' = [s EXCEPT + !.stage = IF ~s.missing THEN "metadata" + ELSE IF AllowFullHistoryFetch \/ Fault = "implicit-recovery" + THEN "recover" ELSE "done", + !.rejected = s.missing /\ ~AllowFullHistoryFetch /\ Fault # "implicit-recovery", + !.lastEvent = "CheckPrerequisites"] + +Recover(success) == + /\ s.stage = "recover" + /\ s' = [s EXCEPT + !.stage = IF success THEN "metadata" ELSE "done", + !.recoveryToken = IF Fault = "wrong-recovery-token" THEN "target" ELSE "head", + !.recovered = TRUE, + !.shallow = IF success THEN FALSE ELSE @, + !.rejected = ~success, + !.diagnosticsSafe = success \/ Fault # "unsafe-diagnostics", + !.lastEvent = IF success THEN "Recover" ELSE "RecoveryFailure"] + +Metadata(outcome) == + /\ s.stage = "metadata" + /\ LET success == outcome = "ok" + IN s' = [s EXCEPT + !.stage = IF success THEN "inspect" ELSE "protect", + !.metadataToken = IF Fault = "wrong-metadata-token" THEN "head" ELSE "target", + !.fallback = ~success, + !.localBounded = ~(outcome \in {"timeout", "overflow"} /\ Fault = "unbounded-local"), + !.diagnosticsSafe = success \/ Fault # "unsafe-diagnostics", + !.lastEvent = IF success THEN "PinTargetMetadata" ELSE "MetadataFallback"] + +Inspect(outcome) == + /\ s.stage = "inspect" + /\ LET success == outcome = "ok" + limited == outcome \in {"timeout", "overflow"} + download == outcome = "missing" /\ Fault \in {"optional-fetch", "lazy-fetch"} + IN s' = [s EXCEPT + !.stage = IF success THEN IF s.tipMerged THEN "tree" ELSE "scan" + ELSE "protect", + !.baseline = IF success /\ s.tipMerged THEN "tip" ELSE @, + !.tipSeen = success /\ s.tipMerged, + !.fallback = ~success, + !.localBounded = ~(limited /\ Fault = "unbounded-local"), + !.optionalDownloaded = download, + !.shallow = IF download THEN FALSE ELSE @, + !.sparseExpanded = download /\ Fault = "optional-fetch", + !.diagnosticsSafe = success \/ Fault # "unsafe-diagnostics", + !.lastEvent = "Inspect:" \o outcome] + +Scan(records, outcome) == + /\ s.stage = "scan" + /\ LET within == records <= HistoryLimit + accept == outcome = "ok" /\ (within \/ Fault = "unbounded-history") + IN s' = [s EXCEPT + !.stage = IF accept THEN "compare" ELSE "protect", + !.historyBounded = ~accept \/ within, + !.localBounded = ~(outcome \in {"timeout", "overflow"} /\ Fault = "unbounded-local"), + !.fallback = ~accept, + !.diagnosticsSafe = outcome = "ok" \/ Fault # "unsafe-diagnostics", + !.lastEvent = IF accept THEN "Scan" ELSE "ScanFallback"] + +ComparisonLimit == + /\ s.stage = "compare" /\ s.comparisons >= ComparisonBudget + /\ (Fault # "unbounded-comparisons" \/ s.comparisons > ComparisonBudget) + /\ s' = [s EXCEPT !.stage = "protect", !.fallback = TRUE, + !.lastEvent = "ComparisonBudgetFallback"] + +Compare(outcome) == + /\ s.stage = "compare" + /\ (s.comparisons < ComparisonBudget \/ + (Fault = "unbounded-comparisons" /\ s.comparisons = ComparisonBudget)) + /\ LET confirmed == outcome \in {"behind", "identical"} + global == outcome \in GlobalFailures + retryGlobal == global /\ Fault = "retry-global-error" + stop == global /\ ~retryGlobal + IN s' = [s EXCEPT + !.comparisons = @ + 1, + !.stage = IF confirmed THEN "tree" ELSE IF stop THEN "protect" ELSE "compare", + !.baseline = IF confirmed THEN "ancestor" ELSE @, + !.fallback = stop, + !.continuedMissing = @ \/ s.lastEvent = "Compare:missing", + !.retriedGlobalFailure = @ \/ retryGlobal, + !.retries = IF Fault = "comparison-retry" THEN 1 ELSE 0, + !.requestBounded = ~(outcome = "timeout" /\ Fault = "unbounded-request"), + !.diagnosticsSafe = ~global \/ Fault # "unsafe-diagnostics", + !.lastEvent = "Compare:" \o outcome] + +BuildTree(outcome) == + /\ s.stage = "tree" + /\ s' = [s EXCEPT + !.stage = "protect", + !.baseline = IF outcome = "ok" THEN @ ELSE "pr-head", + !.fallback = outcome # "ok", + !.localBounded = ~(outcome \in {"timeout", "overflow"} /\ Fault = "unbounded-local"), + !.diagnosticsSafe = outcome = "ok" \/ Fault # "unsafe-diagnostics", + !.lastEvent = IF outcome = "ok" THEN "ValidationTree" ELSE "TreeFallback"] + +ChooseProtection == + /\ s.stage = "protect" + /\ s' = [s EXCEPT + !.stage = "publish", + !.protectionExempt = s.baseline = "tip" \/ + (s.baseline = "ancestor" /\ Fault = "ancestor-exemption"), + !.lastEvent = "ChooseProtection"] + +Publish == + /\ s.stage = "publish" + /\ LET denied == s.agentProtected \/ (s.ancestorProtected /\ ~s.protectionExempt) + IN s' = [s EXCEPT !.stage = "done", !.published = ~denied, + !.rejected = denied, + !.lastEvent = IF denied THEN "ProtectedRejection" ELSE "Publish"] + +Next == + \/ CheckPrerequisites \/ ComparisonLimit \/ ChooseProtection \/ Publish + \/ \E success \in BOOLEAN: Recover(success) + \/ \E outcome \in LocalOutcomes: + Metadata(outcome) \/ Inspect(outcome) \/ BuildTree(outcome) + \/ \E records \in {HistoryLimit, HistoryLimit + 1}, outcome \in LocalOutcomes: + Scan(records, outcome) + \/ \E outcome \in CompareOutcomes: Compare(outcome) + +Spec == Init /\ [][Next]_vars + +TypeOK == + /\ s.stage \in Stages + /\ s.missing \in BOOLEAN /\ s.tipMerged \in BOOLEAN + /\ s.ancestorProtected \in BOOLEAN /\ s.agentProtected \in BOOLEAN + /\ s.shallow \in BOOLEAN /\ s.recovered \in BOOLEAN + /\ s.recoveryToken \in {"none", "head", "target"} + /\ s.metadataToken \in {"none", "head", "target"} + /\ s.optionalDownloaded \in BOOLEAN /\ s.sparseExpanded \in BOOLEAN + /\ s.localBounded \in BOOLEAN /\ s.historyBounded \in BOOLEAN + /\ s.requestBounded \in BOOLEAN /\ s.retries \in 0..1 + /\ s.comparisons \in 0..(ComparisonBudget + 1) + /\ s.retriedGlobalFailure \in BOOLEAN /\ s.diagnosticsSafe \in BOOLEAN + /\ s.baseline \in {"pr-head", "tip", "ancestor"} + /\ s.protectionExempt \in BOOLEAN /\ s.fallback \in BOOLEAN + /\ s.tipSeen \in BOOLEAN + /\ s.continuedMissing \in BOOLEAN + /\ s.published \in BOOLEAN /\ s.rejected \in BOOLEAN + +RecoveryOptIn == s.recovered => AllowFullHistoryFetch /\ s.missing +ShallowPreserved == ~s.recovered => s.shallow = Shallow +NoOptionalMaterialization == ~s.optionalDownloaded /\ ~s.sparseExpanded +RecoveryAuthentication == s.recovered => s.recoveryToken = "head" +MetadataAuthentication == s.metadataToken \in {"none", "target"} +BoundedLocalInspection == s.localBounded +BoundedHistory == s.historyBounded +BoundedComparisons == s.comparisons <= ComparisonBudget +ExactTipFastPath == s.tipSeen => s.comparisons = 0 +ConservativeFallback == s.fallback => s.baseline = "pr-head" +NoGlobalFailureRetry == ~s.retriedGlobalFailure +NoComparisonRetries == s.retries = 0 +BoundedRequests == s.requestBounded +ProtectionProvenance == s.protectionExempt => s.baseline = "tip" +ProtectedPublication == + s.published => ~s.agentProtected /\ (~s.ancestorProtected \/ s.baseline = "tip") +SafeDiagnostics == s.diagnosticsSafe + +\* Deliberately false reachability predicates; checked only with Fault = "none". +NoAncestorBaseline == ~(s.stage = "publish" /\ s.baseline = "ancestor") +NoExactTipBaseline == ~(s.stage = "publish" /\ s.baseline = "tip") +NoRecovery == ~(s.recovered /\ s.stage = "metadata") +NoConservativeFallback == ~s.fallback +NoSafePublication == ~s.published +NoProtectedRejection == ~(s.rejected /\ s.stage = "done" /\ ~s.missing) +NoMissingParentContinuation == ~(s.continuedMissing /\ s.comparisons = 2) +============================================================================= diff --git a/specs/workflow-security/check_test.py b/specs/workflow-security/check_test.py index c4c26634956..90692ccf1cc 100644 --- a/specs/workflow-security/check_test.py +++ b/specs/workflow-security/check_test.py @@ -31,10 +31,15 @@ def test_all_profiles_faults_and_witnesses_run_and_cleanup(self): digest = hashlib.sha256(jar.read_bytes()).hexdigest() configurations = [] destinations = [] + modules = [] def run(command, cwd, **options): configurations.append((cwd / "model.cfg").read_text()) destinations.append(cwd) + modules.append(command[-1]) + self.assertEqual(options["timeout"], 120) + self.assertIn("-workers", command) + self.assertTrue((cwd / command[-1]).is_file()) return check_transport.subprocess.CompletedProcess(command, 0, "", "") with patch.dict(os.environ, {"TLA2TOOLS_JAR": str(jar)}), \ @@ -45,14 +50,68 @@ def run(command, cwd, **options): patch("builtins.print"): check_transport.main() expected = [None, *check_transport.FAULTS.values(), *check_transport.WITNESSES] - self.assertEqual([call.args[2] for call in result.call_args_list], expected * 2) + git_cases = list(check_transport.git_cases()) + self.assertEqual([call.args[2] for call in result.call_args_list], + expected * 2 + [case[-1] for case in git_cases]) for transport in ["patch", "bundle"]: self.assertEqual( sum(f'Transport = "{transport}"' in config for config in configurations), len(expected), ) + git_configurations = configurations[len(expected) * 2:] + secure_profiles = {(allow, shallow) for name, allow, shallow, _, _ in git_cases + if name.startswith("secure-")} + self.assertEqual(secure_profiles, {(False, False), (False, True), + (True, False), (True, True)}) + self.assertEqual(modules, ["SafeOutputTransport.tla"] * (len(expected) * 2) + + ["SafeOutputGit.tla"] * len(git_cases)) + for config, (name, allow, shallow, fault, invariant) in zip(git_configurations, + git_cases): + self.assertIn(f'Fault = "{fault}"', config) + self.assertIn(f"AllowFullHistoryFetch = {str(allow).upper()}", config) + self.assertIn(f"Shallow = {str(shallow).upper()}", config) + self.assertIn("ComparisonBudget = 4", config) + self.assertIn("HistoryLimit = 100", config) + self.assertIn(" ProtectedPublication\n", config) + if name in check_transport.GIT_WITNESSES: + self.assertIn(f"\nINVARIANT {invariant}\n", config) + else: + self.assertNotIn("\nINVARIANT ", config) self.assertTrue(all(not destination.exists() for destination in destinations)) + def test_model_selection(self): + for model in ["transport", "git"]: + with self.subTest(model=model), tempfile.TemporaryDirectory() as directory: + jar = Path(directory) / "tla2tools.jar" + jar.write_bytes(b"local runtime fixture") + with patch.dict(os.environ, {"TLA2TOOLS_JAR": str(jar)}), \ + patch.object(check_transport, "JAR_SHA256", + hashlib.sha256(jar.read_bytes()).hexdigest()), \ + patch.object(check_transport, "run_case") as run: + check_transport.main(model) + expected = "SafeOutputGit" if model == "git" else "SafeOutputTransport" + self.assertGreater(run.call_count, 0) + self.assertTrue(all(call.args[2] == expected for call in run.call_args_list)) + + def test_git_tool_failure_is_surfaced_and_cleaned_up(self): + with tempfile.TemporaryDirectory() as directory: + jar = Path(directory) / "tla2tools.jar" + jar.write_bytes(b"local runtime fixture") + destinations = [] + + def run(command, cwd, **options): + destinations.append(cwd) + return subprocess.CompletedProcess(command, 1, "Error parsing module", "") + + with patch.dict(os.environ, {"TLA2TOOLS_JAR": str(jar)}), \ + patch.object(check_transport, "JAR_SHA256", + hashlib.sha256(jar.read_bytes()).hexdigest()), \ + patch.object(check_transport.subprocess, "run", side_effect=run): + with self.assertRaisesRegex(RuntimeError, "git-secure.*TLC/tooling failure"): + check_transport.main("git") + self.assertEqual(len(destinations), 1) + self.assertFalse(destinations[0].exists()) + class RuntimePinTest(unittest.TestCase): def test_release_pin_matches_workflow_and_documentation(self): diff --git a/specs/workflow-security/check_transport.py b/specs/workflow-security/check_transport.py index 6ed0bbe5a28..af7d87b8385 100644 --- a/specs/workflow-security/check_transport.py +++ b/specs/workflow-security/check_transport.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 -"""Check transport lifecycle profiles, negative controls, and reachability.""" +"""Check safe-output transport/Git profiles, controls, and reachability.""" +import argparse import hashlib import os from pathlib import Path @@ -27,47 +28,93 @@ "NoSuccessfulWrite", "NoRejectedMutation", "NoRateLimitRetry", "NoCleanupFailure", "NoConflictCleanup", ] +GIT_FAULTS = { + "implicit-recovery": "RecoveryOptIn", + "optional-fetch": "NoOptionalMaterialization", + "lazy-fetch": "NoOptionalMaterialization", + "wrong-recovery-token": "RecoveryAuthentication", + "wrong-metadata-token": "MetadataAuthentication", + "unbounded-local": "BoundedLocalInspection", + "unbounded-history": "BoundedHistory", + "unbounded-comparisons": "BoundedComparisons", + "retry-global-error": "NoGlobalFailureRetry", + "comparison-retry": "NoComparisonRetries", + "unbounded-request": "BoundedRequests", + "ancestor-exemption": "ProtectionProvenance", + "unsafe-diagnostics": "SafeDiagnostics", +} +GIT_WITNESSES = [ + "NoAncestorBaseline", "NoExactTipBaseline", "NoRecovery", + "NoConservativeFallback", "NoSafePublication", "NoProtectedRejection", + "NoMissingParentContinuation", +] + + +def git_cases(): + for allow in [False, True]: + for shallow in [False, True]: + yield (f"secure-recovery-{allow}-shallow-{shallow}", allow, shallow, "none", None) + for fault, invariant in GIT_FAULTS.items(): + yield fault, fault == "wrong-recovery-token", True, fault, invariant + for invariant in GIT_WITNESSES: + yield invariant, invariant == "NoRecovery", True, "none", invariant + + +def run_case(jar, temporary, module, name, config, invariant): + directory = Path(temporary) / name + directory.mkdir() + (directory / f"{module}.tla").write_text((ROOT / f"{module}.tla").read_text()) + (directory / "model.cfg").write_text(config) + result = subprocess.run( + [os.environ.get("JAVA_BIN", "java"), "-XX:+UseParallelGC", + "-Xmx1g", "-cp", str(jar), "tlc2.TLC", "-workers", "1", + "-seed", "1", "-fp", "0", "-config", "model.cfg", + "-metadir", "states", f"{module}.tla"], + cwd=directory, capture_output=True, text=True, timeout=120, + ) + try: + counts = check_result(result.returncode, result.stdout + result.stderr, invariant) + except RuntimeError as error: + raise RuntimeError( + f"{name}: {error}\n{result.stdout}{result.stderr}" + ) from error + print(f"{name}: expected result ({counts['distinct']} distinct states)", flush=True) -def main(): +def main(model="all"): jar_env = os.environ.get("TLA2TOOLS_JAR") if not jar_env: raise RuntimeError("Set TLA2TOOLS_JAR to the official v1.8.0 tla2tools.jar") jar = Path(jar_env).resolve() if not jar.is_file() or hashlib.sha256(jar.read_bytes()).hexdigest() != JAR_SHA256: raise RuntimeError("TLA2TOOLS_JAR is missing or does not match the pinned SHA-256") - model = (ROOT / "SafeOutputTransport.tla").read_text() template = (ROOT / "SafeOutputTransport.cfg").read_text() cases = [("secure", "none", None)] cases += [(fault, fault, invariant) for fault, invariant in FAULTS.items()] cases += [(name, "none", name) for name in WITNESSES] with tempfile.TemporaryDirectory(prefix="gh-aw-transport-tlc-") as temporary: - for transport in ["patch", "bundle"]: - for name, fault, invariant in cases: - directory = Path(temporary) / f"{transport}-{name}" - directory.mkdir() - (directory / "SafeOutputTransport.tla").write_text(model) - config = template.replace('Transport = "patch"', f'Transport = "{transport}"') - config = config.replace('Fault = "none"', f'Fault = "{fault}"') - if name in WITNESSES: + if model in {"all", "transport"}: + for transport in ["patch", "bundle"]: + for name, fault, invariant in cases: + config = template.replace('Transport = "patch"', f'Transport = "{transport}"') + config = config.replace('Fault = "none"', f'Fault = "{fault}"') + if name in WITNESSES: + config += f"\nINVARIANT {name}\n" + run_case(jar, temporary, "SafeOutputTransport", f"{transport}-{name}", + config, invariant) + if model in {"all", "git"}: + template = (ROOT / "SafeOutputGit.cfg").read_text() + for name, allow, shallow, fault, invariant in git_cases(): + config = template.replace('Fault = "none"', f'Fault = "{fault}"') + config = config.replace("AllowFullHistoryFetch = FALSE", + f"AllowFullHistoryFetch = {str(allow).upper()}") + config = config.replace("Shallow = TRUE", f"Shallow = {str(shallow).upper()}") + if name in GIT_WITNESSES: config += f"\nINVARIANT {name}\n" - (directory / "model.cfg").write_text(config) - result = subprocess.run( - [os.environ.get("JAVA_BIN", "java"), "-XX:+UseParallelGC", - "-Xmx1g", "-cp", str(jar), "tlc2.TLC", "-workers", "1", - "-seed", "1", "-fp", "0", "-config", "model.cfg", - "-metadir", "states", "SafeOutputTransport.tla"], - cwd=directory, capture_output=True, text=True, timeout=120, - ) - try: - counts = check_result(result.returncode, result.stdout + result.stderr, invariant) - except RuntimeError as error: - raise RuntimeError( - f"{transport}/{name}: {error}\n{result.stdout}{result.stderr}" - ) from error - print(f"{transport}/{name}: expected result " - f"({counts['distinct']} distinct states)", flush=True) + run_case(jar, temporary, "SafeOutputGit", f"git-{name}", config, invariant) if __name__ == "__main__": - main() + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--model", choices=["all", "transport", "git"], default="all") + main(parser.parse_args().model) From 110094a7d261fd68dc872dfc97f98e203df22206 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Oct 2026 02:14:43 +0000 Subject: [PATCH 6/7] Gate merged-base ancestor baselines behind opt-in Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../js/bundle_transport.integration.test.cjs | 4 +- .../setup/js/push_to_pull_request_branch.cjs | 18 +- ...o_pull_request_branch.integration.test.cjs | 9 +- .../js/push_to_pull_request_branch.test.cjs | 29 +- ...o_pull_request_branch_comparisons.test.cjs | 4 +- .../reference/safe-outputs-pull-requests.md | 2 + .../docs/specs/safe-outputs-specification.md | 6 +- pkg/parser/schema_safe_outputs_target_test.go | 42 +++ pkg/parser/schemas/main_workflow_schema.json | 5 + ...mpiler_safe_outputs_config_targets_test.go | 18 ++ pkg/workflow/push_to_pull_request_branch.go | 262 ++++++++---------- ..._outputs_handler_registry_pull_requests.go | 1 + 12 files changed, 226 insertions(+), 174 deletions(-) diff --git a/actions/setup/js/bundle_transport.integration.test.cjs b/actions/setup/js/bundle_transport.integration.test.cjs index ba752775f87..2151b829564 100644 --- a/actions/setup/js/bundle_transport.integration.test.cjs +++ b/actions/setup/js/bundle_transport.integration.test.cjs @@ -136,7 +136,7 @@ describe("offline bundle transport with real Git", () => { const result = spawnSync("git", ["merge-base", "--is-ancestor", head, base], { cwd: source, env: gitEnv() }); return { data: { status: result.status === 0 ? "behind" : "diverged" } }; }); - const recognized = await resolveBundleTrustedBase(exec, {}, oldHead, "refs/bundles/test", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }); + const recognized = await resolveBundleTrustedBase(exec, {}, oldHead, "refs/bundles/test", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }, true); expect(recognized).toBe(mergedBase); expect(compareCommits.mock.calls.some(([args]) => args.head === olderBase)).toBe(false); const baseline = await getBundleValidationBase(exec, {}, oldHead, "refs/bundles/test", recognized); @@ -213,7 +213,7 @@ describe("offline bundle transport with real Git", () => { const compareCommits = vi.fn(async () => ({ data: { status: "behind" } })); const options = { cwd: shallow, env: gitEnv() }; expect(await resolveTrustedBaseTip(boundedValidationExec, options, "main")).toBe(trustedTip); - const recognized = await resolveBundleTrustedBase(boundedValidationExec, options, oldHead, "refs/bundles/test", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }); + const recognized = await resolveBundleTrustedBase(boundedValidationExec, options, oldHead, "refs/bundles/test", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }, true); expect(recognized).toBe(mergedBase); await getBundleValidationBase(boundedValidationExec, options, oldHead, "refs/bundles/test", recognized); diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 56dfd36cb98..fe2fdb38b34 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -248,12 +248,16 @@ async function resolveTrustedBaseTip(exec, gitOptions, baseRef) { * @param {{ owner: string, repo: string }} repoParts * @returns {Promise} */ -async function resolveBundleTrustedBase(exec, gitOptions, oldHead, candidate, trustedTip, githubClient, repoParts) { +async function resolveBundleTrustedBase(exec, gitOptions, oldHead, candidate, trustedTip, githubClient, repoParts, allowMergedBaseBaseline = false) { if (!trustedTip) return ""; const opts = { ...offlineGitOptions(gitOptions), ignoreReturnCode: true }; try { const tipAncestor = await exec.getExecOutput("git", ["merge-base", "--is-ancestor", trustedTip, candidate], opts); if (tipAncestor.exitCode === 0) return trustedTip; + if (!allowMergedBaseBaseline) { + core.debug("Bundle validation: merged base ancestor discovery is disabled"); + return ""; + } const merges = await exec.getExecOutput("git", ["rev-list", "--merges", "--parents", `--max-count=${MAX_BUNDLE_HISTORY_COMMITS + 1}`, `${oldHead}..${candidate}`], opts); if (merges.exitCode !== 0) return ""; @@ -503,6 +507,7 @@ async function main(config = {}) { const fallbackAsPullRequest = config.fallback_as_pull_request !== false; const checkBranchProtection = config.check_branch_protection !== false; const signedCommits = config.signed_commits !== false; + const allowMergedBaseBaseline = config.allow_merged_base_baseline === true; const commitTitleSuffix = config.commit_title_suffix || ""; const maxSizeKb = parsePositiveInteger(config.max_patch_size) ?? 4096; const maxCount = config.max || 0; // 0 means no limit @@ -612,6 +617,10 @@ async function main(config = {}) { const hasPatchFile = !!(patchFilePath && fs.existsSync(patchFilePath)); const applyTransport = hasBundleFile ? "bundle" : "patch"; const deferBundleValidation = hasBundleFile && !signedCommits && !isStaged; + const mergedBaseBaselineHint = + hasBundleFile && !signedCommits && !allowMergedBaseBaseline + ? ". If the bundle includes commits already merged into the current base branch, set allow-merged-base-baseline: true under safe-outputs.push-to-pull-request-branch to recognize the merged base ancestor." + : ""; core.info(`Apply transport mode: ${applyTransport} (patch file present: ${hasPatchFile}, bundle file present: ${hasBundleFile})`); if (bundleFilePath && !hasBundleFile) { core.warning(`Bundle file path was provided but file is not present on disk: ${bundleFilePath}; falling back to patch transport`); @@ -693,8 +702,7 @@ async function main(config = {}) { core.info(`${sizeLabel}: ${sizeForCheckKb} KB (maximum allowed: ${maxSizeKb} KB)`); if (sizeForCheckKb > maxSizeKb) { - let msg; - msg = `Patch size (${sizeForCheckKb} KB) exceeds maximum allowed size (${maxSizeKb} KB)`; + const msg = `Patch size (${sizeForCheckKb} KB) exceeds maximum allowed size (${maxSizeKb} KB)${mergedBaseBaselineHint}`; return { error: { success: false, error: msg }, fallbackFiles }; } @@ -1244,7 +1252,7 @@ async function main(config = {}) { await importLocalBundle(exec, bundleFilePath, `refs/heads/${branchName}`, bundleRef, baseGitOpts); core.info(`Imported local bundle to ${bundleRef}`); - const trustedBase = await resolveBundleTrustedBase(boundedValidationExec, baseGitOpts, rangeBaseRef, bundleRef, trustedTip, githubClient, repoParts); + const trustedBase = await resolveBundleTrustedBase(boundedValidationExec, baseGitOpts, rangeBaseRef, bundleRef, trustedTip, githubClient, repoParts, allowMergedBaseBaseline); const validationBase = await getBundleValidationBase(boundedValidationExec, baseGitOpts, rangeBaseRef, bundleRef, trustedBase); validationBaseRef = validationBase.ref; validationConflictFiles = validationBase.conflictedFiles; @@ -1479,7 +1487,7 @@ async function main(config = {}) { const changedBlobSizeKb = Math.ceil(changedBlobSizeBytes / 1024); core.info(`Changed content size: ${changedBlobSizeKb} KB (maximum allowed: ${maxSizeKb} KB)`); if (changedBlobSizeKb > maxSizeKb) { - const msg = `Changed content size (${changedBlobSizeKb} KB) exceeds maximum allowed size (${maxSizeKb} KB)`; + const msg = `Changed content size (${changedBlobSizeKb} KB) exceeds maximum allowed size (${maxSizeKb} KB)${mergedBaseBaselineHint}`; await exec.exec("git", ["reset", "--hard", rangeBaseRef], baseGitOpts); return { success: false, error: msg }; } diff --git a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs index f456f7d27f5..fc355ffb791 100644 --- a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs @@ -140,7 +140,7 @@ describe("push_to_pull_request_branch bundle integration", () => { const ancestor = execGit(["merge-base", "--is-ancestor", head, base], { cwd: repo, allowFailure: true }); return { data: { status: ancestor.status === 0 ? "behind" : "diverged" } }; }); - const recognizedBase = await resolveBundleTrustedBase(exec, {}, oldHead, "HEAD", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }); + const recognizedBase = await resolveBundleTrustedBase(exec, {}, oldHead, "HEAD", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }, true); expect(recognizedBase).toBe(trustedBase); expect(compareCommits).toHaveBeenCalledTimes(baseAdvanced ? 1 : 0); const { ref: baseline, conflictedFiles } = await getBundleValidationBase(exec, {}, oldHead, "HEAD", recognizedBase); @@ -315,7 +315,12 @@ describe("push_to_pull_request_branch bundle integration", () => { const boundedOutput = module.boundedValidationExec.getExecOutput; // Match the fixture's exec API, which defaults all Git calls to targetRepo. boundedOutputSpy = vi.spyOn(module.boundedValidationExec, "getExecOutput").mockImplementation((command, args, options) => boundedOutput(command, args, { ...options, cwd: targetRepo })); - const handler = await module.main({ signed_commits: false, check_branch_protection: false, protected_files: ["package.json"] }); + const handler = await module.main({ + signed_commits: false, + check_branch_protection: false, + allow_merged_base_baseline: true, + protected_files: ["package.json"], + }); const result = await handler({ branch: "feature" }, {}); expect(result).toMatchObject({ success: false, skipped: true }); expect(result.error).toContain(`${agentReverted ? "patch" : "bundle"} modifies protected files (package.json)`); 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 c2c0c16519c..73c47e899ef 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -364,7 +364,7 @@ index 0000000..abc1234 }); const module = await loadModule(); const confirmed = status === "behind" || status === "identical"; - const base = await module.resolveBundleTrustedBase(mockExec, { cwd: tempDir }, "old-head", "candidate", trustedTip, mockGithub, mockContext.repo); + const base = await module.resolveBundleTrustedBase(mockExec, { cwd: tempDir }, "old-head", "candidate", trustedTip, mockGithub, mockContext.repo, true); expect(base).toBe(confirmed ? mergedBase : ""); expect(mockGithub.rest.repos.compareCommits).toHaveBeenCalledExactlyOnceWith({ ...mockContext.repo, base: trustedTip, head: mergedBase, per_page: 1, request: { timeout: 5000, retries: 0, signal: expect.any(AbortSignal) } }); expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["rev-list", "--topo-order", "--max-count=101", mergedBase, "--not", "old-head"], expect.any(Object)); @@ -2384,10 +2384,14 @@ index 0000000..abc1234 ["agent-created octopus merge", "small patch\n", 16, "merge-agent.txt", "octopus merge", false, "", true], ["agent-created octopus merge after trusted-base fallback", "small patch\n", 16, "merge-agent.txt", "octopus merge", false, "", true, true], ]; - const cases = scenarios.flatMap(scenario => [false, true].map(baseAdvanced => [baseAdvanced, ...scenario])); + const cases = scenarios.flatMap(scenario => [false, true].map(baseAdvanced => [baseAdvanced, true, ...scenario])); + cases.push( + [true, false, "oversized agent patch with older-base discovery disabled", "x".repeat(2048), 16, "merge-agent.txt", "Patch size", false], + [true, false, "oversized expanded content with older-base discovery disabled", "small patch\n", 2048, "merge-agent.txt", "Changed content size", false] + ); it.each(cases)( - "validates with advanced base %s: %s independently of merged base changes", - async (baseAdvanced, _name, agentPatch, contentSize, changedFile, expectedError, binaryConflict, historyFile = "", octopusMerge = false, mergeTreeFailure = false, historyPatch = agentPatch) => { + "validates with advanced base %s and merged-base opt-in %s: %s", + async (baseAdvanced, allowMergedBaseBaseline, _name, agentPatch, contentSize, changedFile, expectedError, binaryConflict, historyFile = "", octopusMerge = false, mergeTreeFailure = false, historyPatch = agentPatch) => { const branch = "merge-size-validation"; const trustedBase = "b".repeat(40); const trustedTip = baseAdvanced ? "d".repeat(40) : trustedBase; @@ -2423,11 +2427,19 @@ index 0000000..abc1234 const pushSpy = vi.spyOn(pushModule, "pushSignedCommits").mockResolvedValue("candidate-head"); try { const module = await loadModule(); - const handler = await module.main({ max_patch_size: 1, signed_commits: false, protected_files: ["package.json"] }); + const handler = await module.main({ + max_patch_size: 1, + signed_commits: false, + allow_merged_base_baseline: allowMergedBaseBaseline, + protected_files: ["package.json"], + }); const result = await handler({ branch, diff_size: 0 }, {}); if (expectedError) { expect(result.success).toBe(false); expect(result.error).toContain(expectedError); + if (baseAdvanced && !allowMergedBaseBaseline && ["Patch size", "Changed content size"].includes(expectedError)) { + expect(result.error).toContain("allow-merged-base-baseline: true"); + } expect(pushSpy).not.toHaveBeenCalled(); } else { expect(result.success).toBe(true); @@ -2438,7 +2450,7 @@ index 0000000..abc1234 } } expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["ls-remote", "--refs", "--exit-code", "origin", "refs/heads/main"], expect.objectContaining({ silent: true })); - if (baseAdvanced) { + if (baseAdvanced && allowMergedBaseBaseline) { expect(mockGithub.rest.repos.compareCommits).toHaveBeenCalledWith({ owner: "test-owner", repo: "test-repo", @@ -2451,6 +2463,11 @@ index 0000000..abc1234 } else { expect(mockGithub.rest.repos.compareCommits).not.toHaveBeenCalled(); } + if (!baseAdvanced || allowMergedBaseBaseline) { + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["merge-tree", "--write-tree", "--name-only", "-z", "remote-head", trustedBase], expect.any(Object)); + } else { + expect(mockExec.getExecOutput).not.toHaveBeenCalledWith("git", ["merge-tree", "--write-tree", "--name-only", "-z", "remote-head", trustedBase], expect.any(Object)); + } expect(mockExec.exec).not.toHaveBeenCalledWith("git", ["fetch", "origin", trustedBase], expect.any(Object)); if (!mergeTreeFailure && expectedError !== "protected files" && expectedError !== "Patch size") { expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["diff", "--name-only", "--no-renames", "-z", `${baseAdvanced ? "remote-head" : validationTree}..refs/bundles/push-feature-branch`], expect.any(Object)); diff --git a/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs b/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs index e3a746f66b6..027c452b8ed 100644 --- a/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs @@ -52,7 +52,7 @@ describe("bundle merge-parent comparison budget", () => { }); async function resolve() { - return resolveBundleTrustedBase(mockExec, { cwd: process.cwd(), env: { GIT_ALLOW_PROTOCOL: "https", GIT_NO_LAZY_FETCH: "0" } }, oldHead, candidate, trustedTip, { rest: { repos: { compareCommits } } }, repoParts); + return resolveBundleTrustedBase(mockExec, { cwd: process.cwd(), env: { GIT_ALLOW_PROTOCOL: "https", GIT_NO_LAZY_FETCH: "0" } }, oldHead, candidate, trustedTip, { rest: { repos: { compareCommits } } }, repoParts, true); } it.each(["missing", "ahead", "diverged", "unknown"])("caps 24 attacker-controlled parents at four comparisons after %s results", async status => { @@ -184,7 +184,7 @@ describe("bounded optional validation Git", () => { const compareCommits = vi.fn(); const client = { rest: { repos: { compareCommits } } }; expect(await resolveTrustedBaseTip(boundedValidationExec, {}, "main")).toBe(""); - expect(await resolveBundleTrustedBase(boundedValidationExec, {}, "old", "candidate", "a".repeat(40), client, { owner: "owner", repo: "repo" })).toBe(""); + expect(await resolveBundleTrustedBase(boundedValidationExec, {}, "old", "candidate", "a".repeat(40), client, { owner: "owner", repo: "repo" }, true)).toBe(""); expect(await getBundleValidationBase(boundedValidationExec, {}, "old", "candidate", "a".repeat(40))).toEqual({ ref: "old", conflictedFiles: [] }); expect(compareCommits).not.toHaveBeenCalled(); expect(JSON.stringify(debug.mock.calls)).not.toContain("private"); diff --git a/docs/src/content/docs/reference/safe-outputs-pull-requests.md b/docs/src/content/docs/reference/safe-outputs-pull-requests.md index b009683125b..dd02a0f6371 100644 --- a/docs/src/content/docs/reference/safe-outputs-pull-requests.md +++ b/docs/src/content/docs/reference/safe-outputs-pull-requests.md @@ -149,6 +149,8 @@ The agent's commits are packaged as a **git bundle** and uploaded as an Actions If the base branch advances between agent start and `safe_outputs` apply, the PR can be created slightly behind the current base and addressed with a rebase when the necessary bundle objects are available. Otherwise, missing prerequisites cause the handler to fail unless recovery is explicitly enabled as described above. If a non-fast-forward race occurs during the push itself, the job creates a fallback PR from a temporary branch so no changes are lost. +For unsigned bundle pushes, the handler recognizes the exact pinned base tip by default. If the base branch has advanced past an ancestor already included in the bundle, that older-ancestor size baseline is opt-in: set `allow-merged-base-baseline: true` under `safe-outputs.push-to-pull-request-branch`. Without it, validation uses the PR-head baseline; a related patch-size error points to the opt-in. This setting only enables bounded metadata/REST discovery and does not fetch base objects or enable history recovery. + An older **patch transport** (`git format-patch` / `git am --3way`) is used when bundle data is unavailable. `--3way` resolves cleanly against an updated base when there are no conflicts; if it cannot, the patch is applied at the agent's original base commit and the PR UI shows the conflicts for manual resolution. :::caution[Shallow checkout and large monorepos] diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index bac5c60b645..08829255c90 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -2645,11 +2645,11 @@ All implementation and test paths in this table are under `actions/setup/js/`. T **BGO-003 — Credential roles**: Recovery MUST use the handler-selected authorized repository and its credentials. For `push_to_pull_request_branch`, this is the validated PR head repository, including a configured automation fork; head credentials MUST be restored before baseline discovery and validation. For `create_pull_request`, recovery uses the target checkout's `origin`. Optional base metadata discovery MUST query only the target checkout's `origin` and use its credentials, not a temporarily installed fork-head token. REST comparisons MUST use the upstream repository client; Git credentials MUST NOT be assumed to authorize REST. -**BGO-004 — Pinned provenance**: For unsigned bundle pushes, optional baseline discovery MUST pin exactly one current base-branch SHA from an exact-ref, metadata-only `ls-remote`. It MUST NOT fetch the pinned object. A locally proven merge of the exact pinned tip MUST bypass REST comparison discovery. Otherwise, a merge parent MAY be selected for size validation only when REST comparison with the pinned SHA as `base` and that parent as `head` reports `behind` or `identical`. Workflow-start SHAs, commit timestamps, and agent metadata MUST NOT substitute for pinned provenance. Parent preference MUST use topological ordering. +**BGO-004 — Pinned provenance and opt-in**: For unsigned bundle pushes, the handler MUST pin exactly one current base-branch SHA from an exact-ref, metadata-only `ls-remote` to preserve exact-tip recognition. It MUST NOT fetch the pinned object. Older-ancestor baseline discovery MUST be disabled by default and enabled only by literal `allow-merged-base-baseline: true` on `safe-outputs.push-to-pull-request-branch`. A locally proven merge of the exact pinned tip MUST bypass REST comparison discovery regardless of the opt-in. When older-ancestor discovery is enabled, a merge parent MAY be selected for size validation only when REST comparison with the pinned SHA as `base` and that parent as `head` reports `behind` or `identical`. Workflow-start SHAs, commit timestamps, and agent metadata MUST NOT substitute for pinned provenance. Parent preference MUST use topological ordering. **BGO-005 — Bounded discovery**: Optional base metadata, ancestry, and merge-tree commands MUST have a 5-second process timeout, a 1 MiB limit per output stream, noninteractive authentication, and no shell interpolation. Each merge-parent/history scan MUST accept at most 100 records, using a 101st record to detect overflow before trusting a partial scan. Octopus-parent discovery MUST fall back rather than expand an unbounded argument list. REST comparison discovery MUST issue at most four requests, each with a 5-second abort deadline and automatic retries disabled. Only candidate-specific missing-parent responses (404) MAY continue within the remaining budget. Authorization, rate-limit, service, transport, timeout, and unknown failures MUST stop comparison discovery immediately. -**BGO-006 — Conservative fallback**: Timeouts, output overflow, missing required local objects, and unsupported merge-tree operations MUST NOT be treated as successful evidence; partial command output MUST NOT establish a baseline. An inconclusive exact-tip ancestry probe MAY proceed to independently bounded parent discovery without downloading the tip. Unavailable pinned metadata, history overflow, an unconfirmed ancestor, or inability to construct its validation tree MUST retain PR-head validation. An optional baseline failure MUST NOT enable recovery downloads even when `allow-full-history-fetch` is true; that flag applies only to prerequisite recovery. Required file/history checks MUST NOT be skipped because optional discovery failed. +**BGO-006 — Conservative fallback**: Timeouts, output overflow, missing required local objects, and unsupported merge-tree operations MUST NOT be treated as successful evidence; partial command output MUST NOT establish a baseline. An inconclusive exact-tip ancestry probe MAY proceed to independently bounded parent discovery without downloading the tip only when `allow-merged-base-baseline: true`. With older-ancestor discovery disabled, or when discovery is inconclusive, unavailable pinned metadata, history overflow, an unconfirmed ancestor, or inability to construct its validation tree MUST retain PR-head validation unless the exact pinned tip was locally proven. An optional baseline failure MUST NOT enable recovery downloads even when `allow-full-history-fetch` is true; that flag applies only to prerequisite recovery. Required file/history checks MUST NOT be skipped because optional discovery failed. **BGO-007 — Separate size and protection provenance**: A validation-only merge tree MAY exclude confirmed ancestor content from patch-size and changed-content-size calculations when the required objects and ancestry are available locally. Only the exact pinned base tip MAY exempt its history and unmodified files from protected-file checks. A non-tip ancestor MUST retain PR-head-based protected-file and history checks, including changes later reverted in side-branch or agent commits. Reachability alone does not prove a historical base-branch state. Agent changes and conflict resolutions MUST remain subject to policy checks; unvalidated octopus merge resolutions MUST be rejected. @@ -2660,7 +2660,7 @@ All implementation and test paths in this table are under `actions/setup/js/`. T | Requirements | Implementation / regression evidence | |---|---| | BGO-001–003 | `git_helpers.cjs`, `git_helpers.test.cjs`, and `bundle_transport.integration.test.cjs`: literal opt-in, scoped fork recovery, unchanged shallow markers and pack inventory, missing blobs still absent, and no optional fetches | -| BGO-004–006 | `push_to_pull_request_branch.cjs` and `push_to_pull_request_branch_comparisons.test.cjs`: 100/101-record boundaries, 24-parent request caps, abort signals, real subprocess time/output limits, fixed diagnostics, and conservative fallback | +| BGO-004–006 | `push_to_pull_request_branch.cjs` and `push_to_pull_request_branch_comparisons.test.cjs`: disabled-by-default discovery, exact-tip compatibility, 100/101-record boundaries, 24-parent request caps, abort signals, real subprocess time/output limits, fixed diagnostics, and conservative fallback | | BGO-007–008 | `push_to_pull_request_branch.test.cjs` and `push_to_pull_request_branch.integration.test.cjs`: exact-tip no-network path, advanced-base sizing, strict non-tip protection, A-add/B-remove side-branch imports, reverted changes, and unchanged PR refs on rejection | Paths in this table are under `actions/setup/js/`. The bounded [Git-operation model](https://github.com/github/gh-aw/blob/main/specs/workflow-security/SafeOutputGit.tla) separates prerequisite recovery from optional discovery and size trust from protection trust. Synthetic negative controls and reachability witnesses check the abstraction, not Git internals, wall-clock guarantees, credential grants, or implementation refinement. diff --git a/pkg/parser/schema_safe_outputs_target_test.go b/pkg/parser/schema_safe_outputs_target_test.go index 6bc4ecfaad3..6537602491a 100644 --- a/pkg/parser/schema_safe_outputs_target_test.go +++ b/pkg/parser/schema_safe_outputs_target_test.go @@ -82,6 +82,48 @@ func TestMainWorkflowSchema_AllowFullHistoryFetch(t *testing.T) { }) } +func TestMainWorkflowSchema_AllowMergedBaseBaseline(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + value any + valid bool + }{ + {"enabled", true, true}, + {"disabled", false, true}, + {"string", "true", false}, + {"expression", "${{ inputs.allow-merged-base-baseline }}", false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + frontmatter := map[string]any{ + "on": "push", + "engine": "copilot", + "safe-outputs": map[string]any{ + "push-to-pull-request-branch": map[string]any{"allow-merged-base-baseline": tc.value}, + }, + } + err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/merged-base-baseline-test.md") + if (err == nil) != tc.valid { + t.Fatalf("schema validity = %v, want %v: %v", err == nil, tc.valid, err) + } + }) + } + t.Run("handler-scoped", func(t *testing.T) { + t.Parallel() + frontmatter := map[string]any{ + "on": "push", + "engine": "copilot", + "safe-outputs": map[string]any{ + "create-pull-request": map[string]any{"allow-merged-base-baseline": true}, + }, + } + if err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/merged-base-baseline-test.md"); err == nil { + t.Fatal("expected merged-base baseline option on create-pull-request to fail schema validation") + } + }) +} + func TestMainWorkflowSchema_SafeOutputConfigCoverage(t *testing.T) { t.Parallel() diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index b1d8b1b19f7..1ca11442e44 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -11257,6 +11257,11 @@ "description": "Allow potentially unbounded history downloads to recover missing local git bundle prerequisites. Disabled by default. Does not enable object downloads for optional validation baselines.", "default": false }, + "allow-merged-base-baseline": { + "type": "boolean", + "description": "Allow bounded REST comparison discovery of older merged base ancestors for unsigned bundle size validation. Disabled by default; the exact pinned base tip remains recognized.", + "default": false + }, "allow-workflows": { "type": "boolean", "description": "When true, adds workflows: write to the GitHub App token permissions. Required when allowed-files targets .github/workflows/ paths. Requires safe-outputs.github-app to be configured because the workflows permission is a GitHub App-only permission and cannot be granted via GITHUB_TOKEN.", diff --git a/pkg/workflow/compiler_safe_outputs_config_targets_test.go b/pkg/workflow/compiler_safe_outputs_config_targets_test.go index 95b83e925b0..ad7314bb2f4 100644 --- a/pkg/workflow/compiler_safe_outputs_config_targets_test.go +++ b/pkg/workflow/compiler_safe_outputs_config_targets_test.go @@ -74,6 +74,24 @@ func TestAllowFullHistoryFetchIsHandlerScoped(t *testing.T) { assert.NotContains(t, handlerRegistry["push_to_pull_request_branch"](cfg), "allow_full_history_fetch") } +func TestAllowMergedBaseBaselineIsHandlerScoped(t *testing.T) { + t.Parallel() + cfg := NewCompiler().extractSafeOutputsConfig(map[string]any{"safe-outputs": map[string]any{ + "create-pull-request": map[string]any{}, + "push-to-pull-request-branch": map[string]any{"allow-merged-base-baseline": true}, + }}) + require.NotNil(t, cfg) + assert.NotContains(t, handlerRegistry["create_pull_request"](cfg), "allow_merged_base_baseline") + assert.Equal(t, true, handlerRegistry["push_to_pull_request_branch"](cfg)["allow_merged_base_baseline"]) + + encoded, err := generateSafeOutputsConfig(&WorkflowData{SafeOutputs: cfg}) + require.NoError(t, err) + var runtime map[string]map[string]any + require.NoError(t, json.Unmarshal([]byte(encoded), &runtime)) + assert.NotContains(t, runtime["create_pull_request"], "allow_merged_base_baseline") + assert.Equal(t, true, runtime["push_to_pull_request_branch"]["allow_merged_base_baseline"]) +} + // TestEmptySafeOutputsConfig tests behavior with no safe outputs func TestEmptySafeOutputsConfig(t *testing.T) { compiler := NewCompiler() diff --git a/pkg/workflow/push_to_pull_request_branch.go b/pkg/workflow/push_to_pull_request_branch.go index a9d1da2e305..6e2224af248 100644 --- a/pkg/workflow/push_to_pull_request_branch.go +++ b/pkg/workflow/push_to_pull_request_branch.go @@ -36,6 +36,7 @@ type PushToPullRequestBranchConfig struct { FallbackAsPullRequest *bool `yaml:"fallback-as-pull-request,omitempty"` // When true (default), creates a fallback pull request if direct push fails due to diverged/non-fast-forward branch. When false, fallback is disabled and pull-requests: write is not requested. SignedCommits *bool `yaml:"signed-commits,omitempty"` // When false, skips GitHub GraphQL signed commits and pushes the local git history directly. Default is true. AllowFullHistoryFetch bool `yaml:"allow-full-history-fetch,omitempty"` // Explicit opt-in to unbounded bundle prerequisite downloads (default: false). + AllowMergedBaseBaseline bool `yaml:"allow-merged-base-baseline,omitempty"` // Enables bounded discovery of older merged base ancestors for unsigned bundle size validation. AllowWorkflows bool `yaml:"allow-workflows,omitempty"` // When true, adds workflows: write to the GitHub App token. Requires safe-outputs.github-app to be configured. CheckBranchProtection *bool `yaml:"check-branch-protection,omitempty"` // When false, skips the branch protection API pre-flight check. Default is true (check enabled). Set to false to avoid needing administration: read permission. } @@ -89,168 +90,121 @@ func buildCheckoutRepository(steps []string, c *Compiler, targetRepoSlug string, // parsePushToPullRequestBranchConfig handles push-to-pull-request-branch configuration func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) *PushToPullRequestBranchConfig { - if configData, exists := outputMap["push-to-pull-request-branch"]; exists { - pushToPullRequestBranchLog.Print("Parsing push-to-pull-request-branch configuration") - pushToBranchConfig := &PushToPullRequestBranchConfig{ - IfNoChanges: "warn", // Default behavior: warn when no changes - } - - // Handle the case where configData is nil (push-to-pull-request-branch: with no value) - if configData == nil { - return pushToBranchConfig - } - - if configMap, ok := configData.(map[string]any); ok { - // Parse target (optional, similar to add-comment) - if targetStr := extractTargetFromMap(configMap, pushToPullRequestBranchLog); targetStr != "" { - pushToBranchConfig.Target = targetStr - } - - // Parse if-no-changes (optional, defaults to "warn") - if ifNoChanges, exists := configMap["if-no-changes"]; exists { - if ifNoChangesStr, ok := ifNoChanges.(string); ok { - // Validate the value - switch ifNoChangesStr { - case "warn", "error", "ignore": - pushToBranchConfig.IfNoChanges = ifNoChangesStr - default: - // Invalid value, use default and log warning - if c.verbose { - fmt.Fprintf(os.Stderr, "Warning: invalid if-no-changes value '%s', using default 'warn'\n", ifNoChangesStr) - } - pushToBranchConfig.IfNoChanges = "warn" - } - } - } - - // Parse ignore-missing-branch-failure (optional, defaults to false) - if ignoreMissingBranchFailure, exists := configMap["ignore-missing-branch-failure"]; exists { - if ignoreMissingBranchFailureBool, ok := ignoreMissingBranchFailure.(bool); ok { - pushToBranchConfig.IgnoreMissingBranchFailure = ignoreMissingBranchFailureBool - } - } - - // Parse required-title-prefix (preferred) with fallback to deprecated title-prefix alias. - pushToBranchConfig.TitlePrefix = extractStringFromMap(configMap, "required-title-prefix", pushToPullRequestBranchLog) - if pushToBranchConfig.TitlePrefix == "" { - pushToBranchConfig.TitlePrefix = extractStringFromMap(configMap, "title-prefix", pushToPullRequestBranchLog) - } - - // Parse required-labels (preferred) with fallback to deprecated labels. - pushToBranchConfig.RequiredLabels = ParseStringArrayOrExprFromConfig(configMap, "required-labels", pushToPullRequestBranchLog) - if len(pushToBranchConfig.RequiredLabels) == 0 { - pushToBranchConfig.RequiredLabels = ParseStringArrayOrExprFromConfig(configMap, "labels", pushToPullRequestBranchLog) - } - - // Parse commit-title-suffix (optional) - if commitTitleSuffix, exists := configMap["commit-title-suffix"]; exists { - if commitTitleSuffixStr, ok := commitTitleSuffix.(string); ok { - pushToBranchConfig.CommitTitleSuffix = commitTitleSuffixStr - } - } - - // Parse github-token-for-extra-empty-commit (optional) - token for pushing empty commit to trigger CI - if emptyCommitToken, exists := configMap["github-token-for-extra-empty-commit"]; exists { - if emptyCommitTokenStr, ok := emptyCommitToken.(string); ok { - pushToBranchConfig.GithubTokenForExtraEmptyCommit = emptyCommitTokenStr - pushToPullRequestBranchLog.Printf("Extra empty commit token configured") - } - } - - // Parse target-repo for cross-repository push - pushToBranchConfig.TargetRepoSlug = extractStringFromMap(configMap, "target-repo", pushToPullRequestBranchLog) - pushToBranchConfig.HeadRepoSlug = extractStringFromMap(configMap, "head-repo", pushToPullRequestBranchLog) - pushToBranchConfig.HeadGitHubToken = extractStringFromMap(configMap, "head-github-token", pushToPullRequestBranchLog) - - // Parse head-github-app manually so that the app-id alias is honoured - // (YAML unmarshal would silently ignore app-id since GitHubAppConfig only - // declares the canonical client-id tag). - if headAppData, exists := configMap["head-github-app"]; exists { - if headAppMap, ok := headAppData.(map[string]any); ok { - pushToPullRequestBranchLog.Print("Parsed head-github-app from config") - pushToBranchConfig.HeadGitHubApp = parseAppConfig(headAppMap) - } - } - - // Parse base-branch for explicit override of the target repo's base branch. - // When unset, the safe-outputs MCP server resolves it at runtime from the local - // checkout metadata (origin/HEAD) or falls back to the repo's default branch. - pushToBranchConfig.BaseBranch = extractStringFromMap(configMap, "base-branch", pushToPullRequestBranchLog) - - // Parse allowed-repos for cross-repository push (expression-aware) - pushToBranchConfig.AllowedRepos = ParseStringArrayOrExprFromConfig(configMap, "allowed-repos", pushToPullRequestBranchLog) - - // Parse protected-files: supports string enum OR object form {policy, exclude}. - exclude := preprocessProtectedFilesField(configMap, pushToPullRequestBranchLog) - pushToBranchConfig.ProtectedFilesExclude = exclude - // Validate policy string (no-op if the field was replaced by preprocessor) - manifestFilesEnums := []string{"blocked", "allowed", "fallback-to-issue", "request_review", "request-review"} - validateStringEnumField(configMap, "protected-files", manifestFilesEnums, pushToPullRequestBranchLog) - if strVal, ok := configMap["protected-files"].(string); ok { - normalised := normaliseProtectedFilesPolicy(strVal) - pushToBranchConfig.ManifestFilesPolicy = &normalised - } - - // Parse allowed-files: list of glob patterns forming a strict allowlist of eligible files - pushToBranchConfig.AllowedFiles = ParseStringArrayFromConfig(configMap, "allowed-files", pushToPullRequestBranchLog) - - // Parse excluded-files: list of glob patterns for files to exclude via git :(exclude) pathspecs - pushToBranchConfig.ExcludedFiles = ParseStringArrayFromConfig(configMap, "excluded-files", pushToPullRequestBranchLog) - - // Parse max-patch-size override (optional, must be > 0) - if maxPatchSize, exists := configMap["max-patch-size"]; exists { - if maxPatchSizeInt, ok := typeutil.ParseIntValue(maxPatchSize); ok && maxPatchSizeInt > 0 { - pushToBranchConfig.MaxPatchSize = maxPatchSizeInt - } - } - - // Parse patch-format: valid values are "bundle" (default) and "am" - patchFormatEnums := []string{"am", "bundle"} - validateStringEnumField(configMap, "patch-format", patchFormatEnums, pushToPullRequestBranchLog) - if patchFormat, exists := configMap["patch-format"]; exists { - if patchFormatStr, ok := patchFormat.(string); ok { - pushToBranchConfig.PatchFormat = patchFormatStr - } - } + configData, exists := outputMap["push-to-pull-request-branch"] + if !exists { + return nil + } + pushToPullRequestBranchLog.Print("Parsing push-to-pull-request-branch configuration") + pushToBranchConfig := &PushToPullRequestBranchConfig{IfNoChanges: "warn"} + if configData == nil { + return pushToBranchConfig + } + configMap, ok := configData.(map[string]any) + if !ok { + return pushToBranchConfig + } - // Parse fallback-as-pull-request (optional, defaults to true) - if fallbackAsPullRequest, exists := configMap["fallback-as-pull-request"]; exists { - if fallbackAsPullRequestBool, ok := fallbackAsPullRequest.(bool); ok { - pushToBranchConfig.FallbackAsPullRequest = &fallbackAsPullRequestBool - } - } + c.parsePushToPullRequestBranchConstraints(configMap, pushToBranchConfig) + c.parsePushToPullRequestBranchRepository(configMap, pushToBranchConfig) + c.parsePushToPullRequestBranchFilePolicies(configMap, pushToBranchConfig) + c.parsePushToPullRequestBranchTransport(configMap, pushToBranchConfig) + c.parsePushToPullRequestBranchOptions(configMap, pushToBranchConfig) + c.parseBaseSafeOutputConfig(configMap, &pushToBranchConfig.BaseSafeOutputConfig, 0) + return pushToBranchConfig +} - // Parse signed-commits (optional, defaults to true) - if signedCommits, exists := configMap["signed-commits"]; exists { - if signedCommitsBool, ok := signedCommits.(bool); ok { - pushToBranchConfig.SignedCommits = &signedCommitsBool +func (c *Compiler) parsePushToPullRequestBranchConstraints(configMap map[string]any, config *PushToPullRequestBranchConfig) { + if targetStr := extractTargetFromMap(configMap, pushToPullRequestBranchLog); targetStr != "" { + config.Target = targetStr + } + if ifNoChanges, exists := configMap["if-no-changes"]; exists { + if value, ok := ifNoChanges.(string); ok { + switch value { + case "warn", "error", "ignore": + config.IfNoChanges = value + default: + if c.verbose { + fmt.Fprintf(os.Stderr, "Warning: invalid if-no-changes value '%s', using default 'warn'\n", value) } } + } + } + if value, ok := configMap["ignore-missing-branch-failure"].(bool); ok { + config.IgnoreMissingBranchFailure = value + } + config.TitlePrefix = extractStringFromMap(configMap, "required-title-prefix", pushToPullRequestBranchLog) + if config.TitlePrefix == "" { + config.TitlePrefix = extractStringFromMap(configMap, "title-prefix", pushToPullRequestBranchLog) + } + config.RequiredLabels = ParseStringArrayOrExprFromConfig(configMap, "required-labels", pushToPullRequestBranchLog) + if len(config.RequiredLabels) == 0 { + config.RequiredLabels = ParseStringArrayOrExprFromConfig(configMap, "labels", pushToPullRequestBranchLog) + } +} - if allow, ok := configMap["allow-full-history-fetch"].(bool); ok { - pushToBranchConfig.AllowFullHistoryFetch = allow - } - - // Parse allow-workflows: when true, adds workflows: write to the GitHub App token - if allowWorkflows, exists := configMap["allow-workflows"]; exists { - if allowWorkflowsBool, ok := allowWorkflows.(bool); ok { - pushToBranchConfig.AllowWorkflows = allowWorkflowsBool - } - } +func (c *Compiler) parsePushToPullRequestBranchRepository(configMap map[string]any, config *PushToPullRequestBranchConfig) { + if value, ok := configMap["commit-title-suffix"].(string); ok { + config.CommitTitleSuffix = value + } + if value, ok := configMap["github-token-for-extra-empty-commit"].(string); ok { + config.GithubTokenForExtraEmptyCommit = value + pushToPullRequestBranchLog.Printf("Extra empty commit token configured") + } + config.TargetRepoSlug = extractStringFromMap(configMap, "target-repo", pushToPullRequestBranchLog) + config.HeadRepoSlug = extractStringFromMap(configMap, "head-repo", pushToPullRequestBranchLog) + config.HeadGitHubToken = extractStringFromMap(configMap, "head-github-token", pushToPullRequestBranchLog) + if headAppData, exists := configMap["head-github-app"]; exists { + if headAppMap, ok := headAppData.(map[string]any); ok { + pushToPullRequestBranchLog.Print("Parsed head-github-app from config") + config.HeadGitHubApp = parseAppConfig(headAppMap) + } + } + config.BaseBranch = extractStringFromMap(configMap, "base-branch", pushToPullRequestBranchLog) + config.AllowedRepos = ParseStringArrayOrExprFromConfig(configMap, "allowed-repos", pushToPullRequestBranchLog) +} - // Parse check-branch-protection: when false, skips the branch protection API pre-flight check - if checkBranchProtection, exists := configMap["check-branch-protection"]; exists { - if checkBranchProtectionBool, ok := checkBranchProtection.(bool); ok { - pushToBranchConfig.CheckBranchProtection = &checkBranchProtectionBool - } - } +func (c *Compiler) parsePushToPullRequestBranchFilePolicies(configMap map[string]any, config *PushToPullRequestBranchConfig) { + config.ProtectedFilesExclude = preprocessProtectedFilesField(configMap, pushToPullRequestBranchLog) + manifestFilesEnums := []string{"blocked", "allowed", "fallback-to-issue", "request_review", "request-review"} + validateStringEnumField(configMap, "protected-files", manifestFilesEnums, pushToPullRequestBranchLog) + if value, ok := configMap["protected-files"].(string); ok { + normalised := normaliseProtectedFilesPolicy(value) + config.ManifestFilesPolicy = &normalised + } + config.AllowedFiles = ParseStringArrayFromConfig(configMap, "allowed-files", pushToPullRequestBranchLog) + config.ExcludedFiles = ParseStringArrayFromConfig(configMap, "excluded-files", pushToPullRequestBranchLog) +} - // Parse common base fields with default max of 0 (no limit) - c.parseBaseSafeOutputConfig(configMap, &pushToBranchConfig.BaseSafeOutputConfig, 0) +func (c *Compiler) parsePushToPullRequestBranchTransport(configMap map[string]any, config *PushToPullRequestBranchConfig) { + if maxPatchSize, exists := configMap["max-patch-size"]; exists { + if value, ok := typeutil.ParseIntValue(maxPatchSize); ok && value > 0 { + config.MaxPatchSize = value } - - return pushToBranchConfig } + patchFormatEnums := []string{"am", "bundle"} + validateStringEnumField(configMap, "patch-format", patchFormatEnums, pushToPullRequestBranchLog) + if value, ok := configMap["patch-format"].(string); ok { + config.PatchFormat = value + } +} - return nil +func (c *Compiler) parsePushToPullRequestBranchOptions(configMap map[string]any, config *PushToPullRequestBranchConfig) { + if value, ok := configMap["fallback-as-pull-request"].(bool); ok { + config.FallbackAsPullRequest = &value + } + if value, ok := configMap["signed-commits"].(bool); ok { + config.SignedCommits = &value + } + if value, ok := configMap["allow-full-history-fetch"].(bool); ok { + config.AllowFullHistoryFetch = value + } + if value, ok := configMap["allow-merged-base-baseline"].(bool); ok { + config.AllowMergedBaseBaseline = value + } + if value, ok := configMap["allow-workflows"].(bool); ok { + config.AllowWorkflows = value + } + if value, ok := configMap["check-branch-protection"].(bool); ok { + config.CheckBranchProtection = &value + } } diff --git a/pkg/workflow/safe_outputs_handler_registry_pull_requests.go b/pkg/workflow/safe_outputs_handler_registry_pull_requests.go index f88ab41e766..85379482642 100644 --- a/pkg/workflow/safe_outputs_handler_registry_pull_requests.go +++ b/pkg/workflow/safe_outputs_handler_registry_pull_requests.go @@ -26,6 +26,7 @@ var pullRequestHandlerRegistry = map[string]handlerBuilder{ AddIfNotEmpty("commit_title_suffix", c.CommitTitleSuffix). AddDefault("max_patch_size", maxPatchSize). AddIfTrue("allow_full_history_fetch", c.AllowFullHistoryFetch). + AddIfTrue("allow_merged_base_baseline", c.AllowMergedBaseBaseline). AddIfNotEmpty("target-repo", c.TargetRepoSlug). AddIfNotEmpty("head-repo", c.HeadRepoSlug). AddIfNotEmpty("base_branch", c.BaseBranch). From c4b66700789f0fce3e1501fd20cc6f7e7b60ae5f Mon Sep 17 00:00:00 2001 From: pelikhan Date: Sun, 18 Oct 2026 19:52:21 -0700 Subject: [PATCH 7/7] Reuse shared Git execution for bounded bundle validation Remove the handler-local child process executor and getExecOutput replacement. Share structured Git process execution with execGitSync while preserving bounded offline discovery, scrubbed optional diagnostics, and conflict-status handling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../js/bundle_transport.integration.test.cjs | 21 ++-- actions/setup/js/git_helpers.cjs | 49 ++++++---- actions/setup/js/git_helpers.test.cjs | 20 ++++ .../setup/js/push_to_pull_request_branch.cjs | 96 ++++++++----------- ...o_pull_request_branch.integration.test.cjs | 9 +- .../js/push_to_pull_request_branch.test.cjs | 6 +- ...o_pull_request_branch_comparisons.test.cjs | 42 +++++--- 7 files changed, 144 insertions(+), 99 deletions(-) diff --git a/actions/setup/js/bundle_transport.integration.test.cjs b/actions/setup/js/bundle_transport.integration.test.cjs index 2151b829564..0be6b2a913c 100644 --- a/actions/setup/js/bundle_transport.integration.test.cjs +++ b/actions/setup/js/bundle_transport.integration.test.cjs @@ -5,8 +5,9 @@ import http from "http"; import os from "os"; import path from "path"; -const { ensureFullHistoryForBundle, importLocalBundle, offlineGitOptions } = require("./git_helpers.cjs"); -const { resolveTrustedBaseTip, resolveBundleTrustedBase, getBundleValidationBase, getBundlePreApplyFiles, boundedValidationExec } = require("./push_to_pull_request_branch.cjs"); +const gitHelpers = require("./git_helpers.cjs"); +const { ensureFullHistoryForBundle, importLocalBundle, offlineGitOptions } = gitHelpers; +const { resolveTrustedBaseTip, resolveBundleTrustedBase, getBundleValidationBase, getBundlePreApplyFiles } = require("./push_to_pull_request_branch.cjs"); const { checkFileProtectionPostApply } = require("./manifest_file_helpers.cjs"); const gitEnv = () => { @@ -28,6 +29,10 @@ function execApi(cwd, { emulateOldGit = false } = {}) { const calls = []; const api = { calls, + async execGitSyncResult(args, options) { + const result = await api.getExecOutput("git", args, { ...options, silent: true, ignoreReturnCode: true }); + return { ...result, status: result.exitCode }; + }, async getExecOutput(command, args, options = {}) { calls.push(args); const env = { ...gitEnv(), ...options.env }; @@ -212,10 +217,10 @@ describe("offline bundle transport with real Git", () => { const before = packInventory(shallow); const compareCommits = vi.fn(async () => ({ data: { status: "behind" } })); const options = { cwd: shallow, env: gitEnv() }; - expect(await resolveTrustedBaseTip(boundedValidationExec, options, "main")).toBe(trustedTip); - const recognized = await resolveBundleTrustedBase(boundedValidationExec, options, oldHead, "refs/bundles/test", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }, true); + expect(await resolveTrustedBaseTip(gitHelpers, options, "main")).toBe(trustedTip); + const recognized = await resolveBundleTrustedBase(gitHelpers, options, oldHead, "refs/bundles/test", trustedTip, { rest: { repos: { compareCommits } } }, { owner: "test-owner", repo: "test-repo" }, true); expect(recognized).toBe(mergedBase); - await getBundleValidationBase(boundedValidationExec, options, oldHead, "refs/bundles/test", recognized); + await getBundleValidationBase(gitHelpers, options, oldHead, "refs/bundles/test", recognized); expect(compareCommits).toHaveBeenCalledTimes(1); expect(exec.calls.some(args => args[0] === "fetch")).toBe(false); @@ -223,11 +228,11 @@ describe("offline bundle transport with real Git", () => { expect(fs.readFileSync(shallowPath, "utf8")).toBe(shallowFile); expect(packInventory(shallow)).toEqual(before); expect(fs.readdirSync(shallow)).toEqual([".git"]); - const absent = await boundedValidationExec.getExecOutput("git", ["cat-file", "-e", trustedTip], offlineGitOptions(options)); - expect(absent.exitCode).not.toBe(0); + const absent = gitHelpers.execGitSyncResult(["cat-file", "-e", trustedTip], offlineGitOptions(options)); + expect(absent.status).not.toBe(0); if (allowFilter) { const blob = git(source, ["rev-parse", `${oldHead}:shared.txt`]); - expect((await boundedValidationExec.getExecOutput("git", ["cat-file", "-e", blob], offlineGitOptions(options))).exitCode).not.toBe(0); + expect(gitHelpers.execGitSyncResult(["cat-file", "-e", blob], offlineGitOptions(options)).status).not.toBe(0); } expect(packInventory(shallow)).toEqual(before); }); diff --git a/actions/setup/js/git_helpers.cjs b/actions/setup/js/git_helpers.cjs index eef4fd0dec7..c391a2adf3b 100644 --- a/actions/setup/js/git_helpers.cjs +++ b/actions/setup/js/git_helpers.cjs @@ -1,7 +1,7 @@ // @ts-check /// -const { spawnSync } = require("child_process"); +const childProcess = require("child_process"); const { ERR_SYSTEM } = require("./error_codes.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { isTransientError } = require("./error_recovery.cjs"); @@ -81,6 +81,35 @@ function ensureSafeDirectoryTrust(gitCwd, logger) { debug?.(`Configured git safe.directory for bridge context: ${gitCwd}`); } +/** + * Execute Git without logging command output, preserving ordinary exit statuses. + * Spawn failures discard partial output; callers must check status before use. + * + * @param {string[]} args + * @param {Object} [options] + * @returns {import("child_process").SpawnSyncReturns} + */ +function execGitSyncResult(args, options = {}) { + const result = childProcess.spawnSync("git", args, { + encoding: "utf8", + maxBuffer: 100 * 1024 * 1024, + timeout: 60_000, + killSignal: "SIGKILL", + ...options, + stdio: options.stdio === "inherit" ? "pipe" : options.stdio, + env: { + ...(options.env || process.env), + GIT_TERMINAL_PROMPT: "0", + GCM_INTERACTIVE: "Never", + GIT_ASKPASS: "/bin/echo", + }, + }); + if (result.error || result.signal || result.status === null) { + return { ...result, status: null, stdout: "", stderr: "" }; + } + return result; +} + /** * Safely execute git command using spawnSync with args array to prevent shell injection. * @@ -119,24 +148,9 @@ function execGitSync(args, options = {}) { // credential rejection fail fast instead of opening an interactive prompt. // - timeout (default 60s) ensures a stuck network/TLS handshake cannot // wedge the calling event loop. Callers can override via options.timeout. - const callerEnv = spawnOptions.env || process.env; - const safeEnv = { - ...callerEnv, - GIT_TERMINAL_PROMPT: "0", - GCM_INTERACTIVE: "Never", - GIT_ASKPASS: "/bin/echo", - }; const defaultTimeoutMs = 60_000; - const result = spawnSync("git", args, { - encoding: "utf8", - maxBuffer: 100 * 1024 * 1024, // 100 MB — prevents ENOBUFS on large diffs (e.g. git format-patch) - timeout: defaultTimeoutMs, - killSignal: "SIGKILL", - ...spawnOptions, - stdio: spawnOptions.stdio === "inherit" ? "pipe" : spawnOptions.stdio, - env: safeEnv, - }); + const result = execGitSyncResult(args, spawnOptions); if (result.error) { // Detect ENOBUFS (buffer overflow) and provide a more actionable message @@ -873,6 +887,7 @@ module.exports = { isTransientGitError, withGitRetry, execGitSync, + execGitSyncResult, backfillCommitObjects, checkImplausibleShallowRange, ensureFullHistoryForBundle, diff --git a/actions/setup/js/git_helpers.test.cjs b/actions/setup/js/git_helpers.test.cjs index 6714e2b2590..c334c457721 100644 --- a/actions/setup/js/git_helpers.test.cjs +++ b/actions/setup/js/git_helpers.test.cjs @@ -43,6 +43,26 @@ describe("git_helpers.cjs", () => { }); describe("execGitSync", () => { + it("exposes structured Git results without logging remote diagnostics", async () => { + const { execGitSyncResult } = await import("./git_helpers.cjs"); + const logger = { debug: vi.fn(), error: vi.fn() }; + global.core = logger; + const result = execGitSyncResult(["invalid-command"]); + expect(result.status).not.toBe(0); + expect(result.stderr).toContain("invalid-command"); + expect(logger.debug).not.toHaveBeenCalled(); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it("discards partial structured output on process failure", async () => { + const { execGitSyncResult } = await import("./git_helpers.cjs"); + const result = execGitSyncResult(["--version"], { maxBuffer: 1 }); + expect(result.status).toBeNull(); + expect(result.error.code).toBe("ENOBUFS"); + expect(result.stdout).toBe(""); + expect(result.stderr).toBe(""); + }); + it("should export execGitSync function", async () => { const { execGitSync } = await import("./git_helpers.cjs"); expect(typeof execGitSync).toBe("function"); diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index fe2fdb38b34..226f417a925 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -7,7 +7,6 @@ const { createReportIssue } = require("./issue_report.cjs"); const fs = require("fs"); const { withTransportIntegrity } = require("./transport_integrity.cjs"); const path = require("path"); -const childProcess = require("child_process"); const { generateStagedPreview } = require("./staged_preview.cjs"); const { isStagedMode } = require("./safe_output_helpers.cjs"); const { pushSignedCommits } = require("./push_signed_commits.cjs"); @@ -24,7 +23,8 @@ const { ERR_SYSTEM, POLICY_FILE_PROTECTION_DENIED_REASON_CODE } = require("./err const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); const { renderTemplateFromFile, buildProtectedFileList, getPromptPath } = require("./messages_core.cjs"); const { withGitHubHostToken } = require("./git_auth_helpers.cjs"); -const { ensureFullHistoryForBundle, importLocalBundle, offlineGitOptions, linearizeRangeAsCommit, ensureSafeDirectoryTrust } = require("./git_helpers.cjs"); +const gitHelpers = require("./git_helpers.cjs"); +const { ensureFullHistoryForBundle, importLocalBundle, offlineGitOptions, linearizeRangeAsCommit, ensureSafeDirectoryTrust } = gitHelpers; const { extractPatchBaseCommit } = require("./commit_sha_helpers.cjs"); const { findRepoCheckout } = require("./find_repo_checkout.cjs"); const { getThreatWarningPresentation } = require("./threat_detection_warning.cjs"); @@ -43,32 +43,12 @@ const MAX_BUNDLE_HISTORY_COMMITS = 100; const BUNDLE_VALIDATION_TIMEOUT_MS = 5000; const BUNDLE_VALIDATION_MAX_OUTPUT = 1024 * 1024; -// @actions/exec has no process timeout or output limit. Keep optional baseline -// discovery separate from bundle recovery, and discard failed command diagnostics. -const boundedValidationExec = { - /** - * @param {string} command - * @param {string[]} args - * @param {import("@actions/exec").ExecOptions} [options] - */ - async getExecOutput(command, args, options = {}) { - if (command !== "git") throw new Error(`${ERR_SYSTEM}: Bundle validation only supports git commands`); - const result = childProcess.spawnSync(command, args, { - cwd: options.cwd, - env: options.env, - encoding: "utf8", - timeout: BUNDLE_VALIDATION_TIMEOUT_MS, - maxBuffer: BUNDLE_VALIDATION_MAX_OUTPUT, - killSignal: "SIGKILL", - stdio: "pipe", - }); - if (result.error || result.signal || result.status === null) { - core.debug("Bundle validation: bounded Git command failed or exceeded its execution/output limit"); - return { exitCode: 128, stdout: "", stderr: "" }; - } - return { exitCode: result.status, stdout: result.stdout, stderr: "" }; - }, -}; +/** + * @param {Record} options + */ +function validationGitOptions(options) { + return { cwd: options.cwd, env: options.env, timeout: BUNDLE_VALIDATION_TIMEOUT_MS, maxBuffer: BUNDLE_VALIDATION_MAX_OUTPUT, killSignal: "SIGKILL", stdio: "pipe" }; +} const MISSING_BRANCH_ERROR_TEMPLATE = branchName => `Branch ${branchName} no longer exists on origin (it may have been deleted), can't push to it.`; const MISSING_REMOTE_REF_PATTERNS = [ "couldn't find remote ref", @@ -165,30 +145,30 @@ async function getBundlePreApplyFiles(exec, gitOptions, rangeBaseRef, bundleRef, * markers remain in the tree. Track conflicted paths too: binary and modify/delete * conflicts may keep one side unchanged without inserting conflict markers. * - * @param {{ getExecOutput: Function }} exec + * @param {{ execGitSyncResult: Function }} git * @param {Record} gitOptions * @param {string} oldHead * @param {string} candidate * @param {string} trustedBase * @returns {Promise<{ ref: string, conflictedFiles: string[] }>} */ -async function getBundleValidationBase(exec, gitOptions, oldHead, candidate, trustedBase) { +async function getBundleValidationBase(git, gitOptions, oldHead, candidate, trustedBase) { const fallback = { ref: oldHead, conflictedFiles: [] }; if (!trustedBase) return fallback; try { - const opts = { ...offlineGitOptions(gitOptions), ignoreReturnCode: true }; - const oldAncestor = await exec.getExecOutput("git", ["merge-base", "--is-ancestor", oldHead, candidate], opts); - const baseAncestor = await exec.getExecOutput("git", ["merge-base", "--is-ancestor", trustedBase, candidate], opts); - const alreadyMerged = await exec.getExecOutput("git", ["merge-base", "--is-ancestor", trustedBase, oldHead], opts); - if (oldAncestor.exitCode !== 0 || baseAncestor.exitCode !== 0 || alreadyMerged.exitCode !== 1) return fallback; + const opts = validationGitOptions(offlineGitOptions(gitOptions)); + const oldAncestor = await git.execGitSyncResult(["merge-base", "--is-ancestor", oldHead, candidate], opts); + const baseAncestor = await git.execGitSyncResult(["merge-base", "--is-ancestor", trustedBase, candidate], opts); + const alreadyMerged = await git.execGitSyncResult(["merge-base", "--is-ancestor", trustedBase, oldHead], opts); + if (oldAncestor.status !== 0 || baseAncestor.status !== 0 || alreadyMerged.status !== 1) return fallback; - const merge = await exec.getExecOutput("git", ["merge-tree", "--write-tree", "--name-only", "-z", oldHead, trustedBase], opts); + const merge = await git.execGitSyncResult(["merge-tree", "--write-tree", "--name-only", "-z", oldHead, trustedBase], opts); const [tree, ...fields] = merge.stdout.split("\0"); - if ((merge.exitCode === 0 || merge.exitCode === 1) && /^[0-9a-f]{40}$/.test(tree)) { + if ((merge.status === 0 || merge.status === 1) && /^[0-9a-f]{40}$/.test(tree)) { const separatorIndex = fields.indexOf(""); return { ref: tree, - conflictedFiles: merge.exitCode === 1 ? fields.slice(0, separatorIndex === -1 ? fields.length : separatorIndex) : [], + conflictedFiles: merge.status === 1 ? fields.slice(0, separatorIndex === -1 ? fields.length : separatorIndex) : [], }; } core.warning("Unable to compute trusted base merge tree; using PR-head validation"); @@ -204,25 +184,25 @@ async function getBundleValidationBase(exec, gitOptions, oldHead, candidate, tru * The target checkout's origin credentials must remain in effect, not the fork * token temporarily installed for PR-head operations. * - * @param {{ getExecOutput: Function }} exec + * @param {{ execGitSyncResult: Function }} git * @param {Object} gitOptions * @param {unknown} baseRef * @returns {Promise} */ -async function resolveTrustedBaseTip(exec, gitOptions, baseRef) { +async function resolveTrustedBaseTip(git, gitOptions, baseRef) { if (typeof baseRef !== "string" || !baseRef) { core.debug("Bundle validation: base branch metadata unavailable; using PR-head validation"); return ""; } const ref = `refs/heads/${baseRef}`; - const opts = { ...gitOptions, env: { ...process.env, ...gitOptions.env, GIT_TERMINAL_PROMPT: "0", GCM_INTERACTIVE: "Never", GIT_ASKPASS: "/bin/echo" }, silent: true, ignoreReturnCode: true }; + const opts = validationGitOptions({ ...gitOptions, env: { ...process.env, ...gitOptions.env } }); try { - const valid = await exec.getExecOutput("git", ["check-ref-format", ref], { ...offlineGitOptions(gitOptions), ignoreReturnCode: true }); - if (valid.exitCode !== 0) throw new Error("Invalid base ref"); - const result = await exec.getExecOutput("git", ["ls-remote", "--refs", "--exit-code", "origin", ref], opts); + const valid = await git.execGitSyncResult(["check-ref-format", ref], validationGitOptions(offlineGitOptions(gitOptions))); + if (valid.status !== 0) throw new Error("Invalid base ref"); + const result = await git.execGitSyncResult(["ls-remote", "--refs", "--exit-code", "origin", ref], opts); const lines = result.stdout.trim().split("\n"); const fields = lines[0].split("\t"); - if (result.exitCode === 0 && lines.length === 1 && fields.length === 2 && /^[0-9a-f]{40}$/.test(fields[0]) && fields[1] === ref) { + if (result.status === 0 && lines.length === 1 && fields.length === 2 && /^[0-9a-f]{40}$/.test(fields[0]) && fields[1] === ref) { core.debug("Bundle validation: pinned trusted base branch tip from remote metadata"); return fields[0]; } @@ -239,7 +219,7 @@ async function resolveTrustedBaseTip(exec, gitOptions, baseRef) { * tip can exempt protected files or history. Topological order prefers newer * parents without relying on timestamps. All object inspection stays offline. * - * @param {{ getExecOutput: Function }} exec + * @param {{ execGitSyncResult: Function }} git * @param {Record} gitOptions * @param {string} oldHead * @param {string} candidate @@ -248,19 +228,19 @@ async function resolveTrustedBaseTip(exec, gitOptions, baseRef) { * @param {{ owner: string, repo: string }} repoParts * @returns {Promise} */ -async function resolveBundleTrustedBase(exec, gitOptions, oldHead, candidate, trustedTip, githubClient, repoParts, allowMergedBaseBaseline = false) { +async function resolveBundleTrustedBase(git, gitOptions, oldHead, candidate, trustedTip, githubClient, repoParts, allowMergedBaseBaseline = false) { if (!trustedTip) return ""; - const opts = { ...offlineGitOptions(gitOptions), ignoreReturnCode: true }; + const opts = validationGitOptions(offlineGitOptions(gitOptions)); try { - const tipAncestor = await exec.getExecOutput("git", ["merge-base", "--is-ancestor", trustedTip, candidate], opts); - if (tipAncestor.exitCode === 0) return trustedTip; + const tipAncestor = await git.execGitSyncResult(["merge-base", "--is-ancestor", trustedTip, candidate], opts); + if (tipAncestor.status === 0) return trustedTip; if (!allowMergedBaseBaseline) { core.debug("Bundle validation: merged base ancestor discovery is disabled"); return ""; } - const merges = await exec.getExecOutput("git", ["rev-list", "--merges", "--parents", `--max-count=${MAX_BUNDLE_HISTORY_COMMITS + 1}`, `${oldHead}..${candidate}`], opts); - if (merges.exitCode !== 0) return ""; + const merges = await git.execGitSyncResult(["rev-list", "--merges", "--parents", `--max-count=${MAX_BUNDLE_HISTORY_COMMITS + 1}`, `${oldHead}..${candidate}`], opts); + if (merges.status !== 0) return ""; const mergeLines = merges.stdout.trim().split("\n").filter(Boolean); if (mergeLines.length > MAX_BUNDLE_HISTORY_COMMITS || mergeLines.some(line => line.split(" ").length > 3)) { core.debug("Bundle validation: merge history exceeds the discovery budget"); @@ -268,8 +248,8 @@ async function resolveBundleTrustedBase(exec, gitOptions, oldHead, candidate, tr } const parents = new Set(mergeLines.flatMap(line => line.split(" ").slice(1)).filter(sha => /^[0-9a-f]{40}$/.test(sha))); if (!parents.size) return ""; - const ordered = await exec.getExecOutput("git", ["rev-list", "--topo-order", `--max-count=${MAX_BUNDLE_HISTORY_COMMITS + 1}`, ...parents, "--not", oldHead], opts); - if (ordered.exitCode !== 0) return ""; + const ordered = await git.execGitSyncResult(["rev-list", "--topo-order", `--max-count=${MAX_BUNDLE_HISTORY_COMMITS + 1}`, ...parents, "--not", oldHead], opts); + if (ordered.status !== 0) return ""; const orderedParents = ordered.stdout.trim().split("\n").filter(Boolean); if (orderedParents.length > MAX_BUNDLE_HISTORY_COMMITS) { core.debug("Bundle validation: parent history exceeds the discovery budget"); @@ -1234,7 +1214,7 @@ async function main(config = {}) { core.info(`Applying changes from bundle: ${bundleFilePath}`); const bundleRef = `refs/bundles/push-${branchName.replace(/[^a-zA-Z0-9-]/g, "-")}`; try { - const trustedTip = signedCommits ? "" : await resolveTrustedBaseTip(boundedValidationExec, baseGitOpts, pullRequest.base?.ref); + const trustedTip = signedCommits ? "" : await resolveTrustedBaseTip(gitHelpers, baseGitOpts, pullRequest.base?.ref); const bundleGitOpts = offlineGitOptions(baseGitOpts); await ensureFullHistoryForBundle( exec, @@ -1252,8 +1232,8 @@ async function main(config = {}) { await importLocalBundle(exec, bundleFilePath, `refs/heads/${branchName}`, bundleRef, baseGitOpts); core.info(`Imported local bundle to ${bundleRef}`); - const trustedBase = await resolveBundleTrustedBase(boundedValidationExec, baseGitOpts, rangeBaseRef, bundleRef, trustedTip, githubClient, repoParts, allowMergedBaseBaseline); - const validationBase = await getBundleValidationBase(boundedValidationExec, baseGitOpts, rangeBaseRef, bundleRef, trustedBase); + const trustedBase = await resolveBundleTrustedBase(gitHelpers, baseGitOpts, rangeBaseRef, bundleRef, trustedTip, githubClient, repoParts, allowMergedBaseBaseline); + const validationBase = await getBundleValidationBase(gitHelpers, baseGitOpts, rangeBaseRef, bundleRef, trustedBase); validationBaseRef = validationBase.ref; validationConflictFiles = validationBase.conflictedFiles; const trustedBaseInValidationBase = validationBaseRef !== rangeBaseRef; @@ -1944,4 +1924,4 @@ async function main(config = {}) { }); } -module.exports = { main, HANDLER_TYPE, getBundlePreApplyFiles, getBundleValidationBase, resolveTrustedBaseTip, resolveBundleTrustedBase, boundedValidationExec }; +module.exports = { main, HANDLER_TYPE, getBundlePreApplyFiles, getBundleValidationBase, resolveTrustedBaseTip, resolveBundleTrustedBase }; diff --git a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs index fc355ffb791..e7dc6685bcd 100644 --- a/actions/setup/js/push_to_pull_request_branch.integration.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.integration.test.cjs @@ -46,6 +46,10 @@ function writeRepoFile(repoDir, relativePath, content) { function createExecApi(cwd) { const api = { + async execGitSyncResult(args, options) { + const result = await api.getExecOutput("git", args, { ...options, silent: true, ignoreReturnCode: true }); + return { ...result, status: result.exitCode }; + }, async getExecOutput(command, args = [], options = {}) { if (command !== "git") { throw new Error(`unexpected command: ${command}`); @@ -312,9 +316,10 @@ describe("push_to_pull_request_branch bundle integration", () => { }; delete require.cache[require.resolve("./push_to_pull_request_branch.cjs")]; const module = require("./push_to_pull_request_branch.cjs"); - const boundedOutput = module.boundedValidationExec.getExecOutput; + const gitHelpers = require("./git_helpers.cjs"); + const boundedOutput = gitHelpers.execGitSyncResult; // Match the fixture's exec API, which defaults all Git calls to targetRepo. - boundedOutputSpy = vi.spyOn(module.boundedValidationExec, "getExecOutput").mockImplementation((command, args, options) => boundedOutput(command, args, { ...options, cwd: targetRepo })); + boundedOutputSpy = vi.spyOn(gitHelpers, "execGitSyncResult").mockImplementation((args, options) => boundedOutput(args, { ...options, cwd: targetRepo })); const handler = await module.main({ signed_commits: false, check_branch_protection: false, 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 73c47e899ef..64a218fe3e6 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -217,6 +217,10 @@ describe("push_to_pull_request_branch.cjs", () => { graphql: vi.fn(), }; + mockExec.execGitSyncResult = async (args, options) => { + const result = await mockExec.getExecOutput("git", args, { ...options, silent: true, ignoreReturnCode: true }); + return { ...result, status: result.exitCode }; + }; global.core = mockCore; global.exec = mockExec; global.context = mockContext; @@ -258,7 +262,7 @@ describe("push_to_pull_request_branch.cjs", () => { async function loadModule() { // Module loads when imported const module = require("./push_to_pull_request_branch.cjs"); - vi.spyOn(module.boundedValidationExec, "getExecOutput").mockImplementation((command, args, options) => mockExec.getExecOutput(command, args, options)); + vi.spyOn(require("./git_helpers.cjs"), "execGitSyncResult").mockImplementation((args, options) => mockExec.execGitSyncResult(args, options)); return module; } diff --git a/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs b/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs index 027c452b8ed..35c818680a8 100644 --- a/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch_comparisons.test.cjs @@ -18,6 +18,10 @@ describe("bundle merge-parent comparison budget", () => { mockCore = { debug: vi.fn(), warning: vi.fn() }; vi.stubGlobal("core", mockCore); mockExec = { + async execGitSyncResult(args, options) { + const result = await mockExec.getExecOutput("git", args, { ...options, silent: true, ignoreReturnCode: true }); + return { ...result, status: result.exitCode }; + }, exec: vi.fn(), getExecOutput: vi.fn(async (_command, args) => { if (args[0] === "merge-base") return { exitCode: 128, stdout: "", stderr: secretDiagnostic }; @@ -150,7 +154,8 @@ describe("bundle merge-parent comparison budget", () => { describe("bounded optional validation Git", () => { const childProcess = require("child_process"); - const { boundedValidationExec, resolveTrustedBaseTip, resolveBundleTrustedBase, getBundleValidationBase } = require("./push_to_pull_request_branch.cjs"); + const gitHelpers = require("./git_helpers.cjs"); + const { resolveTrustedBaseTip, resolveBundleTrustedBase, getBundleValidationBase } = require("./push_to_pull_request_branch.cjs"); let spawn; let realSpawn; let debug; @@ -169,30 +174,41 @@ describe("bounded optional validation Git", () => { it("enforces process and output bounds that caller options cannot relax", async () => { spawn.mockReturnValue({ status: 0, stdout: "git output", stderr: "private diagnostic" }); - expect(await boundedValidationExec.getExecOutput("git", ["rev-list", "HEAD"], { cwd: "/checkout", env: { GIT_ALLOW_PROTOCOL: "" }, timeout: 0, maxBuffer: Infinity })).toEqual({ exitCode: 0, stdout: "git output", stderr: "" }); - expect(spawn).toHaveBeenCalledWith("git", ["rev-list", "HEAD"], expect.objectContaining({ cwd: "/checkout", env: { GIT_ALLOW_PROTOCOL: "" }, timeout: 5000, maxBuffer: 1024 * 1024, killSignal: "SIGKILL", stdio: "pipe" })); + expect(await resolveTrustedBaseTip(gitHelpers, { cwd: "/checkout", env: { GIT_ALLOW_PROTOCOL: "" }, timeout: 0, maxBuffer: Infinity, shell: true }, "main")).toBe(""); + expect(spawn).toHaveBeenCalledWith( + "git", + ["ls-remote", "--refs", "--exit-code", "origin", "refs/heads/main"], + expect.objectContaining({ cwd: "/checkout", env: expect.objectContaining({ GIT_ALLOW_PROTOCOL: "" }), timeout: 5000, maxBuffer: 1024 * 1024, killSignal: "SIGKILL", stdio: "pipe" }) + ); + expect(spawn.mock.calls.every(([, , options]) => options.shell !== true)).toBe(true); + expect(JSON.stringify(debug.mock.calls)).not.toContain("private"); }); - it.each(["ETIMEDOUT", "ENOBUFS", "ENOENT", "SIGKILL"])("discards incomplete output and falls back after %s", async failure => { + it.each(["ETIMEDOUT", "ENOBUFS", "ENOENT", "SIGKILL", "SIGINT"])("discards incomplete output and falls back after %s", async failure => { spawn.mockReturnValue({ - status: failure === "SIGKILL" ? null : 0, - signal: failure === "SIGKILL" ? failure : null, - error: failure === "SIGKILL" ? undefined : Object.assign(new Error("private diagnostic"), { code: failure }), + status: failure.startsWith("SIG") ? null : 0, + signal: failure.startsWith("SIG") ? failure : null, + error: failure.startsWith("SIG") ? undefined : Object.assign(new Error("private diagnostic"), { code: failure }), stdout: "partial private output", stderr: "private diagnostic", }); const compareCommits = vi.fn(); const client = { rest: { repos: { compareCommits } } }; - expect(await resolveTrustedBaseTip(boundedValidationExec, {}, "main")).toBe(""); - expect(await resolveBundleTrustedBase(boundedValidationExec, {}, "old", "candidate", "a".repeat(40), client, { owner: "owner", repo: "repo" }, true)).toBe(""); - expect(await getBundleValidationBase(boundedValidationExec, {}, "old", "candidate", "a".repeat(40))).toEqual({ ref: "old", conflictedFiles: [] }); + expect(await resolveTrustedBaseTip(gitHelpers, {}, "main")).toBe(""); + expect(await resolveBundleTrustedBase(gitHelpers, {}, "old", "candidate", "a".repeat(40), client, { owner: "owner", repo: "repo" }, true)).toBe(""); + expect(await getBundleValidationBase(gitHelpers, {}, "old", "candidate", "a".repeat(40))).toEqual({ ref: "old", conflictedFiles: [] }); expect(compareCommits).not.toHaveBeenCalled(); expect(JSON.stringify(debug.mock.calls)).not.toContain("private"); }); it("preserves merge-tree conflict output for conservative file protection", async () => { - spawn.mockReturnValue({ status: 1, stdout: `${"a".repeat(40)}\0package.json\0\0`, stderr: "private diagnostic" }); - expect(await boundedValidationExec.getExecOutput("git", ["merge-tree"])).toEqual({ exitCode: 1, stdout: `${"a".repeat(40)}\0package.json\0\0`, stderr: "" }); + spawn + .mockReturnValueOnce({ status: 0, stdout: "", stderr: "" }) + .mockReturnValueOnce({ status: 0, stdout: "", stderr: "" }) + .mockReturnValueOnce({ status: 1, stdout: "", stderr: "" }) + .mockReturnValue({ status: 1, stdout: `${"a".repeat(40)}\0package.json\0\0`, stderr: "private diagnostic" }); + expect(await getBundleValidationBase(gitHelpers, {}, "old", "candidate", "base")).toEqual({ ref: "a".repeat(40), conflictedFiles: ["package.json"] }); + expect(JSON.stringify(debug.mock.calls)).not.toContain("private"); }); it.each([ @@ -204,7 +220,7 @@ describe("bounded optional validation Git", () => { result = realSpawn(process.execPath, ["-e", script], options); return result; }); - expect(await boundedValidationExec.getExecOutput("git", ["rev-list", "HEAD"])).toEqual({ exitCode: 128, stdout: "", stderr: "" }); + expect(await resolveTrustedBaseTip(gitHelpers, {}, "main")).toBe(""); expect(result.error.code).toBe(code); expect(result.signal).toBe("SIGKILL"); });