diff --git a/CHANGELOG.md b/CHANGELOG.md index be8490f..1a51214 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.8] - 2026-06-15 + +### Fixed + +- **`_safe_person_dir` docstring corrected**: the previous comment stated "checking after realpath would be too late because realpath follows the link first," which implied `islink` was the primary security guard. The load-bearing path-traversal check is `realpath + startswith`; it rejects both `../../` traversal and symlinks. The `islink` check is a supplementary early-exit that provides a cleaner error message for the symlink sub-case only. + +- **`total_raw` not inflated by all-garbage pages**: `fetch_all_assets` previously added `page_count` to `total_raw` before checking whether any valid dict items existed. A page returning only non-dict items would inflate `total_raw` and produce a misleading "N total, 0 recent" display. `total_raw` now accumulates only after `valid_assets` is confirmed non-empty, so all-garbage pages break without contributing. Mixed pages (some valid, some non-dict) still count `page_count` so transient schema glitches on a partial page don't cause `MIN_FACE_COUNT` to incorrectly skip a real person. + +- **Pagination interruption warning**: when a `RequestException` breaks pagination mid-way (page > 1), a `WARNING` is now logged noting that `total_raw` is a lower bound. Previously the exception was logged at `ERROR` with no indication that the `MIN_FACE_COUNT` comparison was using a partial count. + +- **Config TOCTOU residue**: `config.py` line 135 re-stat'd `_data_cfg` when it was the selected config file, creating a second TOCTOU window after the fix in v0.5.7. The check is now `if _data_cfg_exists or config_file.exists():` — the primary path is never stat'd again, and the legacy path is stat'd at most once. + ## [0.5.7] - 2026-06-15 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index 48446ab..10d0883 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "winnow" -version = "0.5.7" +version = "0.5.8" description = "Selects diverse, high-quality photos from Immich as training data for Frigate face recognition." license = "AGPL-3.0-or-later" requires-python = ">=3.13" diff --git a/winnow/config.py b/winnow/config.py index bc8abb0..dbc1339 100644 --- a/winnow/config.py +++ b/winnow/config.py @@ -132,7 +132,9 @@ class _Config: _data_cfg, ) config_file = _data_cfg if _data_cfg_exists else _LEGACY_CONFIG_FILE - if config_file.exists(): + # _data_cfg_exists already confirmed the primary path — avoid re-stat. + # The short-circuit means the legacy path is stat'd at most once here. + if _data_cfg_exists or config_file.exists(): try: data = json.loads(config_file.read_text()) if self.IMMICH_URL is None: diff --git a/winnow/executor.py b/winnow/executor.py index 965d4ec..155a2f3 100644 --- a/winnow/executor.py +++ b/winnow/executor.py @@ -42,9 +42,11 @@ def _safe_person_dir(output_dir: str, person_name: str) -> str: """Return the output subdirectory for a person, raising ValueError on path traversal. os.path.join silently discards output_dir when person_name is absolute, - and '../..' sequences resolve outside the tree. Both are rejected here. - Symlinks on the raw (unresolved) path are also rejected — checking after - realpath would be too late because realpath follows the link first. + and '../..' sequences resolve outside the tree. Both are rejected by the + realpath+startswith guard, which is the load-bearing security check. + The islink check below provides an earlier, cleaner error message for the + symlink sub-case; it is redundant with (not a replacement for) the + realpath+startswith traversal check. """ raw = os.path.join(output_dir, person_name) if os.path.islink(raw): diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 9e38358..9309da2 100644 --- a/winnow/immich_api.py +++ b/winnow/immich_api.py @@ -86,9 +86,11 @@ def merge_people(survivor_id: str, merge_ids: list[str]) -> bool: def fetch_all_assets(person: dict) -> tuple[list[dict], int]: """Fetch all assets for a person with pagination. - Returns (assets, raw_total) where assets is the list of valid dict items - and raw_total is the total item count seen before non-dict filtering. - raw_total may exceed len(assets) if Immich returned non-dict items. + Returns (assets, total_raw) where assets is the list of valid dict items + and total_raw is the raw item count across pages that had at least one valid + dict. All-garbage pages (every item non-dict) stop pagination and are not + counted. If pagination is interrupted by a network error, total_raw is a + lower bound (a warning is logged). """ name = person.get("name", "Unknown") person_id = person.get("id") @@ -101,7 +103,7 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: logger.debug("Fetching assets for %s...", name) assets: list[dict] = [] - total_raw = 0 # items seen across all pages before non-dict filtering + total_raw = 0 # raw item count across pages that yielded at least one valid dict for page in range(1, MAX_PAGES + 1): try: resp = requests.post( @@ -122,7 +124,6 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: page_assets = page_assets.get("items", []) page_count = len(page_assets) # raw count for termination check before filtering - total_raw += page_count # Single pass: partition valid assets from unexpected non-dict items valid_assets, skipped_count = [], 0 @@ -142,6 +143,11 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: ) break + # Count page_count (not just valid items) so that non-dict items from a + # transient schema issue on a mixed page don't cause MIN_FACE_COUNT to + # skip a real person. Pages where every item is a non-dict are excluded — + # they indicate a structural problem and break above without contributing. + total_raw += page_count assets.extend(valid_assets) logger.debug("Fetched page %s, total: %s", page, len(assets)) @@ -150,6 +156,11 @@ def fetch_all_assets(person: dict) -> tuple[list[dict], int]: except (requests.RequestException, ValueError) as e: logger.error("Exception fetching assets for %s (page %s): %s", name, page, e) + if page > 1: + logger.warning( + "%s: pagination interrupted at page %s — total_raw=%s may undercount actual assets", + name, page, total_raw, + ) break return assets, total_raw diff --git a/winnow/jobs.py b/winnow/jobs.py index 594ba13..4fbf2e0 100644 --- a/winnow/jobs.py +++ b/winnow/jobs.py @@ -268,8 +268,8 @@ def auto_configure(people: list[dict]) -> list[dict]: rprint(f" {name}: {total_raw} total, {len(recent_assets)} recent") # MIN_FACE_COUNT guard: skip people with too few Immich assets. - # Uses total_raw (pre-filter count) so non-dict items from a transient - # Immich schema issue don't cause a person to be skipped incorrectly. + # Uses total_raw so that non-dict items from a transient Immich schema + # issue on a mixed page don't shrink the count below the threshold. # Done here (after fetch) rather than upfront because Immich v2.7.5+ # dropped assetCount from the /api/people response. if min_face_count > 0 and total_raw < min_face_count: