Support scoped storage to fix downloads on API >= 29

added Doing label

changed title from Support Scoped Storage to Support scoped storage to re-enable downloads on API >= 29

changed title from Support scoped storage to re-enable downloads on API >= 29 to Support scoped storage to re-enable downloads on Android 10 and above

changed title from Support scoped storage to re-enable downloads on Android 10 and above to Support scoped storage to re-enable downloads on API >= 29

changed the description

mentioned in issue fenix#40192 (closed)

this commit fixes the bug: http://eweiibe6tdjsdprb4px6rqrzzcsi22m4koia44kc5pcjr7nec2rlxyad.onion/aguestuser/android-components/-/commit/d6444a09c66f713b3d51554e6e39efd11260e5ad

poking to see whether the manner in which we download files acceptably avoids metadata leakage as compared to previous (buggy) method...

unrelated to whether this fix is safe, but found some added confirmation as to why we see this bug. commits like this (showing up in GV v93), which remove the external storage permission for apps at API >= 29:

2639ed83

(doesn't explain why bug shows up in v90, but adds some validation to theory that moz started removing permissions needed for non-scoped-storage usage after they started enforcing its use)

@sysrqb okay so after some analysis, i'm coming around to the conclusion that we shouldn't bother trying to block calls that write metadata about downloaded files to persistent storage at all (as i was still attempting to do here

high level rationale:

the call to addCompletedDownload is delegated to AbstractFetchDownloadService (which we pass as a paramater instead of AndroidDownloadService to block proxy bypasses), which in turn delegates to app.android.DownloadManager which makes a db insertion. thus: this call exposes us to some metadata leakage on the file system but not to any increased danger of proxy bypass (which was our primary concern in !7 (merged))
however, i claim it is impossible to avoid any metadata leakage to the filesystem in any case: and thus we should not try.

full analysis:

if we want to perform downloads at all, then the only-supported implementation for doing so (scoped storage) stores download status on the filesystem anyway (when it calls ContentResolver::insert into the collection representing the OS's shared storage volume, MediaStore.VOLUME_EXTRERNAL_PRIMARY, in the course of executing AbstractFetchDownloadService::useFileStreamScopedStorage [1], [2]
more to the point: the only available implementation for performing downloads (the AbstractFetchDownloadManager we use instead of AndroidDownloadManager) also writes all download metadata (in the form of the fields of the DownloadState data class [3]) to the filesystem when it kicks off the download with:
- FetchDownloadManager::download -> store.dispatch(DownloadAction.AddDownloadAction(download)) [4] ->
- DownloadMiddleware::invoke -> saveDownload(context.store, action.download) [5] ->
- DownloadMiddleware::saveDownload -> downloadStorage.add(download) [6] ->
- DownloadStorage::add(download)-> downloadDao.insert(download.toDownloadEntity()) [7] ->
and the AbstractFetchDownloadService itself, once running a download, may similarly persist metadata about the state of an interrupted download when it calls:
- AbstractFetchDownloadService#performDownload -> setDownloadJobStatus(currentDownloadJobState, FAILED) [8] ->
- (snip)
- DownloadStorage::update -> downloadDao.update(download.toDownloadEntity()) [9]
- thus, since it seems impossible to perform downloading at all without storing metadata on the filesystem in the process, i see little benefit to avoiding storing some metadata once we are done (since whatever leakage we might have wished to avoid has already taken place)
to seal the deal: in all cases, files that have been downloaded are placed in the Downloads directory
- here, they are exposed to pretty much any other app with external storage access (eg: messaging apps that send attachments) via the Gallery collection, which always includes the Downloads directory
- so: even if we blocked metadata from leaking into a database (which is at least somewhat encapsulated from other apps), we would end the process with our file exposed to all other apps via its location in the Downloads directory anyway.
now: perhaps we could consider patching AbstractFetchDownloadManager to not place files in the Downloads directory, but:
- this would require a fair amount of work which would constitute the delivery of a new feature that we have not to date currently supported. since the scope of this ticket is fixing a but to restore behavior -- and not introducing new behavior to store downloads in a different place
- and in any case, we'd still be stuck with figuring out another way to support resuming downloads, which adds even more scope (to work on an app we are eventually trying to replace)

conclusion:

with all that in mind, i argue that we should simply:

remove the BuildConfig.ANDROID_DOWNLOADS_INTEGRATION field altogether
make no attempt to block calls to either AbstractFetchDownloadService::useFileStreamScopedStorage or AbstractFetchDownloadService::addToDownloadSystemDatabaseCompat
feel good about this because these calls (1) do not result in proxy bypass, (2) do not present any increased risk of metadata leakage to the file system over and above what we already currently do, the avoidanced of which would require substantial patches to upstream code that is currently out of scope

enacting this argument in code here: aguestuser/android-components@e2daac07

(MR coming soon)

Upon some consderation, i'm realizing that i might be drawing a false equivalence between all uses of persistent storage in the analysis above. In other words, it is not necessarily the case that the same trust boundaries are observed (or crossed) by (1) AbstractFetchDownloadManager in the course of dispatching/handling actions to initiation/suspend/resume downloads that we do not currently block and are not considering blocking (when it calls DownloadStore::add{,update} in the course of updating download state), and (2) the call by AbstractFetchDownloader::useFileStreamScopedStorage to ContentResolver::insert when initiating a download -- which I am arguing we must allow in order to not break all downloads, (3) the call by AbstractFetchDownloader::addToDownloadSystemDatabaseCompat to DownloadManager::addCompletedDownload[*] -- which I am arguing we should also allow (both for sake of simplicity and because blocking it offers no additional value).

In fact, it would appear likely that the DownloadStore used in (1) is likely to be scoped to Fenix, while the ContentResolver store in (2) might possibly be accessible by other applications (as it pertains to the shared storage volume), and the DownloadManager store accessed in (3) is almost certainly shared. So... should we still allow the calls made in (2) and (3)?

I think the answer is still "yes," primarily because the location we are targeting for storing our files in (the Downloads directory) is a shared volume. There is an argument that we should alter (2) to use a volume that is scoped more narrowly to only be accessible to Tor Browser (would users like this?), in which case it would make sense to disable (3). But if we are okay with outputing downloads to the Download folder (which even after the blocks introduced in !7 (merged) we still did), then (2) will still be targetting shared storage and it there is no additional advantage to disabling (3) -- but some disadvantage to keeping hte code to disable it as it is essentially dead/useless code which adds cognative overhead for no benefit.

So... on balance, I'd say I still shoot out where I shot out yesterday, only I see the trade-off as a bit more complex. Happy to be overruled by either @sysrqb or @duncan, in which case I'd need to mark !80 (merged) as pending and get to work on a patch to modify fenix's use of scoped storage to target a different (more narrowly scoped directory), which will probably involve some UX work, as it will also involve changing user expectations w.r.t. where to find just-downloaded files.

[*] For sake of completeness of documentation, the call travels from this point to DownloadManager::addCompletedDownload, which I cannot link to, but has this implemetnation:

    /**
     * <p> For applications targeting {@link android.os.Build.VERSION_CODES#Q} or above,
     * {@code path} must be within directories owned by the application
     * {e.g. {@link Context#getExternalFilesDir(String)}} or if the application is running under
     * the legacy storage model (see
     * {@link android.R.styleable#AndroidManifestApplication_requestLegacyExternalStorage
     * android:requestLegacyExternalStorage}), {@code path} can also be within the top-level
     * Downloads directory (as returned by
     * {@link Environment#getExternalStoragePublicDirectory(String)} with
     * {@link Environment#DIRECTORY_DOWNLOADS}).
     *
     * {@hide}
     *
     * @deprecated Apps should instead contribute files to
     * {@link android.provider.MediaStore.Downloads} collection to make them available to user
     * as part of Downloads.
     */
    @Deprecated
    public long addCompletedDownload(String title, String description,
            boolean isMediaScannerScannable, String mimeType, String path, long length,
            boolean showNotification, boolean allowWrite, Uri uri, Uri referer) {
        // make sure the input args are non-null/non-zero
        validateArgumentIsNonEmpty("title", title);
        validateArgumentIsNonEmpty("description", description);
        validateArgumentIsNonEmpty("path", path);
        validateArgumentIsNonEmpty("mimeType", mimeType);
        if (length < 0) {
            throw new IllegalArgumentException(" invalid value for param: totalBytes");
        }

        // if there is already an entry with the given path name in downloads.db, return its id
        Request request;
        if (uri != null) {
            request = new Request(uri);
        } else {
            request = new Request(NON_DOWNLOADMANAGER_DOWNLOAD);
        }
        request.setTitle(title)
                .setDescription(description)
                .setMimeType(mimeType);
        if (referer != null) {
            request.addRequestHeader("Referer", referer.toString());
        }
        ContentValues values = request.toContentValues(null);
        values.put(Downloads.Impl.COLUMN_DESTINATION,
                Downloads.Impl.DESTINATION_NON_DOWNLOADMANAGER_DOWNLOAD);
        values.put(Downloads.Impl._DATA, path);
        values.put(Downloads.Impl.COLUMN_MIME_TYPE, resolveMimeType(new File(path)));
        values.put(Downloads.Impl.COLUMN_STATUS, Downloads.Impl.STATUS_SUCCESS);
        values.put(Downloads.Impl.COLUMN_TOTAL_BYTES, length);
        values.put(Downloads.Impl.COLUMN_MEDIA_SCANNED,
                (isMediaScannerScannable) ? Request.SCANNABLE_VALUE_YES :
                        Request.SCANNABLE_VALUE_NO);
        values.put(Downloads.Impl.COLUMN_VISIBILITY, (showNotification) ?
                Request.VISIBILITY_VISIBLE_NOTIFY_ONLY_COMPLETION : Request.VISIBILITY_HIDDEN);
        values.put(Downloads.Impl.COLUMN_ALLOW_WRITE, allowWrite ? 1 : 0);
        Uri downloadUri = mResolver.insert(Downloads.Impl.CONTENT_URI, values);
        if (downloadUri == null) {
            return -1;
        }
        return Long.parseLong(downloadUri.getLastPathSegment());
    }