XSSF: validate row/column/cell index before mutating the sheet XML - #3
Merged
ken-swyfft merged 9 commits intoSep 9, 2026
Conversation
XSSFSheet.CreateRow adds the row element to sheetData before XSSFRow's RowNum setter validates the index. When the setter rejects the index the element is already in the XML, and because _rows is only updated after validation the row is orphaned: invisible to GetRow and PhysicalNumberOfRows, but still written to the file. The result is a workbook Excel refuses to open. Each leaked row carries the same r attribute — XSSFRow's constructor fills in LastRowNum + 2 for an unset r before the setter throws — so the rows are duplicated and precede the real ones, breaking the required ascending order. Excel reports "We found a problem with some content" and strips the sheet's data when repairing it. A caller only has to probe a name that doesn't resolve and catch the exception, which is what "does this optional cell exist?" code does, for the saved file to be corrupt. XSSFSheet.CreateColumn and XSSFRow.CreateCell have the same mutate-then-validate shape. CreateColumn leaves a col definition for a column that was never created; the orphaned cell does not reach the file today only because XSSFRow.OnDocumentWrite rebuilds the row's cell array when the counts disagree. Hoist the checks so nothing is written until the index is known good. The peers already do it this way — SXSSFSheet.CreateRow, SXSSFRow .CreateCell, XSSFColumn.CreateCell and XSSFRow.GetCell all validate first, as does HSSFSheet.CreateRow — so this makes XSSF consistent with the rest of the library. Exception types and messages are unchanged.
NuGet audit fails restore on the 10.0.6 pins — five high-severity advisories against System.Security.Cryptography.Xml, promoted to errors by Warning As Error in NPOI.Benchmarks, which takes down restore for the whole test solution and so every CI run. Bumping the crypto package alone trips NU1109 because System.Formats.Asn1 is pinned at 10.0.6 and Microsoft.Bcl.Cryptography 10.0.10 wants 10.0.10, so both move together. Restore is clean with the audit enabled and the full test solution passes on net10.0 (2803 + 1868 + 79).
…xception type
- GetWrittenSheetXml opens the package read-only, disposes the stream and
drops RegexOptions.Compiled from a single-use regex.
- Match the file's 'for (' spacing.
- XSSFCell.CheckBounds documented as throwing ArgumentException, not
RuntimeException (a Java leftover on the line this patch touched).
Folds in the NuGet audit fix so this branch restores cleanly and CI can actually run the new tests.
NuGet audit fails restore on the 10.0.6 pins — five high-severity advisories against System.Security.Cryptography.Xml, promoted to errors by Warning As Error in NPOI.Benchmarks, which takes down restore for the whole test solution and so every CI run. Bumping the crypto package alone trips NU1109 because System.Formats.Asn1 is pinned at 10.0.6 and Microsoft.Bcl.Cryptography 10.0.10 wants 10.0.10, so both move together. Restore is clean with the audit enabled and the full test solution passes on net10.0 (2803 + 1868 + 79).
…ader Optimize PngUtils.MatchesPngHeader()
…ump-cryptography-xml Bump System.Security.Cryptography.Xml and System.Formats.Asn1 to 10.0.10 (fixes CI)
eli-swyfft
approved these changes
Jul 27, 2026
…ssf-validate-index-before-mutating-sheet-xml
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
4 participants
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.
Fork-side copy of the fix submitted upstream as nissl-lab/npoi#1825.
XSSFSheet.CreateRowadds the row element tosheetDatabeforeXSSFRow.RowNumvalidates the index, so a rejected index leaves an orphaned row in the XML — invisible to the object model, still written to the file. Repeated failed probes produce rows with duplicatervalues, ahead of the real rows, and Excel reports the workbook as corrupt and strips the sheet's data when repairing it.CreateColumnandXSSFRow.CreateCellhave the same mutate-then-validate shape.This is the library-side half of SW-53290; swyfft_web#21903 fixes the call site that was feeding
-1into the adapter. Either fix alone prevents the corruption — this one closes it for every future caller, including the upper-bound case the call-site guard doesn't cover.Exception types and messages are unchanged; the existing
BaseTestRowbounds assertions still hold. Four new tests cover the three creation methods plus an end-to-end check that inspects the persisted part XML. FullNPOI.OOXML.TestCasessuite passes on net10.0 (1872 tests); net472 failures are pre-existing and identical before and after.Packaged as
2.8.0-swyfft.2.(PR opened by Claude, an AI assistant, on Ken's behalf.)