diff --git a/tests/test_tidal_auth_redirect_uri.py b/tests/test_tidal_auth_redirect_uri.py new file mode 100644 index 00000000..caf3a7b9 --- /dev/null +++ b/tests/test_tidal_auth_redirect_uri.py @@ -0,0 +1,203 @@ +"""Regression tests for Tidal /auth/tidal redirect_uri selection. + +Discord-reported (Foxxify): Tidal returned error 1002 ("Invalid +redirect URI") on every authentication attempt. The user had +``http://127.0.0.1:8889/tidal/callback`` registered in his Tidal +Developer Portal (matching the SoulSync UI default + docs), but +SoulSync was sending a network-IP-derived URI like +``http://192.168.x.x:8889/tidal/callback`` because the empty-config +fallback in /auth/tidal built the URI from ``request.host``. Tidal +compares strings exactly, so the URIs didn't match and authentication +failed before the user could even see Tidal's consent screen. + +These tests pin: +1. When ``tidal.redirect_uri`` is configured, that value is sent to + Tidal verbatim. +2. When the config is empty, SoulSync uses the constructor default + (``http://127.0.0.1:/tidal/callback``) — NOT a value built + from request.host. +3. Both cases work whether the user is accessing SoulSync via + localhost or a network IP (the access path is independent from the + authorize redirect_uri). +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch +from urllib.parse import parse_qs, urlparse + +import pytest + + +@pytest.fixture +def auth_route_client(monkeypatch: pytest.MonkeyPatch): + """Flask test client wired to render the Tidal auth flow without + spawning a real TidalClient or hitting external services.""" + # Force the "remote/docker" branch (route only renders the + # instructions page when one of those flags is true). + monkeypatch.setattr( + "os.path.exists", + lambda p: p == "/.dockerenv" or False, + ) + + fake_client = MagicMock() + fake_client.client_id = "fake-id" + fake_client.code_verifier = "v" * 40 + fake_client.code_challenge = "c" * 40 + fake_client.auth_url = "https://login.tidal.com/authorize" + # Constructor default that mirrors core/tidal_client.py:124. + fake_client.redirect_uri = "http://127.0.0.1:8889/tidal/callback" + fake_client._generate_pkce_challenge = MagicMock() + + with patch("core.tidal_client.TidalClient", return_value=fake_client): + with patch("web_server.add_activity_item"): + from web_server import app as flask_app + flask_app.config['TESTING'] = True + yield flask_app.test_client(), fake_client + + +def _extract_authorize_url(html: str) -> str | None: + """Pull the Tidal authorize URL out of the rendered instructions page.""" + import re + m = re.search(r'href="(https://login\.tidal\.com/authorize\?[^"]+)"', html) + return m.group(1) if m else None + + +def _extract_redirect_uri(html: str) -> str | None: + auth_url = _extract_authorize_url(html) + if not auth_url: + return None + parsed = urlparse(auth_url) + qs = parse_qs(parsed.query) + val = qs.get('redirect_uri', [None])[0] + return val + + +# --------------------------------------------------------------------------- +# Configured redirect_uri honored verbatim +# --------------------------------------------------------------------------- + + +class TestConfiguredRedirectUriIsHonored: + def test_localhost_config_sent_when_user_accesses_via_network_ip( + self, auth_route_client, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """The reported Foxxify scenario: user has 127.0.0.1:8889 + registered in Tidal portal AND set in SoulSync config, accesses + the Web UI from his network IP. The authorize URL must contain + the configured 127.0.0.1 URI, NOT a value built from + request.host (which would mismatch the portal and yield + Tidal error 1002).""" + client, _fake = auth_route_client + + from config.settings import config_manager + monkeypatch.setattr( + config_manager, "get", + lambda key, default=None: ( + "http://127.0.0.1:8889/tidal/callback" + if key == "tidal.redirect_uri" else default + ), + ) + + response = client.get("/auth/tidal", base_url="http://192.168.86.50:8008") + html = response.get_data(as_text=True) + + sent = _extract_redirect_uri(html) + assert sent == "http://127.0.0.1:8889/tidal/callback", ( + f"Configured redirect_uri must be sent verbatim — got {sent!r}" + ) + + def test_custom_port_config_sent_verbatim( + self, auth_route_client, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """User with non-default port (e.g. SOULSYNC_TIDAL_CALLBACK_PORT=9999) + and matching portal registration.""" + client, _fake = auth_route_client + + from config.settings import config_manager + monkeypatch.setattr( + config_manager, "get", + lambda key, default=None: ( + "http://127.0.0.1:9999/tidal/callback" + if key == "tidal.redirect_uri" else default + ), + ) + + response = client.get("/auth/tidal", base_url="http://192.168.86.50:8008") + html = response.get_data(as_text=True) + + sent = _extract_redirect_uri(html) + assert sent == "http://127.0.0.1:9999/tidal/callback" + + def test_explicit_network_ip_config_also_honored( + self, auth_route_client, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """User who deliberately registered their network IP with Tidal + and configured SoulSync to match — that registration must also + be honored, not overridden.""" + client, _fake = auth_route_client + + from config.settings import config_manager + monkeypatch.setattr( + config_manager, "get", + lambda key, default=None: ( + "http://192.168.86.50:8889/tidal/callback" + if key == "tidal.redirect_uri" else default + ), + ) + + response = client.get("/auth/tidal", base_url="http://192.168.86.50:8008") + html = response.get_data(as_text=True) + + sent = _extract_redirect_uri(html) + assert sent == "http://192.168.86.50:8889/tidal/callback" + + +# --------------------------------------------------------------------------- +# Empty config falls back to constructor default — NOT request.host +# --------------------------------------------------------------------------- + + +class TestEmptyConfigFallsBackToDefault: + def test_empty_config_uses_constructor_default_not_request_host( + self, auth_route_client, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """The actual Foxxify case (his SoulSync UI display showed the + default but config was empty because the placeholder never got + saved): empty config from a non-localhost request must NOT build + a network-IP redirect URI. The constructor default (matching + the documented portal registration) wins instead.""" + client, _fake = auth_route_client + + from config.settings import config_manager + monkeypatch.setattr( + config_manager, "get", + lambda key, default=None: ("" if key == "tidal.redirect_uri" else default), + ) + + response = client.get("/auth/tidal", base_url="http://192.168.86.50:8008") + html = response.get_data(as_text=True) + + sent = _extract_redirect_uri(html) + assert sent == "http://127.0.0.1:8889/tidal/callback", ( + f"Empty config must fall back to constructor default — got {sent!r}. " + "If this looks like 'http://192.168.x.x:8889/tidal/callback' the " + "request-host fallback got reintroduced and Foxxify's bug is back." + ) + + def test_empty_config_localhost_access_uses_default( + self, auth_route_client, monkeypatch: pytest.MonkeyPatch, + ) -> None: + client, _fake = auth_route_client + + from config.settings import config_manager + monkeypatch.setattr( + config_manager, "get", + lambda key, default=None: ("" if key == "tidal.redirect_uri" else default), + ) + + response = client.get("/auth/tidal", base_url="http://127.0.0.1:8008") + html = response.get_data(as_text=True) + + sent = _extract_redirect_uri(html) + assert sent == "http://127.0.0.1:8889/tidal/callback" diff --git a/web_server.py b/web_server.py index 99dd5d56..eb72aac6 100644 --- a/web_server.py +++ b/web_server.py @@ -5583,19 +5583,32 @@ def auth_tidal(): tidal_oauth_state["code_verifier"] = temp_tidal_client.code_verifier tidal_oauth_state["code_challenge"] = temp_tidal_client.code_challenge - # Use the user's configured redirect_uri from settings — don't override - # with request.host, which in Docker returns the container hostname + # Use the user's configured redirect_uri from settings, falling back + # to the constructor default (``http://127.0.0.1:/tidal/callback``). + # The settings UI displays the default as the placeholder, and SoulSync's + # docs tell users to register THAT URI with their Tidal Developer App + # — Tidal validates the redirect_uri sent in the authorize request + # against the one in the portal, so sending anything else (e.g. a + # network-IP variant built from request.host) returns Tidal error 1002 + # "Invalid redirect URI" and the user can't authenticate. + # + # Docker/remote-access workflow is preserved by the post-auth swap step + # in the instructions page below: SoulSync sends ``127.0.0.1:``, + # Tidal redirects the user's browser to that URI (which fails locally), + # the instructions tell the user to swap ``127.0.0.1`` for the host + # they're accessing SoulSync from, and the swapped URL hits the + # container's exposed callback port. Building the URI from request.host + # at authorize time used to skip the swap entirely but broke users + # who registered the documented default. configured_redirect = config_manager.get('tidal.redirect_uri', '') if configured_redirect: temp_tidal_client.redirect_uri = configured_redirect logger.info(f"Using configured Tidal redirect_uri: {configured_redirect}") else: - # Fallback: dynamically set based on request host (non-Docker local access) - request_host = request.host.split(':')[0] - if request_host not in ('127.0.0.1', 'localhost'): - dynamic_redirect = f"http://{request_host}:8889/tidal/callback" - temp_tidal_client.redirect_uri = dynamic_redirect - logger.info(f"Tidal redirect_uri set from request host: {dynamic_redirect}") + logger.info( + f"Using default Tidal redirect_uri (no config override): " + f"{temp_tidal_client.redirect_uri}" + ) # Store PKCE + redirect_uri for callback to use the same values with tidal_oauth_lock: diff --git a/webui/static/helper.js b/webui/static/helper.js index 57cbebae..7df7aa29 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3448,6 +3448,7 @@ const WHATS_NEW = { { title: 'Fix Lossy Copy Not Deleting Original FLAC', desc: 'with lossy copy enabled and "delete original" turned on (you wanted an mp3-only library), every download still left both the flac and the converted mp3 sitting in the same folder. the setting was being read but never acted on during the conversion step. now the original gets removed right after a successful conversion, with a same-path safety check + graceful handling if the original is already gone or locked.', page: 'settings' }, { title: 'Watchlist Stops Re-Downloading Tracks That Already Exist', desc: 'a track that was already on disk got re-downloaded by the watchlist on every scan because the library had stale album metadata for it (file tagged on the wrong album by an old import) and the album fuzzy comparison declared the track missing. now the watchlist also matches by stable external IDs (spotify / itunes / deezer / tidal / qobuz / musicbrainz / audiodb / hydrabase / isrc) before falling through to the fuzzy block — so any track whose tags or DB row carry a matching ID is recognized as already present regardless of album drift. provider-neutral, falls through to existing fuzzy logic for older imports without IDs.', page: 'watchlist' }, { title: 'Persist Source IDs at Download Time + Backfill on Sync', desc: 'every download already collects spotify/itunes/deezer/tidal/qobuz/musicbrainz/audiodb/hydrabase/isrc IDs during post-processing, but for plex/jellyfin/navidrome users they got dropped on the floor — only enrichment workers eventually wrote them onto the tracks row, hours later. now those IDs persist to the track_downloads table immediately, the media-server sync code copies them onto the new tracks row the moment it gets created, and the watchlist scanner has a second-tier fallback to query provenance directly when the tracks row hasn\'t been synced yet. closes the enrichment-wait window — freshly downloaded files are recognizable on the very next watchlist scan instead of after enrichment catches up.', page: 'library' }, + { title: 'Fix Tidal Auth Error 1002 for Docker / Remote Access', desc: 'tidal returned error 1002 ("invalid redirect URI") on every authentication attempt for users accessing soulsync from a network IP. cause: when the redirect_uri config field was empty (which it usually was, because the UI just shows the default as a placeholder without saving it), the /auth/tidal route silently overrode the constructor default with a uri built from request.host — http://192.168.x.x:8889/tidal/callback. that didn\'t match what users had registered in their tidal developer portal (http://127.0.0.1:8889/tidal/callback per the docs and UI default), so tidal rejected the authorize request before users ever saw the consent screen. fix: drop the request-host fallback entirely. empty config now falls back to the constructor default that matches the documented portal registration. the existing post-auth swap-step instructions handle the docker/remote-access case as designed.', page: 'settings' }, { title: 'Sidebar Library Button Shows Artist Breadcrumb', desc: 'when you open an artist detail page (from library, search, or the global search popover), the sidebar Library button now lights up and rewrites its label to "Library / Artist Name" — long names truncate with an ellipsis and the full name shows on hover. revertes to plain "Library" when you leave. purely visual, no functionality change.', page: 'library' }, { title: 'Enrichment Bubble Routes Consolidated', desc: 'internal — every dashboard enrichment bubble (musicbrainz, spotify, itunes, deezer, discogs, audiodb, lastfm, genius, tidal, qobuz) used to hit its own per-service status / pause / resume route in web_server.py. unified them under a single registry-driven endpoint set: /api/enrichment//. spotify\'s rate-limit guard, lastfm/genius yield-override behavior, and tidal/qobuz extra status fields are encoded as data on the registry. 27 new tests cover the registry behavior.' }, { title: 'Drop Old Per-Service Enrichment Routes', desc: 'internal — followup to the registry consolidation. now that the dashboard has cut over to /api/enrichment//, deleted the 30 hand-rolled per-service routes from web_server.py (musicbrainz/audiodb/discogs/deezer/spotify/itunes/lastfm/genius/tidal/qobuz status+pause+resume). ~510 lines gone from the monolith, no behavior change.' },