gnodet-bot commented on code in PR #2135:
URL: https://github.com/apache/maven-resolver/pull/2135#discussion_r3993944723
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/EnhancedLocalRepositoryManagerFactory.java:
##########
@@ -107,22 +81,29 @@ public class EnhancedLocalRepositoryManagerFactory
implements LocalRepositoryMan
public static final boolean DEFAULT_VERIFY_REAL_PATH = true;
/**
- * Whether to enable "legacy tracking fallback" in LRM. If starting
"greenfield" with Resolver 2 enabled Maven,
- * this should be {@code false}, but for smoother transition of users
using Maven 3.9 or older versions, the default
- * is {@code true}. When the local repository is shared across "older" and
"newer" Maven versions (where "older"
- * Maven versions are Resolver 1.x and "never" Maven versions are Resolver
2.x based), the preferred way is to
- * enable this feature. On the other hand, if local repository is
exclusively used by "newer" Maven versions,
- * like 3.10 or above, for improved Repository cache poisoning protection,
this configuration is recommended
- * to be set to {@code false}.
+ * Marks local repository is meant to be shared (or was shared) with
legacy Maven 3.9 or older versions.
Review Comment:
💡 **Javadoc grammar:** "Marks local repository is meant" is ungrammatical.
```suggestion
* Marks whether the local repository is meant to be shared (or was
shared) with legacy Maven 3.9 or older versions.
```
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/EnhancedLocalRepositoryManager.java:
##########
@@ -326,6 +332,27 @@ private boolean applyTracking(Path path,
LocalArtifactResult result, Properties
result.setRepository(repository);
return true;
}
+ // Same-ID-different-URL fallback: if the tracking file
contains a URL-qualified entry for the
+ // same repository ID but with a different URL hash (e.g. real
Central tracked as
+ // "central-<sha1(realUrl)>=" but the current build overrides
central to "file:target/null"),
+ // the exact lookup misses because sha1(realUrl) !=
sha1(file:target/null). Match by repo-ID
+ // prefix: any entry starting with "filename>repoId-" is
accepted as originating from the same
+ // logical repository.
+ String repoIdPrefix = getKey(path, legacyKey + "-");
+ for (Object key : props.keySet()) {
+ String k = key.toString();
+ if (k.startsWith(repoIdPrefix) && !k.equals(getKey(path,
trackingKey))) {
+ LOGGER.debug(
+ "Accepting locally cached artifact {} via
same-id tracking entry '{}'"
+ + " (current URL-qualified key would
be '{}')",
+ path.getFileName(),
+ k,
+ getKey(path, trackingKey));
+ result.setAvailable(true);
+ result.setRepository(repository);
+ return true;
+ }
+ }
Review Comment:
🟡 **Security trade-off worth documenting:** The same-ID prefix fallback
(`repoId-` prefix match) intentionally relaxes the URL-qualified tracking that
was designed to prevent cache poisoning. When `legacyLocalRepository=true` (the
default), an artifact tracked as `central-<sha1(realCentralUrl)>` will be
accepted by *any* repository whose simple key is `central`, regardless of its
URL.
This is a deliberate trade-off for backward compatibility, but it should be
called out more prominently in the Javadoc — currently the class-level doc
still emphasizes that "two repositories that merely share an id but point at
different URLs are tracked as different origins", which is no longer
unconditionally true when this fallback fires.
Also: the prefix match `k.startsWith(repoIdPrefix)` with `repoIdPrefix =
getKey(path, legacyKey + "-")` relies on the convention that URL-qualified keys
always use `id-<hash>` format. If a future key function uses a different
separator, this breaks silently. Consider extracting the prefix construction to
a shared constant or utility.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/GroupIdRemoteRepositoryFilterSource.java:
##########
@@ -274,14 +270,28 @@ public void postProcess(RepositorySystemSession session,
List<ArtifactResult> ar
}
}
- private Path ruleFile(RepositorySystemSession session, RemoteRepository
remoteRepository) {
- return ruleFiles(session)
- .computeIfAbsent(
- normalizeRemoteRepository(session, remoteRepository),
- r -> getBasedir(session, LOCAL_REPO_PREFIX_DIR,
CONFIG_PROP_BASEDIR, false)
- .resolve(GROUP_ID_FILE_PREFIX
- + repositoryKey(session,
remoteRepository)
- + GROUP_ID_FILE_SUFFIX));
+ /**
+ * Returns the {@link Path} of the user provided rule file. If {@code
forLoad} is {@code true}, returns non-{@code null}
+ * Path ONLY if file found and is readable, otherwise it returns {@code
null}. If {@code forLoad} is {@code false},
+ * then it returns "most specific" user provided file (for saving
purposes).
+ */
+ private Path ruleFile(RepositorySystemSession session, RemoteRepository
remoteRepository, boolean forLoad) {
+ return
ruleFiles(session).computeIfAbsent(normalizeRemoteRepository(session,
remoteRepository), r -> {
+ for (String key : repositoryKeys(session, remoteRepository)) {
+ Path ruleFile = getBasedir(session, LOCAL_REPO_PREFIX_DIR,
CONFIG_PROP_BASEDIR, false)
+ .resolve(GROUP_ID_FILE_PREFIX + key +
GROUP_ID_FILE_SUFFIX);
+ if (!forLoad) {
+ // return most specific
+ return ruleFile;
+ }
+ if (Files.isReadable(ruleFile)) {
+ // return if exists/readable
+ return ruleFile;
+ }
+ }
+ // none exists
+ return null;
+ });
}
private GroupTree cacheRules(RepositorySystemSession session,
RemoteRepository remoteRepository) {
Review Comment:
⚠️ **Cache-vs-mode bug:** `computeIfAbsent` caches the result of the first
call, but the lambda's behavior depends on `forLoad`. If `loadRepositoryRules`
(forLoad=true) runs first for a repo where only the legacy
`groupid-central.txt` exists, the cache stores that legacy path. The subsequent
`postProcess` call (forLoad=false) then gets the cached legacy path instead of
the most-specific tracking-keyed path — so newly recorded rules are appended to
the fallback file.
Conversely, if `postProcess` (forLoad=false) runs first, it caches the
most-specific path (which may not exist yet). The subsequent
`loadRepositoryRules` (forLoad=true) gets that non-existent path, and `filePath
!= null` on line 308 passes even though the file doesn't exist (it was computed
for save, not load). This would cause `Files.lines(filePath)` to throw
`NoSuchFileException`.
The simplest fix: don't cache in `ruleFiles` when `forLoad=true` (use a
separate lookup), or split into two caches (one for load, one for save).
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/EnhancedLocalRepositoryManagerFactory.java:
##########
@@ -107,22 +81,29 @@ public class EnhancedLocalRepositoryManagerFactory
implements LocalRepositoryMan
public static final boolean DEFAULT_VERIFY_REAL_PATH = true;
/**
- * Whether to enable "legacy tracking fallback" in LRM. If starting
"greenfield" with Resolver 2 enabled Maven,
- * this should be {@code false}, but for smoother transition of users
using Maven 3.9 or older versions, the default
- * is {@code true}. When the local repository is shared across "older" and
"newer" Maven versions (where "older"
- * Maven versions are Resolver 1.x and "never" Maven versions are Resolver
2.x based), the preferred way is to
- * enable this feature. On the other hand, if local repository is
exclusively used by "newer" Maven versions,
- * like 3.10 or above, for improved Repository cache poisoning protection,
this configuration is recommended
- * to be set to {@code false}.
+ * Marks local repository is meant to be shared (or was shared) with
legacy Maven 3.9 or older versions.
+ * Maven 3.9 and older versions suffer from "impostor" problem, where
artifact and metadata origin was tracked
+ * only by the remote repository ID, where two remote repositories may
share same ID but different URLs, in fact
+ * they may be completely unrelated to each other (ID clash by mistake),
or, it may be due some sort of "impostor"
+ * attempt, where a malicious repository may pretend like some other
repository.
+ * Right now, we intentionally default to {@code true} to ease users
transitioning, and Resolver 2 will retain
+ * this "old" behavior (will observe legacy tracking entries and will
store remote metadata as before). But,
+ * at some point in the future, the default value will be flipped to
{@code false} (and same change is warmly
+ * recommended for modern Maven users, who do not intend to share local
repository with older Maven versions.
+ * When this configuration set to {@code false}, the "repository key" is
not ID only anymore, but is changed
Review Comment:
💡 **Javadoc says default will flip to `false`** — but the default is already
`true` (line 106). The documentation reads as if the flip hasn't happened yet,
which is correct. But Copilot flagged that the sentence "the default value will
be flipped to `false`" is confusing because a reader might think it means
flipped to `true`. Consider rewording:
```suggestion
* at some point in the future, the default value will be changed to
{@code false} (and same change is warmly
* recommended for modern Maven users, who do not intend to share local
repository with older Maven versions).
```
(Also: missing closing parenthesis on line 93.)
--
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]
