1450449 - Review Uri.fromFile usage

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Jan Henning [:JanH]]

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

Comment 1

[Jan Henning [:JanH]]

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

Comment 2

[Jan Henning [:JanH]]

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.

Comment 3

[Jan Henning [:JanH]]

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.

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1450449 - Part 1: Add FileProvider.

In case we change our thinking on launching of downloaded files and start using
content:// URIs for that case as well, we already allow our FileProvider to
generate URIs for the whole file system using <root-path>. This is because users
can in principle move our download directory to an arbitrary location on the
file system as long as it is accessible to Firefox. However not all of these
locations (e.g. on a removable SD card) can be specified through the other
methods of specifying available files for a FileProvider, so only the <root-
path> option remains.

Review commit: https://reviewboard.mozilla.org/r/248730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248730/

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1450449 - Part 2: Use content:// URI for capturing images from FilePicker.

Since it is only us and the camera app who have to deal with the content:// URI,
it should be safe enough to use content:// URIs on all supported versions.

Review commit: https://reviewboard.mozilla.org/r/248732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248732/

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1450449 - Part 3: Starting from Nougat, share images via content:// URIs.

For sharing images we download the image to a temporary file in our internal
storage area. This is a perfect use case for granting temporary access to the
file only via a content:// URI instead of directly exposing the real file system
path.

Since support for content:// URIs by arbitrary other apps might be patchy on
older Android versions, though, we only start doing this from Nougat onwards.

Review commit: https://reviewboard.mozilla.org/r/248734/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248734/

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1450449 - Part 4: Starting from Nougat, install updates via content:// URIs.

We download the update APK into the public downloads directory and normally the
only relevant app consuming that URI should be the system package installer, but
just to be safe we only switch usage from Nougat onward, too.

Review commit: https://reviewboard.mozilla.org/r/248736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248736/

Comment 8

[Jan Henning [:JanH]]

Attachment: Bug 1450449 - Part 5: Disable file:// URI checks for downloaded files and launching files from Gecko.

This is a case where I disagree with Google's stance about content:// URIs.
They're perfect for granting access to files that might not even be present on
the file system, e.g. virtual files generated on the spot or retrieved from some
database, a cloud storage provider's app granting access to files stored in the
cloud, etc., as well as for being able to selectively grant access to files
conceptually "owned" by a certain app, especially files within the app's private
internal storage.

However when considering files that don't actually "belong" to any specific app
in particular and that are already being stored in a publicly accessible
directory (modulo the READ_EXTERNAL_STORAGE permission, respectively the user
granting access through the Storage Access Framework) somewhere within the
external storage, they also have a number of drawbacks:
- While in practice a number of FileProviders will "leak" the true file system
  path through the content:// URI they generate, the problem remains that
  there's no way to know for sure whether two content:// URIs received from
  different apps are in fact referring to the same file or not.
  In case of our downloads for example, content:// URIs could in principle be
  generated
  * by Firefox itself
  * by the system Downloads app
  * by the system file browser app
  * by any other third-party file browser or similar app that the user might
    have installed
  which e.g. will needlessly clutter up any LRU lists other apps might keep.
- content:// URIs obviously depend on the generating app still being installed.
  So even if we fixed bug 1280184, so that uninstalling Firefox would no longer
  remove the user's downloads, all content:// URIs generated by Firefox re-
  ferring to those files would become invalid anyway.
- Even if the actual file is already sitting in a public directory, when
  accessing it through the content:// URI the receiving app still needs to
  explicitly persist the permissions granted for that URI, and there are some
  signs that you can only persist permissions for a limited number of files. For
  file:// URIs on the other hand the only limitation on the number of file://
  URIS you can remember is the available storage space for storing that URIs,
  i.e. for practical purposes more or less unlimited.
- content:// URIs only grant access to a specific file. If we (or possibly an
  add-on) started implementing saving of websites as on desktop (i.e. HTML +
  associated support files instead of a PDF "copy"), then receiving apps
  couldn't properly those additional support files when getting a content:// URI
  to the main HTML file (see https://issuetracker.google.com/issues/77406791).

Since we do store downloads in the public Downloads folder on the external
storage by default and I believe that conceptually, those files belong to the
user and not Firefox specifically, I propose to continue launching downloaded
files directly through their file:// URI.
To that end, we temporarily disable the corresponding StrictMode restrictions
when required and restore them afterwards.

Review commit: https://reviewboard.mozilla.org/r/248738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248738/

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8982907 [details]
Bug 1450449 - Part 1: Add FileProvider.

https://reviewboard.mozilla.org/r/248730/#review255130

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8982909 [details]
Bug 1450449 - Part 3: Starting from Nougat, share images via content:// URIs.

https://reviewboard.mozilla.org/r/248734/#review255136

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8982910 [details]
Bug 1450449 - Part 4: Starting from Nougat, install updates via content:// URIs.

https://reviewboard.mozilla.org/r/248736/#review255140

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 14

[Jan Henning [:JanH]]

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.

Comment 15

[Jan Henning [:JanH]]

Comment on attachment 8982908 [details]
Bug 1450449 - Part 2: Use content:// URI for capturing images from FilePicker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248732/diff/1-2/

Comment 16

[Jan Henning [:JanH]]

Comment on attachment 8982909 [details]
Bug 1450449 - Part 3: Starting from Nougat, share images via content:// URIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248734/diff/1-2/

Comment 17

[Jan Henning [:JanH]]

Comment on attachment 8982910 [details]
Bug 1450449 - Part 4: Starting from Nougat, install updates via content:// URIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248736/diff/1-2/

Comment 18

[Jan Henning [:JanH]]

Comment on attachment 8982911 [details]
Bug 1450449 - Part 5: Disable file:// URI checks for downloaded files and launching files from Gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248738/diff/1-2/

Comment 19

[Jan Henning [:JanH]]

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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d4d9645e40cb
https://hg.mozilla.org/mozilla-central/rev/dd8790d74cec
https://hg.mozilla.org/mozilla-central/rev/5d0a03ae227a
https://hg.mozilla.org/mozilla-central/rev/eb2c61b1014c
https://hg.mozilla.org/mozilla-central/rev/9175cafb84bd