From 7aadf8f6383ba143b3a1fae462cb1817b66d8618 Mon Sep 17 00:00:00 2001 From: Eran Turgeman Date: Wed, 27 Sep 2023 16:25:28 +0300 Subject: [PATCH] fixed pr issues, fixed tests and fixed readme --- README.md | 1 - packagehandlers/gradlepackagehandler.go | 51 +++++++----- packagehandlers/packagehandlers_test.go | 79 ++++++++----------- testdata/projects/gradle/build.gradle | 4 + .../gradle/fixedBuildGradleForCompare.txt | 65 +++++++++++++++ .../innerProjectForTest/build.gradle.kts | 4 + .../fixedBuildGradleKtsForCompare.txt | 4 + 7 files changed, 144 insertions(+), 64 deletions(-) create mode 100644 testdata/projects/gradle/fixedBuildGradleForCompare.txt diff --git a/README.md b/README.md index 5928ac542..595484a62 100644 --- a/README.md +++ b/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) diff --git a/packagehandlers/gradlepackagehandler.go b/packagehandlers/gradlepackagehandler.go index 57dfef811..87178f471 100644 --- a/packagehandlers/gradlepackagehandler.go +++ b/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) } diff --git a/packagehandlers/packagehandlers_test.go b/packagehandlers/packagehandlers_test.go index c7669dedd..f1c6df68c 100644 --- a/packagehandlers/packagehandlers_test.go +++ b/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) diff --git a/testdata/projects/gradle/build.gradle b/testdata/projects/gradle/build.gradle index 3e8f4a17c..492d8f65f 100755 --- a/testdata/projects/gradle/build.gradle +++ b/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.+') diff --git a/testdata/projects/gradle/fixedBuildGradleForCompare.txt b/testdata/projects/gradle/fixedBuildGradleForCompare.txt new file mode 100644 index 000000000..576c0508d --- /dev/null +++ b/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'] + } + } +} diff --git a/testdata/projects/gradle/innerProjectForTest/build.gradle.kts b/testdata/projects/gradle/innerProjectForTest/build.gradle.kts index 8012e5a1b..85eb85b4f 100644 --- a/testdata/projects/gradle/innerProjectForTest/build.gradle.kts +++ b/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.+') diff --git a/testdata/projects/gradle/innerProjectForTest/fixedBuildGradleKtsForCompare.txt b/testdata/projects/gradle/innerProjectForTest/fixedBuildGradleKtsForCompare.txt index 2358dca53..a174b811b 100644 --- a/testdata/projects/gradle/innerProjectForTest/fixedBuildGradleKtsForCompare.txt +++ b/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.+')