fix: enforce size limit on individual files in build_bundle()

src/specify_cli/bundles/packager.py

@@ -9,6 +9,7 @@
 
 import os
 import re
+import tempfile
 import zipfile
 from dataclasses import dataclass
 from pathlib import Path
@@ -24,6 +25,11 @@
 # Fixed member timestamp (zip epoch) for reproducible, byte-stable artifacts.
 _FIXED_TIMESTAMP = (1980, 1, 1, 0, 0, 0)
 
+# Maximum size (in bytes) for any individual file added to the archive.
+# This prevents a single oversized asset from exhausting memory during
+# compression or transmission.
+MAX_ZIP_MEMBER_BYTES = 10 * 1024 * 1024  # 10 MiB
+
 
 @dataclass
 class BuildResult:
@@ -76,28 +82,80 @@ def build_bundle(
         rf"^{re.escape(manifest.bundle.id)}-"
         r"\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?\.zip$"
     )
+    # A leftover mkstemp staging file (<id>-<version>-<random>.tmp) from a
+    # previous killed build may sit inside out_dir — which defaults to the
+    # bundle source tree — and must never be re-packaged as a member.
+    staging_re = re.compile(
+        rf"^{re.escape(manifest.bundle.id)}-"
+        r"\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?"
+        r"-[0-9A-Za-z_]+\.tmp$"
+    )
     files = _collect_files(
-        bundle_dir, skip=artifact_path, skip_dir=skip_dir, artifact_re=artifact_re
+        bundle_dir,
+        skip=artifact_path,
+        skip_dir=skip_dir,
+        artifact_re=artifact_re,
+        staging_re=staging_re,
     )
-    with zipfile.ZipFile(artifact_path, "w", zipfile.ZIP_DEFLATED) as archive:
-        for file_path in files:
-            # Confinement: every packaged file must live under bundle_dir.
-            ensure_within(bundle_dir, file_path)
-            arcname = file_path.relative_to(bundle_dir).as_posix()
-            # Fixed timestamp so identical inputs yield a byte-for-byte
-            # identical artifact (reproducible builds).
-            info = zipfile.ZipInfo(filename=arcname, date_time=_FIXED_TIMESTAMP)
-            info.compress_type = zipfile.ZIP_DEFLATED
-            # Reproducible, normalized permissions: preserve executability so
-            # bundled scripts (e.g. extension hook scripts) stay runnable after
-            # extraction, but collapse to two canonical modes (0755 when any
-            # execute bit is set on the source, otherwise 0644) so identical
-            # inputs yield a byte-for-byte identical artifact.
-            with file_path.open("rb") as fh:
-                st = os.fstat(fh.fileno())
-                mode = 0o755 if st.st_mode & 0o111 else 0o644
-                info.external_attr = mode << 16
-                archive.writestr(info, fh.read())
+
+    # Build into a temporary sibling and atomically replace the final path only
+    # after every member passes validation.  This prevents a partial/corrupt
+    # archive from being left at the final output path if a member exceeds the
+    # size limit or another error occurs mid-build.
+    tmp_fd, tmp_path_str = tempfile.mkstemp(
+        suffix=".tmp", prefix=f"{manifest.bundle.id}-{manifest.bundle.version}-", dir=str(out_dir)
+    )
+    tmp_path = Path(tmp_path_str)
+    try:
+        with os.fdopen(tmp_fd, "wb") as tmp_fh, zipfile.ZipFile(tmp_fh, "w", zipfile.ZIP_DEFLATED) as archive:
+            for file_path in files:
+                # Confinement: every packaged file must live under bundle_dir.
+                ensure_within(bundle_dir, file_path)
+                arcname = file_path.relative_to(bundle_dir).as_posix()
+                # Fixed timestamp so identical inputs yield a byte-for-byte
+                # identical artifact (reproducible builds).
+                info = zipfile.ZipInfo(filename=arcname, date_time=_FIXED_TIMESTAMP)
+                info.compress_type = zipfile.ZIP_DEFLATED
+                # Reproducible, normalized permissions: preserve executability so
+                # bundled scripts (e.g. extension hook scripts) stay runnable after
+                # extraction, but collapse to two canonical modes (0755 when any
+                # execute bit is set on the source, otherwise 0644) so identical
+                # inputs yield a byte-for-byte identical artifact.
+                with file_path.open("rb") as fh:
+                    st = os.fstat(fh.fileno())
+                    mode = 0o755 if st.st_mode & 0o111 else 0o644
+                    info.external_attr = mode << 16
+                    # Fast metadata rejection: skip files whose size exceeds the
+                    # limit before touching the read path.  Then also bound the
+                    # actual read so a TOCTOU race (file appended after fstat)
+                    # cannot bypass the limit.
+                    if st.st_size > MAX_ZIP_MEMBER_BYTES:
+                        raise BundlerError(
+                            f"Bundle file {arcname} exceeds {MAX_ZIP_MEMBER_BYTES}-byte limit"
+                        )
+                    content = fh.read(MAX_ZIP_MEMBER_BYTES + 1)
+                    if len(content) > MAX_ZIP_MEMBER_BYTES:
+                        raise BundlerError(
+                            f"Bundle file {arcname} exceeds {MAX_ZIP_MEMBER_BYTES}-byte limit"
+                        )
+                    archive.writestr(info, content)
+
+        # All members written successfully — atomically replace the final path.
+        # mkstemp() creates the staging file 0600 (owner-only) and os.replace()
+        # preserves that mode, which would silently publish every rebuild as an
+        # unreadable-to-others archive (a direct ZipFile(path, "w") write used
+        # to produce 0666 & ~umask).  Set an intentional mode first: a rebuild
+        # keeps the existing artifact's mode so publishing pipelines that
+        # chmod'd it are not overridden; a fresh build gets 0644.
+        if artifact_path.exists():
+            os.chmod(tmp_path, artifact_path.stat().st_mode & 0o777)
+        else:
+            os.chmod(tmp_path, 0o644)
+        os.replace(tmp_path, artifact_path)
+    except BaseException:
+        # Clean up the temporary file on any failure (exception, interrupt, etc.)
+        tmp_path.unlink(missing_ok=True)
+        raise
 
     return BuildResult(artifact_path=artifact_path, file_count=len(files))
 
