Merge remote-tracking branch 'origin/dev'

.github/workflows/frogbot-scan-pull-request.yml

@@ -94,6 +94,10 @@ jobs:
           # Displays all existing vulnerabilities, including the ones that were added by the pull request.
           # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+          # [Optional, default: "FALSE"]
+          # When adding new comments on pull requests, keep old comments that were added by previous scans.
+          # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
           # [Optional, default: "TRUE"]
           # Fails the Frogbot task if any security issue is found.
           # JF_FAIL: "FALSE"

docs/install-azure-pipelines.md

@@ -288,6 +288,10 @@ jobs:
             # [Optional, default: "FALSE"]
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
+
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
 
             # [Optional, default: "TRUE"]
             # Fails the Frogbot task if any security issue is found.

docs/install-gitlab.md

@@ -110,6 +110,10 @@ frogbot-scan:
     # Displays all existing vulnerabilities, including the ones that were added by the pull request.
     # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+    # [Optional, default: "FALSE"]
+    # When adding new comments on pull requests, keep old comments that were added by previous scans.
+    # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
     # [Optional, default: "TRUE"]
     # Fails the Frogbot task if any security issue is found.
     # JF_FAIL: "FALSE"

docs/templates/github-actions/frogbot-scan-pull-request.yml

@@ -95,6 +95,10 @@ jobs:
           # Displays all existing vulnerabilities, including the ones that were added by the pull request.
           # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+          # [Optional, default: "FALSE"]
+          # When adding new comments on pull requests, keep old comments that were added by previous scans.
+          # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
           # [Optional, default: "TRUE"]
           # Fails the Frogbot task if any security issue is found.
           # JF_FAIL: "FALSE"

docs/templates/jenkins/scan-pull-request.jenkinsfile

