fix: Prevent patch/bundle desynchronization in safe-outputs

.github/skills/agentic-workflows/SKILL.md

@@ -12,6 +12,7 @@ This skill is a dispatcher: identify the task type, load the matching `.github/a
 Read only the files you need:
 Load these files from `github/gh-aw` (they are not available locally).
 - `.github/aw/agentic-chat.md`
+- `.github/aw/agentic-workflows-mcp.md`
 - `.github/aw/asciicharts.md`
 - `.github/aw/campaign.md`
 - `.github/aw/charts-trending.md`
@@ -27,6 +28,7 @@ Load these files from `github/gh-aw` (they are not available locally).
 - `.github/aw/github-agentic-workflows.md`
 - `.github/aw/github-mcp-server.md`
 - `.github/aw/llms.md`
+- `.github/aw/mcp-clis.md`
 - `.github/aw/memory.md`
 - `.github/aw/messages.md`
 - `.github/aw/network.md`

actions/setup/js/collect_ndjson_output.cjs

@@ -323,8 +323,12 @@ async function main() {
             }
             continue;
           }
-          // Update item with normalized values
-          Object.assign(item, validationResult.normalizedItem);
+          // SECURITY: Use normalizedItem (which strips infrastructure-only fields
+          // like patch_path, bundle_path, base_commit, diff_size) instead of the
+          // original item, to prevent agent-injected transport metadata from
+          // reaching the privileged handler.
+          core.info(`Line ${i + 1}: Valid ${itemType} item`);
+          parsedItems.push(validationResult.normalizedItem);
         } else {
           // Fall back to validateItemWithSafeJobConfig for unknown types
           const jobOutputType = expectedOutputTypes[itemType];
@@ -341,10 +345,9 @@ async function main() {
             }
             Object.assign(item, validation.normalizedItem);
           }
+          core.info(`Line ${i + 1}: Valid ${itemType} item`);
+          parsedItems.push(item);
         }
-
-        core.info(`Line ${i + 1}: Valid ${itemType} item`);
-        parsedItems.push(item);
       } catch (error) {
         const errorMsg = getErrorMessage(error);
         errors.push(`Line ${i + 1}: Invalid JSON - ${errorMsg}`);

actions/setup/js/generate_git_patch.cjs

@@ -49,6 +49,9 @@ function debugLog(message) {
  * @param {string[]} [options.excludedFiles] - Glob patterns for files to exclude from the patch.
  *   Each pattern is passed to `git format-patch` as a `:(exclude)<pattern>` magic pathspec so
  *   matching files are never included in the generated patch.
+ * @param {string} [options.pinnedSha] - SECURITY: When set, use this SHA as the branch tip instead
+ *   of resolving refs/heads/<branchName>. Prevents TOCTOU races where the agent flips the branch
+ *   ref between patch and bundle generation.
  * @returns {Promise<Object>} Object with patch info or error
  */
 async function generateGitPatch(branchName, baseBranch, options = {}) {
@@ -130,15 +133,25 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
   // can use it directly. The From <sha> header in format-patch output contains the
   // *new* commit SHA which won't exist in the target checkout.
   let baseCommitSha = null;
+  let resolvedTipRef = null;
 
   try {
     // Strategy 1: If we have a branch name, check if that branch exists and get its diff
     if (branchName) {
-      debugLog(`Strategy 1: Checking if branch '${branchName}' exists locally`);
-      // Check if the branch exists locally
+      resolvedTipRef = options.pinnedSha || branchName;
+      // SECURITY: When pinnedSha is provided, use it directly as the tip commit instead
+      // of dereferencing refs/heads/<branchName>. This prevents TOCTOU races where the
+      // agent can flip the branch ref between patch and bundle generation.
+      const tipRef = resolvedTipRef;
       try {
-        execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd });
-        debugLog(`Strategy 1: Branch '${branchName}' exists locally`);
+        if (options.pinnedSha) {
+          debugLog(`Strategy 1: Using pinned SHA ${options.pinnedSha} (branch: ${branchName})`);
+        } else {
+          debugLog(`Strategy 1: Checking if branch '${branchName}' exists locally`);
+          // Check if the branch exists locally
+          execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd });
+          debugLog(`Strategy 1: Branch '${branchName}' exists locally`);
+        }
 
         // Determine base ref for patch generation
         let baseRef;
@@ -251,7 +264,7 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
           }
 
           if (defaultBranchRef) {
-            baseRef = execGitSync(["merge-base", "--", defaultBranchRef, branchName], { cwd }).trim();
+            baseRef = execGitSync(["merge-base", "--", defaultBranchRef, tipRef], { cwd }).trim();
             debugLog(`Strategy 1 (full): Computed merge-base: ${baseRef}`);
           } else {
             // No remote refs available - fall through to Strategy 2
@@ -265,12 +278,12 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
         debugLog(`Strategy 1: Resolved baseRef ${baseRef} to SHA ${baseCommitSha}`);
 
         // Count commits to be included
-        const commitCount = parseInt(execGitSync(["rev-list", "--count", `${baseRef}..${branchName}`], { cwd }).trim(), 10);
-        debugLog(`Strategy 1: Found ${commitCount} commits between ${baseRef} and ${branchName}`);
+        const commitCount = parseInt(execGitSync(["rev-list", "--count", `${baseRef}..${tipRef}`], { cwd }).trim(), 10);
+        debugLog(`Strategy 1: Found ${commitCount} commits between ${baseRef} and ${tipRef}`);
 
         if (commitCount > 0) {
           // Generate patch from the determined base to the branch
-          const patchContent = execGitSync(["format-patch", `${baseRef}..${branchName}`, "--stdout", ...excludeArgs()], { cwd });
+          const patchContent = execGitSync(["format-patch", `${baseRef}..${tipRef}`, "--stdout", ...excludeArgs()], { cwd });
 
           if (patchContent && patchContent.trim()) {
             fs.writeFileSync(patchPath, patchContent, "utf8");
@@ -299,16 +312,25 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
         // error preserves the "incremental" contract that the patch reflects only
         // the new commits.
         if (!patchGenerated && mode === "incremental") {
-          debugLog(`Strategy 1 (incremental): format-patch produced no output for ${baseRef}..${branchName} despite ${commitCount} incremental commit(s), refusing to fall through to checkout-base strategies`);
+          debugLog(`Strategy 1 (incremental): format-patch produced no output for ${baseRef}..${tipRef} despite ${commitCount} incremental commit(s), refusing to fall through to checkout-base strategies`);
           return {
             success: false,
-            error: `Cannot generate incremental patch: git format-patch produced no output for ${baseRef}..${branchName} despite ${commitCount} incremental commit(s).`,
+            error: `Cannot generate incremental patch: git format-patch produced no output for ${baseRef}..${tipRef} despite ${commitCount} incremental commit(s).`,
             patchPath: patchPath,
           };
         }
       } catch (branchError) {
-        // Branch does not exist locally
+        // Branch does not exist locally (or pinnedSha failed)
         debugLog(`Strategy 1: Branch '${branchName}' does not exist locally - ${getErrorMessage(branchError)}`);
+        if (options.pinnedSha) {
+          // SECURITY: When pinnedSha is set, fail closed — do not fall through to
+          // other strategies that would resolve a different commit.
+          return {
+            success: false,
+            error: `Pinned SHA ${options.pinnedSha} failed to generate patch: ${getErrorMessage(branchError)}`,
+            patchPath: patchPath,
+          };
+        }
         if (mode === "incremental") {
           return {
             success: false,
@@ -510,7 +532,7 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
     // of the PR head. Use the merge-base as the effective diff base to exclude those
     // merged upstream commits from the size measurement.
     let diffBaseForSize = baseCommitSha;
-    if (mode === "incremental" && baseCommitSha && branchName && defaultBranch) {
+    if (mode === "incremental" && baseCommitSha && resolvedTipRef && defaultBranch) {
       try {
         let baseBranchRemoteRef = null;
         try {
@@ -527,15 +549,15 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
           // baseCommitSha as the diff base.
           let baseIsAncestorOfBranch = false;
           try {
-            execGitSync(["merge-base", "--is-ancestor", "--", baseCommitSha, branchName], { cwd });
+            execGitSync(["merge-base", "--is-ancestor", "--", baseCommitSha, resolvedTipRef], { cwd });
             baseIsAncestorOfBranch = true;
           } catch {
-            // baseCommitSha is not an ancestor of branchName (rebase / force-push)
-            debugLog(`Strategy 1 (incremental): baseCommitSha ${baseCommitSha} is not an ancestor of ${branchName} (rebase/force-push?); skipping merge-base adjustment`);
+            // baseCommitSha is not an ancestor of tipRef (rebase / force-push)
+            debugLog(`Strategy 1 (incremental): baseCommitSha ${baseCommitSha} is not an ancestor of ${resolvedTipRef} (rebase/force-push?); skipping merge-base adjustment`);
           }
 
           if (baseIsAncestorOfBranch) {
-            const mb = execGitSync(["merge-base", "--", baseBranchRemoteRef, branchName], { cwd }).trim();
+            const mb = execGitSync(["merge-base", "--", baseBranchRemoteRef, resolvedTipRef], { cwd }).trim();
             // Check if mb is already an ancestor of baseCommitSha.
             // If it is, baseCommitSha is "later" and the agent did NOT merge the default
             // branch ahead of the PR head — keep baseCommitSha as the diff base.
@@ -561,15 +583,15 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
     }
 
     let diffSize = null;
-    if (mode === "incremental" && diffBaseForSize && branchName) {
+    if (mode === "incremental" && diffBaseForSize && resolvedTipRef) {
       diffSize = computeIncrementalDiffSize({
         baseRef: diffBaseForSize,
-        headRef: branchName,
+        headRef: resolvedTipRef,
         cwd,
         tmpPath: `${patchPath}.diff.tmp`,
         excludedFiles: options.excludedFiles,
       });
-      debugLog(`Final: diffSize=${diffSize ?? "(n/a)"} bytes (baseRef=${diffBaseForSize}..${branchName})`);
+      debugLog(`Final: diffSize=${diffSize ?? "(n/a)"} bytes (baseRef=${diffBaseForSize}..${resolvedTipRef})`);
     }
 
     debugLog(`Final: SUCCESS - patchSize=${patchSize} bytes, patchLines=${patchLines}, diffSize=${diffSize ?? "(n/a)"} bytes, baseCommit=${baseCommitSha || "(unknown)"}`);

actions/setup/js/safe_output_type_validator.cjs

@@ -545,6 +545,14 @@ function validateItem(item, itemType, lineNum, options) {
   }
 
   const normalizedItem = { ...item };
+  // SECURITY: Strip infrastructure fields that must only be set by the MCP handler,
+  // never by the agent. If an agent injects these via NDJSON output, it could bypass
+  // file-protection policy (patch_path/bundle_path point to attacker-controlled files)
+  // or circumvent size limits (diff_size).
+  delete normalizedItem.patch_path;
+  delete normalizedItem.bundle_path;
+  delete normalizedItem.base_commit;
+  delete normalizedItem.diff_size;
   const errors = [];
 
   // Run custom validation first if defined

actions/setup/js/safe_output_type_validator.test.cjs

@@ -803,4 +803,111 @@ describe("safe_output_type_validator", () => {
       expect(result.error).toContain("must contain only strings");
     });
   });
+
+  describe("infrastructure field stripping (security)", () => {
+    it("should strip patch_path from normalizedItem", async () => {
+      const { validateItem } = await import("./safe_output_type_validator.cjs");
+
+      const item = {
+        type: "create_pull_request",
+        title: "Fix bug",
+        body: "Fixes the thing",
+        branch: "fix/bug",
+        patch_path: "/tmp/evil.patch",
+      };
+
+      const result = validateItem(item, "create_pull_request", 1);
+      expect(result.isValid).toBe(true);
+      expect(result.normalizedItem).not.toHaveProperty("patch_path");
+    });
+
+    it("should strip bundle_path from normalizedItem", async () => {
+      const { validateItem } = await import("./safe_output_type_validator.cjs");
+
+      const item = {
+        type: "create_pull_request",
+        title: "Fix bug",
+        body: "Fixes the thing",
+        branch: "fix/bug",
+        bundle_path: "/tmp/evil.bundle",
+      };
+
+      const result = validateItem(item, "create_pull_request", 1);
+      expect(result.isValid).toBe(true);
+      expect(result.normalizedItem).not.toHaveProperty("bundle_path");
+    });
+
+    it("should strip base_commit from normalizedItem", async () => {
+      const { validateItem } = await import("./safe_output_type_validator.cjs");
+
+      const item = {
+        type: "create_pull_request",
+        title: "Fix bug",
+        body: "Fixes the thing",
+        branch: "fix/bug",
+        base_commit: "abc123deadbeef",
+      };
+
+      const result = validateItem(item, "create_pull_request", 1);
+      expect(result.isValid).toBe(true);
+      expect(result.normalizedItem).not.toHaveProperty("base_commit");
+    });
+
+    it("should strip diff_size from normalizedItem", async () => {
+      const { validateItem } = await import("./safe_output_type_validator.cjs");
+
+      const item = {
+        type: "create_pull_request",
+        title: "Fix bug",
+        body: "Fixes the thing",
+        branch: "fix/bug",
+        diff_size: 1,
+      };
+
+      const result = validateItem(item, "create_pull_request", 1);
+      expect(result.isValid).toBe(true);
+      expect(result.normalizedItem).not.toHaveProperty("diff_size");
+    });
+
+    it("should strip all infrastructure fields simultaneously", async () => {
+      const { validateItem } = await import("./safe_output_type_validator.cjs");
+
+      const item = {
+        type: "create_pull_request",
+        title: "Fix bug",
+        body: "Fixes the thing",
+        branch: "fix/bug",
+        patch_path: "/tmp/evil.patch",
+        bundle_path: "/tmp/evil.bundle",
+        base_commit: "abc123",
+        diff_size: 999999,
+      };
+
+      const result = validateItem(item, "create_pull_request", 1);
+      expect(result.isValid).toBe(true);
+      expect(result.normalizedItem).not.toHaveProperty("patch_path");
+      expect(result.normalizedItem).not.toHaveProperty("bundle_path");
+      expect(result.normalizedItem).not.toHaveProperty("base_commit");
+      expect(result.normalizedItem).not.toHaveProperty("diff_size");
+      // Legitimate fields should still be present
+      expect(result.normalizedItem.title).toBe("Fix bug");
+      expect(result.normalizedItem.branch).toBe("fix/bug");
+    });
+
+    it("should preserve non-infrastructure undeclared fields", async () => {
+      const { validateItem } = await import("./safe_output_type_validator.cjs");
+
+      const item = {
+        type: "create_issue",
+        title: "Test",
+        body: "Body text",
+        metadata: { project: "test" },
+      };
+
+      const result = validateItem(item, "create_issue", 1);
+      expect(result.isValid).toBe(true);
+      // Non-infrastructure extra fields should pass through
+      expect(result.normalizedItem.metadata).toEqual({ project: "test" });
+    });
+  });
 });