Add test zips and extensive tests for ImportExportManager

Now all possible combinations of files in the zip (present or not) are checked at the same time

app/src/main/java/org/schabi/newpipe/settings/export/ImportExportManager.kt

@@ -9,6 +9,7 @@ import com.grack.nanojson.JsonWriter
 import org.schabi.newpipe.streams.io.SharpOutputStream
 import org.schabi.newpipe.streams.io.StoredFileHelper
 import org.schabi.newpipe.util.ZipHelper
+import java.io.FileNotFoundException
 import java.io.IOException
 import java.io.ObjectOutputStream
 import java.util.zip.ZipOutputStream
@@ -110,10 +111,12 @@ class ImportExportManager(private val fileLocator: BackupFileLocator) {
     fun loadSerializedPrefs(zipFile: StoredFileHelper, preferences: SharedPreferences) {
         ZipHelper.extractFileFromZip(zipFile, BackupFileLocator.FILE_NAME_SERIALIZED_PREFS) {
             PreferencesObjectInputStream(it).use { input ->
-                val editor = preferences.edit()
-                editor.clear()
                 @Suppress("UNCHECKED_CAST")
                 val entries = input.readObject() as Map<String, *>
+
+                val editor = preferences.edit()
+                editor.clear()
+
                 for ((key, value) in entries) {
                     when (value) {
                         is Boolean -> editor.putBoolean(key, value)
@@ -133,19 +136,24 @@ class ImportExportManager(private val fileLocator: BackupFileLocator) {
                     Log.e(TAG, "Unable to loadSerializedPrefs")
                 }
             }
+        }.let { fileExists ->
+            if (!fileExists) {
+                throw FileNotFoundException(BackupFileLocator.FILE_NAME_SERIALIZED_PREFS)
+            }
         }
     }
 
     /**
      * Remove all shared preferences from the app and load the preferences supplied to the manager.
      */
-    @Throws(JsonParserException::class)
+    @Throws(IOException::class, JsonParserException::class)
     fun loadJsonPrefs(zipFile: StoredFileHelper, preferences: SharedPreferences) {
         ZipHelper.extractFileFromZip(zipFile, BackupFileLocator.FILE_NAME_JSON_PREFS) {
+            val jsonObject = JsonParser.`object`().from(it)
+
             val editor = preferences.edit()
             editor.clear()
 
-            val jsonObject = JsonParser.`object`().from(it)
             for ((key, value) in jsonObject) {
                 when (value) {
                     is Boolean -> editor.putBoolean(key, value)
@@ -162,6 +170,10 @@ class ImportExportManager(private val fileLocator: BackupFileLocator) {
             if (!editor.commit()) {
                 Log.e(TAG, "Unable to loadJsonPrefs")
             }
+        }.let { fileExists ->
+            if (!fileExists) {
+                throw FileNotFoundException(BackupFileLocator.FILE_NAME_JSON_PREFS)
+            }
         }
     }
 }

app/src/test/java/org/schabi/newpipe/settings/ImportAllCombinationsTest.kt

@@ -0,0 +1,184 @@
+package org.schabi.newpipe.settings
+
+import android.content.SharedPreferences
+import org.junit.Assert
+import org.junit.Test
+import org.mockito.Mockito
+import org.schabi.newpipe.settings.export.BackupFileLocator
+import org.schabi.newpipe.settings.export.ImportExportManager
+import org.schabi.newpipe.streams.io.StoredFileHelper
+import us.shandian.giga.io.FileStream
+import java.io.File
+import java.io.IOException
+import java.nio.file.Files
+
+class ImportAllCombinationsTest {
+
+    companion object {
+        private val classloader = ImportExportManager::class.java.classLoader!!
+    }
+
+    private enum class Ser(val id: String) {
+        YES("ser"),
+        VULNERABLE("vulnser"),
+        NO("noser");
+    }
+
+    private data class FailData(
+        val containsDb: Boolean,
+        val containsSer: Ser,
+        val containsJson: Boolean,
+        val filename: String,
+        val throwable: Throwable,
+    )
+
+    private fun testZipCombination(
+        containsDb: Boolean,
+        containsSer: Ser,
+        containsJson: Boolean,
+        filename: String,
+        runTest: (test: () -> Unit) -> Unit,
+    ) {
+        val zipFile = File(classloader.getResource(filename)?.file!!)
+        val zip = Mockito.mock(StoredFileHelper::class.java, Mockito.withSettings().stubOnly())
+        Mockito.`when`(zip.stream).then { FileStream(zipFile) }
+
+        val fileLocator = Mockito.mock(
+            BackupFileLocator::class.java,
+            Mockito.withSettings().stubOnly()
+        )
+        val db = File.createTempFile("newpipe_", "")
+        val dbJournal = File.createTempFile("newpipe_", "")
+        val dbWal = File.createTempFile("newpipe_", "")
+        val dbShm = File.createTempFile("newpipe_", "")
+        Mockito.`when`(fileLocator.db).thenReturn(db)
+        Mockito.`when`(fileLocator.dbJournal).thenReturn(dbJournal)
+        Mockito.`when`(fileLocator.dbShm).thenReturn(dbShm)
+        Mockito.`when`(fileLocator.dbWal).thenReturn(dbWal)
+
+        if (containsDb) {
+            runTest {
+                Assert.assertTrue(ImportExportManager(fileLocator).extractDb(zip))
+                Assert.assertFalse(dbJournal.exists())
+                Assert.assertFalse(dbWal.exists())
+                Assert.assertFalse(dbShm.exists())
+                Assert.assertTrue("database file size is zero", Files.size(db.toPath()) > 0)
+            }
+        } else {
+            runTest {
+                Assert.assertFalse(ImportExportManager(fileLocator).extractDb(zip))
+                Assert.assertTrue(dbJournal.exists())
+                Assert.assertTrue(dbWal.exists())
+                Assert.assertTrue(dbShm.exists())
+                Assert.assertEquals(0, Files.size(db.toPath()))
+            }
+        }
+
+        val preferences = Mockito.mock(SharedPreferences::class.java, Mockito.withSettings().stubOnly())
+        var editor = Mockito.mock(SharedPreferences.Editor::class.java)
+        Mockito.`when`(preferences.edit()).thenReturn(editor)
+        Mockito.`when`(editor.commit()).thenReturn(true)
+
+        when (containsSer) {
+            Ser.YES -> runTest {
+                Assert.assertTrue(ImportExportManager(fileLocator).exportHasSerializedPrefs(zip))
+                ImportExportManager(fileLocator).loadSerializedPrefs(zip, preferences)
+
+                Mockito.verify(editor, Mockito.times(1)).clear()
+                Mockito.verify(editor, Mockito.times(1)).commit()
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putBoolean(Mockito.anyString(), Mockito.anyBoolean())
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putString(Mockito.anyString(), Mockito.anyString())
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putInt(Mockito.anyString(), Mockito.anyInt())
+            }
+            Ser.VULNERABLE -> runTest {
+                Assert.assertTrue(ImportExportManager(fileLocator).exportHasSerializedPrefs(zip))
+                Assert.assertThrows(ClassNotFoundException::class.java) {
+                    ImportExportManager(fileLocator).loadSerializedPrefs(zip, preferences)
+                }
+
+                Mockito.verify(editor, Mockito.never()).clear()
+                Mockito.verify(editor, Mockito.never()).commit()
+            }
+            Ser.NO -> runTest {
+                Assert.assertFalse(ImportExportManager(fileLocator).exportHasSerializedPrefs(zip))
+                Assert.assertThrows(IOException::class.java) {
+                    ImportExportManager(fileLocator).loadSerializedPrefs(zip, preferences)
+                }
+
+                Mockito.verify(editor, Mockito.never()).clear()
+                Mockito.verify(editor, Mockito.never()).commit()
+            }
+        }
+
+        // recreate editor mock so verify() behaves correctly
+        editor = Mockito.mock(SharedPreferences.Editor::class.java)
+        Mockito.`when`(preferences.edit()).thenReturn(editor)
+        Mockito.`when`(editor.commit()).thenReturn(true)
+
+        if (containsJson) {
+            runTest {
+                Assert.assertTrue(ImportExportManager(fileLocator).exportHasJsonPrefs(zip))
+                ImportExportManager(fileLocator).loadJsonPrefs(zip, preferences)
+
+                Mockito.verify(editor, Mockito.times(1)).clear()
+                Mockito.verify(editor, Mockito.times(1)).commit()
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putBoolean(Mockito.anyString(), Mockito.anyBoolean())
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putString(Mockito.anyString(), Mockito.anyString())
+                Mockito.verify(editor, Mockito.atLeastOnce())
+                    .putInt(Mockito.anyString(), Mockito.anyInt())
+            }
+        } else {
+            runTest {
+                Assert.assertFalse(ImportExportManager(fileLocator).exportHasJsonPrefs(zip))
+                Assert.assertThrows(IOException::class.java) {
+                    ImportExportManager(fileLocator).loadJsonPrefs(zip, preferences)
+                }
+
+                Mockito.verify(editor, Mockito.never()).clear()
+                Mockito.verify(editor, Mockito.never()).commit()
+            }
+        }
+    }
+
+    @Test
+    fun `Importing all possible combinations of zip files`() {
+        val failedAssertions = mutableListOf<FailData>()
+        for (containsDb in listOf(true, false)) {
+            for (containsSer in Ser.entries) {
+                for (containsJson in listOf(true, false)) {
+                    val filename = "settings/${if (containsDb) "db" else "nodb"}_${
+                    containsSer.id}_${if (containsJson) "json" else "nojson"}.zip"
+                    testZipCombination(containsDb, containsSer, containsJson, filename) { test ->
+                        try {
+                            test()
+                        } catch (e: Throwable) {
+                            failedAssertions.add(
+                                FailData(
+                                    containsDb, containsSer, containsJson,
+                                    filename, e
+                                )
+                            )
+                        }
+                    }
+                }
+            }
+        }
+
+        if (failedAssertions.isNotEmpty()) {
+            for (a in failedAssertions) {
+                println(
+                    "Assertion failed with containsDb=${a.containsDb}, containsSer=${
+                    a.containsSer}, containsJson=${a.containsJson}, filename=${a.filename}:"
+                )
+                a.throwable.printStackTrace()
+                println()
+            }
+            Assert.fail("${failedAssertions.size} assertions failed")
+        }
+    }
+}

app/src/test/java/org/schabi/newpipe/settings/ImportExportManagerTest.kt

@@ -109,7 +109,7 @@ class ImportExportManagerTest {
         `when`(fileLocator.dbShm).thenReturn(dbShm)
         `when`(fileLocator.dbWal).thenReturn(dbWal)
 
-        val zip = File(classloader.getResource("settings/newpipe.zip")?.file!!)
+        val zip = File(classloader.getResource("settings/db_ser_json.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(zip))
         val success = ImportExportManager(fileLocator).extractDb(storedFileHelper)
 
@@ -128,7 +128,7 @@ class ImportExportManagerTest {
         val dbShm = File.createTempFile("newpipe_", "")
         `when`(fileLocator.db).thenReturn(db)
 
-        val emptyZip = File(classloader.getResource("settings/empty.zip")?.file!!)
+        val emptyZip = File(classloader.getResource("settings/nodb_noser_nojson.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(emptyZip))
         val success = ImportExportManager(fileLocator).extractDb(storedFileHelper)
 
@@ -141,21 +141,21 @@ class ImportExportManagerTest {
 
     @Test
     fun `Contains setting must return true if a settings file exists in the zip`() {
-        val zip = File(classloader.getResource("settings/newpipe.zip")?.file!!)
+        val zip = File(classloader.getResource("settings/db_ser_json.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(zip))
         assertTrue(ImportExportManager(fileLocator).exportHasSerializedPrefs(storedFileHelper))
     }
 
     @Test
-    fun `Contains setting must return false if a no settings file exists in the zip`() {
+    fun `Contains setting must return false if no settings file exists in the zip`() {
-        val emptyZip = File(classloader.getResource("settings/empty.zip")?.file!!)
+        val emptyZip = File(classloader.getResource("settings/nodb_noser_nojson.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(emptyZip))
         assertFalse(ImportExportManager(fileLocator).exportHasSerializedPrefs(storedFileHelper))
     }
 
     @Test
     fun `Preferences must be set from the settings file`() {
-        val zip = File(classloader.getResource("settings/newpipe.zip")?.file!!)
+        val zip = File(classloader.getResource("settings/db_ser_json.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(zip))
 
         val preferences = Mockito.mock(SharedPreferences::class.java, withSettings().stubOnly())
@@ -172,12 +172,10 @@ class ImportExportManagerTest {
 
     @Test
     fun `Importing preferences with a serialization injected class should fail`() {
-        val emptyZip = File(classloader.getResource("settings/vulnerable_serialization.zip")?.file!!)
+        val emptyZip = File(classloader.getResource("settings/db_vulnser_json.zip")?.file!!)
         `when`(storedFileHelper.stream).thenReturn(FileStream(emptyZip))
 
         val preferences = Mockito.mock(SharedPreferences::class.java, withSettings().stubOnly())
-        val editor = Mockito.mock(SharedPreferences.Editor::class.java)
-        `when`(preferences.edit()).thenReturn(editor)
 
         assertThrows(ClassNotFoundException::class.java) {
             ImportExportManager(fileLocator).loadSerializedPrefs(storedFileHelper, preferences)

app/src/test/resources/settings/README.md

@@ -0,0 +1,4 @@
+`*.zip` files in this folder are NewPipe database exports, in all possible configurations:
+- `db` / `nodb` indicates if there is a `newpipe.db` database included or not
+- `ser` / `vulnser` / `noser` indicates if there is a `newpipe.settings` Java-serialized preferences file included, if it is included and contains an injection attack, of if it is not included
+- `json` / `nojson` indicates if there is a `preferences.json` JSON preferences file included or not