spotify oauth: keep the redirect_uri trailing slash (follow-up to #942)
#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.
This commit is contained in:
parent
a1b0e787bc
commit
70dba77711
2 changed files with 29 additions and 11 deletions
|
|
@ -17,9 +17,16 @@ logger = get_logger("spotify_client")
|
||||||
def normalize_spotify_oauth_config(config: Optional[Dict[str, Any]]) -> Dict[str, Any]:
|
def normalize_spotify_oauth_config(config: Optional[Dict[str, Any]]) -> Dict[str, Any]:
|
||||||
"""Normalize Spotify OAuth config before building an auth manager.
|
"""Normalize Spotify OAuth config before building an auth manager.
|
||||||
|
|
||||||
Spotify rejects values that include surrounding whitespace, quotes, or
|
Spotify rejects values that include surrounding whitespace or quotes, and the
|
||||||
newline characters. The settings UI can paste values that carry such
|
settings UI can paste values carrying such formatting, so we trim those — they
|
||||||
formatting, so we trim them before sending them to Spotify.
|
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):
|
if not isinstance(config, dict):
|
||||||
return {}
|
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"):
|
for key in ("client_id", "client_secret", "redirect_uri"):
|
||||||
value = config.get(key, "")
|
value = config.get(key, "")
|
||||||
if isinstance(value, str):
|
if isinstance(value, str):
|
||||||
value = value.strip().strip('"').strip("'")
|
normalized[key] = value.strip().strip('"').strip("'")
|
||||||
if key == "redirect_uri":
|
|
||||||
value = value.rstrip("/")
|
|
||||||
normalized[key] = value
|
|
||||||
else:
|
else:
|
||||||
normalized[key] = value
|
normalized[key] = value
|
||||||
return normalized
|
return normalized
|
||||||
|
|
|
||||||
|
|
@ -6,19 +6,33 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
|
||||||
from core.spotify_client import normalize_spotify_oauth_config
|
from core.spotify_client import normalize_spotify_oauth_config
|
||||||
|
|
||||||
def test_normalization():
|
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 = {
|
config = {
|
||||||
"client_id": " client_id ",
|
"client_id": ' "client_id" ',
|
||||||
"client_secret": " client_secret ",
|
"client_secret": " client_secret ",
|
||||||
"redirect_uri": "http://127.0.0.1:8888/callback/"
|
"redirect_uri": " http://127.0.0.1:8888/callback/ "
|
||||||
}
|
}
|
||||||
expected = {
|
expected = {
|
||||||
"client_id": "client_id",
|
"client_id": "client_id",
|
||||||
"client_secret": "client_secret",
|
"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
|
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():
|
def test_empty_values():
|
||||||
# Empty input values
|
# Empty input values
|
||||||
config = {
|
config = {
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue