Validate coordinates and repository precedence during artifact resolution - #12952
Validate coordinates and repository precedence during artifact resolution#12952slachiewicz wants to merge 6 commits into
Conversation
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.
Relocation groupId/artifactId/version taken from a resolved model are checked against the artifact-coordinate character set before a RelocatedArtifact is built; invalid components fail the descriptor read instead of entering resolution.
Version tokens adopted from repository metadata are checked before use, in both the version resolver and the version range resolver. Metadata carrying a token that is not a valid coordinate component is treated as invalid, so resolution falls back to the requested base version and other repositories still resolve normally.
Applies the same version-token check used by the aether version and version-range resolvers to the maven-compat metadata read path: a token that is not a valid coordinate component is treated as invalid, and the read is skipped rather than merged in. Also carries the version-range resolver's own check onto this branch and adds a resolution test for it, since its sibling change on the aether side shipped without one. Claude-Session: https://claude.ai/code/session_011PdF7DSMbwHn1U9xMSrKN9
95b5890 to
e5a65b3
Compare
…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.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Solid security-hardening PR that adds correct validation of metadata tokens and relocation coordinates against path-traversal and coordinate-injection attacks. The 3.10.x adaptation is well done — validation is correctly placed in the legacy code paths (DefaultRepositoryMetadataManager, DefaultMetadataReader) appropriate for this branch.
Minor suggestions (non-blocking):
DefaultVersionResolverTest(line 105): A comment appears truncated mid-sentence ("so the") — the master variant has the full text: "so the metadata is treated as invalid and resolution falls back to the requested base version." Likely a copy-paste artifact during the backport.- The validation logic duplication across files is acceptable for a stabilized branch where minimizing cross-module changes is desirable.
The validation character set (rejecting /, \, :, ISO control chars, exact ..) is well-chosen for the threat model. Each resolver validates exactly the fields it consumes, which avoids unnecessary strictness. All CI checks pass.
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
|
Related work in The overlap is deliberate for now. This PR checks in Maven because the pinned resolver does not, and a Maven line only picks up the resolver-side change when its Worth a reviewer decision on whether the check should stay in both places once that pin lands, or move down to the resolver. This comment was created with AI assistance. |
2 participants
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.