Skip to content

fix: handle incomplete remote prefix filters - #13386

Closed
efegokdemir wants to merge 3 commits into
apache:maven-3.10.xfrom
efegokdemir:fix/maven-prefix-filter-13385
Closed

efegokdemir wants to merge 3 commits into
apache:maven-3.10.xfrom
efegokdemir:fix/maven-prefix-filter-13385

Conversation

@efegokdemir

@efegokdemir efegokdemir commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

What changed

Maven now enables Resolver's aether.remoteRepositoryFilter.prefixes.verifyDeniedDropsTree behaviour 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.txt file 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

  • Reproduced with a local HTTP repository: Maven 3.10.0 fails without the property and succeeds when it is enabled.
  • The new default-setting test failed before the implementation (expected true, got null) and passes after it; a second test verifies an explicit false override is preserved.
  • mvn -pl maven-core -am verify passed (Maven Core: 404 tests, 1 skipped; all 11 reactor modules successful).
  • Core IT was not run.

Checklist

  • This pull request addresses one issue.
  • The description explains what changed, how, and why.
  • The commit has a meaningful subject line.
  • Unit tests cover the behaviour change.
  • Ran the targeted verify command: mvn -pl maven-core -am verify.
  • Ran the Core IT.
  • I hereby declare this contribution to be licensed under the Apache License Version 2.0, January 2004

Fixes #13385

@github-actions

This comment has been minimized.

@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.

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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ 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.

@efegokdemir

Copy link
Copy Markdown
Contributor Author

Thanks for flagging the security trade-off. I checked Resolver’s current documentation: verifyDeniedDropsTree=true restores the legacy behaviour of dropping the entire auto-discovered prefixes file for the session, so it is broader than a targeted repair. On Maven issue #13385, @cstamas also noted that repository managers should fix their products. I’m not comfortable making that Resolver policy Maven’s default without explicit alignment. @gnodet @cstamas, should Maven keep Resolver’s false default and document the opt-in, or is there a Resolver-supported narrower fix you recommend? I have left the implementation unchanged pending that decision.

@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.

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 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.

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 gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 by ConfigUtils.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.

@gnodet

gnodet commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

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 PrefixesRemoteRepositoryFilterSource. I've also asked the author to enrich the rejection message there so users get an actionable error pointing to the workaround property.

@efegokdemir

Copy link
Copy Markdown
Contributor Author

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.

@efegokdemir efegokdemir closed this Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants