Refactor NPM package updater #1024

eranturgeman · 2026-01-07T09:55:05Z

All tests passed. If this feature is not already covered by the tests, I added new tests.
This pull request is on the dev branch.
I used gofmt for formatting the code before submitting the pull request.
Update documentation about new features / new supported technologies

This PR changes the NPM package handler to test-based fixes instead of cli command fixes.
As past of the change we ease the installation after a fix is performed to only regenerate the lock file, hence reducing the strict build process we used to have and make the process less error prone

… regeneration

… location extraction for a given VulnerabilityDetails

github-actions · 2026-01-07T14:00:40Z

🐸 JFrog Frogbot

…r-npm-package-updater

eyalk007 · 2026-01-07T15:15:07Z

please provide a link to a fix pr

eyalk007

reviewed most of files
please tell me you are done so i can rereveiw

packagehandlers/npmpackageupdater.go

eyalk007 · 2026-01-07T15:08:51Z

packagehandlers/commonpackageupdater.go

 	regexpCompleteFormat := fmt.Sprintf(strings.ToLower(dependencyLineFormat), regexpFitImpactedName, regexpFitImpactedVersion)
 	return regexp.MustCompile(regexpCompleteFormat)
 }
+
+// Extracts unique file paths from the vulnerability's component locations.


remove comment

packagehandlers/commonpackageupdater.go

eyalk007 · 2026-01-07T15:21:24Z

packagehandlers/npmpackageupdater.go

+	// Matches: "package-name": "version" with optional ^ or ~ prefix
+	npmDependencyRegexpPattern = `\s*"%s"\s*:\s*"[~^]?%s"`
+	// Regex pattern for replacement - captures the groups for reconstruction
+	npmDependencyReplacePattern = `(\s*"%s"\s*:\s*")[~^]?[^"]+(")`


regex patterns are self explanatory
no need for comments

eyalk007 · 2026-01-07T15:30:36Z

packagehandlers/npmpackageupdater.go

+func (npm *NpmPackageHandler) getDescriptorsToFixFromVulnerability(vulnDetails *utils.VulnerabilityDetails) ([]string, error) {
+	lockFilePaths := GetVulnerabilityLocations(vulnDetails)


I would rename it to getDescriptorsPaths
Also we said its a bug, so that's why we need to do this
so basically we need to delete this function once they fix the bug?
if so add a todo of deleting the function
also delete comment after rename as it will be self explanatory
So something like:

// TODO: This is a workaround. Engine provides lock file paths but we need descriptor paths. // Delete this function once engine provides descriptor paths directly.

Yes this would be changed once the bug will be fixed. I coded it before we knew its a but and I maintained it since we dont know when the bug will be fixed. Ill add the comment

packagehandlers/npmpackageupdater.go

eyalk007 · 2026-01-07T17:57:50Z

packagehandlers/npmpackageupdater.go

+	// Change to the descriptor directory for the regeneration of the lock file
+	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
+	}
+
+	log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s'", vulnDetails.ImpactedDependencyName, vulnDetails.ImpactedDependencyVersion, vulnDetails.SuggestedFixedVersion))
+	return nil


separation of concerns
the goal of this func is to update the descriptor

this func is both updating the descriptor and tidying up the lockfile

I would separate to 2 function calls in the loop
and would check with product regarding backup

not really. the function that is updating the descriptor is called updateVersionInDescriptor. this is why I called this function fixVulnerabilityInDescriptor as it fixes vulnerability and performing all the steps around it.
we can change it to 'fixVulnerability' if you still think it is not clear enough

eyalk007 · 2026-01-08T08:06:27Z

packagehandlers/npmpackageupdater.go

+	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
+	}
+
+	log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s'", vulnDetails.ImpactedDependencyName, vulnDetails.ImpactedDependencyVersion, vulnDetails.SuggestedFixedVersion))
+	return nil


what if user doesnt have lockfile in git, are we taking this into consideration?
@orto17

I agree - we talked about it before and as far as I remember this is what we decided.
In V2 we have a logic of 'cleaning' whatever that is not in the remote (like node_modules for example).
we can apply same logic here

eyalk007 · 2026-01-08T08:13:41Z

packagehandlers/npmpackageupdater_test.go

@@ -0,0 +1,297 @@
+package packagehandlers


missing test cases:

Version Range Mismatch - meaning lockfile resolves to diff version

