From 9bd5003d4fa455eea0e46f5e73af0cd731a417e5 Mon Sep 17 00:00:00 2001 From: EloquentBrush0x <283442588+EloquentBrush0x@users.noreply.github.com> Date: Mon, 18 May 2026 22:06:53 +0300 Subject: [PATCH] fix(spotify): quarantine dead tokens on terminal refresh failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolve_spotify_runtime_credentials() called _refresh_spotify_oauth_state() without a try/except, so a terminal failure (HTTP 400/401, invalid_grant, refresh_token_reused) raised AuthError but left the dead refresh_token in auth.json. Every subsequent session re-read and retried the same token over the network, failing identically each time. Fix: wrap the refresh call and, when exc.relogin_required is True and a refresh_token is present, clear the dead OAuth fields (access_token, refresh_token, expires_at, expires_in, obtained_at) and write a last_auth_error quarantine marker to auth.json before re-raising. The next call sees no access_token and fails fast with spotify_access_token_missing — no network retry — and the user is prompted to re-authenticate. Mirrors the quarantine pattern already in place for Nous, xAI-OAuth, Codex-OAuth (#28116, #28118), and MiniMax-OAuth (#28119). --- hermes_cli/auth.py | 28 +++++- tests/hermes_cli/test_spotify_auth.py | 119 ++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 3 deletions(-) diff --git a/hermes_cli/auth.py b/hermes_cli/auth.py index 83006e0da3e..10d704cee80 100644 --- a/hermes_cli/auth.py +++ b/hermes_cli/auth.py @@ -2899,9 +2899,31 @@ def resolve_spotify_runtime_credentials( if not should_refresh and refresh_if_expiring: should_refresh = _is_expiring(state.get("expires_at"), refresh_skew_seconds) if should_refresh: - state = _refresh_spotify_oauth_state(state) - _store_provider_state(auth_store, "spotify", state, set_active=False) - _save_auth_store(auth_store) + try: + state = _refresh_spotify_oauth_state(state) + _store_provider_state(auth_store, "spotify", state, set_active=False) + _save_auth_store(auth_store) + except AuthError as exc: + if exc.relogin_required and state.get("refresh_token"): + # Terminal refresh failure — clear dead tokens from auth.json + # so subsequent calls fail fast without a network retry. + # Mirrors the Nous / xAI-OAuth / Codex-OAuth / MiniMax pattern. + for _k in ("access_token", "refresh_token", "expires_at", "expires_in", "obtained_at"): + state.pop(_k, None) + state["last_auth_error"] = { + "provider": "spotify", + "code": exc.code or "refresh_failed", + "message": str(exc), + "reason": "runtime_refresh_failure", + "relogin_required": True, + "at": datetime.now(timezone.utc).isoformat(), + } + try: + _store_provider_state(auth_store, "spotify", state, set_active=False) + _save_auth_store(auth_store) + except Exception as _save_exc: + logger.debug("Spotify OAuth: failed to persist quarantined state: %s", _save_exc) + raise access_token = str(state.get("access_token", "") or "").strip() if not access_token: diff --git a/tests/hermes_cli/test_spotify_auth.py b/tests/hermes_cli/test_spotify_auth.py index e5cd548d424..a2aa8e19d10 100644 --- a/tests/hermes_cli/test_spotify_auth.py +++ b/tests/hermes_cli/test_spotify_auth.py @@ -5,6 +5,7 @@ from types import SimpleNamespace import pytest from hermes_cli import auth as auth_mod +from hermes_cli.auth import AuthError, resolve_spotify_runtime_credentials def test_store_provider_state_can_skip_active_provider() -> None: @@ -181,3 +182,121 @@ def test_spotify_interactive_setup_empty_aborts( env_path = tmp_path / ".env" if env_path.exists(): assert "HERMES_SPOTIFY_CLIENT_ID" not in env_path.read_text() + + +# --------------------------------------------------------------------------- +# Quarantine: terminal refresh failure clears dead tokens (#28139) +# --------------------------------------------------------------------------- + +_STALE_SPOTIFY_STATE = { + "client_id": "test-client", + "redirect_uri": "http://127.0.0.1:43827/spotify/callback", + "api_base_url": auth_mod.DEFAULT_SPOTIFY_API_BASE_URL, + "accounts_base_url": auth_mod.DEFAULT_SPOTIFY_ACCOUNTS_BASE_URL, + "scope": auth_mod.DEFAULT_SPOTIFY_SCOPE, + "granted_scope": auth_mod.DEFAULT_SPOTIFY_SCOPE, + "token_type": "Bearer", + "access_token": "dead-access-token", + "refresh_token": "dead-refresh-token", + "expires_at": "2000-01-01T00:00:00+00:00", + "expires_in": 3600, + "obtained_at": "2000-01-01T00:00:00+00:00", + "auth_type": "oauth_pkce", +} + + +def _seed_spotify_state(tmp_path, state: dict) -> None: + with auth_mod._auth_store_lock(): + store = auth_mod._load_auth_store() + store["active_provider"] = "nous" + auth_mod._store_provider_state(store, "spotify", state, set_active=False) + auth_mod._save_auth_store(store) + + +def test_resolve_credentials_quarantines_dead_tokens_on_terminal_refresh_failure( + tmp_path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Terminal refresh failure (relogin_required=True + refresh_token present) + must clear access_token/refresh_token/expires_* from auth.json and write a + last_auth_error marker so subsequent calls fail fast without a network retry. + Mirrors Nous / xAI-OAuth / Codex-OAuth / MiniMax quarantine pattern. + """ + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + _seed_spotify_state(tmp_path, dict(_STALE_SPOTIFY_STATE)) + + def _terminal_refresh(_state, **_kw): + raise AuthError( + "Spotify token refresh failed. Run `hermes auth spotify` again.", + provider="spotify", + code="spotify_refresh_failed", + relogin_required=True, + ) + + monkeypatch.setattr(auth_mod, "_refresh_spotify_oauth_state", _terminal_refresh) + + with pytest.raises(AuthError) as exc_info: + resolve_spotify_runtime_credentials(force_refresh=True) + + assert exc_info.value.code == "spotify_refresh_failed" + assert exc_info.value.relogin_required is True + + persisted = auth_mod.get_provider_auth_state("spotify") + assert persisted is not None + + # Dead OAuth fields must be cleared. + assert "access_token" not in persisted + assert "refresh_token" not in persisted + assert "expires_at" not in persisted + assert "expires_in" not in persisted + assert "obtained_at" not in persisted + + # Non-credential metadata must be preserved. + assert persisted["client_id"] == "test-client" + assert persisted["api_base_url"] == auth_mod.DEFAULT_SPOTIFY_API_BASE_URL + assert persisted["accounts_base_url"] == auth_mod.DEFAULT_SPOTIFY_ACCOUNTS_BASE_URL + + # Structured diagnostic blob must be written. + err = persisted.get("last_auth_error") + assert isinstance(err, dict) + assert err["provider"] == "spotify" + assert err["code"] == "spotify_refresh_failed" + assert err["reason"] == "runtime_refresh_failure" + assert err["relogin_required"] is True + assert "at" in err + + # Active provider must be unchanged. + assert auth_mod.get_active_provider() == "nous" + + +def test_resolve_credentials_does_not_quarantine_on_transient_refresh_failure( + tmp_path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Transient refresh failure (relogin_required=False, e.g. 429 / 5xx) must + NOT trigger the quarantine path — tokens stay on disk for the next attempt. + """ + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + _seed_spotify_state(tmp_path, dict(_STALE_SPOTIFY_STATE)) + + def _transient_refresh(_state, **_kw): + raise AuthError( + "Spotify token refresh failed: connection error", + provider="spotify", + code="spotify_refresh_failed", + relogin_required=False, + ) + + monkeypatch.setattr(auth_mod, "_refresh_spotify_oauth_state", _transient_refresh) + + with pytest.raises(AuthError) as exc_info: + resolve_spotify_runtime_credentials(force_refresh=True) + + assert exc_info.value.relogin_required is False + + # Tokens must be untouched — no quarantine on transient errors. + persisted = auth_mod.get_provider_auth_state("spotify") + assert persisted is not None + assert persisted["refresh_token"] == "dead-refresh-token" + assert persisted["access_token"] == "dead-access-token" + assert "last_auth_error" not in persisted