From 9119cc43424c24a1e1ee6e40b334f121a80feac9 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 24 Jul 2026 14:32:04 +0500 Subject: [PATCH 1/4] fix(bundle): wrap local .zip errors in BundlerError instead of raw crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _local_manifest_source's .zip branch called zipfile.ZipFile() and yaml.safe_load() with no error handling. A .zip-suffixed file that is not a valid archive raised a raw zipfile.BadZipFile, and a valid zip with a malformed embedded bundle.yml raised a raw yaml.YAMLError. bundle_install only does `except BundlerError`, so both escaped to an unhandled traceback instead of the intended clean `Error: ...` + exit 1. Wrap both failure modes in BundlerError, mirroring the remote-download sibling (_download_remote_manifest already wraps its _local_manifest_source call for exactly this). The directory and bundle.yml branches already report cleanly via BundleManifest.from_file -> load_yaml; this aligns the .zip branch. Valid archives are unaffected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/specify_cli/commands/bundle/__init__.py | 20 +++++++++++++++-- .../integration/test_bundler_local_install.py | 22 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index d8fb22e511..340c493311 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -715,14 +715,30 @@ def _local_manifest_source(arg: str): import yaml as _yaml - with zipfile.ZipFile(candidate) as archive: + try: + archive = zipfile.ZipFile(candidate) + except (zipfile.BadZipFile, OSError) as exc: + # A .zip-suffixed file that is not a valid archive would otherwise + # raise a raw BadZipFile that escapes bundle_install's `except + # BundlerError` and dumps a traceback. Report it cleanly, like the + # remote-download path already does for the same source. + raise BundlerError( + f"Artifact '{candidate}' is not a valid .zip bundle: {exc}" + ) from exc + with archive: try: raw = archive.read("bundle.yml") except KeyError as exc: raise BundlerError( f"Artifact '{candidate}' does not contain a bundle.yml." ) from exc - data = _yaml.safe_load(io.BytesIO(raw)) + try: + data = _yaml.safe_load(io.BytesIO(raw)) + except _yaml.YAMLError as exc: + # Malformed embedded bundle.yml — same rationale as above. + raise BundlerError( + f"Artifact '{candidate}' contains an invalid bundle.yml: {exc}" + ) from exc return BundleManifest.from_dict(data) if candidate.name == "bundle.yml" or candidate.suffix in (".yml", ".yaml"): diff --git a/tests/integration/test_bundler_local_install.py b/tests/integration/test_bundler_local_install.py index 32972ac684..65ad45657f 100644 --- a/tests/integration/test_bundler_local_install.py +++ b/tests/integration/test_bundler_local_install.py @@ -53,6 +53,28 @@ def test_local_source_from_zip_artifact(tmp_path: Path): assert manifest.bundle.id == "demo-bundle" +def test_local_source_corrupt_zip_raises_clean_error(tmp_path: Path): + """A .zip-suffixed file that is not a valid archive raises a clean + BundlerError, not a raw zipfile.BadZipFile that escapes the install + handler's `except BundlerError` and dumps a traceback.""" + bad = tmp_path / "artifact.zip" + bad.write_text("this is not a zip", encoding="utf-8") + with pytest.raises(BundlerError, match="not a valid .zip bundle"): + _local_manifest_source(str(bad)) + + +def test_local_source_zip_with_malformed_bundle_yml_raises_clean_error(tmp_path: Path): + """A valid .zip whose embedded bundle.yml is malformed YAML raises a clean + BundlerError, not a raw yaml.YAMLError.""" + import zipfile + + artifact = tmp_path / "artifact.zip" + with zipfile.ZipFile(artifact, "w") as zf: + zf.writestr("bundle.yml", "key: [unterminated\n") # invalid YAML + with pytest.raises(BundlerError, match="invalid bundle.yml"): + _local_manifest_source(str(artifact)) + + def test_local_source_rejects_unknown_file(tmp_path: Path): weird = tmp_path / "thing.txt" weird.write_text("nope", encoding="utf-8") From 6d5f8d26592208bb5adc5e70cb46f2d9f95e5874 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 25 Jul 2026 09:21:38 +0500 Subject: [PATCH 2/4] fix(bundle): extend the ZIP error boundary to the entry read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: ZipFile() validates only the central directory, so a corrupt MEMBER still failed after the open and escaped bundle_install's `except BundlerError` as a traceback. Verified both failure modes on a directory-intact archive: a flipped byte in a stored member raises zipfile.BadZipFile("Bad CRC-32 for file 'bundle.yml'"), and a mangled deflate stream raises zlib.error. Neither is an OSError subclass, so the read boundary lists BadZipFile/OSError/zlib.error/EOFError explicitly (the review named BadZipFile; zlib.error is the same class of corruption and would have escaped an OSError-only clause). Valid archives are unaffected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/commands/bundle/__init__.py | 32 ++++++++++++------ .../integration/test_bundler_local_install.py | 33 +++++++++++++++++++ 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index 340c493311..f95bbd95a2 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -712,26 +712,38 @@ def _local_manifest_source(arg: str): if candidate.suffix == ".zip": import io import zipfile + import zlib import yaml as _yaml + # A .zip-suffixed file that is not a valid archive would otherwise raise + # a raw BadZipFile that escapes bundle_install's `except BundlerError` + # and dumps a traceback. Report it cleanly, like the remote-download + # path already does for the same source. + # + # The boundary has to cover the ENTRY READ too, not just the open: + # ZipFile() validates only the central directory, so a corrupt member + # still fails later -- a bad CRC raises BadZipFile("Bad CRC-32 for + # file ...") and corrupt deflate data raises zlib.error. Neither is an + # OSError subclass, so both are listed explicitly. + _ZIP_READ_ERRORS = (zipfile.BadZipFile, OSError, zlib.error, EOFError) try: archive = zipfile.ZipFile(candidate) except (zipfile.BadZipFile, OSError) as exc: - # A .zip-suffixed file that is not a valid archive would otherwise - # raise a raw BadZipFile that escapes bundle_install's `except - # BundlerError` and dumps a traceback. Report it cleanly, like the - # remote-download path already does for the same source. raise BundlerError( f"Artifact '{candidate}' is not a valid .zip bundle: {exc}" ) from exc - with archive: - try: + try: + with archive: raw = archive.read("bundle.yml") - except KeyError as exc: - raise BundlerError( - f"Artifact '{candidate}' does not contain a bundle.yml." - ) from exc + except KeyError as exc: + raise BundlerError( + f"Artifact '{candidate}' does not contain a bundle.yml." + ) from exc + except _ZIP_READ_ERRORS as exc: + raise BundlerError( + f"Artifact '{candidate}' is not a valid .zip bundle: {exc}" + ) from exc try: data = _yaml.safe_load(io.BytesIO(raw)) except _yaml.YAMLError as exc: diff --git a/tests/integration/test_bundler_local_install.py b/tests/integration/test_bundler_local_install.py index 65ad45657f..7ec82a0299 100644 --- a/tests/integration/test_bundler_local_install.py +++ b/tests/integration/test_bundler_local_install.py @@ -75,6 +75,39 @@ def test_local_source_zip_with_malformed_bundle_yml_raises_clean_error(tmp_path: _local_manifest_source(str(artifact)) +def test_local_source_zip_with_corrupt_entry_raises_clean_error(tmp_path: Path): + """A .zip whose CENTRAL DIRECTORY is intact but whose member data is corrupt + must also become a BundlerError. ZipFile() only validates the directory, so + these surface at read() time: a bad CRC raises BadZipFile and corrupt deflate + data raises zlib.error -- neither an OSError, so both escaped the open-only + boundary as a traceback. + """ + import zipfile + + # Stored (uncompressed) member with a flipped payload byte -> CRC mismatch. + crc_bad = tmp_path / "crc.zip" + with zipfile.ZipFile(crc_bad, "w", zipfile.ZIP_STORED) as zf: + zf.writestr("bundle.yml", "schema_version: '1.0'\n") + raw = bytearray(crc_bad.read_bytes()) + idx = raw.find(b"schema_version") + raw[idx] ^= 0xFF + crc_bad.write_bytes(bytes(raw)) + with pytest.raises(BundlerError, match="not a valid .zip bundle"): + _local_manifest_source(str(crc_bad)) + + # Deflated member with a mangled compressed stream -> zlib.error. + zlib_bad = tmp_path / "zlib.zip" + with zipfile.ZipFile(zlib_bad, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr("bundle.yml", "schema_version: '1.0'\n") + raw2 = bytearray(zlib_bad.read_bytes()) + start = raw2.find(b"bundle.yml") + len(b"bundle.yml") + for k in range(start, min(start + 12, len(raw2))): + raw2[k] ^= 0xFF + zlib_bad.write_bytes(bytes(raw2)) + with pytest.raises(BundlerError, match="not a valid .zip bundle"): + _local_manifest_source(str(zlib_bad)) + + def test_local_source_rejects_unknown_file(tmp_path: Path): weird = tmp_path / "thing.txt" weird.write_text("nope", encoding="utf-8") From bf1cf35b695dae5f8f5bd5121b07ad6f65dd1f7a Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 25 Jul 2026 10:18:48 +0500 Subject: [PATCH 3/4] fix(bundle): complete the ZIP error boundary and escape archive-supplied text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review follow-up: the exception sets were still incomplete, so a raw exception could escape bundle_install's `except BundlerError` after all. Verified against CPython's zipfile: read() RuntimeError -- password-protected artifact (`zip -e`) NotImplementedError -- unsupported compression method open() NotImplementedError -- unsupported zip version ValueError None are OSError subclasses, so each is now listed explicitly. Also escape the interpolated exception text: zipfile embeds bytes taken FROM THE ARCHIVE in some messages -- BadZipFile("File name in directory 'bundle.yml' and header b'[/red]abcd' differ.") -- and that text reaches the user through _fail(), which renders Rich markup. Unescaped, a crafted member name raised MarkupError instead of reporting the corruption. The user's own typed path is left as-is, matching the other messages in this module. Three new tests (encrypted, unsupported compression, crafted member name) all fail before this commit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/commands/bundle/__init__.py | 41 ++++++++++--- .../integration/test_bundler_local_install.py | 59 +++++++++++++++++++ 2 files changed, 92 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index f95bbd95a2..98202e7d1c 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -715,6 +715,7 @@ def _local_manifest_source(arg: str): import zlib import yaml as _yaml + from rich.markup import escape as _escape_markup # A .zip-suffixed file that is not a valid archive would otherwise raise # a raw BadZipFile that escapes bundle_install's `except BundlerError` @@ -723,15 +724,37 @@ def _local_manifest_source(arg: str): # # The boundary has to cover the ENTRY READ too, not just the open: # ZipFile() validates only the central directory, so a corrupt member - # still fails later -- a bad CRC raises BadZipFile("Bad CRC-32 for - # file ...") and corrupt deflate data raises zlib.error. Neither is an - # OSError subclass, so both are listed explicitly. - _ZIP_READ_ERRORS = (zipfile.BadZipFile, OSError, zlib.error, EOFError) + # still fails later. None of these are OSError subclasses, so each is + # listed explicitly (verified against CPython's zipfile): + # open -- BadZipFile (bad magic/directory), NotImplementedError + # (unsupported zip version), ValueError + # read -- BadZipFile ("Bad CRC-32 for file ..."), zlib.error (corrupt + # deflate stream), EOFError (truncated), RuntimeError + # ("... is encrypted, password required for extraction"), + # NotImplementedError ("That compression method is not + # supported") + _ZIP_OPEN_ERRORS = ( + zipfile.BadZipFile, + OSError, + NotImplementedError, + ValueError, + ) + _ZIP_READ_ERRORS = _ZIP_OPEN_ERRORS + (zlib.error, EOFError, RuntimeError) + + # The messages below escape the interpolated EXCEPTION text: it reaches + # the user through _fail(), which renders Rich markup, and zipfile + # embeds raw bytes from the archive in some messages -- e.g. + # BadZipFile("File name in directory 'bundle.yml' and header + # b'[/red]abcd' differ."). Unescaped, a crafted member name would raise + # MarkupError instead of reporting the corruption. (``candidate`` is the + # user's own typed path and stays as-is, matching the other messages in + # this module.) try: archive = zipfile.ZipFile(candidate) - except (zipfile.BadZipFile, OSError) as exc: + except _ZIP_OPEN_ERRORS as exc: raise BundlerError( - f"Artifact '{candidate}' is not a valid .zip bundle: {exc}" + f"Artifact '{candidate}' is not a valid .zip bundle: " + f"{_escape_markup(str(exc))}" ) from exc try: with archive: @@ -742,14 +765,16 @@ def _local_manifest_source(arg: str): ) from exc except _ZIP_READ_ERRORS as exc: raise BundlerError( - f"Artifact '{candidate}' is not a valid .zip bundle: {exc}" + f"Artifact '{candidate}' is not a valid .zip bundle: " + f"{_escape_markup(str(exc))}" ) from exc try: data = _yaml.safe_load(io.BytesIO(raw)) except _yaml.YAMLError as exc: # Malformed embedded bundle.yml — same rationale as above. raise BundlerError( - f"Artifact '{candidate}' contains an invalid bundle.yml: {exc}" + f"Artifact '{candidate}' contains an invalid bundle.yml: " + f"{_escape_markup(str(exc))}" ) from exc return BundleManifest.from_dict(data) diff --git a/tests/integration/test_bundler_local_install.py b/tests/integration/test_bundler_local_install.py index 7ec82a0299..c24f11c7f8 100644 --- a/tests/integration/test_bundler_local_install.py +++ b/tests/integration/test_bundler_local_install.py @@ -6,6 +6,7 @@ """ from __future__ import annotations +import io import os from pathlib import Path @@ -108,6 +109,64 @@ def test_local_source_zip_with_corrupt_entry_raises_clean_error(tmp_path: Path): _local_manifest_source(str(zlib_bad)) +def test_local_source_encrypted_zip_raises_clean_error(tmp_path: Path): + """A password-protected artifact (``zip -e``) must report cleanly. CPython + raises RuntimeError("... is encrypted, password required for extraction") at + read() time -- not a BadZipFile/OSError, so it escaped the first boundary.""" + import zipfile + + artifact = tmp_path / "enc.zip" + with zipfile.ZipFile(artifact, "w") as zf: + zf.writestr("bundle.yml", "schema_version: '1.0'\n") + raw = bytearray(artifact.read_bytes()) + raw[raw.find(b"PK\x03\x04") + 6] |= 0x01 # local header: encrypted bit + raw[raw.find(b"PK\x01\x02") + 8] |= 0x01 # central dir: encrypted bit + artifact.write_bytes(bytes(raw)) + with pytest.raises(BundlerError, match="not a valid .zip bundle"): + _local_manifest_source(str(artifact)) + + +def test_local_source_unsupported_compression_raises_clean_error(tmp_path: Path): + """An unsupported compression method raises NotImplementedError at read() + time -- also not a BadZipFile/OSError.""" + import zipfile + + artifact = tmp_path / "comp.zip" + with zipfile.ZipFile(artifact, "w") as zf: + zf.writestr("bundle.yml", "schema_version: '1.0'\n") + raw = bytearray(artifact.read_bytes()) + raw[raw.find(b"PK\x01\x02") + 10] = 99 # central dir compress_type + raw[raw.find(b"PK\x03\x04") + 8] = 99 # local header compress_type + artifact.write_bytes(bytes(raw)) + with pytest.raises(BundlerError, match="not a valid .zip bundle"): + _local_manifest_source(str(artifact)) + + +def test_local_source_zip_error_message_escapes_archive_markup(tmp_path: Path): + """zipfile embeds bytes taken FROM THE ARCHIVE in some messages, e.g. + BadZipFile("File name in directory 'bundle.yml' and header b'[/red]abcd' + differ."). That text reaches the user through _fail(), which renders Rich + markup, so it must be escaped or a crafted member name raises MarkupError + instead of reporting the corruption.""" + import zipfile + + from rich.console import Console + + artifact = tmp_path / "markup.zip" + with zipfile.ZipFile(artifact, "w") as zf: + zf.writestr("bundle.yml", "schema_version: '1.0'\n") + raw = bytearray(artifact.read_bytes()) + name_at = raw.find(b"PK\x03\x04") + 30 + raw[name_at:name_at + 10] = b"[/red]abcd" # same length keeps offsets valid + artifact.write_bytes(bytes(raw)) + + with pytest.raises(BundlerError, match="not a valid .zip bundle") as excinfo: + _local_manifest_source(str(artifact)) + + # The message must survive Rich rendering (this is what _fail does). + Console(file=io.StringIO()).print(f"[red]Error:[/red] {excinfo.value}") + + def test_local_source_rejects_unknown_file(tmp_path: Path): weird = tmp_path / "thing.txt" weird.write_text("nope", encoding="utf-8") From aedb9e2b6cf542533b6b46970da7db07f2de1ddb Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 28 Jul 2026 10:33:23 +0500 Subject: [PATCH 4/4] fix(bundle): escape the artifact path in the ZIP error messages too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: the path is interpolated into the same _fail()-rendered messages as the exception text, so it needs the same escaping. A file reachable as `.../[/red].zip` -- a directory named `[` -- puts an unmatched closing tag in the message and raises MarkupError instead of reporting the corruption (confirmed: rendering that exact string raises "closing tag '[/red]' ... doesn't match any open tag"; escaping it renders fine). Escape once into `safe_path` and use it at all three sites. Note the vector is POSIX-specific in practice -- Windows Path() normalizes the forward slash to a backslash, which is inert markup -- so the new test builds the POSIX-style path explicitly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/commands/bundle/__init__.py | 26 ++++++++++--------- .../integration/test_bundler_local_install.py | 22 ++++++++++++++++ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index 98202e7d1c..868f0b4db5 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -741,19 +741,21 @@ def _local_manifest_source(arg: str): ) _ZIP_READ_ERRORS = _ZIP_OPEN_ERRORS + (zlib.error, EOFError, RuntimeError) - # The messages below escape the interpolated EXCEPTION text: it reaches - # the user through _fail(), which renders Rich markup, and zipfile - # embeds raw bytes from the archive in some messages -- e.g. - # BadZipFile("File name in directory 'bundle.yml' and header - # b'[/red]abcd' differ."). Unescaped, a crafted member name would raise - # MarkupError instead of reporting the corruption. (``candidate`` is the - # user's own typed path and stays as-is, matching the other messages in - # this module.) + # The messages below escape BOTH interpolated values, because they reach + # the user through _fail(), which renders Rich markup: + # * the exception text -- zipfile embeds raw bytes from the archive in + # some messages, e.g. BadZipFile("File name in directory + # 'bundle.yml' and header b'[/red]abcd' differ.") + # * the path -- a file reachable as ``.../[/red].zip`` (a directory + # named ``[``) puts an unmatched closing tag in the message + # Unescaped, either would raise MarkupError instead of reporting the + # actual corruption. + safe_path = _escape_markup(str(candidate)) try: archive = zipfile.ZipFile(candidate) except _ZIP_OPEN_ERRORS as exc: raise BundlerError( - f"Artifact '{candidate}' is not a valid .zip bundle: " + f"Artifact '{safe_path}' is not a valid .zip bundle: " f"{_escape_markup(str(exc))}" ) from exc try: @@ -761,11 +763,11 @@ def _local_manifest_source(arg: str): raw = archive.read("bundle.yml") except KeyError as exc: raise BundlerError( - f"Artifact '{candidate}' does not contain a bundle.yml." + f"Artifact '{safe_path}' does not contain a bundle.yml." ) from exc except _ZIP_READ_ERRORS as exc: raise BundlerError( - f"Artifact '{candidate}' is not a valid .zip bundle: " + f"Artifact '{safe_path}' is not a valid .zip bundle: " f"{_escape_markup(str(exc))}" ) from exc try: @@ -773,7 +775,7 @@ def _local_manifest_source(arg: str): except _yaml.YAMLError as exc: # Malformed embedded bundle.yml — same rationale as above. raise BundlerError( - f"Artifact '{candidate}' contains an invalid bundle.yml: " + f"Artifact '{safe_path}' contains an invalid bundle.yml: " f"{_escape_markup(str(exc))}" ) from exc return BundleManifest.from_dict(data) diff --git a/tests/integration/test_bundler_local_install.py b/tests/integration/test_bundler_local_install.py index c24f11c7f8..19fb94fcfe 100644 --- a/tests/integration/test_bundler_local_install.py +++ b/tests/integration/test_bundler_local_install.py @@ -167,6 +167,28 @@ def test_local_source_zip_error_message_escapes_archive_markup(tmp_path: Path): Console(file=io.StringIO()).print(f"[red]Error:[/red] {excinfo.value}") +def test_local_source_zip_error_message_escapes_path_markup(tmp_path: Path): + """The ARTIFACT PATH is interpolated into the same message, so it needs the + same escaping: a file reachable as ``.../[/red].zip`` -- a directory named + ``[`` -- puts an unmatched closing tag in the text and would raise + MarkupError when _fail() renders it (POSIX keeps the forward slash; Windows + normalizes it to a backslash, which is inert).""" + from rich.console import Console + + bracket_dir = tmp_path / "[" + bracket_dir.mkdir() + artifact = bracket_dir / "red].zip" + artifact.write_text("this is not a zip", encoding="utf-8") + + # Pass the path with forward slashes so the message carries "[/red].zip" + # exactly as a POSIX caller would produce it. + posix_style = f"{bracket_dir.as_posix()}/red].zip" + with pytest.raises(BundlerError, match="not a valid .zip bundle") as excinfo: + _local_manifest_source(posix_style) + + Console(file=io.StringIO()).print(f"[red]Error:[/red] {excinfo.value}") + + def test_local_source_rejects_unknown_file(tmp_path: Path): weird = tmp_path / "thing.txt" weird.write_text("nope", encoding="utf-8")