Skip to content

Commit

Permalink
Improve pull request comment and code scanning view (#228)
Browse files Browse the repository at this point in the history
  • Loading branch information
@omerzi
omerzi authored Jan 29, 2023
1 parent b2fabc2 commit 80d5c27
Show file tree
Hide file tree
Showing 14 changed files with 195 additions and 318 deletions.
32 changes: 15 additions & 17 deletions commands/createfixpullrequests.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,21 +60,19 @@ func (cfp *CreateFixPullRequestsCmd) scanAndFixRepository(repoConfig *utils.Frog
for projectIndex, project := range repoConfig.Projects {
projectFullPathWorkingDirs := getFullPathWorkingDirs(&repoConfig.Projects[projectIndex], baseWd)
for _, fullPathWd := range projectFullPathWorkingDirs {
scanResults, err := cfp.scan(project, &repoConfig.Server, xrayScanParams, *repoConfig.FailOnSecurityIssues, fullPathWd)
scanResults, isMultipleRoots, err := cfp.scan(project, &repoConfig.Server, xrayScanParams, *repoConfig.FailOnSecurityIssues, fullPathWd)
if err != nil {
return err
}

// Upload scan results to the relevant Git provider code scanning UI
scanResults = utils.SimplifyScanResults(scanResults)
err = utils.UploadScanToGitProvider(scanResults, repoConfig, branch, client)
err = utils.UploadScanToGitProvider(scanResults, repoConfig, branch, client, isMultipleRoots)
if err != nil {
log.Warn(err)
}

// Fix and create PRs
relativeCurrentWd := utils.GetRelativeWd(fullPathWd, baseWd)
if err = cfp.fixImpactedPackagesAndCreatePRs(project, &repoConfig.Git, branch, client, scanResults, relativeCurrentWd); err != nil {
if err = cfp.fixImpactedPackagesAndCreatePRs(project, &repoConfig.Git, branch, client, scanResults, relativeCurrentWd, isMultipleRoots); err != nil {
return err
}
}
Expand All @@ -84,19 +82,19 @@ func (cfp *CreateFixPullRequestsCmd) scanAndFixRepository(repoConfig *utils.Frog

// Audit the dependencies of the current commit.
func (cfp *CreateFixPullRequestsCmd) scan(project utils.Project, server *coreconfig.ServerDetails, xrayScanParams services.XrayGraphScanParams,
failOnSecurityIssues bool, currentWorkingDir string) ([]services.ScanResponse, error) {
failOnSecurityIssues bool, currentWorkingDir string) ([]services.ScanResponse, bool, error) {
// Audit commit code
scanResults, err := runInstallAndAudit(xrayScanParams, &project, server, failOnSecurityIssues, currentWorkingDir)
scanResults, isMultipleRoots, err := runInstallAndAudit(xrayScanParams, &project, server, failOnSecurityIssues, currentWorkingDir)
if err != nil {
return nil, err
return nil, false, err
}
log.Info("Xray scan completed")
return scanResults, nil
return scanResults, isMultipleRoots, nil
}

func (cfp *CreateFixPullRequestsCmd) fixImpactedPackagesAndCreatePRs(project utils.Project, repoGitParams *utils.Git, branch string,
client vcsclient.VcsClient, scanResults []services.ScanResponse, currentWd string) (err error) {
fixVersionsMap, err := cfp.createFixVersionsMap(&project, scanResults)
client vcsclient.VcsClient, scanResults []services.ScanResponse, currentWd string, isMultipleRoots bool) (err error) {
fixVersionsMap, err := cfp.createFixVersionsMap(&project, scanResults, isMultipleRoots)
if err != nil {
return err
}
Expand Down Expand Up @@ -163,11 +161,11 @@ func (cfp *CreateFixPullRequestsCmd) fixImpactedPackagesAndCreatePRs(project uti
}

// Create fixVersionMap - a map between impacted packages and their fix version
func (cfp *CreateFixPullRequestsCmd) createFixVersionsMap(project *utils.Project, scanResults []services.ScanResponse) (map[string]*FixVersionInfo, error) {
func (cfp *CreateFixPullRequestsCmd) createFixVersionsMap(project *utils.Project, scanResults []services.ScanResponse, isMultipleRoots bool) (map[string]*FixVersionInfo, error) {
fixVersionsMap := map[string]*FixVersionInfo{}
for _, scanResult := range scanResults {
if len(scanResult.Vulnerabilities) > 0 {
vulnerabilities, err := xrayutils.PrepareVulnerabilities(scanResult.Vulnerabilities, false)
vulnerabilities, err := xrayutils.PrepareVulnerabilities(scanResult.Vulnerabilities, isMultipleRoots, true)
if err != nil {
return nil, err
}
Expand All @@ -180,19 +178,19 @@ func (cfp *CreateFixPullRequestsCmd) createFixVersionsMap(project *utils.Project
if !fixVulnerability {
continue
}
vulnFixVersion := getMinimalFixVersion(vulnerability.ImpactedPackageVersion, vulnerability.FixedVersions)
vulnFixVersion := getMinimalFixVersion(vulnerability.ImpactedDependencyVersion, vulnerability.FixedVersions)
if vulnFixVersion == "" {
continue
}

fixVersionInfo, exists := fixVersionsMap[vulnerability.ImpactedPackageName]
fixVersionInfo, exists := fixVersionsMap[vulnerability.ImpactedDependencyName]
if exists {
// More than one vulnerability can exist on the same impacted package.
// Among all possible fix versions that fix the above impacted package, we select the maximum fix version.
fixVersionInfo.UpdateFixVersion(vulnFixVersion)
} else {
// First appearance of a version that fixes the current impacted package
fixVersionsMap[vulnerability.ImpactedPackageName] = NewFixVersionInfo(vulnFixVersion, vulnerability.Technology)
fixVersionsMap[vulnerability.ImpactedDependencyName] = NewFixVersionInfo(vulnFixVersion, vulnerability.Technology)
}
}
}
Expand Down Expand Up @@ -231,7 +229,7 @@ func (cfp *CreateFixPullRequestsCmd) shouldFixVulnerability(project *utils.Proje
}
}
}
if _, exist := cfp.mavenDepToPropertyMap[vulnerability.ImpactedPackageName]; !exist {
if _, exist := cfp.mavenDepToPropertyMap[vulnerability.ImpactedDependencyName]; !exist {
return false, nil
}
}
Expand Down
16 changes: 8 additions & 8 deletions commands/createfixpullrequests_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,13 @@ func getTestDataDir(t *testing.T) (string, string) {
return currentDir, testdataDir
}

/// 1.0 --> 1.0 ≤ x
/// (,1.0] --> x ≤ 1.0
/// (,1.0) --> x < 1.0
/// [1.0] --> x == 1.0
/// (1.0,) --> 1.0 < x
/// (1.0, 2.0) --> 1.0 < x < 2.0
/// [1.0, 2.0] --> 1.0 ≤ x ≤ 2.0
// / 1.0 --> 1.0 ≤ x
// / (,1.0] --> x ≤ 1.0
// / (,1.0) --> x < 1.0
// / [1.0] --> x == 1.0
// / (1.0,) --> 1.0 < x
// / (1.0, 2.0) --> 1.0 < x < 2.0
// / [1.0, 2.0] --> 1.0 ≤ x ≤ 2.0
func TestParseVersionChangeString(t *testing.T) {
tests := []struct {
versionChangeString string
Expand Down Expand Up @@ -184,7 +184,7 @@ func TestPackageTypeFromScan(t *testing.T) {
projectPath := filepath.Join("testdata", "projects", pkgType.ToString())
t.Run(pkgType.ToString(), func(t *testing.T) {
frogbotParams.Projects[0].WorkingDirs = []string{projectPath}
scanResponse, err := testScan.scan(frogbotParams.Projects[0], &frogbotParams.Server, services.XrayGraphScanParams{}, false, projectPath)
scanResponse, _, err := testScan.scan(frogbotParams.Projects[0], &frogbotParams.Server, services.XrayGraphScanParams{}, false, projectPath)
assert.NoError(t, err)
verifyTechnologyNaming(t, scanResponse, pkgType)
})
Expand Down
110 changes: 57 additions & 53 deletions commands/scanpullrequest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,29 +59,29 @@ func scanPullRequest(repoConfig *utils.FrogbotRepoConfig, client vcsclient.VcsCl
xrayScanParams := createXrayScanParams(repoConfig.Watches, repoConfig.JFrogProjectKey)
var vulnerabilitiesRows []formats.VulnerabilityOrViolationRow
for _, project := range repoConfig.Projects {
currentScan, err := auditSource(xrayScanParams, project, &repoConfig.Server)
currentScan, isMultipleRoot, err := auditSource(xrayScanParams, project, &repoConfig.Server)
if err != nil {
return err
}
if repoConfig.IncludeAllVulnerabilities {
log.Info("Frogbot is configured to show all vulnerabilities")
allIssuesRows, err := createAllIssuesRows(currentScan)
allIssuesRows, err := createAllIssuesRows(currentScan, isMultipleRoot)
if err != nil {
return err
}
vulnerabilitiesRows = append(vulnerabilitiesRows, allIssuesRows...)
} else {
// Audit target code
previousScan, err := auditTarget(client, xrayScanParams, project, repoConfig.Branches[0], &repoConfig.Git, &repoConfig.Server)
if err != nil {
return err
}
newIssuesRows, err := createNewIssuesRows(previousScan, currentScan)
if err != nil {
return err
}
vulnerabilitiesRows = append(vulnerabilitiesRows, newIssuesRows...)
continue
}
// Audit target code
previousScan, isMultipleRoot, err := auditTarget(client, xrayScanParams, project, repoConfig.Branches[0], &repoConfig.Git, &repoConfig.Server)
if err != nil {
return err
}
newIssuesRows, err := createNewIssuesRows(previousScan, currentScan, isMultipleRoot)
if err != nil {
return err
}
vulnerabilitiesRows = append(vulnerabilitiesRows, newIssuesRows...)
}

log.Info("Xray scan completed")
Expand Down Expand Up @@ -142,18 +142,18 @@ func getCommentFunctions(simplifiedOutput bool) (utils.GetTitleFunc, utils.GetSe
}

// Create vulnerabilities rows. The rows should contain only the new issues added by this PR
func createNewIssuesRows(previousScan, currentScan []services.ScanResponse) (vulnerabilitiesRows []formats.VulnerabilityOrViolationRow, err error) {
func createNewIssuesRows(previousScan, currentScan []services.ScanResponse, isMultipleRoot bool) (vulnerabilitiesRows []formats.VulnerabilityOrViolationRow, err error) {
previousScanAggregatedResults := aggregateScanResults(previousScan)
currentScanAggregatedResults := aggregateScanResults(currentScan)

if len(currentScanAggregatedResults.Violations) > 0 {
newViolations, err := getNewViolations(previousScanAggregatedResults, currentScanAggregatedResults)
newViolations, err := getNewViolations(previousScanAggregatedResults, currentScanAggregatedResults, isMultipleRoot)
if err != nil {
return vulnerabilitiesRows, err
}
vulnerabilitiesRows = append(vulnerabilitiesRows, newViolations...)
} else if len(currentScanAggregatedResults.Vulnerabilities) > 0 {
newVulnerabilities, err := getNewVulnerabilities(previousScanAggregatedResults, currentScanAggregatedResults)
newVulnerabilities, err := getNewVulnerabilities(previousScanAggregatedResults, currentScanAggregatedResults, isMultipleRoot)
if err != nil {
return vulnerabilitiesRows, err
}
Expand All @@ -175,28 +175,22 @@ func aggregateScanResults(scanResults []services.ScanResponse) services.ScanResp
return aggregateResults
}

// Create vulnerabilities rows. The rows should contain All the issues that were found in this module scan.
func getScanVulnerabilitiesRows(currentScan services.ScanResponse) ([]formats.VulnerabilityOrViolationRow, error) {
if len(currentScan.Violations) > 0 {
violationsRows, _, _, err := xrayutils.PrepareViolations(currentScan.Violations, false)
// Create vulnerabilities rows. The rows should contain all the issues that were found in this module scan.
func getScanVulnerabilitiesRows(violations []services.Violation, vulnerabilities []services.Vulnerability, isMultipleRoot bool) ([]formats.VulnerabilityOrViolationRow, error) {
if len(violations) > 0 {
violationsRows, _, _, err := xrayutils.PrepareViolations(violations, isMultipleRoot, true)
return violationsRows, err
} else if len(currentScan.Vulnerabilities) > 0 {
return xrayutils.PrepareVulnerabilities(currentScan.Vulnerabilities, false)
}
if len(vulnerabilities) > 0 {
return xrayutils.PrepareVulnerabilities(vulnerabilities, isMultipleRoot, true)
}
return []formats.VulnerabilityOrViolationRow{}, nil
}

// Create vulnerabilities rows. The rows should contain All the issues that were found in this PR
func createAllIssuesRows(currentScan []services.ScanResponse) ([]formats.VulnerabilityOrViolationRow, error) {
var vulnerabilitiesRows []formats.VulnerabilityOrViolationRow
for i := 0; i < len(currentScan); i += 1 {
newVulnerabilitiesRows, err := getScanVulnerabilitiesRows(currentScan[i])
if err != nil {
return vulnerabilitiesRows, err
}
vulnerabilitiesRows = append(vulnerabilitiesRows, newVulnerabilitiesRows...)
}
return vulnerabilitiesRows, nil
// Create vulnerabilities rows. The rows should contain all the issues that were found in this PR
func createAllIssuesRows(currentScan []services.ScanResponse, isMultipleRoot bool) ([]formats.VulnerabilityOrViolationRow, error) {
violations, vulnerabilities, _ := xrayutils.SplitScanResults(currentScan)
return getScanVulnerabilitiesRows(violations, vulnerabilities, isMultipleRoot)
}

func createXrayScanParams(watches []string, project string) (params services.XrayGraphScanParams) {
Expand All @@ -215,10 +209,10 @@ func createXrayScanParams(watches []string, project string) (params services.Xra
return
}

func auditSource(xrayScanParams services.XrayGraphScanParams, project utils.Project, server *coreconfig.ServerDetails) ([]services.ScanResponse, error) {
func auditSource(xrayScanParams services.XrayGraphScanParams, project utils.Project, server *coreconfig.ServerDetails) ([]services.ScanResponse, bool, error) {
wd, err := os.Getwd()
if err != nil {
return []services.ScanResponse{}, err
return []services.ScanResponse{}, false, err
}
fullPathWds := getFullPathWorkingDirs(&project, wd)
return runInstallAndAudit(xrayScanParams, &project, server, true, fullPathWds...)
Expand All @@ -240,7 +234,7 @@ func getFullPathWorkingDirs(project *utils.Project, baseWd string) []string {
return fullPathWds
}

func auditTarget(client vcsclient.VcsClient, xrayScanParams services.XrayGraphScanParams, project utils.Project, branch string, git *utils.Git, server *coreconfig.ServerDetails) (res []services.ScanResponse, err error) {
func auditTarget(client vcsclient.VcsClient, xrayScanParams services.XrayGraphScanParams, project utils.Project, branch string, git *utils.Git, server *coreconfig.ServerDetails) (res []services.ScanResponse, isMultipleRoot bool, err error) {
// First download the target repo to temp dir
log.Info("Auditing " + git.RepoName + " " + branch)
wd, cleanup, err := utils.DownloadRepoToTempDir(client, branch, git)
Expand All @@ -258,19 +252,19 @@ func auditTarget(client vcsclient.VcsClient, xrayScanParams services.XrayGraphSc
return runInstallAndAudit(xrayScanParams, &project, server, false, fullPathWds...)
}

func runInstallAndAudit(xrayScanParams services.XrayGraphScanParams, project *utils.Project, server *coreconfig.ServerDetails, failOnInstallationErrors bool, workDirs ...string) (results []services.ScanResponse, err error) {
func runInstallAndAudit(xrayScanParams services.XrayGraphScanParams, project *utils.Project, server *coreconfig.ServerDetails, failOnInstallationErrors bool, workDirs ...string) (results []services.ScanResponse, isMultipleRoot bool, err error) {
for _, wd := range workDirs {
if err = runInstallIfNeeded(project, wd, failOnInstallationErrors); err != nil {
return nil, err
return nil, false, err
}
}

results, _, err = audit.GenericAudit(xrayScanParams, server, false, project.UseWrapper, false,
results, isMultipleRoot, err = audit.GenericAudit(xrayScanParams, server, false, project.UseWrapper, false,
nil, nil, project.PipRequirementsFile, false, workDirs, []string{}...)
if err != nil {
return nil, err
return nil, false, err
}
return results, err
return results, isMultipleRoot, err
}

func runInstallIfNeeded(project *utils.Project, workDir string, failOnInstallationErrors bool) (err error) {
Expand All @@ -297,16 +291,16 @@ func runInstallIfNeeded(project *utils.Project, workDir string, failOnInstallati
return
}

func getNewViolations(previousScan, currentScan services.ScanResponse) (newViolationsRows []formats.VulnerabilityOrViolationRow, err error) {
func getNewViolations(previousScan, currentScan services.ScanResponse, isMultipleRoot bool) (newViolationsRows []formats.VulnerabilityOrViolationRow, err error) {
existsViolationsMap := make(map[string]formats.VulnerabilityOrViolationRow)
violationsRows, _, _, err := xrayutils.PrepareViolations(previousScan.Violations, false)
violationsRows, _, _, err := xrayutils.PrepareViolations(previousScan.Violations, isMultipleRoot, true)
if err != nil {
return violationsRows, err
}
for _, violation := range violationsRows {
existsViolationsMap[getUniqueID(violation)] = violation
}
violationsRows, _, _, err = xrayutils.PrepareViolations(currentScan.Violations, false)
violationsRows, _, _, err = xrayutils.PrepareViolations(currentScan.Violations, isMultipleRoot, true)
if err != nil {
return newViolationsRows, err
}
Expand All @@ -318,16 +312,16 @@ func getNewViolations(previousScan, currentScan services.ScanResponse) (newViola
return
}

func getNewVulnerabilities(previousScan, currentScan services.ScanResponse) (newVulnerabilitiesRows []formats.VulnerabilityOrViolationRow, err error) {
func getNewVulnerabilities(previousScan, currentScan services.ScanResponse, isMultipleRoot bool) (newVulnerabilitiesRows []formats.VulnerabilityOrViolationRow, err error) {
existsVulnerabilitiesMap := make(map[string]formats.VulnerabilityOrViolationRow)
vulnerabilitiesRows, err := xrayutils.PrepareVulnerabilities(previousScan.Vulnerabilities, false)
vulnerabilitiesRows, err := xrayutils.PrepareVulnerabilities(previousScan.Vulnerabilities, isMultipleRoot, true)
if err != nil {
return newVulnerabilitiesRows, err
}
for _, vulnerability := range vulnerabilitiesRows {
existsVulnerabilitiesMap[getUniqueID(vulnerability)] = vulnerability
}
vulnerabilitiesRows, err = xrayutils.PrepareVulnerabilities(currentScan.Vulnerabilities, false)
vulnerabilitiesRows, err = xrayutils.PrepareVulnerabilities(currentScan.Vulnerabilities, isMultipleRoot, true)
if err != nil {
return newVulnerabilitiesRows, err
}
Expand All @@ -340,7 +334,7 @@ func getNewVulnerabilities(previousScan, currentScan services.ScanResponse) (new
}

func getUniqueID(vulnerability formats.VulnerabilityOrViolationRow) string {
return vulnerability.ImpactedPackageName + vulnerability.ImpactedPackageVersion + vulnerability.IssueId
return vulnerability.ImpactedDependencyName + vulnerability.ImpactedDependencyVersion + vulnerability.IssueId
}

func createPullRequestMessage(vulnerabilitiesRows []formats.VulnerabilityOrViolationRow, getBanner utils.GetTitleFunc, getSeverityTag utils.GetSeverityTagFunc) string {
Expand All @@ -349,17 +343,27 @@ func createPullRequestMessage(vulnerabilitiesRows []formats.VulnerabilityOrViola
}
var tableContent string
for _, vulnerability := range vulnerabilitiesRows {
var componentName, componentVersion, cve string
var cve string
var directDependencies, directDependenciesVersions strings.Builder
if len(vulnerability.Components) > 0 {
componentName = vulnerability.ImpactedPackageName
componentVersion = vulnerability.ImpactedPackageVersion
for _, dependency := range vulnerability.Components {
directDependencies.WriteString(fmt.Sprintf("%s; ", dependency.Name))
directDependenciesVersions.WriteString(fmt.Sprintf("%s; ", dependency.Version))
}
}
if len(vulnerability.Cves) > 0 {
cve = vulnerability.Cves[0].Id
}
fixedVersionString := strings.Join(vulnerability.FixedVersions, " ")
tableContent += fmt.Sprintf("\n| %s%8s | %s | %s | %s | %s | %s | %s ", getSeverityTag(utils.IconName(vulnerability.Severity)), vulnerability.Severity, vulnerability.ImpactedPackageName,
vulnerability.ImpactedPackageVersion, fixedVersionString, componentName, componentVersion, cve)
tableContent += fmt.Sprintf("\n| %s%8s | %s | %s | %s | %s | %s | %s ",
getSeverityTag(utils.IconName(vulnerability.Severity)),
vulnerability.Severity,
strings.TrimSuffix(directDependencies.String(), "; "),
strings.TrimSuffix(directDependenciesVersions.String(), "; "),
vulnerability.ImpactedDependencyName,
vulnerability.ImpactedDependencyVersion,
fixedVersionString,
cve)
}
return getBanner(utils.VulnerabilitiesBannerSource) + utils.WhatIsFrogbotMd + utils.TableHeader + tableContent
}
Loading

0 comments on commit 80d5c27

Please sign in to comment.