Audit round 6 (mobile + --lan): access key replaces trust-by-allowlist

Five blind reviewers over 2936d21; 13 confirmed fixes. The design
change: the Host allowlist was CSRF armor being asked to do access
control. --lan now mints a per-run access key — the printed URLs carry
?k=..., the first visit sets a cookie — required on EVERY request,
reads included (shelf photos and pipeline state are private). That
closes DNS-rebinding read exfiltration (GETs were exempt from the old
guard), closes any-LAN-device mutations via a forged localhost Host,
and frees phones from allowlist accuracy — multi-interface machines,
DHCP renewals, and failed IP discovery no longer strand writes. A
foreign Origin is still refused even with the key.

Guard hardening: Host parsed via url.hostname (ports, IPv6 brackets,
case) instead of a manual split; refusals now echo one stderr line
(they were invisible at log_level=warning) and the LAN 403 names the
remedy; startup warns when no LAN IP could be determined instead of
printing hostname-only URLs as if verified.

Silent failures: the queue page no longer freezes blank forever when a
render throws (LAST was recorded before render; one malformed CSV cell
would blank all three ledgers and blame the network) — all three
change-detection pages record LAST only after a successful render, and
the queue null-guards source_photos.

Mobile: touch-size the review/ticket/merge buttons the finger-sized
rule lost to on specificity. Style: the meta-cell builder is one shared
metaLine() helper; the Help page no longer claims localhost-only;
dead -webkit prefix dropped; --lan help text in house style.

Tests: token gating (reads and writes, cookie handoff, foreign-Origin
refusal), the Origin-present + Host-with-port path every real browser
mutation takes (was fully uncovered), run_web_review's lan branch via
monkeypatched uvicorn, and a lan_hosts test that actually pins the
lowercase/non-empty/v4-only invariants the guard depends on.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016jXZFSTZQKzAC8fqpWSz9g

README.md

