fix(addons): improve addon validation [Dev-only]

AnkiDroid/src/main/java/com/ichi2/anki/jsaddons/AddonData.kt

@@ -1,18 +1,5 @@
-/*
- * Copyright (c) 2021 Mani <infinyte01@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify it under
- * the terms of the GNU General Public License as published by the Free Software
- * Foundation; either version 3 of the License, or (at your option) any later
- * version.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT ANY
- * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
- * PARTICULAR PURPOSE. See the GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
+// SPDX-License-Identifier: GPL-3.0-or-later
+// SPDX-FileCopyrightText: Copyright (c) 2021 Mani <infinyte01@gmail.com>
 
 package com.ichi2.anki.jsaddons
 
@@ -40,6 +27,11 @@ import kotlin.jvm.Throws
  * @param name name of npm package, it unique for each package listed on npm
  * @param addonTitle  for showing in AnkiDroid
  * @param icon only required for note editor (single character recommended)
+ * @param description commonly absent from real package.json files; absence never invalidates the addon
+ * @param author commonly absent from real package.json files; absence never invalidates the addon
+ * @param license commonly absent from real package.json files; absence never invalidates the addon
+ * @param dist only exists in the npm registry API response, not in the package.json inside a tarball;
+ *   absence never invalidates the addon
  */
 
 @Serializable
@@ -64,87 +56,103 @@ data class DistInfo(
     val tarball: String,
 )
 
+/** Parsing contract for package.json: unknown fields must never fail parsing */
+private val json = Json { ignoreUnknownKeys = true }
+
+/**
+ * Result of validating a manifest ([AddonData]) as an AnkiDroid addon
+ */
+sealed interface AddonValidationResult {
+    /** The manifest maps to a usable [AddonModel] */
+    data class Valid(
+        val addonModel: AddonModel,
+    ) : AddonValidationResult
+
+    /** The manifest is not a valid AnkiDroid addon, for the reasons in [errors] */
+    data class Invalid(
+        val errors: List<String>,
+    ) : AddonValidationResult
+}
+
 /**
  * Check if npm package is valid or not by fields ankidroidJsApi, keywords (ankidroid-js-addon) and
  * addon_type (reviewer or note editor) in addonData
  *
- * For valid addon the error list will be empty,
- * for not valid addon the error list will contain the error related to the checks
+ * Unreadable and unparseable files are reported as [AddonValidationResult.Invalid]: a caller
+ * listing an addons directory must not be aborted by one corrupt addon
  *
- * @param packageJsonPath package.json file path
- * @return Pair with addonModel and error list
+ * @param packageJson package.json file
+ * @return [AddonValidationResult.Valid] with the mapped model,
+ *   or [AddonValidationResult.Invalid] with the errors related to the failed checks
  */
