From 89b9339c88d16b2f2306cc189d2f421b9a33d092 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 10:27:42 +0000 Subject: [PATCH 1/3] Harden run ZIP import and fix review findings Run ZIP import (RunManager.import_zip): - a run zipped while still running is imported as failed, not finished - refuse more than 100,000 files, a run.json over 1 MB and a params.json over 64 MB before anything is read or written - keep "created"/"finished" only when they are ISO timestamp strings - damaged member data (CRC, deflate) raises RunImportError instead of an uncaught zipfile/zlib error - accept a run with no result files (zip() leaves out an empty results/) - refuse Windows device names (NUL, CON, LPT1, ...) in member paths - the name with its " (n)" suffix stays within MAX_NAME_LENGTH Run import widgets: the message after an import says whether the run is shown on the results pages, and the results pages take it too, so it no longer waits for the Download page. The uploader names the upload limit. "All parameters" dialog: list values from pyOpenMS (bytes) and from params.json (newline-separated text) become lists of str, and the list editor is chosen from the parameter's type, not from its default. build-image.yml strips YAML quotes from the overlay's newName/newTag. package.sh notarizes with the API key only when key, key id and issuer are all set; otherwise it uses the Apple ID, or fails naming what is missing. The get_branding() docstring says what app_icon is for. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FkEGengFwqzeVYN7rGGrrZ --- .github/workflows/build-image.yml | 4 +- electron/scripts/package.sh | 6 +- src/common/common.py | 4 +- src/workflow/RunManager.py | 203 ++++++++++++++++++++---------- src/workflow/StreamlitUI.py | 20 ++- src/workflow/WorkflowManager.py | 4 + src/workflow/run_widgets.py | 37 +++++- tests/test_run_import.py | 101 ++++++++++++++- tests/test_run_import_widgets.py | 53 ++++++++ 9 files changed, 349 insertions(+), 83 deletions(-) create mode 100644 tests/test_run_import_widgets.py diff --git a/.github/workflows/build-image.yml b/.github/workflows/build-image.yml index 733018fc..dc1769b6 100644 --- a/.github/workflows/build-image.yml +++ b/.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 diff --git a/electron/scripts/package.sh b/electron/scripts/package.sh index 98736b59..d36d9133 100755 --- a/electron/scripts/package.sh +++ b/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 diff --git a/src/common/common.py b/src/common/common.py index 0bf1b802..a0e678f5 100644 --- a/src/common/common.py +++ b/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". diff --git a/src/workflow/RunManager.py b/src/workflow/RunManager.py index 95e07ff3..d9fe6420 100644 --- a/src/workflow/RunManager.py +++ b/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,119 @@ 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 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 +386,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 +411,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 +422,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, "/" - 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 diff --git a/src/workflow/StreamlitUI.py b/src/workflow/StreamlitUI.py index aec444b7..1a68ea81 100644 --- a/src/workflow/StreamlitUI.py +++ b/src/workflow/StreamlitUI.py @@ -110,6 +110,16 @@ 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.""" + 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() + return [v for v in str(value or "").split("\n") if v] + + def _short(value) -> str: """A parameter value for a caption or tooltip.""" if isinstance(value, list): @@ -1510,8 +1520,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 +1673,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, diff --git a/src/workflow/WorkflowManager.py b/src/workflow/WorkflowManager.py index d59c8b42..2b636114 100644 --- a/src/workflow/WorkflowManager.py +++ b/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 diff --git a/src/workflow/run_widgets.py b/src/workflow/run_widgets.py index 729b748e..20ed35a5 100644 --- a/src/workflow/run_widgets.py +++ b/src/workflow/run_widgets.py @@ -216,6 +216,28 @@ def run_manager_section( st.rerun() +def show_import_notice(runs: RunManager) -> None: + """ + The message left by run_import() for the rerun after an import. Shown + once: run_import() and the results pages both call this, whichever runs + first takes it. + """ + done = st.session_state.pop(f"_runs-{_slug(runs)}-imported", None) + if not done: + return + if isinstance(done, dict): + name, shown = done.get("name", ""), done.get("shown", False) + else: + name, shown = done, True + if shown: + st.success(f"Imported **{name}**. It is shown on the results pages.") + else: + st.warning( + f"Imported **{name}**. Its run did not finish, so the results " + "pages do not show it; it is listed on the Download page." + ) + + def run_import(runs: RunManager, expanded: bool = False) -> None: """ Upload a run's ZIP (as the download button of run_manager_section() makes @@ -225,13 +247,13 @@ def run_import(runs: RunManager, expanded: bool = False) -> None: slug = _slug(runs) counter = f"_runs-{slug}-import-n" n = st.session_state.get(counter, 0) - done = st.session_state.pop(f"_runs-{slug}-imported", None) - if done: - st.success(f"Imported **{done}**. It is shown on the results pages.") + show_import_notice(runs) with st.expander("📥 Open a run from a ZIP", expanded=expanded): st.caption( "A run downloaded from this page, here or in another workspace or " - "installation: results, parameters and logs are added as a new run." + "installation: results, parameters and logs are added as a new run. " + f"Up to {st.get_option('server.maxUploadSize')} MB " + "(`server.maxUploadSize` in `.streamlit/config.toml`)." ) upload = st.file_uploader( "Run ZIP", type=["zip"], key=f"runs-{slug}-import-{n}", @@ -247,11 +269,14 @@ def run_import(runs: RunManager, expanded: bool = False) -> None: st.error(f"**{upload.name}** was not imported. {e}") return run = runs.get(run_id) - if run.get("status") == FINISHED: + shown = run.get("status") == FINISHED + if shown: set_selected_run(runs, run_id) # A fresh uploader, and the message after the rerun st.session_state[counter] = n + 1 - st.session_state[f"_runs-{slug}-imported"] = run.get("name", run_id) + st.session_state[f"_runs-{slug}-imported"] = { + "name": run.get("name", run_id), "shown": shown, + } st.rerun() diff --git a/tests/test_run_import.py b/tests/test_run_import.py index bfa3329b..bb714745 100644 --- a/tests/test_run_import.py +++ b/tests/test_run_import.py @@ -16,9 +16,12 @@ PROJECT_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) sys.path.append(PROJECT_ROOT) +from src.workflow import RunManager as run_manager_module # noqa: E402 from src.workflow.RunManager import ( # noqa: E402 FAILED, FINISHED, + MAX_NAME_LENGTH, + RUNNING, RunImportError, RunManager, ) @@ -98,7 +101,11 @@ def test_zip_of_the_run_folder_and_minimal_zip(tmp_path): @pytest.mark.parametrize( "name", - ["../evil.txt", "results/../../evil.txt", "/etc/evil", "C:/evil.txt", "results\\..\\evil"], + [ + "../evil.txt", "results/../../evil.txt", "/etc/evil", "C:/evil.txt", + "results\\..\\evil", "results/NUL", "results/con.txt", "results/LPT1/a.tsv", + "results/aux. ", + ], ) def test_path_traversal_is_refused(tmp_path, name): runs = RunManager(tmp_path / "wf") @@ -120,3 +127,95 @@ def test_other_refusals(tmp_path): with pytest.raises(RunImportError, match="more than"): runs.import_zip(_zip({"results/big.bin": "0" * 5000}), max_bytes=1000) assert runs.list_runs() == [] + + +def test_names_like_reserved_ones_are_kept(tmp_path): + runs = RunManager(tmp_path / "wf") + run_id = runs.import_zip(_zip({"results/nullable.tsv": "1", "results/console/a": "2"})) + assert (runs.results_dir(run_id) / "nullable.tsv").exists() + assert (runs.results_dir(run_id) / "console" / "a").exists() + + +def test_run_without_result_files_round_trips(tmp_path): + src = RunManager(tmp_path / "src") + run_id = src.new_run("Empty") + src.finish(run_id, FINISHED) + archive = src.zip(run_id) + with zipfile.ZipFile(archive) as zf: + assert not any(n.startswith("results/") for n in zf.namelist()) + + dst = RunManager(tmp_path / "dst") + new_id = dst.import_zip(archive) + assert dst.get(new_id)["name"] == "Empty" + assert dst.results_dir(new_id).is_dir() + # Also inside one top folder + wrapped = dst.import_zip(_zip({"Run 9/run.json": json.dumps({"name": "Run 9"})})) + assert dst.get(wrapped)["name"] == "Run 9" + + +def test_interrupted_run_is_imported_as_failed(tmp_path): + runs = RunManager(tmp_path / "wf") + run_id = runs.import_zip(_zip({ + "run.json": json.dumps({"name": "Cut off", "status": RUNNING}), + "results/partial.tsv": "1", + })) + assert runs.get(run_id)["status"] == FAILED + + +def test_bad_timestamps_are_replaced(tmp_path): + runs = RunManager(tmp_path / "wf") + run_id = runs.import_zip(_zip({ + "run.json": json.dumps({"created": 123, "finished": ["x"], "status": FINISHED}), + "results/a.tsv": "1", + })) + run = runs.get(run_id) + assert isinstance(run["created"], str) and run["created"] + assert "finished" not in run + # list_runs() sorts by "created": a number there would break it + runs.import_zip(_zip({"run.json": json.dumps({"created": "not a time"}), "results/b": "1"})) + assert len(runs.list_runs()) == 2 + for r in runs.list_runs(): + run_manager_module.format_time(r["created"]) + + +def test_name_stays_within_the_limit_with_a_suffix(tmp_path): + runs = RunManager(tmp_path / "wf") + long = "x" * (MAX_NAME_LENGTH + 20) + entries = {"run.json": json.dumps({"name": long}), "results/a": "1"} + first = runs.import_zip(_zip(entries)) + second = runs.import_zip(_zip(entries)) + third = runs.import_zip(_zip(entries)) + names = [runs.get(r)["name"] for r in (first, second, third)] + assert names[0] == "x" * MAX_NAME_LENGTH + assert names[1].endswith(" (2)") and names[2].endswith(" (3)") + assert all(len(n) <= MAX_NAME_LENGTH for n in names) + assert len(set(names)) == 3 + + +def test_size_and_count_limits(tmp_path, monkeypatch): + runs = RunManager(tmp_path / "wf") + monkeypatch.setattr(run_manager_module, "MAX_IMPORT_MEMBERS", 3) + with pytest.raises(RunImportError, match="files, more than"): + runs.import_zip(_zip({f"results/{i}": "" for i in range(4)})) + monkeypatch.setattr(run_manager_module, "MAX_RUN_FILE_BYTES", 100) + with pytest.raises(RunImportError, match="run.json is larger"): + runs.import_zip(_zip({"run.json": json.dumps({"name": "y" * 200}), "results/a": "1"})) + monkeypatch.setattr(run_manager_module, "MAX_PARAMS_FILE_BYTES", 100) + with pytest.raises(RunImportError, match="params.json is larger"): + runs.import_zip(_zip({"params.json": json.dumps({"k": "z" * 200}), "results/a": "1"})) + assert runs.list_runs() == [] + + +def test_damaged_member_data_is_refused(tmp_path): + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr("results/a.tsv", "".join(f"{i}\t{i * i}\n" for i in range(5000))) + data = bytearray(buf.getvalue()) + # Corrupt the compressed data of the one member, between its local + # header and the central directory at the end + for i in range(200, 240): + data[i] ^= 0xFF + runs = RunManager(tmp_path / "wf") + with pytest.raises(RunImportError, match="damaged"): + runs.import_zip(io.BytesIO(bytes(data))) + assert runs.list_runs() == [] diff --git a/tests/test_run_import_widgets.py b/tests/test_run_import_widgets.py new file mode 100644 index 00000000..feb35202 --- /dev/null +++ b/tests/test_run_import_widgets.py @@ -0,0 +1,53 @@ +""" +The message after a run import (run_widgets.show_import_notice) and how the +"All parameters" dialog reads list values (StreamlitUI._as_str_list). +""" +import os +import sys + +PROJECT_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +sys.path.append(PROJECT_ROOT) + +from streamlit.testing.v1 import AppTest # noqa: E402 + + +def _notice_app(workflow_dir, notice): + def app(workflow_dir, notice): + import streamlit as st + + from src.workflow.RunManager import RunManager + from src.workflow.run_widgets import show_import_notice + + runs = RunManager(workflow_dir) + if "seeded" not in st.session_state: + st.session_state.seeded = True + st.session_state[f"_runs-{runs.workflow_dir.name}-imported"] = notice + show_import_notice(runs) + show_import_notice(runs) + + return AppTest.from_function(app, args=(str(workflow_dir), notice)) + + +def test_notice_depends_on_whether_the_run_is_shown(tmp_path): + at = _notice_app(tmp_path / "my-workflow", {"name": "Run 1", "shown": True}).run() + assert [s.value for s in at.success] == [ + "Imported **Run 1**. It is shown on the results pages." + ] + assert not at.warning + + at = _notice_app(tmp_path / "my-workflow", {"name": "Run 2", "shown": False}).run() + assert not at.success + assert len(at.warning) == 1 and "did not finish" in at.warning[0].value + + # Shown once, and gone on the next rerun + at.run() + assert not at.success and not at.warning + + +def test_list_values_as_str_lists(): + from src.workflow.StreamlitUI import _as_str_list + + assert _as_str_list([b"a", "b", 3]) == ["a", "b", "3"] + assert _as_str_list("a\nb\n") == ["a", "b"] + assert _as_str_list(b"a\nb") == ["a", "b"] + assert _as_str_list("") == [] and _as_str_list(None) == [] From be09e568f507963da243b2d4301a53f7eb2f1d62 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 10:34:48 +0000 Subject: [PATCH 2/3] Keep empty list entries and zero; refuse unreadable ZIP members _as_str_list() split away empty entries and turned 0 into an empty list, so the "All parameters" dialog could show a value as unchanged while run_topp() passes the extra empty argument. It now splits like run_topp() does. import_zip() turns RuntimeError (encrypted members, unsupported compression, over-nested JSON) into RunImportError. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FkEGengFwqzeVYN7rGGrrZ --- src/workflow/RunManager.py | 4 ++++ src/workflow/StreamlitUI.py | 7 +++++-- tests/test_run_import.py | 18 ++++++++++++++++++ tests/test_run_import_widgets.py | 4 +++- 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/workflow/RunManager.py b/src/workflow/RunManager.py index d9fe6420..18e7a1c4 100644 --- a/src/workflow/RunManager.py +++ b/src/workflow/RunManager.py @@ -218,6 +218,10 @@ def import_zip( 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 diff --git a/src/workflow/StreamlitUI.py b/src/workflow/StreamlitUI.py index 1a68ea81..0e8d5832 100644 --- a/src/workflow/StreamlitUI.py +++ b/src/workflow/StreamlitUI.py @@ -112,12 +112,15 @@ def _plain(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.""" + 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() - return [v for v in str(value or "").split("\n") if v] + if value is None or value == "": + return [] + return str(value).split("\n") def _short(value) -> str: diff --git a/tests/test_run_import.py b/tests/test_run_import.py index bb714745..2f8d46ad 100644 --- a/tests/test_run_import.py +++ b/tests/test_run_import.py @@ -219,3 +219,21 @@ def test_damaged_member_data_is_refused(tmp_path): with pytest.raises(RunImportError, match="damaged"): runs.import_zip(io.BytesIO(bytes(data))) assert runs.list_runs() == [] + + +def test_unreadable_members_are_refused(tmp_path, monkeypatch): + runs = RunManager(tmp_path / "wf") + # Over-nested JSON in run.json: json.loads raises RecursionError + nested = "[" * 100_000 + "]" * 100_000 + with pytest.raises(RunImportError, match="can not be read"): + runs.import_zip(_zip({"run.json": nested, "results/a": "1"})) + + # Encrypted members: ZipFile.open raises RuntimeError + def encrypted(*args, **kwargs): + raise RuntimeError("File 'results/a' is encrypted, password required") + + archive = _zip({"results/a": "1"}) + monkeypatch.setattr(zipfile.ZipFile, "open", encrypted) + with pytest.raises(RunImportError, match="can not be read"): + runs.import_zip(archive) + assert runs.list_runs() == [] diff --git a/tests/test_run_import_widgets.py b/tests/test_run_import_widgets.py index feb35202..80d9c6ac 100644 --- a/tests/test_run_import_widgets.py +++ b/tests/test_run_import_widgets.py @@ -48,6 +48,8 @@ def test_list_values_as_str_lists(): from src.workflow.StreamlitUI import _as_str_list assert _as_str_list([b"a", "b", 3]) == ["a", "b", "3"] - assert _as_str_list("a\nb\n") == ["a", "b"] + # Empty entries stay: run_topp() passes them to the tool + assert _as_str_list("a\nb\n") == ["a", "b", ""] + assert _as_str_list(0) == ["0"] assert _as_str_list(b"a\nb") == ["a", "b"] assert _as_str_list("") == [] and _as_str_list(None) == [] From d57db270ad3b527e7f0f387fc0f13f124f187ed0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 10:06:10 +0000 Subject: [PATCH 3/3] CI: wait for the ingress-nginx rollout, not for pods that may not exist yet kubectl wait on a pod selector fails at once with "no matching resources found" when it runs before the controller Deployment has created its pod, which made test-nginx fail now and then (seen on arm64). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FkEGengFwqzeVYN7rGGrrZ --- .github/workflows/build-and-test.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 64955b11..3b25f93e 100644 --- a/.github/workflows/build-and-test.yml +++ b/.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