Restrict what a repository-resolved model contributes to the build - #12953
Restrict what a repository-resolved model contributes to the build#12953slachiewicz wants to merge 3 commits into
Conversation
b022a89 to
24ca4c8
Compare
Models built at VALIDATION_LEVEL_MINIMAL are the ones Maven builds while resolving dependency, parent and BOM-import POMs from a repository, not the operator's own project (see loadPom() in DefaultArtifactDescriptorReader and the BOM-import path in DefaultModelBuilder). Such models now interpolate their user/system properties only against a small, environment-independent allowlist (java.*, os.*, maven.*, the separator properties, and the CI-friendly revision/changelist/sha1); everything else, including env.* and arbitrary -D properties, is left as a literal unresolved expression so it cannot be substituted into things like repository URLs. Operator project builds use a higher validation level and are unaffected. The prior behavior can be restored globally with the new maven.model.dependencyInterpolation.full system/user property.
Models built for dependency resolution (a dependency POM, one of its parents, or an imported BOM) are read at ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL rather than the default STRICT level used for the project being built. For such models, file and property activation conditions are no longer evaluated, and profiles that do still activate (JDK version, operating system, activeByDefault) no longer contribute repositories or plugin repositories. Profiles supplied via settings or the command line are unaffected and keep activating as before.
24ca4c8 to
28b160f
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Well-structured security hardening for repository-resolved models. The validation-level heuristic is a correct adaptation for the 3.10.x codebase (which lacks the RequestType enum from master/4.0.x), and the test coverage is thorough.
One issue to fix before merge:
Garbled comment in DefaultModelBuilder.java (lines 291–293)
The multi-line comment above boolean externalModel appears to merge two overlapping drafts. Line 1 is truncated mid-phrase ("...or an" with no continuation), and line 2 starts a new, unrelated sentence. Compare to the clean version in the master PR (#12948):
// Models built for dependency resolution (a dependency POM, one of its parents, or an
// imported BOM) are read at ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL rather than
// the default STRICT level used for the project being built.The current 3.10.x text reads:
// Models built to resolve a dependency (a dependency POM, one of its parents, or an
// Models resolved for dependency, parent or BOM POMs evaluate only platform-derived
// activation (JDK, OS, activeByDefault).Suggested fix: adapt the clean 3-line comment from the master variant.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Document the behavior and default of FULL_EXTERNAL_INTERPOLATION_PROPERTY more clearly, and note its promotion to Constants with @config in Maven 4.x for auto-generated configuration documentation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Delta Review — New Commit Looks Good
New commit 632787d3 cleanly improves the Javadoc for FULL_EXTERNAL_INTERPOLATION_PROPERTY — the new 11-line version correctly describes the default behavior, opt-in semantics, and notes the Maven 4.x promotion to Constants with @Config. No issues introduced.
Previous finding still applies: The garbled multi-line comment in DefaultModelBuilder.java (lines 291–292) was not addressed in this commit.
🤖 This review was generated by ForgeBot.
2 participants
For models resolved while building a dependency POM, profiles activated by
fileorpropertyare skipped, and profiles that do activate contribute no repositories. Settings and CLI profiles are untouched.Draft while related code paths are reviewed.
A property-interpolation change was removed from this PR after it broke existing integration tests:
MavenITmng4590ImportedPomUsesSystemAndUserPropertiesTestverifies that imported POMs are processed using the same system and user properties as the importing POM, which is deliberate behaviour.