-@Throws(IOException::class)
-fun getAddonModelFromJson(packageJsonPath: String): Pair<AddonModel?, List<String>> {
-    val data = File(packageJsonPath).readBytes().decodeToString()
-    return try {
-        val json = Json { ignoreUnknownKeys = true }
+fun getAddonModelFromJson(packageJson: File): AddonValidationResult =
+    try {
+        val data = packageJson.readBytes().decodeToString()
         getAddonModelFromAddonData(json.decodeFromString(data))
     } catch (exc: SerializationException) {
-        return Pair(null, listOf("Unable to parse manifest: $exc"))
+        AddonValidationResult.Invalid(listOf("Unable to parse manifest: $exc"))
+    } catch (exc: IOException) {
+        AddonValidationResult.Invalid(listOf("Unable to read manifest: $exc"))
     }
-}
 
 /**
  * Get addonModel from addonData
  *
  * @param addonData
- * @return pair of valid addon model and errors list
+ * @return [AddonValidationResult.Valid] with the mapped model,
+ *   or [AddonValidationResult.Invalid] with the errors related to the failed checks
  */
-fun getAddonModelFromAddonData(addonData: AddonData): Pair<AddonModel?, List<String>> {
-    var errorStr: String
-    val errorList: MutableList<String> = ArrayList()
-
+fun getAddonModelFromAddonData(addonData: AddonData): AddonValidationResult {
     // either fields not present in package.json or failed to parse the fields
     if (addonData.name.isNullOrBlank() ||
         addonData.addonTitle.isNullOrBlank() ||
         addonData.main.isNullOrBlank() ||
+        addonData.version.isNullOrBlank() ||
         addonData.ankidroidJsApi.isNullOrBlank() ||
         addonData.addonType.isNullOrBlank() ||
         addonData.homepage.isNullOrBlank() ||
         addonData.keywords.isNullOrEmpty()
     ) {
-        errorStr = "Invalid addon package: fields in package.json are empty or null"
-        errorList.add(errorStr)
+        // the checks below dereference these fields, so they cannot run safely
+        return AddonValidationResult.Invalid(listOf("Invalid addon package: fields in package.json are empty or null"))
     }
 
+    val errorList: MutableList<String> = ArrayList()
+
     // check if name is safe and valid
-    if (!validateName(addonData.name!!)) {
-        errorStr = "Invalid addon package: package name failed validation"
-        errorList.add(errorStr)
+    if (!validateName(addonData.name)) {
+        errorList.add("Invalid addon package: package name failed validation")
     }
 
     if (addonData.addonType != REVIEWER_ADDON && addonData.addonType != NOTE_EDITOR_ADDON) {
-        errorStr = "Invalid addon package: ${addonData.addonType} is not valid addon type, " +
-            "package.json must have 'addonType' fields of 'reviewer' or 'note-editor'"
-        errorList.add(errorStr)
+        errorList.add(
+            "Invalid addon package: ${addonData.addonType} is not valid addon type, " +
+                "package.json must have 'addonType' fields of 'reviewer' or 'note-editor'",
+        )
     }
 
     // if addon type is note editor then it must have icon
     if (addonData.addonType == NOTE_EDITOR_ADDON && addonData.icon.isNullOrBlank()) {
-        errorStr = "Invalid addon package: note editor addon must have 'icon' fields in package.json"
-        errorList.add(errorStr)
+        errorList.add("Invalid addon package: note editor addon must have 'icon' fields in package.json")
     }
 
     // check if ankidroid-js-addon present or not in mapped addonData
-    val jsAddonKeywordsPresent = addonData.keywords?.any { it == ANKIDROID_JS_ADDON_KEYWORDS }
-    if (!jsAddonKeywordsPresent!!) {
-        errorStr = "Invalid addon package: package.json does not have 'ankidroid-js-addon' in ${addonData.keywords} keywords"
-        errorList.add(errorStr)
+    val jsAddonKeywordsPresent = addonData.keywords.any { it == ANKIDROID_JS_ADDON_KEYWORDS }
+    if (!jsAddonKeywordsPresent) {
+        errorList.add("Invalid addon package: package.json does not have 'ankidroid-js-addon' in ${addonData.keywords} keywords")
     }
 
     // Check supplied api and current api
     if (addonData.ankidroidJsApi != CURRENT_JS_API_VERSION) {
-        errorStr = "Invalid addon package: supplied js api version ${addonData.ankidroidJsApi} must " +
-            "be equal to current js api version $CURRENT_JS_API_VERSION"
-        errorList.add(errorStr)
+        errorList.add(
+            "Invalid addon package: supplied js api version ${addonData.ankidroidJsApi} must " +
+                "be equal to current js api version $CURRENT_JS_API_VERSION",
+        )
     }
 
-    val immutableList: List<String> = ArrayList(errorList)
-
-    // there are errors in package.json so return null and errors list
+    // there are errors in package.json so return the errors list
     if (errorList.isNotEmpty()) {
-        return Pair(null, immutableList)
+        return AddonValidationResult.Invalid(errorList)
     }
 
     val icon = if (addonData.addonType == NOTE_EDITOR_ADDON) addonData.icon!! else ""
@@ -153,21 +161,21 @@ fun getAddonModelFromAddonData(addonData: AddonData): Pair<AddonModel?, List<Str
     val addonModel =
         AddonModel(
             name = addonData.name,
-            addonTitle = addonData.addonTitle!!,
+            addonTitle = addonData.addonTitle,
             icon = icon,
-            version = addonData.version!!,
-            description = addonData.description!!,
-            main = addonData.main!!,
-            ankidroidJsApi = addonData.ankidroidJsApi!!,
-            addonType = addonData.addonType!!,
+            version = addonData.version,
+            description = addonData.description.orEmpty(),
+            main = addonData.main,
+            ankidroidJsApi = addonData.ankidroidJsApi,
+            addonType = addonData.addonType,
             keywords = addonData.keywords,
-            author = addonData.author!!,
-            license = addonData.license!!,
-            homepage = addonData.homepage!!,
-            dist = addonData.dist!!,
+            author = addonData.author ?: emptyMap(),
+            license = addonData.license.orEmpty(),
+            homepage = addonData.homepage,
+            dist = addonData.dist,
         )
 
-    return Pair(addonModel, immutableList)
+    return AddonValidationResult.Valid(addonModel)
 }
 
 /**
@@ -185,18 +193,16 @@ fun getAddonModelListFromJson(packageJsonUrl: URL): Pair<List<AddonModel>, List<
             HttpFetcher.fetchThroughHttp(packageJsonUrl.toString())
         }
     val errorList: MutableList<String> = ArrayList()
-    val json = Json { ignoreUnknownKeys = true }
     val addonsData = json.decodeFromString<List<AddonData>>(urlData)
     val addonsModelList = mutableListOf<AddonModel>()
     for (addon in addonsData) {
-        val result = getAddonModelFromAddonData(addon)
-
-        if (result.first == null) {
-            Timber.i("Not a valid addon for AnkiDroid, the errors for the addon:\n %s", result.second)
-            errorList.addAll(result.second)
-            continue
+        when (val result = getAddonModelFromAddonData(addon)) {
+            is AddonValidationResult.Valid -> addonsModelList.add(result.addonModel)
+            is AddonValidationResult.Invalid -> {
+                Timber.i("Not a valid addon for AnkiDroid, the errors for the addon:\n %s", result.errors)
+                errorList.addAll(result.errors)
+            }
         }
-        addonsModelList.add(result.first!!)
     }
 
     return Pair(addonsModelList, errorList.toList())

AnkiDroid/src/main/java/com/ichi2/anki/jsaddons/AddonModel.kt

@@ -1,24 +1,8 @@
-/*
- * Copyright (c) 2021 Mani <infinyte01@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify it under
- * the terms of the GNU General Public License as published by the Free Software
- * Foundation; either version 3 of the License, or (at your option) any later
- * version.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT ANY
- * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
- * PARTICULAR PURPOSE. See the GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
+// SPDX-License-Identifier: GPL-3.0-or-later
+// SPDX-FileCopyrightText: Copyright (c) 2021 Mani <infinyte01@gmail.com>
 
 package com.ichi2.anki.jsaddons
 
-import android.content.SharedPreferences
-import androidx.core.content.edit
-
 data class AddonModel(
     val name: String,
     val addonTitle: String,
@@ -32,35 +16,6 @@ data class AddonModel(
     val author: Map<String, String>,
     val license: String,
     val homepage: String,
-    val dist: DistInfo,
-) {
-    /**
-     * Update preferences for addons with boolean remove, the preferences will be used to store the information about
-     * enabled and disabled addon. So, that other method will return content of script to reviewer or note editor
-     *
-     * @param preferences
-     * @param jsAddonKey  REVIEWER_ADDON_KEY
-     * @param remove    true for removing from prefs
-     *
-     * Android returns a reference to StringSet in SharedPreferences but does not mark it as modified if any changes made,
-     * so commit/apply won't persist it. It needs to make a copy and set the new copy in to persist any StringSet changes
-     * in SharedPreferences.
-     * https://stackoverflow.com/questions/19949182/android-sharedpreferences-string-set-some-items-are-removed-after-app-restart/19949833
-     */
-    fun updatePrefs(
-        preferences: SharedPreferences,
-        jsAddonKey: String,
-        remove: Boolean,
-    ) {
-        val reviewerEnabledAddonSet = preferences.getStringSet(jsAddonKey, HashSet())
-        val newStrSet: MutableSet<String> = reviewerEnabledAddonSet?.toHashSet()!!
-
-        if (remove) {
-            newStrSet.remove(name)
-        } else {
-            newStrSet.add(name)
-        }
-
-        preferences.edit { putStringSet(jsAddonKey, newStrSet) }
-    }
-}
+    /** Tarball location from the npm registry API; null for locally installed addons */
+    val dist: DistInfo?,
+)

AnkiDroid/src/main/java/com/ichi2/anki/jsaddons/AddonsConst.kt

@@ -1,18 +1,5 @@
-/*
- * Copyright (c) 2021 Mani <infinyte01@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify it under
- * the terms of the GNU General Public License as published by the Free Software
- * Foundation; either version 3 of the License, or (at your option) any later
- * version.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT ANY
- * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
- * PARTICULAR PURPOSE. See the GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
+// SPDX-License-Identifier: GPL-3.0-or-later
+// SPDX-FileCopyrightText: Copyright (c) 2021 Mani <infinyte01@gmail.com>
 
 package com.ichi2.anki.jsaddons
 

AnkiDroid/src/main/java/com/ichi2/anki/jsaddons/NpmUtils.kt

@@ -1,18 +1,5 @@
-/*
- * Copyright (c) 2021 Mani <infinyte01@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify it under
- * the terms of the GNU General Public License as published by the Free Software
- * Foundation; either version 3 of the License, or (at your option) any later
- * version.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT ANY
- * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
- * PARTICULAR PURPOSE. See the GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
+// SPDX-License-Identifier: GPL-3.0-or-later
+// SPDX-FileCopyrightText: Copyright (c) 2021 Mani <infinyte01@gmail.com>
 
 package com.ichi2.anki.jsaddons
 

AnkiDroid/src/main/java/com/ichi2/anki/jsaddons/TgzPackageExtract.kt

@@ -36,7 +36,6 @@ package com.ichi2.anki.jsaddons
 import android.content.Context
 import android.text.format.Formatter
 import com.ichi2.anki.R
-import com.ichi2.anki.common.utils.android.showThemedToast
 import com.ichi2.anki.compat.CompatHelper.Companion.compat
 import com.ichi2.utils.FileUtil
 import org.apache.commons.compress.archivers.ArchiveException
@@ -135,9 +134,8 @@ class TgzPackageExtract(
         try {
             compat.createDirectories(addonsPackageDir)
         } catch (e: IOException) {
-            showThemedToast(context, context.getString(R.string.could_not_create_dir, addonsPackageDir.absolutePath), false)
             Timber.w(e)
-            return
+            throw IOException(context.getString(R.string.could_not_create_dir, addonsPackageDir.absolutePath))
         }
 
         // Make sure we have 2x the tar file size in free space (1x for tar file, 1x for unarchived tar file contents
@@ -156,9 +154,12 @@ class TgzPackageExtract(
         try {
             // If space available then unTar it
             unTar(tarTempFile, addonsPackageDir)
-        } catch (e: IOException) {
-            Timber.w("Failed to unTar file")
+        } catch (e: Exception) {
+            // ArchiveException/IllegalStateException from unTar need the same cleanup as
+            // IOException: without it, a failed extraction leaves a partial addon behind
+            Timber.w(e, "Failed to unTar file")
             safeDeleteAddonsPackageDir(addonsPackageDir)
+            throw e
         } finally {
             tarTempFile.delete()
         }
@@ -421,8 +422,12 @@ class TgzPackageExtract(
         }
     }
 
+    /**
+     * Recursively delete [addonsPackageDir], but only if it is inside an `addons` directory:
+     * a defence against deleting an arbitrary directory if a bad path is passed in
+     */
     private fun safeDeleteAddonsPackageDir(addonsPackageDir: AddonsPackageDir) {
-        if (addonsPackageDir.parent != "addons") {
+        if (addonsPackageDir.parentFile?.name != "addons") {
             return
         }
         addonsPackageDir.deleteRecursively()