From c303925ca0d4f34937501c03c8224743d9797021 Mon Sep 17 00:00:00 2001 From: Noor-ul-ain001 Date: Sun, 2 Aug 2026 10:33:43 +0500 Subject: [PATCH] fix(archives): wrap the bare EOFError a truncated tar.gz raises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tarfile` wraps most decompression failures in `TarError`, but a gzip stream that ends before its end-of-stream marker escapes as a bare `EOFError` from the gzip layer. `EOFError` derives from neither `TarError` nor `OSError`, so it bypassed all three of the tar handlers added with tar archive support (#3874): - the format probe in `detect_archive_format`, which caught only `tarfile.TarError`; - `tarfile.open` in `safe_extract_tar`; - member iteration in `safe_extract_tar`. A truncated `.tar.gz` — an interrupted download, a partially written file — therefore raised a raw `EOFError` straight through the caller's `error_type`, so callers catching `ValueError`/`ExtensionError`/ `PresetError` never saw it. In `specify workflow add` the effect is worse than a traceback: Typer treats a bare `EOFError` as a Ctrl-D abort, so the command printed only "Aborted." with no diagnostic at all. The ZIP twin reports "Invalid workflow archive: Invalid ZIP archive: ". Route all three sites through a shared `_TAR_DECOMPRESSION_ERRORS` tuple so they stay in sync. `zlib.error` is included alongside `EOFError`: it is likewise neither a `TarError` nor an `OSError` and can surface from a corrupt deflate block. `OSError` is kept only on the two `safe_extract_tar` sites, which report genuine I/O failures; adding it to the probe would silently swallow them instead. Truncated tar.gz now reports the same clean, domain-typed error as the ZIP path. Tests cover both the short prefix that fails in `tarfile.open` and the longer ones that fail during member iteration — `tarfile` decompresses lazily, so the leak surfaced at different sites depending on how much of the stream survived. Assisted-by: Claude Opus 5 (1M context) Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/_download_security.py | 18 ++++++-- tests/test_download_security.py | 64 +++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/src/specify_cli/_download_security.py b/src/specify_cli/_download_security.py index 9d2d95ea72..eaedfd5b40 100644 --- a/src/specify_cli/_download_security.py +++ b/src/specify_cli/_download_security.py @@ -10,6 +10,7 @@ import tarfile import unicodedata import zipfile +import zlib from collections.abc import Iterator from contextlib import ExitStack, contextmanager from ipaddress import IPv4Address, IPv6Address, ip_address @@ -69,6 +70,13 @@ _BOUNDED_ZIP_COMPRESSION_METHODS = frozenset( (zipfile.ZIP_STORED, zipfile.ZIP_DEFLATED) ) +#: Decompression failures a truncated or corrupt gzip stream raises from +#: ``tarfile``. Most are wrapped in ``TarError``, but a stream that ends +#: mid-member escapes as a bare ``EOFError`` from the gzip layer, and a corrupt +#: deflate block can surface as ``zlib.error``. Neither derives from +#: ``TarError`` or ``OSError``, so both bypass a ``(TarError, OSError)`` handler. +_TAR_DECOMPRESSION_ERRORS = (tarfile.TarError, EOFError, zlib.error) + _ARCHIVE_CONTENT_TYPES: dict[str, ArchiveFormat] = { "application/gzip": "tar.gz", "application/x-gzip": "tar.gz", @@ -166,7 +174,11 @@ def detect_archive_format( try: with tarfile.open(fileobj=archive_file, mode="r:gz"): is_tar_gz = True - except tarfile.TarError: + except _TAR_DECOMPRESSION_ERRORS: + # A truncated gzip stream raises a bare EOFError here rather + # than a TarError, so catching only TarError let it escape + # this probe as a raw exception instead of leaving + # ``is_tar_gz`` False and reporting the format mismatch. pass archive_file.seek(0) except OSError as exc: @@ -1077,7 +1089,7 @@ def safe_extract_tar( mode="r:gz", fileobj=archive_file, ) - except (tarfile.TarError, OSError) as exc: + except (*_TAR_DECOMPRESSION_ERRORS, OSError) as exc: _raise_from(error_type, f"Invalid tar.gz archive: {archive_path}", exc) with archive: @@ -1149,7 +1161,7 @@ def safe_extract_tar( f"of {max_total_bytes} bytes", ) validated.append((member, normalized_name, is_dir)) - except (tarfile.TarError, OSError) as exc: + except (*_TAR_DECOMPRESSION_ERRORS, OSError) as exc: _raise_from( error_type, f"Invalid tar.gz archive: {archive_path}", diff --git a/tests/test_download_security.py b/tests/test_download_security.py index df6f9180d4..f233df4ba9 100644 --- a/tests/test_download_security.py +++ b/tests/test_download_security.py @@ -475,6 +475,70 @@ def test_safe_extract_tar_enforces_entry_and_size_limits(tmp_path): safe_extract_tar(archive_path, tmp_path / "total", max_total_bytes=7) +def _truncated_tar_gz_bytes(keep_bytes): + """Return the leading *keep_bytes* of a multi-member tar.gz's bytes. + + A gzip stream cut short this way ends before its end-of-stream marker, so + reading it raises a bare ``EOFError`` from the gzip layer. ``tarfile`` + decompresses lazily, so *where* that surfaces depends on how much is kept: + a very short prefix fails in ``tarfile.open`` itself, while a longer one + opens fine and only fails once members are iterated. + """ + buffer = io.BytesIO() + with tarfile.open(fileobj=buffer, mode="w:gz") as archive: + for index in range(5): + info = tarfile.TarInfo(f"file{index}.txt") + content = bytes(range(256)) * 400 + info.size = len(content) + archive.addfile(info, io.BytesIO(content)) + return buffer.getvalue()[:keep_bytes] + + +def test_detect_archive_format_rejects_truncated_tar_gz(tmp_path): + # A gzip stream truncated before tarfile can read its first header raises a + # bare EOFError -- not a TarError -- from the format probe. Catching only + # TarError let it escape as a raw exception instead of leaving is_tar_gz + # False and reporting the module's clean format-mismatch error. + archive_path = tmp_path / "truncated.tar.gz" + archive_path.write_bytes(_truncated_tar_gz_bytes(64)) + + with pytest.raises(ValueError, match="format mismatch"): + detect_archive_format(archive_path) + + +@pytest.mark.parametrize("keep_bytes", [64, 512, 2048]) +def test_safe_extract_tar_rejects_truncated_archive(tmp_path, keep_bytes): + # The same bare EOFError, from tarfile.open on a short prefix and from + # member iteration on a longer one. Both sites reported it raw. + archive_path = tmp_path / f"truncated-{keep_bytes}.tar.gz" + archive_path.write_bytes(_truncated_tar_gz_bytes(keep_bytes)) + + with pytest.raises(ValueError, match="Invalid tar.gz archive"): + safe_extract_tar(archive_path, tmp_path / f"out-{keep_bytes}") + + +def test_safe_extract_tar_wraps_truncation_in_caller_error_type(tmp_path): + # The leak bypassed the caller's domain error type entirely, so callers + # that only catch their own error (or ValueError) crashed the command. + archive_path = tmp_path / "truncated.tar.gz" + archive_path.write_bytes(_truncated_tar_gz_bytes(2048)) + + with pytest.raises(_CustomZipError, match="Invalid tar.gz archive"): + safe_extract_tar( + archive_path, + tmp_path / "out", + error_type=_CustomZipError, + ) + + +def test_safe_extract_archive_rejects_truncated_tar_gz(tmp_path): + archive_path = tmp_path / "truncated.tar.gz" + archive_path.write_bytes(_truncated_tar_gz_bytes(2048)) + + with pytest.raises(ValueError): + safe_extract_archive(archive_path, tmp_path / "out") + + @pytest.mark.parametrize("suffix", [".zip", ".tar.gz", ".tgz"]) def test_safe_extract_archive_has_format_parity(tmp_path, suffix): archive_path = tmp_path / f"package{suffix}"