gnodet-bot commented on code in PR #13450:
URL: https://github.com/apache/maven/pull/13450#discussion_r4253537160
##########
impl/maven-impl/src/test/resources/poms/factory/resolved-model-with-profiles.xml:
##########
@@ -64,6 +67,18 @@
<profile.missing.condition>activated</profile.missing.condition>
</properties>
</profile>
+ <!-- POM-declared property activation: suppressed in external builds
(see #13390) -->
+ <profile>
+ <id>pom-declared-property-condition</id>
Review Comment:
đź’¬ **Consider adding a parallel `<condition>` profile to catch the
ConditionProfileActivator path.**
This new `<property>`-activated profile tests the `PropertyProfileActivator`
path correctly. But there is no equivalent test for
`ConditionProfileActivator`: the existing `condition-condition` profile uses
`${some.condition.property}` which is NOT declared in `<properties>`, so it
never fires and does not verify the sandbox blocks the condition path.
A profile like:
```xml
<profile>
<id>pom-declared-condition-condition</id>
<activation>
<condition>${pom.declared.prop} == 'hello'</condition>
</activation>
<properties><profile.pom.condition>activated</profile.pom.condition></properties>
</profile>
```
paired with `assertNull(model.getProperties().get("profile.pom.condition"))`
in the external build test would expose the gap.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileActivationContext.java:
##########
@@ -389,18 +388,9 @@ public boolean isProfileInactive(String profileId) {
return
DefaultProfileActivationContext.this.isProfileInactive(profileId);
}
- /**
- * System property lookup with model property fallback.
- * System properties take precedence; model properties fill in as
a fallback
- * so POM-declared values drive property-activated profiles in
external models.
- */
@Override
public String getSystemProperty(String key) {
- String value =
DefaultProfileActivationContext.this.getSystemProperty(key);
- if (value == null) {
- value =
DefaultProfileActivationContext.this.getModelProperty(key);
- }
- return value;
+ return
DefaultProfileActivationContext.this.getSystemProperty(key);
Review Comment:
⚠️ **`getSystemProperty()` is fixed here, but `getModelProperty()` still
delegates (line 403–404).**
`ConditionProfileActivator.doGetProperty()` calls
`context.getModelProperty(name)` directly (line 189 of
`ConditionProfileActivator.java`), bypassing this sandbox entirely. A
dependency POM with a `<condition>` profile referencing a POM-declared property
(e.g. `<condition>${tzdata.version} == '2024a'</condition>` where
`tzdata.version` is declared in `<properties>`) will still self-activate after
this fix.
To close the gap, suppress `getModelProperty()` in the sandbox too:
```java
@Override
public String getModelProperty(String key) {
return null;
}
```
(If model properties must remain accessible for some interpolation use-case,
the condition activator itself needs a fix instead.)
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultProfileActivationContext.java:
##########
@@ -360,14 +360,13 @@ public DefaultProfileActivationContext setModel(Model
model) {
* <p>
* The sandboxed context:
* <ul>
- * <li><b>Merges model properties into system property lookups</b>:
system properties
- * ({@code java.version}, {@code os.name}, …) are checked first
(preserving JDK/OS
- * activation), then model properties (the POM's own {@code
<properties>}) are used
- * as a fallback. This makes POM-declared properties visible to the
- * {@code PropertyProfileActivator} without modifying the
activator's lookup chain
- * for non-external models.</li>
+ * <li><b>Preserves system properties</b> ({@code java.version}, {@code
os.name}, …)
+ * so JDK/OS activation works.</li>
* <li><b>Suppresses user properties</b> (consumer {@code -D} flags):
they were not
* set for the dependency and must not accidentally activate its
profiles.</li>
+ * <li><b>Does not expose model properties through system property
lookups</b>: a
+ * dependency POM's own {@code <properties>} must not activate its
own
+ * property-based profiles (see #13390).</li>
Review Comment:
đź’¬ **Javadoc scope is narrower than the actual exposure.**
This bullet says model properties are not exposed “through system property
lookups” — which is accurate for `getSystemProperty()`. But
`ConditionProfileActivator` calls `getModelProperty()` directly, not through
the system-property chain. If `getModelProperty()` is fixed (see inline comment
on `getSystemProperty`), this Javadoc becomes fully accurate. As-is it should
note that model properties remain accessible via the `getModelProperty()` API.
--
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]
