-
Notifications
You must be signed in to change notification settings - Fork 23
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
Handle zip imports regression #874
Conversation
b802b74
to
8b4b7f7
Compare
when either a suitable importer or suitable backup handler is not found
…ws vs returning undefined + add tests
8b4b7f7
to
fe0ce64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Given our current circumstance, the PR is an improvement, it tests well and the code looks good.
However… If many other unzip implementations (including the built-in tools on macOS and Windows) don't have a problem with leading slashes, I would strongly argue that Studio shouldn't either. I understand the argument that the spec makes about security, but my response would simply be: "let's automatically fix those filenames instead of outright rejecting the ZIP file". @ashfame, do you know of a way to do this with yauzl, or should we consider looking into replacement libraries for upcoming versions of Studio?
Co-authored-by: Fredrik Rombach Ekelund <fredrik@f26d.dev>
@fredrikekelund Yep, I agree in principle of not leaving users hanging. Please see this comment where I laid down my thoughts on approaching this. I believe we need a archive handler abstraction that contains all this logic. We can continue that discussion there and figure out future next steps and let this PR be merged. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
All good with the Sentry error, it's expected 👍 |
Related issues
Proposed Changes
importBackup()
andimportSite()
to not return undefined and just throwTesting Instructions
Pre-merge Checklist