Use a better file picker - #388
Conversation
| @@ -127,14 +129,52 @@ class Utils { | |||
| } | |||
|
|
|||
| static Future<String?> pickFile(BuildContext context) async { | |||
There was a problem hiding this comment.
nit: context is unused now
|
|
||
| // We get a copy, not the original, so don't leave a plaintext key sitting around | ||
| try { | ||
| await File(file.path).delete(); |
There was a problem hiding this comment.
Android drops each copy in its own <cacheDir>/<uuid>/, and the plugin's targetDirectory.deleteOnExit() (FileUtils.java:130) never fires there - the javadoc attempts deletion only on normal VM termination, which an Android process kill is not. Worth adding a non-recursive .parent.delete() here; it throws harmlessly into the existing catch on iOS, where the parent is shared.
| await File(file.path).delete(); | |
| await File(file.path).delete(); | |
| // Android drops each copy in its own <cacheDir>/<uuid>/; fails harmlessly | |
| // on iOS where the parent is shared | |
| await File(file.path).parent.delete(); |
| try { | ||
| await File(file.path).delete(); | ||
| } catch (err) { | ||
| // Ignoring file delete errors |
There was a problem hiding this comment.
Possible plaintext key material stayed on disk, maybe worth a log at least
| try { | ||
| final cacheDir = await getTemporaryDirectory(); | ||
| final legacyCache = Directory(p.join(cacheDir.path, 'file_picker')); | ||
| if (legacyCache.existsSync()) { |
There was a problem hiding this comment.
existsSync / deleteSync / listSync block the isolate during startup for no benefit, since this is already fire-and-forget. The dart:io docs say it directly at file_system_entity.dart:230: "prefer the asynchronous version to avoid blocking your program."
| if (legacyCache.existsSync()) { | |
| if (await legacyCache.exists()) { | |
| await legacyCache.delete(recursive: true); | |
| } |
| // Ignoring cleanup errors | ||
| } | ||
|
|
||
| // iOS document imports land in <tmp>/<bundle id>-Inbox. systemTemp because |
There was a problem hiding this comment.
I could not confirm anywhere that .import stages into <tmp>/<bundle id>-Inbox. Apple's picker docs do not say, and the archived File System guide's Documents/Inbox entry describes a different API (UIDocumentInteractionController and Mail attachments, not the picker). Worth a one-line debugPrint of the path openFile() returns on a device before merging, because if it is not under systemTemp then the whole iOS branch below matches nothing and is dead code.
|
|
||
| // iOS document imports land in <tmp>/<bundle id>-Inbox. systemTemp because | ||
| // path_provider points getTemporaryDirectory at Library/Caches, not tmp, and it | ||
| // is only our sandbox on iOS. Inboxes only, Share is using the rest of tmp. |
There was a problem hiding this comment.
I think this premise is inverted. Share writes to getTemporaryDirectory() (lib/services/share.dart:24), which path_provider_foundation maps to NSCachesDirectory on iOS - path_provider_foundation_real.dart:41, pinned at 2.6.0 in the lockfile. Share never touches tmp, so there is nothing of ours here to protect from a wider sweep.
|
|
||
| try { | ||
| for (final entry in Directory.systemTemp.listSync()) { | ||
| if (entry is Directory && entry.path.endsWith('-Inbox')) { |
There was a problem hiding this comment.
Blocking, I think. file_picker 12 does not hand back the -Inbox path - it copies to a loose file at the top of tmp. IOSFilePickerHandler.swift:208 calls copyToTemporaryDirectory, defined at :334-345, which writes to NSTemporaryDirectory()/<filename>. So this filter walks straight past the plaintext key it exists to delete.
That file is exactly what the old FilePicker.clearTemporaryFiles() used to remove, since it wiped every entry in tmp (:308-320) - which means after this upgrade the last-imported key stays on disk permanently, the failure mode the method is named for.
| if (entry is Directory && entry.path.endsWith('-Inbox')) { | |
| if (entry is File || (entry is Directory && entry.path.endsWith('-Inbox'))) { | |
| entry.deleteSync(recursive: true); | |
| } |
| file_picker: ^12.0.0-beta.7 | ||
| # We only pick documents (certs, keys, configs). file_selector is first party and | ||
| # has no media picker code, so photo library permissions can never sneak into our binary | ||
| file_selector: ^1.1.0 |
There was a problem hiding this comment.
FYI: the allowed-type root narrows from public.item (file_picker IOSFilePickerHandler.swift:67) to public.data (file_selector_ios.dart:54, pinned 0.5.3+5). Should be invisible for certs, keys and YAML, but it is a real behavior change. Claude suggests testing the Files-app-via-iCloud at least.
|
|
||
| @override | ||
| void initState() { | ||
| // Waits until here so the binding is up, main runs before anyone has made one |
There was a problem hiding this comment.
I don't quite follow the comment, fwiw
2 participants
Trade
file_pickerforfile_selectorand drop temporary files