We're running an app where customers can import data by uploading Excel spreadsheets. Over the years, we've seen some pretty nasty examples of files that contain millions of cells, which causes the import process to stall for a long time and run out of memory.

It seems like these files are not maliciously crafted, and there aren't millions of rows with actual data, but those cells still exist in the XML markup with a reference to some styling or format. My suspicion is that the user typed Cmd+A and applied a format or style, and then the resulting file needs to contain all the rows as placeholders for the styling. Unfortunately, I don't have an example handy, but you might be familiar with that kind of thing already :)

I've come up with the enclosed hack to abort the reading when too many rows have been read. Think of it as a proof of concept -- I'm aware that it's not mergeable in its current form, mostly because of how themaxRows is defined and parsed down. At the very least, it should be part of an options object or something. Also, it would be nice if it could limit the number of cells instead of rows.

At this time, this is basically the only patch that's keeping us from switching back to the latest officially released version of exceljs, so I'm quite eager to get this feature added in some shape or form.

Is this something that other users would like in general?
Any input about how to do it "the right way"?

Plow in the ability to customize the max number of allowed rows in a …

3a6b72f

…worksheet

Copy link

Collaborator

guyonroche commentedApr 17, 2018

@papandreou This sounds like a very sensible feature - especially as it will reduce servers vulnerability to bad xlsx based inputs.

Regarding implementation, my thoughts are:

add an options argument to XLSX.readFile() and read() which can contain things like maxRows (or maxCols, etc). It can also indicate appropriate action - e.g. throw, crop, etc.
The options would be used in all the XLSX.process****Entry functions and passed into the relevant Xform objects
WorksheetXform would translate an option like "maxRows" into a "maxLength" option to give to the ListXform that handles the "sheetData" tag
ListXform can perform the appropriate action if the "maxLength" requirement is about to fail

papandreou added2 commits

April 23, 2018 15:59

Update comment to reflect what's actually implemented

7570693

Accept an options object in XLSX.read(File)? and pass it down where i…

1ef6b60

…t's needed

Copy link

ContributorAuthor

papandreou commentedApr 23, 2018

@guyonroche, thanks for the input! I changed the API now so that the limit is configured via an options object passed toXLSX.read(..., {maxRows: 123}) instead. It feels a little awkward to explicitly pass it down like that in so many steps, am I missing something? Is there a more central place where it can be stored?

Copy link

Collaborator

guyonroche commentedApr 24, 2018

I don't think there is a completely 'tidy' solution - passing options down through the call stack is ok

guyonroche merged commita3c7065 intoexceljs:master

Apr 24, 2018

Copy link

Collaborator

guyonroche commentedApr 24, 2018

@papandreou - I tweaked your changes a bit...

removed the options from the Workbook document as they weren't used
moved the check from RowXform (the child) to ListXform (the parent)
changed the text of the error to 'Max row count exceeded' as it now comes from ListXform
modified the error handling in base-xform
Hope that's ok