Validate legacy metadata inputs and clone proxies before decryption - #12950
Validate legacy metadata inputs and clone proxies before decryption#12950slachiewicz wants to merge 4 commits into
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:
|
2 participants
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.