Harden run ZIP import and fix review findings

.github/workflows/build-and-test.yml

@@ -828,7 +828,10 @@ jobs:
           # moving, not the passage of time: bump kind, then read the support
           # table again.
           kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/controller-v1.15.1/deploy/static/provider/kind/deploy.yaml
-          kubectl wait --namespace ingress-nginx --for=condition=ready pod --selector=app.kubernetes.io/component=controller --timeout=90s
+          # rollout status waits for the controller pod to exist; `kubectl wait`
+          # on a selector fails at once ("no matching resources found") when
+          # it runs before the Deployment has created the pod.
+          kubectl -n ingress-nginx rollout status deployment/ingress-nginx-controller --timeout=180s
 
       - name: Set up Helm
         # kubectl kustomize --enable-helm shells out to `helm` to inflate the

.github/workflows/build-image.yml

@@ -139,8 +139,8 @@ jobs:
           # the template it names the app's own image, and a hardcoded
           # template ref leaves every pod in ErrImageNeverPull.
           overlay=k8s/overlays/prod/kustomization.yaml
-          name=$(awk '$1 == "newName:" {print $2; exit}' "$overlay")
-          tag=$(awk '$1 == "newTag:" {print $2; exit}' "$overlay")
+          name=$(awk '$1 == "newName:" {print $2; exit}' "$overlay" | tr -d "\"'")
+          tag=$(awk '$1 == "newTag:" {print $2; exit}' "$overlay" | tr -d "\"'")
           if [ -z "$name" ] || [ -z "$tag" ]; then
             echo "::error::$overlay has no images newName/newTag to tag the kind image with"
             exit 1

electron/scripts/package.sh

@@ -56,7 +56,11 @@ case "$(uname -s)" in
       export CSC_FOR_PULL_REQUEST=true
       # Notarize with an App Store Connect API key, or else with an Apple ID
       # and an app-specific password.
-      if [[ -n "${APPLE_API_KEY_CONTENT:-}" ]]; then
+      if [[ -n "${APPLE_API_KEY_CONTENT:-}" ]] && { [[ -z "${APPLE_API_KEY_ID:-}" ]] || [[ -z "${APPLE_API_ISSUER:-}" ]]; } && [[ -z "${APPLE_ID:-}" ]]; then
+        echo "::error::APPLE_API_KEY is set, but APPLE_API_KEY_ID or APPLE_API_ISSUER is empty: notarization needs all three"
+        exit 1
+      fi
+      if [[ -n "${APPLE_API_KEY_CONTENT:-}" && -n "${APPLE_API_KEY_ID:-}" && -n "${APPLE_API_ISSUER:-}" ]]; then
         printf '%s\n' "$APPLE_API_KEY_CONTENT" > "${RUNNER_TEMP:-/tmp}/AuthKey.p8"
         trap 'rm -f "${RUNNER_TEMP:-/tmp}/AuthKey.p8"' EXIT
         export APPLE_API_KEY="${RUNNER_TEMP:-/tmp}/AuthKey.p8" APPLE_API_KEY_ID APPLE_API_ISSUER

src/common/common.py

@@ -87,7 +87,9 @@ def get_branding(theme: str | None = None) -> dict[str, str]:
       * "page_icon": the browser tab icon (one image; the tab ignores the theme),
       * "logo": the image at the top of the sidebar,
       * "logo_icon": the smaller image shown when the sidebar is collapsed,
-      * "app_icon": the image an app page shows next to its title,
+      * "app_icon": an icon for the app's own pages to use, e.g. next to a
+        title (the template shows it nowhere itself; it is also the default
+        "logo_icon"),
       * "logo_size": "small", "medium" (default) or "large"; a wide logo with
         the app name in it reads better at "large".
 

src/workflow/RunManager.py

