diff --git a/.github/workflows/frogbot-scan-pull-request.yml b/.github/workflows/frogbot-scan-pull-request.yml index 998c8c91f..af70ce92c 100644 --- a/.github/workflows/frogbot-scan-pull-request.yml +++ b/.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" diff --git a/docs/install-azure-pipelines.md b/docs/install-azure-pipelines.md index 49e10504e..e102ee880 100644 --- a/docs/install-azure-pipelines.md +++ b/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. diff --git a/docs/install-gitlab.md b/docs/install-gitlab.md index 99b95a540..b6fed70aa 100644 --- a/docs/install-gitlab.md +++ b/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" diff --git a/docs/templates/github-actions/frogbot-scan-pull-request.yml b/docs/templates/github-actions/frogbot-scan-pull-request.yml index c69d2dd72..be29c480e 100644 --- a/docs/templates/github-actions/frogbot-scan-pull-request.yml +++ b/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" diff --git a/docs/templates/jenkins/scan-pull-request.jenkinsfile b/docs/templates/jenkins/scan-pull-request.jenkinsfile index 754a31d80..f925ee8ed 100644 --- a/docs/templates/jenkins/scan-pull-request.jenkinsfile +++ b/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" diff --git a/docs/templates/jfrog-pipelines/pipelines-dotnet.yml b/docs/templates/jfrog-pipelines/pipelines-dotnet.yml index 353cc239d..426b726f0 100644 --- a/docs/templates/jfrog-pipelines/pipelines-dotnet.yml +++ b/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" diff --git a/docs/templates/jfrog-pipelines/pipelines-go.yml b/docs/templates/jfrog-pipelines/pipelines-go.yml index f8f5954d7..b5593a463 100644 --- a/docs/templates/jfrog-pipelines/pipelines-go.yml +++ b/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" diff --git a/docs/templates/jfrog-pipelines/pipelines-gradle.yml b/docs/templates/jfrog-pipelines/pipelines-gradle.yml index 8915bfc69..c71776c29 100644 --- a/docs/templates/jfrog-pipelines/pipelines-gradle.yml +++ b/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 diff --git a/docs/templates/jfrog-pipelines/pipelines-maven.yml b/docs/templates/jfrog-pipelines/pipelines-maven.yml index baf0c8d89..a68a7fc24 100644 --- a/docs/templates/jfrog-pipelines/pipelines-maven.yml +++ b/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. diff --git a/docs/templates/jfrog-pipelines/pipelines-npm.yml b/docs/templates/jfrog-pipelines/pipelines-npm.yml index b6d0171c3..99b8ecf4b 100644 --- a/docs/templates/jfrog-pipelines/pipelines-npm.yml +++ b/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: ,... - - # [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. diff --git a/docs/templates/jfrog-pipelines/pipelines-pip.yml b/docs/templates/jfrog-pipelines/pipelines-pip.yml index a58ad8b61..b6d0e5099 100644 --- a/docs/templates/jfrog-pipelines/pipelines-pip.yml +++ b/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" diff --git a/docs/templates/jfrog-pipelines/pipelines-pipenv.yml b/docs/templates/jfrog-pipelines/pipelines-pipenv.yml index ae7075137..9ba8c68ed 100644 --- a/docs/templates/jfrog-pipelines/pipelines-pipenv.yml +++ b/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" diff --git a/docs/templates/jfrog-pipelines/pipelines-poetry.yml b/docs/templates/jfrog-pipelines/pipelines-poetry.yml index 23c206359..22862732f 100644 --- a/docs/templates/jfrog-pipelines/pipelines-poetry.yml +++ b/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" diff --git a/docs/templates/jfrog-pipelines/pipelines-yarn2.yml b/docs/templates/jfrog-pipelines/pipelines-yarn2.yml index 75afde306..980b7861d 100644 --- a/docs/templates/jfrog-pipelines/pipelines-yarn2.yml +++ b/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" diff --git a/packagehandlers/gradlepackagehandler.go b/packagehandlers/gradlepackagehandler.go index 75d0d7b9d..9f0bcd89b 100644 --- a/packagehandlers/gradlepackagehandler.go +++ b/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 diff --git a/packagehandlers/packagehandlers_test.go b/packagehandlers/packagehandlers_test.go index 3dc415b5f..1d37b8828 100644 --- a/packagehandlers/packagehandlers_test.go +++ b/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) { diff --git a/scanpullrequest/scanpullrequest.go b/scanpullrequest/scanpullrequest.go index 9beb54582..98d19c5f6 100644 --- a/scanpullrequest/scanpullrequest.go +++ b/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,6 +96,7 @@ 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) @@ -106,24 +104,7 @@ func scanPullRequest(repo *utils.Repository, client vcsclient.VcsClient) (err er 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 -} diff --git a/scanpullrequest/scanpullrequest_test.go b/scanpullrequest/scanpullrequest_test.go index 39efbc10e..6cf837db6 100644 --- a/scanpullrequest/scanpullrequest_test.go +++ b/scanpullrequest/scanpullrequest_test.go @@ -327,7 +327,7 @@ func TestGetNewVulnerabilities(t *testing.T) { SeverityDetails: formats.SeverityDetails{Severity: "low"}, ImpactedDependencyName: "component-C", }, - Cves: []formats.CveRow{{Id: "CVE-2023-4321", Applicability: &formats.Applicability{Status: "Applicable", Evidence: []formats.Evidence{{Location: formats.Location{File: "file1", StartLine: 1, StartColumn: 10}}}}}}, + Cves: []formats.CveRow{{Id: "CVE-2023-4321", Applicability: &formats.Applicability{Status: "Applicable", Evidence: []formats.Evidence{{Location: formats.Location{File: "file1", StartLine: 1, StartColumn: 10, EndLine: 2, EndColumn: 11, Snippet: "snippet"}}}}}}, Technology: coreutils.Yarn, }, { @@ -338,7 +338,7 @@ func TestGetNewVulnerabilities(t *testing.T) { SeverityDetails: formats.SeverityDetails{Severity: "low"}, ImpactedDependencyName: "component-D", }, - Cves: []formats.CveRow{{Id: "CVE-2023-4321", Applicability: &formats.Applicability{Status: "Applicable", Evidence: []formats.Evidence{{Location: formats.Location{File: "file1", StartLine: 1, StartColumn: 10}}}}}}, + Cves: []formats.CveRow{{Id: "CVE-2023-4321", Applicability: &formats.Applicability{Status: "Applicable", Evidence: []formats.Evidence{{Location: formats.Location{File: "file1", StartLine: 1, StartColumn: 10, EndLine: 2, EndColumn: 11, Snippet: "snippet"}}}}}}, Technology: coreutils.Yarn, }, } @@ -347,39 +347,16 @@ func TestGetNewVulnerabilities(t *testing.T) { vulnerabilities, licenses, err := createNewVulnerabilitiesRows( &audit.Results{ ExtendedScanResults: &xrayutils.ExtendedScanResults{ - XrayResults: []services.ScanResponse{previousScan}, - EntitledForJas: true, - ApplicabilityScanResults: []*sarif.Run{sarif.NewRunWithInformationURI("", ""). - WithResults([]*sarif.Result{ - sarif.NewRuleResult("applic_CVE-2023-4321"). - WithLocations([]*sarif.Location{ - sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation(). - WithUri("file1")). - WithRegion(sarif.NewRegion(). - WithStartLine(1). - WithStartColumn(10))), - }), - }), - }, + XrayResults: []services.ScanResponse{previousScan}, + EntitledForJas: true, + ApplicabilityScanResults: []*sarif.Run{xrayutils.CreateRunWithDummyResults(xrayutils.CreateResultWithOneLocation("file1", 1, 10, 2, 11, "snippet", "applic_CVE-2023-4321", ""))}, }, }, &audit.Results{ ExtendedScanResults: &xrayutils.ExtendedScanResults{ - XrayResults: []services.ScanResponse{currentScan}, - EntitledForJas: true, - ApplicabilityScanResults: []*sarif.Run{sarif.NewRunWithInformationURI("", ""). - WithResults([]*sarif.Result{ - sarif.NewRuleResult("applic_CVE-2023-4321"). - WithLocations([]*sarif.Location{sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation(). - WithUri("file1")). - WithRegion(sarif.NewRegion(). - WithStartLine(1). - WithStartColumn(10))), - }), - }), - }, + XrayResults: []services.ScanResponse{currentScan}, + EntitledForJas: true, + ApplicabilityScanResults: []*sarif.Run{xrayutils.CreateRunWithDummyResults(xrayutils.CreateResultWithOneLocation("file1", 1, 10, 2, 11, "snippet", "applic_CVE-2023-4321", ""))}, }, }, nil, @@ -484,25 +461,6 @@ func TestGetNewVulnerabilitiesCaseNoNewVulnerabilities(t *testing.T) { assert.Len(t, licenses, 0) } -func TestCreatePullRequestMessageNoVulnerabilities(t *testing.T) { - vulnerabilities := []formats.VulnerabilityOrViolationRow{} - message := createPullRequestComment(&utils.IssuesCollection{Vulnerabilities: vulnerabilities}, &outputwriter.StandardOutput{}) - - expectedMessageByte, err := os.ReadFile(filepath.Join("..", "testdata", "messages", "novulnerabilities.md")) - assert.NoError(t, err) - expectedMessage := strings.ReplaceAll(string(expectedMessageByte), "\r\n", "\n") - assert.Equal(t, expectedMessage, message) - - outputWriter := &outputwriter.StandardOutput{} - outputWriter.SetVcsProvider(vcsutils.GitLab) - message = createPullRequestComment(&utils.IssuesCollection{Vulnerabilities: vulnerabilities}, outputWriter) - - expectedMessageByte, err = os.ReadFile(filepath.Join("..", "testdata", "messages", "novulnerabilitiesMR.md")) - assert.NoError(t, err) - expectedMessage = strings.ReplaceAll(string(expectedMessageByte), "\r\n", "\n") - assert.Equal(t, expectedMessage, message) -} - func TestGetAllIssues(t *testing.T) { allowedLicenses := []string{"MIT"} auditResults := &audit.Results{ @@ -515,126 +473,127 @@ func TestGetAllIssues(t *testing.T) { Licenses: []services.License{{Key: "Apache-2.0", Components: map[string]services.Component{"Dep-1": {FixedVersions: []string{"1.2.3"}}}}}, }}, ApplicabilityScanResults: []*sarif.Run{ - utils.GetRunWithDummyResults( - utils.GetDummyResultWithOneLocation("file", 0, 0, "", "applic_CVE-2022-2122", ""), - utils.GetDummyPassingResult("applic_CVE-2023-3122")), + xrayutils.CreateRunWithDummyResults( + xrayutils.CreateDummyPassingResult("applic_CVE-2023-3122"), + xrayutils.CreateResultWithOneLocation("file1", 1, 10, 2, 11, "snippet", "applic_CVE-2022-2122", ""), + ), + }, + IacScanResults: []*sarif.Run{ + xrayutils.CreateRunWithDummyResults( + xrayutils.CreateResultWithLocations("Missing auto upgrade was detected", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "aws-violation"), + ), + ), }, SecretsScanResults: []*sarif.Run{ - utils.GetRunWithDummyResults( - utils.GetDummyResultWithOneLocation("index.js", 2, 13, "access token exposed", "", ""), + xrayutils.CreateRunWithDummyResults( + xrayutils.CreateResultWithLocations("Secret", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("index.js", 5, 6, 7, 8, "access token exposed"), + ), + ), + }, + SastScanResults: []*sarif.Run{ + xrayutils.CreateRunWithDummyResults( + xrayutils.CreateResultWithLocations("XSS Vulnerability", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "snippet"), + ), ), }, EntitledForJas: true, }, } - issuesRows, err := getAllIssues(auditResults, allowedLicenses) - assert.NoError(t, err) - assert.Len(t, issuesRows.Licenses, 1) - assert.Len(t, issuesRows.Vulnerabilities, 2) - assert.Len(t, issuesRows.Secrets, 1) - assert.Equal(t, auditResults.ExtendedScanResults.XrayResults[0].Licenses[0].Key, "Apache-2.0") - assert.Equal(t, "Dep-1", issuesRows.Licenses[0].ImpactedDependencyName) - vuln1 := auditResults.ExtendedScanResults.XrayResults[0].Vulnerabilities[0] - assert.Equal(t, vuln1.Cves[0].Id, issuesRows.Vulnerabilities[0].Cves[0].Id) - assert.Equal(t, vuln1.Severity, issuesRows.Vulnerabilities[0].Severity) - assert.Equal(t, vuln1.Components["Dep-1"].FixedVersions[0], issuesRows.Vulnerabilities[0].FixedVersions[0]) - vuln2 := auditResults.ExtendedScanResults.XrayResults[0].Vulnerabilities[1] - assert.Equal(t, vuln2.Cves[0].Id, issuesRows.Vulnerabilities[1].Cves[0].Id) - assert.Equal(t, vuln2.Severity, issuesRows.Vulnerabilities[1].Severity) - assert.Equal(t, vuln2.Components["Dep-2"].FixedVersions[0], issuesRows.Vulnerabilities[1].FixedVersions[0]) - assert.Equal(t, auditResults.ExtendedScanResults.XrayResults[0].Licenses[0].Key, issuesRows.Licenses[0].LicenseKey) - assert.Equal(t, "Dep-1", issuesRows.Licenses[0].ImpactedDependencyName) - assert.Equal(t, xrayutils.GetResultSeverity(auditResults.ExtendedScanResults.SecretsScanResults[0].Results[0]), issuesRows.Secrets[0].Severity) - assert.Equal(t, xrayutils.GetLocationFileName(auditResults.ExtendedScanResults.SecretsScanResults[0].Results[0].Locations[0]), issuesRows.Secrets[0].File) - assert.Equal(t, *auditResults.ExtendedScanResults.SecretsScanResults[0].Results[0].Locations[0].PhysicalLocation.Region.Snippet.Text, issuesRows.Secrets[0].Snippet) -} -func TestCreatePullRequestComment(t *testing.T) { - vulnerabilities := []formats.VulnerabilityOrViolationRow{ - { - Summary: "Summary XRAY-122345", - ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ - SeverityDetails: formats.SeverityDetails{Severity: "High"}, - ImpactedDependencyName: "github.com/nats-io/nats-streaming-server", - ImpactedDependencyVersion: "v0.21.0", - Components: []formats.ComponentRow{ - { - Name: "github.com/nats-io/nats-streaming-server", - Version: "v0.21.0", - }, + expectedOutput := &utils.IssuesCollection{ + Vulnerabilities: []formats.VulnerabilityOrViolationRow{ + { + Applicable: "Applicable", + FixedVersions: []string{"1.2.3"}, + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "High", SeverityNumValue: 13}, + ImpactedDependencyName: "Dep-1", + }, + Cves: []formats.CveRow{{Id: "CVE-2022-2122", Applicability: &formats.Applicability{Status: "Applicable", Evidence: []formats.Evidence{{Location: formats.Location{File: "file1", StartLine: 1, StartColumn: 10, EndLine: 2, EndColumn: 11, Snippet: "snippet"}}}}}}, + }, + { + Applicable: "Not Applicable", + FixedVersions: []string{"1.2.2"}, + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "Low", SeverityNumValue: 2}, + ImpactedDependencyName: "Dep-2", }, + Cves: []formats.CveRow{{Id: "CVE-2023-3122", Applicability: &formats.Applicability{Status: "Not Applicable"}}}, }, - Applicable: "Undetermined", - FixedVersions: []string{"[0.24.1]"}, - IssueId: "XRAY-122345", - Cves: []formats.CveRow{{}}, }, - { - Summary: "Summary", - ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ - SeverityDetails: formats.SeverityDetails{Severity: "High"}, - ImpactedDependencyName: "github.com/mholt/archiver/v3", - ImpactedDependencyVersion: "v3.5.1", - Components: []formats.ComponentRow{ - { - Name: "github.com/mholt/archiver/v3", - Version: "v3.5.1", - }, + Iacs: []formats.SourceCodeRow{ + { + SeverityDetails: formats.SeverityDetails{ + Severity: "High", + SeverityNumValue: 13, + }, + Finding: "Missing auto upgrade was detected", + Location: formats.Location{ + File: "file1", + StartLine: 1, + StartColumn: 10, + EndLine: 2, + EndColumn: 11, + Snippet: "aws-violation", }, }, - Applicable: "Undetermined", - Cves: []formats.CveRow{}, }, - { - Summary: "Summary CVE-2022-26652", - ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ - SeverityDetails: formats.SeverityDetails{Severity: "Medium"}, - ImpactedDependencyName: "github.com/nats-io/nats-streaming-server", - ImpactedDependencyVersion: "v0.21.0", - Components: []formats.ComponentRow{ - { - Name: "github.com/nats-io/nats-streaming-server", - Version: "v0.21.0", - }, + Secrets: []formats.SourceCodeRow{ + { + SeverityDetails: formats.SeverityDetails{ + Severity: "High", + SeverityNumValue: 13, + }, + Finding: "Secret", + Location: formats.Location{ + File: "index.js", + StartLine: 5, + StartColumn: 6, + EndLine: 7, + EndColumn: 8, + Snippet: "access token exposed", }, }, - Applicable: "Undetermined", - FixedVersions: []string{"[0.24.3]"}, - Cves: []formats.CveRow{{Id: "CVE-2022-26652"}}, }, - } - licenses := []formats.LicenseRow{ - { - LicenseKey: "Apache-2.0", - ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ - SeverityDetails: formats.SeverityDetails{Severity: "High", SeverityNumValue: 13}, - ImpactedDependencyName: "minimatch", - ImpactedDependencyVersion: "1.2.3", - Components: []formats.ComponentRow{ - { - Name: "root", - Version: "1.0.0", - }, - { - Name: "minimatch", - Version: "1.2.3", - }, + Sast: []formats.SourceCodeRow{ + { + SeverityDetails: formats.SeverityDetails{ + Severity: "High", + SeverityNumValue: 13, + }, + Finding: "XSS Vulnerability", + Location: formats.Location{ + File: "file1", + StartLine: 1, + StartColumn: 10, + EndLine: 2, + EndColumn: 11, + Snippet: "snippet", + }, + }, + }, + Licenses: []formats.LicenseRow{ + { + LicenseKey: "Apache-2.0", + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + ImpactedDependencyName: "Dep-1", }, }, }, } - writerOutput := &outputwriter.StandardOutput{} - writerOutput.SetJasOutputFlags(true, true) - message := createPullRequestComment(&utils.IssuesCollection{Vulnerabilities: vulnerabilities, Licenses: licenses}, writerOutput) - - expectedMessage := "
\n\n[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/vulnerabilitiesBannerPR.png)](https://github.com/jfrog/frogbot#readme)\n\n
\n\n\n## šŸ“¦ Vulnerable Dependencies\n\n### āœļø Summary\n\n
\n\n| SEVERITY | CONTEXTUAL ANALYSIS | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS | CVES |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | :---------------------------------: | \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableHighSeverity.png)
High | Undetermined | github.com/nats-io/nats-streaming-server:v0.21.0 | github.com/nats-io/nats-streaming-server:v0.21.0 | [0.24.1] | - |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableHighSeverity.png)
High | Undetermined | github.com/mholt/archiver/v3:v3.5.1 | github.com/mholt/archiver/v3:v3.5.1 | - | - |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableMediumSeverity.png)
Medium | Undetermined | github.com/nats-io/nats-streaming-server:v0.21.0 | github.com/nats-io/nats-streaming-server:v0.21.0 | [0.24.3] | CVE-2022-26652 |\n\n
\n\n## šŸ”¬ Research Details\n\n
\n [ XRAY-122345 ] github.com/nats-io/nats-streaming-server v0.21.0 \n
\n\n**Description:**\nSummary XRAY-122345\n\n\n
\n\n\n
\n github.com/mholt/archiver/v3 v3.5.1 \n
\n\n**Description:**\nSummary\n\n\n
\n\n\n
\n [ CVE-2022-26652 ] github.com/nats-io/nats-streaming-server v0.21.0 \n
\n\n**Description:**\nSummary CVE-2022-26652\n\n\n
\n\n\n## āš–ļø Violated Licenses \n\n
\n\n\n| LICENSE | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | \n| :---------------------: | :----------------------------------: | :-----------------------------------: | \n| Apache-2.0 | root 1.0.0
minimatch 1.2.3 | minimatch 1.2.3 |\n\n
\n\n\n---\n
\n\n[šŸø JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n
" - assert.Equal(t, expectedMessage, message) + issuesRows, err := getAllIssues(auditResults, allowedLicenses) - writerOutput.SetVcsProvider(vcsutils.GitLab) - message = createPullRequestComment(&utils.IssuesCollection{Vulnerabilities: vulnerabilities}, writerOutput) - expectedMessage = "
\n\n[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/vulnerabilitiesBannerMR.png)](https://github.com/jfrog/frogbot#readme)\n\n
\n\n\n## šŸ“¦ Vulnerable Dependencies\n\n### āœļø Summary\n\n
\n\n| SEVERITY | CONTEXTUAL ANALYSIS | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS | CVES |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | :---------------------------------: | \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableHighSeverity.png)
High | Undetermined | github.com/nats-io/nats-streaming-server:v0.21.0 | github.com/nats-io/nats-streaming-server:v0.21.0 | [0.24.1] | - |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableHighSeverity.png)
High | Undetermined | github.com/mholt/archiver/v3:v3.5.1 | github.com/mholt/archiver/v3:v3.5.1 | - | - |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableMediumSeverity.png)
Medium | Undetermined | github.com/nats-io/nats-streaming-server:v0.21.0 | github.com/nats-io/nats-streaming-server:v0.21.0 | [0.24.3] | CVE-2022-26652 |\n\n
\n\n## šŸ”¬ Research Details\n\n
\n [ XRAY-122345 ] github.com/nats-io/nats-streaming-server v0.21.0 \n
\n\n**Description:**\nSummary XRAY-122345\n\n\n
\n\n\n
\n github.com/mholt/archiver/v3 v3.5.1 \n
\n\n**Description:**\nSummary\n\n\n
\n\n\n
\n [ CVE-2022-26652 ] github.com/nats-io/nats-streaming-server v0.21.0 \n
\n\n**Description:**\nSummary CVE-2022-26652\n\n\n
\n\n\n---\n
\n\n[šŸø JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n
" - assert.Equal(t, expectedMessage, message) + if assert.NoError(t, err) { + assert.ElementsMatch(t, expectedOutput.Vulnerabilities, issuesRows.Vulnerabilities) + assert.ElementsMatch(t, expectedOutput.Iacs, issuesRows.Iacs) + assert.ElementsMatch(t, expectedOutput.Secrets, issuesRows.Secrets) + assert.ElementsMatch(t, expectedOutput.Sast, issuesRows.Sast) + assert.ElementsMatch(t, expectedOutput.Licenses, issuesRows.Licenses) + } } func TestScanPullRequest(t *testing.T) { @@ -893,12 +852,9 @@ func TestCreateNewIacRows(t *testing.T) { { name: "No vulnerabilities in source IaC results", targetIacResults: []*sarif.Result{ - sarif.NewRuleResult("").WithLevel(xrayutils.ConvertToSarifLevel("high")).WithLocations([]*sarif.Location{ - sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation().WithUri("file1")). - WithRegion(sarif.NewRegion().WithStartLine(1).WithStartColumn(10).WithSnippet(sarif.NewArtifactContent().WithText("aws violation"))), - ), - }), + xrayutils.CreateResultWithLocations("Missing auto upgrade was detected", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "aws-violation"), + ), }, sourceIacResults: []*sarif.Result{}, expectedAddedIacVulnerabilities: []formats.SourceCodeRow{}, @@ -907,12 +863,9 @@ func TestCreateNewIacRows(t *testing.T) { name: "No vulnerabilities in target IaC results", targetIacResults: []*sarif.Result{}, sourceIacResults: []*sarif.Result{ - sarif.NewRuleResult("").WithLevel(xrayutils.ConvertToSarifLevel("high")).WithLocations([]*sarif.Location{ - sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation().WithUri("file1")). - WithRegion(sarif.NewRegion().WithStartLine(1).WithStartColumn(10).WithSnippet(sarif.NewArtifactContent().WithText("aws violation"))), - ), - }), + xrayutils.CreateResultWithLocations("Missing auto upgrade was detected", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "aws-violation"), + ), }, expectedAddedIacVulnerabilities: []formats.SourceCodeRow{ { @@ -920,11 +873,14 @@ func TestCreateNewIacRows(t *testing.T) { Severity: "High", SeverityNumValue: 13, }, + Finding: "Missing auto upgrade was detected", Location: formats.Location{ File: "file1", StartLine: 1, StartColumn: 10, - Snippet: "aws violation", + EndLine: 2, + EndColumn: 11, + Snippet: "aws-violation", }, }, }, @@ -932,20 +888,14 @@ func TestCreateNewIacRows(t *testing.T) { { name: "Some new vulnerabilities in source IaC results", targetIacResults: []*sarif.Result{ - sarif.NewRuleResult("").WithLevel(xrayutils.ConvertToSarifLevel("high")).WithLocations([]*sarif.Location{ - sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation().WithUri("file1")). - WithRegion(sarif.NewRegion().WithStartLine(1).WithStartColumn(10).WithSnippet(sarif.NewArtifactContent().WithText("aws violation"))), - ), - }), + xrayutils.CreateResultWithLocations("Missing auto upgrade was detected", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "aws-violation"), + ), }, sourceIacResults: []*sarif.Result{ - sarif.NewRuleResult("").WithLevel(xrayutils.ConvertToSarifLevel("medium")).WithLocations([]*sarif.Location{ - sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation().WithUri("file2")). - WithRegion(sarif.NewRegion().WithStartLine(2).WithStartColumn(5).WithSnippet(sarif.NewArtifactContent().WithText("gcp violation"))), - ), - }), + xrayutils.CreateResultWithLocations("enable_private_endpoint=false was detected", "rule", xrayutils.ConvertToSarifLevel("medium"), + xrayutils.CreateLocation("file2", 2, 5, 3, 6, "gcp-violation"), + ), }, expectedAddedIacVulnerabilities: []formats.SourceCodeRow{ { @@ -953,11 +903,14 @@ func TestCreateNewIacRows(t *testing.T) { Severity: "Medium", SeverityNumValue: 11, }, + Finding: "enable_private_endpoint=false was detected", Location: formats.Location{ File: "file2", StartLine: 2, StartColumn: 5, - Snippet: "gcp violation", + EndLine: 3, + EndColumn: 6, + Snippet: "gcp-violation", }, }, }, @@ -966,8 +919,8 @@ func TestCreateNewIacRows(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - targetIacRows := xrayutils.PrepareIacs([]*sarif.Run{sarif.NewRunWithInformationURI("", "").WithResults(tc.targetIacResults)}) - sourceIacRows := xrayutils.PrepareIacs([]*sarif.Run{sarif.NewRunWithInformationURI("", "").WithResults(tc.sourceIacResults)}) + targetIacRows := xrayutils.PrepareIacs([]*sarif.Run{xrayutils.CreateRunWithDummyResults(tc.targetIacResults...)}) + sourceIacRows := xrayutils.PrepareIacs([]*sarif.Run{xrayutils.CreateRunWithDummyResults(tc.sourceIacResults...)}) addedIacVulnerabilities := createNewSourceCodeRows(targetIacRows, sourceIacRows) assert.ElementsMatch(t, tc.expectedAddedIacVulnerabilities, addedIacVulnerabilities) }) @@ -984,12 +937,9 @@ func TestCreateNewSecretRows(t *testing.T) { { name: "No vulnerabilities in source secrets results", targetSecretsResults: []*sarif.Result{ - sarif.NewRuleResult("").WithMessage(sarif.NewTextMessage("Secret")).WithLevel(xrayutils.ConvertToSarifLevel("high")).WithLocations([]*sarif.Location{ - sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation().WithUri("file1")). - WithRegion(sarif.NewRegion().WithStartLine(1).WithStartColumn(10).WithSnippet(sarif.NewArtifactContent().WithText("Sensitive information"))), - ), - }), + xrayutils.CreateResultWithLocations("Secret", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "Sensitive information"), + ), }, sourceSecretsResults: []*sarif.Result{}, expectedAddedSecretsVulnerabilities: []formats.SourceCodeRow{}, @@ -998,12 +948,9 @@ func TestCreateNewSecretRows(t *testing.T) { name: "No vulnerabilities in target secrets results", targetSecretsResults: []*sarif.Result{}, sourceSecretsResults: []*sarif.Result{ - sarif.NewRuleResult("").WithMessage(sarif.NewTextMessage("Secret")).WithLevel(xrayutils.ConvertToSarifLevel("high")).WithLocations([]*sarif.Location{ - sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation().WithUri("file1")). - WithRegion(sarif.NewRegion().WithStartLine(1).WithStartColumn(10).WithSnippet(sarif.NewArtifactContent().WithText("Sensitive information"))), - ), - }), + xrayutils.CreateResultWithLocations("Secret", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "Sensitive information"), + ), }, expectedAddedSecretsVulnerabilities: []formats.SourceCodeRow{ { @@ -1016,6 +963,8 @@ func TestCreateNewSecretRows(t *testing.T) { File: "file1", StartLine: 1, StartColumn: 10, + EndLine: 2, + EndColumn: 11, Snippet: "Sensitive information", }, }, @@ -1024,20 +973,14 @@ func TestCreateNewSecretRows(t *testing.T) { { name: "Some new vulnerabilities in source secrets results", targetSecretsResults: []*sarif.Result{ - sarif.NewRuleResult("").WithMessage(sarif.NewTextMessage("Secret")).WithLevel(xrayutils.ConvertToSarifLevel("high")).WithLocations([]*sarif.Location{ - sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation().WithUri("file1")). - WithRegion(sarif.NewRegion().WithStartLine(1).WithStartColumn(10).WithSnippet(sarif.NewArtifactContent().WithText("Sensitive information"))), - ), - }), + xrayutils.CreateResultWithLocations("Secret", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "Sensitive information"), + ), }, sourceSecretsResults: []*sarif.Result{ - sarif.NewRuleResult("").WithMessage(sarif.NewTextMessage("Secret")).WithLevel(xrayutils.ConvertToSarifLevel("medium")).WithLocations([]*sarif.Location{ - sarif.NewLocationWithPhysicalLocation(sarif.NewPhysicalLocation(). - WithArtifactLocation(sarif.NewArtifactLocation().WithUri("file2")). - WithRegion(sarif.NewRegion().WithStartLine(2).WithStartColumn(5).WithSnippet(sarif.NewArtifactContent().WithText("Confidential data"))), - ), - }), + xrayutils.CreateResultWithLocations("Secret", "rule", xrayutils.ConvertToSarifLevel("medium"), + xrayutils.CreateLocation("file2", 2, 5, 3, 6, "Confidential data"), + ), }, expectedAddedSecretsVulnerabilities: []formats.SourceCodeRow{ { @@ -1050,6 +993,8 @@ func TestCreateNewSecretRows(t *testing.T) { File: "file2", StartLine: 2, StartColumn: 5, + EndLine: 3, + EndColumn: 6, Snippet: "Confidential data", }, }, @@ -1059,14 +1004,99 @@ func TestCreateNewSecretRows(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - targetSecretsRows := xrayutils.PrepareSecrets([]*sarif.Run{sarif.NewRunWithInformationURI("", "").WithResults(tc.targetSecretsResults)}) - sourceSecretsRows := xrayutils.PrepareSecrets([]*sarif.Run{sarif.NewRunWithInformationURI("", "").WithResults(tc.sourceSecretsResults)}) + targetSecretsRows := xrayutils.PrepareSecrets([]*sarif.Run{xrayutils.CreateRunWithDummyResults(tc.targetSecretsResults...)}) + sourceSecretsRows := xrayutils.PrepareSecrets([]*sarif.Run{xrayutils.CreateRunWithDummyResults(tc.sourceSecretsResults...)}) addedSecretsVulnerabilities := createNewSourceCodeRows(targetSecretsRows, sourceSecretsRows) assert.ElementsMatch(t, tc.expectedAddedSecretsVulnerabilities, addedSecretsVulnerabilities) }) } } +func TestCreateNewSastRows(t *testing.T) { + testCases := []struct { + name string + targetSastResults []*sarif.Result + sourceSastResults []*sarif.Result + expectedAddedSastVulnerabilities []formats.SourceCodeRow + }{ + { + name: "No vulnerabilities in source Sast results", + targetSastResults: []*sarif.Result{ + xrayutils.CreateResultWithLocations("XSS Vulnerability", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "snippet"), + ), + }, + sourceSastResults: []*sarif.Result{}, + expectedAddedSastVulnerabilities: []formats.SourceCodeRow{}, + }, + { + name: "No vulnerabilities in target Sast results", + targetSastResults: []*sarif.Result{}, + sourceSastResults: []*sarif.Result{ + xrayutils.CreateResultWithLocations("XSS Vulnerability", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "snippet"), + ), + }, + expectedAddedSastVulnerabilities: []formats.SourceCodeRow{ + { + SeverityDetails: formats.SeverityDetails{ + Severity: "High", + SeverityNumValue: 13, + }, + Finding: "XSS Vulnerability", + Location: formats.Location{ + File: "file1", + StartLine: 1, + StartColumn: 10, + EndLine: 2, + EndColumn: 11, + Snippet: "snippet", + }, + }, + }, + }, + { + name: "Some new vulnerabilities in source Sast results", + targetSastResults: []*sarif.Result{ + xrayutils.CreateResultWithLocations("XSS Vulnerability", "rule", xrayutils.ConvertToSarifLevel("high"), + xrayutils.CreateLocation("file1", 1, 10, 2, 11, "snippet"), + ), + }, + sourceSastResults: []*sarif.Result{ + xrayutils.CreateResultWithLocations("Stack Trace Exposure", "rule", xrayutils.ConvertToSarifLevel("medium"), + xrayutils.CreateLocation("file2", 2, 5, 3, 6, "other-snippet"), + ), + }, + expectedAddedSastVulnerabilities: []formats.SourceCodeRow{ + { + SeverityDetails: formats.SeverityDetails{ + Severity: "Medium", + SeverityNumValue: 11, + }, + Finding: "Stack Trace Exposure", + Location: formats.Location{ + File: "file2", + StartLine: 2, + StartColumn: 5, + EndLine: 3, + EndColumn: 6, + Snippet: "other-snippet", + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + targetSastRows := xrayutils.PrepareSast([]*sarif.Run{xrayutils.CreateRunWithDummyResults(tc.targetSastResults...)}) + sourceSastRows := xrayutils.PrepareSast([]*sarif.Run{xrayutils.CreateRunWithDummyResults(tc.sourceSastResults...)}) + addedSastVulnerabilities := createNewSourceCodeRows(targetSastRows, sourceSastRows) + assert.ElementsMatch(t, tc.expectedAddedSastVulnerabilities, addedSastVulnerabilities) + }) + } +} + func TestDeletePreviousPullRequestMessages(t *testing.T) { repository := &utils.Repository{ Params: utils.Params{ @@ -1081,23 +1111,88 @@ func TestDeletePreviousPullRequestMessages(t *testing.T) { } client := CreateMockVcsClient(t) - // Test with comment returned - client.EXPECT().ListPullRequestComments(context.Background(), "owner", "repo", 17).Return([]vcsclient.CommentInfo{ - {ID: 20, Content: outputwriter.GetBanner(outputwriter.NoVulnerabilityPrBannerSource) + "text \n table\n text text text", Created: time.Unix(3, 0)}, - }, nil) - client.EXPECT().DeletePullRequestComment(context.Background(), "owner", "repo", 17, 20).Return(nil).AnyTimes() - err := deleteExistingPullRequestComment(repository, client) - assert.NoError(t, err) + testCases := []struct { + name string + commentsOnPR []vcsclient.CommentInfo + err error + }{ + { + name: "Test with comment returned", + commentsOnPR: []vcsclient.CommentInfo{ + {ID: 20, Content: outputwriter.GetBanner(outputwriter.NoVulnerabilityPrBannerSource) + "text \n table\n text text text", Created: time.Unix(3, 0)}, + }, + }, + { + name: "Test with no comment returned", + }, + { + name: "Test with error returned", + err: errors.New("error"), + }, + } - // Test with no comment returned - client.EXPECT().ListPullRequestComments(context.Background(), "owner", "repo", 17).Return([]vcsclient.CommentInfo{}, nil) - err = deleteExistingPullRequestComment(repository, client) - assert.NoError(t, err) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Test with comment returned + client.EXPECT().ListPullRequestComments(context.Background(), "owner", "repo", 17).Return(tc.commentsOnPR, tc.err) + client.EXPECT().DeletePullRequestComment(context.Background(), "owner", "repo", 17, 20).Return(nil).AnyTimes() + err := utils.DeleteExistingPullRequestComments(repository, client) + if tc.err == nil { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + }) + } +} + +func TestDeletePreviousPullRequestReviewMessages(t *testing.T) { + repository := &utils.Repository{ + Params: utils.Params{ + Git: utils.Git{ + PullRequestDetails: vcsclient.PullRequestInfo{Target: vcsclient.BranchInfo{ + Repository: "repo", + Owner: "owner", + }, ID: 17}, + }, + }, + OutputWriter: &outputwriter.StandardOutput{}, + } + client := CreateMockVcsClient(t) + + testCases := []struct { + name string + commentsOnPR []vcsclient.CommentInfo + err error + }{ + { + name: "Test with comment returned", + commentsOnPR: []vcsclient.CommentInfo{ + {ID: 20, Content: outputwriter.MarkdownComment(outputwriter.ReviewCommentId) + "text \n table\n text text text", Created: time.Unix(3, 0)}, + }, + }, + { + name: "Test with no comment returned", + }, + { + name: "Test with error returned", + err: errors.New("error"), + }, + } - // Test with error returned - client.EXPECT().ListPullRequestComments(context.Background(), "owner", "repo", 17).Return(nil, errors.New("error")) - err = deleteExistingPullRequestComment(repository, client) - assert.Error(t, err) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Test with comment returned + client.EXPECT().ListPullRequestReviewComments(context.Background(), "", "", 17).Return(tc.commentsOnPR, tc.err) + client.EXPECT().DeletePullRequestReviewComments(context.Background(), "", "", 17, tc.commentsOnPR).Return(nil).AnyTimes() + err := utils.DeleteExistingPullRequestReviewComments(repository, 17, client) + if tc.err == nil { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + }) + } } // Set new logger with output redirection to a null logger. This is useful for negative tests. diff --git a/schema/frogbot-schema.json b/schema/frogbot-schema.json index 874598c34..51c046796 100644 --- a/schema/frogbot-schema.json +++ b/schema/frogbot-schema.json @@ -99,6 +99,11 @@ "description": "Set to true to display all existing vulnerabilities, including the ones that were not added by the pull request.", "title": "Include All Vulnerabilities" }, + "avoidPreviousPrCommentsDeletion": { + "type": "boolean", + "description": "When adding new comments on pull requests, keep old comments that were added by previous scans.", + "title": "Keep Previous Frogbot Comments" + }, "failOnSecurityIssues": { "type": "boolean", "description": "Set to true to fail the job if security issues were found.", diff --git a/schema/schemas_test.go b/schema/schemas_test.go index 1e7cb1440..83b4c91a9 100644 --- a/schema/schemas_test.go +++ b/schema/schemas_test.go @@ -8,12 +8,19 @@ import ( "path/filepath" "strings" "testing" + "time" + clientutils "github.com/jfrog/jfrog-client-go/utils" "github.com/stretchr/testify/assert" "github.com/xeipuuv/gojsonschema" "gopkg.in/yaml.v3" ) +const ( + maxRetriesToDownloadSchema = 5 + durationBetweenSchemaDownloadRetries = 10 * time.Second +) + func TestFrogbotSchema(t *testing.T) { // Load frogbot schema schema, err := os.ReadFile("frogbot-schema.json") @@ -63,16 +70,31 @@ func TestGitHubActionsTemplates(t *testing.T) { // t - Testing object // schema - The schema file to download func downloadFromSchemaStore(t *testing.T, schema string) gojsonschema.JSONLoader { - response, err := http.Get("https://json.schemastore.org/" + schema) - assert.NoError(t, err) + var response *http.Response + var err error + retryExecutor := clientutils.RetryExecutor{ + MaxRetries: maxRetriesToDownloadSchema, + RetriesIntervalMilliSecs: int(durationBetweenSchemaDownloadRetries.Milliseconds()), + ErrorMessage: "Failed to download schema.", + ExecutionHandler: func() (bool, error) { + response, err = http.Get("https://json.schemastore.org/" + schema) + if err != nil { + return true, err + } + if response.StatusCode != http.StatusOK { + return true, fmt.Errorf("failed to download schema. Response status: %s", response.Status) + } + return false, nil + }, + } + assert.NoError(t, retryExecutor.Execute()) + assert.Equal(t, http.StatusOK, response.StatusCode, response.Status) + // Check server response and read schema bytes defer func() { assert.NoError(t, response.Body.Close()) }() - // Check server response - assert.Equal(t, http.StatusOK, response.StatusCode, response.Status) schemaBytes, err := io.ReadAll(response.Body) assert.NoError(t, err) - return gojsonschema.NewBytesLoader(schemaBytes) } diff --git a/testdata/config/frogbot-config-test-unmarshal.yml b/testdata/config/frogbot-config-test-unmarshal.yml index cc0d6cc79..0882935a5 100755 --- a/testdata/config/frogbot-config-test-unmarshal.yml +++ b/testdata/config/frogbot-config-test-unmarshal.yml @@ -29,6 +29,7 @@ - b/c failOnSecurityIssues: true includeAllVulnerabilities: false + avoidPreviousPrCommentsDeletion: true jfrogPlatform: watches: - watch-1 diff --git a/testdata/projects/gradle/build.gradle b/testdata/projects/gradle/build.gradle index 492d8f65f..73d6875e0 100755 --- a/testdata/projects/gradle/build.gradle +++ b/testdata/projects/gradle/build.gradle @@ -45,6 +45,9 @@ dependencies { ['junit:junit:4.7'] ) + implementation "my.group:my.dot.name:1.0.0-beta.test" + implementation group: 'my.group', name: 'my.dot.name', version: '1.0.0-beta.test' + // This dependency is required in order to check we fix only vulnerable rows implementation "junit:junit:5.7" implementation "junit2:junit2:4.7" diff --git a/testdata/projects/gradle/fixedBuildGradleForCompare.txt b/testdata/projects/gradle/fixedBuildGradleForCompare.txt index 576c0508d..440e4058b 100644 --- a/testdata/projects/gradle/fixedBuildGradleForCompare.txt +++ b/testdata/projects/gradle/fixedBuildGradleForCompare.txt @@ -45,6 +45,9 @@ dependencies { ['junit:junit:4.13.1'] ) + implementation "my.group:my.dot.name:1.9.9" + implementation group: 'my.group', name: 'my.dot.name', version: '1.9.9' + // This dependency is required in order to check we fix only vulnerable rows implementation "junit:junit:5.7" implementation "junit2:junit2:4.7" diff --git a/testdata/projects/gradle/innerProjectForTest/build.gradle.kts b/testdata/projects/gradle/innerProjectForTest/build.gradle.kts index 85eb85b4f..4082d4d84 100644 --- a/testdata/projects/gradle/innerProjectForTest/build.gradle.kts +++ b/testdata/projects/gradle/innerProjectForTest/build.gradle.kts @@ -20,6 +20,9 @@ dependencies { runtimeOnly(group = "junit", name = "junit", version = "4.7") + runtimeOnly("my.group:my.dot.name:1.0.0-beta.test") + runtimeOnly(group = 'my.group', name = 'my.dot.name', version = '1.0.0-beta.test') + // This dependencies should not be changed runtimeOnly('junit:junit:5.7') runtimeOnly('junit2:junit2:4.7') diff --git a/testdata/projects/gradle/innerProjectForTest/fixedBuildGradleKtsForCompare.txt b/testdata/projects/gradle/innerProjectForTest/fixedBuildGradleKtsForCompare.txt index a174b811b..d192a53d7 100644 --- a/testdata/projects/gradle/innerProjectForTest/fixedBuildGradleKtsForCompare.txt +++ b/testdata/projects/gradle/innerProjectForTest/fixedBuildGradleKtsForCompare.txt @@ -20,6 +20,9 @@ dependencies { runtimeOnly(group = "junit", name = "junit", version = "4.13.1") + runtimeOnly("my.group:my.dot.name:1.9.9") + runtimeOnly(group = 'my.group', name = 'my.dot.name', version = '1.9.9') + // This dependencies should not be changed runtimeOnly('junit:junit:5.7') runtimeOnly('junit2:junit2:4.7') diff --git a/utils/consts.go b/utils/consts.go index 7cd1cffca..18b55be01 100644 --- a/utils/consts.go +++ b/utils/consts.go @@ -41,19 +41,20 @@ const ( PullRequestTitleTemplateEnv = "JF_PULL_REQUEST_TITLE_TEMPLATE" // Repository environment variables - Ignored if the frogbot-config.yml file is used - InstallCommandEnv = "JF_INSTALL_DEPS_CMD" - RequirementsFileEnv = "JF_REQUIREMENTS_FILE" - WorkingDirectoryEnv = "JF_WORKING_DIR" - jfrogWatchesEnv = "JF_WATCHES" - jfrogProjectEnv = "JF_PROJECT" - IncludeAllVulnerabilitiesEnv = "JF_INCLUDE_ALL_VULNERABILITIES" - FailOnSecurityIssuesEnv = "JF_FAIL" - UseWrapperEnv = "JF_USE_WRAPPER" - DepsRepoEnv = "JF_DEPS_REPO" - MinSeverityEnv = "JF_MIN_SEVERITY" - FixableOnlyEnv = "JF_FIXABLE_ONLY" - AllowedLicensesEnv = "JF_ALLOWED_LICENSES" - WatchesDelimiter = "," + InstallCommandEnv = "JF_INSTALL_DEPS_CMD" + RequirementsFileEnv = "JF_REQUIREMENTS_FILE" + WorkingDirectoryEnv = "JF_WORKING_DIR" + jfrogWatchesEnv = "JF_WATCHES" + jfrogProjectEnv = "JF_PROJECT" + IncludeAllVulnerabilitiesEnv = "JF_INCLUDE_ALL_VULNERABILITIES" + AvoidPreviousPrCommentsDeletionEnv = "JF_AVOID_PREVIOUS_PR_COMMENTS_DELETION" + FailOnSecurityIssuesEnv = "JF_FAIL" + UseWrapperEnv = "JF_USE_WRAPPER" + DepsRepoEnv = "JF_DEPS_REPO" + MinSeverityEnv = "JF_MIN_SEVERITY" + FixableOnlyEnv = "JF_FIXABLE_ONLY" + AllowedLicensesEnv = "JF_ALLOWED_LICENSES" + WatchesDelimiter = "," // Email related environment variables //#nosec G101 -- False positive - no hardcoded credentials. diff --git a/utils/outputwriter/outputwriter.go b/utils/outputwriter/outputwriter.go index cb9eb1955..b6cc6f421 100644 --- a/utils/outputwriter/outputwriter.go +++ b/utils/outputwriter/outputwriter.go @@ -12,12 +12,14 @@ import ( const ( FrogbotTitlePrefix = "[šŸø Frogbot]" CommentGeneratedByFrogbot = "[šŸø JFrog Frogbot](https://github.com/jfrog/frogbot#readme)" + ReviewCommentId = "FrogbotReviewComment" vulnerabilitiesTableHeader = "\n| SEVERITY | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS | CVES |\n| :---------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | :---------------------------------: |" vulnerabilitiesTableHeaderWithContextualAnalysis = "| SEVERITY | CONTEXTUAL ANALYSIS | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS | CVES |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | :---------------------------------: |" iacTableHeader = "\n| SEVERITY | FILE | LINE:COLUMN | FINDING |\n| :---------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: |" vulnerableDependenciesTitle = "## šŸ“¦ Vulnerable Dependencies" summaryTitle = "### āœļø Summary" researchDetailsTitle = "## šŸ”¬ Research Details" + sastTitle = "## šŸŽÆ Static Application Security Testing (SAST) Vulnerability" iacTitle = "## šŸ› ļø Infrastructure as Code" licenseTitle = "## āš–ļø Violated Licenses" contextualAnalysisTitle = "## šŸ“¦šŸ” Contextual Analysis CVE Vulnerability\n" @@ -254,3 +256,11 @@ func getVulnerabilityDescriptionIdentifier(cveRows []formats.CveRow, xrayId stri } return fmt.Sprintf("[ %s ] ", identifier) } + +func GenerateReviewCommentContent(content string, writer OutputWriter) string { + return MarkdownComment(ReviewCommentId) + content + writer.Footer() +} + +func GetFallbackReviewCommentContent(content string, location formats.Location, writer OutputWriter) string { + return MarkdownComment(ReviewCommentId) + GetLocationDescription(location) + content + writer.Footer() +} diff --git a/utils/outputwriter/outputwriter_test.go b/utils/outputwriter/outputwriter_test.go index 2f3ccb8de..db4a06df5 100644 --- a/utils/outputwriter/outputwriter_test.go +++ b/utils/outputwriter/outputwriter_test.go @@ -62,3 +62,118 @@ func testGetLicensesTableContent(t *testing.T, writer OutputWriter) { expected = "\n| License1 | Comp1 1.0 | Dep1 2.0 |\n| License2 | Comp2 2.0 | Dep2 3.0 |" assert.Equal(t, expected, result) } + +func TestMarkAsQuote(t *testing.T) { + testCases := []struct { + input string + expectedOutput string + }{ + { + input: "", + expectedOutput: "``", + }, + { + input: "quote", + expectedOutput: "`quote`", + }, + } + for _, tc := range testCases { + assert.Equal(t, tc.expectedOutput, MarkAsQuote(tc.input)) + } +} + +func TestMarkAsCodeSnippet(t *testing.T) { + testCases := []struct { + input string + expectedOutput string + }{ + { + input: "", + expectedOutput: "```\n\n```", + }, + { + input: "snippet", + expectedOutput: "```\nsnippet\n```", + }, + } + for _, tc := range testCases { + assert.Equal(t, tc.expectedOutput, MarkAsCodeSnippet(tc.input)) + } +} + +func TestGetLocationDescription(t *testing.T) { + testCases := []struct { + input formats.Location + expectedOutput string + }{ + { + input: formats.Location{ + File: "file1", + StartLine: 1, + Snippet: "snippet", + }, + expectedOutput: "\n```\nsnippet\n```\nat `file1` (line 1)\n", + }, + { + input: formats.Location{ + File: "dir/other-dir/file1", + StartLine: 134, + Snippet: "clientTestUtils.ChangeDirAndAssert(t, prevWd)", + }, + expectedOutput: "\n```\nclientTestUtils.ChangeDirAndAssert(t, prevWd)\n```\nat `dir/other-dir/file1` (line 134)\n", + }, + } + for _, tc := range testCases { + assert.Equal(t, tc.expectedOutput, GetLocationDescription(tc.input)) + } +} + +func TestGetJasMarkdownDescription(t *testing.T) { + testCases := []struct { + severity string + finding string + expectedOutput string + }{ + { + severity: "High", + finding: "finding", + expectedOutput: "| Severity | Finding |\n| :--------------: | :---: |\n| High | finding |", + }, + { + severity: "Low", + finding: "finding (other)", + expectedOutput: "| Severity | Finding |\n| :--------------: | :---: |\n| Low | finding (other) |", + }, + } + for _, tc := range testCases { + assert.Equal(t, tc.expectedOutput, GetJasMarkdownDescription(tc.severity, tc.finding)) + } +} + +func TestGetApplicabilityMarkdownDescription(t *testing.T) { + testCases := []struct { + severity string + cve string + impactedDependency string + finding string + expectedOutput string + }{ + { + severity: "High", + cve: "CVE-100-234", + impactedDependency: "dependency:1.0.0", + finding: "applicable finding", + expectedOutput: "| Severity | Impacted Dependency | Finding | CVE |\n| :--------------: | :---: | :---: | :---: |\n| High | dependency:1.0.0 | applicable finding | CVE-100-234 |", + }, + { + severity: "Low", + cve: "CVE-222-233", + impactedDependency: "dependency:3.4.1", + finding: "applicable finding (diff)", + expectedOutput: "| Severity | Impacted Dependency | Finding | CVE |\n| :--------------: | :---: | :---: | :---: |\n| Low | dependency:3.4.1 | applicable finding (diff) | CVE-222-233 |", + }, + } + for _, tc := range testCases { + assert.Equal(t, tc.expectedOutput, GetApplicabilityMarkdownDescription(tc.severity, tc.cve, tc.impactedDependency, tc.finding)) + } +} diff --git a/utils/outputwriter/simplifiedoutput.go b/utils/outputwriter/simplifiedoutput.go index 3acf6145e..fbdeba32d 100644 --- a/utils/outputwriter/simplifiedoutput.go +++ b/utils/outputwriter/simplifiedoutput.go @@ -191,16 +191,16 @@ func (smo *SimplifiedOutput) SastReviewContent(severity, finding, fullDetails st ### Full description %s - ---- -### Code Flows - `, GetJasMarkdownDescription(smo.FormattedSeverity(severity, "Applicable"), finding), fullDetails, )) if len(codeFlows) > 0 { + contentBuilder.WriteString(` +--- +### Code Flows +`) for _, flow := range codeFlows { contentBuilder.WriteString(` diff --git a/utils/outputwriter/simplifiedoutput_test.go b/utils/outputwriter/simplifiedoutput_test.go index 1cfcb65e0..663a5c35d 100644 --- a/utils/outputwriter/simplifiedoutput_test.go +++ b/utils/outputwriter/simplifiedoutput_test.go @@ -375,3 +375,75 @@ func TestSimplifiedOutput_GetLicensesTableContent(t *testing.T) { writer := &SimplifiedOutput{} testGetLicensesTableContent(t, writer) } + +func TestSimplifiedOutput_SastReviewContent(t *testing.T) { + testCases := []struct { + name string + severity string + finding string + fullDetails string + expectedOutput string + codeFlows [][]formats.Location + }{ + { + name: "Sast review comment content", + severity: "Low", + finding: "Stack Trace Exposure", + fullDetails: "\n### Overview\nStack trace exposure is a type of security vulnerability that occurs when a program reveals\nsensitive information, such as the names and locations of internal files and variables,\nin error messages or other diagnostic output. This can happen when a program crashes or\nencounters an error, and the stack trace (a record of the program's call stack at the time\nof the error) is included in the output.", + codeFlows: [][]formats.Location{ + { + { + File: "file2", + StartLine: 1, + StartColumn: 2, + EndLine: 3, + EndColumn: 4, + Snippet: "other-snippet", + }, + { + File: "file", + StartLine: 0, + StartColumn: 0, + EndLine: 0, + EndColumn: 0, + Snippet: "snippet", + }, + }, + { + { + File: "file", + StartLine: 10, + StartColumn: 20, + EndLine: 10, + EndColumn: 30, + Snippet: "a-snippet", + }, + { + File: "file", + StartLine: 0, + StartColumn: 0, + EndLine: 0, + EndColumn: 0, + Snippet: "snippet", + }, + }, + }, + expectedOutput: "\n## šŸŽÆ Static Application Security Testing (SAST) Vulnerability\n\t\n| Severity | Finding |\n| :--------------: | :---: |\n| Low | Stack Trace Exposure |\n\n---\n### Full description\n\n\n### Overview\nStack trace exposure is a type of security vulnerability that occurs when a program reveals\nsensitive information, such as the names and locations of internal files and variables,\nin error messages or other diagnostic output. This can happen when a program crashes or\nencounters an error, and the stack trace (a record of the program's call stack at the time\nof the error) is included in the output.\n\n---\n### Code Flows\n\n\n---\nVulnerable data flow analysis result:\n\nā†˜ļø `other-snippet` (at file2 line 1)\n\nā†˜ļø `snippet` (at file line 0)\n\n\n---\n\n\n\n---\nVulnerable data flow analysis result:\n\nā†˜ļø `a-snippet` (at file line 10)\n\nā†˜ļø `snippet` (at file line 0)\n\n\n---\n\n", + }, + { + name: "No code flows", + severity: "Low", + finding: "Stack Trace Exposure", + fullDetails: "\n### Overview\nStack trace exposure is a type of security vulnerability that occurs when a program reveals\nsensitive information, such as the names and locations of internal files and variables,\nin error messages or other diagnostic output. This can happen when a program crashes or\nencounters an error, and the stack trace (a record of the program's call stack at the time\nof the error) is included in the output.", + expectedOutput: "\n## šŸŽÆ Static Application Security Testing (SAST) Vulnerability\n\t\n| Severity | Finding |\n| :--------------: | :---: |\n| Low | Stack Trace Exposure |\n\n---\n### Full description\n\n\n### Overview\nStack trace exposure is a type of security vulnerability that occurs when a program reveals\nsensitive information, such as the names and locations of internal files and variables,\nin error messages or other diagnostic output. This can happen when a program crashes or\nencounters an error, and the stack trace (a record of the program's call stack at the time\nof the error) is included in the output.\n", + }, + } + + so := &SimplifiedOutput{} + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + output := so.SastReviewContent(tc.severity, tc.finding, tc.fullDetails, tc.codeFlows) + assert.Equal(t, tc.expectedOutput, output) + }) + } +} diff --git a/utils/outputwriter/standardoutput_test.go b/utils/outputwriter/standardoutput_test.go index 291a675b0..5fdb45820 100644 --- a/utils/outputwriter/standardoutput_test.go +++ b/utils/outputwriter/standardoutput_test.go @@ -403,3 +403,137 @@ func TestStandardOutput_GetLicensesTableContent(t *testing.T) { writer := &StandardOutput{} testGetLicensesTableContent(t, writer) } + +func TestStandardOutput_ApplicableCveReviewContent(t *testing.T) { + testCases := []struct { + name string + severity, finding, fullDetails, cve, cveDetails, impactedDependency, remediation string + expectedOutput string + }{ + { + name: "Applicable CVE review comment content", + severity: "Critical", + finding: "The vulnerable function flask.Flask.run is called", + fullDetails: "The scanner checks whether the vulnerable `Development Server` of the `werkzeug` library is used by looking for calls to `werkzeug.serving.run_simple()`.", + cve: "CVE-2022-29361", + cveDetails: "cveDetails", + impactedDependency: "werkzeug:1.0.1", + remediation: "some remediation", + expectedOutput: "\n## šŸ“¦šŸ” Contextual Analysis CVE Vulnerability\n\n\n
\n\n| Severity | Impacted Dependency | Finding | CVE |\n| :--------------: | :---: | :---: | :---: |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableCriticalSeverity.png)
Critical | werkzeug:1.0.1 | The vulnerable function flask.Flask.run is called | CVE-2022-29361 |\n\n
\n\n
\n Description \n
\n\nThe scanner checks whether the vulnerable `Development Server` of the `werkzeug` library is used by looking for calls to `werkzeug.serving.run_simple()`.\n\n
\n\n
\n CVE details \n
\n\ncveDetails\n\n
\n\n\n
\n Remediation \n
\n\nsome remediation\n\n
\n\n", + }, + { + name: "No remediation", + severity: "Critical", + finding: "The vulnerable function flask.Flask.run is called", + fullDetails: "The scanner checks whether the vulnerable `Development Server` of the `werkzeug` library is used by looking for calls to `werkzeug.serving.run_simple()`.", + cve: "CVE-2022-29361", + cveDetails: "cveDetails", + impactedDependency: "werkzeug:1.0.1", + expectedOutput: "\n## šŸ“¦šŸ” Contextual Analysis CVE Vulnerability\n\n\n
\n\n| Severity | Impacted Dependency | Finding | CVE |\n| :--------------: | :---: | :---: | :---: |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableCriticalSeverity.png)
Critical | werkzeug:1.0.1 | The vulnerable function flask.Flask.run is called | CVE-2022-29361 |\n\n
\n\n
\n Description \n
\n\nThe scanner checks whether the vulnerable `Development Server` of the `werkzeug` library is used by looking for calls to `werkzeug.serving.run_simple()`.\n\n
\n\n
\n CVE details \n
\n\ncveDetails\n\n
\n\n", + }, + } + + so := &StandardOutput{} + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + output := so.ApplicableCveReviewContent(tc.severity, tc.finding, tc.fullDetails, tc.cve, tc.cveDetails, tc.impactedDependency, tc.remediation) + assert.Equal(t, tc.expectedOutput, output) + }) + } +} + +func TestStandardOutput_IacReviewContent(t *testing.T) { + testCases := []struct { + name string + severity, finding, fullDetails string + expectedOutput string + }{ + { + name: "Iac review comment content", + severity: "Medium", + finding: "Missing auto upgrade was detected", + fullDetails: "Resource `google_container_node_pool` should have `management.auto_upgrade=true`\n\nVulnerable example - \n```\nresource \"google_container_node_pool\" \"vulnerable_example\" {\n management {\n auto_upgrade = false\n }\n}\n```\n", + expectedOutput: "\n## šŸ› ļø Infrastructure as Code\n\n
\n\n| Severity | Finding |\n| :--------------: | :---: |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableMediumSeverity.png)
Medium | Missing auto upgrade was detected |\n\n
\n\n
\n Full description \n
\n\nResource `google_container_node_pool` should have `management.auto_upgrade=true`\n\nVulnerable example - \n```\nresource \"google_container_node_pool\" \"vulnerable_example\" {\n management {\n auto_upgrade = false\n }\n}\n```\n\n\n
\n\n", + }, + } + + so := &StandardOutput{} + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + output := so.IacReviewContent(tc.severity, tc.finding, tc.fullDetails) + assert.Equal(t, tc.expectedOutput, output) + }) + } +} + +func TestStandardOutput_SastReviewContent(t *testing.T) { + testCases := []struct { + name string + severity string + finding string + fullDetails string + expectedOutput string + codeFlows [][]formats.Location + }{ + { + name: "Sast review comment content", + severity: "Low", + finding: "Stack Trace Exposure", + fullDetails: "\n### Overview\nStack trace exposure is a type of security vulnerability that occurs when a program reveals\nsensitive information, such as the names and locations of internal files and variables,\nin error messages or other diagnostic output. This can happen when a program crashes or\nencounters an error, and the stack trace (a record of the program's call stack at the time\nof the error) is included in the output.", + codeFlows: [][]formats.Location{ + { + { + File: "file2", + StartLine: 1, + StartColumn: 2, + EndLine: 3, + EndColumn: 4, + Snippet: "other-snippet", + }, + { + File: "file", + StartLine: 0, + StartColumn: 0, + EndLine: 0, + EndColumn: 0, + Snippet: "snippet", + }, + }, + { + { + File: "file", + StartLine: 10, + StartColumn: 20, + EndLine: 10, + EndColumn: 30, + Snippet: "a-snippet", + }, + { + File: "file", + StartLine: 0, + StartColumn: 0, + EndLine: 0, + EndColumn: 0, + Snippet: "snippet", + }, + }, + }, + expectedOutput: "\n## šŸŽÆ Static Application Security Testing (SAST) Vulnerability \n\t\n
\n\n| Severity | Finding |\n| :--------------: | :---: |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableLowSeverity.png)
Low | Stack Trace Exposure |\n\n
\n\n
\n Full description \n
\n\n\n### Overview\nStack trace exposure is a type of security vulnerability that occurs when a program reveals\nsensitive information, such as the names and locations of internal files and variables,\nin error messages or other diagnostic output. This can happen when a program crashes or\nencounters an error, and the stack trace (a record of the program's call stack at the time\nof the error) is included in the output.\n\n
\n\n\n\n
\nCode Flows \n\n\n\n
\nVulnerable data flow analysis result \n
\n\nā†˜ļø `other-snippet` (at file2 line 1)\n\nā†˜ļø `snippet` (at file line 0)\n\n\n
\n\n\n\n
\nVulnerable data flow analysis result \n
\n\nā†˜ļø `a-snippet` (at file line 10)\n\nā†˜ļø `snippet` (at file line 0)\n\n\n
\n\n\n\n
\n\n", + }, + { + name: "No code flows", + severity: "Low", + finding: "Stack Trace Exposure", + fullDetails: "\n### Overview\nStack trace exposure is a type of security vulnerability that occurs when a program reveals\nsensitive information, such as the names and locations of internal files and variables,\nin error messages or other diagnostic output. This can happen when a program crashes or\nencounters an error, and the stack trace (a record of the program's call stack at the time\nof the error) is included in the output.", + expectedOutput: "\n## šŸŽÆ Static Application Security Testing (SAST) Vulnerability \n\t\n
\n\n| Severity | Finding |\n| :--------------: | :---: |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableLowSeverity.png)
Low | Stack Trace Exposure |\n\n
\n\n
\n Full description \n
\n\n\n### Overview\nStack trace exposure is a type of security vulnerability that occurs when a program reveals\nsensitive information, such as the names and locations of internal files and variables,\nin error messages or other diagnostic output. This can happen when a program crashes or\nencounters an error, and the stack trace (a record of the program's call stack at the time\nof the error) is included in the output.\n\n
\n\n", + }, + } + + so := &StandardOutput{} + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + output := so.SastReviewContent(tc.severity, tc.finding, tc.fullDetails, tc.codeFlows) + assert.Equal(t, tc.expectedOutput, output) + }) + } +} diff --git a/utils/params.go b/utils/params.go index 172c80df5..f828454b9 100644 --- a/utils/params.go +++ b/utils/params.go @@ -4,8 +4,6 @@ import ( "context" "errors" "fmt" - "github.com/jfrog/frogbot/utils/outputwriter" - xrutils "github.com/jfrog/jfrog-cli-core/v2/xray/utils" "net/http" "net/url" "os" @@ -13,6 +11,9 @@ import ( "strconv" "strings" + "github.com/jfrog/frogbot/utils/outputwriter" + xrutils "github.com/jfrog/jfrog-cli-core/v2/xray/utils" + "github.com/jfrog/build-info-go/utils" "github.com/jfrog/froggit-go/vcsclient" "github.com/jfrog/froggit-go/vcsutils" @@ -109,13 +110,14 @@ func (p *Project) setDefaultsIfNeeded() error { } type Scan struct { - IncludeAllVulnerabilities bool `yaml:"includeAllVulnerabilities,omitempty"` - FixableOnly bool `yaml:"fixableOnly,omitempty"` - FailOnSecurityIssues *bool `yaml:"failOnSecurityIssues,omitempty"` - MinSeverity string `yaml:"minSeverity,omitempty"` - AllowedLicenses []string `yaml:"allowedLicenses,omitempty"` - Projects []Project `yaml:"projects,omitempty"` - EmailDetails `yaml:",inline"` + IncludeAllVulnerabilities bool `yaml:"includeAllVulnerabilities,omitempty"` + FixableOnly bool `yaml:"fixableOnly,omitempty"` + FailOnSecurityIssues *bool `yaml:"failOnSecurityIssues,omitempty"` + AvoidPreviousPrCommentsDeletion bool `yaml:"avoidPreviousPrCommentsDeletion,omitempty"` + MinSeverity string `yaml:"minSeverity,omitempty"` + AllowedLicenses []string `yaml:"allowedLicenses,omitempty"` + Projects []Project `yaml:"projects,omitempty"` + EmailDetails `yaml:",inline"` } type EmailDetails struct { @@ -160,6 +162,11 @@ func (s *Scan) setDefaultsIfNeeded() (err error) { return } } + if !s.AvoidPreviousPrCommentsDeletion { + if s.AvoidPreviousPrCommentsDeletion, err = getBoolEnv(AvoidPreviousPrCommentsDeletionEnv, false); err != nil { + return + } + } if !s.FixableOnly { if s.FixableOnly, err = getBoolEnv(FixableOnlyEnv, false); err != nil { return diff --git a/utils/params_test.go b/utils/params_test.go index 84a89f1a8..ccf1c1acc 100644 --- a/utils/params_test.go +++ b/utils/params_test.go @@ -3,12 +3,13 @@ package utils import ( "errors" "fmt" - "github.com/jfrog/froggit-go/vcsclient" - "github.com/jfrog/jfrog-cli-core/v2/utils/config" "os" "path/filepath" "testing" + "github.com/jfrog/froggit-go/vcsclient" + "github.com/jfrog/jfrog-cli-core/v2/utils/config" + "github.com/jfrog/froggit-go/vcsutils" "github.com/stretchr/testify/assert" ) @@ -320,26 +321,27 @@ func TestExtractInstallationCommandFromEnv(t *testing.T) { func TestGenerateConfigAggregatorFromEnv(t *testing.T) { SetEnvAndAssert(t, map[string]string{ - JFrogUrlEnv: "", - jfrogArtifactoryUrlEnv: "http://127.0.0.1:8081/artifactory", - jfrogXrayUrlEnv: "http://127.0.0.1:8081/xray", - JFrogUserEnv: "admin", - JFrogPasswordEnv: "password", - BranchNameTemplateEnv: "branch-${BRANCH_NAME_HASH}", - CommitMessageTemplateEnv: "commit", - PullRequestTitleTemplateEnv: "pr-title", - InstallCommandEnv: "nuget restore", - UseWrapperEnv: "false", - RequirementsFileEnv: "requirements.txt", - WorkingDirectoryEnv: "a/b", - jfrogProjectEnv: "projectKey", - jfrogWatchesEnv: "watch-1, watch-2, watch-3", - DepsRepoEnv: "deps-remote", - IncludeAllVulnerabilitiesEnv: "true", - FailOnSecurityIssuesEnv: "false", - MinSeverityEnv: "medium", - FixableOnlyEnv: "true", - AllowedLicensesEnv: "MIT, Apache-2.0", + JFrogUrlEnv: "", + jfrogArtifactoryUrlEnv: "http://127.0.0.1:8081/artifactory", + jfrogXrayUrlEnv: "http://127.0.0.1:8081/xray", + JFrogUserEnv: "admin", + JFrogPasswordEnv: "password", + BranchNameTemplateEnv: "branch-${BRANCH_NAME_HASH}", + CommitMessageTemplateEnv: "commit", + PullRequestTitleTemplateEnv: "pr-title", + InstallCommandEnv: "nuget restore", + UseWrapperEnv: "false", + RequirementsFileEnv: "requirements.txt", + WorkingDirectoryEnv: "a/b", + jfrogProjectEnv: "projectKey", + jfrogWatchesEnv: "watch-1, watch-2, watch-3", + DepsRepoEnv: "deps-remote", + IncludeAllVulnerabilitiesEnv: "true", + AvoidPreviousPrCommentsDeletionEnv: "true", + FailOnSecurityIssuesEnv: "false", + MinSeverityEnv: "medium", + FixableOnlyEnv: "true", + AllowedLicensesEnv: "MIT, Apache-2.0", }) defer func() { assert.NoError(t, SanitizeEnv()) @@ -446,11 +448,13 @@ func TestFrogbotConfigAggregator_unmarshalFrogbotConfigYaml(t *testing.T) { secondRepo := configAggregator[1] assert.Equal(t, "mvn-repo", secondRepo.RepoName) assert.Equal(t, []string{"dev"}, secondRepo.Branches) + assert.False(t, secondRepo.AvoidPreviousPrCommentsDeletion) thirdRepo := configAggregator[2] assert.Equal(t, "pip-repo", thirdRepo.RepoName) assert.Equal(t, []string{"test"}, thirdRepo.Branches) assert.True(t, *thirdRepo.FailOnSecurityIssues) assert.False(t, thirdRepo.IncludeAllVulnerabilities) + assert.True(t, thirdRepo.AvoidPreviousPrCommentsDeletion) thirdRepoProject := thirdRepo.Projects[0] assert.Equal(t, "requirements.txt", thirdRepoProject.PipRequirementsFile) assert.ElementsMatch(t, []string{"a/b", "b/c"}, thirdRepoProject.WorkingDirs) diff --git a/utils/reviewcomment.go b/utils/reviewcomment.go index 76c3df852..a306934d2 100644 --- a/utils/reviewcomment.go +++ b/utils/reviewcomment.go @@ -24,19 +24,79 @@ const ( ApplicableComment ReviewCommentType = "Applicable" IacComment ReviewCommentType = "Iac" SastComment ReviewCommentType = "Sast" - - CommentId = "FrogbotReviewComment" ) -func AddReviewComments(repo *Repository, pullRequestID int, client vcsclient.VcsClient, issues *IssuesCollection) (err error) { - if err = deleteOldReviewComments(repo, pullRequestID, client); err != nil { - err = errors.New("couldn't delete pull request review comment: " + err.Error()) +func HandlePullRequestCommentsAfterScan(issues *IssuesCollection, repo *Repository, client vcsclient.VcsClient, pullRequestID int) (err error) { + if !repo.Params.AvoidPreviousPrCommentsDeletion { + log.Debug("Looking for an existing Frogbot pull request comment. Deleting it if it exists...") + // Delete previous PR regular comments, if exists (not related to location of a change) + if err = DeleteExistingPullRequestComments(repo, client); err != nil { + err = errors.New("couldn't delete pull request comment: " + err.Error()) + return + } + // Delete previous PR review comments, if exists (related to location of a change) + if err = DeleteExistingPullRequestReviewComments(repo, pullRequestID, client); err != nil { + err = errors.New("couldn't delete pull request review comment: " + err.Error()) + return + } + } + + // Add summary (SCA, license) scan comment + if err = client.AddPullRequestComment(context.Background(), repo.RepoOwner, repo.RepoName, createPullRequestComment(issues, repo.OutputWriter), pullRequestID); err != nil { + err = errors.New("couldn't add pull request comment: " + err.Error()) return } - if err = deleteOldFallbackComments(repo, pullRequestID, client); err != nil { - err = errors.New("couldn't delete pull request comment: " + err.Error()) + // Handle review comments at the pull request + if err = addReviewComments(repo, pullRequestID, client, issues); err != nil { + err = errors.New("couldn't add review comments: " + err.Error()) return } + return +} + +// Delete existing pull request regular comments (Summary, Fallback review comments) +func DeleteExistingPullRequestComments(repository *Repository, client vcsclient.VcsClient) error { + prDetails := repository.PullRequestDetails + comments, err := 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()) + } + // Previous Fallback review comments + commentsToDelete := getFrogbotReviewComments(comments) + // Previous Summary comments + for _, comment := range comments { + if repository.OutputWriter.IsFrogbotResultComment(comment.Content) { + commentsToDelete = append(commentsToDelete, comment) + } + } + // Delete + if len(commentsToDelete) > 0 { + for _, commentToDelete := range commentsToDelete { + if err = client.DeletePullRequestComment(context.Background(), prDetails.Target.Owner, prDetails.Target.Repository, int(prDetails.ID), int(commentToDelete.ID)); err != nil { + return err + } + } + } + return err +} + +func createPullRequestComment(issues *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 addReviewComments(repo *Repository, pullRequestID int, client vcsclient.VcsClient, issues *IssuesCollection) (err error) { commentsToAdd := getNewReviewComments(repo, issues) if len(commentsToAdd) == 0 { return @@ -55,8 +115,9 @@ func AddReviewComments(repo *Repository, pullRequestID int, client vcsclient.Vcs return } -func deleteOldReviewComments(repo *Repository, pullRequestID int, client vcsclient.VcsClient) (err error) { - // Get all comments in PR +// Delete existing pull request review comments (Applicable, Sast, Iac) +func DeleteExistingPullRequestReviewComments(repo *Repository, pullRequestID int, client vcsclient.VcsClient) (err error) { + // Get all review comments in PR var existingComments []vcsclient.CommentInfo if existingComments, err = client.ListPullRequestReviewComments(context.Background(), repo.RepoOwner, repo.RepoName, pullRequestID); err != nil { err = errors.New("couldn't list existing review comments: " + err.Error()) @@ -72,28 +133,9 @@ func deleteOldReviewComments(repo *Repository, pullRequestID int, client vcsclie return } -func deleteOldFallbackComments(repo *Repository, pullRequestID int, client vcsclient.VcsClient) (err error) { - // Get all comments in PR - existingComments, err := GetSortedPullRequestComments(client, repo.RepoOwner, repo.RepoName, pullRequestID) - if err != nil { - err = errors.New("couldn't list existing regular comments: " + err.Error()) - return - } - // Delete old review comments - if len(existingComments) > 0 { - for _, commentToDelete := range getFrogbotReviewComments(existingComments) { - if err = client.DeletePullRequestComment(context.Background(), repo.RepoOwner, repo.RepoName, pullRequestID, int(commentToDelete.ID)); err != nil { - err = errors.New("couldn't delete pull request regular comment: " + err.Error()) - return - } - } - } - return -} - func getFrogbotReviewComments(existingComments []vcsclient.CommentInfo) (reviewComments []vcsclient.CommentInfo) { for _, comment := range existingComments { - if strings.Contains(comment.Content, CommentId) { + if strings.Contains(comment.Content, outputwriter.ReviewCommentId) { log.Debug("Deleting comment id:", comment.ID) reviewComments = append(reviewComments, comment) } @@ -102,8 +144,7 @@ func getFrogbotReviewComments(existingComments []vcsclient.CommentInfo) (reviewC } func getRegularCommentContent(comment ReviewComment) string { - content := outputwriter.MarkdownComment(CommentId) - return content + outputwriter.GetLocationDescription(comment.Location) + comment.CommentInfo.Content + return outputwriter.MarkdownComment(outputwriter.ReviewCommentId) + outputwriter.GetLocationDescription(comment.Location) + comment.CommentInfo.Content } func getNewReviewComments(repo *Repository, issues *IssuesCollection) (commentsToAdd []ReviewComment) { @@ -119,11 +160,11 @@ func getNewReviewComments(repo *Repository, issues *IssuesCollection) (commentsT } } for _, iac := range issues.Iacs { - commentsToAdd = append(commentsToAdd, generateReviewComment(IacComment, iac.Location, generateReviewCommentContent(IacComment, iac, writer))) + commentsToAdd = append(commentsToAdd, generateReviewComment(IacComment, iac.Location, generateSourceCodeReviewContent(IacComment, iac, writer))) } for _, sast := range issues.Sast { - commentsToAdd = append(commentsToAdd, generateReviewComment(SastComment, sast.Location, generateReviewCommentContent(SastComment, sast, writer))) + commentsToAdd = append(commentsToAdd, generateReviewComment(SastComment, sast.Location, generateSourceCodeReviewContent(SastComment, sast, writer))) } return } @@ -142,13 +183,12 @@ func generateReviewComment(commentType ReviewCommentType, location formats.Locat } -func generateApplicabilityReviewContent(issue formats.Evidence, relatedCve formats.CveRow, relatedVulnerability formats.VulnerabilityOrViolationRow, writer outputwriter.OutputWriter) (content string) { - content = outputwriter.MarkdownComment(CommentId) +func generateApplicabilityReviewContent(issue formats.Evidence, relatedCve formats.CveRow, relatedVulnerability formats.VulnerabilityOrViolationRow, writer outputwriter.OutputWriter) string { remediation := "" if relatedVulnerability.JfrogResearchInformation != nil { remediation = relatedVulnerability.JfrogResearchInformation.Remediation } - content += writer.ApplicableCveReviewContent( + return outputwriter.GenerateReviewCommentContent(writer.ApplicableCveReviewContent( relatedVulnerability.Severity, issue.Reason, relatedCve.Applicability.ScannerDescription, @@ -156,30 +196,25 @@ func generateApplicabilityReviewContent(issue formats.Evidence, relatedCve forma relatedVulnerability.Summary, fmt.Sprintf("%s:%s", relatedVulnerability.ImpactedDependencyName, relatedVulnerability.ImpactedDependencyVersion), remediation, - ) - content += writer.Footer() - return + ), writer) } -func generateReviewCommentContent(commentType ReviewCommentType, issue formats.SourceCodeRow, writer outputwriter.OutputWriter) (content string) { - content = outputwriter.MarkdownComment(CommentId) +func generateSourceCodeReviewContent(commentType ReviewCommentType, issue formats.SourceCodeRow, writer outputwriter.OutputWriter) (content string) { switch commentType { case IacComment: - content += writer.IacReviewContent( + return outputwriter.GenerateReviewCommentContent(writer.IacReviewContent( issue.Severity, issue.Finding, issue.ScannerDescription, - ) + ), writer) case SastComment: - content += writer.SastReviewContent( + return outputwriter.GenerateReviewCommentContent(writer.SastReviewContent( issue.Severity, issue.Finding, issue.ScannerDescription, issue.CodeFlow, - ) + ), writer) } - - content += writer.Footer() return } diff --git a/utils/reviewcomment_test.go b/utils/reviewcomment_test.go new file mode 100644 index 000000000..abc637d40 --- /dev/null +++ b/utils/reviewcomment_test.go @@ -0,0 +1,370 @@ +package utils + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/jfrog/frogbot/utils/outputwriter" + "github.com/jfrog/froggit-go/vcsclient" + "github.com/jfrog/froggit-go/vcsutils" + "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" + "github.com/jfrog/jfrog-cli-core/v2/xray/formats" + "github.com/stretchr/testify/assert" +) + +func TestCreatePullRequestMessageNoVulnerabilities(t *testing.T) { + vulnerabilities := []formats.VulnerabilityOrViolationRow{} + message := createPullRequestComment(&IssuesCollection{Vulnerabilities: vulnerabilities}, &outputwriter.StandardOutput{}) + + expectedMessageByte, err := os.ReadFile(filepath.Join("..", "testdata", "messages", "novulnerabilities.md")) + assert.NoError(t, err) + expectedMessage := strings.ReplaceAll(string(expectedMessageByte), "\r\n", "\n") + assert.Equal(t, expectedMessage, message) + + outputWriter := &outputwriter.StandardOutput{} + outputWriter.SetVcsProvider(vcsutils.GitLab) + message = createPullRequestComment(&IssuesCollection{Vulnerabilities: vulnerabilities}, outputWriter) + + expectedMessageByte, err = os.ReadFile(filepath.Join("..", "testdata", "messages", "novulnerabilitiesMR.md")) + assert.NoError(t, err) + expectedMessage = strings.ReplaceAll(string(expectedMessageByte), "\r\n", "\n") + assert.Equal(t, expectedMessage, message) +} + +func TestCreatePullRequestComment(t *testing.T) { + vulnerabilities := []formats.VulnerabilityOrViolationRow{ + { + Summary: "Summary XRAY-122345", + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "High"}, + ImpactedDependencyName: "github.com/nats-io/nats-streaming-server", + ImpactedDependencyVersion: "v0.21.0", + Components: []formats.ComponentRow{ + { + Name: "github.com/nats-io/nats-streaming-server", + Version: "v0.21.0", + }, + }, + }, + Applicable: "Undetermined", + FixedVersions: []string{"[0.24.1]"}, + IssueId: "XRAY-122345", + Cves: []formats.CveRow{{}}, + }, + { + Summary: "Summary", + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "High"}, + ImpactedDependencyName: "github.com/mholt/archiver/v3", + ImpactedDependencyVersion: "v3.5.1", + Components: []formats.ComponentRow{ + { + Name: "github.com/mholt/archiver/v3", + Version: "v3.5.1", + }, + }, + }, + Applicable: "Undetermined", + Cves: []formats.CveRow{}, + }, + { + Summary: "Summary CVE-2022-26652", + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "Medium"}, + ImpactedDependencyName: "github.com/nats-io/nats-streaming-server", + ImpactedDependencyVersion: "v0.21.0", + Components: []formats.ComponentRow{ + { + Name: "github.com/nats-io/nats-streaming-server", + Version: "v0.21.0", + }, + }, + }, + Applicable: "Undetermined", + FixedVersions: []string{"[0.24.3]"}, + Cves: []formats.CveRow{{Id: "CVE-2022-26652"}}, + }, + } + licenses := []formats.LicenseRow{ + { + LicenseKey: "Apache-2.0", + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "High", SeverityNumValue: 13}, + ImpactedDependencyName: "minimatch", + ImpactedDependencyVersion: "1.2.3", + Components: []formats.ComponentRow{ + { + Name: "root", + Version: "1.0.0", + }, + { + Name: "minimatch", + Version: "1.2.3", + }, + }, + }, + }, + } + + writerOutput := &outputwriter.StandardOutput{} + writerOutput.SetJasOutputFlags(true, true) + message := createPullRequestComment(&IssuesCollection{Vulnerabilities: vulnerabilities, Licenses: licenses}, writerOutput) + + expectedMessage := "
\n\n[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/vulnerabilitiesBannerPR.png)](https://github.com/jfrog/frogbot#readme)\n\n
\n\n\n## šŸ“¦ Vulnerable Dependencies\n\n### āœļø Summary\n\n
\n\n| SEVERITY | CONTEXTUAL ANALYSIS | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS | CVES |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | :---------------------------------: | \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableHighSeverity.png)
High | Undetermined | github.com/nats-io/nats-streaming-server:v0.21.0 | github.com/nats-io/nats-streaming-server:v0.21.0 | [0.24.1] | - |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableHighSeverity.png)
High | Undetermined | github.com/mholt/archiver/v3:v3.5.1 | github.com/mholt/archiver/v3:v3.5.1 | - | - |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableMediumSeverity.png)
Medium | Undetermined | github.com/nats-io/nats-streaming-server:v0.21.0 | github.com/nats-io/nats-streaming-server:v0.21.0 | [0.24.3] | CVE-2022-26652 |\n\n
\n\n## šŸ”¬ Research Details\n\n
\n [ XRAY-122345 ] github.com/nats-io/nats-streaming-server v0.21.0 \n
\n\n**Description:**\nSummary XRAY-122345\n\n\n
\n\n\n
\n github.com/mholt/archiver/v3 v3.5.1 \n
\n\n**Description:**\nSummary\n\n\n
\n\n\n
\n [ CVE-2022-26652 ] github.com/nats-io/nats-streaming-server v0.21.0 \n
\n\n**Description:**\nSummary CVE-2022-26652\n\n\n
\n\n\n## āš–ļø Violated Licenses \n\n
\n\n\n| LICENSE | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | \n| :---------------------: | :----------------------------------: | :-----------------------------------: | \n| Apache-2.0 | root 1.0.0
minimatch 1.2.3 | minimatch 1.2.3 |\n\n
\n\n\n---\n
\n\n[šŸø JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n
" + assert.Equal(t, expectedMessage, message) + + writerOutput.SetVcsProvider(vcsutils.GitLab) + message = createPullRequestComment(&IssuesCollection{Vulnerabilities: vulnerabilities}, writerOutput) + expectedMessage = "
\n\n[![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/vulnerabilitiesBannerMR.png)](https://github.com/jfrog/frogbot#readme)\n\n
\n\n\n## šŸ“¦ Vulnerable Dependencies\n\n### āœļø Summary\n\n
\n\n| SEVERITY | CONTEXTUAL ANALYSIS | DIRECT DEPENDENCIES | IMPACTED DEPENDENCY | FIXED VERSIONS | CVES |\n| :---------------------: | :----------------------------------: | :----------------------------------: | :-----------------------------------: | :---------------------------------: | :---------------------------------: | \n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableHighSeverity.png)
High | Undetermined | github.com/nats-io/nats-streaming-server:v0.21.0 | github.com/nats-io/nats-streaming-server:v0.21.0 | [0.24.1] | - |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableHighSeverity.png)
High | Undetermined | github.com/mholt/archiver/v3:v3.5.1 | github.com/mholt/archiver/v3:v3.5.1 | - | - |\n| ![](https://raw.githubusercontent.com/jfrog/frogbot/master/resources/v2/applicableMediumSeverity.png)
Medium | Undetermined | github.com/nats-io/nats-streaming-server:v0.21.0 | github.com/nats-io/nats-streaming-server:v0.21.0 | [0.24.3] | CVE-2022-26652 |\n\n
\n\n## šŸ”¬ Research Details\n\n
\n [ XRAY-122345 ] github.com/nats-io/nats-streaming-server v0.21.0 \n
\n\n**Description:**\nSummary XRAY-122345\n\n\n
\n\n\n
\n github.com/mholt/archiver/v3 v3.5.1 \n
\n\n**Description:**\nSummary\n\n\n
\n\n\n
\n [ CVE-2022-26652 ] github.com/nats-io/nats-streaming-server v0.21.0 \n
\n\n**Description:**\nSummary CVE-2022-26652\n\n\n
\n\n\n---\n
\n\n[šŸø JFrog Frogbot](https://github.com/jfrog/frogbot#readme)\n\n
" + assert.Equal(t, expectedMessage, message) +} + +func TestGetFrogbotReviewComments(t *testing.T) { + testCases := []struct { + name string + existingComments []vcsclient.CommentInfo + expectedOutput []vcsclient.CommentInfo + }{ + { + name: "No frogbot comments", + existingComments: []vcsclient.CommentInfo{ + { + Content: outputwriter.FrogbotTitlePrefix, + }, + { + Content: "some comment text" + outputwriter.MarkdownComment("with hidden comment"), + }, + { + Content: outputwriter.CommentGeneratedByFrogbot, + }, + }, + expectedOutput: []vcsclient.CommentInfo{}, + }, + { + name: "With frogbot comments", + existingComments: []vcsclient.CommentInfo{ + { + Content: outputwriter.FrogbotTitlePrefix, + }, + { + Content: outputwriter.MarkdownComment(outputwriter.ReviewCommentId) + "A Frogbot review comment", + }, + { + Content: "some comment text" + outputwriter.MarkdownComment("with hidden comment"), + }, + { + Content: outputwriter.ReviewCommentId, + }, + { + Content: outputwriter.CommentGeneratedByFrogbot, + }, + }, + expectedOutput: []vcsclient.CommentInfo{ + { + Content: outputwriter.MarkdownComment(outputwriter.ReviewCommentId) + "A Frogbot review comment", + }, + { + Content: outputwriter.ReviewCommentId, + }, + }, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + output := getFrogbotReviewComments(tc.existingComments) + assert.ElementsMatch(t, tc.expectedOutput, output) + }) + } +} + +func TestGetNewReviewComments(t *testing.T) { + repo := &Repository{OutputWriter: &outputwriter.StandardOutput{}} + testCases := []struct { + name string + issues *IssuesCollection + expectedOutput []ReviewComment + }{ + { + name: "No issues for review comments", + issues: &IssuesCollection{ + Vulnerabilities: []formats.VulnerabilityOrViolationRow{ + { + Summary: "summary-2", + Applicable: "Applicable", + IssueId: "XRAY-2", + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "low"}, + ImpactedDependencyName: "component-C", + }, + Cves: []formats.CveRow{{Id: "CVE-2023-4321"}}, + Technology: coreutils.Npm, + }, + }, + Secrets: []formats.SourceCodeRow{ + { + SeverityDetails: formats.SeverityDetails{ + Severity: "High", + SeverityNumValue: 13, + }, + Finding: "Secret", + Location: formats.Location{ + File: "index.js", + StartLine: 5, + StartColumn: 6, + EndLine: 7, + EndColumn: 8, + Snippet: "access token exposed", + }, + }, + }, + }, + expectedOutput: []ReviewComment{}, + }, + { + name: "With issues for review comments", + issues: &IssuesCollection{ + Vulnerabilities: []formats.VulnerabilityOrViolationRow{ + { + Summary: "summary-2", + Applicable: "Applicable", + IssueId: "XRAY-2", + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "Low"}, + ImpactedDependencyName: "component-C", + }, + Cves: []formats.CveRow{{Id: "CVE-2023-4321", Applicability: &formats.Applicability{Status: "Applicable", Evidence: []formats.Evidence{{Location: formats.Location{File: "file1", StartLine: 1, StartColumn: 10, EndLine: 2, EndColumn: 11, Snippet: "snippet"}}}}}}, + Technology: coreutils.Npm, + }, + }, + Iacs: []formats.SourceCodeRow{ + { + SeverityDetails: formats.SeverityDetails{ + Severity: "High", + SeverityNumValue: 13, + }, + Finding: "Missing auto upgrade was detected", + Location: formats.Location{ + File: "file1", + StartLine: 1, + StartColumn: 10, + EndLine: 2, + EndColumn: 11, + Snippet: "aws-violation", + }, + }, + }, + Sast: []formats.SourceCodeRow{ + { + SeverityDetails: formats.SeverityDetails{ + Severity: "High", + SeverityNumValue: 13, + }, + Finding: "XSS Vulnerability", + Location: formats.Location{ + File: "file1", + StartLine: 1, + StartColumn: 10, + EndLine: 2, + EndColumn: 11, + Snippet: "snippet", + }, + }, + }, + }, + expectedOutput: []ReviewComment{ + { + Location: formats.Location{ + File: "file1", + StartLine: 1, + StartColumn: 10, + EndLine: 2, + EndColumn: 11, + Snippet: "snippet", + }, + Type: ApplicableComment, + CommentInfo: vcsclient.PullRequestComment{ + CommentInfo: vcsclient.CommentInfo{ + Content: outputwriter.GenerateReviewCommentContent(repo.ApplicableCveReviewContent("Low", "", "", "CVE-2023-4321", "summary-2", "component-C:", ""), repo.OutputWriter), + }, + PullRequestDiff: vcsclient.PullRequestDiff{ + OriginalFilePath: "file1", + OriginalStartLine: 1, + OriginalStartColumn: 10, + OriginalEndLine: 2, + OriginalEndColumn: 11, + NewFilePath: "file1", + NewStartLine: 1, + NewStartColumn: 10, + NewEndLine: 2, + NewEndColumn: 11, + }, + }, + }, + { + Location: formats.Location{ + File: "file1", + StartLine: 1, + StartColumn: 10, + EndLine: 2, + EndColumn: 11, + Snippet: "aws-violation", + }, + Type: IacComment, + CommentInfo: vcsclient.PullRequestComment{ + CommentInfo: vcsclient.CommentInfo{ + Content: outputwriter.GenerateReviewCommentContent(repo.IacReviewContent("High", "Missing auto upgrade was detected", ""), repo.OutputWriter), + }, + PullRequestDiff: vcsclient.PullRequestDiff{ + OriginalFilePath: "file1", + OriginalStartLine: 1, + OriginalStartColumn: 10, + OriginalEndLine: 2, + OriginalEndColumn: 11, + NewFilePath: "file1", + NewStartLine: 1, + NewStartColumn: 10, + NewEndLine: 2, + NewEndColumn: 11, + }, + }, + }, + { + Location: formats.Location{ + File: "file1", + StartLine: 1, + StartColumn: 10, + EndLine: 2, + EndColumn: 11, + Snippet: "snippet", + }, + Type: SastComment, + CommentInfo: vcsclient.PullRequestComment{ + CommentInfo: vcsclient.CommentInfo{ + Content: outputwriter.GenerateReviewCommentContent(repo.SastReviewContent("High", "XSS Vulnerability", "", [][]formats.Location{}), repo.OutputWriter), + }, + PullRequestDiff: vcsclient.PullRequestDiff{ + OriginalFilePath: "file1", + OriginalStartLine: 1, + OriginalStartColumn: 10, + OriginalEndLine: 2, + OriginalEndColumn: 11, + NewFilePath: "file1", + NewStartLine: 1, + NewStartColumn: 10, + NewEndLine: 2, + NewEndColumn: 11, + }, + }, + }, + }, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + output := getNewReviewComments(repo, tc.issues) + assert.ElementsMatch(t, tc.expectedOutput, output) + }) + } +} diff --git a/utils/testsutils.go b/utils/testsutils.go index 6b3740caf..2fe2a8822 100644 --- a/utils/testsutils.go +++ b/utils/testsutils.go @@ -6,10 +6,8 @@ import ( goGitConfig "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing/object" biutils "github.com/jfrog/build-info-go/utils" - "github.com/jfrog/gofrog/datastructures" "github.com/jfrog/jfrog-cli-core/v2/utils/config" "github.com/jfrog/jfrog-client-go/utils/io/fileutils" - "github.com/owenrumney/go-sarif/v2/sarif" "os" "path/filepath" "strings" @@ -107,40 +105,3 @@ func CreateDotGitWithCommit(t *testing.T, wd, port string, repositoriesPath ...s assert.NoError(t, err) } } - -func GetRunWithDummyResults(results ...*sarif.Result) *sarif.Run { - run := sarif.NewRunWithInformationURI("", "") - ids := datastructures.MakeSet[string]() - for _, result := range results { - if !ids.Exists(*result.RuleID) { - run.Tool.Driver.Rules = append(run.Tool.Driver.Rules, sarif.NewRule(*result.RuleID)) - ids.Add(*result.RuleID) - } - } - return run.WithResults(results) -} - -func GetDummyPassingResult(ruleId string) *sarif.Result { - kind := "pass" - return &sarif.Result{ - Kind: &kind, - RuleID: &ruleId, - } -} - -func GetDummyResultWithOneLocation(fileName string, startLine, startCol int, snippet, ruleId string, level string) *sarif.Result { - return &sarif.Result{ - Locations: []*sarif.Location{ - { - PhysicalLocation: &sarif.PhysicalLocation{ - ArtifactLocation: &sarif.ArtifactLocation{URI: &fileName}, - Region: &sarif.Region{ - StartLine: &startLine, - StartColumn: &startCol, - Snippet: &sarif.ArtifactContent{Text: &snippet}}}, - }, - }, - Level: &level, - RuleID: &ruleId, - } -} diff --git a/utils/utils.go b/utils/utils.go index a65f7560e..15013017d 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -160,9 +160,8 @@ func ReportUsageOnCommand(commandName string, serverDetails *config.ServerDetail } reporter.Report(reports...) return func() { - if err = reporter.WaitForResponses(); err != nil { - log.Debug(err.Error()) - } + // Ignoring errors on purpose, we don't want to confuse the user with errors on usage reporting. + _ = reporter.WaitForResponses() } } diff --git a/utils/utils_test.go b/utils/utils_test.go index 9146d6476..bb86b0c97 100644 --- a/utils/utils_test.go +++ b/utils/utils_test.go @@ -9,6 +9,8 @@ import ( "github.com/jfrog/jfrog-cli-core/v2/utils/config" "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" "github.com/jfrog/jfrog-cli-core/v2/xray/formats" + "github.com/jfrog/jfrog-cli-core/v2/xray/utils" + "github.com/owenrumney/go-sarif/v2/sarif" "github.com/stretchr/testify/assert" ) @@ -374,3 +376,47 @@ func TestTechArrayToString(t *testing.T) { }) } } + +func TestPrepareRunsForGithubReport(t *testing.T) { + testCases := []struct { + run *sarif.Run + expectedOutput *sarif.Run + }{ + { + run: utils.CreateRunWithDummyResults(), + expectedOutput: sarif.NewRunWithInformationURI(sarifToolName, sarifToolUrl), + }, + { + run: sarif.NewRunWithInformationURI("other tool", "other url").WithResults([]*sarif.Result{ + utils.CreateResultWithOneLocation("file://root/dir/file", 0, 0, 0, 0, "snippet", "rule", "level"), + }).WithInvocations([]*sarif.Invocation{sarif.NewInvocation().WithWorkingDirectory(sarif.NewSimpleArtifactLocation("root/dir"))}), + expectedOutput: sarif.NewRunWithInformationURI(sarifToolName, sarifToolUrl).WithResults([]*sarif.Result{ + utils.CreateResultWithOneLocation("file", 0, 0, 0, 0, "snippet", "rule", "level"), + }).WithInvocations([]*sarif.Invocation{sarif.NewInvocation().WithWorkingDirectory(sarif.NewSimpleArtifactLocation("root/dir"))}), + }, + { + run: sarif.NewRunWithInformationURI("other tool", "other url").WithResults([]*sarif.Result{ + utils.CreateResultWithLocations("findings", "rule", "level", + utils.CreateLocation("file://root/dir/file", 0, 0, 0, 0, "snippet"), + utils.CreateLocation("file://root/dir/dir2/file2", 1, 1, 1, 1, "snippet2"), + ).WithCodeFlows([]*sarif.CodeFlow{utils.CreateCodeFlow(utils.CreateThreadFlow( + utils.CreateLocation("file://root/dir/other/file", 2, 2, 2, 2, "other"), + utils.CreateLocation("file://root/dir/file", 0, 0, 0, 0, "snippet"), + ))}), + }).WithInvocations([]*sarif.Invocation{sarif.NewInvocation().WithWorkingDirectory(sarif.NewSimpleArtifactLocation("root/dir"))}), + expectedOutput: sarif.NewRunWithInformationURI(sarifToolName, sarifToolUrl).WithResults([]*sarif.Result{ + utils.CreateResultWithLocations("findings", "rule", "level", + utils.CreateLocation("file", 0, 0, 0, 0, "snippet"), + utils.CreateLocation("dir2/file2", 1, 1, 1, 1, "snippet2"), + ).WithCodeFlows([]*sarif.CodeFlow{utils.CreateCodeFlow(utils.CreateThreadFlow( + utils.CreateLocation("other/file", 2, 2, 2, 2, "other"), + utils.CreateLocation("file", 0, 0, 0, 0, "snippet"), + ))}), + }).WithInvocations([]*sarif.Invocation{sarif.NewInvocation().WithWorkingDirectory(sarif.NewSimpleArtifactLocation("root/dir"))}), + }, + } + for _, tc := range testCases { + prepareRunsForGithubReport([]*sarif.Run{tc.run}) + assert.Equal(t, tc.expectedOutput, tc.run) + } +}