Review Uri.fromFile usage

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
Last year
9 months ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

(Blocks 1 bug)

Trunk
Firefox 62
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62 fixed)

Details

(Whiteboard: [priority:high])

Attachments

(5 attachments)

As per https://developer.android.com/about/versions/nougat/android-7.0-changes.html#sharing-files, once we bump our target SDK past API23, we can no longer use file URIs for external apps.

This means that we need to review all places where we're using Uri.fromFile and if necessary replace this by using a FileProvider or whatever might be appropriate:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile*.java+Uri.fromFile

E.g. the way we're capturing a new image in our file picker [1][2] perfectly fits the example given in https://inthecheesefactory.com/blog/how-to-share-access-to-file-with-fileprovider-on-android-nougat/en.

[1] https://dxr.mozilla.org/mozilla-central/rev/0405f6006f3a3f653dd42d587c3eefe08cffa37d/mobile/android/base/java/org/mozilla/gecko/FilePicker.java#204
[2] https://dxr.mozilla.org/mozilla-central/rev/0405f6006f3a3f653dd42d587c3eefe08cffa37d/mobile/android/base/java/org/mozilla/gecko/IntentHelper.java#249
From the other things in those search results, this probably concerns our download notification [1] and the updater service [2].
In addition, there are also instances were we trigger opening a file with an external app from within Gecko, e.g. opening a downloaded file from within about:downloads [3], where we'd have to intercept the file URI passed to us from Gecko and replace it with a content URI.

We should also consider using FLAG_GRANT_PERSISTABLE_URI_PERMISSION when sharing files (if that actually works as expected), so other applications can e.g. continue accessing a download file Uri as long as the underlying file on the file system still exists.

To top it off, there's also the question in how far other apps are actually compatible with content URIs [5], i.e. whether we'd want to always convert files to content URIs, ore only do that when running on Android O or later.

(In reply to Jan Henning [:JanH] from comment #0)
> and if necessary replace this by using a FileProvider or whatever might be appropriate
Although a FileProvider probably won't work for files on a (real) external SD card [7], e.g. if the user used a custom download directory (which currently - if it actually works - is only possible via about:config, so not really supported, but still something to keep in mind), or is directly browsing through the external SD card. In that case we'd probably have to implement our own ContentProvider that can deal with providing content URIs both for internal and external storage.

If we don't have the time to do this properly before being forced to raise our target API (see bug 1450450), an escape hatch might be setting a custom StrictMode VmPolicy to overwrite Android's default one, or worst case calling "Method m = StrictMode.class.getMethod("disableDeathOnFileUriExposure");" via reflection [8], although it's anyone's guess as to how long that workaround remains viable.

[1] https://dxr.mozilla.org/mozilla-central/rev/0405f6006f3a3f653dd42d587c3eefe08cffa37d/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java#342
[2] https://dxr.mozilla.org/mozilla-central/rev/0405f6006f3a3f653dd42d587c3eefe08cffa37d/mobile/android/base/java/org/mozilla/gecko/updater/UpdateService.java#699
[3] Which ends up at https://dxr.mozilla.org/mozilla-central/rev/0405f6006f3a3f653dd42d587c3eefe08cffa37d/toolkit/components/downloads/DownloadIntegration.jsm#685,687 and at least mimeInfo.launchWithFile then ends up going through [4] and file.launch() probably would likewise
[4] https://dxr.mozilla.org/mozilla-central/rev/0405f6006f3a3f653dd42d587c3eefe08cffa37d/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#255
[5] We ourselves aren't - there's some minimal support in the file picker [6] for uploading files, but we cannot e.g. view files with a content URI, such as provided by Android's own file explorer (bug 1406903)
[6] https://dxr.mozilla.org/mozilla-central/rev/0405f6006f3a3f653dd42d587c3eefe08cffa37d/mobile/android/base/java/org/mozilla/gecko/FilePickerResultHandler.java#105
[7] Compare the options available here: https://developer.android.com/reference/android/support/v4/content/FileProvider.html#SpecifyFiles
[8] https://github.com/xroche/httrack-android/blob/310b81a031a7862c49c6b4c812fa2f588559b234/app/src/main/java/com/httrack/android/HTTrackActivity.java#L2361
Also, while I can see the point of content:// URIs, namely
a) their content doesn't have to be served directly from the file system, and
b) they allow for more fine-grained permission handling, in that each time you hand out a content:// URI you can decide which permission to grant to the receiving app and also whether access to the content should be persistable or only temporary,

