fix: XLS BIFF8 encryption not applied due to premature password clearing#959
Open
alaahong wants to merge 4 commits into
Open
fix: XLS BIFF8 encryption not applied due to premature password clearing#959alaahong wants to merge 4 commits into
alaahong wants to merge 4 commits into
Conversation
The refactor in 7fe23f0 added a finally block in WorkBookUtil.createWorkBook() that clears the Biff8EncryptionKey ThreadLocal immediately after setting it. However, workbook.write() (which applies BIFF8 encryption) runs later in WriteContextImpl.finish(). At that point the password is already null, so the file content is never encrypted - only a write-protection flag is set. Fix: Remove the premature clearing. The password is correctly cleared by WriteContextImpl.clearEncrypt03() after workbook.write() completes. Tests: - Update WorkBookUtilTest to verify password remains set after createWorkBook() - Add EncryptDataTest.xlsPasswordWrite_isActuallyEncrypted to verify that reading an encrypted XLS without a password fails
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes BIFF8 (XLS) content encryption by ensuring the Biff8EncryptionKey ThreadLocal remains set until workbook.write() runs (later in WriteContextImpl.finish()), and adds tests to prevent regressions.
Changes:
- Stop clearing
Biff8EncryptionKeyprematurely inWorkBookUtil.createWorkBook()for XLS password-protected writes. - Update
WorkBookUtilTestto assert the BIFF8 password remains set after workbook creation. - Add an integration-style test to verify an XLS written with a password cannot be read without the password.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| fesod-sheet/src/main/java/org/apache/fesod/sheet/util/WorkBookUtil.java | Keeps BIFF8 encryption password set during XLS write setup (so encryption can apply during workbook.write()). |
| fesod-sheet/src/test/java/org/apache/fesod/sheet/util/WorkBookUtilTest.java | Updates unit expectation to ensure the BIFF8 password remains available after createWorkBook(). |
| fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/EncryptDataTest.java | Adds a regression test that validates XLS content is actually encrypted (read fails without password). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge the assertThrows lambda argument onto a single line to comply with the project Spotless formatting rules.
- WorkBookUtil: use !StringUtils.isEmpty() instead of != null to prevent empty password ThreadLocal leak (clearEncrypt03 returns early for empty passwords) - EncryptDataTest: use EncryptedDocumentException instead of Exception for more precise assertion of encryption failure
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of the pull request
The refactor in 7fe23f0 added a finally block in WorkBookUtil.createWorkBook() that clears the Biff8EncryptionKey ThreadLocal immediately after setting it. However, workbook.write() (which applies BIFF8 encryption) runs later in WriteContextImpl.finish(). At that point the password is already null, so the file content is never encrypted - only a write-protection flag is set.
Below two files, XLSX can protect the content, but xls only provide write permission protection.
sample-xls-password-buggy.xls
sample-xlsx-password-encrypted.xlsx
Since code fixed, it also protect the content as below
What's changed?
Fix: Remove the premature clearing. The password is correctly cleared by WriteContextImpl.clearEncrypt03() after workbook.write() completes.
Tests:
Checklist