Restrict what a repository-resolved model contributes to the build - #12943
Restrict what a repository-resolved model contributes to the build#12943slachiewicz wants to merge 14 commits into
Conversation
effd394 to
076da19
Compare
Repositories contributed by a model resolved from a repository are merged recessively; repositories supplied by the request or session keep precedence. Gates the replace branch of ModelBuilderSessionState.mergeRepositories() on isBuildRequest(), so only filesystem/project models (BUILD_PROJECT, BUILD_EFFECTIVE, BUILD_CONSUMER) retain replace semantics.
Models built while resolving a dependency POM or one of its parents from a repository (ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY and CONSUMER_PARENT) 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. Models belonging to the project being built use the BUILD_* request types and are unaffected. The prior behavior can be restored globally with the new maven.model.dependencyInterpolation.full system/user property.
Extends the previous restriction to the deprecated compat model-builder interpolator (AbstractStringBasedModelInterpolator), which independently builds its own value-source list. Models built at ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL are the models Maven builds while resolving dependency, parent and BOM-import POMs from a repository; such models now interpolate their user/system properties only against the same allowlist as the modern builder. Operator project builds use a higher validation level and are unaffected. Same maven.model.dependencyInterpolation.full opt-out applies here too.
…pendency POMs Models built to resolve a dependency (ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY) are read from a repository, not authored by the project being built. For such models, file, property, and condition 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, and models belonging to the project's own parent chain, are unaffected.
…s (compat) Extends the previous restriction to the deprecated compat model-builder (DefaultModelBuilder), which independently performs its own profile selection and injection. Models built at ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL are the models Maven builds while resolving dependency, parent and BOM-import POMs from a repository. 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.
readAsParentModel() caches the parent model it builds keyed only by source and activation record. A dependency's own remote parent and a project's own remote parent can be the exact same POM source, and the two must not share that cache entry: only a request originating from CONSUMER_DEPENDENCY skips file, property, and condition activation and drops repositories, so a cache hit computed under one lineage must never be handed to the other. request.getRequestType() cannot carry that distinction by itself, because a parent lookup always derives a CONSUMER_PARENT request regardless of what kind of session triggered it. A new sticky externalOrigin flag is threaded through derive() instead -- seeded on CONSUMER_DEPENDENCY and OR'd forward through every derivation, surviving as many parent hops as the lineage has -- and readAsParentModel() and the shared getActiveProfiles() gate both key off it. The cache is partitioned into two tags accordingly, so the two lineages never share an entry for the same source. Internal to ModelBuilderSessionState; no public API change.
…tory POM A dependencyManagement import (scope=import or type=bom) reads its managed dependencies from whichever POM the coordinates resolve to. system scope and systemPath bind a dependency to a file on the local filesystem, which is only meaningful for projects belonging to the current build. Track, in doLoadDependencyManagement, whether the import POM was resolved from the reactor or from a repository. Managed dependencies declaring system scope or a systemPath in a repository-resolved import are dropped with a warning, and are removed from the cached model so a later import cannot reintroduce them. The previous behaviour remains available, also with a warning, behind the new maven.repository.dependencyManagement.allowSystemScope user property. Imports resolved from the local reactor are unaffected.
70e4286 to
f8e242e
Compare
MavenITmng4379 and MavenITmng3586 cover a systemPath supplied through a property or an environment variable in a POM resolved from a repository; MavenITmng4590 covers dependency management imported from one. Those models now use restricted defaults, so each test requests the previous behaviour explicitly, which exercises the opt-out property, and gains a companion test asserting what the default does instead.
f8e242e to
828258e
Compare
The static fixture pom used a POSIX-only systemPath, which File.isAbsolute() rejects on Windows; model validation then dropped the imported entry before the code under test ran, so the test's assertNull passed for the wrong reason. The test now writes the BOM into a per-test temporary remote repository with a systemPath computed from the current OS, and asserts that an ordinary managed dependency from the same import is present, so the system-scope assertions can no longer pass vacuously. The now-unused static fixture pom is removed.
DefaultProjectBuilder decided between BUILD_EFFECTIVE and CONSUMER_DEPENDENCY partly on the request's validation level. Validation strictness says how closely a model is checked, not whose model it is, and the 2.x project builder API asks for MAVEN_2_0 validation on a file the caller supplies, so such a build was labelled a consumer dependency. A non-null pomFile already marks a model backed by a file the caller pointed at -- a model resolved from a repository arrives as a resolved source with no pomFile -- so that alone now decides it. Covered by MavenITmng4765LocalPomProjectBuilderTest, which builds a local file through that API and reads back a command-line property.
This reverts commit 805d065. RequestType is read for validation strictness as well as for provenance, so promoting every file-backed build to BUILD_EFFECTIVE turned lenient builds strict: DefaultMavenProjectBuilderTest.testPartialResultUponBadDependencyDeclaration, MavenProjectTest.testCloneWithDependencyManagement and PomConstructionTest.testDuplicateDependenciesCauseLastDeclarationToBePickedInLenientMode all depend on the previous behaviour.
The restriction on property interpolation and profile activation for a consumer-dependency or consumer-parent model now also requires the model source to be one Maven resolved from a repository (ModelSource.getModelId() non-null). A POM the caller hands to Maven as a file arrives with the same request type but is not repository-resolved, so it keeps full interpolation and activation. Integration-level cover: MavenITmng4765LocalPomProjectBuilderTest.
gnodet
left a comment
There was a problem hiding this comment.
✅ Well-Designed Security Hardening
Well-structured behavioral change with thorough test coverage and clear opt-out mechanisms. The PR restricts what repository-resolved models contribute to the build, addressing multiple real-world attack vectors (dependency confusion via repository injection, property leaking, profile-based side effects).
The test coverage is excellent: unit tests for both compat and new model builder layers, plus integration tests for MNG-4379, MNG-4590, MNG-3586, and IT0085, each covering both the new default AND the opt-out path.
Two observations (non-blocking):
-
Configuration documentation gap (
DefaultModelInterpolator.java): Themaven.model.dependencyInterpolation.fullopt-out property is declared as a local constant in two implementation classes (DefaultModelInterpolatorandAbstractStringBasedModelInterpolator), unlike its companionmaven.repository.dependencyManagement.allowSystemScopewhich is inConstants.javawith@Config. This means Maven's auto-generated configuration docs will list one opt-out property but not the other — both change long-standing behavior and are equally user-facing. Consider promoting toConstants.javawith@Configand sharing a single constant definition. -
Future-proofing note (
DefaultModelBuilder.java:640): Thereplace && isBuildRequest()guard inmergeRepositoriesis correct for today'sRequestTypevalues, but implicitly assigns recessive merging to any future non-build request type. A brief code comment noting this design choice would help future maintainers.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Milestone | (none) | 4.0.0 |
🔀 Backport Status
- ⏳
master— #12948 (OPEN) - ⏳
maven-3.10.x— #12953 (OPEN) - ℹ️
maven-3.9.x— not targeted (appropriate for behavior-changing enhancement)
🤖 This review was generated by ForgeBot.
Move the maven.model.dependencyInterpolation.full property from local constants in DefaultModelInterpolator and AbstractStringBasedModelInterpolator to Constants.java with @config so it appears in Maven's auto-generated configuration documentation alongside its companion maven.repository.dependencyManagement.allowSystemScope. The compat module keeps a private mirror of the value because it does not depend on maven-api-core. Also add a design-intent comment on the replace && isBuildRequest() guard in DefaultModelBuilder.mergeRepositories to clarify that non-build request types intentionally use recessive merging. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update DefaultModelInterpolatorTest to use Constants.MAVEN_MODEL_DEPENDENCY_INTERPOLATION_FULL and AbstractModelInterpolatorTest to use the string literal since the compat constant is now private. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
There was a problem hiding this comment.
Delta review — two new commits (same as master sibling #12948) address both previous observations:
-
✅ Configuration documentation gap → Fixed.
maven.model.dependencyInterpolation.fullpromoted toConstants.javawith@Config(type = "java.lang.Boolean", defaultValue = "false"),@since 4.1.0, and comprehensive Javadoc. Compat duplicate madeprivatewith cross-reference comment. -
✅ Future-proofing note → Fixed. Design-intent comment added to
mergeRepositoriesexplaining that non-build RequestTypes intentionally fall through to recessive merging.
No new issues in the delta. Consistent with the already-approved master PR.
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
A model resolved from a repository should contribute less to the build than the project's own POM.
BUILD_*requests — the project being built — are unaffected.file,propertyorconditionare skipped, and profiles that do activate contribute no repositories. Settings and CLI profiles are untouched.java.*,os.*,maven.*, the three separators, and the CI-friendlyrevision,changelistandsha1. Everything else, environment variables included, is left as a literal expression.maven.model.dependencyInterpolation.full=truerestores the previous behaviour.systemscope in imported dependency management. Dependency management imported from a POM resolved from a repository no longer contributes entries that declaresystemscope or asystemPath; those entries are dropped and a warning names them. Management imported from the reactor is unaffected, and so is asystemscope declared by the project's own POM.maven.repository.dependencyManagement.allowSystemScope=truerestores the previous behaviour.The last two change behaviour that has been in place for a long time, which is why each ships an opt-out property.
MavenITmng4379,MavenITmng4590andMavenITmng3586keep their original assertions behind the opt-out and each gains a companion test asserting what the new default does instead.MavenITmng3586#testitFromProject, where the project's own POM declares thesystemscope, needs no opt-out.Worth a maintainer's eye on the interpolation default in particular: MNG-4379 has resolved a
systemPathfrom an environment variable in a repository-served POM since 2009, and under the new default such a build fails on the existing "must specify an absolute path" validation rather than resolving.Which of these applies is decided by the model's source kind rather than by the request type: a model
built from
Sources.resolvedSource(...)is one Maven fetched from a repository, while a POM handed toMaven as a file reports no model id.
MavenITmng4765LocalPomProjectBuilderTestcovers the latter, andtwo unit tests assert a caller-supplied model is left alone.
ModelSource.getModelId()exists on master but not on this branch, so it is added here as a@Nullable defaultreturningnull, overridden inSources.ResolvedPathSource. That is an additive, binarycompatible change to
api/maven-api-core, and it keeps this PR and the master one using the samemechanism. Keying on whether the source has a path would not work: caller-supplied sources report none
either, so it would restrict exactly the models this is meant to leave alone.
Each change is a separate commit.
Draft while related code paths are reviewed.