Repository navigation
Conversation
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
🔇 Additional comments: 6
src/xlsx/crypto/cfb.ts: 4
279-301: LGTM!
317-317: LGTM!
400-403: LGTM!
421-438: LGTM!test/encryption.test.ts: 2
126-141: LGTM!
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Use the
Uint8Arrayview bounds in the header check.
new DataView(file.buffer)reads the whole underlying buffer, not thefileview.writeCfballocates its own buffer, so this works now. The check breaks ifwriteCfblater returns a subarray. Passfile.byteOffsetandfile.byteLengthto keep the test robust.Proposed fix
- expect(new DataView(file.buffer).getUint32(72, true)).toBe(difatSectors) + expect(new DataView(file.buffer, file.byteOffset, file.byteLength).getUint32(72, true)).toBe( + difatSectors, + )
ℹ️ Review info
⚙️ Run configuration
- Configuration used: defaults
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0285bb39-b64b-4859-b342-8593be6af3e6
📒 Files selected for processing: 2
src/xlsx/crypto/cfb.tstest/encryption.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
A password-protected workbook over about 7 MB is written broken.
writeXlsxwithencryptionproduced an 8.6 MB container (80,000 cells of random text), andreadXlsx(enc, { password })on that same file failed withDecryptionError: Not a valid encrypted workbook container.The cause iswriteCfb: a CFB header holds only 109 FAT sector locations, which map 109 × 128 sectors of 512 bytes, just under 7 MB. A larger container must list the rest of its FAT in DIFAT sectors, and the writer never wrote any: it set "number of DIFAT sectors" to 0 and stopped at the header's 109. Every sector past the first ~7 MB was unreachable, so the 8 MB and 16 MB streams the new tests write both read back at exactly 7,143,936 bytes.The tests did not catch it because the largest stream any of them wrote was 20 KB.
readCfbalready follows DIFAT chains; it just never met a container fromwriteCfbthat had one.What landed:
writeCfbsizes the FAT and DIFAT together, since the FAT must also cover the DIFAT sectors, which it marksDIFSECT. It writes the DIFAT sectors after the FAT: 127 FAT sector locations each, then the next DIFAT sector's location,ENDOFCHAINon the last andFREESECTpadding. The header now points at the first one (offset 68) and gives the count (offset 72). A container that fits in 109 FAT sectors is written exactly as before.Tests: two
writeCfb→readCfbround trips that need one and two chained DIFAT sectors (8 and 16 MB), checking every byte and the header's DIFAT count. Plus awriteXlsx→readXlsxround trip of an encrypted workbook whose package is over 7 MB (~1 s). All three fail againstmain: the first two read back 7,143,936 bytes, and the third throws theDecryptionErrorabove.bun run testis green.Beyond hucre's own reader, I checked the containers with a separate stdlib-only Python parser written to [MS-CFB] at 1, 8, 16 and 24 MiB (0 to 3 DIFAT sectors). The header and DIFAT chain agree, FAT and DIFAT sectors carry their markers, and the stream bytes are identical.
Not verified: I did not open a large encrypted workbook in Excel or LibreOffice, so the claim rests on the spec, hucre's own reader and the separate parser. Only 512-byte sectors (version 3) are written, as before.
Summary by CodeRabbit
Bug Fixes
Tests