fixed pr issues, fixed tests and fixed readme

README.md

@@ -242,7 +242,6 @@ When Frogbot detects secrets that have been inadvertently exposed within the cod
 
 ### Automatic pull requests creation
 Frogbot scans your Git repositories periodically and automatically creates pull requests for upgrading vulnerable dependencies to a version with a fix.
-> **_NOTE:_** Currently not supported in Gradle.
 
 ![](./images/fix-pr.png)
 

packagehandlers/gradlepackagehandler.go

@@ -3,7 +3,6 @@ package packagehandlers
 import (
 	"fmt"
 	"github.com/jfrog/frogbot/utils"
-	"github.com/jfrog/jfrog-client-go/utils/errorutils"
 	"io/fs"
 	"os"
 	"path/filepath"
@@ -12,8 +11,8 @@ import (
 )
 
 const (
-	groovyBuildFileSuffix         = "build.gradle"
-	kotlinBuildFileSuffix         = "build.gradle.kts"
+	groovyDescriptorFileSuffix    = "build.gradle"
+	kotlinDescriptorFileSuffix    = "build.gradle.kts"
 	apostrophes                   = "[\\\"|\\']"
 	directMapRegexpEntry          = "\\s*%s\\s*[:|=]\\s*"
 	directStringWithVersionFormat = "%s:%s:%s"
@@ -24,10 +23,7 @@ var directMapWithVersionRegexp string
 func init() {
 	// Initializing a regexp pattern for map dependencies
 	// Example: group: "junit", name: "junit", version: "1.0.0" | group = "junit", name = "junit", version = "1.0.0"
-	groupEntry := getMapRegexpEntry("group")
-	nameEntry := getMapRegexpEntry("name")
-	versionEntry := getMapRegexpEntry("version")
-	directMapWithVersionRegexp = groupEntry + "," + nameEntry + "," + versionEntry
+	directMapWithVersionRegexp = getMapRegexpEntry("group") + "," + getMapRegexpEntry("name") + "," + getMapRegexpEntry("version")
 }
 
 type GradlePackageHandler struct {
@@ -62,11 +58,19 @@ func (gph *GradlePackageHandler) updateDirectDependency(vulnDetails *utils.Vulne
 		return
 	}
 
+	isAnyDescriptorFileChanged := false
 	for _, descriptorFilePath := range descriptorFilesPaths {
-		err = fixVulnerabilityIfExists(descriptorFilePath, vulnDetails)
+		var isFileChanged bool
+		isFileChanged, err = fixVulnerabilityIfExists(descriptorFilePath, vulnDetails)
 		if err != nil {
 			return
 		}
+		// We use logical OR to save information over all descriptor files whether there is at least one file that has been changed
+		isAnyDescriptorFileChanged = isAnyDescriptorFileChanged || isFileChanged
+	}
+
+	if !isAnyDescriptorFileChanged {
+		err = fmt.Errorf("impacted package '%s' was not found or could not be fixed in all descriptor files", vulnDetails.ImpactedDependencyName)
 	}
 	return
 }
@@ -83,13 +87,14 @@ func isVersionSupportedForFix(impactedVersion string) bool {
 
 // Collects all descriptor files absolute paths
 func getDescriptorFilesPaths() (descriptorFilesPaths []string, err error) {
-	err = filepath.WalkDir(".", func(path string, d fs.DirEntry, err error) error {
-		if err != nil {
+	err = filepath.WalkDir(".", func(path string, d fs.DirEntry, innerErr error) error {
+		if innerErr != nil {
 			return fmt.Errorf("error has occured when trying to access or traverse the files system: %s", err.Error())
 		}
 
-		if d.Type().IsRegular() && (strings.HasSuffix(path, groovyBuildFileSuffix) || strings.HasSuffix(path, kotlinBuildFileSuffix)) {
-			absFilePath, innerErr := filepath.Abs(path)
+		if strings.HasSuffix(path, groovyDescriptorFileSuffix) || strings.HasSuffix(path, kotlinDescriptorFileSuffix) {
+			var absFilePath string
+			absFilePath, innerErr = filepath.Abs(path)
 			if innerErr != nil {
 				return fmt.Errorf("couldn't retrieve file's absolute path for './%s':%s", path, innerErr.Error())
 			}
@@ -100,26 +105,29 @@ func getDescriptorFilesPaths() (descriptorFilesPaths []string, err error) {
 	return
 }
 
-// Fixes all direct occurrences (string/map) of the given vulnerability in the given descriptor file if vulnerability occurs
-func fixVulnerabilityIfExists(descriptorFilePath string, vulnDetails *utils.VulnerabilityDetails) (err error) {
+// Fixes all direct occurrences of the given vulnerability in the given descriptor file, if vulnerability occurs
+func fixVulnerabilityIfExists(descriptorFilePath string, vulnDetails *utils.VulnerabilityDetails) (isFileChanged bool, err error) {
+	isFileChanged = false
+
 	byteFileContent, err := os.ReadFile(descriptorFilePath)
 	if err != nil {
 		err = fmt.Errorf("couldn't read file '%s': %s", descriptorFilePath, err.Error())
 		return
 	}
 	fileContent := string(byteFileContent)
+	originalFile := fileContent
 
 	depGroup, depName, err := getVulnerabilityGroupAndName(vulnDetails.ImpactedDependencyName)
 	if err != nil {
 		return
 	}
 
-	// Fixing all vulnerable rows in string format
+	// Fixing all vulnerable rows given in a string format. For Example: implementation "junit:junit:4.7"
 	directStringVulnerableRow := fmt.Sprintf(directStringWithVersionFormat, depGroup, depName, vulnDetails.ImpactedDependencyVersion)
 	directStringFixedRow := fmt.Sprintf(directStringWithVersionFormat, depGroup, depName, vulnDetails.SuggestedFixedVersion)
 	fileContent = strings.ReplaceAll(fileContent, directStringVulnerableRow, directStringFixedRow)
 
-	// Fixing all vulnerable rows in a map format
+	// Fixing all vulnerable rows given in a map format. For Example: implementation group: "junit", name: "junit", version: "4.7"
 	mapRegexpForVulnerability := fmt.Sprintf(directMapWithVersionRegexp, depGroup, depName, vulnDetails.ImpactedDependencyVersion)
 	regexpCompiler := regexp.MustCompile(mapRegexpForVulnerability)
 	if rowsMatches := regexpCompiler.FindAllString(fileContent, -1); rowsMatches != nil {
@@ -129,6 +137,12 @@ func fixVulnerabilityIfExists(descriptorFilePath string, vulnDetails *utils.Vuln
 		}
 	}
 
+	// If there is no changes in the file we finish dealing with the current descriptor file
+	if fileContent == originalFile {
+		return
+	}
+	isFileChanged = true
+
 	err = writeUpdatedBuildFile(descriptorFilePath, fileContent)
 	return
 }
@@ -137,7 +151,7 @@ func fixVulnerabilityIfExists(descriptorFilePath string, vulnDetails *utils.Vuln
 func getVulnerabilityGroupAndName(impactedDependencyName string) (depGroup string, depName string, err error) {
 	seperatedImpactedDepName := strings.Split(impactedDependencyName, ":")
 	if len(seperatedImpactedDepName) != 2 {
-		err = errorutils.CheckErrorf("unable to parse impacted dependency name '%s'", impactedDependencyName)
+		err = fmt.Errorf("unable to parse impacted dependency name '%s'", impactedDependencyName)
 		return
 	}
 	return seperatedImpactedDepName[0], seperatedImpactedDepName[1], err
@@ -154,9 +168,8 @@ func writeUpdatedBuildFile(filePath string, fileContent string) (err error) {
 		err = fmt.Errorf("couldn't get file info for file '%s': %s", filePath, err.Error())
 		return
 	}
-	filePerm := fileInfo.Mode()
 
-	err = os.WriteFile(filePath, []byte(fileContent), filePerm)
+	err = os.WriteFile(filePath, []byte(fileContent), fileInfo.Mode())
 	if err != nil {
 		err = fmt.Errorf("couldn't write fixes to file '%s': %q", filePath, err)
 	}

packagehandlers/packagehandlers_test.go

@@ -4,12 +4,10 @@ import (
 	"fmt"
 	testdatautils "github.com/jfrog/build-info-go/build/testdata"
 	biutils "github.com/jfrog/build-info-go/utils"
-	fileutils "github.com/jfrog/build-info-go/utils"
 	"github.com/jfrog/frogbot/utils"
 	"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
 	"github.com/jfrog/jfrog-cli-core/v2/xray/formats"
 	"github.com/stretchr/testify/assert"
-	"math"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -597,13 +595,11 @@ func createTempDirAndChdir(t *testing.T, testdataDir string, tech string) func()
 func assertFixVersionInPackageDescriptor(t *testing.T, test dependencyFixTest, packageDescriptor string) {
 	file, err := os.ReadFile(packageDescriptor)
 	assert.NoError(t, err)
-	if test.fixSupported {
-		assert.Contains(t, string(file), test.vulnDetails.SuggestedFixedVersion)
-		// Verify that case-sensitive packages in python are lowered
-		assert.Contains(t, string(file), strings.ToLower(test.vulnDetails.ImpactedDependencyName))
-	} else {
-		assert.NotContains(t, string(file), test.vulnDetails)
-	}
+
+	assert.Contains(t, string(file), test.vulnDetails.SuggestedFixedVersion)
+	// Verify that case-sensitive packages in python are lowered
+	assert.Contains(t, string(file), strings.ToLower(test.vulnDetails.ImpactedDependencyName))
+
 }
 
 // This function is intended to add unique checks for specific package managers
@@ -615,7 +611,12 @@ func uniquePackageManagerChecks(t *testing.T, test dependencyFixTest) {
 		packageDescriptor := extraArgs[0]
 		assertFixVersionInPackageDescriptor(t, test, packageDescriptor)
 	case coreutils.Gradle:
-		checkVulnVersionFixInBuildFile(t, test)
+		descriptorFilesPaths, err := getDescriptorFilesPaths()
+		assert.NoError(t, err)
+		assert.Equal(t, len(descriptorFilesPaths), 2, "incorrect number of descriptor files found")
+		for _, packageDescriptor := range descriptorFilesPaths {
+			assertFixVersionInPackageDescriptor(t, test, packageDescriptor)
+		}
 	default:
 	}
 }
@@ -647,30 +648,6 @@ func TestGetFixedPackage(t *testing.T) {
 	}
 }
 
-func checkVulnVersionFixInBuildFile(t *testing.T, testcase dependencyFixTest) {
-	depGroup, depName, err := getVulnerabilityGroupAndName(testcase.vulnDetails.ImpactedDependencyName)
-	assert.NoError(t, err)
-
-	stringPatternForVulnerability := fmt.Sprintf(directStringWithVersionFormat, depGroup, depName, testcase.vulnDetails.ImpactedDependencyVersion)
-	mapRegexpForVulnerability := fmt.Sprintf(directMapWithVersionRegexp, depGroup, depName, testcase.vulnDetails.ImpactedDependencyVersion)
-	mapRegexpCompiler := regexp.MustCompile(mapRegexpForVulnerability)
-
-	descriptorFilesPaths, err := getDescriptorFilesPaths()
-	assert.NoError(t, err)
-
-	for _, filePath := range descriptorFilesPaths {
-		fileContent, readErr := os.ReadFile(filePath)
-		assert.NoError(t, readErr)
-
-		// Checking there is no unfixed rows in a string format
-		assert.NotContains(t, fileContent, stringPatternForVulnerability)
-
-		// Checking there is no unfixed rows in a map format
-		rowsMatches := mapRegexpCompiler.FindAllString(string(fileContent), -1)
-		assert.Empty(t, rowsMatches)
-	}
-}
-
 func TestGradleGetDescriptorFilesPaths(t *testing.T) {
 	currDir, err := os.Getwd()
 	assert.NoError(t, err)
@@ -684,7 +661,7 @@ func TestGradleGetDescriptorFilesPaths(t *testing.T) {
 	finalPath, err := os.Getwd()
 	assert.NoError(t, err)
 
-	expectedResults := []string{filepath.Join(finalPath, "build.gradle"), filepath.Join(finalPath, "innerProjectForTest", "build.gradle.kts")}
+	expectedResults := []string{filepath.Join(finalPath, groovyDescriptorFileSuffix), filepath.Join(finalPath, "innerProjectForTest", kotlinDescriptorFileSuffix)}
 
 	buildFilesPaths, err := getDescriptorFilesPaths()
 	assert.NoError(t, err)
@@ -697,29 +674,43 @@ func TestGradleFixVulnerabilityIfExists(t *testing.T) {
 
 	tmpDir, err := os.MkdirTemp("", "")
 	assert.NoError(t, err)
-	assert.NoError(t, biutils.CopyDir(filepath.Join("..", "testdata", "projects", "gradle", "innerProjectForTest"), tmpDir, true, nil))
+	assert.NoError(t, biutils.CopyDir(filepath.Join("..", "testdata", "projects", "gradle"), tmpDir, true, nil))
 	assert.NoError(t, os.Chdir(tmpDir))
 	defer func() {
 		assert.NoError(t, os.Chdir(currDir))
 	}()
 
-	buildFiles, err := getDescriptorFilesPaths()
+	descriptorFiles, err := getDescriptorFilesPaths()
 	assert.NoError(t, err)
 
-	err = fixVulnerabilityIfExists(buildFiles[0], &utils.VulnerabilityDetails{
+	vulnerabilityDetails := &utils.VulnerabilityDetails{
 		SuggestedFixedVersion:       "4.13.1",
 		IsDirectDependency:          true,
-		VulnerabilityOrViolationRow: formats.VulnerabilityOrViolationRow{Technology: coreutils.Gradle, ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ImpactedDependencyName: "junit:junit", ImpactedDependencyVersion: "4.7"}}})
+		VulnerabilityOrViolationRow: formats.VulnerabilityOrViolationRow{Technology: coreutils.Gradle, ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ImpactedDependencyName: "junit:junit", ImpactedDependencyVersion: "4.7"}}}
 
-	assert.NoError(t, err)
+	for _, descriptorFile := range descriptorFiles {
+		var isFileChanged bool
+		isFileChanged, err = fixVulnerabilityIfExists(descriptorFile, vulnerabilityDetails)
+		assert.NoError(t, err)
+		assert.True(t, isFileChanged)
+		compareFixedFileToComparisonFile(t, descriptorFile)
+	}
+}
 
-	finalPath, err := os.Getwd()
-	assert.NoError(t, err)
+func compareFixedFileToComparisonFile(t *testing.T, descriptorFileAbsPath string) {
+	var compareFilePath string
+	if strings.HasSuffix(descriptorFileAbsPath, groovyDescriptorFileSuffix) {
+		curDirPath := strings.TrimSuffix(descriptorFileAbsPath, groovyDescriptorFileSuffix)
+		compareFilePath = filepath.Join(curDirPath, "fixedBuildGradleForCompare.txt")
+	} else {
+		curDirPath := strings.TrimSuffix(descriptorFileAbsPath, kotlinDescriptorFileSuffix)
+		compareFilePath = filepath.Join(curDirPath, "fixedBuildGradleKtsForCompare.txt")
+	}
 
-	expectedFileContent, err := fileutils.ReadNLines(filepath.Join(finalPath, "fixedBuildGradleKtsForCompare.txt"), math.MaxInt)
+	expectedFileContent, err := os.ReadFile(descriptorFileAbsPath)
 	assert.NoError(t, err)
 
-	fixedFileContent, err := fileutils.ReadNLines(filepath.Join(finalPath, "build.gradle.kts"), math.MaxInt)
+	fixedFileContent, err := os.ReadFile(compareFilePath)
 	assert.NoError(t, err)
 
 	assert.ElementsMatch(t, expectedFileContent, fixedFileContent)

testdata/projects/gradle/build.gradle

@@ -45,6 +45,10 @@ dependencies {
             ['junit:junit:4.7']
     )
 
+    // This dependency is required in order to check we fix only vulnerable rows
+    implementation "junit:junit:5.7"
+    implementation "junit2:junit2:4.7"
+
     // This repeated dependency is required to check that 'create-fix' doesn't fix lines with unsupported-version fix
     // When the package was found as vulnerable by xRay and fix is applicable somewhere else in the build file
     implementation(group: 'commons-io', name: 'commons-io', version: '1.+')

testdata/projects/gradle/fixedBuildGradleForCompare.txt

@@ -0,0 +1,65 @@
+plugins {
+    id 'java'
+    id 'application'
+}
+
+mainClassName = 'com.example.Main'
+
+group 'com.example'
+version '1.0-SNAPSHOT'
+
+sourceCompatibility = 1.8
+
+repositories {
+    mavenCentral()
+}
+
+dependencies {
+    implementation 'com.fasterxml.jackson.core:jackson-databind:2.10.1'
+    implementation group: 'commons-collections', name: 'commons-collections', version: '3.2.1'
+
+    // This repeated dependency is required in order to check 'create-fix' captures all formats of direct dependencies
+    implementation 'junit:junit:4.13.1'
+    implementation "junit:junit:4.13.1"
+    implementation 'junit:junit:4.13.1:javadoc'
+    implementation('junit:junit:4.13.1')
+
+    implementation group: 'junit', name: 'junit', version: '4.13.1'
+    implementation group: "junit", name: "junit", version: "4.13.1"
+    implementation group: 'junit', name: 'junit', version: '4.13.1', classifier: 'javadoc'
+    implementation(group: 'junit', name: 'junit', version: '4.13.1', classifier: 'javadoc')
+
+    implementation 'junit:junit:4.13.1',
+            'commons-io:commons-io:2.7'
+
+    implementation(
+            group: 'junit', name: 'junit', version: '4.13.1'
+    )
+
+    implementation("junit:junit:4.13.1") {
+        exclude module: "SomeModule"
+    }
+
+    implementation(
+            [group: 'junit', name: 'junit', version: '4.13.1'],
+            ['junit:junit:4.13.1']
+    )
+
+    // This dependency is required in order to check we fix only vulnerable rows
+    implementation "junit:junit:5.7"
+    implementation "junit2:junit2:4.7"
+
+    // This repeated dependency is required to check that 'create-fix' doesn't fix lines with unsupported-version fix
+    // When the package was found as vulnerable by xRay and fix is applicable somewhere else in the build file
+    implementation(group: 'commons-io', name: 'commons-io', version: '1.+')
+    implementation(group: 'commons-io', name: 'commons-io', version: '[1.1, 3.5)')
+    implementation('commons-io:commons-io:latest.release')
+}
+
+sourceSets {
+    main {
+        java {
+            srcDirs = ['src']
+        }
+    }
+}

testdata/projects/gradle/innerProjectForTest/build.gradle.kts

@@ -20,6 +20,10 @@ dependencies {
     runtimeOnly(group = "junit", name = "junit",
             version = "4.7")
 
+    // This dependencies should not be changed
+    runtimeOnly('junit:junit:5.7')
+    runtimeOnly('junit2:junit2:4.7')
+
     // This repeated dependency is required to check that 'create-fix' doesn't fix lines with unsupported-version fix
     // When the package was found as vulnerable by xRay and fix is applicable somewhere else in the build file
     runtimeOnly(group = 'commons-io', name = 'commons-io', version = '1.+')

testdata/projects/gradle/innerProjectForTest/fixedBuildGradleKtsForCompare.txt

@@ -20,6 +20,10 @@ dependencies {
     runtimeOnly(group = "junit", name = "junit",
             version = "4.13.1")
 
+    // This dependencies should not be changed
+    runtimeOnly('junit:junit:5.7')
+    runtimeOnly('junit2:junit2:4.7')
+
     // This repeated dependency is required to check that 'create-fix' doesn't fix lines with unsupported-version fix
     // When the package was found as vulnerable by xRay and fix is applicable somewhere else in the build file
     runtimeOnly(group = 'commons-io', name = 'commons-io', version = '1.+')