Skip to content

Commit 86c0e5a

Browse files
Copilotpelikhangh-aw-bot
authored
Prevent PR body injection during transfer (#58034)
* Initial plan * Plan PR patch injection fix Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Fix PR mailbox patch injection Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Remove generated progress artifacts Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Restore generated skill state Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Plan review feedback fix Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> * Harden PR transfer patch application Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> * Restore fallback workflow file list Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> * Cover failed PR transfer cleanup Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> * Add PR transfer git integration tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com>
1 parent 37750b6 commit 86c0e5a

3 files changed

Lines changed: 503 additions & 126 deletions

File tree

‎pkg/cli/pr_command.go‎

Lines changed: 118 additions & 126 deletions
Original file line numberDiff line numberDiff line change
@@ -236,29 +236,9 @@ func createPatchFromPR(sourceOwner, sourceRepo string, prInfo *PRInfo, verbose b
236236
return "", errors.New("PR diff is empty")
237237
}
238238

239-
// Create proper mailbox format patch that git am expects
240-
var patchBuilder strings.Builder
241-
242-
// Required mailbox format headers for git am
243-
fmt.Fprintf(&patchBuilder, "From %s Mon Sep 17 00:00:00 2001\n", prInfo.HeadSHA)
244-
fmt.Fprintf(&patchBuilder, "From: %s <%s@users.noreply.github.com>\n", prInfo.AuthorLogin, prInfo.AuthorLogin)
245-
fmt.Fprintf(&patchBuilder, "Date: %s\n", time.Now().Format(time.RFC1123))
246-
fmt.Fprintf(&patchBuilder, "Subject: [PATCH] %s\n", prInfo.Title)
247-
patchBuilder.WriteString("\n")
248-
249-
if prInfo.Body != "" {
250-
fmt.Fprintf(&patchBuilder, "%s\n", prInfo.Body)
251-
patchBuilder.WriteString("\n")
252-
}
253-
254-
fmt.Fprintf(&patchBuilder, "Original-PR: %s#%d\n", prInfo.SourceRepo, prInfo.Number)
255-
fmt.Fprintf(&patchBuilder, "Original-Author: %s\n", prInfo.AuthorLogin)
256-
patchBuilder.WriteString("---\n")
257-
258-
// Add the actual diff content
259-
patchBuilder.Write(diffContent)
260-
261-
if err := os.WriteFile(patchFile, []byte(patchBuilder.String()), constants.FilePermPublic); err != nil {
239+
// Keep the patch as a raw diff. PR metadata is added to the commit separately
240+
// so untrusted body text cannot be interpreted as mailbox or patch content.
241+
if err := os.WriteFile(patchFile, diffContent, constants.FilePermPublic); err != nil {
262242
return "", fmt.Errorf("could not write patch file; ensure required prerequisites are configured, then retry: %w", err)
263243
}
264244

@@ -267,18 +247,34 @@ func createPatchFromPR(sourceOwner, sourceRepo string, prInfo *PRInfo, verbose b
267247
}
268248

269249
return patchFile, nil
270-
} // applyPatchToRepo applies a patch to the target repository and returns the branch name
271-
func applyPatchToRepo(patchFile string, prInfo *PRInfo, targetOwner, targetRepo string, verbose bool) (string, error) {
272-
// Get current branch to restore later
273-
currentBranch, err := getCurrentBranch()
250+
}
251+
252+
func ensureCleanGitWorktree() error {
253+
output, err := exec.Command("git", "status", "--porcelain").Output()
274254
if err != nil {
275-
return "", fmt.Errorf("could not get current branch; ensure required prerequisites are configured, then retry: %w", err)
255+
return fmt.Errorf("could not inspect git status; ensure this is a valid git repository, then retry: %w", err)
256+
}
257+
if strings.TrimSpace(string(output)) != "" {
258+
return errors.New("target repository has uncommitted changes; commit, stash, or remove them before transferring a PR")
276259
}
260+
return nil
261+
}
277262

263+
func resetGitWorktreeToHEAD() error {
264+
if err := exec.Command("git", "reset", "--hard", "HEAD").Run(); err != nil {
265+
return fmt.Errorf("could not reset transfer branch to HEAD: %w", err)
266+
}
267+
if err := exec.Command("git", "clean", "-fd").Run(); err != nil {
268+
return fmt.Errorf("could not remove untracked files from failed patch apply: %w", err)
269+
}
270+
return nil
271+
}
272+
273+
func checkoutUpdatedDefaultBranch(targetOwner, targetRepo string, verbose bool) error {
278274
// Get the default branch of the target repository
279275
defaultBranchOutput, err := workflow.RunGH("Fetching default branch...", "api", fmt.Sprintf("/repos/%s/%s", targetOwner, targetRepo), "--jq", ".default_branch")
280276
if err != nil {
281-
return "", fmt.Errorf("could not get default branch; ensure required prerequisites are configured, then retry: %w", err)
277+
return fmt.Errorf("could not get default branch; ensure required prerequisites are configured, then retry: %w", err)
282278
}
283279
defaultBranch := strings.TrimSpace(string(defaultBranchOutput))
284280

@@ -289,133 +285,129 @@ func applyPatchToRepo(patchFile string, prInfo *PRInfo, targetOwner, targetRepo
289285

290286
cmd := exec.Command("git", "checkout", defaultBranch)
291287
if err := cmd.Run(); err != nil {
292-
return "", fmt.Errorf("could not checkout default branch %s; ensure the branch exists locally and has no conflicting changes, then retry: %w", defaultBranch, err)
288+
return fmt.Errorf("could not checkout default branch %s; ensure the branch exists locally and has no conflicting changes, then retry: %w", defaultBranch, err)
293289
}
294290

295291
cmd = exec.Command("git", "pull", "origin", defaultBranch)
296292
if err := cmd.Run(); err != nil {
297-
return "", fmt.Errorf("could not pull latest %s from origin; ensure network access and remote permissions are available, then retry: %w", defaultBranch, err)
293+
return fmt.Errorf("could not pull latest %s from origin; ensure network access and remote permissions are available, then retry: %w", defaultBranch, err)
298294
}
295+
return nil
296+
}
299297

300-
// Create a new branch for the transfer based on the updated default branch
301-
branchName := fmt.Sprintf("transfer-pr-%d-%d", prInfo.Number, time.Now().Unix())
298+
func applyPatchToIndexWithFallback(patchFile, currentBranch, branchName string, verbose bool) error {
302299
if verbose {
303-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Creating branch: "+branchName))
300+
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Applying patch with git apply..."))
304301
}
305302

306-
if err := createAndSwitchBranch(branchName, verbose); err != nil {
307-
return "", fmt.Errorf("could not create new branch; ensure required prerequisites are configured, then retry: %w", err)
303+
cmd := exec.Command("git", "apply", "--3way", "--index", patchFile)
304+
if err := cmd.Run(); err != nil {
305+
return applyPatchToIndexFallback(patchFile, currentBranch, branchName, verbose)
308306
}
307+
return nil
308+
}
309309

310-
// Apply the patch
310+
func applyPatchToIndexFallback(patchFile, currentBranch, branchName string, verbose bool) error {
311311
if verbose {
312-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Applying patch..."))
313-
314-
// Show some info about the patch file
315-
patchContent, err := os.ReadFile(patchFile)
316-
if err == nil {
317-
lines := strings.Split(string(patchContent), "\n")
318-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Patch file has %d lines", len(lines))))
319-
if len(lines) > 0 {
320-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("First line: "+lines[0]))
321-
}
322-
}
312+
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("3-way merge failed, trying with whitespace options..."))
323313
}
324-
325-
// Check if patch looks like a mailbox format (starts with "From ")
326-
patchContent, err := os.ReadFile(patchFile)
327-
if err != nil {
328-
return "", fmt.Errorf("could not read patch file; ensure required prerequisites are configured, then retry: %w", err)
314+
if resetErr := resetGitWorktreeToHEAD(); resetErr != nil {
315+
_ = exec.Command("git", "checkout", currentBranch).Run()
316+
_ = exec.Command("git", "branch", "-D", branchName).Run()
317+
return resetErr
329318
}
330319

331-
var appliedWithAm bool
332-
isMailboxFormat := strings.HasPrefix(string(patchContent), "From ")
333-
334-
if isMailboxFormat {
335-
// Try git am for mailbox format patches
336-
if verbose {
337-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Applying mailbox format patch with git am..."))
338-
}
339-
340-
cmd = exec.Command("git", "am", patchFile)
341-
if err := cmd.Run(); err == nil {
342-
appliedWithAm = true
343-
if verbose {
344-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Successfully applied patch with git am"))
345-
}
346-
} else {
347-
if verbose {
348-
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("git am failed, trying git apply..."))
349-
}
350-
// Reset any partial am state
351-
_ = exec.Command("git", "am", "--abort").Run()
320+
cmd := exec.Command("git", "apply", "--index", "--ignore-space-change", "--ignore-whitespace", patchFile)
321+
if err := cmd.Run(); err != nil {
322+
reportPatchRejectDetails(patchFile, verbose)
323+
if resetErr := resetGitWorktreeToHEAD(); resetErr != nil {
324+
return resetErr
352325
}
326+
_ = exec.Command("git", "checkout", currentBranch).Run()
327+
_ = exec.Command("git", "branch", "-D", branchName).Run()
328+
return fmt.Errorf("could not apply patch; resolve conflicts manually, then rerun transfer-pr. underlying error: %w", err)
353329
}
330+
return nil
331+
}
354332

355-
if !appliedWithAm {
356-
// Try git apply for standard diff format or as fallback
357-
if verbose {
358-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Applying patch with git apply..."))
359-
}
333+
func reportPatchRejectDetails(patchFile string, verbose bool) {
334+
if !verbose {
335+
return
336+
}
337+
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Standard apply failed, trying with --reject to see what failed..."))
338+
if resetErr := resetGitWorktreeToHEAD(); resetErr != nil {
339+
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to reset before generating reject details: %v", resetErr)))
340+
}
341+
rejectCmd := exec.Command("git", "apply", "--reject", patchFile)
342+
rejectOutput, _ := rejectCmd.CombinedOutput()
343+
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Patch rejection details:"))
344+
fmt.Fprintln(os.Stderr, string(rejectOutput))
345+
}
360346

361-
cmd = exec.Command("git", "apply", "--3way", patchFile)
362-
if err := cmd.Run(); err != nil {
363-
if verbose {
364-
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("3-way merge failed, trying with whitespace options..."))
365-
}
347+
func logPatchSummary(patchFile string, verbose bool) {
348+
if !verbose {
349+
return
350+
}
351+
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Applying patch..."))
352+
patchContent, err := os.ReadFile(patchFile)
353+
if err != nil {
354+
return
355+
}
356+
lines := strings.Split(string(patchContent), "\n")
357+
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Patch file has %d lines", len(lines))))
358+
if len(lines) > 0 {
359+
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("First line: "+lines[0]))
360+
}
361+
}
366362

367-
// Try with --ignore-space-change and --ignore-whitespace
368-
cmd = exec.Command("git", "apply", "--ignore-space-change", "--ignore-whitespace", patchFile)
369-
if err := cmd.Run(); err != nil {
370-
if verbose {
371-
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Standard apply failed, trying with --reject to see what failed..."))
363+
// applyPatchToRepo applies a patch to the target repository and returns the branch name
364+
func applyPatchToRepo(patchFile string, prInfo *PRInfo, targetOwner, targetRepo string, verbose bool) (string, error) {
365+
currentBranch, err := getCurrentBranch()
366+
if err != nil {
367+
return "", fmt.Errorf("could not get current branch; ensure required prerequisites are configured, then retry: %w", err)
368+
}
369+
if err := ensureCleanGitWorktree(); err != nil {
370+
return "", err
371+
}
372+
if err := checkoutUpdatedDefaultBranch(targetOwner, targetRepo, verbose); err != nil {
373+
return "", err
374+
}
372375

373-
// Try with --reject to see which parts fail
374-
rejectCmd := exec.Command("git", "apply", "--reject", patchFile)
375-
rejectOutput, _ := rejectCmd.CombinedOutput()
376-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Patch rejection details:"))
377-
fmt.Fprintln(os.Stderr, string(rejectOutput))
378-
}
376+
branchName := fmt.Sprintf("transfer-pr-%d-%d", prInfo.Number, time.Now().Unix())
377+
if verbose {
378+
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Creating branch: "+branchName))
379+
}
380+
if err := createAndSwitchBranch(branchName, verbose); err != nil {
381+
return "", fmt.Errorf("could not create new branch; ensure required prerequisites are configured, then retry: %w", err)
382+
}
379383

380-
// Try to reset back to original branch and clean up
381-
_ = exec.Command("git", "checkout", currentBranch).Run()
382-
_ = exec.Command("git", "branch", "-D", branchName).Run()
383-
return "", fmt.Errorf("could not apply patch; resolve conflicts manually, then rerun transfer-pr. underlying error: %w", err)
384-
}
385-
}
384+
logPatchSummary(patchFile, verbose)
385+
if err := applyPatchToIndexWithFallback(patchFile, currentBranch, branchName, verbose); err != nil {
386+
return "", err
387+
}
386388

387-
if verbose {
388-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Successfully applied patch with git apply"))
389-
}
390-
} // If we didn't use git am, we need to stage and commit manually
391-
if !appliedWithAm {
392-
// Stage all changes
393-
cmd = exec.Command("git", "add", ".")
394-
if err := cmd.Run(); err != nil {
395-
return "", fmt.Errorf("could not stage changes; ensure required prerequisites are configured, then retry: %w", err)
396-
}
389+
if verbose {
390+
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Successfully applied patch with git apply"))
391+
}
397392

398-
// Create commit with meaningful message
399-
commitMsg := fmt.Sprintf("Transfer PR #%d from %s\n\n%s", prInfo.Number, prInfo.SourceRepo, prInfo.Title)
400-
if prInfo.Body != "" {
401-
commitMsg += "\n\n" + prInfo.Body
402-
}
403-
commitMsg += fmt.Sprintf("\n\nOriginal-PR: %s#%d", prInfo.SourceRepo, prInfo.Number)
404-
commitMsg += "\nOriginal-Author: " + prInfo.AuthorLogin
393+
// Create the commit separately from patch application.
394+
commitMsg := fmt.Sprintf("Transfer PR #%d from %s\n\n%s", prInfo.Number, prInfo.SourceRepo, prInfo.Title)
395+
if prInfo.Body != "" {
396+
commitMsg += "\n\n" + prInfo.Body
397+
}
398+
commitMsg += fmt.Sprintf("\n\nOriginal-PR: %s#%d", prInfo.SourceRepo, prInfo.Number)
399+
commitMsg += "\nOriginal-Author: " + prInfo.AuthorLogin
405400

406-
cmd = exec.Command("git", "commit", "-m", commitMsg)
407-
if err := cmd.Run(); err != nil {
408-
return "", fmt.Errorf("could not commit changes; ensure required prerequisites are configured, then retry: %w", err)
409-
}
410-
} else if verbose {
411-
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Applied patch using git am (includes commit)"))
401+
cmd := exec.Command("git", "commit", "-m", commitMsg)
402+
if err := cmd.Run(); err != nil {
403+
return "", fmt.Errorf("could not commit changes; ensure required prerequisites are configured, then retry: %w", err)
412404
}
413405

414406
return branchName, nil
415407
}
416408

417409
// createTransferPR creates a new PR in the target repository
418-
func createTransferPR(targetOwner, targetRepo string, prInfo *PRInfo, branchName string, verbose bool) error {
410+
func createTransferPR(targetOwner, targetRepo string, prInfo *PRInfo, branchName string, verbose bool) error { //nolint:largefunc
419411
// Check if user has write access to target repository
420412
hasWriteAccess, err := checkRepositoryAccess(targetOwner, targetRepo)
421413
if err != nil && verbose {
@@ -544,7 +536,7 @@ func createTransferPR(targetOwner, targetRepo string, prInfo *PRInfo, branchName
544536
}
545537

546538
// transferPR is the main function that orchestrates the PR transfer
547-
func transferPR(prURL, targetRepo string, verbose bool) error {
539+
func transferPR(prURL, targetRepo string, verbose bool) error { //nolint:largefunc
548540
prLog.Printf("Starting PR transfer: url=%s, targetRepo=%s", prURL, targetRepo)
549541

550542
if verbose {

0 commit comments

Comments
 (0)