Skip to content

Handle zip imports regression - #874

Merged
ashfame merged 9 commits into
trunkfrom
zip_import_regression
Feb 4, 2025
Merged

Handle zip imports regression#874
ashfame merged 9 commits into
trunkfrom
zip_import_regression

Conversation

@ashfame

@ashfame ashfame commented Jan 31, 2025

Copy link
Copy Markdown
Member

Related issues

Proposed Changes

  • Show a specific error to the user about this issue, and suggesting them to re-package the zip archive
  • Change importBackup() and importSite() to not return undefined and just throw
Before After
Screenshot 2025-02-03 at 19 52 33 Screenshot 2025-02-03 at 19 51 26

Testing Instructions

  • Try importing a zip file which has absolute paths in it
  • Before: Error would show as in the linked issue
  • After: Specific error shown

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@ashfame ashfame self-assigned this Jan 31, 2025
@ashfame
ashfame force-pushed the zip_import_regression branch 4 times, most recently from b802b74 to 8b4b7f7 Compare February 3, 2025 16:10
@ashfame
ashfame force-pushed the zip_import_regression branch from 8b4b7f7 to fe0ce64 Compare February 3, 2025 16:23
@ashfame
ashfame requested a review from a team February 3, 2025 17:51
@ashfame
ashfame marked this pull request as ready for review February 3, 2025 17:51
Comment thread src/hooks/use-import-export.tsx Outdated

@fredrikekelund fredrikekelund left a comment

Copy link
Copy Markdown
Contributor

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?

Comment thread src/hooks/use-import-export.tsx Outdated
Comment thread src/lib/import-export/import/import-manager.ts
Comment thread src/hooks/use-import-export.tsx Outdated
Comment thread src/tests/ipc-handlers.test.ts Outdated
ashfame and others added 3 commits February 4, 2025 16:19
@ashfame

ashfame commented Feb 4, 2025

Copy link
Copy Markdown
Member Author

@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.

@ashfame
ashfame merged commit 3d19d94 into trunk Feb 4, 2025
@ashfame
ashfame deleted the zip_import_regression branch February 4, 2025 13:18
@sentry

sentry Bot commented Feb 4, 2025

Copy link
Copy Markdown

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: No suitable importer found for the provided backup contents <anonymous>(main/studio/./src/lib/import-export... View Issue
  • ‼️ Error: No suitable backup handler found for the provided backup file <anonymous>(main/studio/./src/lib/import-export... View Issue

Did you find this useful? React with a 👍 or 👎

@fredrikekelund

Copy link
Copy Markdown
Contributor

All good with the Sentry error, it's expected 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants