From f24c3b9c8e7fcae6324ca880d02abf2a5f42837b Mon Sep 17 00:00:00 2001 From: Thomas Faour Date: Wed, 22 Jul 2026 16:21:37 -0400 Subject: [PATCH] Gate firmware auto-check behind control+CSRF; skip faces with a null bounding box /api/frames/{id}/firmware/check could silently stage new firmware as a side effect (the auto-apply path, when firmware_auto_update is on and a newer release exists) but was gated by require_frame_view instead of require_frame_control like its sibling firmware routes, and being a GET, was exempt from the app's CSRF check (which only applies to non-GET/HEAD/OPTIONS). A linked viewer without control -- or a cross-site page riding a control-holding victim's session via a plain GET -- could trigger an unreviewed firmware install. Now POST + require_frame_control, matching /firmware/apply-latest; the frontend's two callers (passive poll on page load, "Check now" button) both already handle a 409 from a non-controller gracefully via the existing apiError()/control-banner pattern, so this doesn't change UX for a frame's actual controller. Separately: Immich has been observed to return a face detection entry with a null bounding-box field (a still-pending or otherwise incomplete detection). Both places that do arithmetic on those fields -- image_pipeline._face_aware_crop_box (crop_faces display mode) and face_labels.compute_face_labels (manage-menu name labels) -- crashed with an unhandled TypeError on such an entry, taking down that frame's whole photo instead of the intended graceful fallback. Both now skip any face missing a bounding-box field via a shared _has_bounding_box() check; a face list with zero valid entries already degrades cleanly to the plain center crop (the existing inf/-inf sentinel math already handled "no faces" correctly, it just couldn't tell "none passed Immich" apart from "one broken entry" before). --- server/app/face_labels.py | 4 +++- server/app/image_pipeline.py | 12 ++++++++++++ server/app/routers/api_frames.py | 13 ++++++++++--- server/app/static/frame_config.js | 2 +- 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/server/app/face_labels.py b/server/app/face_labels.py index a049cb4..f5e2a44 100644 --- a/server/app/face_labels.py +++ b/server/app/face_labels.py @@ -15,7 +15,7 @@ import io from PIL import Image, ImageOps -from .image_pipeline import _placement_transform, logical_render_size, logical_to_native +from .image_pipeline import _has_bounding_box, _placement_transform, logical_render_size, logical_to_native # Small caps, not arbitrary: each label is its own malloc'd overlay # buffer on the device (see firmware/main/manage_qr_overlay.c), and the @@ -55,6 +55,8 @@ def compute_face_labels(preview_bytes: bytes, faces: list[dict], display_mode: s labels = [] for face in named[:MAX_LABELED_FACES]: + if not _has_bounding_box(face): + continue face_w = face.get("imageWidth") or fitted.width face_h = face.get("imageHeight") or fitted.height img_scale_x = fitted.width / face_w diff --git a/server/app/image_pipeline.py b/server/app/image_pipeline.py index 794568b..ae7a1f6 100644 --- a/server/app/image_pipeline.py +++ b/server/app/image_pipeline.py @@ -118,6 +118,16 @@ def _plain_center_crop_box( return left, top, crop_w, crop_h +def _has_bounding_box(face: dict) -> bool: + """Immich has occasionally been observed to return a face entry with + a still-pending or otherwise incomplete bounding box (a null field) + -- treat it as undetected rather than crash on arithmetic with None.""" + return all( + face.get(k) is not None + for k in ("boundingBoxX1", "boundingBoxX2", "boundingBoxY1", "boundingBoxY2") + ) + + def _face_aware_crop_box( img_width: int, img_height: int, target_width: int, target_height: int, faces: list[dict] ) -> tuple[int, int, int, int]: @@ -138,6 +148,8 @@ def _face_aware_crop_box( min_x = min_y = float("inf") max_x = max_y = float("-inf") for face in faces: + if not _has_bounding_box(face): + continue face_w = face.get("imageWidth") or img_width face_h = face.get("imageHeight") or img_height scale_x = img_width / face_w diff --git a/server/app/routers/api_frames.py b/server/app/routers/api_frames.py index fc60f27..4697ec1 100644 --- a/server/app/routers/api_frames.py +++ b/server/app/routers/api_frames.py @@ -457,15 +457,22 @@ def _apply_gitea_update(db: Session, frame: Frame) -> str: return version -@router.get("/api/frames/{frame_id}/firmware/check") +@router.post("/api/frames/{frame_id}/firmware/check") def api_firmware_check( - force: bool = False, frame: Frame = Depends(require_frame_view), db: Session = Depends(get_db) + force: bool = False, frame: Frame = Depends(require_frame_control), db: Session = Depends(get_db) ): """Throttled check of the configured Gitea repo's latest release (gitea_releases.UPDATE_CHECK_INTERVAL_S). If firmware_auto_update is on and a newer version is found, applies it immediately; otherwise just reports it so the UI can offer the "Update frame" button. - force=true (the "Check now" button) bypasses the throttle.""" + force=true (the "Check now" button) bypasses the throttle. + + require_frame_control (not view), and POST (not GET): this can + silently stage new firmware as a side effect (the auto-apply path + below) exactly like /firmware/apply-latest, so it needs the same + guard that route has -- a linked viewer without control shouldn't be + able to trigger that, and as a GET it would've been exempt from the + CSRF check require_user_api only applies to non-GET/HEAD/OPTIONS.""" if not frame.firmware_update_repo_url: return {"enabled": False} diff --git a/server/app/static/frame_config.js b/server/app/static/frame_config.js index f29d128..6b5d97c 100644 --- a/server/app/static/frame_config.js +++ b/server/app/static/frame_config.js @@ -259,7 +259,7 @@ async function loadFirmwareCheck(force) { const btn = document.getElementById('firmware-update-btn'); const boardEl = document.getElementById('firmware-board'); try { - const resp = await fetch(`${window.FRAME_API}/firmware/check` + (force ? '?force=true' : '')); + const resp = await fetch(`${window.FRAME_API}/firmware/check` + (force ? '?force=true' : ''), { method: 'POST' }); if (!resp.ok) { if (force) { showStatus(false, await apiError(resp));