Maven Auto Fix by orto17 · Pull Request #1064 · jfrog/frogbot

orto17 · 2026-02-18T10:52:22Z

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

github-actions · 2026-02-18T13:13:42Z

🐸 JFrog Frogbot

attiasas

Nice!, take a look at my comments.

consider fixing linter issues
consider adding more test cases to expand coverage

attiasas · 2026-02-19T09:12:47Z

packageupdaters/mavenpackageupdater_test.go

+)
+
+func TestMavenUpdateRegularDependency(t *testing.T) {
+	testProjectPath := filepath.Join("..", "testdata", "packagehandlers")


Suggested change

testProjectPath := filepath.Join("..", "testdata", "packagehandlers")

testProjectPath := filepath.Join("..", "testdata", "packageupdaters")

test fails, you should pass the following.

I also suggest adding sub folder like maven and optionally even more under maven like regular to be prepared for more test cases in future

attiasas · 2026-02-19T09:20:52Z

packageupdaters/mavenpackageupdater.go

+	if project.Parent.GroupId == groupId && project.Parent.ArtifactId == artifactId {
+		pattern := regexp.MustCompile(`(?s)(<parent>\s*<groupId>` + regexp.QuoteMeta(groupId) + `</groupId>\s*<artifactId>` + regexp.QuoteMeta(artifactId) + `</artifactId>\s*<version>)[^<]+(</version>)`)
+		newContent := pattern.ReplaceAll(content, []byte("${1}"+fixedVersion+"${2}"))
+		if !bytes.Equal(content, newContent) {
+			log.Debug("Updated parent", toDependencyName(groupId, artifactId), "to", fixedVersion)
+			return true, newContent


make sure this will not fail on pom with vars. $ in a property may cause interaption to the replacer.

try and add test case for a pom similar to:

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> <modelVersion>4.0.0</modelVersion> <groupId>com.example</groupId> <artifactId>demo-app</artifactId> <version>1.0.0</version> <packaging>jar</packaging>    <properties> <java.version>17</java.version> <spring.boot.version>3.2.5</spring.boot.version> <junit.version>5.10.2</junit.version> <lombok.version>1.18.32</lombok.version> </properties>    <dependencyManagement> <dependencies> <dependency> <groupId>org.springframework.boot</groupId> <artifactId>spring-boot-dependencies</artifactId> <version>${spring.boot.version}</version> <type>pom</type> <scope>import</scope> </dependency> </dependencies> </dependencyManagement>    <dependencies>  <dependency> <groupId>org.springframework.boot</groupId> <artifactId>spring-boot-starter-web</artifactId> </dependency>  <dependency> <groupId>org.projectlombok</groupId> <artifactId>lombok</artifactId> <version>${lombok.version}</version> <scope>provided</scope> </dependency>  <dependency> <groupId>org.junit.jupiter</groupId> <artifactId>junit-jupiter</artifactId> <version>${junit.version}</version> <scope>test</scope> </dependency> </dependencies>    <build> <plugins>  <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-compiler-plugin</artifactId> <version>3.11.0</version> <configuration> <source>${java.version}</source> <target>${java.version}</target> </configuration> </plugin> </plugins> </build> </project>

attiasas · 2026-02-19T09:22:48Z

packageupdaters/mavenpackageupdater.go

+	var pomPaths []string
+	for _, component := range vulnDetails.Components {
+		if component.PreferredLocation != nil && component.PreferredLocation.File != "" {
+			pomPaths = append(pomPaths, component.PreferredLocation.File)


this may cause duplication, maybe use pomPaths := datastructures.MakeSet[string]()?

attiasas · 2026-02-19T09:28:09Z

packageupdaters/mavenpackageupdater.go

+	pomPaths := m.getVulnerabilityOccurrences(vulnDetails)
+	if len(pomPaths) == 0 {
+		return fmt.Errorf("no pom.xml locations found for %s - Components array is empty or missing Location data", vulnDetails.ImpactedDependencyName)
 	}


Suggested change

pomPaths := m.getVulnerabilityOccurrences(vulnDetails)

if len(pomPaths) == 0 {

return fmt.Errorf("no pom.xml locations found for %s - Components array is empty or missing Location data", vulnDetails.ImpactedDependencyName)

}

pomPaths := m.getVulnerabilityOccurrences(vulnDetails)

if len(pomPaths) == 0 {

return fmt.Errorf("no pom.xml locations found for %s - Components array is empty or missing Location data", vulnDetails.ImpactedDependencyName)

}

llog.Verbose(fmt.Sprintf("Found vulnerability %s occurrences for component %s in %s", vulnDetails.IssueId, vulnDetails.ImpactedDependencyVersion, strings.Join(pomPaths, ", ")))

Consider adding logs

eranturgeman · 2026-02-22T09:12:38Z

packageupdaters/mavenpackageupdater_test.go

+	"github.com/jfrog/frogbot/v2/utils"
+)
+
+func TestMavenUpdateRegularDependency(t *testing.T) {


please unify TestMavenUpdateRegularDependency, TestMavenUpdateDependencyManagement, TestMavenUpdatePropertyVersion and TestMavenUpdateParentPOM if possible, the code seems pretty similar to all of them.
Make usre to give a clear name for each test case in the t.Run

eranturgeman · 2026-02-22T09:13:07Z

packageupdaters/mavenpackageupdater_test.go

+	assert.NotContains(t, string(modifiedPom), "<version>2.5.0</version>")
+}
+
+func TestMavenDependencyNotFound(t *testing.T) {


please unify with TestMavenIndirectDependencyNotSupported for one test that checks the failing fixes

utils/outputwriter/markdowntable.go

eranturgeman · 2026-02-22T09:27:52Z

packageupdaters/mavenpackageupdater.go

-		_, clearMavenDepTreeRun, err = mph.CreateTempDirWithSettingsXmlIfNeeded()
-		if err != nil {
-			return
+func (m *MavenPackageUpdater) getVulnerabilityOccurrences(vulnDetails *utils.VulnerabilityDetails) []string {


remove this function and dont use preferredLocation in general (this is intended for the CLI not for Frogbot).
please use GetVulnerabilityLocations to get all occurences of descriptors to fix, and then iterate them all and fix each of them

eranturgeman · 2026-02-22T09:30:34Z

packageupdaters/mavenpackageupdater.go

+	var errors []string
+	for _, pomPath := range pomPaths {
+		if err := m.updatePomFile(pomPath, groupId, artifactId, vulnDetails.SuggestedFixedVersion); err != nil {
+			errors = append(errors, fmt.Sprintf("%s: %v", pomPath, err))


Suggested change

errors = append(errors, fmt.Sprintf("%s: %v", pomPath, err))

failedFixErrorMsg := fmt.Errorf("failed to fix '%s' in descriptor '%s': %w", vulnDetails.ImpactedDependencyName, descriptorPath, fixErr)

err = errors.Join(err, failedFixErrorMsg)

also we can add log.Warn(failedFixErrorMsg) to match what we did in Golang and NPM

create an array called 'failingDescriptors' and accumulate the names of the descriptors that failed so we can indicate them all in the final error - see my error suggestion

eranturgeman · 2026-02-22T09:32:01Z

packageupdaters/mavenpackageupdater.go

-		IsMavenDepTreeInstalled: true,
+	var errors []string
+	for _, pomPath := range pomPaths {
+		if err := m.updatePomFile(pomPath, groupId, artifactId, vulnDetails.SuggestedFixedVersion); err != nil {


Suggested change

if err := m.updatePomFile(pomPath, groupId, artifactId, vulnDetails.SuggestedFixedVersion); err != nil {

if fixErr := m.updatePomFile(pomPath, groupId, artifactId, vulnDetails.SuggestedFixedVersion); fixErr != nil {

eranturgeman · 2026-02-22T09:32:44Z

packageupdaters/mavenpackageupdater.go

-	depTreeParams := &java.DepTreeParams{
-		Server:                  scanDetails.ServerDetails,
-		IsMavenDepTreeInstalled: true,
+	var errors []string


eranturgeman · 2026-02-22T09:33:35Z

packageupdaters/mavenpackageupdater.go

-	pomPaths []pomPath
-	// mavenDepTreeManager handles the installation and execution of the maven-dep-tree to obtain all the project poms and running mvn commands
-	*java.MavenDepTreeManager
+	if len(errors) > 0 {


Suggested change

if len(errors) > 0 {

if err != nil

eranturgeman · 2026-02-22T09:40:40Z

packageupdaters/mavenpackageupdater.go

-	// mavenDepTreeManager handles the installation and execution of the maven-dep-tree to obtain all the project poms and running mvn commands
-	*java.MavenDepTreeManager
+	if len(errors) > 0 {
+		return fmt.Errorf("failed to update pom.xml files:\n%s", strings.Join(errors, "\n"))


Suggested change

return fmt.Errorf("failed to update pom.xml files:\n%s", strings.Join(errors, "\n"))

return fmt.Errorf("encountered errors while fixing '%s' vulnerability in descriptors [%s]: %w", vulnDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), err)

eranturgeman · 2026-02-22T09:42:44Z

packageupdaters/mavenpackageupdater.go

+	content, err := os.ReadFile(pomPath)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to update Pom file: failed to read %s: %w", pomPath, err)


Suggested change

return fmt.Errorf("failed to update Pom file: failed to read %s: %w", pomPath, err)

return fmt.Errorf("failed to read %s: %w", pomPath, err)

we dont need to state here 'failed to update pom' this will be a duplication with the wrapping error

eranturgeman · 2026-02-22T09:43:11Z

packageupdaters/mavenpackageupdater.go

-		mph.pomDependencies = make(map[string]pomDependencyDetails)
+	var project mavenProject
+	if err = xml.Unmarshal(content, &project); err != nil {
+		return fmt.Errorf("failed to update Pom file: failed to parse %s: %w", pomPath, err)


Suggested change

return fmt.Errorf("failed to update Pom file: failed to parse %s: %w", pomPath, err)

return fmt.Errorf("failed to parse %s: %w", pomPath, err)

eranturgeman · 2026-02-22T09:49:48Z

packageupdaters/mavenpackageupdater.go

-			return err
+
+	var updated bool
+	newContent := content


what is this used for? this is not a copy of the content but rather a copy of the pointer, so we change the actual 'content' anyway. Also - there is no revert to this backup in case of a failure from what I see.

eranturgeman · 2026-02-22T09:56:12Z

packageupdaters/mavenpackageupdater.go

+	var updated bool
+	newContent := content
+
+	if updated, newContent = m.updateInParent(&project, groupId, artifactId, fixedVersion, newContent); updated {


if removing this 'newContent := content' line make sure to pass 'content' here in the parameters

eranturgeman · 2026-02-22T10:02:12Z

packageupdaters/mavenpackageupdater.go

+}

-	err = mph.getProjectPoms()
+func (m *MavenPackageUpdater) updatePomFile(pomPath, groupId, artifactId, fixedVersion string) error {


There is something wrong with the fixing process in general. for each vulnerability we might need to fix it in 3 different places (parent, manegement and regular).
In order to keep the pom.xml valid we have to make sure to update in ALL necessary locations or in none of them.
I suggest the following:
Attempt to make all 3 fixes on the content WITHOUT writing to the file after each time. if all fixes that are needed succeeded we write to the file (and we also save 2 writes to the file). if any of the needed failed - we return an error and we dont even have to restore a backup since we only write to the file at the end

eranturgeman · 2026-02-22T10:05:31Z

packageupdaters/mavenpackageupdater.go

+		pattern := regexp.MustCompile(`(?s)(<parent>\s*<groupId>` + regexp.QuoteMeta(groupId) + `</groupId>\s*<artifactId>` + regexp.QuoteMeta(artifactId) + `</artifactId>\s*<version>)[^<]+(</version>)`)
+		newContent := pattern.ReplaceAll(content, []byte("${1}"+fixedVersion+"${2}"))
+		if !bytes.Equal(content, newContent) {
+			log.Debug("Updated parent", toDependencyName(groupId, artifactId), "to", fixedVersion)


redundant log for my opinoin. lets log the final result if we managed to update the dep in all places not in every section. for big projects this will create a massive log

eranturgeman · 2026-02-22T10:05:39Z

packageupdaters/mavenpackageupdater.go

+			pattern := regexp.MustCompile(`(?s)(<groupId>` + regexp.QuoteMeta(groupId) + `</groupId>\s*<artifactId>` + regexp.QuoteMeta(artifactId) + `</artifactId>\s*<version>)[^<]+(</version>)`)
+			newContent := pattern.ReplaceAll(content, []byte("${1}"+fixedVersion+"${2}"))
+			if !bytes.Equal(content, newContent) {
+				log.Debug("Updated dependency", toDependencyName(groupId, artifactId), "to", fixedVersion)


redundant log for my opinoin. lets log the final result if we managed to update the dep in all places not in every section. for big projects this will create a massive log

eranturgeman · 2026-02-22T10:05:43Z

packageupdaters/mavenpackageupdater.go

+			pattern := regexp.MustCompile(`(<` + regexp.QuoteMeta(propertyName) + `>)[^<]+(</` + regexp.QuoteMeta(propertyName) + `>)`)
+			newContent := pattern.ReplaceAll(content, []byte("${1}"+newValue+"${2}"))
+			if !bytes.Equal(content, newContent) {
+				log.Debug("Updated property", propertyName, "to", newValue)


redundant log for my opinoin. lets log the final result if we managed to update the dep in all places not in every section. for big projects this will create a massive log

eranturgeman

see my comments

orto17 requested a deployment to frogbot February 18, 2026 10:52 — with GitHub Actions Waiting

orto17 added new feature Automatically generated release notes safe to test Approve running integration tests on a pull request labels Feb 18, 2026

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

orto17 requested a review from eranturgeman February 18, 2026 10:54

orto17 added 2 commits February 18, 2026 14:32

maven package updater

76a62f5

after cr

06765a6

orto17 force-pushed the maven-fix branch from 4a10b61 to 06765a6 Compare February 18, 2026 12:32

orto17 requested a deployment to frogbot February 18, 2026 12:32 — with GitHub Actions Waiting

PreferredLocation

a904eb0

orto17 requested a deployment to frogbot February 18, 2026 12:35 — with GitHub Actions Waiting

add supported maven tech

f92a3d3

orto17 requested a deployment to frogbot February 18, 2026 12:38 — with GitHub Actions Waiting

fix linter issues

bf8a570

orto17 requested a deployment to frogbot February 18, 2026 13:08 — with GitHub Actions Waiting

orto17 added the safe to test Approve running integration tests on a pull request label Feb 18, 2026

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

attiasas reviewed Feb 19, 2026

View reviewed changes

eranturgeman added improvement Automatically generated release notes and removed improvement Automatically generated release notes labels Feb 22, 2026

eranturgeman reviewed Feb 22, 2026

View reviewed changes

utils/outputwriter/markdowntable.go Show resolved Hide resolved

eranturgeman reviewed Feb 22, 2026

View reviewed changes

	testProjectPath := filepath.Join("..", "testdata", "packagehandlers")
	testProjectPath := filepath.Join("..", "testdata", "packageupdaters")

	errors = append(errors, fmt.Sprintf("%s: %v", pomPath, err))
	failedFixErrorMsg := fmt.Errorf("failed to fix '%s' in descriptor '%s': %w", vulnDetails.ImpactedDependencyName, descriptorPath, fixErr)
	err = errors.Join(err, failedFixErrorMsg)

	if err := m.updatePomFile(pomPath, groupId, artifactId, vulnDetails.SuggestedFixedVersion); err != nil {
	if fixErr := m.updatePomFile(pomPath, groupId, artifactId, vulnDetails.SuggestedFixedVersion); fixErr != nil {

	return fmt.Errorf("failed to update pom.xml files:\n%s", strings.Join(errors, "\n"))
	return fmt.Errorf("encountered errors while fixing '%s' vulnerability in descriptors [%s]: %w", vulnDetails.ImpactedDependencyName, strings.Join(failingDescriptors, ", "), err)

	return fmt.Errorf("failed to update Pom file: failed to read %s: %w", pomPath, err)
	return fmt.Errorf("failed to read %s: %w", pomPath, err)

	return fmt.Errorf("failed to update Pom file: failed to parse %s: %w", pomPath, err)
	return fmt.Errorf("failed to parse %s: %w", pomPath, err)

Comments

Conversation

orto17 commented Feb 18, 2026

Uh oh!

github-actions bot commented Feb 18, 2026

Uh oh!

attiasas left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eranturgeman Feb 22, 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!

eranturgeman Feb 22, 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!

eranturgeman Feb 22, 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!

eranturgeman Feb 22, 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!

eranturgeman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eranturgeman Feb 22, 2026 •

edited

Loading

eranturgeman Feb 22, 2026 •

edited

Loading

eranturgeman Feb 22, 2026 •

edited

Loading

eranturgeman Feb 22, 2026 •

edited

Loading