From 89b438974f4d67d4f66d78518ce335c7c8b57c59 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 2 Jun 2026 11:01:28 -0700 Subject: [PATCH] Fix #766: Navidrome album covers blank in the sync editor (+ other modals) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sync editor renders server covers as , but no Flask route ever served that path — so every Navidrome cover 404'd, on every album, art or not. The source (left) side then went blank too: a source row with no native art (e.g. YouTube, which provides none at mirror time) falls back to borrowing the matched server track's cover — i.e. that same dead route. So both sides collapsed to nothing. Fix: - New NavidromeClient.build_cover_art_url(cover_id) — builds the absolute, Subsonic-authenticated getCoverArt URL (base_url + token/salt), keeping credentials server-side. Uses a FIXED cover-art salt so the URL is deterministic for a given (server, password, cover_id): a rotating salt (as in _generate_auth_params) would make every request a unique URL → image-cache miss every time + a dead, never-reused cache row per fetch. Token auth doesn't require a unique salt, and the password is never exposed (only its salted md5). - New route /api/navidrome/cover/ — resolves that URL and streams the image through the shared image cache (same pattern as /api/image-proxy), with a private max-age so the browser caches by the stable route URL. Effect: server side works for any album that has art in Navidrome; matched source rows with no native art now borrow the (now-working) server cover. Unmatched YouTube rows stay blank — no image exists anywhere to show. Tests: tests/test_navidrome_cover_url.py (8) — URL structure + salted-token auth (never the raw password), determinism (same id -> same URL so the cache hits; different id/password -> different URL), optional size, and the not-connected / no-id / no-credentials guards. Caveats: not executed against a live Navidrome (no server in CI) — the URL builder is unit-tested; the route's cache→HTTP→bytes round-trip is read-verified only. Scope is the sync editor's Navidrome route; Plex/Jellyfin server-cover branches and any other modals using a different mechanism are untouched. --- core/navidrome_client.py | 37 ++++++++++++++ tests/test_navidrome_cover_url.py | 83 +++++++++++++++++++++++++++++++ web_server.py | 27 ++++++++++ 3 files changed, 147 insertions(+) create mode 100644 tests/test_navidrome_cover_url.py diff --git a/core/navidrome_client.py b/core/navidrome_client.py index 7ec9d286..765153bc 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -3,6 +3,7 @@ import hashlib import secrets from typing import List, Optional, Dict, Any from datetime import datetime +from urllib.parse import urlencode import json from utils.logging_config import get_logger from config.settings import config_manager @@ -316,6 +317,42 @@ class NavidromeClient(MediaServerClient): 'f': 'json' # Response format } + # Fixed salt for cover-art URLs ONLY. Subsonic token auth (t=md5(password + # +salt), s=salt) does not require a unique salt per request, and a stable + # one makes the cover URL deterministic — so the image cache and the + # browser cache actually HIT. The rotating salt from _generate_auth_params + # would make every request a unique URL → cache miss every time + a dead, + # never-reused cache row per fetch (#766 review). The password is never + # exposed either way (only its salted md5). + _COVER_ART_SALT = 'soulsync-cover' + + def build_cover_art_url(self, cover_id, size=None) -> Optional[str]: + """Absolute, Subsonic-authenticated getCoverArt URL for ``cover_id``. + + Deterministic for a given (server, password, cover_id) so it caches. + The web layer proxies this to the browser (sync editor + modals). + Returns ``None`` when not connected or no id was supplied. #766: the + ``/api/navidrome/cover/`` route had no working URL behind it, so + every Navidrome cover came back blank.""" + if not self.base_url or not cover_id: + return None + if not self.username or not self.password: + return None + salt = self._COVER_ART_SALT + token = hashlib.md5((self.password + salt).encode()).hexdigest() + params = { + 'u': self.username, + 't': token, + 's': salt, + 'v': '1.16.1', + 'c': 'SoulSync', + 'f': 'json', # harmless for getCoverArt — it returns image binary + 'id': str(cover_id), + } + if size: + params['size'] = str(size) + return f"{self.base_url}/rest/getCoverArt?{urlencode(params)}" + # Subsonic endpoints that modify data — use POST to avoid URL length limits _WRITE_ENDPOINTS = frozenset({ 'createPlaylist', 'updatePlaylist', 'deletePlaylist', diff --git a/tests/test_navidrome_cover_url.py b/tests/test_navidrome_cover_url.py new file mode 100644 index 00000000..67894bbb --- /dev/null +++ b/tests/test_navidrome_cover_url.py @@ -0,0 +1,83 @@ +"""Tests for Navidrome cover-art URL building (#766). + +The sync editor + modals referenced /api/navidrome/cover/ but no route +served it, and the URL behind it had to be a fully-authenticated Subsonic +getCoverArt URL. build_cover_art_url is that builder — these pin its shape and +the not-connected guards (the token/salt are random per call, so we assert +structure + required params rather than an exact string). +""" + +from __future__ import annotations + +from urllib.parse import parse_qs, urlsplit + +from core.navidrome_client import NavidromeClient + + +def _connected_client(): + c = NavidromeClient() + c.base_url = "https://nav.example.com" + c.username = "boulder" + c.password = "hunter2" + return c + + +def test_builds_authenticated_cover_url(): + url = _connected_client().build_cover_art_url("al-123") + parts = urlsplit(url) + assert parts.scheme == "https" + assert parts.netloc == "nav.example.com" + assert parts.path == "/rest/getCoverArt" + q = parse_qs(parts.query) + assert q["id"] == ["al-123"] + assert q["u"] == ["boulder"] + # Subsonic token auth: salted md5, never the raw password. + assert q["t"] and q["t"][0] != "hunter2" + assert q["s"] # salt present + assert "hunter2" not in url + for required in ("t", "s", "v", "c"): + assert required in q + + +def test_cover_url_is_deterministic_so_the_cache_hits(): + # #766 review: the URL must be stable for a given (server, password, + # cover_id) — otherwise the image cache keys on a rotating salt and misses + # every request, re-fetching Navidrome each time + leaking dead rows. + c = _connected_client() + assert c.build_cover_art_url("al-123") == c.build_cover_art_url("al-123") + # ...and different covers still produce different URLs. + assert c.build_cover_art_url("al-123") != c.build_cover_art_url("al-999") + + +def test_cover_url_changes_with_password(): + # A password change must invalidate the cached URL (new token). + c1 = _connected_client() + c2 = _connected_client() + c2.password = "different" + assert c1.build_cover_art_url("al-1") != c2.build_cover_art_url("al-1") + + +def test_size_param_optional(): + assert "size" not in (_connected_client().build_cover_art_url("x") or "") + assert "size=300" in _connected_client().build_cover_art_url("x", size=300) + + +def test_cover_id_is_stringified(): + url = _connected_client().build_cover_art_url(12345) + assert "id=12345" in url + + +def test_returns_none_when_not_connected(): + c = NavidromeClient() # base_url is None + assert c.build_cover_art_url("al-1") is None + + +def test_returns_none_for_empty_cover_id(): + assert _connected_client().build_cover_art_url("") is None + assert _connected_client().build_cover_art_url(None) is None + + +def test_returns_none_without_credentials(): + c = NavidromeClient() + c.base_url = "https://nav.example.com" # but no username/password + assert c.build_cover_art_url("al-1") is None diff --git a/web_server.py b/web_server.py index dbb363e1..2849ac1c 100644 --- a/web_server.py +++ b/web_server.py @@ -4133,6 +4133,33 @@ def select_jellyfin_music_library(): logger.error(f"Error setting Jellyfin music library: {e}") return jsonify({"success": False, "error": str(e)}), 500 +@app.route('/api/navidrome/cover/', methods=['GET']) +def navidrome_cover(cover_id): + """Proxy a Navidrome (Subsonic) cover-art image to the browser. + + The sync editor and other modals reference /api/navidrome/cover/, + but no route served it — so every Navidrome cover came back blank (#766). + We build the authenticated getCoverArt URL server-side (keeping Subsonic + credentials off the client) and stream it through the shared image cache. + """ + try: + client = media_server_engine.client('navidrome') + if not client: + return '', 404 + url = client.build_cover_art_url(cover_id) + if not url: + return '', 404 + from core.image_cache import get_image_cache + cached = get_image_cache().get_url(url) + response = send_file(cached.path, mimetype=cached.mime_type, conditional=True) + max_age = int(config_manager.get("image_cache.ttl_seconds", 2592000)) + response.headers['Cache-Control'] = f'private, max-age={max_age}' + return response + except Exception as exc: + logger.debug("navidrome cover proxy failed for %s: %s", cover_id, exc) + return '', 502 + + @app.route('/api/navidrome/music-folders', methods=['GET']) def get_navidrome_music_folders(): """Get list of available music folders from Navidrome"""