@@ -107,6 +107,10 @@ pipeline {
         // Displays all existing vulnerabilities, including the ones that were added by the pull request.
         // JF_INCLUDE_ALL_VULNERABILITIES= "TRUE"
 
+        // [Optional, default: "FALSE"]
+        // When adding new comments on pull requests, keep old comments that were added by previous scans.
+        // JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION= "TRUE"
+
         // [Optional, default: "TRUE"]
         // Fails the Frogbot task if any security issue is found.
         // JF_FAIL= "FALSE"

docs/templates/jfrog-pipelines/pipelines-dotnet.yml

@@ -126,6 +126,10 @@ pipelines:
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
             # [Optional, default: "TRUE"]
             # Fails the Frogbot task if any security issue is found.
             # JF_FAIL: "FALSE"

docs/templates/jfrog-pipelines/pipelines-go.yml

@@ -119,6 +119,10 @@ pipelines:
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
             # [Optional, default: "TRUE"]
             # Fails the Frogbot task if any security issue is found.
             # JF_FAIL: "FALSE"

docs/templates/jfrog-pipelines/pipelines-gradle.yml

@@ -119,6 +119,10 @@ pipelines:
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
             # [Optional]
             # Frogbot will download the project dependencies if they're not cached locally. To download the
             # dependencies from a virtual repository in Artifactory, set the name of the repository. There's no

docs/templates/jfrog-pipelines/pipelines-maven.yml

@@ -129,6 +129,10 @@ pipelines:
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
             # [Optional]
             # Template for the branch name generated by Frogbot when creating pull requests with fixes.
             # The template must include {BRANCH_NAME_HASH}, to ensure that the generated branch name is unique.

docs/templates/jfrog-pipelines/pipelines-npm.yml

@@ -126,6 +126,10 @@ pipelines:
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
             # [Optional, default: "TRUE"]
             # Fails the Frogbot task if any security issue is found.
             # JF_FAIL: "FALSE"
@@ -140,18 +144,6 @@ pipelines:
             # Relative path to the project in the git repository
             # JF_WORKING_DIR: path/to/project/dir
 
-            # [Optional]
-            # Xray Watches. Learn more about them here: https://www.jfrog.com/confluence/display/JFROG/Configuring+Xray+Watches
-            # JF_WATCHES: <watch-1>,<watch-2>...<watch-n>
-
-            # [Optional, default: "FALSE"]
-            # Displays all existing vulnerabilities, including the ones that were added by the pull request.
-            # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
-
-            # [Optional, default: "TRUE"]
-            # Fails the Frogbot task if any security issue is found.
-            # JF_FAIL: "FALSE"
-
             # [Optional]
             # Template for the branch name generated by Frogbot when creating pull requests with fixes.
             # The template must include {BRANCH_NAME_HASH}, to ensure that the generated branch name is unique.

docs/templates/jfrog-pipelines/pipelines-pip.yml

@@ -132,6 +132,10 @@ pipelines:
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
             # [Optional, default: "TRUE"]
             # Fails the Frogbot task if any security issue is found.
             # JF_FAIL: "FALSE"

docs/templates/jfrog-pipelines/pipelines-pipenv.yml

@@ -125,6 +125,10 @@ pipelines:
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
             # [Optional, default: "TRUE"]
             # Fails the Frogbot task if any security issue is found.
             # JF_FAIL: "FALSE"

docs/templates/jfrog-pipelines/pipelines-poetry.yml

@@ -125,6 +125,10 @@ pipelines:
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
             # [Optional, default: "TRUE"]
             # Fails the Frogbot task if any security issue is found.
             # JF_FAIL: "FALSE"

docs/templates/jfrog-pipelines/pipelines-yarn2.yml

@@ -132,6 +132,10 @@ pipelines:
             # Displays all existing vulnerabilities, including the ones that were added by the pull request.
             # JF_INCLUDE_ALL_VULNERABILITIES: "TRUE"
 
+            # [Optional, default: "FALSE"]
+            # When adding new comments on pull requests, keep old comments that were added by previous scans.
+            # JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION: "TRUE"
+
             # [Optional, default: "TRUE"]
             # Fails the Frogbot task if any security issue is found.
             # JF_FAIL: "FALSE"

packagehandlers/gradlepackagehandler.go

@@ -61,7 +61,7 @@ func (gph *GradlePackageHandler) updateDirectDependency(vulnDetails *utils.Vulne
 	isAnyDescriptorFileChanged := false
 	for _, descriptorFilePath := range descriptorFilesPaths {
 		var isFileChanged bool
-		isFileChanged, err = fixVulnerabilityIfExists(descriptorFilePath, vulnDetails)
+		isFileChanged, err = fixGradleVulnerabilityIfExists(descriptorFilePath, vulnDetails)
 		if err != nil {
 			return
 		}
@@ -106,7 +106,7 @@ func getDescriptorFilesPaths() (descriptorFilesPaths []string, 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) {
+func fixGradleVulnerabilityIfExists(descriptorFilePath string, vulnDetails *utils.VulnerabilityDetails) (isFileChanged bool, err error) {
 	byteFileContent, err := os.ReadFile(descriptorFilePath)
 	if err != nil {
 		err = fmt.Errorf("couldn't read file '%s': %s", descriptorFilePath, err.Error())
@@ -125,8 +125,13 @@ func fixVulnerabilityIfExists(descriptorFilePath string, vulnDetails *utils.Vuln
 	directStringFixedRow := fmt.Sprintf(directStringWithVersionFormat, depGroup, depName, vulnDetails.SuggestedFixedVersion)
 	fileContent = strings.ReplaceAll(fileContent, directStringVulnerableRow, directStringFixedRow)
 
+	// We replace '.' characters to '\\.' since '.' in order to correctly capture '.' character using regexps
+	regexpAdjustedDepGroup := strings.ReplaceAll(depGroup, ".", "\\.")
+	regexpAdjustedDepName := strings.ReplaceAll(depName, ".", "\\.")
+	regexpAdjustedImpactedVersion := strings.ReplaceAll(vulnDetails.ImpactedDependencyVersion, ".", "\\.")
+
 	// 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)
+	mapRegexpForVulnerability := fmt.Sprintf(directMapWithVersionRegexp, regexpAdjustedDepGroup, regexpAdjustedDepName, regexpAdjustedImpactedVersion)
 	regexpCompiler := regexp.MustCompile(mapRegexpForVulnerability)
 	if rowsMatches := regexpCompiler.FindAllString(fileContent, -1); rowsMatches != nil {
 		for _, entry := range rowsMatches {
@@ -152,8 +157,7 @@ func getVulnerabilityGroupAndName(impactedDependencyName string) (depGroup strin
 		err = fmt.Errorf("unable to parse impacted dependency name '%s'", impactedDependencyName)
 		return
 	}
-	// We replace '.' characters to '\\.' since '.' in order to correctly capture '.' character using regexps
-	return strings.ReplaceAll(seperatedImpactedDepName[0], ".", "\\."), strings.ReplaceAll(seperatedImpactedDepName[1], ".", "\\."), err
+	return seperatedImpactedDepName[0], seperatedImpactedDepName[1], err
 }
 
 // Writes the updated content of the descriptor's file into the file

packagehandlers/packagehandlers_test.go

@@ -682,7 +682,26 @@ func TestGradleGetDescriptorFilesPaths(t *testing.T) {
 	assert.ElementsMatch(t, expectedResults, buildFilesPaths)
 }
 
-func TestGradleFixVulnerabilityIfExists(t *testing.T) {
+func TestFixGradleVulnerabilityIfExists(t *testing.T) {
+	var testcases = []struct {
+		vulnerabilityDetails *utils.VulnerabilityDetails
+	}{
+		// Basic check
+		{
+			vulnerabilityDetails: &utils.VulnerabilityDetails{
+				SuggestedFixedVersion:       "4.13.1",
+				IsDirectDependency:          true,
+				VulnerabilityOrViolationRow: formats.VulnerabilityOrViolationRow{Technology: coreutils.Gradle, ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ImpactedDependencyName: "junit:junit", ImpactedDependencyVersion: "4.7"}}},
+		},
+		// This testcase checks a fix with a vulnerability that has '.' in the dependency's group and name + more complex version, including letters, to check that the regexp captures them correctly
+		{
+			vulnerabilityDetails: &utils.VulnerabilityDetails{
+				SuggestedFixedVersion:       "1.9.9",
+				IsDirectDependency:          true,
+				VulnerabilityOrViolationRow: formats.VulnerabilityOrViolationRow{Technology: coreutils.Gradle, ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ImpactedDependencyName: "my.group:my.dot.name", ImpactedDependencyVersion: "1.0.0-beta.test"}}},
+		},
+	}
+
 	currDir, err := os.Getwd()
 	assert.NoError(t, err)
 
@@ -697,18 +716,16 @@ func TestGradleFixVulnerabilityIfExists(t *testing.T) {
 	descriptorFiles, err := getDescriptorFilesPaths()
 	assert.NoError(t, err)
 
-	vulnerabilityDetails := &utils.VulnerabilityDetails{
-		SuggestedFixedVersion:       "4.13.1",
-		IsDirectDependency:          true,
-		VulnerabilityOrViolationRow: formats.VulnerabilityOrViolationRow{Technology: coreutils.Gradle, ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ImpactedDependencyName: "junit:junit", ImpactedDependencyVersion: "4.7"}}}
-
 	for _, descriptorFile := range descriptorFiles {
-		var isFileChanged bool
-		isFileChanged, err = fixVulnerabilityIfExists(descriptorFile, vulnerabilityDetails)
-		assert.NoError(t, err)
-		assert.True(t, isFileChanged)
+		for _, testcase := range testcases {
+			var isFileChanged bool
+			isFileChanged, err = fixGradleVulnerabilityIfExists(descriptorFile, testcase.vulnerabilityDetails)
+			assert.NoError(t, err)
+			assert.True(t, isFileChanged)
+		}
 		compareFixedFileToComparisonFile(t, descriptorFile)
 	}
+
 }
 
 func compareFixedFileToComparisonFile(t *testing.T, descriptorFileAbsPath string) {

scanpullrequest/scanpullrequest.go

@@ -6,10 +6,8 @@ import (
 	"fmt"
 	"golang.org/x/exp/slices"
 	"os"
-	"strings"
 
 	"github.com/jfrog/frogbot/utils"
-	"github.com/jfrog/frogbot/utils/outputwriter"
 	"github.com/jfrog/froggit-go/vcsclient"
 	"github.com/jfrog/froggit-go/vcsutils"
 	"github.com/jfrog/gofrog/datastructures"
@@ -24,7 +22,6 @@ const (
 	securityIssueFoundErr   = "issues were detected by Frogbot\n You can avoid marking the Frogbot scan as failed by setting failOnSecurityIssues to false in the " + utils.FrogbotConfigFile + " file"
 	noGitHubEnvErr          = "frogbot did not scan this PR, because a GitHub Environment named 'frogbot' does not exist. Please refer to the Frogbot documentation for instructions on how to create the Environment"
 	noGitHubEnvReviewersErr = "frogbot did not scan this PR, because the existing GitHub Environment named 'frogbot' doesn't have reviewers selected. Please refer to the Frogbot documentation for instructions on how to create the Environment"
-	frogbotCommentNotFound  = -1
 )
 
 type ScanPullRequestCmd struct{}
@@ -99,31 +96,15 @@ func scanPullRequest(repo *utils.Repository, client vcsclient.VcsClient) (err er
 		return
 	}
 
+	// Output results
 	shouldSendExposedSecretsEmail := issues.SecretsExists() && repo.SmtpServer != ""
 	if shouldSendExposedSecretsEmail {
 		secretsEmailDetails := utils.NewSecretsEmailDetails(client, repo, issues.Secrets)
 		if err = utils.AlertSecretsExposed(secretsEmailDetails); err != nil {
 			return
 		}
 	}
-
-	// Delete previous Frogbot pull request message if exists
-	if err = deleteExistingPullRequestComment(repo, client); err != nil {
-		return
-	}
-
-	// Create a pull request message
-	message := createPullRequestComment(issues, repo.OutputWriter)
-
-	// Add SCA scan comment
-	if err = client.AddPullRequestComment(context.Background(), repo.RepoOwner, repo.RepoName, message, int(pullRequestDetails.ID)); err != nil {
-		err = errors.New("couldn't add pull request comment: " + err.Error())
-		return
-	}
-
-	// Handle review comments at the pull request
-	if err = utils.AddReviewComments(repo, int(pullRequestDetails.ID), client, issues); err != nil {
-		err = errors.New("couldn't add review comments: " + err.Error())
+	if err = utils.HandlePullRequestCommentsAfterScan(issues, repo, client, int(pullRequestDetails.ID)); err != nil {
 		return
 	}
 
@@ -438,43 +419,3 @@ func getViolatedLicenses(allowedLicenses []string, licenses []formats.LicenseRow
 	}
 	return violatedLicenses
 }
-
-func createPullRequestComment(issues *utils.IssuesCollection, writer outputwriter.OutputWriter) string {
-	if !issues.IssuesExists() {
-		return writer.NoVulnerabilitiesTitle() + writer.UntitledForJasMsg() + writer.Footer()
-	}
-	comment := strings.Builder{}
-	comment.WriteString(writer.VulnerabilitiesTitle(true))
-	comment.WriteString(writer.VulnerabilitiesContent(issues.Vulnerabilities))
-	comment.WriteString(writer.LicensesContent(issues.Licenses))
-	comment.WriteString(writer.UntitledForJasMsg())
-	comment.WriteString(writer.Footer())
-
-	return comment.String()
-}
-
-func deleteExistingPullRequestComment(repository *utils.Repository, client vcsclient.VcsClient) error {
-	log.Debug("Looking for an existing Frogbot pull request comment. Deleting it if it exists...")
-	prDetails := repository.PullRequestDetails
-	comments, err := utils.GetSortedPullRequestComments(client, prDetails.Target.Owner, prDetails.Target.Repository, int(prDetails.ID))
-	if err != nil {
-		return fmt.Errorf(
-			"failed to get comments. the following details were used in order to fetch the comments: <%s/%s> pull request #%d. the error received: %s",
-			repository.RepoOwner, repository.RepoName, int(repository.PullRequestDetails.ID), err.Error())
-	}
-
-	commentID := frogbotCommentNotFound
-	for _, comment := range comments {
-		if repository.OutputWriter.IsFrogbotResultComment(comment.Content) {
-			log.Debug("Found previous Frogbot comment with the id:", comment.ID)
-			commentID = int(comment.ID)
-			break
-		}
-	}
-
-	if commentID != frogbotCommentNotFound {
-		err = client.DeletePullRequestComment(context.Background(), prDetails.Target.Owner, prDetails.Target.Repository, int(prDetails.ID), commentID)
-	}
-
-	return err
-}