You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/10/13 00:11:55 UTC
[PR] Fix #13390: stop POM properties from self-activating profiles in external builds [maven]
gnodet opened a new pull request, #13450:
URL: https://github.com/apache/maven/pull/13450
## Summary
- A dependency POM's own `<properties>` must not activate its property-based profiles
- The `externalActivationContext()` sandbox was merging model properties into the system-property lookup, causing POMs like jruby-base to self-activate profiles unintentionally (#13390)
- Remove the merge — only system properties and negated-property checks drive profile activation in external builds, restoring 3.9.x semantics
- This does **not** affect #13084 (RESTEasy), which uses a negated-property profile (`!resteasy.dependencies.eap`) that works correctly without the merge
### Background
The #13084 fix introduced a sandboxed `ProfileActivationContext` for external model builds. The sandbox correctly suppresses user `-D` flags and file activation, but it also merged POM `<properties>` into the system-property lookup as a new behavior that never existed in 3.9.x. This caused #13390: POMs that declare a property with the same name as a profile activation key (like jruby-base's `tzdata.version`) would self-activate that profile.
Analysis of the actual RESTEasy POM that motivated #13084 shows it uses a **negated-property** profile (`!resteasy.dependencies.eap`), not a positive POM-property activation. Negated-property profiles work correctly without the merge — the property is absent from the sandbox, so `!flag` evaluates to true.
### Changes
**Both stacks** (compat `maven-model-builder` and new `impl/maven-impl`):
- `DefaultModelBuilder` / `DefaultProfileActivationContext`: remove POM-property merge from external-model sandbox
- `PropertyProfileActivator`: update comments to reflect new semantics
- `ProfileActivationContext` interfaces: update Javadoc on model/project properties
**Tests**: added jruby-style (#13390 regression) and resteasy-style (#13084 non-regression) test cases
## Test plan
- [x] Compat `ExternalModelProfileActivationTest` — 12 tests pass
- [ ] Impl `DefaultModelBuilderTest` — has pre-existing `ApiRunner` compile failure on master (unrelated)
- [ ] Integration test with jruby-base-9.4.2.0 dependency (manual verification)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13390: stop POM properties from self-activating profiles in external builds [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #13450:
URL: https://github.com/apache/maven/pull/13450
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13390: stop POM properties from self-activating profiles in external builds [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #13450:
URL: https://github.com/apache/maven/pull/13450#issuecomment-6149178494
Thanks for the review!
**Finding 1 addressed**: `ConditionProfileActivator.doGetProperty()` calls `context.getModelProperty()` directly — the original sandbox only stopped model properties from leaking into `getSystemProperty()`, but `getModelProperty()` itself was still delegating to the real context. Fixed by overriding `getModelProperty()` to return `null` in `withoutUserPropertiesAndFilesystem()`, alongside `getUserProperty()`.
**Finding 2 addressed**: Updated the Javadoc bullet to accurately describe that model properties are suppressed via `getModelProperty()` returning `null`, not just through the system-property lookup.
**Finding 3 addressed**: Added a `pom-declared-property-via-condition` profile to the test fixture (`resolved-model-with-profiles.xml`) with `<condition>${pom.declared.prop} == 'hello'</condition>`, and added assertions:
- `testDependencyModelActivatesOnlyEnvironmentIndependentProfiles`: `assertNull(profile.pom.condition)` — sandbox suppresses it
- `testProjectBuildEvaluatesAllActivators`: `assertEquals("activated", profile.pom.condition)` — non-sandboxed context exposes POM properties
Same fix applied to #13452 (4.0.x, which also has `ConditionProfileActivator`). PR #13451 (3.10.x) has no impl stack so no change needed there — its `PropertyProfileActivator` already only reads from `getUserProperties()` + `getSystemProperties()`.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #13390: stop POM properties from self-activating profiles in external builds [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org