Copilot commented on code in PR #2188:
URL: https://github.com/apache/maven-resolver/pull/2188#discussion_r4250726019
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -364,24 +363,32 @@ private CachedPrefixes cachePrefixes(
private static final PrefixTree ENABLED_NO_INPUT = new
PrefixTree("enabled-no-input");
private static final PrefixTree BROKEN = new PrefixTree("broken");
+ private enum VerificationOutcome {
+ SERVED,
+ ABSENT,
+ UNKNOWN
+ }
+
/**
* The cached per remote repository prefixes state: the effective {@link
PrefixTree}, whether it was
* auto-discovered (as only auto-discovered prefixes are subject to denied
path verification, see
- * {@link #CONFIG_PROP_VERIFY_DENIED}), whether verification happened
already, and the denied path (if any)
- * that verification proved the remote repository actually serves.
+ * {@link #CONFIG_PROP_VERIFY_DENIED}), per-path caching of paths verified
as served, and per-path caching
+ * of definitively absent paths, with verification outcomes maintained
independently for each path.
*/
private static final class CachedPrefixes {
private static final CachedPrefixes DISABLED_PREFIXES = new
CachedPrefixes(DISABLED, false);
private static final CachedPrefixes NO_INPUT_PREFIXES = new
CachedPrefixes(ENABLED_NO_INPUT, false);
private volatile PrefixTree prefixTree;
private final boolean autoDiscovered;
- private final AtomicBoolean verifyClaimed = new AtomicBoolean(false);
- private volatile String verifiedServedPath;
+ 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();
}
Review Comment:
The per-repository/session `verifiedServedPaths` and especially
`verifiedAbsentPaths` sets can grow without bound if many distinct denied paths
are encountered (e.g., large builds, misconfigured repos, or adversarial inputs
producing many 404 verifications). Consider bounding these caches (max size
with eviction) to prevent unbounded memory growth while still keeping
verification/probe costs under control.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -628,21 +667,29 @@ private boolean
isVerifyDeniedDropsTreeEnabled(RemoteRepository repository) {
/**
* Checks whether the remote repository actually serves given path,
using a lightweight existence check
- * (the transporter sits below the filtering connector, so no
recursion can happen). Any failure (path
- * not present, transport problem) yields {@code false}: the prefixes
verdict is overridden (or, with
- * {@link #CONFIG_PROP_VERIFY_DENIED_DROPS_TREE}, the whole file
dropped) only when the remote repository
- * provably serves the denied path.
+ * (the transporter sits below the filtering connector, so no
recursion can happen). Only definitive
+ * {@link Transporter#ERROR_NOT_FOUND} is recorded as absent so
transient transport or network failures
+ * do not permanently poison the negative cache.
*/
- private boolean remoteRepositoryServesPath(RemoteRepository
repository, String path) {
- try (Transporter transporter =
transporterProvider.newTransporter(session, repository)) {
+ 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();
+ }
}
}
Review Comment:
Returning `VerificationOutcome.ABSENT` on `URISyntaxException` is logically
incorrect: URI construction failure does not prove the path is missing
remotely, and it can cause a permanently negative-cached denial for paths
containing characters requiring encoding. Treat this case as `UNKNOWN` (no
negative caching), or ensure the `path` is encoded/constructed in a way that
cannot throw `URISyntaxException` for valid repository paths.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/PrefixesRemoteRepositoryFilterSource.java:
##########
@@ -590,12 +620,21 @@ private Result acceptPrefix(RemoteRepository repository,
String path) {
+ " (verified served despite stale
auto-discovered prefixes)");
}
}
- return result(
- accepted,
- NAME,
- accepted
- ? "Path " + path + " allowed from " +
repository.getId()
- : "Path " + path + " NOT allowed from " +
repository.getId());
+ return accepted
+ ? result(true, NAME, "Path " + path + " allowed from " +
repository.getId())
+ : notAllowedResult(repository, path, cachedPrefixes);
+ }
+
+ private Result notAllowedResult(RemoteRepository repository, String
path, CachedPrefixes cachedPrefixes) {
+ String reasoning = "Path " + path + " NOT allowed from " +
repository.getId();
+ if (cachedPrefixes.autoDiscovered()) {
+ 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";;
Review Comment:
This message is emitted for any auto-discovered denial, including cases
where verification is already enabled and/or the path was definitively verified
as absent. In those cases, stating the prefix file 'appears incomplete' and
suggesting `...verifyDenied.<repo>=true` is misleading/redundant. Consider
tailoring the reasoning based on state (e.g., verification disabled → suggest
enabling; verified absent → state it was checked and not found; verification
outcome unknown → state verification failed transiently and will be retried).
--
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]
