Fix config read-modify-write race and two firmware buffer edge cases

Found by a thorough code review:

- server/app/config.py's load()/save() each locked only their own file
  I/O, not the full read-modify-write cycle each route does around
  them. Since uvicorn dispatches sync routes to a thread pool, two
  concurrent requests (e.g. the device's own poll landing alongside a
  web UI edit) could each load() the same on-disk state and the
  second's save() silently clobber the first's changes. Added
  config.locked() (backed by an RLock, since load()/save() also take
  the lock internally) and wrapped every mutating route's
  load/mutate/save span in it -- kept outside the lock wherever a route
  also does slow Immich network I/O, re-loading fresh state right
  before the actual mutation instead. Verified with a new concurrency
  stress test (many concurrent /api/queue/promote and /api/config
  calls) alongside the existing scratch suite.
- firmware/main/root.html's SSID/password/toolsserver/access-token
  inputs had no maxlength, so pasting something longer than the
  matching NVS buffer (wifi_provisioning.h's FRAME_CFG_*_MAX_LEN) was
  silently truncated with no indication why the device later can't
  connect or gets 401s.
- frame_client.c's share_url buffer (256 bytes) could be too small in
  the worst case -- toolsserver (128) + "/frame/share/" + asset_id (47)
  + "?token=" + access_token (64) can reach ~266 bytes, silently
  dropping the token off a request that would then just 401 with no
  obvious cause. Widened to 320.

firmware/main/frame_client.c

@@ -608,7 +608,14 @@ static esp_err_t show_menu_level(const frame_config_t *cfg, fetch_action_t actio
     char location_line1[32];
     char location_line2[32];
     char taken_at[32];
-    char share_url[256];
+    /* Wider than the other URL buffers in this file: unlike a fixed path,
+     * this one stacks toolsserver (up to 128) + "/frame/share/" + an
+     * asset_id (up to 47) + "?token=" + an access_token (up to 64) --
+     * worst case ~266 bytes, which a 256-byte buffer could silently
+     * truncate the token off of (build_url()'s bounds check avoids an
+     * overflow, but a truncated/dropped token still means the resulting
+     * request just 401s with no obvious cause). */
+    char share_url[320];
     fetch_photo_info(cfg, location_line1, sizeof(location_line1), location_line2,
                       sizeof(location_line2), taken_at, sizeof(taken_at), share_url, sizeof(share_url));
 

firmware/main/root.html

@@ -76,26 +76,30 @@
         <p>Please connect me to your local wifi network</p>
         
         <!-- The action attribute should point to wherever your server handles the data -->
+        <!-- maxlength on each field below mirrors its NVS buffer size in
+             wifi_provisioning.h (FRAME_CFG_*_MAX_LEN) -- firmware silently
+             truncates past that length, so keep these in sync if those
+             change. -->
         <form action="/save_config" method="POST">
             
             <div class="input-group">
                 <label for="ssid">Wifi SSID</label>
-                <input type="text" id="ssid" name="ssid" placeholder="Network Name" required>
+                <input type="text" id="ssid" name="ssid" placeholder="Network Name" maxlength="32" required>
             </div>
 
             <div class="input-group">
                 <label for="password">Password</label>
-                <input type="password" id="password" name="password" placeholder="Network Password">
+                <input type="password" id="password" name="password" placeholder="Network Password" maxlength="64">
             </div>
 
             <div class="input-group">
                 <label for="toolsserver">Tools Server</label>
-                <input type="text" id="toolsserver" name="toolsserver" placeholder="e.g. 192.168.1.50:8080 or https://frame.example.com" required>
+                <input type="text" id="toolsserver" name="toolsserver" placeholder="e.g. 192.168.1.50:8080 or https://frame.example.com" maxlength="128" required>
             </div>
 
             <div class="input-group">
                 <label for="access_token">Access Token (optional)</label>
-                <input type="text" id="access_token" name="access_token" placeholder="only if the server's MANAGEMENT_TOKEN is set">
+                <input type="text" id="access_token" name="access_token" placeholder="only if the server's MANAGEMENT_TOKEN is set" maxlength="64">
             </div>
 
             <button type="submit">Submit</button>

server/app/config.py

@@ -5,14 +5,18 @@ from __future__ import annotations
 
 import json
 import os
+from contextlib import contextmanager
 from pathlib import Path
-from threading import Lock
+from threading import RLock
+from typing import Iterator
 
 from pydantic import BaseModel
 
 CONFIG_PATH = Path(os.environ.get("CONFIG_PATH", "/data/config.json"))
 
-_lock = Lock()
+# Reentrant so load()/save() can each take it internally for their own I/O
+# while a caller also holds it for a whole locked() span (see below).
+_lock = RLock()
 
 
 class FrameConfig(BaseModel):
@@ -63,3 +67,16 @@ def save(cfg: FrameConfig) -> None:
     with _lock:
         CONFIG_PATH.parent.mkdir(parents=True, exist_ok=True)
         CONFIG_PATH.write_text(cfg.model_dump_json(indent=2))
+
+
+@contextmanager
+def locked() -> Iterator[None]:
+    """Serializes an entire load-mutate-save cycle. load()/save() each
+    only lock their own I/O, which isn't enough by itself: uvicorn
+    dispatches sync routes to a thread pool, so two concurrent requests
+    (e.g. the device's own poll landing alongside a web UI edit) can each
+    load() the same on-disk state and the second save() silently clobber
+    the first's changes. Route handlers that mutate config should wrap
+    their whole load/mutate/save span in this."""
+    with _lock:
+        yield

server/app/main.py

@@ -122,21 +122,22 @@ def api_config_save(
     # docker-compose.yml.example) -- config.load() already applies them,
     # and this handler doesn't touch cfg.immich_url/immich_api_key at all,
     # so there's nothing here that could overwrite or clear them.
-    cfg = config.load()
-    if album_id != cfg.album_id:
-        # A newly selected album starts clean -- the old current photo and
-        # queue don't mean anything in the new album's context.
-        cfg.current_asset_id = ""
-        cfg.current_asset_set_at = 0.0
-        cfg.queue = []
-        cfg.queue_cursor = 0
-        cfg.history = []
-    cfg.album_id = album_id
-    cfg.order = order if order in ("sequential", "shuffle") else "sequential"
-    cfg.refresh_interval_s = max(MIN_REFRESH_INTERVAL_S, min(MAX_REFRESH_INTERVAL_S, refresh_interval_s))
-    cfg.smart_crop_faces = smart_crop_faces
-    cfg.queue_target_len = max(MIN_QUEUE_TARGET_LEN, min(MAX_QUEUE_TARGET_LEN, queue_target_len))
-    config.save(cfg)
+    with config.locked():
+        cfg = config.load()
+        if album_id != cfg.album_id:
+            # A newly selected album starts clean -- the old current photo and
+            # queue don't mean anything in the new album's context.
+            cfg.current_asset_id = ""
+            cfg.current_asset_set_at = 0.0
+            cfg.queue = []
+            cfg.queue_cursor = 0
+            cfg.history = []
+        cfg.album_id = album_id
+        cfg.order = order if order in ("sequential", "shuffle") else "sequential"
+        cfg.refresh_interval_s = max(MIN_REFRESH_INTERVAL_S, min(MAX_REFRESH_INTERVAL_S, refresh_interval_s))
+        cfg.smart_crop_faces = smart_crop_faces
+        cfg.queue_target_len = max(MIN_QUEUE_TARGET_LEN, min(MAX_QUEUE_TARGET_LEN, queue_target_len))
+        config.save(cfg)
     return {"status": "saved"}
 
 
@@ -189,8 +190,10 @@ def frame_image():
     client = ImmichClient(cfg.immich_url, cfg.immich_api_key)
     assets = _list_assets(client, cfg)
 
-    if photo_queue.get_current(cfg, assets):
-        config.save(cfg)
+    with config.locked():
+        cfg = config.load()  # re-read: state may have changed since the unlocked read above
+        if photo_queue.get_current(cfg, assets):
+            config.save(cfg)
 
     return Response(content=_render_asset(client, cfg, cfg.current_asset_id), media_type="application/octet-stream")
 
@@ -206,8 +209,10 @@ def frame_advance():
     client = ImmichClient(cfg.immich_url, cfg.immich_api_key)
     assets = _list_assets(client, cfg)
 
-    photo_queue.advance_forced(cfg, assets)
-    config.save(cfg)
+    with config.locked():
+        cfg = config.load()  # re-read: state may have changed since the unlocked read above
+        photo_queue.advance_forced(cfg, assets)
+        config.save(cfg)
 
     return Response(content=_render_asset(client, cfg, cfg.current_asset_id), media_type="application/octet-stream")
 
@@ -226,8 +231,10 @@ def frame_back():
     client = ImmichClient(cfg.immich_url, cfg.immich_api_key)
     assets = _list_assets(client, cfg)
 
-    photo_queue.back_forced(cfg, assets)
-    config.save(cfg)
+    with config.locked():
+        cfg = config.load()  # re-read: state may have changed since the unlocked read above
+        photo_queue.back_forced(cfg, assets)
+        config.save(cfg)
 
     return Response(content=_render_asset(client, cfg, cfg.current_asset_id), media_type="application/octet-stream")
 
@@ -314,8 +321,10 @@ def frame_photo_info():
     client = ImmichClient(cfg.immich_url, cfg.immich_api_key)
     assets = _list_assets(client, cfg)
 
-    if photo_queue.get_current(cfg, assets):
-        config.save(cfg)
+    with config.locked():
+        cfg = config.load()  # re-read: state may have changed since the unlocked read above
+        if photo_queue.get_current(cfg, assets):
+            config.save(cfg)
 
     if not cfg.current_asset_id:
         raise HTTPException(404, "No current photo")
@@ -379,8 +388,10 @@ def frame_face_labels():
     client = ImmichClient(cfg.immich_url, cfg.immich_api_key)
     assets = _list_assets(client, cfg)
 
-    if photo_queue.get_current(cfg, assets):
-        config.save(cfg)
+    with config.locked():
+        cfg = config.load()  # re-read: state may have changed since the unlocked read above
+        if photo_queue.get_current(cfg, assets):
+            config.save(cfg)
 
     if not cfg.current_asset_id:
         return {"count": 0}
@@ -418,11 +429,13 @@ def api_queue():
     client = ImmichClient(cfg.immich_url, cfg.immich_api_key)
     assets = _list_assets(client, cfg)
 
-    current_changed = photo_queue.get_current(cfg, assets)
-    queue_before = list(cfg.queue)
-    photo_queue.sync_queue_length(cfg, assets)
-    if current_changed or cfg.queue != queue_before:
-        config.save(cfg)
+    with config.locked():
+        cfg = config.load()  # re-read: state may have changed since the unlocked read above
+        current_changed = photo_queue.get_current(cfg, assets)
+        queue_before = list(cfg.queue)
+        photo_queue.sync_queue_length(cfg, assets)
+        if current_changed or cfg.queue != queue_before:
+            config.save(cfg)
 
     def entry(asset_id: str) -> dict:
         return {"id": asset_id, "thumbnail_url": f"/api/photo-thumbnail/{asset_id}"}
@@ -445,12 +458,13 @@ def api_queue_reorder(body: QueueReorderRequest):
     the client sent that's no longer actually queued is dropped, and any
     ID the server has that the client didn't know about is appended
     rather than lost."""
-    cfg = config.load()
-    current_set = set(cfg.queue)
-    reordered = [asset_id for asset_id in body.queue if asset_id in current_set]
-    reordered += [asset_id for asset_id in cfg.queue if asset_id not in set(reordered)]
-    cfg.queue = reordered
-    config.save(cfg)
+    with config.locked():
+        cfg = config.load()
+        current_set = set(cfg.queue)
+        reordered = [asset_id for asset_id in body.queue if asset_id in current_set]
+        reordered += [asset_id for asset_id in cfg.queue if asset_id not in set(reordered)]
+        cfg.queue = reordered
+        config.save(cfg)
     return {"status": "saved"}
 
 
@@ -465,11 +479,12 @@ def api_queue_promote(body: QueuePromoteRequest):
     client supplying a full, exactly-current snapshot of the queue at
     all, so it can't fail due to the queue having shifted server-side
     since the browser's last fetch."""
-    cfg = config.load()
-    if body.asset_id not in cfg.queue:
-        raise HTTPException(400, "That photo is no longer in the upcoming queue")
-    cfg.queue = [body.asset_id] + [asset_id for asset_id in cfg.queue if asset_id != body.asset_id]
-    config.save(cfg)
+    with config.locked():
+        cfg = config.load()
+        if body.asset_id not in cfg.queue:
+            raise HTTPException(400, "That photo is no longer in the upcoming queue")
+        cfg.queue = [body.asset_id] + [asset_id for asset_id in cfg.queue if asset_id != body.asset_id]
+        config.save(cfg)
     return {"status": "saved"}