Resolver 2.0.23 and validation cleanup by cstamas · Pull Request #13078 · apache/maven

cstamas · 2026-09-08T10:01:33Z

We had overlapping changes in Resolver and Maven, align them.

ascheman · 2026-09-08T10:21:22Z

Sure about the SNAPSHOT-dependency, @cstamas ? Or will you resolve this before you make the draft final?

cstamas · 2026-09-08T10:24:48Z

We usually run "main" branches (in quotes, as we have several "main" branches, the master, the maven-4.0.x, the maven-3.10.x, etc) just before the release with SNAPSHOT versions, as we do produce and deploy them, is fine to get another layer of confirmation. OTOH, you cannot release anyway with a SNAPSHOT, hence, this is completely okay, and we did it before as well.

cstamas · 2026-09-08T10:33:15Z

So, all of ITs explode, reason is f013 in this commit:
apache/maven-resolver@339161b

@gnodet ping

gnodet · 2026-09-08T11:32:32Z

@cstamas The root cause is the tracking key function change from nid to nid_hurl in commit 339161b (f013). All existing _remote.repositories entries use ID-only keys (e.g. artifact>central=), but the new resolver looks for URL-qualified keys (artifact>central-<sha1>=). No match → every cached artifact appears "present but unavailable" → re-download from the IT's fake repos → 💥

This affects all 9 IT jobs (4229 test failures, 507 unique test classes on ubuntu/JDK26 alone). The existenceCheckRelabel change (f005) is secondary — even with the old existence-check behavior, the tracking key mismatch would cause failures because the fake repos can't serve the artifacts either way.

Fix PR: apache/maven-resolver#2133 — adds a backward-compatible fallback in applyTracking(): when the nid_hurl lookup misses, tries the system-wide key function (nid) as fallback. If the legacy key matches, the artifact is accepted with a debug log. The entry gets upgraded to nid_hurl on next download. Security intent of f013 is preserved for new entries.

