Refactor maven xml updater #1011

eyalk007 · 2025-12-28T09:01:38Z

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

- Update MavenPackageUpdater to handle multiple Components - Loop through all Components and update each pom.xml file - Extracts file paths from Component.Location.File - Handles same vulnerability in multiple modules (e.g., backend + frontend) - All changes go into ONE PR (same vuln, same fix) - Update all tests to populate Components array with Location - Backward compatible: falls back to 'pom.xml' if no Components

- Remove backward compatibility fallback that masked bugs - If Components array is empty or missing Location data, fail explicitly - Better error message explains the issue is with scan results - Forces engine to always populate Components with Location - Prevents silent failures in multi-module projects

- Add constants for magic strings (separator, property prefix/suffix) - Extract parseMavenCoordinate() and toMavenCoordinate() helpers - Extract extractPropertyName() for property placeholder parsing - Remove all comments (code is self-documenting) - Simplify debug logs (no fmt.Sprintf) - DRY: no repetition anywhere - All tests still pass

- Better naming: matches ImpactedDependencyName field - parseDependencyName() / toDependencyName() - More accurate: we parse dependency names (groupId:artifactId), not coordinates (which include version)

- Replace encoding/xml with github.com/beevik/etree - Use DOM-based XML manipulation instead of unmarshal/marshal - Should preserve formatting, namespaces, and all XML attributes - All tests still pass - Need to test on real repo to verify formatting preservation

- Parse XML to understand structure (encoding/xml) - Use regex to replace ONLY the version text - Preserves ALL formatting, namespaces, blank lines, comments - Minimal diffs - only version numbers change - All tests pass - Ready for real-world testing

- Document all 5 test cases (simple, property, parent, depMgmt, multi-module) - Document engine limitations (parent POM resolution, non-standard pom names) - Document text-based replacement approach - Document multi-module support - Recommendations for engine team - Testing checklist

scanrepository/scanrepository.go

eyalk007 · 2025-12-28T09:04:42Z

packagehandlers/mavenpackageupdater.go

+func (mpu *MavenPackageUpdater) getPomPaths(vulnDetails *utils.VulnerabilityDetails) []string {
+	var pomPaths []string
+	for _, component := range vulnDetails.Components {
+		if component.Location != nil && component.Location.File != "" {
+			pomPaths = append(pomPaths, component.Location.File)
+		}
+	}
+	return pomPaths
+}


this gets all occurrences of a vulnerability (the exact package and version)
can be moved to a utils because multiple package updaters will use this logic

I will move it an insert it to a dictionary first so we filter any duplicates if any exist.
please use GetVulnerabilityLocations once npm new updater is merged

eyalk007 · 2025-12-28T09:06:33Z

packagehandlers/mavenpackageupdater.go