Multiple Occurrences - multiple occurrence of the same version once as lets say a dep and once as a dev dep

Diff types of deps - we have peer deps, deps, dev deps, peerDeps, optionalDeps
plus we have overrides

Maybe a test for the rollback functionality if we keep it

orto17 · 2026-01-12T07:45:48Z

packagehandlers/commonpackageupdater.go

 }
+
+// Extracts unique file paths from the vulnerability's component locations.
+func GetVulnerabilityLocations(vulnDetails *utils.VulnerabilityDetails) []string {


i though we want to delete the common package updater completely. i dont think its same to have both utils file and common package updater file. My suggestion is to have a "package updaters utils", and have all new generic logic added to there.
meaning, at the end of the refactor, there will be no usage in common package updater at all.

orto17 · 2026-01-12T07:47:00Z

packagehandlers/commonpackageupdater.go

+
+// Extracts unique file paths from the vulnerability's component locations.
+func GetVulnerabilityLocations(vulnDetails *utils.VulnerabilityDetails) []string {
+	pathsSet := datastructures.MakeSet[string]()


also it looks very similar to getPomPaths function from the Maven refactor. so i suggest @eyalk007 will make it generic and @eranturgeman will use the new generic function in utils.

orto17 · 2026-01-12T07:49:43Z

packagehandlers/commonpackageupdater_test.go

i think we should write new utils tests in a nee file named to utils_test, so it ill not mix with the current
specific tech tests, it will be eaisier to review and follow the "v3 tests" vs the "v2 tests"

orto17 · 2026-01-12T07:53:29Z

packagehandlers/npmpackageupdater.go

+	npmDependencyReplacePattern = `(\s*"%s"\s*:\s*")[~^]?[^"]+(")`
+)
+
+var npmInstallEnvVars = map[string]string{


is this map specific to npm? looks like we can extract it to utils and have all relevant updaters use it

orto17 · 2026-01-12T08:00:51Z

packagehandlers/npmpackageupdater.go

+	noUpdateNotifierEnv:    "1",
+}
+
+type NpmPackageHandler struct {


NpmPackageUpdater
and lets delete CommonPackageHandler and use utils functions instead

orto17 · 2026-01-12T08:08:00Z

packagehandlers/npmpackageupdater.go

+}
+
+// Returns all descriptors related to the vulnerability based on its lock file locations
+func (npm *NpmPackageHandler) getDescriptorsToFixFromVulnerability(vulnDetails *utils.VulnerabilityDetails) ([]string, error) {


todo - delete when Avi fixes the descriptor bug
also delete TestNpmGetDescriptorPathsFromVulnerability (even though it looks very good)

orto17 · 2026-01-12T08:52:42Z

packagehandlers/npmpackageupdater.go

+		return err
+	}
+
+	originalWd, err := os.Getwd()


originalWd or currentWd?

orto17 · 2026-01-12T08:53:22Z

packagehandlers/npmpackageupdater.go

+
+	for _, descriptorPath := range descriptorPaths {
+		if fixErr := npm.fixVulnerabilityInDescriptor(vulnDetails, descriptorPath, originalWd, vulnRegexp); fixErr != nil {
+			err = errors.Join(err, fixErr)


can you add a warn log here in case of an error?

orto17 · 2026-01-12T08:55:28Z

packagehandlers/commonpackageupdater.go

 // This function adjusts the name and version of a dependency to conform to a regular expression format and constructs the complete regular expression pattern for searching.
 // Note: 'dependencyLineFormat' should be a template with two placeholders to be populated. The first one will be replaced with 'impactedName', and the second one with 'impactedVersion'.
 // Note: All supplied arguments are converted to lowercase. Hence, when utilizing this function, the file in which we search for the patterns must also be converted to lowercase.
 // Note: This function may not support all package manager dependency formats. It is designed for package managers where the dependency's name consists of a single component.


i think this comment is redundant including the notes

orto17 · 2026-01-12T08:59:15Z

packagehandlers/npmpackageupdater.go

+		return fmt.Errorf("failed to get current working directory: %w", err)
+	}
+
+	vulnRegexp := GetVulnerabilityRegexCompiler(vulnDetails.ImpactedDependencyName, vulnDetails.ImpactedDependencyVersion, npmDependencyRegexpPattern)


cant there be a scenario where the regex is invalid?

orto17 · 2026-01-12T09:03:13Z

packagehandlers/npmpackageupdater.go

+		}
+	}
+	if err != nil {
+		return fmt.Errorf("failed to fix vulnerability in one of the following descriptors [%s]: %w", strings.Join(descriptorPaths, ", "), err)


this log message/error message is somehow too "aggressive" to my opinion. if some of the fixes succeeded, it looks bad when we add a "fail to fix..." log. can we change the message to something more gentle?

orto17 · 2026-01-12T09:11:54Z

packagehandlers/npmpackageupdater.go

+func (npm *NpmPackageHandler) fixVulnerabilityInDescriptor(vulnDetails *utils.VulnerabilityDetails, descriptorPath string, originalWd string, vulnRegexp *regexp.Regexp) (err error) {
+	descriptorContent, err := os.ReadFile(descriptorPath)
+	if err != nil {
+		return fmt.Errorf("failed to read file '%s': %w", descriptorPath, err)


for each error/warn message i think is it should be more informative on from where the error is, for example:
fmt.Errorf("failed to fix vulnerability in descriptor: failed to read file '%s': %w", descriptorPath, err)

orto17 · 2026-01-12T09:26:07Z

packagehandlers/npmpackageupdater.go

+
+	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 {


i dont think we should roll back. i think we should add a warning log/comment that the lock file was not updated. and have them the fix PR with only the descriptor changed.

orto17 · 2026-01-12T09:28:24Z

packagehandlers/npmpackageupdater.go

+	return updatedContent, nil
+}
+
+func (npm *NpmPackageHandler) regenerateLockFileWithRetry() error {


the func structure should be:

if err != nil { log.Debug(fmt.Sprintf("First npm install attempt failed: %s. Retrying...", err.Error())) if err = npm.runNpmInstall(); err != nil { return fmt.Errorf("npm install failed after retry: %w", err) }

......
}

eranturgeman added 4 commits January 7, 2026 11:44

refactoring npm package handler to use text based fixes and lock file…

aa9c313

… regeneration

improving GetVulnerabilityRegexCompiler and adding extractor for file…

1b56a43

… location extraction for a given VulnerabilityDetails

renamed file to better adjust new package refactoring and naming

23f38c8

adding tests for new functions

5c3e233

eranturgeman requested a deployment to frogbot January 7, 2026 09:55 — with GitHub Actions Waiting

eranturgeman requested review from eyalk007 and orto17 January 7, 2026 09:55

fix UpdateDependency test for NPM

0d44549

eranturgeman requested a deployment to frogbot January 7, 2026 13:48 — with GitHub Actions Waiting

remove comments

5e027cc

eranturgeman had a problem deploying to frogbot January 7, 2026 13:55 — with GitHub Actions Failure

eranturgeman added safe to test Approve running integration tests on a pull request improvement Automatically generated release notes labels Jan 7, 2026

github-actions bot removed the safe to test Approve running integration tests on a pull request label Jan 7, 2026

Merge branch 'v3_er' of https://github.com/jfrog/frogbot into refacto…

42ba784

…r-npm-package-updater

eranturgeman requested a deployment to frogbot January 7, 2026 14:05 — with GitHub Actions Waiting

improve tests

28504c1

eranturgeman requested a deployment to frogbot January 8, 2026 07:57 — with GitHub Actions Waiting

/

888f5a9

eranturgeman requested a deployment to frogbot January 8, 2026 07:58 — with GitHub Actions Waiting

eyalk007 requested changes Jan 8, 2026

View reviewed changes

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/npmpackageupdater.go

return err

}

originalWd, err := os.Getwd()

Copy link

Contributor

orto17 Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

originalWd or currentWd?

orto17 reviewed Jan 12, 2026

View reviewed changes

		func (npm NpmPackageHandler) getDescriptorsToFixFromVulnerability(vulnDetails utils.VulnerabilityDetails) ([]string, error) {
		lockFilePaths := GetVulnerabilityLocations(vulnDetails)

Refactor NPM package updater #1024

Are you sure you want to change the base?

Refactor NPM package updater #1024

Uh oh!

Conversation

eranturgeman commented Jan 7, 2026

Uh oh!

github-actions bot commented Jan 7, 2026

Uh oh!

eyalk007 commented Jan 7, 2026

Uh oh!

eyalk007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalk007 Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orto17 Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eyalk007 Jan 8, 2026 •

edited

Loading

orto17 Jan 12, 2026 •

edited

Loading