diff --git a/core/image_cache.py b/core/image_cache.py index 040c1126..e1087e34 100644 --- a/core/image_cache.py +++ b/core/image_cache.py @@ -172,11 +172,14 @@ class ImageCache: raise ImageCacheError(f"Upstream response is not an image: {mime_type}") declared_size = response.headers.get("Content-Length") + expected_bytes = None try: - if declared_size and int(declared_size) > self.max_download_bytes: - raise ImageCacheError("Image exceeds configured size limit") + if declared_size: + expected_bytes = int(declared_size) + if expected_bytes > self.max_download_bytes: + raise ImageCacheError("Image exceeds configured size limit") except ValueError: - pass + expected_bytes = None ext = mimetypes.guess_extension(mime_type) or ".img" if ext == ".jpe": @@ -205,6 +208,22 @@ class ImageCache: if total <= 0: raise ImageCacheError("Image response was empty") + # Truncation guard (#750): a dropped/short connection makes + # iter_content end early WITHOUT raising, so a partial image would + # otherwise be committed as status='ok' and cached permanently — + # rendering as a half-decoded cover (top strip, rest grey). If the + # server declared a Content-Length and we got fewer bytes, treat it + # as a failed download: discard the tmp file and don't cache it, so + # the next request retries fresh instead of serving a broken file. + if expected_bytes is not None and total < expected_bytes: + try: + tmp_path.unlink(missing_ok=True) + except Exception as cleanup_exc: + logger.debug("image_cache tmp cleanup failed: %s", cleanup_exc) + raise ImageCacheError( + f"Truncated image download: got {total} of {expected_bytes} bytes" + ) + os.replace(tmp_path, path) expires_at = now + self.ttl_seconds with self._db_lock: diff --git a/tests/test_image_cache.py b/tests/test_image_cache.py index f4556757..f4d21bde 100644 --- a/tests/test_image_cache.py +++ b/tests/test_image_cache.py @@ -4,12 +4,16 @@ from core.image_cache import ImageCache class FakeResponse: - def __init__(self, body: bytes, *, status_code: int = 200, content_type: str = "image/jpeg"): + def __init__(self, body: bytes, *, status_code: int = 200, content_type: str = "image/jpeg", + declared_length: int | None = None): self.body = body self.status_code = status_code + # declared_length lets a test simulate a truncated download: the server + # promises N bytes (Content-Length) but the body delivers fewer. + length = len(body) if declared_length is None else declared_length self.headers = { "Content-Type": content_type, - "Content-Length": str(len(body)), + "Content-Length": str(length), } self.closed = False @@ -51,6 +55,73 @@ def test_get_url_fetches_once_then_serves_cached_file(tmp_path): assert len(calls) == 1 +def test_truncated_download_is_rejected_not_cached(tmp_path): + """#750: a short/dropped download (body shorter than the declared + Content-Length) must NOT be committed as a good cache entry — otherwise the + half-decoded cover (top strip, rest grey) is served forever. It should raise + and leave nothing cached, so the next request retries fresh.""" + calls = [] + + def fetcher(url, **kwargs): + calls.append(url) + # Server promises 5000 bytes but only delivers 800 (connection dropped). + return FakeResponse(b"x" * 800, declared_length=5000) + + cache = ImageCache(tmp_path, fetcher=fetcher) + url = "https://images.example.test/big-cover.jpg" + + raised = False + try: + cache.get_url(url) + except Exception as exc: + raised = True + assert "Truncated" in str(exc) or "truncated" in str(exc) + assert raised, "Expected a truncated download to raise" + + # Nothing partial left on disk for this key. + import glob + key = ImageCache.key_for_url(url) + leftover = glob.glob(str(tmp_path / "**" / f"{key}*"), recursive=True) + leftover = [p for p in leftover if not p.endswith(".sqlite3")] + assert leftover == [], f"truncated file should not be cached, found: {leftover}" + + # A subsequent SUCCESSFUL fetch works (not poisoned by the failed attempt). + cache2 = ImageCache( + tmp_path, + fetcher=lambda u, **k: FakeResponse(b"complete-jpeg-bytes"), + ) + result = cache2.get_url(url) + assert result.path.read_bytes() == b"complete-jpeg-bytes" + + +def test_complete_download_with_content_length_succeeds(tmp_path): + """Positive control: a full download whose body matches Content-Length is + cached normally (the truncation guard doesn't false-positive).""" + cache = ImageCache( + tmp_path, + fetcher=lambda u, **k: FakeResponse(b"a-real-cover"), # declared==actual + ) + result = cache.get_url("https://images.example.test/ok-cover.jpg") + assert result.path.read_bytes() == b"a-real-cover" + assert result.status == "miss" + + +def test_no_content_length_still_caches(tmp_path): + """Some CDNs omit Content-Length (chunked transfer). With no declared size + we can't detect truncation, so we must NOT reject — cache as before.""" + class NoLengthResponse(FakeResponse): + def __init__(self, body): + super().__init__(body) + del self.headers["Content-Length"] + + cache = ImageCache( + tmp_path, + fetcher=lambda u, **k: NoLengthResponse(b"chunked-cover-bytes"), + ) + result = cache.get_url("https://images.example.test/chunked.jpg") + assert result.path.read_bytes() == b"chunked-cover-bytes" + + def test_get_url_rejects_non_image_responses(tmp_path): cache = ImageCache( tmp_path,