+)
+
+const (
+	mavenCoordinateSeparator = ":"


can rename to dependency instead of coordinate

eyalk007 · 2025-12-28T09:07:35Z

packagehandlers/mavenpackageupdater.go

+	updated := false
+	newContent := content
+
+	if updated, newContent = mpu.updateInParent(&project, groupId, artifactId, fixedVersion, newContent); updated {


currently not working because of the engine

packagehandlers/mavenpackageupdater.go

- When a config profile is fetched from XSC, explicitly set CreateAutoFixPr = false - This ensures autofix is disabled for repositories using config profiles - Autofix behavior can still be controlled via environment variables when not using config profiles

- Restore the check that respects CreateAutoFixPr configuration - When CreateAutoFixPr is false, the scan runs in detection mode only - No fix PRs are created when this flag is disabled

- This change should only be in v3_er branch, not in refactor-maven-xml-updater - The autofix behavior is controlled elsewhere

eyalk007 · 2026-01-06T09:39:09Z

packagehandlers/mavenpackageupdater.go

+	if updated, newContent = mpu.updateInParent(&project, groupId, artifactId, fixedVersion, newContent); updated {
+		if err := os.WriteFile(pomPath, newContent, 0644); err != nil {
+			return fmt.Errorf("failed to write %s: %w", pomPath, err)
+		}
+		log.Debug("Successfully updated", pomPath)
+		return nil
+	}
+
+	if updated, newContent = mpu.updateInDependencies(&project, project.Dependencies, groupId, artifactId, fixedVersion, newContent); updated {
+		if err := os.WriteFile(pomPath, newContent, 0644); err != nil {
+			return fmt.Errorf("failed to write %s: %w", pomPath, err)
+		}
+		log.Debug("Successfully updated", pomPath)
+		return nil
+	}
+
+	if project.DependencyManagement != nil {
+		if updated, newContent = mpu.updateInDependencies(&project, project.DependencyManagement.Dependencies, groupId, artifactId, fixedVersion, newContent); updated {
+			if err := os.WriteFile(pomPath, newContent, 0644); err != nil {
+				return fmt.Errorf("failed to write %s: %w", pomPath, err)
+			}
+			log.Debug("Successfully updated", pomPath)
+			return nil
+		}
+	}
+


maybe need to rethink the pecking order

in maven if you were bold enough to have the same dep, in multiple contexts
in practice the closest one in hierarchy wins version wise

but here im not checking according to hierarchy and fixing only one instance (i am searching for same package and same version though)

so maybe do it -
update in deps -> update dep management -> update parent

and not exit early
in practice updating all is not needed for the vuln fix, but its the safest approach

eranturgeman · 2026-01-06T09:42:46Z

packagehandlers/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>)`)


please see if you can use GetVulnerabilityRegexCompiler that is found in commonpackagehandler.go
I fixed it to fix all escaping chars, and we can reuse this code.
Also please define the patterns as consts

eranturgeman · 2026-01-06T09:42:57Z

packagehandlers/mavenpackageupdater.go

+				return mpu.updateProperty(project, propertyName, fixedVersion, content)
+			}
+
+			pattern := regexp.MustCompile(`(?s)(<groupId>` + regexp.QuoteMeta(groupId) + `</groupId>\s*<artifactId>` + regexp.QuoteMeta(artifactId) + `</artifactId>\s*<version>)[^<]+(</version>)`)


please see if you can use GetVulnerabilityRegexCompiler that is found in commonpackagehandler.go
I fixed it to fix all escaping chars, and we can reuse this code.
Also please define the patterns as consts

eranturgeman · 2026-01-06T09:43:03Z

packagehandlers/mavenpackageupdater.go

+
+	for _, prop := range project.Properties.Props {
+		if prop.XMLName.Local == propertyName {
+			pattern := regexp.MustCompile(`(<` + regexp.QuoteMeta(propertyName) + `>)[^<]+(</` + regexp.QuoteMeta(propertyName) + `>)`)


please see if you can use GetVulnerabilityRegexCompiler that is found in commonpackagehandler.go
I fixed it to fix all escaping chars, and we can reuse this code.
Also please define the patterns as consts

eranturgeman · 2026-01-07T09:30:57Z

packagehandlers/packagehandlers_test.go

-	<scope> compile		</scope>
-</dependency>`,
-	}
+func TestMavenUpdateRegularDependency(t *testing.T) {


please move all teste related to maven that are not for functions in commonpackagehandler_test.go to a new file: mavenpackagehandler_test.go

orto17 · 2026-01-11T11:38:26Z

utils/utils.go

 		"Note: Frogbot currently cannot address certain vulnerabilities in some package managers, which may result in the absence of changes", err.PackageName)
 }

 // VulnerabilityDetails serves as a container for essential information regarding a vulnerability that is going to be addressed and resolved


this comment also can be removed

packagehandlers/mavenpackageupdater.go

orto17 · 2026-01-12T06:52:36Z

packagehandlers/mavenpackageupdater.go

