From 3e80574728f129e804d79a74ec5c484b42596e31 Mon Sep 17 00:00:00 2001 From: ameel Date: Wed, 9 Sep 2026 20:40:53 +1000 Subject: [PATCH] Fix credential persistence deadlock and bound login network waits --- librespot/core.py | 23 ++++++++++++--- librespot/oauth.py | 18 ++++++++---- tests/test_credential_deadlock.py | 22 ++++++++++++++ tests/test_oauth_regressions.py | 49 +++++++++++++++++++++++++++++++ tests/test_session_timeouts.py | 38 ++++++++++++++++++++++++ 5 files changed, 141 insertions(+), 9 deletions(-) create mode 100644 tests/test_credential_deadlock.py create mode 100644 tests/test_oauth_regressions.py create mode 100644 tests/test_session_timeouts.py diff --git a/librespot/core.py b/librespot/core.py index 2c56814..69fc6f7 100644 --- a/librespot/core.py +++ b/librespot/core.py @@ -450,7 +450,7 @@ def request(service_type: str) -> typing.Any: """ response = requests.get("{}?type={}".format(ApResolver.base_url, - service_type)) + service_type), timeout=(10, 30)) if response.status_code != 200: if response.status_code == 502: raise RuntimeError( @@ -1239,7 +1239,10 @@ def create_client(conf: Configuration) -> requests.Session: return client def credentials(self) -> dict: - ap_welcome = self.ap_welcome() + # Persistence runs before authentication finishes; never wait on our own login. + ap_welcome = getattr(self, '_Session__ap_welcome', None) + if ap_welcome is None: + raise RuntimeError('Session has not received a welcome packet') reusable = ap_welcome.reusable_auth_credentials reusable_type = Authentication.AuthenticationType.Name( ap_welcome.reusable_auth_credentials_type) @@ -1430,7 +1433,14 @@ def __authenticate_partial(self, self.__send_unchecked( Packet.Type.login, client_response_encrypted_proto.SerializeToString()) - packet = self.cipher_pair.receive_encoded(self.connection) + self.connection.set_timeout(30) + try: + packet = self.cipher_pair.receive_encoded(self.connection) + except (RuntimeError, OSError) as exc: + self.connection.close() + raise RuntimeError("Spotify session authentication did not complete: " + str(exc)) from exc + else: + self.connection.set_timeout(0) if packet.is_cmd(Packet.Type.ap_welcome): self.__ap_welcome = Authentication.APWelcome() self.__ap_welcome.ParseFromString(packet.payload) @@ -1990,7 +2000,12 @@ def create(address: str, conf) -> Session.ConnectionHolder: ap_address = address.split(":")[0] ap_port = int(address.split(":")[1]) sock = socket.socket() - sock.connect((ap_address, ap_port)) + sock.settimeout(30) + try: + sock.connect((ap_address, ap_port)) + except BaseException: + sock.close() + raise return Session.ConnectionHolder(sock) def close(self) -> None: diff --git a/librespot/oauth.py b/librespot/oauth.py index b80e961..7585cff 100644 --- a/librespot/oauth.py +++ b/librespot/oauth.py @@ -90,7 +90,7 @@ def request_token(self): if not self.__code: raise RuntimeError("You need to provide a code before!") - request_data = self.__spotify_token_data + request_data = self.__spotify_token_data.copy() request_data["grant_type"] = "authorization_code" request_data["client_id"] = self.__client_id request_data["redirect_uri"] = self.__redirect_url @@ -101,6 +101,7 @@ def request_token(self): self.__spotify_token, headers=CaseInsensitiveDict({"Content-Type": "application/x-www-form-urlencoded"}), data=request_data, + timeout=(10, 30), ) if response.status_code != 200: raise RuntimeError("Received status code %d: %s" % (response.status_code, response.reason)) @@ -113,7 +114,7 @@ def refresh_token(self): if self.__token_expires_at > datetime.now(): return self - request_data = self.__spotify_token_data + request_data = self.__spotify_token_data.copy() request_data["grant_type"] = "refresh_token" request_data["client_id"] = self.__client_id request_data["refresh_token"] = self.__refresh_token @@ -122,6 +123,7 @@ def refresh_token(self): self.__spotify_token, headers=CaseInsensitiveDict({"Content-Type": "application/x-www-form-urlencoded"}), data=request_data, + timeout=(10, 30), ) if response.status_code != 200: raise RuntimeError("Received status code %d: %s" % (response.status_code, response.reason)) @@ -211,14 +213,20 @@ def run_callback_server(self): self.__server_timeout ) logging.info("OAuth: Waiting for callback on %s", url.hostname + ":" + str(url.port)) - self.__start_server() + try: + self.__start_server() + finally: + self.__server.server_close() def flow(self): logging.info("OAuth: Visit in your browser and log in: %s ", self.get_auth_url()) self.run_callback_server() - self.request_token() + try: + self.request_token() + except requests.Timeout as exc: + raise RuntimeError("Spotify token exchange timed out (10s connect / 30s read). Check connectivity to accounts.spotify.com and retry authorization.") from exc return self.get_credentials() def close(self): if self.__server: - self.__server.shutdown() + self.__server.server_close() diff --git a/tests/test_credential_deadlock.py b/tests/test_credential_deadlock.py new file mode 100644 index 0000000..9e21a53 --- /dev/null +++ b/tests/test_credential_deadlock.py @@ -0,0 +1,22 @@ +from librespot.core import Session, Authentication +from unittest.mock import Mock +import faulthandler +faulthandler.dump_traceback_later(5, exit=True) +s = Session.__new__(Session) +s._Session__auth_lock_bool = True +s.ap_welcome = Mock(side_effect=AssertionError('Must not wait for complete authentication')) +s._Session__ap_welcome = Authentication.APWelcome( + canonical_username='test', + reusable_auth_credentials=b'fake-test-credential', + reusable_auth_credentials_type=Authentication.AuthenticationType.AUTHENTICATION_STORED_SPOTIFY_CREDENTIALS) +result = s.credentials() +assert result['username'] == 'test' +s.ap_welcome.assert_not_called() +del s._Session__ap_welcome +try: + s.credentials() + raise AssertionError('Must reject missing welcome') +except RuntimeError: + pass +faulthandler.cancel_dump_traceback_later() +print('PASS: credentials available during unfinished authentication; missing welcome rejected') diff --git a/tests/test_oauth_regressions.py b/tests/test_oauth_regressions.py new file mode 100644 index 0000000..8cde6cb --- /dev/null +++ b/tests/test_oauth_regressions.py @@ -0,0 +1,49 @@ +import importlib.util +from pathlib import Path +from unittest.mock import patch, Mock +import requests +import threading +import urllib.request +import socket + +import librespot.oauth as module +OAuth = module.OAuth + +oauth = OAuth('test-client', 'http://127.0.0.1:4381/login', None) +oauth.set_code('test-code') +response = Mock(status_code=200) +response.json.return_value = {'access_token': 'test-token', 'expires_in': 3600} +with patch.object(module.requests, 'post', return_value=response) as post: + oauth.request_token() + assert post.call_args.kwargs['timeout'] == (10, 30) + assert oauth.has_token() + +oauth2 = OAuth('test', 'http://127.0.0.1:4381/login', None) +with patch.object(oauth2, 'run_callback_server'), patch.object(oauth2, 'request_token', side_effect=requests.Timeout): + try: + oauth2.flow() + raise AssertionError('Expected timeout') + except RuntimeError as exc: + assert 'timed out' in str(exc) + +with socket.socket() as probe: + probe.bind(('127.0.0.1', 0)) + port = probe.getsockname()[1] +callback = OAuth('test', f'http://127.0.0.1:{port}/login', None).set_timeout(5) +ready = threading.Event() +original_init = OAuth.CallbackServer.__init__ +def signal_ready(self, *args, **kwargs): + original_init(self, *args, **kwargs) + ready.set() +with patch.object(OAuth.CallbackServer, '__init__', signal_ready): + thread = threading.Thread(target=callback.run_callback_server) + thread.start() + assert ready.wait(5) + with urllib.request.urlopen(f'http://127.0.0.1:{port}/login?code=test-code', timeout=5) as result: + assert result.status == 200 + thread.join(5) + assert not thread.is_alive() + assert callback._OAuth__code == 'test-code' + assert callback._OAuth__server.socket.fileno() == -1 + callback.close() +print('PASS: token request timeout, timeout error, real local callback, server cleanup') diff --git a/tests/test_session_timeouts.py b/tests/test_session_timeouts.py new file mode 100644 index 0000000..b13910c --- /dev/null +++ b/tests/test_session_timeouts.py @@ -0,0 +1,38 @@ +import importlib.util +from pathlib import Path +from unittest.mock import Mock, patch + +import librespot.core as m + +response = Mock(status_code=200) +response.json.return_value = {'accesspoint': ['example:443']} +with patch.object(m.requests, 'get', return_value=response) as get: + m.ApResolver.request('accesspoint') + assert get.call_args.kwargs['timeout'] == (10, 30) + +sock = Mock() +sock.connect.side_effect = TimeoutError('test timeout') +with patch.object(m.socket, 'socket', return_value=sock): + try: + m.Session.ConnectionHolder.create('example:443', None) + raise AssertionError('Expected timeout') + except TimeoutError: + pass + sock.settimeout.assert_called_with(30) + sock.close.assert_called_once() + +session = m.Session.__new__(m.Session) +session.connection = Mock() +session.cipher_pair = Mock() +session.cipher_pair.receive_encoded.side_effect = RuntimeError('Failed to receive packet') +session._Session__inner = Mock(device_id='0' * 40) +session._Session__send_unchecked = Mock() +creds = m.Authentication.LoginCredentials(typ=m.Authentication.AuthenticationType.AUTHENTICATION_SPOTIFY_TOKEN, auth_data=b'test') +try: + session._Session__authenticate_partial(creds, False) + raise AssertionError('Expected authentication failure') +except RuntimeError as exc: + assert 'Spotify session authentication did not complete' in str(exc) +session.connection.set_timeout.assert_called_with(30) +session.connection.close.assert_called_once() +print('PASS: resolver timeout, connection timeout cleanup, authentication failure cleanup')