…allback (#2133) ## Summary When the tracking key function is URL-qualified (`nid_hurl`, the default since 2.0.23 / commit 339161b), tracking entries written by an older resolver using ID-only keys (`nid` format, e.g. `artifact>central=`) are invisible to the new lookup which expects `artifact>central-<sha1>=`. This causes **all** artifacts cached in the local repository before the upgrade to appear as "present but unavailable", triggering full re-downloads from remote repositories. In CI environments and integration tests that use fake/file-based repositories (like maven-integration-testing), this breaks resolution entirely — see [apache/maven#13078 (comment)](apache/maven#13078 (comment)). ## Root Cause The tracking key change from `nid` to `nid_hurl` (finding f013 in the security audit) makes existing `_remote.repositories` entries invisible: - **Old entry**: `maven-core-3.8.6.pom>central=` - **New lookup**: `maven-core-3.8.6.pom>central-<sha1 of repo URL>=` → no match The `isTracked()` check finds the old `central=` entry, so the artifact is treated as "tracked but not for the current repo" rather than "untracked" — the untracked inter-op fallback does not apply, and the artifact is rejected. ## Fix Add a backward-compatible fallback in `applyTracking()`: when the URL-qualified (`nid_hurl`) lookup misses, try the system-wide key function (`nid` by default) as a fallback. If that matches, accept the artifact and log a debug message. The legacy entry will be upgraded to the new key format on the next download. This preserves the f013 security improvement (URL-qualified tracking prevents same-id-different-URL repository poisoning for *new* entries) while providing a smooth upgrade path from older resolvers. ## Tests - Updated `testUrlQualifiedTrackingTreatsLegacyIdOnlyEntriesAsStale` → `testUrlQualifiedTrackingAcceptsLegacyIdOnlyEntriesViaFallback`: legacy entries from a matching repo are now accepted - Added `testUrlQualifiedTrackingRejectsLegacyIdOnlyEntriesFromDifferentRepo`: legacy entries from a *different* repo are still rejected - `testUrlQualifiedTrackingDistinguishesSameIdDifferentUrl`: unchanged — URL-qualified entries still prevent same-id-different-URL poisoning

gnodet · 2026-09-08T16:14:59Z

Updated the resolver fix PR based on @cstamas's analysis. The first fix only handled legacy nid → nid_hurl migration, but the real problem is different:

The ITs override central to file:target/null to prevent remote access. With nid_hurl, the tracking key includes a hash of the repo URL: central-<sha1(https://repo.maven.apache.org/maven2)> ≠ central-<sha1(file:target/null)>. Same repo ID, different URL hash → tracking key mismatch → artifact treated as "present but unavailable" → tries to download from file:target/null → 💥

The fix now has a two-stage fallback:

Stage 1: ID-only match (handles legacy nid entries from older resolvers)
Stage 2: repo-ID prefix match (handles same-id-different-URL nid_hurl entries — the IT scenario)

This effectively relaxes f013 to recording-only: entries are written with URL-qualified keys, but lookups fall back to repo-ID matching. All 585 resolver tests pass.

Fix PR: apache/maven-resolver#2133 (force-pushed with the updated fix)

ascheman · 2026-09-09T07:26:57Z

@cstamas — the SNAPSHOT and IT-explosion questions are resolved on my side (thanks, and to @gnodet for #2133). One code point from the validation consolidation itself, independent of the IT fix:

The removed DefaultMetadataReader.validateMetadata() validated both plugin.getArtifactId() and plugin.getPrefix(), but the consolidated ValidatingMetadataXpp3Reader.validate() adds only plugin.getArtifactId() — getPrefix() is no longer validated. Since the plugin prefix is carried forward as a coordinate/path component (the old reader's comment flagged prefixes as such), is dropping its validation intentional, or should it get a PathUtils.validatePathComponent(plugin.getPrefix(), "plugin/prefix") alongside the artifactId?

Two minors: validate() went public → private (fine — internal package, no external caller left), and there's no test yet exercising the plugin-token validation path.

gnodet

Review: validation consolidation looks correct, one gap to fix

The consolidation from two separate validation sites (DefaultMetadataReader.validateMetadata() and DefaultRepositoryMetadataManager.validateVersioning()) into the single ValidatingMetadataXpp3Reader.validate() is clean — it eliminates the duplicate validateVersioning() call and the now-redundant per-reader validation logic.

The new validate() method correctly covers versioning tokens (latest, release, versions[], snapshotVersions[].version, snapshot.timestamp) and plugin.getArtifactId(). The snapshot timestamp validation and metadata.getVersion() validation are new additions not present in either old reader — good.

However, as @ascheman noted: plugin.getPrefix() validation was dropped. The removed DefaultMetadataReader.validateMetadata() explicitly validated both plugin.getArtifactId() and plugin.getPrefix(), with its Javadoc stating these "get carried forward by callers as if they were already-validated path and coordinate components." The consolidated reader only validates plugin.getArtifactId().

Fix: add PathUtils.validatePathComponent(plugin.getPrefix(), "plugin/prefix"); alongside the artifactId validation in ValidatingMetadataXpp3Reader.validate().

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of @gnodet

Co-authored-by: Guillaume Nodet <gnodet@gmail.com>

…x-resolver-2023

gnodet-bot

Re-review: previous finding addressed, one gap remains

The plugin.getPrefix() validation raised in the previous review is now correctly added alongside plugin.getArtifactId() in ValidatingMetadataXpp3Reader.validate() — that concern is resolved.

The consolidation itself is clean:

The double validateVersioning() call in DefaultRepositoryMetadataManager.readMetadata() (once inside the try block and once after it — running twice on the happy path) is correctly removed
validate() going private is safe — no external callers exist in the codebase
IllegalArgumentException → IOException wrapping is consistent and chains the cause correctly
Resolver 2.0.23 is released on Central ✓

One gap: the new plugin validation path (plugin/artifactId, plugin/prefix) has no test coverage. The existing tests exercise versioning tokens and snapshot timestamps, but there is no test with a metadata file containing an invalid plugin token (e.g. a plugin.getPrefix() with a / or ..). The validation code is correct, but there's no guard against a regression there.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-17T07:32:43Z

+            for (Plugin plugin : metadata.getPlugins()) {
+                PathUtils.validatePathComponent(plugin.getArtifactId(), "plugin/artifactId");
+                PathUtils.validatePathComponent(plugin.getPrefix(), "plugin/prefix");


⚠️ Missing test coverage: The plugin validation block added here (lines 69–71) is not exercised by any test. The existing tests in DefaultRepositoryMetadataManagerTest and DefaultRepositoryMetadataManagerValidationTest only cover versioning tokens and snapshot timestamps — neither fires when plugin.getArtifactId() or plugin.getPrefix() contains an invalid character. Add a test resource (e.g. metadata-invalid-plugin-prefix/maven-metadata.xml) with a <plugin> whose prefix contains / or .., and assert the expected exception.

gnodet-bot

Re-review: previous finding addressed — LGTM

The missing plugin validation test coverage flagged in the last review is now in place:

testMetadataWithInvalidPluginArtifactIdIsRejected() → exercises plugin/artifactId rejection on some/../artifactid (contains /)
testMetadataWithInvalidPluginPrefixIsRejected() → exercises plugin/prefix rejection on some/prefix (contains /)

The message substring assertions ("Invalid plugin/artifactId", "Invalid plugin/prefix") correctly match the PathUtils.validatePathComponent error format, which propagates through the IOException wrapping in ValidatingMetadataXpp3Reader.read() and the RepositoryMetadataReadException in DefaultRepositoryMetadataManager.readMetadata().

Rest of the consolidation remains solid: no external callers of validate() (making it private is safe), IllegalArgumentException → IOException chaining is correct, and the resolver bump to 2.0.23 is clean.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

Resolver 2.0.23-SNAPSHOT and validation cleanup

fa77334

We had overlapping changes in Resolver and Maven, align them.

cstamas added this to the 3.10.0 milestone Sep 8, 2026

cstamas requested review from ascheman, gnodet and slachiewicz September 8, 2026 10:01

cstamas self-assigned this Sep 8, 2026

cstamas added the dependencies Pull requests that update a dependency file label Sep 8, 2026

gnodet mentioned this pull request Sep 8, 2026

fix: accept locally cached artifacts via same-id fallback when tracking URL changes apache/maven-resolver#2133

Merged

gnodet mentioned this pull request Sep 8, 2026

fix: extend tracking fallback to accept same-id-different-URL entries apache/maven-resolver#2134

Closed

slachiewicz removed their request for review September 9, 2026 14:53

gnodet reviewed Sep 9, 2026

View reviewed changes

Comment thread ...rc/main/java/org/apache/maven/repository/internal/metadata/ValidatingMetadataXpp3Reader.java

cstamas and others added 3 commits September 17, 2026 09:23

Apply suggestion from @gnodet

c6f62b4

Co-authored-by: Guillaume Nodet <gnodet@gmail.com>

Fixes post merge

593da53

Merge remote-tracking branch 'upstream/maven-3.10.x' into maven-3.10.…

52d361a

…x-resolver-2023

cstamas marked this pull request as ready for review September 17, 2026 07:27

cstamas requested a review from gnodet September 17, 2026 07:27

gnodet-bot reviewed Sep 17, 2026

View reviewed changes

cstamas changed the title ~~Resolver 2.0.23-SNAPSHOT and validation cleanup~~ Resolver 2.0.23 and validation cleanup Sep 17, 2026

Add more tests; PR comment addressed

0b989e8

gnodet-bot approved these changes Sep 17, 2026

View reviewed changes

Navigation Menu

Resolver 2.0.23 and validation cleanup - #13078

Resolver 2.0.23 and validation cleanup#13078
cstamas wants to merge 5 commits into
apache:maven-3.10.xfrom
cstamas:maven-3.10.x-resolver-2023

cstamas commented Sep 8, 2026 •

edited

Loading

Uh oh!

ascheman commented Sep 8, 2026

Uh oh!

cstamas commented Sep 8, 2026

Uh oh!

cstamas commented Sep 8, 2026 •

edited

Loading

Uh oh!

gnodet commented Sep 8, 2026

Uh oh!

gnodet commented Sep 8, 2026

Uh oh!

ascheman commented Sep 9, 2026

Uh oh!

gnodet left a comment

Uh oh!

Uh oh!

gnodet-bot left a comment

Uh oh!

gnodet-bot Sep 17, 2026

Uh oh!

gnodet-bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cstamas commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascheman commented Sep 8, 2026

Uh oh!

cstamas commented Sep 8, 2026

Uh oh!

cstamas commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnodet commented Sep 8, 2026

Uh oh!

gnodet commented Sep 8, 2026

Uh oh!

ascheman commented Sep 9, 2026

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet-bot Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cstamas commented Sep 8, 2026 •

edited

Loading

cstamas commented Sep 8, 2026 •

edited

Loading