diff --git a/packageupdaters/commonpackageupdater.go b/packageupdaters/commonpackageupdater.go index 58906e8c5..bc3bf7cd8 100644 --- a/packageupdaters/commonpackageupdater.go +++ b/packageupdaters/commonpackageupdater.go @@ -3,6 +3,7 @@ package packageupdaters import ( "fmt" "io/fs" + "os" "os/exec" "path/filepath" "regexp" @@ -135,6 +136,26 @@ func BuildPackageWithVersionRegex(impactedName, impactedVersion, dependencyLineF return regexp.MustCompile(regexpCompleteFormat) } +func getAbsolutePathUnderWd(path string) (string, error) { + absPath, err := filepath.Abs(filepath.Clean(path)) + if err != nil { + return "", fmt.Errorf("failed to resolve path %q: %w", path, err) + } + wd, err := os.Getwd() + if err != nil { + return "", fmt.Errorf("failed to get working directory: %w", err) + } + wdAbs, err := filepath.Abs(wd) + if err != nil { + return "", fmt.Errorf("failed to resolve working directory: %w", err) + } + rel, err := filepath.Rel(wdAbs, absPath) + if err != nil || strings.HasPrefix(rel, "..") || rel == ".." { + return "", fmt.Errorf("path %q is outside project directory", path) + } + return absPath, nil +} + func GetVulnerabilityLocations(vulnDetails *utils.VulnerabilityDetails, namesFilters []string, ignoreFilters []string) []string { pathsSet := datastructures.MakeSet[string]() for _, component := range vulnDetails.Components { diff --git a/packageupdaters/gradlepackageupdater.go b/packageupdaters/gradlepackageupdater.go index 504464473..719917fbd 100644 --- a/packageupdaters/gradlepackageupdater.go +++ b/packageupdaters/gradlepackageupdater.go @@ -2,10 +2,11 @@ package packageupdaters import ( "fmt" - "github.com/jfrog/frogbot/v2/utils" "os" "regexp" "strings" + + "github.com/jfrog/frogbot/v2/utils" ) const ( diff --git a/packageupdaters/npmpackageupdater.go b/packageupdaters/npmpackageupdater.go index 747b0c176..c60615857 100644 --- a/packageupdaters/npmpackageupdater.go +++ b/packageupdaters/npmpackageupdater.go @@ -155,7 +155,12 @@ func (npm *NpmPackageUpdater) RegenerateLockfile(vulnDetails *utils.Vulnerabilit if err = npm.regenerateLockFileWithRetry(); err != nil { log.Warn(fmt.Sprintf("Failed to regenerate lock file after updating '%s' to version '%s': %s. Rolling back...", vulnDetails.ImpactedDependencyName, vulnDetails.SuggestedFixedVersion, err.Error())) - if rollbackErr := os.WriteFile(descriptorPath, backupContent, 0644); rollbackErr != nil { + safePath, pathErr := getAbsolutePathUnderWd(descriptorPath) + if pathErr != nil { + return fmt.Errorf("failed to rollback descriptor: %w (original error: %v)", pathErr, err) + } + // #nosec G703 -- path validated under working directory by getAbsolutePathUnderWd + if rollbackErr := os.WriteFile(safePath, backupContent, 0644); rollbackErr != nil { return fmt.Errorf("failed to rollback descriptor after lock file regeneration failure: %w (original error: %v)", rollbackErr, err) } return err diff --git a/utils/outputwriter/outputcontent.go b/utils/outputwriter/outputcontent.go index 984523dc8..baa6d141c 100644 --- a/utils/outputwriter/outputcontent.go +++ b/utils/outputwriter/outputcontent.go @@ -287,8 +287,9 @@ func getSecurityViolationsContent(issues issues.ScansIssuesCollection, writer Ou if len(issues.ScaViolations) == 0 { return []string{} } - content = append(content, getSecurityViolationsSummaryTable(issues.ScaViolations, writer)) - content = append(content, getScaSecurityIssueDetailsContent(issues.ScaViolations, true, writer)...) + aggregated := aggregateVulnerabilitiesOrViolationsByCve(issues.ScaViolations) + content = append(content, getSecurityViolationsSummaryTable(aggregated, writer)) + content = append(content, getScaSecurityIssueDetailsContent(aggregated, true, writer)...) return ConvertContentToComments(content, writer, getDecoratorWithSecurityViolationTitle(writer)) } @@ -422,12 +423,78 @@ func getScaLicenseViolationDetails(violation formats.LicenseViolationRow, writer // Sca Vulnerabilities +func vulnerabilitySummaryKey(v formats.VulnerabilityOrViolationRow) string { + ids := make([]string, 0, len(v.Cves)) + for _, cve := range v.Cves { + ids = append(ids, cve.Id) + } + sort.Strings(ids) + if len(ids) == 0 { + return v.IssueId + } + return strings.Join(ids, ",") +} + +func uniqueStrings(s []string) []string { + seen := make(map[string]struct{}, len(s)) + out := make([]string, 0, len(s)) + for _, v := range s { + if _, ok := seen[v]; ok { + continue + } + seen[v] = struct{}{} + out = append(out, v) + } + return out +} + +func aggregateVulnerabilitiesOrViolationsByCve(vulnerabilities []formats.VulnerabilityOrViolationRow) []formats.VulnerabilityOrViolationRow { + if len(vulnerabilities) == 0 { + return vulnerabilities + } + byKey := make(map[string]*formats.VulnerabilityOrViolationRow) + for i := range vulnerabilities { + v := &vulnerabilities[i] + key := vulnerabilitySummaryKey(*v) + if existing, ok := byKey[key]; ok { + existing.ImpactPaths = append(existing.ImpactPaths, v.ImpactPaths...) + if len(v.FixedVersions) > 0 { + existing.FixedVersions = uniqueStrings(append(existing.FixedVersions, v.FixedVersions...)) + } + } else { + agg := *v + agg.ImpactPaths = append([][]formats.ComponentRow(nil), v.ImpactPaths...) + agg.FixedVersions = append([]string(nil), v.FixedVersions...) + heapCopy := new(formats.VulnerabilityOrViolationRow) + *heapCopy = agg + byKey[key] = heapCopy + } + } + result := make([]formats.VulnerabilityOrViolationRow, 0, len(byKey)) + for _, v := range byKey { + result = append(result, *v) + } + // Preserve relative order by first occurrence + order := make(map[string]int) + for i, v := range vulnerabilities { + key := vulnerabilitySummaryKey(v) + if _, ok := order[key]; !ok { + order[key] = i + } + } + sort.Slice(result, func(i, j int) bool { + return order[vulnerabilitySummaryKey(result[i])] < order[vulnerabilitySummaryKey(result[j])] + }) + return result +} + func GetVulnerabilitiesContent(vulnerabilities []formats.VulnerabilityOrViolationRow, writer OutputWriter) (content []string) { if len(vulnerabilities) == 0 { return []string{} } - content = append(content, getVulnerabilitiesSummaryTable(vulnerabilities, writer)) - content = append(content, getScaSecurityIssueDetailsContent(vulnerabilities, false, writer)...) + aggregated := aggregateVulnerabilitiesOrViolationsByCve(vulnerabilities) + content = append(content, getVulnerabilitiesSummaryTable(aggregated, writer)) + content = append(content, getScaSecurityIssueDetailsContent(aggregated, false, writer)...) return ConvertContentToComments(content, writer, getDecoratorWithScaVulnerabilitiesTitle(writer)) } diff --git a/utils/outputwriter/outputcontent_test.go b/utils/outputwriter/outputcontent_test.go index b7bd7ccf8..2ab40c022 100644 --- a/utils/outputwriter/outputcontent_test.go +++ b/utils/outputwriter/outputcontent_test.go @@ -2,6 +2,7 @@ package outputwriter import ( "path/filepath" + "strings" "testing" "github.com/jfrog/froggit-go/vcsutils" @@ -431,6 +432,92 @@ func TestVulnerabilitiesContent(t *testing.T) { } } +func TestAggregateVulnerabilitiesByCve(t *testing.T) { + t.Run("empty returns empty", func(t *testing.T) { + got := aggregateVulnerabilitiesOrViolationsByCve(nil) + assert.Nil(t, got) + got = aggregateVulnerabilitiesOrViolationsByCve([]formats.VulnerabilityOrViolationRow{}) + assert.Empty(t, got) + }) + + t.Run("same CVE merged into one row with combined paths", func(t *testing.T) { + cve := "CVE-2017-1000487" + in := []formats.VulnerabilityOrViolationRow{ + { + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + ImpactedDependencyName: "pkg", ImpactedDependencyVersion: "1.0", + }, + ImpactPaths: [][]formats.ComponentRow{{{Name: "root", Version: "1.0"}, {Name: "pkg", Version: "1.0"}}}, + FixedVersions: []string{"2.0", "3.0"}, + Cves: []formats.CveRow{{Id: cve}}, + IssueId: "XRAY-111", + }, + { + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + ImpactedDependencyName: "pkg", ImpactedDependencyVersion: "2.0", + }, + ImpactPaths: [][]formats.ComponentRow{{{Name: "root", Version: "1.0"}, {Name: "other", Version: "1.0"}, {Name: "pkg", Version: "2.0"}}}, + FixedVersions: []string{"2.0", "4.0"}, // 2.0 duplicate + Cves: []formats.CveRow{{Id: cve}}, + IssueId: "XRAY-222", + }, + } + got := aggregateVulnerabilitiesOrViolationsByCve(in) + assert.Len(t, got, 1) + assert.Len(t, got[0].ImpactPaths, 2) + assert.Equal(t, []string{"2.0", "3.0", "4.0"}, got[0].FixedVersions) // deduplicated + assert.Equal(t, "pkg", got[0].ImpactedDependencyName) + assert.Equal(t, "1.0", got[0].ImpactedDependencyVersion) // first occurrence kept + assert.Equal(t, "XRAY-111", got[0].IssueId) + }) + + t.Run("different CVEs stay separate", func(t *testing.T) { + in := []formats.VulnerabilityOrViolationRow{ + {Cves: []formats.CveRow{{Id: "CVE-A"}}, ImpactPaths: [][]formats.ComponentRow{{{Name: "a", Version: "1"}}}}, + {Cves: []formats.CveRow{{Id: "CVE-B"}}, ImpactPaths: [][]formats.ComponentRow{{{Name: "b", Version: "1"}}}}, + } + got := aggregateVulnerabilitiesOrViolationsByCve(in) + assert.Len(t, got, 2) + }) +} + +func TestVulnerabilitiesContent_aggregatesSameCveIntoOneRow(t *testing.T) { + sameCve := "CVE-2017-1000487" + vulnerabilities := []formats.VulnerabilityOrViolationRow{ + { + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "Critical"}, + ImpactedDependencyName: "org.codehaus.plexus:plexus-utils", + ImpactedDependencyVersion: "1.5.15", + }, + Applicable: "Not Applicable", + ImpactPaths: [][]formats.ComponentRow{ + {{Name: "root", Version: "1.0.0"}, {Name: "some-dep", Version: "1.0.0"}, {Name: "org.codehaus.plexus:plexus-utils", Version: "1.5.15"}}, + }, + Cves: []formats.CveRow{{Id: sameCve}}, + }, + { + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + SeverityDetails: formats.SeverityDetails{Severity: "Critical"}, + ImpactedDependencyName: "org.codehaus.plexus:plexus-utils", + ImpactedDependencyVersion: "1.5.1", + }, + Applicable: "Not Applicable", + ImpactPaths: [][]formats.ComponentRow{ + {{Name: "root", Version: "1.0.0"}, {Name: "other-dep", Version: "1.0.0"}, {Name: "org.codehaus.plexus:plexus-utils", Version: "1.5.1"}}, + }, + Cves: []formats.CveRow{{Id: sameCve}}, + }, + } + content := GetVulnerabilitiesContent(vulnerabilities, &SimplifiedOutput{}) + assert.NotEmpty(t, content) + tableSection := content[0] + assert.Contains(t, tableSection, sameCve, "output should mention the CVE") + assert.Contains(t, tableSection, "2 Transitive", "same CVE in two transitive paths should be aggregated and show 2 Transitive") + // CVE should appear only once in the table (one row), not twice + assert.Equal(t, 1, strings.Count(tableSection, sameCve), "CVE should appear once in the summary table (one aggregated row)") +} + func TestSecurityViolationsContent(t *testing.T) { testCases := []struct { name string