+	Dependencies []mavenDep `xml:"dependencies>dependency"`
+}
+
+func (mpu *MavenPackageUpdater) UpdateDependency(vulnDetails *utils.VulnerabilityDetails) error {


to my opinion mpu is not clear, and it should be changed to "m" or "maven" , but just a suggestion

was just following hte foramt you set
can keep it you prefer mavenPackageUpdater
or mavenpu?

orto17 · 2026-01-12T06:57:56Z

packagehandlers/mavenpackageupdater.go

+	return nil
+}
+
+func (mpu *MavenPackageUpdater) getPomPaths(vulnDetails *utils.VulnerabilityDetails) []string {


even tough we are trying to avoid comments i think for this function we need to add an explanation of what it returns or change the signature to something like:

getPomPaths(vulnDetails *utils.VulnerabilityDetails) (occurrences []string) getVulnerabilityOccurences(vulnDetails *utils.VulnerabilityDetails) []string

because it was unclear to me until i saw your comments

i mean we can just change hte name to get descriptor paths

get vulnerability occurrences is also an option

dont think a comment is needed just need to find the right function name

orto17 · 2026-01-12T07:01:34Z

packagehandlers/mavenpackageupdater.go

+	}
+
+	if len(errors) > 0 {
+		return fmt.Errorf("failed to update pom.xml files:\n%s", strings.Join(errors, "\n"))


regarding errors & fix flow in general, i think this entire flow should not raise an error. meaning if the fix is failed frogbot will add a warn log and finishes execution without an error. i need to take it with Barb, but WDYT?

regardless of the decision i think we need to add Warn logs in case of errors during the fix, it will be easier to read the logs this way in case we are looping and have several fixes at once.

orto17 · 2026-01-12T07:05:10Z

packagehandlers/mavenpackageupdater.go

+func (mpu *MavenPackageUpdater) updatePomFile(pomPath, groupId, artifactId, fixedVersion string) error {
+	content, err := os.ReadFile(pomPath)
+	if err != nil {
+		return fmt.Errorf("failed to read %s: %w", pomPath, err)


logs will be easier to understand if we'll add the step we are currently at: fmt.Errorf("failed to update Pom file: failed to read %s: %w", pomPath, err)

orto17 · 2026-01-12T07:06:37Z

packagehandlers/mavenpackageupdater.go

+	}
+
+	var project mavenProject
+	if err := xml.Unmarshal(content, &project); err != nil {


you can make it if err = xml.Unmarshal(content, &project); since err var exist (and same goes to the other err vars in this function)

orto17 · 2026-01-12T07:08:46Z

packagehandlers/mavenpackageupdater.go

+		if err := os.WriteFile(pomPath, newContent, 0644); err != nil {
+			return fmt.Errorf("failed to write %s: %w", pomPath, err)
+		}
+		log.Debug("Successfully updated", pomPath)


log.Debug("Successfully updated Pom Parent", pomPath) log.Debug("Successfully updated Dependency Management", pomPath)

orto17 · 2026-01-12T07:13:31Z

packagehandlers/mavenpackageupdater.go

+	return parts[0], parts[1], nil
+}
+
+func toDependencyName(groupId, artifactId string) string {


maybe this func can also move to utils file and get coordinateSeparator as an argument?

why? its specific to maven (maybe gradle will use it but this format is maven only for now

orto17 · 2026-01-12T07:15:19Z

packagehandlers/mavenpackageupdater.go

+			return fmt.Errorf("failed to write %s: %w", pomPath, err)
+		}
+		log.Debug("Successfully updated", pomPath)
+		return nil


there is no option for "two" changes needed? for example both in Dependencies and in DependencyManagement?

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

packagehandlers/mavenpackageupdater.go

+				return mpu.updateProperty(project, propertyName, fixedVersion, content)
+			}
+
+			pattern := regexp.MustCompile(`(?s)(<groupId>` + regexp.QuoteMeta(groupId) + `</groupId>\s*<artifactId>` + regexp.QuoteMeta(artifactId) + `</artifactId>\s*<version>)[^<]+(</version>)`)


what do you think about having this regexes as consts? they are repeating for parent fix and for dependencies fix , plus it will make it more readable

`</artifactId>\s*<version>)[^<]+(</version>)` `</groupId>\s*<artifactId>`

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

packagehandlers/mavenpackageupdater.go

+
+	for _, prop := range project.Properties.Props {
+		if prop.XMLName.Local == propertyName {
+			pattern := regexp.MustCompile(`(<` + regexp.QuoteMeta(propertyName) + `>)[^<]+(</` + regexp.QuoteMeta(propertyName) + `>)`)


i think this one must be a const even tough it appears only once: >)[^<]+(</

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

packagehandlers/mavenpackageupdater.go

+
+	if project.DependencyManagement != nil {
+		if updated, newContent = mpu.updateInDependencies(&project, project.DependencyManagement.Dependencies, groupId, artifactId, fixedVersion, newContent); updated {
+			if err := os.WriteFile(pomPath, newContent, 0644); err != nil {


no sure it will make sense but maybe those line can be extracted to utils since they are duplicated and implement a file update utility:

if err := os.WriteFile(pomPath, newContent, 0644); err != nil { return fmt.Errorf("failed to write %s: %w", pomPath, err) } log.Debug("Successfully updated", pomPath)

im against making thing generic until we see them in mutlipe uses
the next pcakge manager that does it need to move it to utils

eyalk007 added 7 commits December 27, 2025 20:35

Maven: Rename parseMavenCoordinate → parseDependencyName

77f4763

- Better naming: matches ImpactedDependencyName field - parseDependencyName() / toDependencyName() - More accurate: we parse dependency names (groupId:artifactId), not coordinates (which include version)

eyalk007 self-assigned this Dec 28, 2025

eyalk007 added the improvement Automatically generated release notes label Dec 28, 2025

eyalk007 requested a deployment to frogbot December 28, 2025 09:01 — with GitHub Actions Waiting

eyalk007 requested a deployment to frogbot December 28, 2025 09:02 — with GitHub Actions Waiting

eyalk007 commented Dec 28, 2025

View reviewed changes

scanrepository/scanrepository.go Outdated Show resolved Hide resolved

eyalk007 commented Dec 28, 2025

View reviewed changes

packagehandlers/mavenpackageupdater.go Outdated Show resolved Hide resolved

eyalk007 requested a deployment to frogbot December 28, 2025 09:09 — with GitHub Actions Waiting

eyalk007 force-pushed the refactor-maven-xml-updater branch from 6c8a76c to c2727a7 Compare December 28, 2025 09:34

eyalk007 requested a deployment to frogbot December 28, 2025 09:34 — with GitHub Actions Waiting

Remove orto_demo_repo submodule

d10294c

eyalk007 force-pushed the refactor-maven-xml-updater branch from c2727a7 to d10294c Compare December 28, 2025 20:05

eyalk007 requested a deployment to frogbot December 28, 2025 20:05 — with GitHub Actions Waiting

Remove unrelated docs files

f64a000

eyalk007 requested a deployment to frogbot December 28, 2025 20:07 — with GitHub Actions Waiting

eyalk007 requested a deployment to frogbot December 30, 2025 17:11 — with GitHub Actions Waiting

eyalk007 force-pushed the refactor-maven-xml-updater branch from b39f15b to 2ba32ea Compare December 31, 2025 13:28

eyalk007 requested a deployment to frogbot December 31, 2025 13:28 — with GitHub Actions Waiting

delete the md

addaf88

eyalk007 requested a deployment to frogbot December 31, 2025 13:34 — with GitHub Actions Waiting

eyalk007 requested a deployment to frogbot December 31, 2025 13:38 — with GitHub Actions Waiting

eyalk007 force-pushed the refactor-maven-xml-updater branch from fb4c056 to 0cb9521 Compare December 31, 2025 18:07

eyalk007 requested a deployment to frogbot December 31, 2025 18:07 — with GitHub Actions Waiting

Uncomment CreateAutoFixPr check in scan repository

cd2e6df

- Restore the check that respects CreateAutoFixPr configuration - When CreateAutoFixPr is false, the scan runs in detection mode only - No fix PRs are created when this flag is disabled

eyalk007 force-pushed the refactor-maven-xml-updater branch from 0cb9521 to cd2e6df Compare December 31, 2025 18:14

eyalk007 requested a deployment to frogbot December 31, 2025 18:14 — with GitHub Actions Waiting

Revert: Remove CreateAutoFixPr = false from config profile fetch

4aa60e4

- This change should only be in v3_er branch, not in refactor-maven-xml-updater - The autofix behavior is controlled elsewhere

eyalk007 had a problem deploying to frogbot December 31, 2025 18:17 — with GitHub Actions Failure

eyalk007 commented Jan 6, 2026

View reviewed changes

eranturgeman reviewed Jan 6, 2026

View reviewed changes

eranturgeman reviewed Jan 7, 2026

View reviewed changes

orto17 reviewed Jan 11, 2026

View reviewed changes

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/mavenpackageupdater.go Show resolved Hide resolved

orto17 reviewed Jan 12, 2026

View reviewed changes

cr fixes

ec6f3c0

eyalk007 requested a deployment to frogbot January 12, 2026 08:13 — with GitHub Actions Waiting

Refactor maven xml updater #1011

Are you sure you want to change the base?

Refactor maven xml updater #1011

Uh oh!

Conversation

eyalk007 commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

eyalk007 Dec 28, 2025 • 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 Jan 6, 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!

eyalk007 Dec 28, 2025 • 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!

Uh oh!

eyalk007 Jan 6, 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!

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!

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!

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!

Reviewers

eyalk007 commented Dec 28, 2025 •

edited

Loading

eyalk007 Dec 28, 2025 •

edited

Loading

eranturgeman Jan 6, 2026 •

edited

Loading

eyalk007 Dec 28, 2025 •

edited

Loading

eyalk007 Jan 6, 2026 •

edited

Loading

orto17 Jan 12, 2026 •

edited

Loading

orto17 Jan 12, 2026 •

edited

Loading

orto17 Jan 12, 2026 •

edited

Loading