@@ -90,7 +90,7 @@ Non-secret knobs (`photos_dir`, `data_dir`, the vision model, the rate limit) li
 uv run bggpipe web    # opens http://127.0.0.1:8377/ — the whole app in the browser
 ```
 
-The app is localhost-only by default. `--lan` also serves it to your local network — handy for proofreading from the couch or snapping shelf photos on your phone and uploading them straight into the Photos page — but mind the trade: there is no login, so anyone on the network can operate the pipeline. Use it on networks you trust (or use a device VPN like Tailscale against the localhost default instead).
+The app is localhost-only by default. `--lan` also serves it to your local network — handy for proofreading from the couch or snapping shelf photos on your phone and uploading them straight into the Photos page. It prints a link carrying a per-run access key (`?k=...`): open that exact link on the phone once and a cookie remembers it. The key is the only lock — there is no login behind it — so still prefer networks you trust (or use a device VPN like Tailscale against the localhost default instead).
 
 Six pages in one local app: **Pipeline** (run stages, watch live output), **Photos** (drag-and-drop upload, gallery, reshoot tickets), **Review** (keyboard-first match and edition decisions), **Titles** (every read off your shelves, alphabetized — and where you proofread them: fix misreads, add cues, split multi-copy lines, remove non-games), **Queue** (exactly what upload will do, plus its full log), and **Library** (your enriched collection, browsable once real BGG data lands). The real upload sits behind a confirmation and behind the stub-data lock. Prefer the terminal? Every stage is also a command, and the two interfaces share all state:
 

src/bggpipe/cli.py

@@ -100,8 +100,8 @@ def web(
         bool,
         typer.Option(
             "--lan",
-            help="Also serve to your local network (phone, tablet). The app "
-            "has no login — only use on a network you trust.",
+            help="Also serve to your local network behind a per-run access "
+            "key (trusted networks only)",
         ),
     ] = False,
     config: ConfigOpt = None,

src/bggpipe/static/app.css

@@ -479,7 +479,7 @@ button.danger { color: var(--stop-ink); border-color: var(--stop); background: #
   .wordmark small { display: inline; margin-left: .5rem; font-size: .68rem; }
   nav[aria-label="Primary"] {
     flex-direction: row; padding: 0 .4rem .1rem;
-    overflow-x: auto; -webkit-overflow-scrolling: touch;
+    overflow-x: auto;
   }
   nav[aria-label="Primary"] a {
     flex: 0 0 auto;
@@ -499,6 +499,9 @@ button.danger { color: var(--stop-ink); border-color: var(--stop); background: #
   h1 { font-size: 1.3rem; margin: .9rem 0 .7rem; }
   .keyhelp { display: none; }  /* no keyboard on a phone */
   button, .linkbtn { padding: .45rem .9rem; }  /* finger-sized */
+  /* component buttons set their own padding at higher specificity —
+   * they need the touch bump spelled out */
+  .rowactions button, .ticket button, .card.merge button { padding: .4rem .8rem; }
 
   .card, .ticket { flex-direction: column; }
   .shots { flex-basis: auto; }

src/bggpipe/static/app.js

@@ -82,6 +82,16 @@ async function refreshBadges() {
 refreshBadges().catch(() => {});
 setInterval(() => refreshBadges().catch(() => {}), 5000);
 
+/* One-line match summary for a titles/photo table row. Empty when the
+ * row has no BGG data yet, so the mobile stacker can hide the cell. */
+function metaLine(c) {
+  return [
+    c.bgg_name ? esc(c.bgg_name) + (c.bgg_id ? " · " + esc(c.bgg_id) : "") : "",
+    c.version_name ? esc(c.version_name) : "",
+    c.type === "rpgitem" ? `<span class="chip open">RPG · local only</span>` : "",
+  ].filter(Boolean).join(" · ");
+}
+
 /* Status chip for a catalog entry — shared by the catalog and photo pages. */
 function statusChip(c) {
   if (c.status === "awaiting_resolve") return `<span class="chip wait">awaiting BGG</span>`;

src/bggpipe/templates/pages/help.html

@@ -61,6 +61,6 @@
 <h2 id="files">Your data, on disk</h2>
 <div class="card">
   <p>Everything lives in flat files under <code>data/</code> — inspectable, hand-editable, and git-friendly. The pipeline artifacts: <code>titles.json</code> (what was read), <code>matches.csv</code> (what it matched), <code>to_add.csv</code>/<code>to_update.csv</code> (what upload will do), <code>upload_log.csv</code> (what it did), <code>games.json</code> (the library). Your curation: <code>title_edits.json</code>, <code>title_splits.json</code>, <code>title_removals.json</code>, <code>unidentified_dismissed.json</code>.</p>
-  <p>Credentials never live in files — only environment variables, set up by <code>bggpipe init</code>. The app runs on localhost only.</p>
+  <p>Credentials never live in files — only environment variables, set up by <code>bggpipe init</code>. The app serves localhost only, unless started with <code>--lan</code> — that opens it to your network behind a per-run access key printed at startup (no login beyond the key; trusted networks only).</p>
   <p>More depth: the README covers setup and photo technique; <code>docs/bgg-upload-flow.md</code> documents the upload automation.</p>
 </div>

src/bggpipe/templates/pages/photo.html

@@ -64,10 +64,7 @@ function render(state, photos) {
       <tr>
         <td class="t">${esc(c.title_raw)}</td>
         <td>${statusChip(c)}</td>
-        <td class="meta">${[
-          c.bgg_name ? esc(c.bgg_name) + (c.bgg_id ? " · " + esc(c.bgg_id) : "") : "",
-          c.version_name ? esc(c.version_name) : "",
-        ].filter(Boolean).join(" · ")}</td>
+        <td class="meta">${metaLine(c)}</td>
       </tr>`).join("") + `</table></div>`
     : `<p class="empty">${info.extracted
         ? "No titles were read from this photo."
@@ -97,8 +94,8 @@ async function refresh() {
   ]);
   const payload = JSON.stringify([state.catalog, state.unidentified, photos]);
   if (payload === LAST) return;
-  LAST = payload;
   render(state, photos);
+  LAST = payload;  // after render: a throw must not freeze the page as "current"
 }
 
 document.addEventListener("keydown", e => {

src/bggpipe/templates/pages/queue.html

@@ -19,7 +19,7 @@ function render(q) {
     ? table(["game", "version", "seen in"], q.to_add.map(r => `
         <tr><td class="t">${esc(r.bgg_name)} <span class="meta">· ${esc(r.bgg_id)}</span></td>
         <td>${r.version_name ? esc(r.version_name) : `<span class="meta">no version</span>`}</td>
