diff --git a/api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java b/api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java index 7ed3befffac1..49670e36a6ec 100644 --- a/api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java +++ b/api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java @@ -816,5 +816,40 @@ public final class Constants { */ public static final String MAVEN_MODEL_PROCESSOR_REFERENCE_TYPE_PREFIX = "maven.model.processor.referenceType."; + /** + * User property for allowing {@code system} scope and {@code systemPath} in dependency + * management imported from repository-resolved POMs (BOMs). + * + * Dependency management imported from the local reactor is not affected by this property. + * + * @since 4.1.0 + */ + @Config(type = "java.lang.Boolean", defaultValue = "false") + public static final String MAVEN_REPOSITORY_DEPENDENCY_MANAGEMENT_ALLOW_SYSTEM_SCOPE = + "maven.repository.dependencyManagement.allowSystemScope"; + + /** + * User property for opting back into the previous behavior of interpolating + * repository-resolved models (dependencies and parents) against the full set of + * session properties (system, environment and CLI). + * + * + * @since 4.1.0 + */ + @Config(type = "java.lang.Boolean", defaultValue = "false") + public static final String MAVEN_MODEL_DEPENDENCY_INTERPOLATION_FULL = "maven.model.dependencyInterpolation.full"; + private Constants() {} } diff --git a/compat/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java b/compat/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java index 97a7b8b3c935..fbd1d692906f 100644 --- a/compat/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java +++ b/compat/maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java @@ -290,6 +290,11 @@ protected ModelBuildingResult build(ModelBuildingRequest request, Collection parentIds = new LinkedHashSet<>(); List lineage = new ArrayList<>(); + // 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. + boolean externalModel = isExternalModelBuildingRequest(request); + for (ModelData currentData = resultData; currentData != null; ) { lineage.add(currentData); @@ -309,8 +314,10 @@ protected ModelBuildingResult build(ModelBuildingRequest request, Collection interpolatedProfiles = getInterpolatedProfiles(rawModel, profileActivationContext, problems); tmpModel.setProfiles(interpolatedProfiles); - List activePomProfiles = - profileSelector.getActiveProfiles(tmpModel.getProfiles(), profileActivationContext, problems); + List activePomProfiles = profileSelector.getActiveProfiles( + externalModel ? withoutFileAndPropertyActivation(interpolatedProfiles) : tmpModel.getProfiles(), + profileActivationContext, + problems); List rawProfiles = new ArrayList<>(); for (Profile activePomProfile : activePomProfiles) { @@ -320,7 +327,11 @@ protected ModelBuildingResult build(ModelBuildingRequest request, Collection mutator) { return interpolatedActivations; } + /** + * Determines whether the given request builds a model for dependency resolution, i.e. a POM + * read from a remote repository (a dependency POM, one of its parents or an imported BOM) + * rather than a POM belonging to the project being built. Such requests are issued with + * {@link ModelBuildingRequest#VALIDATION_LEVEL_MINIMAL}, see for instance + * {@code DefaultArtifactDescriptorReader#loadPom}. + */ + private static boolean isExternalModelBuildingRequest(ModelBuildingRequest request) { + return request.getValidationLevel() < ModelBuildingRequest.VALIDATION_LEVEL_MAVEN_2_0; + } + + /** + * Returns the profiles from the given list whose activation does not depend on a file or a + * property. Profiles activated by JDK version, operating system, or marked + * {@code activeByDefault} are unaffected, since those conditions are a function of the build + * platform rather than of the model content. + */ + private static List withoutFileAndPropertyActivation(List profiles) { + List eligible = new ArrayList<>(profiles.size()); + for (Profile profile : profiles) { + Activation activation = profile.getActivation(); + if (activation == null || (activation.getFile() == null && activation.getProperty() == null)) { + eligible.add(profile); + } + } + return eligible; + } + + /** + * Returns a copy of the given profile with its repositories and plugin repositories cleared. + */ + private static Profile withoutRepositories(Profile profile) { + Profile stripped = profile.clone(); + stripped.setRepositories(Collections.emptyList()); + stripped.setPluginRepositories(Collections.emptyList()); + return stripped; + } + @Override public ModelBuildingResult build(ModelBuildingRequest request, ModelBuildingResult result) throws ModelBuildingException { diff --git a/compat/maven-model-builder/src/main/java/org/apache/maven/model/interpolation/AbstractStringBasedModelInterpolator.java b/compat/maven-model-builder/src/main/java/org/apache/maven/model/interpolation/AbstractStringBasedModelInterpolator.java index 13b81619a531..5469c2067054 100644 --- a/compat/maven-model-builder/src/main/java/org/apache/maven/model/interpolation/AbstractStringBasedModelInterpolator.java +++ b/compat/maven-model-builder/src/main/java/org/apache/maven/model/interpolation/AbstractStringBasedModelInterpolator.java @@ -50,6 +50,13 @@ */ @Deprecated(since = "4.0.0") public abstract class AbstractStringBasedModelInterpolator implements ModelInterpolator { + + /** + * Local mirror of {@code org.apache.maven.api.Constants.MAVEN_MODEL_DEPENDENCY_INTERPOLATION_FULL}. + * This compat module does not depend on {@code maven-api-core}, so the value is duplicated here. + */ + private static final String FULL_EXTERNAL_INTERPOLATION_PROPERTY = "maven.model.dependencyInterpolation.full"; + private static final List PROJECT_PREFIXES = Arrays.asList("pom.", "project."); private static final Collection TRANSLATED_PATH_EXPRESSIONS; @@ -156,25 +163,70 @@ public Object getValue(String expression) { valueSources.add(modelValueSource1); - valueSources.add(new MapBasedValueSource(config.getUserProperties())); + // Models built at VALIDATION_LEVEL_MINIMAL are the models Maven builds while resolving + // dependency, parent and BOM-import POMs from a repository, not the operator's own + // project. Such models interpolate only against their own properties and a small set + // of environment-independent expressions; everything else in the user/system property + // space stays uninterpolated. Operator project builds use a higher validation level and + // keep the full set of value sources, unchanged from previous behavior. + boolean restricted = restrictExternalModelInterpolation(config); + + ValueSource userPropertiesValueSource = new MapBasedValueSource(config.getUserProperties()); + valueSources.add(restricted ? restrictToSafeExpressions(userPropertiesValueSource) : userPropertiesValueSource); // Overwrite existing values in model properties. Otherwise, it's not possible // to define them via command line e.g.: mvn -Drevision=6.5.7 ... versionProcessor.overwriteModelProperties(modelProperties, config); valueSources.add(new MapBasedValueSource(modelProperties)); - valueSources.add(new MapBasedValueSource(config.getSystemProperties())); + ValueSource systemPropertiesValueSource = new MapBasedValueSource(config.getSystemProperties()); + valueSources.add( + restricted ? restrictToSafeExpressions(systemPropertiesValueSource) : systemPropertiesValueSource); + + if (!restricted) { + valueSources.add(new AbstractValueSource(false) { + @Override + public Object getValue(String expression) { + return config.getSystemProperties().getProperty("env." + expression); + } + }); + } + + valueSources.add(modelValueSource2); - valueSources.add(new AbstractValueSource(false) { + return valueSources; + } + + private static boolean restrictExternalModelInterpolation(ModelBuildingRequest config) { + return config.getValidationLevel() < ModelBuildingRequest.VALIDATION_LEVEL_MAVEN_2_0 + && !Boolean.parseBoolean(config.getSystemProperties().getProperty(FULL_EXTERNAL_INTERPOLATION_PROPERTY)) + && !Boolean.parseBoolean(config.getUserProperties().getProperty(FULL_EXTERNAL_INTERPOLATION_PROPERTY)); + } + + private static ValueSource restrictToSafeExpressions(ValueSource source) { + return new AbstractValueSource(false) { @Override public Object getValue(String expression) { - return config.getSystemProperties().getProperty("env." + expression); + return isSafeExternalExpression(expression) ? source.getValue(expression) : null; } - }); - - valueSources.add(modelValueSource2); + }; + } - return valueSources; + /** + * Expressions that models built at {@link ModelBuildingRequest#VALIDATION_LEVEL_MINIMAL} + * may still resolve from the session properties: JVM- and Maven-defined properties, plus + * the CI-friendly version properties (MNG-5895). All other expressions are left literal. + */ + private static boolean isSafeExternalExpression(String expression) { + return expression.startsWith("java.") + || expression.startsWith("os.") + || expression.startsWith("maven.") + || "file.separator".equals(expression) + || "path.separator".equals(expression) + || "line.separator".equals(expression) + || "revision".equals(expression) + || "changelist".equals(expression) + || "sha1".equals(expression); } protected List createPostProcessors( diff --git a/compat/maven-model-builder/src/test/java/org/apache/maven/model/building/ExternalModelProfileActivationTest.java b/compat/maven-model-builder/src/test/java/org/apache/maven/model/building/ExternalModelProfileActivationTest.java new file mode 100644 index 000000000000..f6163374a2be --- /dev/null +++ b/compat/maven-model-builder/src/test/java/org/apache/maven/model/building/ExternalModelProfileActivationTest.java @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.model.building; + +import java.util.Properties; + +import org.apache.maven.model.Model; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Models built at {@link ModelBuildingRequest#VALIDATION_LEVEL_MINIMAL} come from POMs resolved + * from a repository during dependency resolution (a dependency POM, one of its parents, or an + * imported BOM), see for instance {@code DefaultArtifactDescriptorReader#loadPom}. Their file and + * property activators are not evaluated, and their profiles contribute no repositories. A project + * build, at {@link ModelBuildingRequest#VALIDATION_LEVEL_STRICT}, still evaluates every activator. + * Platform-derived activation (JDK version, operating system, activeByDefault) is unaffected at + * either level. + */ +class ExternalModelProfileActivationTest { + + private static final String POM = "\n" + + " 4.0.0\n" + + " thegroup\n" + + " withprofiles\n" + + " 1\n" + + " pom\n" + + " \n" + + " \n" + + " file-condition\n" + + " \n" + + " \n" + + " ${some.dir}\n" + + " \n" + + " \n" + + " \n" + + " activated\n" + + " \n" + + " \n" + + " \n" + + " property-condition\n" + + " \n" + + " \n" + + " some.gating.property\n" + + " \n" + + " \n" + + " \n" + + " activated\n" + + " \n" + + " \n" + + " \n" + + " jdk-condition\n" + + " \n" + + " [1,)\n" + + " \n" + + " \n" + + " activated\n" + + " \n" + + " \n" + + " \n" + + " profile-repo\n" + + " https://repo.example.test/profile\n" + + " \n" + + " \n" + + " \n" + + " \n" + + "\n"; + + private Model build(int validationLevel) throws Exception { + ModelBuilder builder = new DefaultModelBuilderFactory().newInstance(); + + Properties systemProperties = new Properties(); + systemProperties.putAll(System.getProperties()); + systemProperties.setProperty("some.dir", System.getProperty("java.io.tmpdir")); + systemProperties.setProperty("some.gating.property", "true"); + + DefaultModelBuildingRequest request = new DefaultModelBuildingRequest(); + request.setModelSource(new StringModelSource(POM)); + request.setValidationLevel(validationLevel); + request.setSystemProperties(systemProperties); + + return builder.build(request).getEffectiveModel(); + } + + @Test + void testProjectBuildEvaluatesAllActivators() throws Exception { + Model model = build(ModelBuildingRequest.VALIDATION_LEVEL_STRICT); + + assertEquals("activated", model.getProperties().get("profile.file")); + assertEquals("activated", model.getProperties().get("profile.property")); + assertEquals("activated", model.getProperties().get("profile.jdk")); + assertTrue(model.getRepositories().stream().anyMatch(r -> "profile-repo".equals(r.getId()))); + } + + @Test + void testDependencyPomActivatesOnlyEnvironmentIndependentProfiles() throws Exception { + Model model = build(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL); + + assertNull(model.getProperties().get("profile.file")); + assertNull(model.getProperties().get("profile.property")); + assertEquals("activated", model.getProperties().get("profile.jdk")); + assertTrue(model.getRepositories().stream().noneMatch(r -> "profile-repo".equals(r.getId()))); + } +} diff --git a/compat/maven-model-builder/src/test/java/org/apache/maven/model/interpolation/AbstractModelInterpolatorTest.java b/compat/maven-model-builder/src/test/java/org/apache/maven/model/interpolation/AbstractModelInterpolatorTest.java index becdafdf6c60..029223b3368c 100644 --- a/compat/maven-model-builder/src/test/java/org/apache/maven/model/interpolation/AbstractModelInterpolatorTest.java +++ b/compat/maven-model-builder/src/test/java/org/apache/maven/model/interpolation/AbstractModelInterpolatorTest.java @@ -319,6 +319,61 @@ public void testEnvars() throws Exception { assertEquals("/path/to/home", out.getProperties().get("outputDirectory")); } + @Test + public void testMinimalValidationInterpolationUsesRestrictedPropertySet() throws Exception { + context.put("env.HOME", "/path/to/home"); + context.put("some.property", "other-value"); + context.put("java.version", "21"); + + Map modelProperties = new HashMap<>(); + modelProperties.put("envDir", "${env.HOME}"); + modelProperties.put("propDir", "${some.property}"); + modelProperties.put("jdk", "${java.version}"); + + Model model = new Model(org.apache.maven.api.model.Model.newBuilder() + .properties(modelProperties) + .build()); + + ModelInterpolator interpolator = createInterpolator(); + + final SimpleProblemCollector collector = new SimpleProblemCollector(); + ModelBuildingRequest config = createModelBuildingRequest(context); + config.setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL); + Model out = interpolator.interpolateModel(model, new File("."), config, collector); + assertProblemFree(collector); + + // At minimal validation level (the level used for models built while resolving + // dependency, parent and BOM-import POMs) env and arbitrary system/user properties + // stay literal... + assertEquals("${env.HOME}", out.getProperties().get("envDir")); + assertEquals("${some.property}", out.getProperties().get("propDir")); + // ...while JVM-defined and other well-known expressions keep resolving. + assertEquals("21", out.getProperties().get("jdk")); + } + + @Test + public void testFullInterpolationOptOutRestoresPreviousBehaviorAtMinimalValidationLevel() throws Exception { + context.put("env.HOME", "/path/to/home"); + context.put("maven.model.dependencyInterpolation.full", "true"); + + Map modelProperties = new HashMap<>(); + modelProperties.put("envDir", "${env.HOME}"); + + Model model = new Model(org.apache.maven.api.model.Model.newBuilder() + .properties(modelProperties) + .build()); + + ModelInterpolator interpolator = createInterpolator(); + + final SimpleProblemCollector collector = new SimpleProblemCollector(); + ModelBuildingRequest config = createModelBuildingRequest(context); + config.setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL); + Model out = interpolator.interpolateModel(model, new File("."), config, collector); + assertProblemFree(collector); + + assertEquals("/path/to/home", out.getProperties().get("envDir")); + } + @Test public void envarExpressionThatEvaluatesToNullReturnsTheLiteralString() throws Exception { diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java index 988e2956bb59..a2cee1684746 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java @@ -139,6 +139,7 @@ public class DefaultModelBuilder implements ModelBuilder { private static final String FILE = "file"; private static final String IMPORT = "import"; private static final String PARENT = "parent"; + private static final String PARENT_EXTERNAL = "parent-external"; private static final String MODEL = "model"; private final Logger logger = LoggerFactory.getLogger(getClass()); @@ -292,6 +293,14 @@ List getExternalRepositories() { // Contains both GAV coordinates (groupId:artifactId:version) and file paths final Set parentChain; + // Sticky across derive(): true for a session that is itself resolving a dependency + // (ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY), or that was derived, directly + // or transitively, from such a session -- for instance a dependency's own parent POM. + // Kept separate from request.getRequestType() because a parent lookup always derives a + // CONSUMER_PARENT request regardless of what kind of session triggered it, which would + // otherwise lose the distinction this flag preserves. + final boolean externalOrigin; + ModelBuilderSessionState(ModelBuilderRequest request) { this( request.getSession(), @@ -304,7 +313,8 @@ List getExternalRepositories() { List.of(), repos(request), repos(request), - new LinkedHashSet<>()); + new LinkedHashSet<>(), + isExternalOrigin(request)); } static List repos(ModelBuilderRequest request) { @@ -382,7 +392,8 @@ private ModelBuilderSessionState( List pomRepositories, List externalRepositories, List repositories, - Set parentChain) { + Set parentChain, + boolean externalOrigin) { this.session = session; this.request = request; this.result = result; @@ -394,6 +405,7 @@ private ModelBuilderSessionState( this.externalRepositories = externalRepositories; this.repositories = repositories; this.parentChain = parentChain; + this.externalOrigin = externalOrigin; this.result.setSource(this.request.getSource()); } @@ -447,6 +459,7 @@ private ModelBuilderSessionState derive( derivedRepos = repositoryFactory.aggregate(session, pomRepositories, derivedExtRepos, false); } } + boolean derivedExternalOrigin = externalOrigin || isExternalOrigin(request); return new ModelBuilderSessionState( session, request, @@ -458,7 +471,8 @@ private ModelBuilderSessionState derive( pomRepositories, derivedExtRepos, derivedRepos, - new LinkedHashSet<>()); + new LinkedHashSet<>(), + derivedExternalOrigin); } @Override @@ -661,7 +675,13 @@ public void mergeRepositories(Model model, boolean replace) { && (repo.getId() == null || !repo.getId().contains("${"))) .map(session::createRemoteRepository) .toList(); - if (replace) { + // Repositories contributed by a model resolved from a repository are merged + // recessively; repositories supplied by the request or session keep precedence. + // Note: the isBuildRequest() guard means any future non-build RequestType will + // also use recessive merging (the else branch). This is intentional — only a + // build request has a well-defined set of session/request repositories that + // should take precedence; dependency and parent resolution do not. + if (replace && isBuildRequest()) { Set ids = repos.stream().map(RemoteRepository::getId).collect(Collectors.toSet()); repositories = repositories.stream() .filter(r -> !ids.contains(r.getId())) @@ -1665,12 +1685,37 @@ private void addActivePomProfiles(String modelId, List activePomProfile private List getActiveProfiles( Collection interpolatedProfiles, DefaultProfileActivationContext profileActivationContext) { if (isBuildRequestWithActivation()) { - return profileSelector.getActiveProfiles(interpolatedProfiles, profileActivationContext, this); + Collection eligibleProfiles = interpolatedProfiles; + if (externalOrigin) { + // A model resolved to satisfy dependency resolution -- a dependency POM + // itself, or one of its parents, reached transitively -- evaluates only + // platform-derived activation (JDK version, operating system, + // activeByDefault); its profiles contribute no repositories. + eligibleProfiles = interpolatedProfiles.stream() + .filter(profile -> !hasFileOrPropertyOrConditionActivation(profile)) + .map(profile -> profile.withRepositories(List.of()).withPluginRepositories(List.of())) + .toList(); + } + return profileSelector.getActiveProfiles(eligibleProfiles, profileActivationContext, this); } else { return List.of(); } } + /** + * Determines whether the given profile's activation depends on file existence, a + * property, or a condition expression, as opposed to being a function of the build + * platform (JDK version, operating system) or {@code activeByDefault}. + */ + private static boolean hasFileOrPropertyOrConditionActivation(Profile profile) { + Activation activation = profile.getActivation(); + return activation != null + && (activation.getFile() != null + || activation.getProperty() != null + || (activation.getCondition() != null + && !activation.getCondition().isBlank())); + } + Model readFileModel() throws ModelBuilderException { return readFileModel(new HashSet<>()); } @@ -2072,8 +2117,23 @@ private record ParentModelWithProfiles(Model model, List activatedProfi */ Model readAsParentModel(DefaultProfileActivationContext profileActivationContext, Set parentChain) throws ModelBuilderException { + // Partition the cache by externalOrigin so a parent model resolved while building + // the operator's own project never shares an entry with the same source resolved + // while resolving a dependency: the two contexts activate profiles differently (see + // getActiveProfiles below), and the model built for one must not be reused for the + // other, even though both are keyed off the same underlying source. + // + // This partition is a defensive backstop, not the primary guard: cache(source, tag, + // supplier) additionally scopes each entry to the top-level request (see + // getOuterRequest()), which falls back to the request object's own identity once its + // RequestTrace has no further request-typed ancestor. Two independently-built request + // objects therefore land in different buckets regardless of this tag, and never reach + // this collision in practice; the tag matters only when two reads end up sharing a + // request object (as derive() calls from a common ancestor can), which is why it is + // kept even though the getActiveProfiles gate above already decides the correct + // activation for each read on its own. Map parentsPerContext = - cache(request.getSource(), PARENT, ConcurrentHashMap::new); + cache(request.getSource(), externalOrigin ? PARENT_EXTERNAL : PARENT, ConcurrentHashMap::new); for (Map.Entry e : parentsPerContext.entrySet()) { @@ -2395,11 +2455,13 @@ private Model doLoadDependencyManagement( Collection importIds) { Model importModel; ModelSource importSource; + boolean repositoryResolved = false; try { importSource = resolveReactorModel(groupId, artifactId, version); if (importSource == null) { importSource = modelResolver.resolveModel( request.getSession(), repositories, dependency, new AtomicReference<>()); + repositoryResolved = true; } } catch (ModelBuilderException | ModelResolverException e) { StringBuilder buffer = new StringBuilder(256); @@ -2450,9 +2512,68 @@ private Model doLoadDependencyManagement( importModel = importResult.getEffectiveModel(); + if (repositoryResolved) { + importModel = rejectSystemScopeFromRepositoryImport(importModel, dependency); + } + return importModel; } + /** + * Dependency management imported (as a BOM) from a POM resolved from a repository, rather + * than from the local reactor, may not declare {@code system} scope or a + * {@code systemPath} for a managed dependency: by default, offending entries are dropped + * from the imported management (so a cached import cannot re-introduce them) and a + * warning is emitted, unless the + * {@code maven.repository.dependencyManagement.allowSystemScope} user property is set to + * {@code true}, in which case they are imported as before, with a warning. Dependency + * management imported from the local reactor is not affected. + */ + private Model rejectSystemScopeFromRepositoryImport(Model importModel, Dependency dependency) { + DependencyManagement importMgmt = importModel != null ? importModel.getDependencyManagement() : null; + if (importMgmt == null) { + return importModel; + } + String offending = importMgmt.getDependencies().stream() + .filter(DefaultModelBuilder::usesSystemScope) + .map(Dependency::getManagementKey) + .collect(Collectors.joining(", ")); + if (offending.isEmpty()) { + return importModel; + } + String allow = request.getUserProperties() + .getOrDefault( + Constants.MAVEN_REPOSITORY_DEPENDENCY_MANAGEMENT_ALLOW_SYSTEM_SCOPE, + request.getSystemProperties() + .get(Constants.MAVEN_REPOSITORY_DEPENDENCY_MANAGEMENT_ALLOW_SYSTEM_SCOPE)); + if (Boolean.parseBoolean(allow)) { + add( + Severity.WARNING, + Version.V41, + "The import POM " + ModelProblemUtils.toId(importModel) + + " declares 'system' scope or 'systemPath' for " + offending + + "; importing it because the '" + + Constants.MAVEN_REPOSITORY_DEPENDENCY_MANAGEMENT_ALLOW_SYSTEM_SCOPE + + "' user property is set to 'true'.", + dependency.getLocation("")); + return importModel; + } + add( + Severity.WARNING, + Version.V41, + "The import POM " + ModelProblemUtils.toId(importModel) + + " was resolved from a repository and declares 'system' scope or 'systemPath' for " + + offending + "; these entries are not imported. Remove the 'system' scope from the" + + " imported POM, or set the '" + + Constants.MAVEN_REPOSITORY_DEPENDENCY_MANAGEMENT_ALLOW_SYSTEM_SCOPE + + "' user property to 'true' to import them as before.", + dependency.getLocation("")); + List retained = importMgmt.getDependencies().stream() + .filter(d -> !usesSystemScope(d)) + .collect(Collectors.toList()); + return importModel.withDependencyManagement(importMgmt.withDependencies(retained)); + } + ModelSource resolveReactorModel(String groupId, String artifactId, String version) throws ModelBuilderException { Set sources = mappedSources.get(new GAKey(groupId, artifactId)); @@ -2608,6 +2729,24 @@ static String getVersion(Model model) { return version; } + /** + * Whether the model this request builds was resolved from a repository rather than supplied to + * Maven. {@link org.apache.maven.api.services.Sources#resolvedSource} carries the resolved + * model's coordinates and is the only source kind that does; a POM built from a file the caller + * pointed at reports none. + */ + static boolean isExternalOrigin(ModelBuilderRequest request) { + return request.getRequestType() == ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY + && request.getSource() != null + && request.getSource().getModelId() != null; + } + + static boolean usesSystemScope(Dependency dependency) { + return "system".equals(dependency.getScope()) + || (dependency.getSystemPath() != null + && !dependency.getSystemPath().isEmpty()); + } + private DefaultProfileActivationContext getProfileActivationContext(ModelBuilderRequest request, Model model) { return new DefaultProfileActivationContext( pathTranslator, diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java index 1932f3fc5515..2effe0f66bdf 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelInterpolator.java @@ -31,6 +31,7 @@ import java.util.function.Function; import java.util.function.UnaryOperator; +import org.apache.maven.api.Constants; import org.apache.maven.api.di.Inject; import org.apache.maven.api.di.Named; import org.apache.maven.api.di.Singleton; @@ -41,6 +42,7 @@ import org.apache.maven.api.services.ModelBuilderRequest; import org.apache.maven.api.services.ModelProblem; import org.apache.maven.api.services.ModelProblemCollector; +import org.apache.maven.api.services.ModelSource; import org.apache.maven.api.services.model.ModelInterpolator; import org.apache.maven.api.services.model.PathTranslator; import org.apache.maven.api.services.model.RootLocator; @@ -202,8 +204,19 @@ String doCallback( return new MavenBuildTimestamp(request.getSession().getStartTime(), model.getProperties()) .formattedTimestamp(); } + // Models built at ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY are the models + // Maven builds while resolving a dependency POM from a repository, not the operator's + // own project (see DefaultModelBuilder#resolveAndReadParentExternally and + // DefaultArtifactDescriptorReader for the equivalent path used before this builder is + // invoked). Such models interpolate their user/system properties only against a small, + // environment-independent allowlist; everything else, including environment variables + // and arbitrary -D properties, is left as a literal unresolved expression. + boolean restricted = restrictExternalModelInterpolation(request); + // user properties - String value = request.getUserProperties().get(expression); + String value = (!restricted || isSafeExternalExpression(expression)) + ? request.getUserProperties().get(expression) + : null; // model properties (check before prefixed model reflection to avoid recursion) if (value == null) { value = model.getProperties().get(expression); @@ -221,11 +234,11 @@ String doCallback( } } // system properties - if (value == null) { + if (value == null && (!restricted || isSafeExternalExpression(expression))) { value = request.getSystemProperties().get(expression); } // environment variables - if (value == null) { + if (value == null && !restricted) { value = request.getSystemProperties().get("env." + expression); } // un-prefixed model reflection @@ -235,6 +248,46 @@ String doCallback( return value; } + private static boolean restrictExternalModelInterpolation(ModelBuilderRequest request) { + ModelBuilderRequest.RequestType type = request.getRequestType(); + boolean externalModel = (type == ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY + || type == ModelBuilderRequest.RequestType.CONSUMER_PARENT) + && isRepositoryResolved(request.getSource()); + return externalModel + && !Boolean.parseBoolean( + request.getSystemProperties().get(Constants.MAVEN_MODEL_DEPENDENCY_INTERPOLATION_FULL)) + && !Boolean.parseBoolean( + request.getUserProperties().get(Constants.MAVEN_MODEL_DEPENDENCY_INTERPOLATION_FULL)); + } + + /** + * Whether the given source is one Maven resolved from a repository rather than one supplied to + * it. {@link org.apache.maven.api.services.Sources#resolvedSource} carries the resolved model's + * coordinates, and it is the only source kind that does; a POM built from a file the caller + * pointed at reports none. + */ + private static boolean isRepositoryResolved(ModelSource source) { + return source != null && source.getModelId() != null; + } + + /** + * Expressions that models built at {@link ModelBuilderRequest.RequestType#CONSUMER_DEPENDENCY} + * or {@link ModelBuilderRequest.RequestType#CONSUMER_PARENT} may still resolve from the + * session properties: JVM- and Maven-defined properties, plus the CI-friendly version + * properties (MNG-5895). All other expressions are left literal. + */ + private static boolean isSafeExternalExpression(String expression) { + return expression.startsWith("java.") + || expression.startsWith("os.") + || expression.startsWith("maven.") + || "file.separator".equals(expression) + || "path.separator".equals(expression) + || "line.separator".equals(expression) + || "revision".equals(expression) + || "changelist".equals(expression) + || "sha1".equals(expression); + } + String projectProperty(Model model, Path projectDir, String subExpr, boolean prefixed) { if (projectDir != null) { if (subExpr.equals("basedir")) { diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java index e88cb09ca53a..cf52c6b1e547 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java @@ -49,6 +49,7 @@ import org.apache.maven.api.model.Model; import org.apache.maven.api.model.Profile; import org.apache.maven.api.model.Repository; +import org.apache.maven.api.services.BuilderProblem; import org.apache.maven.api.services.ModelBuilder; import org.apache.maven.api.services.ModelBuilderRequest; import org.apache.maven.api.services.ModelBuilderResult; @@ -129,6 +130,285 @@ public void testPropertiesAndProfiles() { assertEquals("21", result.getEffectiveModel().getProperties().get("maven.compiler.release")); } + /** + * Models built at {@link ModelBuilderRequest.RequestType#CONSUMER_DEPENDENCY} come from a + * dependency POM resolved from a repository. Their file, property, and condition activators + * are not evaluated, and their profiles contribute no repositories. A project build, at + * {@link ModelBuilderRequest.RequestType#BUILD_PROJECT}, still evaluates every activator. + * Platform-derived activation (JDK version, operating system, activeByDefault) is unaffected + * at either level. + */ + private ModelBuilderRequest.ModelBuilderRequestBuilder resolvedProfilesRequest( + ModelBuilderRequest.RequestType requestType) { + Map systemProperties = new HashMap<>(); + for (String name : System.getProperties().stringPropertyNames()) { + systemProperties.put(name, System.getProperty(name)); + } + systemProperties.put("some.dir", System.getProperty("java.io.tmpdir")); + systemProperties.put("some.gating.property", "true"); + systemProperties.put("some.condition.property", "true"); + return ModelBuilderRequest.builder() + .session(session) + .requestType(requestType) + .systemProperties(systemProperties) + .source(Sources.buildSource(getPom("resolved-model-with-profiles"))); + } + + @Test + public void testProjectBuildEvaluatesAllActivators() { + ModelBuilderRequest request = resolvedProfilesRequest(ModelBuilderRequest.RequestType.BUILD_PROJECT) + .build(); + Model model = builder.newSession().build(request).getEffectiveModel(); + + assertEquals("activated", model.getProperties().get("profile.file")); + assertEquals("activated", model.getProperties().get("profile.property")); + assertEquals("activated", model.getProperties().get("profile.condition")); + assertEquals("activated", model.getProperties().get("profile.jdk")); + assertTrue(model.getRepositories().stream().anyMatch(r -> "profile-repo".equals(r.getId()))); + } + + @Test + public void testDependencyModelActivatesOnlyEnvironmentIndependentProfiles() { + ModelBuilderRequest request = resolvedProfilesRequest(ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY) + .source(Sources.resolvedSource( + getPom("resolved-model-with-profiles"), + "org.apache.maven.test:resolved-model-with-profiles:1.0.0")) + .build(); + Model model = builder.newSession().build(request).getEffectiveModel(); + + assertNull(model.getProperties().get("profile.file")); + assertNull(model.getProperties().get("profile.property")); + assertNull(model.getProperties().get("profile.condition")); + assertEquals("activated", model.getProperties().get("profile.jdk")); + assertTrue(model.getRepositories().stream().noneMatch(r -> "profile-repo".equals(r.getId()))); + } + + /** + * A model built at {@link ModelBuilderRequest.RequestType#CONSUMER_DEPENDENCY} whose source + * is one Maven was merely pointed at -- {@link Sources#buildSource} rather than a source + * Maven resolved from a repository -- is not treated as coming from a repository. Every + * activator still runs, exactly as for a project build. + */ + @Test + public void testCallerSuppliedModelActivatesAllProfiles() { + ModelBuilderRequest request = resolvedProfilesRequest(ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY) + .build(); + Model model = builder.newSession().build(request).getEffectiveModel(); + + assertEquals("activated", model.getProperties().get("profile.file")); + assertEquals("activated", model.getProperties().get("profile.property")); + assertEquals("activated", model.getProperties().get("profile.condition")); + assertEquals("activated", model.getProperties().get("profile.jdk")); + assertTrue(model.getRepositories().stream().anyMatch(r -> "profile-repo".equals(r.getId()))); + } + + private Map parentActivationSystemProperties() { + Map systemProperties = new HashMap<>(); + for (String name : System.getProperties().stringPropertyNames()) { + systemProperties.put(name, System.getProperty(name)); + } + systemProperties.put("some.dir", System.getProperty("java.io.tmpdir")); + systemProperties.put("some.gating.property", "true"); + systemProperties.put("some.condition.property", "true"); + return systemProperties; + } + + private DefaultProfileActivationContext parentActivationContext(Map systemProperties) { + org.apache.maven.api.services.Lookup lookup = session.getService(org.apache.maven.api.services.Lookup.class); + return new DefaultProfileActivationContext( + lookup.lookup(org.apache.maven.api.services.model.PathTranslator.class), + lookup.lookup(org.apache.maven.api.services.model.RootLocator.class), + lookup.lookup(org.apache.maven.api.services.Interpolator.class), + List.of(), + List.of(), + systemProperties, + Map.of(), + Model.newInstance()); + } + + /** + * readAsParentModel() caches by (source, tag), where tag depends on externalOrigin, through + * a generic per-request cache keyed off the request object itself. To exercise that + * partition directly -- independent of how deep a real build's RequestTrace ancestry happens + * to be -- both reads below derive from the exact same request instance, differing only in + * which parent session state (one already externalOrigin=true, one false) they derive from. + * That guarantees both calls address the same underlying per-request cache bucket, so this + * proves the tag partition itself separates them, not an accidental difference elsewhere. + */ + @Test + public void testResolvedDependencyParentCacheDoesNotShareActivationWithProjectParent() throws Exception { + // Trigger mainSession creation with a regular project build. + ModelBuilderRequest request = ModelBuilderRequest.builder() + .session(session) + .requestType(ModelBuilderRequest.RequestType.BUILD_PROJECT) + .source(Sources.buildSource(getPom("props-and-profiles"))) + .build(); + ModelBuilder.ModelBuilderSession mbs = builder.newSession(); + mbs.build(request); + DefaultModelBuilder.ModelBuilderSessionState mainState = + ((DefaultModelBuilder.ModelBuilderSessionImpl) mbs).mainSession; + + Map systemProperties = parentActivationSystemProperties(); + ModelSource parentSource = Sources.buildSource(getPom("resolved-model-with-profiles")); + + // A session already carrying externalOrigin=true, as a dependency's own session would + // after resolving through at least one CONSUMER_DEPENDENCY hop. + DefaultModelBuilder.ModelBuilderSessionState dependencyAncestorState = + mainState.derive(ModelBuilderRequest.builder(mainState.request) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY) + .source(Sources.resolvedSource( + getPom("props-and-profiles"), "org.apache.maven.test:props-and-profiles:1.0.0")) + .build()); + assertTrue(dependencyAncestorState.externalOrigin); + + // The exact same request instance is then used to derive a parent-lookup session from + // each ancestor. Sharing one request object guarantees both reads address the same + // per-request cache bucket, regardless of RequestTrace ancestry depth. + ModelBuilderRequest sharedParentRequest = ModelBuilderRequest.builder(mainState.request) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_PARENT) + .source(parentSource) + .systemProperties(systemProperties) + .build(); + + DefaultModelBuilder.ModelBuilderSessionState projectState = mainState.derive(sharedParentRequest); + assertFalse(projectState.externalOrigin, "derived from a BUILD_PROJECT ancestor, must not be external"); + Model projectParentModel = + projectState.readAsParentModel(parentActivationContext(systemProperties), new HashSet<>()); + + assertEquals("activated", projectParentModel.getProperties().get("profile.file")); + assertEquals("activated", projectParentModel.getProperties().get("profile.property")); + assertEquals("activated", projectParentModel.getProperties().get("profile.condition")); + assertEquals("activated", projectParentModel.getProperties().get("profile.jdk")); + assertTrue(projectParentModel.getRepositories().stream().anyMatch(r -> "profile-repo".equals(r.getId()))); + + DefaultModelBuilder.ModelBuilderSessionState dependencyState = + dependencyAncestorState.derive(sharedParentRequest); + assertTrue(dependencyState.externalOrigin, "derived from a CONSUMER_DEPENDENCY ancestor, must stay external"); + Model dependencyParentModel = + dependencyState.readAsParentModel(parentActivationContext(systemProperties), new HashSet<>()); + + assertNull(dependencyParentModel.getProperties().get("profile.file")); + assertNull(dependencyParentModel.getProperties().get("profile.property")); + assertNull(dependencyParentModel.getProperties().get("profile.condition")); + assertEquals("activated", dependencyParentModel.getProperties().get("profile.jdk")); + assertTrue(dependencyParentModel.getRepositories().stream().noneMatch(r -> "profile-repo".equals(r.getId()))); + + // The first (fully activated) result must not have been altered by the second read. + assertEquals("activated", projectParentModel.getProperties().get("profile.file")); + assertTrue(projectParentModel.getRepositories().stream().anyMatch(r -> "profile-repo".equals(r.getId()))); + + // Repeating with a second, fresh shared request -- dependency read first this time -- + // must not bleed the other way either. + ModelBuilderRequest sharedParentRequest2 = ModelBuilderRequest.builder(mainState.request) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_PARENT) + .source(parentSource) + .systemProperties(systemProperties) + .build(); + + DefaultModelBuilder.ModelBuilderSessionState dependencyState2 = + dependencyAncestorState.derive(sharedParentRequest2); + Model dependencyParentModel2 = + dependencyState2.readAsParentModel(parentActivationContext(systemProperties), new HashSet<>()); + assertNull(dependencyParentModel2.getProperties().get("profile.file")); + assertTrue(dependencyParentModel2.getRepositories().stream().noneMatch(r -> "profile-repo".equals(r.getId()))); + + DefaultModelBuilder.ModelBuilderSessionState projectState2 = mainState.derive(sharedParentRequest2); + Model projectParentModel2 = + projectState2.readAsParentModel(parentActivationContext(systemProperties), new HashSet<>()); + assertEquals("activated", projectParentModel2.getProperties().get("profile.file")); + assertTrue(projectParentModel2.getRepositories().stream().anyMatch(r -> "profile-repo".equals(r.getId()))); + } + + /** + * The externalOrigin flag must survive more than one {@code derive()} hop: a dependency's + * own parent (itself read as a CONSUMER_PARENT session, not CONSUMER_DEPENDENCY) must still + * be treated as external, since request.getRequestType() alone cannot carry that distinction + * once a parent lookup has overwritten it. + */ + @Test + public void testExternalOriginPropagatesThroughGrandparentHop() throws Exception { + ModelBuilderRequest request = ModelBuilderRequest.builder() + .session(session) + .requestType(ModelBuilderRequest.RequestType.BUILD_PROJECT) + .source(Sources.buildSource(getPom("props-and-profiles"))) + .build(); + ModelBuilder.ModelBuilderSession mbs = builder.newSession(); + mbs.build(request); + DefaultModelBuilder.ModelBuilderSessionState mainState = + ((DefaultModelBuilder.ModelBuilderSessionImpl) mbs).mainSession; + + Map systemProperties = parentActivationSystemProperties(); + ModelSource dependencySource = + Sources.resolvedSource(getPom("props-and-profiles"), "org.apache.maven.test:props-and-profiles:1.0.0"); + ModelSource grandparentSource = Sources.buildSource(getPom("resolved-model-with-profiles")); + + // Hop 1: a dependency's own POM, resolved from a repository. externalOrigin becomes + // true here. + DefaultModelBuilder.ModelBuilderSessionState dependencyState = + mainState.derive(ModelBuilderRequest.builder(mainState.request) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY) + .source(dependencySource) + .systemProperties(systemProperties) + .build()); + assertTrue(dependencyState.externalOrigin); + + // Hop 2: that dependency's own parent. Its own request type is CONSUMER_PARENT, not + // CONSUMER_DEPENDENCY -- externalOrigin must still be true, inherited from hop 1. + DefaultModelBuilder.ModelBuilderSessionState grandparentHopState = + dependencyState.derive(ModelBuilderRequest.builder(dependencyState.request) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_PARENT) + .source(grandparentSource) + .systemProperties(systemProperties) + .build()); + assertTrue( + grandparentHopState.externalOrigin, + "externalOrigin must survive a second derive() hop, not just the first"); + + Model grandparentModel = + grandparentHopState.readAsParentModel(parentActivationContext(systemProperties), new HashSet<>()); + + assertNull(grandparentModel.getProperties().get("profile.file")); + assertNull(grandparentModel.getProperties().get("profile.property")); + assertNull(grandparentModel.getProperties().get("profile.condition")); + assertEquals("activated", grandparentModel.getProperties().get("profile.jdk")); + assertTrue(grandparentModel.getRepositories().stream().noneMatch(r -> "profile-repo".equals(r.getId()))); + } + + /** + * {@code BUILD_CONSUMER} requests already skip all profile activation + * ({@code isBuildRequestWithActivation()} returns false for that type) -- unrelated to, and + * unaffected by, the externalOrigin distinction. Locking that down explicitly since it is + * adjacent code this change reads but does not modify. + */ + @Test + public void testBuildConsumerSkipsAllProfileActivation() throws Exception { + ModelBuilderRequest request = ModelBuilderRequest.builder() + .session(session) + .requestType(ModelBuilderRequest.RequestType.BUILD_PROJECT) + .source(Sources.buildSource(getPom("props-and-profiles"))) + .build(); + ModelBuilder.ModelBuilderSession mbs = builder.newSession(); + mbs.build(request); + DefaultModelBuilder.ModelBuilderSessionState mainState = + ((DefaultModelBuilder.ModelBuilderSessionImpl) mbs).mainSession; + + Map systemProperties = parentActivationSystemProperties(); + DefaultModelBuilder.ModelBuilderSessionState buildConsumerState = + mainState.derive(ModelBuilderRequest.builder(mainState.request) + .requestType(ModelBuilderRequest.RequestType.BUILD_CONSUMER) + .source(Sources.buildSource(getPom("resolved-model-with-profiles"))) + .systemProperties(systemProperties) + .build()); + + Model model = buildConsumerState.readAsParentModel(parentActivationContext(systemProperties), new HashSet<>()); + + assertNull(model.getProperties().get("profile.file")); + assertNull(model.getProperties().get("profile.property")); + assertNull(model.getProperties().get("profile.condition")); + assertNull(model.getProperties().get("profile.jdk")); + assertTrue(model.getRepositories().stream().noneMatch(r -> "profile-repo".equals(r.getId()))); + } + @Test void testMappedSourcesSupportsConcurrentUpdates() throws Exception { ModelBuilderRequest request = ModelBuilderRequest.builder() @@ -272,6 +552,67 @@ public void testMergeRepositories() throws Exception { assertEquals("central", repositories.get(2).getId()); // default } + /** + * Repositories contributed by a model resolved from a repository are merged recessively: + * the repositories already held by the request or session -- e.g. "central" -- keep their + * id and URL. readEffectiveModel() requests replace semantics for every request type, so + * this is exercised directly against {@link DefaultModelBuilder.ModelBuilderSessionState}. + */ + @Test + public void testResolvedPomRepositoryMergedRecessively() throws Exception { + // Trigger mainSession creation with a regular project build. + ModelBuilderRequest request = ModelBuilderRequest.builder() + .session(session) + .requestType(ModelBuilderRequest.RequestType.BUILD_PROJECT) + .source(Sources.buildSource(getPom("props-and-profiles"))) + .build(); + ModelBuilder.ModelBuilderSession mbs = builder.newSession(); + mbs.build(request); + DefaultModelBuilder.ModelBuilderSessionState mainState = + ((DefaultModelBuilder.ModelBuilderSessionImpl) mbs).mainSession; + + // A model resolved from a repository (e.g. a dependency's POM) derives its own session + // state the same way DefaultArtifactDescriptorReader.loadPom does. + ModelBuilderRequest dependencyRequest = ModelBuilderRequest.builder() + .session(session) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY) + .repositoryMerging(ModelBuilderRequest.RepositoryMerging.REQUEST_DOMINANT) + .source(Sources.resolvedSource(getPom("props-and-profiles"), "org.example:downloaded:1.0.0")) + .build(); + DefaultModelBuilder.ModelBuilderSessionState state = mainState.derive(dependencyRequest); + + RemoteRepository central = state.getRepositories().stream() + .filter(r -> "central".equals(r.getId())) + .findFirst() + .orElseThrow(); + + // The resolved model declares a repository that reuses the "central" id. + Model model = Model.newBuilder() + .repositories(List.of(Repository.newBuilder() + .id("central") + .url("https://secondary.example/m2") + .build())) + .build(); + + // readEffectiveModel() requests replace semantics for every request type. + state.mergeRepositories(model, true); + + List repositories = state.getRepositories(); + RemoteRepository mergedCentral = repositories.stream() + .filter(r -> "central".equals(r.getId())) + .findFirst() + .orElseThrow(); + assertEquals( + central.getUrl(), + mergedCentral.getUrl(), + "repository declared by a resolved model must be merged recessively, keeping the" + + " existing repository's URL under a shared id"); + assertTrue( + repositories.stream().noneMatch(r -> r.getUrl().contains("secondary.example")), + "repository declared by a resolved model must not enter the resolution repositories" + + " under an id it does not own"); + } + /** * Verifies that when multiple repositories share the same ID (e.g., after mirror injection * maps both "central" and a profile-defined repo to the same mirror ID), their policies are @@ -970,6 +1311,126 @@ public void testBomTypeImpliesImportWithoutScope() { assertEquals("0.1", managed.getVersion()); } + /** + * Dependency management imported from a repository-resolved POM does not contribute + * {@code system} scope or a {@code systemPath}; such entries are dropped with a warning. + * + *

The imported BOM is written into a temporary remote repository at run time, with a + * {@code systemPath} that is a real absolute path on whichever OS the test runs on + * (a POSIX-only path such as {@code /etc/...} is not absolute on Windows, which would make + * model validation reject the entry before the code under test ever ran). + */ + @Test + public void testSystemScopeIgnoredOutsideProjectDeclaration(@TempDir Path tempDir) throws Exception { + Path basedir = Paths.get(System.getProperty("basedir", "")); + Path remoteRepoPath = tempDir.resolve("remote-repo"); + Path bomDir = remoteRepoPath.resolve("org/apache/maven/its/system-scope-bom/1.0"); + Files.createDirectories(bomDir); + + Path systemPathFile = tempDir.resolve("provided-tool.jar"); + Files.createFile(systemPathFile); + + String bomPom = "\n" + "\n" + + " 4.0.0\n" + + " org.apache.maven.its\n" + + " system-scope-bom\n" + + " 1.0\n" + + " pom\n" + + " \n" + + " \n" + + " \n" + + " org.apache.maven.its\n" + + " system-scope-companion\n" + + " 1.0\n" + + " \n" + + " \n" + + " org.apache.maven.its\n" + + " system-scope-dep\n" + + " 1.0\n" + + " system\n" + + " " + + systemPathFile.toAbsolutePath() + "\n" + " \n" + + " \n" + + " \n" + + "\n"; + Files.writeString(bomDir.resolve("system-scope-bom-1.0.pom"), bomPom); + + // default: the build succeeds, the offending entry is dropped, and a warning is emitted + Path localRepoPath = basedir.resolve("target/local-repo-system-scope-bom-reject"); + Session rejectSession = ApiRunner.createSession( + injector -> injector.bindInstance(DefaultModelBuilderTest.class, this), localRepoPath); + RemoteRepository remoteRepository = rejectSession.createRemoteRepository( + RemoteRepository.CENTRAL_ID, remoteRepoPath.toUri().toString()); + rejectSession = rejectSession.withRemoteRepositories(List.of(remoteRepository)); + ModelBuilder rejectBuilder = rejectSession.getService(ModelBuilder.class); + + ModelBuilderRequest request = ModelBuilderRequest.builder() + .session(rejectSession) + .requestType(ModelBuilderRequest.RequestType.BUILD_PROJECT) + .source(Sources.buildSource(getPom("import-system-scope-bom"))) + .build(); + ModelBuilderResult rejectResult = rejectBuilder.newSession().build(request); + DependencyManagement rejectManagement = rejectResult.getEffectiveModel().getDependencyManagement(); + assertNotNull( + rejectManagement.getDependencies().stream() + .filter(d -> "system-scope-companion".equals(d.getArtifactId())) + .findFirst() + .orElse(null), + "The import itself must have happened: the ordinary managed entry from the " + + "imported BOM should be present"); + Dependency rejected = rejectManagement.getDependencies().stream() + .filter(d -> "system-scope-dep".equals(d.getArtifactId())) + .findFirst() + .orElse(null); + assertNull(rejected, "By default the 'system' scope managed entry should not be imported"); + assertTrue( + rejectResult + .getProblemCollector() + .problems() + .anyMatch(p -> p.getSeverity() == BuilderProblem.Severity.WARNING + && p.getMessage().contains("'system' scope or 'systemPath'") + && p.getMessage() + .contains(Constants.MAVEN_REPOSITORY_DEPENDENCY_MANAGEMENT_ALLOW_SYSTEM_SCOPE)), + "Expected a warning about 'system' scope in the repository-imported BOM"); + + // explicit opt-out (fresh session/local repo, so the sanitized import is not served from the cache) + Path allowedLocalRepoPath = basedir.resolve("target/local-repo-system-scope-bom-allow"); + Session allowedSession = ApiRunner.createSession( + injector -> injector.bindInstance(DefaultModelBuilderTest.class, this), allowedLocalRepoPath); + allowedSession = allowedSession.withRemoteRepositories(List.of(allowedSession.createRemoteRepository( + RemoteRepository.CENTRAL_ID, remoteRepoPath.toUri().toString()))); + ModelBuilder allowedBuilder = allowedSession.getService(ModelBuilder.class); + + ModelBuilderRequest allowed = ModelBuilderRequest.builder() + .session(allowedSession) + .requestType(ModelBuilderRequest.RequestType.BUILD_PROJECT) + .userProperties(Map.of(Constants.MAVEN_REPOSITORY_DEPENDENCY_MANAGEMENT_ALLOW_SYSTEM_SCOPE, "true")) + .source(Sources.buildSource(getPom("import-system-scope-bom"))) + .build(); + ModelBuilderResult result = allowedBuilder.newSession().build(allowed); + DependencyManagement allowedManagement = result.getEffectiveModel().getDependencyManagement(); + assertNotNull( + allowedManagement.getDependencies().stream() + .filter(d -> "system-scope-companion".equals(d.getArtifactId())) + .findFirst() + .orElse(null), + "The import itself must have happened: the ordinary managed entry from the " + + "imported BOM should be present"); + Dependency managed = allowedManagement.getDependencies().stream() + .filter(d -> "system-scope-dep".equals(d.getArtifactId())) + .findFirst() + .orElse(null); + assertNotNull(managed, "With the opt-out property the managed entry should be imported"); + assertEquals("system", managed.getScope()); + assertTrue( + result.getProblemCollector() + .problems() + .anyMatch(p -> p.getSeverity() == BuilderProblem.Severity.WARNING + && p.getMessage() + .contains(Constants.MAVEN_REPOSITORY_DEPENDENCY_MANAGEMENT_ALLOW_SYSTEM_SCOPE)), + "Opting out should still emit a warning about the imported 'system' scope"); + } + @Test void testBomImportWarningsReportedWhereImportsAreDeclared() { Path pom = Paths.get("src/test/resources/poms/factory/mng-8450/pom.xml").toAbsolutePath(); diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java index 065bc79ecd5e..4ee3e14c599d 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelInterpolatorTest.java @@ -36,6 +36,7 @@ import com.google.common.jimfs.Configuration; import com.google.common.jimfs.Jimfs; +import org.apache.maven.api.Constants; import org.apache.maven.api.Session; import org.apache.maven.api.di.Priority; import org.apache.maven.api.di.Provides; @@ -48,6 +49,7 @@ import org.apache.maven.api.model.Scm; import org.apache.maven.api.services.Lookup; import org.apache.maven.api.services.ModelBuilderRequest; +import org.apache.maven.api.services.Sources; import org.apache.maven.api.services.model.ModelInterpolator; import org.apache.maven.api.services.model.RootLocator; import org.apache.maven.impl.model.profile.SimpleProblemCollector; @@ -443,6 +445,101 @@ public void testEnvars() throws Exception { assertEquals("/path/to/home", out.getProperties().get("outputDirectory")); } + @Test + public void testDependencyModelInterpolationUsesRestrictedPropertySet() throws Exception { + context.put("env.HOME", "/path/to/home"); + context.put("some.property", "other-value"); + context.put("java.version", "21"); + + Map modelProperties = new HashMap<>(); + modelProperties.put("envDir", "${env.HOME}"); + modelProperties.put("propDir", "${some.property}"); + modelProperties.put("jdk", "${java.version}"); + + Model model = Model.newBuilder().properties(modelProperties).build(); + + final SimpleProblemCollector collector = new SimpleProblemCollector(); + ModelBuilderRequest request = createModelBuildingRequest(context) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY) + .source(Sources.resolvedSource(Paths.get("."), "org.apache.maven.test:dependency:1.0.0")) + .build(); + Model out = interpolator.interpolateModel(model, Paths.get("."), request, collector); + assertProblemFree(collector); + + // A model built while resolving a dependency POM from a repository does not interpolate + // environment variables or arbitrary system/user properties... + assertEquals("${env.HOME}", out.getProperties().get("envDir")); + assertEquals("${some.property}", out.getProperties().get("propDir")); + // ...while JVM-defined and other well-known expressions keep resolving. + assertEquals("21", out.getProperties().get("jdk")); + } + + @Test + public void testParentModelInterpolationUsesRestrictedPropertySet() throws Exception { + context.put("env.HOME", "/path/to/home"); + + Map modelProperties = new HashMap<>(); + modelProperties.put("envDir", "${env.HOME}"); + + Model model = Model.newBuilder().properties(modelProperties).build(); + + final SimpleProblemCollector collector = new SimpleProblemCollector(); + ModelBuilderRequest request = createModelBuildingRequest(context) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_PARENT) + .source(Sources.resolvedSource(Paths.get("."), "org.apache.maven.test:parent:1.0.0")) + .build(); + Model out = interpolator.interpolateModel(model, Paths.get("."), request, collector); + assertProblemFree(collector); + + assertEquals("${env.HOME}", out.getProperties().get("envDir")); + } + + @Test + public void testCallerSuppliedModelInterpolationIsNotRestricted() throws Exception { + context.put("env.HOME", "/path/to/home"); + context.put("some.property", "other-value"); + + Map modelProperties = new HashMap<>(); + modelProperties.put("envDir", "${env.HOME}"); + modelProperties.put("propDir", "${some.property}"); + + Model model = Model.newBuilder().properties(modelProperties).build(); + + final SimpleProblemCollector collector = new SimpleProblemCollector(); + // A POM the caller hands to Maven as a file arrives with the same CONSUMER_DEPENDENCY + // request type as a dependency POM resolved from a repository, but its source is not + // one Maven resolved -- so the restricted property set above does not apply to it. + ModelBuilderRequest request = createModelBuildingRequest(context) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY) + .source(Sources.buildSource(Paths.get("."))) + .build(); + Model out = interpolator.interpolateModel(model, Paths.get("."), request, collector); + assertProblemFree(collector); + + assertEquals("/path/to/home", out.getProperties().get("envDir")); + assertEquals("other-value", out.getProperties().get("propDir")); + } + + @Test + public void testFullInterpolationOptOutRestoresPreviousBehaviorForResolvedDependencyModel() throws Exception { + context.put("env.HOME", "/path/to/home"); + context.put(Constants.MAVEN_MODEL_DEPENDENCY_INTERPOLATION_FULL, "true"); + + Map modelProperties = new HashMap<>(); + modelProperties.put("envDir", "${env.HOME}"); + + Model model = Model.newBuilder().properties(modelProperties).build(); + + final SimpleProblemCollector collector = new SimpleProblemCollector(); + ModelBuilderRequest request = createModelBuildingRequest(context) + .requestType(ModelBuilderRequest.RequestType.CONSUMER_DEPENDENCY) + .build(); + Model out = interpolator.interpolateModel(model, Paths.get("."), request, collector); + assertProblemFree(collector); + + assertEquals("/path/to/home", out.getProperties().get("envDir")); + } + @Test public void envarExpressionThatEvaluatesToNullReturnsTheLiteralString() throws Exception { diff --git a/impl/maven-impl/src/test/resources/poms/factory/import-system-scope-bom.xml b/impl/maven-impl/src/test/resources/poms/factory/import-system-scope-bom.xml new file mode 100644 index 000000000000..616bb010e51f --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/factory/import-system-scope-bom.xml @@ -0,0 +1,35 @@ + + + + org.apache.maven.tests + import-system-scope-bom + 1.0-SNAPSHOT + jar + + + + + org.apache.maven.its + system-scope-bom + 1.0 + pom + import + + + + diff --git a/impl/maven-impl/src/test/resources/poms/factory/resolved-model-with-profiles.xml b/impl/maven-impl/src/test/resources/poms/factory/resolved-model-with-profiles.xml new file mode 100644 index 000000000000..46a923eba6de --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/factory/resolved-model-with-profiles.xml @@ -0,0 +1,71 @@ + + + + org.apache.maven.tests + resolved-model-with-profiles + 1.0-SNAPSHOT + pom + + + file-condition + + + ${some.dir} + + + + activated + + + + property-condition + + + some.gating.property + + + + activated + + + + condition-condition + + ${some.condition.property} == 'true' + + + activated + + + + jdk-condition + + [1,) + + + activated + + + + profile-repo + https://repo.example.test/profile + + + + + diff --git a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenIT0085TransitiveSystemScopeTest.java b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenIT0085TransitiveSystemScopeTest.java index 62d9900f3299..71a8c9376a67 100644 --- a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenIT0085TransitiveSystemScopeTest.java +++ b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenIT0085TransitiveSystemScopeTest.java @@ -24,6 +24,7 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; public class MavenIT0085TransitiveSystemScopeTest extends AbstractMavenIntegrationTestCase { @@ -48,10 +49,40 @@ public void testit0085() throws Exception { verifier.addCliArgument("--settings"); verifier.addCliArgument("settings.xml"); verifier.addCliArgument("validate"); + verifier.addCliArgument("-Dmaven.model.dependencyInterpolation.full=true"); verifier.execute(); verifier.verifyErrorFreeLog(); Collection lines = verifier.loadLines("target/test.txt"); assertTrue(lines.contains("system.jar"), lines.toString()); } + + /** + * Verify that by default the path of a system-scope dependency declared by a POM resolved from a + * repository is not interpolated from the session properties, so the expression is left as + * written and reported as an invalid path. + * + * @throws Exception in case of failure + */ + @Test + public void testit0085DefaultLeavesPropertyUnresolved() throws Exception { + Path testDir = extractResources("it0085"); + + Verifier verifier = newVerifier(testDir); + verifier.setAutoclean(false); + verifier.deleteDirectory("target"); + verifier.deleteArtifacts("org.apache.maven.its.it0085"); + verifier.getSystemProperties().setProperty("test.home", testDir.toString()); + verifier.filterFile("settings-template.xml", "settings.xml"); + verifier.addCliArgument("--settings"); + verifier.addCliArgument("settings.xml"); + verifier.addCliArgument("validate"); + try { + verifier.execute(); + verifier.verifyErrorFreeLog(); + fail("Build should not succeed"); + } catch (VerificationException e) { + verifier.verifyTextInLog("must specify an absolute path but is ${test.home}/system.jar"); + } + } } diff --git a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng3586SystemScopePluginDependencyTest.java b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng3586SystemScopePluginDependencyTest.java index d1c94cb7eedd..d2f2a12af256 100644 --- a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng3586SystemScopePluginDependencyTest.java +++ b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng3586SystemScopePluginDependencyTest.java @@ -24,6 +24,7 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.fail; /** * This is a test set for MNG-3586. @@ -52,6 +53,7 @@ public void testitFromPlugin() throws Exception { verifier.addCliArgument("--settings"); verifier.addCliArgument("settings.xml"); verifier.addCliArgument("validate"); + verifier.addCliArgument("-Dmaven.model.dependencyInterpolation.full=true"); verifier.execute(); verifier.verifyErrorFreeLog(); @@ -80,4 +82,33 @@ public void testitFromProject() throws Exception { Properties props = verifier.loadProperties("target/pcl.properties"); assertEquals("1", props.getProperty("maven-core-it.properties.count")); } + + /** + * Test that by default a plugin POM resolved from a repository does not interpolate the path of + * its system-scope dependency from the session properties, so the expression is left as written + * and reported as an invalid path. + * + * @throws Exception in case of failure + */ + @Test + public void testitFromPluginDefaultLeavesPropertyUnresolved() throws Exception { + Path testDir = extractResources("mng-3586/test-1"); + + Verifier verifier = newVerifier(testDir); + verifier.setAutoclean(false); + verifier.deleteDirectory("target"); + verifier.deleteArtifacts("org.apache.maven.its.mng3586"); + verifier.getSystemProperties().setProperty("test.home", testDir.toString()); + verifier.filterFile("settings-template.xml", "settings.xml"); + verifier.addCliArgument("--settings"); + verifier.addCliArgument("settings.xml"); + verifier.addCliArgument("validate"); + try { + verifier.execute(); + verifier.verifyErrorFreeLog(); + fail("Build should not succeed"); + } catch (VerificationException e) { + verifier.verifyTextInLog("must specify an absolute path but is ${test.home}/tools.jar"); + } + } } diff --git a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4379TransitiveSystemPathInterpolatedWithEnvVarTest.java b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4379TransitiveSystemPathInterpolatedWithEnvVarTest.java index 7835cb31efb0..620a9c26a874 100644 --- a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4379TransitiveSystemPathInterpolatedWithEnvVarTest.java +++ b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4379TransitiveSystemPathInterpolatedWithEnvVarTest.java @@ -24,6 +24,7 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; /** * This is a test set for MNG-4379. @@ -51,10 +52,40 @@ public void testit() throws Exception { verifier.addCliArgument("-s"); verifier.addCliArgument("settings.xml"); verifier.addCliArguments("validate"); + verifier.addCliArgument("-Dmaven.model.dependencyInterpolation.full=true"); verifier.execute(); verifier.verifyErrorFreeLog(); List classpath = verifier.loadLines("target/classpath.txt"); assertTrue(classpath.contains("pom.xml"), classpath.toString()); } + + /** + * Test that by default the path of a system-scope dependency declared by a POM resolved from a + * repository is not interpolated using environment variables, so the expression is left as + * written and reported as an invalid path. + * + * @throws Exception in case of failure + */ + @Test + public void testitDefaultLeavesEnvironmentVariableUnresolved() throws Exception { + Path testDir = extractResources("mng-4379"); + + Verifier verifier = newVerifier(testDir); + verifier.setAutoclean(false); + verifier.deleteDirectory("target"); + verifier.deleteArtifacts("org.apache.maven.its.mng4379"); + verifier.filterFile("settings-template.xml", "settings.xml"); + verifier.setEnvironmentVariable("MNG_4379_HOME", testDir.toString()); + verifier.addCliArgument("-s"); + verifier.addCliArgument("settings.xml"); + verifier.addCliArguments("validate"); + try { + verifier.execute(); + verifier.verifyErrorFreeLog(); + fail("Build should not succeed"); + } catch (VerificationException e) { + verifier.verifyTextInLog("must specify an absolute path but is ${env.MNG_4379_HOME}/pom.xml"); + } + } } diff --git a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4590ImportedPomUsesSystemAndUserPropertiesTest.java b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4590ImportedPomUsesSystemAndUserPropertiesTest.java index bba1bf13875c..c22c0e16a329 100644 --- a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4590ImportedPomUsesSystemAndUserPropertiesTest.java +++ b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4590ImportedPomUsesSystemAndUserPropertiesTest.java @@ -51,6 +51,8 @@ public void testit() throws Exception { verifier.addCliArgument("--settings"); verifier.addCliArgument("settings.xml"); verifier.addCliArgument("validate"); + verifier.addCliArgument("-Dmaven.repository.dependencyManagement.allowSystemScope=true"); + verifier.addCliArgument("-Dmaven.model.dependencyInterpolation.full=true"); verifier.execute(); verifier.verifyErrorFreeLog(); @@ -61,4 +63,31 @@ public void testit() throws Exception { testDir.resolve("pom.xml"), Path.of(props.getProperty("project.dependencyManagement.dependencies.0.systemPath"))); } + + /** + * Verify that by default a POM imported from a repository contributes no managed dependency + * that declares {@code system} scope or a {@code systemPath}. + * + * @throws Exception in case of failure + */ + @Test + public void testitDefaultOmitsSystemScopedManagedDependency() throws Exception { + Path testDir = extractResources("mng-4590"); + + Verifier verifier = newVerifier(testDir); + verifier.setAutoclean(false); + verifier.deleteDirectory("target"); + verifier.deleteArtifacts("org.apache.maven.its.mng4590"); + verifier.filterFile("settings-template.xml", "settings.xml"); + verifier.setEnvironmentVariable("MAVEN_OPTS", "-Dtest.file=pom.xml"); + verifier.addCliArgument("-Dtest.dir=" + testDir.toString()); + verifier.addCliArgument("--settings"); + verifier.addCliArgument("settings.xml"); + verifier.addCliArgument("validate"); + verifier.execute(); + verifier.verifyErrorFreeLog(); + + Properties props = verifier.loadProperties("target/pom.properties"); + assertEquals("0", props.getProperty("project.dependencyManagement.dependencies")); + } }