utafrali commented on code in PR #13141:
URL: https://github.com/apache/maven/pull/13141#discussion_r4015809616
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -1722,10 +1722,14 @@ private List<Profile> getActiveProfiles(
// 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.
+ // activeByDefault). Repository stripping is intentionally
omitted here:
+ // the sandbox activation context (see #13112) already
suppresses consumer
+ // -D flags and file conditions, so only
legitimately-active profiles reach
+ // injection. Stripping their repositories would break the
established
+ // project → dep1 → dep2 pattern where dep1 declares
dep2's repository
+ // inside an activeByDefault or JDK-activated profile.
eligibleProfiles = interpolatedProfiles.stream()
.filter(profile ->
!hasFileOrPropertyOrConditionActivation(profile))
Review Comment:
The updated comment states that only "legitimately-active profiles reach
injection" thanks to the sandbox activation context, but the very next line
still filters via `!hasFileOrPropertyOrConditionActivation(profile)`. That
belt-and-suspenders is fine, but the PR description lists "POM-declared
`<properties>`" as a legitimately-active source, and such profiles are still
excluded here because they use `<property>` activation. Consider clarifying the
comment so readers do not conclude that POM property activation is honored for
external models when in fact only JDK/OS/activeByDefault activation reaches
injection.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java:
##########
@@ -180,7 +180,9 @@ public void
testDependencyModelActivatesOnlyEnvironmentIndependentProfiles() {
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())));
+ // Repositories from legitimately-active profiles (JDK-activated) must
be honored:
+ // stripping them would break the project → dep1 → dep2 pattern. See
#13100, #13116.
Review Comment:
The three flipped assertions all cover a JDK-activated profile. Since the PR
description explicitly claims `activeByDefault` profile repositories are also
honored, it would be worth adding a small case (or extending an existing
fixture) that exercises an `activeByDefault=true` profile and asserts its
repository is retained. Right now that path is only covered by prose in the
comment, not by a test.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java:
##########
@@ -1722,10 +1722,14 @@ private List<Profile> getActiveProfiles(
// 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.
+ // activeByDefault). Repository stripping is intentionally
omitted here:
+ // the sandbox activation context (see #13112) already
suppresses consumer
+ // -D flags and file conditions, so only
legitimately-active profiles reach
+ // injection. Stripping their repositories would break the
established
+ // project → dep1 → dep2 pattern where dep1 declares
dep2's repository
+ // inside an activeByDefault or JDK-activated profile.
eligibleProfiles = interpolatedProfiles.stream()
Review Comment:
The security implication of this change deserves a pointer in-code. Removing
the strip means a transitive dependency's POM can now contribute a repository
via a JDK/OS/activeByDefault profile with no filtering at all. The description
defers the redefinition/shadowing concern to #428, but adding a `// TODO(#428):
shadowing central via profile repos should warn/fail` next to this comment
would help future readers understand this is a known and deliberately-deferred
hardening item, not an oversight.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
