From fa65d8e9c4e82ba3becd0960dec3a9576f11cd14 Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Wed, 23 Sep 2026 22:48:05 +0500 Subject: [PATCH 1/2] fix: enforce size limit on individual files in build_bundle() Reject files exceeding MAX_ZIP_MEMBER_BYTES (10 MiB) before archiving, bounding the actual read to prevent a TOCTOU bypass of the size check. Build into a temporary sibling file and atomically replace the final artifact only after every member passes validation, so a rejected member never leaves a partial/corrupt archive at the output path; the staging file is cleaned up on any failure. --- src/specify_cli/bundles/packager.py | 74 ++++++++++++++++------ tests/specify_cli/bundles/test_packager.py | 65 ++++++++++++++++++- 2 files changed, 119 insertions(+), 20 deletions(-) diff --git a/src/specify_cli/bundles/packager.py b/src/specify_cli/bundles/packager.py index bd80bf7b7c..fe42a144ab 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: @@ -79,25 +85,55 @@ def build_bundle( files = _collect_files( bundle_dir, skip=artifact_path, skip_dir=skip_dir, artifact_re=artifact_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. + 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)) diff --git a/tests/specify_cli/bundles/test_packager.py b/tests/specify_cli/bundles/test_packager.py index 0044e37830..c6f61fe714 100644 --- a/tests/specify_cli/bundles/test_packager.py +++ b/tests/specify_cli/bundles/test_packager.py @@ -9,7 +9,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 +234,66 @@ 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}" From 2619a890286c44d7c466c5638c574d042b038e23 Mon Sep 17 00:00:00 2001 From: Quratulain-bilal Date: Wed, 23 Sep 2026 22:53:01 +0500 Subject: [PATCH 2/2] fix: set intentional artifact mode and exclude staging files from collection mkstemp() creates the staging file 0600 and os.replace() preserves that mode, so every rebuilt artifact became owner-only; chmod the staging file before replacing (preserving an existing artifact's mode on rebuild, otherwise 0644). Also exclude leftover mkstemp staging files (--.tmp) from _collect_files so a killed build's partial archive is never re-packaged when out_dir is the bundle source tree. --- src/specify_cli/bundles/packager.py | 28 ++++++++++++++++- tests/specify_cli/bundles/test_packager.py | 36 ++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/bundles/packager.py b/src/specify_cli/bundles/packager.py index fe42a144ab..be266aa9da 100644 --- a/src/specify_cli/bundles/packager.py +++ b/src/specify_cli/bundles/packager.py @@ -82,8 +82,20 @@ 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, ) # Build into a temporary sibling and atomically replace the final path only @@ -129,6 +141,16 @@ def build_bundle( 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.) @@ -151,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, @@ -176,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 diff --git a/tests/specify_cli/bundles/test_packager.py b/tests/specify_cli/bundles/test_packager.py index c6f61fe714..741bdfef3d 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 @@ -297,3 +298,38 @@ def test_temp_file_cleaned_up_on_failure(tmp_path: Path): 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