@@ -15,10 +15,12 @@
 """
 
 import json
+import re
 import shutil
 import stat
 import uuid
 import zipfile
+import zlib
 from datetime import datetime
 from pathlib import Path
 from typing import Optional
@@ -27,6 +29,10 @@
 MAX_NAME_LENGTH = 100
 # Largest unpacked size import_zip() accepts by default
 MAX_IMPORT_BYTES = 20 * 1024**3
+# Most files a run ZIP may add, and the largest run.json / params.json read
+MAX_IMPORT_MEMBERS = 100_000
+MAX_RUN_FILE_BYTES = 1024**2
+MAX_PARAMS_FILE_BYTES = 64 * 1024**2
 
 
 class RunImportError(ValueError):
@@ -188,7 +194,10 @@ def import_zip(
 
         Every member is checked before anything is written: absolute paths,
         ``..``, drive letters, backslashes and symbolic links are refused, as
-        is a ZIP that unpacks to more than ``max_bytes``. Unpacking goes to a
+        as are names Windows reserves (``NUL``, ``CON``, ...), a ZIP that
+        unpacks to more than ``max_bytes`` or holds more than
+        MAX_IMPORT_MEMBERS files, and an oversized run.json or params.json.
+        A run that was still running when it was zipped is imported as failed. Unpacking goes to a
         temporary folder that becomes the run only when it is complete.
 
         Args:
@@ -204,84 +213,123 @@ def import_zip(
             archive = zipfile.ZipFile(source)
         except (zipfile.BadZipFile, OSError) as e:
             raise RunImportError("This is not a ZIP file.") from e
-        with archive:
-            members = [m for m in archive.infolist() if not m.is_dir()]
-            for m in archive.infolist():
-                _check_member(m)
-            total = sum(m.file_size for m in members)
-            if total > max_bytes:
-                raise RunImportError(
-                    f"The ZIP unpacks to {total / 1024**3:.1f} GB, more than "
-                    f"the {max_bytes / 1024**3:.1f} GB allowed."
-                )
-            prefix = _run_prefix([m.filename for m in members])
-            if prefix is None:
+        try:
+            with archive:
+                return self._import_archive(archive, name, max_bytes)
+        except (zipfile.BadZipFile, zlib.error, EOFError) as e:
+            raise RunImportError(f"The ZIP is damaged ({e}).") from e
+        except RuntimeError as e:
+            # Encrypted members, an unsupported compression method, or JSON
+            # nested too deeply to parse
+            raise RunImportError(f"The ZIP can not be read ({e}).") from e
+        except OSError as e:
+            raise RunImportError(f"The ZIP could not be unpacked ({e}).") from e
+
+    def _import_archive(self, archive: zipfile.ZipFile, name: str, max_bytes: int) -> str:
+        members = [m for m in archive.infolist() if not m.is_dir()]
+        for m in archive.infolist():
+            _check_member(m)
+        if len(members) > MAX_IMPORT_MEMBERS:
+            raise RunImportError(
+                f"The ZIP holds {len(members)} files, more than the "
+                f"{MAX_IMPORT_MEMBERS} allowed."
+            )
+        total = sum(m.file_size for m in members)
+        if total > max_bytes:
+            raise RunImportError(
+                f"The ZIP unpacks to {total / 1024**3:.1f} GB, more than "
+                f"the {max_bytes / 1024**3:.1f} GB allowed."
+            )
+        prefix = _run_prefix([m.filename for m in members])
+        if prefix is None:
+            raise RunImportError(
+                "This ZIP holds no run: a run's ZIP has a results/ "
+                "folder or a run.json."
+            )
+
+        for m in members:
+            limit = {
+                f"{prefix}{RUN_FILE}": MAX_RUN_FILE_BYTES,
+                f"{prefix}params.json": MAX_PARAMS_FILE_BYTES,
+            }.get(m.filename)
+            if limit is not None and m.file_size > limit:
                 raise RunImportError(
-                    "This ZIP holds no run: a run's ZIP has a results/ folder."
+                    f"Refused: {m.filename} is larger than "
+                    f"{limit / 1024**2:.0f} MB."
                 )
 
+        meta = {}
+        if f"{prefix}{RUN_FILE}" in archive.namelist():
+            try:
+                meta = json.loads(archive.read(f"{prefix}{RUN_FILE}"))
+            except ValueError:
+                meta = {}
+        if not isinstance(meta, dict):
             meta = {}
-            if f"{prefix}{RUN_FILE}" in archive.namelist():
+        status = meta.get("status")
+        if status == RUNNING:
+            # Zipped while it ran, or after its process died: its results
+            # may be incomplete.
+            status = FAILED
+        elif status not in (FINISHED, FAILED, CANCELLED):
+            status = FINISHED
+
+        tmp = Path(self.workflow_dir, f".run-import-{uuid.uuid4().hex[:8]}")
+        tmp.mkdir(parents=True)
+        try:
+            for m in members:
+                rel = m.filename[len(prefix):]
+                if not rel or rel == RUN_FILE:
+                    continue
+                target = Path(tmp, *rel.split("/"))
+                target.parent.mkdir(parents=True, exist_ok=True)
+                with archive.open(m) as src, open(target, "wb") as dst:
+                    shutil.copyfileobj(src, dst)
+            params = Path(tmp, "params.json")
+            if params.exists():
                 try:
-                    meta = json.loads(archive.read(f"{prefix}{RUN_FILE}"))
+                    if not isinstance(json.loads(params.read_text(encoding="utf-8")), dict):
+                        raise ValueError
                 except ValueError:
-                    meta = {}
-            if not isinstance(meta, dict):
-                meta = {}
-
-            tmp = Path(self.workflow_dir, f".run-import-{uuid.uuid4().hex[:8]}")
-            tmp.mkdir(parents=True)
-            try:
-                for m in members:
-                    rel = m.filename[len(prefix):]
-                    if not rel or rel == RUN_FILE:
-                        continue
-                    target = Path(tmp, *rel.split("/"))
-                    target.parent.mkdir(parents=True, exist_ok=True)
-                    with archive.open(m) as src, open(target, "wb") as dst:
-                        shutil.copyfileobj(src, dst)
-                params = Path(tmp, "params.json")
-                if params.exists():
-                    try:
-                        if not isinstance(json.loads(params.read_text(encoding="utf-8")), dict):
-                            raise ValueError
-                    except ValueError:
-                        params.unlink()
-                Path(tmp, "results").mkdir(exist_ok=True)
-
-                run = {
-                    "name": _clean_name(name) or _clean_name(meta.get("name")) or "Imported run",
-                    "created": meta.get("created") or _now(),
-                    "status": meta.get("status") if meta.get("status") in (
-                        FINISHED, FAILED, CANCELLED
-                    ) else FINISHED,
-                    "imported": _now(),
-                }
-                if meta.get("finished"):
-                    run["finished"] = meta["finished"]
-                run["name"] = self._unique_name(run["name"][:MAX_NAME_LENGTH])
-
-                self.runs_dir.mkdir(parents=True, exist_ok=True)
-                base = datetime.now().strftime("%Y%m%d-%H%M%S")
-                run_id, n = base, 1
-                while self.run_dir(run_id).exists():
-                    n += 1
-                    run_id = f"{base}-{n}"
-                with open(Path(tmp, RUN_FILE), "w", encoding="utf-8") as f:
-                    json.dump(run, f, indent=2)
-                tmp.rename(self.run_dir(run_id))
-            finally:
-                shutil.rmtree(tmp, ignore_errors=True)
+                    params.unlink()
+            Path(tmp, "results").mkdir(exist_ok=True)
+
+            run = {
+                "name": _clean_name(name) or _clean_name(meta.get("name")) or "Imported run",
+                "created": _iso_time(meta.get("created")) or _now(),
+                "status": status,
+                "imported": _now(),
+            }
+            if _iso_time(meta.get("finished")):
+                run["finished"] = _iso_time(meta["finished"])
+            run["name"] = self._unique_name(run["name"])
+
+            self.runs_dir.mkdir(parents=True, exist_ok=True)
+            base = datetime.now().strftime("%Y%m%d-%H%M%S")
+            run_id, n = base, 1
+            while self.run_dir(run_id).exists():
+                n += 1
+                run_id = f"{base}-{n}"
+            with open(Path(tmp, RUN_FILE), "w", encoding="utf-8") as f:
+                json.dump(run, f, indent=2)
+            tmp.rename(self.run_dir(run_id))
+        finally:
+            shutil.rmtree(tmp, ignore_errors=True)
         return run_id
 
     def _unique_name(self, name: str) -> str:
+        """``name``, or ``name (n)`` if a run has it, within MAX_NAME_LENGTH."""
         taken = {r.get("name") for r in self.list_runs()}
+        name = name[:MAX_NAME_LENGTH]
         if name not in taken:
             return name
         n = 2
-        while f"{name} ({n})" in taken:
+        while True:
+            suffix = f" ({n})"
+            candidate = name[: MAX_NAME_LENGTH - len(suffix)].rstrip() + suffix
+            if candidate not in taken:
+                return candidate
             n += 1
-        return f"{name} ({n})"
 
     def adopt_legacy_results(self) -> Optional[str]:
         """
