Copilot commented on code in PR #959:
URL: https://github.com/apache/fesod/pull/959#discussion_r3609619218
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/WorkBookUtil.java:
##########
@@ -93,12 +93,8 @@ public static void createWorkBook(WriteWorkbookHolder
writeWorkbookHolder) throw
writeWorkbookHolder.setCachedWorkbook(hssfWorkbook);
writeWorkbookHolder.setWorkbook(hssfWorkbook);
if (writeWorkbookHolder.getPassword() != null) {
- try {
-
Biff8EncryptionKey.setCurrentUserPassword(writeWorkbookHolder.getPassword());
-
hssfWorkbook.writeProtectWorkbook(writeWorkbookHolder.getPassword(),
StringUtils.EMPTY);
- } finally {
- Biff8EncryptionKey.setCurrentUserPassword(null);
- }
+
Biff8EncryptionKey.setCurrentUserPassword(writeWorkbookHolder.getPassword());
+
hssfWorkbook.writeProtectWorkbook(writeWorkbookHolder.getPassword(),
StringUtils.EMPTY);
}
Review Comment:
The new behavior relies on WriteContextImpl.finish() to clear the BIFF8
encryption ThreadLocal, but this block currently sets the password whenever it
is non-null (including the empty string). WriteContextImpl.clearEncrypt03()
returns early for empty passwords, so an empty password would leave
Biff8EncryptionKey set and leak across subsequent writes on the same thread.
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/EncryptDataTest.java:
##########
@@ -104,4 +105,37 @@ private void readAndWrite(
Assertions.assertEquals(10, dataList.size());
Assertions.assertNotNull(dataList.get(0).getName());
}
+
+ /**
+ * Verifies that an XLS file written with a password is actually encrypted
at the BIFF8 record level,
+ * not merely flagged as write-protected. Without the correct password,
reading the file content
+ * must fail.
+ */
+ @Test
+ void xlsPasswordWrite_isActuallyEncrypted() throws Exception {
+ File file = createTempFile("enc-verify", ExcelFormat.XLS);
+
+ // Write an encrypted XLS file
+ FesodSheet.write(file, SimpleData.class)
+ .excelType(ExcelTypeEnum.XLS)
+ .password(PASSWORD)
+ .sheet()
+ .doWrite(TestDataBuilder.simpleData(10));
+
+ // Reading without the password must fail because the content is
BIFF8-encrypted
+ Assertions.assertThrows(
+ Exception.class,
+ () -> FesodSheet.read(file, SimpleData.class, new
CollectingReadListener<SimpleData>())
+ .excelType(ExcelTypeEnum.XLS)
+ .sheet()
+ .doReadSync());
Review Comment:
assertThrows(Exception.class, ...) is very broad and can make this test pass
for unrelated failures (e.g., parsing regressions). Since the intent is to
verify encryption behavior, assert a more specific failure type such as POI's
EncryptedDocumentException when reading the XLS without a password.
--
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]
