Skip to content

Fix #13084: sandbox profile activation context for external model builds - #13158

Merged
gnodet merged 2 commits into
apache:maven-4.0.xfrom
gnodet:backport/13084-sandbox-external-profile-activation-to-4.0.x
Sep 17, 2026
Merged

gnodet merged 2 commits into
apache:maven-4.0.xfrom
gnodet:backport/13084-sandbox-external-profile-activation-to-4.0.x

Conversation

@gnodet

@gnodet gnodet commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Backport of #13112 (master) and #13114 (maven-3.10.x) to maven-4.0.x.

Instead of blanket-filtering file and property profiles in external model builds (dependency POMs, parent POMs, imported BOMs), evaluate them against a sandboxed ProfileActivationContext that:

  • Merges model <properties> into system property lookups — POM-declared properties drive activation in dependency builds.
  • Suppresses user properties — consumer -D flags must not activate dependency profiles.
  • Pre-filters file-activated profiles (returning false from exists() would incorrectly activate <missing> profiles).
  • Strips repository contributions from active external profiles.

Both compat and impl stacks updated identically. Tests cover all activation paths including the system-property opt-out of default-on profiles.

@gnodet gnodet added the bug Something isn't working label Sep 16, 2026
@gnodet gnodet added this to the 4.0.0-rc-7 milestone Sep 16, 2026
…el builds

Backport of apache#13112 (master) and apache#13114 (maven-3.10.x) to maven-4.0.x.

Instead of blanket-filtering file and property profiles in external model
builds (dependency POMs, parent POMs, imported BOMs), evaluate them against
a sandboxed ProfileActivationContext that:

 - Merges model properties into system property lookups (so POM-declared
   <properties> drive property-activated profiles in dependency builds).
 - Suppresses user properties (consumer -D flags must not activate
   dependency profiles — they were not set for that artifact).
 - Pre-filters file-activated profiles (returning false from exists() would
   incorrectly activate <missing> profiles).
 - Strips repository contributions from all active external profiles.

Both compat and impl stacks are updated identically. The key insight is that
model properties are merged into system properties inside the sandbox rather
than changing PropertyProfileActivator's lookup chain, to avoid unintended
activation when a project POM declares a property that coincidentally matches
a profile's activation condition.

Tests: ExternalModelProfileActivationTest (compat) and DefaultModelBuilderTest
(impl) cover file suppression, user-property suppression, system-property
preservation, POM-declared-property activation, negated-property default-on
preservation, and system-property opt-out of the default-on pattern.
@gnodet
gnodet force-pushed the backport/13084-sandbox-external-profile-activation-to-4.0.x branch from b76b537 to be80893 Compare September 16, 2026 19:13

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two findings on the sandbox implementation.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues in the sandbox implementation worth fixing before merge.


Finding 1 — Mutable map leak in compat externalActivationContext (inline below)

See inline comment on compat/maven-model-builder/.../DefaultModelBuilder.java line 550.


Finding 2 — Condition profiles using missing(path) incorrectly activate in the external-model sandbox

The pre-filter in impl/maven-impl/.../DefaultModelBuilder.java (around the nonFileProfiles stream) only drops profiles whose activation.file != null. Condition profiles whose expression calls missing(…) pass through unchanged. Inside the sandbox context.exists() always returns false, so missing(path) evaluates to !false = true — the profile fires regardless of whether the publisher's path actually exists on the consumer machine. This is the same <missing>-flips-to-active footgun the pre-filter was introduced to prevent for <activation><file>, now re-exposed for condition expressions.

Before this PR, hasFileOrPropertyOrConditionActivation excluded all condition profiles from external models (the same helper is still used today in the BUILD_CONSUMER path). That protection was removed for CONSUMER_DEPENDENCY when the sandbox replaced the hard filter. No test covers a condition profile with exists()/missing() against a CONSUMER_DEPENDENCY build.

Suggested fix — extend the pre-filter:

private static boolean hasFileConditionExpression(Profile p) {
    Activation a = p.getActivation();
    if (a == null || a.getCondition() == null) return false;
    String c = a.getCondition();
    return c.contains("exists(") || c.contains("missing(");
}

Then change the filter from:

.filter(p -> p.getActivation() == null
        || p.getActivation().getFile() == null)

to:

.filter(p -> p.getActivation() == null
        || (p.getActivation().getFile() == null
                && !hasFileConditionExpression(p)))

A test asserting that a missing('/publisher/private/file') condition profile does not activate in a CONSUMER_DEPENDENCY build is required.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

…ilter missing()-condition profiles in CONSUMER_DEPENDENCY builds

# Conflicts:
#	impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review after f321eab98f. Both prior findings addressed:

Finding 1 (mutable map leak in compat sandbox) — Fixed. The empty-projectProps branch now wraps delegate.getSystemProperties() with Collections.unmodifiableMap() (compat DefaultModelBuilder.java line 548). The non-empty branch was already defensive; both branches are now consistent.

Finding 2 (missing()-condition profiles activate unconditionally in sandbox) — Fixed. hasFileConditionExpression() helper added in impl DefaultModelBuilder, pre-filters condition profiles whose expression contains exists( or missing( before they reach the sandbox. The test in resolved-model-with-profiles.xml + DefaultModelBuilderTest.testDependencyModelActivatesOnlyEnvironmentIndependentProfiles confirms suppression in CONSUMER_DEPENDENCY and that the profile still fires in a project build (where the path genuinely doesn't exist).

I verified that the compat stack's withoutFileActivation not filtering condition profiles is a non-issue: maven-model-builder has no ConditionProfileActivator — condition activation is impl-only. The hasFileConditionExpression string heuristic (contains("exists(") / contains("missing(")) can only produce false positives (conservative: drop a legitimate profile), never false negatives. The design choice that POM-declared property profiles activate in external builds but not project builds is explicitly asserted and documented.

Clear to merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

@gnodet
gnodet merged commit ee0ed81 into apache:maven-4.0.x Sep 17, 2026
22 checks passed
@gnodet
gnodet deleted the backport/13084-sandbox-external-profile-activation-to-4.0.x branch September 17, 2026 15:59
gnodet added a commit that referenced this pull request Sep 17, 2026
… pre-filter missing()-condition profiles in CONSUMER_DEPENDENCY builds (#13164)

Forward-port of findings from the review of PR #13158 (4.0.x backport):

- compat: externalActivationContext() returned mutable map from getSystemProperties()
  on both the empty-projectProps branch (raw delegate.getSystemProperties()) and the
  non-empty branch (unwrapped HashMap). Wrap both with Collections.unmodifiableMap().

- impl: the nonFileProfiles pre-filter only excluded profiles with file activation.
  A condition profile using missing(path) passes the filter and enters the sandbox
  where context.exists() always returns false, so missing() = !false = true fires
  unconditionally — the same footgun as <file><missing>. Add hasFileConditionExpression()
  helper and extend the filter to also exclude condition profiles containing exists()/missing().

Tests: extended testProjectBuildEvaluatesAllActivators (missing-condition fires in
project build) and testDependencyModelActivatesOnlyEnvironmentIndependentProfiles
(missing-condition suppressed in CONSUMER_DEPENDENCY).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants