Skip to content

Avoid unhandled rejection on XML parse error #813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 16, 2019

Conversation

papandreou
Copy link
Contributor

Ran into some maliciously crafted .xlsx files that contained invalid XML, which triggered an unhandled rejection because the stream no longer had an error handler at that point. Seems like I introduced this bug myself back in #541, so it's only fair that I fix it.

The test case is a little out there, but I couldn't find another way to test for an unhandled rejection. The workbook.xlsx.readFile promise already got rejected as it was supposed to.

@alubbe
Copy link
Member

alubbe commented May 14, 2019

There seems to something wrong with your prettier configuration, did you run npm install before committing? Can you try fixing it?

@papandreou
Copy link
Contributor Author

The prettier setup looked incomplete, and there seemed to be some dev deps missing, so I just tried some things to satisfy the pre-commit hook.
Not near a computer right now and won’t be for a while, so you’re very welcome to just ditch the last commit and fix up the formatting the right way.

@guyonroche
Copy link
Collaborator

@alubbe regarding the prettier - I changed the config to be lint only - without the fixing as I ended up with a situation where the fixer was outputting code that the linter rejected. In a way - I tend to prefer fixing the lint issues manually it gives the chance to fix the code in a better way than the lint fixer can

@guyonroche guyonroche merged commit b16751c into exceljs:master May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants