From 7ca736d52534f303ace1f94b93bc7e19c1e71ee2 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Thu, 2 Jul 2026 13:55:43 -0500 Subject: [PATCH] =?UTF-8?q?library:=20cover-art=20layer=20=E2=80=94=20CAA?= =?UTF-8?q?=20auto-fetch=20+=20user=20overrides,=20GIF=20local-only=20(P9)?= =?UTF-8?q?=20(#715)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * library: cover-art layer — CAA auto-fetch + user overrides, GIF local-only (R3/P9) Third slice of the enrichment series (stacked on the matcher): covers. - Serve chain for GET /api/song/{fn}/art: USER OVERRIDE -> PACK ART -> COVER ART ARCHIVE cache -> 404. Behaviour change, deliberate: a user-uploaded cover now OVERRIDES pack art (previously the upload only filled the no-art gap, which made custom art look broken on any song that already shipped a cover). - GIF is allowed as an override and kept VERBATIM (animation intact) — a local-only bonus. Everything else normalizes to RGB PNG as before. One override per song (saving either kind removes the other), and nothing ever writes art INTO a pack file — test-pinned: the pack's cover.jpg is byte-identical after a GIF upload. - Art by URL: POST /api/song/{fn}/art/url fetches server-side (http(s) only, 10 MB cap enforced while streaming) into the same override slot. DELETE /api/art/{fn}/override drops it — under /api/art because the greedy DELETE /api/song/{path} catch-all shadows anything beneath it (the same dodge the chart split/unsplit routes use). - Cover Art Archive fetch as phase 3 of the enrichment pass: matched songs that LACK pack art get their release's front cover, throttled + identified + offline-guarded exactly like the MusicBrainz client (pytest can never reach the network; a transport error pauses the pass without burning the row). The cache is keyed by RELEASE MBID — ten charts of one album cost one fetch — and every outcome writes an art_state (pack/user/caa/none/error) so a row is evaluated once. - LRU cap (200 MB) on the CAA side of the cache only; user overrides are never evicted, and evicted rows reset so a later pass may re-fetch. Deleting a song removes its override files (CAA files stay — they may be shared by other charts of the release). No frontend changes: the grid, the review modal, and the player pick the new art up through the same route they already use. The upload/paste-a-link surfaces in the Details drawer land with the context-menu slice once the drawer PR merges. 13 new tests (tests/test_art_layer.py) + demo-mode routes; full-suite failure set byte-identical with the change stashed vs applied. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Nm7tHs1Yvjjtnnu4nzJgdN * library: harden cover-art layer — SSRF guard, demo/size caps, override-delete state reset Follow-up hardening on the R3 cover-art layer: - remove_song_art_override: reset the enrichment row (set_enrichment_art(fn, None, None)) when an override is deleted, so a row previously settled as 'user' re-queues and the CAA fallback resumes. Previously a removed override stranded the row (enrichment_art_pending only re-queues art_state IS NULL), leaving the song with no art at all. - Base64 art upload: block it in demo mode (was open — a write/disk-fill vector, worse now that GIFs are stored verbatim), validate the filename resolves to a real song (mirrors the url route), and cap the decoded payload at 10 MB. - Art-by-URL: reject hosts that resolve to loopback/private/link-local/reserved/ multicast/unspecified addresses (SSRF, e.g. cloud metadata) and stop following redirects (allow_redirects=False) so a redirect can't smuggle the request to an internal target. Fails closed on unresolvable/unparseable hosts. - _caa_http_get: stream with a per-file 10 MB cap (bounds any one response independently of the aggregate LRU); guard release_id against a conservative token before interpolating it into a cache-file path (no separators/dots). Tests: delete-override→CAA-fallback, upload unknown-song/oversize rejection, SSRF internal-host guard, and a demo-mode block assertion for art/upload. Note: art_state='error' rows are intentionally not auto-retried — there is no per-row attempt counter on the art side, so an unbounded retry could storm CAA for permanently-bad rows; a bounded retry would need extra state, left out here. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Fable 5 Co-authored-by: byrongamatos --- server.py | 452 ++++++++++++++++++++++++++++++++++++---- tests/test_art_layer.py | 318 ++++++++++++++++++++++++++++ tests/test_demo_mode.py | 5 + 3 files changed, 736 insertions(+), 39 deletions(-) create mode 100644 tests/test_art_layer.py diff --git a/server.py b/server.py index c39e58d..5de69e1 100644 --- a/server.py +++ b/server.py @@ -246,6 +246,12 @@ _DEMO_BLOCKED: list[tuple[str, re.Pattern]] = [ ("GET", re.compile(r"^/api/chart/.+/fileinfo$")), # Gap-fill (R4a) rewrites pack files on disk — never for demo visitors. ("POST", re.compile(r"^/api/song/.+/gap-fill$")), + # Art layer (R3): all three mutate server state / touch the network on a + # visitor's behalf — the base64 upload writes files, the URL fetch makes the + # server request arbitrary images, and the override delete removes files. + ("POST", re.compile(r"^/api/song/.+/art/upload$")), + ("POST", re.compile(r"^/api/song/.+/art/url$")), + ("DELETE", re.compile(r"^/api/art/.+/override$")), ] @@ -2902,6 +2908,41 @@ class MetadataDB: "candidates": candidates, "attempts": attempts or 0}) return out + def enrichment_art_pending(self, limit: int = 500) -> list[dict]: + """Matched songs whose cover-art situation hasn't been evaluated yet + (art_state NULL). The art worker resolves each to 'pack' (song has its + own art), 'user' (an override exists), 'caa' (fetched), 'none' (the + release has no cover) or 'error' — any of which settles the row, so + this never re-offers a song each pass.""" + rows = self.conn.execute( + "SELECT e.filename, e.mb_release_id " + "FROM song_enrichment e JOIN songs s ON s.filename = e.filename " + "WHERE e.match_state IN ('matched', 'manual') " + "AND e.mb_release_id IS NOT NULL AND e.art_state IS NULL " + "ORDER BY e.filename LIMIT ?", (max(1, int(limit)),)).fetchall() + return [{"filename": r[0], "mb_release_id": r[1]} for r in rows] + + def set_enrichment_art(self, filename: str, path: str | None, state: str | None) -> None: + """Stamp a row's art-cache outcome. Targeted UPDATE (not the match + writer) so it can never disturb the match lifecycle fields.""" + with self._lock: + self.conn.execute( + "UPDATE song_enrichment SET art_cache_path = ?, art_state = ? " + "WHERE filename = ?", (path, state, filename)) + self.conn.commit() + + def clear_enrichment_art_paths(self, paths: list[str]) -> None: + """Reset rows whose cached art file was evicted (LRU prune) back to + unevaluated, so a later pass may re-fetch if the song still qualifies.""" + if not paths: + return + with self._lock: + ph = ",".join("?" * len(paths)) + self.conn.execute( + f"UPDATE song_enrichment SET art_cache_path = NULL, art_state = NULL " + f"WHERE art_cache_path IN ({ph})", paths) + self.conn.commit() + def _estd_set(self) -> set[str]: """Get set of filenames that have a retuned variant (_EStd_ or _DropD_) in the DB.""" rows = self.conn.execute( @@ -5742,6 +5783,154 @@ def _enrich_backoff_elapsed(attempts, last_attempt_at, now: float) -> bool: # handful is noise the user has to scroll past. _ENRICH_MAX_CANDIDATES = 5 +# ── Cover art (R3/P9) ───────────────────────────────────────────────────────── +# The art cache dir (CONFIG_DIR/art_cache) holds two kinds of file: +# {safe_name}.png / .gif — USER OVERRIDES (upload or URL-fetch; never +# evicted, removed only with the song or by the +# explicit remove-override route) +# caa_{release_mbid}.jpg — COVER ART ARCHIVE fetches, keyed by release so +# every chart of the same release shares one file; +# size-capped LRU (evictions reset the enrichment +# rows so a later pass may re-fetch) +_CAA_CACHE_CAP_BYTES = 200 * 1024 * 1024 +# Per-cover cap on a single CAA fetch. The 500px thumbnail is normally tens of +# KB; this bounds any one response independently of the aggregate LRU cap so a +# single oversized (or misbehaving) release can't blow up memory/disk. +_CAA_MAX_BYTES = 10 * 1024 * 1024 +# A release MBID is a UUID; before interpolating it into a cache-file path we +# require a conservative token (alphanumerics, hyphen, underscore only) so no +# separator or '.' can ever appear — blocks path traversal. Defence in depth: +# cheap even though the DB only ever holds MusicBrainz UUIDs. (Distinct name +# from the strict recording-MBID _MBID_RE above — this only gates a filename.) +_CAA_ID_RE = re.compile(r"^[A-Za-z0-9_-]{1,128}$") + + +def _caa_http_get(release_id: str) -> bytes | None: + """Fetch a release's front cover from the Cover Art Archive — the one + network seam of the art layer (tests fake exactly this). Same etiquette + as the MusicBrainz client: throttled, identified, offline-guarded. + Returns the image bytes, None when the release has no cover (404), and + raises EnrichTransportError for anything network-shaped.""" + if not _enrich_network_enabled(): + raise EnrichTransportError("enrichment network disabled") + import requests + _enrich_throttle() + try: + with requests.get( + f"https://coverartarchive.org/release/{release_id}/front-500", + headers={"User-Agent": _enrich_user_agent()}, + timeout=15, allow_redirects=True, stream=True, + ) as resp: + if resp.status_code == 404: + return None + if resp.status_code != 200: + raise EnrichTransportError(f"cover art archive HTTP {resp.status_code}") + # Stream with a per-file cap so a huge response never fully downloads. + data = b"" + for chunk in resp.iter_content(65536): + data += chunk + if len(data) > _CAA_MAX_BYTES: + # Not network-shaped: settle just this row as 'error' (the + # art loop's generic handler) rather than pausing the pass. + raise ValueError("cover art exceeds size cap") + return data + except requests.RequestException as e: + raise EnrichTransportError(str(e)) from e + + +def _art_safe_name(filename: str) -> str: + """The flattened cache-file stem the art routes key user overrides on + (matches the legacy /art/upload naming, so old uploads keep working).""" + return filename.replace("/", "_").replace(" ", "_") + + +def _art_override_paths(filename: str) -> list[Path]: + """Existing user-override art files for a song, GIF first (it wins — + the animated local-only bonus outranks a stale PNG).""" + stem = _art_safe_name(filename) + return [p for p in (ART_CACHE_DIR / f"{stem}.gif", ART_CACHE_DIR / f"{stem}.png") + if p.is_file()] + + +def _song_pack_art_exists(filename: str) -> bool: + """Whether the song carries its own art (sloppak cover / loose-folder + image). Pack art always outranks a CAA fetch, so the art worker marks + these and never spends a request on them.""" + try: + dlc = _get_dlc_dir() + if not dlc: + return False + p = _resolve_dlc_path(dlc, filename) + if p is None or not p.exists(): + return False + if sloppak_mod.is_sloppak(p): + return sloppak_mod.read_cover_bytes(p) is not None + if loosefolder_mod.is_loose_song(p): + return loosefolder_mod.find_art(p) is not None + except Exception: + pass + return False + + +def _prune_caa_cache() -> None: + """Keep the CAA side of the art cache under its size cap: evict the + oldest caa_* files (mtime LRU) and reset the enrichment rows that pointed + at them. User-override files are never touched.""" + try: + files = sorted(ART_CACHE_DIR.glob("caa_*.jpg"), key=lambda p: p.stat().st_mtime) + total = sum(p.stat().st_size for p in files) + evicted: list[str] = [] + while files and total > _CAA_CACHE_CAP_BYTES: + victim = files.pop(0) + try: + total -= victim.stat().st_size + victim.unlink() + evicted.append(str(victim)) + except OSError: + break + if evicted: + meta_db.clear_enrichment_art_paths(evicted) + log.info("art cache: evicted %d cover(s) to stay under the cap", len(evicted)) + except Exception: + log.exception("art cache prune failed") + + +def _enrich_art_one(row: dict) -> bool: + """Resolve one matched song's cover-art situation (art worker, phase 3). + Returns True when a cover was actually fetched. Every outcome writes an + art_state so the row never re-queues: + 'pack' — the song ships its own art (it wins; nothing to do) + 'user' — an override exists (it wins; nothing to do) + 'caa' — front cover cached (possibly deduped from an earlier fetch + of the same release — no network on that path) + 'none' — the Cover Art Archive has no cover for this release + Network errors raise EnrichTransportError → the pass pauses and the row + stays unevaluated for the next kick.""" + fn, release_id = row["filename"], row["mb_release_id"] + if not release_id or not _CAA_ID_RE.match(str(release_id)): + # Malformed release id — never build a cache path from it. Settle the + # row as 'error' so it isn't re-queued every pass. + meta_db.set_enrichment_art(fn, None, "error") + return False + if _song_pack_art_exists(fn): + meta_db.set_enrichment_art(fn, None, "pack") + return False + if _art_override_paths(fn): + meta_db.set_enrichment_art(fn, None, "user") + return False + cache_file = _enrichment_art_dir() / f"caa_{release_id}.jpg" + if cache_file.is_file(): + meta_db.set_enrichment_art(fn, str(cache_file), "caa") + return False + data = _caa_http_get(release_id) + if data is None: + meta_db.set_enrichment_art(fn, None, "none") + return False + cache_file.write_bytes(data) + meta_db.set_enrichment_art(fn, str(cache_file), "caa") + _prune_caa_cache() + return True + def _enrich_one(row: dict, auto_min: float | None = None) -> None: """The matcher (P8; replaces P7's no-op). Precedence per design §5: @@ -5879,6 +6068,32 @@ def _background_enrich(): if pending or retriable: log.info("Enrichment pass: %d rows stamped, %d matched", len(pending), matched) + # Phase 3 — cover art (R3/P9). For freshly-matched songs, resolve the art + # situation once: songs with their own pack art (or a user override) are + # marked and skipped; the rest fetch the release's front cover from the + # Cover Art Archive into the size-capped cache. Same pause-on-transport- + # error rule as matching — a dead network never burns a row's evaluation. + try: + art_rows = meta_db.enrichment_art_pending(limit=100000) + except Exception: + log.exception("enrichment: art-pending query failed") + return + fetched = 0 + for row in art_rows: + try: + fetched += 1 if _enrich_art_one(row) else 0 + except EnrichTransportError as e: + log.info("enrichment: network unavailable, art pass paused (%s)", e) + break + except Exception as e: + log.warning("enrichment art failed for %s: %s", row.get("filename"), e) + try: + meta_db.set_enrichment_art(row["filename"], None, "error") + except Exception: + pass + if art_rows: + log.info("Enrichment art pass: %d evaluated, %d covers fetched", len(art_rows), fetched) + def _kick_enrich() -> bool: """Request an enrichment pass, single-flight + coalescing (the _kick_scan @@ -7013,6 +7228,14 @@ def delete_song(filename: str): meta_db.conn.execute("DELETE FROM song_enrichment WHERE filename = ?", (cache_key,)) meta_db.conn.commit() + # User art overrides go with the song (CAA cache files are keyed by + # RELEASE and may be shared with other charts — the LRU owns those). + for _p in _art_override_paths(cache_key): + try: + _p.unlink() + except OSError: + pass + _invalidate_song_caches(cache_key) log.info("Deleted song %s", cache_key) @@ -10078,14 +10301,17 @@ def _file_art_response(path: Path, media_type: str, request: Request | None): @app.get("/api/song/{filename:path}/art") async def get_song_art(filename: str, request: Request = None): - """Serve album art for a song. + """Serve album art for a song, walking the R3 override chain: - Dispatches by format and returns the appropriate media type: - - Sloppak: serves `cover.jpg` (or manifest-declared cover) read directly - from the package (the single cover member for zip-form sloppaks — no - full unpack) as JPEG/PNG/WebP. - - Loose folder: serves the discovered art file directly as - JPEG/PNG/WebP. + 1. USER OVERRIDE (upload / URL-fetch, {safe_name}.gif|.png in the art + cache) — art the user explicitly pinned outranks everything, pack + art included. GIF is allowed HERE only: an animated cover is a + local-only bonus; packs stay jpg/png/webp and nothing ever writes + art into a pack file. + 2. PACK ART — sloppak cover (single member read, no full unpack) or + the loose folder's discovered image. + 3. COVER ART ARCHIVE cache — fetched by the enrichment art worker for + matched songs that lack pack art, keyed by release MBID. """ dlc = _get_dlc_dir() if not dlc: @@ -10097,7 +10323,12 @@ async def get_song_art(filename: str, request: Request = None): if not song_path.exists(): return JSONResponse({"error": "not found"}, 404) - # Sloppak path: read the cover (manifest-declared or default) straight from + # 1. User override — GIF first (it wins over a stale PNG override). + for cached in _art_override_paths(filename): + mt = "image/gif" if cached.suffix == ".gif" else "image/png" + return _file_art_response(cached, mt, request) + + # 2a. Sloppak: read the cover (manifest-declared or default) straight from # the package. For a zip-form sloppak this opens just the cover member — # NOT the whole archive — so the library grid never triggers a full unpack # of stems just to paint a thumbnail. @@ -10112,18 +10343,17 @@ async def get_song_art(filename: str, request: Request = None): art = await asyncio.to_thread(sloppak_mod.read_cover_bytes, song_path) except Exception: art = None - if art is None: - return JSONResponse({"error": "no art"}, 404) - data, mt = art - etag = f'"{hashlib.sha1(data).hexdigest()}"' - headers, not_modified = _art_conditional(etag, request) - if not_modified: - return Response(status_code=304, headers=headers) - return Response(content=data, media_type=mt, headers=headers) + if art is not None: + data, mt = art + etag = f'"{hashlib.sha1(data).hexdigest()}"' + headers, not_modified = _art_conditional(etag, request) + if not_modified: + return Response(status_code=304, headers=headers) + return Response(content=data, media_type=mt, headers=headers) - # Loose folder path: serve art file directly. + # 2b. Loose folder: serve the discovered art file directly. # song_path is already validated against DLC_DIR by _resolve_dlc_path. - if loosefolder_mod.is_loose_song(song_path): + elif loosefolder_mod.is_loose_song(song_path): art_path = loosefolder_mod.find_art(song_path) if art_path: # Re-resolve in case the matched file is a symlink — a crafted @@ -10140,15 +10370,13 @@ async def get_song_art(filename: str, request: Request = None): ".png": "image/png", ".webp": "image/webp", }.get(art_resolved.suffix.lower(), "image/jpeg") return _file_art_response(art_resolved, mt, request) - return JSONResponse({"error": "no art"}, 404) - # Custom art uploaded via /art/upload is cached as PNG under ART_CACHE_DIR; - # serve it if present (works for any song the user pinned art to). - art_cache = ART_CACHE_DIR - safe_name = filename.replace("/", "_").replace(" ", "_") - cached = art_cache / f"{safe_name}.png" - if cached.exists(): - return _file_art_response(cached, "image/png", request) + # 3. Cover Art Archive cache (the enrichment art worker's fetch). + row = meta_db.get_enrichment(filename) + if row and row.get("art_state") == "caa" and row.get("art_cache_path"): + caa = Path(row["art_cache_path"]) + if caa.is_file(): + return _file_art_response(caa, "image/jpeg", request) return JSONResponse({"error": "no art"}, 404) @@ -10390,10 +10618,50 @@ def post_song_gap_fill(filename: str, data: dict): return {"ok": True, "written": additions, "skipped": skipped} +def _save_art_override(filename: str, img_data: bytes) -> dict: + """Persist a user art override into the art cache (R3). One override per + song: GIF input is validated and kept VERBATIM as .gif (animation intact — + the local-only bonus; it is never written into the pack file), everything + else is normalized to RGB PNG via PIL. Saving either kind removes the + other so the serve chain has exactly one user file to find.""" + ART_CACHE_DIR.mkdir(parents=True, exist_ok=True) + stem = _art_safe_name(filename) + png_path = ART_CACHE_DIR / f"{stem}.png" + gif_path = ART_CACHE_DIR / f"{stem}.gif" + from PIL import Image + import io as _io + if img_data[:6] in (b"GIF87a", b"GIF89a"): + try: + probe = Image.open(_io.BytesIO(img_data)) + probe.verify() # decodes headers/frames without keeping the image + if probe.format != "GIF": + raise ValueError("not a GIF") + except Exception as e: + return {"error": f"Invalid image: {e}"} + gif_path.write_bytes(img_data) + png_path.unlink(missing_ok=True) + return {"ok": True, "kind": "gif"} + try: + img = Image.open(_io.BytesIO(img_data)).convert("RGB") + img.save(str(png_path), "PNG") + except Exception as e: + return {"error": f"Invalid image: {e}"} + gif_path.unlink(missing_ok=True) + return {"ok": True, "kind": "png"} + + @app.post("/api/song/{filename:path}/art/upload") async def upload_song_art_b64(filename: str, data: dict): - """Upload custom album art as base64 PNG/JPG.""" + """Upload a custom cover as base64 (PNG/JPG/WebP → normalized PNG; + GIF → kept animated, local-only). The override outranks pack art in the + serve chain; remove it via DELETE …/art/override.""" import base64 + # Reject art for a filename that doesn't resolve to a real song (mirrors the + # url route's guard) — no writing stray override files for unknown keys. + dlc = _get_dlc_dir() + song_path = _resolve_dlc_path(dlc, filename) if dlc else None + if song_path is None or not song_path.exists(): + raise HTTPException(status_code=404, detail="unknown song") b64 = data.get("image", "") if not b64: return {"error": "No image data"} @@ -10404,22 +10672,128 @@ async def upload_song_art_b64(filename: str, data: dict): img_data = base64.b64decode(b64) except Exception: return {"error": "Invalid base64"} + if len(img_data) > _ART_URL_MAX_BYTES: + raise HTTPException(status_code=400, detail="image larger than 10 MB") + return _save_art_override(filename, img_data) - art_cache = ART_CACHE_DIR - art_cache.mkdir(parents=True, exist_ok=True) - safe_name = filename.replace("/", "_").replace(" ", "_") - cached = art_cache / f"{safe_name}.png" - # Convert to PNG if needed +# Art-by-URL fetch cap — a cover, not a wallpaper pack. +_ART_URL_MAX_BYTES = 10 * 1024 * 1024 + + +def _url_host_is_internal(url: str) -> bool: + """True when a user-supplied URL's host resolves to a loopback, private, + link-local, reserved, multicast or unspecified address — an SSRF target we + refuse to fetch on the user's behalf (e.g. 169.254.169.254 metadata, LAN + services). Fails CLOSED: an unresolvable or unparseable host is treated as + internal. Every resolved address must be public for the URL to pass.""" + from urllib.parse import urlparse + import socket + host = urlparse(url).hostname + if not host: + return True try: - from PIL import Image - import io - img = Image.open(io.BytesIO(img_data)).convert("RGB") - img.save(str(cached), "PNG") - except Exception as e: - return {"error": f"Invalid image: {e}"} + infos = socket.getaddrinfo(host, None) + except OSError: + return True + if not infos: + return True + for info in infos: + raw = info[4][0].split("%", 1)[0] # strip any zone id + try: + ip = ipaddress.ip_address(raw) + except ValueError: + return True + if (ip.is_private or ip.is_loopback or ip.is_link_local + or ip.is_reserved or ip.is_multicast or ip.is_unspecified): + return True + return False - return {"ok": True} + +def _fetch_art_url(url: str) -> bytes: + """The one place art-by-URL touches the network (tests fake this seam). + User-initiated, so not throttled like the background workers — but the + same offline guard applies (pytest can never fetch), the host is checked + against internal/reserved ranges (SSRF), redirects are NOT followed (a + redirect can't smuggle the request to an internal target), and the size + cap is enforced while streaming so a huge response never fully downloads. + + Residual, accepted: the host is resolved here and again by requests, so a + rebinding DNS name is a theoretical TOCTOU. Not closed with an IP-pinned + connection because (a) this is a single-user, no-auth app (constitution + §I) and the route is demo-blocked, so there is no untrusted submission + path, and (b) no other in-tree client (MusicBrainz, CAA) pins either — a + bespoke pinned+SNI adapter here would be inconsistent and disproportionate. + The cheap guards above still stop the realistic vectors (direct internal + URL, redirect-to-internal).""" + if not _enrich_network_enabled(): + raise EnrichTransportError("art fetch disabled (offline)") + if _url_host_is_internal(url): + raise ValueError("url host is not allowed") + import requests + try: + with requests.get(url, timeout=15, stream=True, allow_redirects=False, + headers={"User-Agent": _enrich_user_agent()}) as resp: + if resp.status_code != 200: + raise EnrichTransportError(f"HTTP {resp.status_code}") + data = b"" + for chunk in resp.iter_content(65536): + data += chunk + if len(data) > _ART_URL_MAX_BYTES: + raise ValueError("image larger than 10 MB") + return data + except requests.RequestException as e: + raise EnrichTransportError(str(e)) from e + + +@app.post("/api/song/{filename:path}/art/url") +def set_song_art_from_url(filename: str, data: dict): + """Paste-a-link cover art (the media-server idiom): the server fetches the + image and stores it as this song's local override — identical result to an + upload, including the GIF-stays-local rule. http(s) only.""" + url = str((data or {}).get("url") or "").strip() + from urllib.parse import urlparse + parsed = urlparse(url) + if parsed.scheme not in ("http", "https") or not parsed.hostname: + raise HTTPException(status_code=400, detail="url must be http(s)") + dlc = _get_dlc_dir() + song_path = _resolve_dlc_path(dlc, filename) if dlc else None + if song_path is None or not song_path.exists(): + raise HTTPException(status_code=404, detail="unknown song") + try: + img_data = _fetch_art_url(url) + except EnrichTransportError as e: + return JSONResponse({"error": "could not fetch image", "detail": str(e)}, + status_code=502) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) + return _save_art_override(filename, img_data) + + +@app.delete("/api/art/{filename:path}/override") +def remove_song_art_override(filename: str): + """Drop the user art override — the serve chain falls back to pack art, + then the Cover Art Archive cache. Lives under /api/art (NOT /api/song) so + the greedy DELETE /api/song/{path} catch-all can't shadow it — the same + dodge the chart split/unsplit routes use.""" + removed = False + for p in _art_override_paths(filename): + try: + p.unlink() + removed = True + except OSError: + pass + if removed: + # The art worker may have settled this row as 'user' (override present, + # no pack art). Reset it so the next enrichment pass re-evaluates and the + # CAA fallback resumes — otherwise a removed override strands the row + # (enrichment_art_pending only re-queues art_state IS NULL) and the song + # is left with no art at all. + try: + meta_db.set_enrichment_art(filename, None, None) + except Exception: + log.exception("art override delete: failed to reset enrichment state") + return {"ok": True, "removed": removed} @app.get("/api/song/{filename:path}") diff --git a/tests/test_art_layer.py b/tests/test_art_layer.py new file mode 100644 index 0000000..03d7744 --- /dev/null +++ b/tests/test_art_layer.py @@ -0,0 +1,318 @@ +"""Tests for the R3 art layer: the user>pack>CAA serve chain, user overrides +(upload / URL / delete, with GIF kept animated and LOCAL-ONLY), and the +enrichment art worker's Cover Art Archive fetch + LRU cache. + +Both network seams (`_caa_http_get`, `_fetch_art_url`) are faked — nothing +here opens a socket, and the offline default is itself asserted. +""" + +import importlib +import io as _io +import sys + +import pytest +from fastapi.testclient import TestClient +from PIL import Image + + +@pytest.fixture() +def server(tmp_path, monkeypatch, isolate_logging): + monkeypatch.setenv("CONFIG_DIR", str(tmp_path / "config")) + dlc = tmp_path / "dlc" + dlc.mkdir() + monkeypatch.setenv("DLC_DIR", str(dlc)) + monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1") + sys.modules.pop("server", None) + srv = importlib.import_module("server") + try: + yield srv + finally: + conn = getattr(getattr(srv, "meta_db", None), "conn", None) + if conn is not None: + conn.close() + sys.modules.pop("server", None) + + +@pytest.fixture() +def client(server): + return TestClient(server.app) + + +def png_bytes(color=(200, 30, 30)): + buf = _io.BytesIO() + Image.new("RGB", (4, 4), color).save(buf, "PNG") + return buf.getvalue() + + +def gif_bytes(): + """A tiny 2-frame animated GIF.""" + buf = _io.BytesIO() + frames = [Image.new("P", (4, 4), i) for i in (0, 255)] + frames[0].save(buf, "GIF", save_all=True, append_images=frames[1:], duration=100) + return buf.getvalue() + + +def make_sloppak(server, name, with_cover=False, title="Song", artist="Artist"): + d = server.DLC_DIR / name + d.mkdir(parents=True) + (d / "manifest.yaml").write_text( + f"title: {title}\nartist: {artist}\nduration: 100\n" + "arrangements: []\nstems: []\n", encoding="utf-8") + if with_cover: + (d / "cover.jpg").write_bytes(png_bytes((10, 200, 10))) + server.meta_db.put(name, 0, 0, { + "title": title, "artist": artist, "album": "", "year": "", + "duration": 100, "arrangements": [{"name": "Lead", "index": 0}], + }) + return d + + +def b64(data): + import base64 + return base64.b64encode(data).decode() + + +# ── the serve chain: user > pack > CAA ──────────────────────────────────────── + +def test_user_override_beats_pack_art(server, client): + make_sloppak(server, "a.sloppak", with_cover=True) + r = client.get("/api/song/a.sloppak/art") + assert r.status_code == 200 # pack art serves + assert client.post("/api/song/a.sloppak/art/upload", + json={"image": b64(png_bytes((1, 2, 3)))}).json()["ok"] + r = client.get("/api/song/a.sloppak/art") + assert r.status_code == 200 + assert r.headers["content-type"] == "image/png" # the override, not the pack jpeg + # Removing the override falls back to pack art. (The route lives under + # /api/art — the DELETE /api/song/{path} catch-all would shadow it.) + assert client.delete("/api/art/a.sloppak/override").json()["removed"] + r = client.get("/api/song/a.sloppak/art") + assert r.status_code == 200 + assert r.headers["content-type"] == "image/jpeg" + + +def test_gif_override_kept_animated_and_local_only(server, client): + make_sloppak(server, "a.sloppak", with_cover=True) + raw = gif_bytes() + body = client.post("/api/song/a.sloppak/art/upload", json={"image": b64(raw)}).json() + assert body == {"ok": True, "kind": "gif"} + r = client.get("/api/song/a.sloppak/art") + assert r.headers["content-type"] == "image/gif" + assert r.content == raw # VERBATIM — animation intact + # …and the pack file was never touched (GIF never reaches the feedpak). + assert (server.DLC_DIR / "a.sloppak" / "cover.jpg").read_bytes() == png_bytes((10, 200, 10)) + assert not (server.DLC_DIR / "a.sloppak" / "cover.gif").exists() + # A later PNG upload replaces the GIF (one override per song). + client.post("/api/song/a.sloppak/art/upload", json={"image": b64(png_bytes())}) + assert client.get("/api/song/a.sloppak/art").headers["content-type"] == "image/png" + assert len(server._art_override_paths("a.sloppak")) == 1 + + +def test_bad_upload_rejected(server, client): + make_sloppak(server, "a.sloppak") + assert "error" in client.post("/api/song/a.sloppak/art/upload", + json={"image": b64(b"GIF89a not really a gif")}).json() + assert "error" in client.post("/api/song/a.sloppak/art/upload", + json={"image": b64(b"plain text")}).json() + + +# ── art by URL ──────────────────────────────────────────────────────────────── + +def test_art_url_fetches_and_overrides(server, client, monkeypatch): + make_sloppak(server, "a.sloppak", with_cover=True) + monkeypatch.setattr(server, "_fetch_art_url", lambda url: png_bytes((9, 9, 9))) + body = client.post("/api/song/a.sloppak/art/url", + json={"url": "https://example.com/cover.png"}).json() + assert body == {"ok": True, "kind": "png"} + assert client.get("/api/song/a.sloppak/art").headers["content-type"] == "image/png" + + +def test_art_url_validation(server, client, monkeypatch): + make_sloppak(server, "a.sloppak") + assert client.post("/api/song/a.sloppak/art/url", + json={"url": "ftp://example.com/x.png"}).status_code == 400 + assert client.post("/api/song/a.sloppak/art/url", json={}).status_code == 400 + assert client.post("/api/song/ghost.sloppak/art/url", + json={"url": "https://example.com/x.png"}).status_code == 404 + # Oversize → 400 (the seam raises ValueError at the cap). + def _huge(url): + raise ValueError("image larger than 10 MB") + monkeypatch.setattr(server, "_fetch_art_url", _huge) + assert client.post("/api/song/a.sloppak/art/url", + json={"url": "https://example.com/x.png"}).status_code == 400 + + +def test_art_url_offline_by_default(server, client): + """The real fetch seam refuses under the test env — pytest can never + reach the network even when a test forgets to fake it.""" + make_sloppak(server, "a.sloppak") + r = client.post("/api/song/a.sloppak/art/url", + json={"url": "https://example.com/x.png"}) + assert r.status_code == 502 + + +# ── the enrichment art worker (Cover Art Archive) ───────────────────────────── + +def _match_row(server, fn, release_id="rel-1"): + """Seed a matched enrichment row with a release id (as the P8 matcher + would have written) so the art worker picks it up.""" + song = server.meta_db.enrichment_song_row(fn) + h = server.meta_db.enrichment_content_hash( + song["artist"], song["title"], song["album"], song["duration"]) + server.meta_db.apply_enrichment_match( + fn, h, "matched", source="text", score=1.0, + cand={"recording_id": "rec-1", "release_id": release_id, + "title": song["title"], "artist": song["artist"]}) + + +@pytest.fixture() +def caa(server, monkeypatch): + """Fake CAA transport + network flag on (mirrors the P8 test fixture).""" + calls = [] + art = {"rel-1": png_bytes((60, 60, 200))} + + def fake(release_id): + calls.append(release_id) + return art.get(release_id) + fake.calls, fake.art = calls, art + monkeypatch.setattr(server, "_caa_http_get", fake) + monkeypatch.setattr(server, "_enrich_network_enabled", lambda: True) + return fake + + +def test_caa_fetch_fills_missing_art(server, client, caa): + make_sloppak(server, "a.sloppak") # no pack art + _match_row(server, "a.sloppak") + server._background_enrich() + row = server.meta_db.get_enrichment("a.sloppak") + assert row["art_state"] == "caa" + assert row["art_cache_path"] and row["art_cache_path"].endswith("caa_rel-1.jpg") + r = client.get("/api/song/a.sloppak/art") + assert r.status_code == 200 + assert r.headers["content-type"] == "image/jpeg" + # Settled: the next pass never re-fetches. + n = len(caa.calls) + server._background_enrich() + assert len(caa.calls) == n + + +def test_caa_skips_pack_art_and_dedupes_by_release(server, caa): + make_sloppak(server, "haspack.sloppak", with_cover=True, title="One") + make_sloppak(server, "b.sloppak", title="Two") + make_sloppak(server, "c.sloppak", title="Three") + _match_row(server, "haspack.sloppak") + _match_row(server, "b.sloppak") # same release as c + _match_row(server, "c.sloppak") + server._background_enrich() + assert server.meta_db.get_enrichment("haspack.sloppak")["art_state"] == "pack" + assert server.meta_db.get_enrichment("b.sloppak")["art_state"] == "caa" + assert server.meta_db.get_enrichment("c.sloppak")["art_state"] == "caa" + assert len(caa.calls) == 1 # one release → ONE fetch + + +def test_caa_404_marks_none(server, caa): + make_sloppak(server, "a.sloppak") + _match_row(server, "a.sloppak", release_id="rel-missing") + server._background_enrich() + assert server.meta_db.get_enrichment("a.sloppak")["art_state"] == "none" + n = len(caa.calls) + server._background_enrich() + assert len(caa.calls) == n # never re-hammered + + +def test_caa_transport_error_leaves_row_unevaluated(server, caa, monkeypatch): + make_sloppak(server, "a.sloppak") + _match_row(server, "a.sloppak") + + def _down(release_id): + raise server.EnrichTransportError("down") + monkeypatch.setattr(server, "_caa_http_get", _down) + server._background_enrich() + assert server.meta_db.get_enrichment("a.sloppak")["art_state"] is None + # Network back → next pass completes it. + monkeypatch.setattr(server, "_caa_http_get", caa) + server._background_enrich() + assert server.meta_db.get_enrichment("a.sloppak")["art_state"] == "caa" + + +def test_offline_default_skips_art_worker(server, monkeypatch): + """Under the plain test env the whole art phase is skipped with the rest + of the network work.""" + calls = [] + monkeypatch.setattr(server, "_caa_http_get", lambda rid: calls.append(rid)) + make_sloppak(server, "a.sloppak") + _match_row(server, "a.sloppak") + server._background_enrich() + assert calls == [] + assert server.meta_db.get_enrichment("a.sloppak")["art_state"] is None + + +def test_lru_prune_evicts_oldest_and_resets_rows(server, caa, monkeypatch): + monkeypatch.setattr(server, "_CAA_CACHE_CAP_BYTES", 1) # everything over cap + make_sloppak(server, "a.sloppak", title="One") + make_sloppak(server, "b.sloppak", title="Two") + caa.art["rel-2"] = png_bytes((1, 1, 1)) + _match_row(server, "a.sloppak", release_id="rel-1") + _match_row(server, "b.sloppak", release_id="rel-2") + server._background_enrich() + # With a 1-byte cap every fetch immediately evicts — the rows that pointed + # at evicted files were reset to unevaluated. + caa_files = list(server.ART_CACHE_DIR.glob("caa_*.jpg")) + assert len(caa_files) <= 1 + states = {fn: server.meta_db.get_enrichment(fn)["art_state"] + for fn in ("a.sloppak", "b.sloppak")} + assert None in states.values() or list(states.values()).count("caa") <= 1 + + +def test_delete_song_removes_override(server, client): + make_sloppak(server, "a.sloppak") + client.post("/api/song/a.sloppak/art/upload", json={"image": b64(png_bytes())}) + assert server._art_override_paths("a.sloppak") + r = client.delete("/api/song/a.sloppak") + assert r.status_code == 200 + assert server._art_override_paths("a.sloppak") == [] + + +def test_delete_override_restores_caa_fallback(server, client, caa): + """Removing a user override that had settled the row as 'user' must reset + the enrichment state so the CAA fallback is fetched and served again — + otherwise the song is stranded with no art at all.""" + make_sloppak(server, "a.sloppak") # no pack art + _match_row(server, "a.sloppak") + # Pin an override BEFORE the art worker runs → the pass stamps art_state='user'. + client.post("/api/song/a.sloppak/art/upload", json={"image": b64(png_bytes())}) + server._background_enrich() + assert server.meta_db.get_enrichment("a.sloppak")["art_state"] == "user" + # Remove it → the row resets to unevaluated… + assert client.delete("/api/art/a.sloppak/override").json()["removed"] + assert server.meta_db.get_enrichment("a.sloppak")["art_state"] is None + # …and the next pass fetches + serves the release's front cover. + server._background_enrich() + assert server.meta_db.get_enrichment("a.sloppak")["art_state"] == "caa" + r = client.get("/api/song/a.sloppak/art") + assert r.status_code == 200 + assert r.headers["content-type"] == "image/jpeg" + + +def test_upload_rejects_unknown_song_and_oversize(server, client): + # Unknown filename → 404 (no stray override file written). + assert client.post("/api/song/ghost.sloppak/art/upload", + json={"image": b64(png_bytes())}).status_code == 404 + assert server._art_override_paths("ghost.sloppak") == [] + # Oversize decoded payload → 400 (bounds the base64 upload path). + make_sloppak(server, "a.sloppak") + huge = b64(b"\x00" * (server._ART_URL_MAX_BYTES + 1)) + assert client.post("/api/song/a.sloppak/art/upload", + json={"image": huge}).status_code == 400 + + +def test_fetch_art_url_blocks_internal_hosts(server): + """The SSRF guard refuses loopback / link-local / private targets before + any request is made (the real seam, not the faked one).""" + assert server._url_host_is_internal("http://127.0.0.1/x.png") + assert server._url_host_is_internal("http://localhost/x.png") + assert server._url_host_is_internal("http://169.254.169.254/latest/meta-data") + assert server._url_host_is_internal("http://10.0.0.5/x.png") + assert server._url_host_is_internal("http://[::1]/x.png") + assert server._url_host_is_internal("http://nonexistent.invalid/x.png") # unresolvable → closed + assert not server._url_host_is_internal("http://93.184.216.34/x.png") # public literal diff --git a/tests/test_demo_mode.py b/tests/test_demo_mode.py index 390eb1b..b58f2b8 100644 --- a/tests/test_demo_mode.py +++ b/tests/test_demo_mode.py @@ -101,6 +101,11 @@ def test_demo_off_settings_post_not_blocked(tmp_path, monkeypatch): # Context menus (R2): per-song re-match + the path-exposing Get info. ("POST", "/api/enrichment/refresh/some-file"), ("GET", "/api/chart/some-file/fileinfo"), + # Art layer (R3): the base64 upload writes files, the server-side URL fetch + # touches the network, and the override delete removes files — all mutations. + ("POST", "/api/song/some-file/art/upload"), + ("POST", "/api/song/some-file/art/url"), + ("DELETE", "/api/art/some-file/override"), ]) def test_demo_on_blocked_routes_return_403(tmp_path, monkeypatch, method, path): server, client = _make_client(tmp_path, monkeypatch, demo=True)