From 143f2c712da30768e5f14538f497c30a0d641cde Mon Sep 17 00:00:00 2001 From: Or Toren Date: Sun, 6 Sep 2026 12:47:04 +0300 Subject: [PATCH 01/12] Add NuGet package updater for plain PackageReference fixes Supports single-project .csproj fixes for plain PackageReference (Include or Update attribute, version as attribute or child element), with optional packages.lock.json regeneration via 'dotnet restore --force-evaluate --no-dependencies' when a lock file is present and tracked in git. Central Package Management (Directory.Packages.props) and packages.config are explicitly out of scope for now - a CPM-governed reference is detected and reported distinctly via CentralPackageManagementFixNotSupported rather than silently failing or being misdiagnosed as "not found". --- .../packageupdaters/commonpackageupdater.go | 3 + .../commonpackageupdater_test.go | 2 +- .../packageupdaters/nugetpackageupdater.go | 210 ++++++++++ .../nugetpackageupdater_test.go | 384 ++++++++++++++++++ remediation/sca/packageupdaters/types.go | 11 +- .../CpmSibling/CpmSibling.csproj | 11 + .../Project.csproj | 13 + .../WithLockFile/WithLockFile.csproj | 12 + .../WithLockFile/packages.lock.json | 13 + 9 files changed, 655 insertions(+), 4 deletions(-) create mode 100644 remediation/sca/packageupdaters/nugetpackageupdater.go create mode 100644 remediation/sca/packageupdaters/nugetpackageupdater_test.go create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/CpmSibling/CpmSibling.csproj create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/Project.csproj create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithLockFile/WithLockFile.csproj create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithLockFile/packages.lock.json diff --git a/remediation/sca/packageupdaters/commonpackageupdater.go b/remediation/sca/packageupdaters/commonpackageupdater.go index 40282ca66..1437fc3b4 100644 --- a/remediation/sca/packageupdaters/commonpackageupdater.go +++ b/remediation/sca/packageupdaters/commonpackageupdater.go @@ -47,6 +47,7 @@ var SupportedFixTechnologies = []techutils.Technology{ techutils.Go, techutils.Pnpm, techutils.Docker, + techutils.Nuget, } func GetCompatiblePackageUpdater(fixDetails *FixDetails) (PackageUpdater, bool) { @@ -63,6 +64,8 @@ func GetCompatiblePackageUpdater(fixDetails *FixDetails) (PackageUpdater, bool) return &PnpmPackageUpdater{}, true case techutils.Docker: return &DockerPackageUpdater{}, true + case techutils.Nuget: + return &NugetPackageUpdater{}, true default: return nil, false } diff --git a/remediation/sca/packageupdaters/commonpackageupdater_test.go b/remediation/sca/packageupdaters/commonpackageupdater_test.go index 0965fe3c6..bbb3ea03b 100644 --- a/remediation/sca/packageupdaters/commonpackageupdater_test.go +++ b/remediation/sca/packageupdaters/commonpackageupdater_test.go @@ -982,9 +982,9 @@ func TestGetCompatiblePackageUpdater(t *testing.T) { {techutils.Pip, true, &PythonPackageUpdater{}}, {techutils.Poetry, true, &PythonPackageUpdater{}}, {techutils.Pipenv, true, &PythonPackageUpdater{}}, + {techutils.Nuget, true, &NugetPackageUpdater{}}, {techutils.Yarn, false, nil}, {techutils.Gradle, false, nil}, - {techutils.Nuget, false, nil}, {techutils.Conan, false, nil}, } for _, tt := range tests { diff --git a/remediation/sca/packageupdaters/nugetpackageupdater.go b/remediation/sca/packageupdaters/nugetpackageupdater.go new file mode 100644 index 000000000..0a5638954 --- /dev/null +++ b/remediation/sca/packageupdaters/nugetpackageupdater.go @@ -0,0 +1,210 @@ +package packageupdaters + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "regexp" + "strings" + + "github.com/jfrog/jfrog-client-go/utils/log" +) + +const csprojFileSuffix = ".csproj" + +const ( + nugetPackageReferenceElementPattern = `(?s)]*/>|]*[^/]>.*?` + nugetKeyAttrPattern = `(?i)\b(?:Include|Update)\s*=\s*["']%s["']` + nugetVersionAttrPattern = `(?is)(\bVersion\s*=\s*["'])[^"']*(["'])` + nugetVersionElementPattern = `(?is)()[^<]*()` + + nugetLockFileName = "packages.lock.json" + nugetRestoreForceEvaluateFlag = "--force-evaluate" + // Deliberately narrower than what Renovate/Dependabot pass: --no-dependencies keeps a fix scoped + // to the touched project's own lock file, instead of also restoring (and diffing) every project + // it references via ProjectReference. + nugetRestoreNoDependenciesFlag = "--no-dependencies" +) + +// NugetRestoreEnvVars suppresses first-run banner noise and telemetry prompts observed when +// invoking a freshly-installed dotnet CLI, on top of the inherited environment. +var NugetRestoreEnvVars = map[string]string{ + "DOTNET_NOLOGO": "1", + "DOTNET_CLI_TELEMETRY_OPTOUT": "1", + "DOTNET_SKIP_FIRST_TIME_EXPERIENCE": "1", +} + +type NugetPackageUpdater struct { + CommonPackageUpdater +} + +func (n *NugetPackageUpdater) UpdateDependency(fixDetails *FixDetails) error { + if !fixDetails.IsDirectDependency { + return &ErrUnsupportedFix{ + PackageName: fixDetails.ImpactedDependencyName, + FixedVersion: fixDetails.SuggestedFixedVersion, + ErrorType: IndirectDependencyFixNotSupported, + } + } + + csprojPaths := collectCsprojPaths(fixDetails) + if len(csprojPaths) == 0 { + return fmt.Errorf("no .csproj locations found for %s - Components array is empty or missing Location data", fixDetails.ImpactedDependencyName) + } + log.Verbose(fmt.Sprintf("Found vulnerability %s occurrences for component %s in %s", fixDetails.IssueId, fixDetails.ImpactedDependencyVersion, strings.Join(csprojPaths, ", "))) + + originalWd, err := os.Getwd() + if err != nil { + return fmt.Errorf("failed to get current working directory: %w", err) + } + + var failingDescriptors []string + for _, csprojPath := range csprojPaths { + if fixErr := n.fixVulnerabilityAndRestore(csprojPath, fixDetails.ImpactedDependencyName, fixDetails.SuggestedFixedVersion, originalWd); fixErr != nil { + log.Warn(fixErr.Error()) + err = errors.Join(err, fmt.Errorf("failed to fix '%s' in descriptor '%s': %w", fixDetails.ImpactedDependencyName, csprojPath, fixErr)) + failingDescriptors = append(failingDescriptors, csprojPath) + } else { + log.Debug("Updated successfully " + csprojPath) + } + } + + if err != nil { + return fmt.Errorf("encountered errors while fixing '%s' vulnerability in descriptors [%s]: %w", fixDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), err) + } + return nil +} + +func collectCsprojPaths(fixDetails *FixDetails) []string { + var paths []string + for _, path := range GetVulnerabilityLocations(fixDetails, []string{}, []string{}) { + if strings.HasSuffix(path, csprojFileSuffix) { + paths = append(paths, path) + } + } + return paths +} + +func (n *NugetPackageUpdater) fixVulnerabilityAndRestore(csprojPath, packageName, fixedVersion, originalWd string) error { + //#nosec G304 -- csprojPath from descriptor discovery in the scanned repository. + originalCsproj, err := os.ReadFile(csprojPath) + if err != nil { + return fmt.Errorf("failed to read %s: %w", csprojPath, err) + } + + updatedCsproj, err := updatePackageReferenceVersion(originalCsproj, packageName, fixedVersion) + if err != nil { + return fmt.Errorf("%w in %s", err, csprojPath) + } + + //#nosec G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. + if err = os.WriteFile(csprojPath, updatedCsproj, 0644); err != nil { + return fmt.Errorf("failed to write %s: %w", csprojPath, err) + } + + lockFilePath := filepath.Join(filepath.Dir(csprojPath), nugetLockFileName) + //#nosec G304 -- lockFilePath is derived from csprojPath, itself from descriptor discovery. + originalLockFile, err := os.ReadFile(lockFilePath) + if err != nil { + if os.IsNotExist(err) { + // No lock file for this project - nothing further to regenerate. + return nil + } + return rollbackCsproj(csprojPath, originalCsproj, fmt.Errorf("failed to read %s: %w", lockFilePath, err)) + } + + lockFileTracked, checkErr := IsFileTrackedByGit(lockFilePath, originalWd) + if checkErr != nil { + log.Debug(fmt.Sprintf("Failed to check if lock file is tracked in git: %s. Proceeding with lock file regeneration.", checkErr.Error())) + lockFileTracked = true + } + if !lockFileTracked { + log.Debug(fmt.Sprintf("Lock file '%s' is not tracked in git, skipping lock file regeneration", lockFilePath)) + return nil + } + + if err = n.runDotnetRestore(csprojPath); err != nil { + log.Warn(fmt.Sprintf("Failed to regenerate lock file after updating '%s' to version '%s': %s. Rolling back...", packageName, fixedVersion, err.Error())) + return rollbackCsprojAndLock(csprojPath, originalCsproj, lockFilePath, originalLockFile, err) + } + return nil +} + +func (n *NugetPackageUpdater) runDotnetRestore(csprojPath string) error { + //#nosec G204 -- csprojPath from descriptor discovery; runs only after user approval. + cmd := exec.Command("dotnet", "restore", csprojPath, nugetRestoreForceEvaluateFlag, nugetRestoreNoDependenciesFlag) + cmd.Env = n.BuildEnvWithOverrides(NugetRestoreEnvVars) + log.Debug(fmt.Sprintf("Running 'dotnet restore %s %s %s'", csprojPath, nugetRestoreForceEvaluateFlag, nugetRestoreNoDependenciesFlag)) + + output, err := cmd.CombinedOutput() + if len(output) > 0 { + log.Debug(fmt.Sprintf("dotnet restore output:\n%s", string(output))) + } + if err != nil { + return fmt.Errorf("dotnet restore failed: %s\n%s", err.Error(), output) + } + return nil +} + +// rollbackCsproj restores the descriptor to its pre-fix content and returns origErr, or a wrapped +// error if the rollback itself fails. +func rollbackCsproj(csprojPath string, originalCsproj []byte, origErr error) error { + //#nosec G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. + if rollbackErr := os.WriteFile(csprojPath, originalCsproj, 0644); rollbackErr != nil { + return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", csprojPath, rollbackErr, origErr) + } + return origErr +} + +// rollbackCsprojAndLock restores both the descriptor and the lock file to their pre-fix content, +// so a failed restore never leaves the project in a half-fixed state. +func rollbackCsprojAndLock(csprojPath string, originalCsproj []byte, lockFilePath string, originalLockFile []byte, origErr error) error { + //#nosec G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. + if rollbackErr := os.WriteFile(csprojPath, originalCsproj, 0644); rollbackErr != nil { + return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", csprojPath, rollbackErr, origErr) + } + //#nosec G306 -- lockFilePath derived from csprojPath, from the same scan workflow. + if rollbackErr := os.WriteFile(lockFilePath, originalLockFile, 0644); rollbackErr != nil { + return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", lockFilePath, rollbackErr, origErr) + } + return origErr +} + +func updatePackageReferenceVersion(content []byte, packageName, fixedVersion string) ([]byte, error) { + element := regexp.MustCompile(nugetPackageReferenceElementPattern) + keyAttr := regexp.MustCompile(fmt.Sprintf(nugetKeyAttrPattern, regexp.QuoteMeta(packageName))) + versionAttr := regexp.MustCompile(nugetVersionAttrPattern) + versionElement := regexp.MustCompile(nugetVersionElementPattern) + + var fixedAny, foundWithoutVersion bool + updatedContent := element.ReplaceAllFunc(content, func(match []byte) []byte { + if !keyAttr.Match(match) { + return match + } + switch { + case versionAttr.Match(match): + fixedAny = true + return versionAttr.ReplaceAll(match, []byte("${1}"+fixedVersion+"${2}")) + case versionElement.Match(match): + fixedAny = true + return versionElement.ReplaceAll(match, []byte("${1}"+fixedVersion+"${2}")) + default: + foundWithoutVersion = true + return match + } + }) + + if fixedAny { + return updatedContent, nil + } + if foundWithoutVersion { + return nil, &ErrUnsupportedFix{ + PackageName: packageName, + FixedVersion: fixedVersion, + ErrorType: CentralPackageManagementFixNotSupported, + } + } + return nil, fmt.Errorf("dependency %s not found", packageName) +} diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go new file mode 100644 index 000000000..5abcbefdc --- /dev/null +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -0,0 +1,384 @@ +package packageupdaters + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "runtime" + "testing" + + biutils "github.com/jfrog/build-info-go/utils" + "github.com/jfrog/jfrog-cli-security/tests/utils/integration" + "github.com/jfrog/jfrog-cli-security/utils/formats" + "github.com/jfrog/jfrog-cli-security/utils/techutils" + "github.com/jfrog/jfrog-client-go/utils/io/fileutils" + "github.com/stretchr/testify/assert" +) + +// writeFakeDotnetRestore stands in for the real dotnet CLI: it writes lockFileContent next to the +// .csproj passed as its second argument (mirroring where 'dotnet restore' would write +// packages.lock.json), then exits with exitCode - letting the regeneration/rollback paths be +// tested deterministically without a real .NET SDK. +func writeFakeDotnetRestore(t *testing.T, dir string, exitCode int, lockFileContent string) { + if runtime.GOOS == "windows" { + t.Skip("fake tool executable is a POSIX shell script") + } + script := fmt.Sprintf(`#!/bin/sh +projdir=$(dirname "$2") +cat > "$projdir/packages.lock.json" <<'EOF' +%s +EOF +exit %d +`, lockFileContent, exitCode) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "dotnet"), []byte(script), 0o755)) +} + +func TestNugetUpdateDependency(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + updateAttributeCsproj := ` + + net8.0 + + + + +` + + childElementVersionCsproj := ` + + net8.0 + + + + 12.0.3 + + +` + + testCases := []struct { + name string + customCsproj string // if non-empty, overwrites Project.csproj after copying testdata + fixDetails *FixDetails + expectedContains []string + expectedNotContain []string + }{ + { + name: "IncludeThenVersion", + fixDetails: &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: "Project.csproj"}}}}, + }, + expectedContains: []string{`Include="Newtonsoft.Json" Version="13.0.1"`}, + expectedNotContain: []string{`Version="12.0.3"`}, + }, + { + name: "VersionThenInclude", + fixDetails: &FixDetails{ + SuggestedFixedVersion: "2.12.0", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Serilog", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: "Project.csproj"}}}}, + }, + expectedContains: []string{`Version="2.12.0" Include="Serilog"`}, + expectedNotContain: []string{`Version="2.10.0"`}, + }, + { + name: "UpdateAttribute", + customCsproj: updateAttributeCsproj, + fixDetails: &FixDetails{ + SuggestedFixedVersion: "17.9.0", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Microsoft.NET.Test.Sdk", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: "Project.csproj"}}}}, + }, + expectedContains: []string{`Update="Microsoft.NET.Test.Sdk" Version="17.9.0"`}, + expectedNotContain: []string{`Version="17.8.0"`}, + }, + { + name: "ChildElementVersion", + customCsproj: childElementVersionCsproj, + fixDetails: &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: "Project.csproj"}}}}, + }, + expectedContains: []string{`13.0.1`}, + expectedNotContain: []string{`12.0.3`}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + if tc.customCsproj != "" { + assert.NoError(t, os.WriteFile(filepath.Join(tmpDir, "Project.csproj"), []byte(tc.customCsproj), 0644)) + } + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(tc.fixDetails) + assert.NoError(t, err) + + modifiedCsproj, err := os.ReadFile("Project.csproj") + assert.NoError(t, err) + content := string(modifiedCsproj) + for _, s := range tc.expectedContains { + assert.Contains(t, content, s) + } + for _, s := range tc.expectedNotContain { + assert.NotContains(t, content, s) + } + }) + } +} + +// TestNugetUpdateDependencyPartialSuccess verifies that when the same vulnerable package is +// evidenced in multiple .csproj files and only some are fixable, the fixable ones are still +// updated - a CPM-governed sibling failing must not abort fixes to the others. +func TestNugetUpdateDependencyPartialSuccess(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{ + {File: "Project.csproj"}, + {File: filepath.Join("CpmSibling", "CpmSibling.csproj")}, + }}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.Error(t, err) + var unsupportedErr *ErrUnsupportedFix + assert.True(t, errors.As(err, &unsupportedErr)) + assert.Equal(t, CentralPackageManagementFixNotSupported, unsupportedErr.ErrorType) + + fixedProject, err := os.ReadFile("Project.csproj") + assert.NoError(t, err) + assert.Contains(t, string(fixedProject), `Include="Newtonsoft.Json" Version="13.0.1"`) + + cpmSibling, err := os.ReadFile(filepath.Join("CpmSibling", "CpmSibling.csproj")) + assert.NoError(t, err) + assert.Contains(t, string(cpmSibling), `Include="Newtonsoft.Json" />`) +} + +// TestNugetUpdateDependencyRegeneratesLockFile verifies that when a packages.lock.json exists +// next to the fixed .csproj, it gets regenerated via 'dotnet restore --force-evaluate +// --no-dependencies' after the descriptor edit. +func TestNugetUpdateDependencyRegeneratesLockFile(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + toolDir := t.TempDir() + regeneratedLock := `{"version":1,"dependencies":{"net8.0":{"Newtonsoft.Json":{"type":"Direct","resolved":"13.0.1"}}}}` + writeFakeDotnetRestore(t, toolDir, 0, regeneratedLock) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: filepath.Join("WithLockFile", "WithLockFile.csproj")}}}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.NoError(t, err) + + fixedCsproj, err := os.ReadFile(filepath.Join("WithLockFile", "WithLockFile.csproj")) + assert.NoError(t, err) + assert.Contains(t, string(fixedCsproj), `Include="Newtonsoft.Json" Version="13.0.1"`) + + lockFile, err := os.ReadFile(filepath.Join("WithLockFile", "packages.lock.json")) + assert.NoError(t, err) + assert.Contains(t, string(lockFile), `"resolved":"13.0.1"`) +} + +// TestNugetUpdateDependencyRollsBackOnRestoreFailure verifies that when 'dotnet restore' fails +// after the descriptor edit, both the .csproj and the lock file are restored to their original +// content - never leaving the project in a half-fixed state. +func TestNugetUpdateDependencyRollsBackOnRestoreFailure(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + originalLockFile, err := os.ReadFile(filepath.Join("WithLockFile", "packages.lock.json")) + assert.NoError(t, err) + originalCsproj, err := os.ReadFile(filepath.Join("WithLockFile", "WithLockFile.csproj")) + assert.NoError(t, err) + + toolDir := t.TempDir() + // Simulate a restore that partially writes a bad lock file before failing, so the rollback + // is proven to actually restore the original content rather than trivially no-op. + writeFakeDotnetRestore(t, toolDir, 1, `{"corrupted": true}`) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: filepath.Join("WithLockFile", "WithLockFile.csproj")}}}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.Error(t, err) + assert.Contains(t, err.Error(), "dotnet restore failed") + + rolledBackCsproj, err := os.ReadFile(filepath.Join("WithLockFile", "WithLockFile.csproj")) + assert.NoError(t, err) + assert.Equal(t, originalCsproj, rolledBackCsproj) + + rolledBackLockFile, err := os.ReadFile(filepath.Join("WithLockFile", "packages.lock.json")) + assert.NoError(t, err) + assert.Equal(t, originalLockFile, rolledBackLockFile) +} + +func TestNugetUpdateDependencyErrors(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + testCases := []struct { + name string + fixDetails *FixDetails + useTestData bool + assertErr func(t *testing.T, err error) + }{ + { + name: "DependencyNotFound", + fixDetails: &FixDetails{ + SuggestedFixedVersion: "1.0.0", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "NonExistent.Package", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: "Project.csproj"}}}}, + }, + useTestData: true, + assertErr: func(t *testing.T, err error) { + assert.Error(t, err) + assert.Contains(t, err.Error(), "NonExistent.Package") + }, + }, + { + name: "CentrallyManagedVersionNotSupported", + fixDetails: &FixDetails{ + SuggestedFixedVersion: "1.1.118", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "StyleCop.Analyzers", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: "Project.csproj"}}}}, + }, + useTestData: true, + assertErr: func(t *testing.T, err error) { + assert.Error(t, err) + var unsupportedErr *ErrUnsupportedFix + assert.True(t, errors.As(err, &unsupportedErr)) + assert.Equal(t, CentralPackageManagementFixNotSupported, unsupportedErr.ErrorType) + }, + }, + { + name: "IndirectDependencyNotSupported", + fixDetails: &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: false, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: "Project.csproj"}}}}, + }, + useTestData: false, + assertErr: func(t *testing.T, err error) { + assert.Error(t, err) + var unsupportedErr *ErrUnsupportedFix + assert.True(t, errors.As(err, &unsupportedErr)) + assert.Equal(t, IndirectDependencyFixNotSupported, unsupportedErr.ErrorType) + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if tc.useTestData { + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + } + updater := &NugetPackageUpdater{} + err := updater.UpdateDependency(tc.fixDetails) + tc.assertErr(t, err) + }) + } +} diff --git a/remediation/sca/packageupdaters/types.go b/remediation/sca/packageupdaters/types.go index ecacfad50..b1f304ba3 100644 --- a/remediation/sca/packageupdaters/types.go +++ b/remediation/sca/packageupdaters/types.go @@ -23,7 +23,8 @@ type FixDetails struct { type UnsupportedErrorType string const ( - IndirectDependencyFixNotSupported UnsupportedErrorType = "IndirectDependencyFixNotSupported" + IndirectDependencyFixNotSupported UnsupportedErrorType = "IndirectDependencyFixNotSupported" + CentralPackageManagementFixNotSupported UnsupportedErrorType = "CentralPackageManagementFixNotSupported" ) type ErrUnsupportedFix struct { @@ -33,10 +34,14 @@ type ErrUnsupportedFix struct { } func (err *ErrUnsupportedFix) Error() string { - if err.ErrorType == IndirectDependencyFixNotSupported { + switch err.ErrorType { + case IndirectDependencyFixNotSupported: return fmt.Sprintf("skipping fix of vulnerable package '%s' version '%s' - indirect dependency fix is not supported", err.PackageName, err.FixedVersion) + case CentralPackageManagementFixNotSupported: + return fmt.Sprintf("skipping fix of vulnerable package '%s' version '%s' - package version is centrally managed (NuGet Central Package Management) and fixing it is not yet supported", err.PackageName, err.FixedVersion) + default: + return fmt.Sprintf("skipping fix of vulnerable package '%s' version '%s' - build tools dependency fix is not supported", err.PackageName, err.FixedVersion) } - return fmt.Sprintf("skipping fix of vulnerable package '%s' version '%s' - build tools dependency fix is not supported", err.PackageName, err.FixedVersion) } type PackageUpdater interface { diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/CpmSibling/CpmSibling.csproj b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/CpmSibling/CpmSibling.csproj new file mode 100644 index 000000000..8ec0b84e2 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/CpmSibling/CpmSibling.csproj @@ -0,0 +1,11 @@ + + + + net8.0 + + + + + + + diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/Project.csproj b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/Project.csproj new file mode 100644 index 000000000..64d587786 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/Project.csproj @@ -0,0 +1,13 @@ + + + + net8.0 + + + + + + + + + diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithLockFile/WithLockFile.csproj b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithLockFile/WithLockFile.csproj new file mode 100644 index 000000000..d6059befe --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithLockFile/WithLockFile.csproj @@ -0,0 +1,12 @@ + + + + net8.0 + true + + + + + + + diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithLockFile/packages.lock.json b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithLockFile/packages.lock.json new file mode 100644 index 000000000..11fff8398 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithLockFile/packages.lock.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "dependencies": { + "net8.0": { + "Newtonsoft.Json": { + "type": "Direct", + "requested": "[12.0.3, )", + "resolved": "12.0.3", + "contentHash": "6mgjfnRB4jKMlzHSl+VD+Fk4tsfGyu+CBjBTP1sOnCPQ7z+H5DwCZjxr75tGD1jXTAcgwmttSaTgWJ0t9Uu9jA==" + } + } + } +} From 0f3318b192e6b2c3cea5ff6df593c1ab149ccf52 Mon Sep 17 00:00:00 2001 From: Or Toren Date: Sun, 6 Sep 2026 12:48:07 +0300 Subject: [PATCH 02/12] remove redundant comments --- .../sca/packageupdaters/nugetpackageupdater.go | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/remediation/sca/packageupdaters/nugetpackageupdater.go b/remediation/sca/packageupdaters/nugetpackageupdater.go index 0a5638954..789d5608b 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater.go @@ -20,16 +20,11 @@ const ( nugetVersionAttrPattern = `(?is)(\bVersion\s*=\s*["'])[^"']*(["'])` nugetVersionElementPattern = `(?is)()[^<]*()` - nugetLockFileName = "packages.lock.json" - nugetRestoreForceEvaluateFlag = "--force-evaluate" - // Deliberately narrower than what Renovate/Dependabot pass: --no-dependencies keeps a fix scoped - // to the touched project's own lock file, instead of also restoring (and diffing) every project - // it references via ProjectReference. + nugetLockFileName = "packages.lock.json" + nugetRestoreForceEvaluateFlag = "--force-evaluate" nugetRestoreNoDependenciesFlag = "--no-dependencies" ) -// NugetRestoreEnvVars suppresses first-run banner noise and telemetry prompts observed when -// invoking a freshly-installed dotnet CLI, on top of the inherited environment. var NugetRestoreEnvVars = map[string]string{ "DOTNET_NOLOGO": "1", "DOTNET_CLI_TELEMETRY_OPTOUT": "1", @@ -148,8 +143,6 @@ func (n *NugetPackageUpdater) runDotnetRestore(csprojPath string) error { return nil } -// rollbackCsproj restores the descriptor to its pre-fix content and returns origErr, or a wrapped -// error if the rollback itself fails. func rollbackCsproj(csprojPath string, originalCsproj []byte, origErr error) error { //#nosec G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. if rollbackErr := os.WriteFile(csprojPath, originalCsproj, 0644); rollbackErr != nil { @@ -158,8 +151,6 @@ func rollbackCsproj(csprojPath string, originalCsproj []byte, origErr error) err return origErr } -// rollbackCsprojAndLock restores both the descriptor and the lock file to their pre-fix content, -// so a failed restore never leaves the project in a half-fixed state. func rollbackCsprojAndLock(csprojPath string, originalCsproj []byte, lockFilePath string, originalLockFile []byte, origErr error) error { //#nosec G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. if rollbackErr := os.WriteFile(csprojPath, originalCsproj, 0644); rollbackErr != nil { From a08b9ce19161bffb6089c97b7e886cc5def087c0 Mon Sep 17 00:00:00 2001 From: Or Toren Date: Sun, 6 Sep 2026 12:53:56 +0300 Subject: [PATCH 03/12] remove redundant comments --- .../nugetpackageupdater_test.go | 24 ++++--------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go index 5abcbefdc..ec1fa56ae 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater_test.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -9,17 +9,14 @@ import ( "testing" biutils "github.com/jfrog/build-info-go/utils" + "github.com/jfrog/jfrog-client-go/utils/io/fileutils" + "github.com/stretchr/testify/assert" + "github.com/jfrog/jfrog-cli-security/tests/utils/integration" "github.com/jfrog/jfrog-cli-security/utils/formats" "github.com/jfrog/jfrog-cli-security/utils/techutils" - "github.com/jfrog/jfrog-client-go/utils/io/fileutils" - "github.com/stretchr/testify/assert" ) -// writeFakeDotnetRestore stands in for the real dotnet CLI: it writes lockFileContent next to the -// .csproj passed as its second argument (mirroring where 'dotnet restore' would write -// packages.lock.json), then exits with exitCode - letting the regeneration/rollback paths be -// tested deterministically without a real .NET SDK. func writeFakeDotnetRestore(t *testing.T, dir string, exitCode int, lockFileContent string) { if runtime.GOOS == "windows" { t.Skip("fake tool executable is a POSIX shell script") @@ -62,7 +59,7 @@ func TestNugetUpdateDependency(t *testing.T) { testCases := []struct { name string - customCsproj string // if non-empty, overwrites Project.csproj after copying testdata + customCsproj string fixDetails *FixDetails expectedContains []string expectedNotContain []string @@ -153,9 +150,6 @@ func TestNugetUpdateDependency(t *testing.T) { } } -// TestNugetUpdateDependencyPartialSuccess verifies that when the same vulnerable package is -// evidenced in multiple .csproj files and only some are fixable, the fixable ones are still -// updated - a CPM-governed sibling failing must not abort fixes to the others. func TestNugetUpdateDependencyPartialSuccess(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") @@ -200,9 +194,6 @@ func TestNugetUpdateDependencyPartialSuccess(t *testing.T) { assert.Contains(t, string(cpmSibling), `Include="Newtonsoft.Json" />`) } -// TestNugetUpdateDependencyRegeneratesLockFile verifies that when a packages.lock.json exists -// next to the fixed .csproj, it gets regenerated via 'dotnet restore --force-evaluate -// --no-dependencies' after the descriptor edit. func TestNugetUpdateDependencyRegeneratesLockFile(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") @@ -246,9 +237,6 @@ func TestNugetUpdateDependencyRegeneratesLockFile(t *testing.T) { assert.Contains(t, string(lockFile), `"resolved":"13.0.1"`) } -// TestNugetUpdateDependencyRollsBackOnRestoreFailure verifies that when 'dotnet restore' fails -// after the descriptor edit, both the .csproj and the lock file are restored to their original -// content - never leaving the project in a half-fixed state. func TestNugetUpdateDependencyRollsBackOnRestoreFailure(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") @@ -272,8 +260,6 @@ func TestNugetUpdateDependencyRollsBackOnRestoreFailure(t *testing.T) { assert.NoError(t, err) toolDir := t.TempDir() - // Simulate a restore that partially writes a bad lock file before failing, so the rollback - // is proven to actually restore the original content rather than trivially no-op. writeFakeDotnetRestore(t, toolDir, 1, `{"corrupted": true}`) t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) @@ -377,7 +363,7 @@ func TestNugetUpdateDependencyErrors(t *testing.T) { }() } updater := &NugetPackageUpdater{} - err := updater.UpdateDependency(tc.fixDetails) + err = updater.UpdateDependency(tc.fixDetails) tc.assertErr(t, err) }) } From a36869148339ef67967e865049ee7bcee7cb980f Mon Sep 17 00:00:00 2001 From: Or Toren Date: Tue, 8 Sep 2026 10:02:08 +0300 Subject: [PATCH 04/12] Fix gosec G703 findings on rollback/write paths The write-path #nosec suppressions only listed G306 (file permissions), missing G703 (path traversal via taint analysis) - the rule actually firing for os.WriteFile calls whose path parameter flows directly from descriptor discovery, without any indirection breaking the taint chain. Matches the existing #nosec G703 G306 convention already used for the same pattern in mavenpackageupdater.go and commonpackageupdater.go. --- remediation/sca/packageupdaters/nugetpackageupdater.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/remediation/sca/packageupdaters/nugetpackageupdater.go b/remediation/sca/packageupdaters/nugetpackageupdater.go index 789d5608b..b3c4c9a49 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater.go @@ -94,7 +94,7 @@ func (n *NugetPackageUpdater) fixVulnerabilityAndRestore(csprojPath, packageName return fmt.Errorf("%w in %s", err, csprojPath) } - //#nosec G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. + //#nosec G703 G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. if err = os.WriteFile(csprojPath, updatedCsproj, 0644); err != nil { return fmt.Errorf("failed to write %s: %w", csprojPath, err) } @@ -144,7 +144,7 @@ func (n *NugetPackageUpdater) runDotnetRestore(csprojPath string) error { } func rollbackCsproj(csprojPath string, originalCsproj []byte, origErr error) error { - //#nosec G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. + //#nosec G703 G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. if rollbackErr := os.WriteFile(csprojPath, originalCsproj, 0644); rollbackErr != nil { return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", csprojPath, rollbackErr, origErr) } @@ -152,11 +152,11 @@ func rollbackCsproj(csprojPath string, originalCsproj []byte, origErr error) err } func rollbackCsprojAndLock(csprojPath string, originalCsproj []byte, lockFilePath string, originalLockFile []byte, origErr error) error { - //#nosec G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. + //#nosec G703 G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. if rollbackErr := os.WriteFile(csprojPath, originalCsproj, 0644); rollbackErr != nil { return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", csprojPath, rollbackErr, origErr) } - //#nosec G306 -- lockFilePath derived from csprojPath, from the same scan workflow. + //#nosec G703 G306 -- lockFilePath derived from csprojPath, from the same scan workflow. if rollbackErr := os.WriteFile(lockFilePath, originalLockFile, 0644); rollbackErr != nil { return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", lockFilePath, rollbackErr, origErr) } From b2a0250f3fefe2f42f31c0e69ecb83ef7b5081f4 Mon Sep 17 00:00:00 2001 From: Or Toren Date: Tue, 8 Sep 2026 11:01:30 +0300 Subject: [PATCH 05/12] Address PR #874 review: restore side effects, CPM false positives High: dotnet restore no longer dirties the tree beyond the descriptor and lock file. Snapshots whether obj/ existed before restore and removes it afterward only if this fix created it; a 15-minute timeout (same budget as npm/yarn) now bounds the restore call via context.WithTimeout. Medium: a PackageReference with no inline version is no longer reported as specifically "Central Package Management" - it could just as well be sourced from Directory.Build.props, a VersionOverride, or be an SDK-implicit reference. Renamed to NoInlineVersionFixNotSupported with a message that doesn't overclaim which one applies. Medium: UpdateDependency no longer surfaces an error when at least one descriptor was actually fixed - a caller treating any non-nil error as "nothing happened" would otherwise discard an already-applied, successful sibling fix. Failures are still logged. Medium: project file matching is now case-insensitive and covers .fsproj/ .vbproj in addition to .csproj (also added to techutils' Nuget descriptor list, since evidence for those suffixes wouldn't reach this code otherwise). Lock-file regeneration/rollback tests now run on Windows too (previously skipped) via a portable fake dotnet script that also captures invoked args - added a test asserting --force-evaluate/--no-dependencies are always passed, doubling as a marker for the known limitation that --no-dependencies can leave a stale lock file in a referenced project. Low: documented (not changed) that a property-valued Version="$(X)" gets rewritten to a literal pin, unlike Maven's property-definition-aware handling - a deliberate simplification, not an oversight. --- .../packageupdaters/nugetpackageupdater.go | 175 ++++++++++---- .../nugetpackageupdater_test.go | 219 ++++++++++++++++-- remediation/sca/packageupdaters/types.go | 13 +- utils/techutils/techutils.go | 4 +- 4 files changed, 340 insertions(+), 71 deletions(-) diff --git a/remediation/sca/packageupdaters/nugetpackageupdater.go b/remediation/sca/packageupdaters/nugetpackageupdater.go index b3c4c9a49..c4f446566 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater.go @@ -1,6 +1,7 @@ package packageupdaters import ( + "context" "errors" "fmt" "os" @@ -12,19 +13,38 @@ import ( "github.com/jfrog/jfrog-client-go/utils/log" ) -const csprojFileSuffix = ".csproj" +// nugetProjectFileSuffixes are matched case-insensitively, since MSBuild project file suffixes +// aren't guaranteed to be written in any particular casing. +var nugetProjectFileSuffixes = []string{".csproj", ".fsproj", ".vbproj"} const ( nugetPackageReferenceElementPattern = `(?s)]*/>|]*[^/]>.*?` nugetKeyAttrPattern = `(?i)\b(?:Include|Update)\s*=\s*["']%s["']` - nugetVersionAttrPattern = `(?is)(\bVersion\s*=\s*["'])[^"']*(["'])` - nugetVersionElementPattern = `(?is)()[^<]*()` + // nugetVersionAttrPattern matches whatever is already inside Version="...", including an + // MSBuild property reference like "$(FooVersion)" - such a reference gets overwritten with the + // literal fixed version rather than resolved and updated at its property definition. That's a + // deliberate simplification, and a real behavior change versus how MSBuild itself would resolve + // it; Maven's updater handles the analogous ${property} case by updating the definition instead. + nugetVersionAttrPattern = `(?is)(\bVersion\s*=\s*["'])[^"']*(["'])` + nugetVersionElementPattern = `(?is)()[^<]*()` - nugetLockFileName = "packages.lock.json" - nugetRestoreForceEvaluateFlag = "--force-evaluate" + nugetLockFileName = "packages.lock.json" + nugetObjDirName = "obj" + + nugetRestoreForceEvaluateFlag = "--force-evaluate" + // --no-dependencies keeps a fix scoped to the touched project's own lock file, instead of also + // restoring (and diffing) every project it references via ProjectReference - a deliberate + // divergence from what Renovate/Dependabot themselves pass. + // + // Known limitation: if the bumped package's transitive dependencies are only pulled in through + // a referenced project (not the touched project itself), that referenced project's own + // packages.lock.json can end up stale relative to the new resolution, since --no-dependencies + // prevents restore from touching it at all. nugetRestoreNoDependenciesFlag = "--no-dependencies" ) +// NugetRestoreEnvVars suppresses first-run banner noise and telemetry prompts observed when +// invoking a freshly-installed dotnet CLI, on top of the inherited environment. var NugetRestoreEnvVars = map[string]string{ "DOTNET_NOLOGO": "1", "DOTNET_CLI_TELEMETRY_OPTOUT": "1", @@ -44,70 +64,94 @@ func (n *NugetPackageUpdater) UpdateDependency(fixDetails *FixDetails) error { } } - csprojPaths := collectCsprojPaths(fixDetails) - if len(csprojPaths) == 0 { - return fmt.Errorf("no .csproj locations found for %s - Components array is empty or missing Location data", fixDetails.ImpactedDependencyName) + projectFilePaths := collectProjectFilePaths(fixDetails) + if len(projectFilePaths) == 0 { + return fmt.Errorf("no NuGet project locations found for %s - Components array is empty or missing Location data", fixDetails.ImpactedDependencyName) } - log.Verbose(fmt.Sprintf("Found vulnerability %s occurrences for component %s in %s", fixDetails.IssueId, fixDetails.ImpactedDependencyVersion, strings.Join(csprojPaths, ", "))) + log.Verbose(fmt.Sprintf("Found vulnerability %s occurrences for component %s in %s", fixDetails.IssueId, fixDetails.ImpactedDependencyVersion, strings.Join(projectFilePaths, ", "))) originalWd, err := os.Getwd() if err != nil { return fmt.Errorf("failed to get current working directory: %w", err) } + var fixErrors error var failingDescriptors []string - for _, csprojPath := range csprojPaths { - if fixErr := n.fixVulnerabilityAndRestore(csprojPath, fixDetails.ImpactedDependencyName, fixDetails.SuggestedFixedVersion, originalWd); fixErr != nil { + var fixedAny bool + for _, projectFilePath := range projectFilePaths { + if fixErr := n.fixVulnerabilityAndRestore(projectFilePath, fixDetails.ImpactedDependencyName, fixDetails.SuggestedFixedVersion, originalWd); fixErr != nil { log.Warn(fixErr.Error()) - err = errors.Join(err, fmt.Errorf("failed to fix '%s' in descriptor '%s': %w", fixDetails.ImpactedDependencyName, csprojPath, fixErr)) - failingDescriptors = append(failingDescriptors, csprojPath) + fixErrors = errors.Join(fixErrors, fmt.Errorf("failed to fix '%s' in descriptor '%s': %w", fixDetails.ImpactedDependencyName, projectFilePath, fixErr)) + failingDescriptors = append(failingDescriptors, projectFilePath) } else { - log.Debug("Updated successfully " + csprojPath) + fixedAny = true + log.Debug("Updated successfully " + projectFilePath) } } - if err != nil { - return fmt.Errorf("encountered errors while fixing '%s' vulnerability in descriptors [%s]: %w", fixDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), err) + if fixErrors == nil { + return nil } - return nil + if fixedAny { + // At least one descriptor was fixed - don't fail the whole vulnerability just because a + // sibling descriptor (e.g. one governed by Central Package Management) couldn't be fixed. + // A caller treating any error as "nothing happened" would otherwise discard an + // already-applied, successful fix. + log.Warn(fmt.Sprintf("Partially fixed '%s': could not fix descriptor(s) [%s]: %s", fixDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), fixErrors.Error())) + return nil + } + return fmt.Errorf("encountered errors while fixing '%s' vulnerability in descriptors [%s]: %w", fixDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), fixErrors) } -func collectCsprojPaths(fixDetails *FixDetails) []string { +// GetVulnerabilityLocations matches by exact file name, which doesn't work here since a project +// file's base name varies per project - so collect every known descriptor location and filter by +// suffix instead. +func collectProjectFilePaths(fixDetails *FixDetails) []string { var paths []string for _, path := range GetVulnerabilityLocations(fixDetails, []string{}, []string{}) { - if strings.HasSuffix(path, csprojFileSuffix) { + if hasNugetProjectFileSuffix(path) { paths = append(paths, path) } } return paths } -func (n *NugetPackageUpdater) fixVulnerabilityAndRestore(csprojPath, packageName, fixedVersion, originalWd string) error { - //#nosec G304 -- csprojPath from descriptor discovery in the scanned repository. - originalCsproj, err := os.ReadFile(csprojPath) +func hasNugetProjectFileSuffix(path string) bool { + lowerPath := strings.ToLower(path) + for _, suffix := range nugetProjectFileSuffixes { + if strings.HasSuffix(lowerPath, suffix) { + return true + } + } + return false +} + +func (n *NugetPackageUpdater) fixVulnerabilityAndRestore(projectFilePath, packageName, fixedVersion, originalWd string) error { + //#nosec G304 -- projectFilePath from descriptor discovery in the scanned repository. + originalProjectFile, err := os.ReadFile(projectFilePath) if err != nil { - return fmt.Errorf("failed to read %s: %w", csprojPath, err) + return fmt.Errorf("failed to read %s: %w", projectFilePath, err) } - updatedCsproj, err := updatePackageReferenceVersion(originalCsproj, packageName, fixedVersion) + updatedProjectFile, err := updatePackageReferenceVersion(originalProjectFile, packageName, fixedVersion) if err != nil { - return fmt.Errorf("%w in %s", err, csprojPath) + return fmt.Errorf("%w in %s", err, projectFilePath) } - //#nosec G703 G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. - if err = os.WriteFile(csprojPath, updatedCsproj, 0644); err != nil { - return fmt.Errorf("failed to write %s: %w", csprojPath, err) + //#nosec G703 G306 -- projectFilePath from scan workflow; 0644 for VCS-tracked sources. + if err = os.WriteFile(projectFilePath, updatedProjectFile, 0644); err != nil { + return fmt.Errorf("failed to write %s: %w", projectFilePath, err) } - lockFilePath := filepath.Join(filepath.Dir(csprojPath), nugetLockFileName) - //#nosec G304 -- lockFilePath is derived from csprojPath, itself from descriptor discovery. + lockFilePath := filepath.Join(filepath.Dir(projectFilePath), nugetLockFileName) + //#nosec G304 -- lockFilePath is derived from projectFilePath, itself from descriptor discovery. originalLockFile, err := os.ReadFile(lockFilePath) if err != nil { if os.IsNotExist(err) { // No lock file for this project - nothing further to regenerate. return nil } - return rollbackCsproj(csprojPath, originalCsproj, fmt.Errorf("failed to read %s: %w", lockFilePath, err)) + return rollbackProjectFile(projectFilePath, originalProjectFile, fmt.Errorf("failed to read %s: %w", lockFilePath, err)) } lockFileTracked, checkErr := IsFileTrackedByGit(lockFilePath, originalWd) @@ -120,49 +164,86 @@ func (n *NugetPackageUpdater) fixVulnerabilityAndRestore(csprojPath, packageName return nil } - if err = n.runDotnetRestore(csprojPath); err != nil { + if err = n.runDotnetRestore(projectFilePath); err != nil { log.Warn(fmt.Sprintf("Failed to regenerate lock file after updating '%s' to version '%s': %s. Rolling back...", packageName, fixedVersion, err.Error())) - return rollbackCsprojAndLock(csprojPath, originalCsproj, lockFilePath, originalLockFile, err) + return rollbackProjectFileAndLock(projectFilePath, originalProjectFile, lockFilePath, originalLockFile, err) } return nil } -func (n *NugetPackageUpdater) runDotnetRestore(csprojPath string) error { - //#nosec G204 -- csprojPath from descriptor discovery; runs only after user approval. - cmd := exec.Command("dotnet", "restore", csprojPath, nugetRestoreForceEvaluateFlag, nugetRestoreNoDependenciesFlag) +// runDotnetRestore regenerates the lock file next to projectFilePath. It also removes the obj/ +// directory if restore created it (dotnet restore writes project.assets.json and other build +// artifacts there), so a fix PR doesn't pick up unrelated build output alongside the intended +// descriptor/lock file changes - mirroring the cleanup other updaters already do for their own +// install artifacts. +func (n *NugetPackageUpdater) runDotnetRestore(projectFilePath string) error { + objDir := filepath.Join(filepath.Dir(projectFilePath), nugetObjDirName) + objDirExisted := dirExists(objDir) + defer func() { + if objDirExisted { + return + } + if cleanupErr := os.RemoveAll(objDir); cleanupErr != nil { + log.Warn(fmt.Sprintf("Failed to remove restore-generated '%s': %s", objDir, cleanupErr.Error())) + } + }() + + ctx, cancel := context.WithTimeout(context.Background(), nodePackageManagerInstallTimeout) + defer cancel() + + //#nosec G204 -- projectFilePath from descriptor discovery; runs only after user approval. + cmd := exec.CommandContext(ctx, "dotnet", "restore", projectFilePath, nugetRestoreForceEvaluateFlag, nugetRestoreNoDependenciesFlag) cmd.Env = n.BuildEnvWithOverrides(NugetRestoreEnvVars) - log.Debug(fmt.Sprintf("Running 'dotnet restore %s %s %s'", csprojPath, nugetRestoreForceEvaluateFlag, nugetRestoreNoDependenciesFlag)) + log.Debug(fmt.Sprintf("Running 'dotnet restore %s %s %s'", projectFilePath, nugetRestoreForceEvaluateFlag, nugetRestoreNoDependenciesFlag)) output, err := cmd.CombinedOutput() if len(output) > 0 { log.Debug(fmt.Sprintf("dotnet restore output:\n%s", string(output))) } + if errors.Is(ctx.Err(), context.DeadlineExceeded) || errors.Is(err, context.DeadlineExceeded) { + return fmt.Errorf("dotnet restore timed out after %v", nodePackageManagerInstallTimeout) + } if err != nil { return fmt.Errorf("dotnet restore failed: %s\n%s", err.Error(), output) } return nil } -func rollbackCsproj(csprojPath string, originalCsproj []byte, origErr error) error { - //#nosec G703 G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. - if rollbackErr := os.WriteFile(csprojPath, originalCsproj, 0644); rollbackErr != nil { - return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", csprojPath, rollbackErr, origErr) +func dirExists(path string) bool { + info, err := os.Stat(path) + return err == nil && info.IsDir() +} + +// rollbackProjectFile restores the descriptor to its pre-fix content and returns origErr, or a +// wrapped error if the rollback itself fails. +func rollbackProjectFile(projectFilePath string, originalProjectFile []byte, origErr error) error { + //#nosec G703 G306 -- projectFilePath from scan workflow; 0644 for VCS-tracked sources. + if rollbackErr := os.WriteFile(projectFilePath, originalProjectFile, 0644); rollbackErr != nil { + return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", projectFilePath, rollbackErr, origErr) } return origErr } -func rollbackCsprojAndLock(csprojPath string, originalCsproj []byte, lockFilePath string, originalLockFile []byte, origErr error) error { - //#nosec G703 G306 -- csprojPath from scan workflow; 0644 for VCS-tracked sources. - if rollbackErr := os.WriteFile(csprojPath, originalCsproj, 0644); rollbackErr != nil { - return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", csprojPath, rollbackErr, origErr) +// rollbackProjectFileAndLock restores both the descriptor and the lock file to their pre-fix +// content, so a failed restore never leaves the project in a half-fixed state. +func rollbackProjectFileAndLock(projectFilePath string, originalProjectFile []byte, lockFilePath string, originalLockFile []byte, origErr error) error { + //#nosec G703 G306 -- projectFilePath from scan workflow; 0644 for VCS-tracked sources. + if rollbackErr := os.WriteFile(projectFilePath, originalProjectFile, 0644); rollbackErr != nil { + return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", projectFilePath, rollbackErr, origErr) } - //#nosec G703 G306 -- lockFilePath derived from csprojPath, from the same scan workflow. + //#nosec G703 G306 -- lockFilePath derived from projectFilePath, from the same scan workflow. if rollbackErr := os.WriteFile(lockFilePath, originalLockFile, 0644); rollbackErr != nil { return fmt.Errorf("failed to rollback '%s': %w (original error: %v)", lockFilePath, rollbackErr, origErr) } return origErr } +// updatePackageReferenceVersion patches the Version on the element matching +// packageName, in place, without re-serializing the surrounding XML. If no inline version is +// found - as an attribute or a child element - the package may be centrally managed (Directory. +// Packages.props), version-supplied via Directory.Build.props, overridden elsewhere, or an +// SDK-implicit reference; none of those are resolved here, so this is reported distinctly from +// "not found" rather than guessing which one applies. func updatePackageReferenceVersion(content []byte, packageName, fixedVersion string) ([]byte, error) { element := regexp.MustCompile(nugetPackageReferenceElementPattern) keyAttr := regexp.MustCompile(fmt.Sprintf(nugetKeyAttrPattern, regexp.QuoteMeta(packageName))) @@ -194,7 +275,7 @@ func updatePackageReferenceVersion(content []byte, packageName, fixedVersion str return nil, &ErrUnsupportedFix{ PackageName: packageName, FixedVersion: fixedVersion, - ErrorType: CentralPackageManagementFixNotSupported, + ErrorType: NoInlineVersionFixNotSupported, } } return nil, fmt.Errorf("dependency %s not found", packageName) diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go index ec1fa56ae..f4fff1966 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater_test.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -2,10 +2,10 @@ package packageupdaters import ( "errors" - "fmt" "os" "path/filepath" "runtime" + "strconv" "testing" biutils "github.com/jfrog/build-info-go/utils" @@ -17,17 +17,42 @@ import ( "github.com/jfrog/jfrog-cli-security/utils/techutils" ) -func writeFakeDotnetRestore(t *testing.T, dir string, exitCode int, lockFileContent string) { +// writeFakeDotnetRestore stands in for the real dotnet CLI: it writes lockFileContent next to the +// project file passed as its second argument (mirroring where 'dotnet restore' would write +// packages.lock.json), appends the received arguments to /args.log, optionally creates an +// obj/ directory alongside the project (mirroring dotnet restore's own build-artifact output), +// then exits with exitCode - letting the regeneration/rollback/cleanup paths, and the exact flags +// used, be tested deterministically without a real .NET SDK, on POSIX and Windows alike. +func writeFakeDotnetRestore(t *testing.T, dir string, exitCode int, lockFileContent string, createObjDir bool) { + lockContentPath := filepath.Join(dir, "lockfile-content.json") + assert.NoError(t, os.WriteFile(lockContentPath, []byte(lockFileContent), 0o644)) + argsLogPath := filepath.Join(dir, "args.log") + if runtime.GOOS == "windows" { - t.Skip("fake tool executable is a POSIX shell script") + mkObjLine := "" + if createObjDir { + mkObjLine = "if not exist \"%projdir%obj\" mkdir \"%projdir%obj\"\r\n" + } + script := "@echo off\r\n" + + "echo %*>>\"" + argsLogPath + "\"\r\n" + + "for %%F in (\"%2\") do set projdir=%%~dpF\r\n" + + "copy /Y \"" + lockContentPath + "\" \"%projdir%packages.lock.json\">nul\r\n" + + mkObjLine + + "exit /b " + strconv.Itoa(exitCode) + "\r\n" + assert.NoError(t, os.WriteFile(filepath.Join(dir, "dotnet.cmd"), []byte(script), 0o755)) + return + } + + mkObjLine := "" + if createObjDir { + mkObjLine = "mkdir -p \"$projdir/obj\"\n" } - script := fmt.Sprintf(`#!/bin/sh -projdir=$(dirname "$2") -cat > "$projdir/packages.lock.json" <<'EOF' -%s -EOF -exit %d -`, lockFileContent, exitCode) + script := "#!/bin/sh\n" + + "echo \"$@\" >> \"" + argsLogPath + "\"\n" + + "projdir=$(dirname \"$2\")\n" + + "cp \"" + lockContentPath + "\" \"$projdir/packages.lock.json\"\n" + + mkObjLine + + "exit " + strconv.Itoa(exitCode) + "\n" assert.NoError(t, os.WriteFile(filepath.Join(dir, "dotnet"), []byte(script), 0o755)) } @@ -180,10 +205,9 @@ func TestNugetUpdateDependencyPartialSuccess(t *testing.T) { updater := &NugetPackageUpdater{} err = updater.UpdateDependency(fixDetails) - assert.Error(t, err) - var unsupportedErr *ErrUnsupportedFix - assert.True(t, errors.As(err, &unsupportedErr)) - assert.Equal(t, CentralPackageManagementFixNotSupported, unsupportedErr.ErrorType) + // A successful sibling fix must not be reported as an error just because the CPM-governed one + // couldn't be fixed - the failure is logged, not surfaced as the call's result. + assert.NoError(t, err) fixedProject, err := os.ReadFile("Project.csproj") assert.NoError(t, err) @@ -213,7 +237,7 @@ func TestNugetUpdateDependencyRegeneratesLockFile(t *testing.T) { toolDir := t.TempDir() regeneratedLock := `{"version":1,"dependencies":{"net8.0":{"Newtonsoft.Json":{"type":"Direct","resolved":"13.0.1"}}}}` - writeFakeDotnetRestore(t, toolDir, 0, regeneratedLock) + writeFakeDotnetRestore(t, toolDir, 0, regeneratedLock, false) t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) fixDetails := &FixDetails{ @@ -260,7 +284,7 @@ func TestNugetUpdateDependencyRollsBackOnRestoreFailure(t *testing.T) { assert.NoError(t, err) toolDir := t.TempDir() - writeFakeDotnetRestore(t, toolDir, 1, `{"corrupted": true}`) + writeFakeDotnetRestore(t, toolDir, 1, `{"corrupted": true}`, false) t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) fixDetails := &FixDetails{ @@ -313,7 +337,7 @@ func TestNugetUpdateDependencyErrors(t *testing.T) { }, }, { - name: "CentrallyManagedVersionNotSupported", + name: "NoInlineVersionNotSupported", fixDetails: &FixDetails{ SuggestedFixedVersion: "1.1.118", IsDirectDependency: true, @@ -326,7 +350,7 @@ func TestNugetUpdateDependencyErrors(t *testing.T) { assert.Error(t, err) var unsupportedErr *ErrUnsupportedFix assert.True(t, errors.As(err, &unsupportedErr)) - assert.Equal(t, CentralPackageManagementFixNotSupported, unsupportedErr.ErrorType) + assert.Equal(t, NoInlineVersionFixNotSupported, unsupportedErr.ErrorType) }, }, { @@ -368,3 +392,162 @@ func TestNugetUpdateDependencyErrors(t *testing.T) { }) } } + +func TestHasNugetProjectFileSuffix(t *testing.T) { + tests := []struct { + path string + want bool + }{ + {"Project.csproj", true}, + {"Project.CSProj", true}, + {"Project.CSPROJ", true}, + {"Project.fsproj", true}, + {"Project.FSPROJ", true}, + {"Project.vbproj", true}, + {"Project.VBPROJ", true}, + {filepath.Join("src", "Project.csproj"), true}, + {"Directory.Packages.props", false}, + {"packages.config", false}, + {"Project.sln", false}, + } + for _, tt := range tests { + t.Run(tt.path, func(t *testing.T) { + assert.Equal(t, tt.want, hasNugetProjectFileSuffix(tt.path)) + }) + } +} + +// TestNugetUpdateDependencyRestoreScopedFlags documents the exact restore invocation: both +// --force-evaluate and --no-dependencies must always be passed. --no-dependencies is what keeps a +// fix scoped to the touched project's own lock file - and is also the source of a known +// limitation (a bumped package's transitives living in a referenced project can leave that +// project's own lock file stale), so this test doubles as a marker for that tradeoff. +func TestNugetUpdateDependencyRestoreScopedFlags(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + toolDir := t.TempDir() + writeFakeDotnetRestore(t, toolDir, 0, `{}`, false) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: filepath.Join("WithLockFile", "WithLockFile.csproj")}}}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.NoError(t, err) + + argsLog, err := os.ReadFile(filepath.Join(toolDir, "args.log")) + assert.NoError(t, err) + assert.Contains(t, string(argsLog), "--force-evaluate") + assert.Contains(t, string(argsLog), "--no-dependencies") +} + +// TestNugetUpdateDependencyCleansUpGeneratedObjDir verifies that an obj/ directory created by +// restore (dotnet writes project.assets.json and other build artifacts there) is removed +// afterward, so a fix PR doesn't pick up unrelated build output alongside the intended +// descriptor/lock file changes. +func TestNugetUpdateDependencyCleansUpGeneratedObjDir(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + objDir := filepath.Join("WithLockFile", "obj") + _, statErr := os.Stat(objDir) + assert.True(t, os.IsNotExist(statErr), "obj/ should not exist before the fix") + + toolDir := t.TempDir() + writeFakeDotnetRestore(t, toolDir, 0, `{}`, true) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: filepath.Join("WithLockFile", "WithLockFile.csproj")}}}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.NoError(t, err) + + _, statErr = os.Stat(objDir) + assert.True(t, os.IsNotExist(statErr), "obj/ created by restore should be cleaned up afterward") +} + +// TestNugetUpdateDependencyPreservesPreexistingObjDir verifies that an obj/ directory that +// already existed before the fix (e.g. from a prior local build) is left alone, even though +// restore also touches it. +func TestNugetUpdateDependencyPreservesPreexistingObjDir(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + objDir := filepath.Join("WithLockFile", "obj") + assert.NoError(t, os.MkdirAll(objDir, 0755)) + sentinelPath := filepath.Join(objDir, "sentinel.txt") + assert.NoError(t, os.WriteFile(sentinelPath, []byte("keep-me"), 0644)) + + toolDir := t.TempDir() + writeFakeDotnetRestore(t, toolDir, 0, `{}`, true) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: filepath.Join("WithLockFile", "WithLockFile.csproj")}}}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.NoError(t, err) + + sentinel, err := os.ReadFile(sentinelPath) + assert.NoError(t, err) + assert.Equal(t, "keep-me", string(sentinel)) +} diff --git a/remediation/sca/packageupdaters/types.go b/remediation/sca/packageupdaters/types.go index b1f304ba3..dc72be458 100644 --- a/remediation/sca/packageupdaters/types.go +++ b/remediation/sca/packageupdaters/types.go @@ -23,8 +23,13 @@ type FixDetails struct { type UnsupportedErrorType string const ( - IndirectDependencyFixNotSupported UnsupportedErrorType = "IndirectDependencyFixNotSupported" - CentralPackageManagementFixNotSupported UnsupportedErrorType = "CentralPackageManagementFixNotSupported" + IndirectDependencyFixNotSupported UnsupportedErrorType = "IndirectDependencyFixNotSupported" + // NoInlineVersionFixNotSupported covers any PackageReference found with no inline version - + // whether it's actually governed by Central Package Management, supplied via + // Directory.Build.props, overridden elsewhere, or an SDK-implicit reference. The updater can't + // tell these apart from the reference site alone, so it reports them all the same way rather + // than guessing. + NoInlineVersionFixNotSupported UnsupportedErrorType = "NoInlineVersionFixNotSupported" ) type ErrUnsupportedFix struct { @@ -37,8 +42,8 @@ func (err *ErrUnsupportedFix) Error() string { switch err.ErrorType { case IndirectDependencyFixNotSupported: return fmt.Sprintf("skipping fix of vulnerable package '%s' version '%s' - indirect dependency fix is not supported", err.PackageName, err.FixedVersion) - case CentralPackageManagementFixNotSupported: - return fmt.Sprintf("skipping fix of vulnerable package '%s' version '%s' - package version is centrally managed (NuGet Central Package Management) and fixing it is not yet supported", err.PackageName, err.FixedVersion) + case NoInlineVersionFixNotSupported: + return fmt.Sprintf("skipping fix of vulnerable package '%s' version '%s' - no inline version found on the reference (may be centrally managed, supplied via Directory.Build.props, overridden elsewhere, or an SDK-implicit reference) and fixing it is not yet supported", err.PackageName, err.FixedVersion) default: return fmt.Sprintf("skipping fix of vulnerable package '%s' version '%s' - build tools dependency fix is not supported", err.PackageName, err.FixedVersion) } diff --git a/utils/techutils/techutils.go b/utils/techutils/techutils.go index b9e77b0be..8e31cd012 100644 --- a/utils/techutils/techutils.go +++ b/utils/techutils/techutils.go @@ -264,8 +264,8 @@ var technologiesData = map[Technology]TechData{ }, Nuget: { formal: "NuGet", - indicators: []string{".sln", ".slnx", ".csproj"}, - packageDescriptors: []string{".sln", ".slnx", ".csproj"}, + indicators: []string{".sln", ".slnx", ".csproj", ".fsproj", ".vbproj"}, + packageDescriptors: []string{".sln", ".slnx", ".csproj", ".fsproj", ".vbproj"}, // .NET CLI is used for NuGet projects execCommand: "dotnet", packageInstallationCommand: "add", From d39bcc2ce8cc56b66ab5832e526c882624978af0 Mon Sep 17 00:00:00 2001 From: Or Toren Date: Wed, 9 Sep 2026 09:28:37 +0300 Subject: [PATCH 06/12] Add test coverage for multiple independent self-contained projects Confirms the existing evidence loop already fixes the same vulnerable package correctly across several unrelated .csproj files - each patched from its own original content and starting version, with no interaction between them - without needing any production code changes. --- .../nugetpackageupdater_test.go | 47 +++++++++++++++++++ .../IndependentSibling.csproj | 12 +++++ 2 files changed, 59 insertions(+) create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/IndependentSibling/IndependentSibling.csproj diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go index f4fff1966..771166e05 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater_test.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -218,6 +218,53 @@ func TestNugetUpdateDependencyPartialSuccess(t *testing.T) { assert.Contains(t, string(cpmSibling), `Include="Newtonsoft.Json" />`) } +func TestNugetUpdateDependencyMultipleIndependentProjects(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{ + {File: "Project.csproj"}, + {File: filepath.Join("IndependentSibling", "IndependentSibling.csproj")}, + }}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.NoError(t, err) + + fixedProject, err := os.ReadFile("Project.csproj") + assert.NoError(t, err) + projectContent := string(fixedProject) + assert.Contains(t, projectContent, `Include="Newtonsoft.Json" Version="13.0.1"`) + assert.NotContains(t, projectContent, `Version="12.0.3"`) + assert.Contains(t, projectContent, `Version="2.10.0" Include="Serilog"`) + + fixedSibling, err := os.ReadFile(filepath.Join("IndependentSibling", "IndependentSibling.csproj")) + assert.NoError(t, err) + siblingContent := string(fixedSibling) + assert.Contains(t, siblingContent, `Include="Newtonsoft.Json" Version="13.0.1"`) + assert.NotContains(t, siblingContent, `Version="11.0.2"`) + assert.Contains(t, siblingContent, `Include="NUnit" Version="3.13.3"`) +} + func TestNugetUpdateDependencyRegeneratesLockFile(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/IndependentSibling/IndependentSibling.csproj b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/IndependentSibling/IndependentSibling.csproj new file mode 100644 index 000000000..d756f0198 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/IndependentSibling/IndependentSibling.csproj @@ -0,0 +1,12 @@ + + + + net8.0 + + + + + + + + From 06c4087cc5d2d06b229b2620054756bcde31a5cb Mon Sep 17 00:00:00 2001 From: Or Toren Date: Wed, 9 Sep 2026 09:33:58 +0300 Subject: [PATCH 07/12] Test lock-file regeneration and rollback isolation across projects Every prior lock-file test used exactly one project with one lock file, so nothing exercised the combination step 3 is actually about: multiple independent projects each regenerating their own lock file, and - more importantly - restore failing for one project while succeeding for another in the same call. Confirms there's no shared state leaking across loop iterations: a failing sibling rolls back on its own, and does not affect an already-applied, successful fix elsewhere. --- .../nugetpackageupdater_test.go | 136 ++++++++++++++++++ .../FailProjectLockFile.csproj | 12 ++ .../FailProjectLockFile/packages.lock.json | 13 ++ 3 files changed, 161 insertions(+) create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/FailProjectLockFile/FailProjectLockFile.csproj create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/FailProjectLockFile/packages.lock.json diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go index 771166e05..8b9bce657 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater_test.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -56,6 +56,34 @@ func writeFakeDotnetRestore(t *testing.T, dir string, exitCode int, lockFileCont assert.NoError(t, os.WriteFile(filepath.Join(dir, "dotnet"), []byte(script), 0o755)) } +// writeFakeDotnetRestoreFailingForPath behaves like writeFakeDotnetRestore, except it fails +// (without touching the lock file) only when the project file path it's invoked with contains +// pathMarker - letting one project's restore fail while a sibling's succeeds in the same test run. +func writeFakeDotnetRestoreFailingForPath(t *testing.T, dir string, pathMarker string, lockFileContent string) { + lockContentPath := filepath.Join(dir, "lockfile-content.json") + assert.NoError(t, os.WriteFile(lockContentPath, []byte(lockFileContent), 0o644)) + + if runtime.GOOS == "windows" { + script := "@echo off\r\n" + + "echo %2 | findstr /C:\"" + pathMarker + "\" >nul\r\n" + + "if %errorlevel%==0 exit /b 1\r\n" + + "for %%F in (\"%2\") do set projdir=%%~dpF\r\n" + + "copy /Y \"" + lockContentPath + "\" \"%projdir%packages.lock.json\">nul\r\n" + + "exit /b 0\r\n" + assert.NoError(t, os.WriteFile(filepath.Join(dir, "dotnet.cmd"), []byte(script), 0o755)) + return + } + + script := "#!/bin/sh\n" + + "case \"$2\" in\n" + + " *" + pathMarker + "*) exit 1 ;;\n" + + "esac\n" + + "projdir=$(dirname \"$2\")\n" + + "cp \"" + lockContentPath + "\" \"$projdir/packages.lock.json\"\n" + + "exit 0\n" + assert.NoError(t, os.WriteFile(filepath.Join(dir, "dotnet"), []byte(script), 0o755)) +} + func TestNugetUpdateDependency(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") @@ -308,6 +336,114 @@ func TestNugetUpdateDependencyRegeneratesLockFile(t *testing.T) { assert.Contains(t, string(lockFile), `"resolved":"13.0.1"`) } +// TestNugetUpdateDependencyMultipleProjectsEachRegenerateOwnLockFile verifies that when the same +// vulnerable package is evidenced in two independent projects, each with its own lock file, both +// lock files are regenerated - not just the first one, or a shared/leaked state across loop +// iterations that only ends up applying to one of them. +func TestNugetUpdateDependencyMultipleProjectsEachRegenerateOwnLockFile(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + toolDir := t.TempDir() + regeneratedLock := `{"version":1,"dependencies":{"net8.0":{"Newtonsoft.Json":{"type":"Direct","resolved":"13.0.1"}}}}` + writeFakeDotnetRestore(t, toolDir, 0, regeneratedLock, false) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{ + {File: filepath.Join("WithLockFile", "WithLockFile.csproj")}, + {File: filepath.Join("FailProjectLockFile", "FailProjectLockFile.csproj")}, + }}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.NoError(t, err) + + for _, dir := range []string{"WithLockFile", "FailProjectLockFile"} { + lockFile, readErr := os.ReadFile(filepath.Join(dir, "packages.lock.json")) + assert.NoError(t, readErr) + assert.Contains(t, string(lockFile), `"resolved":"13.0.1"`, "lock file in %s should have been regenerated", dir) + } +} + +// TestNugetUpdateDependencyRestoreFailureIsolatedPerProject verifies that when restore fails for +// one of several independent projects but succeeds for another, only the failing project is +// rolled back - a successful sibling's fix and regenerated lock file must survive. +func TestNugetUpdateDependencyRestoreFailureIsolatedPerProject(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + originalFailProjectCsproj, err := os.ReadFile(filepath.Join("FailProjectLockFile", "FailProjectLockFile.csproj")) + assert.NoError(t, err) + originalFailProjectLock, err := os.ReadFile(filepath.Join("FailProjectLockFile", "packages.lock.json")) + assert.NoError(t, err) + + toolDir := t.TempDir() + writeFakeDotnetRestoreFailingForPath(t, toolDir, "FailProjectLockFile", `{"version":1,"dependencies":{"net8.0":{"Newtonsoft.Json":{"type":"Direct","resolved":"13.0.1"}}}}`) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{ + {File: filepath.Join("WithLockFile", "WithLockFile.csproj")}, + {File: filepath.Join("FailProjectLockFile", "FailProjectLockFile.csproj")}, + }}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + // The failing project must not turn the successful sibling's fix into a reported error. + assert.NoError(t, err) + + fixedCsproj, err := os.ReadFile(filepath.Join("WithLockFile", "WithLockFile.csproj")) + assert.NoError(t, err) + assert.Contains(t, string(fixedCsproj), `Include="Newtonsoft.Json" Version="13.0.1"`) + fixedLock, err := os.ReadFile(filepath.Join("WithLockFile", "packages.lock.json")) + assert.NoError(t, err) + assert.Contains(t, string(fixedLock), `"resolved":"13.0.1"`) + + rolledBackCsproj, err := os.ReadFile(filepath.Join("FailProjectLockFile", "FailProjectLockFile.csproj")) + assert.NoError(t, err) + assert.Equal(t, originalFailProjectCsproj, rolledBackCsproj) + rolledBackLock, err := os.ReadFile(filepath.Join("FailProjectLockFile", "packages.lock.json")) + assert.NoError(t, err) + assert.Equal(t, originalFailProjectLock, rolledBackLock) +} + func TestNugetUpdateDependencyRollsBackOnRestoreFailure(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/FailProjectLockFile/FailProjectLockFile.csproj b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/FailProjectLockFile/FailProjectLockFile.csproj new file mode 100644 index 000000000..d6059befe --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/FailProjectLockFile/FailProjectLockFile.csproj @@ -0,0 +1,12 @@ + + + + net8.0 + true + + + + + + + diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/FailProjectLockFile/packages.lock.json b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/FailProjectLockFile/packages.lock.json new file mode 100644 index 000000000..11fff8398 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/FailProjectLockFile/packages.lock.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "dependencies": { + "net8.0": { + "Newtonsoft.Json": { + "type": "Direct", + "requested": "[12.0.3, )", + "resolved": "12.0.3", + "contentHash": "6mgjfnRB4jKMlzHSl+VD+Fk4tsfGyu+CBjBTP1sOnCPQ7z+H5DwCZjxr75tGD1jXTAcgwmttSaTgWJ0t9Uu9jA==" + } + } + } +} From 67401eabf17f1ab356e7a15d1c2a16da41655155 Mon Sep 17 00:00:00 2001 From: Or Toren Date: Wed, 9 Sep 2026 09:44:27 +0300 Subject: [PATCH 08/12] remove redundant comments --- .../packageupdaters/nugetpackageupdater.go | 18 ------------ .../nugetpackageupdater_test.go | 28 ------------------- 2 files changed, 46 deletions(-) diff --git a/remediation/sca/packageupdaters/nugetpackageupdater.go b/remediation/sca/packageupdaters/nugetpackageupdater.go index c4f446566..4cf7b3930 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater.go @@ -103,9 +103,6 @@ func (n *NugetPackageUpdater) UpdateDependency(fixDetails *FixDetails) error { return fmt.Errorf("encountered errors while fixing '%s' vulnerability in descriptors [%s]: %w", fixDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), fixErrors) } -// GetVulnerabilityLocations matches by exact file name, which doesn't work here since a project -// file's base name varies per project - so collect every known descriptor location and filter by -// suffix instead. func collectProjectFilePaths(fixDetails *FixDetails) []string { var paths []string for _, path := range GetVulnerabilityLocations(fixDetails, []string{}, []string{}) { @@ -171,11 +168,6 @@ func (n *NugetPackageUpdater) fixVulnerabilityAndRestore(projectFilePath, packag return nil } -// runDotnetRestore regenerates the lock file next to projectFilePath. It also removes the obj/ -// directory if restore created it (dotnet restore writes project.assets.json and other build -// artifacts there), so a fix PR doesn't pick up unrelated build output alongside the intended -// descriptor/lock file changes - mirroring the cleanup other updaters already do for their own -// install artifacts. func (n *NugetPackageUpdater) runDotnetRestore(projectFilePath string) error { objDir := filepath.Join(filepath.Dir(projectFilePath), nugetObjDirName) objDirExisted := dirExists(objDir) @@ -214,8 +206,6 @@ func dirExists(path string) bool { return err == nil && info.IsDir() } -// rollbackProjectFile restores the descriptor to its pre-fix content and returns origErr, or a -// wrapped error if the rollback itself fails. func rollbackProjectFile(projectFilePath string, originalProjectFile []byte, origErr error) error { //#nosec G703 G306 -- projectFilePath from scan workflow; 0644 for VCS-tracked sources. if rollbackErr := os.WriteFile(projectFilePath, originalProjectFile, 0644); rollbackErr != nil { @@ -224,8 +214,6 @@ func rollbackProjectFile(projectFilePath string, originalProjectFile []byte, ori return origErr } -// rollbackProjectFileAndLock restores both the descriptor and the lock file to their pre-fix -// content, so a failed restore never leaves the project in a half-fixed state. func rollbackProjectFileAndLock(projectFilePath string, originalProjectFile []byte, lockFilePath string, originalLockFile []byte, origErr error) error { //#nosec G703 G306 -- projectFilePath from scan workflow; 0644 for VCS-tracked sources. if rollbackErr := os.WriteFile(projectFilePath, originalProjectFile, 0644); rollbackErr != nil { @@ -238,12 +226,6 @@ func rollbackProjectFileAndLock(projectFilePath string, originalProjectFile []by return origErr } -// updatePackageReferenceVersion patches the Version on the element matching -// packageName, in place, without re-serializing the surrounding XML. If no inline version is -// found - as an attribute or a child element - the package may be centrally managed (Directory. -// Packages.props), version-supplied via Directory.Build.props, overridden elsewhere, or an -// SDK-implicit reference; none of those are resolved here, so this is reported distinctly from -// "not found" rather than guessing which one applies. func updatePackageReferenceVersion(content []byte, packageName, fixedVersion string) ([]byte, error) { element := regexp.MustCompile(nugetPackageReferenceElementPattern) keyAttr := regexp.MustCompile(fmt.Sprintf(nugetKeyAttrPattern, regexp.QuoteMeta(packageName))) diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go index 8b9bce657..a835e41f2 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater_test.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -17,12 +17,6 @@ import ( "github.com/jfrog/jfrog-cli-security/utils/techutils" ) -// writeFakeDotnetRestore stands in for the real dotnet CLI: it writes lockFileContent next to the -// project file passed as its second argument (mirroring where 'dotnet restore' would write -// packages.lock.json), appends the received arguments to /args.log, optionally creates an -// obj/ directory alongside the project (mirroring dotnet restore's own build-artifact output), -// then exits with exitCode - letting the regeneration/rollback/cleanup paths, and the exact flags -// used, be tested deterministically without a real .NET SDK, on POSIX and Windows alike. func writeFakeDotnetRestore(t *testing.T, dir string, exitCode int, lockFileContent string, createObjDir bool) { lockContentPath := filepath.Join(dir, "lockfile-content.json") assert.NoError(t, os.WriteFile(lockContentPath, []byte(lockFileContent), 0o644)) @@ -56,9 +50,6 @@ func writeFakeDotnetRestore(t *testing.T, dir string, exitCode int, lockFileCont assert.NoError(t, os.WriteFile(filepath.Join(dir, "dotnet"), []byte(script), 0o755)) } -// writeFakeDotnetRestoreFailingForPath behaves like writeFakeDotnetRestore, except it fails -// (without touching the lock file) only when the project file path it's invoked with contains -// pathMarker - letting one project's restore fail while a sibling's succeeds in the same test run. func writeFakeDotnetRestoreFailingForPath(t *testing.T, dir string, pathMarker string, lockFileContent string) { lockContentPath := filepath.Join(dir, "lockfile-content.json") assert.NoError(t, os.WriteFile(lockContentPath, []byte(lockFileContent), 0o644)) @@ -336,10 +327,6 @@ func TestNugetUpdateDependencyRegeneratesLockFile(t *testing.T) { assert.Contains(t, string(lockFile), `"resolved":"13.0.1"`) } -// TestNugetUpdateDependencyMultipleProjectsEachRegenerateOwnLockFile verifies that when the same -// vulnerable package is evidenced in two independent projects, each with its own lock file, both -// lock files are regenerated - not just the first one, or a shared/leaked state across loop -// iterations that only ends up applying to one of them. func TestNugetUpdateDependencyMultipleProjectsEachRegenerateOwnLockFile(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") @@ -384,9 +371,6 @@ func TestNugetUpdateDependencyMultipleProjectsEachRegenerateOwnLockFile(t *testi } } -// TestNugetUpdateDependencyRestoreFailureIsolatedPerProject verifies that when restore fails for -// one of several independent projects but succeeds for another, only the failing project is -// rolled back - a successful sibling's fix and regenerated lock file must survive. func TestNugetUpdateDependencyRestoreFailureIsolatedPerProject(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") @@ -600,11 +584,6 @@ func TestHasNugetProjectFileSuffix(t *testing.T) { } } -// TestNugetUpdateDependencyRestoreScopedFlags documents the exact restore invocation: both -// --force-evaluate and --no-dependencies must always be passed. --no-dependencies is what keeps a -// fix scoped to the touched project's own lock file - and is also the source of a known -// limitation (a bumped package's transitives living in a referenced project can leave that -// project's own lock file stale), so this test doubles as a marker for that tradeoff. func TestNugetUpdateDependencyRestoreScopedFlags(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") @@ -644,10 +623,6 @@ func TestNugetUpdateDependencyRestoreScopedFlags(t *testing.T) { assert.Contains(t, string(argsLog), "--no-dependencies") } -// TestNugetUpdateDependencyCleansUpGeneratedObjDir verifies that an obj/ directory created by -// restore (dotnet writes project.assets.json and other build artifacts there) is removed -// afterward, so a fix PR doesn't pick up unrelated build output alongside the intended -// descriptor/lock file changes. func TestNugetUpdateDependencyCleansUpGeneratedObjDir(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") @@ -689,9 +664,6 @@ func TestNugetUpdateDependencyCleansUpGeneratedObjDir(t *testing.T) { assert.True(t, os.IsNotExist(statErr), "obj/ created by restore should be cleaned up afterward") } -// TestNugetUpdateDependencyPreservesPreexistingObjDir verifies that an obj/ directory that -// already existed before the fix (e.g. from a prior local build) is left alone, even though -// restore also touches it. func TestNugetUpdateDependencyPreservesPreexistingObjDir(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") From ca972bce480955ea76c932368e4e30eb020adede Mon Sep 17 00:00:00 2001 From: Or Toren Date: Wed, 9 Sep 2026 10:01:16 +0300 Subject: [PATCH 09/12] Test ProjectReference isolation between projects A project with a vulnerable PackageReference alongside a ProjectReference to a sibling requires no new fix logic - evidence stays scoped to the declaring project, matching real scanner behavior already verified this session. Proves the fix stays that way in practice: the ProjectReference element itself is left untouched, and the referenced project's own descriptor and lock file are byte-identical afterward - the fix never reaches into it at either level. --- .../nugetpackageupdater_test.go | 57 +++++++++++++++++++ .../ReferencedProject.csproj | 12 ++++ .../ReferencedProject/packages.lock.json | 13 +++++ .../WithProjectReference.csproj | 13 +++++ .../WithProjectReference/packages.lock.json | 13 +++++ 5 files changed, 108 insertions(+) create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/ReferencedProject/ReferencedProject.csproj create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/ReferencedProject/packages.lock.json create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithProjectReference/WithProjectReference.csproj create mode 100644 tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithProjectReference/packages.lock.json diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go index a835e41f2..db99d5520 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater_test.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -428,6 +428,63 @@ func TestNugetUpdateDependencyRestoreFailureIsolatedPerProject(t *testing.T) { assert.Equal(t, originalFailProjectLock, rolledBackLock) } +func TestNugetUpdateDependencyDoesNotTouchReferencedProject(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + originalReferencedCsproj, err := os.ReadFile(filepath.Join("ReferencedProject", "ReferencedProject.csproj")) + assert.NoError(t, err) + originalReferencedLock, err := os.ReadFile(filepath.Join("ReferencedProject", "packages.lock.json")) + assert.NoError(t, err) + + toolDir := t.TempDir() + regeneratedLock := `{"version":1,"dependencies":{"net8.0":{"Newtonsoft.Json":{"type":"Direct","resolved":"13.0.1"}}}}` + writeFakeDotnetRestore(t, toolDir, 0, regeneratedLock, false) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: filepath.Join("WithProjectReference", "WithProjectReference.csproj")}}}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.NoError(t, err) + + fixedCsproj, err := os.ReadFile(filepath.Join("WithProjectReference", "WithProjectReference.csproj")) + assert.NoError(t, err) + fixedContent := string(fixedCsproj) + assert.Contains(t, fixedContent, `Include="Newtonsoft.Json" Version="13.0.1"`) + assert.Contains(t, fixedContent, ``) + + fixedLock, err := os.ReadFile(filepath.Join("WithProjectReference", "packages.lock.json")) + assert.NoError(t, err) + assert.Contains(t, string(fixedLock), `"resolved":"13.0.1"`) + + referencedCsproj, err := os.ReadFile(filepath.Join("ReferencedProject", "ReferencedProject.csproj")) + assert.NoError(t, err) + assert.Equal(t, originalReferencedCsproj, referencedCsproj) + referencedLock, err := os.ReadFile(filepath.Join("ReferencedProject", "packages.lock.json")) + assert.NoError(t, err) + assert.Equal(t, originalReferencedLock, referencedLock) +} + func TestNugetUpdateDependencyRollsBackOnRestoreFailure(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/ReferencedProject/ReferencedProject.csproj b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/ReferencedProject/ReferencedProject.csproj new file mode 100644 index 000000000..d372a7f28 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/ReferencedProject/ReferencedProject.csproj @@ -0,0 +1,12 @@ + + + + net8.0 + true + + + + + + + diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/ReferencedProject/packages.lock.json b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/ReferencedProject/packages.lock.json new file mode 100644 index 000000000..740d68ca6 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/ReferencedProject/packages.lock.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "dependencies": { + "net8.0": { + "Serilog": { + "type": "Direct", + "requested": "[2.10.0, )", + "resolved": "2.10.0", + "contentHash": "N0654CBHz7audO23dz8dLEQIVWZi4uY29VkQ7T0Wo9CtHvbpfoAcGRAO/nsr1oOMz2Sza2SLnrhTNMH4X8IX6w==" + } + } + } +} diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithProjectReference/WithProjectReference.csproj b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithProjectReference/WithProjectReference.csproj new file mode 100644 index 000000000..dc5a9cb60 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithProjectReference/WithProjectReference.csproj @@ -0,0 +1,13 @@ + + + + net8.0 + true + + + + + + + + diff --git a/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithProjectReference/packages.lock.json b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithProjectReference/packages.lock.json new file mode 100644 index 000000000..11fff8398 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation-packageupdaters/WithProjectReference/packages.lock.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "dependencies": { + "net8.0": { + "Newtonsoft.Json": { + "type": "Direct", + "requested": "[12.0.3, )", + "resolved": "12.0.3", + "contentHash": "6mgjfnRB4jKMlzHSl+VD+Fk4tsfGyu+CBjBTP1sOnCPQ7z+H5DwCZjxr75tGD1jXTAcgwmttSaTgWJ0t9Uu9jA==" + } + } + } +} From 0771c7807bb65028ca18a34fb03c3b242d290c1a Mon Sep 17 00:00:00 2001 From: Or Toren Date: Wed, 9 Sep 2026 10:18:11 +0300 Subject: [PATCH 10/12] Test mixed lock-file presence across independent projects --- .../nugetpackageupdater_test.go | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go index db99d5520..9405ef97d 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater_test.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "runtime" "strconv" + "strings" "testing" biutils "github.com/jfrog/build-info-go/utils" @@ -284,6 +285,61 @@ func TestNugetUpdateDependencyMultipleIndependentProjects(t *testing.T) { assert.Contains(t, siblingContent, `Include="NUnit" Version="3.13.3"`) } +func TestNugetUpdateDependencyMixedLockFilePresence(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + toolDir := t.TempDir() + regeneratedLock := `{"version":1,"dependencies":{"net8.0":{"Newtonsoft.Json":{"type":"Direct","resolved":"13.0.1"}}}}` + writeFakeDotnetRestore(t, toolDir, 0, regeneratedLock, false) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{ + {File: "Project.csproj"}, + {File: filepath.Join("WithLockFile", "WithLockFile.csproj")}, + }}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.NoError(t, err) + + fixedProject, err := os.ReadFile("Project.csproj") + assert.NoError(t, err) + assert.Contains(t, string(fixedProject), `Include="Newtonsoft.Json" Version="13.0.1"`) + _, statErr := os.Stat("packages.lock.json") + assert.True(t, os.IsNotExist(statErr), "no lock file should appear next to a project that never had one") + + fixedWithLock, err := os.ReadFile(filepath.Join("WithLockFile", "WithLockFile.csproj")) + assert.NoError(t, err) + assert.Contains(t, string(fixedWithLock), `Include="Newtonsoft.Json" Version="13.0.1"`) + lockFile, err := os.ReadFile(filepath.Join("WithLockFile", "packages.lock.json")) + assert.NoError(t, err) + assert.Contains(t, string(lockFile), `"resolved":"13.0.1"`) + + argsLog, err := os.ReadFile(filepath.Join(toolDir, "args.log")) + assert.NoError(t, err) + assert.Equal(t, 1, strings.Count(string(argsLog), "\n"), "restore should only run once, for the project that actually has a lock file") +} + func TestNugetUpdateDependencyRegeneratesLockFile(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") From 1e303f4cfdfa2d9f97ae7cbbbabfc68f059cccfe Mon Sep 17 00:00:00 2001 From: Or Toren Date: Wed, 16 Sep 2026 10:30:40 +0300 Subject: [PATCH 11/12] Address PR #874 review: NuGet fix contract, git-tracked lock check, case-insensitive element match - Align partial-descriptor-failure handling with Maven/npm: always return the joined error instead of swallowing it to nil when at least one sibling succeeded. - Resolve the lock file path to an absolute path before the git-tracked check, fixing a filepath.Rel fail-open for relative evidence paths; add a real-git test that verifies restore is skipped for a genuinely untracked lock file. - Match case-insensitively, since MSBuild element names are not case-sensitive. - Add a real 'dotnet restore' integration test case (--test.remediation) to confirm --force-evaluate --no-dependencies against an actual CLI, and generalize a Python-only lowercase-name assertion in the shared test helper. Co-Authored-By: Claude Sonnet 5 --- .../commonpackageupdater_test.go | 25 ++++- .../packageupdaters/nugetpackageupdater.go | 26 ++--- .../nugetpackageupdater_test.go | 100 ++++++++++++++++-- .../nuget/indirect-project/Placeholder.csproj | 11 ++ .../nuget/remediation/Remediation.csproj | 12 +++ .../nuget/remediation/packages.lock.json | 13 +++ 6 files changed, 164 insertions(+), 23 deletions(-) create mode 100644 tests/testdata/projects/package-managers/nuget/indirect-project/Placeholder.csproj create mode 100644 tests/testdata/projects/package-managers/nuget/remediation/Remediation.csproj create mode 100644 tests/testdata/projects/package-managers/nuget/remediation/packages.lock.json diff --git a/remediation/sca/packageupdaters/commonpackageupdater_test.go b/remediation/sca/packageupdaters/commonpackageupdater_test.go index 118c958d5..fb1c6b5e1 100644 --- a/remediation/sca/packageupdaters/commonpackageupdater_test.go +++ b/remediation/sca/packageupdaters/commonpackageupdater_test.go @@ -168,6 +168,22 @@ func TestUpdateDependency(t *testing.T) { descriptorsToCheck: []string{"package.json"}, }, }, + + // Nuget test cases - exercises the real 'dotnet' CLI (unlike the fake-dotnet unit tests in + // nugetpackageupdater_test.go), to confirm '--force-evaluate --no-dependencies' are flags + // a real restore actually accepts and acts on. + { + { + fixDetails: createFixDetails(techutils.Nuget, "Newtonsoft.Json", "", "13.0.1", false, ""), + fixSupported: false, + }, + { + fixDetails: createFixDetails(techutils.Nuget, "Newtonsoft.Json", "", "13.0.1", true, "Remediation.csproj"), + fixSupported: true, + descriptorsToCheck: []string{"Remediation.csproj"}, + lockFileToVerifyItsChange: "packages.lock.json", + }, + }, } for _, testBatch := range testCases { @@ -266,8 +282,13 @@ func assertFixVersionInPackageDescriptor(t *testing.T, test dependencyFixTest, p assert.NoError(t, err) assert.Contains(t, string(file), test.fixDetails.SuggestedFixedVersion) - // Verify that case-sensitive packages in python are lowered - assert.Contains(t, string(file), strings.ToLower(test.fixDetails.ImpactedDependencyName)) + expectedName := test.fixDetails.ImpactedDependencyName + switch test.fixDetails.Technology { + case techutils.Pip, techutils.Poetry, techutils.Pipenv: + // Python package names are normalized to lowercase on fix. + expectedName = strings.ToLower(expectedName) + } + assert.Contains(t, string(file), expectedName) } } diff --git a/remediation/sca/packageupdaters/nugetpackageupdater.go b/remediation/sca/packageupdaters/nugetpackageupdater.go index 4cf7b3930..07d6c5a8d 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater.go @@ -18,7 +18,9 @@ import ( var nugetProjectFileSuffixes = []string{".csproj", ".fsproj", ".vbproj"} const ( - nugetPackageReferenceElementPattern = `(?s)]*/>|]*[^/]>.*?` + // (?i) accounts for MSBuild element names being case-insensitive (e.g. is + // just as valid as ), even though this casing is rare in practice. + nugetPackageReferenceElementPattern = `(?is)]*/>|]*[^/]>.*?` nugetKeyAttrPattern = `(?i)\b(?:Include|Update)\s*=\s*["']%s["']` // nugetVersionAttrPattern matches whatever is already inside Version="...", including an // MSBuild property reference like "$(FooVersion)" - such a reference gets overwritten with the @@ -77,30 +79,20 @@ func (n *NugetPackageUpdater) UpdateDependency(fixDetails *FixDetails) error { var fixErrors error var failingDescriptors []string - var fixedAny bool for _, projectFilePath := range projectFilePaths { if fixErr := n.fixVulnerabilityAndRestore(projectFilePath, fixDetails.ImpactedDependencyName, fixDetails.SuggestedFixedVersion, originalWd); fixErr != nil { log.Warn(fixErr.Error()) fixErrors = errors.Join(fixErrors, fmt.Errorf("failed to fix '%s' in descriptor '%s': %w", fixDetails.ImpactedDependencyName, projectFilePath, fixErr)) failingDescriptors = append(failingDescriptors, projectFilePath) } else { - fixedAny = true log.Debug("Updated successfully " + projectFilePath) } } - if fixErrors == nil { - return nil + if fixErrors != nil { + return fmt.Errorf("encountered errors while fixing '%s' vulnerability in descriptors [%s]: %w", fixDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), fixErrors) } - if fixedAny { - // At least one descriptor was fixed - don't fail the whole vulnerability just because a - // sibling descriptor (e.g. one governed by Central Package Management) couldn't be fixed. - // A caller treating any error as "nothing happened" would otherwise discard an - // already-applied, successful fix. - log.Warn(fmt.Sprintf("Partially fixed '%s': could not fix descriptor(s) [%s]: %s", fixDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), fixErrors.Error())) - return nil - } - return fmt.Errorf("encountered errors while fixing '%s' vulnerability in descriptors [%s]: %w", fixDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), fixErrors) + return nil } func collectProjectFilePaths(fixDetails *FixDetails) []string { @@ -151,7 +143,11 @@ func (n *NugetPackageUpdater) fixVulnerabilityAndRestore(projectFilePath, packag return rollbackProjectFile(projectFilePath, originalProjectFile, fmt.Errorf("failed to read %s: %w", lockFilePath, err)) } - lockFileTracked, checkErr := IsFileTrackedByGit(lockFilePath, originalWd) + absLockFilePath := lockFilePath + if !filepath.IsAbs(absLockFilePath) { + absLockFilePath = filepath.Join(originalWd, absLockFilePath) + } + lockFileTracked, checkErr := IsFileTrackedByGit(absLockFilePath, originalWd) if checkErr != nil { log.Debug(fmt.Sprintf("Failed to check if lock file is tracked in git: %s. Proceeding with lock file regeneration.", checkErr.Error())) lockFileTracked = true diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go index 9405ef97d..95f7e12c2 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater_test.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -8,7 +8,10 @@ import ( "strconv" "strings" "testing" + "time" + git "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing/object" biutils "github.com/jfrog/build-info-go/utils" "github.com/jfrog/jfrog-client-go/utils/io/fileutils" "github.com/stretchr/testify/assert" @@ -102,6 +105,15 @@ func TestNugetUpdateDependency(t *testing.T) { ` + lowercaseElementCsproj := ` + + net8.0 + + + + +` + testCases := []struct { name string customCsproj string @@ -159,6 +171,19 @@ func TestNugetUpdateDependency(t *testing.T) { expectedContains: []string{`13.0.1`}, expectedNotContain: []string{`12.0.3`}, }, + { + name: "LowercaseElementName", + customCsproj: lowercaseElementCsproj, + fixDetails: &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: "Project.csproj"}}}}, + }, + expectedContains: []string{`Include="Newtonsoft.Json" Version="13.0.1"`}, + expectedNotContain: []string{`Version="12.0.3"`}, + }, } for _, tc := range testCases { @@ -195,7 +220,7 @@ func TestNugetUpdateDependency(t *testing.T) { } } -func TestNugetUpdateDependencyPartialSuccess(t *testing.T) { +func TestNugetUpdateDependencyPartialFailureKeepsSuccessfulWrites(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") currDir, err := os.Getwd() @@ -225,9 +250,9 @@ func TestNugetUpdateDependencyPartialSuccess(t *testing.T) { updater := &NugetPackageUpdater{} err = updater.UpdateDependency(fixDetails) - // A successful sibling fix must not be reported as an error just because the CPM-governed one - // couldn't be fixed - the failure is logged, not surfaced as the call's result. - assert.NoError(t, err) + // Matches Maven/npm: a sibling failure is still surfaced as an error, but it doesn't roll back + // whatever other descriptors were already fixed successfully. + assert.Error(t, err) fixedProject, err := os.ReadFile("Project.csproj") assert.NoError(t, err) @@ -383,6 +408,68 @@ func TestNugetUpdateDependencyRegeneratesLockFile(t *testing.T) { assert.Contains(t, string(lockFile), `"resolved":"13.0.1"`) } +// TestNugetUpdateDependencySkipsRestoreForUntrackedLockFile uses a real git repository (rather than +// a bare temp dir, as every other test in this file does) so that IsFileTrackedByGit takes its +// genuine "not tracked" path instead of failing open because the directory isn't a git repo at all. +func TestNugetUpdateDependencySkipsRestoreForUntrackedLockFile(t *testing.T) { + integration.InitUnitTest(t) + testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") + currDir, err := os.Getwd() + assert.NoError(t, err) + + tmpDir, err := os.MkdirTemp("", "nuget-test-*") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + assert.NoError(t, biutils.CopyDir(testProjectPath, tmpDir, true, nil)) + assert.NoError(t, os.Chdir(tmpDir)) + defer func() { + assert.NoError(t, os.Chdir(currDir)) + }() + + repo, err := git.PlainInit(tmpDir, false) + assert.NoError(t, err) + worktree, err := repo.Worktree() + assert.NoError(t, err) + _, err = worktree.Add(filepath.Join("WithLockFile", "WithLockFile.csproj")) + assert.NoError(t, err) + // packages.lock.json is deliberately left untracked (not added/committed). + signature := &object.Signature{Name: "test", Email: "test@example.com", When: time.Now()} + _, err = worktree.Commit("track project file only", &git.CommitOptions{Author: signature}) + assert.NoError(t, err) + + toolDir := t.TempDir() + writeFakeDotnetRestore(t, toolDir, 0, `{"version":1,"dependencies":{"net8.0":{"Newtonsoft.Json":{"type":"Direct","resolved":"13.0.1"}}}}`, false) + t.Setenv("PATH", toolDir+string(os.PathListSeparator)+os.Getenv("PATH")) + + originalLockFile, err := os.ReadFile(filepath.Join("WithLockFile", "packages.lock.json")) + assert.NoError(t, err) + + fixDetails := &FixDetails{ + SuggestedFixedVersion: "13.0.1", + IsDirectDependency: true, + Technology: techutils.Nuget, + ImpactedDependencyName: "Newtonsoft.Json", + Components: []formats.ComponentRow{{Evidences: []formats.Location{{File: filepath.Join("WithLockFile", "WithLockFile.csproj")}}}}, + } + + updater := &NugetPackageUpdater{} + err = updater.UpdateDependency(fixDetails) + assert.NoError(t, err) + + fixedCsproj, err := os.ReadFile(filepath.Join("WithLockFile", "WithLockFile.csproj")) + assert.NoError(t, err) + assert.Contains(t, string(fixedCsproj), `Include="Newtonsoft.Json" Version="13.0.1"`, "the reference itself is still updated regardless of lock file tracking") + + lockFileAfter, err := os.ReadFile(filepath.Join("WithLockFile", "packages.lock.json")) + assert.NoError(t, err) + assert.Equal(t, originalLockFile, lockFileAfter, "untracked lock file must be left untouched, not regenerated") + + _, statErr := os.Stat(filepath.Join(toolDir, "args.log")) + assert.True(t, os.IsNotExist(statErr), "dotnet restore must not run at all for an untracked lock file") +} + func TestNugetUpdateDependencyMultipleProjectsEachRegenerateOwnLockFile(t *testing.T) { integration.InitUnitTest(t) testProjectPath := filepath.Join("..", "..", "..", "tests", "testdata", "projects", "package-managers", "nuget", "remediation-packageupdaters") @@ -466,8 +553,9 @@ func TestNugetUpdateDependencyRestoreFailureIsolatedPerProject(t *testing.T) { updater := &NugetPackageUpdater{} err = updater.UpdateDependency(fixDetails) - // The failing project must not turn the successful sibling's fix into a reported error. - assert.NoError(t, err) + // The failing project is still reported as an error, but its rollback stays isolated to itself - + // it must not affect the sibling project that was fixed successfully. + assert.Error(t, err) fixedCsproj, err := os.ReadFile(filepath.Join("WithLockFile", "WithLockFile.csproj")) assert.NoError(t, err) diff --git a/tests/testdata/projects/package-managers/nuget/indirect-project/Placeholder.csproj b/tests/testdata/projects/package-managers/nuget/indirect-project/Placeholder.csproj new file mode 100644 index 000000000..1573afd7f --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/indirect-project/Placeholder.csproj @@ -0,0 +1,11 @@ + + + + net8.0 + + + + + + + diff --git a/tests/testdata/projects/package-managers/nuget/remediation/Remediation.csproj b/tests/testdata/projects/package-managers/nuget/remediation/Remediation.csproj new file mode 100644 index 000000000..d6059befe --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation/Remediation.csproj @@ -0,0 +1,12 @@ + + + + net8.0 + true + + + + + + + diff --git a/tests/testdata/projects/package-managers/nuget/remediation/packages.lock.json b/tests/testdata/projects/package-managers/nuget/remediation/packages.lock.json new file mode 100644 index 000000000..4f7e12682 --- /dev/null +++ b/tests/testdata/projects/package-managers/nuget/remediation/packages.lock.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "dependencies": { + "net8.0": { + "Newtonsoft.Json": { + "type": "Direct", + "requested": "[12.0.3, )", + "resolved": "12.0.3", + "contentHash": "6mgjfnRB4jKMlzHSl+VD+oUc1IebOZabkbyWj2RiTgWwYPPuaK1H97G1sHqGwPlS5npiF5Q0OrxN1wni2n5QWg==" + } + } + } +} \ No newline at end of file From e428c3f869ecc0e7f85afdf49e0a9dee242b9569 Mon Sep 17 00:00:00 2001 From: Or Toren Date: Thu, 17 Sep 2026 10:42:27 +0300 Subject: [PATCH 12/12] after code review --- .../sca/packageupdaters/nugetpackageupdater.go | 6 +++--- .../packageupdaters/nugetpackageupdater_test.go | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/remediation/sca/packageupdaters/nugetpackageupdater.go b/remediation/sca/packageupdaters/nugetpackageupdater.go index 07d6c5a8d..a1a170b5d 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater.go @@ -246,9 +246,6 @@ func updatePackageReferenceVersion(content []byte, packageName, fixedVersion str } }) - if fixedAny { - return updatedContent, nil - } if foundWithoutVersion { return nil, &ErrUnsupportedFix{ PackageName: packageName, @@ -256,5 +253,8 @@ func updatePackageReferenceVersion(content []byte, packageName, fixedVersion str ErrorType: NoInlineVersionFixNotSupported, } } + if fixedAny { + return updatedContent, nil + } return nil, fmt.Errorf("dependency %s not found", packageName) } diff --git a/remediation/sca/packageupdaters/nugetpackageupdater_test.go b/remediation/sca/packageupdaters/nugetpackageupdater_test.go index 95f7e12c2..cff5c3f83 100644 --- a/remediation/sca/packageupdaters/nugetpackageupdater_test.go +++ b/remediation/sca/packageupdaters/nugetpackageupdater_test.go @@ -907,3 +907,18 @@ func TestNugetUpdateDependencyPreservesPreexistingObjDir(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "keep-me", string(sentinel)) } + +func TestUpdatePackageReferenceVersionRejectsMixedInlineAndNonInline(t *testing.T) { + content := []byte(` + + + + +`) + + updated, err := updatePackageReferenceVersion(content, "Newtonsoft.Json", "13.0.1") + assert.Nil(t, updated) + var unsupportedErr *ErrUnsupportedFix + assert.True(t, errors.As(err, &unsupportedErr)) + assert.Equal(t, NoInlineVersionFixNotSupported, unsupportedErr.ErrorType) +}