Fix credential persistence deadlock and bound login network waits

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:

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()

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')

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')

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')