From 5b3252fff1b6c6329d68c216aba1030de1ee9228 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 14 Oct 2026 15:00:26 +0000 Subject: [PATCH 1/7] Enforce validated safe-output patch and bundle integrity at runtime Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- actions/setup/js/create_pull_request.cjs | 80 ++++-- actions/setup/js/create_pull_request.test.cjs | 117 ++++---- actions/setup/js/extra_empty_commit.cjs | 7 +- actions/setup/js/push_signed_commits.cjs | 16 +- .../setup/js/push_to_pull_request_branch.cjs | 48 +++- .../js/push_to_pull_request_branch.test.cjs | 51 ++-- actions/setup/js/transport_integrity.cjs | 159 +++++++++++ actions/setup/js/transport_integrity.test.cjs | 187 ++++++++++++ .../js/transport_integrity_handlers.test.cjs | 265 ++++++++++++++++++ 9 files changed, 813 insertions(+), 117 deletions(-) create mode 100644 actions/setup/js/transport_integrity.cjs create mode 100644 actions/setup/js/transport_integrity.test.cjs create mode 100644 actions/setup/js/transport_integrity_handlers.test.cjs diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 067234a8a67..21969dfc300 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -42,6 +42,7 @@ const { findAgent, getIssueDetails, assignAgentToIssue } = require("./assign_age const { ensureFullHistoryForBundle, extractBundlePrerequisiteCommits, getBundlePrerequisites, isShallowOrSparseCheckout, linearizeRangeAsCommit } = require("./git_helpers.cjs"); const { parseDiffGitHeader: parseDiffGitHeaderPaths, extractDiffGitHeaderEntries } = require("./patch_path_helpers.cjs"); const { resolveTransportPaths } = require("./resolve_transport_paths.cjs"); +const { withTransportIntegrity } = require("./transport_integrity.cjs"); const { resolveDefaultMentions, resolveMentionsForItem } = require("./resolve_mentions_from_payload.cjs"); const { MANAGED_FALLBACK_ISSUE_LABEL, @@ -451,9 +452,10 @@ async function rewriteBundleBranchAsSingleCommit(baseBranch, execApi, bundleFile * @param {string} body - Issue body * @param {string[]} labels - Issue labels * @param {string[] | null} assignees - Sanitized assignees (null = omit field) + * @param {() => void} assertUnchanged - Check captured transport before each publication attempt * @returns {Promise<{data: any, issueRepoParts: {owner: string, repo: string}}>} */ -async function createFallbackIssue(githubClient, repoParts, title, body, labels, assignees) { +async function createFallbackIssue(githubClient, repoParts, title, body, labels, assignees, assertUnchanged) { body = appendExecutionProvenance(body, process.env.GH_AW_WORKFLOW_NAME || "Workflow", ""); if (body.length > MAX_GITHUB_BODY_LENGTH) { throw new Error(`Fallback issue body exceeds GitHub's maximum length of ${MAX_GITHUB_BODY_LENGTH} characters`); @@ -485,9 +487,11 @@ async function createFallbackIssue(githubClient, repoParts, title, body, labels, const triedOwnerRepos = new Set(); const innerCreate = async () => { try { + assertUnchanged(); const response = await githubClient.rest.issues.create(payload); return { ...response, issueRepoParts: { owner: payload.owner, repo: payload.repo } }; } catch (error) { + assertUnchanged(); const status = typeof error === "object" && error !== null && "status" in error ? error.status : undefined; const message = getErrorMessage(error).toLowerCase(); const isAssigneeError = status === 422 && (message.includes("assignee") || message.includes("assignees") || message.includes("unprocessable")); @@ -662,6 +666,7 @@ function enforcePullRequestLimits(patchContent, maxFiles = MAX_FILES) { * @param {string} [options.remoteTarget] - Remote name or URL used for remote branch existence checks. * @param {string} [options.remoteToken] - Optional token used for authenticated remote branch checks. * @param {string} [options.cwd] - Optional working directory for git operations; scopes git config overrides to the correct checkout. + * @param {() => void} [options.assertTransportUnchanged] * @returns {Promise} The (possibly renamed) branch name to use going forward. */ async function handleRemoteBranchCollision(branchName, preserveBranchName, options = {}) { @@ -676,11 +681,13 @@ async function handleRemoteBranchCollision(branchName, preserveBranchName, optio } else { checkResult = await checkRemoteBranch(); } + options.assertTransportUnchanged?.(); const { stdout } = checkResult; if (stdout.trim()) { remoteBranchExists = true; } } catch (checkError) { + options.assertTransportUnchanged?.(); core.info(`Remote branch check failed (non-fatal): ${getErrorMessage(checkError)}`); } @@ -713,9 +720,11 @@ async function handleRemoteBranchCollision(branchName, preserveBranchName, optio core.warning(`Remote branch ${branchName} already exists - reusing it (recreate-ref enabled, force-deleting remote ref)`); let deleteBlocked = false; try { + options.assertTransportUnchanged?.(); await githubClient.rest.git.deleteRef({ owner, repo, ref: `heads/${branchName}` }); core.info(`Deleted remote branch ${branchName} to reuse it`); } catch (deleteError) { + options.assertTransportUnchanged?.(); /** @type {any} */ const err = deleteError; const status = err && typeof err === "object" ? err.status : undefined; @@ -982,7 +991,8 @@ async function main(config = {}) { * @param {Object} resolvedTemporaryIds - Map of temporary IDs to {repo, number} * @returns {Promise} Result with success/error status and PR details */ - return async function handleCreatePullRequest(message, resolvedTemporaryIds) { + return withTransportIntegrity(async function handleCreatePullRequest(message, resolvedTemporaryIds, transport) { + const exec = transport.guardExec(globalThis.exec); // Check if we've hit the max limit if (processedCount >= maxCount) { core.warning(`Skipping create_pull_request: max count of ${maxCount} reached`); @@ -1009,6 +1019,8 @@ async function main(config = {}) { // the entry it writes, but the validation step strips them as a defense // against agent-forged values. Recover them by re-deriving from `branch`. const transportPaths = resolveTransportPaths(pullRequestItem, defaultTargetRepo); + transport.captureOptional(transportPaths.patchPath); + transport.captureOptional(transportPaths.bundlePath); const patchFilePath = transportPaths.patchPath; core.info(`Patch file path: ${patchFilePath || "(not set)"}`); @@ -1234,6 +1246,7 @@ async function main(config = {}) { // Check if patch file exists and has valid content. // Always require patch content for policy enforcement, even when bundle transport // is used for apply-time commit transport. + transport.assertUnchanged(); const hasBundleFile = !!(bundleFilePath && fs.existsSync(bundleFilePath)); const applyTransport = hasBundleFile ? "bundle" : "patch"; core.info(`Apply transport mode: ${applyTransport} (bundle file present: ${hasBundleFile})`); @@ -1281,7 +1294,7 @@ async function main(config = {}) { let isEmpty = true; if (hasPatchFile) { try { - patchContent = fs.readFileSync(patchFilePath, "utf8"); + patchContent = transport.read(patchFilePath).toString("utf8"); } catch (err) { throw new Error(`Failed to read file ${patchFilePath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -1325,7 +1338,8 @@ async function main(config = {}) { }); try { - const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, fallbackTitle, fallbackBody, fallbackLabels, configAssignees); + transport.assertUnchanged(); + const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, fallbackTitle, fallbackBody, fallbackLabels, configAssignees, transport.assertUnchanged); core.info(`Created fallback issue #${issue.number}: ${issue.html_url}`); await assignCopilotToFallbackIssueIfEnabled(issueRepoParts.owner, issueRepoParts.repo, issue.number); await updateActivationComment(github, context, core, issue.html_url, issue.number, "issue", { activationCommentLinks }); @@ -1484,7 +1498,7 @@ async function main(config = {}) { if (patchFilePath && fs.existsSync(patchFilePath)) { let patchStats; try { - patchStats = fs.readFileSync(patchFilePath, "utf8"); + patchStats = transport.read(patchFilePath).toString("utf8"); } catch (err) { throw new Error(`Failed to read file ${patchFilePath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -1814,6 +1828,7 @@ async function main(config = {}) { try { await applyBundleToBranch(bundleFilePath, branchName, originalAgentBranch, exec, baseBranch); } catch (bundleError) { + transport.assertUnchanged(); core.error(`Failed to apply bundle: ${getErrorMessage(bundleError)}`); return { success: false, error: "Failed to apply bundle" }; } @@ -1825,7 +1840,9 @@ async function main(config = {}) { { const forkCwd = process.cwd(); const runBundlePush = async () => { + transport.assertUnchanged(); branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, { + assertTransportUnchanged: transport.assertUnchanged, recreateRef, githubClient: pushGithubClient, owner: pushRepoParts.owner, @@ -1835,7 +1852,9 @@ async function main(config = {}) { cwd: forkCwd, }); + transport.assertUnchanged(); await pushSignedCommits({ + assertTransportUnchanged: transport.assertUnchanged, githubClient: pushGithubClient, owner: pushRepoParts.owner, repo: pushRepoParts.repo, @@ -1863,6 +1882,7 @@ async function main(config = {}) { core.info("Could not count new commits - extra empty commit will be skipped"); } } catch (initialPushError) { + transport.assertUnchanged(); /** @type {unknown} */ let pushError = initialPushError; let pushRecovered = false; @@ -1875,8 +1895,10 @@ async function main(config = {}) { await rewriteBundleBranchAsSingleCommit(baseBranch, exec, bundleFilePath, { excludedFiles: Array.isArray(config.excluded_files) ? config.excluded_files : [], }); - const runRetryPush = async () => - pushSignedCommits({ + const runRetryPush = async () => { + transport.assertUnchanged(); + return pushSignedCommits({ + assertTransportUnchanged: transport.assertUnchanged, githubClient: pushGithubClient, owner: pushRepoParts.owner, repo: pushRepoParts.repo, @@ -1890,6 +1912,7 @@ async function main(config = {}) { currentRepo: itemRepo, validationConfig: config, }); + }; await runRetryPush(); core.info("Changes pushed to branch after bundle rewrite retry"); @@ -1902,6 +1925,7 @@ async function main(config = {}) { } pushRecovered = true; } catch (retryPushError) { + transport.assertUnchanged(); pushError = retryPushError; } } @@ -1969,7 +1993,8 @@ gh pr create --title ${shellQuote(title)} --base ${shellQuote(baseBranch)} --hea ${issueSafeFallbackFooter}`; try { - const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees); + transport.assertUnchanged(); + const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees, transport.assertUnchanged); core.info(`Created fallback issue #${issue.number}: ${issue.html_url}`); await assignCopilotToFallbackIssueIfEnabled(issueRepoParts.owner, issueRepoParts.repo, issue.number); @@ -2046,7 +2071,7 @@ ${issueSafeFallbackFooter}`; if (patchContent !== originalPatchContent) { core.info("Resolved temporary ID references in patch content"); try { - fs.writeFileSync(patchFilePath, patchContent, "utf8"); + transport.transform(patchFilePath, () => Buffer.from(patchContent, "utf8")); } catch (err) { throw new Error(`Failed to write file ${patchFilePath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -2071,6 +2096,7 @@ ${issueSafeFallbackFooter}`; core.info("Patch applied successfully"); patchApplied = true; } catch (patchError) { + transport.assertUnchanged(); core.error(`Failed to apply patch with --3way: ${getErrorMessage(patchError)}`); const recoveredFromAddAddConflict = await tryRecoverGitAmAddAddConflict(exec); @@ -2150,6 +2176,7 @@ ${issueSafeFallbackFooter}`; await capturePostApplyBaseRef(); await exec.exec("git", ["am", "--3way", patchFilePath]); } catch (fallbackPatchError) { + transport.assertUnchanged(); core.warning(`Fallback git am --3way failed: ${getErrorMessage(fallbackPatchError)}`); const recoveredFallback = await tryRecoverGitAmAddAddConflict(exec); if (!recoveredFallback.recovered) { @@ -2170,6 +2197,7 @@ ${issueSafeFallbackFooter}`; patchApplied = true; } } catch (fallbackError) { + transport.assertUnchanged(); core.warning(`Fallback to original base commit failed: ${getErrorMessage(fallbackError)}`); } } @@ -2220,7 +2248,9 @@ ${issueSafeFallbackFooter}`; { const forkCwd = process.cwd(); const runPatchPush = async () => { + transport.assertUnchanged(); branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, { + assertTransportUnchanged: transport.assertUnchanged, recreateRef, githubClient: pushGithubClient, owner: pushRepoParts.owner, @@ -2230,7 +2260,9 @@ ${issueSafeFallbackFooter}`; cwd: forkCwd, }); + transport.assertUnchanged(); await pushSignedCommits({ + assertTransportUnchanged: transport.assertUnchanged, githubClient: pushGithubClient, owner: pushRepoParts.owner, repo: pushRepoParts.repo, @@ -2260,6 +2292,7 @@ ${issueSafeFallbackFooter}`; core.info("Could not count new commits - extra empty commit will be skipped"); } } catch (pushError) { + transport.assertUnchanged(); // Push failed - create fallback issue instead of PR (if fallback is enabled) core.error(`Git push failed: ${getErrorMessage(pushError)}`); @@ -2290,7 +2323,7 @@ ${issueSafeFallbackFooter}`; if (patchFilePath && fs.existsSync(patchFilePath)) { let patchContent; try { - patchContent = fs.readFileSync(patchFilePath, "utf8"); + patchContent = transport.read(patchFilePath).toString("utf8"); } catch (err) { throw new Error(`Failed to read file ${patchFilePath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -2341,7 +2374,8 @@ ${patchPreview} ${issueSafeFallbackFooter}`; try { - const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees); + transport.assertUnchanged(); + const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees, transport.assertUnchanged); core.info(`Created fallback issue #${issue.number}: ${issue.html_url}`); await assignCopilotToFallbackIssueIfEnabled(issueRepoParts.owner, issueRepoParts.repo, issue.number); @@ -2401,7 +2435,9 @@ ${issueSafeFallbackFooter}`; const forkCwd = process.cwd(); const runEmptyPush = async () => { + transport.assertUnchanged(); branchName = await handleRemoteBranchCollision(branchName, preserveBranchName, { + assertTransportUnchanged: transport.assertUnchanged, recreateRef, githubClient: pushGithubClient, owner: pushRepoParts.owner, @@ -2411,7 +2447,9 @@ ${issueSafeFallbackFooter}`; cwd: forkCwd, }); + transport.assertUnchanged(); await pushSignedCommits({ + assertTransportUnchanged: transport.assertUnchanged, githubClient: pushGithubClient, owner: pushRepoParts.owner, repo: pushRepoParts.repo, @@ -2439,6 +2477,7 @@ ${issueSafeFallbackFooter}`; core.info("Could not count new commits - extra empty commit will be skipped"); } } catch (pushError) { + transport.assertUnchanged(); const error = `Failed to push empty branch: ${getErrorMessage(pushError)}`; core.error(error); return { @@ -2516,7 +2555,8 @@ ${issueSafeFallbackFooter}`; } try { - const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees); + transport.assertUnchanged(); + const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees, transport.assertUnchanged); core.info(`Created protected-file-protection review issue #${issue.number}: ${issue.html_url}`); @@ -2566,6 +2606,7 @@ ${issueSafeFallbackFooter}`; if (body.length > MAX_GITHUB_BODY_LENGTH) { throw new Error(`Pull request body exceeds GitHub's maximum length of ${MAX_GITHUB_BODY_LENGTH} characters`); } + transport.assertUnchanged(); const { data: pullRequest } = await createOrUpdatePullRequest({ githubClient, repoParts, @@ -2788,7 +2829,9 @@ ${issueSafeFallbackFooter}`; // This works around the GITHUB_TOKEN limitation where pushes don't trigger CI events. // Restricting to exactly 1 new commit prevents the CI trigger token being used on // multi-commit branches where workflow files may have been iteratively modified. + transport.assertUnchanged(); const ciTriggerResult = await pushExtraEmptyCommit({ + assertTransportUnchanged: transport.assertUnchanged, branchName, repoOwner: pushRepoParts.owner, repoName: pushRepoParts.repo, @@ -2812,6 +2855,7 @@ ${issueSafeFallbackFooter}`; head_repo: pushRepo, }; } catch (prError) { + transport.assertUnchanged(); const errorMessage = getErrorMessage(prError); core.warning(`Failed to create pull request: ${errorMessage}`); @@ -2831,7 +2875,7 @@ ${issueSafeFallbackFooter}`; if (patchFilePath && fs.existsSync(patchFilePath)) { let patchContent; try { - patchContent = fs.readFileSync(patchFilePath, "utf8"); + patchContent = transport.read(patchFilePath).toString("utf8"); } catch (err) { throw new Error(`Failed to read file ${patchFilePath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -2849,7 +2893,8 @@ ${issueSafeFallbackFooter}`; }); try { - const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees); + transport.assertUnchanged(); + const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees, transport.assertUnchanged); core.info(`Created fallback issue #${issue.number}: ${issue.html_url}`); await assignCopilotToFallbackIssueIfEnabled(issueRepoParts.owner, issueRepoParts.repo, issue.number); @@ -2897,7 +2942,7 @@ ${issueSafeFallbackFooter}`; if (patchFilePath && fs.existsSync(patchFilePath)) { let patchContent; try { - patchContent = fs.readFileSync(patchFilePath, "utf8"); + patchContent = transport.read(patchFilePath).toString("utf8"); } catch (err) { throw new Error(`Failed to read file ${patchFilePath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -2923,7 +2968,8 @@ ${patchPreview} ${issueSafeFallbackFooter}`; try { - const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees); + transport.assertUnchanged(); + const { data: issue, issueRepoParts } = await createFallbackIssue(githubClient, repoParts, title, fallbackBody, mergeFallbackIssueLabels(effectiveFallbackLabels), configAssignees, transport.assertUnchanged); core.info(`Created fallback issue #${issue.number}: ${issue.html_url}`); await assignCopilotToFallbackIssueIfEnabled(issueRepoParts.owner, issueRepoParts.repo, issue.number); @@ -2958,7 +3004,7 @@ ${issueSafeFallbackFooter}`; process.chdir(originalCwd); } } - }; // End of handleCreatePullRequest + }); // End of handleCreatePullRequest } // End of main module.exports = { diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index f1ff6bae672..0439d420c63 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -36,6 +36,12 @@ function canonicalBundlePath(branch, repo) { createdTransportPaths.add(p); return p; } +function isBundleSnapshot(candidate, original) { + return typeof candidate === "string" && path.basename(candidate) === path.basename(original) && path.basename(path.dirname(candidate)).startsWith(".gh-aw-transport-"); +} +function bundleSnapshot(original) { + return expect.stringMatching(new RegExp(`\\.gh-aw-transport-[^/]+/${path.basename(original).replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}$`)); +} function cleanupCanonicalTransports() { for (const p of createdTransportPaths) { try { @@ -606,7 +612,7 @@ index 0000000..abc1234 expect(result.success).toBe(true); // Initial bundle fetch is now via getExecOutput (with ignoreReturnCode: true) rather than exec, // so the bundle fetch appears in getExecOutput.mock.calls. - const bundleFetchCall = global.exec.getExecOutput.mock.calls.find(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); + const bundleFetchCall = global.exec.getExecOutput.mock.calls.find(([, args]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath)); if (!bundleFetchCall) { throw new Error("expected bundle fetch call via getExecOutput"); } @@ -835,10 +841,10 @@ index 0000000..abc1234 return Promise.resolve({ exitCode: 0, stdout: "1\n", stderr: "" }); } // Initial bundle fetch via getExecOutput with ignoreReturnCode: the JSONL branch ref is missing - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode) { return Promise.resolve({ exitCode: 1, stderr: "fatal: couldn't find remote ref refs/heads/ops-review-may09-2026", stdout: "" }); } - if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads" && args[2] === bundlePath) { + if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads" && isBundleSnapshot(args[2], bundlePath)) { return Promise.resolve({ exitCode: 0, stdout: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/main\naaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa HEAD\n", @@ -856,8 +862,8 @@ index 0000000..abc1234 const result = await handler({ title: "Test PR", body: "Test body", branch: "ops-review-may09-2026" }, {}); expect(result.success).toBe(true); - expect(global.exec.getExecOutput).toHaveBeenCalledWith("git", ["bundle", "list-heads", bundlePath]); - const resolvedFetchCall = global.exec.exec.mock.calls.find(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && args[2].startsWith("refs/heads/main:")); + expect(global.exec.getExecOutput).toHaveBeenCalledWith("git", ["bundle", "list-heads", bundleSnapshot(bundlePath)]); + const resolvedFetchCall = global.exec.exec.mock.calls.find(([, args]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && args[2].startsWith("refs/heads/main:")); if (!resolvedFetchCall) { throw new Error("expected resolved bundle fetch call"); } @@ -895,15 +901,15 @@ index 0000000..abc1234 return Promise.resolve({ exitCode: 0, stdout: "1\n", stderr: "" }); } // Initial bundle fetch (refs/heads/* refspec) fails because the JSONL branch ref is absent - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("refs/heads/")) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("refs/heads/")) { return Promise.resolve({ exitCode: 1, stderr: "fatal: couldn't find remote ref refs/heads/docs/update-migration-version-2026-05-19-4fe3b9f7f99fc1d6", stdout: "" }); } // HEAD-based bundle fetch (fallback path) succeeds — no prerequisite errors - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("HEAD:")) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("HEAD:")) { return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }); } // Bundle contains only HEAD — no refs/heads/* entry (the bug scenario) - if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads" && args[2] === bundlePath) { + if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads" && isBundleSnapshot(args[2], bundlePath)) { return Promise.resolve({ exitCode: 0, stdout: "ac85f4047717ec43c931d750575f5251c45dc705 HEAD\n", @@ -921,11 +927,11 @@ index 0000000..abc1234 const result = await handler({ title: "Test PR", body: "Test body", branch: "docs/update-migration-version-2026-05-19-4fe3b9f7f99fc1d6" }, {}); expect(result.success).toBe(true); - expect(global.exec.getExecOutput).toHaveBeenCalledWith("git", ["bundle", "list-heads", bundlePath]); + expect(global.exec.getExecOutput).toHaveBeenCalledWith("git", ["bundle", "list-heads", bundleSnapshot(bundlePath)]); // HEAD-based bundle fetch is now performed via getExecOutput (ignoreReturnCode: true) // so the code can distinguish prerequisite errors from other failures. const headFetchCall = global.exec.getExecOutput.mock.calls.find( - ([, args, opts]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && typeof args[2] === "string" && args[2].startsWith("HEAD:") && opts && opts.ignoreReturnCode + ([, args, opts]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && typeof args[2] === "string" && args[2].startsWith("HEAD:") && opts && opts.ignoreReturnCode ); if (!headFetchCall) { throw new Error("expected HEAD-based bundle fetch call via getExecOutput"); @@ -972,15 +978,15 @@ index 0000000..abc1234 return Promise.resolve({ exitCode: 0, stdout: "1\n", stderr: "" }); } // Initial bundle fetch (named ref) fails — bundle only has HEAD - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("refs/heads/")) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("refs/heads/")) { return Promise.resolve({ exitCode: 1, stderr: `fatal: couldn't find remote ref refs/heads/${branchName}`, stdout: "" }); } // HEAD-based bundle fetch fails because prerequisite (feature-branch tip) is missing - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("HEAD:")) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("HEAD:")) { return Promise.resolve({ exitCode: 1, stderr: `error: Repository lacks these prerequisite commits:\nerror: ${featureBranchTip}`, stdout: "" }); } // Bundle contains only HEAD — no refs/heads/* entry - if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads" && args[2] === bundlePath) { + if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads" && isBundleSnapshot(args[2], bundlePath)) { return Promise.resolve({ exitCode: 0, stdout: `ac85f4047717ec43c931d750575f5251c45dc705 HEAD\n`, @@ -1001,7 +1007,7 @@ index 0000000..abc1234 // Feature-branch tip prerequisite is fetched from origin expect(global.exec.exec).toHaveBeenCalledWith("git", ["fetch", "--filter=blob:none", "origin", featureBranchTip]); // After prerequisite recovery, HEAD bundle is retried via exec - const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && typeof args[2] === "string" && args[2].startsWith("HEAD:")); + const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && typeof args[2] === "string" && args[2].startsWith("HEAD:")); expect(bundleRetryFetchCalls.length).toBe(1); }); @@ -1038,13 +1044,13 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-list") { return Promise.resolve({ exitCode: 0, stdout: "1\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("refs/heads/")) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("refs/heads/")) { return Promise.resolve({ exitCode: 1, stderr: `fatal: couldn't find remote ref refs/heads/${branchName}`, stdout: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("HEAD:")) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode && typeof args[2] === "string" && args[2].startsWith("HEAD:")) { return Promise.resolve({ exitCode: 1, stderr: `error: Repository lacks these prerequisite commits:\nerror: ${featureBranchTip}`, stdout: "" }); } - if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads" && args[2] === bundlePath) { + if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads" && isBundleSnapshot(args[2], bundlePath)) { return Promise.resolve({ exitCode: 0, stdout: `ac85f4047717ec43c931d750575f5251c45dc705 HEAD\n`, @@ -1058,7 +1064,7 @@ index 0000000..abc1234 }); global.exec.exec.mockImplementation((cmd, args) => { - if (cmd === "git" && Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && typeof args[2] === "string" && args[2].startsWith("HEAD:")) { + if (cmd === "git" && Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && typeof args[2] === "string" && args[2].startsWith("HEAD:")) { throw new Error("fatal: failed to read HEAD-only bundle"); } return Promise.resolve(0); @@ -1107,7 +1113,7 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-list") { return Promise.resolve({ exitCode: 0, stdout: "1\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode) { return Promise.resolve({ exitCode: 1, stderr: `error: Repository lacks these prerequisite commits:\nerror: ${missingSha}`, stdout: "" }); } if (cmd === "git" && args && args[0] === "ls-remote") { @@ -1124,9 +1130,9 @@ index 0000000..abc1234 // Prerequisites are fetched from origin via exec with --filter=blob:none to avoid downloading blobs expect(global.exec.exec).toHaveBeenCalledWith("git", ["fetch", "--filter=blob:none", "origin", missingSha]); // Retry bundle fetch is via exec (only the retry, not the initial attempt which was getExecOutput) - const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); + const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath)); expect(bundleRetryFetchCalls.length).toBe(1); - expect(global.exec.getExecOutput).not.toHaveBeenCalledWith("git", ["bundle", "list-heads", bundlePath]); + expect(global.exec.getExecOutput).not.toHaveBeenCalledWith("git", ["bundle", "list-heads", bundleSnapshot(bundlePath)]); }); it("should fetch all prerequisite commits in a single origin fetch and retry bundle fetch", async () => { @@ -1161,7 +1167,7 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-list") { return Promise.resolve({ exitCode: 0, stdout: "1\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode) { return Promise.resolve({ exitCode: 1, stderr: `error: Repository lacks these prerequisite commits:\nerror: ${missingSha1}\nerror: ${missingSha2}`, stdout: "" }); } if (cmd === "git" && args && args[0] === "ls-remote") { @@ -1176,9 +1182,9 @@ index 0000000..abc1234 expect(result.success).toBe(true); expect(global.exec.exec).toHaveBeenCalledWith("git", ["fetch", "--filter=blob:none", "origin", missingSha1, missingSha2]); - const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); + const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath)); expect(bundleRetryFetchCalls.length).toBe(1); - expect(global.exec.getExecOutput).not.toHaveBeenCalledWith("git", ["bundle", "list-heads", bundlePath]); + expect(global.exec.getExecOutput).not.toHaveBeenCalledWith("git", ["bundle", "list-heads", bundleSnapshot(bundlePath)]); }); it("should fail when fetching prerequisite commits from origin fails", async () => { @@ -1212,7 +1218,7 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-list") { return Promise.resolve({ exitCode: 0, stdout: "1\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode) { return Promise.resolve({ exitCode: 1, stderr: `error: Repository lacks these prerequisite commits:\nerror: ${missingSha}`, stdout: "" }); } if (cmd === "git" && args && args[0] === "ls-remote") { @@ -1235,7 +1241,7 @@ index 0000000..abc1234 expect(result.error).toBe("Failed to apply bundle"); expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("Failed to apply bundle: fatal: couldn't connect to 'origin'")); // No retry bundle fetch via exec — failed at prerequisite origin fetch - const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); + const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath)); expect(bundleRetryFetchCalls.length).toBe(0); }); @@ -1270,7 +1276,7 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-list") { return Promise.resolve({ exitCode: 0, stdout: "1\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode) { return Promise.resolve({ exitCode: 1, stderr: `error: Repository lacks these prerequisite commits:\nerror: ${missingSha}`, stdout: "" }); } if (cmd === "git" && args && args[0] === "ls-remote") { @@ -1279,7 +1285,7 @@ index 0000000..abc1234 return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }); }); global.exec.exec.mockImplementation((cmd, args) => { - if (cmd === "git" && Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath) { + if (cmd === "git" && Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath)) { throw new Error("fatal: failed to read bundle"); } return Promise.resolve(0); @@ -1292,7 +1298,7 @@ index 0000000..abc1234 expect(result.success).toBe(false); expect(result.error).toBe("Failed to apply bundle"); expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("Bundle fetch failed after fetching 1 prerequisite commit(s): fatal: failed to read bundle")); - const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); + const bundleRetryFetchCalls = global.exec.exec.mock.calls.filter(([, args]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath)); expect(bundleRetryFetchCalls.length).toBe(1); }); @@ -1325,8 +1331,8 @@ index 0000000..abc1234 expect(result.success).toBe(true); // The initial bundle fetch uses getExecOutput (not exec.exec) — ensure it never uses the direct branch refspec - expect(global.exec.getExecOutput).not.toHaveBeenCalledWith("git", ["fetch", bundlePath, "refs/heads/autoloop/perf-comparison:refs/heads/autoloop/perf-comparison"], expect.anything()); - const bundleFetchCall = global.exec.getExecOutput.mock.calls.find(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); + expect(global.exec.getExecOutput).not.toHaveBeenCalledWith("git", ["fetch", bundleSnapshot(bundlePath), "refs/heads/autoloop/perf-comparison:refs/heads/autoloop/perf-comparison"], expect.anything()); + const bundleFetchCall = global.exec.getExecOutput.mock.calls.find(([, args]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath)); if (!bundleFetchCall) { throw new Error("expected bundle fetch call"); } @@ -4857,40 +4863,31 @@ describe("create_pull_request - E003 file-limit fallback-to-issue", () => { } it("should create a fallback issue when E003 fires and fallback_as_issue is true (default)", async () => { - const patchPath = getPatchPathForBranch("data/refresh"); - const runtimeFs = require("fs"); - const originalExistsSync = runtimeFs.existsSync.bind(runtimeFs); - const originalReadFileSync = runtimeFs.readFileSync.bind(runtimeFs); - const existsSpy = vi.spyOn(runtimeFs, "existsSync").mockImplementation(file => String(file) === patchPath || originalExistsSync(file)); - const readSpy = vi.spyOn(runtimeFs, "readFileSync").mockImplementation((file, ...args) => (String(file) === patchPath ? buildOversizedPatch(101) : originalReadFileSync(file, ...args))); - try { - const { main } = require("./create_pull_request.cjs"); - const handler = await main({}); - const result = await handler({ title: "Data refresh PR", body: "Daily update", branch: "data/refresh" }, {}); + const patchPath = canonicalPatchPath("data/refresh"); + fs.writeFileSync(patchPath, buildOversizedPatch(101)); + const { main } = require("./create_pull_request.cjs"); + const handler = await main({}); + const result = await handler({ title: "Data refresh PR", body: "Daily update", branch: "data/refresh" }, {}); - expect(result.success).toBe(true); - expect(result.fallback_used).toBe(true); - expect(result.issue_number).toBe(55); + expect(result.success).toBe(true); + expect(result.fallback_used).toBe(true); + expect(result.issue_number).toBe(55); - // A fallback issue should have been created - expect(global.github.rest.issues.create).toHaveBeenCalledTimes(1); - const issueCall = global.github.rest.issues.create.mock.calls[0][0]; + // A fallback issue should have been created + expect(global.github.rest.issues.create).toHaveBeenCalledTimes(1); + const issueCall = global.github.rest.issues.create.mock.calls[0][0]; - // The body should contain the E003 error message and the actionable fix - expect(issueCall.body).toContain("E003"); - expectExecutionProvenance(issueCall.body); - expect(issueCall.body).toContain("max-patch-files"); + // The body should contain the E003 error message and the actionable fix + expect(issueCall.body).toContain("E003"); + expectExecutionProvenance(issueCall.body); + expect(issueCall.body).toContain("max-patch-files"); - // The suggested limit must be >= the actual file count (101), not maxFiles * 2 (200) - expect(issueCall.body).toContain("max-patch-files: 101"); - expect(issueCall.body).not.toContain("max-patch-files: 200"); + // The suggested limit must be >= the actual file count (101), not maxFiles * 2 (200) + expect(issueCall.body).toContain("max-patch-files: 101"); + expect(issueCall.body).not.toContain("max-patch-files: 200"); - // PR creation should NOT have been attempted - expect(global.github.rest.pulls.create).not.toHaveBeenCalled(); - } finally { - existsSpy.mockRestore(); - readSpy.mockRestore(); - } + // PR creation should NOT have been attempted + expect(global.github.rest.pulls.create).not.toHaveBeenCalled(); }); it("should use the actual received file count (not maxFiles*2) as the suggested limit", async () => { diff --git a/actions/setup/js/extra_empty_commit.cjs b/actions/setup/js/extra_empty_commit.cjs index e35293e7c27..18dca390adf 100644 --- a/actions/setup/js/extra_empty_commit.cjs +++ b/actions/setup/js/extra_empty_commit.cjs @@ -52,9 +52,10 @@ function isCrossRepoTarget(repoOwner, repoName) { * empty commit when exactly 1 new commit was pushed, preventing accidental workflow-file * modifications on multi-commit branches and reducing loop risk. * @param {string[]|string} [options.allowedRepos] - Allowed repository patterns for allowlist validation + * @param {() => void} [options.assertTransportUnchanged] * @returns {Promise<{success: boolean, skipped?: boolean, error?: string}>} */ -async function pushExtraEmptyCommit({ branchName, repoOwner, repoName, commitMessage, newCommitCount, allowedRepos: allowedReposInput }) { +async function pushExtraEmptyCommit({ branchName, repoOwner, repoName, commitMessage, newCommitCount, allowedRepos: allowedReposInput, assertTransportUnchanged }) { const token = process.env.GH_AW_CI_TRIGGER_TOKEN; if (!token || !token.trim()) { @@ -207,6 +208,7 @@ async function pushExtraEmptyCommit({ branchName, repoOwner, repoName, commitMes try { core.info(`Attempting to create verified empty commit via GitHub API (createCommitOnBranch)`); const ciGithubClient = global.getOctokit(token.trim()); + assertTransportUnchanged?.(); const apiResult = await ciGithubClient.graphql( `mutation($input: CreateCommitOnBranchInput!) { createCommitOnBranch(input: $input) { commit { oid } } @@ -236,6 +238,7 @@ async function pushExtraEmptyCommit({ branchName, repoOwner, repoName, commitMes } committedViaApi = true; } catch (apiError) { + assertTransportUnchanged?.(); core.info(`GitHub API commit creation unavailable, falling back to git commit + push: ${getErrorMessage(apiError)}`); } } @@ -252,6 +255,7 @@ async function pushExtraEmptyCommit({ branchName, repoOwner, repoName, commitMes await exec.exec("git", ["remote", "add", "ci-trigger", remoteUrl]); await exec.exec("git", ["commit", "--allow-empty", "-m", message]); await require("./work_queue_git_effects.cjs").assertGitPushAuthorized({ remote: "ci-trigger", branch: branchName }); + assertTransportUnchanged?.(); await exec.exec("git", ["push", "ci-trigger", branchName]); } @@ -277,6 +281,7 @@ async function pushExtraEmptyCommit({ branchName, repoOwner, repoName, commitMes } } } catch (error) { + assertTransportUnchanged?.(); const errorMessage = getErrorMessage(error); core.warning(`Failed to push extra empty commit: ${errorMessage}`); diff --git a/actions/setup/js/push_signed_commits.cjs b/actions/setup/js/push_signed_commits.cjs index 6490596df5b..dc1ab87d48b 100644 --- a/actions/setup/js/push_signed_commits.cjs +++ b/actions/setup/js/push_signed_commits.cjs @@ -285,12 +285,14 @@ async function lsRemoteHeadOid(branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken * @param {any} [opts.gitAuthEnv] * @param {string} [opts.pushRemoteUrl] * @param {string} [opts.pushToken] + * @param {() => void} [opts.assertTransportUnchanged] * @returns {Promise} */ -async function pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken }) { +async function pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken, assertTransportUnchanged }) { const pushArgs = pushRemoteUrl ? ["push", pushRemoteUrl, branch] : ["push", "origin", branch]; const pushOnce = async () => { await require("./work_queue_git_effects.cjs").assertGitPushAuthorized({ remote: pushRemoteUrl || "origin", branch, cwd, gitAuthEnv }); + assertTransportUnchanged?.(); const result = await getGitExecOutput(pushArgs, { cwd, env: { ...process.env, ...(gitAuthEnv || {}) }, @@ -397,6 +399,7 @@ async function resolveLocalHeadSha(cwd) { * @param {string} [opts.currentRepo] - Repository slug used for same-repo temporary ID resolution * @param {Record} [opts.validationConfig] - Optional safe-output policy config applied to synthesized GraphQL fileChanges * @param {(context: {cwd: string, branch: string, output: string}) => (boolean|Promise)} [opts.resolveRebaseConflict] + * @param {() => void} [opts.assertTransportUnchanged] - Check captured safe-output inputs immediately before remote writes * @returns {Promise} SHA of the commit that landed on the target branch */ async function pushSignedCommits({ @@ -416,6 +419,7 @@ async function pushSignedCommits({ currentRepo, validationConfig, resolveRebaseConflict, + assertTransportUnchanged, }) { const effectiveCurrentRepo = currentRepo || `${owner}/${repo}`; const temporaryIdMap = loadTemporaryIdMapFromResolved(resolvedTemporaryIds, { @@ -430,7 +434,7 @@ async function pushSignedCommits({ if (signedCommits === false) { if (requireBaseRefMatch) throw new Error(`${ERR_VALIDATION}: signed commits are required for an atomic ledger transition`); core.info(`pushSignedCommits: signed-commits disabled (using direct git push) for branch ${branch}`); - const headSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken }); + const headSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken, assertTransportUnchanged }); core.info(`pushSignedCommits: git push and HEAD resolution completed, HEAD=${headSha}`); return headSha; } @@ -445,7 +449,7 @@ async function pushSignedCommits({ } core.info(`pushSignedCommits: empty baseRef detected (orphan branch first push), using git push directly for branch ${branch}`); try { - const headSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken }); + const headSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken, assertTransportUnchanged }); core.info(`pushSignedCommits: git push completed for orphan branch, HEAD=${headSha}`); return headSha; } catch (pushErr) { @@ -812,6 +816,7 @@ async function pushSignedCommits({ } core.info(`pushSignedCommits: creating remote branch ${branch} at parent OID ${expectedHeadOid}`); try { + assertTransportUnchanged?.(); await githubClient.rest.git.createRef({ owner, repo, @@ -820,6 +825,7 @@ async function pushSignedCommits({ }); core.info(`pushSignedCommits: remote branch ${branch} created successfully`); } catch (createRefError) { + assertTransportUnchanged?.(); /** @type {any} */ const err = createRefError; const status = err && typeof err === "object" ? err.status : undefined; @@ -866,6 +872,7 @@ async function pushSignedCommits({ }; core.info(`pushSignedCommits: calling createCommitOnBranch mutation (expectedHeadOid=${expectedHeadOid})`); + assertTransportUnchanged?.(); const result = await githubClient.graphql( `mutation($input: CreateCommitOnBranchInput!) { createCommitOnBranch(input: $input) { commit { oid } } @@ -882,6 +889,7 @@ async function pushSignedCommits({ core.info(`pushSignedCommits: all ${shas.length} commit(s) pushed as signed commits`); return lastOid ?? shas[shas.length - 1]; } catch (err) { + assertTransportUnchanged?.(); if (err instanceof PushSignedCommitsUnsupportedShape) { throw new Error( `${ERR_VALIDATION}: pushSignedCommits: refusing unsigned push for branch '${branch}': ${stripLeadingErrorCode(getErrorMessage(err))}. ` + @@ -899,7 +907,7 @@ async function pushSignedCommits({ throw new Error(`${ERR_API}: pushSignedCommits: signed commit push failed for branch '${branch}' and git push fallback is disabled: ${getErrorMessage(err)}`, { cause: err }); } core.warning(`pushSignedCommits: GraphQL signed push failed, falling back to git push: ${getErrorMessage(err)}`); - const fallbackSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken }); + const fallbackSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv, pushRemoteUrl, pushToken, assertTransportUnchanged }); core.info(`pushSignedCommits: git push fallback completed, using pushed SHA ${fallbackSha}`); return fallbackSha; } diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index b1df498000a..46dfb25763e 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -3,6 +3,7 @@ /** @type {typeof import("fs")} */ const fs = require("fs"); +const { withTransportIntegrity } = require("./transport_integrity.cjs"); const path = require("path"); const { generateStagedPreview } = require("./staged_preview.cjs"); const { isStagedMode } = require("./safe_output_helpers.cjs"); @@ -451,7 +452,8 @@ async function main(config = {}) { * @param {import('./types/handler-factory').ResolvedTemporaryIds} resolvedTemporaryIds - Map of temporary IDs to resolved IDs * @returns {Promise} */ - return async function handlePushToPullRequestBranch(message, resolvedTemporaryIds) { + return withTransportIntegrity(async function handlePushToPullRequestBranch(message, resolvedTemporaryIds, transport) { + const exec = transport.guardExec(globalThis.exec); // Check max count if (maxCount > 0 && processedCount >= maxCount) { core.info(`Skipping message - max count (${maxCount}) reached`); @@ -464,6 +466,8 @@ async function main(config = {}) { // the entry it writes, but the validation step strips them as a defense // against agent-forged values. Recover them by re-deriving from `branch`. const transportPaths = resolveTransportPaths(message, defaultTargetRepo); + transport.captureOptional(transportPaths.patchPath); + transport.captureOptional(transportPaths.bundlePath); const patchFilePath = transportPaths.patchPath; core.info(`Patch file path: ${patchFilePath || "(not set)"}`); @@ -503,7 +507,7 @@ async function main(config = {}) { let patchContent; try { - patchContent = fs.readFileSync(patchFilePath, "utf8"); + patchContent = transport.read(patchFilePath).toString("utf8"); } catch (err) { throw new Error(`Failed to read file ${patchFilePath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -630,7 +634,7 @@ async function main(config = {}) { if (patchFilePath && fs.existsSync(patchFilePath)) { let patchStats; try { - patchStats = fs.readFileSync(patchFilePath, "utf8"); + patchStats = transport.read(patchFilePath).toString("utf8"); } catch (err) { throw new Error(`Failed to read file ${patchFilePath}: ${getErrorMessage(err)}`, { cause: err }); } @@ -884,6 +888,7 @@ async function main(config = {}) { } const createProtectedFilesFallbackIssue = async files => { + transport.assertUnchanged(); const runUrl = buildWorkflowRunUrl(context, context.repo); const runId = context.runId; const artifactFileName = hasBundleFile ? bundleFilePath.replace("/tmp/gh-aw/", "") : patchFilePath ? patchFilePath.replace("/tmp/gh-aw/", "") : "aw-unknown.patch"; @@ -911,14 +916,16 @@ async function main(config = {}) { try { const { data: issue } = await withRetry( - () => - githubClient.rest.issues.create({ + () => { + transport.assertUnchanged(); + return githubClient.rest.issues.create({ owner: repoParts.owner, repo: repoParts.repo, title: issueTitle, body: issueBody, labels: ["agentic-workflows"], - }), + }); + }, RATE_LIMIT_RETRY_CONFIG, `create manifest-protection review issue in ${repoParts.owner}/${repoParts.repo}` ); @@ -1257,6 +1264,7 @@ async function main(config = {}) { // Non-fatal cleanup } } catch (bundleError) { + transport.assertUnchanged(); core.error(`Failed to apply bundle: ${getErrorMessage(bundleError)}`); // Clean up temp ref if it exists try { @@ -1274,18 +1282,18 @@ async function main(config = {}) { core.info(`Appending commit title suffix: "${commitTitleSuffix}"`); // Read the patch file - let patchContent = fs.readFileSync(patchFilePath, "utf8"); + let patchContent = transport.read(patchFilePath).toString("utf8"); // Modify Subject lines in the patch to append the suffix patchContent = patchContent.replace(/^Subject: (?:\[PATCH\] )?(.*)$/gm, (match, title) => `Subject: [PATCH] ${title}${commitTitleSuffix}`); - // Write the modified patch back - fs.writeFileSync(patchFilePath, patchContent, "utf8"); + // Transform only the private snapshot; keep the original baseline. + transport.transform(patchFilePath, () => Buffer.from(patchContent, "utf8")); core.info(`Patch modified with commit title suffix: "${commitTitleSuffix}"`); } // Log first 100 lines of patch for debugging - const finalPatchContent = fs.readFileSync(patchFilePath, "utf8"); + const finalPatchContent = transport.read(patchFilePath).toString("utf8"); const patchLines = finalPatchContent.split("\n"); const previewLineCount = Math.min(100, patchLines.length); core.info(`Patch preview (first ${previewLineCount} of ${patchLines.length} lines):`); @@ -1298,6 +1306,7 @@ async function main(config = {}) { await exec.exec("git", ["am", "--3way", patchFilePath], baseGitOpts); core.info("Patch applied successfully"); } catch (error) { + transport.assertUnchanged(); core.warning(`Initial patch apply failed, attempting add/add recovery: ${getErrorMessage(error)}`); let recoveredFromAddAddConflict = false; @@ -1489,6 +1498,7 @@ async function main(config = {}) { "Please review the changes carefully before merging.", ].join("\n"); + transport.assertUnchanged(); const { data: reviewPR } = await githubClient.rest.pulls.create({ owner: repoParts.owner, repo: repoParts.repo, @@ -1525,6 +1535,7 @@ async function main(config = {}) { pr_url: reviewPR.html_url, }; } catch (reviewError) { + transport.assertUnchanged(); core.error(`Failed to create review PR: ${getErrorMessage(reviewError)}`); return { success: false, error: `Failed to create review PR: ${getErrorMessage(reviewError)}` }; } @@ -1573,8 +1584,10 @@ async function main(config = {}) { // "Unable to determine if workflow can be created or updated due to timeout; `workflows` // scope may be required." — a transient server-side condition, not a real scope problem. // Retry with exponential backoff before treating the push as failed. - const pushedSha = await withTransientPushRetry(() => - pushSignedCommits({ + const pushedSha = await withTransientPushRetry(() => { + transport.assertUnchanged(); + return pushSignedCommits({ + assertTransportUnchanged: transport.assertUnchanged, githubClient: pushGithubClient, owner: pushRepoParts.owner, repo: pushRepoParts.repo, @@ -1588,14 +1601,15 @@ async function main(config = {}) { resolvedTemporaryIds, currentRepo: itemRepo, validationConfig: config, - }) - ); + }); + }); if (pushedSha) { pushedCommitSha = pushedSha; core.info(`pushSignedCommits returned pushed SHA: ${pushedSha}`); } core.info(`Changes committed and pushed to branch: ${branchName}`); } catch (pushError) { + transport.assertUnchanged(); const pushErrorMessage = getErrorMessage(pushError); core.error(`Failed to push changes: ${pushErrorMessage}`); @@ -1687,6 +1701,7 @@ async function main(config = {}) { `Workflow run: ${buildWorkflowRunUrl(context, context.repo)}`, ].join("\n"); + transport.assertUnchanged(); const { data: fallbackPR } = await githubClient.rest.pulls.create({ owner: repoParts.owner, repo: repoParts.repo, @@ -1713,6 +1728,7 @@ async function main(config = {}) { url: fallbackPR.html_url, }; } catch (fallbackError) { + transport.assertUnchanged(); const fallbackErrorMessage = getErrorMessage(fallbackError); core.error(`Failed to create fallback pull request: ${fallbackErrorMessage}`); userMessage = `${userMessage} Fallback pull request creation also failed: ${fallbackErrorMessage}`; @@ -1825,7 +1841,9 @@ async function main(config = {}) { // Restricting to exactly 1 new commit prevents the CI trigger token being used on // multi-commit branches where workflow files may have been iteratively modified. if (hasChanges) { + transport.assertUnchanged(); const ciTriggerResult = await pushExtraEmptyCommit({ + assertTransportUnchanged: transport.assertUnchanged, branchName, repoOwner: pushRepoParts.owner, repoName: pushRepoParts.repo, @@ -1850,7 +1868,7 @@ async function main(config = {}) { branchStateBefore || (remoteHeadBeforePatch ? { head_sha: remoteHeadBeforePatch } : null), commitSha ? { head_sha: commitSha } : null ); - }; + }); } module.exports = { main, HANDLER_TYPE, getBundlePreApplyFiles, getBundleValidationBase }; 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 926834013e4..603e3da7ee9 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -28,6 +28,12 @@ function canonicalBundlePath(branch, repo) { createdTransportPaths.add(p); return p; } +function isBundleSnapshot(candidate, original) { + return typeof candidate === "string" && path.basename(candidate) === path.basename(original) && path.basename(path.dirname(candidate)).startsWith(".gh-aw-transport-"); +} +function bundleSnapshot(original) { + return expect.stringMatching(new RegExp(`\\.gh-aw-transport-[^/]+/${path.basename(original).replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}$`)); +} function cleanupCanonicalTransports() { for (const p of createdTransportPaths) { try { @@ -2616,9 +2622,9 @@ index 0000000..abc1234 expect(result.success).toBe(true); // Initial bundle fetch is via getExecOutput (with ignoreReturnCode: true), not exec.exec - const bundleFetchCall = mockExec.getExecOutput.mock.calls.find(([, args, options]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode); + const bundleFetchCall = mockExec.getExecOutput.mock.calls.find(([, args, options]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode); expect(bundleFetchCall).toBeDefined(); - expect(bundleFetchCall[1]).toEqual(["fetch", bundlePath, "refs/heads/feature-branch:refs/bundles/push-feature-branch"]); + expect(bundleFetchCall[1]).toEqual(["fetch", bundleSnapshot(bundlePath), "refs/heads/feature-branch:refs/bundles/push-feature-branch"]); expect(mockExec.exec).toHaveBeenCalledWith("git", ["update-ref", "refs/heads/feature-branch", "refs/bundles/push-feature-branch", "remote-head"], expect.any(Object)); expect(mockExec.exec).toHaveBeenCalledWith("git", ["reset", "--hard"], expect.any(Object)); expect(mockExec.exec).not.toHaveBeenCalledWith("git", ["merge", "--ff-only", "refs/bundles/push-feature-branch"], expect.any(Object)); @@ -2669,7 +2675,7 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-parse" && args[1] === "--is-shallow-repository") { return Promise.resolve({ exitCode: 0, stdout: "false\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode) { return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }); } if (cmd === "git" && args[0] === "diff" && args[1] === "--name-only" && args[2] === "--no-renames") { @@ -2731,7 +2737,7 @@ index 0000000..abc1234 expect(result.success).toBe(true); // Bundle fetch must use the sanitized API branch name, not the agent-supplied message.branch - const bundleFetchCall = mockExec.getExecOutput.mock.calls.find(([, args, opts]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath && opts && opts.ignoreReturnCode); + const bundleFetchCall = mockExec.getExecOutput.mock.calls.find(([, args, opts]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && opts && opts.ignoreReturnCode); expect(bundleFetchCall).toBeDefined(); // Refspec must use the sanitized name "feature-branch", not "feature-branch; rm -rf /" expect(bundleFetchCall[1][2]).toMatch(/^refs\/heads\/feature-branch:/); @@ -2763,13 +2769,13 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-parse" && args[1] === "--is-shallow-repository") { return Promise.resolve({ exitCode: 0, stdout: "true\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && args[2].startsWith("refs/heads/") && options && options.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && args[2].startsWith("refs/heads/") && options && options.ignoreReturnCode) { return Promise.resolve({ exitCode: 128, stdout: "", stderr: "fatal: couldn't find remote ref refs/heads/feature-branch" }); } if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads") { return Promise.resolve({ exitCode: 0, stdout: `${bundleHead} HEAD\n`, stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && args[2].startsWith("HEAD:") && options && options.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && args[2].startsWith("HEAD:") && options && options.ignoreReturnCode) { return Promise.resolve({ exitCode: 1, stdout: "", stderr: `error: Repository lacks these prerequisite commits:\nerror: ${prerequisiteSha}` }); } if (cmd === "git" && args[0] === "rev-list") { @@ -2783,10 +2789,10 @@ index 0000000..abc1234 const result = await handler({ branch: "feature-branch", diff_size: 5 * 1024 }, {}); expect(result.success).toBe(true); - expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["bundle", "list-heads", bundlePath], expect.any(Object)); - expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["fetch", bundlePath, "HEAD:refs/bundles/push-feature-branch"], expect.objectContaining({ ignoreReturnCode: true })); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["bundle", "list-heads", bundleSnapshot(bundlePath)], expect.any(Object)); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["fetch", bundleSnapshot(bundlePath), "HEAD:refs/bundles/push-feature-branch"], expect.objectContaining({ ignoreReturnCode: true })); expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", "--filter=blob:none", "origin", prerequisiteSha], expect.any(Object)); - expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", bundlePath, "HEAD:refs/bundles/push-feature-branch"], expect.any(Object)); + expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", bundleSnapshot(bundlePath), "HEAD:refs/bundles/push-feature-branch"], expect.any(Object)); } finally { pushSignedSpy.mockRestore(); } @@ -2813,7 +2819,7 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-parse" && args[1] === "--is-shallow-repository") { return Promise.resolve({ exitCode: 0, stdout: "false\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && args[2].startsWith("refs/heads/feature-branch:") && options?.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && args[2].startsWith("refs/heads/feature-branch:") && options?.ignoreReturnCode) { return Promise.resolve({ exitCode: 128, stdout: "", stderr: "fatal: couldn't find remote ref refs/heads/feature-branch" }); } if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads") { @@ -2834,7 +2840,7 @@ index 0000000..abc1234 expect(result.success).toBe(true); expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["check-ref-format", bundleSourceRef], expect.objectContaining({ ignoreReturnCode: true })); - expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["fetch", bundlePath, `${bundleSourceRef}:refs/bundles/push-feature-branch`], expect.objectContaining({ ignoreReturnCode: true })); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["fetch", bundleSnapshot(bundlePath), `${bundleSourceRef}:refs/bundles/push-feature-branch`], expect.objectContaining({ ignoreReturnCode: true })); } finally { pushSignedSpy.mockRestore(); } @@ -2861,7 +2867,7 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-parse" && args[1] === "--is-shallow-repository") { return Promise.resolve({ exitCode: 0, stdout: "false\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && args[2].startsWith("refs/heads/feature-branch:") && options?.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && args[2].startsWith("refs/heads/feature-branch:") && options?.ignoreReturnCode) { return Promise.resolve({ exitCode: 128, stdout: "", stderr: "fatal: couldn't find remote ref refs/heads/feature-branch" }); } if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads") { @@ -2881,7 +2887,7 @@ index 0000000..abc1234 const result = await handler({ branch: "feature-branch", diff_size: 5 * 1024 }, {}); expect(result.success).toBe(true); - expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["fetch", bundlePath, "HEAD:refs/bundles/push-feature-branch"], expect.objectContaining({ ignoreReturnCode: true })); + expect(mockExec.getExecOutput).toHaveBeenCalledWith("git", ["fetch", bundleSnapshot(bundlePath), "HEAD:refs/bundles/push-feature-branch"], expect.objectContaining({ ignoreReturnCode: true })); } finally { pushSignedSpy.mockRestore(); } @@ -2905,7 +2911,7 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-parse" && args[1] === "HEAD") { return Promise.resolve({ exitCode: 0, stdout: "remote-head\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && args[2].startsWith("refs/heads/feature-branch:") && options?.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && args[2].startsWith("refs/heads/feature-branch:") && options?.ignoreReturnCode) { return Promise.resolve({ exitCode: 128, stdout: "", stderr: "fatal: couldn't find remote ref refs/heads/feature-branch" }); } if (cmd === "git" && args[0] === "bundle" && args[1] === "list-heads") { @@ -2948,7 +2954,7 @@ index 0000000..abc1234 if (cmd === "git" && args[0] === "rev-parse" && args[1] === "--is-shallow-repository") { return Promise.resolve({ exitCode: 0, stdout: "false\n", stderr: "" }); } - if (cmd === "git" && args[0] === "fetch" && args[1] === bundlePath && options && options.ignoreReturnCode) { + if (cmd === "git" && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath) && options && options.ignoreReturnCode) { // Initial bundle fetch fails with prerequisite error (the real race condition scenario) return Promise.resolve({ exitCode: 1, stderr: `error: Repository lacks these prerequisite commits:\nerror: ${missingSha}`, stdout: "" }); } @@ -2964,9 +2970,9 @@ index 0000000..abc1234 // the local clone already has all blobs and we must not convert it to a partial clone. expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", "origin", missingSha], expect.any(Object)); // Should have retried the bundle fetch via exec after fetching prerequisites - const bundleRetryFetch = mockExec.exec.mock.calls.find(([, args]) => Array.isArray(args) && args[0] === "fetch" && args[1] === bundlePath); + const bundleRetryFetch = mockExec.exec.mock.calls.find(([, args]) => Array.isArray(args) && args[0] === "fetch" && isBundleSnapshot(args[1], bundlePath)); expect(bundleRetryFetch).toBeDefined(); - expect(bundleRetryFetch[1]).toEqual(["fetch", bundlePath, "refs/heads/feature-branch:refs/bundles/push-feature-branch"]); + expect(bundleRetryFetch[1]).toEqual(["fetch", bundleSnapshot(bundlePath), "refs/heads/feature-branch:refs/bundles/push-feature-branch"]); } finally { pushSignedSpy.mockRestore(); } @@ -3313,6 +3319,12 @@ index 0000000..abc1234 describe("commit title suffix", () => { it("should append suffix to commit messages in patch", async () => { const patchPath = createPatchFile("should-append-suffix-to-commit-messages-in-patch"); + const originalPatch = fs.readFileSync(patchPath, "utf8"); + let appliedPatch; + mockExec.exec.mockImplementation(async (_command, args) => { + if (args[0] === "am" && args[1] === "--3way") appliedPatch = fs.readFileSync(args[2], "utf8"); + return 0; + }); mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); const module = await loadModule(); @@ -3322,9 +3334,8 @@ index 0000000..abc1234 expect(result.success).toBe(true); expect(mockCore.info).toHaveBeenCalledWith('Appending commit title suffix: " [skip ci]"'); - // Verify patch was modified - const modifiedPatch = fs.readFileSync(patchPath, "utf8"); - expect(modifiedPatch).toContain("[skip ci]"); + expect(appliedPatch).toContain("[skip ci]"); + expect(fs.readFileSync(patchPath, "utf8")).toBe(originalPatch); }); }); diff --git a/actions/setup/js/transport_integrity.cjs b/actions/setup/js/transport_integrity.cjs new file mode 100644 index 00000000000..1d960e4bd02 --- /dev/null +++ b/actions/setup/js/transport_integrity.cjs @@ -0,0 +1,159 @@ +// @ts-check + +const fs = require("fs"); +const path = require("path"); +const os = require("os"); +const crypto = require("crypto"); + +class TransportIntegrityError extends Error { + constructor(filePath, cause = undefined) { + super(`SECURITY: Safe-output transport changed after capture: ${filePath}`, { cause }); + this.name = "TransportIntegrityError"; + } +} + +const digest = bytes => crypto.createHash("sha256").update(bytes).digest("hex"); + +/** + * Capture validation inputs once. Git receives private, read-only snapshots, never + * the agent-writable originals, so checking a hash cannot race with git's open(). + */ +function createTransportIntegrity() { + /** @type {Map} */ + const inputs = new Map(); + let directory = ""; + /** @type {TransportIntegrityError | null} */ + let failure = null; + + function fail(filePath, cause = undefined) { + failure ||= new TransportIntegrityError(filePath, cause); + throw failure; + } + + function captureOptional(filePath) { + if (!filePath || inputs.has(filePath)) return; + try { + const fd = fs.openSync(filePath, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW); + try { + const stat = fs.fstatSync(fd); + if (!stat.isFile()) fail(filePath); + const bytes = fs.readFileSync(fd); + inputs.set(filePath, { bytes, hash: digest(bytes), dev: stat.dev, ino: stat.ino }); + } finally { + fs.closeSync(fd); + } + } catch { + // A resolved canonical path was present. Losing access to it must not be + // interpreted as an absent/empty patch (including allow-empty handlers). + fail(filePath); + } + } + + function assertUnchanged() { + if (failure) throw failure; + for (const [filePath, input] of inputs) { + try { + const fd = fs.openSync(filePath, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW); + try { + const stat = fs.fstatSync(fd); + if (!stat.isFile() || stat.dev !== input.dev || stat.ino !== input.ino || digest(fs.readFileSync(fd)) !== input.hash) fail(filePath); + } finally { + fs.closeSync(fd); + } + if (input.snapshot && digest(fs.readFileSync(input.snapshot)) !== digest(input.bytes)) fail(filePath); + } catch { + fail(filePath); + } + } + } + + function read(filePath) { + assertUnchanged(); + const input = inputs.get(filePath); + if (!input?.bytes) throw new Error(`No captured transport file: ${filePath}`); + return Buffer.from(input.bytes); + } + + function snapshot(filePath) { + const input = inputs.get(filePath); + if (!input?.bytes) return filePath; + try { + if (!input.snapshot) { + if (!directory) { + // Keep transport files outside the checkout: git add -A recovery must + // never stage the private validation inputs into the resulting commit. + directory = fs.mkdtempSync(path.join(os.tmpdir(), ".gh-aw-transport-")); + fs.chmodSync(directory, 0o700); + } + input.snapshot = path.join(directory, path.basename(filePath)); + fs.writeFileSync(input.snapshot, input.bytes, { mode: 0o400, flag: "wx" }); + } + } catch (error) { + fail(filePath, error); + } + return input.snapshot; + } + + // Only trusted transformations of previously captured validation bytes belong + // here. The original baseline is deliberately never updated from disk. + function transform(filePath, transformBytes) { + const bytes = transformBytes(read(filePath)); + const input = inputs.get(filePath); + if (!input) throw new Error(`No captured transport file: ${filePath}`); + if (input.snapshot) { + try { + fs.unlinkSync(input.snapshot); + } catch (error) { + fail(filePath, error); + } + input.snapshot = undefined; + } + input.bytes = Buffer.from(bytes); + } + + function guardExec(execApi) { + const guarded = { ...execApi }; + for (const method of ["exec", "getExecOutput"]) { + guarded[method] = async (command, args = [], ...options) => { + assertUnchanged(); + const stableArgs = command === "git" ? args.map(arg => (inputs.has(arg) ? snapshot(arg) : arg)) : args; + try { + return await execApi[method](command, stableArgs, ...options); + } finally { + // Also detect changes made while git was running, before any push/API + // side effects, including when the command itself failed. + assertUnchanged(); + } + }; + } + return guarded; + } + + function dispose() { + try { + if (directory) fs.rmSync(directory, { recursive: true, force: true }); + } catch (error) { + throw new Error("Failed to remove safe-output transport snapshots", { cause: error }); + } + } + + return { captureOptional, read, transform, assertUnchanged, guardExec, dispose }; +} + +function withTransportIntegrity(handler) { + return async (message, resolvedTemporaryIds) => { + const transport = createTransportIntegrity(); + try { + const result = await handler(message, resolvedTemporaryIds, transport); + transport.assertUnchanged(); + return result; + } catch (error) { + if (!(error instanceof TransportIntegrityError)) throw error; + return { success: false, error: error.message }; + } finally { + transport.dispose(); + } + }; +} + +module.exports = { createTransportIntegrity, withTransportIntegrity, TransportIntegrityError }; diff --git a/actions/setup/js/transport_integrity.test.cjs b/actions/setup/js/transport_integrity.test.cjs new file mode 100644 index 00000000000..66cf7d649b3 --- /dev/null +++ b/actions/setup/js/transport_integrity.test.cjs @@ -0,0 +1,187 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import fs from "fs"; +import path from "path"; +import os from "os"; +import { createTransportIntegrity, TransportIntegrityError, withTransportIntegrity } from "./transport_integrity.cjs"; + +describe("safe-output transport integrity", () => { + let directory; + let patch; + let bundle; + let transport; + + beforeEach(() => { + directory = fs.mkdtempSync(path.join(process.cwd(), ".transport-integrity-test-")); + patch = path.join(directory, "aw-test.patch"); + bundle = path.join(directory, "aw-test.bundle"); + fs.writeFileSync(patch, Buffer.from([0x61, 0xff, 0x62, 0x0a])); + fs.writeFileSync(bundle, "bundle original"); + transport = createTransportIntegrity(); + transport.captureOptional(patch); + transport.captureOptional(bundle); + }); + + afterEach(() => { + transport.dispose(); + fs.rmSync(directory, { recursive: true, force: true }); + }); + + it("validates and passes exactly the captured raw bytes through private read-only snapshots", async () => { + const expected = transport.read(patch); + let snapshotPath; + const api = { + exec: vi.fn(async (_command, args) => { + snapshotPath = args[2]; + expect(snapshotPath).not.toBe(patch); + expect(fs.readFileSync(snapshotPath)).toEqual(expected); + expect(fs.statSync(snapshotPath).mode & 0o777).toBe(0o400); + expect(fs.statSync(path.dirname(snapshotPath)).mode & 0o777).toBe(0o700); + expect(path.dirname(path.dirname(snapshotPath))).toBe(os.tmpdir()); + return 0; + }), + }; + await transport.guardExec(api).exec("git", ["am", "--3way", patch]); + transport.dispose(); + expect(fs.existsSync(snapshotPath)).toBe(false); + expect(fs.readFileSync(patch)).toEqual(expected); + }); + + it.each(["same-size mutation", "replacement", "deletion", "symlink"])("rejects %s before git reads a patch", async attack => { + if (attack === "same-size mutation") fs.writeFileSync(patch, Buffer.from([0x61, 0xfe, 0x62, 0x0a])); + else { + fs.renameSync(patch, `${patch}.original`); + if (attack === "replacement") fs.copyFileSync(`${patch}.original`, patch); + if (attack === "symlink") fs.symlinkSync(`${patch}.original`, patch); + } + const api = { exec: vi.fn() }; + await expect(transport.guardExec(api).exec("git", ["am", "--3way", patch])).rejects.toThrow(TransportIntegrityError); + expect(api.exec).not.toHaveBeenCalled(); + }); + + it("does not reopen the original even if it changes after the final pre-git check", async () => { + const original = transport.read(patch); + let applied; + const api = { + exec: vi.fn(async (_command, args) => { + fs.writeFileSync(patch, "evil"); + applied = fs.readFileSync(args[2]); + return 0; + }), + }; + await expect(transport.guardExec(api).exec("git", ["am", "--3way", patch])).rejects.toThrow(TransportIntegrityError); + expect(applied).toEqual(original); + }); + + it("fails closed when a private snapshot cannot be created", async () => { + const cause = new Error("disk full"); + const write = vi.spyOn(fs, "writeFileSync").mockImplementationOnce(() => { + throw cause; + }); + const api = { exec: vi.fn() }; + try { + await expect(transport.guardExec(api).exec("git", ["am", "--3way", patch])).rejects.toMatchObject({ name: "TransportIntegrityError", cause }); + await expect(transport.guardExec(api).exec("git", ["am", "--3way", patch])).rejects.toThrow(TransportIntegrityError); + expect(api.exec).not.toHaveBeenCalled(); + } finally { + write.mockRestore(); + } + }); + + it.each(["fetch", "list-heads", "verify"])("freezes bundle bytes for %s and rejects tampering before retries", async operation => { + const api = { + getExecOutput: vi.fn(async (_command, args) => { + const stablePath = args.find(arg => arg.endsWith(".bundle")); + expect(stablePath).not.toBe(bundle); + expect(fs.readFileSync(stablePath, "utf8")).toBe("bundle original"); + return { exitCode: 1, stdout: "", stderr: "retry needed" }; + }), + }; + const exec = transport.guardExec(api); + const args = operation === "fetch" ? ["fetch", bundle, "HEAD:refs/test"] : ["bundle", operation, bundle]; + await exec.getExecOutput("git", args); + fs.writeFileSync(bundle, "bundle tampered"); + await expect(exec.getExecOutput("git", args)).rejects.toThrow(TransportIntegrityError); + expect(api.getExecOutput).toHaveBeenCalledTimes(1); + }); + + it("reuses the identical bundle snapshot across head discovery and fetch retries", async () => { + const paths = []; + const api = { + getExecOutput: vi.fn(async (_command, args) => { + const snapshot = args.find(arg => arg.endsWith(".bundle")); + paths.push(snapshot); + expect(fs.readFileSync(snapshot, "utf8")).toBe("bundle original"); + return { exitCode: 0, stdout: "", stderr: "" }; + }), + }; + const exec = transport.guardExec(api); + await exec.getExecOutput("git", ["bundle", "verify", bundle]); + await exec.getExecOutput("git", ["fetch", bundle, "refs/heads/feature:refs/test"]); + await exec.getExecOutput("git", ["bundle", "list-heads", bundle]); + await exec.getExecOutput("git", ["fetch", bundle, "HEAD:refs/test"]); + expect(new Set(paths).size).toBe(1); + }); + + it("fails closed if a resolved artifact disappears before capture", () => { + fs.unlinkSync(patch); + const next = createTransportIntegrity(); + expect(() => next.captureOptional(patch)).toThrow(TransportIntegrityError); + next.dispose(); + }); + + it.each(["symlink", "directory"])("reports a %s capture as an integrity error", kind => { + fs.unlinkSync(patch); + if (kind === "symlink") fs.symlinkSync(bundle, patch); + else fs.mkdirSync(patch); + const next = createTransportIntegrity(); + expect(() => next.captureOptional(patch)).toThrow(TransportIntegrityError); + next.dispose(); + }); + + it("allows trusted transformations without changing or re-blessing the original baseline", async () => { + transport.dispose(); + fs.writeFileSync(patch, "Subject: [PATCH] Update\n+#aw_example\n"); + transport = createTransportIntegrity(); + transport.captureOptional(patch); + transport.transform(patch, bytes => Buffer.from(bytes.toString().replace("#aw_example", "#123"))); + transport.transform(patch, bytes => Buffer.from(bytes.toString().replace("Update", "Update [suffix]"))); + const api = { + exec: vi.fn(async (_command, args) => { + expect(fs.readFileSync(args[2], "utf8")).toBe("Subject: [PATCH] Update [suffix]\n+#123\n"); + return 0; + }), + }; + await transport.guardExec(api).exec("git", ["am", "--3way", patch]); + expect(fs.readFileSync(patch, "utf8")).toBe("Subject: [PATCH] Update\n+#aw_example\n"); + fs.writeFileSync(patch, "Subject: [PATCH] Update\n+#aw_changed\n"); + expect(() => transport.transform(patch, bytes => bytes)).toThrow(TransportIntegrityError); + }); + + it.each(["return", "throw", "tamper"])("cleans up snapshots on handler %s", async outcome => { + let snapshotPath; + const handler = withTransportIntegrity(async (_message, _ids, guard) => { + guard.captureOptional(patch); + await guard + .guardExec({ + exec: async (_command, args) => { + snapshotPath = args[2]; + return 0; + }, + }) + .exec("git", ["am", "--3way", patch]); + if (outcome === "throw") throw new Error("handler failure"); + if (outcome === "tamper") { + fs.unlinkSync(patch); + guard.assertUnchanged(); + } + return { success: true }; + }); + if (outcome === "throw") await expect(handler({}, {})).rejects.toThrow("handler failure"); + else { + const result = await handler({}, {}); + expect(result.success).toBe(outcome === "return"); + if (outcome === "tamper") expect(result.error).toContain("SECURITY:"); + } + expect(fs.existsSync(path.dirname(snapshotPath))).toBe(false); + }); +}); diff --git a/actions/setup/js/transport_integrity_handlers.test.cjs b/actions/setup/js/transport_integrity_handlers.test.cjs new file mode 100644 index 00000000000..d10e36a8063 --- /dev/null +++ b/actions/setup/js/transport_integrity_handlers.test.cjs @@ -0,0 +1,265 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createRequire } from "module"; +import fs from "fs"; +import path from "path"; +import { useExecutionProvenanceFixture } from "./test_execution_provenance.js"; + +useExecutionProvenanceFixture(); +const require = createRequire(import.meta.url); +const patchContent = `From ${"a".repeat(40)} Mon Sep 17 00:00:00 2001 +From: Test +Date: Thu, 1 Jan 2026 00:00:00 +0000 +Subject: [PATCH] Update + +diff --git a/file.txt b/file.txt +--- a/file.txt ++++ b/file.txt +@@ -1 +1 @@ +-old ++new +`; + +describe.each(["create_pull_request", "push_to_pull_request_branch"])("%s transport integrity", handlerName => { + let directory; + let patch; + let bundle; + let originalResolver; + let originalEnv; + let snapshots; + + beforeEach(() => { + originalEnv = { ...process.env }; + directory = fs.mkdtempSync(path.join(process.cwd(), ".transport-handler-test-")); + patch = path.join(directory, "aw-feature.patch"); + bundle = path.join(directory, "aw-feature.bundle"); + fs.writeFileSync(patch, patchContent); + snapshots = new Set(); + process.env.GH_AW_PROMPTS_DIR = path.resolve("..", "md"); + process.env.GITHUB_REPOSITORY = "test-owner/test-repo"; + process.env.GITHUB_BASE_REF = "main"; + process.env.GH_AW_WORKFLOW_ID = "integrity-test"; + delete process.env.GH_AW_SAFE_OUTPUTS_STAGED; + global.core = { + info: vi.fn(), + warning: vi.fn(), + debug: vi.fn(), + error: vi.fn(), + summary: { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue(undefined) }, + }; + const pr = { + number: 123, + state: "open", + title: "Test", + labels: [], + head: { ref: "feature", sha: "b".repeat(40), repo: { full_name: "test-owner/test-repo", owner: { login: "test-owner" } } }, + base: { ref: "main", sha: "c".repeat(40), repo: { full_name: "test-owner/test-repo", owner: { login: "test-owner" } } }, + }; + global.context = { + eventName: "pull_request", + repo: { owner: "test-owner", repo: "test-repo" }, + payload: { pull_request: pr, repository: { html_url: "https://github.com/test-owner/test-repo" } }, + }; + global.github = { + rest: { + pulls: { get: vi.fn().mockResolvedValue({ data: pr }), create: vi.fn(), list: vi.fn().mockResolvedValue({ data: [] }) }, + repos: { get: vi.fn().mockResolvedValue({ data: { default_branch: "main" } }), getBranchProtection: vi.fn().mockRejectedValue({ status: 404 }) }, + issues: { create: vi.fn(), addLabels: vi.fn(), addAssignees: vi.fn() }, + git: { getRef: vi.fn().mockRejectedValue({ status: 404 }), deleteRef: vi.fn(), createRef: vi.fn() }, + }, + graphql: vi.fn(), + }; + global.exec = { + exec: vi.fn().mockResolvedValue(0), + getExecOutput: vi.fn(async (_command, args) => { + if (args[0] === "rev-parse" && args[1] === "HEAD") return { exitCode: 0, stdout: "b".repeat(40), stderr: "" }; + if (args[0] === "diff" && args.includes("--name-only")) return { exitCode: 0, stdout: "file.txt\0", stderr: "" }; + return { exitCode: 0, stdout: "", stderr: "" }; + }), + }; + // Keep fixtures local; path-derivation security is covered independently by + // resolve_transport_paths.test.cjs. The handler still ignores message paths. + const resolver = require("./resolve_transport_paths.cjs"); + originalResolver = resolver.resolveTransportPaths; + resolver.resolveTransportPaths = () => ({ patchPath: patch, bundlePath: fs.existsSync(bundle) ? bundle : undefined }); + delete require.cache[require.resolve(`./${handlerName}.cjs`)]; + }); + + afterEach(() => { + require("./resolve_transport_paths.cjs").resolveTransportPaths = originalResolver; + delete require.cache[require.resolve(`./${handlerName}.cjs`)]; + for (const key of Object.keys(process.env)) if (!(key in originalEnv)) delete process.env[key]; + Object.assign(process.env, originalEnv); + for (const snapshot of snapshots) expect(fs.existsSync(path.dirname(snapshot))).toBe(false); + fs.rmSync(directory, { recursive: true, force: true }); + for (const key of ["core", "github", "context", "exec"]) delete global[key]; + }); + + async function run(config = {}, ids = {}) { + const { main } = require(`./${handlerName}.cjs`); + const handler = await main({ base_branch: "main", signed_commits: false, allow_empty: true, ...config }); + return handler({ branch: "feature", title: "Test", body: "Body", patch: "/untrusted.patch", bundle: "/untrusted.bundle" }, ids); + } + + function assertNoRemoteSideEffects() { + expect(global.github.rest.pulls.create).not.toHaveBeenCalled(); + expect(global.github.rest.issues.create).not.toHaveBeenCalled(); + expect(global.github.graphql).not.toHaveBeenCalled(); + expect(global.github.rest.git.deleteRef).not.toHaveBeenCalled(); + expect(global.github.rest.git.createRef).not.toHaveBeenCalled(); + expect(global.exec.exec.mock.calls.some(([, args]) => args[0] === "push")).toBe(false); + } + + it.each(["same-size mutation", "replacement", "deletion"])("fails closed on patch %s after validation", async attack => { + let attacked = false; + global.exec.exec.mockImplementation(async (_command, args) => { + if (!attacked && args[0] === "fetch") { + attacked = true; + if (attack === "same-size mutation") fs.writeFileSync(patch, patchContent.replace("+new", "+bad")); + if (attack === "replacement") { + fs.renameSync(patch, `${patch}.old`); + fs.writeFileSync(patch, patchContent); + } + if (attack === "deletion") fs.unlinkSync(patch); + } + return 0; + }); + const result = await run(); + expect(attacked).toBe(true); + expect(result.success).toBe(false); + expect(result.error).toContain("SECURITY:"); + expect(global.exec.exec.mock.calls.some(([, args]) => args[0] === "am")).toBe(false); + assertNoRemoteSideEffects(); + }); + + it("uses a stable patch during git am and does not recover tampering as a conflict", async () => { + let applied; + global.exec.exec.mockImplementation(async (_command, args) => { + if (args[0] === "am" && args[1] === "--3way") { + const snapshot = args[2]; + snapshots.add(snapshot); + fs.writeFileSync(patch, patchContent.replace("+new", "+bad")); + applied = fs.readFileSync(snapshot, "utf8"); + throw new Error("git conflict"); + } + return 0; + }); + const result = await run(); + expect(applied).toBe(patchContent); + expect(result.error).toContain("SECURITY:"); + expect(global.exec.exec.mock.calls.filter(([, args]) => args[0] === "am")).toHaveLength(1); + assertNoRemoteSideEffects(); + }); + + it.each(["patch", "bundle"])("rejects %s mutations after successful application and before publication", async mode => { + if (mode === "bundle") fs.writeFileSync(bundle, "bundle original"); + let attacked = false; + global.core.info.mockImplementation(message => { + const applied = mode === "patch" ? message === "Patch applied successfully" : handlerName === "create_pull_request" ? /^Checked out branch .* from bundle$/.test(message) : message.startsWith("Updated branch to bundle tip from "); + if (applied && !attacked) { + attacked = true; + fs.writeFileSync(mode === "bundle" ? bundle : patch, "tampered after import"); + } + }); + const result = await run(); + expect(attacked).toBe(true); + expect(result.success).toBe(false); + expect(result.error).toContain("SECURITY:"); + assertNoRemoteSideEffects(); + }); + + it.runIf(handlerName === "create_pull_request")("rejects mutation during collision ls-remote before deleting an existing ref", async () => { + const originalOutput = global.exec.getExecOutput.getMockImplementation(); + let attacked = false; + global.exec.getExecOutput.mockImplementation(async (command, args) => { + if (args[0] === "ls-remote" && args[1] === "--heads") { + attacked = true; + fs.writeFileSync(patch, "tampered during collision check"); + return { exitCode: 0, stdout: `${"b".repeat(40)} refs/heads/feature\n`, stderr: "" }; + } + return originalOutput(command, args); + }); + const result = await run({ preserve_branch_name: true, recreate_ref: true }); + expect(attacked).toBe(true); + expect(result.error).toContain("SECURITY:"); + assertNoRemoteSideEffects(); + }); + + it("checks integrity inside signed push after asynchronous commit validation and before remote mutation", async () => { + const originalOutput = global.exec.getExecOutput.getMockImplementation(); + const sha = "a".repeat(40); + const parent = "b".repeat(40); + let attacked = false; + global.exec.getExecOutput.mockImplementation(async (command, args) => { + if (args[0] === "rev-parse" && args[1]?.endsWith("^{commit}")) return { exitCode: 0, stdout: parent, stderr: "" }; + if (args[0] === "rev-list" && args.includes("--parents")) return { exitCode: 0, stdout: `${sha} ${parent}\n`, stderr: "" }; + if (args[0] === "ls-remote") return { exitCode: 0, stdout: `${parent} refs/heads/feature\n`, stderr: "" }; + if (args[0] === "log" && args.includes("--format=%B") && args.includes(sha)) { + attacked = true; + fs.writeFileSync(patch, "tampered during signed-push validation"); + return { exitCode: 0, stdout: "Update\n", stderr: "" }; + } + return originalOutput(command, args); + }); + const result = await run({ signed_commits: true }); + expect(attacked).toBe(true); + expect(result.error).toContain("SECURITY:"); + assertNoRemoteSideEffects(); + }); + + it.each(["initial import", "head discovery", "prerequisite recovery"])("freezes bundle import and refuses tampering during %s", async stage => { + fs.writeFileSync(bundle, "bundle original"); + const originalOutput = global.exec.getExecOutput.getMockImplementation(); + let imports = 0; + let attacked = false; + const tamper = () => { + attacked = true; + fs.writeFileSync(bundle, "bundle tampered"); + }; + global.exec.exec.mockImplementation(async (_command, args) => { + if (stage === "prerequisite recovery" && args[0] === "fetch" && args.includes("d".repeat(40))) tamper(); + return 0; + }); + global.exec.getExecOutput.mockImplementation(async (command, args) => { + if (args[0] === "fetch" && args.some(arg => arg.endsWith(".bundle"))) { + imports++; + const snapshot = args.find(arg => arg.endsWith(".bundle")); + snapshots.add(snapshot); + expect(fs.readFileSync(snapshot, "utf8")).toBe("bundle original"); + if (stage === "initial import") tamper(); + const stderr = stage === "head discovery" ? "couldn't find remote ref" : `Repository lacks these prerequisite commits:\n${"d".repeat(40)}\n`; + return { exitCode: 1, stdout: "", stderr }; + } + if (stage === "head discovery" && args[0] === "bundle" && args[1] === "list-heads") { + tamper(); + return { exitCode: 0, stdout: `${"e".repeat(40)} HEAD\n`, stderr: "" }; + } + return originalOutput(command, args); + }); + const result = await run(); + expect(attacked).toBe(true); + expect(imports).toBe(1); + expect(result.success).toBe(false); + expect(result.error).toContain("SECURITY:"); + assertNoRemoteSideEffects(); + }); + + it("applies trusted patch transformations from captured bytes without rewriting canonical artifacts", async () => { + const content = patchContent.replace("+new", "+#aw_example"); + fs.writeFileSync(patch, content); + let applied; + global.exec.exec.mockImplementation(async (_command, args) => { + if (args[0] === "am" && args[1] === "--3way") { + snapshots.add(args[2]); + applied = fs.readFileSync(args[2], "utf8"); + // Stop before push; no conflict recovery should bless or reread disk. + fs.unlinkSync(patch); + } + return 0; + }); + const result = await run({ commit_title_suffix: " [suffix]" }, { aw_example: { repo: "test-owner/test-repo", number: 123 } }); + expect(result.error).toContain("SECURITY:"); + if (handlerName === "create_pull_request") expect(applied).toContain("+#123"); + else expect(applied).toContain("Subject: [PATCH] Update [suffix]"); + assertNoRemoteSideEffects(); + }); +}); From 40c71f95ad05d3f2da56bc19e38e3f041495f7e9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 14 Oct 2026 15:05:29 +0000 Subject: [PATCH 2/7] Recheck safe-output transport integrity on PR creation retries Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../js/create_or_update_pull_request.cjs | 10 ++++--- .../js/create_or_update_pull_request.test.cjs | 30 +++++++++++++++++++ actions/setup/js/create_pull_request.cjs | 1 + 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/actions/setup/js/create_or_update_pull_request.cjs b/actions/setup/js/create_or_update_pull_request.cjs index 63d9bda75e0..bd5d2316746 100644 --- a/actions/setup/js/create_or_update_pull_request.cjs +++ b/actions/setup/js/create_or_update_pull_request.cjs @@ -3,10 +3,11 @@ const { withRetry, RATE_LIMIT_RETRY_CONFIG } = require("./error_recovery.cjs"); async function createOrUpdatePullRequest(options) { - const { githubClient, repoParts, title, body, branchName, headRepo, baseBranch, draft } = options; + const { githubClient, repoParts, title, body, branchName, headRepo, baseBranch, draft, assertTransportUnchanged } = options; return withRetry( - () => - githubClient.rest.pulls.create({ + () => { + assertTransportUnchanged?.(); + return githubClient.rest.pulls.create({ owner: repoParts.owner, repo: repoParts.repo, title, @@ -15,7 +16,8 @@ async function createOrUpdatePullRequest(options) { ...(headRepo ? { head_repo: headRepo } : {}), base: baseBranch, draft, - }), + }); + }, RATE_LIMIT_RETRY_CONFIG, `create pull request in ${repoParts.owner}/${repoParts.repo}` ); diff --git a/actions/setup/js/create_or_update_pull_request.test.cjs b/actions/setup/js/create_or_update_pull_request.test.cjs index 01c96e0532c..bd47068aa8c 100644 --- a/actions/setup/js/create_or_update_pull_request.test.cjs +++ b/actions/setup/js/create_or_update_pull_request.test.cjs @@ -28,4 +28,34 @@ describe("createOrUpdatePullRequest", () => { await createOrUpdatePullRequest({ ...options, githubClient }); expect(githubClient.rest.pulls.create).toHaveBeenCalledWith(expect.not.objectContaining({ head_repo: expect.anything() })); }); + + it("rechecks transport integrity before a retried API write", async () => { + vi.useFakeTimers(); + vi.stubGlobal("core", { info: vi.fn(), debug: vi.fn(), warning: vi.fn() }); + let tampered = false; + const githubClient = { + rest: { + pulls: { + create: vi.fn(async () => { + tampered = true; + throw Object.assign(new Error("Service unavailable"), { status: 503 }); + }), + }, + }, + }; + const assertTransportUnchanged = vi.fn(() => { + if (tampered) throw new Error("SECURITY: Safe-output transport changed after capture"); + }); + try { + const result = createOrUpdatePullRequest({ ...options, githubClient, assertTransportUnchanged }); + const rejection = expect(result).rejects.toThrow("SECURITY:"); + await vi.runAllTimersAsync(); + await rejection; + expect(assertTransportUnchanged).toHaveBeenCalledTimes(2); + expect(githubClient.rest.pulls.create).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + vi.unstubAllGlobals(); + } + }); }); diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 21969dfc300..60f5b3ba9d8 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -2608,6 +2608,7 @@ ${issueSafeFallbackFooter}`; } transport.assertUnchanged(); const { data: pullRequest } = await createOrUpdatePullRequest({ + assertTransportUnchanged: transport.assertUnchanged, githubClient, repoParts, title, From 016dc4113d84048551449c2a150ed39e2b0a3621 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Wed, 14 Oct 2026 11:31:14 -0700 Subject: [PATCH 3/7] Fix safe-output transport guard regressions Reject nonregular artifacts without blocking, exclude permanent integrity failures from transient retries, preserve filesystem diagnostics, and permit only registered local Git cleanup after failure. Avoid full artifact scans around unrelated commands while retaining byte verification at trust boundaries. Fixes #68786 Fixes #68787 Fixes #68788 Fixes #68789 Fixes #68790 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../js/create_or_update_pull_request.test.cjs | 24 +- actions/setup/js/error_recovery.cjs | 1 + actions/setup/js/error_recovery.test.cjs | 5 + actions/setup/js/transport_integrity.cjs | 162 ++++++++++--- actions/setup/js/transport_integrity.test.cjs | 220 +++++++++++++++++- .../setup/js/transport_integrity_git.test.cjs | 113 +++++++++ .../js/transport_integrity_handlers.test.cjs | 3 +- 7 files changed, 486 insertions(+), 42 deletions(-) create mode 100644 actions/setup/js/transport_integrity_git.test.cjs diff --git a/actions/setup/js/create_or_update_pull_request.test.cjs b/actions/setup/js/create_or_update_pull_request.test.cjs index bd47068aa8c..128fd8c2d9d 100644 --- a/actions/setup/js/create_or_update_pull_request.test.cjs +++ b/actions/setup/js/create_or_update_pull_request.test.cjs @@ -3,6 +3,7 @@ import { createRequire } from "module"; const require = createRequire(import.meta.url); const { createOrUpdatePullRequest } = require("./create_or_update_pull_request.cjs"); +const { TransportIntegrityError } = require("./transport_integrity.cjs"); describe("createOrUpdatePullRequest", () => { const options = { @@ -29,7 +30,7 @@ describe("createOrUpdatePullRequest", () => { expect(githubClient.rest.pulls.create).toHaveBeenCalledWith(expect.not.objectContaining({ head_repo: expect.anything() })); }); - it("rechecks transport integrity before a retried API write", async () => { + it.each(["feature", "fix-network-timeout"])("rechecks transport integrity before a retried API write for %s", async branch => { vi.useFakeTimers(); vi.stubGlobal("core", { info: vi.fn(), debug: vi.fn(), warning: vi.fn() }); let tampered = false; @@ -44,7 +45,7 @@ describe("createOrUpdatePullRequest", () => { }, }; const assertTransportUnchanged = vi.fn(() => { - if (tampered) throw new Error("SECURITY: Safe-output transport changed after capture"); + if (tampered) throw new TransportIntegrityError(`/tmp/gh-aw/aw-${branch}.patch`); }); try { const result = createOrUpdatePullRequest({ ...options, githubClient, assertTransportUnchanged }); @@ -53,6 +54,25 @@ describe("createOrUpdatePullRequest", () => { await rejection; expect(assertTransportUnchanged).toHaveBeenCalledTimes(2); expect(githubClient.rest.pulls.create).toHaveBeenCalledTimes(1); + expect(core.info.mock.calls.filter(([message]) => message.startsWith("Retry attempt"))).toHaveLength(1); + } finally { + vi.useRealTimers(); + vi.unstubAllGlobals(); + } + }); + + it.each(["network", "timeout", "rate-limit"])("does not retry a pre-write integrity failure containing %s", async branch => { + vi.useFakeTimers(); + vi.stubGlobal("core", { info: vi.fn(), debug: vi.fn(), warning: vi.fn() }); + const githubClient = { rest: { pulls: { create: vi.fn() } } }; + const assertTransportUnchanged = vi.fn(() => { + throw new TransportIntegrityError(`/tmp/gh-aw/aw-${branch}.patch`); + }); + try { + await expect(createOrUpdatePullRequest({ ...options, githubClient, assertTransportUnchanged })).rejects.toThrow("SECURITY:"); + expect(assertTransportUnchanged).toHaveBeenCalledTimes(1); + expect(githubClient.rest.pulls.create).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); } finally { vi.useRealTimers(); vi.unstubAllGlobals(); diff --git a/actions/setup/js/error_recovery.cjs b/actions/setup/js/error_recovery.cjs index 2a69481bb04..326d1c159ef 100644 --- a/actions/setup/js/error_recovery.cjs +++ b/actions/setup/js/error_recovery.cjs @@ -78,6 +78,7 @@ function hasRateLimitIndicator(messageLower) { * @returns {boolean} True if the error is transient and should be retried */ function isTransientError(error) { + if (error?.code === "ERR_TRANSPORT_INTEGRITY") return false; const errorMsg = getErrorMessage(error); const errorMsgLower = errorMsg.trimStart().toLowerCase(); const status = error?.response?.status ?? error?.status ?? null; diff --git a/actions/setup/js/error_recovery.test.cjs b/actions/setup/js/error_recovery.test.cjs index ebf7fc0cd68..b8342c0f47b 100644 --- a/actions/setup/js/error_recovery.test.cjs +++ b/actions/setup/js/error_recovery.test.cjs @@ -12,6 +12,7 @@ global.core = { }; import { withRetry, isTransientError, getRetryAfterMs, enhanceError, createValidationError, createOperationError, DEFAULT_RETRY_CONFIG, RATE_LIMIT_RETRY_CONFIG } from "./error_recovery.cjs"; +import { TransportIntegrityError } from "./transport_integrity.cjs"; describe("error_recovery", () => { beforeEach(() => { @@ -19,6 +20,10 @@ describe("error_recovery", () => { }); describe("isTransientError", () => { + it.each(["network", "timeout", "rate limit", "secondary rate limit", "econnreset"])("never retries transport-integrity errors whose paths contain %s", text => { + expect(isTransientError(new TransportIntegrityError(`/tmp/gh-aw/aw-${text}.patch`))).toBe(false); + }); + it("should identify network errors as transient", () => { expect(isTransientError(new Error("Network error occurred"))).toBe(true); expect(isTransientError(new Error("ECONNRESET"))).toBe(true); diff --git a/actions/setup/js/transport_integrity.cjs b/actions/setup/js/transport_integrity.cjs index 1d960e4bd02..fbfa9f771b3 100644 --- a/actions/setup/js/transport_integrity.cjs +++ b/actions/setup/js/transport_integrity.cjs @@ -4,11 +4,17 @@ const fs = require("fs"); const path = require("path"); const os = require("os"); const crypto = require("crypto"); +const READ_FLAGS = fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW | fs.constants.O_NONBLOCK; class TransportIntegrityError extends Error { - constructor(filePath, cause = undefined) { - super(`SECURITY: Safe-output transport changed after capture: ${filePath}`, { cause }); + /** @param {unknown} [cause] */ + constructor(filePath, cause = undefined, operation = "verify", reason = "artifact changed") { + const systemCode = cause && typeof cause === "object" && "code" in cause && typeof cause.code === "string" && /^E[A-Z0-9]+$/.test(cause.code) ? ` (${cause.code})` : ""; + super(`SECURITY: Safe-output transport changed after capture: ${filePath}; ${operation}: ${reason}${systemCode}`, { cause }); this.name = "TransportIntegrityError"; + this.code = "ERR_TRANSPORT_INTEGRITY"; + this.operation = operation; + this.reason = reason; } } @@ -19,54 +25,69 @@ const digest = bytes => crypto.createHash("sha256").update(bytes).digest("hex"); * the agent-writable originals, so checking a hash cannot race with git's open(). */ function createTransportIntegrity() { - /** @type {Map} */ + /** @typedef {{hash: string, dev: number, ino: number, size: number}} FileIdentity */ + /** @type {Map} */ const inputs = new Map(); + /** @type {Map Promise>} */ + const cleanups = new Map(); let directory = ""; /** @type {TransportIntegrityError | null} */ let failure = null; - function fail(filePath, cause = undefined) { - failure ||= new TransportIntegrityError(filePath, cause); + /** @returns {never} */ + function fail(filePath, cause = undefined, operation = "verify", reason = "unable to read artifact") { + failure ||= new TransportIntegrityError(filePath, cause, operation, reason); throw failure; } function captureOptional(filePath) { if (!filePath || inputs.has(filePath)) return; try { - const fd = fs.openSync(filePath, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW); + const fd = fs.openSync(filePath, READ_FLAGS); try { const stat = fs.fstatSync(fd); - if (!stat.isFile()) fail(filePath); + if (!stat.isFile()) fail(filePath, undefined, "capture", "not a regular file"); const bytes = fs.readFileSync(fd); - inputs.set(filePath, { bytes, hash: digest(bytes), dev: stat.dev, ino: stat.ino }); + inputs.set(filePath, { bytes, hash: digest(bytes), dev: stat.dev, ino: stat.ino, size: stat.size }); } finally { fs.closeSync(fd); } - } catch { + } catch (error) { // A resolved canonical path was present. Losing access to it must not be // interpreted as an absent/empty patch (including allow-empty handlers). - fail(filePath); + fail(filePath, error, "capture"); } } - function assertUnchanged() { - if (failure) throw failure; - for (const [filePath, input] of inputs) { + /** @param {FileIdentity} identity */ + function verifyFile(filePath, identity, contents) { + try { + const fd = fs.openSync(filePath, READ_FLAGS); try { - const fd = fs.openSync(filePath, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW); - try { - const stat = fs.fstatSync(fd); - if (!stat.isFile() || stat.dev !== input.dev || stat.ino !== input.ino || digest(fs.readFileSync(fd)) !== input.hash) fail(filePath); - } finally { - fs.closeSync(fd); - } - if (input.snapshot && digest(fs.readFileSync(input.snapshot)) !== digest(input.bytes)) fail(filePath); - } catch { - fail(filePath); + const stat = fs.fstatSync(fd); + if (!stat.isFile()) fail(filePath, undefined, "verify", "not a regular file"); + if (stat.dev !== identity.dev || stat.ino !== identity.ino) fail(filePath, undefined, "verify", "file identity changed"); + if (stat.size !== identity.size || (contents && digest(fs.readFileSync(fd)) !== identity.hash)) fail(filePath, undefined, "verify", "content changed"); + } finally { + fs.closeSync(fd); } + } catch (error) { + fail(filePath, error); + } + } + + function checkUnchanged(contents) { + if (failure) throw failure; + for (const [filePath, input] of inputs) { + verifyFile(filePath, input, contents); + if (input.snapshot) verifyFile(input.snapshot.path, input.snapshot, contents); } } + function assertUnchanged() { + checkUnchanged(true); + } + function read(filePath) { assertUnchanged(); const input = inputs.get(filePath); @@ -85,13 +106,15 @@ function createTransportIntegrity() { directory = fs.mkdtempSync(path.join(os.tmpdir(), ".gh-aw-transport-")); fs.chmodSync(directory, 0o700); } - input.snapshot = path.join(directory, path.basename(filePath)); - fs.writeFileSync(input.snapshot, input.bytes, { mode: 0o400, flag: "wx" }); + const snapshotPath = path.join(directory, path.basename(filePath)); + fs.writeFileSync(snapshotPath, input.bytes, { mode: 0o400, flag: "wx" }); + const stat = fs.statSync(snapshotPath); + input.snapshot = { path: snapshotPath, hash: digest(input.bytes), dev: stat.dev, ino: stat.ino, size: stat.size }; } } catch (error) { - fail(filePath, error); + fail(filePath, error, "snapshot", "unable to create snapshot"); } - return input.snapshot; + return input.snapshot.path; } // Only trusted transformations of previously captured validation bytes belong @@ -102,9 +125,9 @@ function createTransportIntegrity() { if (!input) throw new Error(`No captured transport file: ${filePath}`); if (input.snapshot) { try { - fs.unlinkSync(input.snapshot); + fs.unlinkSync(input.snapshot.path); } catch (error) { - fail(filePath, error); + fail(filePath, error, "transform", "unable to remove snapshot"); } input.snapshot = undefined; } @@ -115,20 +138,66 @@ function createTransportIntegrity() { const guarded = { ...execApi }; for (const method of ["exec", "getExecOutput"]) { guarded[method] = async (command, args = [], ...options) => { - assertUnchanged(); + const cleanupKey = cleanupArgs => JSON.stringify([options[0]?.cwd || process.cwd(), cleanupArgs]); + const key = cleanupKey(args); + // Only cleanup registered by this guard may bypass a sticky failure. + if (command === "git" && cleanups.has(key)) { + const result = await execApi[method](command, args, ...options); + if ((method === "exec" ? result : result.exitCode) === 0) cleanups.delete(key); + return result; + } + const usesTransport = command === "git" && args.some(arg => inputs.has(arg)); + const verifyContents = usesTransport || (command === "git" && args[0] === "push"); + // Full byte checks remain mandatory at reads, application, publication, + // and handler completion. Unrelated local commands only check identity. + checkUnchanged(verifyContents); const stableArgs = command === "git" ? args.map(arg => (inputs.has(arg) ? snapshot(arg) : arg)) : args; + const cleanupOptions = [{ ...options[0], cwd: options[0]?.cwd || process.cwd() }, ...options.slice(1)]; + let amCleanupKey; + if (usesTransport && args[0] === "am" && args[1] === "--3way") { + amCleanupKey = cleanupKey(["am", "--abort"]); + cleanups.set(amCleanupKey, () => execApi.exec("git", ["am", "--abort"], ...cleanupOptions)); + } + if (usesTransport && args[0] === "fetch") { + for (const refspec of args.slice(2)) { + const ref = refspec.split(":")[1]; + if (ref?.startsWith("refs/bundles/")) { + const cleanup = ["update-ref", "-d", ref]; + cleanups.set(cleanupKey(cleanup), () => execApi.exec("git", cleanup, ...cleanupOptions)); + } + } + } try { - return await execApi[method](command, stableArgs, ...options); + const result = await execApi[method](command, stableArgs, ...options); + if ((method === "exec" ? result : result.exitCode) === 0) { + if (amCleanupKey) cleanups.delete(amCleanupKey); + if (command === "git" && args[0] === "am" && args[1] === "--continue") cleanups.delete(cleanupKey(["am", "--abort"])); + } + return result; } finally { - // Also detect changes made while git was running, before any push/API - // side effects, including when the command itself failed. - assertUnchanged(); + // Detect identity changes immediately; apply/push commands also + // recheck bytes, including when the command itself failed. + checkUnchanged(verifyContents); } }; } return guarded; } + async function cleanup() { + const errors = []; + for (const run of cleanups.values()) { + try { + const exitCode = await run(); + if (typeof exitCode === "number" && exitCode !== 0) throw new Error(`Git cleanup exited with code ${exitCode}`); + } catch (error) { + errors.push(error); + } + } + cleanups.clear(); + if (errors.length) throw new AggregateError(errors, "Failed to clean up safe-output Git state", { cause: failure }); + } + function dispose() { try { if (directory) fs.rmSync(directory, { recursive: true, force: true }); @@ -137,7 +206,18 @@ function createTransportIntegrity() { } } - return { captureOptional, read, transform, assertUnchanged, guardExec, dispose }; + return { + captureOptional, + read, + transform, + assertUnchanged, + guardExec, + cleanup, + dispose, + get failure() { + return failure; + }, + }; } function withTransportIntegrity(handler) { @@ -148,10 +228,16 @@ function withTransportIntegrity(handler) { transport.assertUnchanged(); return result; } catch (error) { - if (!(error instanceof TransportIntegrityError)) throw error; - return { success: false, error: error.message }; + const integrityError = transport.failure || (error instanceof TransportIntegrityError ? error : null); + if (!integrityError) throw error; + if (typeof core !== "undefined") core.error(integrityError.message); + return { success: false, error: integrityError.message, error_type: integrityError.code }; } finally { - transport.dispose(); + try { + await transport.cleanup(); + } finally { + transport.dispose(); + } } }; } diff --git a/actions/setup/js/transport_integrity.test.cjs b/actions/setup/js/transport_integrity.test.cjs index 66cf7d649b3..be6bc15a00c 100644 --- a/actions/setup/js/transport_integrity.test.cjs +++ b/actions/setup/js/transport_integrity.test.cjs @@ -2,6 +2,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import fs from "fs"; import path from "path"; import os from "os"; +import crypto from "crypto"; +import { spawnSync } from "child_process"; import { createTransportIntegrity, TransportIntegrityError, withTransportIntegrity } from "./transport_integrity.cjs"; describe("safe-output transport integrity", () => { @@ -125,10 +127,226 @@ describe("safe-output transport integrity", () => { it("fails closed if a resolved artifact disappears before capture", () => { fs.unlinkSync(patch); const next = createTransportIntegrity(); - expect(() => next.captureOptional(patch)).toThrow(TransportIntegrityError); + expect(() => next.captureOptional(patch)).toThrow(expect.objectContaining({ name: "TransportIntegrityError", operation: "capture", cause: expect.objectContaining({ code: "ENOENT" }) })); next.dispose(); }); + it.skipIf(process.platform === "win32").each(["capture", "verify", "snapshot"])("rejects FIFO input without blocking during %s", operation => { + const filename = path.join(directory, "fifo.patch"); + const result = spawnSync( + process.execPath, + [ + "-e", + ` + const fs = require("fs"); + const { execFileSync } = require("child_process"); + require("os").tmpdir = () => require("path").dirname(process.argv[2]); + async function main() { + const guard = require(process.argv[1]).createTransportIntegrity(); + let filename = process.argv[2]; + if (process.argv[3] !== "capture") { + fs.writeFileSync(filename, "original"); + guard.captureOptional(filename); + if (process.argv[3] === "snapshot") { + await guard.guardExec({exec: async (_command, args) => { + filename = args[2]; + return 0; + }}).exec("git", ["am", "--3way", filename]); + } + fs.unlinkSync(filename); + } + execFileSync("mkfifo", [filename]); + try { + if (process.argv[3] === "capture") guard.captureOptional(filename); + else guard.assertUnchanged(); + process.exitCode = 1; + } catch (error) { + if (error.code !== "ERR_TRANSPORT_INTEGRITY" || error.reason !== "not a regular file") throw error; + } finally { + guard.dispose(); + } + } + main().catch(error => { console.error(error); process.exitCode = 1; }); + `, + path.resolve("transport_integrity.cjs"), + filename, + operation, + ], + { timeout: 2000, encoding: "utf8" } + ); + expect(result.error).toBeUndefined(); + expect(result.stderr).toBe(""); + expect(result.status).toBe(0); + }); + + it.each(["openSync", "readFileSync"])("preserves the filesystem cause and operation when %s fails", method => { + const cause = Object.assign(new Error("private diagnostic detail"), { code: method === "openSync" ? "EACCES" : "EIO" }); + const spy = vi.spyOn(fs, method).mockImplementationOnce(() => { + throw cause; + }); + try { + expect(() => transport.assertUnchanged()).toThrow(expect.objectContaining({ cause, operation: "verify", reason: "unable to read artifact" })); + expect(() => transport.assertUnchanged()).toThrow(cause.code); + expect(() => transport.assertUnchanged()).not.toThrow("private diagnostic detail"); + } finally { + spy.mockRestore(); + } + }); + + it("distinguishes identity replacement from a same-size content change", () => { + fs.writeFileSync(patch, Buffer.from([0x61, 0xfe, 0x62, 0x0a])); + expect(() => transport.assertUnchanged()).toThrow(expect.objectContaining({ reason: "content changed" })); + const next = createTransportIntegrity(); + try { + next.captureOptional(patch); + fs.renameSync(patch, `${patch}.old`); + fs.copyFileSync(`${patch}.old`, patch); + expect(() => next.assertUnchanged()).toThrow(expect.objectContaining({ reason: "file identity changed" })); + } finally { + next.dispose(); + } + }); + + it("does not read or hash full artifacts for unrelated local commands, but verifies bytes before publishing", async () => { + transport.dispose(); + fs.writeFileSync(bundle, Buffer.alloc(8 * 1024 * 1024)); + transport = createTransportIntegrity(); + transport.captureOptional(patch); + transport.captureOptional(bundle); + const api = { exec: vi.fn(async () => 0) }; + const exec = transport.guardExec(api); + await exec.exec("git", ["bundle", "verify", bundle]); + const read = vi.spyOn(fs, "readFileSync"); + const hash = vi.spyOn(crypto, "createHash"); + try { + for (let i = 0; i < 100; i++) await exec.exec("git", ["rev-parse", "HEAD"]); + expect(read).not.toHaveBeenCalled(); + expect(hash).not.toHaveBeenCalled(); + transport.assertUnchanged(); + expect(read).toHaveBeenCalledTimes(3); + expect(hash).toHaveBeenCalledTimes(3); + fs.writeFileSync(patch, Buffer.from([0x61, 0xfe, 0x62, 0x0a])); + await expect(exec.exec("git", ["push", "origin", "feature"])).rejects.toThrow(TransportIntegrityError); + expect(api.exec.mock.calls.some(([, args]) => args[0] === "push")).toBe(false); + } finally { + read.mockRestore(); + hash.mockRestore(); + } + }); + + it("rechecks private snapshot bytes before application", async () => { + let snapshot; + const api = { + exec: vi.fn(async (_command, args) => { + snapshot = args[2]; + return 0; + }), + }; + const exec = transport.guardExec(api); + await exec.exec("git", ["bundle", "verify", bundle]); + fs.chmodSync(snapshot, 0o600); + fs.writeFileSync(snapshot, "bundle modified"); + await expect(exec.exec("git", ["fetch", bundle, "HEAD:refs/test"])).rejects.toThrow(TransportIntegrityError); + expect(api.exec).toHaveBeenCalledTimes(1); + }); + + it("reports structured integrity diagnostics and logs a safe error", async () => { + vi.stubGlobal("core", { error: vi.fn() }); + try { + const handler = withTransportIntegrity(async (_message, _ids, guard) => { + guard.captureOptional(`${patch}.missing`); + }); + const result = await handler({}, {}); + expect(result).toMatchObject({ success: false, error_type: "ERR_TRANSPORT_INTEGRITY" }); + expect(result.error).toContain("capture: unable to read artifact (ENOENT)"); + expect(core.error).toHaveBeenCalledWith(result.error); + } finally { + vi.unstubAllGlobals(); + } + }); + + it("preserves integrity diagnostics when a handler wraps the original error", async () => { + const handler = withTransportIntegrity(async (_message, _ids, guard) => { + try { + guard.captureOptional(`${patch}.missing`); + } catch (error) { + throw new Error("Failed to read patch", { cause: error }); + } + }); + await expect(handler({}, {})).resolves.toMatchObject({ success: false, error_type: "ERR_TRANSPORT_INTEGRITY", error: expect.stringContaining("ENOENT") }); + }); + + it("only permits registered local cleanup after failure, never remote writes or arbitrary ref deletion", async () => { + const api = { + exec: vi.fn(async (_command, args) => { + if (args[0] === "am" && args[1] === "--3way") { + fs.unlinkSync(patch); + throw new Error("conflict"); + } + return 0; + }), + }; + const handler = withTransportIntegrity(async (_message, _ids, guard) => { + guard.captureOptional(patch); + const exec = guard.guardExec(api); + await expect(exec.exec("git", ["am", "--3way", patch])).rejects.toThrow(TransportIntegrityError); + for (const args of [ + ["push", "origin", "feature"], + ["update-ref", "-d", "refs/heads/main"], + ["update-ref", "-d", "refs/bundles/not-registered"], + ]) { + await expect(exec.exec("git", args)).rejects.toThrow(TransportIntegrityError); + } + }); + await expect(handler({}, {})).resolves.toMatchObject({ success: false }); + expect(api.exec.mock.calls.map(([, args]) => args.slice(0, 2))).toEqual([ + ["am", "--3way"], + ["am", "--abort"], + ]); + }); + + it("surfaces cleanup errors and still removes private snapshots", async () => { + let snapshotPath; + const handler = withTransportIntegrity(async (_message, _ids, guard) => { + guard.captureOptional(patch); + await guard + .guardExec({ + exec: async (_command, args) => { + if (args[1] === "--abort") throw new Error("abort failed"); + snapshotPath = args[2]; + fs.unlinkSync(patch); + throw new Error("conflict"); + }, + }) + .exec("git", ["am", "--3way", patch]); + }); + await expect(handler({}, {})).rejects.toMatchObject({ message: "Failed to clean up safe-output Git state", errors: [expect.objectContaining({ message: "abort failed" })] }); + expect(fs.existsSync(path.dirname(snapshotPath))).toBe(false); + }); + + it("pins cleanup to the original checkout even when caller options change", async () => { + const options = { cwd: directory }; + const api = { + exec: vi.fn(async (_command, args) => { + if (args[1] === "--3way") { + fs.unlinkSync(patch); + throw new Error("conflict"); + } + return 0; + }), + }; + const handler = withTransportIntegrity(async (_message, _ids, guard) => { + guard.captureOptional(patch); + try { + await guard.guardExec(api).exec("git", ["am", "--3way", patch], options); + } finally { + options.cwd = path.dirname(directory); + } + }); + await expect(handler({}, {})).resolves.toMatchObject({ success: false }); + expect(api.exec).toHaveBeenCalledWith("git", ["am", "--abort"], expect.objectContaining({ cwd: directory })); + }); + it.each(["symlink", "directory"])("reports a %s capture as an integrity error", kind => { fs.unlinkSync(patch); if (kind === "symlink") fs.symlinkSync(bundle, patch); diff --git a/actions/setup/js/transport_integrity_git.test.cjs b/actions/setup/js/transport_integrity_git.test.cjs new file mode 100644 index 00000000000..2d623b0db04 --- /dev/null +++ b/actions/setup/js/transport_integrity_git.test.cjs @@ -0,0 +1,113 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createRequire } from "module"; +import { spawnSync } from "child_process"; +import fs from "fs"; +import os from "os"; +import path from "path"; +import { withTransportIntegrity } from "./transport_integrity.cjs"; +import { useExecutionProvenanceFixture } from "./test_execution_provenance.js"; + +useExecutionProvenanceFixture(); +const require = createRequire(import.meta.url); + +describe("transport integrity Git cleanup", () => { + let directory; + let repo; + + function git(args) { + const result = spawnSync("git", args, { + cwd: repo, + encoding: "utf8", + env: { ...process.env, GIT_CONFIG_GLOBAL: os.devNull, GIT_CONFIG_NOSYSTEM: "1" }, + }); + if (result.error) throw result.error; + return { exitCode: result.status, stdout: result.stdout, stderr: result.stderr }; + } + + function mustGit(args) { + const result = git(args); + if (result.exitCode !== 0) throw new Error(result.stderr); + return result.stdout; + } + + function execApi(onCommand) { + const run = async (command, args, options = {}) => { + if (command !== "git") throw new Error(`Unexpected command: ${command}`); + const result = git(args); + onCommand(args, result); + if (result.exitCode !== 0 && !options.ignoreReturnCode) throw new Error(result.stderr); + return result; + }; + return { + exec: vi.fn(async (command, args, options) => (await run(command, args, options)).exitCode), + getExecOutput: vi.fn(run), + }; + } + + beforeEach(() => { + directory = fs.mkdtempSync(path.join(process.cwd(), ".transport-git-test-")); + repo = path.join(directory, "repo"); + fs.mkdirSync(repo); + mustGit(["init", "-q", "-b", "main"]); + mustGit(["config", "user.name", "Test"]); + mustGit(["config", "user.email", "test@example.com"]); + mustGit(["config", "commit.gpgsign", "false"]); + fs.writeFileSync(path.join(repo, "file.txt"), "base\n"); + mustGit(["add", "file.txt"]); + mustGit(["commit", "-qm", "base"]); + vi.stubGlobal("core", { info: vi.fn(), warning: vi.fn(), error: vi.fn(), debug: vi.fn() }); + vi.stubEnv("GH_AW_PROMPTS_DIR", path.resolve("..", "md")); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.unstubAllEnvs(); + fs.rmSync(directory, { recursive: true, force: true }); + }); + + it("deletes imported temporary bundle refs after an integrity failure without publishing", async () => { + mustGit(["branch", "feature"]); + const bundle = path.join(directory, "aw-feature.bundle"); + mustGit(["bundle", "create", bundle, "feature"]); + const api = execApi(args => { + if (args[0] === "fetch" && args[1].endsWith(".bundle")) fs.writeFileSync(bundle, "changed after import"); + }); + const { applyBundleToBranch } = require("./create_pull_request.cjs"); + const handler = withTransportIntegrity(async (_message, _ids, transport) => { + transport.captureOptional(bundle); + await applyBundleToBranch(bundle, "result", "feature", transport.guardExec(api), "main"); + return { success: true }; + }); + await expect(handler({}, {})).resolves.toMatchObject({ success: false, error_type: "ERR_TRANSPORT_INTEGRITY" }); + expect(mustGit(["for-each-ref", "--format=%(refname)", "refs/bundles/"]).trim()).toBe(""); + expect(api.exec.mock.calls.some(([, args]) => args[0] === "push")).toBe(false); + expect(api.exec).toHaveBeenCalledWith("git", ["update-ref", "-d", expect.stringMatching(/^refs\/bundles\/create-pr-result-/)]); + }); + + it.each([false, true])("aborts a real patch conflict and restores the checkout (transport changed: %s)", async tamper => { + mustGit(["checkout", "-qb", "feature"]); + fs.writeFileSync(path.join(repo, "file.txt"), "feature\n"); + mustGit(["commit", "-qam", "feature"]); + const patch = path.join(directory, "aw-feature.patch"); + fs.writeFileSync(patch, mustGit(["format-patch", "-1", "--stdout"])); + mustGit(["checkout", "-q", "main"]); + fs.writeFileSync(path.join(repo, "file.txt"), "target\n"); + mustGit(["commit", "-qam", "target"]); + const before = mustGit(["rev-parse", "HEAD"]); + const api = execApi(args => { + if (tamper && args[0] === "am" && args[1] === "--3way") fs.writeFileSync(patch, "changed during apply"); + }); + const handler = withTransportIntegrity(async (_message, _ids, transport) => { + transport.captureOptional(patch); + await transport.guardExec(api).exec("git", ["am", "--3way", patch], { cwd: repo }); + return { success: true }; + }); + if (tamper) await expect(handler({}, {})).resolves.toMatchObject({ success: false, error_type: "ERR_TRANSPORT_INTEGRITY" }); + else await expect(handler({}, {})).rejects.toThrow(); + expect(fs.existsSync(path.join(repo, ".git", "rebase-apply"))).toBe(false); + expect(mustGit(["rev-parse", "HEAD"])).toBe(before); + expect(fs.readFileSync(path.join(repo, "file.txt"), "utf8")).toBe("target\n"); + expect(mustGit(["status", "--porcelain"])).toBe(""); + expect(api.exec).toHaveBeenCalledWith("git", ["am", "--abort"], expect.objectContaining({ cwd: repo })); + }); +}); diff --git a/actions/setup/js/transport_integrity_handlers.test.cjs b/actions/setup/js/transport_integrity_handlers.test.cjs index d10e36a8063..fd05b63bed7 100644 --- a/actions/setup/js/transport_integrity_handlers.test.cjs +++ b/actions/setup/js/transport_integrity_handlers.test.cjs @@ -146,7 +146,8 @@ describe.each(["create_pull_request", "push_to_pull_request_branch"])("%s transp const result = await run(); expect(applied).toBe(patchContent); expect(result.error).toContain("SECURITY:"); - expect(global.exec.exec.mock.calls.filter(([, args]) => args[0] === "am")).toHaveLength(1); + expect(global.exec.exec.mock.calls.filter(([, args]) => args[0] === "am" && args[1] === "--3way")).toHaveLength(1); + expect(global.exec.exec).toHaveBeenCalledWith("git", ["am", "--abort"], expect.objectContaining({ cwd: expect.any(String) })); assertNoRemoteSideEffects(); }); From 3c5a61a6528856b4fdcf318109263f5c05b0d143 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Wed, 14 Oct 2026 11:51:22 -0700 Subject: [PATCH 4/7] Specify safe-output transport integrity and lifecycle invariants Document PTI-001 through PTI-007, map runtime conformance evidence, and clarify the compiler threat boundary without adding a CTR rule. Add a bounded transport model with patch/bundle profiles, negative controls, reachability witnesses, and a pinned-runtime checker that cleans temporary artifacts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../docs/specs/safe-outputs-specification.md | 40 ++++- specs/compiler-threat-detection-changelog.md | 16 ++ specs/compiler-threat-detection-spec.md | 5 +- specs/workflow-security/README.md | 64 +++++++ .../workflow-security/SafeOutputTransport.cfg | 16 ++ .../workflow-security/SafeOutputTransport.tla | 168 ++++++++++++++++++ specs/workflow-security/check_test.py | 42 +++++ specs/workflow-security/check_transport.py | 71 ++++++++ 8 files changed, 419 insertions(+), 3 deletions(-) create mode 100644 specs/workflow-security/SafeOutputTransport.cfg create mode 100644 specs/workflow-security/SafeOutputTransport.tla create mode 100644 specs/workflow-security/check_transport.py diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index 184fb4e6a74..fbc10583dc2 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.9
+**Version**: 1.29.10
**Status**: Working Draft
-**Publication Date**: 2026-10-12
+**Publication Date**: 2026-10-14
**Editor**: GitHub Agentic Workflows Team
**This Version**: [safe-outputs-specification](/gh-aw/specs/safe-outputs-specification/)
**Latest Published Version**: This document @@ -2569,6 +2569,7 @@ safe-outputs: 11. **Summary and Manifest Provenance**: Successful executions MUST record `head_repo` in the safe-output summary and machine-readable manifest. 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). **Configuration Parameters**: @@ -2603,6 +2604,35 @@ These requirements apply to `create_pull_request` and `push_to_pull_request_bran **Conformance Evidence**: The real-git regression in `actions/setup/js/push_signed_commits.test.cjs` advances the base, seeds the remote delivery branch at that base, and simulates GraphQL signing failure with fallback enabled. It verifies attached `HEAD`, the updated named branch, advanced-base ancestry, and delivery of both agent and upstream file contents. +##### Patch and Bundle Transport Integrity Requirements + +These requirements apply to `create_pull_request` and `push_to_pull_request_branch`, including unsigned delivery, bundle recovery, signed delivery, PR/issue publication retries, and optional CI-trigger commits. They bind handler-time policy validation and Git consumption to captured bytes; capture alone does not establish policy approval or detection provenance. + +**PTI-001 — Trusted capture**: The privileged handler MUST derive canonical transport paths from the validated branch and repository rather than agent-supplied paths. For each resolved patch or bundle, it MUST capture raw bytes, a cryptographic digest, and descriptor-verified device/inode identity. Opens MUST reject symlinks and nonregular files without blocking on FIFOs. Missing or unreadable resolved artifacts MUST fail closed, including in allow-empty mode. + +**PTI-002 — Stable consumption**: Git operations that consume a captured path MUST receive a private, read-only snapshot outside the checkout, not the agent-writable original. A bundle's verification, head discovery, imports, and recovery retries MUST use the same snapshot unless a trusted transformation explicitly replaces it. Capture and snapshots MUST preserve raw bytes, including non-UTF-8 data. + +**PTI-003 — Integrity boundaries and cost**: Full content-digest and identity checks of originals and existing snapshots MUST run at transport reads/application, immediately before publication attempts, and at handler completion. Publication includes remote ref creation/deletion, signed mutations, direct pushes, PR creation, and issue fallbacks; each retried attempt MUST recheck. Unrelated local commands MAY use lightweight identity/type/size checks, but these MUST NOT replace byte checks at trust boundaries. Implementations MUST cache snapshot digests and MUST NOT reread or hash full artifacts around every unrelated command. + +**PTI-004 — Trusted transformations**: Temporary-ID substitution and commit-title suffixing MUST operate on captured bytes and affect only trusted in-memory content and its private snapshot. They MUST NOT rewrite canonical artifacts or refresh the original digest/identity baseline from disk. The transformed bytes consumed by Git MUST be the trusted transformation of the validated input. + +**PTI-005 — Permanent failure**: An integrity failure MUST remain sticky for the invocation and prevent further application or publication. It MUST NOT enter conflict recovery, publication fallbacks, or transient/rate-limit retries, regardless of branch names, paths, or message substrings. A previously completed remote write is not rolled back by this contract; implementations MUST NOT claim transactional rollback across GitHub operations. + +**PTI-006 — Restricted cleanup**: Integrity failure MUST NOT prevent registered local cleanup of temporary bundle refs or an in-progress patch application. Cleanup MUST be limited to resources registered by the current guard and pinned to the original checkout; it MUST NOT permit arbitrary ref deletion, remote writes, or renewed artifact consumption. Private snapshots MUST be disposed on success and failure, even when Git cleanup fails. Cleanup failures MUST be surfaced rather than treated as success. + +**PTI-007 — Diagnostics**: Capture and verification MUST preserve filesystem error causes and distinguish operation, nonregular input, identity replacement, and content mismatch. Handler errors MUST expose the stable `ERR_TRANSPORT_INTEGRITY` classification, including when an intermediate helper wraps the error. Logs MUST use safe operation/reason/system-code diagnostics and MUST NOT expose artifact contents, secret values, or raw filesystem cause text. + +**Conformance Evidence**: + +| Requirement | Implementation | Regression evidence | +|---|---|---| +| PTI-001–004 | `resolve_transport_paths.cjs`, `transport_integrity.cjs`, both PR handlers | `resolve_transport_paths.test.cjs`, `transport_integrity.test.cjs`, `transport_integrity_handlers.test.cjs` | +| PTI-003 cost bound | Lightweight local checks and cached snapshot digests | An 8 MiB bundle and 100 unrelated commands require zero full-file reads and hashes; subsequent publication still rejects same-size mutation | +| PTI-005 | `error_recovery.cjs`, `create_or_update_pull_request.cjs`, guarded publication callbacks | `error_recovery.test.cjs`, `create_or_update_pull_request.test.cjs`; real typed errors with network/timeout/rate-limit paths | +| PTI-006–007 | Registered cleanup and structured diagnostics in `transport_integrity.cjs` | `transport_integrity_git.test.cjs` verifies real Git refs and conflict rollback; helper tests cover checkout pinning, failed cleanup, cause retention, and safe logging | + +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. + **Security Requirements**: - Branch name sanitization (prevent injection) @@ -3672,6 +3702,7 @@ For all Linear types, GraphQL source, endpoint, protocol, and host are implement - Successful executions MUST record `head_repo` in the safe-output summary and machine-readable manifest - 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). --- @@ -6120,6 +6151,11 @@ 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.10** (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. +- **Added**: Runtime regression mappings and a bounded transport lifecycle TLA+ model; no new compiler rule or token permission is introduced. + **Version 1.29.9** (2026-10-12): - **Specified**: Successful stale-base rebases preserve attached `HEAD` and advance the named delivery branch; permitted git-push fallback delivers that rebased tip without broadening signing or fallback policy. diff --git a/specs/compiler-threat-detection-changelog.md b/specs/compiler-threat-detection-changelog.md index 1764fac79b5..1caf52d6304 100644 --- a/specs/compiler-threat-detection-changelog.md +++ b/specs/compiler-threat-detection-changelog.md @@ -16,6 +16,7 @@ This changelog records the version history and the dated mapping audits for `spe | Version | Change | |---|---| +| 1.0.52 | Audit-only clarification of the safe-output patch/bundle runtime integrity boundary, PTI-001–007 and bounded TLA+ evidence; no new CTR rule or generated-workflow compatibility change. | | 1.0.51 | Extends CTR-008 to reject `pull_request_target` with `on.roles` containing `read` or `all` in strict mode; in non-strict mode, the dangerous feature flag suppresses only the roles warning. | | 1.0.50 | Extended CTR-032 with creating-run traceability for every issue comment, PR description, and PR review comment from first publication. Specified HTML execution markers except percent-encoded Markdown anchor markers for review comments and replies, independent of visible footers, with forged-marker and destination API filtering coverage required. | | 1.0.49 | Added CTR-032 for step-scoped safe-output publishing identity, replacement of forged execution-provenance comments, and fail-closed runtime metadata validation. | @@ -47,6 +48,21 @@ This changelog records the version history and the dated mapping audits for `spe ## Mapping Audits +### Mapping Audit (2026-10-14) + +Reviewed `transport_integrity.cjs`, both PR handlers, publication retry callbacks, +and the transport regression tests. Captured validation bytes are consumed +through private snapshots; nonregular files fail promptly, integrity errors +remain permanent, and only registered local Git cleanup may proceed after failure. +Full byte checks remain mandatory at trust boundaries, without full artifact +scans around unrelated local commands. Filesystem causes and safe diagnostics +remain available without publishing artifact contents. + +The controls are runtime obligations in Safe Outputs PTI-001–007, with bounded +state-machine evidence in `workflow-security/SafeOutputTransport.tla`. They +complement artifact provenance, detection, authorization, and CTR-005 routing +rather than adding a compiler-detectable construct or a new `CTR-*` rule. + ### Mapping Audit (2026-10-12) Reviewed the allow-listed GitHub App checkout exception for `pull_request_review` diff --git a/specs/compiler-threat-detection-spec.md b/specs/compiler-threat-detection-spec.md index ca43ff8aa06..7e2d5214f83 100644 --- a/specs/compiler-threat-detection-spec.md +++ b/specs/compiler-threat-detection-spec.md @@ -7,7 +7,7 @@ sidebar: # GitHub Actions Compiler Threat Detection Specification -**Version**: 1.0.51 +**Version**: 1.0.52 **Status**: Candidate Recommendation **Latest Version**: https://github.com/github/gh-aw/blob/main/specs/compiler-threat-detection-spec.md **Editors**: GitHub Next (GitHub, Inc.) @@ -32,6 +32,7 @@ Each version maps to the minimum compatible binary. A version change MUST update | Versions | Minimum gh-aw | Compatibility | |---|---:|---| +| `1.0.52` | Unreleased | Audit-only; patch/bundle validation-to-application integrity is a runtime boundary governed by Safe Outputs PTI-001–007 and its TLA+ model. No new CTR rule, permissions, or lock-file schema change. | | `1.0.51` | Unreleased | Extends CTR-008 to reject `pull_request_target` with `on.roles` containing `read` or `all` in strict mode and permit suppression of only the corresponding warning in non-strict mode via an explicit dangerous feature flag. | | `1.0.50` | Unreleased | Extends CTR-032 with mandatory creating-run traceability from first publication, HTML markers for issue comments and PR descriptions, and Markdown anchor markers for PR review comments. Versioned activation markers and review-comment persistence coverage remain implementation obligations. | | `1.0.49` | Unreleased | Adds CTR-032; safe-output publishing jobs supply step-level check-run identity, and runtime publishers replace forged execution-provenance markers with trusted metadata. | @@ -69,6 +70,8 @@ Runtime trust boundaries inside the agent workspace are out of scope for this sp PR-branch checkout authorization in `actions/setup/js/checkout_pr_branch.cjs` is also a runtime trust control, not a compiler-detectable threat. The same-repository bot exception covers `pull_request` and `pull_request_target` `opened` or `synchronize` events with matching bot sender and actor plus equal runtime, PR head, and PR base repository IDs. For `issue_comment`, `pull_request_review_comment`, and `pull_request_review`, an explicitly `on.bots`-allow-listed App may bypass the collaborator check only when `sender.type` is `Bot`, the sender and matching comment or review author logins match exactly, the sender is canonically equivalent to the runtime actor (App slug or `[bot]` form), the runtime repository ID is positive, and the fetched PR head and base repository IDs both match it. Forks, unlisted bots, unverifiable or mismatched identities, and other actors retain the collaborator-permission check. The normative requirements are in the Security Architecture Specification, RS-05a; this control does not add a `CTR-*` rule. +Patch/bundle replacement between safe-output validation and Git application is likewise a runtime boundary, not a new compiler threat rule. Safe Outputs [PTI-001–007](../docs/src/content/docs/specs/safe-outputs-specification.md#patch-and-bundle-transport-integrity-requirements) require canonical capture, nonblocking regular-file checks, private snapshot consumption, full byte checks at application/publication boundaries, permanent integrity-error classification, registered local cleanup, and safe diagnostics. The implementation is `actions/setup/js/transport_integrity.cjs`, used by both PR handlers and their publication callbacks; evidence is mapped in the safe-output specification and [`SafeOutputTransport.tla`](workflow-security/SafeOutputTransport.tla). Artifact run/invocation provenance, schema validation, detector gating, and CTR-005 safe-output routing remain independent requirements: none proves that a mutable artifact still contains the bytes validated earlier. This amendment changes no generated workflow construct, GitHub App permission, suppression rule, or `CTR-*` identifier. + Sandbox bypass includes the provenance of the agent's own configuration. On pull-request triggers the workspace holds head-branch content that the PR author controls, so agent configuration folders and root instruction files are attacker-controlled inputs until the generated job replaces them with the base-branch snapshot captured before that content was checked out. Activation also reads workflow imports and skills before the agent job runs. Restoring agent configuration later cannot make instructions already loaded during activation trustworthy; the activation sparse checkout must select a trusted ref independently. diff --git a/specs/workflow-security/README.md b/specs/workflow-security/README.md index b32e3c1bd4d..113b9c98bf7 100644 --- a/specs/workflow-security/README.md +++ b/specs/workflow-security/README.md @@ -42,6 +42,7 @@ sandboxed agent principal, even when GitHub Actions runs both in the same job. | Git | `checkouts`, `cleanup`, `agentBegan`, `privilegedCheckout`, `operations`, `errors` | Per-repository auth, temporary setup credentials, verified cleanup/failure, available blobs/refs, sparse/shallow state, local operations, REST, and privileged push. | | `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. | | Comment/review-triggered PR checkout | [`CommentTriggeredPRCheckout.tla`](CommentTriggeredPRCheckout.tla) | Allow-listed Bot sender and exact comment/review-author identity, canonical App slug/bot actor equivalence, and same-repository head/base boundaries. | +| Patch/bundle application | [`SafeOutputTransport.tla`](SafeOutputTransport.tla) | Captured bytes, trusted transformations, private snapshots, publication retries, sticky failures, bounded local checking cost, and finally cleanup. | The `issue` effect creates at most one validated issue in the main public repository. The `pull-request` effect prepares a full checkout in the privileged @@ -101,6 +102,69 @@ confirm TLC reports `NoUntrustedCheckoutAcceptance`. This model specifies the compiler decision boundary; it does not model Git's parser or prove safety of the contents at an otherwise trusted mutable ref. +## Safe-output transport lifecycle + +[`SafeOutputTransport.tla`](SafeOutputTransport.tla) supplements the job-level +model with the runtime contract in Safe Outputs +[PTI-001–007](../../docs/src/content/docs/specs/safe-outputs-specification.md#patch-and-bundle-transport-integrity-requirements). +It separates original bytes/identity, captured baselines, trusted transformed +bytes, private snapshots, applied bytes, and publication-time verification. +One adversarial mutation may change bytes, identity, regular-file status, or +an existing snapshot between boundaries; FIFO replacement also occurs before +capture. Rate-limited +publication can retry once; each attempt rechecks integrity. + +The patch profile registers an abort after conflicting application; the bundle +profile registers a temporary ref. Both complete through finally cleanup. +Cleanup may itself fail, but snapshots must still be disposed and the failure +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 +the pinned TLA+ runtime described below. The runner rejects incorrect tooling +status or invariant names and automatically removes temporary configurations, +TLC states, and traces: + +```bash +TLA2TOOLS_JAR=/path/to/tla2tools.jar JAVA_BIN=/path/to/java \ + python3 specs/workflow-security/check_transport.py +``` + +`SafeOutputTransport.cfg` defaults to `Transport = "patch"`. The runner also +checks `Transport = "bundle"`. For each synthetic negative control, it changes +only `Fault` and requires TLC exit code 12 with the corresponding invariant: + +| Fault | Expected invariant | +|---|---| +| `blocking-open` | `NonblockingOpen` | +| `reopen-original` | `StableConsumption` | +| `rebaseline` | `OriginalBaseline` | +| `skip-publication-check` | `PublicationIntegrity` | +| `retry-integrity` | `PermanentFailure` | +| `cleanup-blocked` | `CleanupDisposition` | +| `cleanup-error-hidden` | `CleanupDisposition` | +| `unsafe-diagnostics` | `SafeDiagnostics` | +| `redundant-hashing` | `BoundedLocalCost` | + +With `Fault = "none"`, append each of `NoSuccessfulWrite`, +`NoRejectedMutation`, `NoRateLimitRetry`, `NoCleanupFailure`, and +`NoConflictCleanup` as an invariant to obtain positive reachability witnesses +for publication, rejection, retry, surfaced cleanup failure, and conflict +cleanup, respectively. These deliberately false predicates and synthetic +faults are not product vulnerabilities. + +The bounded model assumes snapshot privacy, collision-free byte classes, +trusted transformations and exact registered-cleanup dispatch. It does not +model arbitrary same-user filesystem races, Git parsing, App API semantics, +unbounded retries, credential selection, or the full structured logging +payload. Those remain implementation obligations covered by +`transport_integrity.test.cjs`, `transport_integrity_handlers.test.cjs`, +`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 `CompiledWorkflow`; +the transport model is checked separately, like the checkout submodels. + ## Comment- and review-triggered pull request checkout [`CommentTriggeredPRCheckout.tla`](CommentTriggeredPRCheckout.tla) exhaustively diff --git a/specs/workflow-security/SafeOutputTransport.cfg b/specs/workflow-security/SafeOutputTransport.cfg new file mode 100644 index 00000000000..c72319e24d6 --- /dev/null +++ b/specs/workflow-security/SafeOutputTransport.cfg @@ -0,0 +1,16 @@ +SPECIFICATION Spec +CONSTANTS + Fault = "none" + Transport = "patch" +INVARIANTS + TypeOK + StableConsumption + OriginalBaseline + PublicationIntegrity + PermanentFailure + NonblockingOpen + SafeDiagnostics + BoundedLocalCost + CleanupConfinement + CleanupDisposition +CHECK_DEADLOCK FALSE diff --git a/specs/workflow-security/SafeOutputTransport.tla b/specs/workflow-security/SafeOutputTransport.tla new file mode 100644 index 00000000000..7712e827fb2 --- /dev/null +++ b/specs/workflow-security/SafeOutputTransport.tla @@ -0,0 +1,168 @@ +------------------------- MODULE SafeOutputTransport ------------------------- +EXTENDS Naturals, Sequences + +CONSTANTS Fault, Transport +ASSUME Transport \in {"patch", "bundle"} +ASSUME Fault \in {"none", "blocking-open", "reopen-original", "rebaseline", + "skip-publication-check", "retry-integrity", + "cleanup-blocked", "cleanup-error-hidden", "unsafe-diagnostics", + "redundant-hashing"} + +\* Byte classes abstract collision-free digests, not Git's or the OS's parser. +VARIABLE s +vars == <> +Stages == {"capture", "validate", "apply", "conflict", "publish", "complete", + "failed", "done"} +Resources == {"am-state", "bundle-ref"} + +Init == + s = [stage |-> "capture", original |-> 0, identity |-> 0, + regular |-> TRUE, captured |-> 0, capturedIdentity |-> 0, + trusted |-> 0, snapshot |-> 0, snapshotExists |-> FALSE, + transformed |-> FALSE, mutated |-> FALSE, localChecked |-> FALSE, + failed |-> FALSE, diagnosticsSafe |-> TRUE, blocked |-> FALSE, + permanentRetries |-> 0, localHashes |-> 0, attempts |-> 0, + resources |-> {}, cleanupError |-> FALSE, cleanupReported |-> FALSE, + applied |-> {}, writes |-> {}, lastEvent |-> "Init"] + +Integrity == + /\ s.regular /\ s.original = s.captured + /\ s.identity = s.capturedIdentity + /\ (~s.snapshotExists \/ s.snapshot = s.trusted) + +Reject == + s' = [s EXCEPT !.stage = "failed", !.failed = TRUE, + !.diagnosticsSafe = Fault # "unsafe-diagnostics", + !.blocked = ~s.regular /\ Fault = "blocking-open", + !.permanentRetries = IF Fault = "retry-integrity" THEN 1 ELSE 0, + !.lastEvent = "IntegrityFailure"] + +Capture == + /\ s.stage = "capture" + /\ IF ~s.regular THEN Reject + ELSE s' = [s EXCEPT !.stage = "validate", !.captured = s.original, + !.capturedIdentity = s.identity, !.trusted = s.original, + !.lastEvent = "Capture"] + +Mutate(kind) == + /\ s.stage \in {"capture", "validate", "apply", "conflict", "publish", "complete"} + /\ ~s.mutated + /\ kind \in {"bytes", "identity", "fifo", "snapshot"} + /\ (s.stage # "capture" \/ kind = "fifo") + /\ (kind # "snapshot" \/ s.snapshotExists) + /\ s' = [s EXCEPT + !.original = IF kind = "bytes" THEN 1 ELSE @, + !.identity = IF kind = "identity" THEN 1 ELSE @, + !.regular = IF kind = "fifo" THEN FALSE ELSE @, + !.snapshot = IF kind = "snapshot" THEN 1 ELSE @, + !.captured = IF kind = "bytes" /\ Fault = "rebaseline" THEN 1 ELSE @, + !.mutated = TRUE, !.lastEvent = "Mutate:" \o kind] + +Transform == + /\ s.stage = "validate" /\ ~s.transformed + /\ IF ~Integrity THEN Reject + ELSE s' = [s EXCEPT !.trusted = 2, !.transformed = TRUE, + !.lastEvent = "TrustedTransform"] + +Validate == + /\ s.stage = "validate" + /\ IF ~Integrity THEN Reject + ELSE s' = [s EXCEPT !.stage = "apply", !.snapshot = s.trusted, + !.snapshotExists = TRUE, !.lastEvent = "ValidateSnapshot"] + +LocalCommand == + /\ s.stage \in {"apply", "publish"} /\ ~s.localChecked + /\ s' = [s EXCEPT !.localChecked = TRUE, + !.localHashes = IF Fault = "redundant-hashing" THEN 1 ELSE 0, + !.lastEvent = "UnrelatedLocalCommand"] + +Apply(success) == + /\ s.stage = "apply" + /\ IF ~Integrity THEN Reject + ELSE s' = [s EXCEPT + !.stage = IF success THEN "publish" ELSE "conflict", + !.applied = @ \cup { + [bytes |-> IF Fault = "reopen-original" THEN s.original ELSE s.snapshot, + validated |-> s.trusted]}, + !.resources = IF Transport = "bundle" THEN {"bundle-ref"} + ELSE IF success THEN {} ELSE {"am-state"}, + !.lastEvent = IF success THEN "Apply" ELSE "ApplyConflict"] + +CheckConflict == + /\ s.stage = "conflict" + /\ IF ~Integrity THEN Reject + ELSE s' = [s EXCEPT !.stage = "failed", !.lastEvent = "CheckConflict"] + +\* A rate-limited attempt has no effect; a subsequent attempt checks again. +Publish(success) == + /\ s.stage = "publish" /\ s.attempts < 2 + /\ IF ~Integrity /\ Fault # "skip-publication-check" THEN Reject + ELSE s' = [s EXCEPT + !.stage = IF success \/ s.attempts = 1 THEN "complete" ELSE @, + !.attempts = @ + 1, + !.writes = IF success + THEN @ \cup {[verified |-> Integrity, failed |-> s.failed]} + ELSE @, + !.lastEvent = IF success THEN "Publish" ELSE "RateLimited"] + +Complete == + /\ s.stage = "complete" + /\ IF ~Integrity THEN Reject + ELSE s' = [s EXCEPT !.stage = "failed", !.lastEvent = "HandlerComplete"] + +\* "failed" is the common finally path, also used after successful completion. +Cleanup(success) == + /\ s.stage = "failed" + /\ LET denied == s.failed /\ Fault = "cleanup-blocked" + error == s.resources # {} /\ (~success \/ denied) + IN s' = [s EXCEPT + !.stage = "done", !.snapshotExists = FALSE, + !.resources = IF error THEN @ ELSE {}, + !.cleanupError = error, + !.cleanupReported = error /\ ~denied /\ Fault # "cleanup-error-hidden", + !.lastEvent = IF error THEN "CleanupFailure" ELSE "Cleanup"] + +Next == + \/ Capture \/ Transform \/ Validate \/ LocalCommand \/ CheckConflict \/ Complete + \/ \E kind \in {"bytes", "identity", "fifo", "snapshot"}: Mutate(kind) + \/ \E success \in BOOLEAN: Apply(success) \/ Publish(success) \/ Cleanup(success) + +Spec == Init /\ [][Next]_vars + +TypeOK == + /\ s.stage \in Stages + /\ s.original \in 0..1 /\ s.captured \in 0..1 + /\ s.identity \in 0..1 /\ s.capturedIdentity \in 0..1 + /\ s.trusted \in 0..2 /\ s.snapshot \in 0..2 + /\ s.regular \in BOOLEAN /\ s.snapshotExists \in BOOLEAN + /\ s.transformed \in BOOLEAN /\ s.mutated \in BOOLEAN + /\ s.localChecked \in BOOLEAN /\ s.failed \in BOOLEAN + /\ s.diagnosticsSafe \in BOOLEAN /\ s.blocked \in BOOLEAN + /\ s.permanentRetries \in 0..1 /\ s.localHashes \in 0..1 + /\ s.attempts \in 0..2 /\ s.resources \subseteq Resources + /\ s.cleanupError \in BOOLEAN /\ s.cleanupReported \in BOOLEAN + /\ s.applied \subseteq [bytes: 0..2, validated: 0..2] + /\ s.writes \subseteq [verified: BOOLEAN, failed: BOOLEAN] + +StableConsumption == \A a \in s.applied: a.bytes = a.validated +OriginalBaseline == s.stage # "capture" => s.captured = 0 +PublicationIntegrity == \A w \in s.writes: w.verified /\ ~w.failed +PermanentFailure == s.permanentRetries = 0 +NonblockingOpen == ~s.blocked +SafeDiagnostics == s.diagnosticsSafe +BoundedLocalCost == s.localHashes = 0 +CleanupConfinement == + s.resources \subseteq IF Transport = "patch" THEN {"am-state"} ELSE {"bundle-ref"} +CleanupDisposition == + s.stage = "done" => + /\ ~s.snapshotExists + /\ (s.resources = {} \/ s.cleanupReported) + /\ (s.cleanupError => s.cleanupReported) + +\* Deliberately false reachability predicates, not safety requirements. +NoSuccessfulWrite == s.writes = {} +NoRejectedMutation == ~s.failed +NoRateLimitRetry == s.attempts < 2 +NoCleanupFailure == ~s.cleanupError +NoConflictCleanup == ~(s.stage = "done" /\ s.applied # {} /\ s.attempts = 0) +============================================================================= diff --git a/specs/workflow-security/check_test.py b/specs/workflow-security/check_test.py index 91b02795de7..7f189ee7ab2 100644 --- a/specs/workflow-security/check_test.py +++ b/specs/workflow-security/check_test.py @@ -9,6 +9,48 @@ from unittest.mock import patch import check +import check_transport + + +class TransportRunnerTest(unittest.TestCase): + def test_untrusted_runtime_is_rejected_before_execution(self): + with tempfile.TemporaryDirectory() as directory: + jar = Path(directory) / "tla2tools.jar" + jar.write_bytes(b"untrusted runtime") + with patch.dict(os.environ, {"TLA2TOOLS_JAR": str(jar)}), \ + patch.object(check_transport.subprocess, "run") as run: + with self.assertRaisesRegex(RuntimeError, "pinned SHA-256"): + check_transport.main() + run.assert_not_called() + + def test_all_profiles_faults_and_witnesses_run_and_cleanup(self): + with tempfile.TemporaryDirectory() as directory: + jar = Path(directory) / "tla2tools.jar" + jar.write_bytes(b"local runtime fixture") + digest = hashlib.sha256(jar.read_bytes()).hexdigest() + configurations = [] + destinations = [] + + def run(command, cwd, **options): + configurations.append((cwd / "model.cfg").read_text()) + destinations.append(cwd) + return check_transport.subprocess.CompletedProcess(command, 0, "", "") + + with patch.dict(os.environ, {"TLA2TOOLS_JAR": str(jar)}), \ + patch.object(check_transport, "JAR_SHA256", digest), \ + patch.object(check_transport.subprocess, "run", side_effect=run), \ + patch.object(check_transport, "check_result", + return_value={"distinct": 1}) as result, \ + 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) + for transport in ["patch", "bundle"]: + self.assertEqual( + sum(f'Transport = "{transport}"' in config for config in configurations), + len(expected), + ) + self.assertTrue(all(not destination.exists() for destination in destinations)) class RuntimePinTest(unittest.TestCase): diff --git a/specs/workflow-security/check_transport.py b/specs/workflow-security/check_transport.py new file mode 100644 index 00000000000..ad83b405aff --- /dev/null +++ b/specs/workflow-security/check_transport.py @@ -0,0 +1,71 @@ +#!/usr/bin/env python3 +"""Check transport lifecycle profiles, negative controls, and reachability.""" + +import hashlib +import os +from pathlib import Path +import subprocess +import tempfile + +from check import JAR_SHA256, ROOT, check_result + + +FAULTS = { + "blocking-open": "NonblockingOpen", + "reopen-original": "StableConsumption", + "rebaseline": "OriginalBaseline", + "skip-publication-check": "PublicationIntegrity", + "retry-integrity": "PermanentFailure", + "cleanup-blocked": "CleanupDisposition", + "cleanup-error-hidden": "CleanupDisposition", + "unsafe-diagnostics": "SafeDiagnostics", + "redundant-hashing": "BoundedLocalCost", +} +WITNESSES = [ + "NoSuccessfulWrite", "NoRejectedMutation", "NoRateLimitRetry", + "NoCleanupFailure", "NoConflictCleanup", +] + + +def main(): + 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: + 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) + + +if __name__ == "__main__": + main() From fa22065af0022d971cdb3b63569a5352d5914f18 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:11:32 +0000 Subject: [PATCH 5/7] Close safe-output integrity review gaps Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../js/create_or_update_pull_request.cjs | 6 +- .../js/create_or_update_pull_request.test.cjs | 26 +++++++ actions/setup/js/create_pull_request.cjs | 10 ++- actions/setup/js/create_pull_request.test.cjs | 20 ++++- .../js/create_pull_request_helpers.test.cjs | 15 ++++ actions/setup/js/error_recovery.cjs | 2 +- actions/setup/js/transport_integrity.cjs | 74 ++++++++++++++++--- .../setup/js/transport_integrity_git.test.cjs | 59 +++++++++++++++ .../js/transport_integrity_handlers.test.cjs | 10 +++ 9 files changed, 204 insertions(+), 18 deletions(-) diff --git a/actions/setup/js/create_or_update_pull_request.cjs b/actions/setup/js/create_or_update_pull_request.cjs index bd5d2316746..3d1ba689013 100644 --- a/actions/setup/js/create_or_update_pull_request.cjs +++ b/actions/setup/js/create_or_update_pull_request.cjs @@ -5,9 +5,9 @@ const { withRetry, RATE_LIMIT_RETRY_CONFIG } = require("./error_recovery.cjs"); async function createOrUpdatePullRequest(options) { const { githubClient, repoParts, title, body, branchName, headRepo, baseBranch, draft, assertTransportUnchanged } = options; return withRetry( - () => { + async () => { assertTransportUnchanged?.(); - return githubClient.rest.pulls.create({ + const response = await githubClient.rest.pulls.create({ owner: repoParts.owner, repo: repoParts.repo, title, @@ -17,6 +17,8 @@ async function createOrUpdatePullRequest(options) { base: baseBranch, draft, }); + assertTransportUnchanged?.(); + return response; }, RATE_LIMIT_RETRY_CONFIG, `create pull request in ${repoParts.owner}/${repoParts.repo}` diff --git a/actions/setup/js/create_or_update_pull_request.test.cjs b/actions/setup/js/create_or_update_pull_request.test.cjs index 128fd8c2d9d..f02a8c81286 100644 --- a/actions/setup/js/create_or_update_pull_request.test.cjs +++ b/actions/setup/js/create_or_update_pull_request.test.cjs @@ -78,4 +78,30 @@ describe("createOrUpdatePullRequest", () => { vi.unstubAllGlobals(); } }); + + it("rejects a transport mutation after successful PR creation before returning", async () => { + vi.stubGlobal("core", { debug: vi.fn(), info: vi.fn(), warning: vi.fn() }); + let tampered = false; + const githubClient = { + rest: { + pulls: { + create: vi.fn(async () => { + tampered = true; + return { data: { number: 42 } }; + }), + }, + }, + }; + const assertTransportUnchanged = vi.fn(() => { + if (tampered) throw new TransportIntegrityError("/tmp/aw-feature.patch"); + }); + + try { + await expect(createOrUpdatePullRequest({ ...options, githubClient, assertTransportUnchanged })).rejects.toThrow("SECURITY:"); + expect(githubClient.rest.pulls.create).toHaveBeenCalledTimes(1); + expect(assertTransportUnchanged).toHaveBeenCalledTimes(2); + } finally { + vi.unstubAllGlobals(); + } + }); }); diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 60f5b3ba9d8..8f1bdbebc54 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -375,10 +375,10 @@ async function applyBundleToBranch(bundleFilePath, branchName, originalAgentBran * revert or absorb commits that were added to the base branch after the agent's checkout. * * @param {string} baseBranch - * @param {{ exec: Function, getExecOutput: Function }} execApi + * @param {{ exec: Function, getExecOutput: Function, registerRewriteCleanup?: (originalHead: string, gitOpts?: Object) => () => void }} execApi * @param {string} [bundleFilePath] - Optional path to the bundle file; used to extract the * precise base commit the agent worked from. - * @param {{ excludedFiles?: string[] }} [options] + * @param {{ excludedFiles?: string[], gitOpts?: Object }} [options] * @returns {Promise} */ async function rewriteBundleBranchAsSingleCommit(baseBranch, execApi, bundleFilePath, options = {}) { @@ -427,10 +427,15 @@ async function rewriteBundleBranchAsSingleCommit(baseBranch, execApi, bundleFile } core.warning(`Rewriting bundled commits to a single linear commit for signed push compatibility (base: ${baseRef})`); + const gitArgs = options.gitOpts ? [options.gitOpts] : []; + const { stdout: originalHeadOut } = await execApi.getExecOutput("git", ["rev-parse", "HEAD"], ...gitArgs); + const releaseRewriteCleanup = execApi.registerRewriteCleanup?.(originalHeadOut.trim(), options.gitOpts || { cwd: process.cwd() }); const newHead = await linearizeRangeAsCommit(baseRef, commitHeadline, execApi, { + ...(options.gitOpts ? { gitOpts: options.gitOpts } : {}), excludedFiles: options.excludedFiles, rebaseOnto: fallbackBaseRef, }); + releaseRewriteCleanup?.(); core.info(`Bundle rewrite completed (new HEAD: ${newHead})`); } @@ -489,6 +494,7 @@ async function createFallbackIssue(githubClient, repoParts, title, body, labels, try { assertUnchanged(); const response = await githubClient.rest.issues.create(payload); + assertUnchanged(); return { ...response, issueRepoParts: { owner: payload.owner, repo: payload.repo } }; } catch (error) { assertUnchanged(); diff --git a/actions/setup/js/create_pull_request.test.cjs b/actions/setup/js/create_pull_request.test.cjs index 0439d420c63..fdbacc05837 100644 --- a/actions/setup/js/create_pull_request.test.cjs +++ b/actions/setup/js/create_pull_request.test.cjs @@ -783,7 +783,8 @@ index 0000000..abc1234 } if (cmd === "git" && args[0] === "rev-parse" && args[1] === "HEAD") { revParseHeadCallCount += 1; - return Promise.resolve({ exitCode: 0, stdout: revParseHeadCallCount === 1 ? "old-head-sha\n" : "new-head-sha\n", stderr: "" }); + const head = revParseHeadCallCount === 1 ? "a".repeat(40) : "d".repeat(40); + return Promise.resolve({ exitCode: 0, stdout: `${head}\n`, stderr: "" }); } if (cmd === "git" && args[0] === "log" && args[1] === "-1" && args[2] === "--format=%s" && args[3] === "HEAD") { return Promise.resolve({ exitCode: 0, stdout: "bundle merge headline\n", stderr: "" }); @@ -4890,6 +4891,23 @@ describe("create_pull_request - E003 file-limit fallback-to-issue", () => { expect(global.github.rest.pulls.create).not.toHaveBeenCalled(); }); + it("rejects a transport mutation after successful fallback issue creation", async () => { + const patchPath = canonicalPatchPath("data/refresh"); + fs.writeFileSync(patchPath, buildOversizedPatch(101)); + global.github.rest.issues.create.mockImplementationOnce(async () => { + fs.writeFileSync(patchPath, "tampered after fallback issue creation"); + return { data: { number: 55, html_url: "https://github.com/test/issues/55" } }; + }); + + const { main } = require("./create_pull_request.cjs"); + const handler = await main({}); + const result = await handler({ title: "Data refresh PR", body: "Daily update", branch: "data/refresh" }, {}); + + expect(global.github.rest.issues.create).toHaveBeenCalledTimes(1); + expect(result.success).toBe(false); + expect(result.error).toContain("SECURITY:"); + }); + it("should use the actual received file count (not maxFiles*2) as the suggested limit", async () => { const patchPath = canonicalPatchPath("api/regen"); fs.writeFileSync(patchPath, buildOversizedPatch(220)); diff --git a/actions/setup/js/create_pull_request_helpers.test.cjs b/actions/setup/js/create_pull_request_helpers.test.cjs index ba3e66e4ae6..7d1150d5f8b 100644 --- a/actions/setup/js/create_pull_request_helpers.test.cjs +++ b/actions/setup/js/create_pull_request_helpers.test.cjs @@ -40,6 +40,7 @@ const { buildManualBranchApplyCommands, withTransientPushRetry, } = require("./create_pull_request_helpers.cjs"); +const { TransportIntegrityError } = require("./transport_integrity.cjs"); function runGit(args, cwd) { const result = spawnSync("git", args, { cwd, encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }); @@ -91,6 +92,20 @@ describe("withTransientPushRetry", () => { await expect(resultPromise).resolves.toBe("ok"); expect(operation).toHaveBeenCalledTimes(2); }); + + it("does not retry integrity errors whose artifact path resembles a workflows-scope timeout", async () => { + vi.useFakeTimers(); + const error = new TransportIntegrityError("/tmp/unable to determine if workflow can be created or updated.patch"); + const operation = vi.fn().mockRejectedValue(error); + + const result = withTransientPushRetry(operation); + const rejection = expect(result).rejects.toBe(error); + await vi.runAllTimersAsync(); + await rejection; + + expect(operation).toHaveBeenCalledTimes(1); + expect(vi.getTimerCount()).toBe(0); + }); }); // --------------------------------------------------------------------------- diff --git a/actions/setup/js/error_recovery.cjs b/actions/setup/js/error_recovery.cjs index 326d1c159ef..21450fc9176 100644 --- a/actions/setup/js/error_recovery.cjs +++ b/actions/setup/js/error_recovery.cjs @@ -283,7 +283,7 @@ async function withRetry(operation, config = {}, operationName = "operation") { const errorMsg = getErrorMessage(error); // Check if this error should be retried - if (!fullConfig.shouldRetry(error)) { + if (error?.code === "ERR_TRANSPORT_INTEGRITY" || !fullConfig.shouldRetry(error)) { core.debug(`${operationName} failed with non-retryable error: ${errorMsg}`); throw enhanceError(error, { operation: operationName, diff --git a/actions/setup/js/transport_integrity.cjs b/actions/setup/js/transport_integrity.cjs index fbfa9f771b3..830485f024f 100644 --- a/actions/setup/js/transport_integrity.cjs +++ b/actions/setup/js/transport_integrity.cjs @@ -28,7 +28,7 @@ function createTransportIntegrity() { /** @typedef {{hash: string, dev: number, ino: number, size: number}} FileIdentity */ /** @type {Map} */ const inputs = new Map(); - /** @type {Map Promise>} */ + /** @type {Map Promise, execute?: (method: string) => Promise}>} */ const cleanups = new Map(); let directory = ""; /** @type {TransportIntegrityError | null} */ @@ -134,15 +134,29 @@ function createTransportIntegrity() { input.bytes = Buffer.from(bytes); } + async function hasGitStateDirectory(execApi, stateDirectory, cleanupOptions) { + if (typeof execApi.getExecOutput !== "function") return true; + const { exitCode, stdout } = await execApi.getExecOutput("git", ["rev-parse", "--git-path", stateDirectory], { + ...cleanupOptions[0], + ignoreReturnCode: true, + silent: true, + }); + if (exitCode !== 0) throw new Error(`Unable to inspect Git state directory: ${stateDirectory}`); + const statePath = stdout.trim(); + return !!statePath && fs.existsSync(path.isAbsolute(statePath) ? statePath : path.resolve(cleanupOptions[0].cwd, statePath)); + } + function guardExec(execApi) { const guarded = { ...execApi }; for (const method of ["exec", "getExecOutput"]) { guarded[method] = async (command, args = [], ...options) => { - const cleanupKey = cleanupArgs => JSON.stringify([options[0]?.cwd || process.cwd(), cleanupArgs]); - const key = cleanupKey(args); + const cleanupKey = (cwd, cleanupArgs) => JSON.stringify([path.resolve(cwd || process.cwd()), cleanupArgs]); + const cwd = path.resolve(options[0]?.cwd || process.cwd()); + const key = cleanupKey(cwd, args); // Only cleanup registered by this guard may bypass a sticky failure. - if (command === "git" && cleanups.has(key)) { - const result = await execApi[method](command, args, ...options); + const registeredCleanup = cleanups.get(key); + if (command === "git" && registeredCleanup) { + const result = registeredCleanup.execute ? await registeredCleanup.execute(method) : await execApi[method](command, args, ...options); if ((method === "exec" ? result : result.exitCode) === 0) cleanups.delete(key); return result; } @@ -152,18 +166,25 @@ function createTransportIntegrity() { // and handler completion. Unrelated local commands only check identity. checkUnchanged(verifyContents); const stableArgs = command === "git" ? args.map(arg => (inputs.has(arg) ? snapshot(arg) : arg)) : args; - const cleanupOptions = [{ ...options[0], cwd: options[0]?.cwd || process.cwd() }, ...options.slice(1)]; + const cleanupOptions = [{ ...options[0], cwd }, ...options.slice(1)]; let amCleanupKey; if (usesTransport && args[0] === "am" && args[1] === "--3way") { - amCleanupKey = cleanupKey(["am", "--abort"]); - cleanups.set(amCleanupKey, () => execApi.exec("git", ["am", "--abort"], ...cleanupOptions)); + const abortArgs = ["am", "--abort"]; + amCleanupKey = cleanupKey(cwd, abortArgs); + const runAbort = async cleanupMethod => { + if (!(await hasGitStateDirectory(execApi, "rebase-apply", cleanupOptions))) { + return cleanupMethod === "exec" ? 0 : { exitCode: 0, stdout: "", stderr: "" }; + } + return execApi[cleanupMethod]("git", abortArgs, ...cleanupOptions); + }; + cleanups.set(amCleanupKey, { run: () => runAbort("exec"), execute: runAbort }); } if (usesTransport && args[0] === "fetch") { for (const refspec of args.slice(2)) { const ref = refspec.split(":")[1]; if (ref?.startsWith("refs/bundles/")) { const cleanup = ["update-ref", "-d", ref]; - cleanups.set(cleanupKey(cleanup), () => execApi.exec("git", cleanup, ...cleanupOptions)); + cleanups.set(cleanupKey(cwd, cleanup), { run: () => execApi.exec("git", cleanup, ...cleanupOptions) }); } } } @@ -171,7 +192,7 @@ function createTransportIntegrity() { const result = await execApi[method](command, stableArgs, ...options); if ((method === "exec" ? result : result.exitCode) === 0) { if (amCleanupKey) cleanups.delete(amCleanupKey); - if (command === "git" && args[0] === "am" && args[1] === "--continue") cleanups.delete(cleanupKey(["am", "--abort"])); + if (command === "git" && args[0] === "am" && args[1] === "--continue") cleanups.delete(cleanupKey(cwd, ["am", "--abort"])); } return result; } finally { @@ -181,14 +202,43 @@ function createTransportIntegrity() { } }; } + guarded.registerRewriteCleanup = (originalHead, gitOpts = {}) => { + if (!/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(originalHead)) { + throw new Error("Cannot register bundle rewrite cleanup without a resolved original HEAD"); + } + const checkout = path.resolve(gitOpts.cwd || process.cwd()); + const cleanupOptions = [{ ...gitOpts, cwd: checkout }]; + /** @param {string[]} args @param {() => Promise} run @param {(method: string) => Promise} [execute] @returns {string} */ + const register = (args, run, execute) => { + const key = JSON.stringify([checkout, args]); + cleanups.set(key, { run, execute }); + return key; + }; + const abortArgs = ["rebase", "--abort"]; + const resetArgs = ["reset", "--hard", originalHead]; + const abortRebase = async cleanupMethod => { + const inMergeRebase = await hasGitStateDirectory(execApi, "rebase-merge", cleanupOptions); + const inApplyRebase = await hasGitStateDirectory(execApi, "rebase-apply", cleanupOptions); + if (!(inMergeRebase || inApplyRebase)) { + return cleanupMethod === "exec" ? 0 : { exitCode: 0, stdout: "", stderr: "" }; + } + return execApi[cleanupMethod]("git", abortArgs, ...cleanupOptions); + }; + const abortKey = register(abortArgs, () => abortRebase("exec"), abortRebase); + const resetKey = register(resetArgs, () => execApi.exec("git", resetArgs, ...cleanupOptions)); + return () => { + cleanups.delete(abortKey); + cleanups.delete(resetKey); + }; + }; return guarded; } async function cleanup() { const errors = []; - for (const run of cleanups.values()) { + for (const cleanup of cleanups.values()) { try { - const exitCode = await run(); + const exitCode = await cleanup.run(); if (typeof exitCode === "number" && exitCode !== 0) throw new Error(`Git cleanup exited with code ${exitCode}`); } catch (error) { errors.push(error); diff --git a/actions/setup/js/transport_integrity_git.test.cjs b/actions/setup/js/transport_integrity_git.test.cjs index 2d623b0db04..e8c8c253646 100644 --- a/actions/setup/js/transport_integrity_git.test.cjs +++ b/actions/setup/js/transport_integrity_git.test.cjs @@ -110,4 +110,63 @@ describe("transport integrity Git cleanup", () => { expect(mustGit(["status", "--porcelain"])).toBe(""); expect(api.exec).toHaveBeenCalledWith("git", ["am", "--abort"], expect.objectContaining({ cwd: repo })); }); + + it.each(["explicit abort", "finalizer cleanup"])("does not mask malformed patch errors with %s when git am never starts", async cleanup => { + const patch = path.join(directory, "malformed.patch"); + fs.writeFileSync(patch, "not a format-patch file\n"); + const api = execApi(() => {}); + const handler = withTransportIntegrity(async (_message, _ids, transport) => { + transport.captureOptional(patch); + const exec = transport.guardExec(api); + try { + await exec.exec("git", ["am", "--3way", patch], { cwd: repo }); + } catch (error) { + if (cleanup === "explicit abort") await exec.exec("git", ["am", "--abort"], { cwd: repo }); + throw error; + } + }); + + await expect(handler({}, {})).rejects.toThrow("Patch format detection failed"); + + expect(api.exec.mock.calls.some(([, args]) => args[0] === "am" && args[1] === "--abort")).toBe(false); + expect(mustGit(["status", "--porcelain"])).toBe(""); + }); + + it("restores the pinned checkout after integrity failure during a conflicting bundle rewrite", async () => { + const baseHead = mustGit(["rev-parse", "HEAD"]).trim(); + mustGit(["checkout", "-qb", "feature"]); + fs.writeFileSync(path.join(repo, "file.txt"), "feature\n"); + mustGit(["commit", "-qam", "feature"]); + const originalHead = mustGit(["rev-parse", "HEAD"]).trim(); + const bundle = path.join(directory, "aw-feature.bundle"); + mustGit(["bundle", "create", bundle, "feature", `^${baseHead}`]); + + mustGit(["checkout", "-q", "main"]); + fs.writeFileSync(path.join(repo, "file.txt"), "target\n"); + mustGit(["commit", "-qam", "target"]); + mustGit(["update-ref", "refs/remotes/origin/main", mustGit(["rev-parse", "HEAD"]).trim()]); + mustGit(["checkout", "-q", "feature"]); + + const api = execApi(args => { + if (args[0] === "rebase" && args[1] === "--onto") { + fs.unlinkSync(bundle); + fs.writeFileSync(bundle, "changed during rebase"); + } + }); + const handler = withTransportIntegrity(async (_message, _ids, transport) => { + transport.captureOptional(bundle); + const { rewriteBundleBranchAsSingleCommit } = require("./create_pull_request.cjs"); + await rewriteBundleBranchAsSingleCommit("main", transport.guardExec(api), bundle, { gitOpts: { cwd: repo } }); + }); + + await expect(handler({}, {})).resolves.toMatchObject({ success: false, error_type: "ERR_TRANSPORT_INTEGRITY" }); + + expect(api.exec).toHaveBeenCalledWith("git", ["rebase", "--abort"], expect.objectContaining({ cwd: repo })); + expect(api.exec).toHaveBeenCalledWith("git", ["reset", "--hard", originalHead], expect.objectContaining({ cwd: repo })); + expect(mustGit(["rev-parse", "HEAD"]).trim()).toBe(originalHead); + expect(fs.readFileSync(path.join(repo, "file.txt"), "utf8")).toBe("feature\n"); + expect(fs.existsSync(path.join(repo, ".git", "rebase-merge"))).toBe(false); + expect(fs.existsSync(path.join(repo, ".git", "rebase-apply"))).toBe(false); + expect(mustGit(["status", "--porcelain"])).toBe(""); + }); }); diff --git a/actions/setup/js/transport_integrity_handlers.test.cjs b/actions/setup/js/transport_integrity_handlers.test.cjs index fd05b63bed7..1d730dd29b6 100644 --- a/actions/setup/js/transport_integrity_handlers.test.cjs +++ b/actions/setup/js/transport_integrity_handlers.test.cjs @@ -133,6 +133,15 @@ describe.each(["create_pull_request", "push_to_pull_request_branch"])("%s transp it("uses a stable patch during git am and does not recover tampering as a conflict", async () => { let applied; + const applySession = path.join(directory, "active-rebase-apply"); + fs.mkdirSync(applySession); + const originalGetExecOutput = global.exec.getExecOutput.getMockImplementation(); + global.exec.getExecOutput.mockImplementation(async (command, args, options) => { + if (command === "git" && args[0] === "rev-parse" && args[1] === "--git-path" && args[2] === "rebase-apply") { + return { exitCode: 0, stdout: `${applySession}\n`, stderr: "" }; + } + return originalGetExecOutput(command, args, options); + }); global.exec.exec.mockImplementation(async (_command, args) => { if (args[0] === "am" && args[1] === "--3way") { const snapshot = args[2]; @@ -141,6 +150,7 @@ describe.each(["create_pull_request", "push_to_pull_request_branch"])("%s transp applied = fs.readFileSync(snapshot, "utf8"); throw new Error("git conflict"); } + if (args[0] === "am" && args[1] === "--abort") fs.rmSync(applySession, { recursive: true, force: true }); return 0; }); const result = await run(); From 60543232d5d72bc4609dd4e39104cc75e9827f21 Mon Sep 17 00:00:00 2001 From: pelikhan Date: Wed, 14 Oct 2026 15:13:44 -0700 Subject: [PATCH 6/7] Bind transport resolution to the qualified target and reject ambiguity Generate patch and bundle names with the authorized qualified slug in both PR producers. Resolve the same target before privileged capture, reject competing or mixed namespaces permanently, and preserve only a unique legacy namespace. Add producer/consumer, handler, and formal-model regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- actions/setup/js/create_pull_request.cjs | 19 ++- .../setup/js/push_to_pull_request_branch.cjs | 18 ++- actions/setup/js/resolve_transport_paths.cjs | 45 +++---- .../setup/js/resolve_transport_paths.test.cjs | 127 ++++++++---------- actions/setup/js/safe_outputs_handlers.cjs | 8 +- .../setup/js/safe_outputs_handlers.test.cjs | 48 +++++++ .../js/transport_integrity_handlers.test.cjs | 52 +++++++ .../docs/specs/safe-outputs-specification.md | 5 +- specs/workflow-security/README.md | 7 + .../workflow-security/SafeOutputTransport.cfg | 2 + .../workflow-security/SafeOutputTransport.tla | 25 +++- specs/workflow-security/check_transport.py | 2 + 12 files changed, 228 insertions(+), 130 deletions(-) diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 8f1bdbebc54..3c7210258c0 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -1021,10 +1021,17 @@ async function main(config = {}) { core.info(`Processing create_pull_request: title=${pullRequestItem.title || "No title"}, bodyLength=${pullRequestItem.body?.length || 0}`); + const repoResult = resolveAndValidateRepo(pullRequestItem, defaultTargetRepo, allowedRepos, "pull request"); + if (!repoResult.success) { + core.warning(`Skipping pull request: ${repoResult.error}`); + return { success: false, error: repoResult.error }; + } + const { repo: itemRepo, repoParts } = repoResult; + // Determine the patch and bundle file paths. The MCP server sets these on // the entry it writes, but the validation step strips them as a defense // against agent-forged values. Recover them by re-deriving from `branch`. - const transportPaths = resolveTransportPaths(pullRequestItem, defaultTargetRepo); + const transportPaths = resolveTransportPaths(pullRequestItem, itemRepo); transport.captureOptional(transportPaths.patchPath); transport.captureOptional(transportPaths.bundlePath); const patchFilePath = transportPaths.patchPath; @@ -1035,16 +1042,6 @@ async function main(config = {}) { if (bundleFilePath) { core.info(`Bundle file path: ${bundleFilePath}`); } - // Resolve and validate target repository - const repoResult = resolveAndValidateRepo(pullRequestItem, defaultTargetRepo, allowedRepos, "pull request"); - if (!repoResult.success) { - core.warning(`Skipping pull request: ${repoResult.error}`); - return { - success: false, - error: repoResult.error, - }; - } - const { repo: itemRepo, repoParts } = repoResult; const allowedMentionAliases = await resolveMentionsForItem(context, githubClient, core, config.mentions, defaultMentionAliases, defaultTargetRepo, repoResult); core.info(`Target repository: ${itemRepo}`); let pushRepo = itemRepo; diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 46dfb25763e..caacb52420c 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -462,10 +462,17 @@ async function main(config = {}) { processedCount++; + const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "push to PR branch"); + if (!repoResult.success) { + return { success: false, error: repoResult.error }; + } + const itemRepo = repoResult.repo; + const repoParts = repoResult.repoParts; + // Determine the patch and bundle file paths. The MCP server sets these on // the entry it writes, but the validation step strips them as a defense // against agent-forged values. Recover them by re-deriving from `branch`. - const transportPaths = resolveTransportPaths(message, defaultTargetRepo); + const transportPaths = resolveTransportPaths(message, itemRepo); transport.captureOptional(transportPaths.patchPath); transport.captureOptional(transportPaths.bundlePath); const patchFilePath = transportPaths.patchPath; @@ -700,15 +707,6 @@ async function main(config = {}) { return { success: false, error: "Pull request number is required but not found" }; } - // Resolve and validate target repository - // For cross-repo scenarios, the PR may be in a different repository than the workflow - const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "push to PR branch"); - if (!repoResult.success) { - return { success: false, error: repoResult.error }; - } - const itemRepo = repoResult.repo; - const repoParts = repoResult.repoParts; - core.info(`Target repository: ${itemRepo}`); // Resolve the checkout directory for the target repo. diff --git a/actions/setup/js/resolve_transport_paths.cjs b/actions/setup/js/resolve_transport_paths.cjs index 07f27aea25e..0d97710e55b 100644 --- a/actions/setup/js/resolve_transport_paths.cjs +++ b/actions/setup/js/resolve_transport_paths.cjs @@ -5,10 +5,11 @@ const fs = require("fs"); const { getPatchPathForBranch, getPatchPathForBranchInRepo } = require("./git_patch_utils.cjs"); const { getBundlePathForBranch, getBundlePathForBranchInRepo } = require("./generate_git_bundle.cjs"); +const { TransportIntegrityError } = require("./transport_integrity.cjs"); /** * Derive patch and bundle file paths for a safe-output message from the - * validated `branch` (and optional `repo`) fields. + * validated branch and qualified target repository. * * Paths are always re-derived from `branch` so that even a malicious agent * cannot point the privileged handler at a file outside the canonical @@ -16,37 +17,31 @@ const { getBundlePathForBranch, getBundlePathForBranchInRepo } = require("./gene * `getPatchPathForBranch` / `getBundlePathForBranch` enforces that prefix. * * @param {Object} message - The safe-output message - * @param {string} [defaultTargetRepo] - Default target repo slug used as a fallback - * candidate for multi-repo path computation + * @param {string} [qualifiedRepo] - Authorized, fully qualified target repo slug * @returns {{patchPath: string|undefined, bundlePath: string|undefined}} */ -function resolveTransportPaths(message, defaultTargetRepo) { +function resolveTransportPaths(message, qualifiedRepo) { const branch = message.branch; if (!branch) { return { patchPath: undefined, bundlePath: undefined }; } - /** @type {(string|null)[]} */ - const repoCandidates = []; - if (message.repo) repoCandidates.push(message.repo); - if (defaultTargetRepo && defaultTargetRepo !== message.repo) repoCandidates.push(defaultTargetRepo); - repoCandidates.push(null); - let patchPath; - let bundlePath; - for (const repo of repoCandidates) { - const p = repo ? getPatchPathForBranchInRepo(branch, repo) : getPatchPathForBranch(branch); - if (fs.existsSync(p)) { - patchPath = p; - break; - } + const repoCandidates = qualifiedRepo ? [qualifiedRepo, null] : [null]; + const candidates = repoCandidates + .map(repo => { + const patch = repo ? getPatchPathForBranchInRepo(branch, repo) : getPatchPathForBranch(branch); + const bundle = repo ? getBundlePathForBranchInRepo(branch, repo) : getBundlePathForBranch(branch); + return { + patchPath: fs.existsSync(patch) ? patch : undefined, + bundlePath: fs.existsSync(bundle) ? bundle : undefined, + }; + }) + .filter(candidate => candidate.patchPath || candidate.bundlePath); + // Patch and bundle must belong to one namespace, never independently selected + // from qualified and legacy artifacts left by separate producer invocations. + if (candidates.length > 1) { + throw new TransportIntegrityError(branch, undefined, "resolve", "ambiguous transport candidates"); } - for (const repo of repoCandidates) { - const p = repo ? getBundlePathForBranchInRepo(branch, repo) : getBundlePathForBranch(branch); - if (fs.existsSync(p)) { - bundlePath = p; - break; - } - } - return { patchPath, bundlePath }; + return candidates[0] || { patchPath: undefined, bundlePath: undefined }; } module.exports = { resolveTransportPaths }; diff --git a/actions/setup/js/resolve_transport_paths.test.cjs b/actions/setup/js/resolve_transport_paths.test.cjs index 4dd5a62fa72..81d56fe5d97 100644 --- a/actions/setup/js/resolve_transport_paths.test.cjs +++ b/actions/setup/js/resolve_transport_paths.test.cjs @@ -1,95 +1,80 @@ // @ts-check -import { describe, it, expect, beforeEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import crypto from "crypto"; import fs from "fs"; - import { resolveTransportPaths } from "./resolve_transport_paths.cjs"; +import { getPatchPathForBranch, getPatchPathForBranchInRepo } from "./git_patch_utils.cjs"; +import { getBundlePathForBranch, getBundlePathForBranchInRepo } from "./generate_git_bundle.cjs"; +let branch; +const created = new Set(); beforeEach(() => { - // git_patch_utils.cjs hardcodes /tmp/gh-aw fs.mkdirSync("/tmp/gh-aw", { recursive: true }); + branch = `resolve-${crypto.randomUUID()}`; +}); +afterEach(() => { + for (const file of created) fs.rmSync(file, { force: true }); + created.clear(); }); +function write(kind, repo = undefined) { + const file = kind === "patch" ? (repo ? getPatchPathForBranchInRepo(branch, repo) : getPatchPathForBranch(branch)) : repo ? getBundlePathForBranchInRepo(branch, repo) : getBundlePathForBranch(branch); + created.add(file); + fs.writeFileSync(file, `${kind} content`); + return file; +} + describe("resolveTransportPaths", () => { - it("returns undefined values when message has no branch", () => { - const result = resolveTransportPaths({}, "owner/repo"); - expect(result.patchPath).toBeUndefined(); - expect(result.bundlePath).toBeUndefined(); + it("returns undefined values when message has no branch or no files exist", () => { + expect(resolveTransportPaths({}, "owner/repo")).toEqual({ patchPath: undefined, bundlePath: undefined }); + expect(resolveTransportPaths({ branch }, "owner/repo")).toEqual({ patchPath: undefined, bundlePath: undefined }); }); - it("recovers single-repo patch path from branch when not present on the message", () => { - const branch = `resolve-test-${Date.now()}`; - const expectedPatch = `/tmp/gh-aw/aw-${branch}.patch`; - const expectedBundle = `/tmp/gh-aw/aw-${branch}.bundle`; - fs.writeFileSync(expectedPatch, "patch-content"); - fs.writeFileSync(expectedBundle, "bundle-content"); - try { - const result = resolveTransportPaths({ branch }, undefined); - expect(result.patchPath).toBe(expectedPatch); - expect(result.bundlePath).toBe(expectedBundle); - } finally { - fs.rmSync(expectedPatch, { force: true }); - fs.rmSync(expectedBundle, { force: true }); - } + it.each([undefined, "owner/repo"])("accepts a unique legacy namespace with target %s", repo => { + const patchPath = write("patch"); + const bundlePath = write("bundle"); + expect(resolveTransportPaths({ branch }, repo)).toEqual({ patchPath, bundlePath }); }); - it("prefers multi-repo path from message.repo when that file exists", () => { - const branch = `resolve-multi-${Date.now()}`; - const repo = "owner/repo"; - const sanitizedRepo = "owner-repo"; - const expectedPatch = `/tmp/gh-aw/aw-${sanitizedRepo}-${branch}.patch`; - fs.writeFileSync(expectedPatch, "patch-content"); - try { - const result = resolveTransportPaths({ branch, repo }, undefined); - expect(result.patchPath).toBe(expectedPatch); - } finally { - fs.rmSync(expectedPatch, { force: true }); - } + it.each(["repo", " owner/repo ", undefined])("uses only the validated qualified slug, not raw repo %s", repo => { + const patchPath = write("patch", "owner/repo"); + const bundlePath = write("bundle", "owner/repo"); + write("patch", "repo"); + write("bundle", "owner/different"); + expect(resolveTransportPaths({ branch, repo, patch: "/untrusted.patch", bundle: "/untrusted.bundle" }, "owner/repo")).toEqual({ patchPath, bundlePath }); }); - it("falls back to defaultTargetRepo when message.repo is absent", () => { - const branch = `resolve-default-${Date.now()}`; - const repo = "owner/defaultrepo"; - const sanitizedRepo = "owner-defaultrepo"; - const expectedPatch = `/tmp/gh-aw/aw-${sanitizedRepo}-${branch}.patch`; - fs.writeFileSync(expectedPatch, "patch-content"); - try { - const result = resolveTransportPaths({ branch }, repo); - expect(result.patchPath).toBe(expectedPatch); - } finally { - fs.rmSync(expectedPatch, { force: true }); - } + it.each(["patch", "bundle"])("accepts a single qualified %s without its companion", kind => { + const file = write(kind, "owner/repo"); + expect(resolveTransportPaths({ branch }, "owner/repo")).toEqual({ + patchPath: kind === "patch" ? file : undefined, + bundlePath: kind === "bundle" ? file : undefined, + }); }); - it("falls back to single-repo path when neither multi-repo candidate exists", () => { - const branch = `resolve-fallback-${Date.now()}`; - const expectedPatch = `/tmp/gh-aw/aw-${branch}.patch`; - fs.writeFileSync(expectedPatch, "patch-content"); - try { - const result = resolveTransportPaths({ branch, repo: "owner/nonexistent" }, "owner/alsoNo"); - expect(result.patchPath).toBe(expectedPatch); - } finally { - fs.rmSync(expectedPatch, { force: true }); - } + it.each([ + ["patch", "patch"], + ["bundle", "bundle"], + ["patch", "bundle"], + ["bundle", "patch"], + ])("rejects qualified %s plus legacy %s instead of preferring or mixing them", (qualifiedKind, legacyKind) => { + write(qualifiedKind, "owner/repo"); + write(legacyKind); + expect(() => resolveTransportPaths({ branch }, "owner/repo")).toThrow(expect.objectContaining({ code: "ERR_TRANSPORT_INTEGRITY", operation: "resolve", reason: "ambiguous transport candidates" })); }); - it("returns undefined when no candidate file exists on disk", () => { - const branch = `resolve-missing-${Date.now()}`; - const result = resolveTransportPaths({ branch }, "owner/repo"); - expect(result.patchPath).toBeUndefined(); - expect(result.bundlePath).toBeUndefined(); + it("rejects duplicate complete namespaces even when their contents are identical", () => { + for (const repo of [undefined, "owner/repo"]) { + write("patch", repo); + write("bundle", repo); + } + expect(() => resolveTransportPaths({ branch }, "owner/repo")).toThrow("ambiguous transport candidates"); }); - it("recovers bundle path independently of patch path", () => { - const branch = `resolve-bundle-only-${Date.now()}`; - const expectedBundle = `/tmp/gh-aw/aw-${branch}.bundle`; - fs.writeFileSync(expectedBundle, "bundle-content"); - try { - const result = resolveTransportPaths({ branch }, undefined); - expect(result.patchPath).toBeUndefined(); - expect(result.bundlePath).toBe(expectedBundle); - } finally { - fs.rmSync(expectedBundle, { force: true }); - } + it("does not use a different repository's artifacts as a fallback", () => { + write("patch", "owner/default"); + write("bundle", "owner/default"); + expect(resolveTransportPaths({ branch, repo: "other" }, "owner/other")).toEqual({ patchPath: undefined, bundlePath: undefined }); }); }); diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index 92e2e219a6a..e88273c939b 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -1069,13 +1069,10 @@ function createHandlers(server, appendSafeOutput, config = {}) { const useBundle = patchFormat === "bundle"; // Build common options for both patch and bundle generation - const transportOptions = {}; + const transportOptions = { repoSlug: repoResult.repo }; if (repoCwd) { transportOptions.cwd = repoCwd; } - if (repoSlug) { - transportOptions.repoSlug = repoSlug; - } // Pass per-handler token so cross-repo PATs are used for git fetch when configured. // Falls back to GITHUB_TOKEN if not set. if (prConfig["github-token"]) { @@ -1501,7 +1498,7 @@ function createHandlers(server, appendSafeOutput, config = {}) { } // Build common options for both patch and bundle generation - const pushTransportOptions = { mode: "incremental" }; + const pushTransportOptions = { mode: "incremental", repoSlug: repoResult.repo }; if (prHeadBaseline) { if (prHeadBaseline.ref) { pushTransportOptions.incrementalBaseRef = prHeadBaseline.ref; @@ -1512,7 +1509,6 @@ function createHandlers(server, appendSafeOutput, config = {}) { } if (repoCwd) { pushTransportOptions.cwd = repoCwd; - pushTransportOptions.repoSlug = repoResult.repo; } // Pass per-handler token so cross-repo PATs are used for git fetch when configured. // Falls back to GITHUB_TOKEN if not set. diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index f7bbaa1c7d5..66ed3d84c89 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -4,6 +4,9 @@ import fs from "fs"; import path from "path"; import { execSync } from "child_process"; import { createHandlers, hasUpdatePullRequestFields } from "./safe_outputs_handlers.cjs"; +import { resolveTransportPaths } from "./resolve_transport_paths.cjs"; +import { getPatchPathForBranchInRepo } from "./git_patch_utils.cjs"; +import { getBundlePathForBranchInRepo } from "./generate_git_bundle.cjs"; import { looksLikeExploratoryBranch, normalizeProbeValue, @@ -1017,6 +1020,51 @@ describe("safe_outputs_handlers", () => { }); }); + describe.each([ + ["create_pull_request", "createPullRequestHandler"], + ["push_to_pull_request_branch", "pushToPullRequestBranchHandler"], + ])("%s producer/consumer repository identity", (type, handlerName) => { + it.each([ + { name: "implicit workflow repository", repo: undefined, target: undefined }, + { name: "bare repository name", repo: "test-repo", target: undefined }, + { name: "padded qualified repository", repo: " test-owner/test-repo ", target: undefined }, + { name: "explicit repository overriding the default", repo: "test-repo", target: "test-owner/default" }, + { name: "fork-backed head repository", repo: undefined, target: undefined, head: "automation/fork" }, + ])("round-trips $name through qualified patch and bundle paths", async ({ repo, target, head }) => { + process.env.GITHUB_REPOSITORY = "test-owner/test-repo"; + process.env.GITHUB_BASE_REF = "main"; + const branch = `transport-${crypto.randomUUID()}`; + const patchPath = getPatchPathForBranchInRepo(branch, "test-owner/test-repo"); + const bundlePath = getBundlePathForBranchInRepo(branch, "test-owner/test-repo"); + try { + execSync("git init -b main && git config user.email test@example.com && git config user.name Test", { cwd: testWorkspaceDir, stdio: "pipe" }); + fs.writeFileSync(path.join(testWorkspaceDir, "file.txt"), "base\n"); + execSync("git add file.txt && git commit -m base", { cwd: testWorkspaceDir, stdio: "pipe" }); + const base = execSync("git rev-parse HEAD", { cwd: testWorkspaceDir, stdio: "pipe" }).toString().trim(); + execSync(`git checkout -b ${branch} && git remote add origin https://github.com/test-owner/test-repo.git && git update-ref refs/remotes/origin/main ${base} && git update-ref refs/remotes/origin/${branch} ${base}`, { + cwd: testWorkspaceDir, + stdio: "pipe", + }); + fs.writeFileSync(path.join(testWorkspaceDir, "file.txt"), "changed\n"); + execSync("git add file.txt && git commit -m change", { cwd: testWorkspaceDir, stdio: "pipe" }); + const producer = createHandlers(mockServer, mockAppendSafeOutput, { + [type]: { base_branch: "main", patch_format: "bundle", "target-repo": target, "head-repo": head, allowed_repos: ["test-owner/test-repo"] }, + }); + const result = await producer[handlerName]({ repo, branch, title: "Transport changes", body: "Changes", pull_request_number: 123 }); + expect(result.isError, result.content[0].text).not.toBe(true); + const response = JSON.parse(result.content[0].text); + expect(response.patch.path).toBe(patchPath); + expect(response.bundle.path).toBe(bundlePath); + const entry = mockAppendSafeOutput.mock.calls[0][0]; + expect(resolveTransportPaths(entry, "test-owner/test-repo")).toEqual({ patchPath, bundlePath }); + } finally { + fs.rmSync(patchPath, { force: true }); + fs.rmSync(bundlePath, { force: true }); + delete process.env.GITHUB_BASE_REF; + } + }); + }); + describe("createPullRequestHandler", () => { /** * Creates a side-repo checkout where: diff --git a/actions/setup/js/transport_integrity_handlers.test.cjs b/actions/setup/js/transport_integrity_handlers.test.cjs index 1d730dd29b6..91e0ddd3f37 100644 --- a/actions/setup/js/transport_integrity_handlers.test.cjs +++ b/actions/setup/js/transport_integrity_handlers.test.cjs @@ -3,6 +3,8 @@ import { createRequire } from "module"; import fs from "fs"; import path from "path"; import { useExecutionProvenanceFixture } from "./test_execution_provenance.js"; +import { getPatchPathForBranch, getPatchPathForBranchInRepo } from "./git_patch_utils.cjs"; +import { getBundlePathForBranch, getBundlePathForBranchInRepo } from "./generate_git_bundle.cjs"; useExecutionProvenanceFixture(); const require = createRequire(import.meta.url); @@ -109,6 +111,56 @@ describe.each(["create_pull_request", "push_to_pull_request_branch"])("%s transp expect(global.exec.exec.mock.calls.some(([, args]) => args[0] === "push")).toBe(false); } + it.each(["duplicate patch", "duplicate bundle", "split namespace"])("rejects ambiguous %s before Git, API calls, or fallbacks", async kind => { + const branch = `ambiguous-${path.basename(directory)}`; + const files = [ + kind === "duplicate bundle" ? getBundlePathForBranchInRepo(branch, "test-owner/test-repo") : getPatchPathForBranchInRepo(branch, "test-owner/test-repo"), + kind === "duplicate bundle" ? getBundlePathForBranch(branch) : getPatchPathForBranch(branch), + ]; + if (kind === "split namespace") files[1] = getBundlePathForBranch(branch); + fs.mkdirSync("/tmp/gh-aw", { recursive: true }); + try { + for (const file of files) fs.writeFileSync(file, patchContent); + require("./resolve_transport_paths.cjs").resolveTransportPaths = originalResolver; + delete require.cache[require.resolve(`./${handlerName}.cjs`)]; + const { main } = require(`./${handlerName}.cjs`); + const handler = await main({ base_branch: "main", allow_empty: true, fallback_as_issue: true }); + const result = await handler({ branch, title: "Test", body: "Body", repo: "test-repo" }, {}); + expect(result).toEqual(expect.objectContaining({ success: false, error_type: "ERR_TRANSPORT_INTEGRITY", error: expect.stringContaining("ambiguous transport candidates") })); + expect(global.core.error).toHaveBeenCalledWith(expect.stringContaining("resolve: ambiguous transport candidates")); + expect(global.exec.exec).not.toHaveBeenCalled(); + expect(global.exec.getExecOutput).not.toHaveBeenCalled(); + assertNoRemoteSideEffects(); + } finally { + for (const file of files) fs.rmSync(file, { force: true }); + } + }); + + it.each([undefined, "test-repo", " test-owner/test-repo "])("resolves repo %s with the qualified authorized slug before applying", async repo => { + const branch = `qualified-${path.basename(directory)}`; + const file = getPatchPathForBranchInRepo(branch, "test-owner/test-repo"); + fs.mkdirSync("/tmp/gh-aw", { recursive: true }); + try { + fs.writeFileSync(file, patchContent); + const resolve = vi.fn(originalResolver); + require("./resolve_transport_paths.cjs").resolveTransportPaths = resolve; + delete require.cache[require.resolve(`./${handlerName}.cjs`)]; + const { main } = require(`./${handlerName}.cjs`); + const handler = await main({ base_branch: "main", signed_commits: false }); + global.exec.exec.mockImplementation(async () => { + fs.unlinkSync(file); + return 0; + }); + const result = await handler({ branch, repo, title: "Test", body: "Body" }, {}); + expect(resolve).toHaveBeenCalledWith(expect.objectContaining({ repo }), "test-owner/test-repo"); + expect(result.error_type).toBe("ERR_TRANSPORT_INTEGRITY"); + expect(global.exec.exec).toHaveBeenCalled(); + assertNoRemoteSideEffects(); + } finally { + fs.rmSync(file, { force: true }); + } + }); + it.each(["same-size mutation", "replacement", "deletion"])("fails closed on patch %s after validation", async attack => { let attacked = false; global.exec.exec.mockImplementation(async (_command, args) => { diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index ac2dfea4d2f..279cd627176 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -2610,7 +2610,7 @@ These requirements apply to `create_pull_request` and `push_to_pull_request_bran These requirements apply to `create_pull_request` and `push_to_pull_request_branch`, including unsigned delivery, bundle recovery, signed delivery, PR/issue publication retries, and optional CI-trigger commits. They bind handler-time policy validation and Git consumption to captured bytes; capture alone does not establish policy approval or detection provenance. -**PTI-001 — Trusted capture**: The privileged handler MUST derive canonical transport paths from the validated branch and repository rather than agent-supplied paths. For each resolved patch or bundle, it MUST capture raw bytes, a cryptographic digest, and descriptor-verified device/inode identity. Opens MUST reject symlinks and nonregular files without blocking on FIFOs. Missing or unreadable resolved artifacts MUST fail closed, including in allow-empty mode. +**PTI-001 — Trusted capture**: Producer and consumer MUST derive canonical transport paths from the same validated branch and fully qualified, authorized target repository slug, not raw agent-supplied repository/path values or the head repository of a fork-backed delivery. Bare names and surrounding whitespace MUST be normalized by repository authorization before path resolution. Producers MUST qualify filenames even for the implicit workflow repository. Consumers MAY accept a unique legacy unqualified namespace, but MUST reject multiple matching namespaces before capture or side effects, including duplicate patches/bundles and a patch in one namespace paired with a bundle in another. They MUST NOT search another default repository after resolving an explicit target. For each resolved patch or bundle, the handler MUST capture raw bytes, a cryptographic digest, and descriptor-verified device/inode identity. Opens MUST reject symlinks and nonregular files without blocking on FIFOs. Missing or unreadable resolved artifacts MUST fail closed, including in allow-empty mode. **PTI-002 — Stable consumption**: Git operations that consume a captured path MUST receive a private, read-only snapshot outside the checkout, not the agent-writable original. A bundle's verification, head discovery, imports, and recovery retries MUST use the same snapshot unless a trusted transformation explicitly replaces it. Capture and snapshots MUST preserve raw bytes, including non-UTF-8 data. @@ -2628,7 +2628,7 @@ These requirements apply to `create_pull_request` and `push_to_pull_request_bran | Requirement | Implementation | Regression evidence | |---|---|---| -| PTI-001–004 | `resolve_transport_paths.cjs`, `transport_integrity.cjs`, both PR handlers | `resolve_transport_paths.test.cjs`, `transport_integrity.test.cjs`, `transport_integrity_handlers.test.cjs` | +| PTI-001–004 | `safe_outputs_handlers.cjs`, `resolve_transport_paths.cjs`, `transport_integrity.cjs`, both PR handlers | `safe_outputs_handlers.test.cjs` producer/consumer round trips; `resolve_transport_paths.test.cjs`, `transport_integrity.test.cjs`, `transport_integrity_handlers.test.cjs` | | PTI-003 cost bound | Lightweight local checks and cached snapshot digests | An 8 MiB bundle and 100 unrelated commands require zero full-file reads and hashes; subsequent publication still rejects same-size mutation | | PTI-005 | `error_recovery.cjs`, `create_or_update_pull_request.cjs`, guarded publication callbacks | `error_recovery.test.cjs`, `create_or_update_pull_request.test.cjs`; real typed errors with network/timeout/rate-limit paths | | PTI-006–007 | Registered cleanup and structured diagnostics in `transport_integrity.cjs` | `transport_integrity_git.test.cjs` verifies real Git refs and conflict rollback; helper tests cover checkout pinning, failed cleanup, cause retention, and safe logging | @@ -6200,6 +6200,7 @@ This specification revision aligns with directly relevant `CHANGELOG.md` entries **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. +- **Specified**: Producer/consumer qualified repository identity and fail-closed ambiguous namespace selection, including mixed patch/bundle namespaces and unique legacy compatibility. - **Added**: Runtime regression mappings and a bounded transport lifecycle TLA+ model; no new compiler rule or token permission is introduced. - Scoped RCR8-RCR15 multi-repository controls to literal `multi: true`; required single-selector runtime validation in both jobs when omitted/false, without list restrictions or changes to write authorization. - Extended the bounded checkout model with explicit mode, single-selector rejection, negative controls, and reachability witnesses. diff --git a/specs/workflow-security/README.md b/specs/workflow-security/README.md index 6116e07d809..ded53229723 100644 --- a/specs/workflow-security/README.md +++ b/specs/workflow-security/README.md @@ -110,6 +110,11 @@ model with the runtime contract in Safe Outputs [PTI-001–007](../../docs/src/content/docs/specs/safe-outputs-specification.md#patch-and-bundle-transport-integrity-requirements). It separates original bytes/identity, captured baselines, trusted transformed bytes, private snapshots, applied bytes, and publication-time verification. +Resolution requires producer/consumer agreement on the qualified repository +and a single qualified or legacy namespace for the patch/bundle pair; ambiguity +fails before capture. Namespaces abstract exact shared filename derivation; +real-Git producer/consumer round trips cover bare, padded, implicit, overridden, +and fork-backed target repository cases. One adversarial mutation may change bytes, identity, regular-file status, or an existing snapshot between boundaries; FIFO replacement also occurs before capture. Rate-limited @@ -138,6 +143,8 @@ only `Fault` and requires TLC exit code 12 with the corresponding invariant: | Fault | Expected invariant | |---|---| +| `unqualified-producer` | `QualifiedRepository` | +| `ambiguous-selection` | `UnambiguousResolution` | | `blocking-open` | `NonblockingOpen` | | `reopen-original` | `StableConsumption` | | `rebaseline` | `OriginalBaseline` | diff --git a/specs/workflow-security/SafeOutputTransport.cfg b/specs/workflow-security/SafeOutputTransport.cfg index c72319e24d6..bb0d41ca393 100644 --- a/specs/workflow-security/SafeOutputTransport.cfg +++ b/specs/workflow-security/SafeOutputTransport.cfg @@ -5,6 +5,8 @@ CONSTANTS INVARIANTS TypeOK StableConsumption + QualifiedRepository + UnambiguousResolution OriginalBaseline PublicationIntegrity PermanentFailure diff --git a/specs/workflow-security/SafeOutputTransport.tla b/specs/workflow-security/SafeOutputTransport.tla index 7712e827fb2..12ec88b74fe 100644 --- a/specs/workflow-security/SafeOutputTransport.tla +++ b/specs/workflow-security/SafeOutputTransport.tla @@ -1,22 +1,24 @@ ------------------------- MODULE SafeOutputTransport ------------------------- -EXTENDS Naturals, Sequences +EXTENDS Naturals, Sequences, FiniteSets CONSTANTS Fault, Transport ASSUME Transport \in {"patch", "bundle"} ASSUME Fault \in {"none", "blocking-open", "reopen-original", "rebaseline", "skip-publication-check", "retry-integrity", "cleanup-blocked", "cleanup-error-hidden", "unsafe-diagnostics", - "redundant-hashing"} + "redundant-hashing", "unqualified-producer", "ambiguous-selection"} \* Byte classes abstract collision-free digests, not Git's or the OS's parser. VARIABLE s vars == <> -Stages == {"capture", "validate", "apply", "conflict", "publish", "complete", +Stages == {"resolve", "capture", "validate", "apply", "conflict", "publish", "complete", "failed", "done"} Resources == {"am-state", "bundle-ref"} Init == - s = [stage |-> "capture", original |-> 0, identity |-> 0, + s = [stage |-> "resolve", original |-> 0, identity |-> 0, + producerRepo |-> IF Fault = "unqualified-producer" THEN "bare" ELSE "owner/repo", + consumerRepo |-> "owner/repo", namespaces |-> {}, resolved |-> FALSE, regular |-> TRUE, captured |-> 0, capturedIdentity |-> 0, trusted |-> 0, snapshot |-> 0, snapshotExists |-> FALSE, transformed |-> FALSE, mutated |-> FALSE, localChecked |-> FALSE, @@ -44,6 +46,13 @@ Capture == !.capturedIdentity = s.identity, !.trusted = s.original, !.lastEvent = "Capture"] +Resolve(namespaces) == + /\ s.stage = "resolve" + /\ namespaces \in {{"qualified"}, {"legacy"}, {"qualified", "legacy"}} + /\ IF Cardinality(namespaces) > 1 /\ Fault # "ambiguous-selection" THEN Reject + ELSE s' = [s EXCEPT !.stage = "capture", !.namespaces = namespaces, + !.resolved = TRUE, !.lastEvent = "Resolve"] + Mutate(kind) == /\ s.stage \in {"capture", "validate", "apply", "conflict", "publish", "complete"} /\ ~s.mutated @@ -123,6 +132,8 @@ Cleanup(success) == !.lastEvent = IF error THEN "CleanupFailure" ELSE "Cleanup"] Next == + \/ \E namespaces \in {{"qualified"}, {"legacy"}, {"qualified", "legacy"}}: + Resolve(namespaces) \/ Capture \/ Transform \/ Validate \/ LocalCommand \/ CheckConflict \/ Complete \/ \E kind \in {"bytes", "identity", "fifo", "snapshot"}: Mutate(kind) \/ \E success \in BOOLEAN: Apply(success) \/ Publish(success) \/ Cleanup(success) @@ -131,6 +142,8 @@ Spec == Init /\ [][Next]_vars TypeOK == /\ s.stage \in Stages + /\ s.producerRepo \in {"bare", "owner/repo"} /\ s.consumerRepo = "owner/repo" + /\ s.namespaces \subseteq {"qualified", "legacy"} /\ s.resolved \in BOOLEAN /\ s.original \in 0..1 /\ s.captured \in 0..1 /\ s.identity \in 0..1 /\ s.capturedIdentity \in 0..1 /\ s.trusted \in 0..2 /\ s.snapshot \in 0..2 @@ -145,7 +158,9 @@ TypeOK == /\ s.writes \subseteq [verified: BOOLEAN, failed: BOOLEAN] StableConsumption == \A a \in s.applied: a.bytes = a.validated -OriginalBaseline == s.stage # "capture" => s.captured = 0 +QualifiedRepository == s.resolved => s.producerRepo = s.consumerRepo +UnambiguousResolution == s.resolved => Cardinality(s.namespaces) = 1 +OriginalBaseline == s.stage \notin {"resolve", "capture"} => s.captured = 0 PublicationIntegrity == \A w \in s.writes: w.verified /\ ~w.failed PermanentFailure == s.permanentRetries = 0 NonblockingOpen == ~s.blocked diff --git a/specs/workflow-security/check_transport.py b/specs/workflow-security/check_transport.py index ad83b405aff..6ed0bbe5a28 100644 --- a/specs/workflow-security/check_transport.py +++ b/specs/workflow-security/check_transport.py @@ -11,6 +11,8 @@ FAULTS = { + "unqualified-producer": "QualifiedRepository", + "ambiguous-selection": "UnambiguousResolution", "blocking-open": "NonblockingOpen", "reopen-original": "StableConsumption", "rebaseline": "OriginalBaseline", From 73751ae0effc39b1243a690616d5c268db33076d Mon Sep 17 00:00:00 2001 From: pelikhan Date: Wed, 14 Oct 2026 15:32:57 -0700 Subject: [PATCH 7/7] Reject dash-only safe-output branch names Prevent salts from reviving empty normalized branch names and reject invalid transport filename components on both producers and consumers. Clarify the verified review-fix contracts for publication, retries, and cleanup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- actions/setup/js/generate_git_bundle.cjs | 4 +-- actions/setup/js/git_patch_utils.cjs | 6 ++++- actions/setup/js/git_patch_utils.test.cjs | 3 +++ actions/setup/js/normalize_branch_name.cjs | 2 ++ .../setup/js/normalize_branch_name.test.cjs | 7 +++++ .../setup/js/resolve_transport_paths.test.cjs | 8 ++++++ actions/setup/js/safe_outputs_handlers.cjs | 6 +++++ .../setup/js/safe_outputs_handlers.test.cjs | 27 +++++++++++++++++++ .../docs/specs/safe-outputs-specification.md | 11 ++++---- specs/workflow-security/README.md | 4 ++- 10 files changed, 69 insertions(+), 9 deletions(-) diff --git a/actions/setup/js/generate_git_bundle.cjs b/actions/setup/js/generate_git_bundle.cjs index 821bb8891a7..0611ca1c704 100644 --- a/actions/setup/js/generate_git_bundle.cjs +++ b/actions/setup/js/generate_git_bundle.cjs @@ -14,7 +14,7 @@ const path = require("path"); const { getErrorMessage } = require("./error_helpers.cjs"); const { generateGitPatch } = require("./generate_git_patch.cjs"); const { ensureOriginRemoteTrackingRef, execGitSync } = require("./git_helpers.cjs"); -const { isAncestorCommit, describeGitFailure } = require("./git_patch_utils.cjs"); +const { isAncestorCommit, describeGitFailure, sanitizeBranchNameForPatch } = require("./git_patch_utils.cjs"); const { normalizeCommitSHA } = require("./commit_sha_helpers.cjs"); const { ERR_SYSTEM } = require("./error_codes.cjs"); @@ -51,7 +51,7 @@ function sanitizeForFilename(value, fallback) { * @returns {string} The sanitized branch name safe for use in a filename */ function sanitizeBranchNameForBundle(branchName) { - return sanitizeForFilename(branchName, "unknown"); + return sanitizeBranchNameForPatch(branchName); } /** diff --git a/actions/setup/js/git_patch_utils.cjs b/actions/setup/js/git_patch_utils.cjs index c3bb923c5a3..660f2a75ebc 100644 --- a/actions/setup/js/git_patch_utils.cjs +++ b/actions/setup/js/git_patch_utils.cjs @@ -51,7 +51,11 @@ function sanitizeForFilename(value, fallback) { * @returns {string} The sanitized branch name safe for use in a filename */ function sanitizeBranchNameForPatch(branchName) { - return sanitizeForFilename(branchName, "unknown"); + const sanitized = sanitizeForFilename(branchName, "unknown"); + if (!sanitized || (branchName && /^-+$/.test(branchName.trim()))) { + throw new Error("Invalid branch name: provide a name containing more than dashes or filename separators."); + } + return sanitized; } /** diff --git a/actions/setup/js/git_patch_utils.test.cjs b/actions/setup/js/git_patch_utils.test.cjs index 3e72f0c2b28..0c90bd92b45 100644 --- a/actions/setup/js/git_patch_utils.test.cjs +++ b/actions/setup/js/git_patch_utils.test.cjs @@ -97,6 +97,9 @@ describe("git_patch_utils - pure helpers", () => { it("uses empty fallback for empty repo slugs", () => { expect(sanitizeRepoSlugForPatch("")).toBe(""); }); + it.each(["-", "---", " --- ", "///"])("rejects branch %s instead of deriving an empty transport filename", branch => { + expect(() => sanitizeBranchNameForPatch(branch)).toThrow("Invalid branch name"); + }); it("sanitizes owner/repo slugs", () => { expect(sanitizeRepoSlugForPatch("github/Gh-AW")).toBe("github-gh-aw"); }); diff --git a/actions/setup/js/normalize_branch_name.cjs b/actions/setup/js/normalize_branch_name.cjs index 6d20e27dbcb..1c430d5b2fa 100644 --- a/actions/setup/js/normalize_branch_name.cjs +++ b/actions/setup/js/normalize_branch_name.cjs @@ -42,6 +42,8 @@ function normalizeBranchName(branchName, salt = null) { // Ensure it doesn't end with a dash after truncation normalized = normalized.replace(/-+$/, ""); + if (!normalized) return normalized; + // Append a salt suffix when provided if (salt) { normalized = `${normalized}-${salt}`; diff --git a/actions/setup/js/normalize_branch_name.test.cjs b/actions/setup/js/normalize_branch_name.test.cjs index 41e9fb7857e..aaa4b13d573 100644 --- a/actions/setup/js/normalize_branch_name.test.cjs +++ b/actions/setup/js/normalize_branch_name.test.cjs @@ -97,6 +97,13 @@ describe("normalizeBranchName", () => { expect(normalizeBranchName("feature/my-branch")).toBe("feature/my-branch"); }); + it.each(["-", "---", " --- ", "!@#$%^&*()"])("does not turn an empty normalized branch %s into a salted branch", async branch => { + const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); + + expect(normalizeBranchName(branch)).toBe(""); + expect(normalizeBranchName(branch, "abc123")).toBe(""); + }); + it("should truncate to 128 chars before appending salt", async () => { const { normalizeBranchName } = await import("./normalize_branch_name.cjs"); diff --git a/actions/setup/js/resolve_transport_paths.test.cjs b/actions/setup/js/resolve_transport_paths.test.cjs index 81d56fe5d97..0906ef55ae2 100644 --- a/actions/setup/js/resolve_transport_paths.test.cjs +++ b/actions/setup/js/resolve_transport_paths.test.cjs @@ -26,6 +26,14 @@ function write(kind, repo = undefined) { } describe("resolveTransportPaths", () => { + it.each(["-", "---", " --- "])("rejects dash-only branch %s in both patch and bundle paths", branch => { + for (const repo of [undefined, "owner/repo"]) { + expect(() => resolveTransportPaths({ branch }, repo)).toThrow("Invalid branch name"); + expect(() => (repo ? getPatchPathForBranchInRepo(branch, repo) : getPatchPathForBranch(branch))).toThrow("Invalid branch name"); + expect(() => (repo ? getBundlePathForBranchInRepo(branch, repo) : getBundlePathForBranch(branch))).toThrow("Invalid branch name"); + } + }); + it("returns undefined values when message has no branch or no files exist", () => { expect(resolveTransportPaths({}, "owner/repo")).toEqual({ patchPath: undefined, bundlePath: undefined }); expect(resolveTransportPaths({ branch }, "owner/repo")).toEqual({ patchPath: undefined, bundlePath: undefined }); diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index 34d1394e031..763000c70a9 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -1025,6 +1025,9 @@ function createHandlers(server, appendSafeOutput, config = {}) { }; } + if (!normalizeBranchName(entry.branch)) { + return buildIntentErrorResponse("Invalid branch name: provide a name containing more than dashes."); + } const intentValidationError = validateCreatePullRequestIntent(entry); if (intentValidationError) { return buildIntentErrorResponse(intentValidationError); @@ -1466,6 +1469,9 @@ function createHandlers(server, appendSafeOutput, config = {}) { }; } + if (!normalizeBranchName(entry.branch)) { + return buildIntentErrorResponse("Invalid branch name: provide a name containing more than dashes."); + } const intentValidationError = validatePushToPullRequestBranchIntent(entry); if (intentValidationError) { return buildIntentErrorResponse(intentValidationError); diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index 327409680d9..d634ff3444b 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -1598,6 +1598,22 @@ describe("safe_outputs_handlers", () => { expect(mockAppendSafeOutput).not.toHaveBeenCalled(); }); + it.each(["-", "---", " --- "])("rejects dash-only branch %s even when empty PRs are allowed", async branch => { + handlers = createHandlers(mockServer, mockAppendSafeOutput, { + create_pull_request: { allow_empty: true, base_branch: "main" }, + }); + + const result = await handlers.createPullRequestHandler({ + branch, + title: "Reject invalid branch", + body: "Do not publish from a dash-only branch.", + }); + + expect(result.isError).toBe(true); + expect(JSON.parse(result.content[0].text).error).toContain("Invalid branch name"); + expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + }); + it("should validate the resolved current branch before recording a PR intent", async () => { handlers = createHandlers(mockServer, mockAppendSafeOutput, { create_pull_request: { @@ -2153,6 +2169,17 @@ describe("safe_outputs_handlers", () => { expect(responseData.error).not.toContain("requires pull_request_number"); }); + it.each(["-", "---", " --- "])("rejects dash-only push branch %s before recording an intent", async branch => { + const result = await handlers.pushToPullRequestBranchHandler({ + branch, + message: "Apply requested changes.", + }); + + expect(result.isError).toBe(true); + expect(JSON.parse(result.content[0].text).error).toContain("Invalid branch name"); + expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + }); + it("should reject obvious exploratory test payloads before recording a PR branch update intent", async () => { // The agent can no longer supply `branch`; the handler derives it from // the current working checkout. Model the failure mode where the diff --git a/docs/src/content/docs/specs/safe-outputs-specification.md b/docs/src/content/docs/specs/safe-outputs-specification.md index 279cd627176..7aa0752ec76 100644 --- a/docs/src/content/docs/specs/safe-outputs-specification.md +++ b/docs/src/content/docs/specs/safe-outputs-specification.md @@ -2614,13 +2614,13 @@ These requirements apply to `create_pull_request` and `push_to_pull_request_bran **PTI-002 — Stable consumption**: Git operations that consume a captured path MUST receive a private, read-only snapshot outside the checkout, not the agent-writable original. A bundle's verification, head discovery, imports, and recovery retries MUST use the same snapshot unless a trusted transformation explicitly replaces it. Capture and snapshots MUST preserve raw bytes, including non-UTF-8 data. -**PTI-003 — Integrity boundaries and cost**: Full content-digest and identity checks of originals and existing snapshots MUST run at transport reads/application, immediately before publication attempts, and at handler completion. Publication includes remote ref creation/deletion, signed mutations, direct pushes, PR creation, and issue fallbacks; each retried attempt MUST recheck. Unrelated local commands MAY use lightweight identity/type/size checks, but these MUST NOT replace byte checks at trust boundaries. Implementations MUST cache snapshot digests and MUST NOT reread or hash full artifacts around every unrelated command. +**PTI-003 — Integrity boundaries and cost**: Full content-digest and identity checks of originals and existing snapshots MUST run at transport reads/application, immediately before publication attempts, and at handler completion. Publication includes remote ref creation/deletion, signed mutations, direct pushes, PR creation, and issue fallbacks; each retried attempt MUST recheck. Successful PR and fallback-issue creation MUST also recheck after the awaited API request and before returning its response to callers that can enable auto-merge, close older PRs, or assign a coding agent. Unrelated local commands MAY use lightweight identity/type/size checks, but these MUST NOT replace byte checks at trust boundaries. Implementations MUST cache snapshot digests and MUST NOT reread or hash full artifacts around every unrelated command. **PTI-004 — Trusted transformations**: Temporary-ID substitution and commit-title suffixing MUST operate on captured bytes and affect only trusted in-memory content and its private snapshot. They MUST NOT rewrite canonical artifacts or refresh the original digest/identity baseline from disk. The transformed bytes consumed by Git MUST be the trusted transformation of the validated input. -**PTI-005 — Permanent failure**: An integrity failure MUST remain sticky for the invocation and prevent further application or publication. It MUST NOT enter conflict recovery, publication fallbacks, or transient/rate-limit retries, regardless of branch names, paths, or message substrings. A previously completed remote write is not rolled back by this contract; implementations MUST NOT claim transactional rollback across GitHub operations. +**PTI-005 — Permanent failure**: An integrity failure MUST remain sticky for the invocation and prevent further application or publication. It MUST NOT enter conflict recovery, publication fallbacks, or transient/rate-limit retries, regardless of branch names, paths, or message substrings. The shared retry loop MUST reject the stable integrity error classification before invoking any custom retry classifier. A previously completed remote write is not rolled back by this contract; implementations MUST NOT claim transactional rollback across GitHub operations. -**PTI-006 — Restricted cleanup**: Integrity failure MUST NOT prevent registered local cleanup of temporary bundle refs or an in-progress patch application. Cleanup MUST be limited to resources registered by the current guard and pinned to the original checkout; it MUST NOT permit arbitrary ref deletion, remote writes, or renewed artifact consumption. Private snapshots MUST be disposed on success and failure, even when Git cleanup fails. Cleanup failures MUST be surfaced rather than treated as success. +**PTI-006 — Restricted cleanup**: Integrity failure MUST NOT prevent registered local cleanup of temporary bundle refs, an in-progress patch application, or a bundle-rewrite rebase. Rewrite rollback MUST be registered before mutation and pinned to the original checkout and trusted pre-rewrite HEAD; only the registered rebase abort and restoration to that exact HEAD MAY bypass the sticky failure. Explicit and finalizer patch aborts MUST be no-ops when no apply session exists in the pinned checkout, without suppressing genuine abort failures. Cleanup MUST be limited to resources registered by the current guard and pinned to the original checkout; it MUST NOT permit arbitrary resets, ref deletion, remote writes, or renewed artifact consumption. Private snapshots MUST be disposed on success and failure, even when Git cleanup fails. Cleanup failures MUST be surfaced rather than treated as success. **PTI-007 — Diagnostics**: Capture and verification MUST preserve filesystem error causes and distinguish operation, nonregular input, identity replacement, and content mismatch. Handler errors MUST expose the stable `ERR_TRANSPORT_INTEGRITY` classification, including when an intermediate helper wraps the error. Logs MUST use safe operation/reason/system-code diagnostics and MUST NOT expose artifact contents, secret values, or raw filesystem cause text. @@ -2631,13 +2631,13 @@ These requirements apply to `create_pull_request` and `push_to_pull_request_bran | PTI-001–004 | `safe_outputs_handlers.cjs`, `resolve_transport_paths.cjs`, `transport_integrity.cjs`, both PR handlers | `safe_outputs_handlers.test.cjs` producer/consumer round trips; `resolve_transport_paths.test.cjs`, `transport_integrity.test.cjs`, `transport_integrity_handlers.test.cjs` | | PTI-003 cost bound | Lightweight local checks and cached snapshot digests | An 8 MiB bundle and 100 unrelated commands require zero full-file reads and hashes; subsequent publication still rejects same-size mutation | | PTI-005 | `error_recovery.cjs`, `create_or_update_pull_request.cjs`, guarded publication callbacks | `error_recovery.test.cjs`, `create_or_update_pull_request.test.cjs`; real typed errors with network/timeout/rate-limit paths | -| PTI-006–007 | Registered cleanup and structured diagnostics in `transport_integrity.cjs` | `transport_integrity_git.test.cjs` verifies real Git refs and conflict rollback; helper tests cover checkout pinning, failed cleanup, cause retention, and safe logging | +| PTI-006–007 | Registered cleanup and structured diagnostics in `transport_integrity.cjs` | `transport_integrity_git.test.cjs` verifies real Git refs, conflicting-rebase rollback, and malformed-patch aborts; helper tests cover checkout pinning, failed cleanup, cause retention, and safe logging | 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. **Security Requirements**: -- Branch name sanitization (prevent injection) +- 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. - Patch content validation - Size limits on commits - `head-repo` MUST be either the same repository as `target-repo` or an explicitly configured automation-owned fork; arbitrary contributor forks MUST NOT be used as write targets @@ -6201,6 +6201,7 @@ This specification revision aligns with directly relevant `CHANGELOG.md` entries - **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. - **Specified**: Producer/consumer qualified repository identity and fail-closed ambiguous namespace selection, including mixed patch/bundle namespaces and unique legacy compatibility. +- **Clarified**: Integrity checks after awaited PR/issue creation, permanent-error rejection before custom retry classifiers, scoped bundle-rewrite rollback, and session-aware patch aborts. - **Added**: Runtime regression mappings and a bounded transport lifecycle TLA+ model; no new compiler rule or token permission is introduced. - Scoped RCR8-RCR15 multi-repository controls to literal `multi: true`; required single-selector runtime validation in both jobs when omitted/false, without list restrictions or changes to write authorization. - Extended the bounded checkout model with explicit mode, single-selector rejection, negative controls, and reachability witnesses. diff --git a/specs/workflow-security/README.md b/specs/workflow-security/README.md index ded53229723..1ef2c38f4f6 100644 --- a/specs/workflow-security/README.md +++ b/specs/workflow-security/README.md @@ -166,7 +166,9 @@ The bounded model assumes snapshot privacy, collision-free byte classes, trusted transformations and exact registered-cleanup dispatch. It does not model arbitrary same-user filesystem races, Git parsing, App API semantics, unbounded retries, credential selection, or the full structured logging -payload. Those remain implementation obligations covered by +payload. Publication is atomic in this abstraction; post-await checks before +auto-merge, older-PR closure, or coding-agent assignment and exact-HEAD rewrite +rollback remain implementation obligations covered by `transport_integrity.test.cjs`, `transport_integrity_handlers.test.cjs`, `transport_integrity_git.test.cjs`, `error_recovery.test.cjs`, and `create_or_update_pull_request.test.cjs` under `actions/setup/js/`.