for things that
a) really are just plain files on the file system, and
b) already in a world-readable (modulo the READ_EXTERNAL_STORAGE permission) location, and
c) philosophically speaking maybe don't actually really *belong* to a specific app

using content:// URIs seems somewhat over the top.

So if I download an image with Firefox, then
- opening the file via about:downloads uses one content:// URI
- opening the file from the system download manager uses a different URI
- opening the file using a file manager (e.g. the internal one) introduces a third content URI
- and browsing the downloads folder directly from within your gallery app might access that file directly from the file system

Now good luck maintaining a LRU list and deduplicating all those entries with that...

Or the other way around - somebody wants to view a locally stored HTML file and the file explorer (e.g. Android's built-in file explorer that is available since Marshmallow I think?) uses a content:// URI to launch an HTML viewing app: In that case any resources (e.g. images) or links using relative URIs won't work, because even if you can guess the correct content:// URI for accessing those resources (because e.g. the Android file explorer leaks the file system path in its content:// URI, which makes it predictable), you won't actually have been granted the permission to access those files. [1]

So for installing a downloaded APK from our internal updater, or for our image sharing code that downloads the image to be shared to a temporary file before handing out the sharing intent [2] (or maybe any sharing actions in general) I absolutely agree that using a content:// URI would be the right choice going forward.

For launching an ACTION_VIEW intent for a file that is already sitting in the *public* Downloads directory on the other hand, I'd therefore tend to disagree with Google and say that - except for the requirement for the receiving app to posses the Storage permission - file:// URIs are clearly the better choice.
I've also looked through the framework sources and setting a new StrictMode VmPolicy should be enough to avoid FileUriExposedExceptions, no need to use reflection there [3]. If we fixed our image sharing/image capturing/update installing code to actually use content:// URIs through a FileProvider or something, we could even just relax the StrictMode policy temporarily when launching downloaded files and otherwise keep it on default.

[1] Just save a complete page using Firefox or otherwise mirror a website to disk and copy it to your phone. If you use a file explorer that uses intents with file:// URIs and view the page in Firefox or Android's built-in HTML viewer, everything works fine. If you use Android's built-in file explorer and launch the same page in Android's built-in HTML viewer using a content:// URI, suddenly all images will be missing and relative links within the mirrored file structure stop working. Chrome only registers itself for content:// URIs and suffers from the same problem (https://bugs.chromium.org/p/chromium/issues/detail?id=827801).
[2] https://dxr.mozilla.org/mozilla-central/rev/0e8eb01368600eb552dd558c83c64a3b6a0b89b8/mobile/android/base/java/org/mozilla/gecko/widget/GeckoActionProvider.java#291
[3] Can't test it, though, since my phone is still on 6.0.1 and I haven't managed to get the emulator working in my development VM for quite a while.
See Also: → 1406903
To summarize: My personal recommendation would be therefore be to fix up
- sharing of images (GeckoActionProvider)
- the intent for installation of downloaded updates (UpdateService)
- the code for capturing an image for the FilePicker with the help IntentHelper

so that on Android N and above we use a ContentProvider (ideally a simple FileProvider, so that we have less work to implement this ourselves) when passing out intents in those cases.

For launching of downloaded files, whether via a notification or through GeckoAppShell/GeckoInterface, I still think that file:// URIs are a better fit, as we're downloading the files to the public downloads folder anyway and this also makes it easier to support user-configured download folders in arbitrary locations, including especially on an external SD card, so we should just temporarily set a custom StrictMode VmPolicy for that case.
Assignee: nobody → jh+bugzilla
Whiteboard: [priority:high]
Comment on attachment 8982907 [details]
Bug 1450449 - Part 1: Add FileProvider.

https://reviewboard.mozilla.org/r/248730/#review255130
Attachment #8982907 - Flags: review?(nchen) → review+
Comment on attachment 8982908 [details]
Bug 1450449 - Part 2: Use content:// URI for capturing images from FilePicker.

https://reviewboard.mozilla.org/r/248732/#review255134

::: mobile/android/base/java/org/mozilla/gecko/IntentHelper.java:259
(Diff revision 1)
> +        if (AppConstants.Versions.preLollipop) {
> +            // As per https://github.com/commonsguy/cw-omnibus/blob/master/Camera/FileProvider/
> +            // app/src/main/java/com/commonsware/android/camcon/MainActivity.java - at least we
> +            // don't have to support anything below Jelly Bean.
> +            ClipData clip =
> +                    ClipData.newUri(context.getContentResolver(), "A photo", destination);

"A photo" should be localized. Or I think we can use empty string.
Attachment #8982908 - Flags: review?(nchen) → review+
Comment on attachment 8982909 [details]
Bug 1450449 - Part 3: Starting from Nougat, share images via content:// URIs.

https://reviewboard.mozilla.org/r/248734/#review255136
Attachment #8982909 - Flags: review?(nchen) → review+
Comment on attachment 8982910 [details]
Bug 1450449 - Part 4: Starting from Nougat, install updates via content:// URIs.

https://reviewboard.mozilla.org/r/248736/#review255140
Attachment #8982910 - Flags: review?(nchen) → review+
Comment on attachment 8982911 [details]
Bug 1450449 - Part 5: Disable file:// URI checks for downloaded files and launching files from Gecko.

https://reviewboard.mozilla.org/r/248738/#review255142
Attachment #8982911 - Flags: review?(nchen) → review+
Comment on attachment 8982908 [details]
Bug 1450449 - Part 2: Use content:// URI for capturing images from FilePicker.

https://reviewboard.mozilla.org/r/248732/#review255134

> "A photo" should be localized. Or I think we can use empty string.

I tried this on an API16 S3 Mini and didn't see that string being used anywhere, at least with the default camera app, and according to https://stackoverflow.com/a/39504849 this can probably be generally expected, plus we're already [using hard-coded strings elsewhere](https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile*java+clipdata.&redirect=false), but on the other hand yes, I might as well leave it empty then.
Tested the file download handling changes from part 5 by locally enabling penaltyDeath() for VmPolicy violations, and without that patch clicking on an entry in about:downloads or attempting to show the "download completed" notification do indeed crash Firefox. With that patch included however, everything works fine, so for notifications the moment the PendingIntent is created is indeed the relevant point.
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/d4d9645e40cb
Part 1: Add FileProvider. r=jchen
https://hg.mozilla.org/integration/autoland/rev/dd8790d74cec
Part 2: Use content:// URI for capturing images from FilePicker. r=jchen
https://hg.mozilla.org/integration/autoland/rev/5d0a03ae227a
Part 3: Starting from Nougat, share images via content:// URIs. r=jchen
https://hg.mozilla.org/integration/autoland/rev/eb2c61b1014c
Part 4: Starting from Nougat, install updates via content:// URIs. r=jchen
https://hg.mozilla.org/integration/autoland/rev/9175cafb84bd
Part 5: Disable file:// URI checks for downloaded files and launching files from Gecko. r=jchen
See Also: → 1476681
See Also: → 1484472
See Also: → 1500906
You need to log in before you can comment on or make changes to this bug.