From 2550329ab5832c0d2773d209528af8850959e93f Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 4 Sep 2017 16:12:20 +0200 Subject: [PATCH] switch etag cache check to purely client-side Instead of including the etag in the HTTP GET request and letting the server set the Response Code depending on whether the etag machines, this makes the client first issue a HEAD request, which is uses to get the etag and the file size. We need to do the HEAD beforehand anyway to get the file size for resumable downloads, and this approach prevents the server from using the etag as a form of tracking cookie: http://lucb1e.com/rp/cookielesscookies/ closes #562 --- .../org/fdroid/fdroid/IndexV1Updater.java | 5 -- .../java/org/fdroid/fdroid/RepoUpdater.java | 6 -- .../fdroid/net/BluetoothDownloader.java | 8 --- .../org/fdroid/fdroid/net/Downloader.java | 6 -- .../org/fdroid/fdroid/net/HttpDownloader.java | 67 +++++++------------ .../fdroid/net/LocalFileDownloader.java | 5 -- 6 files changed, 23 insertions(+), 74 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java b/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java index bf12dad1a..b199ebbb4 100644 --- a/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java +++ b/app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java @@ -89,11 +89,6 @@ public class IndexV1Updater extends RepoUpdater { if (downloader.isNotFound()) { return false; } - if (downloader.isCached()) { - // The index is unchanged since we last read it. We just mark - // everything that came from this repo as being updated. - Utils.debugLog(TAG, "Repo index for " + dataUri + " is up to date (by etag)"); - } hasChanged = downloader.hasChanged(); if (!hasChanged) { diff --git a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java index b76475c03..2ac6e7da9 100644 --- a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java +++ b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java @@ -132,12 +132,6 @@ public class RepoUpdater { downloader.setListener(downloadListener); downloader.download(); - if (downloader.isCached()) { - // The index is unchanged since we last read it. We just mark - // everything that came from this repo as being updated. - Utils.debugLog(TAG, "Repo index for " + indexUrl + " is up to date (by etag)"); - } - } catch (IOException e) { if (downloader != null && downloader.outputFile != null) { if (!downloader.outputFile.delete()) { diff --git a/app/src/main/java/org/fdroid/fdroid/net/BluetoothDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/BluetoothDownloader.java index 7e743574e..8ab0c4704 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/BluetoothDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/BluetoothDownloader.java @@ -84,14 +84,6 @@ public class BluetoothDownloader extends Downloader { connection.closeQuietly(); } - @Override - public boolean isCached() { - FileDetails details = getFileDetails(); - return details != null && - details.getCacheTag() != null && - details.getCacheTag().equals(getCacheTag()); - } - @Override protected void close() { if (connection != null) { diff --git a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java index 00f237a3e..1ef195bb7 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java @@ -75,18 +75,12 @@ public abstract class Downloader { this.cacheTag = cacheTag; } - boolean wantToCheckCache() { - return cacheTag != null; - } - public abstract boolean hasChanged(); protected abstract int totalDownloadSize(); public abstract void download() throws IOException, InterruptedException; - public abstract boolean isCached(); - /** * @return whether the requested file was not found in the repo (e.g. HTTP 404 Not Found) */ diff --git a/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java index de4b272e7..5a2818098 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java @@ -1,7 +1,8 @@ package org.fdroid.fdroid.net; +import android.text.TextUtils; import com.nostra13.universalimageloader.core.download.BaseImageDownloader; - +import info.guardianproject.netcipher.NetCipher; import org.apache.commons.io.FileUtils; import org.fdroid.fdroid.BuildConfig; import org.fdroid.fdroid.FDroidApp; @@ -17,18 +18,15 @@ import java.net.HttpURLConnection; import java.net.MalformedURLException; import java.net.URL; -import info.guardianproject.netcipher.NetCipher; - public class HttpDownloader extends Downloader { private static final String TAG = "HttpDownloader"; - private static final String HEADER_IF_NONE_MATCH = "If-None-Match"; private static final String HEADER_FIELD_ETAG = "ETag"; private final String username; private final String password; private HttpURLConnection connection; - private int statusCode = -1; + private boolean newFileAvailableOnServer; HttpDownloader(URL url, File destFile) throws FileNotFoundException, MalformedURLException { @@ -70,24 +68,33 @@ public class HttpDownloader extends Downloader { } /** - * Get a remote file, checking the HTTP response code. If 'etag' is not - * {@code null}, it's passed to the server as an If-None-Match header, in - * which case expect a 304 response if nothing changed. In the event of a - * 200 response ONLY, 'retag' (which should be passed empty) may contain - * an etag value for the response, or it may be left empty if none was - * available. + * Get a remote file, checking the HTTP response code and the {@code etag}. + * In order to prevent the {@code etag} from being used as a form of tracking + * cookie, this code never sends the {@code etag} to the server. Instead, it + * uses a {@code HEAD} request to get the {@code etag} from the server, then + * only issues a {@code GET} if the {@code etag} has changed. + * + * @see Cookieless cookies */ @Override public void download() throws IOException, InterruptedException { // get the file size from the server HttpURLConnection tmpConn = getConnection(); tmpConn.setRequestMethod("HEAD"); + String etag = tmpConn.getHeaderField(HEADER_FIELD_ETAG); + int contentLength = -1; int statusCode = tmpConn.getResponseCode(); tmpConn.disconnect(); + newFileAvailableOnServer = false; switch (statusCode) { case 200: contentLength = tmpConn.getContentLength(); + if (!TextUtils.isEmpty(etag) && etag.equals(cacheTag)) { + Utils.debugLog(TAG, sourceUrl + " is cached, not downloading"); + return; + } + newFileAvailableOnServer = true; break; case 404: notFound = true; @@ -96,6 +103,7 @@ public class HttpDownloader extends Downloader { Utils.debugLog(TAG, "HEAD check of " + sourceUrl + " returned " + statusCode + ": " + tmpConn.getResponseMessage()); } + boolean resumable = false; long fileLength = outputFile.length(); if (fileLength > contentLength) { @@ -106,7 +114,9 @@ public class HttpDownloader extends Downloader { resumable = true; } setupConnection(resumable); - doDownload(resumable); + Utils.debugLog(TAG, "downloading " + sourceUrl + " (is resumable: " + resumable + ")"); + downloadFromStream(8192, resumable); + cacheTag = connection.getHeaderField(HEADER_FIELD_ETAG); } private boolean isSwapUrl() { @@ -147,37 +157,6 @@ public class HttpDownloader extends Downloader { } } - private void doDownload(boolean resumable) throws IOException, InterruptedException { - if (wantToCheckCache()) { - setupCacheCheck(); - Utils.debugLog(TAG, "Checking cached status of " + sourceUrl); - statusCode = connection.getResponseCode(); - } - - if (isCached()) { - Utils.debugLog(TAG, sourceUrl + " is cached, so not downloading (HTTP " + statusCode + ")"); - } else { - Utils.debugLog(TAG, "Need to download " + sourceUrl + " (is resumable: " + resumable + ")"); - downloadFromStream(8192, resumable); - updateCacheCheck(); - } - } - - @Override - public boolean isCached() { - return wantToCheckCache() && statusCode == 304; - } - - private void setupCacheCheck() { - if (cacheTag != null) { - connection.setRequestProperty(HEADER_IF_NONE_MATCH, cacheTag); - } - } - - private void updateCacheCheck() { - cacheTag = connection.getHeaderField(HEADER_FIELD_ETAG); - } - // Testing in the emulator for me, showed that figuring out the // filesize took about 1 to 1.5 seconds. // To put this in context, downloading a repo of: @@ -193,7 +172,7 @@ public class HttpDownloader extends Downloader { @Override public boolean hasChanged() { - return this.statusCode != 304; + return newFileAvailableOnServer; } @Override diff --git a/app/src/main/java/org/fdroid/fdroid/net/LocalFileDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/LocalFileDownloader.java index e684aa693..d9d85e5d6 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/LocalFileDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/LocalFileDownloader.java @@ -49,9 +49,4 @@ public class LocalFileDownloader extends Downloader { notFound = true; } } - - @Override - public boolean isCached() { - return false; - } }