diff --git a/src/specify_cli/bundles/manifest.py b/src/specify_cli/bundles/manifest.py index 71304bc682..b85caec083 100644 --- a/src/specify_cli/bundles/manifest.py +++ b/src/specify_cli/bundles/manifest.py @@ -85,8 +85,8 @@ def components(self) -> list[ComponentRef]: # -- construction --------------------------------------------------------- @classmethod - def from_file(cls, path: Path) -> "BundleManifest": - data = load_yaml(path) + def from_file(cls, path: Path, *, max_bytes: int | None = None) -> "BundleManifest": + data = load_yaml(path, max_bytes=max_bytes) manifest = cls.from_dict(data) manifest.source_path = Path(path) return manifest diff --git a/src/specify_cli/bundles/packager.py b/src/specify_cli/bundles/packager.py index bd80bf7b7c..183f59a37f 100644 --- a/src/specify_cli/bundles/packager.py +++ b/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: @@ -48,7 +54,10 @@ def build_bundle( "README.md describing it." ) - manifest = BundleManifest.from_file(manifest_path) + # The member loop below bounds every packaged file, but the manifest is + # parsed (fully read into memory) before that loop ever runs, so bound the + # read here as well. + manifest = BundleManifest.from_file(manifest_path, max_bytes=MAX_ZIP_MEMBER_BYTES) report = validate_manifest(manifest) if not report.ok: raise BundlerError( @@ -76,28 +85,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 (--.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 +176,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 +202,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 diff --git a/src/specify_cli/bundles/yamlio.py b/src/specify_cli/bundles/yamlio.py index 40b3be4924..5ba5a501a8 100644 --- a/src/specify_cli/bundles/yamlio.py +++ b/src/specify_cli/bundles/yamlio.py @@ -38,26 +38,41 @@ def ensure_within(root: Path, candidate: Path) -> Path: return candidate_resolved -def load_yaml(path: Path) -> Any: +def load_yaml(path: Path, *, max_bytes: int | None = None) -> Any: """Parse a YAML file, returning ``{}`` only for an *empty* document. - A non-empty document is returned exactly as parsed — including a + A non-empty document is returned exactly as parsed � including a non-mapping such as ``[]``, ``false``, ``0``, ``''``, or an explicit null - (``null``/``~``) — so callers can validate the top-level shape (e.g. reject + (``null``/``~``) � so callers can validate the top-level shape (e.g. reject a non-mapping config) instead of having it silently coerced to an empty mapping. ``yaml.safe_load`` returns ``None`` for *both* an empty document and an explicit null scalar, so ``yaml.compose`` (which yields no node only for a truly empty document) is used to tell them apart: an empty document becomes - ``{}`` while an explicit ``null``/``~`` is returned as ``None`` for the + ``{}`` while an explicit ``null`` is returned as ``None`` for the caller to reject. + + When ``max_bytes`` is given the file is read through a bounded read, so a + file that grows after the size check (or is simply huge) never reaches the + YAML parser. """ path = Path(path) if not path.exists(): raise BundlerError(f"File not found: {path}") try: - text = path.read_text(encoding="utf-8") + if max_bytes is None: + text = path.read_text(encoding="utf-8") + else: + with open(path, "rb") as fh: + raw = fh.read(max_bytes + 1) + if len(raw) > max_bytes: + raise BundlerError( + f"File {path} exceeds {max_bytes}-byte limit" + ) + text = raw.decode("utf-8") + except BundlerError: + raise except (OSError, UnicodeError) as exc: # A non-UTF-8 file raises UnicodeDecodeError, which is a ValueError -- # NOT an OSError -- so it escaped this module's "IO failures degrade diff --git a/tests/specify_cli/bundles/test_packager.py b/tests/specify_cli/bundles/test_packager.py index 0044e37830..7851589a36 100644 --- a/tests/specify_cli/bundles/test_packager.py +++ b/tests/specify_cli/bundles/test_packager.py @@ -2,6 +2,7 @@ from __future__ import annotations import os +import stat import zipfile from pathlib import Path @@ -9,7 +10,7 @@ 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,135 @@ 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 + + +def test_oversized_manifest_is_rejected_before_parse(tmp_path: Path): + """``bundle.yml`` is fully read by the manifest parser before the member + loop's size check runs, so that read must be bounded too. + + The message must come from the manifest read (it carries the full path), + not from the later member check (which names only the archive name). + """ + bundle = _make_bundle(tmp_path / "b") + manifest_path = bundle / "bundle.yml" + original = manifest_path.read_text(encoding="utf-8") + manifest_path.write_text( + original + "\n#" + "x" * MAX_ZIP_MEMBER_BYTES + "\n", encoding="utf-8" + ) + + with pytest.raises(BundlerError) as excinfo: + build_bundle(bundle, output_dir=tmp_path / "out") + + message = str(excinfo.value) + assert "exceeds" in message + assert str(manifest_path) in message + + +def test_manifest_within_limit_still_builds(tmp_path: Path): + """A manifest under the limit keeps the normal build path working.""" + bundle = _make_bundle(tmp_path / "b") + out_dir = tmp_path / "out" + + result = build_bundle(bundle, output_dir=out_dir) + + assert result.artifact_path.exists() + with zipfile.ZipFile(result.artifact_path) as archive: + assert "bundle.yml" in archive.namelist()