gnodet-bot commented on code in PR #13141:
URL: https://github.com/apache/maven/pull/13141#discussion_r4015832760
##########
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
Review Comment:
⚠️ **Misleading comment — #13112 is not yet merged**
Line 1726 says *"the sandbox activation context (see #13112) **already**
suppresses consumer -D flags and file conditions"* — but #13112 is still open.
When this PR stands alone (which it explicitly claims to do: *"Stacks cleanly
on top of or independently of #13112"*), the
`hasFileOrPropertyOrConditionActivation` filter on line 1732 is what provides
the suppression, not the sandbox.
The word "already" falsely implies the sandbox is in effect. A reader
relying on that comment to understand the safety argument will be misled.
Fix the comment to attribute the guarantee to the actual mechanism:
```suggestion
// activeByDefault). Repository stripping is
intentionally omitted here:
// file and property activation is already excluded by
the
// hasFileOrPropertyOrConditionActivation filter below,
so only
// legitimately-active profiles reach injection.
// Once #13112 (sandbox activation context) is merged,
the sandbox
// will additionally gate -D flags before this point.
```
##########
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.
+ assertTrue(model.getRepositories().stream().anyMatch(r ->
"profile-repo".equals(r.getId())));
Review Comment:
⚠️ **`activeByDefault` repository path is untested**
Building on the gap already noted by @utafrali: the fixture
`resolved-model-with-profiles` (via `resolvedProfilesRequest`) uses a
JDK-activated profile to carry `profile-repo`. The comment added here says
*"Repositories from legitimately-active profiles (JDK-activated) must be
honored"* — but the PR description and the impl comment both claim
`activeByDefault` profiles are also honored.
The `activeByDefault` path is exercised only by prose, not by any assertion.
An `activeByDefault` profile whose repository gets stripped would silently pass
all existing tests. Add a dedicated test method (or extend the existing fixture
with an `activeByDefault` profile) asserting the repository is retained.
##########
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))
- .map(profile ->
profile.withRepositories(List.of()).withPluginRepositories(List.of()))
.toList();
}
return profileSelector.getActiveProfiles(eligibleProfiles,
profileActivationContext, this);
Review Comment:
🔒 **Deferred security concern deserves an in-code marker**
With the strip removed, a transitive dependency's POM can now contribute a
repository through a JDK/OS/`activeByDefault` profile with no URL validation.
The PR description correctly defers the central-shadowing concern to #428, but
nothing in the code tells a future reader this is a known,
deliberately-deferred hardening item rather than an oversight.
Add a TODO next to the `return profileSelector.getActiveProfiles(...)` line:
```suggestion
return profileSelector.getActiveProfiles(eligibleProfiles,
profileActivationContext, this);
// TODO(#428): repositories contributed by external-model
profiles can shadow
// central; a WARN/FAIL policy for URL mismatches should be
added separately.
```
⚠️ This suggestion block spans lines that a single-line replacement can't
cover cleanly — add the TODO as a comment on the line immediately after `return
profileSelector.getActiveProfiles(..., this);`.
--
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]
