From 3c1fd69bfa4d5582344cdaaeeff9b731fecee3cb Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Sat, 31 Oct 2026 22:39:20 -0700 Subject: [PATCH] fix(thumbnails): never plan or delete through a linked key folder (#320) Review finding P1 on #591: in folder mode a source key folder that is a link or junction to a folder outside was still handed to the planner, so Convert outdated could remove an older-format file outside the thumbnail folder. ensure_store now refuses a linked store before any planning, and each removal of an old preview passes may_write at the moment it runs. --- py/api/preview_store.py | 12 +++++++++++- py/api/thumbnail_convert.py | 8 +++++++- tests/test_preview_store_folder.py | 27 +++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/py/api/preview_store.py b/py/api/preview_store.py index e4c1a932..f2ebf49c 100644 --- a/py/api/preview_store.py +++ b/py/api/preview_store.py @@ -414,7 +414,9 @@ def _key_folder(folder, key): def ensure_store(root, location=None, *, create=False): """``store_dir`` for a writer: in folder mode a source without a key gets one (recorded in the marker); with *create* its key folder is made (never - the thumbnail folder itself). None while the folder is unavailable.""" + the thumbnail folder itself). None while the folder is unavailable, and + for a key folder that is a link, junction or reparse point: nothing is + planned, written or removed through it.""" location = location or current() store = store_dir(root, location) if location.mode == SOURCE_MODE or (store is None and not location.available): @@ -423,6 +425,14 @@ def ensure_store(root, location=None, *, create=False): store = _assign_key(root, location) if store is None: return None + try: + with store_io(): + linked = preview_location.is_link_like(store) + except OSError: + return None + if linked: + logger.warning(f"Skipping the thumbnail folder {store}: it is a link") + return None if create: try: with store_io(): diff --git a/py/api/thumbnail_convert.py b/py/api/thumbnail_convert.py index a12d8be1..72434151 100644 --- a/py/api/thumbnail_convert.py +++ b/py/api/thumbnail_convert.py @@ -44,7 +44,7 @@ import os import time from pathlib import Path -from . import thumbnail_settings +from . import preview_store, thumbnail_settings from .thumbnail_encode import ThumbnailVerifyError, verify_thumbnail, writer_settings from .admin import _plural, _scan_location, _scan_roots, _scan_status_line from .images import _thumbnail_is_fresh, _worker_threads, _thumbnail_batch_size @@ -393,6 +393,12 @@ class ThumbnailConvertMixin: removed, with no bytes (another run or a sibling removed it).""" removed, kept = 0, [] for path, size in old: + if not preview_store.may_write(path): + # Folder mode: never a file outside a key folder, or one + # reached through a link (checked at the moment of the delete). + self.logger.warning(f"Not removing {path}: outside the thumbnails") + kept.append(path) + continue try: os.remove(path) except FileNotFoundError: diff --git a/tests/test_preview_store_folder.py b/tests/test_preview_store_folder.py index 2f2333b3..413da72b 100644 --- a/tests/test_preview_store_folder.py +++ b/tests/test_preview_store_folder.py @@ -469,6 +469,33 @@ class TestHostileMarker(FolderModeCase): self.assertEqual(victim.read_bytes(), b"keep me") self.assertEqual(sorted(p.name for p in outside.iterdir()), ["v_thumb.webp"]) + def link_key_folder_outside(self): + """The source's key folder replaced by a link to a folder outside.""" + outside = self.comfy_dir / "outside" + outside.mkdir() + store = self.key_folder() + store.rmdir() + try: + store.symlink_to(outside, target_is_directory=True) + except OSError: + self.skipTest("symlinks not permitted") + return outside + + @unittest.skipUnless(hasattr(os, "symlink"), "needs symlinks") + async def test_convert_never_plans_or_deletes_through_a_linked_key_folder(self): + outside = self.link_key_folder_outside() + old = outside / "c_thumb.png" + old.write_bytes(b"keep me") + make_png(self.output_dir / "c.png", size=(640, 480)) + # A linked store is refused before any planning (bot review, P1). + self.assertIsNone(preview_store.ensure_store(self.output_dir)) + plan = self.api._plan_gallery_thumbnails([self.output_dir], mode="convert") + self.assertEqual(plan["targets"], []) + # And every delete passes the ownership check at the moment it runs. + removed, kept = self.api._remove_old_previews([(old, 7)]) + self.assertEqual((removed, kept), (0, [old])) + self.assertEqual(old.read_bytes(), b"keep me") + async def test_a_write_outside_the_folder_is_refused(self): with self.assertRaises(OSError): preview_store.make_parent(self.comfy_dir / "elsewhere" / "a_thumb.webp")