Validate metadata inputs across legacy and new API paths, clone proxies before decryption by slachiewicz · Pull Request #12945 · apache/maven

slachiewicz · 2026-08-30T18:20:32Z

Backport to maven-4.0.x of the metadata validation and proxy fixes from #12950 (master).

Legacy compat layer (`compat/`)

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.
Repository key and metadata tokens. The repository key is validated before it becomes part of a local file name, and version tokens from downloaded metadata are validated at the single point all metadata passes through, which also covers the latest/release/snapshot transformations.
Proxy decryption. DefaultSettingsDecrypter now 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`)

Shared validation utility. MetadataInputValidator consolidates coordinate-component and version-token validation: rejects .., /, \, :, and ISO control characters.
Version resolution. DefaultVersionResolver and DefaultVersionRangeResolver validate parsed Versioning elements from maven-metadata.xml before the values are used to compose filesystem paths or artifact coordinates.
Relocation validation. DistributionManagementArtifactRelocationSource validates relocation groupId, artifactId, and version before constructing a RelocatedArtifact.

Each change is a separate commit.

DefaultRepositoryMetadataManager.resolve() caught ChecksumFailedException through the generic TransferFailedException handler, so a checksum mismatch under checksumPolicy=fail only logged a warning and kept the previously cached metadata, and the finally block touched the update-check file regardless, deferring the next check for a full update interval. Catch ChecksumFailedException ahead of the generic handler and fail resolution instead, and only touch the update-check file on success, not-found, or a generic transfer failure, so a checksum failure is retried on the next build rather than cached.

…ches DefaultRepositoryMetadataManager.getArtifactMetadataFromDeploymentRepository and LegacyRepositorySystem.retrieve hardcoded checksumPolicy=warn for every transfer, so a repository configured with checksumPolicy=fail (or the global -C/--strict-checksums flag) was not honored on the deploy-metadata fetch or on generic retrieve() calls. Resolve the effective policy from the repository instead: the metadata fetch uses the same per-metadata policy resolution the download path already uses, and retrieve() takes the stricter of the repository's release and snapshot policies since a generic path cannot be classified as either.

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

Well-crafted security-hardening PR that correctly threads configured checksum policies through three legacy metadata paths, adds input validation for repository keys and metadata version tokens, and fixes a proxy-cloning inconsistency.

Highlights:

The checksum policy fix is well-reasoned: resolve() properly fails on ChecksumFailedException under the strict policy, getArtifactMetadataFromDeploymentRepository now uses the configured policy, and LegacyRepositorySystem.retrieve() takes the stricter of release/snapshot policies.
The catch ordering (ChecksumFailedException before TransferFailedException) is essential and correctly placed.
The proxy cloning fix in DefaultSettingsDecrypter correctly mirrors the existing server-cloning pattern.
validateVersioning provides comprehensive validation at the single metadata read entry point.
Test coverage is thorough across all changes.
Clean commit structure with descriptive messages.

Minor observations (non-blocking):

isInvalidPathToken / validateRepositoryKey are duplicated in LegacyLocalRepositoryManager and AbstractRepositoryMetadata — acceptable for deprecated compat code.
getArtifactMetadataFromDeploymentRepository still touches the update tracker in finally (even on checksum failures), unlike resolve() which deliberately skips it — minor inconsistency on the deployment path.

Consistent with the already-approved 3.10.x variant (#12954), well-adapted for the Maven 4 compat model.

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>

…solvers Add MetadataInputValidator shared utility to consolidate validation logic for the impl/maven-impl resolver layer. Wire validation into DefaultVersionResolver, DefaultVersionRangeResolver, and DistributionManagementArtifactRelocationSource to reject path-traversal sequences (..), separators (/, \), drive-letter delimiters (:), and ISO control characters in metadata version tokens and relocation coordinates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet

Review of new commit e1d9a852b9 ("Validate metadata inputs and relocation coordinates in the new API resolvers").

This commit is the 4.0.x equivalent of commit 5e1e17ee60 already reviewed on master (#12950). The code is byte-for-byte identical, which means two issues from the master review carry over, plus one 4.0.x-specific issue:

NUL bytes in test (medium): MetadataInputValidatorTest.java contains literal NUL bytes (0x00) on lines 73 and 100, causing git to treat the file as binary (+0 -0 in diffs). This makes the test completely invisible in code review. Use \0 escape or \u0000 instead of raw NUL.
Wrong @since tag (medium): MetadataInputValidator.java line 36 has @since 4.1.0, which was correct on master but was not adjusted for the 4.0.x branch (targeting 4.0.0-SNAPSHOT). Should be @since 4.0.0-rc-7 or the appropriate 4.0.x release version.
Code duplication (low): validateVersionToken reimplements the validation logic already in isInvalidCoordinateComponent instead of delegating to it.

📋 PR Metadata

Aspect	Current	Suggested
Milestone	(none)	`4.0.0-rc-7`

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

@SInCE

…e NUL bytes - Fix @SInCE 4.1.0 → 4.0.0-rc-7 (correct for the maven-4.0.x branch) - 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

All three findings from the previous review have been addressed correctly in commit b2433840f0:

✅ NUL bytes in test — Replaced with Java \0 escape sequences. The file is now clean text (155 lines), no longer treated as binary by git.
✅ Wrong @since tag — Fixed from @since 4.1.0 to @since 4.0.0, which is correct for the maven-4.0.x branch (latest tag: maven-4.0.0-rc-6).
✅ Code duplication — validateVersionToken now delegates to isInvalidCoordinateComponent(value) with a single if-statement, replacing the 7-line duplicated logic block. Behavior is equivalent.

The fix commit is clean and minimal — touches only the lines called out in the review, no unrelated changes.

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>

…taInputValidator Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…hs (#12978) * Validate the repository key used in legacy metadata file names AbstractRepositoryMetadata.getLocalFilename rejects a repository key that is the ".." token, contains '/', '\', ':', or an ISO control character, before using it in a local file name. LegacyLocalRepositoryManager.ArtifactMetadataAdapter applies the same validation. Backport of the corresponding fix from PR #12945 (maven-4.0.x). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Validate metadata version tokens and fail closed on checksum mismatches DefaultRepositoryMetadataManager.readMetadata applies validateVersioning to reject metadata carrying version tokens (latest, release, versions, snapshot versions, snapshot timestamp) that contain '..', '/', '\', ':', or ISO control characters. DefaultRepositoryMetadataManager.resolve now catches ChecksumFailedException ahead of the generic TransferFailedException handler. Under checksumPolicy=fail a checksum mismatch fails metadata resolution instead of only logging a warning. The update-check file is only touched on success, not-found, or a generic transfer failure, so a checksum failure is retried on the next build. Backport of the corresponding fixes from PR #12945 (maven-4.0.x). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Honour configured checksum policy on the legacy compat paths DefaultRepositoryMetadataManager.getArtifactMetadataFromDeploymentRepository and LegacyRepositorySystem.retrieve hardcoded checksumPolicy=warn for every transfer, so a repository configured with checksumPolicy=fail (or the global -C/--strict-checksums flag) was not honored on the deploy-metadata fetch or on generic retrieve() calls. Resolve the effective policy from the repository instead: the metadata fetch uses the same per-metadata policy resolution the download path already uses, and retrieve() takes the stricter of the repository's release and snapshot policies since a generic path cannot be classified as either. Backport of the corresponding fix from PR #12945 (maven-4.0.x). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

…es before decryption (#12950) * Validate repository key and metadata version tokens on the legacy path 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. * Clone proxy before decrypting its password 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. * Validate the repository key used in legacy metadata file names * Honour configured checksum policy on the legacy compat paths 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 #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> * Validate metadata inputs and relocation coordinates in the new API resolvers 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> * Address review: simplify validateVersionToken, replace NUL bytes in test - 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> --------- Co-authored-by: Guillaume Nodet <gnodet@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

slachiewicz added 4 commits August 30, 2026 19:54

slachiewicz added the bug Something isn't working label Aug 30, 2026

slachiewicz added this to the 4.0.0-rc-7 milestone Aug 30, 2026

slachiewicz marked this pull request as draft August 30, 2026 19:05

slachiewicz requested review from cstamas, gnodet and slawekjaranowski August 30, 2026 19:06

Validate the repository key used in legacy metadata file names

9d03e0f

slachiewicz marked this pull request as ready for review August 30, 2026 21:49

gnodet approved these changes Aug 30, 2026

View reviewed changes

gnodet mentioned this pull request Aug 30, 2026

Validate metadata inputs across legacy and new API paths, clone proxies before decryption #12950

Merged

gnodet mentioned this pull request Aug 31, 2026

Honour configured policy and validate inputs on the legacy compat paths #12978

Merged

7 tasks

gnodet changed the title ~~Honour configured policy and validate inputs on the legacy compat paths~~ Validate metadata inputs across legacy and new API paths, clone proxies before decryption Sep 1, 2026

gnodet reviewed Sep 1, 2026

View reviewed changes

Comment thread impl/maven-impl/src/main/java/org/apache/maven/impl/resolver/MetadataInputValidator.java Outdated

gnodet mentioned this pull request Sep 1, 2026

Validate metadata inputs and honour checksum policy on legacy paths #13002

Merged

7 tasks

gnodet force-pushed the pr/legacy-compat-4.0.x branch from 67e753f to b243384 Compare September 1, 2026 13:35

gnodet approved these changes Sep 1, 2026

View reviewed changes

Merge maven-4.0.x after Set 3 merge: resolve conflicts keeping Metada…

9729066

…taInputValidator Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gnodet merged commit 25b3b04 into apache:maven-4.0.x Sep 1, 2026
1 check failed

github-actions Bot assigned gnodet Sep 1, 2026

Navigation Menu

Uh oh!

Validate metadata inputs across legacy and new API paths, clone proxies before decryption - #12945

Validate metadata inputs across legacy and new API paths, clone proxies before decryption#12945
gnodet merged 8 commits into
apache:maven-4.0.xfrom
slachiewicz:pr/legacy-compat-4.0.x

slachiewicz commented Aug 30, 2026 •

edited by gnodet

Loading

Uh oh!

gnodet left a comment

Uh oh!

gnodet left a comment

Uh oh!

Uh oh!

gnodet left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

slachiewicz commented Aug 30, 2026 • edited by gnodet Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Legacy compat layer (compat/)

New API resolver layer (impl/maven-impl)

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

📋 PR Metadata

Uh oh!

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

slachiewicz commented Aug 30, 2026 •

edited by gnodet

Loading

Legacy compat layer (`compat/`)

New API resolver layer (`impl/maven-impl`)