fix: audit hardening — input validation, error handling, and robustness (#25)

* fix: audit hardening — input validation, error handling, and robustness

- immich_api: guard person["id"] with .get() + early return on missing field
- immich_api: include page number in pagination exception log
- immich_api: validate faces response is a list before indexing
- executor: wrap Image.open() in try/except for non-image HTTP responses
- executor: strip leading 'v' from Frigate version before parsing (v0.16.0 was misread)
- config: wrap FRIGATE_SCORE_CEILING float() parse in try/except with warning
- config: warn when both DATA_DIR and legacy CWD config files exist simultaneously
- scheduler: wrap PID file write in try/except so /tmp failures don't crash startup
- scheduler: clamp sleep to 60s max to bound recovery time after NTP clock jumps
- frigate_api: log unexpected non-list type in get_frigate_person_files at DEBUG

* fix: LIMIT env var crash and symlink guard on person output dir

- jobs: wrap int(LIMIT) parse in try/except — bad value (e.g. "30.5", "all")
  now logs a warning and falls back to the default instead of crashing
- executor: check for symlink before shutil.rmtree on person_dir — prevents
  following a symlink out of OUTPUT_DIR on a shared volume

* chore: bump version to 0.5.3

CHANGELOG.md

@@ -7,6 +7,34 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.5.3] - 2026-06-14
+
+### Fixed
+
+- **`LIMIT` env var crash**: non-integer values (e.g. `"30.5"`, `"all"`) now log a warning and fall back to the default instead of raising `ValueError` at startup.
+
+- **Symlink guard on person output dir**: `shutil.rmtree` is now skipped if `person_dir` resolves to a symlink, preventing traversal out of `OUTPUT_DIR` on a shared volume.
+
+- **`person["id"]` KeyError**: malformed Immich API responses missing the `id` field now log an error and skip that person instead of crashing the job.
+
+- **Face data response type validation**: `fetch_face_data` now validates that the `/api/faces` response is a list before indexing, guarding against null or non-list API responses.
+
+- **Pagination error log includes page number**: the exception log in `fetch_all_assets` now includes the page number that failed.
+
+- **`Image.open()` wrapped for non-image responses**: PIL parse errors on thumbnail fetches (e.g. reverse-proxy HTML error page returning 200) are now caught and logged instead of propagating.
+
+- **Frigate version `v`-prefix handling**: `v0.16.0`-style version strings are now correctly parsed; the leading `v` was previously misread, causing the too-old warning to never fire.
+
+- **`FRIGATE_SCORE_CEILING` parse guard**: a non-float value in `.env` now logs a warning and disables the ceiling instead of crashing at startup.
+
+- **Dual config file warning**: a log warning is emitted when both `DATA_DIR/.immich_config.json` and the legacy CWD config file exist simultaneously.
+
+- **PID file write guard**: `OSError` on `/tmp/winnow.pid` write is now caught and logged instead of crashing the scheduler.
+
+- **Scheduler sleep clamped to 60 s**: bounds recovery time after an NTP clock step.
+
+- **`get_frigate_person_files` non-list debug log**: consistent with `get_all_frigate_person_files`.
+
 ## [0.5.2] - 2026-06-14
 
 ### Fixed

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "winnow"
-version = "0.5.2"
+version = "0.5.3"
 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"

scheduler.py

@@ -31,7 +31,10 @@ def _run_scheduler() -> None:
         print("Error: CRON_SCHEDULE environment variable is required.", flush=True)
         sys.exit(1)
 
-    Path("/tmp/winnow.pid").write_text(str(os.getpid()))
+    try:
+        Path("/tmp/winnow.pid").write_text(str(os.getpid()))
+    except OSError as e:
+        print(f"Warning: could not write PID file: {e}", flush=True)
 
     now = time.time()
     cron = croniter(schedule, now)
@@ -53,7 +56,7 @@ def _run_scheduler() -> None:
                 print(f"winnow run failed: {e}", flush=True)
             next_run = cron.get_next(float)
             print(f"Next run: {time.strftime('%Y-%m-%d %H:%M:%S', time.localtime(next_run))}", flush=True)
-        time.sleep(max(1, next_run - time.time()))
+        time.sleep(min(60, max(1, next_run - time.time())))
 
 
 if __name__ == "__main__":

winnow/config.py

@@ -93,7 +93,14 @@ class _Config:
         self.MAX_AUTO_IMAGES = int(os.getenv("MAX_AUTO_IMAGES", "20"))
         self.QUALITY_REPLACEMENT = os.getenv("QUALITY_REPLACEMENT", "true").lower() in ("true", "1", "yes")
         _ceiling_env = os.getenv("FRIGATE_SCORE_CEILING", "").strip()
-        self.FRIGATE_SCORE_CEILING = float(_ceiling_env) if _ceiling_env else None
+        if _ceiling_env:
+            try:
+                self.FRIGATE_SCORE_CEILING = float(_ceiling_env)
+            except ValueError:
+                logging.warning("FRIGATE_SCORE_CEILING=%r is not a valid float — ignoring", _ceiling_env)
+                self.FRIGATE_SCORE_CEILING = None
+        else:
+            self.FRIGATE_SCORE_CEILING = None
         self.ENABLE_FRIGATE_SCORES = os.getenv("ENABLE_FRIGATE_SCORES", "true").lower() in ("true", "1", "yes")
         self.FACE_MARGIN = float(os.getenv("FACE_MARGIN", "0.15"))
         self.USE_FULL_RESOLUTION = os.getenv("USE_FULL_RESOLUTION", "true").lower() in ("true", "1", "yes")
