Clear new locally created files before pushing a fix (#772)

Author: eranturgeman

scanrepository/scanrepository.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"github.com/go-git/go-git/v5"
 	biutils "github.com/jfrog/build-info-go/utils"
 	"os"
 	"path/filepath"
@@ -313,7 +314,7 @@ func (cfp *ScanRepositoryCmd) fixMultiplePackages(fullProjectPath string, vulner
 	return
 }
 
-// fixIssuesSinglePR fixes all the vulnerabilities in a single aggregated pull request.
+// Fixes all the vulnerabilities in a single aggregated pull request.
 // If an existing aggregated fix is present, it checks for different scan results.
 // If the scan results are the same, no action is taken.
 // Otherwise, it performs a force push to the same branch and reopens the pull request if it was closed.
@@ -398,6 +399,9 @@ func (cfp *ScanRepositoryCmd) openFixingPullRequest(repository *utils.Repository
 		return &utils.ErrNothingToCommit{PackageName: vulnDetails.ImpactedDependencyName}
 	}
 	commitMessage := cfp.gitManager.GenerateCommitMessage(vulnDetails.ImpactedDependencyName, vulnDetails.SuggestedFixedVersion)
+	if err = cfp.cleanNewFilesMissingInRemote(); err != nil {
+		log.Warn(fmt.Sprintf("failed fo clean untracked files from '%s' due to the following errors: %s", cfp.baseWd, err.Error()))
+	}
 	if err = cfp.gitManager.AddAllAndCommit(commitMessage); err != nil {
 		return
 	}
@@ -443,10 +447,13 @@ func (cfp *ScanRepositoryCmd) createOrUpdatePullRequest(repository *utils.Reposi
 	return pullRequestInfo, utils.DeletePullRequestComments(repository, cfp.scanDetails.Client(), int(pullRequestInfo.ID))
 }
 
-// openAggregatedPullRequest handles the opening or updating of a pull request when the aggregate mode is active.
+// Handles the opening or updating of a pull request when the aggregate mode is active.
 // If a pull request is already open, Frogbot will update the branch and the pull request body.
 func (cfp *ScanRepositoryCmd) openAggregatedPullRequest(repository *utils.Repository, fixBranchName string, pullRequestInfo *vcsclient.PullRequestInfo, vulnerabilities []*utils.VulnerabilityDetails) (err error) {
 	commitMessage := cfp.gitManager.GenerateAggregatedCommitMessage(cfp.projectTech)
+	if err = cfp.cleanNewFilesMissingInRemote(); err != nil {
+		return
+	}
 	if err = cfp.gitManager.AddAllAndCommit(commitMessage); err != nil {
 		return
 	}
@@ -456,6 +463,38 @@ func (cfp *ScanRepositoryCmd) openAggregatedPullRequest(repository *utils.Reposi
 	return cfp.handleFixPullRequestContent(repository, fixBranchName, pullRequestInfo, vulnerabilities...)
 }
 
+func (cfp *ScanRepositoryCmd) cleanNewFilesMissingInRemote() error {
+	// Open the local repository
+	localRepo, err := git.PlainOpen(cfp.baseWd)
+	if err != nil {
+		return err
+	}
+
+	// Getting the repository working tree
+	worktree, err := localRepo.Worktree()
+	if err != nil {
+		return err
+	}
+
+	// Getting the working tree status
+	gitStatus, err := worktree.Status()
+	if err != nil {
+		return err
+	}
+
+	for relativeFilePath, status := range gitStatus {
+		if status.Worktree == git.Untracked {
+			log.Debug(fmt.Sprintf("Untracking file '%s' that was created locally during the scan/fix process", relativeFilePath))
+			fileDeletionErr := os.Remove(filepath.Join(cfp.baseWd, relativeFilePath))
+			if fileDeletionErr != nil {
+				err = errors.Join(err, fmt.Errorf("file '%s': %s\n", relativeFilePath, fileDeletionErr.Error()))
+				continue
+			}
+		}
+	}
+	return err
+}
+
 func (cfp *ScanRepositoryCmd) preparePullRequestDetails(vulnerabilitiesDetails ...*utils.VulnerabilityDetails) (prTitle, prBody string, otherComments []string, err error) {
 	if cfp.dryRun && cfp.aggregateFixes {
 		// For testings, don't compare pull request body as scan results order may change.

scanrepository/scanrepository_test.go

@@ -3,13 +3,6 @@ package scanrepository
 import (
 	"errors"
 	"fmt"
-	"net/http/httptest"
-	"os"
-	"os/exec"
-	"path/filepath"
-	"strings"
-	"testing"
-
 	"github.com/google/go-github/v45/github"
 	biutils "github.com/jfrog/build-info-go/utils"
 	"github.com/jfrog/frogbot/v2/utils"
@@ -25,6 +18,12 @@ import (
 	"github.com/jfrog/jfrog-client-go/xray/services"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"net/http/httptest"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
 )
 
 const rootTestDir = "scanrepository"
@@ -85,6 +84,7 @@ func TestScanRepositoryCmd_Run(t *testing.T) {
 		configPath                     string
 		expectedPackagesInBranch       map[string][]string
 		expectedVersionUpdatesInBranch map[string][]string
+		expectedMissingFilesInBranch   map[string][]string
 		packageDescriptorPaths         []string
 		aggregateFixes                 bool
 		allowPartialResults            bool
@@ -100,6 +100,7 @@ func TestScanRepositoryCmd_Run(t *testing.T) {
 			testName:                       "aggregate-multi-dir",
 			expectedPackagesInBranch:       map[string][]string{"frogbot-update-npm-dependencies-master": {"uuid", "minimatch", "mpath", "minimist"}},
 			expectedVersionUpdatesInBranch: map[string][]string{"frogbot-update-npm-dependencies-master": {"^1.2.6", "^9.0.0", "^0.8.4", "^3.0.5"}},
+			expectedMissingFilesInBranch:   map[string][]string{"frogbot-update-npm-dependencies-master": {"npm1/package-lock.json", "npm2/package-lock.json"}},
 			packageDescriptorPaths:         []string{"npm1/package.json", "npm2/package.json"},
 			aggregateFixes:                 true,
 			configPath:                     "../testdata/scanrepository/cmd/aggregate-multi-dir/.frogbot/frogbot-config.yml",
@@ -108,6 +109,7 @@ func TestScanRepositoryCmd_Run(t *testing.T) {
 			testName:                       "aggregate-multi-project",
 			expectedPackagesInBranch:       map[string][]string{"frogbot-update-npm-dependencies-master": {"uuid", "minimatch", "mpath"}, "frogbot-update-Pip-dependencies-master": {"pyjwt", "pexpect"}},
 			expectedVersionUpdatesInBranch: map[string][]string{"frogbot-update-npm-dependencies-master": {"^9.0.0", "^0.8.4", "^3.0.5"}, "frogbot-update-Pip-dependencies-master": {"2.4.0"}},
+			expectedMissingFilesInBranch:   map[string][]string{"frogbot-update-npm-dependencies-master": {"npm/package-lock.json"}},
 			packageDescriptorPaths:         []string{"npm/package.json", "pip/requirements.txt"},
 			aggregateFixes:                 true,
 			configPath:                     "../testdata/scanrepository/cmd/aggregate-multi-project/.frogbot/frogbot-config.yml",
@@ -221,6 +223,14 @@ func TestScanRepositoryCmd_Run(t *testing.T) {
 					assert.Contains(t, string(resultDiff), updatedVersion)
 				}
 			}
+
+			if len(test.expectedMissingFilesInBranch) > 0 {
+				for branch, expectedMissingFiles := range test.expectedMissingFilesInBranch {
+					resultDiff, err := verifyLockFileDiff(branch, expectedMissingFiles...)
+					assert.NoError(t, err)
+					assert.Empty(t, resultDiff)
+				}
+			}
 		})
 	}
 }
@@ -669,6 +679,72 @@ func TestPreparePullRequestDetails(t *testing.T) {
 	assert.ElementsMatch(t, expectedExtraComments, extraComments)
 }
 
+// This test simulates the cleaning action of cleanNewFilesMissingInRemote.
+// Every file that has been newly CREATED after cloning the repo (here - after creating .git repo) should be removed. Every other file should be kept.
+func TestCleanNewFilesMissingInRemote(t *testing.T) {
+	testCases := []struct {
+		name                 string
+		relativeTestDirPath  string
+		createFileBeforeInit bool
+	}{
+		{
+			name:                 "new_file_should_remain",
+			relativeTestDirPath:  filepath.Join(rootTestDir, "cmd", "aggregate"),
+			createFileBeforeInit: true,
+		},
+		{
+			name:                 "new_file_should_be_deleted",
+			relativeTestDirPath:  filepath.Join(rootTestDir, "cmd", "aggregate"),
+			createFileBeforeInit: false,
+		},
+	}
+
+	baseDir, outerErr := os.Getwd()
+	assert.NoError(t, outerErr)
+	defer func() {
+		assert.NoError(t, os.Chdir(baseDir))
+	}()
+
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			testDir, cleanup := utils.CopyTestdataProjectsToTemp(t, test.relativeTestDirPath)
+			defer cleanup()
+
+			var file *os.File
+			if test.createFileBeforeInit {
+				var fileError error
+				file, fileError = os.CreateTemp(testDir, test.name)
+				assert.NoError(t, fileError)
+			}
+
+			utils.CreateDotGitWithCommit(t, testDir, "1234", "")
+
+			if !test.createFileBeforeInit {
+				var fileError error
+				file, fileError = os.CreateTemp(testDir, test.name)
+				assert.NoError(t, fileError)
+			}
+
+			// Making a change in the file so it will be modified in the working tree
+			_, err := file.WriteString("My initial string")
+			assert.NoError(t, err)
+			assert.NoError(t, file.Close())
+
+			scanRepoCmd := ScanRepositoryCmd{baseWd: testDir}
+			assert.NoError(t, scanRepoCmd.cleanNewFilesMissingInRemote())
+
+			exists, err := fileutils.IsFileExists(file.Name(), false)
+			assert.NoError(t, err)
+			if test.createFileBeforeInit {
+				assert.True(t, exists)
+			} else {
+				assert.False(t, exists)
+			}
+		})
+	}
+
+}
+
 func verifyTechnologyNaming(t *testing.T, scanResponse []services.ScanResponse, expectedType string) {
 	for _, resp := range scanResponse {
 		for _, vulnerability := range resp.Vulnerabilities {
@@ -698,3 +774,24 @@ func verifyDependencyFileDiff(baseBranch string, fixBranch string, packageDescri
 	}
 	return
 }
+
+func verifyLockFileDiff(branchToInspect string, lockFiles ...string) (output []byte, err error) {
+	log.Debug(fmt.Sprintf("Checking lock files differences in %s between branches 'master' and '%s'", lockFiles, branchToInspect))
+	// Suppress condition always false warning
+	//goland:noinspection ALL
+	var args []string
+	if coreutils.IsWindows() {
+		args = []string{"/c", "git", "ls-tree", branchToInspect, "--"}
+		args = append(args, lockFiles...)
+		output, err = exec.Command("cmd", args...).Output()
+	} else {
+		args = []string{"ls-tree", branchToInspect, "--"}
+		args = append(args, lockFiles...)
+		output, err = exec.Command("git", args...).Output()
+	}
+	var exitError *exec.ExitError
+	if errors.As(err, &exitError) {
+		err = errors.New("git error: " + string(exitError.Stderr))
+	}
+	return
+}

utils/git.go

@@ -466,6 +466,18 @@ func (gm *GitManager) dryRunClone(destination string) error {
 	return nil
 }
 
+func (gm *GitManager) GetRemoteGitUrl() string {
+	return gm.remoteGitUrl
+}
+
+func (gm *GitManager) GetAuth() *githttp.BasicAuth {
+	return gm.auth
+}
+
+func (gm *GitManager) GetRemoteName() string {
+	return gm.remoteName
+}
+
 func toBasicAuth(username, token string) *githttp.BasicAuth {
 	// The username can be anything except for an empty string
 	if username == "" {