Repository navigation
fix: handle incomplete remote prefix filters - #13386
efegokdemir wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
gnodet-bot
left a comment
There was a problem hiding this comment.
This fix is mechanically correct — putIfAbsent after putAll(userProperties) correctly lets users override the default, and the overall structure is sound. However, there's a substantive concern about overriding a deliberate Resolver security default that should be discussed before merging.
Security default being overridden
The new line sets aether.remoteRepositoryFilter.prefixes.verifyDeniedDropsTree = true as Maven's default. According to the Resolver changelog, this property was introduced with a false default to preserve dependency-confusion protection: when a prefix filter denies a path, Resolver (with false) lets the request fall through to other repositories rather than silently dropping it. Setting true fixes MRESOLVER-559 (the reported problem) but removes that fall-through safety net.
This is a trade-off between availability (the fix) and dependency-confusion protection (the current Resolver default). It's a meaningful behavioral change that warrants explicit discussion with the Resolver maintainers before shipping as Maven's default.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| // 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); |
There was a problem hiding this comment.
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.
|
Thanks for flagging the security trade-off. I checked Resolver’s current documentation: |
gnodet-bot
left a comment
There was a problem hiding this comment.
Thanks for the fix — the mechanical part looks good: putIfAbsent is correctly placed after putAll(userProperties) so explicit user settings always win, and the tests cover both the default injection and the user-override case cleanly.
Two concerns before this is merged:
1. Security trade-off needs a second pair of eyes
This PR overrides Resolver's default for aether.remoteRepositoryFilter.prefixes.verifyDeniedDropsTree to true. The prefixes filter is Maven's primary defence against dependency-confusion attacks, so changing its default behaviour — even to improve availability — is a security-sensitive call that ideally needs a second maintainer to sign off. Is there a prior discussion or security@ thread that covers this trade-off?
2. Inline comment may be inaccurate
The description of false as "Resolver's fall-through behaviour" deserves a second look. If false means the filter continues to enforce denials (rather than falling through), the description is inverted and will mislead future readers. Worth cross-checking against Resolver's PrefixesRemoteRepositoryFilter implementation before merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
The latest commit corrects the comment to accurately describe both true and false behaviors. The code and tests are mechanically sound.
The one open question: this PR defaults verifyDeniedDropsTree to true, overriding Resolver's intentional false default (introduced in 2.0.22/2.0.23 specifically for dependency-confusion protection). true drops the entire auto-discovered prefix tree after a verified denial — restoring legacy availability behavior, but weakening the protection. Resolver chose false for good reason; Maven overriding it upward is a behavioral/compatibility change that warrants an explicit maintainer decision.
@gnodet @cstamas — does Maven intentionally relax the dependency-confusion guard here? The prior thread (PRRT_kwDOAAMmk86qelhG) is still the right place for that discussion. Happy to approve once either of you signs off on the default choice.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet
left a comment
There was a problem hiding this comment.
Thanks for the fix and the clear write-up! The approach is correct in spirit — we do need a way to handle the case where a virtual repository's auto-discovered prefix file is incomplete — but I think the fix belongs one layer lower, in maven-resolver itself rather than in Maven.
Here's the reasoning:
The rejection message is the root problem. Right now, when PrefixesRemoteRepositoryFilterSource rejects a path, the reasoning() string passed into ArtifactFilteredOutException is just:
Path /com/example/internal/1.0/internal-1.0.jar NOT allowed from central
No hint of what caused it, no workaround. That's what users actually see in the error output, and it's what sends them on a multi-hour debugging session.
Resolver already has all the context it needs at that point:
- The
RemoteRepository(→ repo ID for the per-repo property suffix) - Whether the prefix tree was auto-discovered vs. user-provided (user-provided files are deliberate and shouldn't suggest overrides)
- The exact config property name constants (
CONFIG_PROP_VERIFY_DENIED_DROPS_TREE) including the per-repo-ID suffix convention already supported byConfigUtils.getBoolean()
So the fix would be: in acceptPrefix(), when the path is rejected and the tree is auto-discovered, enrich the reasoning string to say something like:
Path /com/example/internal/1.0/internal-1.0.jar NOT allowed from central
(auto-discovered prefix file appears incomplete). If this repository serves a
virtual aggregate, set -Daether.remoteRepositoryFilter.prefixes.verifyDeniedDropsTree.central=true
or report this to the repository administrator.
See https://maven.apache.org/resolver/remote-repository-filtering.html
This is self-contained, targeted to the specific repo ID, and requires no changes in Maven at all — the enriched message propagates naturally through filterResult.reasoning() into the exception Maven surfaces.
On the verifyDeniedDropsTree=true global default: this is a meaningful security trade-off. Resolver's deliberate false default preserves dependency-confusion protection for all repositories — setting true globally drops the prefix tree for any repo that serves an inconsistently-prefixed path, not just broken Artifactory/Nexus virtuals. The per-repo suffix (...verifyDeniedDropsTree.central=true) is the right granularity here, and it's already supported by Resolver.
I'd suggest closing this PR and raising one on apache/maven-resolver to enrich the rejection message along these lines. That would give users an actionable error message for this common misconfiguration without changing any resolution behaviour in Maven.
|
Note: there is already a PR open on maven-resolver that addresses the underlying behavior: apache/maven-resolver#2188. It fixes per-path verification caching and transient failure handling in |
|
Following your guidance, I opened apache/maven-resolver#2194 for the Resolver-side diagnostic. It keeps filtering behaviour unchanged and adds the repository-specific workaround only for denied paths from auto-discovered prefixes. Closing this Maven PR. |
What changed
Maven now enables Resolver's
aether.remoteRepositoryFilter.prefixes.verifyDeniedDropsTreebehaviour by default. When Resolver verifies that a path denied by an auto-discovered prefix file exists, it can discard that incomplete prefix tree and continue resolving the artifact. Explicit user settings still override the default.Why
A virtual repository can expose a
.meta/prefixes.txtfile that does not include artifacts available through its other members. Maven 3.10.0 then rejects those artifacts even after confirming that the denied POM exists.Testing
expected true, got null) and passes after it; a second test verifies an explicitfalseoverride is preserved.mvn -pl maven-core -am verifypassed (Maven Core: 404 tests, 1 skipped; all 11 reactor modules successful).Checklist
mvn -pl maven-core -am verify.Fixes #13385