@@ -116,6 +123,13 @@ class _Config:
         # Prefer DATA_DIR/.immich_config.json (volume-safe in Docker) and fall back
         # to the legacy CWD path so existing installations continue to work.
         _data_cfg = Path(self.DATA_DIR) / ".immich_config.json"
+        if _data_cfg.exists() and _LEGACY_CONFIG_FILE.exists():
+            logging.warning(
+                "Two config files found: %s and %s — using %s. Remove the legacy file to silence this.",
+                _data_cfg,
+                _LEGACY_CONFIG_FILE,
+                _data_cfg,
+            )
         config_file = _data_cfg if _data_cfg.exists() else _LEGACY_CONFIG_FILE
         if config_file.exists():
             try:

winnow/executor.py

@@ -100,6 +100,9 @@ def execute_jobs(jobs: list[dict]) -> None:
                 logger.error(str(e))
                 continue
             # Face crops are transient (uploaded then discarded); wipe before each run.
+            if os.path.islink(person_dir):
+                logger.error("person_dir %s is a symlink — refusing to remove", person_dir)
+                continue
             if os.path.isdir(person_dir):
                 shutil.rmtree(person_dir)
             os.makedirs(person_dir, exist_ok=True)
@@ -137,7 +140,14 @@ def execute_jobs(jobs: list[dict]) -> None:
                             headers=get_headers(),
                             timeout=30,
                         )
-                        img = Image.open(BytesIO(resp.content)) if resp.ok else None
+                        if resp.ok:
+                            try:
+                                img = Image.open(BytesIO(resp.content))
+                            except Exception:
+                                logger.warning("Invalid image data for asset %s", asset["id"])
+                                img = None
+                        else:
+                            img = None
 
                     if img is None:
                         progress.console.print(f"[red]Failed download {asset['id']}[/red]")
@@ -208,7 +218,7 @@ def upload_to_frigate(jobs: list[dict]) -> None:
     _frigate_version = get_frigate_version()
     if _frigate_version is not None:
         try:
-            parts = [int(x) for x in _frigate_version.split("-")[0].split(".") if x.isdigit()]
+            parts = [int(x) for x in _frigate_version.lstrip("v").split("-")[0].split(".") if x.isdigit()]
             if len(parts) >= 2 and (parts[0], parts[1]) < (0, 16):
                 rprint(
                     f"  [yellow]⚠  Frigate {_frigate_version} detected — "

winnow/frigate_api.py

@@ -86,7 +86,10 @@ def get_frigate_person_files(person_name: str) -> list[str] | None:
     if data is None:
         return None
     files = data.get(person_name)
-    return files if isinstance(files, list) else []
+    if files is not None and not isinstance(files, list):
+        logger.debug("Frigate API: unexpected type for %r — got %s, not list", person_name, type(files).__name__)
+        return []
+    return files if files is not None else []
 
 
 def recognize_face(file_path: str) -> tuple[str | None, float] | None:

winnow/immich_api.py

@@ -86,7 +86,10 @@ def merge_people(survivor_id: str, merge_ids: list[str]) -> bool:
 def fetch_all_assets(person: dict) -> list[dict]:
     """Fetch all assets for a person with pagination."""
     name = person.get("name", "Unknown")
-    person_id = person["id"]
+    person_id = person.get("id")
+    if not person_id:
+        logger.error("Person dict missing 'id' field for %s — skipping asset fetch", name)
+        return []
     url = f"{Config.IMMICH_URL}/api/search/metadata"
     page_size = 1000
 
@@ -120,7 +123,7 @@ def fetch_all_assets(person: dict) -> list[dict]:
                 break
 
         except (requests.RequestException, ValueError) as e:
-            logger.error("Exception fetching assets for %s: %s", name, e)
+            logger.error("Exception fetching assets for %s (page %s): %s", name, page, e)
             break
 
     return assets
@@ -152,18 +155,20 @@ def fetch_face_data(asset_id: str, person_id: str | None = None) -> FaceData | N
             return None
 
         faces = resp.json()
-        if not faces:
+        if not isinstance(faces, list) or not faces:
             return None
 
         # Match the target person if specified
         face = None
         if person_id:
             face = next(
-                (f for f in faces if (f.get("person") or {}).get("id") == person_id),
+                (f for f in faces if isinstance(f, dict) and (f.get("person") or {}).get("id") == person_id),
                 None,
             )
         if face is None:
-            face = faces[0]  # Fall back to first/largest face
+            face = faces[0] if isinstance(faces[0], dict) else None
+        if face is None:
+            return None
 
         bbox = (
             face.get("boundingBoxX1", 0),

winnow/jobs.py

@@ -68,11 +68,22 @@ def _resolve_strategy(strategy: str, has_embedding: bool) -> tuple[int | str, st
     custom_limit = os.environ.get("LIMIT", "").strip()
 
     if not has_embedding:
-        limit = int(custom_limit) if custom_limit else 30
+        if custom_limit:
+            try:
+                limit = int(custom_limit)
+            except ValueError:
+                logger.warning("LIMIT=%r is not a valid integer — using default 30", custom_limit)
+                limit = 30
+        else:
+            limit = 30
         return limit, "time"
 
     if custom_limit:
-        return int(custom_limit), "smart"
+        try:
+            return int(custom_limit), "smart"
+        except ValueError:
+            logger.warning("LIMIT=%r is not a valid integer — using adaptive strategy", custom_limit)
+            # fall through to strategy_map
 
     strategy_map = {
         "adaptive": ("auto", "smart"),