Prevent PR body injection during transfer - #58034
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review locally; using noop because safe-output review submission tools are not exposed as callable developer tools in this environment.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
A tiny package-global seam adds indirection for a single call and makes the test path more stateful than necessary. Call workflow.RunGH directly and keep the test at the package boundary. net: -2 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58034 · codex · mai10 · 4.06 AIC · ⌖ 8.07 AIC · ⊞ 12.8K
Comment /ponytail to run again
| ) | ||
|
|
||
| var prLog = logger.New("cli:pr_command") | ||
| var prRunGH = workflow.RunGH |
There was a problem hiding this comment.
pkg/cli/pr_command.go:26: yagni: package-global prRunGH seam for one call. Call workflow.RunGH directly and keep the test at the package boundary.
There was a problem hiding this comment.
Addressed in 1b6dc96: removed the package-level prRunGH seam and the test now exercises workflow.RunGH through a fake gh binary at the package boundary.
There was a problem hiding this comment.
Reviewed the git mailbox injection fix. The change correctly removes the hand-built mailbox format and replaces it with a raw gh pr diff write plus a separately-constructed git commit -m invocation via exec.Command (argv, not shell), which prevents untrusted PR body/title content from being interpreted as patch or mailbox structure. The regression test confirms a --- separator plus injected diff content in the PR body no longer leaks into the patch file. No blocking issues found in the changed lines.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 26.3 AIC · ⌖ 13.2 AIC · ⊞ 8.3K
There was a problem hiding this comment.
🟡 Changes recommended
Broad staging and unsafe retry cleanup could leak unrelated files or leave the checkout conflicted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents PR-body injection during transfer by separating patch application from commit metadata.
Changes:
- Applies raw
gh pr diffoutput withgit apply. - Commits PR metadata separately.
- Adds regression coverage for mailbox separators.
File summaries
| File | Review |
|---|---|
pkg/cli/pr_command.go |
Critical (2 votes): git add . may stage unrelated local files; scope staging and add regression coverage. Moderate (1 vote): reset conflicted state before retries and failure cleanup. |
pkg/cli/pr_command_test.go |
Verifies patch files contain only raw PR diffs. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| } else if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Applied patch using git am (includes commit)")) | ||
| // Stage all changes and create the commit separately from patch application. | ||
| cmd = exec.Command("git", "add", ".") |
There was a problem hiding this comment.
Addressed in 1b6dc96: transfer now rejects a dirty target worktree, applies patches with git apply --index, removes the broad git add ., and adds a regression proving unrelated ignored files are not committed.
| cmd = exec.Command("git", "apply", "--3way", patchFile) | ||
| if err := cmd.Run(); err != nil { |
There was a problem hiding this comment.
Addressed in 1b6dc96: failed 3-way applies are reset to HEAD before fallback, and failed fallback/reject diagnostics reset before switching back and deleting the transfer branch.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — this is a solid, well-scoped fix for the mailbox-injection vulnerability (CVE-worthy: untrusted PR body text was previously spliced into a git am mailbox and interpreted as patch/header structure). The core fix is correct: writing only the raw diff to the patch file, always using git apply instead of git am, and building the commit message via git commit -m (an argv, not shell/patch text) means untrusted Body/Title content can no longer be parsed as patch structure.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed, not just symptom: removing
git am/mailbox parsing entirely (rather than just sanitizing the body) eliminates the injection vector at its source — good root-cause fix per/diagnosing-bugs. - Test coverage gap: the new
TestCreatePatchFromPRWritesOnlyDifftest only covers the patch-file-writing half (createPatchFromPR). The commit-message-building half inapplyPatchToRepo, whereprInfo.Body/prInfo.Titleare interpolated intogit commit -m, remains untested by this PR. Since-mmessage text isn't parsed by git as patch structure, this is low risk, but a regression test would make the security property fully verifiable in CI rather than only partially. - Dependency injection pattern (
prRunGH) is a nice, minimal seam for testability — consistent with good/codebase-designpractice of adding a small seam rather than a larger interface.
Positive Highlights
- ✅ Clean removal of ~100 lines of fallback/mailbox-detection logic — net negative diff, simpler control flow, easier to reason about.
- ✅ Commit message construction is unchanged in content (Original-PR/Original-Author trailers preserved) while eliminating the injection surface — good backward compatibility.
- ✅ Regression test explicitly encodes the vulnerability scenario (
Bodycontaining---\ndiff --git ...), which is exactly the right kind of test per/tdd.
@copilot please address the review comments above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 76.7 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
| if string(got) != string(diff) { | ||
| t.Fatalf("patch contents = %q, want raw diff %q", got, diff) | ||
| } | ||
| } |
There was a problem hiding this comment.
[/tdd] Good regression test for createPatchFromPR, but the applyPatchToRepo commit-building path (the other half of this fix — where prInfo.Body/prInfo.Title now flow into git commit -m) has no test coverage in this PR.
💡 Suggested addition
A test that stubs the git commands (or runs against a temp git repo) and asserts a malicious Body such as "msg\n---\ndiff --git a/injected b/injected" ends up verbatim inside the commit message rather than being reinterpreted, would close the loop on the vulnerability end-to-end (patch application and commit creation), not just the patch-file write.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 1b6dc96: added coverage that runs the commit path in a temp git repo and verifies a malicious body containing --- and diff text remains verbatim in the commit message instead of creating injected files.
|
@copilot Please address the current blocking review feedback, refresh the branch if needed, and run the
Please summarize the fix and rerun outcome for maintainers after the next push.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please review the latest head and address the remaining blocking feedback before handing back to maintainers.
Please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
|
@copilot add git integration tests |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed at latest head |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added in |
|
🎉 This pull request is included in a new release. Release: |
PR bodies were embedded in a hand-built git mailbox, allowing a malicious
---line to inject patch content duringgh aw pr transfer.Patch handling
gh pr diffoutput to the patch file.git ammailbox parsing and always usegit apply.Commit metadata
git commit -m.