@@ -342,6 +390,21 @@ def _is_safe_id(run_id: str) -> bool:
     return bool(run_id) and "/" not in run_id and "\\" not in run_id and run_id not in (".", "..")
 
 
+def _iso_time(value) -> Optional[str]:
+    """``value`` if it is an ISO timestamp string, else None."""
+    if not isinstance(value, str):
+        return None
+    try:
+        datetime.fromisoformat(value)
+    except ValueError:
+        return None
+    return value
+
+
+# Names Windows maps to devices, with or without an extension (NUL, nul.txt)
+_WINDOWS_RESERVED = re.compile(r"^(con|prn|aux|nul|com[1-9]|lpt[1-9])(\..*)?$", re.I)
+
+
 def _check_member(member: zipfile.ZipInfo) -> None:
     """Refuse a ZIP member that could land outside the folder it is unpacked to."""
     name = member.filename
@@ -352,6 +415,7 @@ def _check_member(member: zipfile.ZipInfo) -> None:
         or ".." in parts
         or (len(name) > 1 and name[1] == ":")
         or "\x00" in name
+        or any(_WINDOWS_RESERVED.match(p.rstrip(" .")) for p in parts if p)
     ):
         raise RunImportError(f"Refused: the ZIP has an unsafe path ({name!r}).")
     mode = member.external_attr >> 16
