fix(archives): wrap the bare EOFError a truncated tar.gz raises (#3938)
* fix(archives): wrap the bare EOFError a truncated tar.gz raises `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: <path>". 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) <noreply@anthropic.com> * test(archives): cover the bare zlib.error a corrupt deflate block raises Review feedback: the `zlib.error` arm of `_TAR_DECOMPRESSION_ERRORS` was not exercised. Every regression added with the fix truncates a valid deflate stream, which raises `EOFError`, so `zlib.error` could regress independently of the EOF handling. It is genuinely reachable, but only under a narrower condition than the truncation cases. `tarfile` converts `zlib.error` to `ReadError` while reading a member *header*, but the forward seek it performs to skip member *data* (`tarfile.next`) sits outside that conversion, so a corrupt region past the first header escapes raw. Reaching that seek needs members larger than the gzip read buffer: with small members the whole stream is decompressed during the first header read and the error is wrapped. The new fixture therefore uses two 256 KiB members at `compresslevel=1` — a ~7 KiB archive — corrupted past the midpoint so the first header still reads clean. Adds four tests: the two `safe_extract_tar` sites (plain and with a caller-supplied `error_type`), the `safe_extract_archive` entry point with a caller-supplied `error_type`, and a guard asserting the fixture still reaches the module as a bare `zlib.error` — so if a future Python wraps it, that fails loudly instead of the coverage silently decaying into a duplicate of the `EOFError` cases. Verified test-the-test: the three wrapping tests fail against the unmodified `_download_security.py` with a raw `zlib.error: Error -3 while decompressing data: invalid distance code`, and pass with the fix. Also corrects the scope claimed for the probe site. Fuzzing 2800 corrupt archives never produced a bare `zlib.error` from `tarfile.open` alone, because the only read it performs is the header read that `tarfile` already converts. The probe's `zlib.error` arm is defensive, not load-bearing; the tuple comment and a detection test now say so rather than implying coverage that cannot exist. Assisted-by: Claude Opus 5 (1M context) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(archives): make the corrupt-deflate fixture zlib-version independent CI failure on macos-latest/3.13: `test_corrupt_deflate_fixture_raises_bare_zlib_error` failed with `gzip.BadGzipFile: CRC check failed`. The other five pytest jobs were fail-fast cancellations, not real failures, and ruff was already green. The fixture built its corruption by XOR-ing 64 arbitrary bytes mid-stream. Whether that produces a *structural* deflate error is zlib-version dependent: on the macOS runner the mangled bytes still decoded, so the stream instead failed the trailing gzip CRC check and raised `BadGzipFile` -- an `OSError`, which the pre-fix `(TarError, OSError)` handler already caught. The guard test exists precisely to catch that degradation, and it did its job. Replaces the XOR with a deflate block header whose `BTYPE` is the reserved value `0b11`. Every zlib rejects that identically as "invalid block type", and it fails during decompression rather than at the CRC check, so no version can turn it into a `TarError` or `OSError`. The stream is assembled by hand (`compressobj(-15)` + explicit gzip header/trailer) so the invalid block lands a controlled 256 KiB into the first member's data -- past the gzip read buffer, so the first header still reads clean and the failure surfaces from the forward seek in `tarfile.next`, which is the site the raw `zlib.error` escapes from. A sweep over clean-prefix sizes confirms a wide margin: with 512 KiB members every prefix from 160 KiB up yields a bare `zlib.error`, versus the transition below ~131 KiB where `tarfile` still wraps it as `ReadError`. The hand-built gzip header also zeroes the mtime field, so the fixture is now byte-identical across builds instead of embedding a timestamp. Strengthens the guard to assert what the fix actually depends on -- that the exception is neither a `TarError` nor an `OSError` -- so the fixture cannot silently decay into an already-caught type again. Production code is unchanged from ef49acc; this is test-only. Verified test-the-test by dropping the `zlib.error` arm from `_TAR_DECOMPRESSION_ERRORS`: the three wrapping tests fail with the raw `zlib.error: Error -3 while decompressing data: invalid block type`, and pass with it restored. `tests/test_download_security.py`: 193 passed. `ruff check src tests` (the exact CI command): all checks passed. Assisted-by: Claude Opus 4.8 (1M context) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Noor ul ain
@@ -10,6 +10,7 @@ import struct
|
||||
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,19 @@ _ZIP_MAX_COMMENT_BYTES = (1 << 16) - 1
|
||||
_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 two escape raw, and
|
||||
#: neither derives from ``TarError`` or ``OSError``, so both bypass a
|
||||
#: ``(TarError, OSError)`` handler:
|
||||
#:
|
||||
#: * ``EOFError`` -- from the gzip layer when the stream ends before its
|
||||
#: end-of-stream marker, i.e. a truncated archive.
|
||||
#: * ``zlib.error`` -- from a corrupt deflate block. ``tarfile`` converts this
|
||||
#: to ``ReadError`` while reading a member *header*, but the forward seek it
|
||||
#: performs to skip member *data* sits outside that conversion, so a corrupt
|
||||
#: region past the first header escapes raw.
|
||||
_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 +180,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 +1095,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 +1167,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}",
|
||||
|
||||
@@ -475,6 +475,194 @@ 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")
|
||||
|
||||
|
||||
#: Bytes of the first member's data that decompress cleanly before the invalid
|
||||
#: deflate block. Must exceed the gzip read buffer so ``tarfile`` has to seek
|
||||
#: forward over member data to reach the second header -- see
|
||||
#: ``_corrupt_deflate_tar_gz_bytes``. The members are twice this size, so the
|
||||
#: corruption stays well inside the first member's data.
|
||||
_CORRUPT_DEFLATE_CLEAN_BYTES = 256 * 1024
|
||||
_CORRUPT_DEFLATE_MEMBER_BYTES = 2 * _CORRUPT_DEFLATE_CLEAN_BYTES
|
||||
|
||||
|
||||
def _corrupt_deflate_tar_gz_bytes():
|
||||
"""Return a tar.gz whose deflate stream is corrupt mid-member.
|
||||
|
||||
Unlike truncation, which the gzip layer reports as ``EOFError``, an invalid
|
||||
deflate block raises ``zlib.error``. ``tarfile`` converts that to
|
||||
``ReadError`` when it surfaces while reading a member *header*, but the
|
||||
forward seek it performs to skip over member *data* sits outside that
|
||||
conversion, so the raw ``zlib.error`` escapes from there.
|
||||
|
||||
Two details keep this deterministic across zlib versions:
|
||||
|
||||
* The corruption is a block header whose ``BTYPE`` is the reserved value
|
||||
``0b11``, which every zlib rejects as "invalid block type". Mangling
|
||||
arbitrary bytes instead is *not* portable -- the garbage may still decode
|
||||
structurally and fail the later gzip CRC check as ``BadGzipFile`` (an
|
||||
``OSError``, which the handler already caught) rather than raising
|
||||
``zlib.error`` at all.
|
||||
* The stream is assembled by hand so the invalid block lands after
|
||||
``_CORRUPT_DEFLATE_CLEAN_BYTES`` of valid data. That is past the gzip read
|
||||
buffer, so the first header reads clean and the failure happens during the
|
||||
seek over member data rather than during a header read.
|
||||
"""
|
||||
plain = io.BytesIO()
|
||||
with tarfile.open(fileobj=plain, mode="w") as archive:
|
||||
for index in range(2):
|
||||
info = tarfile.TarInfo(f"file{index}.txt")
|
||||
content = bytes((i * 7 + index) % 256 for i in range(1024)) * (
|
||||
_CORRUPT_DEFLATE_MEMBER_BYTES // 1024
|
||||
)
|
||||
info.size = len(content)
|
||||
archive.addfile(info, io.BytesIO(content))
|
||||
|
||||
clean_prefix = plain.getvalue()[:_CORRUPT_DEFLATE_CLEAN_BYTES]
|
||||
compressor = zlib.compressobj(1, zlib.DEFLATED, -15)
|
||||
deflate = compressor.compress(clean_prefix)
|
||||
deflate += compressor.flush(zlib.Z_SYNC_FLUSH)
|
||||
deflate += b"\x06" # BTYPE=0b11 (reserved) -> "invalid block type"
|
||||
|
||||
gzip_header = b"\x1f\x8b\x08\x00" + b"\x00" * 4 + b"\x00\xff"
|
||||
trailer = struct.pack("<II", zlib.crc32(clean_prefix), len(clean_prefix))
|
||||
return gzip_header + deflate + trailer
|
||||
|
||||
|
||||
def test_corrupt_deflate_fixture_raises_bare_zlib_error():
|
||||
# Guards the fixture itself: the tests below are only meaningful while this
|
||||
# archive reaches the module as a bare zlib.error -- neither a TarError nor
|
||||
# an OSError, so a (TarError, OSError) handler would miss it. If a future
|
||||
# Python or zlib wraps it, this fails loudly instead of the coverage
|
||||
# silently decaying into a duplicate of the EOFError cases.
|
||||
archive_file = io.BytesIO(_corrupt_deflate_tar_gz_bytes())
|
||||
|
||||
with tarfile.open(fileobj=archive_file, mode="r:gz") as archive:
|
||||
with pytest.raises(zlib.error) as excinfo:
|
||||
for _member in archive:
|
||||
pass
|
||||
|
||||
# The whole point of the zlib.error arm: a (TarError, OSError) handler --
|
||||
# what the two extraction sites had before the fix -- does not catch this.
|
||||
# tarfile.ReadError and gzip.BadGzipFile would both be caught already, so
|
||||
# if the fixture ever degrades into one of those it proves nothing.
|
||||
assert not isinstance(excinfo.value, tarfile.TarError)
|
||||
assert not isinstance(excinfo.value, OSError)
|
||||
|
||||
|
||||
def test_detect_archive_format_accepts_corrupt_deflate_tar_gz(tmp_path):
|
||||
# Detection is a format probe, not an integrity check: tarfile.open reads
|
||||
# only the first member header, which is intact here, so the archive is
|
||||
# correctly identified as tar.gz and the corruption is caught later by
|
||||
# safe_extract_tar (see the tests below).
|
||||
#
|
||||
# Note this does not exercise the probe's zlib.error handling, which is
|
||||
# unreachable: the header read is inside tarfile's own
|
||||
# zlib.error -> ReadError conversion, so the probe sees ReadError. The
|
||||
# zlib.error arm of _TAR_DECOMPRESSION_ERRORS is defensive at this site and
|
||||
# load-bearing only at the two safe_extract_tar sites.
|
||||
archive_path = tmp_path / "corrupt.tar.gz"
|
||||
archive_path.write_bytes(_corrupt_deflate_tar_gz_bytes())
|
||||
|
||||
assert detect_archive_format(archive_path) == "tar.gz"
|
||||
|
||||
|
||||
def test_safe_extract_tar_rejects_corrupt_deflate(tmp_path):
|
||||
archive_path = tmp_path / "corrupt.tar.gz"
|
||||
archive_path.write_bytes(_corrupt_deflate_tar_gz_bytes())
|
||||
|
||||
with pytest.raises(ValueError, match="Invalid tar.gz archive"):
|
||||
safe_extract_tar(archive_path, tmp_path / "out")
|
||||
|
||||
|
||||
def test_safe_extract_tar_wraps_corrupt_deflate_in_caller_error_type(tmp_path):
|
||||
# zlib.error must reach the caller's domain error type, exactly as EOFError
|
||||
# does, so this cannot regress independently of the truncation handling.
|
||||
archive_path = tmp_path / "corrupt.tar.gz"
|
||||
archive_path.write_bytes(_corrupt_deflate_tar_gz_bytes())
|
||||
|
||||
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_wraps_corrupt_deflate_in_caller_error_type(tmp_path):
|
||||
archive_path = tmp_path / "corrupt.tar.gz"
|
||||
archive_path.write_bytes(_corrupt_deflate_tar_gz_bytes())
|
||||
|
||||
with pytest.raises(_CustomZipError, match="Invalid tar.gz archive"):
|
||||
safe_extract_archive(
|
||||
archive_path,
|
||||
tmp_path / "out",
|
||||
error_type=_CustomZipError,
|
||||
)
|
||||
|
||||
|
||||
@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}"
|
||||
|
||||
Reference in New Issue
Block a user