From 2e08504682e05f213fb53b9a465605e7bda3ae02 Mon Sep 17 00:00:00 2001 From: Holden Salomon Date: Sun, 14 Jun 2026 19:39:35 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20audit=20hardening=20=E2=80=94=20input=20?= =?UTF-8?q?validation,=20error=20handling,=20and=20robustness=20(#25)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 28 ++++++++++++++++++++++++++++ pyproject.toml | 2 +- scheduler.py | 7 +++++-- winnow/config.py | 16 +++++++++++++++- winnow/executor.py | 14 ++++++++++++-- winnow/frigate_api.py | 5 ++++- winnow/immich_api.py | 15 ++++++++++----- winnow/jobs.py | 15 +++++++++++++-- 8 files changed, 88 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 450fc8b..db12bf4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/pyproject.toml b/pyproject.toml index 0b0e30f..d5422e0 100644 --- a/pyproject.toml +++ b/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" diff --git a/scheduler.py b/scheduler.py index ead22c6..99368d5 100644 --- a/scheduler.py +++ b/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__": diff --git a/winnow/config.py b/winnow/config.py index 4281c71..c65f5dd 100644 --- a/winnow/config.py +++ b/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: diff --git a/winnow/executor.py b/winnow/executor.py index 55eefb1..23c8186 100644 --- a/winnow/executor.py +++ b/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 — " diff --git a/winnow/frigate_api.py b/winnow/frigate_api.py index eb12ccf..124148b 100644 --- a/winnow/frigate_api.py +++ b/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: diff --git a/winnow/immich_api.py b/winnow/immich_api.py index 96ce3ff..8ac9bf2 100644 --- a/winnow/immich_api.py +++ b/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), diff --git a/winnow/jobs.py b/winnow/jobs.py index fa2016d..e9810d3 100644 --- a/winnow/jobs.py +++ b/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"),