From 70dba77711411a15205efd870553b99254aba149 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sun, 28 Jun 2026 13:07:29 -0700 Subject: [PATCH] spotify oauth: keep the redirect_uri trailing slash (follow-up to #942) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #942's normalize_spotify_oauth_config trimmed whitespace/quotes (good — those can't be part of a real credential) but ALSO rstrip("/")'d the redirect_uri. that's unsafe: Spotify matches the redirect URI EXACTLY against the app's dashboard registration, and a trailing slash is a legitimate part of a URI. stripping it would silently break anyone who registered '…/callback/' (we'd send '…/callback' → INVALID_CLIENT: Invalid redirect URI) — trading one failure mode for a sneakier one the user can't diagnose (SoulSync no longer sends what they typed). drop the rstrip; keep the whitespace/quote trim. the value is now preserved verbatim apart from unambiguous paste garbage. flipped the test that asserted the strip to assert the slash is kept (and that whitespace/quotes around it are still trimmed), + a dedicated regression guard. the #942 integration test mocks normalize, so it's unaffected. 262 spotify/oauth tests green. credit: builds on HellRa1SeR's #942. --- core/spotify_client.py | 18 +++++++++++------- tests/test_spotify_client.py | 22 ++++++++++++++++++---- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/core/spotify_client.py b/core/spotify_client.py index cf3859ca..d8e5df8e 100644 --- a/core/spotify_client.py +++ b/core/spotify_client.py @@ -17,9 +17,16 @@ logger = get_logger("spotify_client") def normalize_spotify_oauth_config(config: Optional[Dict[str, Any]]) -> Dict[str, Any]: """Normalize Spotify OAuth config before building an auth manager. - Spotify rejects values that include surrounding whitespace, quotes, or - newline characters. The settings UI can paste values that carry such - formatting, so we trim them before sending them to Spotify. + Spotify rejects values that include surrounding whitespace or quotes, and the + settings UI can paste values carrying such formatting, so we trim those — they + can never be part of a real credential. + + We deliberately do NOT strip a trailing slash from ``redirect_uri``: Spotify + matches the redirect URI EXACTLY against the app's dashboard registration, and + a trailing slash is a legitimate part of a URI. Stripping it would silently + break anyone who registered ``…/callback/`` (we'd send ``…/callback`` → + "INVALID_CLIENT: Invalid redirect URI"). So the value is preserved verbatim + apart from the unambiguous whitespace/quote garbage (#942 follow-up). """ if not isinstance(config, dict): return {} @@ -28,10 +35,7 @@ def normalize_spotify_oauth_config(config: Optional[Dict[str, Any]]) -> Dict[str for key in ("client_id", "client_secret", "redirect_uri"): value = config.get(key, "") if isinstance(value, str): - value = value.strip().strip('"').strip("'") - if key == "redirect_uri": - value = value.rstrip("/") - normalized[key] = value + normalized[key] = value.strip().strip('"').strip("'") else: normalized[key] = value return normalized diff --git a/tests/test_spotify_client.py b/tests/test_spotify_client.py index cf9210ee..86820eef 100644 --- a/tests/test_spotify_client.py +++ b/tests/test_spotify_client.py @@ -6,19 +6,33 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1])) from core.spotify_client import normalize_spotify_oauth_config def test_normalization(): - # Normal case with leading/trailing whitespace and quotes + # Whitespace + quotes are stripped (paste garbage); the redirect_uri's + # trailing slash is PRESERVED — Spotify matches it exactly against the app + # dashboard, so stripping it could break a valid registration (#942 follow-up). config = { - "client_id": " client_id ", + "client_id": ' "client_id" ', "client_secret": " client_secret ", - "redirect_uri": "http://127.0.0.1:8888/callback/" + "redirect_uri": " http://127.0.0.1:8888/callback/ " } expected = { "client_id": "client_id", "client_secret": "client_secret", - "redirect_uri": "http://127.0.0.1:8888/callback" + "redirect_uri": "http://127.0.0.1:8888/callback/" # slash kept } assert normalize_spotify_oauth_config(config) == expected + +def test_trailing_slash_on_redirect_uri_is_preserved(): + """Regression guard: Spotify requires an EXACT redirect-URI match against the + app dashboard, so a trailing slash a user registered must NOT be stripped — + stripping it would send '…/callback' and trigger INVALID_CLIENT (#942).""" + with_slash = {"client_id": "x", "client_secret": "y", + "redirect_uri": "http://127.0.0.1:8888/callback/"} + without_slash = {"client_id": "x", "client_secret": "y", + "redirect_uri": "http://127.0.0.1:8888/callback"} + assert normalize_spotify_oauth_config(with_slash)["redirect_uri"] == "http://127.0.0.1:8888/callback/" + assert normalize_spotify_oauth_config(without_slash)["redirect_uri"] == "http://127.0.0.1:8888/callback" + def test_empty_values(): # Empty input values config = {