From b36392c62bfa44e70513bfd94727ac11a1db5466 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 16:29:09 -0700 Subject: [PATCH 01/25] Fix: a '/' in a song title was treated as a path separator (#835) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit YouTube/Tidal/Qobuz results encode the name as ``id||title``. When the title itself contains a '/' (e.g. the Sawano AoT track "YouSeeBIGGIRL/T:T"), two places wrongly basename-split it on the slash and kept only the last segment ("T:T"): - core/downloads/file_finder.py — the completed-download finder truncated its search target to "T:T", so the real on-disk file (slash sanitised by the writer) never matched → "not found after processing" → the download got QUARANTINED. Now an encoded ``id||title`` keeps the whole title as the target and contributes no remote-directory components; real Soulseek PATHS still get basename + dir extraction unchanged. - webui/static/downloads.js — the manual-search FILE column showed only "T:T". Added a ``||``-aware short-label helper (mirrors the correct handling already used elsewhere in the file); real file paths still show their basename. Tests: the finder locates "YouSeeBIGGIRL∕T: T.mp3" from the encoded title "…||YouSeeBIGGIRL/T:T" (the screenshot case), doesn't match an unrelated file, and a genuine Soulseek path still resolves to its last segment. 21 finder tests + 64 script-split integrity tests pass. --- core/downloads/file_finder.py | 33 ++++++++++++------ tests/downloads/test_file_finder.py | 53 +++++++++++++++++++++++++++++ webui/static/downloads.js | 13 ++++++- 3 files changed, 87 insertions(+), 12 deletions(-) diff --git a/core/downloads/file_finder.py b/core/downloads/file_finder.py index 98aa4a66..0e6e03a7 100644 --- a/core/downloads/file_finder.py +++ b/core/downloads/file_finder.py @@ -83,15 +83,17 @@ def _normalize_for_finding(text: str) -> str: def _extract_basename(api_filename: str) -> str: - """Cross-platform rightmost-separator split, with YouTube / - Tidal ``id||title`` encoded filenames pre-normalised — the id - half is stripped so the title becomes the basename. Mirrors - the strip-then-split order ``web_server`` used.""" + """Cross-platform rightmost-separator split for a real remote PATH. + + A YouTube/Tidal/Qobuz ``id||title`` encoded filename is handled by + returning the title VERBATIM: the title is not a filesystem path, so a '/' + in it (e.g. the Sawano track ``YouSeeBIGGIRL/T:T``) is part of the name and + must NOT be split on (issue #835).""" if not api_filename: return "" if '||' in api_filename: _id, title = api_filename.split('||', 1) - api_filename = title + return title last_slash = max(api_filename.rfind('/'), api_filename.rfind('\\')) return api_filename[last_slash + 1:] if last_slash != -1 else api_filename @@ -236,14 +238,23 @@ def find_completed_audio_file( ``None`` when the file isn't found anywhere — callers should treat that as "not yet" (still mid-write) or "lost". """ - # YouTube / Tidal encoded filenames carry the id ahead of ``||``. - # Strip it up front so basename + dir-component extraction both - # operate on the title half. + # YouTube / Tidal / Qobuz encoded filenames carry the id ahead of ``||``. + # The title half is NOT a filesystem path: a '/' in it (e.g. the Sawano + # track ``YouSeeBIGGIRL/T:T``) is part of the title, so it must NOT be + # basename-split or read as a remote directory component — doing so + # truncated the search target to ``T:T`` and the real file was never found, + # quarantining valid downloads (issue #835). Real remote paths (Soulseek) + # still get basename + dir-component extraction. + encoded_title = None if api_filename and '||' in api_filename: - _id, api_filename = api_filename.split('||', 1) - target_basename = _extract_basename(api_filename) + _id, encoded_title = api_filename.split('||', 1) + if encoded_title is not None: + target_basename = encoded_title + api_dirs = [] + else: + target_basename = _extract_basename(api_filename) + api_dirs = _api_dir_parts(api_filename) normalized_target = _normalize_for_finding(target_basename) - api_dirs = _api_dir_parts(api_filename) best_dl_path, dl_sim = _search_in_directory( download_dir, 'downloads', target_basename, normalized_target, api_dirs, diff --git a/tests/downloads/test_file_finder.py b/tests/downloads/test_file_finder.py index a012faa3..5e7d6291 100644 --- a/tests/downloads/test_file_finder.py +++ b/tests/downloads/test_file_finder.py @@ -343,3 +343,56 @@ def test_fuzzy_rejects_low_similarity(tmp_path): if __name__ == '__main__': pytest.main([__file__, '-v']) + + +# --------------------------------------------------------------------------- +# Issue #835 — a '/' in a YouTube/Tidal title is part of the NAME, not a path +# separator. The encoded ``id||title`` finder previously basename-split the +# title ("YouSeeBIGGIRL/T:T" -> "T:T"), so the real on-disk file (with the +# slash sanitised) never matched and valid downloads got quarantined. +# --------------------------------------------------------------------------- + +from core.downloads.file_finder import _extract_basename + + +def test_encoded_title_with_slash_is_not_basename_split(): + # The Sawano AoT track. The id||title encoding must keep the whole title. + assert _extract_basename('vy63u2hKoPE||YouSeeBIGGIRL/T:T') == 'YouSeeBIGGIRL/T:T' + + +def test_finds_youtube_file_whose_title_contains_a_slash(tmp_path): + downloads = tmp_path / 'downloads' + # On disk the slash is sanitised to a look-alike and the colon spaced out, + # exactly as in the issue screenshot. + target = downloads / 'YouSeeBIGGIRL∕T: T.mp3' + _touch(target) + + found, location = find_completed_audio_file( + str(downloads), 'vy63u2hKoPE||YouSeeBIGGIRL/T:T', + ) + + assert found == str(target), 'pre-#835 this truncated the target to "T:T" and missed the file' + assert location == 'downloads' + + +def test_slash_title_does_not_match_an_unrelated_file(tmp_path): + # Guard against the fix being too loose: a different track must NOT match. + downloads = tmp_path / 'downloads' + _touch(downloads / 'Some Totally Different Song.mp3') + + found, _ = find_completed_audio_file( + str(downloads), 'vy63u2hKoPE||YouSeeBIGGIRL/T:T', + ) + assert found is None + + +def test_real_soulseek_path_still_basenamed(tmp_path): + # Regression: a genuine remote PATH must still resolve to its last segment. + downloads = tmp_path / 'downloads' + target = downloads / '01 - Suddenly.flac' + _touch(target) + assert _extract_basename(r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac') == '01 - Suddenly.flac' + found, _ = find_completed_audio_file( + str(downloads), r'shared\Billy Ocean\Very Best Of\01 - Suddenly.flac', + ) + assert found == str(target) diff --git a/webui/static/downloads.js b/webui/static/downloads.js index aeca1bd1..3faa2cc6 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -3060,8 +3060,19 @@ function _candidatesFmtDur(ms) { // rows (different click binding scope). ``showSourceBadge`` adds a small // per-row source pill — used in hybrid "All sources" mode where the user // otherwise can't tell which source a row came from. +// Display label for a candidate's filename. Encoded ``id||title`` sources +// (youtube/tidal/qobuz/hifi) carry the title after ``||`` — a '/' in that title +// is part of the name, NOT a path separator, so it must not be basename-split +// (issue #835: "YouSeeBIGGIRL/T:T" was showing as just "T:T"). Real file paths +// (Soulseek) keep the rightmost-segment basename. +function _ssShortFileLabel(filename) { + if (!filename) return '-'; + if (filename.includes('||')) return filename.split('||').slice(1).join('||'); + return filename.split(/[/\\]/).pop(); +} + function _renderCandidateRow(c, index, rowClass, showSourceBadge) { - const shortFile = c.filename ? c.filename.split(/[/\\]/).pop() : '-'; + const shortFile = _ssShortFileLabel(c.filename); const qBadge = c.quality ? `${c.quality.toUpperCase()}` : ''; From 1517794e230d43c56a96ea47e5cb381b94608a27 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 16:55:08 -0700 Subject: [PATCH 02/25] Fix: manual Find & Add recreated the Jellyfin/Emby playlist (#837) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Automations + auto-sync respect 'append' mode and preserve a server playlist's description + cover image, but manually matching a missing track ("Find & add") recreated the whole playlist and wiped them. Root cause: the add-track endpoint's Jellyfin branch called `update_playlist()`, which deletes + recreates the playlist on Jellyfin/Emby. Switched it to the purpose-built `append_to_playlist([the one found track])` — the same in-place, dedupe-safe op the 'append' sync mode already uses — so the playlist (and its description/image) is preserved and only the missing track is added. append_to_playlist reads `.id` off the track, so the endpoint now sets it (it previously only set ratingKey). Plex (in-place addItems) and Navidrome (in-place Subsonic updatePlaylist) were already non-destructive; Emby routes through the jellyfin branch, so this covers it too. Tests: the add-track endpoint appends in place and never calls update_playlist; a link-to-existing-track touches nothing. 18 tests pass (incl. the existing append-mode suite). --- ...t_issue_837_find_add_preserves_playlist.py | 94 +++++++++++++++++++ web_server.py | 16 +++- 2 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 tests/test_issue_837_find_add_preserves_playlist.py diff --git a/tests/test_issue_837_find_add_preserves_playlist.py b/tests/test_issue_837_find_add_preserves_playlist.py new file mode 100644 index 00000000..8ee5a2d2 --- /dev/null +++ b/tests/test_issue_837_find_add_preserves_playlist.py @@ -0,0 +1,94 @@ +"""#837 — manual Find & Add must NOT recreate the Jellyfin/Emby playlist. + +Reporter (carlosjfcasero, Emby/Jellyfin): automations + auto-sync respect the +'append' sync mode and preserve the playlist's description/image, but manually +matching a missing track ("Find & add") recreated the whole playlist and wiped +them. Root cause: the add-track endpoint's Jellyfin branch called +`update_playlist(full list)` (delete + recreate) instead of the in-place +`append_to_playlist`. These pin that the endpoint now appends in place. + +(Emby routes through the 'jellyfin' branch — no separate emby branch exists.) +""" + +from __future__ import annotations + +import os +import tempfile +from types import SimpleNamespace + +import pytest + +_TMP = tempfile.mkdtemp(prefix='soulsync-testdb-837-') +os.environ['DATABASE_PATH'] = os.path.join(_TMP, 'i837.db') +os.environ['SOULSYNC_TEST_DB_READY'] = '1' + +web_server = pytest.importorskip('web_server') + +_GUID = 'aaaaaaaa-bbbb-cccc-dddd-000000000001' + + +class _FakeJellyfin: + def __init__(self, existing=None): + self.existing = existing or [] + self.append_calls = [] + self.update_calls = [] + + def get_playlist_tracks(self, pid): + return [SimpleNamespace(ratingKey=str(r)) for r in self.existing] + + def append_to_playlist(self, name, tracks): + self.append_calls.append((name, [getattr(t, 'id', None) for t in tracks])) + return True + + def update_playlist(self, name, tracks): # the destructive recreate path + self.update_calls.append((name, tracks)) + return True + + +class _FakeEngine: + def __init__(self, jf): + self._jf = jf + + def client(self, name): + return self._jf if name == 'jellyfin' else None + + +@pytest.fixture +def client(): + return web_server.app.test_client() + + +def _wire(monkeypatch, jf): + monkeypatch.setattr(web_server, 'media_server_engine', _FakeEngine(jf)) + monkeypatch.setattr(web_server.config_manager, 'get_active_media_server', lambda: 'jellyfin') + # the durable source->server match write touches the DB; not under test here + monkeypatch.setattr(web_server, '_persist_find_and_add_match', lambda *a, **k: None) + + +def test_find_and_add_appends_in_place_not_recreate(client, monkeypatch): + jf = _FakeJellyfin(existing=[]) # the missing track isn't on the server yet + _wire(monkeypatch, jf) + + resp = client.post('/api/server/playlist/PL1/add-track', + json={'track_id': _GUID, 'playlist_name': 'Disney'}) + body = resp.get_json() + + assert body['success'] and body['message'] == 'Track added' + assert len(jf.append_calls) == 1, 'should append in place' + assert jf.update_calls == [], 'must NOT recreate the playlist (#837)' + # append_to_playlist reads `.id` off the track — the endpoint must set it + assert jf.append_calls[0][1] == [_GUID] + + +def test_find_and_add_link_to_existing_track_touches_nothing(client, monkeypatch): + # Matching a source to a track already in the playlist is a LINK, not an add. + jf = _FakeJellyfin(existing=[_GUID]) + _wire(monkeypatch, jf) + + resp = client.post('/api/server/playlist/PL1/add-track', + json={'track_id': _GUID, 'playlist_name': 'Disney', + 'source_track_id': 'spotify-xyz'}) + body = resp.get_json() + + assert body['success'] and body['message'] == 'Track linked' + assert jf.append_calls == [] and jf.update_calls == [] diff --git a/web_server.py b/web_server.py index 54c47f33..f95f2380 100644 --- a/web_server.py +++ b/web_server.py @@ -19056,14 +19056,24 @@ def server_playlist_add_track(playlist_id): elif active_server == 'jellyfin' and media_server_engine.client('jellyfin'): from core.sync.playlist_edit import plan_playlist_add - current_tracks = media_server_engine.client('jellyfin').get_playlist_tracks(playlist_id) or [] + jf = media_server_engine.client('jellyfin') + current_tracks = jf.get_playlist_tracks(playlist_id) or [] track_ids = [str(t.ratingKey) for t in current_tracks] # Matching an unmatched source to a track already in the playlist # is a LINK, not a second copy — don't duplicate it (#768). plan = plan_playlist_add(track_ids, track_id, is_link=bool(source_track_id), position=position) if plan['should_insert']: - new_track_objs = [type('T', (), {'ratingKey': tid, 'title': ''})() for tid in plan['new_ids']] - media_server_engine.client('jellyfin').update_playlist(playlist_name, new_track_objs) + # #837: append the ONE found track IN PLACE. The old path called + # update_playlist(full track list), which on Jellyfin/Emby deletes + # and recreates the playlist — wiping its description + cover image. + # append_to_playlist adds in place (dedupe-safe), the same + # non-destructive op the 'append' sync mode already uses. It reads + # `.id` (not ratingKey) off each track, so set both. + new_track_obj = type('T', (), { + 'id': str(track_id), 'ratingKey': str(track_id), + 'title': server_track_title or '', + })() + jf.append_to_playlist(playlist_name, [new_track_obj]) _persist_find_and_add_match(source_track_id, active_server, track_id, server_track_title, source_title, source_artist, source_provider) return jsonify({"success": True, "message": "Track linked" if not plan['should_insert'] else "Track added"}) From 27d738e7b1a3fa902b3366f8d2b4ff964cfdae97 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 17:23:13 -0700 Subject: [PATCH 03/25] Fix: Find & Add library search buried exact matches (case-sensitive ordering) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported via Find & Add (Billie Eilish "bad guy"): the track was in the library and on Plex, but never showed in the modal's 20 results. Root cause (proven against the real 307k-track DB): the search did `ORDER BY tracks.title`, which is case-SENSITIVE in SQLite (BINARY collation sorts 'B' before 'b'). Billie's title is lowercase "bad guy"; everyone else's is "Bad Guy", so all the capitalised ones sorted first, filled the LIMIT, and her exact match landed at ~#25 — cut off. - search_tracks now ranks by relevance: exact title match first (case-insensitive via unidecode_lower), then prefix, then alphabetical — so an exact match can't be sorted below the limit by a capital letter. Helps every caller. - Added a rank-only `rank_artist` hint (never filters): Find & Add already knows the source track's artist, so it now passes it and the exact title+artist match floats to #1. Filtering was deliberately avoided — if the track is tagged under a slightly different artist on the server, a filter would re-hide it. Verified on the real DB: title-only "bad guy" now surfaces Billie at #4 (was >#20); with the artist hint she's #1. Seam tests: lowercase exact title isn't buried; rank hint floats the match without filtering; exact title beats a superstring title. 10 tests pass. --- database/music_database.py | 56 ++++++++++++++++++---- tests/test_search_tracks_relevance.py | 68 +++++++++++++++++++++++++++ web_server.py | 7 ++- webui/static/pages-extra.js | 8 +++- 4 files changed, 127 insertions(+), 12 deletions(-) create mode 100644 tests/test_search_tracks_relevance.py diff --git a/database/music_database.py b/database/music_database.py index 7b352a34..3c2ebf86 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -6631,17 +6631,22 @@ class MusicDatabase: logger.error(f"Error searching artists with query '{query}': {e}") return [] - def search_tracks(self, title: str = "", artist: str = "", limit: int = 50, server_source: str = None) -> List[DatabaseTrack]: - """Search tracks by title and/or artist name with Unicode-aware fuzzy matching""" + def search_tracks(self, title: str = "", artist: str = "", limit: int = 50, server_source: str = None, + rank_artist: str = None) -> List[DatabaseTrack]: + """Search tracks by title and/or artist name with Unicode-aware fuzzy matching. + + ``rank_artist`` is a relevance-only hint (never filters): when given, rows + by that artist rank to the top so an exact title+artist match wins over + same-title tracks by other artists.""" try: if not title and not artist: return [] - + conn = self._get_connection() cursor = conn.cursor() - + # STRATEGY 1: Try basic SQL LIKE search first (fastest) - basic_results = self._search_tracks_basic(cursor, title, artist, limit, server_source) + basic_results = self._search_tracks_basic(cursor, title, artist, limit, server_source, rank_artist) if basic_results: logger.debug(f"Basic search found {len(basic_results)} results") @@ -6681,14 +6686,18 @@ class MusicDatabase: logger.error(f"API: Error searching tracks with title='{title}', artist='{artist}': {e}") return [] - def _search_tracks_basic(self, cursor, title: str, artist: str, limit: int, server_source: str = None) -> List[DatabaseTrack]: + def _search_tracks_basic(self, cursor, title: str, artist: str, limit: int, server_source: str = None, + rank_artist: str = None) -> List[DatabaseTrack]: """Basic SQL LIKE search - fastest method""" - rows = self._search_tracks_basic_rows(cursor, title, artist, limit, server_source) + rows = self._search_tracks_basic_rows(cursor, title, artist, limit, server_source, rank_artist) return self._rows_to_tracks(rows) def _search_tracks_basic_rows(self, cursor, title: str, artist: str, limit: int, - server_source: Optional[str] = None): - """Basic SQL LIKE search returning raw rows (shared by DatabaseTrack and dict-returning callers).""" + server_source: Optional[str] = None, rank_artist: Optional[str] = None): + """Basic SQL LIKE search returning raw rows (shared by DatabaseTrack and dict-returning callers). + + ``rank_artist`` is a relevance-only hint (does NOT filter): when given, + rows by that artist sort to the top so an exact title+artist match wins.""" where_conditions = [] params = [] @@ -6711,6 +6720,33 @@ class MusicDatabase: return [] where_clause = " AND ".join(where_conditions) + + # Relevance ordering. The old `ORDER BY tracks.title` was case-SENSITIVE + # (SQLite BINARY collation sorts 'B' before 'b'), so a lowercase exact + # title like Billie Eilish's "bad guy" sorted BELOW every capitalised + # "Bad Guy" and got cut off by LIMIT. Now: exact title match first, then + # prefix, then — when an artist is given — exact/contains artist match, + # finally case-insensitive alphabetical. unidecode_lower folds case + + # accents, matching the WHERE clause. + order_parts, order_params = [], [] + if title: + norm_title = self._normalize_for_comparison(title) + order_parts.append( + "CASE WHEN unidecode_lower(tracks.title) = ? THEN 0 " + "WHEN unidecode_lower(tracks.title) LIKE ? THEN 1 ELSE 2 END") + order_params.extend([norm_title, f"{norm_title}%"]) + _rank_artist = artist or rank_artist + if _rank_artist: + norm_artist = self._normalize_for_comparison(_rank_artist) + order_parts.append( + "CASE WHEN unidecode_lower(artists.name) = ? THEN 0 " + "WHEN unidecode_lower(artists.name) LIKE ? THEN 1 ELSE 2 END") + order_params.extend([norm_artist, f"%{norm_artist}%"]) + order_parts.append("unidecode_lower(tracks.title)") + order_parts.append("unidecode_lower(artists.name)") + order_by = ", ".join(order_parts) + + params.extend(order_params) params.append(limit) cursor.execute(f""" @@ -6719,7 +6755,7 @@ class MusicDatabase: JOIN artists ON tracks.artist_id = artists.id JOIN albums ON tracks.album_id = albums.id WHERE {where_clause} - ORDER BY tracks.title, artists.name + ORDER BY {order_by} LIMIT ? """, params) diff --git a/tests/test_search_tracks_relevance.py b/tests/test_search_tracks_relevance.py new file mode 100644 index 00000000..7c020356 --- /dev/null +++ b/tests/test_search_tracks_relevance.py @@ -0,0 +1,68 @@ +"""Find & Add library search relevance (Billie Eilish 'bad guy' report). + +Root cause proven against the real DB: `ORDER BY tracks.title` is case-SENSITIVE +(SQLite BINARY sorts 'B' before 'b'), so a lowercase exact title like Billie +Eilish's "bad guy" sorted BELOW every capitalised "Bad Guy" and fell past the +result LIMIT — it never showed in the modal even though it was in the library. + +Fix: rank by relevance (exact title first, case-insensitive), and accept a +rank-only artist hint so an exact title+artist match wins — without FILTERING +(filtering would re-hide the track if it's tagged under a slightly different +artist on the server). +""" + +from __future__ import annotations + +import pytest + +from database.music_database import MusicDatabase + + +@pytest.fixture +def db(tmp_path): + return MusicDatabase(str(tmp_path / "music.db")) + + +def _insert(db, tid, title, artist_id, artist_name): + with db._get_connection() as conn: + conn.execute("INSERT OR IGNORE INTO artists (id, name) VALUES (?, ?)", (artist_id, artist_name)) + conn.execute("INSERT OR IGNORE INTO albums (id, title, artist_id) VALUES (?, ?, ?)", (artist_id, "Alb", artist_id)) + conn.execute( + "INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path) " + "VALUES (?, ?, ?, ?, 1, 180, ?)", + (tid, artist_id, artist_id, title, f"/m/{tid}.mp3"), + ) + conn.commit() + + +def test_lowercase_exact_title_not_buried_by_case(db): + # Capitalised "Bad Guy" tracks would (pre-fix) fill the LIMIT and sort + # the lowercase "bad guy" below them, cutting it off. + _insert(db, 1, "Bad Guy", 1, "Yara") + _insert(db, 2, "Bad Guy", 2, "Zelda") + _insert(db, 3, "bad guy", 3, "Billie Eilish") + + names = [t.artist_name for t in db.search_tracks(title="bad guy", limit=2)] + assert "Billie Eilish" in names, "lowercase exact title must not be sorted past the limit" + + +def test_rank_artist_hint_floats_match_to_top_without_filtering(db): + _insert(db, 1, "Bad Guy", 1, "Aaa Artist") + _insert(db, 2, "Bad Guy", 2, "Bbb Artist") + _insert(db, 3, "bad guy", 3, "Billie Eilish") + + results = db.search_tracks(title="bad guy", limit=10, rank_artist="Billie Eilish") + names = [t.artist_name for t in results] + assert names[0] == "Billie Eilish", "the hinted artist's exact match should rank first" + # …but it must NOT filter — the other artists' versions are still there. + assert len(results) == 3 + assert {"Aaa Artist", "Bbb Artist"} <= set(names) + + +def test_exact_title_outranks_superstring_title(db): + # "bad guy" should beat "Bad Guy Necessity" / "Bad Guys" for the query. + _insert(db, 1, "Bad Guy Necessity", 1, "Aardvark") # would sort first alphabetically + _insert(db, 2, "bad guy", 2, "Billie Eilish") + + top = db.search_tracks(title="bad guy", limit=5)[0] + assert top.title.lower() == "bad guy" and top.artist_name == "Billie Eilish" diff --git a/web_server.py b/web_server.py index f95f2380..88973d39 100644 --- a/web_server.py +++ b/web_server.py @@ -19184,6 +19184,10 @@ def library_search_tracks(): """Search SoulSync's local database for tracks (for manual match correction).""" try: query = request.args.get('q', '').strip() + # Optional source-artist relevance hint (Find & Add knows the artist of + # the track it's matching) — used only to rank exact title+artist matches + # to the top, NOT to filter. + artist_hint = request.args.get('artist', '').strip() limit = int(request.args.get('limit', 10)) if not query: return jsonify({"success": True, "tracks": []}) @@ -19213,7 +19217,8 @@ def library_search_tracks(): return f"{_art_prefix}{url}{_art_suffix}" return url - results = database.search_tracks(title=query, artist='', limit=limit, server_source=active_server) + results = database.search_tracks(title=query, artist='', limit=limit, + server_source=active_server, rank_artist=artist_hint) tracks = [] for t in results: diff --git a/webui/static/pages-extra.js b/webui/static/pages-extra.js index d930bb37..0dd10a46 100644 --- a/webui/static/pages-extra.js +++ b/webui/static/pages-extra.js @@ -1680,6 +1680,10 @@ async function serverSearchReplace(trackIndex, mode) { const searchQuery = src.name ? src.name.trim() : (svr.title || '').trim(); const contextArtist = src.artist || svr.artist || ''; const contextName = src.name || svr.title || ''; + // Pass the source artist as a relevance hint so an exact title+artist match + // ranks to the top of the library search instead of being buried under + // same-title tracks by other artists (#: "bad guy" by Billie Eilish). + _serverEditorState.searchArtist = contextArtist; const existing = document.getElementById('server-search-overlay'); if (existing) existing.remove(); @@ -1756,7 +1760,9 @@ async function _serverSearchExecute() { if (resultsHeader) resultsHeader.textContent = ''; try { - const response = await fetch(`/api/library/search-tracks?q=${encodeURIComponent(query)}&limit=20`); + const artistHint = (_serverEditorState && _serverEditorState.searchArtist) || ''; + const response = await fetch(`/api/library/search-tracks?q=${encodeURIComponent(query)}&limit=20` + + (artistHint ? `&artist=${encodeURIComponent(artistHint)}` : '')); const data = await response.json(); if (!data.success || !data.tracks || data.tracks.length === 0) { From 56bdcee4344399ca533127ef2cd7def9008a3e5b Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 18:16:22 -0700 Subject: [PATCH 04/25] Fix: rejected slskd download hung the task at 'downloading' forever (#836) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A wishlist track (or tracks in an album) that slskd accepted then REJECTED would sit at "DOWNLOADING... 0%" indefinitely, spam an ERROR every status poll ("…Completed, Rejected - letting monitor handle retry"), and — for albums — block the whole batch from ever completing. Root defect: the status formatter's non-manual error branch keeps the task 'downloading' and trusts the retry monitor to resolve it, with NO backstop. When the monitor can't make progress (a rejected transfer with no other source), the task is stuck forever. Backstop: measure how long the ERROR state has persisted (keyed off the task's last status transition, so a slow-but-healthy transfer is never failed, and each monitor-retry episode gets a fresh window). Once it exceeds the monitor's retry window (60s, vs the monitor's ~15s) with no resolution, mark the task failed and fire the worker-freeing completion callback so the batch can finish. Also log the error ONCE per episode instead of every 2s poll. The healthy path is untouched — a working retry transitions the task before the grace, so this never fires. Manual picks still fail immediately (unchanged). Tests: rejected-within-grace stays downloading; rejected-beyond-grace fails + schedules completion; manual pick fails immediately. 45 status tests pass. --- core/downloads/status.py | 69 ++++++++++++++++++++---- tests/downloads/test_downloads_status.py | 63 ++++++++++++++++++++++ 2 files changed, 123 insertions(+), 9 deletions(-) diff --git a/core/downloads/status.py b/core/downloads/status.py index 010581b9..f49a202b 100644 --- a/core/downloads/status.py +++ b/core/downloads/status.py @@ -36,6 +36,15 @@ from utils.logging_config import get_logger # Project logger factory so these lines reach app.log (soulsync.* namespace). logger = get_logger("downloads.status") +# #836 backstop: how long an slskd error state (Rejected/Failed/Errored/TimedOut) +# may persist on a non-manual task before the status formatter gives up on the +# retry monitor and marks it failed. The monitor's own retry window is ~15s +# (3 × 5s); this is well beyond it so a healthy retry always wins, and it only +# fires when the monitor genuinely can't make progress (e.g. a rejected transfer +# with no other source) — which otherwise hangs the task at 'downloading 0%' +# forever and blocks the whole batch from completing. +ERROR_STATE_TERMINAL_GRACE_SECONDS = 60 + def _schedule_completion_callback(deps, batch_id: str, task_id: str, success: bool) -> None: """Fire ``deps.on_download_completed`` on a one-shot daemon thread so @@ -394,17 +403,59 @@ def build_batch_status_data(batch_id: str, batch: dict, live_transfers_lookup: d # release the lock. _schedule_completion_callback(deps, batch_id, task_id, False) else: - # UNIFIED ERROR HANDLING: Let monitor handle errors for consistency - # Monitor will detect errored state and trigger retry within 5 seconds - logger.error(f"Task {task_id} API shows error state: {state_str} - letting monitor handle retry") + # Normally the retry monitor picks up an errored state and + # retries within ~15s. But if it can't make progress — e.g. an + # slskd transfer rejected with no other source — the task would + # otherwise sit at 'downloading 0%' forever, spam an ERROR every + # poll, AND block its batch from ever completing (#836: a rejected + # wishlist track, or rejected tracks in an album download). + # + # Backstop: measure how long the ERROR state has persisted (not + # how long the task has downloaded, so a slow-but-healthy transfer + # isn't failed). Once it exceeds the monitor's retry window with no + # resolution, mark the task failed so the worker frees and the + # batch can finish. A working retry transitions the task out of the + # error state first, clearing the timer below — so the healthy path + # never hits this. + # A monitor retry transitions the task (newer + # status_change_time), which restarts the window so each + # error EPISODE gets a fresh grace. If the monitor never + # transitions it (the stuck case), the window keeps growing. + err_since = task.get('_error_state_since') + if err_since is None or task.get('status_change_time', 0) > err_since: + task['_error_state_since'] = err_since = current_time + task.pop('_error_state_logged', None) + error_age = current_time - err_since - # Keep task in current status (downloading/queued) so monitor can detect error - # Don't mark as failed here - let the unified retry system handle it - if task['status'] in ['searching', 'downloading', 'queued']: - task_status['status'] = task['status'] # Keep current status for monitor + if error_age > ERROR_STATE_TERMINAL_GRACE_SECONDS: + err_msg = live_info.get('errorMessage') or live_info.get('error') or '' + task['status'] = 'failed' + task['error_message'] = ( + str(err_msg) if err_msg + else f'Download failed (state: {state_str})' + ) + task_status['status'] = 'failed' + task_status['error_message'] = task['error_message'] + logger.warning( + f"Task {task_id} stuck in error state '{state_str}' for " + f"{error_age:.0f}s with no retry progress — marking failed (#836)" + ) + _schedule_completion_callback(deps, batch_id, task_id, False) else: - task_status['status'] = 'downloading' # Default to downloading for error detection - task['status'] = 'downloading' + # Within the retry window — keep current status so the monitor + # can act. Log once per episode, not every poll, to stop the + # 2-second ERROR spam the reporter saw. + if not task.get('_error_state_logged'): + logger.warning( + f"Task {task_id} API shows error state: {state_str} " + f"- letting monitor handle retry" + ) + task['_error_state_logged'] = True + if task['status'] in ['searching', 'downloading', 'queued']: + task_status['status'] = task['status'] # Keep current status for monitor + else: + task_status['status'] = 'downloading' # Default to downloading for error detection + task['status'] = 'downloading' elif 'Completed' in state_str or 'Succeeded' in state_str: # Verify bytes actually transferred before trusting state string expected_size = live_info.get('size', 0) diff --git a/tests/downloads/test_downloads_status.py b/tests/downloads/test_downloads_status.py index 522137e5..7050e3e6 100644 --- a/tests/downloads/test_downloads_status.py +++ b/tests/downloads/test_downloads_status.py @@ -761,3 +761,66 @@ def test_unified_response_caps_persistent_history_tail(): assert requested_limits == [50] assert len(out['downloads']) == 50 assert out['total'] == 50 + + +# --------------------------------------------------------------------------- +# #836 — a rejected slskd transfer must not hang the task at 'downloading' +# forever. The monitor normally retries; when it can't make progress, a backstop +# in the status formatter marks the task failed after a grace window so the +# worker frees and the batch can complete. +# --------------------------------------------------------------------------- + +def test_rejected_within_grace_keeps_downloading_for_monitor(): + import time + deps, _ = _build_deps() + now = time.time() + download_tasks['t1'] = { + 'track_index': 0, 'status': 'downloading', 'track_info': {}, + 'filename': 'song.flac', 'username': 'u1', + 'status_change_time': now - 5, + } + live = {'u1::song.flac': {'state': 'Completed, Rejected', 'percentComplete': 0}} + batch = {'phase': 'downloading', 'queue': ['t1']} + out = st.build_batch_status_data('b1', batch, live, deps) + # inside the grace window — give the monitor its chance, don't fail yet + assert out['tasks'][0]['status'] == 'downloading' + assert download_tasks['t1']['status'] == 'downloading' + + +def test_rejected_beyond_grace_marks_failed_and_frees_worker(): + import time + completed = [] + deps, _ = _build_deps() + deps.on_download_completed = lambda b, t, s: completed.append((b, t, s)) + now = time.time() + download_tasks['t1'] = { + 'track_index': 0, 'status': 'downloading', 'track_info': {}, + 'filename': 'song.flac', 'username': 'u1', + # error persisted past the grace, no monitor transition since + 'status_change_time': now - 130, + '_error_state_since': now - (st.ERROR_STATE_TERMINAL_GRACE_SECONDS + 30), + } + live = {'u1::song.flac': {'state': 'Completed, Rejected', 'errorMessage': 'peer rejected'}} + batch = {'phase': 'downloading', 'queue': ['t1']} + out = st.build_batch_status_data('b1', batch, live, deps) + assert out['tasks'][0]['status'] == 'failed' + assert download_tasks['t1']['status'] == 'failed' + assert download_tasks['t1']['error_message'] == 'peer rejected' + time.sleep(0.05) # completion callback runs on a daemon thread + assert completed == [('b1', 't1', False)] # batch can now finish + + +def test_manual_pick_rejected_fails_immediately_without_grace(): + import time + deps, _ = _build_deps() + now = time.time() + download_tasks['t1'] = { + 'track_index': 0, 'status': 'downloading', 'track_info': {}, + 'filename': 'song.flac', 'username': 'u1', + '_user_manual_pick': True, 'status_change_time': now, + } + live = {'u1::song.flac': {'state': 'Completed, Rejected'}} + batch = {'phase': 'downloading', 'queue': ['t1']} + out = st.build_batch_status_data('b1', batch, live, deps) + assert out['tasks'][0]['status'] == 'failed' # immediate, no 60s wait + assert download_tasks['t1']['status'] == 'failed' From a15fe158423af55af86c9487aee473ac22990edc Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 18:31:43 -0700 Subject: [PATCH 05/25] Fix: auto-sync capped public Spotify playlists at 100 tracks (#838) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression in 2.6.9. The spotify_public source adapter (used by auto-sync / refresh_mirrored) called scrape_spotify_embed() directly — the embed widget only exposes ~100 tracks — instead of fetch_spotify_public(), the wrapper the rest of the app uses, which pulls the full list via the paginated public API and only falls back to the embed on failure. So initial discovery got the whole playlist but every auto-sync re-fetch truncated it to 100. Switched the adapter to fetch_spotify_public (same return shape — drop-in). Albums still resolve via the embed (already whole); on any failure it falls back to the embed exactly as before. Test: the adapter returns all 150 tracks when the full fetch yields 150 (was capped at 100). 29 adapter tests pass. --- core/playlists/sources/spotify_public.py | 9 +++++-- tests/test_playlist_sources_adapters.py | 31 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/core/playlists/sources/spotify_public.py b/core/playlists/sources/spotify_public.py index 51189687..4ab21164 100644 --- a/core/playlists/sources/spotify_public.py +++ b/core/playlists/sources/spotify_public.py @@ -35,14 +35,19 @@ class SpotifyPublicPlaylistSource(PlaylistSource): """``playlist_id`` is a Spotify URL or ``open.spotify.com`` URI.""" from core.spotify_public_scraper import ( parse_spotify_url, - scrape_spotify_embed, + fetch_spotify_public, ) parsed = parse_spotify_url(playlist_id) if not parsed: return None - data = scrape_spotify_embed(parsed["type"], parsed["id"]) + # #838: use the full-fetch wrapper (paginates past the embed widget's + # ~100-track cap), NOT scrape_spotify_embed directly. The rest of the app + # already uses fetch_spotify_public; the adapter calling the capped embed + # scraper is why auto-sync truncated >100-track playlists to 100 while the + # initial discovery got the whole thing. Same return shape, so drop-in. + data = fetch_spotify_public(parsed["type"], parsed["id"]) if not isinstance(data, dict) or data.get("error"): return None diff --git a/tests/test_playlist_sources_adapters.py b/tests/test_playlist_sources_adapters.py index 18ef4d3b..38e7b518 100644 --- a/tests/test_playlist_sources_adapters.py +++ b/tests/test_playlist_sources_adapters.py @@ -915,3 +915,34 @@ def test_mirror_dict_spotify_public_emits_spotify_hint(): extra = _json.loads(d["extra_data"]) assert extra["discovered"] is False assert extra["spotify_hint"]["id"] == "sptrk" + + +def test_spotify_public_adapter_paginates_past_100(monkeypatch): + """#838: auto-sync truncated >100-track playlists to 100 because the adapter + called the embed scraper (≤100) directly instead of the full-fetch wrapper. + With the wrapper, a playlist whose full fetch returns 150 tracks keeps all 150.""" + src = SpotifyPublicPlaylistSource() + + monkeypatch.setattr( + "core.spotify_public_scraper.parse_spotify_url", + lambda url: {"type": "playlist", "id": "big"}, + ) + full = { + "id": "big", "type": "playlist", "name": "Big PL", "subtitle": "owner", + "url": "https://open.spotify.com/playlist/big", "url_hash": "bighash", + "tracks": [ + {"id": f"t{i}", "name": f"Song {i}", "artists": [{"name": "A"}], + "duration_ms": 1000, "is_explicit": False, "track_number": i + 1} + for i in range(150) + ], + } + # The full paginated path succeeds → wrapper returns all 150 (no embed cap). + monkeypatch.setattr( + "core.spotify_public_api.fetch_public_playlist_full", + lambda spotify_id: full, + ) + + detail = src.get_playlist("https://open.spotify.com/playlist/big") + assert detail is not None + assert len(detail.tracks) == 150, "pre-#838 the adapter capped this at 100" + assert detail.meta.track_count == 150 From 0afa3c9705b4a3b6a73463ced7dc05236107a3b2 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 18:49:32 -0700 Subject: [PATCH 06/25] Fix: "Discovery state not found" when fixing a match after restart/import (#843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The discovery FIX → Confirm flow 404'd with "Discovery state not found" whenever the in-memory discovery state was gone — a server restart, or an imported playlist that wasn't discovered in THIS process — even though the card is still shown from persisted data (the reporter's log shows "Returning 0 stored ... playlists for hydration", i.e. the in-memory states were empty). The thing that actually makes a manual fix STICK is writing it to the discovery cache (save_discovery_cache_match), keyed by the original track's name + artist — which doesn't need the in-memory state at all. But the endpoint 404'd on the missing state before reaching that write, so the fix was dead after a restart. - update_discovery_match (core/discovery/endpoints.py) now only does the in-memory result update when the state exists; the durable discovery-cache write always runs, falling back to client-provided original_name/original_artist when there's no in-memory state. With neither a state nor originals it still 404s (unchanged). - The FIX confirm (wishlist-tools.js) now sends original_name/original_artist (from the source track it already has) so the backend can key the cache. Covers all sources that share the helper (tidal/deezer/qobuz/spotify-public). Tests: no-state-but-originals saves the cache + returns success; no-state-no- originals still 404s; existing with-state path unchanged. 73 discovery tests pass. --- core/discovery/endpoints.py | 73 +++++++++++++-------- tests/discovery/test_discovery_endpoints.py | 36 ++++++++++ webui/static/wishlist-tools.js | 5 ++ 3 files changed, 86 insertions(+), 28 deletions(-) diff --git a/core/discovery/endpoints.py b/core/discovery/endpoints.py index 69dbb0d8..cd0f0190 100644 --- a/core/discovery/endpoints.py +++ b/core/discovery/endpoints.py @@ -579,45 +579,62 @@ def update_discovery_match( return {'error': 'Missing required fields'}, 400 state = states.get(identifier) - if not state: - return {'error': 'Discovery state not found'}, 404 + result = None + if state: + if track_index >= len(state['discovery_results']): + return {'error': 'Invalid track index'}, 400 - if track_index >= len(state['discovery_results']): - return {'error': 'Invalid track index'}, 400 + result = state['discovery_results'][track_index] + old_status = result.get('status') - result = state['discovery_results'][track_index] - old_status = result.get('status') + result['status'] = 'Found' + result['status_class'] = 'found' + result['spotify_track'] = spotify_track['name'] + result['spotify_artist'] = join_artist_names(spotify_track['artists']) if isinstance(spotify_track['artists'], list) else extract_artist_name(spotify_track['artists']) + result['spotify_album'] = spotify_track['album'] + result['spotify_id'] = spotify_track['id'] - result['status'] = 'Found' - result['status_class'] = 'found' - result['spotify_track'] = spotify_track['name'] - result['spotify_artist'] = join_artist_names(spotify_track['artists']) if isinstance(spotify_track['artists'], list) else extract_artist_name(spotify_track['artists']) - result['spotify_album'] = spotify_track['album'] - result['spotify_id'] = spotify_track['id'] + duration_ms = spotify_track.get('duration_ms', 0) + if duration_ms: + minutes = duration_ms // 60000 + seconds = (duration_ms % 60000) // 1000 + result['duration'] = f"{minutes}:{seconds:02d}" + else: + result['duration'] = '0:00' - duration_ms = spotify_track.get('duration_ms', 0) - if duration_ms: - minutes = duration_ms // 60000 - seconds = (duration_ms % 60000) // 1000 - result['duration'] = f"{minutes}:{seconds:02d}" - else: - result['duration'] = '0:00' + result['spotify_data'] = build_fix_modal_spotify_data(spotify_track) + result['wing_it_fallback'] = False + result['manual_match'] = True - result['spotify_data'] = build_fix_modal_spotify_data(spotify_track) - result['wing_it_fallback'] = False - result['manual_match'] = True + if old_status != 'found' and old_status != 'Found': + state['spotify_matches'] = state.get('spotify_matches', 0) + 1 - if old_status != 'found' and old_status != 'Found': - state['spotify_matches'] = state.get('spotify_matches', 0) + 1 + logger.info(f"Manual match updated: {source_log_label} - {identifier} - track {track_index}") + logger.info(f" → {result['spotify_artist']} - {result['spotify_track']}") - logger.info(f"Manual match updated: {source_log_label} - {identifier} - track {track_index}") - logger.info(f" → {result['spotify_artist']} - {result['spotify_track']}") - - try: original_track = result.get(original_track_key, {}) original_name = original_track.get('name', spotify_track['name']) original_artist = original_artist_getter(original_track) + else: + # #843: the in-memory discovery state can be gone — a server restart, + # or an imported playlist that wasn't discovered in THIS process — + # while the card is still shown from persisted data. The DURABLE part + # of a manual fix (writing the match to the discovery cache so future + # syncs resolve it) doesn't need the in-memory state, only the original + # track's name + artist, which the client now sends. Fall back to those + # instead of 404ing the fix into uselessness. + original_name = (data.get('original_name') or '').strip() + original_artist = (data.get('original_artist') or '').strip() + if not original_name and not original_artist: + return {'error': 'Discovery state not found'}, 404 + if not original_name: + original_name = spotify_track['name'] + logger.info( + f"Manual match (no in-memory state) → discovery cache: " + f"{source_log_label} - {identifier} - '{original_name}' by '{original_artist}'" + ) + try: cache_key = get_discovery_cache_key(original_name, original_artist) artists_list = spotify_track['artists'] if isinstance(artists_list, list): diff --git a/tests/discovery/test_discovery_endpoints.py b/tests/discovery/test_discovery_endpoints.py index 2f1b0ace..d4ad7f2d 100644 --- a/tests/discovery/test_discovery_endpoints.py +++ b/tests/discovery/test_discovery_endpoints.py @@ -955,3 +955,39 @@ def test_snapshot_exception_returns_500(): raise RuntimeError('db down') body, code = save_bubble_snapshot(lambda: {'bubbles': [1]}, **_snap_kwargs(_BoomDB())) assert code == 500 and body == {'success': False, 'error': 'db down'} + + +def test_update_match_no_state_but_originals_saves_cache(): + """#843: the in-memory discovery state can be gone (server restart, or an + imported playlist not discovered this run) while the card is still shown from + persisted data. A manual fix must still save the match to the discovery cache + from the client-provided originals instead of 404ing.""" + from core.discovery.endpoints import update_discovery_match + db = _FakeCacheDB() + gj, kw, _ = _update_kwargs(cache_db=db, json_data={ + 'identifier': 'gone-from-memory', 'track_index': 0, + 'original_name': 'Acid Dream', 'original_artist': 'Cherrymoon Traxx', + 'spotify_track': { + 'id': 'sp1', 'name': 'Acid Dream (The Prophet remix)', + 'artists': ['Cherry Moon Trax'], 'album': 'X', 'duration_ms': 1000, + }, + }) + body, code = update_discovery_match({}, gj, **kw) # empty in-memory states + + assert code == 200 and body['success'] is True + assert body['result'] is None # nothing to update in memory + # the durable cache match WAS written, keyed by the original track + assert len(db.saved) == 1 + saved = db.saved[0] + assert saved[0] == 'acid dream' # cache_key from get_discovery_cache_key + assert saved[5] == 'Acid Dream' and saved[6] == 'Cherrymoon Traxx' # original name/artist + + +def test_update_match_no_state_and_no_originals_still_404(): + """Without an in-memory state AND without originals to key the cache, there's + genuinely nothing to do — keep the 404 (the existing safety).""" + from core.discovery.endpoints import update_discovery_match + gj, kw, _ = _update_kwargs(json_data={ + 'identifier': 'gone', 'track_index': 0, 'spotify_track': {'id': 'x'}}) + body, code = update_discovery_match({}, gj, **kw) + assert code == 404 and body == {'error': 'Discovery state not found'} diff --git a/webui/static/wishlist-tools.js b/webui/static/wishlist-tools.js index 1842e72a..64230654 100644 --- a/webui/static/wishlist-tools.js +++ b/webui/static/wishlist-tools.js @@ -410,6 +410,11 @@ async function selectDiscoveryFixTrack(track) { const requestBody = { identifier: backendIdentifier, track_index: trackIndex, + // #843: send the original (source) track so the backend can still save + // the match to the discovery cache when its in-memory discovery state + // is gone (server restart / imported playlist not discovered this run). + original_name: currentDiscoveryFix.sourceTrack || '', + original_artist: currentDiscoveryFix.sourceArtist || '', spotify_track: { id: track.id, name: track.name, From 5bc27e6268bca14aebf0a58625fd3b1bb55cd349 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 18:57:43 -0700 Subject: [PATCH 07/25] #843 follow-up: key the no-state cache save by the FIRST artist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #843 fallback saved the discovery-cache match using the client's original_artist verbatim — but the client sends a joined "A, B, C" string, while EVERY in-memory + sync path keys the cache by the first artist (artists[0]). So a multi-artist fix (the reporter's exact "Cherrymoon Traxx, Hermol, SBM, BELS" case) would have saved under a key the sync never looks up — the fix would "succeed" with no error but silently never apply. Reduce the client artist to the first (split on comma) in the no-state branch so its cache key matches the in-memory/sync convention exactly. Single-artist tracks are unaffected. Test: no-state save now keys by the first artist, and a new test pins that the no-state and in-memory paths produce an IDENTICAL cache key for the same multi-artist track. 74 discovery tests pass. --- core/discovery/endpoints.py | 6 ++++ tests/discovery/test_discovery_endpoints.py | 40 ++++++++++++++++++--- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/core/discovery/endpoints.py b/core/discovery/endpoints.py index cd0f0190..2d0d2042 100644 --- a/core/discovery/endpoints.py +++ b/core/discovery/endpoints.py @@ -629,6 +629,12 @@ def update_discovery_match( return {'error': 'Discovery state not found'}, 404 if not original_name: original_name = spotify_track['name'] + # Key the cache by the FIRST artist — every in-memory + sync path uses + # artists[0], but the client may send a joined "A, B, C" string. Without + # this, a multi-artist track would save under a key the sync never looks + # up (full string ≠ first artist), so the fix would silently never apply. + if original_artist: + original_artist = original_artist.split(',')[0].strip() logger.info( f"Manual match (no in-memory state) → discovery cache: " f"{source_log_label} - {identifier} - '{original_name}' by '{original_artist}'" diff --git a/tests/discovery/test_discovery_endpoints.py b/tests/discovery/test_discovery_endpoints.py index d4ad7f2d..5aa80d27 100644 --- a/tests/discovery/test_discovery_endpoints.py +++ b/tests/discovery/test_discovery_endpoints.py @@ -966,7 +966,9 @@ def test_update_match_no_state_but_originals_saves_cache(): db = _FakeCacheDB() gj, kw, _ = _update_kwargs(cache_db=db, json_data={ 'identifier': 'gone-from-memory', 'track_index': 0, - 'original_name': 'Acid Dream', 'original_artist': 'Cherrymoon Traxx', + 'original_name': 'Acid Dream', + # multi-artist joined string, exactly like the #843 reporter's track + 'original_artist': 'Cherrymoon Traxx, Hermol, SBM, BELS', 'spotify_track': { 'id': 'sp1', 'name': 'Acid Dream (The Prophet remix)', 'artists': ['Cherry Moon Trax'], 'album': 'X', 'duration_ms': 1000, @@ -976,11 +978,41 @@ def test_update_match_no_state_but_originals_saves_cache(): assert code == 200 and body['success'] is True assert body['result'] is None # nothing to update in memory - # the durable cache match WAS written, keyed by the original track + # the durable cache match WAS written — and CRUCIALLY keyed by the FIRST + # artist, matching every in-memory/sync path (which use artists[0]). A + # full-string key here would silently never be looked up on sync. assert len(db.saved) == 1 saved = db.saved[0] - assert saved[0] == 'acid dream' # cache_key from get_discovery_cache_key - assert saved[5] == 'Acid Dream' and saved[6] == 'Cherrymoon Traxx' # original name/artist + assert saved[1] == 'cherrymoon traxx' # cache_key artist = first artist (not the full string) + assert saved[6] == 'Cherrymoon Traxx' # original_artist reduced to first + + +def test_update_match_no_state_key_matches_in_memory_key(): + """The no-state save and the normal in-memory save must produce the SAME cache + key for the same multi-artist track — else the fix wouldn't apply on sync.""" + from core.discovery.endpoints import update_discovery_match + sp = {'id': 'sp1', 'name': 'M', 'artists': ['Z'], 'album': 'A', 'duration_ms': 1} + + # in-memory path: result carries the original track as an artists LIST + db1 = _FakeCacheDB() + state = {'discovery_results': [{'tidal_track': {'name': 'Acid Dream', + 'artists': ['Cherrymoon Traxx', 'Hermol', 'SBM', 'BELS']}}], + 'spotify_matches': 0} + gj1, kw1, _ = _update_kwargs(cache_db=db1, json_data={ + 'identifier': 'p', 'track_index': 0, 'spotify_track': sp}) + update_discovery_match({'p': state}, gj1, **kw1) + + # no-state path: client sends the joined string + db2 = _FakeCacheDB() + gj2, kw2, _ = _update_kwargs(cache_db=db2, json_data={ + 'identifier': 'p', 'track_index': 0, 'spotify_track': sp, + 'original_name': 'Acid Dream', + 'original_artist': 'Cherrymoon Traxx, Hermol, SBM, BELS'}) + update_discovery_match({}, gj2, **kw2) + + # same cache key (name, artist) → the fix applies identically either way + assert db1.saved[0][0] == db2.saved[0][0] + assert db1.saved[0][1] == db2.saved[0][1] def test_update_match_no_state_and_no_originals_still_404(): From 4d1b9a56393463ce60515ed2a88d07c80979c1c1 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 19:33:44 -0700 Subject: [PATCH 08/25] Artist Sync: guard stale-removal against an unreachable mount + gate it admin-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The enhanced-tab "Sync" button's stale-removal phase deleted any track whose file wasn't on disk, with NO guard — so if the music storage was momentarily unavailable (sleeping NAS, dropped mount, unmounted Docker volume, WSL hiccup), os.path.exists returned False for EVERY file and one click wiped the whole artist (tracks + their now-"empty" albums) from the DB. The deep-scan path already had a 50%-stale safety net (#828); this endpoint never got one. - New core/library/stale_guard.py: is_implausible_stale_removal(missing, total) — a tested rule (skip removal when missing > 50% of a >=5-track set), centralised so every stale-removal site can share it. - sync_artist_library: if the guard trips, SKIP removal (delete nothing), return removal_skipped + warn; the frontend shows "storage may be offline — skipped" instead of silently deleting. Empty-album cleanup now also only runs on the non-skipped path and uses `album_id IS NOT NULL` (fixes the NOT IN-with-NULL no-op). Frontend also refreshes the view on additions, not just removals. - @admin_only on the endpoint — it deletes tracks + albums but was ungated, while the sibling delete_album endpoint is gated. Deep scan was already safe (different mechanism: server-diff + its own 50% guard). Tests: guard unit rules; endpoint skips removal when all files missing (keeps the tracks), removes only the genuinely-gone few otherwise, and 403s for non-admins. 7 new tests pass. --- core/library/stale_guard.py | 45 ++++++++++++++++ tests/test_artist_sync_stale_guard.py | 77 +++++++++++++++++++++++++++ tests/test_stale_guard.py | 28 ++++++++++ web_server.py | 33 +++++++++--- webui/static/library.js | 25 +++++---- 5 files changed, 191 insertions(+), 17 deletions(-) create mode 100644 core/library/stale_guard.py create mode 100644 tests/test_artist_sync_stale_guard.py create mode 100644 tests/test_stale_guard.py diff --git a/core/library/stale_guard.py b/core/library/stale_guard.py new file mode 100644 index 00000000..c614934c --- /dev/null +++ b/core/library/stale_guard.py @@ -0,0 +1,45 @@ +"""Guard against mass-deleting library rows when storage is unreachable. + +The library "sync" / cleanup paths mark a track stale when its file isn't on +disk and then delete the row. But ``os.path.exists`` returns False for EVERY +file when the music storage is momentarily unavailable — a sleeping NAS, a +dropped network mount, an unmounted Docker volume, a WSL mount hiccup. Without a +guard, one click then wipes the whole artist/library from the DB even though the +files are fine. + +This mirrors the safety the deep-scan path already had (``database_update_worker`` +skips removal when stale > 50% of a >100-track library — issue #828). Centralised +here so every stale-removal site can share one tested rule. +""" + +from __future__ import annotations + +# Don't second-guess tiny sets — a 2-track artist legitimately losing both files +# shouldn't be blocked. Above this, an implausibly large missing fraction almost +# always means "storage down", not "files actually deleted". +DEFAULT_MIN_TOTAL = 5 +DEFAULT_MAX_MISSING_FRACTION = 0.5 + + +def is_implausible_stale_removal( + missing_count: int, + total_count: int, + *, + min_total: int = DEFAULT_MIN_TOTAL, + max_fraction: float = DEFAULT_MAX_MISSING_FRACTION, +) -> bool: + """True when ``missing_count`` is too large a share of ``total_count`` to be a + real deletion — i.e. the storage is probably unreachable and the caller should + SKIP removal (and warn) rather than delete. + + Returns False for small sets (< ``min_total``) so normal cleanup of a few + genuinely-gone files still works. + """ + if total_count <= 0 or missing_count <= 0: + return False + if total_count < min_total: + return False + return missing_count > total_count * max_fraction + + +__all__ = ["is_implausible_stale_removal", "DEFAULT_MIN_TOTAL", "DEFAULT_MAX_MISSING_FRACTION"] diff --git a/tests/test_artist_sync_stale_guard.py b/tests/test_artist_sync_stale_guard.py new file mode 100644 index 00000000..6715df75 --- /dev/null +++ b/tests/test_artist_sync_stale_guard.py @@ -0,0 +1,77 @@ +"""Artist 'Sync' button (enhanced tab) must not wipe an artist when storage is +unreachable, and must stay admin-only (it deletes tracks + albums). #828 pattern.""" + +from __future__ import annotations + +import os +import tempfile + +import pytest + +_TMP = tempfile.mkdtemp(prefix='soulsync-testdb-stale-') +os.environ['DATABASE_PATH'] = os.path.join(_TMP, 's.db') +os.environ['SOULSYNC_TEST_DB_READY'] = '1' + +web_server = pytest.importorskip('web_server') + + +@pytest.fixture +def client(): + return web_server.app.test_client() + + +def _seed(artist_id, *, missing, present, tmp_path): + """Artist with server_source NULL (skips the media-server pull phase), an + album, ``present`` tracks pointing at real files + ``missing`` at dead paths.""" + db = web_server.get_database() + album_id = artist_id * 10 + with db._get_connection() as conn: + conn.execute("INSERT OR REPLACE INTO artists (id, name, server_source) VALUES (?, ?, NULL)", + (artist_id, f'Artist {artist_id}')) + conn.execute("INSERT OR REPLACE INTO albums (id, title, artist_id) VALUES (?, 'Alb', ?)", + (album_id, artist_id)) + tid = artist_id * 1000 + for i in range(present): + p = tmp_path / f"{artist_id}_present_{i}.flac" + p.write_bytes(b'\x00\x00') + conn.execute("INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path) " + "VALUES (?, ?, ?, ?, 1, 100, ?)", (tid, album_id, artist_id, f'P{i}', str(p))) + tid += 1 + for i in range(missing): + conn.execute("INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path) " + "VALUES (?, ?, ?, ?, 1, 100, ?)", (tid, album_id, artist_id, f'M{i}', f'/nonexistent/{artist_id}_{i}.flac')) + tid += 1 + conn.commit() + + +def _track_count(artist_id): + db = web_server.get_database() + with db._get_connection() as conn: + return conn.execute("SELECT COUNT(*) c FROM tracks WHERE artist_id = ?", (artist_id,)).fetchone()['c'] + + +def test_all_files_missing_skips_removal_and_keeps_tracks(client, tmp_path): + _seed(9001, missing=8, present=0, tmp_path=tmp_path) + body = client.post('/api/library/artist/9001/sync').get_json() + assert body['success'] is True + assert body['removal_skipped'] is True # guard tripped + assert body['stale_removed'] == 0 + assert _track_count(9001) == 8 # nothing deleted — storage looked down + + +def test_a_few_missing_files_are_removed(client, tmp_path): + _seed(9002, missing=2, present=8, tmp_path=tmp_path) + body = client.post('/api/library/artist/9002/sync').get_json() + assert body['success'] is True + assert body['removal_skipped'] is False + assert body['stale_removed'] == 2 # only the genuinely-gone ones + assert _track_count(9002) == 8 + + +def test_sync_is_admin_only(client, tmp_path): + _seed(9003, missing=2, present=2, tmp_path=tmp_path) + nonadmin = web_server.get_database().create_profile(name=f'u_{os.urandom(3).hex()}') + with client.session_transaction() as sess: + sess['profile_id'] = nonadmin + assert client.post('/api/library/artist/9003/sync').status_code == 403 + assert _track_count(9003) == 4 # untouched diff --git a/tests/test_stale_guard.py b/tests/test_stale_guard.py new file mode 100644 index 00000000..6259847e --- /dev/null +++ b/tests/test_stale_guard.py @@ -0,0 +1,28 @@ +"""Storage-unreachable guard for library stale-removal (artist sync, #828 pattern).""" + +from __future__ import annotations + +from core.library.stale_guard import is_implausible_stale_removal as g + + +def test_all_missing_in_a_real_collection_is_blocked(): + # 40/40 missing → almost certainly a down mount, not 40 real deletions. + assert g(40, 40) is True + assert g(30, 40) is True # 75% missing — still implausible + + +def test_a_few_genuinely_missing_files_are_allowed(): + assert g(3, 40) is False # normal cleanup of a few gone files + assert g(20, 40) is False # exactly 50% is NOT over the threshold + + +def test_tiny_sets_are_never_blocked(): + # A 2-track artist legitimately losing both must still clean up. + assert g(2, 2) is False + assert g(4, 4) is False # below min_total (5) + + +def test_edge_inputs(): + assert g(0, 0) is False + assert g(0, 100) is False # nothing missing + assert g(5, 5) is True # min_total met, all missing diff --git a/web_server.py b/web_server.py index 88973d39..9df659ad 100644 --- a/web_server.py +++ b/web_server.py @@ -12225,6 +12225,7 @@ def redownload_start(track_id): @app.route('/api/library/artist//sync', methods=['POST']) +@admin_only def sync_artist_library(artist_id): """Bidirectional sync: pull new content from media server AND remove stale entries.""" try: @@ -12336,6 +12337,7 @@ def sync_artist_library(artist_id): # ── Phase 2: Remove stale entries (files no longer on disk) ── stale_removed = 0 empty_albums_removed = 0 + removal_skipped = False with database._get_connection() as conn: cursor = conn.cursor() @@ -12352,16 +12354,31 @@ def sync_artist_library(artist_id): if not resolved or not os.path.exists(resolved): stale_ids.append(track['id']) - if stale_ids: + # Storage-unreachable guard (#828 pattern): if an implausibly large + # fraction of this artist's tracks look missing, the disk/mount is + # almost certainly down — don't wipe the artist over a flaky mount. + from core.library.stale_guard import is_implausible_stale_removal + if is_implausible_stale_removal(len(stale_ids), len(tracks)): + removal_skipped = True + logger.warning( + f"[Artist Sync] {artist_name}: {len(stale_ids)}/{len(tracks)} tracks look " + f"missing — skipping stale removal (storage likely unreachable)" + ) + elif stale_ids: placeholders = ','.join('?' for _ in stale_ids) cursor.execute(f"DELETE FROM tracks WHERE id IN ({placeholders})", stale_ids) stale_removed = len(stale_ids) - cursor.execute(""" - DELETE FROM albums WHERE artist_id = ? - AND id NOT IN (SELECT DISTINCT album_id FROM tracks) - """, (db_artist_id,)) - empty_albums_removed = cursor.rowcount + # Only prune empty albums when we actually removed tracks — never on the + # skipped path, where "empty" would be a false reading of a down mount. + # ``album_id IS NOT NULL`` avoids the NOT IN-with-NULL gotcha that would + # otherwise no-op this cleanup whenever a track has a null album_id. + if not removal_skipped: + cursor.execute(""" + DELETE FROM albums WHERE artist_id = ? + AND id NOT IN (SELECT DISTINCT album_id FROM tracks WHERE album_id IS NOT NULL) + """, (db_artist_id,)) + empty_albums_removed = cursor.rowcount cursor.execute(""" UPDATE albums SET track_count = ( @@ -12372,7 +12389,8 @@ def sync_artist_library(artist_id): conn.commit() logger.warning(f"[Artist Sync] {artist_name}: +{new_albums} albums, +{new_tracks} tracks, " - f"-{stale_removed} stale, -{empty_albums_removed} empty albums") + f"-{stale_removed} stale, -{empty_albums_removed} empty albums" + f"{' (removal skipped — storage unreachable)' if removal_skipped else ''}") return jsonify({ "success": True, @@ -12382,6 +12400,7 @@ def sync_artist_library(artist_id): "new_tracks": new_tracks, "stale_removed": stale_removed, "empty_albums_removed": empty_albums_removed, + "removal_skipped": removal_skipped, }) except Exception as e: diff --git a/webui/static/library.js b/webui/static/library.js index 34823ce6..e0731f18 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -3192,16 +3192,21 @@ function renderArtistMetaPanel(artist) { const res = await fetch(`/api/library/artist/${artist.id}/sync`, { method: 'POST' }); const data = await res.json(); if (data.success) { - const parts = []; - if (data.new_albums > 0) parts.push(`+${data.new_albums} albums`); - if (data.new_tracks > 0) parts.push(`+${data.new_tracks} tracks`); - if (data.stale_removed > 0) parts.push(`${data.stale_removed} stale removed`); - if (data.empty_albums_removed > 0) parts.push(`${data.empty_albums_removed} empty albums cleaned`); - if (data.name_updated) parts.push('name updated'); - if (parts.length === 0) parts.push('Already in sync'); - showToast(`${data.artist_name}: ${parts.join(', ')}`, 'success'); - // Refresh enhanced view if anything changed - if (data.stale_removed > 0 || data.empty_albums_removed > 0) { + if (data.removal_skipped) { + // Storage looked unreachable — we deliberately did NOT delete. + showToast(`${data.artist_name}: most files looked missing — skipped removal in case your music storage is offline. Check it's mounted, then sync again.`, 'warning'); + } else { + const parts = []; + if (data.new_albums > 0) parts.push(`+${data.new_albums} albums`); + if (data.new_tracks > 0) parts.push(`+${data.new_tracks} tracks`); + if (data.stale_removed > 0) parts.push(`${data.stale_removed} stale removed`); + if (data.empty_albums_removed > 0) parts.push(`${data.empty_albums_removed} empty albums cleaned`); + if (data.name_updated) parts.push('name updated'); + if (parts.length === 0) parts.push('Already in sync'); + showToast(`${data.artist_name}: ${parts.join(', ')}`, 'success'); + } + // Refresh enhanced view if anything changed (additions OR removals) + if (data.new_albums > 0 || data.new_tracks > 0 || data.stale_removed > 0 || data.empty_albums_removed > 0) { loadEnhancedViewData(artist.id); } } else { From d82d02b92174b7f882514137d5a76580c3447763 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 19:43:25 -0700 Subject: [PATCH 09/25] =?UTF-8?q?Artist=20Sync:=20unify=20with=20deep=20sc?= =?UTF-8?q?an=20=E2=80=94=20server-diff=20stale=20removal,=20scoped=20to?= =?UTF-8?q?=20one=20artist?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per the original intent, "Sync" is now a single-artist deep scan: it uses the SAME reconciliation source as the whole-library deep scan instead of a separate disk-existence check. - Phase 1 already calls the deep-scan worker's _process_artist_with_content; now it passes seen_track_ids so the pull collects the server's current track IDs for the artist (existing + new), exactly as the library deep scan does. - Phase 2 stale = (artist's DB tracks for this server) − seen, then delete_stale_tracks(server_source) — identical mechanism to deep scan, scoped to one artist. The old os.path.exists disk check (which could mass-delete on an unreachable mount) is gone. - Removal only runs when the server pull SUCCEEDED — no trustworthy 'seen' set (no server, unreachable, or a failed pull) → skip, never delete. The is_implausible_stale_removal guard (>50% unseen) stays as the same safety net deep scan has for a flaky response. @admin_only retained. Tests rewritten for the server-diff model: removes only tracks the server no longer has; guard skips when most are unseen; a failed pull skips removal entirely; admin-only. 8 tests pass. --- tests/test_artist_sync_stale_guard.py | 110 ++++++++++++++++---------- web_server.py | 106 ++++++++++++++----------- webui/static/library.js | 9 ++- 3 files changed, 135 insertions(+), 90 deletions(-) diff --git a/tests/test_artist_sync_stale_guard.py b/tests/test_artist_sync_stale_guard.py index 6715df75..0da6a1ac 100644 --- a/tests/test_artist_sync_stale_guard.py +++ b/tests/test_artist_sync_stale_guard.py @@ -1,14 +1,16 @@ -"""Artist 'Sync' button (enhanced tab) must not wipe an artist when storage is -unreachable, and must stay admin-only (it deletes tracks + albums). #828 pattern.""" +"""Artist 'Sync' = a single-artist deep scan: stale removal is a server-diff +(tracks the media server no longer has), with the same safety net + admin gate as +the whole-library deep scan. #828 pattern.""" from __future__ import annotations import os import tempfile +from types import SimpleNamespace import pytest -_TMP = tempfile.mkdtemp(prefix='soulsync-testdb-stale-') +_TMP = tempfile.mkdtemp(prefix='soulsync-testdb-sync2-') os.environ['DATABASE_PATH'] = os.path.join(_TMP, 's.db') os.environ['SOULSYNC_TEST_DB_READY'] = '1' @@ -20,58 +22,86 @@ def client(): return web_server.app.test_client() -def _seed(artist_id, *, missing, present, tmp_path): - """Artist with server_source NULL (skips the media-server pull phase), an - album, ``present`` tracks pointing at real files + ``missing`` at dead paths.""" +def _seed(artist_id, track_ids): + """Plex artist + album + tracks (server_source='plex').""" db = web_server.get_database() - album_id = artist_id * 10 + aid, album_id = str(artist_id), artist_id * 10 with db._get_connection() as conn: - conn.execute("INSERT OR REPLACE INTO artists (id, name, server_source) VALUES (?, ?, NULL)", - (artist_id, f'Artist {artist_id}')) + conn.execute("INSERT OR REPLACE INTO artists (id, name, server_source) VALUES (?, ?, 'plex')", + (aid, f'Artist {aid}')) conn.execute("INSERT OR REPLACE INTO albums (id, title, artist_id) VALUES (?, 'Alb', ?)", - (album_id, artist_id)) - tid = artist_id * 1000 - for i in range(present): - p = tmp_path / f"{artist_id}_present_{i}.flac" - p.write_bytes(b'\x00\x00') - conn.execute("INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path) " - "VALUES (?, ?, ?, ?, 1, 100, ?)", (tid, album_id, artist_id, f'P{i}', str(p))) - tid += 1 - for i in range(missing): - conn.execute("INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path) " - "VALUES (?, ?, ?, ?, 1, 100, ?)", (tid, album_id, artist_id, f'M{i}', f'/nonexistent/{artist_id}_{i}.flac')) - tid += 1 + (album_id, aid)) + for tid in track_ids: + conn.execute("INSERT OR REPLACE INTO tracks (id, album_id, artist_id, title, track_number, " + "duration, file_path, server_source) VALUES (?, ?, ?, ?, 1, 100, ?, 'plex')", + (tid, album_id, aid, f'T{tid}', f'/m/{tid}.flac')) conn.commit() -def _track_count(artist_id): +def _track_ids(artist_id): db = web_server.get_database() with db._get_connection() as conn: - return conn.execute("SELECT COUNT(*) c FROM tracks WHERE artist_id = ?", (artist_id,)).fetchone()['c'] + return {r['id'] for r in conn.execute("SELECT id FROM tracks WHERE artist_id = ?", (str(artist_id),))} -def test_all_files_missing_skips_removal_and_keeps_tracks(client, tmp_path): - _seed(9001, missing=8, present=0, tmp_path=tmp_path) - body = client.post('/api/library/artist/9001/sync').get_json() - assert body['success'] is True - assert body['removal_skipped'] is True # guard tripped +def _mock_server_pull(monkeypatch, *, seen, success=True): + """Fake the media server returning `seen` track IDs for the artist.""" + class _FakeServer: + def fetchItem(self, _id): + return SimpleNamespace(title=None) # truthy artist, no name change + + class _FakePlex: + server = _FakeServer() + + class _FakeEngine: + def client(self, name): + return _FakePlex() if name == 'plex' else None + + monkeypatch.setattr(web_server, 'media_server_engine', _FakeEngine()) + + class _FakeWorker: + def __init__(self, *a, **k): + self.database = None + def _process_artist_with_content(self, server_artist, skip_existing_tracks=False, seen_track_ids=None): + if seen_track_ids is not None: + seen_track_ids.update(seen) + return (success, 'ok', 0, 0) + + monkeypatch.setattr('core.database_update_worker.DatabaseUpdateWorker', _FakeWorker) + + +def test_removes_tracks_the_server_no_longer_has(client, monkeypatch): + _seed(7001, [f't{i}' for i in range(1, 11)]) # t1..t10 in DB + _mock_server_pull(monkeypatch, seen={f't{i}' for i in range(1, 9)}) # server has t1..t8 + body = client.post('/api/library/artist/7001/sync').get_json() + assert body['success'] and body['removal_skipped'] is False + assert body['stale_removed'] == 2 # t9,t10 gone from server + assert _track_ids(7001) == {f't{i}' for i in range(1, 9)} + + +def test_guard_skips_when_most_tracks_unseen(client, monkeypatch): + _seed(7002, [f't{i}' for i in range(1, 11)]) + _mock_server_pull(monkeypatch, seen={'t1'}) # 9/10 unseen → flaky response + body = client.post('/api/library/artist/7002/sync').get_json() + assert body['removal_skipped'] is True assert body['stale_removed'] == 0 - assert _track_count(9001) == 8 # nothing deleted — storage looked down + assert len(_track_ids(7002)) == 10 # nothing deleted -def test_a_few_missing_files_are_removed(client, tmp_path): - _seed(9002, missing=2, present=8, tmp_path=tmp_path) - body = client.post('/api/library/artist/9002/sync').get_json() - assert body['success'] is True - assert body['removal_skipped'] is False - assert body['stale_removed'] == 2 # only the genuinely-gone ones - assert _track_count(9002) == 8 +def test_failed_pull_skips_removal(client, monkeypatch): + _seed(7003, [f't{i}' for i in range(1, 11)]) + _mock_server_pull(monkeypatch, seen=set(), success=False) # pull failed → no trustworthy view + body = client.post('/api/library/artist/7003/sync').get_json() + assert body['removal_skipped'] is True + assert body['stale_removed'] == 0 + assert len(_track_ids(7003)) == 10 -def test_sync_is_admin_only(client, tmp_path): - _seed(9003, missing=2, present=2, tmp_path=tmp_path) +def test_sync_is_admin_only(client, monkeypatch): + _seed(7004, ['t1', 't2']) + _mock_server_pull(monkeypatch, seen={'t1', 't2'}) nonadmin = web_server.get_database().create_profile(name=f'u_{os.urandom(3).hex()}') with client.session_transaction() as sess: sess['profile_id'] = nonadmin - assert client.post('/api/library/artist/9003/sync').status_code == 403 - assert _track_count(9003) == 4 # untouched + assert client.post('/api/library/artist/7004/sync').status_code == 403 + assert len(_track_ids(7004)) == 2 # untouched diff --git a/web_server.py b/web_server.py index 9df659ad..4e2e1ac0 100644 --- a/web_server.py +++ b/web_server.py @@ -12271,6 +12271,11 @@ def sync_artist_library(artist_id): new_albums = 0 new_tracks = 0 name_updated = False + # Single-artist deep scan: collect the server track IDs we see during the + # pull. Stale removal (Phase 2) is a server-diff against this set — the SAME + # mechanism the whole-library deep scan uses, just scoped to one artist. + seen_track_ids = set() + pull_succeeded = False if server_source: media_client = None @@ -12325,68 +12330,73 @@ def sync_artist_library(artist_id): artist_name = new_name name_updated = True - # Process artist content (deep scan mode — skip existing, preserve enrichment) + # Process artist content (deep scan mode — skip existing, + # preserve enrichment) and collect the server's track IDs + # for this artist into seen_track_ids. success, details, new_albums, new_tracks = worker._process_artist_with_content( - server_artist, skip_existing_tracks=True + server_artist, skip_existing_tracks=True, seen_track_ids=seen_track_ids ) + # Only a successful pull gives a trustworthy 'seen' set; a + # failure/partial would make every track look stale. + pull_succeeded = bool(success) logger.info(f"[Artist Sync] Server pull for {artist_name}: {details}") except Exception as e: logger.error(f"[Artist Sync] Server pull failed for {artist_name}: {e}") - # ── Phase 2: Remove stale entries (files no longer on disk) ── + # ── Phase 2: Remove stale entries (tracks the server no longer has) ── + # Server-diff, exactly like the whole-library deep scan: stale = this + # artist's DB tracks that were NOT seen on the server during the pull. stale_removed = 0 empty_albums_removed = 0 removal_skipped = False - with database._get_connection() as conn: - cursor = conn.cursor() - cursor.execute("SELECT id, file_path FROM tracks WHERE artist_id = ?", (db_artist_id,)) - tracks = cursor.fetchall() - - stale_ids = [] - for track in tracks: - fp = track['file_path'] - if not fp: - stale_ids.append(track['id']) - continue - resolved = _resolve_library_file_path(fp) - if not resolved or not os.path.exists(resolved): - stale_ids.append(track['id']) - - # Storage-unreachable guard (#828 pattern): if an implausibly large - # fraction of this artist's tracks look missing, the disk/mount is - # almost certainly down — don't wipe the artist over a flaky mount. - from core.library.stale_guard import is_implausible_stale_removal - if is_implausible_stale_removal(len(stale_ids), len(tracks)): - removal_skipped = True - logger.warning( - f"[Artist Sync] {artist_name}: {len(stale_ids)}/{len(tracks)} tracks look " - f"missing — skipping stale removal (storage likely unreachable)" + if not pull_succeeded: + # No trustworthy server view (no server configured, unreachable, or the + # pull failed) — without it we can't tell stale from "server was down", + # so we remove nothing rather than risk wiping the artist. + removal_skipped = True + logger.info(f"[Artist Sync] {artist_name}: server pull unavailable — skipping stale removal") + else: + with database._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT id FROM tracks WHERE artist_id = ? AND server_source = ?", + (db_artist_id, server_source), ) - elif stale_ids: - placeholders = ','.join('?' for _ in stale_ids) - cursor.execute(f"DELETE FROM tracks WHERE id IN ({placeholders})", stale_ids) - stale_removed = len(stale_ids) + artist_track_ids = {row['id'] for row in cursor.fetchall()} + stale = artist_track_ids - seen_track_ids + + # Same safety net as deep scan (#828): if an implausibly large share + # of the artist's tracks went unseen, treat it as a flaky server + # response and skip rather than mass-delete. + from core.library.stale_guard import is_implausible_stale_removal + if is_implausible_stale_removal(len(stale), len(artist_track_ids)): + removal_skipped = True + logger.warning( + f"[Artist Sync] {artist_name}: {len(stale)}/{len(artist_track_ids)} tracks " + f"unseen on server — skipping stale removal (likely a flaky response)" + ) + elif stale: + stale_removed = database.delete_stale_tracks(stale, server_source) - # Only prune empty albums when we actually removed tracks — never on the - # skipped path, where "empty" would be a false reading of a down mount. - # ``album_id IS NOT NULL`` avoids the NOT IN-with-NULL gotcha that would - # otherwise no-op this cleanup whenever a track has a null album_id. if not removal_skipped: - cursor.execute(""" - DELETE FROM albums WHERE artist_id = ? - AND id NOT IN (SELECT DISTINCT album_id FROM tracks WHERE album_id IS NOT NULL) - """, (db_artist_id,)) - empty_albums_removed = cursor.rowcount - - cursor.execute(""" - UPDATE albums SET track_count = ( - SELECT COUNT(*) FROM tracks WHERE tracks.album_id = albums.id - ) WHERE artist_id = ? - """, (db_artist_id,)) - - conn.commit() + with database._get_connection() as conn: + cursor = conn.cursor() + # Prune albums left with no tracks. ``album_id IS NOT NULL`` + # avoids the NOT IN-with-NULL gotcha that would otherwise no-op + # this whenever a track has a null album_id. + cursor.execute(""" + DELETE FROM albums WHERE artist_id = ? + AND id NOT IN (SELECT DISTINCT album_id FROM tracks WHERE album_id IS NOT NULL) + """, (db_artist_id,)) + empty_albums_removed = cursor.rowcount + cursor.execute(""" + UPDATE albums SET track_count = ( + SELECT COUNT(*) FROM tracks WHERE tracks.album_id = albums.id + ) WHERE artist_id = ? + """, (db_artist_id,)) + conn.commit() logger.warning(f"[Artist Sync] {artist_name}: +{new_albums} albums, +{new_tracks} tracks, " f"-{stale_removed} stale, -{empty_albums_removed} empty albums" diff --git a/webui/static/library.js b/webui/static/library.js index e0731f18..4eaf6c03 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -3193,8 +3193,13 @@ function renderArtistMetaPanel(artist) { const data = await res.json(); if (data.success) { if (data.removal_skipped) { - // Storage looked unreachable — we deliberately did NOT delete. - showToast(`${data.artist_name}: most files looked missing — skipped removal in case your music storage is offline. Check it's mounted, then sync again.`, 'warning'); + // Couldn't get a trustworthy server view — we deliberately did NOT delete. + const parts = []; + if (data.new_albums > 0) parts.push(`+${data.new_albums} albums`); + if (data.new_tracks > 0) parts.push(`+${data.new_tracks} tracks`); + if (data.name_updated) parts.push('name updated'); + const added = parts.length ? ` (${parts.join(', ')})` : ''; + showToast(`${data.artist_name}: couldn't fully confirm against your media server — skipped removing tracks to be safe${added}.`, 'warning'); } else { const parts = []; if (data.new_albums > 0) parts.push(`+${data.new_albums} albums`); From aa3aae695ddf50e02a357c06064add5fbff7f12b Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 20:36:49 -0700 Subject: [PATCH 10/25] Security: opt-in reverse-proxy mode (ProxyFix + Secure cookie) + nginx guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tier 1 of "secure behind a reverse proxy". STRICTLY opt-in so direct/LAN installs are byte-for-byte unchanged. - core/security/reverse_proxy.py: apply_reverse_proxy_mode(app, config_get) — a no-op unless security.trust_reverse_proxy=true. When OFF (default), the app is untouched: no ProxyFix, X-Forwarded-* stays UNtrusted (a direct client can't spoof its IP/scheme), session cookie keeps Flask defaults. When ON (operator is behind nginx/Caddy/Traefik with TLS): trust one proxy hop's X-Forwarded-*, and mark the session cookie Secure + SameSite=Lax. Any config error → safe no-op, never breaks startup. - Wired once at app init. - Support/REVERSE-PROXY.md: nginx (with the Socket.IO Upgrade headers people always miss) / Caddy / Traefik configs, the setting, and the "put auth in front (Authelia/Authentik/oauth2-proxy)" recommendation + the off-for-plain-HTTP note. Tests: off (and missing-key, and a config exception) is a strict no-op — not ProxyFix-wrapped, cookie defaults intact; on wraps ProxyFix + secures the cookie; and the real web_server app is NOT in proxy mode by default. 5 tests pass. --- Support/REVERSE-PROXY.md | 122 ++++++++++++++++++++++++++++ core/security/reverse_proxy.py | 44 ++++++++++ tests/test_credentials_endpoints.py | 9 ++ tests/test_reverse_proxy_mode.py | 52 ++++++++++++ web_server.py | 8 ++ 5 files changed, 235 insertions(+) create mode 100644 Support/REVERSE-PROXY.md create mode 100644 core/security/reverse_proxy.py create mode 100644 tests/test_reverse_proxy_mode.py diff --git a/Support/REVERSE-PROXY.md b/Support/REVERSE-PROXY.md new file mode 100644 index 00000000..08050924 --- /dev/null +++ b/Support/REVERSE-PROXY.md @@ -0,0 +1,122 @@ +# Running SoulSync behind a reverse proxy (nginx / Caddy / Traefik) + +Putting SoulSync behind a reverse proxy lets you serve it over **HTTPS** and — the +important part — put **authentication** in front of it before exposing it to the +internet. This guide covers the safe setup. + +> **The golden rule:** the safest way to expose *any* self-hosted app publicly is +> to require authentication at the proxy (an auth layer), **not** to rely on the +> app's own protection. SoulSync's launch PIN is a useful fallback, but it is not +> a substitute for a real auth layer on a public instance. + +--- + +## 1. Turn on reverse-proxy mode + +By default SoulSync does **not** trust proxy headers (so a direct client can't spoof +its IP or pretend the connection is HTTPS). If you're behind a proxy that +terminates TLS, opt in by setting this in your `config.json`: + +```json +{ + "security": { + "trust_reverse_proxy": true + } +} +``` + +When enabled, SoulSync trusts `X-Forwarded-For/Proto/Host/Port` from **one** proxy +hop and marks its session cookie `Secure` (HTTPS-only) + `SameSite=Lax`. **Leave +it off if you access SoulSync directly over http:// on your LAN** — turning it on +would make the session cookie HTTPS-only and break plain-HTTP access. + +Restart SoulSync after changing it. + +--- + +## 2. nginx + +SoulSync uses WebSockets (Socket.IO), so the `Upgrade`/`Connection` headers are +**required** — without them live updates silently stop working. + +```nginx +server { + listen 443 ssl; + server_name soulsync.example.com; + + ssl_certificate /etc/letsencrypt/live/soulsync.example.com/fullchain.pem; + ssl_certificate_key /etc/letsencrypt/live/soulsync.example.com/privkey.pem; + + # Large library scans / uploads + client_max_body_size 0; + + location / { + proxy_pass http://127.0.0.1:8008; + + proxy_set_header Host $host; + proxy_set_header X-Real-IP $remote_addr; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-Proto $scheme; + proxy_set_header X-Forwarded-Host $host; + + # Required for Socket.IO / live updates + proxy_http_version 1.1; + proxy_set_header Upgrade $http_upgrade; + proxy_set_header Connection "upgrade"; + + proxy_read_timeout 3600s; # long-running scans + proxy_send_timeout 3600s; + } +} +``` + +--- + +## 3. Caddy + +Caddy handles TLS automatically and proxies WebSockets out of the box: + +```caddy +soulsync.example.com { + reverse_proxy 127.0.0.1:8008 +} +``` + +Caddy sets `X-Forwarded-*` for you. (Add an auth provider directive if you want +auth at the proxy — see below.) + +--- + +## 4. Traefik + +Traefik proxies WebSockets automatically and forwards the headers. Point a router +at the SoulSync service on port `8008` with your TLS resolver; no extra WebSocket +config is needed. + +--- + +## 5. Add authentication in front (recommended for public instances) + +Pick one: + +- **Auth proxy** — [Authelia](https://www.authelia.com/), + [Authentik](https://goauthentik.io/), or + [oauth2-proxy](https://oauth2-proxy.github.io/oauth2-proxy/). These sit in front + of SoulSync and force a login (with 2FA) before any request reaches it. Best + option for internet exposure. +- **HTTP Basic Auth** — quick and simple (nginx `auth_basic` / Caddy `basicauth`). + Better than nothing; weaker than an auth proxy. +- **SoulSync launch PIN** — set an admin PIN in Settings. Enforced server-side, so + it can't be bypassed by hitting the API directly — but it's a shared PIN, so + treat it as a fallback, not your only gate. + +--- + +## Troubleshooting + +- **Live updates / progress bars don't move** → the WebSocket `Upgrade`/`Connection` + headers are missing (nginx) or your proxy is buffering. Check section 2. +- **Login won't stick / "session expired"** → you enabled `trust_reverse_proxy` but + are reaching SoulSync over plain `http://`. The session cookie is now HTTPS-only; + use `https://`, or turn the setting off for direct HTTP access. +- **Scans time out** → raise `proxy_read_timeout` / `proxy_send_timeout`. diff --git a/core/security/reverse_proxy.py b/core/security/reverse_proxy.py new file mode 100644 index 00000000..f7733c9d --- /dev/null +++ b/core/security/reverse_proxy.py @@ -0,0 +1,44 @@ +"""Opt-in reverse-proxy mode. + +Default OFF. When off this is a strict no-op: the Flask app is left exactly as it +was, ``X-Forwarded-*`` headers are NOT trusted (so a direct client can't spoof its +IP/scheme), and the session cookie keeps Flask's defaults. So a normal direct / +LAN install is byte-for-byte unchanged. + +Only when the operator explicitly sets ``security.trust_reverse_proxy: true`` — +they're running behind nginx / Caddy / Traefik that terminates TLS — do we: + - trust the proxy's ``X-Forwarded-For/Proto/Host/Port`` (correct client IP, + HTTPS detection, redirects), and + - mark the session cookie ``Secure`` (HTTPS-only) + ``SameSite=Lax``. + +Gated this way the security/UX change is scoped strictly to people who turned it +on; everyone else is untouched. +""" + +from __future__ import annotations + +CONFIG_KEY = "security.trust_reverse_proxy" + + +def apply_reverse_proxy_mode(app, config_get) -> bool: + """Apply reverse-proxy hardening to ``app`` iff the operator enabled it. + + ``config_get`` is a ``config_manager.get``-style callable ``(key, default)``. + Returns True if proxy mode was enabled, False (no-op) otherwise. Never raises + out — a failure to enable falls back to the safe no-op behaviour. + """ + try: + if not config_get(CONFIG_KEY, False): + return False + from werkzeug.middleware.proxy_fix import ProxyFix + # Trust exactly one proxy hop for each forwarded header. + app.wsgi_app = ProxyFix(app.wsgi_app, x_for=1, x_proto=1, x_host=1, x_port=1) + app.config["SESSION_COOKIE_SECURE"] = True + app.config["SESSION_COOKIE_SAMESITE"] = "Lax" + return True + except Exception: + # If anything goes wrong, behave like off — never break startup over this. + return False + + +__all__ = ["apply_reverse_proxy_mode", "CONFIG_KEY"] diff --git a/tests/test_credentials_endpoints.py b/tests/test_credentials_endpoints.py index a4dba6b9..6f979779 100644 --- a/tests/test_credentials_endpoints.py +++ b/tests/test_credentials_endpoints.py @@ -346,3 +346,12 @@ def test_tidal_source_adapter_resolves_per_profile(): from core.playlists.sources.tidal import TidalPlaylistSource src = TidalPlaylistSource(web_server.get_tidal_client_for_profile) assert src._client() is web_server.tidal_client # admin -> global, unchanged + + +def test_real_app_not_in_reverse_proxy_mode_by_default(): + # Direct/LAN installs (no security.trust_reverse_proxy set) must not get + # ProxyFix or a forced-Secure cookie — proves zero impact for normal users. + from werkzeug.middleware.proxy_fix import ProxyFix + assert not isinstance(web_server.app.wsgi_app, ProxyFix) + assert web_server.app.config.get('SESSION_COOKIE_SECURE') in (None, False) + assert web_server.app.config.get('SESSION_COOKIE_SAMESITE') is None diff --git a/tests/test_reverse_proxy_mode.py b/tests/test_reverse_proxy_mode.py new file mode 100644 index 00000000..d938ef68 --- /dev/null +++ b/tests/test_reverse_proxy_mode.py @@ -0,0 +1,52 @@ +"""Opt-in reverse-proxy mode must be a STRICT no-op when off (default), so a +direct/LAN install is byte-for-byte unchanged, and only harden when enabled.""" + +from __future__ import annotations + +from flask import Flask +from werkzeug.middleware.proxy_fix import ProxyFix + +from core.security.reverse_proxy import apply_reverse_proxy_mode, CONFIG_KEY + + +def _cfg(value): + """A config_manager.get-style callable returning `value` for the proxy key.""" + return lambda key, default=None: value if key == CONFIG_KEY else default + + +def test_off_by_default_is_a_strict_noop(): + app = Flask(__name__) + + enabled = apply_reverse_proxy_mode(app, _cfg(False)) # default/off + + assert enabled is False + assert not isinstance(app.wsgi_app, ProxyFix) # NOT wrapped + # Flask defaults untouched — cookie not forced Secure, no SameSite override + assert app.config.get('SESSION_COOKIE_SECURE') in (None, False) + assert app.config.get('SESSION_COOKIE_SAMESITE') is None + + +def test_missing_key_is_also_a_noop(): + app = Flask(__name__) + assert apply_reverse_proxy_mode(app, lambda key, default=None: default) is False + assert not isinstance(app.wsgi_app, ProxyFix) + + +def test_on_wraps_proxyfix_and_secures_cookie(): + app = Flask(__name__) + + enabled = apply_reverse_proxy_mode(app, _cfg(True)) + + assert enabled is True + assert isinstance(app.wsgi_app, ProxyFix) # forwarded headers trusted + assert app.config['SESSION_COOKIE_SECURE'] is True # cookie HTTPS-only + assert app.config['SESSION_COOKIE_SAMESITE'] == 'Lax' + + +def test_failure_falls_back_to_noop(): + # A config_get that raises must not break startup — treated as off. + app = Flask(__name__) + def boom(key, default=None): + raise RuntimeError('config exploded') + assert apply_reverse_proxy_mode(app, boom) is False + assert not isinstance(app.wsgi_app, ProxyFix) diff --git a/web_server.py b/web_server.py index 4e2e1ac0..a69858b4 100644 --- a/web_server.py +++ b/web_server.py @@ -346,6 +346,14 @@ def _init_flask_secret_key(): app.secret_key = _init_flask_secret_key() +# --- Reverse-proxy mode (opt-in, default OFF) --- +# OFF by default → a strict no-op, so direct/LAN installs are unchanged. Only when +# the operator sets security.trust_reverse_proxy=true (behind nginx/Caddy/Traefik +# with TLS) does this trust X-Forwarded-* + mark the session cookie Secure. +from core.security.reverse_proxy import apply_reverse_proxy_mode as _apply_reverse_proxy_mode +if _apply_reverse_proxy_mode(app, config_manager.get): + logger.info("[Security] Reverse-proxy mode ON: trusting X-Forwarded-* and Secure session cookie") + # --- WebSocket (Socket.IO) Setup --- from core.socketio_cors import ( resolve_cors_origins as _resolve_socketio_cors_origins, From 0d1e949798b39e8af4b98afde51d46f2e70c66e1 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 20:47:46 -0700 Subject: [PATCH 11/25] Security: brute-force limiter on the launch-PIN unlock (Tier 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A publicly-exposed instance gated only by the launch PIN was brute-forceable. Added a lenient in-memory failed-attempt limiter (core/security/rate_limit.py): 10 wrong PINs from one IP within 5 min → 429 with Retry-After, failures age out on their own (self-heal, no persistent lockout), and a CORRECT entry clears that IP instantly. Wired into /api/profiles/verify-launch-pin. By design it can only ever trigger on a flood of WRONG PINs — correct entry, a couple of typos, or a no-PIN install are never affected, so normal use sees no change. Keyed per-IP so an attacker can't lock out a legit user. Tests: limiter is lenient under threshold, trips on a flood, success clears it, failures self-heal, per-IP isolation; endpoint returns 429 after 10 wrong PINs with Retry-After. 6 tests pass. --- core/security/rate_limit.py | 54 +++++++++++++++++++++++++++++ tests/test_credentials_endpoints.py | 24 +++++++++++++ tests/test_pin_rate_limit.py | 50 ++++++++++++++++++++++++++ web_server.py | 16 +++++++++ 4 files changed, 144 insertions(+) create mode 100644 core/security/rate_limit.py create mode 100644 tests/test_pin_rate_limit.py diff --git a/core/security/rate_limit.py b/core/security/rate_limit.py new file mode 100644 index 00000000..1fd7b56d --- /dev/null +++ b/core/security/rate_limit.py @@ -0,0 +1,54 @@ +"""Lenient in-memory failed-attempt limiter for the launch-PIN unlock. + +Brute-force protection for a publicly-exposed instance. Deliberately lenient: only +a FLOOD of failures from one client trips it, and a single success clears that +client immediately — so a legitimate user typing their PIN (even with a few typos) +never hits it. Failures age out on their own, so a tripped client self-heals +without any persistent lockout state. + +Keyed by client IP. In-memory is fine here: the launch lock is a coarse gate, not +per-account auth, and a process restart simply forgets attempts (fail-open, which +is correct for a self-hosted convenience lock). +""" + +from __future__ import annotations + +from collections import defaultdict +from typing import Dict, List, Tuple + + +class AttemptLimiter: + def __init__(self, max_attempts: int = 10, window_seconds: int = 300): + """``max_attempts`` failures within ``window_seconds`` → locked until the + oldest failure in the window ages out.""" + self.max_attempts = max_attempts + self.window = window_seconds + self._failures: Dict[str, List[float]] = defaultdict(list) + + def _prune(self, key: str, now: float) -> List[float]: + recent = [t for t in self._failures.get(key, []) if now - t < self.window] + if recent: + self._failures[key] = recent + else: + self._failures.pop(key, None) + return recent + + def is_locked(self, key: str, now: float) -> Tuple[bool, int]: + """(locked, retry_after_seconds). retry_after is when the oldest in-window + failure expires, so the client unlocks naturally.""" + recent = self._prune(key, now) + if len(recent) >= self.max_attempts: + retry_after = int(self.window - (now - min(recent))) + 1 + return True, max(retry_after, 1) + return False, 0 + + def record_failure(self, key: str, now: float) -> None: + self._prune(key, now) + self._failures[key].append(now) + + def record_success(self, key: str) -> None: + """A correct entry clears that client's failure history immediately.""" + self._failures.pop(key, None) + + +__all__ = ["AttemptLimiter"] diff --git a/tests/test_credentials_endpoints.py b/tests/test_credentials_endpoints.py index 6f979779..1da0a4f6 100644 --- a/tests/test_credentials_endpoints.py +++ b/tests/test_credentials_endpoints.py @@ -355,3 +355,27 @@ def test_real_app_not_in_reverse_proxy_mode_by_default(): assert not isinstance(web_server.app.wsgi_app, ProxyFix) assert web_server.app.config.get('SESSION_COOKIE_SECURE') in (None, False) assert web_server.app.config.get('SESSION_COOKIE_SAMESITE') is None + + +def test_verify_launch_pin_rate_limited_after_flood(client): + # A flood of WRONG PINs from one IP gets 429; cleaned up so neither the lock + # nor the temp PIN leaks to other tests (the limiter is a process singleton). + from werkzeug.security import generate_password_hash + db = web_server.get_database() + with db._get_connection() as conn: # admin needs a PIN so wrong ones actually fail + conn.execute("UPDATE profiles SET pin_hash = ? WHERE id = 1", + (generate_password_hash('1234', method='pbkdf2:sha256'),)) + conn.commit() + web_server._launch_pin_limiter.record_success('127.0.0.1') # clean slate + try: + for _ in range(10): + assert client.post('/api/profiles/verify-launch-pin', + json={'pin': 'definitely-wrong'}).status_code == 401 + r = client.post('/api/profiles/verify-launch-pin', json={'pin': 'definitely-wrong'}) + assert r.status_code == 429 + assert 'Retry-After' in r.headers + finally: + web_server._launch_pin_limiter.record_success('127.0.0.1') + with db._get_connection() as conn: + conn.execute("UPDATE profiles SET pin_hash = NULL WHERE id = 1") + conn.commit() diff --git a/tests/test_pin_rate_limit.py b/tests/test_pin_rate_limit.py new file mode 100644 index 00000000..54ee6656 --- /dev/null +++ b/tests/test_pin_rate_limit.py @@ -0,0 +1,50 @@ +"""Launch-PIN brute-force limiter: only a flood of WRONG PINs trips it, a correct +entry clears it instantly, and it self-heals as failures age out — so normal use +is never affected.""" + +from __future__ import annotations + +from core.security.rate_limit import AttemptLimiter + + +def test_under_threshold_is_never_locked(): + lim = AttemptLimiter(max_attempts=10, window_seconds=300) + for i in range(9): # 9 < 10 → never locked + lim.record_failure('1.2.3.4', now=1000 + i) + locked, _ = lim.is_locked('1.2.3.4', now=1010) + assert locked is False + + +def test_flood_trips_the_lock_with_retry_after(): + lim = AttemptLimiter(max_attempts=10, window_seconds=300) + for i in range(10): + lim.record_failure('1.2.3.4', now=1000 + i) + locked, retry_after = lim.is_locked('1.2.3.4', now=1010) + assert locked is True + assert retry_after > 0 + + +def test_success_clears_immediately(): + lim = AttemptLimiter(max_attempts=10, window_seconds=300) + for i in range(10): + lim.record_failure('1.2.3.4', now=1000 + i) + assert lim.is_locked('1.2.3.4', now=1010)[0] is True + lim.record_success('1.2.3.4') # correct PIN + assert lim.is_locked('1.2.3.4', now=1011)[0] is False + + +def test_failures_age_out_self_heal(): + lim = AttemptLimiter(max_attempts=10, window_seconds=300) + for i in range(10): + lim.record_failure('1.2.3.4', now=1000 + i) + assert lim.is_locked('1.2.3.4', now=1010)[0] is True + # well past the window → all failures expired → unlocked + assert lim.is_locked('1.2.3.4', now=2000)[0] is False + + +def test_per_ip_isolation(): + lim = AttemptLimiter(max_attempts=10, window_seconds=300) + for i in range(10): + lim.record_failure('attacker', now=1000 + i) + assert lim.is_locked('attacker', now=1010)[0] is True + assert lim.is_locked('legit-user', now=1010)[0] is False # not punished for someone else diff --git a/web_server.py b/web_server.py index a69858b4..4a4ea709 100644 --- a/web_server.py +++ b/web_server.py @@ -398,6 +398,11 @@ def inject_webui_assets(): 'vite_assets': build_webui_vite_assets, } +# Brute-force limiter for the launch-PIN unlock (lenient; only a flood of wrong +# PINs from one IP trips it — correct entry clears it instantly). +from core.security.rate_limit import AttemptLimiter as _AttemptLimiter +_launch_pin_limiter = _AttemptLimiter(max_attempts=10, window_seconds=300) + # --- Launch PIN gate (before_request hook) --- @app.before_request def _enforce_launch_pin(): @@ -25255,6 +25260,15 @@ def get_current_profile(): def verify_launch_pin(): """Verify PIN for launch lock screen""" try: + # Brute-force guard: only a flood of WRONG PINs from one IP trips this; a + # correct entry clears it instantly, so normal use is never affected. + _ip = request.remote_addr or 'unknown' + _now = time.time() + _locked, _retry_after = _launch_pin_limiter.is_locked(_ip, _now) + if _locked: + return (jsonify({'success': False, 'error': 'Too many attempts — please wait and try again'}), + 429, {'Retry-After': str(_retry_after)}) + data = request.json or {} pin = data.get('pin', '') if not pin: @@ -25263,8 +25277,10 @@ def verify_launch_pin(): database = get_database() # Validate against admin profile (ID 1) if not database.verify_profile_pin(1, pin): + _launch_pin_limiter.record_failure(_ip, _now) return jsonify({'success': False, 'error': 'Invalid PIN'}), 401 + _launch_pin_limiter.record_success(_ip) session['launch_pin_verified'] = True return jsonify({'success': True}) except Exception as e: From 5e5bc12e45b3d753507d162aaffdb9b8db68f928 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 20:48:51 -0700 Subject: [PATCH 12/25] Security: add gated security headers in reverse-proxy mode (Tier 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fold a conservative security-header set into the SAME opt-in proxy mode, so it's zero-impact when off. When security.trust_reverse_proxy is on, an after_request adds X-Content-Type-Options: nosniff, X-Frame-Options: SAMEORIGIN, and HSTS (safe — only honoured over the proxy's HTTPS), via setdefault so it never clobbers a header the proxy already set. No CSP (needs per-deploy tuning; better at the proxy). When OFF (default), the after_request isn't registered → no headers added. Tests: off adds none of the headers; on adds all three. Doc updated. 6 tests pass. --- Support/REVERSE-PROXY.md | 18 ++++++++++++++---- core/security/reverse_proxy.py | 16 ++++++++++++++++ tests/test_reverse_proxy_mode.py | 29 +++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/Support/REVERSE-PROXY.md b/Support/REVERSE-PROXY.md index 08050924..acf8e488 100644 --- a/Support/REVERSE-PROXY.md +++ b/Support/REVERSE-PROXY.md @@ -25,10 +25,20 @@ terminates TLS, opt in by setting this in your `config.json`: } ``` -When enabled, SoulSync trusts `X-Forwarded-For/Proto/Host/Port` from **one** proxy -hop and marks its session cookie `Secure` (HTTPS-only) + `SameSite=Lax`. **Leave -it off if you access SoulSync directly over http:// on your LAN** — turning it on -would make the session cookie HTTPS-only and break plain-HTTP access. +When enabled, SoulSync: +- trusts `X-Forwarded-For/Proto/Host/Port` from **one** proxy hop (correct client + IP, HTTPS detection, redirects), +- marks its session cookie `Secure` (HTTPS-only) + `SameSite=Lax`, and +- sends conservative security headers (`X-Content-Type-Options: nosniff`, + `X-Frame-Options: SAMEORIGIN`, `Strict-Transport-Security`). No CSP is set — tune + one at your proxy if you want it. + +**Leave it off if you access SoulSync directly over http:// on your LAN** — turning +it on would make the session cookie HTTPS-only and break plain-HTTP access. With it +off, none of the above applies and SoulSync behaves exactly as before. + +> The launch PIN is also brute-force limited (10 wrong attempts from an IP → a +> short cooldown), regardless of this setting — a correct PIN is never affected. Restart SoulSync after changing it. diff --git a/core/security/reverse_proxy.py b/core/security/reverse_proxy.py index f7733c9d..ab1a1689 100644 --- a/core/security/reverse_proxy.py +++ b/core/security/reverse_proxy.py @@ -35,6 +35,22 @@ def apply_reverse_proxy_mode(app, config_get) -> bool: app.wsgi_app = ProxyFix(app.wsgi_app, x_for=1, x_proto=1, x_host=1, x_port=1) app.config["SESSION_COOKIE_SECURE"] = True app.config["SESSION_COOKIE_SAMESITE"] = "Lax" + + # Security headers — registered ONLY in proxy mode (so a direct/LAN install + # gets none of them). Conservative set that won't break a same-origin app: + # nosniff, clickjacking protection, and HSTS (safe: only honoured over the + # HTTPS the proxy terminates). No CSP here — it needs per-deployment tuning + # and is better added at the proxy. setdefault() so we never clobber a + # header the proxy already set. + @app.after_request + def _security_headers(response): + response.headers.setdefault("X-Content-Type-Options", "nosniff") + response.headers.setdefault("X-Frame-Options", "SAMEORIGIN") + response.headers.setdefault( + "Strict-Transport-Security", "max-age=31536000; includeSubDomains" + ) + return response + return True except Exception: # If anything goes wrong, behave like off — never break startup over this. diff --git a/tests/test_reverse_proxy_mode.py b/tests/test_reverse_proxy_mode.py index d938ef68..c69abe24 100644 --- a/tests/test_reverse_proxy_mode.py +++ b/tests/test_reverse_proxy_mode.py @@ -50,3 +50,32 @@ def test_failure_falls_back_to_noop(): raise RuntimeError('config exploded') assert apply_reverse_proxy_mode(app, boom) is False assert not isinstance(app.wsgi_app, ProxyFix) + + +def test_off_adds_no_security_headers(): + app = Flask(__name__) + apply_reverse_proxy_mode(app, _cfg(False)) + + @app.route('/ping') + def _ping(): + return 'ok' + + resp = app.test_client().get('/ping') + # direct/LAN install: none of the proxy-mode headers are added + assert 'X-Content-Type-Options' not in resp.headers + assert 'X-Frame-Options' not in resp.headers + assert 'Strict-Transport-Security' not in resp.headers + + +def test_on_adds_security_headers(): + app = Flask(__name__) + apply_reverse_proxy_mode(app, _cfg(True)) + + @app.route('/ping') + def _ping(): + return 'ok' + + resp = app.test_client().get('/ping') + assert resp.headers.get('X-Content-Type-Options') == 'nosniff' + assert resp.headers.get('X-Frame-Options') == 'SAMEORIGIN' + assert 'max-age=' in resp.headers.get('Strict-Transport-Security', '') From 86d0a0dd62d245ff9d64523c7c29bdb525b6ecac Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 20:57:48 -0700 Subject: [PATCH 13/25] Security: trust a forward-auth proxy user header (Tier 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lets SoulSync sit behind Authelia/Authentik/oauth2-proxy as the gatekeeper: when security.auth_proxy_header names a header (e.g. Remote-User), a request carrying it is treated as already-authenticated and passes the launch lock — the proxy did the login (with 2FA). - core/security/auth_proxy.py: trusted_proxy_user(get_header, header_name) — returns the user iff the configured header is present + non-empty; empty header name (the default) → always None → feature off. - _enforce_launch_pin ORs it into pin_verified. OFF by default, so a direct install is unaffected AND a client-spoofed header does nothing unless the operator opted in. - Doc'd in Support/REVERSE-PROXY.md with the must-strip-client-headers warning. This is the lightweight Tier 3 (auth-proxy integration), not a full per-user login — the proxy owns identity; SoulSync trusts it. Tests: helper off/on/blank/exception-safe; integration — trusted header passes the gate, no header is locked, and (the safety pin) a spoofed header is IGNORED when the feature is off. 6 tests pass. --- Support/REVERSE-PROXY.md | 12 +++++++++ core/security/auth_proxy.py | 40 +++++++++++++++++++++++++++++ tests/test_auth_proxy.py | 31 ++++++++++++++++++++++ tests/test_credentials_endpoints.py | 32 +++++++++++++++++++++++ web_server.py | 13 +++++++++- 5 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 core/security/auth_proxy.py create mode 100644 tests/test_auth_proxy.py diff --git a/Support/REVERSE-PROXY.md b/Support/REVERSE-PROXY.md index acf8e488..45f77fb4 100644 --- a/Support/REVERSE-PROXY.md +++ b/Support/REVERSE-PROXY.md @@ -114,6 +114,18 @@ Pick one: [oauth2-proxy](https://oauth2-proxy.github.io/oauth2-proxy/). These sit in front of SoulSync and force a login (with 2FA) before any request reaches it. Best option for internet exposure. + + SoulSync can **trust the proxy's authenticated-user header** so the launch PIN is + skipped once the proxy has logged you in. Set the header name in `config.json`: + + ```json + { "security": { "auth_proxy_header": "Remote-User" } } + ``` + + > ⚠️ **Only enable this behind a proxy you control that STRIPS any client-supplied + > copy of that header.** Otherwise a direct visitor could send `Remote-User: admin` + > and walk straight in. It's **off by default** — an unset header name means + > SoulSync ignores the header entirely (a spoofed one does nothing). - **HTTP Basic Auth** — quick and simple (nginx `auth_basic` / Caddy `basicauth`). Better than nothing; weaker than an auth proxy. - **SoulSync launch PIN** — set an admin PIN in Settings. Enforced server-side, so diff --git a/core/security/auth_proxy.py b/core/security/auth_proxy.py new file mode 100644 index 00000000..f59c6824 --- /dev/null +++ b/core/security/auth_proxy.py @@ -0,0 +1,40 @@ +"""Trust an authenticated-user header from a forward-auth proxy. + +When SoulSync sits behind an auth proxy (Authelia / Authentik / oauth2-proxy), the +proxy authenticates the user and passes their identity in a header (commonly +``Remote-User``). With ``security.auth_proxy_header`` set to that header name, +SoulSync treats a request carrying it as already-authenticated and lets it past the +launch lock — the proxy is the gatekeeper. + +OFF by default (empty header name) → a strict no-op; the launch PIN behaves exactly +as before. + +⚠️ SECURITY: only enable this behind a proxy you control that STRIPS any +client-supplied copy of the header. Otherwise a direct client could send +``Remote-User: admin`` and walk straight in. This is why it's opt-in and never on +by default. +""" + +from __future__ import annotations + +from typing import Callable, Optional + + +def trusted_proxy_user(get_header: Callable[[str], Optional[str]], + header_name: str) -> Optional[str]: + """Return the authenticated username from the configured proxy header, or None. + + ``get_header`` is a ``request.headers.get``-style callable. ``header_name`` is + the configured header (e.g. ``Remote-User``); empty/None disables the feature + (always returns None), so a non-proxy install is unaffected. + """ + if not header_name: + return None + try: + value = (get_header(header_name) or "").strip() + except Exception: + return None + return value or None + + +__all__ = ["trusted_proxy_user"] diff --git a/tests/test_auth_proxy.py b/tests/test_auth_proxy.py new file mode 100644 index 00000000..2883bf73 --- /dev/null +++ b/tests/test_auth_proxy.py @@ -0,0 +1,31 @@ +"""Forward-auth proxy header trust (Tier 3): OFF by default → no-op; when the +operator configures a header, a request carrying it is treated as authenticated.""" + +from __future__ import annotations + +from core.security.auth_proxy import trusted_proxy_user + + +def _headers(d): + return lambda name: d.get(name) + + +def test_off_when_no_header_configured(): + # empty header name → feature disabled → always None (direct install unaffected) + assert trusted_proxy_user(_headers({'Remote-User': 'alice'}), '') is None + assert trusted_proxy_user(_headers({'Remote-User': 'alice'}), None) is None + + +def test_returns_user_when_header_present(): + assert trusted_proxy_user(_headers({'Remote-User': 'alice'}), 'Remote-User') == 'alice' + + +def test_none_when_configured_header_absent_or_blank(): + assert trusted_proxy_user(_headers({}), 'Remote-User') is None + assert trusted_proxy_user(_headers({'Remote-User': ' '}), 'Remote-User') is None + + +def test_get_header_exception_is_safe(): + def boom(_name): + raise RuntimeError('header lookup blew up') + assert trusted_proxy_user(boom, 'Remote-User') is None diff --git a/tests/test_credentials_endpoints.py b/tests/test_credentials_endpoints.py index 1da0a4f6..7cfebc6f 100644 --- a/tests/test_credentials_endpoints.py +++ b/tests/test_credentials_endpoints.py @@ -379,3 +379,35 @@ def test_verify_launch_pin_rate_limited_after_flood(client): with db._get_connection() as conn: conn.execute("UPDATE profiles SET pin_hash = NULL WHERE id = 1") conn.commit() + + +def test_auth_proxy_header_satisfies_launch_lock(client, monkeypatch): + # Lock on + Remote-User trusted → a request with the header passes the gate. + real_get = web_server.config_manager.get + def fake_get(key, default=None): + if key == 'security.require_pin_on_launch': + return True + if key == 'security.auth_proxy_header': + return 'Remote-User' + return real_get(key, default) + monkeypatch.setattr(web_server.config_manager, 'get', fake_get) + + assert client.get('/api/profiles/me/connections').status_code == 401 # no header → locked + assert client.get('/api/profiles/me/connections', + headers={'Remote-User': 'alice'}).status_code == 200 # trusted → in + + +def test_spoofed_auth_proxy_header_ignored_when_feature_off(client, monkeypatch): + # THE safety pin: feature OFF (default) → a client-sent Remote-User must NOT + # bypass the lock. Only an operator who explicitly configured it gets the trust. + real_get = web_server.config_manager.get + def fake_get(key, default=None): + if key == 'security.require_pin_on_launch': + return True + if key == 'security.auth_proxy_header': + return '' # OFF (default) + return real_get(key, default) + monkeypatch.setattr(web_server.config_manager, 'get', fake_get) + + assert client.get('/api/profiles/me/connections', + headers={'Remote-User': 'admin'}).status_code == 401 # spoof ignored → still locked diff --git a/web_server.py b/web_server.py index 4a4ea709..c85c46f4 100644 --- a/web_server.py +++ b/web_server.py @@ -421,10 +421,21 @@ def _enforce_launch_pin(): if not require_pin: return from core.security.launch_lock import request_is_locked, is_html_navigation + # An auth proxy (Authelia/Authentik/oauth2-proxy) that already authenticated the + # user counts as verified — opt-in via security.auth_proxy_header, OFF (empty) + # by default so a direct install is unaffected. + from core.security.auth_proxy import trusted_proxy_user + try: + _proxy_header = config_manager.get('security.auth_proxy_header', '') or '' + except Exception: + _proxy_header = '' + _verified = bool(session.get('launch_pin_verified', False)) or bool( + trusted_proxy_user(request.headers.get, _proxy_header) + ) if request_is_locked( request.path, request.method, require_pin=require_pin, - pin_verified=bool(session.get('launch_pin_verified', False)), + pin_verified=_verified, ): # A browser navigating to a sub-page (deep link / refresh) should land # on the lock screen, not raw JSON — bounce it to the root, which serves From fb8c8a71c698d1a7810e7d035edbc8c64c93ff5b Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 21:17:26 -0700 Subject: [PATCH 14/25] Security UI: Settings toggles for reverse-proxy mode + auth-proxy header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Config is DB-backed (metadata.app_config) — there is no config.json — so the reverse-proxy settings I added earlier had NO way to be set by a user and were effectively dead. Added them to Settings → Security, next to the launch-PIN toggle: - "Behind a reverse proxy" checkbox (security.trust_reverse_proxy) — help text notes it's for nginx/Caddy/Traefik+TLS, to leave OFF for direct/LAN http://, and that it needs a restart (applied at app init). - "Auth proxy user header" field (security.auth_proxy_header) — e.g. Remote-User, with the must-strip-client-headers warning; blank = off. Wired into the existing settings load + save; the save loop already persists every key in the security object via config_manager.set, so no backend change needed. Fixed Support/REVERSE-PROXY.md to point at Settings → Security instead of a nonexistent config.json. Off by default → zero impact for direct users. 64 script-split integrity tests pass. --- Support/REVERSE-PROXY.md | 18 ++++-------------- webui/index.html | 18 ++++++++++++++++++ webui/static/settings.js | 8 ++++++++ 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/Support/REVERSE-PROXY.md b/Support/REVERSE-PROXY.md index 45f77fb4..1e6152ff 100644 --- a/Support/REVERSE-PROXY.md +++ b/Support/REVERSE-PROXY.md @@ -15,15 +15,8 @@ internet. This guide covers the safe setup. By default SoulSync does **not** trust proxy headers (so a direct client can't spoof its IP or pretend the connection is HTTPS). If you're behind a proxy that -terminates TLS, opt in by setting this in your `config.json`: - -```json -{ - "security": { - "trust_reverse_proxy": true - } -} -``` +terminates TLS, turn on **Settings → Security → "Behind a reverse proxy"** and +**restart SoulSync** (this option applies at startup). When enabled, SoulSync: - trusts `X-Forwarded-For/Proto/Host/Port` from **one** proxy hop (correct client @@ -116,11 +109,8 @@ Pick one: option for internet exposure. SoulSync can **trust the proxy's authenticated-user header** so the launch PIN is - skipped once the proxy has logged you in. Set the header name in `config.json`: - - ```json - { "security": { "auth_proxy_header": "Remote-User" } } - ``` + skipped once the proxy has logged you in. Set the header name in **Settings → + Security → "Auth proxy user header"** (e.g. `Remote-User`). > ⚠️ **Only enable this behind a proxy you control that STRIPS any client-supplied > copy of that header.** Otherwise a direct visitor could send `Remote-User: admin` diff --git a/webui/index.html b/webui/index.html index 0fdd3e47..3d90c7e6 100644 --- a/webui/index.html +++ b/webui/index.html @@ -5993,6 +5993,24 @@ Origins (full URL, no trailing slash) allowed to open WebSocket connections to this instance — one per line, or comma-separated. Leave empty for same-origin only (the secure default; works for direct access and most reverse-proxy setups). Add your public domain here if you reach SoulSync via a reverse proxy or custom domain and the WebSocket fails to connect. Use * on its own line to allow any origin (insecure — only do this if you understand why you need it). + +
+ +
+ Enable only if SoulSync runs behind nginx / Caddy / Traefik that terminates HTTPS. Trusts the proxy's X-Forwarded-* headers, marks the session cookie HTTPS-only, and adds security headers. Leave OFF for direct or LAN access over http:// — turning it on would make the session cookie HTTPS-only and break plain-HTTP access. Takes effect after a restart. See Support/REVERSE-PROXY.md. +
+
+ +
+ + +
+ If an auth proxy (Authelia / Authentik / oauth2-proxy) logs users in in front of SoulSync, enter the header it sets (e.g. Remote-User) and SoulSync will skip the launch PIN for already-authenticated requests. Only set this behind a proxy that strips any client-supplied copy of the header — otherwise it can be spoofed. Leave blank to disable (the default). +
+
diff --git a/webui/static/settings.js b/webui/static/settings.js index 30288c68..1a1a77ac 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -1398,6 +1398,12 @@ async function loadSettingsData() { const corsField = document.getElementById('security-cors-origins'); if (corsField) corsField.value = corsOrigins; + // Reverse-proxy mode + auth-proxy header (default off / empty). + const trustProxy = document.getElementById('security-trust-proxy'); + if (trustProxy) trustProxy.checked = settings.security?.trust_reverse_proxy || false; + const authHeader = document.getElementById('security-auth-proxy-header'); + if (authHeader) authHeader.value = settings.security?.auth_proxy_header || ''; + // Check if admin has a PIN set const profilesRes = await fetch('/api/profiles'); const profilesData = await profilesRes.json(); @@ -3146,6 +3152,8 @@ async function saveSettings(quiet = false) { security: { require_pin_on_launch: document.getElementById('security-require-pin')?.checked || false, cors_origins: document.getElementById('security-cors-origins')?.value?.trim() || '', + trust_reverse_proxy: document.getElementById('security-trust-proxy')?.checked || false, + auth_proxy_header: document.getElementById('security-auth-proxy-header')?.value?.trim() || '', } }; From 8e1b678d6f45fc473440a259dc93272f632c01a9 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 21:57:44 -0700 Subject: [PATCH 15/25] Native login (increment 1/3): per-profile password DB layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opt-in username/password login — profiles become real accounts. This is the data layer: a per-profile login password, kept SEPARATE from the quick-switch PIN (different security purpose; a 4-digit PIN must not become the password guarding a public instance). - Additive migration: profiles.password_hash column (idempotent, metadata-flagged). - set_profile_password / verify_profile_password / profile_has_password / get_profile_by_name (the login username = profile name, unique + case-insensitive). - Security default: a profile with NO password is NOT loginable (verify returns False) — unlike the PIN where "no PIN = always valid". You can't authenticate to an account with no credential. Tests: migration adds the column; set/verify; no-password-never-loginable; clearing; name lookup; and password is fully independent of the PIN. 6 tests pass. Next: the login endpoint + require_login gate (increment 2). --- database/music_database.py | 80 ++++++++++++++++++++++++++++++++++ tests/test_profile_password.py | 67 ++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 tests/test_profile_password.py diff --git a/database/music_database.py b/database/music_database.py index 3c2ebf86..88f23593 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -460,6 +460,7 @@ class MusicDatabase: self._add_profile_support_v4(cursor) self._add_profile_settings(cursor) self._add_profile_listenbrainz_support(cursor) + self._add_profile_password_support(cursor) self._add_profile_service_credentials(cursor) self._add_service_credential_sets(cursor) self._add_soul_id_columns(cursor) @@ -5375,6 +5376,85 @@ class MusicDatabase: logger.error(f"Error verifying PIN for profile {profile_id}: {e}") return False + # ── Per-profile LOGIN password (opt-in username/password mode) ──────────── + # Separate from the quick-switch PIN on purpose: the PIN is a low-stakes + # convenience on a trusted LAN; the password authenticates an account for + # public exposure. Conflating them would make logins as weak as a 4-digit PIN. + + def set_profile_password(self, profile_id: int, password: str) -> bool: + """Set (or clear, when password is falsy) a profile's login password.""" + try: + from werkzeug.security import generate_password_hash + pwd_hash = generate_password_hash(password, method='pbkdf2:sha256') if password else None + with self._get_connection() as conn: + conn.execute("UPDATE profiles SET password_hash = ? WHERE id = ?", (pwd_hash, profile_id)) + conn.commit() + return True + except Exception as e: + logger.error(f"Error setting password for profile {profile_id}: {e}") + return False + + def verify_profile_password(self, profile_id: int, password: str) -> bool: + """Verify a profile's login password. Unlike the PIN, a profile with NO + password set is NOT loginable (returns False) — you can't authenticate to + an account that has no credential.""" + try: + from werkzeug.security import check_password_hash + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute("SELECT password_hash FROM profiles WHERE id = ?", (profile_id,)) + row = cursor.fetchone() + if not row or not row['password_hash']: + return False # no password set → cannot log in + return check_password_hash(row['password_hash'], password) + except Exception as e: + logger.error(f"Error verifying password for profile {profile_id}: {e}") + return False + + def profile_has_password(self, profile_id: int) -> bool: + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute("SELECT password_hash FROM profiles WHERE id = ?", (profile_id,)) + row = cursor.fetchone() + return bool(row and row['password_hash']) + except Exception as e: + logger.error(f"Error checking password for profile {profile_id}: {e}") + return False + + def get_profile_by_name(self, name: str) -> Optional[Dict[str, Any]]: + """Look up a profile by name (the login username), case-insensitive.""" + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT id, name, is_admin FROM profiles WHERE LOWER(name) = LOWER(?)", + (name or '',)) + row = cursor.fetchone() + if not row: + return None + return {'id': row['id'], 'name': row['name'], 'is_admin': bool(row['is_admin'])} + except Exception as e: + logger.error(f"Error looking up profile by name '{name}': {e}") + return None + + def _add_profile_password_support(self, cursor): + """Add a per-profile login password column (separate from pin_hash).""" + try: + cursor.execute("SELECT value FROM metadata WHERE key = 'profiles_password_v1' LIMIT 1") + if cursor.fetchone(): + return # Already migrated + logger.info("Applying per-profile login-password migration...") + try: + cursor.execute("ALTER TABLE profiles ADD COLUMN password_hash TEXT DEFAULT NULL") + except sqlite3.OperationalError: + pass # Column already exists + cursor.execute( + "INSERT OR REPLACE INTO metadata (key, value) VALUES ('profiles_password_v1', '1')") + logger.info("Per-profile login-password migration completed") + except Exception as e: + logger.error(f"Error in login-password migration: {e}") + def close(self): """Close database connection (no-op since we create connections per operation)""" # Each operation creates and closes its own connection, so nothing to do here diff --git a/tests/test_profile_password.py b/tests/test_profile_password.py new file mode 100644 index 00000000..798e6d91 --- /dev/null +++ b/tests/test_profile_password.py @@ -0,0 +1,67 @@ +"""Per-profile LOGIN password (opt-in username/password mode) — DB layer. + +Separate from the quick-switch PIN: a profile with no password set is NOT +loginable (you can't authenticate to an account with no credential), unlike the +PIN where 'no PIN = always valid'. +""" + +from __future__ import annotations + +import pytest + +from database.music_database import MusicDatabase + + +@pytest.fixture +def db(tmp_path): + return MusicDatabase(str(tmp_path / "music.db")) + + +def test_migration_adds_password_hash_column(db): + with db._get_connection() as conn: + cols = [r[1] for r in conn.execute("PRAGMA table_info(profiles)").fetchall()] + assert 'password_hash' in cols + + +def test_set_and_verify_password(db): + pid = db.create_profile(name='Brock') + assert db.profile_has_password(pid) is False # none yet + assert db.verify_profile_password(pid, 'hunter2') is False # no password → not loginable + + db.set_profile_password(pid, 'hunter2') + assert db.profile_has_password(pid) is True + assert db.verify_profile_password(pid, 'hunter2') is True + assert db.verify_profile_password(pid, 'wrong') is False + + +def test_no_password_is_never_loginable(db): + # Unlike the PIN (no PIN = always valid), a passwordless account can't log in. + pid = db.create_profile(name='NoPass') + assert db.verify_profile_password(pid, '') is False + assert db.verify_profile_password(pid, 'anything') is False + + +def test_clearing_password(db): + pid = db.create_profile(name='Temp') + db.set_profile_password(pid, 'pw') + assert db.profile_has_password(pid) is True + db.set_profile_password(pid, '') # clear + assert db.profile_has_password(pid) is False + assert db.verify_profile_password(pid, 'pw') is False + + +def test_get_profile_by_name_case_insensitive(db): + pid = db.create_profile(name='Daughter') + assert db.get_profile_by_name('daughter')['id'] == pid + assert db.get_profile_by_name('DAUGHTER')['id'] == pid + assert db.get_profile_by_name('nobody') is None + + +def test_password_is_independent_of_pin(db): + # Setting a password must not touch the PIN and vice-versa (separate creds). + from werkzeug.security import generate_password_hash + pid = db.create_profile(name='Both', pin_hash=generate_password_hash('1234', method='pbkdf2:sha256')) + db.set_profile_password(pid, 'longpassword') + assert db.verify_profile_pin(pid, '1234') is True # PIN still works + assert db.verify_profile_password(pid, 'longpassword') is True # password works + assert db.verify_profile_password(pid, '1234') is False # PIN is NOT the password From 92cbef90f9286cb451080845a90bee4c8d642d07 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 22:01:53 -0700 Subject: [PATCH 16/25] Native login (increment 2/3): login/logout endpoints + require_login gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backend auth for opt-in username/password mode (security.require_login, default off → zero change; the launch PIN + picker behave exactly as today). - core/security/login_gate.py: pure gate (mirrors launch_lock) — when login mode is on, an unauthenticated session reaches only the page shell, /api/auth/login, /api/auth/logout, /api/profiles/current, /api/setup/status, and the key-authed /api/v1 API. Deliberately does NOT expose the profile list pre-auth (you type your name, not pick from a roster). - _enforce_login before_request enforces it; _enforce_launch_pin no-ops when login mode is on (login replaces the shared PIN, per design). - POST /api/auth/login (username = profile name, case-insensitive; brute-force limited per IP; generic error so names don't leak) + POST /api/auth/logout. - Anti-lockout: the settings save refuses to turn ON login mode until the admin account has a password. Tests: gate blocks→login→access→logout→blocked; case-insensitive username; wrong password / passwordless profile / unknown user all 401 generically; login list not exposed pre-auth; can't enable login without an admin password. 12 tests pass. Next: the login screen + set-password UI + the toggle (increment 3). --- core/security/login_gate.py | 53 ++++++++++++++++++++ tests/test_login_endpoints.py | 82 ++++++++++++++++++++++++++++++ tests/test_login_gate.py | 36 ++++++++++++++ web_server.py | 93 +++++++++++++++++++++++++++++++++++ 4 files changed, 264 insertions(+) create mode 100644 core/security/login_gate.py create mode 100644 tests/test_login_endpoints.py create mode 100644 tests/test_login_gate.py diff --git a/core/security/login_gate.py b/core/security/login_gate.py new file mode 100644 index 00000000..bf521df4 --- /dev/null +++ b/core/security/login_gate.py @@ -0,0 +1,53 @@ +"""Pure gate decision for opt-in username/password login mode. + +When ``security.require_login`` is on, every request must come from an +authenticated session; unauthenticated requests are blocked except the page shell, +the login/logout flow, and the key-authed public API. This is the per-user +equivalent of (and replacement for) the shared launch-PIN gate. + +Deliberately does NOT allowlist the profile LIST or picker — in login mode you log +in by typing your name + password, you don't pick from an exposed roster. +""" + +from __future__ import annotations + +# GET endpoints the login screen itself needs before auth. +_ALLOWED_GET = frozenset({ + '/api/profiles/current', # how the frontend detects login state + '/api/setup/status', # first-run check runs before the login screen +}) + +# POST endpoints that drive the login flow. +_ALLOWED_POST = frozenset({ + '/api/auth/login', + '/api/auth/logout', +}) + + +def login_request_is_blocked(path: str, method: str, *, + require_login: bool, authenticated: bool) -> bool: + """True when the login gate must reject this request (login mode on + the + session isn't authenticated and the path isn't part of the login flow).""" + if not require_login or authenticated: + return False + + path = path or '' + method = (method or 'GET').upper() + + # Page shell + assets needed to render the login screen. + if path == '/' or path.startswith('/static/') or path.startswith('/favicon'): + return False + + # Key-authed public API governs itself (its own key auth). + if path.startswith('/api/v1/') and not path.startswith('/api/v1/api-keys-internal'): + return False + + if method == 'GET' and path in _ALLOWED_GET: + return False + if method == 'POST' and path in _ALLOWED_POST: + return False + + return True + + +__all__ = ['login_request_is_blocked'] diff --git a/tests/test_login_endpoints.py b/tests/test_login_endpoints.py new file mode 100644 index 00000000..4b9854b0 --- /dev/null +++ b/tests/test_login_endpoints.py @@ -0,0 +1,82 @@ +"""Username/password login endpoints + gate (opt-in login mode).""" + +from __future__ import annotations + +import os +import tempfile + +import pytest + +_TMP = tempfile.mkdtemp(prefix='soulsync-testdb-login-') +os.environ['DATABASE_PATH'] = os.path.join(_TMP, 'l.db') +os.environ['SOULSYNC_TEST_DB_READY'] = '1' + +web_server = pytest.importorskip('web_server') + + +@pytest.fixture +def client(): + return web_server.app.test_client() + + +def _enable_login(monkeypatch): + real_get = web_server.config_manager.get + monkeypatch.setattr(web_server.config_manager, 'get', + lambda k, d=None: True if k == 'security.require_login' else real_get(k, d)) + web_server._login_limiter.record_success('127.0.0.1') # clean slate + + +_GATED = '/api/profiles/me/connections' # a normal, non-allowlisted endpoint + + +def test_login_gate_blocks_then_authenticated_access(client, monkeypatch): + db = web_server.get_database() + pid = db.create_profile(name='LoginUser') + db.set_profile_password(pid, 'secretpw') + _enable_login(monkeypatch) + + assert client.get(_GATED).status_code == 401 # not logged in → blocked + + r = client.post('/api/auth/login', json={'username': 'LoginUser', 'password': 'secretpw'}) + assert r.status_code == 200 and r.get_json()['success'] is True + + assert client.get(_GATED).status_code == 200 # authenticated → in + + assert client.post('/api/auth/logout').get_json()['success'] is True + assert client.get(_GATED).status_code == 401 # logged out → blocked again + + +def test_login_is_case_insensitive_on_username(client, monkeypatch): + db = web_server.get_database() + db.set_profile_password(db.create_profile(name='CaseUser'), 'pw') + _enable_login(monkeypatch) + assert client.post('/api/auth/login', json={'username': 'caseuser', 'password': 'pw'}).status_code == 200 + + +def test_wrong_password_401_generic(client, monkeypatch): + db = web_server.get_database() + db.set_profile_password(db.create_profile(name='WrongPwUser'), 'right') + _enable_login(monkeypatch) + r = client.post('/api/auth/login', json={'username': 'WrongPwUser', 'password': 'nope'}) + assert r.status_code == 401 + assert 'username or password' in r.get_json()['error'].lower() # generic — no name-leak + + +def test_passwordless_profile_cannot_login(client, monkeypatch): + db = web_server.get_database() + db.create_profile(name='NoPwUser') # no password set + _enable_login(monkeypatch) + assert client.post('/api/auth/login', json={'username': 'NoPwUser', 'password': 'x'}).status_code == 401 + + +def test_unknown_user_401(client, monkeypatch): + _enable_login(monkeypatch) + assert client.post('/api/auth/login', json={'username': 'ghost', 'password': 'x'}).status_code == 401 + + +def test_cannot_enable_login_without_admin_password(client): + # admin (1) has no password → enabling login mode is refused (anti-lockout) + web_server.get_database().set_profile_password(1, '') + r = client.post('/api/settings', json={'security': {'require_login': True}}) + assert r.status_code == 400 + assert 'password' in r.get_json().get('error', '').lower() diff --git a/tests/test_login_gate.py b/tests/test_login_gate.py new file mode 100644 index 00000000..51e9699e --- /dev/null +++ b/tests/test_login_gate.py @@ -0,0 +1,36 @@ +"""Pure login-gate decision (opt-in username/password mode).""" + +from __future__ import annotations + +from core.security.login_gate import login_request_is_blocked as blocked + + +def test_off_never_blocks(): + assert blocked('/api/anything', 'GET', require_login=False, authenticated=False) is False + + +def test_authenticated_never_blocked(): + assert blocked('/api/anything', 'GET', require_login=True, authenticated=True) is False + + +def test_unauthenticated_blocked_on_normal_api(): + assert blocked('/api/library', 'GET', require_login=True, authenticated=False) is True + + +def test_login_flow_and_shell_allowed_unauthenticated(): + for p in ('/', '/static/app.js', '/favicon.ico'): + assert blocked(p, 'GET', require_login=True, authenticated=False) is False + assert blocked('/api/auth/login', 'POST', require_login=True, authenticated=False) is False + assert blocked('/api/auth/logout', 'POST', require_login=True, authenticated=False) is False + assert blocked('/api/profiles/current', 'GET', require_login=True, authenticated=False) is False + assert blocked('/api/setup/status', 'GET', require_login=True, authenticated=False) is False + + +def test_profile_list_NOT_exposed_pre_auth(): + # login mode = type your name, don't pick from an exposed roster + assert blocked('/api/profiles', 'GET', require_login=True, authenticated=False) is True + + +def test_key_authed_public_api_allowed(): + assert blocked('/api/v1/search', 'GET', require_login=True, authenticated=False) is False + assert blocked('/api/v1/api-keys-internal', 'GET', require_login=True, authenticated=False) is True diff --git a/web_server.py b/web_server.py index c85c46f4..8345b3ee 100644 --- a/web_server.py +++ b/web_server.py @@ -402,6 +402,36 @@ def inject_webui_assets(): # PINs from one IP trips it — correct entry clears it instantly). from core.security.rate_limit import AttemptLimiter as _AttemptLimiter _launch_pin_limiter = _AttemptLimiter(max_attempts=10, window_seconds=300) +_login_limiter = _AttemptLimiter(max_attempts=10, window_seconds=300) + + +def _require_login_enabled(): + try: + return bool(config_manager.get('security.require_login', False)) if config_manager else False + except Exception: + return False + + +# --- Login gate (opt-in username/password mode; replaces the launch PIN) --- +@app.before_request +def _enforce_login(): + """Server-side enforcement of username/password login. No-op unless + security.require_login is on. When on, an unauthenticated session can only + reach the page shell + the login flow + the key-authed public API.""" + if not _require_login_enabled(): + return + from core.security.login_gate import login_request_is_blocked + from core.security.launch_lock import is_html_navigation + if login_request_is_blocked( + request.path, request.method, + require_login=True, + authenticated=bool(session.get('login_authenticated', False)), + ): + if is_html_navigation(request.method, request.headers.get('Accept', ''), + request.headers.get('Sec-Fetch-Mode', '')): + return redirect('/') + return jsonify({"error": "login_required", "login_required": True}), 401 + # --- Launch PIN gate (before_request hook) --- @app.before_request @@ -414,6 +444,10 @@ def _enforce_launch_pin(): on, except the page shell + the unlock flow + the key-authed public API. No-ops entirely when ``security.require_pin_on_launch`` is off (the default). """ + # Login mode replaces the launch PIN entirely — when it's on, _enforce_login + # owns the gate and this no-ops. + if _require_login_enabled(): + return try: require_pin = bool(config_manager.get('security.require_pin_on_launch', False)) if config_manager else False except Exception: @@ -3079,6 +3113,14 @@ def handle_settings(): if not new_settings: return jsonify({"success": False, "error": "No data received."}), 400 + # Anti-lockout: refuse to turn ON login mode until the admin account + # has a password — otherwise enabling it would lock everyone out. + _sec_in = new_settings.get('security') or {} + if _sec_in.get('require_login') and not config_manager.get('security.require_login', False): + if not get_database().profile_has_password(1): + return jsonify({"success": False, + "error": "Set an admin password before enabling login mode."}), 400 + if 'active_media_server' in new_settings: config_manager.set_active_media_server(new_settings['active_media_server']) @@ -25297,6 +25339,57 @@ def verify_launch_pin(): except Exception as e: return jsonify({'success': False, 'error': str(e)}), 500 + +@app.route('/api/auth/login', methods=['POST']) +def auth_login(): + """Username/password login (opt-in login mode). Username = profile name. + Brute-force limited per IP; a profile with no password set can't log in.""" + try: + _ip = request.remote_addr or 'unknown' + _now = time.time() + _locked, _retry_after = _login_limiter.is_locked(_ip, _now) + if _locked: + return (jsonify({'success': False, 'error': 'Too many attempts — please wait and try again'}), + 429, {'Retry-After': str(_retry_after)}) + + data = request.json or {} + username = (data.get('username') or '').strip() + password = data.get('password') or '' + if not username or not password: + return jsonify({'success': False, 'error': 'Username and password required'}), 400 + + database = get_database() + profile = database.get_profile_by_name(username) + # Same generic error + a recorded failure whether the name or password is + # wrong — don't leak which names exist. + if not profile or not database.verify_profile_password(profile['id'], password): + _login_limiter.record_failure(_ip, _now) + return jsonify({'success': False, 'error': 'Invalid username or password'}), 401 + + _login_limiter.record_success(_ip) + session['login_authenticated'] = True + session['profile_id'] = profile['id'] + # A fresh login also clears any stale launch-PIN flag. + session.pop('launch_pin_verified', None) + return jsonify({'success': True, 'profile': { + 'id': profile['id'], 'name': profile['name'], 'is_admin': profile['is_admin'], + }}) + except Exception as e: + return jsonify({'success': False, 'error': str(e)}), 500 + + +@app.route('/api/auth/logout', methods=['POST']) +def auth_logout(): + """Log out — clears the authenticated session.""" + try: + session.pop('login_authenticated', None) + session.pop('profile_id', None) + session.pop('launch_pin_verified', None) + return jsonify({'success': True}) + except Exception as e: + return jsonify({'success': False, 'error': str(e)}), 500 + + @app.route('/api/profiles/reset-pin-via-credential', methods=['POST']) def reset_pin_via_credential(): """Reset admin PIN by verifying a known API credential""" From 21dfbb39b0c9bd63f1228aee709d28fcdc4ec2eb Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 22:10:33 -0700 Subject: [PATCH 17/25] Native login (increment 3/3): login screen, set-password, Settings toggle, logout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The UI that makes opt-in login usable. Off by default → your LAN setup is unchanged (none of this appears unless security.require_login is on). - Login screen overlay (reuses the launch-PIN styling): username + password → /api/auth/login → reload into the app. Shown when /api/profiles/current reports login_required (checked before profile selection). - POST /api/profiles//set-password (admin, or self) to set/clear a login password, distinct from the PIN. - Settings → Security: "Login password (admin account)" field + a "Require login" toggle (with the anti-lockout note). Wired into the existing settings load/save. - Sign-out button in the profile bar, revealed only in login mode (login_mode flag on /api/profiles/current); soulsyncLogout() → /api/auth/logout → reload. Tests: set-password sets/clears + verifies; /api/profiles/current signals login_required. 20 login/password tests pass; 64 script-split integrity pass. Remaining (small follow-up): a password field in the Manage Profiles edit form so admins can set OTHER profiles' passwords from the UI (the endpoint already exists). --- tests/test_login_endpoints.py | 18 +++++++++ web_server.py | 25 ++++++++++++ webui/index.html | 38 ++++++++++++++++++ webui/static/init.js | 76 +++++++++++++++++++++++++++++++++++ webui/static/settings.js | 3 ++ 5 files changed, 160 insertions(+) diff --git a/tests/test_login_endpoints.py b/tests/test_login_endpoints.py index 4b9854b0..f943a9ca 100644 --- a/tests/test_login_endpoints.py +++ b/tests/test_login_endpoints.py @@ -80,3 +80,21 @@ def test_cannot_enable_login_without_admin_password(client): r = client.post('/api/settings', json={'security': {'require_login': True}}) assert r.status_code == 400 assert 'password' in r.get_json().get('error', '').lower() + + +def test_set_password_endpoint(client): + db = web_server.get_database() + pid = db.create_profile(name='SetPwTest') + # admin (default session) can set any profile's login password + r = client.post(f'/api/profiles/{pid}/set-password', json={'password': 'newpw123'}) + body = r.get_json() + assert body['success'] is True and body['has_password'] is True + assert db.verify_profile_password(pid, 'newpw123') is True + # clearing it + assert client.post(f'/api/profiles/{pid}/set-password', json={'password': ''}).get_json()['has_password'] is False + + +def test_profiles_current_signals_login_required(client, monkeypatch): + _enable_login(monkeypatch) + body = client.get('/api/profiles/current').get_json() + assert body.get('login_required') is True # frontend uses this to show the sign-in screen diff --git a/web_server.py b/web_server.py index 8345b3ee..3f2e3866 100644 --- a/web_server.py +++ b/web_server.py @@ -25282,6 +25282,12 @@ def select_profile(): def get_current_profile(): """Get the currently selected profile from session""" try: + # Login mode: when on and the session isn't authenticated, tell the + # frontend to show the sign-in screen (this is checked before profile + # selection, since there's no profile until you log in). + if _require_login_enabled() and not session.get('login_authenticated', False): + return jsonify({'success': False, 'login_required': True}), 200 + pid = session.get('profile_id') if not pid: return jsonify({'success': False, 'error': 'No profile selected'}), 200 @@ -25305,6 +25311,7 @@ def get_current_profile(): 'success': True, 'profile': profile, 'launch_pin_required': bool(require_pin) and not pin_verified, + 'login_mode': _require_login_enabled(), }) except Exception as e: return jsonify({'success': False, 'error': str(e)}), 500 @@ -25469,6 +25476,24 @@ def set_profile_pin(profile_id): except Exception as e: return jsonify({'success': False, 'error': str(e)}), 500 + +@app.route('/api/profiles//set-password', methods=['POST']) +def set_profile_password_endpoint(profile_id): + """Set or clear a profile's LOGIN password (admin, or the profile itself). + Distinct from the quick-switch PIN.""" + try: + database = get_database() + current_pid = get_current_profile_id() + current = database.get_profile(current_pid) + if not current or (not current['is_admin'] and current_pid != profile_id): + return jsonify({'success': False, 'error': 'Unauthorized'}), 403 + data = request.json or {} + password = data.get('password', '') + ok = database.set_profile_password(profile_id, password) + return jsonify({'success': bool(ok), 'has_password': database.profile_has_password(profile_id)}) + except Exception as e: + return jsonify({'success': False, 'error': str(e)}), 500 + # --- Per-Profile ListenBrainz Settings --- def _get_lb_credentials_for_profile(profile_id=None): diff --git a/webui/index.html b/webui/index.html index 3d90c7e6..4bbf1510 100644 --- a/webui/index.html +++ b/webui/index.html @@ -51,6 +51,19 @@ + + + @@ -6011,6 +6027,28 @@ If an auth proxy (Authelia / Authentik / oauth2-proxy) logs users in in front of SoulSync, enter the header it sets (e.g. Remote-User) and SoulSync will skip the launch PIN for already-authenticated requests. Only set this behind a proxy that strips any client-supplied copy of the header — otherwise it can be spoofed. Leave blank to disable (the default). + +
+ +
+ +
+ Set a password for the admin account, then turn on "Require login" below. Your username is your profile name. Set passwords for other profiles in Manage Profiles. +
+ + + +
+ +
+ +
+ When enabled, a sign-in screen replaces the profile picker + launch PIN — everyone signs in with their account name + password. Set the admin password above first (you can't enable this without one, to avoid locking yourself out). Best for instances exposed to the internet. +
+
diff --git a/webui/static/init.js b/webui/static/init.js index 71f94496..a032bc4b 100644 --- a/webui/static/init.js +++ b/webui/static/init.js @@ -340,9 +340,21 @@ async function initProfileSystem() { // Check if a session already has a profile selected const currentRes = await fetch('/api/profiles/current'); const currentData = await currentRes.json(); + // Login mode: show the sign-in screen and defer everything else until + // the user authenticates. + if (currentData.login_required) { + showLoginScreen(); + return false; + } if (currentData.success && currentData.profile) { setCurrentProfile(currentData.profile); + // Login mode → reveal the Sign out button in the profile bar. + if (currentData.login_mode) { + const lb = document.getElementById('logout-btn'); + if (lb) lb.style.display = ''; + } + // Check if launch PIN is required if (currentData.launch_pin_required) { showLaunchPinScreen(); @@ -387,6 +399,48 @@ async function initProfileSystem() { } } +// ── Login Screen (username/password mode) ────────────────────────────── + +function showLoginScreen() { + const overlay = document.getElementById('login-overlay'); + if (!overlay) return; + overlay.style.display = 'flex'; + const u = document.getElementById('login-username'); + if (u) setTimeout(() => u.focus(), 50); +} + +async function submitLogin() { + const username = (document.getElementById('login-username')?.value || '').trim(); + const password = document.getElementById('login-password')?.value || ''; + const errEl = document.getElementById('login-error'); + const btn = document.getElementById('login-submit'); + const showErr = (msg) => { if (errEl) { errEl.textContent = msg; errEl.style.display = 'block'; } }; + if (errEl) errEl.style.display = 'none'; + if (!username || !password) { showErr('Enter your username and password'); return; } + if (btn) { btn.disabled = true; btn.textContent = 'Signing in...'; } + try { + const res = await fetch('/api/auth/login', { + method: 'POST', headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ username, password }), + }); + const data = await res.json(); + if (data.success) { + window.location.reload(); // authenticated → reload into the app + } else { + showErr(res.status === 429 ? 'Too many attempts — wait a moment.' : (data.error || 'Sign in failed')); + if (btn) { btn.disabled = false; btn.textContent = 'Sign in'; } + } + } catch (e) { + showErr('Connection error'); + if (btn) { btn.disabled = false; btn.textContent = 'Sign in'; } + } +} + +async function soulsyncLogout() { + try { await fetch('/api/auth/logout', { method: 'POST' }); } catch (e) { /* reload anyway */ } + window.location.reload(); +} + // ── Launch PIN Lock Screen ───────────────────────────────────────────── function showLaunchPinScreen() { @@ -451,6 +505,28 @@ function showLaunchPinScreen() { // ── Security Settings Helpers ────────────────────────────────────────── +async function saveLoginPassword() { + const input = document.getElementById('security-login-password'); + const msg = document.getElementById('security-login-password-msg'); + const password = input?.value || ''; + const show = (text, ok) => { + if (!msg) return; + msg.textContent = text; + msg.style.color = ok ? '#4caf50' : '#ff5252'; + msg.style.display = 'block'; + }; + if (!password || password.length < 6) { show('Password must be at least 6 characters', false); return; } + try { + const res = await fetch('/api/profiles/1/set-password', { + method: 'POST', headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ password }), + }); + const data = await res.json(); + if (data.success) { show('Admin login password saved', true); if (input) input.value = ''; } + else show(data.error || 'Failed to save password', false); + } catch (e) { show('Connection error', false); } +} + async function saveSecurityPin() { const pin = document.getElementById('security-new-pin').value; const confirm = document.getElementById('security-confirm-pin').value; diff --git a/webui/static/settings.js b/webui/static/settings.js index 1a1a77ac..3a5967b6 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -1403,6 +1403,8 @@ async function loadSettingsData() { if (trustProxy) trustProxy.checked = settings.security?.trust_reverse_proxy || false; const authHeader = document.getElementById('security-auth-proxy-header'); if (authHeader) authHeader.value = settings.security?.auth_proxy_header || ''; + const reqLogin = document.getElementById('security-require-login'); + if (reqLogin) reqLogin.checked = settings.security?.require_login || false; // Check if admin has a PIN set const profilesRes = await fetch('/api/profiles'); @@ -3154,6 +3156,7 @@ async function saveSettings(quiet = false) { cors_origins: document.getElementById('security-cors-origins')?.value?.trim() || '', trust_reverse_proxy: document.getElementById('security-trust-proxy')?.checked || false, auth_proxy_header: document.getElementById('security-auth-proxy-header')?.value?.trim() || '', + require_login: document.getElementById('security-require-login')?.checked || false, } }; From 9e40f5c12d9b6b0b365c4f08b56aaa3731aae4c9 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 22:19:29 -0700 Subject: [PATCH 18/25] =?UTF-8?q?tests:=20pin/login=20mode=20isolation=20?= =?UTF-8?q?=E2=80=94=20PIN=20gate=20unaffected=20when=20login=20off;=20bot?= =?UTF-8?q?h-off=20=3D=20unguarded?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pins the zero-impact guarantee: with require_login off (default), the launch PIN still enforces exactly as before (the login deferral doesn't fire), and with both off there's no gate at all (today's behavior). --- tests/test_login_endpoints.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/test_login_endpoints.py b/tests/test_login_endpoints.py index f943a9ca..2441c5c5 100644 --- a/tests/test_login_endpoints.py +++ b/tests/test_login_endpoints.py @@ -98,3 +98,30 @@ def test_profiles_current_signals_login_required(client, monkeypatch): _enable_login(monkeypatch) body = client.get('/api/profiles/current').get_json() assert body.get('login_required') is True # frontend uses this to show the sign-in screen + + +def test_pin_gate_unaffected_when_login_off(client, monkeypatch): + # THE guarantee: with login mode OFF (default) and the launch PIN ON, the PIN + # gate must STILL enforce — the login feature must not weaken or bypass it. + real_get = web_server.config_manager.get + def fake_get(key, default=None): + if key == 'security.require_login': + return False # login OFF (default) + if key == 'security.require_pin_on_launch': + return True # PIN ON + return real_get(key, default) + monkeypatch.setattr(web_server.config_manager, 'get', fake_get) + + # Unverified session, PIN required → the launch-PIN gate still 401s. + assert client.get('/api/profiles/me/connections').status_code == 401 + # And /api/profiles/current reports the PIN screen, NOT login. + body = client.get('/api/profiles/current').get_json() + assert body.get('login_required') is not True + + +def test_everything_normal_when_both_off(client, monkeypatch): + # Default install: login OFF + PIN OFF → no gate at all (today's behavior). + real_get = web_server.config_manager.get + monkeypatch.setattr(web_server.config_manager, 'get', + lambda k, d=None: False if k in ('security.require_login', 'security.require_pin_on_launch') else real_get(k, d)) + assert client.get('/api/profiles/me/connections').status_code == 200 # reachable, unguarded From 613688a9ad6d815aea92e6ce7b810caf1a60a2c4 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 22:24:54 -0700 Subject: [PATCH 19/25] Login recovery (DB + backend): security question to reset a forgotten password MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the forgot-login-password gap. A per-profile recovery question + answer lets a locked-out user reset their own password. - DB: additive recovery_question + recovery_answer_hash columns (idempotent migration). set/get-question/verify/has methods; answer is hashed (pbkdf2) and matched forgivingly (trim + lowercase + collapse whitespace). No recovery set → never verifies. - Endpoints (allowlisted in the login gate so they work pre-auth): GET /api/auth/recovery-question?username= (generic 404 when absent), POST /api/auth/recovery-reset {username, answer, new_password} — brute-force limited; a correct answer sets the new password + authenticates the session. POST /api/profiles//set-recovery (admin or self) to configure it. Tests: set/get/verify, forgiving match, hashed-not-plaintext, no-recovery-never- verifies, full reset flow (wrong answer rejected + password intact; correct answer resets), unknown-user 404. 25 tests pass. Next: the Settings + login-screen UI. --- core/security/login_gate.py | 6 ++- database/music_database.py | 88 ++++++++++++++++++++++++++++++++++ tests/test_login_endpoints.py | 38 +++++++++++++++ tests/test_profile_recovery.py | 60 +++++++++++++++++++++++ web_server.py | 69 ++++++++++++++++++++++++++ 5 files changed, 259 insertions(+), 2 deletions(-) create mode 100644 tests/test_profile_recovery.py diff --git a/core/security/login_gate.py b/core/security/login_gate.py index bf521df4..87fb0375 100644 --- a/core/security/login_gate.py +++ b/core/security/login_gate.py @@ -13,14 +13,16 @@ from __future__ import annotations # GET endpoints the login screen itself needs before auth. _ALLOWED_GET = frozenset({ - '/api/profiles/current', # how the frontend detects login state - '/api/setup/status', # first-run check runs before the login screen + '/api/profiles/current', # how the frontend detects login state + '/api/setup/status', # first-run check runs before the login screen + '/api/auth/recovery-question', # forgot-password: fetch the security question }) # POST endpoints that drive the login flow. _ALLOWED_POST = frozenset({ '/api/auth/login', '/api/auth/logout', + '/api/auth/recovery-reset', # forgot-password: answer + set a new password }) diff --git a/database/music_database.py b/database/music_database.py index 88f23593..5816065c 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -461,6 +461,7 @@ class MusicDatabase: self._add_profile_settings(cursor) self._add_profile_listenbrainz_support(cursor) self._add_profile_password_support(cursor) + self._add_profile_recovery_support(cursor) self._add_profile_service_credentials(cursor) self._add_service_credential_sets(cursor) self._add_soul_id_columns(cursor) @@ -5455,6 +5456,93 @@ class MusicDatabase: except Exception as e: logger.error(f"Error in login-password migration: {e}") + # ── Login-password recovery (security question + answer) ────────────────── + + @staticmethod + def _normalize_recovery_answer(answer: str) -> str: + """Forgiving match: trim + lowercase + collapse internal whitespace.""" + return ' '.join((answer or '').strip().lower().split()) + + def set_profile_recovery(self, profile_id: int, question: str, answer: str) -> bool: + """Set (or clear, when either is empty) a profile's recovery Q + answer.""" + try: + from werkzeug.security import generate_password_hash + q = (question or '').strip() + norm = self._normalize_recovery_answer(answer) + if not q or not norm: + question_val, answer_hash = None, None # clear + else: + question_val = q + answer_hash = generate_password_hash(norm, method='pbkdf2:sha256') + with self._get_connection() as conn: + conn.execute( + "UPDATE profiles SET recovery_question = ?, recovery_answer_hash = ? WHERE id = ?", + (question_val, answer_hash, profile_id)) + conn.commit() + return True + except Exception as e: + logger.error(f"Error setting recovery for profile {profile_id}: {e}") + return False + + def get_profile_recovery_question(self, profile_id: int) -> Optional[str]: + """The recovery question text, or None if none set.""" + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute("SELECT recovery_question FROM profiles WHERE id = ?", (profile_id,)) + row = cursor.fetchone() + return row['recovery_question'] if row and row['recovery_question'] else None + except Exception as e: + logger.error(f"Error reading recovery question for profile {profile_id}: {e}") + return None + + def verify_profile_recovery_answer(self, profile_id: int, answer: str) -> bool: + """Verify the recovery answer. No recovery set → never verifies.""" + try: + from werkzeug.security import check_password_hash + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute("SELECT recovery_answer_hash FROM profiles WHERE id = ?", (profile_id,)) + row = cursor.fetchone() + if not row or not row['recovery_answer_hash']: + return False + return check_password_hash(row['recovery_answer_hash'], self._normalize_recovery_answer(answer)) + except Exception as e: + logger.error(f"Error verifying recovery answer for profile {profile_id}: {e}") + return False + + def profile_has_recovery(self, profile_id: int) -> bool: + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute("SELECT recovery_answer_hash FROM profiles WHERE id = ?", (profile_id,)) + row = cursor.fetchone() + return bool(row and row['recovery_answer_hash']) + except Exception as e: + logger.error(f"Error checking recovery for profile {profile_id}: {e}") + return False + + def _add_profile_recovery_support(self, cursor): + """Add recovery question + answer-hash columns (login-password recovery).""" + try: + cursor.execute("SELECT value FROM metadata WHERE key = 'profiles_recovery_v1' LIMIT 1") + if cursor.fetchone(): + return # Already migrated + logger.info("Applying per-profile recovery-question migration...") + for col_sql in ( + "ALTER TABLE profiles ADD COLUMN recovery_question TEXT DEFAULT NULL", + "ALTER TABLE profiles ADD COLUMN recovery_answer_hash TEXT DEFAULT NULL", + ): + try: + cursor.execute(col_sql) + except sqlite3.OperationalError: + pass # Column already exists + cursor.execute( + "INSERT OR REPLACE INTO metadata (key, value) VALUES ('profiles_recovery_v1', '1')") + logger.info("Per-profile recovery-question migration completed") + except Exception as e: + logger.error(f"Error in recovery-question migration: {e}") + def close(self): """Close database connection (no-op since we create connections per operation)""" # Each operation creates and closes its own connection, so nothing to do here diff --git a/tests/test_login_endpoints.py b/tests/test_login_endpoints.py index 2441c5c5..390b8d3b 100644 --- a/tests/test_login_endpoints.py +++ b/tests/test_login_endpoints.py @@ -125,3 +125,41 @@ def test_everything_normal_when_both_off(client, monkeypatch): monkeypatch.setattr(web_server.config_manager, 'get', lambda k, d=None: False if k in ('security.require_login', 'security.require_pin_on_launch') else real_get(k, d)) assert client.get('/api/profiles/me/connections').status_code == 200 # reachable, unguarded + + +def test_recovery_flow_resets_password(client, monkeypatch): + db = web_server.get_database() + pid = db.create_profile(name='RecoverMe') + db.set_profile_password(pid, 'oldpassword') + db.set_profile_recovery(pid, 'First pet?', 'Rex') + _enable_login(monkeypatch) + + # forgot-password flow is reachable pre-auth + q = client.get('/api/auth/recovery-question?username=RecoverMe').get_json() + assert q['success'] and q['question'] == 'First pet?' + + # wrong answer → 401, password unchanged + bad = client.post('/api/auth/recovery-reset', + json={'username': 'RecoverMe', 'answer': 'Fido', 'new_password': 'newpass1'}) + assert bad.status_code == 401 + assert db.verify_profile_password(pid, 'oldpassword') is True + + # correct answer → password reset + authenticated + ok = client.post('/api/auth/recovery-reset', + json={'username': 'RecoverMe', 'answer': 'rex', 'new_password': 'brandnew1'}) + assert ok.status_code == 200 and ok.get_json()['success'] is True + assert db.verify_profile_password(pid, 'brandnew1') is True + assert db.verify_profile_password(pid, 'oldpassword') is False + + +def test_recovery_question_404_for_unknown(client, monkeypatch): + _enable_login(monkeypatch) + assert client.get('/api/auth/recovery-question?username=ghost').status_code == 404 + + +def test_set_recovery_endpoint(client): + db = web_server.get_database() + pid = db.create_profile(name='SetRec') + r = client.post(f'/api/profiles/{pid}/set-recovery', json={'question': 'Q?', 'answer': 'A'}) + assert r.get_json()['has_recovery'] is True + assert db.verify_profile_recovery_answer(pid, 'a') is True diff --git a/tests/test_profile_recovery.py b/tests/test_profile_recovery.py new file mode 100644 index 00000000..f730b74f --- /dev/null +++ b/tests/test_profile_recovery.py @@ -0,0 +1,60 @@ +"""Login-password recovery via security question + answer — DB layer.""" + +from __future__ import annotations + +import pytest + +from database.music_database import MusicDatabase + + +@pytest.fixture +def db(tmp_path): + return MusicDatabase(str(tmp_path / "music.db")) + + +def test_migration_adds_recovery_columns(db): + with db._get_connection() as conn: + cols = [r[1] for r in conn.execute("PRAGMA table_info(profiles)").fetchall()] + assert 'recovery_question' in cols and 'recovery_answer_hash' in cols + + +def test_set_get_verify(db): + pid = db.create_profile(name='RecUser') + assert db.profile_has_recovery(pid) is False + assert db.get_profile_recovery_question(pid) is None + + db.set_profile_recovery(pid, 'First pet?', 'Rex') + assert db.profile_has_recovery(pid) is True + assert db.get_profile_recovery_question(pid) == 'First pet?' + assert db.verify_profile_recovery_answer(pid, 'Rex') is True + assert db.verify_profile_recovery_answer(pid, 'Fido') is False + + +def test_answer_match_is_forgiving(db): + pid = db.create_profile(name='Forgiving') + db.set_profile_recovery(pid, 'City?', ' New York ') + assert db.verify_profile_recovery_answer(pid, 'new york') is True # case + spacing + assert db.verify_profile_recovery_answer(pid, 'NEW YORK') is True + + +def test_no_recovery_never_verifies(db): + pid = db.create_profile(name='NoRec') + assert db.verify_profile_recovery_answer(pid, '') is False + assert db.verify_profile_recovery_answer(pid, 'anything') is False + + +def test_clearing_recovery(db): + pid = db.create_profile(name='ClearRec') + db.set_profile_recovery(pid, 'Q?', 'A') + assert db.profile_has_recovery(pid) is True + db.set_profile_recovery(pid, '', '') + assert db.profile_has_recovery(pid) is False + assert db.get_profile_recovery_question(pid) is None + + +def test_answer_is_hashed_not_plaintext(db): + pid = db.create_profile(name='Hashed') + db.set_profile_recovery(pid, 'Q?', 'secretanswer') + with db._get_connection() as conn: + stored = conn.execute("SELECT recovery_answer_hash FROM profiles WHERE id = ?", (pid,)).fetchone()[0] + assert 'secretanswer' not in stored and stored.startswith('pbkdf2:') diff --git a/web_server.py b/web_server.py index 3f2e3866..78557973 100644 --- a/web_server.py +++ b/web_server.py @@ -25397,6 +25397,75 @@ def auth_logout(): return jsonify({'success': False, 'error': str(e)}), 500 +@app.route('/api/auth/recovery-question', methods=['GET']) +def auth_recovery_question(): + """Return the recovery security-question for a username (forgot-password flow). + Generic when the user/question is absent — don't confirm which names exist.""" + try: + username = (request.args.get('username') or '').strip() + database = get_database() + profile = database.get_profile_by_name(username) if username else None + question = database.get_profile_recovery_question(profile['id']) if profile else None + if not question: + return jsonify({'success': False, 'error': 'No recovery question available'}), 404 + return jsonify({'success': True, 'question': question}) + except Exception as e: + return jsonify({'success': False, 'error': str(e)}), 500 + + +@app.route('/api/auth/recovery-reset', methods=['POST']) +def auth_recovery_reset(): + """Reset a login password by answering the recovery question. Brute-force + limited; a correct answer sets the new password and authenticates the session.""" + try: + _ip = request.remote_addr or 'unknown' + _now = time.time() + _locked, _retry_after = _login_limiter.is_locked(_ip, _now) + if _locked: + return (jsonify({'success': False, 'error': 'Too many attempts — please wait and try again'}), + 429, {'Retry-After': str(_retry_after)}) + + data = request.json or {} + username = (data.get('username') or '').strip() + answer = data.get('answer') or '' + new_password = data.get('new_password') or '' + if not username or not answer or not new_password: + return jsonify({'success': False, 'error': 'Username, answer and new password are required'}), 400 + if len(new_password) < 6: + return jsonify({'success': False, 'error': 'New password must be at least 6 characters'}), 400 + + database = get_database() + profile = database.get_profile_by_name(username) + if not profile or not database.verify_profile_recovery_answer(profile['id'], answer): + _login_limiter.record_failure(_ip, _now) + return jsonify({'success': False, 'error': 'Incorrect answer'}), 401 + + _login_limiter.record_success(_ip) + database.set_profile_password(profile['id'], new_password) + session['login_authenticated'] = True + session['profile_id'] = profile['id'] + session.pop('launch_pin_verified', None) + return jsonify({'success': True}) + except Exception as e: + return jsonify({'success': False, 'error': str(e)}), 500 + + +@app.route('/api/profiles//set-recovery', methods=['POST']) +def set_profile_recovery_endpoint(profile_id): + """Set or clear a profile's recovery question + answer (admin, or self).""" + try: + database = get_database() + current_pid = get_current_profile_id() + current = database.get_profile(current_pid) + if not current or (not current['is_admin'] and current_pid != profile_id): + return jsonify({'success': False, 'error': 'Unauthorized'}), 403 + data = request.json or {} + ok = database.set_profile_recovery(profile_id, data.get('question', ''), data.get('answer', '')) + return jsonify({'success': bool(ok), 'has_recovery': database.profile_has_recovery(profile_id)}) + except Exception as e: + return jsonify({'success': False, 'error': str(e)}), 500 + + @app.route('/api/profiles/reset-pin-via-credential', methods=['POST']) def reset_pin_via_credential(): """Reset admin PIN by verifying a known API credential""" From 5c80ee1010d87e3efbe06c75039486043e8a1862 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Wed, 10 Jun 2026 22:28:07 -0700 Subject: [PATCH 20/25] Login recovery (UI): Settings setup + "Forgot password?" on the login screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Settings → Security: a recovery-question picker (5 presets + Custom) + answer field + Save, posting to /api/profiles/1/set-recovery. handleRecoveryQuestionChange reveals the custom box. - Login screen: a "Forgot password?" link opens a recovery view — enter username → fetch your question → answer + new password → reset → reload signed in. Reuses the launch-PIN overlay styling/structure (entry + recovery views). All inert unless login mode is on, so a default/LAN install never sees any of it. 64 script-split integrity tests pass (every new handler resolves). --- webui/index.html | 54 ++++++++++++++++++++++---- webui/static/init.js | 91 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 7 deletions(-) diff --git a/webui/index.html b/webui/index.html index 4bbf1510..8da0f94a 100644 --- a/webui/index.html +++ b/webui/index.html @@ -54,13 +54,33 @@ @@ -6040,6 +6060,26 @@ +
+ +
+ Lets you reset a forgotten login password by answering this. Recommended if you enable login. (You can set one per profile in Manage Profiles too.) +
+ + + + + +
+
- +
-

🔒 Security

- -