Validate metadata inputs across legacy and new API paths, clone proxies before decryption - #12950
Conversation
AbstractRepositoryMetadata.getLocalFilename rejects a repository key that is the ".." token, contains '/', '\', ':', or an ISO control character, before using it in a local file name. DefaultRepositoryMetadataManager.readMetadata applies the same check to every version token carried by parsed repository metadata (latest, release, versions, snapshot versions, snapshot timestamp) before the metadata is merged and used to resolve a version. Matches the check already used for relocation and version-range coordinates elsewhere in the resolver.
The proxy loop in DefaultSettingsDecrypter.decrypt mutated the caller's live Proxy objects directly, unlike the server loop just above it which already clones before mutating. Since compat model objects propagate setter changes up into their parent Settings delegate, this meant a decrypted proxy password could end up written back into the session-wide Settings object. Clone the proxy first, mirroring the server handling, so decryption only ever touches the copies returned in the result.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The repository key validation and proxy clone-before-decrypt changes are correct, well-tested, and consistent with the approved 4.0.x (#12945) and 3.10.x (#12954) variants.
However, this PR appears to be missing two checksum-policy commits that are present in the 4.0.x variant (#12945):
1. Description/content mismatch (medium)
The PR description states "Checksum policy. Three legacy metadata paths ignored the configured checksum policy and defaulted to a warning regardless of a fail setting; the configured policy is now threaded through and honoured." — but the diff does not include the commits that implement this:
DefaultRepositoryMetadataManager.getArtifactMetadataFromDeploymentRepository()(line 358) still hardcodesCHECKSUM_POLICY_WARNDefaultRepositoryMetadataManager.resolve()lacks aChecksumFailedExceptioncatch clauseLegacyRepositorySystem.retrieve()(line 663) still hardcodesCHECKSUM_POLICY_WARN
The approved 4.0.x PR (#12945) has 5 commits; this master PR has only 3 — the two missing are "Fail closed on metadata checksum mismatches under the fail policy" and "Thread the configured checksum policy through two legacy metadata fetches".
Suggestion: Either cherry-pick the two missing checksum commits into this PR, or update the PR description to remove the checksum bullet.
2. Minor: code duplication (low)
isInvalidPathToken() / validateRepositoryKey() are duplicated in LegacyLocalRepositoryManager and AbstractRepositoryMetadata — acceptable for deprecated compat code.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Milestone | (none) | 4.1.0 |
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
The legacy metadata manager and repository system hardcoded CHECKSUM_POLICY_WARN, silently ignoring the operator's --strict-checksums / -C flag and per-repository checksumPolicy settings. This brings the master branch in line with the maven-4.0.x fix (PR apache#12945): - Catch ChecksumFailedException in resolve() and propagate it as a RepositoryMetadataResolutionException under CHECKSUM_POLICY_FAIL - Move updateCheckManager.touch() out of the finally block so that failed transfers do not suppress retries for a full update interval - Use the repository's configured checksum policy for deployment metadata retrieval instead of hardcoded WARN - Pick the stricter of release/snapshot policies in LegacyRepositorySystem.retrieve() instead of hardcoded WARN Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Delta re-review — Previous finding resolved. The new commit 6ff4b8a cleanly addresses the missing checksum-policy issue.
Previous finding resolved:
✅ Checksum policy now honoured on all three legacy paths:
DefaultRepositoryMetadataManager.getArtifactMetadataFromDeploymentRepository()— no longer hardcodesCHECKSUM_POLICY_WARNDefaultRepositoryMetadataManager.resolve()—ChecksumFailedExceptioncatch clause correctly placed before theTransferFailedExceptioncatch (important sinceChecksumFailedException extends TransferFailedException)LegacyRepositorySystem.retrieve()— usesgetChecksumPolicy()which returns the stricter of releases/snapshots policies via a rank helper
Design observations (non-blocking):
updateCheckManager.touch()wisely restructured: moved fromfinallyto explicit success/error paths, deliberately omitting it fromChecksumFailedExceptionso the next build retries immediately instead of trusting stale metadata- The "stricter of releases/snapshots" approach in
LegacyRepositorySystem.getChecksumPolicy()is the right choice for a generic path that can't be classified - Two focused tests verify the strict-fail behaviour
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Milestone | (none) | 4.1.0 |
This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
Update on cross-branch coverage: The original review on this PR incorrectly referenced #12954 as a related variant. PR #12954 is actually a different set of fixes (credential scoping, relocation validation, exact server ID matching) — not a port of this PR. The correct cross-branch map for the fixes in this PR (path traversal validation + checksum policy + proxy clone) is:
Separately, the fixes from #12954 (credential scoping + relocation validation + server ID matching) have been forward-ported:
|
…solvers Add MetadataInputValidator, a shared utility in impl/maven-impl that rejects version tokens, snapshot timestamps, and relocation coordinates containing path-traversal sequences (..), separators (/, \, :), or ISO control characters. Wire the validator into the new-API implementations: - DefaultVersionResolver.readVersions() validates parsed Versioning - DefaultVersionRangeResolver.readVersions() validates parsed Versioning - DistributionManagementArtifactRelocationSource validates relocation groupId, artifactId, and version before applying This mirrors the compat-layer guards already present in this PR and eliminates the gap where maven-metadata.xml content bypasses the model validator entirely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Delta re-review — New commit 5e1e17ee extends the validation to the new API resolvers, addressing the gap we flagged in sibling #12976. The MetadataInputValidator utility and its wiring into both version resolvers and the relocation source are sound.
Two observations:
1. [medium] Test file invisible in diffs due to literal NUL bytes
MetadataInputValidatorTest.java contains 2 literal NUL bytes (as control-character test data), causing git to classify it as binary (Binary files /dev/null and b/... differ). The entire 140+ lines of test coverage are invisible in the PR diff. Replacing the raw NUL bytes with Java \0 escape sequences (e.g., "abc\0def" instead of a literal NUL) would preserve test semantics while making the file diffable.
2. [low] validateVersionToken duplicates isInvalidCoordinateComponent logic
validateVersionToken (line ~90) reimplements the exact same null/empty, .., /, \, :, and ISO control char checks that isInvalidCoordinateComponent (lines 47-59) already performs. Could simplify to:
public static void validateVersionToken(String value, String description) throws IOException {
if (isInvalidCoordinateComponent(value)) {
throw new IOException("Rejecting metadata with invalid " + description + " '" + value + "'");
}
}What looks good:
- Extracting a shared
MetadataInputValidatorutility is cleaner than the compat-layer's inlined validation - Fail-fast pattern in both version resolvers (validate before assigning) is correct
DistributionManagementArtifactRelocationSourceTestprovides good coverage of relocation validation- Correctly not validating
SnapshotVersion.extension/classifier(used only for lookup-key matching, not filesystem paths)
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Milestone | (none) | 4.1.0 |
This review was generated by an AI agent (Claude Code) and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
gnodet
changed the title
- validateVersionToken now delegates to isInvalidCoordinateComponent instead of reimplementing the same logic - Replace raw NUL bytes in MetadataInputValidatorTest with \0 escape sequences so git treats the file as text and diffs are visible Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Delta re-review — Previous findings addressed.
✅ NUL bytes in test file — Literal NUL bytes replaced with Java \0 escape sequences. File now renders as text in future diffs.
✅ validateVersionToken duplication — Method body simplified to delegate to isInvalidCoordinateComponent, eliminating the duplicated null/empty + path-traversal + control-char logic. Runtime behavior preserved.
Clean follow-up commit with no new issues introduced.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Milestone | (none) | 4.1.0 |
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
…policy (#13002) Backport of the path traversal validation and checksum policy fixes from #12950 (master) / #12945 (maven-4.0.x) / #12978 (maven-3.10.x) to the maven-3.9.x branch. - Path traversal: AbstractRepositoryMetadata.getLocalFilename() and LegacyLocalRepositoryManager.ArtifactMetadataAdapter.getLocalFilename() now reject repository keys containing '..', '/', '\', ':', or ISO control characters. - Metadata token validation: DefaultRepositoryMetadataManager.readMetadata() validates every version token (latest, release, versions, snapshot versions, snapshot timestamp) for the same characters after parsing. - Checksum policy: resolve() now catches ChecksumFailedException separately and fails metadata resolution under checksumPolicy=fail. getArtifactMetadataFromDeploymentRepository() and LegacyRepositorySystem.retrieve() resolve the effective policy from the repository configuration instead of hardcoding 'warn'. The proxy clone fix (DefaultSettingsDecrypter) is not included because 3.9.x already clones each proxy before decryption. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…utValidator Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Fixes on both the legacy compatibility paths and the new API resolver layer.
Legacy compat layer (
compat/)failsetting; the configured policy is now threaded through and honoured.DefaultSettingsDecrypternow clones each proxy before setting its decrypted password, mirroring the existing behaviour for servers, so decryption no longer mutates the caller's settings objects. Maven 3 already clones here.New API resolver layer (
impl/maven-impl)MetadataInputValidatorconsolidates coordinate-component and version-token validation: rejects..,/,\,:, and ISO control characters.DefaultVersionResolverandDefaultVersionRangeResolvervalidate parsedVersioningelements frommaven-metadata.xmlbefore the values are used to compose filesystem paths or artifact coordinates.DistributionManagementArtifactRelocationSourcevalidates relocationgroupId,artifactId, andversionbefore constructing aRelocatedArtifact.Each change is a separate commit.