Honour configured policy and validate inputs on the legacy compat paths - #12945
Open
slachiewicz wants to merge 5 commits into
Open
Honour configured policy and validate inputs on the legacy compat paths#12945slachiewicz wants to merge 5 commits into
slachiewicz wants to merge 5 commits into
Conversation
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.
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-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 onChecksumFailedExceptionunder the strict policy,getArtifactMetadataFromDeploymentRepositorynow uses the configured policy, andLegacyRepositorySystem.retrieve()takes the stricter of release/snapshot policies. - The catch ordering (
ChecksumFailedExceptionbeforeTransferFailedException) is essential and correctly placed. - The proxy cloning fix in
DefaultSettingsDecryptercorrectly mirrors the existing server-cloning pattern. validateVersioningprovides 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/validateRepositoryKeyare duplicated inLegacyLocalRepositoryManagerandAbstractRepositoryMetadata— acceptable for deprecated compat code.getArtifactMetadataFromDeploymentRepositorystill touches the update tracker infinally(even on checksum failures), unlikeresolve()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
gnodet
added a commit
to slachiewicz/maven
that referenced
this pull request
Aug 31, 2026
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>
7 tasks
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.
Fixes on the legacy compatibility paths.
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.Each change is a separate commit.
Draft while related code paths are reviewed — the same guard may be needed in sibling implementations of these interfaces, and I would rather establish that before asking for review time.