Update to Resolver 2.0.21#12504
Conversation
Also add validations.
There was a problem hiding this comment.
Pull request overview
Updates Maven’s embedded Resolver integration to the targeted 2.0.21 line and wires in additional validation to catch malformed/uninterpolated coordinates and invalid metadata version/path components earlier in the resolution pipeline.
Changes:
- Bumps the Maven Resolver version property to 2.0.21-SNAPSHOT.
- Introduces a validating metadata reader and switches multiple metadata read paths to use it.
- Adds a Resolver
Validator+ValidatorFactoryto reject placeholder-containing coordinates and validate artifact/metadata components.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Updates the Resolver version property used across the build. |
| maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/metadata/ValidatingMetadataXpp3Reader.java | Adds a validating wrapper around MetadataXpp3Reader using Resolver PathUtils validation. |
| maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenMetadata.java | Switches metadata file parsing to the validating reader. |
| maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionResolver.java | Uses validating metadata parsing when reading available versions. |
| maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java | Uses validating metadata parsing when reading available versions. |
| maven-core/src/main/java/org/apache/maven/internal/aether/MavenValidatorFactory.java | Adds a ValidatorFactory providing Maven’s validator instance to Resolver. |
| maven-core/src/main/java/org/apache/maven/internal/aether/MavenValidator.java | Adds a Maven-specific Validator to reject placeholders and validate components. |
| maven-core/src/main/java/org/apache/maven/artifact/repository/metadata/io/DefaultMetadataReader.java | Switches metadata reading to the validating reader. |
| maven-core/src/main/java/org/apache/maven/artifact/repository/metadata/AbstractRepositoryMetadata.java | Switches repository metadata update reads to the validating reader. |
| maven-compat/src/main/java/org/apache/maven/artifact/repository/metadata/DefaultRepositoryMetadataManager.java | Switches legacy metadata reads to the validating reader. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <cipherVersion>2.0</cipherVersion> | ||
| <jxpathVersion>1.4.0</jxpathVersion> | ||
| <resolverVersion>2.0.20</resolverVersion> | ||
| <resolverVersion>2.0.21-SNAPSHOT</resolverVersion> |
| public static Metadata validate(Metadata metadata) { | ||
| if (metadata != null) { | ||
| PathUtils.validatePathComponent(metadata.getVersion(), "version"); | ||
| Versioning versioning = metadata.getVersioning(); | ||
| if (versioning != null) { |
| @Override | ||
| public void validateArtifact(Artifact artifact) throws IllegalArgumentException { | ||
| if (containsPlaceholder(artifact.getGroupId()) | ||
| || containsPlaceholder(artifact.getArtifactId()) | ||
| || containsPlaceholder(artifact.getVersion()) | ||
| || containsPlaceholder(artifact.getClassifier()) | ||
| || containsPlaceholder(artifact.getExtension())) { | ||
| throw new IllegalArgumentException("Not fully interpolated artifact " + artifact); | ||
| } | ||
| PathUtils.validateArtifactComponents(artifact); | ||
| } |
gnodet
left a comment
There was a problem hiding this comment.
Review: Update to Resolver 2.0.21
Clean and well-structured update that brings security hardening to Maven 3.10.x via Resolver 2.0.21.
What this PR does well
- Security hardening — adds
ValidatingMetadataXpp3Readerto reject path-traversal attacks in metadata versions, andMavenValidatorto catch un-interpolated${...}placeholders before they reach the resolver - Backward compatibility — intentionally omits
validateRemoteRepository()(documented with GH-7398 reference), since users commonly have${env.TOKEN}in repository URLs - Consistent pattern — the
MavenValidator/MavenValidatorFactorypair follows the established Resolver SPI pattern - SNAPSHOT version — standard development workflow for this branch (prior commits show the same SNAPSHOT → release bump pattern)
Observations (informational)
-
[Low] No dedicated unit tests for
ValidatingMetadataXpp3ReaderorMavenValidator— the existing integration testMavenITgh12305InvalidCollectRequestUninterpolatedManagedDepsTestexercises the un-interpolated placeholder path, and all 18 CI matrix entries pass. The validation logic itself is straightforward delegation toPathUtilsutilities, so integration-level coverage is a reasonable trade-off. -
[Info]
PathUtils.validatePathComponent()/validateArtifactComponents()/validateMetadataComponents()— these come from the unreleased Resolver 2.0.21. Since the same author (cstamas) wrote both the Resolver PR (apache/maven-resolver#1959) and this Maven PR, and all CI passes, this is low-risk. -
[Info] RuntimeException propagation — in some metadata-reading paths (
MavenMetadata.read(),DefaultMetadataReader,AbstractRepositoryMetadata), anIllegalArgumentExceptionfrom validation would propagate uncaught. In the primary version-resolution paths, the broadcatch (Exception e)handles it gracefully. For the other paths, failing on invalid metadata is the desired security behavior.
Checklist
| Check | Status |
|---|---|
| Tests | ✓ Integration tests pass (18/18 CI jobs) |
| Docs | ✓ Javadoc with @since 3.10.0, GH-7398 reference |
| Commit convention | ✓ |
| Public API / backward compat | ✓ Internal packages only |
| Security | ✓ Positive — adds hardening |
| CI | ✓ All pass |
LGTM.
Reviewed with Claude Code on behalf of Guillaume Nodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
ascheman
left a comment
There was a problem hiding this comment.
LGTM, and I verified it end-to-end rather than just reading the diff. (Non-binding review.)
Coverage is complete. git grep 'new MetadataXpp3Reader()' on current maven-3.10.x yields exactly the six files this PR migrates to ValidatingMetadataXpp3Reader — no metadata parse site is left unvalidated.
Wiring is correct. MavenValidatorFactory (@Named @Singleton) is picked up via Sisu collection injection into resolver's DefaultRepositorySystemValidator(List<ValidatorFactory>) — no extra registration needed.
Empirical proof it bites. I built a distribution from the PR head (b4b42148ec, resolver 2.0.21-SNAPSHOT) and resolved a version range [0,) against a hand-crafted maven-metadata.xml whose <version> is ../../../../../../../../tmp/pwned:
- Baseline 3.10.0-rc-1 (no fix): the traversal string flows straight into a real filesystem path — the resolver tries to open a lock file literally named
.../.locks/artifact~org.evil~widget~pom~../../../../../../../../tmp/pwned.lock. The..segments are live path components; it only fails incidentally, nothing rejects the traversal. - This PR: caught at parse time —
[WARNING] ... is invalid: Invalid versioning/latest: must not contain '..', '/' or '\': ../../../../../../../../tmp/pwned(IllegalArgumentException at PathUtils.validatePathComponent:91). The bad version is dropped, the range sees no candidates, and the traversal string never reaches any filesystem path.
Nice touch that a poisoned repo is treated as "contributed no usable versions" (per-repository WARNING) rather than a hard build-abort.
Two remarks, neither blocking:
- Merge gate (stating the obvious):
resolverVersionis2.0.21-SNAPSHOT; needs released 2.0.21 before this lands — CI is only green via apache-snapshots. - Nit:
ValidatingMetadataXpp3Reader.validate()intentionally validates only the version-ish fields, notmetadata.groupId/artifactIdorsnapshotVersions[*].classifier/extension— presumably because those storage paths derive from request-side coordinates, whichMavenValidatoralready covers. A one-line comment saying so would preempt the question for future readers.
Also add validations.
Fixes #12484