From f792435fc27a02a7cecb1c9abea21cb720995540 Mon Sep 17 00:00:00 2001 From: Eyal Kapon Date: Mon, 26 Jan 2026 18:35:39 +0200 Subject: [PATCH 1/5] feat: Add Yarn package handler with refactored Node.js utilities - Add comprehensive Yarn support (Classic v1 and Berry v2+) - Detect Yarn version by lockfile header (__metadata: for Berry) - Use correct commands per version: - Yarn 1: yarn install --ignore-scripts --frozen-lockfile=false - Yarn 2+: yarn install --mode update-lockfile - Support resolutions section (Yarn-specific) - Simplified env vars (CI=true only, flags handle the rest) - Extract shared Node.js utilities to nodepackageupdaterutils.go - UpdatePackageJsonDependency: JSON manipulation with sjson/gjson - GetDescriptorsToFixFromVulnerability: Derive package.json from lockfiles - UpdatePackageAndRegenerateLock: Orchestration with rollback - Shared by npm, Yarn (and future pnpm) - Refactor npm handler to use shared utilities - Reduce code duplication - Maintain all existing functionality - All tests pass - Delete old buggy yarnpackagehandler.go - Had critical bug: checked global Yarn version instead of project version - Replaced with lockfile-based detection - Add comprehensive test coverage - 383 lines of Yarn-specific tests - Tests for version detection, JSON updates, resolutions, env isolation - All npm tests updated and passing Net: -282 lines deleted, +671 lines added (new functionality) --- packagehandlers/commonpackageupdater.go | 23 +- packagehandlers/nodepackageupdaterutils.go | 144 ++++++++ packagehandlers/npmpackageupdater.go | 152 +------- packagehandlers/npmpackageupdater_test.go | 56 +-- packagehandlers/yarnpackagehandler.go | 82 ----- packagehandlers/yarnpackageupdater.go | 142 ++++++++ packagehandlers/yarnpackageupdater_test.go | 382 +++++++++++++++++++++ scanrepository/scanrepository_test.go | 3 +- 8 files changed, 702 insertions(+), 282 deletions(-) create mode 100644 packagehandlers/nodepackageupdaterutils.go delete mode 100644 packagehandlers/yarnpackagehandler.go create mode 100644 packagehandlers/yarnpackageupdater.go create mode 100644 packagehandlers/yarnpackageupdater_test.go diff --git a/packagehandlers/commonpackageupdater.go b/packagehandlers/commonpackageupdater.go index 8bdc5294d..95cb54192 100644 --- a/packagehandlers/commonpackageupdater.go +++ b/packagehandlers/commonpackageupdater.go @@ -3,6 +3,7 @@ package packagehandlers import ( "fmt" "io/fs" + "os" "os/exec" "path/filepath" "regexp" @@ -16,7 +17,6 @@ import ( "golang.org/x/exp/slices" ) -// PackageHandler interface to hold operations on packages type PackageHandler interface { UpdateDependency(details *utils.VulnerabilityDetails) error } @@ -32,7 +32,7 @@ func GetCompatiblePackageHandler(vulnDetails *utils.VulnerabilityDetails, detail case techutils.Npm: handler = &NpmPackageUpdater{} case techutils.Yarn: - handler = &YarnPackageHandler{} + handler = &YarnPackageUpdater{} case techutils.Pip: handler = &PythonPackageHandler{pipRequirementsFile: defaultRequirementFile} case techutils.Maven: @@ -57,7 +57,6 @@ type CommonPackageHandler struct { depsRepo string } -// UpdateDependency updates the impacted package to the fixed version func (cph *CommonPackageHandler) UpdateDependency(vulnDetails *utils.VulnerabilityDetails, installationCommand string, extraArgs ...string) (err error) { // Lower the package name to avoid duplicates impactedPackage := strings.ToLower(vulnDetails.ImpactedDependencyName) @@ -89,9 +88,7 @@ func getFixedPackage(impactedPackage string, versionOperator string, suggestedFi return } -// Recursively scans the current directory for descriptor files based on the provided list of suffixes, while excluding paths that match the specified exclusion patterns. -// The patternsToExclude must be provided as regexp patterns. For instance, if the pattern ".*node_modules.*" is provided, any paths containing "node_modules" will be excluded from the result. -// Returns a slice of all discovered descriptor files, represented as absolute paths. +// patternsToExclude must be regexp patterns (e.g., ".*node_modules.*" excludes paths containing "node_modules") func (cph *CommonPackageHandler) GetAllDescriptorFilesFullPaths(descriptorFilesSuffixes []string, patternsToExclude ...string) (descriptorFilesFullPaths []string, err error) { if len(descriptorFilesSuffixes) == 0 { return @@ -149,3 +146,17 @@ func GetVulnerabilityLocations(vulnDetails *utils.VulnerabilityDetails, namesFil } return pathsSet.ToSlice() } + +func buildIsolatedEnv(envVars map[string]string) []string { + var env []string + for _, e := range os.Environ() { + key := strings.SplitN(e, "=", 2)[0] + if _, shouldOverride := envVars[key]; !shouldOverride { + env = append(env, e) + } + } + for key, value := range envVars { + env = append(env, fmt.Sprintf("%s=%s", key, value)) + } + return env +} diff --git a/packagehandlers/nodepackageupdaterutils.go b/packagehandlers/nodepackageupdaterutils.go new file mode 100644 index 000000000..09ddef05e --- /dev/null +++ b/packagehandlers/nodepackageupdaterutils.go @@ -0,0 +1,144 @@ +package packagehandlers + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/jfrog/frogbot/v2/utils" + "github.com/jfrog/jfrog-client-go/utils/io/fileutils" + "github.com/jfrog/jfrog-client-go/utils/log" + "github.com/tidwall/gjson" + "github.com/tidwall/sjson" +) + +const ( + packageJsonFileName = "package.json" + dependenciesSectionName = "dependencies" + devDependenciesSectionName = "devDependencies" + optionalDependenciesSectionName = "optionalDependencies" +) + +func UpdatePackageJsonDependency(content []byte, packageName, newVersion string, allowedSections []string, descriptorPath string) ([]byte, error) { + updated := false + escapedName := escapeJsonPathKey(packageName) + + for _, section := range allowedSections { + path := section + "." + escapedName + if gjson.GetBytes(content, path).Exists() { + var err error + content, err = sjson.SetBytes(content, path, newVersion) + if err != nil { + return nil, fmt.Errorf("failed to set version for '%s' in section '%s': %w", packageName, section, err) + } + updated = true + } + } + + if !updated { + return nil, fmt.Errorf("package '%s' not found in allowed sections [%s] in '%s'", packageName, strings.Join(allowedSections, ", "), descriptorPath) + } + return content, nil +} + +func escapeJsonPathKey(key string) string { + r := strings.NewReplacer(".", "\\.", "*", "\\*", "?", "\\?") + return r.Replace(key) +} + +// Assumes package.json is in the same directory as the lockfile +func GetPackageJsonPathsFromLockfilePaths(lockFilePaths []string) ([]string, error) { + var descriptorPaths []string + for _, lockFilePath := range lockFilePaths { + descriptorPath := filepath.Join(filepath.Dir(lockFilePath), packageJsonFileName) + fileExists, err := fileutils.IsFileExists(descriptorPath, false) + if err != nil { + return nil, err + } + if !fileExists { + return nil, fmt.Errorf("descriptor file '%s' not found for lock file '%s'", descriptorPath, lockFilePath) + } + descriptorPaths = append(descriptorPaths, descriptorPath) + } + return descriptorPaths, nil +} + +// TODO: this function is a workaround that handles the bug where only lock files are provided in vulnerability locations, instead of the descriptor files. +// TODO: After the bug is fixed we can simply call GetVulnerabilityLocations(vulnDetails, []string{packageJsonFileName}) and verify it exists (delete func & test) +func GetDescriptorsToFixFromVulnerability(vulnDetails *utils.VulnerabilityDetails, lockFileName string) ([]string, error) { + lockFilePaths := GetVulnerabilityLocations(vulnDetails, []string{lockFileName}) + if len(lockFilePaths) == 0 { + return nil, fmt.Errorf("no location evidence was found for package %s", vulnDetails.ImpactedDependencyName) + } + + return GetPackageJsonPathsFromLockfilePaths(lockFilePaths) +} + +func UpdatePackageInDescriptor(packageName, newVersion, descriptorPath string, allowedSections []string) ([]byte, error) { + descriptorContent, err := os.ReadFile(descriptorPath) + if err != nil { + return nil, fmt.Errorf("failed to read file '%s': %w", descriptorPath, err) + } + + backupContent := make([]byte, len(descriptorContent)) + copy(backupContent, descriptorContent) + + updatedContent, err := UpdatePackageJsonDependency(descriptorContent, packageName, newVersion, allowedSections, descriptorPath) + if err != nil { + return nil, fmt.Errorf("failed to update version in descriptor: %w", err) + } + + if err = os.WriteFile(descriptorPath, updatedContent, 0644); err != nil { + return nil, fmt.Errorf("failed to write updated descriptor '%s': %w", descriptorPath, err) + } + return backupContent, nil +} + +func RegenerateLockfile(packageName, newVersion, descriptorPath, originalWd string, backupContent []byte, regenerateLockfileFn func() error) (err error) { + descriptorDir := filepath.Dir(descriptorPath) + if err = os.Chdir(descriptorDir); err != nil { + return fmt.Errorf("failed to change directory to '%s': %w", descriptorDir, err) + } + defer func() { + if chErr := os.Chdir(originalWd); chErr != nil { + err = errors.Join(err, fmt.Errorf("failed to return to original directory: %w", chErr)) + } + }() + + if err = regenerateLockfileFn(); err != nil { + log.Warn(fmt.Sprintf("Failed to regenerate lock file after updating '%s' to version '%s': %s. Rolling back...", packageName, newVersion, err.Error())) + if rollbackErr := os.WriteFile(descriptorPath, backupContent, 0644); rollbackErr != nil { + return fmt.Errorf("failed to rollback descriptor after lock file regeneration failure: %w (original error: %v)", rollbackErr, err) + } + return err + } + return nil +} + +func UpdatePackageAndRegenerateLock(packageName, oldVersion, newVersion, descriptorPath, originalWd, lockFileName string, allowedSections []string, regenerateLockfileFn func() error) error { + backupContent, err := UpdatePackageInDescriptor(packageName, newVersion, descriptorPath, allowedSections) + if err != nil { + return err + } + + lockFileTracked, checkErr := utils.IsFileTrackedByGit(lockFileName, 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' does not exist in remote, skipping lock file regeneration", lockFileName)) + log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s' without regenerating lock file", packageName, oldVersion, newVersion, descriptorPath)) + return nil + } + + if err = RegenerateLockfile(packageName, newVersion, descriptorPath, originalWd, backupContent, regenerateLockfileFn); err != nil { + return err + } + + log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s'", packageName, oldVersion, newVersion, descriptorPath)) + return nil +} diff --git a/packagehandlers/npmpackageupdater.go b/packagehandlers/npmpackageupdater.go index 1e825568c..671aa3296 100644 --- a/packagehandlers/npmpackageupdater.go +++ b/packagehandlers/npmpackageupdater.go @@ -6,15 +6,11 @@ import ( "fmt" "os" "os/exec" - "path/filepath" "strings" "time" "github.com/jfrog/frogbot/v2/utils" - "github.com/jfrog/jfrog-client-go/utils/io/fileutils" "github.com/jfrog/jfrog-client-go/utils/log" - "github.com/tidwall/gjson" - "github.com/tidwall/sjson" ) const ( @@ -29,7 +25,6 @@ const ( configLevelEnv = "NPM_CONFIG_LOGLEVEL" ciEnv = "CI" - npmDescriptorFileName = "package.json" npmLockFileName = "package-lock.json" dependenciesSection = "dependencies" devDependenciesSection = "devDependencies" @@ -65,7 +60,7 @@ func (npm *NpmPackageUpdater) UpdateDependency(vulnDetails *utils.VulnerabilityD } func (npm *NpmPackageUpdater) updateDirectDependency(vulnDetails *utils.VulnerabilityDetails) error { - descriptorPaths, err := npm.getDescriptorsToFixFromVulnerability(vulnDetails) + descriptorPaths, err := GetDescriptorsToFixFromVulnerability(vulnDetails, npmLockFileName) if err != nil { return err } @@ -77,7 +72,16 @@ func (npm *NpmPackageUpdater) updateDirectDependency(vulnDetails *utils.Vulnerab var failingDescriptors []string for _, descriptorPath := range descriptorPaths { - if fixErr := npm.fixVulnerabilityAndRegenerateLock(vulnDetails, descriptorPath, originalWd); fixErr != nil { + if fixErr := UpdatePackageAndRegenerateLock( + vulnDetails.ImpactedDependencyName, + vulnDetails.ImpactedDependencyVersion, + vulnDetails.SuggestedFixedVersion, + descriptorPath, + originalWd, + npmLockFileName, + npmAllowedSections, + npm.regenerateLockFileWithRetry, + ); fixErr != nil { failedFixErrorMsg := fmt.Errorf("failed to fix '%s' in descriptor '%s': %w", vulnDetails.ImpactedDependencyName, descriptorPath, fixErr) log.Warn(failedFixErrorMsg.Error()) err = errors.Join(err, failedFixErrorMsg) @@ -91,124 +95,6 @@ func (npm *NpmPackageUpdater) updateDirectDependency(vulnDetails *utils.Vulnerab return nil } -// TODO: this function is a workaround that handles the bug where only lock files are provided in vulnerability locations, instead of the descriptor files. -// TODO: After the bug is fixed we can simply call GetVulnerabilityLocations(vulnDetails, []string{npmDescriptorFileName}) and verify it exists (delete func & test) -func (npm *NpmPackageUpdater) getDescriptorsToFixFromVulnerability(vulnDetails *utils.VulnerabilityDetails) ([]string, error) { - lockFilePaths := GetVulnerabilityLocations(vulnDetails, []string{npmLockFileName}) - if len(lockFilePaths) == 0 { - return nil, fmt.Errorf("no location evidence was found for package %s", vulnDetails.ImpactedDependencyName) - } - - var descriptorPaths []string - for _, lockFilePath := range lockFilePaths { - // We currently assume the descriptor resides in the same directory as the lock file, and this is the only supported use case - descriptorPath := filepath.Join(filepath.Dir(lockFilePath), npmDescriptorFileName) - fileExists, err := fileutils.IsFileExists(descriptorPath, false) - if err != nil { - return nil, err - } - if !fileExists { - return nil, fmt.Errorf("descriptor file '%s' not found for lock file '%s': %w", descriptorPath, lockFilePath, err) - } - descriptorPaths = append(descriptorPaths, descriptorPath) - } - return descriptorPaths, nil -} - -func (npm *NpmPackageUpdater) fixVulnerabilityAndRegenerateLock(vulnDetails *utils.VulnerabilityDetails, descriptorPath string, originalWd string) error { - backupContent, err := npm.updateDescriptor(vulnDetails, descriptorPath) - if err != nil { - return err - } - - lockFileTracked, checkErr := utils.IsFileTrackedByGit(npmLockFileName, 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' does not exist in remote, skipping lock file regeneration", npmLockFileName)) - log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s' without regenerating lock file", vulnDetails.ImpactedDependencyName, vulnDetails.ImpactedDependencyVersion, vulnDetails.SuggestedFixedVersion, descriptorPath)) - return nil - } - - if err = npm.RegenerateLockfile(vulnDetails, descriptorPath, originalWd, backupContent); err != nil { - return err - } - - log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s'", vulnDetails.ImpactedDependencyName, vulnDetails.ImpactedDependencyVersion, vulnDetails.SuggestedFixedVersion, descriptorPath)) - return nil -} - -func (npm *NpmPackageUpdater) updateDescriptor(vulnDetails *utils.VulnerabilityDetails, descriptorPath string) ([]byte, error) { - descriptorContent, err := os.ReadFile(descriptorPath) - if err != nil { - return nil, fmt.Errorf("failed to read file '%s': %w", descriptorPath, err) - } - - backupContent := make([]byte, len(descriptorContent)) - copy(backupContent, descriptorContent) - - updatedContent, err := npm.getFixedDescriptor(descriptorContent, vulnDetails.ImpactedDependencyName, vulnDetails.SuggestedFixedVersion, descriptorPath) - if err != nil { - return nil, fmt.Errorf("failed to update version in descriptor: %w", err) - } - - if err = os.WriteFile(descriptorPath, updatedContent, 0644); err != nil { - return nil, fmt.Errorf("failed to write updated descriptor '%s': %w", descriptorPath, err) - } - return backupContent, nil -} - -func (npm *NpmPackageUpdater) RegenerateLockfile(vulnDetails *utils.VulnerabilityDetails, descriptorPath, originalWd string, backupContent []byte) (err error) { - descriptorDir := filepath.Dir(descriptorPath) - if err = os.Chdir(descriptorDir); err != nil { - return fmt.Errorf("failed to change directory to '%s': %w", descriptorDir, err) - } - defer func() { - if chErr := os.Chdir(originalWd); chErr != nil { - err = errors.Join(err, fmt.Errorf("failed to return to original directory: %w", chErr)) - } - }() - - if err = npm.regenerateLockFileWithRetry(); err != nil { - log.Warn(fmt.Sprintf("Failed to regenerate lock file after updating '%s' to version '%s': %s. Rolling back...", vulnDetails.ImpactedDependencyName, vulnDetails.SuggestedFixedVersion, err.Error())) - if rollbackErr := os.WriteFile(descriptorPath, backupContent, 0644); rollbackErr != nil { - return fmt.Errorf("failed to rollback descriptor after lock file regeneration failure: %w (original error: %v)", rollbackErr, err) - } - return err - } - return nil -} - -func (npm *NpmPackageUpdater) getFixedDescriptor(content []byte, packageName, newVersion, descriptorPath string) ([]byte, error) { - updated := false - escapedName := escapeJsonPathKey(packageName) - - for _, section := range npmAllowedSections { - path := section + "." + escapedName - if gjson.GetBytes(content, path).Exists() { - var err error - content, err = sjson.SetBytes(content, path, newVersion) - if err != nil { - return nil, fmt.Errorf("failed to set version for '%s' in section '%s': %w", packageName, section, err) - } - updated = true - } - } - - if !updated { - return nil, fmt.Errorf("package '%s' not found in allowed sections [%s] in '%s'", packageName, strings.Join(npmAllowedSections, ", "), descriptorPath) - } - return content, nil -} - -func escapeJsonPathKey(key string) string { - r := strings.NewReplacer(".", "\\.", "*", "\\*", "?", "\\?") - return r.Replace(key) -} - func (npm *NpmPackageUpdater) regenerateLockFileWithRetry() error { err := npm.runNpmInstall() if err != nil { @@ -238,7 +124,7 @@ func (npm *NpmPackageUpdater) runNpmInstall() error { //#nosec G204 -- False positive - the subprocess only runs after the user's approval cmd := exec.CommandContext(ctx, "npm", args...) - cmd.Env = npm.buildIsolatedEnv() + cmd.Env = buildIsolatedEnv(npmInstallEnvVars) output, err := cmd.CombinedOutput() if errors.Is(ctx.Err(), context.DeadlineExceeded) { @@ -251,17 +137,3 @@ func (npm *NpmPackageUpdater) runNpmInstall() error { return nil } - -func (npm *NpmPackageUpdater) buildIsolatedEnv() []string { - var env []string - for _, e := range os.Environ() { - key := strings.SplitN(e, "=", 2)[0] - if _, shouldOverride := npmInstallEnvVars[key]; !shouldOverride { - env = append(env, e) - } - } - for key, value := range npmInstallEnvVars { - env = append(env, fmt.Sprintf("%s=%s", key, value)) - } - return env -} diff --git a/packagehandlers/npmpackageupdater_test.go b/packagehandlers/npmpackageupdater_test.go index d9ab24235..9fc501049 100644 --- a/packagehandlers/npmpackageupdater_test.go +++ b/packagehandlers/npmpackageupdater_test.go @@ -13,8 +13,6 @@ import ( ) func TestGetFixedDescriptor(t *testing.T) { - npm := &NpmPackageUpdater{} - testcases := []struct { name string originalContent string @@ -139,7 +137,7 @@ func TestGetFixedDescriptor(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - result, err := npm.getFixedDescriptor([]byte(tc.originalContent), tc.packageName, tc.newVersion, "package.json") + result, err := UpdatePackageJsonDependency([]byte(tc.originalContent), tc.packageName, tc.newVersion, npmAllowedSections, "package.json") if tc.expectError { assert.Error(t, err) @@ -151,52 +149,6 @@ func TestGetFixedDescriptor(t *testing.T) { } } -func TestEscapeJsonPathKey(t *testing.T) { - testcases := []struct { - name string - input string - expected string - }{ - { - name: "simple package name", - input: "lodash", - expected: "lodash", - }, - { - name: "scoped package", - input: "@types/node", - expected: "@types/node", - }, - { - name: "package with dot", - input: "vue.config", - expected: "vue\\.config", - }, - { - name: "package with multiple dots", - input: "some.package.name", - expected: "some\\.package\\.name", - }, - { - name: "package with asterisk", - input: "package*name", - expected: "package\\*name", - }, - { - name: "package with question mark", - input: "package?name", - expected: "package\\?name", - }, - } - - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - result := escapeJsonPathKey(tc.input) - assert.Equal(t, tc.expected, result) - }) - } -} - func TestBuildIsolatedEnv(t *testing.T) { testcases := []struct { name string @@ -222,8 +174,7 @@ func TestBuildIsolatedEnv(t *testing.T) { assert.NoError(t, os.Setenv(ciEnv, "false")) } - npm := &NpmPackageUpdater{} - env := npm.buildIsolatedEnv() + env := buildIsolatedEnv(npmInstallEnvVars) envMap := make(map[string]string) envCount := make(map[string]int) @@ -249,7 +200,6 @@ func TestBuildIsolatedEnv(t *testing.T) { } func TestGetDescriptorsToFixFromVulnerability(t *testing.T) { - npm := &NpmPackageUpdater{} tmpDir, err := os.MkdirTemp("", "npm-descriptor-test-") assert.NoError(t, err) defer func() { @@ -335,7 +285,7 @@ func TestGetDescriptorsToFixFromVulnerability(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - result, err := npm.getDescriptorsToFixFromVulnerability(tc.vulnDetails) + result, err := GetDescriptorsToFixFromVulnerability(tc.vulnDetails, npmLockFileName) if tc.expectError { assert.Error(t, err) if tc.errorContains != "" { diff --git a/packagehandlers/yarnpackagehandler.go b/packagehandlers/yarnpackagehandler.go deleted file mode 100644 index dc51d3853..000000000 --- a/packagehandlers/yarnpackagehandler.go +++ /dev/null @@ -1,82 +0,0 @@ -package packagehandlers - -import ( - "errors" - "fmt" - biUtils "github.com/jfrog/build-info-go/build/utils" - "github.com/jfrog/frogbot/v2/utils" - "github.com/jfrog/gofrog/version" - "github.com/jfrog/jfrog-client-go/utils/io/fileutils" - "github.com/jfrog/jfrog-client-go/utils/log" -) - -const ( - yarnV2Version = "2.0.0" - yarnV1PackageUpdateCmd = "upgrade" - yarnV2PackageUpdateCmd = "up" - modulesFolderFlag = "--modules-folder=" -) - -type YarnPackageHandler struct { - CommonPackageHandler -} - -func (yarn *YarnPackageHandler) UpdateDependency(vulnDetails *utils.VulnerabilityDetails) error { - if vulnDetails.IsDirectDependency { - return yarn.updateDirectDependency(vulnDetails) - } else { - return &utils.ErrUnsupportedFix{ - PackageName: vulnDetails.ImpactedDependencyName, - FixedVersion: vulnDetails.SuggestedFixedVersion, - ErrorType: utils.IndirectDependencyFixNotSupported, - } - } -} - -func (yarn *YarnPackageHandler) updateDirectDependency(vulnDetails *utils.VulnerabilityDetails) (err error) { - isYarn1, executableYarnVersion, err := isYarnV1Project() - if err != nil { - return - } - - var installationCommand string - var extraArgs []string - - if isYarn1 { - installationCommand = yarnV1PackageUpdateCmd - // This dir is created to store node_modules that are created during updating packages in Yarn V1. This dir is to be deleted and not pushed into the PR - var tmpNodeModulesDir string - tmpNodeModulesDir, err = fileutils.CreateTempDir() - defer func() { - err = errors.Join(err, fileutils.RemoveTempDir(tmpNodeModulesDir)) - }() - - if err != nil { - return - } - extraArgs = append(extraArgs, modulesFolderFlag+tmpNodeModulesDir) - } else { - installationCommand = yarnV2PackageUpdateCmd - } - err = yarn.CommonPackageHandler.UpdateDependency(vulnDetails, installationCommand, extraArgs...) - if err != nil { - err = fmt.Errorf("running 'yarn %s for '%s' failed:\n%s\nHint: The Yarn version that was used is: %s. If your project was built with a different major version of Yarn, please configure your CI runner to include it", - installationCommand, - vulnDetails.ImpactedDependencyName, - err.Error(), - executableYarnVersion) - } - return -} - -// isYarnV1Project gets the current executed yarn version and returns whether the current yarn version is V1 or not -func isYarnV1Project() (isYarn1 bool, executableYarnVersion string, err error) { - // NOTICE: in case your global yarn version is 1.x this function will always return true even if the project is originally in higher yarn version - executableYarnVersion, err = biUtils.GetVersion("yarn", "") - if err != nil { - return - } - log.Info("Using Yarn version: ", executableYarnVersion) - isYarn1 = version.NewVersion(executableYarnVersion).Compare(yarnV2Version) > 0 - return -} diff --git a/packagehandlers/yarnpackageupdater.go b/packagehandlers/yarnpackageupdater.go new file mode 100644 index 000000000..b4c812b4e --- /dev/null +++ b/packagehandlers/yarnpackageupdater.go @@ -0,0 +1,142 @@ +package packagehandlers + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "strings" + "time" + + "github.com/jfrog/frogbot/v2/utils" + "github.com/jfrog/jfrog-client-go/utils/log" +) + +const ( + yarnLockFileName = "yarn.lock" + resolutionsSection = "resolutions" + yarnInstallTimeout = 15 * time.Minute + yarnIgnoreScriptsFlag = "--ignore-scripts" + yarnFrozenLockfileFlag = "--frozen-lockfile=false" + yarnModeFlag = "--mode" + yarnModeUpdateLockfile = "update-lockfile" + ciEnvYarn = "CI" + yarnBerryLockfileHeader = "__metadata:" +) + +var yarnAllowedSections = []string{dependenciesSectionName, devDependenciesSectionName, optionalDependenciesSectionName, resolutionsSection} + +var yarnInstallEnvVars = map[string]string{ + ciEnvYarn: "true", +} + +type YarnPackageUpdater struct { + CommonPackageHandler +} + +func (yarn *YarnPackageUpdater) UpdateDependency(vulnDetails *utils.VulnerabilityDetails) error { + if vulnDetails.IsDirectDependency { + return yarn.updateDirectDependency(vulnDetails) + } + return &utils.ErrUnsupportedFix{ + PackageName: vulnDetails.ImpactedDependencyName, + FixedVersion: vulnDetails.SuggestedFixedVersion, + ErrorType: utils.IndirectDependencyFixNotSupported, + } +} + +func (yarn *YarnPackageUpdater) updateDirectDependency(vulnDetails *utils.VulnerabilityDetails) error { + descriptorPaths, err := GetDescriptorsToFixFromVulnerability(vulnDetails, yarnLockFileName) + if err != nil { + return err + } + + originalWd, err := os.Getwd() + if err != nil { + return fmt.Errorf("failed to get current working directory: %w", err) + } + + var failingDescriptors []string + for _, descriptorPath := range descriptorPaths { + if fixErr := UpdatePackageAndRegenerateLock( + vulnDetails.ImpactedDependencyName, + vulnDetails.ImpactedDependencyVersion, + vulnDetails.SuggestedFixedVersion, + descriptorPath, + originalWd, + yarnLockFileName, + yarnAllowedSections, + yarn.regenerateLockFileWithRetry, + ); fixErr != nil { + failedFixErrorMsg := fmt.Errorf("failed to fix '%s' in descriptor '%s': %w", vulnDetails.ImpactedDependencyName, descriptorPath, fixErr) + log.Warn(failedFixErrorMsg.Error()) + err = errors.Join(err, failedFixErrorMsg) + failingDescriptors = append(failingDescriptors, descriptorPath) + } + } + if err != nil { + return fmt.Errorf("encountered errors while fixing '%s' vulnerability in descriptors [%s]: %w", vulnDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), err) + } + + return nil +} + +func (yarn *YarnPackageUpdater) regenerateLockFileWithRetry() error { + err := yarn.runYarnInstall() + if err != nil { + log.Debug(fmt.Sprintf("First yarn install attempt failed: %s. Retrying...", err.Error())) + if err = yarn.runYarnInstall(); err != nil { + return fmt.Errorf("yarn install failed after retry: %w", err) + } + } + return nil +} + +func (yarn *YarnPackageUpdater) detectYarnVersion() (isBerry bool, err error) { + content, err := os.ReadFile(yarnLockFileName) + if err != nil { + return false, fmt.Errorf("failed to read %s: %w", yarnLockFileName, err) + } + if strings.HasPrefix(string(content), yarnBerryLockfileHeader) { + return true, nil + } + return false, nil +} + +func (yarn *YarnPackageUpdater) runYarnInstall() error { + isBerry, err := yarn.detectYarnVersion() + if err != nil { + return fmt.Errorf("failed to detect yarn version: %w", err) + } + + args := []string{"install"} + + if isBerry { + args = append(args, yarnModeFlag, yarnModeUpdateLockfile) + } else { + args = append(args, yarnIgnoreScriptsFlag, yarnFrozenLockfileFlag) + } + + fullCommand := "yarn " + strings.Join(args, " ") + log.Debug(fmt.Sprintf("Running '%s' (Yarn %s)", fullCommand, map[bool]string{true: "Berry 2+", false: "Classic 1"}[isBerry])) + + ctx, cancel := context.WithTimeout(context.Background(), yarnInstallTimeout) + defer cancel() + + //#nosec G204 -- False positive - the subprocess only runs after the user's approval + cmd := exec.CommandContext(ctx, "yarn", args...) + + cmd.Env = buildIsolatedEnv(yarnInstallEnvVars) + output, err := cmd.CombinedOutput() + + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return fmt.Errorf("yarn install timed out after %v", yarnInstallTimeout) + } + + if err != nil { + return fmt.Errorf("yarn install failed: %w\nOutput: %s", err, string(output)) + } + + return nil +} diff --git a/packagehandlers/yarnpackageupdater_test.go b/packagehandlers/yarnpackageupdater_test.go new file mode 100644 index 000000000..76114e5af --- /dev/null +++ b/packagehandlers/yarnpackageupdater_test.go @@ -0,0 +1,382 @@ +package packagehandlers + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/jfrog/frogbot/v2/utils" + "github.com/jfrog/jfrog-cli-security/utils/formats" + "github.com/jfrog/jfrog-client-go/utils/io/fileutils" + "github.com/stretchr/testify/assert" +) + +func TestYarnGetFixedDescriptor(t *testing.T) { + testcases := []struct { + name string + originalContent string + packageName string + newVersion string + expectedContent string + expectError bool + }{ + { + name: "update exact version", + originalContent: `{"dependencies": {"minimist": "1.2.5"}}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: `{"dependencies": {"minimist": "1.2.6"}}`, + expectError: false, + }, + { + name: "update version with caret prefix - removes prefix", + originalContent: `{"dependencies": {"lodash": "^4.17.0"}}`, + packageName: "lodash", + newVersion: "4.17.21", + expectedContent: `{"dependencies": {"lodash": "4.17.21"}}`, + expectError: false, + }, + { + name: "update version with tilde prefix - removes prefix", + originalContent: `{"dependencies": {"express": "~4.18.0"}}`, + packageName: "express", + newVersion: "4.18.2", + expectedContent: `{"dependencies": {"express": "4.18.2"}}`, + expectError: false, + }, + { + name: "update scoped package", + originalContent: `{"dependencies": {"@types/node": "18.0.0"}}`, + packageName: "@types/node", + newVersion: "18.11.0", + expectedContent: `{"dependencies": {"@types/node": "18.11.0"}}`, + expectError: false, + }, + { + name: "package not found", + originalContent: `{"dependencies": {"lodash": "4.17.0"}}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: "", + expectError: true, + }, + { + name: "update in devDependencies", + originalContent: `{"devDependencies": {"minimist": "1.2.5"}}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: `{"devDependencies": {"minimist": "1.2.6"}}`, + expectError: false, + }, + { + name: "update in optionalDependencies", + originalContent: `{"optionalDependencies": {"minimist": "1.2.5"}}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: `{"optionalDependencies": {"minimist": "1.2.6"}}`, + expectError: false, + }, + { + name: "update in resolutions section (Yarn-specific)", + originalContent: `{"dependencies": {"express": "4.18.0"}, "resolutions": {"minimist": "1.2.5"}}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: `{"dependencies": {"express": "4.18.0"}, "resolutions": {"minimist": "1.2.6"}}`, + expectError: false, + }, + { + name: "update in both dependencies and resolutions", + originalContent: `{"dependencies": {"minimist": "1.2.5"}, "resolutions": {"minimist": "1.2.5"}}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: `{"dependencies": {"minimist": "1.2.6"}, "resolutions": {"minimist": "1.2.6"}}`, + expectError: false, + }, + { + name: "skip peerDependencies section - package only in peerDependencies", + originalContent: `{"dependencies": {"express": "4.18.0"}, "peerDependencies": {"minimist": "1.2.5"}}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: "", + expectError: true, + }, + { + name: "skip peerDependencies section - package in both dependencies and peerDependencies", + originalContent: `{"dependencies": {"minimist": "1.2.5"}, "peerDependencies": {"minimist": "1.2.5"}}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: `{"dependencies": {"minimist": "1.2.6"}, "peerDependencies": {"minimist": "1.2.5"}}`, + expectError: false, + }, + { + name: "update in multiple allowed sections", + originalContent: `{"dependencies": {"minimist": "1.2.5"}, "devDependencies": {"minimist": "1.2.5"}}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: `{"dependencies": {"minimist": "1.2.6"}, "devDependencies": {"minimist": "1.2.6"}}`, + expectError: false, + }, + { + name: "package name with dot", + originalContent: `{"dependencies": {"vue.config": "1.0.0"}}`, + packageName: "vue.config", + newVersion: "2.0.0", + expectedContent: `{"dependencies": {"vue.config": "2.0.0"}}`, + expectError: false, + }, + { + name: "no dependency sections", + originalContent: `{"name": "test", "version": "1.0.0"}`, + packageName: "minimist", + newVersion: "1.2.6", + expectedContent: "", + expectError: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + result, err := UpdatePackageJsonDependency([]byte(tc.originalContent), tc.packageName, tc.newVersion, yarnAllowedSections, "package.json") + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.expectedContent, string(result)) + } + }) + } +} + +func TestYarnBuildIsolatedEnv(t *testing.T) { + testcases := []struct { + name string + predefineEnv bool + }{ + { + name: "sets required env vars", + predefineEnv: false, + }, + { + name: "overrides conflicting user env vars", + predefineEnv: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + if tc.predefineEnv { + originalCI := os.Getenv(ciEnvYarn) + defer func() { + assert.NoError(t, os.Setenv(ciEnvYarn, originalCI)) + }() + assert.NoError(t, os.Setenv(ciEnvYarn, "false")) + } + + env := buildIsolatedEnv(yarnInstallEnvVars) + + envMap := make(map[string]string) + envCount := make(map[string]int) + for _, e := range env { + parts := strings.SplitN(e, "=", 2) + if len(parts) == 2 { + envMap[parts[0]] = parts[1] + envCount[parts[0]]++ + } + } + + assert.Equal(t, "true", envMap[ciEnvYarn]) + + if tc.predefineEnv { + assert.Equal(t, 1, envCount[ciEnvYarn], "CI should appear exactly once") + } + }) + } +} + +func TestYarnGetDescriptorsToFixFromVulnerability(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "yarn-descriptor-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + packageJsonPath := filepath.Join(tmpDir, "package.json") + assert.NoError(t, os.WriteFile(packageJsonPath, []byte(`{"name": "test"}`), 0644)) + + nestedDir := filepath.Join(tmpDir, "apps", "frontend") + assert.NoError(t, os.MkdirAll(nestedDir, 0755)) + nestedPackageJsonPath := filepath.Join(nestedDir, "package.json") + assert.NoError(t, os.WriteFile(nestedPackageJsonPath, []byte(`{"name": "frontend"}`), 0644)) + + testcases := []struct { + name string + vulnDetails *utils.VulnerabilityDetails + expectedPaths []string + expectError bool + errorContains string + }{ + { + name: "derives package.json from yarn.lock path", + vulnDetails: &utils.VulnerabilityDetails{ + VulnerabilityOrViolationRow: formats.VulnerabilityOrViolationRow{ + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + ImpactedDependencyName: "minimist", + Components: []formats.ComponentRow{ + {Name: "minimist", Version: "1.2.5", Location: &formats.Location{File: filepath.Join(tmpDir, "yarn.lock")}}, + }, + }, + }, + }, + expectedPaths: []string{packageJsonPath}, + expectError: false, + }, + { + name: "derives package.json from nested directory", + vulnDetails: &utils.VulnerabilityDetails{ + VulnerabilityOrViolationRow: formats.VulnerabilityOrViolationRow{ + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + ImpactedDependencyName: "minimist", + Components: []formats.ComponentRow{ + {Name: "minimist", Version: "1.2.5", Location: &formats.Location{File: filepath.Join(nestedDir, "yarn.lock")}}, + }, + }, + }, + }, + expectedPaths: []string{nestedPackageJsonPath}, + expectError: false, + }, + { + name: "error when no location evidence found", + vulnDetails: &utils.VulnerabilityDetails{ + VulnerabilityOrViolationRow: formats.VulnerabilityOrViolationRow{ + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + ImpactedDependencyName: "minimist", + Components: []formats.ComponentRow{}, + }, + }, + }, + expectedPaths: nil, + expectError: true, + errorContains: "no location evidence was found", + }, + { + name: "error when descriptor file does not exist", + vulnDetails: &utils.VulnerabilityDetails{ + VulnerabilityOrViolationRow: formats.VulnerabilityOrViolationRow{ + ImpactedDependencyDetails: formats.ImpactedDependencyDetails{ + ImpactedDependencyName: "minimist", + Components: []formats.ComponentRow{ + {Name: "minimist", Version: "1.2.5", Location: &formats.Location{File: "/nonexistent/path/yarn.lock"}}, + }, + }, + }, + }, + expectedPaths: nil, + expectError: true, + errorContains: "not found for lock file", + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + result, err := GetDescriptorsToFixFromVulnerability(tc.vulnDetails, yarnLockFileName) + if tc.expectError { + assert.Error(t, err) + if tc.errorContains != "" { + assert.Contains(t, err.Error(), tc.errorContains) + } + assert.Nil(t, result) + } else { + assert.NoError(t, err) + assert.ElementsMatch(t, tc.expectedPaths, result) + } + }) + } +} + +func TestYarnDetectVersion(t *testing.T) { + testcases := []struct { + name string + lockfileContent string + expectedIsBerry bool + }{ + { + name: "Yarn Berry - detected by __metadata header", + lockfileContent: `__metadata: + version: 6 + cacheKey: 8 + +"minimist@npm:^1.2.6": + version: 1.2.6 + resolution: "minimist@npm:1.2.6"`, + expectedIsBerry: true, + }, + { + name: "Yarn Classic - any other lockfile format", + lockfileContent: `# yarn lockfile v1 + +minimist@^1.2.5: + version "1.2.6" + resolved "https://registry.yarnpkg.com/minimist/-/minimist-1.2.6.tgz"`, + expectedIsBerry: false, + }, + { + name: "Yarn Classic - ambiguous header defaults to Classic", + lockfileContent: `# Some other header + +minimist@^1.2.5: + version "1.2.6"`, + expectedIsBerry: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "yarn-version-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + originalWd, err := os.Getwd() + assert.NoError(t, err) + defer func() { + assert.NoError(t, os.Chdir(originalWd)) + }() + + assert.NoError(t, os.Chdir(tmpDir)) + + lockfilePath := filepath.Join(tmpDir, yarnLockFileName) + assert.NoError(t, os.WriteFile(lockfilePath, []byte(tc.lockfileContent), 0644)) + + yarn := &YarnPackageUpdater{} + isBerry, err := yarn.detectYarnVersion() + + assert.NoError(t, err) + assert.Equal(t, tc.expectedIsBerry, isBerry) + }) + } +} + +func TestYarnDetectVersionError(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "yarn-version-error-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + originalWd, err := os.Getwd() + assert.NoError(t, err) + defer func() { + assert.NoError(t, os.Chdir(originalWd)) + }() + + assert.NoError(t, os.Chdir(tmpDir)) + + yarn := &YarnPackageUpdater{} + _, err = yarn.detectYarnVersion() + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to read yarn.lock") +} diff --git a/scanrepository/scanrepository_test.go b/scanrepository/scanrepository_test.go index f6a4b011d..6d89cd2fb 100644 --- a/scanrepository/scanrepository_test.go +++ b/scanrepository/scanrepository_test.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - services2 "github.com/jfrog/jfrog-client-go/xsc/services" "net/http" "net/http/httptest" "os" @@ -14,6 +13,8 @@ import ( "strings" "testing" + services2 "github.com/jfrog/jfrog-client-go/xsc/services" + "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/protocol/packp" "github.com/go-git/go-git/v5/plumbing/protocol/packp/capability" From b2dc712add5d2a8969a67d3a38b156cedd236e35 Mon Sep 17 00:00:00 2001 From: Eyal Kapon Date: Tue, 27 Jan 2026 10:07:46 +0200 Subject: [PATCH 2/5] test: Add comprehensive unit tests for Node.js utility functions - Test GetPackageJsonPathsFromLockfilePaths: path derivation, file checks, edge cases - Test UpdatePackageInDescriptor: file updates, backup creation, error handling - Test RegenerateLockfile: success, rollback on failure, rollback failure handling - Test UpdatePackageAndRegenerateLock: full orchestration, git tracking logic Total: 366 lines, 12 test cases covering critical rollback logic --- .../nodepackageupdaterutils_test.go | 366 ++++++++++++++++++ 1 file changed, 366 insertions(+) create mode 100644 packagehandlers/nodepackageupdaterutils_test.go diff --git a/packagehandlers/nodepackageupdaterutils_test.go b/packagehandlers/nodepackageupdaterutils_test.go new file mode 100644 index 000000000..a3f0df1e4 --- /dev/null +++ b/packagehandlers/nodepackageupdaterutils_test.go @@ -0,0 +1,366 @@ +package packagehandlers + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/jfrog/jfrog-client-go/utils/io/fileutils" + "github.com/stretchr/testify/assert" +) + +func TestGetPackageJsonPathsFromLockfilePaths(t *testing.T) { + testcases := []struct { + name string + lockfilePaths []string + expectedPaths []string + expectError bool + }{ + { + name: "single lockfile in root", + lockfilePaths: []string{"package-lock.json"}, + expectedPaths: []string{"package.json"}, + expectError: false, + }, + { + name: "single lockfile in subdirectory", + lockfilePaths: []string{"frontend/package-lock.json"}, + expectedPaths: []string{"frontend/package.json"}, + expectError: false, + }, + { + name: "multiple lockfiles in different directories", + lockfilePaths: []string{"app1/package-lock.json", "app2/package-lock.json"}, + expectedPaths: []string{"app1/package.json", "app2/package.json"}, + expectError: false, + }, + { + name: "empty input", + lockfilePaths: []string{}, + expectedPaths: nil, + expectError: false, + }, + { + name: "descriptor file doesn't exist", + lockfilePaths: []string{"nonexistent/package-lock.json"}, + expectError: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "lockfile-paths-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + originalWd, err := os.Getwd() + assert.NoError(t, err) + defer func() { + assert.NoError(t, os.Chdir(originalWd)) + }() + + assert.NoError(t, os.Chdir(tmpDir)) + + if !tc.expectError { + for _, expectedPath := range tc.expectedPaths { + dir := filepath.Dir(expectedPath) + if dir != "." { + assert.NoError(t, os.MkdirAll(dir, 0755)) + } + assert.NoError(t, os.WriteFile(expectedPath, []byte("{}"), 0644)) + } + } + + result, err := GetPackageJsonPathsFromLockfilePaths(tc.lockfilePaths) + + if tc.expectError { + assert.Error(t, err) + assert.Contains(t, err.Error(), "descriptor file") + assert.Contains(t, err.Error(), "not found") + return + } + + assert.NoError(t, err) + assert.Equal(t, tc.expectedPaths, result) + }) + } +} + +func TestUpdatePackageInDescriptor(t *testing.T) { + testcases := []struct { + name string + initialContent string + packageName string + newVersion string + allowedSections []string + expectError bool + expectedInContent string + notExpectedInContent string + }{ + { + name: "update package in dependencies", + initialContent: `{ + "dependencies": { + "lodash": "4.17.20" + } +}`, + packageName: "lodash", + newVersion: "4.17.21", + allowedSections: []string{"dependencies"}, + expectError: false, + expectedInContent: `"lodash": "4.17.21"`, + notExpectedInContent: `"lodash": "4.17.20"`, + }, + { + name: "package not found", + initialContent: `{ + "dependencies": { + "axios": "0.21.4" + } +}`, + packageName: "lodash", + newVersion: "4.17.21", + allowedSections: []string{"dependencies"}, + expectError: true, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "update-descriptor-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + descriptorPath := filepath.Join(tmpDir, "package.json") + assert.NoError(t, os.WriteFile(descriptorPath, []byte(tc.initialContent), 0644)) + + backupContent, err := UpdatePackageInDescriptor(tc.packageName, tc.newVersion, descriptorPath, tc.allowedSections) + + if tc.expectError { + assert.Error(t, err) + return + } + + assert.NoError(t, err) + assert.Equal(t, []byte(tc.initialContent), backupContent, "Backup should match original content") + + updatedContent, err := os.ReadFile(descriptorPath) + assert.NoError(t, err) + assert.Contains(t, string(updatedContent), tc.expectedInContent) + if tc.notExpectedInContent != "" { + assert.NotContains(t, string(updatedContent), tc.notExpectedInContent) + } + }) + } +} + +func TestRegenerateLockfile_Success(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "regenerate-lockfile-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + originalWd, err := os.Getwd() + assert.NoError(t, err) + + descriptorPath := filepath.Join(tmpDir, "package.json") + descriptorContent := []byte(`{"name": "test", "dependencies": {"lodash": "4.17.21"}}`) + assert.NoError(t, os.WriteFile(descriptorPath, descriptorContent, 0644)) + + backupContent := []byte(`{"name": "test", "dependencies": {"lodash": "4.17.20"}}`) + + regenerateCalled := false + mockRegenerate := func() error { + regenerateCalled = true + return nil + } + + err = RegenerateLockfile("lodash", "4.17.21", descriptorPath, originalWd, backupContent, mockRegenerate) + + assert.NoError(t, err) + assert.True(t, regenerateCalled, "Regenerate function should have been called") + + currentContent, err := os.ReadFile(descriptorPath) + assert.NoError(t, err) + assert.Equal(t, descriptorContent, currentContent, "Descriptor should remain updated on success") +} + +func TestRegenerateLockfile_FailureWithRollback(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "regenerate-lockfile-rollback-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + originalWd, err := os.Getwd() + assert.NoError(t, err) + + descriptorPath := filepath.Join(tmpDir, "package.json") + updatedContent := []byte(`{"name": "test", "dependencies": {"lodash": "4.17.21"}}`) + assert.NoError(t, os.WriteFile(descriptorPath, updatedContent, 0644)) + + backupContent := []byte(`{"name": "test", "dependencies": {"lodash": "4.17.20"}}`) + + regenerateError := errors.New("npm install failed") + mockRegenerate := func() error { + return regenerateError + } + + err = RegenerateLockfile("lodash", "4.17.21", descriptorPath, originalWd, backupContent, mockRegenerate) + + assert.Error(t, err) + assert.Equal(t, regenerateError, err, "Original error should be returned") + + rolledBackContent, err := os.ReadFile(descriptorPath) + assert.NoError(t, err) + assert.Equal(t, backupContent, rolledBackContent, "Descriptor should be rolled back to original") +} + +func TestRegenerateLockfile_RollbackFailure(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "regenerate-lockfile-rollback-fail-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + originalWd, err := os.Getwd() + assert.NoError(t, err) + + descriptorPath := filepath.Join(tmpDir, "package.json") + updatedContent := []byte(`{"name": "test", "dependencies": {"lodash": "4.17.21"}}`) + assert.NoError(t, os.WriteFile(descriptorPath, updatedContent, 0644)) + + backupContent := []byte(`{"name": "test", "dependencies": {"lodash": "4.17.20"}}`) + + regenerateError := errors.New("npm install failed") + mockRegenerate := func() error { + assert.NoError(t, os.Remove(descriptorPath)) + return regenerateError + } + + err = RegenerateLockfile("lodash", "4.17.21", descriptorPath, originalWd, backupContent, mockRegenerate) + + assert.Error(t, err) + assert.Equal(t, regenerateError, err, "Original regenerate error should be returned even if rollback fails") +} + +func TestUpdatePackageAndRegenerateLock_Success(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "update-package-success-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + originalWd, err := os.Getwd() + assert.NoError(t, err) + defer func() { + assert.NoError(t, os.Chdir(originalWd)) + }() + + assert.NoError(t, os.Chdir(tmpDir)) + + descriptorPath := filepath.Join(tmpDir, "package.json") + initialContent := `{"dependencies": {"lodash": "4.17.20"}}` + assert.NoError(t, os.WriteFile(descriptorPath, []byte(initialContent), 0644)) + + regenerateCalled := false + mockRegenerate := func() error { + regenerateCalled = true + return nil + } + + err = UpdatePackageAndRegenerateLock( + "lodash", + "4.17.20", + "4.17.21", + descriptorPath, + originalWd, + "package-lock.json", + []string{"dependencies"}, + mockRegenerate, + ) + + assert.NoError(t, err) + + updatedContent, err := os.ReadFile(descriptorPath) + assert.NoError(t, err) + assert.Contains(t, string(updatedContent), `"lodash": "4.17.21"`) + + // Regeneration happens when git check fails (defaults to true) + // This is the correct behavior: better to regenerate when uncertain + assert.True(t, regenerateCalled, "Regenerate should be called when git check is uncertain") +} + +func TestUpdatePackageAndRegenerateLock_UpdateDescriptorFailure(t *testing.T) { + originalWd, err := os.Getwd() + assert.NoError(t, err) + + err = UpdatePackageAndRegenerateLock( + "nonexistent", + "1.0.0", + "2.0.0", + "/nonexistent/package.json", + originalWd, + "package-lock.json", + []string{"dependencies"}, + func() error { return nil }, + ) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "failed to read file") +} + +func TestGetPackageJsonPathsFromLockfilePaths_EdgeCases(t *testing.T) { + testcases := []struct { + name string + lockfilePaths []string + expectedPaths []string + }{ + { + name: "lockfile with . in directory name", + lockfilePaths: []string{"app.v2/package-lock.json"}, + expectedPaths: []string{"app.v2/package.json"}, + }, + { + name: "lockfile name variations", + lockfilePaths: []string{"package-lock.json", "yarn.lock", "pnpm-lock.yaml"}, + expectedPaths: []string{"package.json", "package.json", "package.json"}, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "edge-cases-test-") + assert.NoError(t, err) + defer func() { + assert.NoError(t, fileutils.RemoveTempDir(tmpDir)) + }() + + originalWd, err := os.Getwd() + assert.NoError(t, err) + defer func() { + assert.NoError(t, os.Chdir(originalWd)) + }() + + assert.NoError(t, os.Chdir(tmpDir)) + + for _, expectedPath := range tc.expectedPaths { + dir := filepath.Dir(expectedPath) + if dir != "." { + assert.NoError(t, os.MkdirAll(dir, 0755)) + } + assert.NoError(t, os.WriteFile(expectedPath, []byte("{}"), 0644)) + } + + result, err := GetPackageJsonPathsFromLockfilePaths(tc.lockfilePaths) + assert.NoError(t, err) + assert.Equal(t, tc.expectedPaths, result) + }) + } +} From 63cb3d1b53487c83d2bc98c82530fc16d3946284 Mon Sep 17 00:00:00 2001 From: Eyal Kapon Date: Tue, 27 Jan 2026 10:37:28 +0200 Subject: [PATCH 3/5] refactor: Unexport internal Node.js utility functions - Only GetDescriptorsToFixFromVulnerability and UpdatePackageAndRegenerateLock are exported (called by npm/Yarn) - All other functions are now internal (lowercase): getPackageJsonPathsFromLockfilePaths, updatePackageInDescriptor, regenerateLockfile, updatePackageJsonDependency, escapeJsonPathKey - Revert scanrepository_test.go import ordering to match upstream/v3_er - All tests passing --- packagehandlers/nodepackageupdaterutils.go | 111 +++++++++--------- .../nodepackageupdaterutils_test.go | 12 +- packagehandlers/npmpackageupdater_test.go | 2 +- packagehandlers/yarnpackageupdater_test.go | 2 +- scanrepository/scanrepository_test.go | 3 +- 5 files changed, 65 insertions(+), 65 deletions(-) diff --git a/packagehandlers/nodepackageupdaterutils.go b/packagehandlers/nodepackageupdaterutils.go index 09ddef05e..503d2b753 100644 --- a/packagehandlers/nodepackageupdaterutils.go +++ b/packagehandlers/nodepackageupdaterutils.go @@ -21,35 +21,46 @@ const ( optionalDependenciesSectionName = "optionalDependencies" ) -func UpdatePackageJsonDependency(content []byte, packageName, newVersion string, allowedSections []string, descriptorPath string) ([]byte, error) { - updated := false - escapedName := escapeJsonPathKey(packageName) +// TODO: this function is a workaround that handles the bug where only lock files are provided in vulnerability locations, instead of the descriptor files. +// TODO: After the bug is fixed we can simply call GetVulnerabilityLocations(vulnDetails, []string{packageJsonFileName}) and verify it exists (delete func & test) +func GetDescriptorsToFixFromVulnerability(vulnDetails *utils.VulnerabilityDetails, lockFileName string) ([]string, error) { + lockFilePaths := GetVulnerabilityLocations(vulnDetails, []string{lockFileName}) + if len(lockFilePaths) == 0 { + return nil, fmt.Errorf("no location evidence was found for package %s", vulnDetails.ImpactedDependencyName) + } - for _, section := range allowedSections { - path := section + "." + escapedName - if gjson.GetBytes(content, path).Exists() { - var err error - content, err = sjson.SetBytes(content, path, newVersion) - if err != nil { - return nil, fmt.Errorf("failed to set version for '%s' in section '%s': %w", packageName, section, err) - } - updated = true - } + return getPackageJsonPathsFromLockfilePaths(lockFilePaths) +} + +func UpdatePackageAndRegenerateLock(packageName, oldVersion, newVersion, descriptorPath, originalWd, lockFileName string, allowedSections []string, regenerateLockfileFn func() error) error { + backupContent, err := updatePackageInDescriptor(packageName, newVersion, descriptorPath, allowedSections) + if err != nil { + return err } - if !updated { - return nil, fmt.Errorf("package '%s' not found in allowed sections [%s] in '%s'", packageName, strings.Join(allowedSections, ", "), descriptorPath) + lockFileTracked, checkErr := utils.IsFileTrackedByGit(lockFileName, 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 } - return content, nil -} -func escapeJsonPathKey(key string) string { - r := strings.NewReplacer(".", "\\.", "*", "\\*", "?", "\\?") - return r.Replace(key) + if !lockFileTracked { + log.Debug(fmt.Sprintf("Lock file '%s' does not exist in remote, skipping lock file regeneration", lockFileName)) + log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s' without regenerating lock file", packageName, oldVersion, newVersion, descriptorPath)) + return nil + } + + if err = regenerateLockfile(packageName, newVersion, descriptorPath, originalWd, backupContent, regenerateLockfileFn); err != nil { + return err + } + + log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s'", packageName, oldVersion, newVersion, descriptorPath)) + return nil } -// Assumes package.json is in the same directory as the lockfile -func GetPackageJsonPathsFromLockfilePaths(lockFilePaths []string) ([]string, error) { +// ==================== Internal Helper Functions ==================== + +func getPackageJsonPathsFromLockfilePaths(lockFilePaths []string) ([]string, error) { var descriptorPaths []string for _, lockFilePath := range lockFilePaths { descriptorPath := filepath.Join(filepath.Dir(lockFilePath), packageJsonFileName) @@ -65,18 +76,7 @@ func GetPackageJsonPathsFromLockfilePaths(lockFilePaths []string) ([]string, err return descriptorPaths, nil } -// TODO: this function is a workaround that handles the bug where only lock files are provided in vulnerability locations, instead of the descriptor files. -// TODO: After the bug is fixed we can simply call GetVulnerabilityLocations(vulnDetails, []string{packageJsonFileName}) and verify it exists (delete func & test) -func GetDescriptorsToFixFromVulnerability(vulnDetails *utils.VulnerabilityDetails, lockFileName string) ([]string, error) { - lockFilePaths := GetVulnerabilityLocations(vulnDetails, []string{lockFileName}) - if len(lockFilePaths) == 0 { - return nil, fmt.Errorf("no location evidence was found for package %s", vulnDetails.ImpactedDependencyName) - } - - return GetPackageJsonPathsFromLockfilePaths(lockFilePaths) -} - -func UpdatePackageInDescriptor(packageName, newVersion, descriptorPath string, allowedSections []string) ([]byte, error) { +func updatePackageInDescriptor(packageName, newVersion, descriptorPath string, allowedSections []string) ([]byte, error) { descriptorContent, err := os.ReadFile(descriptorPath) if err != nil { return nil, fmt.Errorf("failed to read file '%s': %w", descriptorPath, err) @@ -85,7 +85,7 @@ func UpdatePackageInDescriptor(packageName, newVersion, descriptorPath string, a backupContent := make([]byte, len(descriptorContent)) copy(backupContent, descriptorContent) - updatedContent, err := UpdatePackageJsonDependency(descriptorContent, packageName, newVersion, allowedSections, descriptorPath) + updatedContent, err := updatePackageJsonDependency(descriptorContent, packageName, newVersion, allowedSections, descriptorPath) if err != nil { return nil, fmt.Errorf("failed to update version in descriptor: %w", err) } @@ -96,7 +96,7 @@ func UpdatePackageInDescriptor(packageName, newVersion, descriptorPath string, a return backupContent, nil } -func RegenerateLockfile(packageName, newVersion, descriptorPath, originalWd string, backupContent []byte, regenerateLockfileFn func() error) (err error) { +func regenerateLockfile(packageName, newVersion, descriptorPath, originalWd string, backupContent []byte, regenerateLockfileFn func() error) (err error) { descriptorDir := filepath.Dir(descriptorPath) if err = os.Chdir(descriptorDir); err != nil { return fmt.Errorf("failed to change directory to '%s': %w", descriptorDir, err) @@ -117,28 +117,29 @@ func RegenerateLockfile(packageName, newVersion, descriptorPath, originalWd stri return nil } -func UpdatePackageAndRegenerateLock(packageName, oldVersion, newVersion, descriptorPath, originalWd, lockFileName string, allowedSections []string, regenerateLockfileFn func() error) error { - backupContent, err := UpdatePackageInDescriptor(packageName, newVersion, descriptorPath, allowedSections) - if err != nil { - return err - } - - lockFileTracked, checkErr := utils.IsFileTrackedByGit(lockFileName, 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 - } +func updatePackageJsonDependency(content []byte, packageName, newVersion string, allowedSections []string, descriptorPath string) ([]byte, error) { + updated := false + escapedName := escapeJsonPathKey(packageName) - if !lockFileTracked { - log.Debug(fmt.Sprintf("Lock file '%s' does not exist in remote, skipping lock file regeneration", lockFileName)) - log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s' without regenerating lock file", packageName, oldVersion, newVersion, descriptorPath)) - return nil + for _, section := range allowedSections { + path := section + "." + escapedName + if gjson.GetBytes(content, path).Exists() { + var err error + content, err = sjson.SetBytes(content, path, newVersion) + if err != nil { + return nil, fmt.Errorf("failed to set version for '%s' in section '%s': %w", packageName, section, err) + } + updated = true + } } - if err = RegenerateLockfile(packageName, newVersion, descriptorPath, originalWd, backupContent, regenerateLockfileFn); err != nil { - return err + if !updated { + return nil, fmt.Errorf("package '%s' not found in allowed sections [%s] in '%s'", packageName, strings.Join(allowedSections, ", "), descriptorPath) } + return content, nil +} - log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s'", packageName, oldVersion, newVersion, descriptorPath)) - return nil +func escapeJsonPathKey(key string) string { + r := strings.NewReplacer(".", "\\.", "*", "\\*", "?", "\\?") + return r.Replace(key) } diff --git a/packagehandlers/nodepackageupdaterutils_test.go b/packagehandlers/nodepackageupdaterutils_test.go index a3f0df1e4..c1eda807a 100644 --- a/packagehandlers/nodepackageupdaterutils_test.go +++ b/packagehandlers/nodepackageupdaterutils_test.go @@ -74,7 +74,7 @@ func TestGetPackageJsonPathsFromLockfilePaths(t *testing.T) { } } - result, err := GetPackageJsonPathsFromLockfilePaths(tc.lockfilePaths) + result, err := getPackageJsonPathsFromLockfilePaths(tc.lockfilePaths) if tc.expectError { assert.Error(t, err) @@ -139,7 +139,7 @@ func TestUpdatePackageInDescriptor(t *testing.T) { descriptorPath := filepath.Join(tmpDir, "package.json") assert.NoError(t, os.WriteFile(descriptorPath, []byte(tc.initialContent), 0644)) - backupContent, err := UpdatePackageInDescriptor(tc.packageName, tc.newVersion, descriptorPath, tc.allowedSections) + backupContent, err := updatePackageInDescriptor(tc.packageName, tc.newVersion, descriptorPath, tc.allowedSections) if tc.expectError { assert.Error(t, err) @@ -181,7 +181,7 @@ func TestRegenerateLockfile_Success(t *testing.T) { return nil } - err = RegenerateLockfile("lodash", "4.17.21", descriptorPath, originalWd, backupContent, mockRegenerate) + err = regenerateLockfile("lodash", "4.17.21", descriptorPath, originalWd, backupContent, mockRegenerate) assert.NoError(t, err) assert.True(t, regenerateCalled, "Regenerate function should have been called") @@ -212,7 +212,7 @@ func TestRegenerateLockfile_FailureWithRollback(t *testing.T) { return regenerateError } - err = RegenerateLockfile("lodash", "4.17.21", descriptorPath, originalWd, backupContent, mockRegenerate) + err = regenerateLockfile("lodash", "4.17.21", descriptorPath, originalWd, backupContent, mockRegenerate) assert.Error(t, err) assert.Equal(t, regenerateError, err, "Original error should be returned") @@ -244,7 +244,7 @@ func TestRegenerateLockfile_RollbackFailure(t *testing.T) { return regenerateError } - err = RegenerateLockfile("lodash", "4.17.21", descriptorPath, originalWd, backupContent, mockRegenerate) + err = regenerateLockfile("lodash", "4.17.21", descriptorPath, originalWd, backupContent, mockRegenerate) assert.Error(t, err) assert.Equal(t, regenerateError, err, "Original regenerate error should be returned even if rollback fails") @@ -358,7 +358,7 @@ func TestGetPackageJsonPathsFromLockfilePaths_EdgeCases(t *testing.T) { assert.NoError(t, os.WriteFile(expectedPath, []byte("{}"), 0644)) } - result, err := GetPackageJsonPathsFromLockfilePaths(tc.lockfilePaths) + result, err := getPackageJsonPathsFromLockfilePaths(tc.lockfilePaths) assert.NoError(t, err) assert.Equal(t, tc.expectedPaths, result) }) diff --git a/packagehandlers/npmpackageupdater_test.go b/packagehandlers/npmpackageupdater_test.go index 9fc501049..6c45dc19e 100644 --- a/packagehandlers/npmpackageupdater_test.go +++ b/packagehandlers/npmpackageupdater_test.go @@ -137,7 +137,7 @@ func TestGetFixedDescriptor(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - result, err := UpdatePackageJsonDependency([]byte(tc.originalContent), tc.packageName, tc.newVersion, npmAllowedSections, "package.json") + result, err := updatePackageJsonDependency([]byte(tc.originalContent), tc.packageName, tc.newVersion, npmAllowedSections, "package.json") if tc.expectError { assert.Error(t, err) diff --git a/packagehandlers/yarnpackageupdater_test.go b/packagehandlers/yarnpackageupdater_test.go index 76114e5af..50b16afa7 100644 --- a/packagehandlers/yarnpackageupdater_test.go +++ b/packagehandlers/yarnpackageupdater_test.go @@ -137,7 +137,7 @@ func TestYarnGetFixedDescriptor(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - result, err := UpdatePackageJsonDependency([]byte(tc.originalContent), tc.packageName, tc.newVersion, yarnAllowedSections, "package.json") + result, err := updatePackageJsonDependency([]byte(tc.originalContent), tc.packageName, tc.newVersion, yarnAllowedSections, "package.json") if tc.expectError { assert.Error(t, err) diff --git a/scanrepository/scanrepository_test.go b/scanrepository/scanrepository_test.go index 6d89cd2fb..f6a4b011d 100644 --- a/scanrepository/scanrepository_test.go +++ b/scanrepository/scanrepository_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + services2 "github.com/jfrog/jfrog-client-go/xsc/services" "net/http" "net/http/httptest" "os" @@ -13,8 +14,6 @@ import ( "strings" "testing" - services2 "github.com/jfrog/jfrog-client-go/xsc/services" - "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/protocol/packp" "github.com/go-git/go-git/v5/plumbing/protocol/packp/capability" From b4f7c86b850cf1801172faf78a4f6d74bec8e47a Mon Sep 17 00:00:00 2001 From: Eyal Kapon Date: Tue, 27 Jan 2026 10:43:06 +0200 Subject: [PATCH 4/5] chore: Upgrade jfrog-cli-security to v1.26.0 - Upgraded from v1.24.2 to v1.26.0 - Also upgraded related dependencies: - build-info-go - jfrog-cli-core/v2 - jfrog-cli-artifactory - jfrog-client-go - All tests passing --- go.mod | 10 +++++----- go.sum | 20 ++++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/go.mod b/go.mod index 1f28c7628..add1e701f 100644 --- a/go.mod +++ b/go.mod @@ -7,13 +7,13 @@ require ( github.com/go-git/go-git/v5 v5.16.3 github.com/golang/mock v1.6.0 github.com/google/go-github/v45 v45.2.0 - github.com/jfrog/build-info-go v1.12.5-0.20251209171349-eb030db986f9 + github.com/jfrog/build-info-go v1.13.1-0.20260120103048-d7f367bfa36e github.com/jfrog/froggit-go v1.20.6 github.com/jfrog/gofrog v1.7.6 - github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20251211075913-35ebcd308e93 - github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20251210085744-f8481d179ac5 - github.com/jfrog/jfrog-cli-security v1.24.2 - github.com/jfrog/jfrog-client-go v1.55.1-0.20251217080430-c92b763b7465 + github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260120063955-c654c159290e + github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260112010739-87fc7275623c + github.com/jfrog/jfrog-cli-security v1.26.0 + github.com/jfrog/jfrog-client-go v1.55.1-0.20260120055025-12f25e12798a github.com/owenrumney/go-sarif/v3 v3.2.3 github.com/stretchr/testify v1.11.1 github.com/tidwall/gjson v1.18.0 diff --git a/go.sum b/go.sum index dcca0ea96..400c8b53a 100644 --- a/go.sum +++ b/go.sum @@ -136,22 +136,22 @@ github.com/jedib0t/go-pretty/v6 v6.7.5 h1:9dJSWTJnsXJVVAbvxIFxeHf/JxoJd7GUl5o3Uz github.com/jedib0t/go-pretty/v6 v6.7.5/go.mod h1:YwC5CE4fJ1HFUDeivSV1r//AmANFHyqczZk+U6BDALU= github.com/jfrog/archiver/v3 v3.6.1 h1:LOxnkw9pOn45DzCbZNFV6K0+6dCsQ0L8mR3ZcujO5eI= github.com/jfrog/archiver/v3 v3.6.1/go.mod h1:VgR+3WZS4N+i9FaDwLZbq+jeU4B4zctXL+gL4EMzfLw= -github.com/jfrog/build-info-go v1.12.5-0.20251209171349-eb030db986f9 h1:CL7lp7Y7srwQ1vy1btX66t4wbztzEGQbqi/9tdEz7xk= -github.com/jfrog/build-info-go v1.12.5-0.20251209171349-eb030db986f9/go.mod h1:9W4U440fdTHwW1HiB/R0VQvz/5q8ZHsms9MWcq+JrdY= +github.com/jfrog/build-info-go v1.13.1-0.20260120103048-d7f367bfa36e h1:STiWjuLtlEFR1H3kSKw6vDGhGdtUmV6O+ljPfrQ14sI= +github.com/jfrog/build-info-go v1.13.1-0.20260120103048-d7f367bfa36e/go.mod h1:+OCtMb22/D+u7Wne5lzkjJjaWr0LRZcHlDwTH86Mpwo= github.com/jfrog/froggit-go v1.20.6 h1:Xp7+LlEh0m1KGrQstb+u0aGfjRUtv1eh9xQBV3571jQ= github.com/jfrog/froggit-go v1.20.6/go.mod h1:obSG1SlsWjktkuqmKtpq7MNTTL63e0ot+ucTnlOMV88= github.com/jfrog/gofrog v1.7.6 h1:QmfAiRzVyaI7JYGsB7cxfAJePAZTzFz0gRWZSE27c6s= github.com/jfrog/gofrog v1.7.6/go.mod h1:ntr1txqNOZtHplmaNd7rS4f8jpA5Apx8em70oYEe7+4= github.com/jfrog/jfrog-apps-config v1.0.1 h1:mtv6k7g8A8BVhlHGlSveapqf4mJfonwvXYLipdsOFMY= github.com/jfrog/jfrog-apps-config v1.0.1/go.mod h1:8AIIr1oY9JuH5dylz2S6f8Ym2MaadPLR6noCBO4C22w= -github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20251211075913-35ebcd308e93 h1:rpkJZN0TigpAGY/bfgmLO4nwhyhkr0gkBTLz/0B5zS8= -github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20251211075913-35ebcd308e93/go.mod h1:7cCaRhXorlbyXZgiW5bplCExFxlnROaG21K12d8inpQ= -github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20251210085744-f8481d179ac5 h1:GYE67ubwl+ZRw3CcXFUi49EwwQp6k+qS8sX0QuHDHO8= -github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20251210085744-f8481d179ac5/go.mod h1:BMoGi2rG0udCCeaghqlNgiW3fTmT+TNnfTnBoWFYgcg= -github.com/jfrog/jfrog-cli-security v1.24.2 h1:nyI0lNYR8i6yZYeBDsBJnURYsMnFKEmt7QH4vaNxtGM= -github.com/jfrog/jfrog-cli-security v1.24.2/go.mod h1:3FXD5IkKtdQOm9CZk6cR7q0iC6PaGMnjqzZqRcQp2r0= -github.com/jfrog/jfrog-client-go v1.55.1-0.20251217080430-c92b763b7465 h1:Ff3BlNPndrAfa1xFI/ORFzfWTxQxF0buWG61PEJwd3U= -github.com/jfrog/jfrog-client-go v1.55.1-0.20251217080430-c92b763b7465/go.mod h1:WQ5Y+oKYyHFAlCbHN925bWhnShTd2ruxZ6YTpb76fpU= +github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260120063955-c654c159290e h1:F/VQ7UJ4jaEr9tLJ8jLfy4BF4Obhhd0pWu007SBSHt8= +github.com/jfrog/jfrog-cli-artifactory v0.8.1-0.20260120063955-c654c159290e/go.mod h1:LbhCULfa/eIPSXNgQ5Xw8BIZRmJ0qfF2I4sPa7AHXkY= +github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260112010739-87fc7275623c h1:K9anqOZ7ASxlsijsl9u4jh92wqqIvJA4kTYfXrcOmJA= +github.com/jfrog/jfrog-cli-core/v2 v2.60.1-0.20260112010739-87fc7275623c/go.mod h1:+Hnaikp/xCSPD/q7txxRy4Zc0wzjW/usrCSf+6uONSQ= +github.com/jfrog/jfrog-cli-security v1.26.0 h1:FcLshS1Ahm0++nV5q7UluFTCVRxH2wEIbqO7ZBag++I= +github.com/jfrog/jfrog-cli-security v1.26.0/go.mod h1:r9E0BdlNy6mq6gkRGslZRZaYe6WeGhLkpUm8+oEUOvU= +github.com/jfrog/jfrog-client-go v1.55.1-0.20260120055025-12f25e12798a h1:tbHqd+9SJB6pMJn9aXkD4aMYfwsKwah5kuhZV6Q+e88= +github.com/jfrog/jfrog-client-go v1.55.1-0.20260120055025-12f25e12798a/go.mod h1:sCE06+GngPoyrGO0c+vmhgMoVSP83UMNiZnIuNPzU8U= github.com/jhump/protoreflect v1.15.1 h1:HUMERORf3I3ZdX05WaQ6MIpd/NJ434hTp5YiKgfCL6c= github.com/jhump/protoreflect v1.15.1/go.mod h1:jD/2GMKKE6OqX8qTjhADU1e6DShO+gavG9e0Q693nKo= github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88/go.mod h1:3w7q1U84EfirKl04SVQ/s7nPm1ZPhiXd34z40TNz36k= From 515b7e7961fdc535139cc2d2db946f8eefe6f649 Mon Sep 17 00:00:00 2001 From: Eyal Kapon Date: Tue, 27 Jan 2026 11:00:32 +0200 Subject: [PATCH 5/5] fix: Handle both package.json and lockfile in component locations - Primary: Look for package.json in component locations (Yarn/newer scans) - Fallback: Look for lockfile and derive package.json path (npm/older scans) - Verify lockfile exists in same directory before accepting descriptor - Fixes 'no location evidence found' error for Yarn projects - All tests passing --- packagehandlers/nodepackageupdaterutils.go | 23 ++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/packagehandlers/nodepackageupdaterutils.go b/packagehandlers/nodepackageupdaterutils.go index 503d2b753..9eb084e8f 100644 --- a/packagehandlers/nodepackageupdaterutils.go +++ b/packagehandlers/nodepackageupdaterutils.go @@ -21,9 +21,28 @@ const ( optionalDependenciesSectionName = "optionalDependencies" ) -// TODO: this function is a workaround that handles the bug where only lock files are provided in vulnerability locations, instead of the descriptor files. -// TODO: After the bug is fixed we can simply call GetVulnerabilityLocations(vulnDetails, []string{packageJsonFileName}) and verify it exists (delete func & test) func GetDescriptorsToFixFromVulnerability(vulnDetails *utils.VulnerabilityDetails, lockFileName string) ([]string, error) { + // Get package.json paths from component locations + descriptorPaths := GetVulnerabilityLocations(vulnDetails, []string{packageJsonFileName}) + if len(descriptorPaths) > 0 { + // Verify the corresponding lockfile exists in the same directory + var validDescriptorPaths []string + for _, descriptorPath := range descriptorPaths { + lockFilePath := filepath.Join(filepath.Dir(descriptorPath), lockFileName) + fileExists, err := fileutils.IsFileExists(lockFilePath, false) + if err != nil { + return nil, err + } + if fileExists { + validDescriptorPaths = append(validDescriptorPaths, descriptorPath) + } + } + if len(validDescriptorPaths) > 0 { + return validDescriptorPaths, nil + } + } + + // Fallback: try to get lockfile paths and derive package.json from them (old behavior) lockFilePaths := GetVulnerabilityLocations(vulnDetails, []string{lockFileName}) if len(lockFilePaths) == 0 { return nil, fmt.Errorf("no location evidence was found for package %s", vulnDetails.ImpactedDependencyName)