PR Decorator Fix - Same CVEs should be aggregated in the same record

packageupdaters/commonpackageupdater.go

@@ -3,6 +3,7 @@ package packageupdaters
 import (
 	"fmt"
 	"io/fs"
+	"os"
 	"os/exec"
 	"path/filepath"
 	"regexp"
@@ -138,6 +139,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 {

packageupdaters/conanpackageupdater.go

@@ -68,7 +68,12 @@ func (conan *ConanPackageUpdater) updateConanFile(conanFilePath string, vulnDeta
 		log.Debug(fmt.Sprintf("impacted dependency '%s' not found in descriptor '%s', moving to the next descriptor if exists...", impactedDependency, conanFilePath))
 		return false, nil
 	}
-	if err = os.WriteFile(conanFilePath, []byte(fixedFile), 0600); err != nil {
+	safePath, err := getAbsolutePathUnderWd(conanFilePath)
+	if err != nil {
+		return false, err
+	}
+	// #nosec G703 -- path validated under working directory by getAbsolutePathUnderWd
+	if err = os.WriteFile(safePath, []byte(fixedFile), 0600); err != nil {
 		err = fmt.Errorf("an error occured while writing the fixed version of %s to the requirements file '%s': %s", vulnDetails.ImpactedDependencyName, conanFilePath, err.Error())
 	}
 	isFileChanged = true

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 (
@@ -149,7 +150,12 @@ func writeUpdatedBuildFile(filePath string, fileContent string) (err error) {
 		return
 	}
 
-	err = os.WriteFile(filePath, []byte(fileContent), fileInfo.Mode())
+	safePath, err := getAbsolutePathUnderWd(filePath)
+	if err != nil {
+		return err
+	}
+	// #nosec G703 -- path validated under working directory by getAbsolutePathUnderWd
+	err = os.WriteFile(safePath, []byte(fileContent), fileInfo.Mode())
 	if err != nil {
 		err = fmt.Errorf("couldn't write fixes to file '%s': %q", filePath, err)
 	}

packageupdaters/npmpackageupdater.go

@@ -137,7 +137,12 @@ func (npm *NpmPackageUpdater) updateDescriptor(vulnDetails *utils.VulnerabilityD
 		return nil, fmt.Errorf("failed to update version in descriptor: %w", err)
 	}
 
-	if err = os.WriteFile(descriptorPath, updatedContent, 0644); err != nil {
+	safePath, err := getAbsolutePathUnderWd(descriptorPath)
+	if err != nil {
+		return nil, err
+	}
+	// #nosec G703 -- path validated under working directory by getAbsolutePathUnderWd
+	if err = os.WriteFile(safePath, updatedContent, 0644); err != nil {
 		return nil, fmt.Errorf("failed to write updated descriptor '%s': %w", descriptorPath, err)
 	}
 	return backupContent, nil
@@ -156,7 +161,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

utils/outputwriter/outputcontent.go

@@ -286,8 +286,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))
 }
 
@@ -421,12 +422,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))
 }
 

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