@@ -115,6 +173,7 @@ def _collect_files(
     skip: Path,
     skip_dir: Path | None = None,
     artifact_re: re.Pattern[str] | None = None,
+    staging_re: re.Pattern[str] | None = None,
 ) -> list[Path]:
     collected: list[Path] = []
     # followlinks=False so a symlinked directory is never descended into,
@@ -140,6 +199,9 @@ def _collect_files(
             if artifact_re is not None and artifact_re.match(name):
                 # A prior build artifact for this bundle — never re-package it.
                 continue
+            if staging_re is not None and staging_re.match(name):
+                # A leftover packager staging file — never re-package it.
+                continue
             if path.is_symlink():
                 # Skip symlinked files to avoid escaping the bundle directory.
                 continue

tests/specify_cli/bundles/test_packager.py

@@ -2,14 +2,15 @@
 from __future__ import annotations
 
 import os
+import stat
 import zipfile
 from pathlib import Path
 
 import pytest
 import yaml
 
 from specify_cli.bundler import BundlerError
-from specify_cli.bundles.packager import build_bundle
+from specify_cli.bundles.packager import MAX_ZIP_MEMBER_BYTES, build_bundle
 from tests.specify_cli.bundles.helpers import valid_manifest_dict
 
 
@@ -234,3 +235,101 @@ def test_toctou_stat_read_consistency(tmp_path: Path):
     assert content == b"\x00\x01\x02\x03"
     assert modes["assets/data.bin"] == 0o644
     assert modes["README.md"] == 0o644
+
+
+def test_oversized_file_is_rejected(tmp_path: Path):
+    """A file exceeding MAX_ZIP_MEMBER_BYTES must be rejected."""
+    bundle = _make_bundle(tmp_path / "b")
+    large = bundle / "assets" / "large.bin"
+    large.parent.mkdir(parents=True, exist_ok=True)
+    large.write_bytes(b"\x00" * (MAX_ZIP_MEMBER_BYTES + 1))
+
+    with pytest.raises(BundlerError, match="exceeds"):
+        build_bundle(bundle, output_dir=tmp_path / "out")
+
+
+def test_boundary_size_file_is_accepted(tmp_path: Path):
+    """A file exactly at MAX_ZIP_MEMBER_BYTES must be accepted."""
+    bundle = _make_bundle(tmp_path / "b")
+    boundary = bundle / "assets" / "boundary.bin"
+    boundary.parent.mkdir(parents=True, exist_ok=True)
+    boundary.write_bytes(b"\x00" * MAX_ZIP_MEMBER_BYTES)
+
+    result = build_bundle(bundle, output_dir=tmp_path / "out")
+    with zipfile.ZipFile(result.artifact_path) as archive:
+        content = archive.read("assets/boundary.bin")
+    assert len(content) == MAX_ZIP_MEMBER_BYTES
+
+
+def test_oversized_file_does_not_corrupt_output(tmp_path: Path):
+    """When an oversized file is rejected, the output path must not contain
+    a partial/corrupt archive — it should either not exist or contain the
+    previous valid artifact (if any)."""
+    bundle = _make_bundle(tmp_path / "b", extra_files={"good.txt": "ok"})
+
+    # First build succeeds.
+    out_dir = tmp_path / "out"
+    first = build_bundle(bundle, output_dir=out_dir)
+    first_bytes = first.artifact_path.read_bytes()
+
+    # Add an oversized file.
+    large = bundle / "assets" / "large.bin"
+    large.parent.mkdir(parents=True, exist_ok=True)
+    large.write_bytes(b"\x00" * (MAX_ZIP_MEMBER_BYTES + 1))
+
+    # Second build fails — but the original artifact must remain intact.
+    with pytest.raises(BundlerError, match="exceeds"):
+        build_bundle(bundle, output_dir=out_dir)
+
+    assert first.artifact_path.exists()
+    assert first.artifact_path.read_bytes() == first_bytes
+
+
+def test_temp_file_cleaned_up_on_failure(tmp_path: Path):
+    """No .tmp files must remain in the output directory after a failed build."""
+    bundle = _make_bundle(tmp_path / "b")
+    large = bundle / "assets" / "large.bin"
+    large.parent.mkdir(parents=True, exist_ok=True)
+    large.write_bytes(b"\x00" * (MAX_ZIP_MEMBER_BYTES + 1))
+
+    out_dir = tmp_path / "out"
+    with pytest.raises(BundlerError):
+        build_bundle(bundle, output_dir=out_dir)
+
+    tmp_files = list(out_dir.glob("*.tmp"))
+    assert tmp_files == [], f"Leftover temp files: {tmp_files}"
+
+
+def test_leftover_staging_file_is_not_packaged(tmp_path: Path):
+    """A leftover mkstemp staging file from a prior killed build must never be
+    collected — even with the default out_dir (the bundle source tree)."""
+    bundle = _make_bundle(
+        tmp_path / "b",
+        extra_files={"demo-bundle-1.2.0-abcd1234.tmp": "partial"},
+    )
+    result = build_bundle(bundle)
+    with zipfile.ZipFile(result.artifact_path) as archive:
+        names = set(archive.namelist())
+    assert "demo-bundle-1.2.0-abcd1234.tmp" not in names
+
+
+@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits")
+def test_artifact_is_world_readable_after_build(tmp_path: Path):
+    """A fresh build must publish a 0644 artifact, not the 0600 mode that
+    mkstemp() creates the staging file with."""
+    bundle = _make_bundle(tmp_path / "b")
+    result = build_bundle(bundle, output_dir=tmp_path / "out")
+    assert stat.S_IMODE(result.artifact_path.stat().st_mode) == 0o644
+
+
+@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits")
+def test_rebuild_preserves_existing_artifact_mode(tmp_path: Path):
+    """A rebuild must keep a pre-existing artifact's mode instead of resetting
+    it (publishing pipelines may have chmod'd the artifact deliberately)."""
+    bundle = _make_bundle(tmp_path / "b")
+    out_dir = tmp_path / "out"
+    first = build_bundle(bundle, output_dir=out_dir)
+    os.chmod(first.artifact_path, 0o640)
+
+    second = build_bundle(bundle, output_dir=out_dir)
+    assert stat.S_IMODE(second.artifact_path.stat().st_mode) == 0o640