gnodet commented on PR #2084:
URL: https://github.com/apache/maven-resolver/pull/2084#issuecomment-5476002866
@cstamas — thanks for the detailed review, both points are important. Let me
clarify the scope of the change, because the implementation already splits the
behaviour along exactly the line you're drawing:
### PROVIDED / REMOTE_INCLUDED checksums (`validateChecksums()`)
These are checksums that are **already in hand** — no HTTP roundtrips
involved, just iterating over an in-memory map. Here the change is intentional:
if a `TrustedChecksumsSource` provides both SHA-512 and MD5 for an artifact,
and the SHA-512 mismatches but MD5 matches, the old code might accept on the
MD5 match first (depending on map iteration order), silently masking the
SHA-512 mismatch. The new code checks **all comparable checksums** and rejects
if any one mismatches. This is purely in-memory — zero extra cost.
### REMOTE_EXTERNAL checksums (`validateExternalChecksums()`)
This is where your roundtrip concern applies — and the code **already
preserves the lazy early-return**. The only change is adding a `&& !rejected`
guard:
```java
// Old:
} else if (checksumPolicy.onChecksumMatch(factory.getName(),
REMOTE_EXTERNAL)) {
return true; // early accept on first match
}
// New:
} else if (checksumPolicy.onChecksumMatch(factory.getName(), REMOTE_EXTERNAL)
&& !rejected) {
return true; // still early accept — unless a prior algorithm already
mismatched
}
```
**Happy path is identical**: SHA-1 matches → early return, no extra fetches.
The guard only kicks in when a **prior stronger checksum already mismatched** —
which is either an attack or corruption. In that case the old code would have
continued iterating too (it doesn't return on mismatch), but would then accept
on the next weaker match — that's the security gap this fixes.
### Regarding the "optional checksums" idea
You're right that a more general solution would be marking individual
algorithms as optional vs. required, which would cleanly handle mixed repos
like Central. That's a worthwhile enhancement but orthogonal to this fix — it
would be a separate feature PR with its own configuration surface (e.g.
`aether.checksums.optionalAlgorithms`). This PR only tightens the existing
logic: "if you have a checksum comparison and it mismatches, don't let a weaker
match override it."
tl;dr — **no extra roundtrips** in the normal case, and the documented
first-match-wins contract for remote external checksums is preserved. The only
behavioral change is: a mismatch on a stronger algorithm can no longer be
masked by a match on a weaker one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
