fix: Prevent patch/bundle desynchronization in safe-outputs#36762
Conversation
…540)
Three security fixes to prevent agents from bypassing file-protection
policy through transport artifact manipulation:
1. Strip infrastructure fields in validateItem():
- Delete patch_path, bundle_path, base_commit, diff_size from
normalizedItem in the validation engine
- Push normalizedItem (not original item) in collect_ndjson_output.cjs
- Prevents agents from injecting arbitrary file paths or size values
through NDJSON output
2. Pin branch SHA in MCP handlers:
- Resolve refs/heads/<branch> to a SHA once before generating artifacts
- Pass pinnedSha to generateGitPatch so it uses the pinned commit
directly (skipping show-ref verification)
- Fail closed if pinnedSha generation encounters errors
3. TOCTOU verification after bundle generation:
- After generating both patch and bundle, verify the branch ref still
points to the pinned SHA
- If the agent flipped the ref between operations, abort with an error
- Applied to both create_pull_request and push_to_pull_request_branch
Includes 6 regression tests verifying infrastructure field stripping.
Refs: github/agentic-workflows#540
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ smoke-ci: safeoutputs CLI comment + comment-memory run (26921046033)
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the safe-outputs pipeline against transport artifact manipulation by (1) stripping infrastructure-only fields during validation/ingestion, and (2) reducing patch/bundle desynchronization risk by pinning branch refs to a SHA and adding a post-generation TOCTOU check in the privileged handlers.
Changes:
- Strip
patch_path,bundle_path,base_commit, anddiff_sizefromvalidateItem()normalized output and ensure ingestion usesnormalizedItem. - Add branch SHA pinning + TOCTOU verification to
create_pull_requestandpush_to_pull_request_branchhandlers. - Add regression tests covering infrastructure field stripping.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/safe_outputs_handlers.cjs | Pins branch SHA before artifact generation and verifies ref stability after bundle generation. |
| actions/setup/js/safe_output_type_validator.cjs | Deletes infrastructure-only fields from normalizedItem during validation. |
| actions/setup/js/collect_ndjson_output.cjs | Ensures ingestion pushes normalizedItem (with stripped infra fields) rather than the raw item. |
| actions/setup/js/generate_git_patch.cjs | Adds pinnedSha support to avoid dereferencing mutable refs during patch generation. |
| actions/setup/js/safe_output_type_validator.test.cjs | Adds tests asserting infra field stripping behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
| // 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 = options.pinnedSha || branchName; |
| } catch (pinError) { | ||
| server.debug(`Failed to pin branch '${entry.branch}': ${getErrorMessage(pinError)}`); | ||
| // If we can't resolve the branch, let the downstream functions handle the error | ||
| pinnedSha = null; | ||
| } |
| } catch (pinError) { | ||
| server.debug(`Failed to pin branch '${entry.branch}': ${getErrorMessage(pinError)}`); | ||
| pushPinnedSha = null; | ||
| } |
| // SECURITY: Use normalizedItem (which only contains declared schema | ||
| // fields) instead of the original item, to prevent agent-injected | ||
| // infrastructure fields (patch_path, bundle_path, base_commit, diff_size) | ||
| // from reaching the privileged handler. |
|
|
|
@copilot address review feedback |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed. I pushed follow-up fixes in commits
|
|
@lpcox you need to create draft PRs as the draft -> ready triggers PR reviewers. |
|
@copilot address the feedback #36762 (comment) |
|
🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨ |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ Testing safeoutputs CLI |
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
✅ All tools validated successfully! Agent Container Smoke Test confirms agent container is ready. |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
The linked feedback is about PR lifecycle (opening as draft, then marking ready), not code in this branch. There’s no repository code change to apply for this request. I can’t change PR draft/ready state from here, so this should be handled when creating/updating the PR. |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Smoke Test Results\n- GitHub MCP Testing: ✅\n- Web Fetch Testing: ✅\n- File Writing Testing: ✅\n- Bash Tool Testing: ✅\n- Build gh-aw: ❌\n\nOverall Status: FAILWarning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke test summary for PR #36762
Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
🧪 Smoke Test: Claude — Run 26927666920Core #1–#13: ✅ except #2 Overall: PARTIAL — run Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · opus48 5.3M
| // 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; |
There was a problem hiding this comment.
Good defensive move stripping patch_path here — this prevents agent-injected infrastructure fields from reaching the handler. 👍
| delete normalizedItem.patch_path; | ||
| delete normalizedItem.bundle_path; | ||
| delete normalizedItem.base_commit; | ||
| delete normalizedItem.diff_size; |
There was a problem hiding this comment.
Consider centralizing this list of stripped fields (patch_path, bundle_path, base_commit, diff_size) into a named constant for reuse and easier auditing.
|
PR: fix: Prevent patch/bundle desynchronization in safe-outputs Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
Summary
Prevents patch/bundle desynchronization in safe-outputs by pinning the branch ref to a SHA before transport artifact generation and verifying it hasn't changed after bundle creation (closes #540). Also fixes agent-injected metadata from leaking into privileged handlers by normalizing items before dispatch.
Changes
Security / Bug Fixes
actions/setup/js/safe_outputs_handlers.cjs— Added TOCTOU protection: pins the branch ref to a SHA before transport artifact generation and verifies the ref hasn't changed after bundle creation; aborts with a clear error if the ref was flipped concurrently. This closes the patch/bundle desynchronization window described in #540.actions/setup/js/collect_ndjson_output.cjs— Fixed item processing to usenormalizedItem(which strips infrastructure-only transport fields) instead of the raw item for known types, preventing agent-injected metadata from reaching privileged handlers.Tests
actions/setup/js/safe_outputs_handlers.test.cjs— Updated test assertions to match the new"Failed to pin branch"and"Bundle transport requires branch pinning"error messages, replacing the previous patch-generation error messages.Documentation / Tooling
.github/skills/agentic-workflows/SKILL.md— Addedagentic-workflows-mcp.mdandmcp-clis.mdto the skill's file-loading list.Impact
Related
a233d7860)