Copilot commented on code in PR #12165:
URL: https://github.com/apache/cloudstack/pull/12165#discussion_r2695235597
##########
utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java:
##########
@@ -142,4 +142,19 @@ public static String calculateChecksum(File file) {
throw new CloudRuntimeException(errMsg, e);
}
}
+
+ public static String prependAlgorithm(String checksum) {
+ if (StringUtils.isEmpty(checksum)) {
+ return checksum;
+ }
+ int checksumLength = checksum.length();
+ Map<String, Integer> paddingLengths = getChecksumLengthsMap();
+ for (Map.Entry<String, Integer> entry : paddingLengths.entrySet()) {
+ if (entry.getValue().equals(checksumLength)) {
+ String algorithm = entry.getKey();
+ return String.format("{%s}%s", algorithm, checksum);
+ }
+ }
+ return checksum;
+ }
Review Comment:
The `prependAlgorithm` method doesn't check if the algorithm prefix is
already present before prepending, which could result in double-prefixing
(e.g., `{SHA-512}{SHA-512}checksum`). Add a check using the existing
`isAlgorithmPresent()` method at the start of the function to return the
checksum unchanged if it already has the prefix.
##########
utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java:
##########
@@ -142,4 +142,19 @@ public static String calculateChecksum(File file) {
throw new CloudRuntimeException(errMsg, e);
}
}
+
+ public static String prependAlgorithm(String checksum) {
+ if (StringUtils.isEmpty(checksum)) {
+ return checksum;
+ }
+ int checksumLength = checksum.length();
+ Map<String, Integer> paddingLengths = getChecksumLengthsMap();
+ for (Map.Entry<String, Integer> entry : paddingLengths.entrySet()) {
+ if (entry.getValue().equals(checksumLength)) {
+ String algorithm = entry.getKey();
+ return String.format("{%s}%s", algorithm, checksum);
+ }
+ }
Review Comment:
When multiple hash algorithms have different lengths but one checksum length
could theoretically match multiple entries in the map, the iteration order is
non-deterministic since `getChecksumLengthsMap()` returns a `HashMap`. While
the current set of algorithms have unique lengths, this creates ambiguity.
Consider using a `LinkedHashMap` with algorithms ordered by preference (most
secure first), or add a comment documenting that all supported algorithms have
unique digest lengths.
```suggestion
String selectedAlgorithm = null;
// In case multiple algorithms share the same digest length, choose
deterministically
for (Map.Entry<String, Integer> entry : paddingLengths.entrySet()) {
if (entry.getValue().equals(checksumLength)) {
String algorithm = entry.getKey();
if (selectedAlgorithm == null ||
algorithm.compareTo(selectedAlgorithm) < 0) {
selectedAlgorithm = algorithm;
}
}
}
if (selectedAlgorithm != null) {
return String.format("{%s}%s", selectedAlgorithm, checksum);
}
```
##########
utils/src/main/java/org/apache/cloudstack/utils/security/DigestHelper.java:
##########
@@ -142,4 +142,19 @@ public static String calculateChecksum(File file) {
throw new CloudRuntimeException(errMsg, e);
}
}
+
+ public static String prependAlgorithm(String checksum) {
Review Comment:
The new `prependAlgorithm` method lacks test coverage. Add test cases
covering: (1) checksum without prefix for each supported algorithm, (2)
checksum that already has a prefix, (3) null/empty checksum, (4) checksum with
invalid length that doesn't match any algorithm.
--
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]
