fix(history): reject invalid inspection targets (#209)

py/api/images.py

@@ -20,7 +20,13 @@ from PIL import Image, UnidentifiedImageError
 from .prompts import bad_request, parse_page_params, publish_image_paths
 from .prompts import thumbnail_url_for
 from .prompts import safe_error_message as _safe_error
-from .inspection_service import CurrentFileChanged, inspect_current_file
+from .inspection_service import (
+    CurrentFileChanged,
+    IMAGE_EXTENSIONS as _IMAGE_EXTENSIONS,
+    SERVABLE_EXTENSIONS,
+    VIDEO_EXTENSIONS,
+    inspect_current_file,
+)
 
 try:
     from ...utils.parallel import map_parallel
@@ -29,23 +35,8 @@ except ImportError:
     from utils.parallel import map_parallel
     from utils.inspection_response import encode_inspection_response
 
-# Only these file types are ever served by the image routes, regardless of
-# what sits inside an allowed directory (defence in depth for the gallery).
-IMAGE_EXTENSIONS = frozenset(
-    {
-        ".png",
-        ".jpg",
-        ".jpeg",
-        ".webp",
-        ".gif",
-        ".bmp",
-        ".tiff",
-        ".tif",
-        ".mp4",
-        ".webm",
-        ".mov",
-    }
-)
+
+IMAGE_EXTENSIONS = _IMAGE_EXTENSIONS
 
 
 def _public_path(path):
@@ -97,11 +88,7 @@ def _media_access_error(path, allowed_dirs):
 
 # Media types the gallery scans and thumbnails (a superset is never served).
 GALLERY_IMAGE_EXTENSIONS = frozenset({".png", ".jpg", ".jpeg", ".webp", ".gif"})
-VIDEO_EXTENSIONS = frozenset({".mp4", ".webm", ".avi", ".mov", ".mkv", ".m4v", ".wmv"})
 MEDIA_EXTENSIONS = GALLERY_IMAGE_EXTENSIONS | VIDEO_EXTENSIONS
-# What the serve routes hand out: every image type above plus every video type
-# the gallery lists, so a card never points at a 403.
-SERVABLE_EXTENSIONS = IMAGE_EXTENSIONS | VIDEO_EXTENSIONS
 
 # Caps on request-triggered filesystem work.
 MAX_SCAN_DEPTH = 12

py/api/inspection_service.py

@@ -1,5 +1,7 @@
 """One bounded current-file inspection worker shared by physical identities."""
 
+import os
+import stat
 from pathlib import Path
 
 try:
@@ -18,23 +20,56 @@ except ImportError:
     from utils.video_metadata import is_video_path
 
 
+# Only these file types are served or inspected within an authorized directory.
+IMAGE_EXTENSIONS = frozenset(
+    {
+        ".png",
+        ".jpg",
+        ".jpeg",
+        ".webp",
+        ".gif",
+        ".bmp",
+        ".tiff",
+        ".tif",
+        ".mp4",
+        ".webm",
+        ".mov",
+    }
+)
+VIDEO_EXTENSIONS = frozenset({".mp4", ".webm", ".avi", ".mov", ".mkv", ".m4v", ".wmv"})
+SERVABLE_EXTENSIONS = IMAGE_EXTENSIONS | VIDEO_EXTENSIONS
+
+
 class CurrentFileChanged(Exception):
-    """File facts changed across the bounded metadata observation."""
+    """File facts are invalid or changed across the bounded metadata observation."""
+
+
+def _current_file_facts(path: Path) -> os.stat_result:
+    if path.suffix.lower() not in SERVABLE_EXTENSIONS:
+        raise CurrentFileChanged
+    try:
+        observed = path.stat()
+    except (FileNotFoundError, NotADirectoryError, PermissionError):
+        raise CurrentFileChanged from None
+    if (
+        not stat.S_ISREG(observed.st_mode)
+        or not observed.st_mode & 0o444
+        or not os.access(path, os.R_OK)
+    ):
+        raise CurrentFileChanged
+    return observed
 
 
 def inspect_current_file(
     path: Path, *, source_label: str, saved: dict | None, library: dict
 ) -> dict:
-    before = path.stat()
+    before = _current_file_facts(path)
     try:
         inspection = read_inspection_file(path)
     except MetadataReadError:
         inspection = None
-    try:
-        after = path.stat()
-    except FileNotFoundError:
-        raise CurrentFileChanged from None
-    fields = ("st_dev", "st_ino", "st_size", "st_mtime_ns", "st_ctime_ns")
+    after = _current_file_facts(path)
+    fields = ("st_dev", "st_ino", "st_mode", "st_size", "st_mtime_ns", "st_ctime_ns")
     if any(getattr(before, key) != getattr(after, key) for key in fields):
         raise CurrentFileChanged
     if inspection is None:

tests/test_api_history_outputs.py

@@ -4,11 +4,13 @@ import asyncio
 import errno
 import importlib
 import json
+import os
 import sqlite3
 import threading
 from contextlib import ExitStack
 from pathlib import Path
 from types import SimpleNamespace
+from unittest import skipUnless
 from unittest.mock import patch
 
 from aiohttp import web
@@ -1088,3 +1090,150 @@ class HistoryOutputAPITests(ImageAPITestCase):
             body = await response.json()
             self.assert_error(response, body, 503, "storage-unavailable")
             self.assertEqual(body["recovery"], {"action": "retry-read"})
+
+    async def test_inspection_rejects_directory_replacement_before_parser(self):
+        path = png(self.output_dir / "image.png", prompt=graph("Current portrait"))
+        start = self.capture([self.ref()])
+        descriptor = await self.media(start)
+        response = await self.current(descriptor["inspection_url"])
+        body = await response.json()
+        self.assertEqual(response.status, 200, body)
+        self.assertEqual(body["prompts"]["positive"]["text"], "Current portrait")
+        service = importlib.import_module("py.api.inspection_service")
+        inspect = service.inspect_current_file
+        read = service.read_inspection_file
+        reads = []
+
+        def replaced(target, **kwargs):
+            path.unlink()
+            path.mkdir()
+            return inspect(target, **kwargs)
+
+        def traced(target):
+            reads.append(target)
+            return read(target)
+
+        with patch("py.api.history.inspect_current_file", side_effect=replaced):
+            with patch.object(service, "read_inspection_file", side_effect=traced):
+                response = await self.current(descriptor["inspection_url"])
+                body = await response.json()
+        self.assert_error(response, body, 409, "candidate-stale")
+        self.assertEqual(reads, [], "Nonregular targets must not reach the parser")
+        self.assertTrue(path.is_dir())
+        self.assertNotIn("Current portrait", json.dumps(body))
+
+    @skipUnless(os.name == "posix", "FIFO replacement is a POSIX filesystem case")
+    async def test_inspection_rejects_fifo_replacement_before_parser(self):
+        path = make_png(self.output_dir / "image.png")
+        start = self.capture([self.ref()])
+        descriptor = await self.media(start)
+        response = await self.current(descriptor["inspection_url"])
+        self.assertEqual(response.status, 200, await response.json())
+        service = importlib.import_module("py.api.inspection_service")
+        inspect = service.inspect_current_file
+        reads = []
+
+        def replaced(target, **kwargs):
+            path.unlink()
+            os.mkfifo(path)
+            return inspect(target, **kwargs)
+
+        def forbidden(target):
+            reads.append(target)
+            raise AssertionError("FIFO must be rejected before a metadata open")
+
+        with patch("py.api.history.inspect_current_file", side_effect=replaced):
+            with patch.object(service, "read_inspection_file", side_effect=forbidden):
+                response = await self.current(descriptor["inspection_url"])
+                body = await response.json()
+        self.assert_error(response, body, 409, "candidate-stale")
+        self.assertEqual(reads, [], "A FIFO must never reach the blocking parser")
+
+    async def test_inspection_rejects_unreadable_file_before_parser(self):
+        path = png(self.output_dir / "image.png", prompt=graph("Readable portrait"))
+        start = self.capture([self.ref()])
+        descriptor = await self.media(start)
+        service = importlib.import_module("py.api.inspection_service")
+        inspect = service.inspect_current_file
+        read = service.read_inspection_file
+        access = os.access
+        revoked = False
+        reads = []
+
+        def unreadable(target, **kwargs):
+            nonlocal revoked
+            revoked = True
+            return inspect(target, **kwargs)
+
+        def current_access(target, mode, **kwargs):
+            if revoked and Path(target) == path:
+                return False
+            return access(target, mode, **kwargs)
+
+        def traced(target):
+            reads.append(target)
+            return read(target)
+
+        with patch("py.api.history.inspect_current_file", side_effect=unreadable):
+            with patch("os.access", side_effect=current_access):
+                with patch.object(service, "read_inspection_file", side_effect=traced):
+                    response = await self.current(descriptor["inspection_url"])
+                    body = await response.json()
+        self.assert_error(response, body, 409, "candidate-stale")
+        self.assertEqual(reads, [], "Unreadable targets must not reach the parser")
+        response = await self.current(descriptor["inspection_url"])
+        body = await response.json()
+        self.assertEqual(response.status, 200, body)
+        self.assertEqual(body["prompts"]["positive"]["text"], "Readable portrait")
+
+    async def test_inspection_rejects_directory_replacement_after_parser(self):
+        path = png(self.output_dir / "image.png", prompt=graph("Current portrait"))
+        start = self.capture([self.ref()])
+        descriptor = await self.media(start)
+        service = importlib.import_module("py.api.inspection_service")
+        read = service.read_inspection_file
+
+        def replaced(target):
+            result = read(target)
+            path.unlink()
+            path.mkdir()
+            return result
+
+        with patch.object(service, "read_inspection_file", side_effect=replaced):
+            response = await self.current(descriptor["inspection_url"])
+            body = await response.json()
+        self.assert_error(response, body, 409, "candidate-stale")
+        self.assertNotIn("Current portrait", json.dumps(body))
+
+    async def test_inspection_rejects_readability_revoked_after_parser(self):
+        path = png(self.output_dir / "image.png", prompt=graph("Readable portrait"))
+        before = path.read_bytes()
+        start = self.capture([self.ref()])
+        descriptor = await self.media(start)
+        service = importlib.import_module("py.api.inspection_service")
+        read = service.read_inspection_file
+        access = os.access
+        revoked = False
+
+        def changed(target):
+            nonlocal revoked
+            result = read(target)
+            revoked = True
+            return result
+
+        def current_access(target, mode, **kwargs):
+            if revoked and Path(target) == path:
+                return False
+            return access(target, mode, **kwargs)
+
+        with patch.object(service, "read_inspection_file", side_effect=changed):
+            with patch("os.access", side_effect=current_access):
+                response = await self.current(descriptor["inspection_url"])
+                body = await response.json()
+        self.assert_error(response, body, 409, "candidate-stale")
+        self.assertNotIn("Readable portrait", json.dumps(body))
+        self.assertEqual(path.read_bytes(), before)
+        response = await self.current(descriptor["inspection_url"])
+        body = await response.json()
+        self.assertEqual(response.status, 200, body)
+        self.assertEqual(body["prompts"]["positive"]["text"], "Readable portrait")

tests/test_history_current.py

@@ -408,6 +408,33 @@ class CurrentLocationTests(unittest.TestCase):
         self.assertIsNone(result["library"].get("pair_id"))
         self.assertIsNone(result["saved"].get("pair"))
 
+    def test_inspection_rejects_unsupported_suffix_before_parser(self):
+        service = self.module("py.api.inspection_service")
+        path = make_png(self.output / "image.png")
+        observed = service.inspect_current_file(
+            path, source_label="output", saved=None, library={"status": "none"}
+        )
+        self.assertEqual(observed["file"]["media_type"], "image")
+        self.assertEqual(observed["inspection"].format, "PNG")
+        unsupported = self.output / "image.txt"
+        unsupported.write_bytes(path.read_bytes())
+        read = service.read_inspection_file
+        reads = []
+
+        def traced(target):
+            reads.append(target)
+            return read(target)
+
+        with patch.object(service, "read_inspection_file", side_effect=traced):
+            with self.assertRaises(service.CurrentFileChanged):
+                service.inspect_current_file(
+                    unsupported,
+                    source_label="output",
+                    saved=None,
+                    library={"status": "none"},
+                )
+        self.assertEqual(reads, [], "Unsupported media must not reach the parser")
+
 
 class HistoryImportTests(unittest.TestCase):
     def test_api_import_does_not_require_a_running_host_server(self):