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.
This commit is contained in:
+11
-1
@@ -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():
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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")
|
||||
|
||||
Reference in New Issue
Block a user