Validate coordinates and repository precedence during artifact resolution - #12949
Validate coordinates and repository precedence during artifact resolution#12949slachiewicz wants to merge 5 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 and snapshot tokens adopted from downloaded repository metadata are checked before they are spliced into a resolved coordinate, in the version resolver and version range resolver (both the native resolver stack and the maven-compat resolver-provider path), and in relocation coordinates read from a resolved project's distribution management. 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.
Follow-up to the previous commit: the rejection message now names which field (latest version, release version, snapshot version, snapshot timestamp) carried the invalid token, in both the native resolver stack and the maven-compat resolver-provider path, matching the message shape used elsewhere for this class of check.
acd626d to
0807740
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Well-structured security-hardening PR that adds input validation for artifact coordinates and repository metadata tokens at trust boundaries. The validation rules are correct, the test coverage is thorough, and all CI checks pass. Three observations for consideration:
1. Validation logic duplication (medium)
The validateVersionToken method is duplicated across 4 files (compat and impl DefaultVersionRangeResolver and DefaultVersionResolver), and isInvalidCoordinateComponent is duplicated across 3+ files (MavenMetadataSource, compat and impl DistributionManagementArtifactRelocationSource). The pure boolean check could potentially be extracted to a shared utility (e.g., in maven-builder-support), though the compat/impl module split and differing exception types partially justify the duplication. Worth considering for future maintainability.
2. Control characters in error messages (low)
When validateVersionToken rejects a value containing control characters, the raw untrusted value is embedded verbatim in the exception message. If logged, embedded newlines/tabs could produce misleading multi-line log entries. Sanitizing the value (e.g., replacing control chars with Unicode escape sequences) would be a small improvement.
3. Silent repository precedence override (low)
DefaultModelResolver (line ~135) now silently returns when a model-declared repository conflicts with an external repository ID. There's a code comment explaining the rationale, but no debug-level log. Adding a LOGGER.debug(...) message would aid troubleshooting builds where repository precedence matters (note: requires adding a Logger to the class).
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Milestone | (none) | 4.1.0 |
Backport status: ✅ All maintenance branches covered (#12944 → 4.0.x, #12952 → 3.10.x, #12955 → 3.9.x)
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.