Skip to content
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

fix #1412: Picking a folder in iOS causes the original folder to be deleted. #1457

Merged
merged 4 commits into from
Apr 22, 2024

Conversation

twinstar6980
Copy link
Contributor

@twinstar6980 twinstar6980 commented Mar 9, 2024

If use file_picker version 5.3.3 or higher, when an ios user picks a directory, the original directory will be deleted (actually, moved to the app's sandbox tmp directory, and the user cannot access them), and file_picker returns a path in tmp directory.

This PR fixes this BUG. file_picker will return the absolute path to the folder, just like before v5.3.3.

#1412

if ([[NSFileManager defaultManager] fileExistsAtPath:tempURL.path]) {
[[NSFileManager defaultManager] removeItemAtPath:tempURL.path error:&error];
NSMutableArray<NSURL *> *newUrls;
if(controller.documentPickerMode == UIDocumentPickerModeOpen) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentPickerMode is deprecated. Could you rewrite this to only use the deprecated API up to iOS 14 ?

See https://developer.apple.com/documentation/uikit/uidocumentpickerviewcontroller#1658475

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't have a mac device now, so I can't rewrite and test it.
In addition, I noticed that the deprecated API was also used in the previous code. Maybe this also needs to be migrated to the new API, but I am not familiar with objc.
BTW, on my own ios device, when using the init(forOpeningContentTypes:) swift api, I can directly get the absolute path of file/directory. Why doesn't file_picker use this api, but makes a cache file?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not now why file_picker uses a cache for that, maybe @miguelpruivo knows?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not now why file_picker uses a cache for that, maybe @miguelpruivo knows?

There are 2 approaches when opening descriptors for files within app's container

  1. Opening a resource directly to the file through URI/UTI or;
  2. Caching a copy of the file so it can be manipulated individually within the app and without any constraint regarding the original file;

2 is typically the preferred option since most of the apps use the files for either manipulating (editing) or, even if it's just for sending somewhere, it doesn't have any consequence. If I recall, this was the original approach from the Flutter's image_picker so I followed the same path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@navaronbracke @miguelpruivo
Please pay attention to #1412 . This bug is very serious on iOS and will endanger user data security.
It has been half a year since I raised the issue, but this dangerous bug still exists and has not been fixed.
This PR is made based on the current implementation, I know that UIDocumentPickerMode is a deprecated api, but the code in this library always uses this deprecated api when creating UIDocumentPickerViewController.
In this case, why is this PR banned because I use the same deprecated API? (In fact, another PR #1452 that was approved also used the deprecated UIDocumentPickerMode api).
I think a more reasonable solution is to first pass this PR to solve the BUG instead of banning this PR because of an deprecated api that already exists in the library.
Please allow me to repeat it again: this bug is very serious, it will delete files without the user knowing! Please don't keep this bug alive for longer.

@miguelpruivo miguelpruivo self-assigned this Mar 20, 2024
@miguelpruivo miguelpruivo added needs investigation iOS Issue applies to iOS platform labels Mar 20, 2024
@navaronbracke
Copy link
Collaborator

@twinkles-twinstar Can you a) fix the merge conflict and b) update the version and changelog? The version can bump the patch version.

@twinstar6980
Copy link
Contributor Author

@twinkles-twinstar Can you a) fix the merge conflict and b) update the version and changelog? The version can bump the patch version.

Completed, please review.

@navaronbracke navaronbracke merged commit 137840a into miguelpruivo:master Apr 22, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issue applies to iOS platform needs investigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants