Repository navigation
Fix prefix filter verification for denied artifact paths - #2188
harshab054 wants to merge 3 commits into
Conversation
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Thanks for tackling the per-path caching — the direction is right. A few observations:
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Thanks for this — the per-path verification caching and the VerificationOutcome enum to distinguish transient from definitive failures are the right improvements here.
One thing that's missing and would make a real difference for users: an actionable error message when a path is rejected.
Right now, when a denied path makes it through the pre-checks and ends up getting a hard rejection (e.g. verifyDenied is disabled, or the path is in verifiedAbsentPaths), the reasoning() string in the result is just:
Path /com/example/internal/1.0/internal-1.0.jar NOT allowed from central
That's what users see in ArtifactFilteredOutException, and it gives them no hint of what's happening or how to work around it.
Since we're already in PrefixesRemoteRepositoryFilterSource and have access to cachedPrefixes.autoDiscovered(), repository.getId(), and the config property name constants, this is the ideal place to enrich the message. When the tree is auto-discovered and the path is rejected, the reasoning could be 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 with an incomplete prefix file, set
-Daether.remoteRepositoryFilter.prefixes.verifyDenied.central=true
or report this to the repository administrator.
See https://maven.apache.org/resolver/remote-repository-filtering.html
For user-provided prefix files (not auto-discovered), keep the bare message — those denials are deliberate.
Would you be willing to add this here? It's a small addition but would save a lot of debugging time for users hitting this in the wild.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The latest commits address all three previous observations — good follow-up:
- ✅ Javadoc updated —
CachedPrefixesclass doc now correctly describes per-path caching of both served and absent paths. - ✅
synchronizedblock comment added — the intent (preventing duplicate verification of the same path, not global serialization) is now documented inline. - ✅ Wasted allocations resolved —
DISABLED_PREFIXES/NO_INPUT_PREFIXESnow useCollections.emptySet()whenautoDiscovered=false; the empty sets allocated for static singletons are gone.
The test additions are solid: transportFailureDoesNotNegativeCachePath directly validates the UNKNOWN/retry semantics, unservedDeniedPathRemainsBlockedAndDoesNotDisableFilter covers negative caching, and the reasoning-string assertions in deniedPathAbsentFromRemoteStaysDenied pin the actionable error message content.
One informational note: remoteRepositoryServesPath uses a manual try/finally instead of try-with-resources. This is justified — transporter.classify(e) must be called inside the catch block where transporter needs to be in scope; try-with-resources would scope the variable to the try block only. No change needed.
No blocking issues.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
I am very uneasy on changes like this (and previous #2082) and here is why: prefixes are usually (and are meant) to be resolved from remote, but, there are (may be) cases, when user wants to provide them manually, check out the RRF demo: https://github.com/cstamas/rrf-demo/tree/master/.mvn/rrf So in latter case, prefixes now become "advice" only? In other word, in latter case they are "user provided configuration", that Maven may opt to simply ignore/override? Like, I configured it intentionally like this, but will Maven obey or not, is up to some heuristics. We may then want to distinguish, between "user provided" (when content comes from a configured file) and "remotely resolved" (when it comes from remote)? And this is the reason why I was pushing for "black or white": when present, no magic but fully obeyed, or when absent (404 or user configures it disabled) just work without it. Essentially, it boils down to one thing: "fix your MRM, it is broken". No magic or heuristic helps here, when software is broken. |
|
Thanks for raising this concern. I agree that explicitly user-provided prefix filters should be enforced strictly and should not be silently overridden. My intention with this change was to address incomplete, automatically discovered prefix files while preserving strict filtering for user-provided configurations. Would you recommend limiting the verification behavior strictly to automatically discovered prefixes, while keeping user-provided prefix files fully enforced? Or would you prefer a strict, black-or-white policy for both cases? I'd be happy to adjust the implementation based on the intended Resolver behavior. |
|
Yes, that would make sense: limit this behaviour for automatically discovered prefix files only. And many-many thanks for the work so far! |
|
Hi @cstamas, I've updated the PR to restrict denied-path verification exclusively to automatically discovered prefix files, while keeping user-provided prefix files strictly enforced. I've also addressed the singleton allocation and synchronization documentation feedback. The targeted tests and all 19 reported CI matrix jobs are passing. Whenever you have time, could you please take another look? Thank you for the feedback! |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
What changed in this PR
Fixes Maven Resolver’s prefix filter behavior when a repository serves artifact paths that are missing from an auto-discovered prefixes.txt, by verifying denied paths against the remote repository and caching the outcomes.
Changes:
- Add per-artifact-path verification for denied paths when prefixes are auto-discovered (served paths become allowed; definite 404s become negative-cached; transient failures are retried).
- Extend reasoning/messages for denied artifacts to include repository-specific verification guidance and documentation links.
- Expand tests to cover multiple independent denied paths, per-path caching (positive/negative), and transient transport failures.
| File | Description |
|---|---|
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java |
Implements per-path verification outcomes and caching; updates denial reasoning and verification behavior. |
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSourceVerifyDeniedTest.java |
Adds/updates test coverage for per-path verification, caching semantics, and reasoning content. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private VerificationOutcome remoteRepositoryServesPath(RemoteRepository repository, String path) { | ||
| Transporter transporter = null; | ||
| try { | ||
| transporter = transporterProvider.newTransporter(session, repository); | ||
| transporter.peek(new PeekTask(new URI(null, null, path, null))); | ||
| return true; | ||
| return VerificationOutcome.SERVED; | ||
| } catch (URISyntaxException e) { | ||
| logger.debug("Cannot construct URI for denied path {} of {}", path, repository, e); | ||
| return false; | ||
| return VerificationOutcome.ABSENT; | ||
| } catch (Exception e) { | ||
| logger.debug("Verification of denied path {} against {} failed", path, repository, e); | ||
| return false; | ||
| if (transporter != null && transporter.classify(e) == Transporter.ERROR_NOT_FOUND) { | ||
| return VerificationOutcome.ABSENT; | ||
| } | ||
| return VerificationOutcome.UNKNOWN; | ||
| } finally { | ||
| if (transporter != null) { | ||
| transporter.close(); | ||
| } | ||
| } | ||
| } |
| private final Set<String> verifiedServedPaths; | ||
| private final Set<String> verifiedAbsentPaths; | ||
|
|
||
| private CachedPrefixes(PrefixTree prefixTree, boolean autoDiscovered) { | ||
| this.prefixTree = prefixTree; | ||
| this.autoDiscovered = autoDiscovered; | ||
| this.verifiedServedPaths = autoDiscovered ? ConcurrentHashMap.newKeySet() : Collections.emptySet(); | ||
| this.verifiedAbsentPaths = autoDiscovered ? ConcurrentHashMap.newKeySet() : Collections.emptySet(); | ||
| } |
| reasoning += " (auto-discovered prefix file appears incomplete). If this repository serves a " | ||
| + "virtual aggregate with an incomplete prefix file, set -D" | ||
| + CONFIG_PROP_VERIFY_DENIED + "." + repository.getId() | ||
| + "=true or report this to the repository administrator. " | ||
| + "See https://maven.apache.org/resolver/remote-repository-filtering.html"; |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all previous observations have been addressed, and the new commit is clean.
The delta (fixing verifiedAbsentResult on the post-lock path, extracting inconclusiveVerificationResult, correcting URISyntaxException → UNKNOWN to prevent false negative caching, leading-slash stripping, and the offline-aware reasoning branch in notAllowedResult) is correct throughout. All new tests exercise the right code paths.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Description
Fixes the Maven Resolver prefix filter behavior when a repository serves an artifact path that is missing from its "prefixes.txt".
A repository such as an Artifactory/Nexus virtual repository may expose a prefixes file generated from a public repository index while also serving private/internal artifacts. When the requested artifact path is absent from that prefixes file, the prefix filter can reject an artifact that the repository actually serves.
This change verifies a denied path against the repository before permanently rejecting it.
Implementation
Security
The implementation preserves the security invariant introduced by Resolver PR #2082.
A successful verification of one denied path does NOT disable the prefix filter for the entire repository or authorize unrelated artifact paths.
Only the exact remotely verified path is allowed.
Tests
Related Maven Issue
This addresses the underlying Maven Resolver behavior reported in Apache Maven issue #13385:
apache/maven#13385
The original issue is reported against Maven because Maven users experience the resolution failure, while the prefix filtering implementation itself lives in Maven Resolver.