@@ -362,14 +426,21 @@ def _check_member(member: zipfile.ZipInfo) -> None:
 def _run_prefix(names: list[str]) -> Optional[str]:
     """
     Where the run sits in the ZIP: "" when results/ is at the top, "<dir>/"
-    when everything is in one folder, None when there is no results/ folder.
+    when everything is in one folder, None when it holds no run. A run is
+    a results/ folder or a run.json: zip() leaves out an empty results/.
     """
-    if any(n.startswith("results/") for n in names):
+    def is_run(prefix: str) -> bool:
+        return any(
+            n.startswith(f"{prefix}results/") or n == f"{prefix}{RUN_FILE}"
+            for n in names
+        )
+
+    if is_run(""):
         return ""
     tops = {n.split("/", 1)[0] for n in names}
     if len(tops) == 1:
         top = tops.pop() + "/"
-        if any(n.startswith(f"{top}results/") for n in names):
+        if is_run(top):
             return top
     return None
 

src/workflow/StreamlitUI.py

@@ -110,6 +110,19 @@ def _plain(value):
     return value
 
 
+def _as_str_list(value) -> list[str]:
+    """A list parameter's value as a list of str: bytes items decoded, and
+    newline-separated text (as params.json stores lists) split into items,
+    empty ones included, as CommandExecutor.run_topp() passes them."""
+    if isinstance(value, (list, tuple)):
+        return [v.decode() if isinstance(v, bytes) else str(v) for v in value]
+    if isinstance(value, bytes):
+        value = value.decode()
+    if value is None or value == "":
+        return []
+    return str(value).split("\n")
+
+
 def _short(value) -> str:
     """A parameter value for a caption or tooltip."""
     if isinstance(value, list):
@@ -1510,8 +1523,10 @@ def _all_parameters_dialog(
             name = key.decode().split(":1:")[1]
             default = defaults.get(name, entry.value)
             value = user.get(name, default)
-            if isinstance(entry.value, list) and isinstance(value, str):
-                value = value.split("\n") if value else []
+            if isinstance(entry.value, list):
+                # pyOpenMS gives list items as bytes, and params.json or
+                # custom_defaults can hold a list as newline-separated text
+                default, value = _as_str_list(default), _as_str_list(value)
             rows.append(
                 {
                     "name": name,
@@ -1661,8 +1676,8 @@ def to_preset(k=wkey, r=row, v=preset_val):
         if isinstance(default, bool):
             current = value if isinstance(value, bool) else str(value).lower() == "true"
             c_value.checkbox(label, value=current, **common)
-        elif isinstance(default, list):
-            items = [str(v) for v in value] if isinstance(value, list) else str(value).split("\n")
+        elif row["is_list"]:
+            items = _as_str_list(value)
             if row["valid_strings"]:
                 c_value.multiselect(
                     label,

src/workflow/WorkflowManager.py

@@ -339,6 +339,10 @@ def show_results_section(self) -> None:
         A multi-run workflow first shows a run picker and points
         self.results_dir at the chosen run.
         """
+        if self.multi_run:
+            from .run_widgets import show_import_notice
+
+            show_import_notice(self.runs)
         if self.multi_run and not self.select_run():
             from .run_widgets import run_import