From c3234028951c6303dc06b3afeb448c50c064fe99 Mon Sep 17 00:00:00 2001 From: Thomas Faour Date: Mon, 27 Jul 2026 14:38:26 +0000 Subject: [PATCH] Fix scan-to-download auth and share every photo widget's current photo The share QR's URL carried no auth params at all, so it silently fell back through require_device's legacy-token resolution to whichever frame happened to still be flagged legacy -- working only by accident for a single frame, sharing the wrong frame's photos for any other, and going fully dead once that frame's legacy flag was cleared. Move the endpoint to manage.py, keyed on the frame's own manage_token (same pattern /m/ already uses) instead of device auth. Since the server now resolves assets itself instead of trusting a caller-supplied asset_id, it naturally generalizes to gather every photo widget's current photo into one Immich share link, not just one "primary" widget's. --- server/README.md | 20 ++-- server/app/immich_client.py | 10 +- server/app/routers/api_widgets.py | 4 +- server/app/routers/common.py | 10 +- server/app/routers/device.py | 39 +------- server/app/routers/manage.py | 46 ++++++++- server/tests/test_device_widget_dispatch.py | 37 +------ server/tests/test_manage_widget_dispatch.py | 103 ++++++++++++++++++++ 8 files changed, 178 insertions(+), 91 deletions(-) diff --git a/server/README.md b/server/README.md index 4cec5ca..51d0e02 100644 --- a/server/README.md +++ b/server/README.md @@ -121,9 +121,11 @@ Pages: `/` (routing hub), `/setup`, `/login`, `/claim`, `/settings`, this response may grow. - `GET /frame/photo-info` -- location/date overlay text for the manage menu (city + abbreviated US/CAN region or country, `MM/DD/YY`). -- `GET /frame/share/{asset_id}` -- creates a 30-minute public Immich - share link and 302s to it; scoped to the photo currently showing or - queued on *this* frame only. +- `GET /frame/share/{manage_token}` -- creates a 30-minute public Immich + share link covering every photo widget's currently-showing photo on + *this* frame and 302s to it. Authenticated by the frame's own + `manage_token` (see the manage QR below), not device credentials -- a + phone scanning the QR has no way to supply `?id=`/`?token=`. - `GET /frame/face-labels` -- up to 4 named faces with 800x480 positions, flattened (`name_0`/`x_0`/`y_0`, ...) for the device's flat-scalar parser. @@ -200,12 +202,12 @@ Pages: `/` (routing hub), `/setup`, `/login`, `/claim`, `/settings`, in the web UI (or using "Show next") only rearranges what's already in that lookahead; it doesn't add or remove photos from the album. - Auth in one breath: browsers use sessions (+CSRF), devices use - per-frame tokens (`?id=` + `?token=`), the manage QR uses its own - limited token, and `MANAGEMENT_TOKEN` survives only as the migration - credential for pre-multi-frame firmware. `/frame/share` stays scoped - to photos this frame is actually showing or has queued, not any - Immich asset ID someone might guess -- a second layer a leaked device - token alone wouldn't bypass. + per-frame tokens (`?id=` + `?token=`), the manage QR and the + scan-to-download QR both use the frame's own `manage_token` (device + tokens don't work for either -- neither is ever called by firmware, + both are opened by a phone that has no way to supply `?id=`/`?token=`), + and `MANAGEMENT_TOKEN` survives only as the migration credential for + pre-multi-frame firmware. - Calendar frame mode (`app/calendar_feed.py`) expands recurring events (RRULE/EXDATE/DST) via [`recurring-ical-events`](https://pypi.org/project/recurring-ical-events/), which is LGPL-3.0-or-later -- the only non-permissively-licensed diff --git a/server/app/immich_client.py b/server/app/immich_client.py index dc35e24..270fbff 100644 --- a/server/app/immich_client.py +++ b/server/app/immich_client.py @@ -80,10 +80,10 @@ class ImmichClient: resp.raise_for_status() return resp.json() - def create_share_link(self, asset_id: str, expires_in_s: int) -> str: - """Creates a public, view-only Immich share link for a single - asset, expiring expires_in_s seconds from now, and returns its - public URL. Used by the manage-button overlay's share QR -- + def create_share_link(self, asset_ids: list[str], expires_in_s: int) -> str: + """Creates a public, view-only Immich share link covering one or + more assets, expiring expires_in_s seconds from now, and returns + its public URL. Used by the manage-button overlay's share QR -- created lazily (only when someone actually scans it), not when the button's pressed, so the expiry clock starts when it's actually used.""" @@ -93,7 +93,7 @@ class ImmichClient: headers=self._headers, json={ "type": "INDIVIDUAL", - "assetIds": [asset_id], + "assetIds": list(asset_ids), "expiresAt": expires_at, "allowUpload": False, "allowDownload": True, diff --git a/server/app/routers/api_widgets.py b/server/app/routers/api_widgets.py index f3c860b..0e134e2 100644 --- a/server/app/routers/api_widgets.py +++ b/server/app/routers/api_widgets.py @@ -493,8 +493,8 @@ def api_widget_thumbnail( """Scoped to what this widget is actually showing/queuing -- a user merely linked to view this frame shouldn't be able to pull thumbnails for arbitrary asset ids in the owner's Immich library, only this - widget's own curated album. Same rule device.frame_share and - manage.manage_thumbnail already enforce.""" + widget's own curated album. Same rule manage.manage_thumbnail + already enforces.""" frame, widget = frame_widget _require_widget_type(widget, "photos") pcfg = db.get(PhotoWidgetConfig, widget.id) diff --git a/server/app/routers/common.py b/server/app/routers/common.py index 54ad68a..32a6e77 100644 --- a/server/app/routers/common.py +++ b/server/app/routers/common.py @@ -449,7 +449,15 @@ def build_manage_content(db: Session, frame: Frame, request) -> dict: exif = asset.get("exifInfo") or {} content["location_lines"] = _format_location(exif) content["taken_at"] = _format_taken_at(exif) - content["share_url"] = f"{base}/frame/share/{primary_cfg.current_asset_id}" + + # Unlike location/date-taken (a single fixed corner, so tied to the + # one "primary" widget above), the share link covers every photo + # widget's current photo (see manage.manage_share) -- so it only + # needs *some* photo widget to have a current photo, not specifically + # the primary one, and doesn't depend on the EXIF fetch above + # succeeding. + if any(db.get(PhotoWidgetConfig, w.id).current_asset_id for w in photo_widgets): + content["share_url"] = f"{base}/frame/share/{frame.manage_token}" panel_w, panel_h = logical_render_size(frame.orientation) face_labels: list[dict] = [] diff --git a/server/app/routers/device.py b/server/app/routers/device.py index 66d3246..dc3ebfe 100644 --- a/server/app/routers/device.py +++ b/server/app/routers/device.py @@ -15,9 +15,8 @@ from __future__ import annotations import logging import time -import httpx from fastapi import APIRouter, Depends, HTTPException, Request -from fastapi.responses import FileResponse, RedirectResponse, Response +from fastapi.responses import FileResponse, Response from pydantic import BaseModel from sqlalchemy import delete, func, select from sqlalchemy.orm import Session @@ -27,7 +26,7 @@ from ..auth import get_server_settings, require_device from ..db import frame_locked, get_db from ..firmware import firmware_path from ..image_pipeline import logical_render_size, render_panel, render_placeholder -from ..models import BatteryLog, Frame, FrameButtonAction, PhotoWidgetConfig, Widget +from ..models import BatteryLog, Frame, FrameButtonAction, Widget from ..widgets import WIDGET_TYPES from .common import ( BATTERY_HISTORY_MAX, @@ -35,9 +34,6 @@ from .common import ( RECHARGE_JUMP_PCT, RECHARGE_LOOKBACK, build_manage_content, - immich_client_for, - immich_creds, - photo_widgets_for_frame, ) logger = logging.getLogger(__name__) @@ -356,34 +352,3 @@ def frame_firmware(frame: Frame = Depends(require_device)): if not path.exists(): raise HTTPException(404, "No firmware uploaded") return FileResponse(path, media_type="application/octet-stream") - - -@router.get("/frame/share/{asset_id}") -def frame_share(asset_id: str, frame: Frame = Depends(require_device), db: Session = Depends(get_db)): - """Creates a 30-minute public Immich share link for asset_id and - redirects to it -- what the manage overlay's bottom-left QR code - points to. The link is created lazily, when this actually gets hit - (i.e. when someone scans it), not when the manage button was - pressed, so the 30-minute window starts when it's actually used. - Also scoped to the photo currently showing or queued on one of THIS - frame's own photo widgets -- not any arbitrary Immich asset id -- as - a second layer even a leaked token wouldn't bypass.""" - url, key = immich_creds(frame) - if not url or not key: - raise HTTPException(400, "Immich URL/API key not configured yet") - - photo_widgets = photo_widgets_for_frame(db, frame) - showing_or_queued = any( - asset_id == cfg.current_asset_id or asset_id in cfg.queue - for cfg in (db.get(PhotoWidgetConfig, w.id) for w in photo_widgets) - ) - if not showing_or_queued: - raise HTTPException(404, "That photo isn't currently showing or queued on this frame") - - client = immich_client_for(frame) - try: - share_url = client.create_share_link(asset_id, expires_in_s=1800) - except httpx.HTTPError as e: - raise HTTPException(502, f"Could not create share link: {e}") from e - - return RedirectResponse(share_url) diff --git a/server/app/routers/manage.py b/server/app/routers/manage.py index 820fdf2..8aebb9e 100644 --- a/server/app/routers/manage.py +++ b/server/app/routers/manage.py @@ -11,7 +11,7 @@ import logging import httpx from fastapi import APIRouter, Depends, HTTPException, Request -from fastapi.responses import HTMLResponse, Response +from fastapi.responses import HTMLResponse, RedirectResponse, Response from fastapi.templating import Jinja2Templates from pydantic import BaseModel from sqlalchemy import select @@ -19,8 +19,14 @@ from sqlalchemy.orm import Session from .. import photo_queue, quiet_hours from ..db import get_db, widget_locked -from ..models import Frame -from .common import immich_client_for, list_assets, photo_widget_config_or_404 +from ..models import Frame, PhotoWidgetConfig +from .common import ( + immich_client_for, + immich_creds, + list_assets, + photo_widget_config_or_404, + photo_widgets_for_frame, +) logger = logging.getLogger(__name__) @@ -120,3 +126,37 @@ def manage_thumbnail(asset_id: str, frame: Frame = Depends(require_manage), db: except httpx.HTTPError as e: raise HTTPException(502, f"Could not download thumbnail from Immich: {e}") from e return Response(content=content, media_type=content_type) + + +@router.get("/frame/share/{manage_token}") +def manage_share(frame: Frame = Depends(require_manage), db: Session = Depends(get_db)): + """Creates a 30-minute public Immich share link covering every photo + widget's currently-displayed asset on this frame, and redirects to it + -- what the manage overlay's bottom-left QR code points to. Lazily + created (only when someone actually scans it, not when the manage + button was pressed), so the 30-minute window starts at actual use. + Keyed on this frame's own manage_token, like the rest of this router, + rather than device credentials -- a phone scanning a QR code has no + way to supply the device's ?id=/?token=, which is why this used to + silently fall back to whichever frame happened to still carry the + legacy migration token instead of the frame that was actually + scanned.""" + photo_widgets = photo_widgets_for_frame(db, frame) + asset_ids: list[str] = [] + for widget in photo_widgets: + cfg = db.get(PhotoWidgetConfig, widget.id) + if cfg.current_asset_id and cfg.current_asset_id not in asset_ids: + asset_ids.append(cfg.current_asset_id) + if not asset_ids: + raise HTTPException(404, "No photos currently showing on this frame") + + url, key = immich_creds(frame) + if not url or not key: + raise HTTPException(400, "Immich URL/API key not configured yet") + + client = immich_client_for(frame) + try: + share_url = client.create_share_link(asset_ids, expires_in_s=1800) + except httpx.HTTPError as e: + raise HTTPException(502, f"Could not create share link: {e}") from e + return RedirectResponse(share_url) diff --git a/server/tests/test_device_widget_dispatch.py b/server/tests/test_device_widget_dispatch.py index 5998598..dfd685f 100644 --- a/server/tests/test_device_widget_dispatch.py +++ b/server/tests/test_device_widget_dispatch.py @@ -1,7 +1,7 @@ """End-to-end HTTP tests for the widget-system cutover in -routers/device.py -- /frame/image, /frame/advance, /frame/back, and -/frame/share against real widget rows (via the migration-backfilled -frame #1, or a purpose-built second frame), a real TestClient, real +routers/device.py -- /frame/image, /frame/advance, and /frame/back +against real widget rows (via the migration-backfilled frame #1, or a +purpose-built second frame), a real TestClient, real render_panel/compose_into. Only Immich itself is mocked (monkeypatched at the app.widgets.photos module boundary, same pattern as test_widgets_photos.py) -- everything else in the pipeline is real. @@ -23,7 +23,6 @@ from app.models import ( Frame, FrameButtonAction, PhotoWidgetConfig, - User, Widget, ) @@ -145,33 +144,3 @@ def test_two_widget_frame_composites_both_and_next_targets_calendar(client, db_s photo_cfg = db_session.get(PhotoWidgetConfig, photo_widget.id) assert cal_cfg.browse_offset == 1 assert photo_cfg.current_asset_id == "" # untouched -- NEXT was never bound to it - - -def test_frame_share_checks_widget_scoped_state(client, db_session, monkeypatch): - client.post("/setup", data={"username": "alice", "password": "hunter22"}) - frame = db_session.get(Frame, 1) - frame.owner_user_id = db_session.query(User).filter_by(username="alice").one().id - widget = db_session.query(Widget).filter_by(frame_id=frame.id, widget_type="photos").one() - cfg = db_session.get(PhotoWidgetConfig, widget.id) - cfg.album_id = "album-1" - cfg.current_asset_id = "asset-1" - cfg.queue = ["asset-2"] - frame.immich_url = "http://immich.example.com" - frame.immich_api_key = "key" - db_session.commit() - - # Not showing/queued -- rejected before ever touching Immich - resp = client.get("/frame/share/asset-not-on-this-frame") - assert resp.status_code == 404 - - # Currently showing -- allowed through to the (mocked) Immich call - monkeypatch.setattr( - "app.routers.device.immich_client_for", - lambda frame: type("C", (), {"create_share_link": lambda self, asset_id, expires_in_s: "https://immich.example.com/share/abc"})(), - ) - resp = client.get("/frame/share/asset-1", follow_redirects=False) - assert resp.status_code in (302, 303, 307) - - # Queued (not current) -- also allowed - resp = client.get("/frame/share/asset-2", follow_redirects=False) - assert resp.status_code in (302, 303, 307) diff --git a/server/tests/test_manage_widget_dispatch.py b/server/tests/test_manage_widget_dispatch.py index 77a1b97..d70ae7b 100644 --- a/server/tests/test_manage_widget_dispatch.py +++ b/server/tests/test_manage_widget_dispatch.py @@ -119,3 +119,106 @@ def test_manage_thumbnail_scoped_to_showing_or_queued(client, db_session, monkey def test_unknown_manage_token_404s(client, db_session): resp = client.get("/api/m/not-a-real-token/queue") assert resp.status_code == 404 + + +def test_manage_share_requires_current_photo(client, db_session): + client.post("/setup", data={"username": "alice", "password": "hunter22"}) + frame, _ = _configure_photo_widget(db_session) + resp = client.get(f"/frame/share/{frame.manage_token}", follow_redirects=False) + assert resp.status_code == 404 + + +def test_manage_share_creates_link_and_redirects(client, db_session, monkeypatch): + client.post("/setup", data={"username": "alice", "password": "hunter22"}) + frame, widget = _configure_photo_widget(db_session) + cfg = db_session.get(PhotoWidgetConfig, widget.id) + cfg.current_asset_id = "asset-1" + db_session.commit() + + calls = [] + monkeypatch.setattr( + "app.routers.manage.immich_client_for", + lambda frame: type("C", (), { + "create_share_link": lambda self, asset_ids, expires_in_s: ( + calls.append(asset_ids) or "https://immich.example.com/share/abc" + ), + })(), + ) + resp = client.get(f"/frame/share/{frame.manage_token}", follow_redirects=False) + assert resp.status_code in (302, 303, 307) + assert calls == [["asset-1"]] + + +def test_manage_share_covers_every_photo_widget(client, db_session, monkeypatch): + from app.models import Widget + + client.post("/setup", data={"username": "alice", "password": "hunter22"}) + frame, widget = _configure_photo_widget(db_session) + cfg = db_session.get(PhotoWidgetConfig, widget.id) + cfg.current_asset_id = "asset-1" + + second_widget = Widget(frame_id=frame.id, widget_type="photos", x=4, y=0, w=4, h=5, + sort_order=1, created_at=0) + db_session.add(second_widget) + db_session.flush() + db_session.add(PhotoWidgetConfig(widget_id=second_widget.id, album_id="album-2", current_asset_id="asset-9")) + db_session.commit() + + calls = [] + monkeypatch.setattr( + "app.routers.manage.immich_client_for", + lambda frame: type("C", (), { + "create_share_link": lambda self, asset_ids, expires_in_s: ( + calls.append(asset_ids) or "https://immich.example.com/share/abc" + ), + })(), + ) + resp = client.get(f"/frame/share/{frame.manage_token}", follow_redirects=False) + assert resp.status_code in (302, 303, 307) + assert calls == [["asset-1", "asset-9"]] + + +def test_manage_share_scoped_to_its_own_frame(client, db_session, monkeypatch): + """The regression test for the original bug: each frame's own share + QR must only ever surface that frame's own current photo, never + another frame's -- previously it silently resolved via whichever + frame happened to still carry the legacy migration token, regardless + of which frame's QR was actually scanned.""" + from app.models import Widget + + client.post("/setup", data={"username": "alice", "password": "hunter22"}) + frame_a, widget_a = _configure_photo_widget(db_session) + cfg_a = db_session.get(PhotoWidgetConfig, widget_a.id) + cfg_a.current_asset_id = "asset-a" + + frame_b = Frame( + name="Frame B", device_id="112233445566", device_token="devtok-b", + manage_token="mtok-b", orientation="landscape", created_at=0, + immich_url="http://immich.example.com", immich_api_key="key", + ) + db_session.add(frame_b) + db_session.flush() + widget_b = Widget(frame_id=frame_b.id, widget_type="photos", x=0, y=0, w=4, h=5, + sort_order=0, created_at=0) + db_session.add(widget_b) + db_session.flush() + db_session.add(PhotoWidgetConfig(widget_id=widget_b.id, album_id="album-b", current_asset_id="asset-b")) + db_session.commit() + + calls = [] + monkeypatch.setattr( + "app.routers.manage.immich_client_for", + lambda frame: type("C", (), { + "create_share_link": lambda self, asset_ids, expires_in_s: ( + calls.append(asset_ids) or "https://immich.example.com/share/abc" + ), + })(), + ) + client.get(f"/frame/share/{frame_a.manage_token}", follow_redirects=False) + client.get(f"/frame/share/{frame_b.manage_token}", follow_redirects=False) + assert calls == [["asset-a"], ["asset-b"]] + + +def test_unknown_manage_share_token_404s(client, db_session): + resp = client.get("/frame/share/not-a-real-token") + assert resp.status_code == 404