From 2926cb76823abf95881ee25cf331680988231acf Mon Sep 17 00:00:00 2001 From: XiangRongLin <41164160+XiangRongLin@users.noreply.github.com> Date: Sat, 23 Jan 2021 09:02:11 +0100 Subject: [PATCH 1/3] Respect expires header when checking for new version It was called to many times and acted similar to a DOS attack. --- .../schabi/newpipe/CheckForNewAppVersion.java | 63 +++++++++------- .../org/schabi/newpipe/NewVersionManager.kt | 28 ++++++++ app/src/main/res/values/settings_keys.xml | 1 + .../schabi/newpipe/NewVersionManagerTest.kt | 72 +++++++++++++++++++ 4 files changed, 139 insertions(+), 25 deletions(-) create mode 100644 app/src/main/java/org/schabi/newpipe/NewVersionManager.kt create mode 100644 app/src/test/java/org/schabi/newpipe/NewVersionManagerTest.kt diff --git a/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java b/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java index f8497ea27..630fb01ff 100644 --- a/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java +++ b/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java @@ -10,22 +10,19 @@ import android.content.pm.Signature; import android.net.ConnectivityManager; import android.net.Uri; import android.util.Log; - import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.core.app.NotificationCompat; import androidx.core.app.NotificationManagerCompat; import androidx.core.content.ContextCompat; import androidx.preference.PreferenceManager; - import com.grack.nanojson.JsonObject; import com.grack.nanojson.JsonParser; import com.grack.nanojson.JsonParserException; - -import org.schabi.newpipe.report.ErrorActivity; -import org.schabi.newpipe.report.ErrorInfo; -import org.schabi.newpipe.report.UserAction; - +import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers; +import io.reactivex.rxjava3.core.Maybe; +import io.reactivex.rxjava3.disposables.Disposable; +import io.reactivex.rxjava3.schedulers.Schedulers; import java.io.ByteArrayInputStream; import java.io.InputStream; import java.security.MessageDigest; @@ -34,11 +31,9 @@ import java.security.cert.CertificateEncodingException; import java.security.cert.CertificateException; import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; - -import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers; -import io.reactivex.rxjava3.core.Maybe; -import io.reactivex.rxjava3.disposables.Disposable; -import io.reactivex.rxjava3.schedulers.Schedulers; +import org.schabi.newpipe.report.ErrorActivity; +import org.schabi.newpipe.report.ErrorInfo; +import org.schabi.newpipe.report.UserAction; public final class CheckForNewAppVersion { private CheckForNewAppVersion() { } @@ -176,6 +171,7 @@ public final class CheckForNewAppVersion { @Nullable public static Disposable checkNewVersion(@NonNull final App app) { final SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(app); + final NewVersionManager manager = new NewVersionManager(); // Check if user has enabled/disabled update checking // and if the current apk is a github one or not. @@ -183,31 +179,48 @@ public final class CheckForNewAppVersion { return null; } - return Maybe - .fromCallable(() -> { - if (!isConnected(app)) { - return null; - } + final long expiry = prefs.getLong(app.getString(R.string.update_expiry_key), 0); + if (manager.isExpired(expiry)) { + return null; + } - // Make a network request to get latest NewPipe data. - return DownloaderImpl.getInstance().get(NEWPIPE_API_URL).responseBody(); - }) + return Maybe + .fromCallable(() -> { + if (!isConnected(app)) { + return null; + } + + // Make a network request to get latest NewPipe data. + return DownloaderImpl.getInstance().get(NEWPIPE_API_URL); + }) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe( response -> { + try { + final long newExpiry = manager + .coerceExpiry(response.getHeader("expires")); + prefs.edit() + .putLong(app.getString(R.string.update_expiry_key), newExpiry) + .apply(); + } catch (final Exception e) { + if (DEBUG) { + Log.w(TAG, "Could not extract and save new expiry date", e); + } + } + // Parse the json from the response. try { final JsonObject githubStableObject = JsonParser.object() - .from(response).getObject("flavors").getObject("github") - .getObject("stable"); + .from(response.responseBody()).getObject("flavors") + .getObject("github").getObject("stable"); final String versionName = githubStableObject - .getString("version"); + .getString("version"); final int versionCode = githubStableObject - .getInt("version_code"); + .getInt("version_code"); final String apkLocationUrl = githubStableObject - .getString("apk"); + .getString("apk"); compareAppVersionAndShowNotification(app, versionName, apkLocationUrl, versionCode); diff --git a/app/src/main/java/org/schabi/newpipe/NewVersionManager.kt b/app/src/main/java/org/schabi/newpipe/NewVersionManager.kt new file mode 100644 index 000000000..36de1ecfc --- /dev/null +++ b/app/src/main/java/org/schabi/newpipe/NewVersionManager.kt @@ -0,0 +1,28 @@ +package org.schabi.newpipe + +import java.time.Instant +import java.time.ZonedDateTime +import java.time.format.DateTimeFormatter + +class NewVersionManager { + + fun isExpired(expiry: Long): Boolean { + return Instant.ofEpochSecond(expiry).isBefore(Instant.now()) + } + + /** + * Coerce expiry date time in between 6 hours and 72 hours from now + * + * @return Epoch second of expiry date time + */ + fun coerceExpiry(expiryString: String?): Long { + val now = ZonedDateTime.now() + return expiryString?.let { + + var expiry = ZonedDateTime.from(DateTimeFormatter.RFC_1123_DATE_TIME.parse(expiryString)) + expiry = maxOf(expiry, now.plusHours(6)) + expiry = minOf(expiry, now.plusHours(72)) + expiry.toEpochSecond() + } ?: now.plusHours(6).toEpochSecond() + } +} diff --git a/app/src/main/res/values/settings_keys.xml b/app/src/main/res/values/settings_keys.xml index 1c4ad11a3..011bd850a 100644 --- a/app/src/main/res/values/settings_keys.xml +++ b/app/src/main/res/values/settings_keys.xml @@ -342,6 +342,7 @@ update_app_key update_pref_screen_key + update_expiry_key system diff --git a/app/src/test/java/org/schabi/newpipe/NewVersionManagerTest.kt b/app/src/test/java/org/schabi/newpipe/NewVersionManagerTest.kt new file mode 100644 index 000000000..d2dacc783 --- /dev/null +++ b/app/src/test/java/org/schabi/newpipe/NewVersionManagerTest.kt @@ -0,0 +1,72 @@ +package org.schabi.newpipe + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import java.time.Instant +import java.time.ZoneId +import java.time.format.DateTimeFormatter +import kotlin.math.abs + +class NewVersionManagerTest { + + private lateinit var manager: NewVersionManager + + @Before + fun setup() { + manager = NewVersionManager() + } + + @Test + fun `Expiry is reached`() { + val oneHourEarlier = Instant.now().atZone(ZoneId.of("GMT")).minusHours(1) + + val expired = manager.isExpired(oneHourEarlier.toEpochSecond()) + + assertTrue(expired) + } + + @Test + fun `Expiry is not reached`() { + val oneHourLater = Instant.now().atZone(ZoneId.of("GMT")).plusHours(1) + + val expired = manager.isExpired(oneHourLater.toEpochSecond()) + + assertFalse(expired) + } + + /** + * Equal within a range of 5 seconds + */ + private fun assertNearlyEqual(a: Long, b: Long) { + assertTrue(abs(a - b) < 5) + } + + @Test + fun `Expiry must be returned as is because it is inside the acceptable range of 6-72 hours`() { + val sixHoursLater = Instant.now().atZone(ZoneId.of("GMT")).plusHours(6) + + val coerced = manager.coerceExpiry(DateTimeFormatter.RFC_1123_DATE_TIME.format(sixHoursLater)) + + assertNearlyEqual(sixHoursLater.toEpochSecond(), coerced) + } + + @Test + fun `Expiry must be increased to 6 hours if below`() { + val tooLow = Instant.now().atZone(ZoneId.of("GMT")).plusHours(5) + + val coerced = manager.coerceExpiry(DateTimeFormatter.RFC_1123_DATE_TIME.format(tooLow)) + + assertNearlyEqual(tooLow.plusHours(1).toEpochSecond(), coerced) + } + + @Test + fun `Expiry must be decreased to 72 hours if above`() { + val tooHigh = Instant.now().atZone(ZoneId.of("GMT")).plusHours(73) + + val coerced = manager.coerceExpiry(DateTimeFormatter.RFC_1123_DATE_TIME.format(tooHigh)) + + assertNearlyEqual(tooHigh.minusHours(1).toEpochSecond(), coerced) + } +} From e98838ad7eda4908e79b38087750ba6f11993f70 Mon Sep 17 00:00:00 2001 From: XiangRongLin <41164160+XiangRongLin@users.noreply.github.com> Date: Sat, 30 Jan 2021 09:32:17 +0100 Subject: [PATCH 2/3] Invert usage of manager.isExpired() When it's expired it means, that the app should get the data. Meaning it should not abort prematurely by returning null. Co-authored-by: Tobias Groza --- app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java b/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java index 630fb01ff..8bea85b19 100644 --- a/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java +++ b/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java @@ -180,7 +180,7 @@ public final class CheckForNewAppVersion { } final long expiry = prefs.getLong(app.getString(R.string.update_expiry_key), 0); - if (manager.isExpired(expiry)) { + if (!manager.isExpired(expiry)) { return null; } From bdc85b435ca3d8937eeb366e5a0b875c29a415da Mon Sep 17 00:00:00 2001 From: XiangRongLin <41164160+XiangRongLin@users.noreply.github.com> Date: Sat, 30 Jan 2021 14:24:25 +0100 Subject: [PATCH 3/3] Add comments explaining the expiry field Co-authored-by: Tobias Groza --- .../main/java/org/schabi/newpipe/CheckForNewAppVersion.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java b/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java index 8bea85b19..63baef547 100644 --- a/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java +++ b/app/src/main/java/org/schabi/newpipe/CheckForNewAppVersion.java @@ -179,6 +179,8 @@ public final class CheckForNewAppVersion { return null; } + // Check if the last request has happened a certain time ago + // to reduce the number of API requests. final long expiry = prefs.getLong(app.getString(R.string.update_expiry_key), 0); if (!manager.isExpired(expiry)) { return null; @@ -198,6 +200,8 @@ public final class CheckForNewAppVersion { .subscribe( response -> { try { + // Store a timestamp which needs to be exceeded, + // before a new request to the API is made. final long newExpiry = manager .coerceExpiry(response.getHeader("expires")); prefs.edit()