[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
[tor-commits] [tor-browser/tor-browser-60.4.0esr-8.5-1] Bug 1450449 - Part 5: Disable file:// URI checks for downloaded files and launching files from Gecko. r=jchen
commit ad9b3c0d704dceab75c2b3f6246740c78d8f7c04
Author: Jan Henning <jh+bugzilla@xxxxxxxxxxxxxxx>
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")
_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits