Fix #13100: honor repositories from legitimately-active external model profiles

[gnodet]

Problem

After 03c947d8 (and its successor #13114 which sandboxes the activation context), profile repositories in dependency POMs are always stripped from active profiles, even when the profile activated legitimately via JDK version, OS, activeByDefault, or a POM-declared property.

This breaks the established Maven pattern:

project → dep1 → dep2

where dep1 knows dep2 is not on Central and declares its repository inside a JDK- or activeByDefault-activated profile.

Discussed in #13100 (comment by @cstamas).

Root cause

The withoutRepositories() stripping (compat stack) and the .map(profile → profile.withRepositories(…)) stripping (impl stack) are applied to all active profiles unconditionally, regardless of why they activated. But the sandbox activation context in #13114 already guarantees that only legitimate profiles survive to injection:

Activation kind	Activates in sandbox?	Repos contributed?
JDK / OS / activeByDefault	✅ Yes	✅ Honored (this PR)
POM-declared <property>	✅ Yes	✅ Honored (this PR)
Consumer -D flag	❌ Suppressed by sandbox	❌ Never reaches injection
File condition	❌ Pre-filtered	❌ Never reaches injection

The stripping was therefore over-broad and broke legitimate use-cases with no security benefit (suppression already happens earlier in the sandbox).

Solution

Remove the repository stripping from both stacks for the externalOrigin / VALIDATION_LEVEL_MINIMAL branch:

• compat (maven-model-builder): drop the withoutRepositories() call and the dead helper method from DefaultModelBuilder
• impl (maven-impl): drop the .map() stripping from the externalOrigin branch of getActiveProfiles()

The BUILD_CONSUMER stripping (line 1755 in impl) is intentional and unchanged — consumer POMs must not leak profile repositories into the published artifact.

Notes

• Depends on / stacks on top of [3.10.x] Fix #13084: sandbox profile activation context for external model builds #13114 (the sandbox activation context). The sandbox is what makes this safe.
• The redefinition concern raised by @cstamas (a dependency shadowing Central with a different URL) is orthogonal and should be addressed separately as a WARN/FAIL policy — not via blanket stripping.

Changes

• DefaultModelBuilder (compat): remove withoutRepositories() call and helper
• DefaultModelBuilder (impl): remove .map() stripping from externalOrigin branch; update comment
• ExternalModelProfileActivationTest: invert and rename the last test to assert the corrected behavior

[gnodet-bot]

Review: Fix #13100 — honor repositories from legitimately-active external model profiles

The sandbox approach in commit 4c8b837 is correct. Suppressing user properties and file checks via withoutUserPropertiesAndFilesystem() while preserving system and model properties is the right model. The PropertyProfileActivator fallthrough to getModelProperty() in both stacks correctly handles POM-declared-property activation. The test matrix in ExternalModelProfileActivationTest is thorough and accurate.

Commit 65e4821 (the actual #13100 fix) is logically sound — once the sandbox guarantees only legitimately-activated profiles survive, removing the blanket stripping is correct. However, this commit failed to update four noneMatch assertions in DefaultModelBuilderTest (impl stack) that assert the old behavior (repos always stripped from external dependency builds). Those four assertions will now fail.

Stale assertions — will fail

DefaultModelBuilderTest has the following noneMatch assertions for externalOrigin=true models:

• Line 188 — testDependencyModelActivatesOnlyEnvironmentIndependentProfiles: asserts noneMatch for a CONSUMER_DEPENDENCY resolved-source model. With repo stripping removed, the JDK-activated profile's profile-repo will now appear → assertion fails.
• Line 301 — testResolvedDependencyParentCacheDoesNotShareActivationWithProjectParent: same issue for dependencyParentModel.
• Line 320 — same test, second pass (dependencyParentModel2).
• Line 381 — testExternalOriginPropagatesThroughGrandparentHop: same issue for the grandparent model built under externalOrigin=true.

Note: the noneMatch at line 420 (testBuildConsumerActivatesOnlyDeterministicProfiles) is correct — BUILD_CONSUMER uses a separate branch that still strips repos, unchanged by this PR.

Fix: change lines 188, 301, 320, 381 from noneMatch to anyMatch (mirroring what ExternalModelProfileActivationTest now asserts).

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

[gnodet-bot]

Re-review: Fix #13100 — honor repositories from legitimately-active external model profiles

The new commit (80f709526b) still does not update ResolvedDependencyProfileActivationTest, and CI is failing on all three platforms with exactly the assertion predicted in the previous review.

Failing test (all 3 platforms)

org.apache.maven.model.building.ResolvedDependencyProfileActivationTest
  .testDependencyPomActivatesOnlyEnvironmentIndependentProfiles -- FAILURE
  ResolvedDependencyProfileActivationTest.java:121 expected: <true> but was: <false>

Root cause

ResolvedDependencyProfileActivationTest.java:121 still asserts the old behaviour:

trueAssertTrue(model.getRepositories().stream().noneMatch(r -> "profile-repo".equals(r.getId())));

With withoutRepositories() removed, the JDK-activated profile's profile-repo does survive to the built model — exactly the intent of this PR. The assertion must be inverted:

trueAssertTrue(model.getRepositories().stream().anyMatch(r -> "profile-repo".equals(r.getId())));

Also: stale class-level Javadoc (line 34)

The class comment says:

their profiles contribute no repositories

That was the pre-fix description. After this change, platform-activated profiles do contribute repositories for VALIDATION_LEVEL_MINIMAL builds; only file- and property-activated ones are suppressed. The Javadoc should be updated to reflect the corrected semantic, for example:

their file- and property-activated profiles are not evaluated; platform-activated profiles (JDK, OS, activeByDefault) still fire and may contribute repositories.

Logical correctness of the production change

The production code change itself remains sound — once #13114's sandbox guarantees that only legitimately-activated profiles reach injection, removing the blanket stripping is correct and no security regression is introduced. The only blocker is the two-line test / Javadoc update.

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

[gnodet-bot]

Re-review: Fix #13100 — honor repositories from legitimately-active external model profiles

Main blocker: resolved ✅

The failing test is fixed: testDependencyPomActivatesOnlyEnvironmentIndependentProfiles is now renamed testDependencyPomHonorsJdkActivatedProfileRepositories and the assertion correctly inverted to anyMatch. This is exactly what was flagged in the previous review.

The production change remains sound — withoutFileAndPropertyActivation() still gates profile selection, so only legitimately sandbox-compatible profiles reach injection; removing the blanket withoutRepositories() stripping from injection is correct.

Remaining nit: stale class-level Javadoc

The class-level Javadoc on ResolvedDependencyProfileActivationTest (lines 34–41) still says:

their profiles contribute no repositories

That was the pre-fix invariant. After this change, the true invariant is narrower: file- and property-activated profiles are suppressed, but platform-activated ones (JDK, OS, activeByDefault) fire legitimately and may contribute repositories. The Javadoc should be updated, for example:

 * Models built at {@link ModelBuildingRequest#VALIDATION_LEVEL_MINIMAL} come from POMs resolved
 * from a repository during dependency resolution (a dependency POM, one of its parents, or an
 * imported BOM), see for instance {@code DefaultArtifactDescriptorReader#loadPom}. Their file and
 * property activators are not evaluated. Platform-activated profiles (JDK version, OS,
 * {@code activeByDefault}) still fire and may contribute repositories; only file- and
 * property-activated profiles are suppressed. A project build, at
 * {@link ModelBuildingRequest#VALIDATION_LEVEL_STRICT}, still evaluates every activator.

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

[gnodet]

Fixed in 8ca2654: updated the class-level Javadoc on ResolvedDependencyProfileActivationTest to reflect the new invariant (platform-activated profiles may contribute repositories; only file- and property-activated ones are suppressed).

[gnodet-bot]

Re-review: Fix #13100 — honor repositories from legitimately-active external model profiles

All previous findings addressed ✅

• Stale noneMatch assertions in DefaultModelBuilderTest (lines 188, 301, 320, 381) — fixed in 9ff6500f by renaming the test and inverting to anyMatch. ✅
• Stale class-level Javadoc on ResolvedDependencyProfileActivationTest — fixed in 8ca2654b. The updated Javadoc correctly describes the narrowed invariant: file- and property-activated profiles are suppressed; platform-activated profiles (JDK, OS, activeByDefault) still fire and may contribute repositories. Proper {@code} and {@link} markup used throughout. ✅

Production change

Removing withoutRepositories(activeProfile) from the injection loop is correct. The sandbox (withoutFileAndPropertyActivation) already gates which profiles reach injection for external models — blanket stripping after the fact was redundant and over-broad. The helper method deletion is clean dead-code removal.

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

[gnodet-bot]

Re-review: Fix #13100 — honor repositories from legitimately-active external model profiles

All previous findings addressed ✅

New commit b864ff09 reconciles this 3.10.x PR with the parallel master-stack PR (#13141). Two additions:

1. TODO(#13146) comment at the profile injection site in DefaultModelBuilder — correctly references the open issue tracking the deferred central-shadowing WARN/FAIL policy. ✅

2. testActiveByDefaultProfileRepositoryHonored() test — the isolated inline POM approach is correct. Without isolation, the JDK-activated sibling in the shared POM fixture would cancel activeByDefault (standard Maven rule), making the assertion vacuous. StringModelSource exists in maven-model-builder/src/main/java — the import is valid on the 3.10.x stack. No model resolver is needed since the inline POM declares no parent. ✅

The production change (removing withoutRepositories() stripping) remains sound — the sandbox already ensures only legitimately-activated profiles reach injection for external models.

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

[github-actions]

@gnodet Please assign appropriate label to PR according to the type of change.