diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index 34e38beb..a2546ecc 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -9,9 +9,9 @@ on: workflow_dispatch: inputs: version_tag: - description: 'Version tag (e.g. 2.6.9)' + description: 'Version tag (e.g. 2.7.0)' required: true - default: '2.6.9' + default: '2.7.0' jobs: build-and-push: diff --git a/Support/REVERSE-PROXY.md b/Support/REVERSE-PROXY.md new file mode 100644 index 00000000..1e6152ff --- /dev/null +++ b/Support/REVERSE-PROXY.md @@ -0,0 +1,134 @@ +# 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, 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 + 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. + +--- + +## 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. + + 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 **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` + > 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 + 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/discovery/endpoints.py b/core/discovery/endpoints.py index 69dbb0d8..2d0d2042 100644 --- a/core/discovery/endpoints.py +++ b/core/discovery/endpoints.py @@ -579,45 +579,68 @@ 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'] + # 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}'" + ) + try: cache_key = get_discovery_cache_key(original_name, original_artist) artists_list = spotify_track['artists'] if isinstance(artists_list, list): 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/core/downloads/status.py b/core/downloads/status.py index 4acd8248..f425cceb 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 @@ -400,17 +409,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/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/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/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/core/security/login_gate.py b/core/security/login_gate.py new file mode 100644 index 00000000..87fb0375 --- /dev/null +++ b/core/security/login_gate.py @@ -0,0 +1,55 @@ +"""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 + '/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 +}) + + +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/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/core/security/reverse_proxy.py b/core/security/reverse_proxy.py new file mode 100644 index 00000000..ab1a1689 --- /dev/null +++ b/core/security/reverse_proxy.py @@ -0,0 +1,60 @@ +"""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" + + # 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. + return False + + +__all__ = ["apply_reverse_proxy_mode", "CONFIG_KEY"] diff --git a/database/music_database.py b/database/music_database.py index f131a03b..a3b2fb03 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -460,6 +460,8 @@ 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_recovery_support(cursor) self._add_profile_service_credentials(cursor) self._add_service_credential_sets(cursor) self._add_soul_id_columns(cursor) @@ -5267,6 +5269,9 @@ class MusicDatabase: 'avatar_url': row['avatar_url'] if 'avatar_url' in columns else None, 'is_admin': bool(row['is_admin']), 'has_pin': row['pin_hash'] is not None, + 'has_password': row['password_hash'] is not None if 'password_hash' in columns else False, + 'has_recovery': row['recovery_answer_hash'] is not None if 'recovery_answer_hash' in columns else False, + 'recovery_question': row['recovery_question'] if 'recovery_question' in columns else None, 'home_page': row['home_page'] if 'home_page' in columns else None, 'allowed_pages': json.loads(ap_raw) if ap_raw else None, 'can_download': bool(row['can_download']) if 'can_download' in columns else True, @@ -5297,6 +5302,9 @@ class MusicDatabase: 'avatar_url': row['avatar_url'] if 'avatar_url' in columns else None, 'is_admin': bool(row['is_admin']), 'has_pin': row['pin_hash'] is not None, + 'has_password': row['password_hash'] is not None if 'password_hash' in columns else False, + 'has_recovery': row['recovery_answer_hash'] is not None if 'recovery_answer_hash' in columns else False, + 'recovery_question': row['recovery_question'] if 'recovery_question' in columns else None, 'home_page': row['home_page'] if 'home_page' in columns else None, 'allowed_pages': json.loads(ap_raw) if ap_raw else None, 'can_download': bool(row['can_download']) if 'can_download' in columns else True, @@ -5398,6 +5406,172 @@ 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}") + + # ── 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 @@ -6654,17 +6828,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") @@ -6704,14 +6883,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 = [] @@ -6734,6 +6917,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""" @@ -6742,7 +6952,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/discovery/test_discovery_endpoints.py b/tests/discovery/test_discovery_endpoints.py index 2f1b0ace..5aa80d27 100644 --- a/tests/discovery/test_discovery_endpoints.py +++ b/tests/discovery/test_discovery_endpoints.py @@ -955,3 +955,71 @@ 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', + # 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, + }, + }) + 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 — 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[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(): + """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/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' 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/tests/test_artist_sync_stale_guard.py b/tests/test_artist_sync_stale_guard.py new file mode 100644 index 00000000..0da6a1ac --- /dev/null +++ b/tests/test_artist_sync_stale_guard.py @@ -0,0 +1,107 @@ +"""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-sync2-') +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, track_ids): + """Plex artist + album + tracks (server_source='plex').""" + db = web_server.get_database() + 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 (?, ?, 'plex')", + (aid, f'Artist {aid}')) + conn.execute("INSERT OR REPLACE INTO albums (id, title, artist_id) VALUES (?, 'Alb', ?)", + (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_ids(artist_id): + db = web_server.get_database() + with db._get_connection() as conn: + return {r['id'] for r in conn.execute("SELECT id FROM tracks WHERE artist_id = ?", (str(artist_id),))} + + +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 len(_track_ids(7002)) == 10 # nothing deleted + + +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, 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/7004/sync').status_code == 403 + assert len(_track_ids(7004)) == 2 # untouched 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 a4dba6b9..7cfebc6f 100644 --- a/tests/test_credentials_endpoints.py +++ b/tests/test_credentials_endpoints.py @@ -346,3 +346,68 @@ 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 + + +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() + + +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/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/tests/test_login_endpoints.py b/tests/test_login_endpoints.py new file mode 100644 index 00000000..390b8d3b --- /dev/null +++ b/tests/test_login_endpoints.py @@ -0,0 +1,165 @@ +"""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() + + +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 + + +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 + + +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_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/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/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 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 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/tests/test_reverse_proxy_mode.py b/tests/test_reverse_proxy_mode.py new file mode 100644 index 00000000..c69abe24 --- /dev/null +++ b/tests/test_reverse_proxy_mode.py @@ -0,0 +1,81 @@ +"""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) + + +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', '') 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/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 121455d6..e9817578 100644 --- a/web_server.py +++ b/web_server.py @@ -40,7 +40,7 @@ logger = setup_logging(_log_level, _log_path) # App version — single source of truth for backup metadata, system-info, update check, etc. # Semver: MAJOR.MINOR.PATCH. Bump at each dev→main release. -_SOULSYNC_BASE_VERSION = "2.6.9" +_SOULSYNC_BASE_VERSION = "2.7.0" def _build_version_string(): """Append short commit hash to version when available (e.g. 2.35+abc1234).""" @@ -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, @@ -390,6 +398,41 @@ 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) +_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 def _enforce_launch_pin(): @@ -401,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: @@ -408,10 +455,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 @@ -3055,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']) @@ -12613,6 +12679,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: @@ -12658,6 +12725,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 @@ -12712,55 +12784,77 @@ 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() + 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), + ) + artist_track_ids = {row['id'] for row in cursor.fetchall()} + stale = artist_track_ids - seen_track_ids - 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']) + # 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) - if 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 - - 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() + if not removal_skipped: + 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") + f"-{stale_removed} stale, -{empty_albums_removed} empty albums" + f"{' (removal skipped — storage unreachable)' if removal_skipped else ''}") return jsonify({ "success": True, @@ -12770,6 +12864,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: @@ -19449,14 +19544,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"}) @@ -19567,6 +19672,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": []}) @@ -19596,7 +19705,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: @@ -25565,6 +25675,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 @@ -25588,6 +25704,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 @@ -25596,6 +25713,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: @@ -25604,13 +25730,135 @@ 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: 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/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""" @@ -25690,6 +25938,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 e9c836ce..5f30b75f 100644 --- a/webui/index.html +++ b/webui/index.html @@ -51,6 +51,40 @@ + + + @@ -5958,40 +5995,122 @@ - +
-

🔒 Security

- -