commit ad9b3c0d704dceab75c2b3f6246740c78d8f7c04 Author: Jan Henning <jh+bugzilla@buttercookie.de> Date: Sun May 13 00:07:48 2018 +0200 Bug 1450449 - Part 5: Disable file:// URI checks for downloaded files and launching files from Gecko. r=jchen 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 (modulo the READ_EXTERNAL_STORAGE permission, respectively the user granting access through the Storage Access Framework) directory 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 all referring to the same file 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 those 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 open those additional support files (images, style sheets, etc.) 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. MozReview-Commit-ID: LuIYIA5FSGf --HG-- extra : rebase_source : a690b3097fdb03591f25f05a944c9ca3c05ddd04 --- .../org/mozilla/gecko/notifications/NotificationHelper.java | 7 +++++++ .../src/main/java/org/mozilla/gecko/GeckoAppShell.java | 10 +++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java b/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java index 35366609da49..713e9f9d4b3c 100644 --- a/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java +++ b/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java @@ -32,6 +32,7 @@ import android.content.pm.PackageManager; import android.content.pm.ResolveInfo; import android.graphics.Bitmap; import android.net.Uri; +import android.os.StrictMode; import android.support.v4.util.SimpleArrayMap; import android.util.Log; @@ -295,8 +296,14 @@ public final class NotificationHelper implements BundleEventListener { // scheme to prevent Fennec from popping up. final Intent viewFileIntent = createIntentIfDownloadCompleted(message); if (builder != null && viewFileIntent != null && mContext != null) { + // Bug 1450449 - Downloaded files already are already in a public directory and aren't + // really owned exclusively by Firefox, so there's no real benefit to using + // content:// URIs here. + StrictMode.VmPolicy prevPolicy = StrictMode.getVmPolicy(); + StrictMode.setVmPolicy(StrictMode.VmPolicy.LAX); final PendingIntent pIntent = PendingIntent.getActivity( mContext, 0, viewFileIntent, PendingIntent.FLAG_UPDATE_CURRENT); + StrictMode.setVmPolicy(prevPolicy); builder.setAutoCancel(true); builder.setContentIntent(pIntent); diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java index 34ba3315f295..3d21d7bb2f56 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java @@ -86,6 +86,7 @@ import android.os.Environment; import android.os.Looper; import android.os.ParcelFileDescriptor; import android.os.PowerManager; +import android.os.StrictMode; import android.os.SystemClock; import android.os.Vibrator; import android.provider.Settings; @@ -956,7 +957,14 @@ public class GeckoAppShell if (geckoInterface == null) { return false; } - return geckoInterface.openUriExternal(targetURI, mimeType, packageName, className, action, title); + // Bug 1450449 - Downloaded files already are already in a public directory and aren't + // really owned exclusively by Firefox, so there's no real benefit to using + // content:// URIs here. + StrictMode.VmPolicy prevPolicy = StrictMode.getVmPolicy(); + StrictMode.setVmPolicy(StrictMode.VmPolicy.LAX); + boolean success = geckoInterface.openUriExternal(targetURI, mimeType, packageName, className, action, title); + StrictMode.setVmPolicy(prevPolicy); + return success; } @WrapForJNI(dispatchTo = "gecko")