Validate coordinates and repository precedence during artifact resolution - #12955
Open
slachiewicz wants to merge 7 commits into
Open
Validate coordinates and repository precedence during artifact resolution#12955slachiewicz wants to merge 7 commits into
slachiewicz wants to merge 7 commits into
Conversation
slachiewicz
force-pushed
the
pr/artifact-resolution-3.9.x
branch
from
August 30, 2026 18:32
9f8d489 to
2b4441a
Compare
slachiewicz
marked this pull request as draft
August 30, 2026 18:54
Repositories declared by a resolved model no longer replace a same-id repository supplied by the request or session; the ids supplied at resolver construction keep their precedence on the replace pass. A repository the model itself declared is still refreshed in place, e.g. once its URL has been interpolated.
slachiewicz
force-pushed
the
pr/artifact-resolution-3.9.x
branch
from
August 30, 2026 19:01
2b4441a to
86d5e40
Compare
Repositories declared by a resolved model no longer replace a same-id repository supplied by the request or session; the ids supplied at resolver construction keep their precedence on the replace pass. A repository the model itself declared is still refreshed in place, e.g. once its URL has been interpolated.
…data DefaultMetadataReader now checks parsed metadata the same way DefaultVersionResolver already does: version tokens (release, latest, versions, snapshot versions, snapshot timestamp) and plugin entries' artifactId and prefix must be valid coordinate components, rejecting "..", '/', '\\', ':' and ISO control characters. Callers that read metadata through this class already treat a read failure as invalid metadata, so a rejected document degrades gracefully instead of being adopted as a resolved version or plugin artifactId.
… ones" This reverts commit ef74d16. ProjectModelResolver tracks in repositoryIds only the ids a model has declared, so the replace path is reached only for an id this model registered earlier. Guarding it on the id also being present in the externally supplied list therefore never protected an externally supplied repository; it only blocked a model from refreshing its own declaration when the two ids happened to match. MavenITmng5639ImportScopePomResolutionTest does exactly that: it declares central in the POM and relies on the refreshed URL once interpolation has run.
slachiewicz
marked this pull request as ready for review
August 30, 2026 21:49
gnodet
approved these changes
Aug 30, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Well-adapted 3.9.x backport of the coordinate validation and repository precedence hardening. This completes the 4-PR series (#12949 master, #12944 4.0.x, #12952 3.10.x, #12955 3.9.x).
Highlights:
- The
ProjectModelResolverrevert is correct: in 3.9.x,ProjectModelResolveronly puts model-declared ids inrepositoryIds, so the external-id guard would never protect an external repo — it would instead breakMavenITmng5639ImportScopePomResolutionTest. Good catch. - The validation logic is consistent with the other 3 variants, properly adapted for the 3.9.x codebase.
- CI passes on all platforms.
Minor observations (non-blocking, already filed on master #12949):
- Control characters in rejected values are embedded verbatim in error messages (minor log injection risk).
- Silent return when a model-declared repository conflicts with an external id — a debug log would aid troubleshooting.
- Validation logic duplication is acceptable for a stabilized branch.
- The 3.10.x variant adds validation in
DefaultRepositoryMetadataManager.readMetadata()that this variant doesn't cover — low risk since the compat path is primarily used during deployment.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Member
Author
|
Related work in That matters for this branch specifically: 3.9.x pins resolver 1.9.27, so it is #2092 rather than the master-line PRs that would eventually reach it. Until that pin moves, the check in this PR is the only one on the path. Worth a reviewer decision on whether the check should stay here once a resolver bump lands. This comment was created with AI assistance. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Coordinates and repository lists arriving from a resolved artifact descriptor are validated before use.
groupId/artifactId/versionare checked against the artifact-coordinate character set before aRelocatedArtifactis built; malformed components fail the descriptor read rather than entering resolution. A follow-up commit covers the remaining rejected character classes.maven-metadata.xmlare validated before being spliced into a resolved version, so malformed metadata fails the read rather than producing a malformed coordinate. The rejection message names the field.Each change is a separate commit.
Draft while related code paths are reviewed — the same validation may be needed in sibling implementations of these interfaces, and I would rather establish that before asking for review time.
A
systemPathchange was removed from this PR: it brokeMavenITmng4379TransitiveSystemPathInterpolatedWithEnvVarTest, which covers deliberate MNG-4379 behaviour — a dependency resolved from a repository declaring its ownsystem-scope dependency with the path interpolated from an environment variable.