gnodet-bot commented on code in PR #13386:
URL: https://github.com/apache/maven/pull/13386#discussion_r4222128833
##########
maven-core/src/main/java/org/apache/maven/internal/aether/DefaultRepositorySystemSessionFactory.java:
##########
@@ -238,6 +238,10 @@ public RepositorySystemSession.SessionBuilder
newRepositorySessionBuilder(MavenE
// Resolver's ConfigUtils solely rely on config properties, that is
why we need to add both here as well.
configProps.putAll(request.getSystemProperties());
configProps.putAll(request.getUserProperties());
+ // Auto-discovered prefix files from virtual repositories may describe
only one member repository.
+ // If verification proves such a file incomplete, favour resolving
available artifacts by dropping it.
+ // Explicit user-provided prefix files remain authoritative in
Resolver, and users can override this default.
+
configProps.putIfAbsent("aether.remoteRepositoryFilter.prefixes.verifyDeniedDropsTree",
Boolean.TRUE);
Review Comment:
⚠️ **Security default override:** This sets `verifyDeniedDropsTree=true` as
Maven's default, overriding Resolver's deliberate `false` default. Resolver
chose `false` to preserve dependency-confusion protection — when a prefix file
denies a path, the request falls through to other repositories rather than
being silently dropped. Setting `true` fixes MRESOLVER-559 but removes that
fall-through safety net.
This trade-off (availability vs. dependency-confusion protection) is worth
explicit alignment with the Resolver maintainers before shipping as Maven's
hardcoded default.
##########
maven-core/src/main/java/org/apache/maven/internal/aether/DefaultRepositorySystemSessionFactory.java:
##########
@@ -238,6 +238,10 @@ public RepositorySystemSession.SessionBuilder
newRepositorySessionBuilder(MavenE
// Resolver's ConfigUtils solely rely on config properties, that is
why we need to add both here as well.
configProps.putAll(request.getSystemProperties());
configProps.putAll(request.getUserProperties());
+ // Auto-discovered prefix files from virtual repositories may describe
only one member repository.
+ // If verification proves such a file incomplete, favour resolving
available artifacts by dropping it.
+ // Explicit user-provided prefix files remain authoritative in
Resolver, and users can override this default.
Review Comment:
💬 **Misleading comment:** This line describes Resolver's prefix-file
behaviour (that user-provided prefix files stay authoritative), not *why*
`putIfAbsent` is safe or what trade-off this default introduces. Consider
replacing it with something that explains the drops-tree vs. fall-through
trade-off, e.g.: `// Default true: drops verified-denied tree entries (fixes
MRESOLVER-559); users may set false to restore fall-through behaviour.`
##########
maven-core/src/test/java/org/apache/maven/internal/aether/DefaultRepositorySystemSessionFactoryTest.java:
##########
@@ -116,6 +120,35 @@ void malformedServerRepositoryOriginIsIgnored() throws
Exception {
assertNull(selector.getAuthentication(repository("internal",
"https://repo.example.org/releases/";)));
}
+ @Test
+ void
dropsAnAutoDiscoveredPrefixesFileWhenItDeniesAnExistingPathByDefault() throws
Exception {
Review Comment:
💬 **Test name describes runtime Resolver behaviour, not what the test
verifies.**
`dropsAnAutoDiscoveredPrefixesFileWhenItDeniesAnExistingPathByDefault`
describes what the Resolver does at runtime with this config, but the test only
asserts that
`getConfigProperties().get("aether.remoteRepositoryFilter.prefixes.verifyDeniedDropsTree")`
equals `Boolean.TRUE`. Consider a name like
`configuresVerifyDeniedDropsTreeTrueByDefault` that directly describes what the
test checks.
--
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]
