Copilot commented on code in PR #2135:
URL: https://github.com/apache/maven-resolver/pull/2135#discussion_r3993266654
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/EnhancedLocalRepositoryManagerFactory.java:
##########
@@ -159,25 +140,17 @@ public LocalRepositoryManager
newInstance(RepositorySystemSession session, Local
|| trackingFilename.contains("..")) {
trackingFilename = DEFAULT_TRACKING_FILENAME;
}
- boolean legacyTrackingFallback =
- ConfigUtils.getBoolean(session,
DEFAULT_LEGACY_TRACKING_FALLBACK, CONFIG_PROP_LEGACY_TRACKING_FALLBACK);
+ boolean legacyLocalRepository =
+ ConfigUtils.getBoolean(session,
DEFAULT_LEGACY_LOCAL_REPOSITORY, CONFIG_PROP_LEGACY_LOCAL_REPOSITORY);
Review Comment:
The old `CONFIG_PROP_LEGACY_TRACKING_FALLBACK` setting is no longer read
here. A user who explicitly set that opt-out to `false` will silently get the
new `legacyLocalRepository` default of `true`, enabling the
same-ID/different-URL acceptance path and weakening the provenance protection
they selected. Preserve the old property as a compatibility alias (and retain
its public constant) when resolving the new option.
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/remoterepo/RepositoryKeyFunctionFactory.java:
##########
@@ -50,7 +75,29 @@ public interface RepositoryKeyFunctionFactory {
* no configuration lookup happens but the {@code
defaultValue} is used to create the
* repository key function.
* @return The repository key function.
+ * @deprecated Use {@link #repositoryKeyFunction(Class,
RepositorySystemSession, String, String...)} instead.
+ */
+ @Deprecated
+ default RepositoryKeyFunction repositoryKeyFunction(
+ Class<?> owner, RepositorySystemSession session, String
defaultValue, String configurationKey) {
+ return repositoryKeyFunction(owner, session, defaultValue, new
String[] {configurationKey});
+ }
+
+ /**
+ * Method that based on configuration returns the "repository key
function". The returned function will be session
+ * cached if session is equipped with cache, otherwise it will be non
cached. Method never returns {@code null}.
+ * Only the {@code configurationKey} parameter may be {@code null} in
which case no configuration lookup happens
+ * but the {@code defaultValue} is directly used instead.
+ *
+ * @param owner The "owner" of key function (used to create cache-key),
must not be {@code null}.
+ * @param session The repository session, must not be {@code null}.
+ * @param defaultValue The default value of repository key configuration,
must not be {@code null}.
+ * @param configurationKeys The configuration keys to lookup configuration
from, may be {@code null}, in which case
+ * no configuration lookup happens but the {@code
defaultValue} is used to create the
+ * repository key function.
+ * @return The repository key function.
+ * @since 2.0.23
*/
RepositoryKeyFunction repositoryKeyFunction(
- Class<?> owner, RepositorySystemSession session, String
defaultValue, String configurationKey);
+ Class<?> owner, RepositorySystemSession session, String
defaultValue, String... configurationKeys);
Review Comment:
Adding the varargs overload as the new abstract method is not source- or
binary-compatible with existing `RepositoryKeyFunctionFactory` implementations
that implement only the former `String configurationKey` method. The deprecated
method being a default does not make those implementations implement the new
abstract signature; calls through the new tracking/system defaults can fail to
link. Keep a compatible abstract contract and make the varargs form an adapting
default, or otherwise provide both implementations.
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/remoterepo/RepositoryKeyFunctionFactory.java:
##########
@@ -32,10 +33,34 @@ public interface RepositoryKeyFunctionFactory {
*
* @param session The repository session, must not be {@code null}.
* @return The repository key function.
- * @see #repositoryKeyFunction(Class, RepositorySystemSession, String,
String)
+ * @see #repositoryKeyFunction(Class, RepositorySystemSession, String,
String[])
* @see
org.eclipse.aether.ConfigurationProperties#REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION
*/
- RepositoryKeyFunction systemRepositoryKeyFunction(RepositorySystemSession
session);
+ default RepositoryKeyFunction
systemRepositoryKeyFunction(RepositorySystemSession session) {
+ return repositoryKeyFunction(
+ RepositoryKeyFunctionFactory.class,
+ session,
+
ConfigurationProperties.DEFAULT_REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION,
+
ConfigurationProperties.REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION);
+ }
+
+ /**
+ * Returns system-wide tracking repository key function.
+ *
+ * @param session The repository session, must not be {@code null}.
+ * @return The repository key function.
+ * @see #repositoryKeyFunction(Class, RepositorySystemSession, String,
String[])
+ * @see
org.eclipse.aether.ConfigurationProperties#REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION
+ * @since 2.0.23
+ */
+ default RepositoryKeyFunction
trackingRepositoryKeyFunction(RepositorySystemSession session) {
+ return repositoryKeyFunction(
+ RepositoryKeyFunctionFactory.class,
+ session,
+
ConfigurationProperties.DEFAULT_REPOSITORY_TRACKING_REPOSITORY_KEY_FUNCTION,
+
ConfigurationProperties.REPOSITORY_TRACKING_REPOSITORY_KEY_FUNCTION,
+
ConfigurationProperties.REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION);
Review Comment:
Both convenience methods pass `RepositoryKeyFunctionFactory.class` as the
cache owner, while `DefaultRepositoryKeyFunctionFactory` uses that owner as the
session-cache namespace. As soon as one function is requested for a
repository/context, the other can read the cached value and return the wrong
key (for example, `nid` is reused as the default `nid_hurl` tracking key),
making behavior depend on call order. Use distinct cache namespaces or include
the selected configuration in the cache key.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java:
##########
@@ -620,6 +620,32 @@ public enum HttpVersion {
public static final String
DEFAULT_REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION = "nid";
+ /**
+ * Repository key function used for the provenance tracking entries this
local repository manager writes and
+ * consults, and for nothing else. With an ID-only key, "came from
+ * repository X" means X's possibly colliding label: a repository declared
in an untrusted (for example,
+ * transitively resolved) POM under the same ID as a trusted repository
would be tracked as the same origin and
+ * could poison a shared local repository. The default is therefore the
URL-qualified {@code "nid_hurl"}
+ * function, scoped to tracking entries only: repository identity
everywhere else (repository aggregation and
+ * mirror merging, artifact and metadata path composition, split local
repository prefixes) keeps following the
+ * system-wide key function, whose default is unchanged - so no
aggregation semantics change and no local
+ * repository re-layout occurs. If the system-wide function
Review Comment:
This description still says artifact/metadata path composition and split
local-repository prefixes use the system-wide key and that no local re-layout
occurs, but `DefaultLocalPathPrefixComposerFactory` now passes
`trackingRepositoryKeyFunction`. With the default `nid_hurl` tracking key,
split paths can include the URL hash. Update this public configuration
documentation to describe the tracking-key-based path behavior.
##########
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 is local repository 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 true} (and same change is warmly
+ * recommended for modern Maven users, who do not intend to share local
repository with older Maven versions.
Review Comment:
The migration note says the default will eventually be flipped to `true`,
but this option is already declared with a default of `true` on the next line
and the surrounding text recommends `false` for modern-only repositories. The
future value should be `false` so the documentation describes the intended
security transition.
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/remoterepo/RepositoryKeyFunctionFactory.java:
##########
@@ -32,10 +33,34 @@ public interface RepositoryKeyFunctionFactory {
*
* @param session The repository session, must not be {@code null}.
* @return The repository key function.
- * @see #repositoryKeyFunction(Class, RepositorySystemSession, String,
String)
+ * @see #repositoryKeyFunction(Class, RepositorySystemSession, String,
String[])
* @see
org.eclipse.aether.ConfigurationProperties#REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION
*/
- RepositoryKeyFunction systemRepositoryKeyFunction(RepositorySystemSession
session);
+ default RepositoryKeyFunction
systemRepositoryKeyFunction(RepositorySystemSession session) {
+ return repositoryKeyFunction(
+ RepositoryKeyFunctionFactory.class,
+ session,
+
ConfigurationProperties.DEFAULT_REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION,
+
ConfigurationProperties.REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION);
+ }
+
+ /**
+ * Returns system-wide tracking repository key function.
+ *
+ * @param session The repository session, must not be {@code null}.
+ * @return The repository key function.
+ * @see #repositoryKeyFunction(Class, RepositorySystemSession, String,
String[])
+ * @see
org.eclipse.aether.ConfigurationProperties#REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION
Review Comment:
The tracking-specific convenience method links to
`REPOSITORY_SYSTEM_REPOSITORY_KEY_FUNCTION`, so generated API documentation
points users at the wrong configuration property. This reference should name
`REPOSITORY_TRACKING_REPOSITORY_KEY_FUNCTION` while noting the system property
is only the fallback.
##########
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))) {
Review Comment:
The same-ID fallback builds its prefix from `simpleRepositoryKeyFunction`,
but that function is not just the repository ID for repository managers: it
appends a mirror/context hash. A URL-qualified entry is keyed as `id-<url
hash>` and therefore will not start with `id-<simple-key hash>-`, so the
fallback added here fails for repository-manager repositories even when their
IDs match. Derive the prefix from an ID-only key instead of the full simple key.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/checksum/SummaryFileTrustedChecksumsSource.java:
##########
@@ -188,7 +184,8 @@ protected Map<String, String> doGetTrustedMetadataChecksums(
List<ChecksumAlgorithmFactory> checksumAlgorithmFactories) {
return doGetTrustedPathChecksums(
session,
- localPathComposer.getPathForMetadata(metadata,
repositoryKey(session, artifactRepository)),
+ localPathComposer.getPathForMetadata(
+ metadata, repositoryKey(session,
artifactRepository).get(0)),
artifactRepository,
checksumAlgorithmFactories);
Review Comment:
This metadata path is always composed with the tracking key, even though the
loop below intentionally checks both tracking and system repository keys.
Existing summary files written with the previous default (`nid`) store metadata
under the system-key path, so loading the fallback summary file still cannot
find those entries. The lookup needs to try the corresponding metadata path for
each repository key, not just the first one.
This issue also appears on line 213 of the same file.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/RemoteRepositoryFilterSourceSupport.java:
##########
@@ -111,18 +102,22 @@ protected RemoteRepository normalizeRemoteRepository(
}
/**
- * Returns repository key to be used on file system layout.
+ * Returns repository keys to be used on file system layout for user
provided files. They are ordered as
+ * "most specific" (using {@link {@link
RepositoryKeyFunctionFactory#trackingRepositoryKeyFunction(RepositorySystemSession)}}
Review Comment:
This Javadoc contains a nested `{@link}` tag (`{@link {@link ...}}`), which
is malformed and can produce invalid generated API documentation. Remove the
inner tag and close the surrounding sentence after the single link.
--
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]
