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

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

		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

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eyalk007 Jan 8, 2026 •

edited

Loading