Skip to content

Commit dbbf877

Browse files
dnegreiradnegreira
authored
fix(maven): resolve property updates from parent POMs (#90)
* fix(maven): update properties in the POM that defines them What: - Resolve Maven property updates to the POM where the property is declared. - Check the current POM first, then the direct parent POM, including parent relativePath values that point to a directory. - Route dependency patches that use `${property}` through the same resolver before calling UpdatePom. - Stop creating missing properties when the target property cannot be found. - Add tests for current and parent property resolution, parent directory paths, dependency-backed property updates, shared properties, and missing-property errors. Why: - Maven modules often inherit dependency versions from parent POM properties. - Updating or creating the property in the child POM can shadow the parent and leave the real version source unchanged. - Resolving property ownership before patching keeps updates in the correct file and fails clearly when the property is not defined. Fixes: https://linear.app/chainguard/issue/AUTO-631/ Signed-off-by: David Negreira <david.negreira@chainguard.dev> * fix(maven): fail on conflicting version updates What: - Add a generic ErrVersionConflict for conflicting requested versions. - Detect when multiple dependency patches for the same direct dependency request different versions. - Detect when dependencies sharing the same Maven property request different versions. - Detect when an explicit property update conflicts with a dependency update controlled by that property. - Add tests for direct dependency, shared property, and explicit property conflict cases. Why: - Maven properties can control multiple dependencies, so only one requested version can actually be applied. - Previously those conflicts surfaced later as confusing validation warnings like “dependency not found”. - Failing early gives users a clear explanation before writing inconsistent or surprising POM updates. Fixes: https://linear.app/chainguard/issue/AUTO-640/ Signed-off-by: David Negreira <david.negreira@chainguard.dev> * fix(maven): reject POM updates outside project root What: - Validate Maven POM paths before updating and before writing files. - Resolve symlinks before checking whether a POM path stays under the configured root. - Add coverage for unsafe parent relative paths, absolute parent paths, direct manifest paths, and symlink escapes. Why: - Parent POM relative paths come from project XML and can point outside the repository. - Without a root-boundary check, Maven updates could write to attacker-controlled paths outside the project. - The tests lock in that only POMs inside the configured project root can be updated. Signed-off-by: David Negreira <david.negreira@chainguard.dev> * fix(maven): resolve properties through parent POM chain What: - Walk the full local parent POM chain when resolving where a Maven property is defined. - Track visited POM paths to stop parent relativePath cycles. - Collect inherited properties through the same parent chain during validation. - Add tests for grandparent-defined dependency properties and parent POM cycles. Why: - Maven can resolve properties through multi-level parent chains, but we previously stopped after one parent. - This caused valid leaf -> parent -> root configurations to fail with ErrPropertyNotFound. - Cycle detection prevents malformed parent chains from looping forever. Signed-off-by: David Negreira <david.negreira@chainguard.dev> * fix(maven): small fixes - Return error instead of just logging whent a property is not found - Change matchedPatches from map[Patch]bool to map[Patch]struct{}, Using struct{} makes that intent clearer and avoids storing a bool value that is always true. Signed-off-by: David Negreira <david.negreira@chainguard.dev> * fix(maven): add mavenLanguageName const Signed-off-by: David Negreira <david.negreira@chainguard.dev> --------- Signed-off-by: David Negreira <david.negreira@chainguard.dev>
1 parent 2bee3b7 commit dbbf877

4 files changed

Lines changed: 1486 additions & 32 deletions

File tree

‎pkg/languages/java/maven/analyzer.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func (ma *MavenAnalyzer) Analyze(ctx context.Context, projectPath string) (*anal
4848
}
4949

5050
result := &analyzer.AnalysisResult{
51-
Language: "maven",
51+
Language: mavenLanguageName,
5252
Dependencies: make(map[string]*analyzer.DependencyInfo),
5353
Properties: make(map[string]string),
5454
PropertyUsage: make(map[string]int),
@@ -375,7 +375,7 @@ func (ma *MavenAnalyzer) analyzeAllPoms(ctx context.Context, rootDir string) (*a
375375
}
376376

377377
result := &analyzer.AnalysisResult{
378-
Language: "maven",
378+
Language: mavenLanguageName,
379379
Dependencies: make(map[string]*analyzer.DependencyInfo),
380380
Properties: make(map[string]string),
381381
PropertyUsage: make(map[string]int),

‎pkg/languages/java/maven/maven.go‎

Lines changed: 94 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,8 @@ var (
3434
// ErrInvalidVersion is returned when a version string fails validation.
3535
ErrInvalidVersion = errors.New("invalid version string")
3636

37-
// ErrPomNotFound is returned when pom.xml is not found.
38-
ErrPomNotFound = errors.New("pom.xml not found")
37+
// ErrPomNotFound is returned when pom file is not found.
38+
ErrPomNotFound = errors.New("pom file not found")
3939

4040
// ErrPropertyValidationFailed is returned when property validation fails.
4141
ErrPropertyValidationFailed = errors.New("property validation failed")
@@ -53,8 +53,12 @@ var (
5353
ErrNoPOMsFound = errors.New("no Maven POM files found")
5454
)
5555

56-
// DefaultManifestFile is the conventional Maven POM filename.
57-
const DefaultManifestFile = "pom.xml"
56+
const (
57+
// DefaultManifestFile is the conventional Maven POM filename.
58+
DefaultManifestFile = "pom.xml"
59+
60+
mavenLanguageName = "maven"
61+
)
5862

5963
// Maven implements the BuildTool interface for Maven projects.
6064
type Maven struct{}
@@ -91,7 +95,7 @@ func IsMavenPom(path string) (bool, error) {
9195

9296
// Name returns the build tool identifier.
9397
func (m *Maven) Name() string {
94-
return "maven"
98+
return mavenLanguageName
9599
}
96100

97101
// Detect checks if a Maven project is present in the directory.
@@ -147,7 +151,7 @@ func depDisplayName(dep languages.Dependency) string {
147151
return "<unknown>"
148152
}
149153

150-
// pomFilePath returns the pom.xml path to use, honouring ManifestFile when set.
154+
// pomFilePath returns the pom.xml file path to use, honouring ManifestFile when set.
151155
func pomFilePath(cfg *languages.UpdateConfig) string {
152156
if cfg.ManifestFile != "" {
153157
return cfg.ManifestFile
@@ -184,7 +188,7 @@ func (m *Maven) Update(ctx context.Context, cfg *languages.UpdateConfig) error {
184188
}
185189
}
186190

187-
// Find pom.xml
191+
// Find pom file
188192
pomPath := pomFilePath(cfg)
189193
if _, err := os.Stat(pomPath); os.IsNotExist(err) {
190194
return fmt.Errorf("%w in: %s", ErrPomNotFound, pomPath)
@@ -207,23 +211,74 @@ func (m *Maven) Update(ctx context.Context, cfg *languages.UpdateConfig) error {
207211
}
208212
}
209213

210-
// Perform the update
211-
updatedPom, err := UpdatePom(ctx, pomPath, patches, cfg.Properties)
214+
// Dependency patches can target versions declared as ${property}. Resolve
215+
// those first so the property update is sent to the POM that defines it.
216+
patches, propertyUpdates, err := dependencyPropertyUpdates(ctx, pomPath, patches, cfg.Properties)
212217
if err != nil {
213-
return fmt.Errorf("failed to update pom.xml: %w", err)
218+
return fmt.Errorf("failed to resolve dependency property updates: %w", err)
219+
}
220+
221+
// Resolve each property to the POM file where it is actually defined.
222+
for propName, propValue := range cfg.Properties {
223+
propertyPomPath, err := resolvePropertyPomPath(ctx, pomPath, propName)
224+
if err != nil {
225+
return fmt.Errorf("failed to resolve file where property %s is set: %w", propName, err)
226+
}
227+
propertyUpdates = append(propertyUpdates, pomPropertyUpdate{
228+
pomFile: propertyPomPath,
229+
propertyName: propName,
230+
propertyValue: propValue,
231+
})
232+
}
233+
234+
// Group property updates so each POM file is patched once.
235+
propertiesByPom := make(map[string][]pomPropertyUpdate)
236+
for _, propertyUpdate := range propertyUpdates {
237+
propertiesByPom[propertyUpdate.pomFile] = append(propertiesByPom[propertyUpdate.pomFile], propertyUpdate)
238+
}
239+
if len(patches) > 0 && propertiesByPom[pomPath] == nil {
240+
propertiesByPom[pomPath] = nil
241+
}
242+
243+
updatedPoms := make(map[string][]byte)
244+
for updatePomPath, groupedPropertyUpdates := range propertiesByPom {
245+
var pomPatches []Patch
246+
if updatePomPath == pomPath {
247+
pomPatches = patches
248+
}
249+
if err := validatePathWithinRoot(cfg.RootDir, updatePomPath); err != nil {
250+
return fmt.Errorf("refusing to update pom file %s: %w", updatePomPath, err)
251+
}
252+
// Convert this POM's grouped property updates into patch entries.
253+
properties := make(map[string]string, len(groupedPropertyUpdates))
254+
for _, propertyUpdate := range groupedPropertyUpdates {
255+
properties[propertyUpdate.propertyName] = propertyUpdate.propertyValue
256+
}
257+
updatedPom, err := UpdatePom(ctx, updatePomPath, pomPatches, properties)
258+
if err != nil {
259+
return fmt.Errorf("failed to update pom file %s: %w", updatePomPath, err)
260+
}
261+
updatedPoms[updatePomPath] = updatedPom
214262
}
215263

216264
if cfg.DryRun {
217-
clog.InfoContextf(ctx, "Dry run mode: not writing changes to %s", pomPath)
265+
clog.InfoContextf(ctx, "Dry run mode: not writing Maven POM changes")
218266
return nil
219267
}
220268

221-
// Write updated POM back to file
222-
if err := os.WriteFile(pomPath, updatedPom, 0o600); err != nil {
223-
return fmt.Errorf("failed to write updated pom.xml: %w", err)
269+
for updatedPomPath, updatedPom := range updatedPoms {
270+
if err := validatePathWithinRoot(cfg.RootDir, updatedPomPath); err != nil {
271+
return fmt.Errorf("refusing to write updated pom file %s: %w", updatedPomPath, err)
272+
}
273+
if err := os.WriteFile(updatedPomPath, updatedPom, 0o600); err != nil {
274+
return fmt.Errorf("failed to write updated pom file %s: %w", updatedPomPath, err)
275+
}
276+
clog.InfoContextf(ctx, "Successfully updated %s", updatedPomPath)
224277
}
225278

226-
clog.InfoContextf(ctx, "Successfully updated %s", pomPath)
279+
if len(updatedPoms) == 0 {
280+
clog.InfoContextf(ctx, "No Maven POM changes needed")
281+
}
227282

228283
return nil
229284
}
@@ -237,8 +292,9 @@ func (m *Maven) Validate(ctx context.Context, cfg *languages.UpdateConfig) error
237292
// Parse the updated POM
238293
project, err := ParsePom(pomPath)
239294
if err != nil {
240-
return fmt.Errorf("failed to parse updated pom.xml: %w", err)
295+
return fmt.Errorf("failed to parse updated pom file %s: %w", pomPath, err)
241296
}
297+
properties := pomProperties(ctx, pomPath, project)
242298

243299
// Validate dependencies
244300
for _, dep := range cfg.Dependencies {
@@ -257,7 +313,7 @@ func (m *Maven) Validate(ctx context.Context, cfg *languages.UpdateConfig) error
257313
if project.Dependencies != nil {
258314
for _, pomDep := range *project.Dependencies {
259315
key := fmt.Sprintf("%s:%s", pomDep.GroupID, pomDep.ArtifactID)
260-
if key == searchKey && resolveVersion(pomDep.Version, project.Properties) == dep.Version {
316+
if key == searchKey && resolveVersion(pomDep.Version, properties) == dep.Version {
261317
found = true
262318
break
263319
}
@@ -268,7 +324,7 @@ func (m *Maven) Validate(ctx context.Context, cfg *languages.UpdateConfig) error
268324
if !found && project.DependencyManagement != nil && project.DependencyManagement.Dependencies != nil {
269325
for _, pomDep := range *project.DependencyManagement.Dependencies {
270326
key := fmt.Sprintf("%s:%s", pomDep.GroupID, pomDep.ArtifactID)
271-
if key == searchKey && resolveVersion(pomDep.Version, project.Properties) == dep.Version {
327+
if key == searchKey && resolveVersion(pomDep.Version, properties) == dep.Version {
272328
found = true
273329
break
274330
}
@@ -281,16 +337,16 @@ func (m *Maven) Validate(ctx context.Context, cfg *languages.UpdateConfig) error
281337
}
282338

283339
// Validate properties
284-
if project.Properties != nil {
285-
for propName, expectedValue := range cfg.Properties {
286-
if actualValue, exists := project.Properties.Entries[propName]; exists {
340+
for propName, expectedValue := range cfg.Properties {
341+
if properties != nil {
342+
if actualValue, exists := properties.Entries[propName]; exists {
287343
if actualValue != expectedValue {
288344
return fmt.Errorf("%w: property %s has value %s, expected %s", ErrPropertyValidationFailed, propName, actualValue, expectedValue)
289345
}
290-
} else {
291-
log.Warnf("Property not found: %s", propName)
346+
continue
292347
}
293348
}
349+
return fmt.Errorf("%w: property %s not found", ErrPropertyNotFound, propName)
294350
}
295351

296352
log.Infof("Validation completed successfully")
@@ -326,7 +382,11 @@ func applyPrecedenceRules(ctx context.Context, pomPath string, deps []languages.
326382
// Check if this dependency uses a property
327383
if depInfo, exists := analysis.Dependencies[depKey]; exists && depInfo.UsesProperty {
328384
// Check if the property is being updated
329-
if _, propertyBeingUpdated := properties[depInfo.PropertyName]; propertyBeingUpdated {
385+
if propertyValue, propertyBeingUpdated := properties[depInfo.PropertyName]; propertyBeingUpdated {
386+
if dep.Version != "" && propertyValue != dep.Version {
387+
return nil, fmt.Errorf("%w: dependency %s requests %s but property %s is explicitly set to %s",
388+
ErrVersionConflict, depKey, dep.Version, depInfo.PropertyName, propertyValue)
389+
}
330390
log.Infof("Skipping direct patch for %s (property %s takes precedence)", depKey, depInfo.PropertyName)
331391
continue // Skip this dependency, property wins
332392
}
@@ -345,6 +405,7 @@ func applyPrecedenceRules(ctx context.Context, pomPath string, deps []languages.
345405
// convertDependenciesToPatches converts unified dependencies to Maven-specific patches.
346406
func convertDependenciesToPatches(deps []languages.Dependency) ([]Patch, error) {
347407
patches := make([]Patch, 0, len(deps))
408+
requestedVersions := make(map[string]string)
348409

349410
for _, dep := range deps {
350411
patch := Patch{
@@ -382,6 +443,15 @@ func convertDependenciesToPatches(deps []languages.Dependency) ([]Patch, error)
382443
ErrMissingRequiredFields, patch.GroupID, patch.ArtifactID, patch.Version, dep.Name)
383444
}
384445

446+
if patch.Version != "" {
447+
depKey := fmt.Sprintf("%s:%s", patch.GroupID, patch.ArtifactID)
448+
if requestedVersion, exists := requestedVersions[depKey]; exists && requestedVersion != patch.Version {
449+
return nil, fmt.Errorf("%w: dependency %s requests both %s and %s",
450+
ErrVersionConflict, depKey, requestedVersion, patch.Version)
451+
}
452+
requestedVersions[depKey] = patch.Version
453+
}
454+
385455
patches = append(patches, patch)
386456
}
387457

0 commit comments

Comments
 (0)