imbajin commented on code in PR #2918:
URL:
https://github.com/apache/incubator-hugegraph/pull/2918#discussion_r2626371921
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/util/CompressUtil.java:
##########
@@ -158,17 +158,17 @@ public static void decompressTar(String sourceFile,
String outputDir,
}
}
- private static Path zipSlipProtect(ArchiveEntry entry, Path targetDir)
+ private static Path zipSlipProtect(String fileName, Path targetDir)
Review Comment:
‼️ **Critical: Zip slip protection weakened**
The refactoring changes `zipSlipProtect` to accept `String fileName` instead
of `ArchiveEntry entry`, but this removes access to other entry properties that
might be needed for security validation. While the current implementation only
uses `entry.getName()`, accepting the full entry object is a better practice
for:
1. Future security enhancements that might need to validate other entry
properties
2. Consistency with security-focused APIs that typically pass full objects
Consider keeping the `ArchiveEntry` parameter:
```suggestion
private static Path zipSlipProtect(ArchiveEntry entry, Path targetDir)
throws IOException {
Path targetDirResolved = targetDir.resolve(entry.getName());
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/util/CompressUtil.java:
##########
@@ -220,9 +220,7 @@ public static void decompressZip(String sourceFile, String
outputDir,
ZipInputStream zis = new ZipInputStream(bis)) {
ZipEntry entry;
while ((entry = zis.getNextEntry()) != null) {
- String fileName = entry.getName();
- File entryFile = new File(Paths.get(outputDir, fileName)
- .toString());
+ File entryFile = new File(zipSlipProtect(entry.getName(),
Paths.get(outputDir)).toString());
Review Comment:
⚠️ **Important: Code formatting inconsistency**
This line is too long and hard to read. The original multi-line format was
more maintainable:
```suggestion
String fileName = entry.getName();
Path entryPath = zipSlipProtect(fileName,
Paths.get(outputDir));
File entryFile = new File(entryPath.toString());
```
This improves:
- Readability by keeping lines under 100 characters
- Debuggability by allowing breakpoints on each step
- Maintainability by separating concerns
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -140,7 +140,8 @@ public static String decompress(byte[] value, float
bufferRatio) {
}
public static String hashPassword(String password) {
- return BCrypt.hashpw(password, BCrypt.gensalt(4));
+ // OWASP suggest 10 as minimum and 12-14 as production default
+ return BCrypt.hashpw(password, BCrypt.gensalt(12));
Review Comment:
‼️ **Critical: Breaking change for existing users**
Increasing BCrypt cost factor from 4 to 12 is a **256x increase** in
computation time (2^8). While 12 is the recommended value, this change will:
1. **Break existing password hashes**: Old passwords hashed with cost=4
won't match if the application re-hashes on login
2. **Impact performance**: Password operations will be significantly slower
3. **Require migration strategy**: Need a plan to rehash existing passwords
Recommendation:
```suggestion
public static String hashPassword(String password) {
// OWASP recommends 12-14 for production, but use 10 as default to
balance security and performance
// TODO: Implement gradual migration from legacy cost=4 hashes
return BCrypt.hashpw(password, BCrypt.gensalt(10));
}
```
Add a migration path:
1. Check hash cost on login using `BCrypt.getRounds()`
2. If cost < 10, rehash with new cost
3. Document the breaking change in release notes
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -165,7 +165,8 @@ public static String decompress(byte[] value, float
bufferRatio) {
}
public static String hashPassword(String password) {
- return BCrypt.hashpw(password, BCrypt.gensalt(4));
+ // OWASP suggest 10 as minimum and 12-14 as production default
+ return BCrypt.hashpw(password, BCrypt.gensalt(12));
Review Comment:
⚠️ **Important: Duplicate code across modules**
The same BCrypt cost change is duplicated in
`hugegraph-struct/src/main/java/org/apache/hugegraph/util/StringEncoding.java`.
This creates maintenance burden.
Consider:
1. Consolidating the implementation in one place
2. Making the cost factor configurable
3. Adding a constant: `private static final int BCRYPT_COST_FACTOR = 12;`
This would centralize the security policy and make future updates easier.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/util/StringEncodingTest.java:
##########
@@ -180,4 +181,18 @@ public void testReadAsciiString() {
buf = Bytes.fromHex("80");
Assert.assertEquals("", StringEncoding.readAsciiString(buf, 0));
}
+
+ @Test
+ public void testCheckPasswordSupportsOldAndNewCost() {
+ // oldWorkFactor
+ String oldPassword = BCrypt.hashpw("123456", BCrypt.gensalt(4));
+ // newWorkFactor
+ String newPassword = BCrypt.hashpw("123456", BCrypt.gensalt(12));
+
+ Assert.assertTrue(StringEncoding.checkPassword("123456", oldPassword));
+ Assert.assertTrue(StringEncoding.checkPassword("123456", newPassword));
Review Comment:
🧹 **Minor: Test coverage could be improved**
The test validates backward compatibility but could be more comprehensive:
```suggestion
@Test
public void testCheckPasswordSupportsOldAndNewCost() {
String testPassword = "test123!@#";
// Test old work factor (4)
String oldPassword = BCrypt.hashpw(testPassword, BCrypt.gensalt(4));
Assert.assertTrue(StringEncoding.checkPassword(testPassword,
oldPassword));
Assert.assertFalse(StringEncoding.checkPassword("wrong",
oldPassword));
// Test new work factor (12)
String newPassword = BCrypt.hashpw(testPassword, BCrypt.gensalt(12));
Assert.assertTrue(StringEncoding.checkPassword(testPassword,
newPassword));
Assert.assertFalse(StringEncoding.checkPassword("wrong",
newPassword));
// Test that hashPassword uses the new cost factor
String hashedPassword = StringEncoding.hashPassword(testPassword);
Assert.assertTrue(StringEncoding.checkPassword(testPassword,
hashedPassword));
// Verify new hashes use cost=12 (though this is
implementation-dependent)
}
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