-        <td class="meta">${esc(r.source_photos.split(";").join(", "))}</td></tr>`))
+        <td class="meta">${esc((r.source_photos ?? "").split(";").join(", "))}</td></tr>`))
     : `<p class="empty">Nothing queued — run <b>diff</b> from the <a href="/">pipeline</a> first.</p>`;
 
   html += `<h2>Version updates <span class="count">— ${q.to_update.length} existing entr${q.to_update.length === 1 ? "y" : "ies"} gaining a version</span></h2>`;
@@ -48,8 +48,8 @@ async function refresh() {
   const q = await fetchJSON("/api/queue");
   const payload = JSON.stringify(q);
   if (payload === LAST) return;
-  LAST = payload;
   render(q);
+  LAST = payload;  // after render: a throw must not freeze the page as "current"
 }
 
 refresh().catch(err => errorBanner(err.message || err));

src/bggpipe/templates/pages/titles.html

@@ -59,11 +59,7 @@ function render() {
           ${c.shaky ? `<span class="chip shaky"
               title="the model wasn't sure of this read — press ✓ if it's right, or edit it">shaky read</span>` : ""}</td>
         <td>${statusChip(c)}</td>
-        <td class="meta">${[
-          c.bgg_name ? esc(c.bgg_name) + (c.bgg_id ? " · " + esc(c.bgg_id) : "") : "",
-          c.version_name ? esc(c.version_name) : "",
-          c.type === "rpgitem" ? `<span class="chip open">RPG · local only</span>` : "",
-        ].filter(Boolean).join(" · ")}</td>
+        <td class="meta">${metaLine(c)}</td>
         <td class="meta">${c.photos.map(p =>
           `<a href="/photos/view/${encodeURIComponent(p)}">${esc(p)}</a>`
         ).join(", ")}</td>
@@ -92,9 +88,9 @@ async function refresh() {
   if (EDITING) return;  // never repaint under an open editor
   const payload = JSON.stringify(state.catalog);
   if (payload === LAST) return;
-  LAST = payload;
   CATALOG = state.catalog;
   render();
+  LAST = payload;  // after render: a throw must not freeze the page as "current"
 }
 
 document.getElementById("catbody").addEventListener("click", async e => {

src/bggpipe/webreview.py

@@ -17,6 +17,7 @@ from __future__ import annotations
 import io
 import json
 import os
+import secrets
 import socket
 import threading
 import time
@@ -279,6 +280,7 @@ def create_app(
     stages: dict[str, Callable[..., object]] | None = None,
     jobs: JobRunner | None = None,
     allowed_hosts: set[str] | None = None,
+    lan_token: str | None = None,
 ) -> FastAPI:
     app = FastAPI(title="bggpipe")
     stages = stages or _default_stages(cfg)
@@ -305,22 +307,64 @@ def create_app(
     app_warnings: list[str] = list(startup_notes)
 
     ALLOWED_HOSTS = {"127.0.0.1", "localhost", "testserver"} | (allowed_hosts or set())
+    LAN_COOKIE = "bggpipe_key"
 
     @app.middleware("http")
     async def origin_guard(request, call_next):
         # A hostile webpage can fire preflight-free cross-origin POSTs at a
-        # localhost server (bodyless run triggers, multipart photo posts).
-        # Mutations must come from us: same-host, and no foreign Origin.
-        if request.method not in ("GET", "HEAD", "OPTIONS"):
-            host = (request.headers.get("host") or "").split(":")[0]
+        # localhost server (bodyless run triggers, multipart photo posts) —
+        # and via DNS rebinding it can read GETs too. url.hostname parses
+        # the Host header properly (ports, IPv6 brackets, lowercase).
+        host = (request.url.hostname or "").lower()
         origin = request.headers.get("origin")
         origin_host = urlsplit(origin).hostname if origin else None
-            if host not in ALLOWED_HOSTS or (
-                origin_host is not None and origin_host not in ALLOWED_HOSTS
+        if lan_token is not None:
+            # --lan has no login, so EVERY request — reads included: shelf
+            # photos and pipeline state are private — needs the per-run
+            # key from the printed URL; a cookie carries it afterwards.
+            supplied = (
+                request.query_params.get("k") or request.cookies.get(LAN_COOKIE) or ""
+            )
+            if not secrets.compare_digest(supplied, lan_token):
+                typer.echo(
+                    f"refused {request.method} {request.url.path} "
+                    f"(host {host!r}): missing or wrong access key",
+                    err=True,
+                )
+                return JSONResponse(
+                    {
+                        "detail": "missing or wrong access key — open the "
+                        "exact URL printed where the server started "
+                        "(it ends in ?k=...)"
+                    },
+                    status_code=403,
+                )
+            if request.method not in ("GET", "HEAD", "OPTIONS") and (
+                origin_host is not None and origin_host != host
             ):
                 return JSONResponse(
                     {"detail": "cross-origin request refused"}, status_code=403
                 )
+            response = await call_next(request)
+            if request.query_params.get("k"):
+                # the key came in the typed URL: hand it to the browser so
+                # navigation and fetches keep working without it
+                response.set_cookie(
+                    LAN_COOKIE, lan_token, httponly=True, samesite="lax"
+                )
+            return response
+        if request.method not in ("GET", "HEAD", "OPTIONS") and (
+            host not in ALLOWED_HOSTS
+            or (origin_host is not None and origin_host not in ALLOWED_HOSTS)
+        ):
+            typer.echo(
+                f"refused {request.method} {request.url.path}: host {host!r}"
+                f" / origin {origin_host!r} not on this server's allowlist",
+                err=True,
+            )
+            return JSONResponse(
+                {"detail": "cross-origin request refused"}, status_code=403
+            )
         return await call_next(request)
 
     def freshen() -> None:
@@ -1019,8 +1063,8 @@ def _dev_app() -> FastAPI:
     from bggpipe.config import load_config
 
     path = os.environ.get("BGGPIPE_CONFIG") or None
-    extra = lan_hosts() if os.environ.get("BGGPIPE_LAN") else None
-    return create_app(load_config(Path(path) if path else None), allowed_hosts=extra)
+    token = os.environ.get("BGGPIPE_LAN_TOKEN") or None
+    return create_app(load_config(Path(path) if path else None), lan_token=token)
 
 
 def run_web_review(
@@ -1036,17 +1080,24 @@ def run_web_review(
     import uvicorn
 
     url = f"http://127.0.0.1:{port}{landing}"
-    extra_hosts = lan_hosts() if lan else None
+    token = secrets.token_urlsafe(6) if lan else None
     if lan:
-        addresses = ", ".join(f"http://{h}:{port}/" for h in sorted(extra_hosts or []))
+        ips = sorted(h for h in lan_hosts() if h.replace(".", "").isdigit())
+        names = sorted(h for h in lan_hosts() if not h.replace(".", "").isdigit())
+        addresses = ", ".join(f"http://{h}:{port}/?k={token}" for h in ips + names)
+        typer.echo(f"bggpipe web UI: {url}?k={token}")
+        typer.echo(f"  from your phone, open: {addresses}")
         typer.echo(
-            f"bggpipe web UI: {url}  — ALSO reachable from your network: {addresses}"
-        )
-        typer.echo(
-            "  --lan: the app has NO login. Anyone on this network can run "
+            "  --lan: no login beyond that key — anyone who has it can run "
             "stages, change your data, and (once unlocked) drive uploads "
             "to your BGG account. Use only on a network you trust."
         )
+        if not ips:
+            typer.echo(
+                "  couldn't determine this machine's network address — find "
+                "it in your network settings and open "
+                f"http://<that-ip>:{port}/?k={token}"
+            )
     else:
         typer.echo(
             f"bggpipe web UI: {url}  (localhost only; dashboard at /, review "
@@ -1061,8 +1112,8 @@ def run_web_review(
         # write — i.e. on every review decision.
         if config_path:
             os.environ["BGGPIPE_CONFIG"] = str(config_path)
-        if lan:
-            os.environ["BGGPIPE_LAN"] = "1"
+        if token:
+            os.environ["BGGPIPE_LAN_TOKEN"] = token
         typer.echo("  --dev: restarting on source changes")
         uvicorn.run(
             "bggpipe.webreview:_dev_app",
@@ -1075,7 +1126,7 @@ def run_web_review(
         )
     else:
         uvicorn.run(
-            create_app(cfg, allowed_hosts=extra_hosts),
+            create_app(cfg, lan_token=token),
             host="0.0.0.0" if lan else "127.0.0.1",  # noqa: S104 — opted in
             port=port,
             log_level="warning",

tests/test_webreview.py

@@ -894,9 +894,87 @@ def test_lan_allowed_hosts_admit_network_but_not_strangers(tmp_path):
     )
 
 
-def test_lan_hosts_reports_this_machine(tmp_path):
+def test_lan_hosts_are_lowercase_nonempty_and_v4_only(monkeypatch):
+    import socket as socket_mod
+
     from bggpipe.webreview import lan_hosts
 
+    monkeypatch.setattr(socket_mod, "gethostname", lambda: "Erics-Mac.Example.COM")
+    monkeypatch.setattr(
+        socket_mod,
+        "getaddrinfo",
+        lambda *a, **k: (_ for _ in ()).throw(OSError("no dns")),
+    )
     hosts = lan_hosts()
-    assert hosts  # at least the hostname
-    assert all(h == h.lower() or "." in h for h in hosts)
+    assert "erics-mac.example.com" in hosts
+    assert "erics-mac.local" in hosts
+    assert "" not in hosts  # an empty entry would admit Host-less mutations
+    assert all(":" not in h for h in hosts)  # no v6 forms, no ports
+
+
+def test_lan_token_gates_every_request(tmp_path):
+    cfg = make_cfg(tmp_path)
+    app = create_app(cfg, client=unauthorized_client(tmp_path), lan_token="sekret")
+    phone = TestClient(app, base_url="http://192.168.1.99:8377")
+    # reads are gated too: shelf photos and state are private
+    assert phone.get("/api/state").status_code == 403
+    assert phone.get("/api/state?k=wrong").status_code == 403
+    first = phone.get("/titles?k=sekret")
+    assert first.status_code == 200
+    assert "bggpipe_key" in first.cookies  # the URL key becomes a cookie
+    # cookie carries the session: mutations work from ANY host the phone
+    # used (no allowlist dependence — DHCP/multi-interface safe), with a
+    # same-origin Origin header and a port, like a real phone browser
+    res = phone.post(
+        "/api/edit-title",
+        json={"title_raw": "Citadels", "source_photos": "shelf.jpg", "confirm": True},
+        headers={"origin": "http://192.168.1.99:8377"},
+    )
+    assert res.status_code == 200
+    # a foreign Origin is still refused even with the key
+    assert (
+        phone.post(
+            "/api/dismiss",
+            json={"photo": "shelf.jpg"},
+            headers={"origin": "http://evil.example"},
+        ).status_code
+        == 403
+    )
+
+
+def test_localhost_mutations_pass_with_origin_and_port(tmp_path):
+    # the path every real browser takes: Origin present + port in Host —
+    # regressing the header parsing must fail loudly here
+    cfg = make_cfg(tmp_path)
+    app = create_app(cfg, client=unauthorized_client(tmp_path))
+    web = TestClient(app, base_url="http://localhost:8377")
+    res = web.post(
+        "/api/edit-title",
+        json={"title_raw": "Citadels", "source_photos": "shelf.jpg", "confirm": True},
+        headers={"origin": "http://localhost:8377"},
+    )
+    assert res.status_code == 200
+    assert (
+        TestClient(app, base_url="http://attacker.example:8377")
+        .post(
+            "/api/edit-title",
+            json={"title_raw": "Citadels", "source_photos": "shelf.jpg"},
+        )
+        .status_code
+        == 403
+    )
+
+
+def test_run_web_review_lan_branch_binds_and_warns(tmp_path, monkeypatch, capsys):
+    import bggpipe.webreview as wr
+
+    captured = {}
+    monkeypatch.setattr("uvicorn.run", lambda app, **kw: captured.update(kw, app=app))
+    cfg = make_cfg(tmp_path)
+    wr.run_web_review(cfg, port=9999, lan=True)
+    out = capsys.readouterr().out
+    assert captured["host"] == "0.0.0.0"
+    assert "?k=" in out  # every printed URL carries the access key
+    assert "Use only on a network you trust" in out
+    wr.run_web_review(cfg, port=9999, lan=False)
+    assert captured["host"] == "127.0.0.1"