Complete overhaul of Twitch support #1219
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR overhauls Twitch and Kick support by introducing a generic OAuth2 client with PKCE, implementing separate broadcaster and chat token flows, replacing Twitch PubSub with EventSub WebSockets, refactoring chat and redemption modules into modular, type-safe methods, centralizing OAuth2 redirect handling in the webserver, adding a startup splash screen with progress reporting, and expanding tests to cover all new flows. Sequence diagram for OAuth2 redirect handling in webserversequenceDiagram
participant User as User
participant Browser as Browser
participant WebServer as WebHandler
participant OAuth2 as OAuth2Client
User->>Browser: Clicks authentication link
Browser->>WebServer: Sends redirect with code/state
WebServer->>OAuth2: Validates state, exchanges code for token
OAuth2-->>WebServer: Returns access/refresh tokens
WebServer->>WebServer: Stores tokens in config
WebServer-->>Browser: Renders success/failure page
Sequence diagram for Twitch chat authentication with dual tokenssequenceDiagram
participant Tray as Tray
participant TwitchChat as TwitchChat
participant TwitchOAuth2 as TwitchOAuth2
participant TwitchAPI as Twitch
Tray->>TwitchChat: Start chat process
TwitchChat->>TwitchOAuth2: Check for chat token
alt Chat token present and valid
TwitchChat->>TwitchAPI: Authenticate with chat token
else No chat token or invalid
TwitchChat->>TwitchOAuth2: Check for broadcaster token
alt Broadcaster token valid
TwitchChat->>TwitchAPI: Authenticate with broadcaster token
else No valid tokens
TwitchChat-->>Tray: Authentication failed
end
end
TwitchChat->>TwitchAPI: Start chat connection
Sequence diagram for Twitch redemptions using EventSub WebSocketsequenceDiagram
participant Tray as Tray
participant TwitchRedemptions as TwitchRedemptions
participant TwitchOAuth2 as TwitchOAuth2
participant TwitchAPI as Twitch
participant EventSub as EventSubWebsocket
Tray->>TwitchRedemptions: Start redemptions process
TwitchRedemptions->>TwitchOAuth2: Get broadcaster token
TwitchRedemptions->>TwitchAPI: Authenticate with broadcaster token
TwitchRedemptions->>EventSub: Start EventSub WebSocket
EventSub-->>TwitchRedemptions: Channel point redemption event
TwitchRedemptions->>TwitchRedemptions: Process redemption
TwitchRedemptions->>TwitchChat: (optional) Send chat message
Class diagram for new OAuth2 client and service-specific subclassesclassDiagram
class OAuth2Client {
+client_id: str
+client_secret: str
+redirect_uri: str
+access_token: str
+refresh_token: str
+get_auth_url(token_type)
+validate_token_async(token)
+refresh_access_token_async(refresh_token)
+revoke_token(token)
+clear_stored_tokens()
+get_stored_tokens()
+clear_all_authentication()
+static cleanup_stray_temp_credentials(config)
}
class TwitchOAuth2 {
+validate_token_async(token)
+validate_token_sync(token)
+get_oauth_status()
+get_redirect_uri(token_type)
}
class KickOAuth2 {
+validate_token_async(token)
+validate_token_sync(token)
}
OAuth2Client <|-- TwitchOAuth2
OAuth2Client <|-- KickOAuth2
Class diagram for refactored TwitchChat and TwitchRedemptionsclassDiagram
class TwitchChat {
-twitch: Twitch
-chat
-twitchcustom: bool
-tasks: set
+run_chat(twitchlogin)
+_token_validation()
+_refresh_chat_token()
+_try_custom_token(token)
+_try_oauth2_authentication()
+_setup_chat_connection(channel)
+_start_chat_monitoring()
+_cleanup_on_exit(twitchlogin)
+on_twitchchat_incoming_message(msg)
+on_twitchchat_message(msg)
+on_twitchchat_whatsnowplayingversion(cmd)
+check_command_perms(profile, command)
+do_command(msg)
+redemption_to_chat_request_bridge(command, reqdata)
+handle_request(command, params, username)
+_post_template(msg, templatein, moremetadata, jinja2driver)
+stop()
}
class TwitchRedemptions {
-eventsub: EventSubWebsocket
-twitch: Twitch
-user_id: str
+run_redemptions(chat)
+callback_redemption(data)
+stop()
+_setup_eventsub_connection()
+_get_authenticated_user(redemption_login)
+_check_custom_rewards(user)
+_verify_channel_config(user)
+_setup_eventsub_websocket()
}
Class diagram for refactored TwitchSettings UI logicclassDiagram
class TwitchSettings {
-widget
-uihelp
-oauth: TwitchOAuth2
-status_timer
+connect(uihelp, widget)
+load(config, widget)
+save(config, widget, subprocesses)
+verify(widget)
+update_token_name()
+update_oauth_status()
+start_status_timer()
+stop_status_timer()
+cleanup()
+clear_authentication()
+_copy_auth_link()
+_copy_broadcaster_auth_link()
+_copy_chat_auth_link()
+_copy_auth_link_with_message(token_type, success_title, success_message)
}
Class diagram for refactored WebHandler OAuth2 redirect logicclassDiagram
class WebHandler {
+kickredirect_handler(request)
+twitchredirect_handler(request)
+twitchchatredirect_handler(request)
+_handle_oauth_redirect(request, oauth_config)
}
Class diagram for StartupWindow integration and SystemTray refactorclassDiagram
class Tray {
-startup_window: StartupWindow
+_initialize_core_components(beam)
+_setup_about_window()
+_setup_database_and_processes()
+_setup_settings_window(beam)
+_setup_tray_menu()
+_finalize_initialization()
+_handle_installer_dialogs()
+_setup_file_watcher()
+_update_startup_progress(message)
}
class StartupWindow {
+update_progress(message)
+show()
+hide()
}
Tray --> StartupWindow : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aw-was-here - I've reviewed your changes and they look great!
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `nowplaying/twitch/constants.py:15` </location>
<code_context>
+USER_AUTH_SCOPES: list[AuthScope] = [
+ AuthScope.CHAT_READ,
+ AuthScope.CHAT_EDIT,
+ AuthScope.CHANNEL_READ_REDEMPTIONS,
+ AuthScope.USER_BOT
+]
+
</code_context>
<issue_to_address>
USER_AUTH_SCOPES includes 'AuthScope.USER_BOT', which may not exist in all TwitchAPI versions.
'AuthScope.USER_BOT' may not be available in all TwitchAPI versions, which could cause a runtime error. Please check for its existence or specify the minimum required version.
</issue_to_address>
### Comment 2
<location> `nowplaying/twitch/settings.py:55` </location>
<code_context>
- widget.token_lineedit.setText(config.cparser.value('twitchbot/chattoken'))
widget.secret_lineedit.setText(config.cparser.value('twitchbot/secret'))
+
+ # Set single redirect URI
+ port = config.cparser.value('webserver/port', type=int) or 8899
+ redirect_uri = f'http://localhost:{port}/twitchredirect'
+
+ # Save redirect URI to config
+ config.cparser.setValue('twitchbot/redirecturi', redirect_uri)
+
+ if hasattr(widget, 'redirecturi_lineedit'):
</code_context>
<issue_to_address>
Redirect URI is always overwritten on load/save, which may break custom setups.
Currently, the redirect URI is set to a default value regardless of any existing user configuration. Please update the logic to only set the default if no custom value exists, or notify the user before overwriting.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Set single redirect URI
port = config.cparser.value('webserver/port', type=int) or 8899
redirect_uri = f'http://localhost:{port}/twitchredirect'
# Save redirect URI to config
config.cparser.setValue('twitchbot/redirecturi', redirect_uri)
if hasattr(widget, 'redirecturi_lineedit'):
=======
# Set single redirect URI only if not already set
existing_redirect_uri = config.cparser.value('twitchbot/redirecturi', type=str)
if not existing_redirect_uri:
port = config.cparser.value('webserver/port', type=int) or 8899
redirect_uri = f'http://localhost:{port}/twitchredirect'
# Save redirect URI to config
config.cparser.setValue('twitchbot/redirecturi', redirect_uri)
else:
redirect_uri = existing_redirect_uri
if hasattr(widget, 'redirecturi_lineedit'):
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `nowplaying/twitch/settings.py:70` </location>
<code_context>
+ self.update_oauth_status()
self.update_token_name()
+ # Start periodic status updates for real-time refresh detection
+ self.start_status_timer()
+
@staticmethod
</code_context>
<issue_to_address>
Potential for multiple QTimers if load() is called repeatedly.
Call stop_status_timer() before start_status_timer() to prevent multiple active timers and potential resource leaks.
</issue_to_address>
### Comment 4
<location> `nowplaying/twitch/settings.py:214` </location>
<code_context>
+
+ def stop_status_timer(self):
+ ''' Stop periodic status updates '''
+ if self.status_timer:
+ self.status_timer.stop()
+ self.status_timer = None
+
+ def cleanup(self):
</code_context>
<issue_to_address>
Potential for timer not being stopped on widget destruction.
Make sure cleanup() is invoked when the widget is closed, or tie the timer's lifetime to the widget to prevent it from running after destruction.
</issue_to_address>
### Comment 5
<location> `nowplaying/twitch/chat.py:185` </location>
<code_context>
- self.chat = await Chat(
- self.twitch, initial_channel=self.config.cparser.value('twitchbot/channel'))
+ channel = self.config.cparser.value('twitchbot/channel')
+ initial_channels = [channel] if channel else None
+ self.chat = await Chat(self.twitch, initial_channel=initial_channels)
self.chat.register_event(ChatEvent.READY, self.on_twitchchat_ready)
self.chat.register_event(ChatEvent.MESSAGE, self.on_twitchchat_incoming_message)
</code_context>
<issue_to_address>
initial_channel parameter may be None if channel is not set.
Validating that the channel is set before starting the chat will prevent failures or unexpected behavior if initial_channels is None.
</issue_to_address>
### Comment 6
<location> `nowplaying/twitch/oauth2.py:108` </location>
<code_context>
+ if not self.state:
+ self.state = self.config.cparser.value('twitchbot/temp_state')
+
+ if not self.code_verifier:
+ self.cleanup_temp_pkce_params()
+ raise ValueError("Code verifier not generated. Call get_authorization_url first.")
+
+ # Enforce state parameter presence for robust CSRF protection
</code_context>
<issue_to_address>
exchange_code_for_token() assumes code_verifier is present, but may not be if config is cleared.
If code_verifier is missing due to config being cleared or a process restart, the token exchange will fail. Please improve the error message or add a recovery option for this case.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if not self.code_verifier:
self.cleanup_temp_pkce_params()
raise ValueError("Code verifier not generated. Call get_authorization_url first.")
=======
if not self.code_verifier:
self.cleanup_temp_pkce_params()
raise ValueError(
"Code verifier not found. This may be due to configuration being cleared or a process/application restart. "
"Please re-initiate the authorization flow by calling get_authorization_url() again."
)
>>>>>>> REPLACE
</suggested_fix>
### Comment 7
<location> `tests/twitch/test_twitch_utils.py:96` </location>
<code_context>
+ assert result is False
+
+
+def test_qtsafe_validate_twitch_oauth_token_empty():
+ """Test OAuth token validation with empty token."""
+ result = nowplaying.twitch.utils.qtsafe_validate_twitch_oauth_token('')
+ assert result is False
+
+ result = nowplaying.twitch.utils.qtsafe_validate_twitch_oauth_token(None)
</code_context>
<issue_to_address>
Consider adding a test for malformed JSON in token validation.
Add a test case where requests.get returns malformed JSON to verify that qtsafe_validate_twitch_oauth_token handles JSONDecodeError properly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_qtsafe_validate_twitch_oauth_token_empty():
"""Test OAuth token validation with empty token."""
result = nowplaying.twitch.utils.qtsafe_validate_twitch_oauth_token('')
assert result is False
result = nowplaying.twitch.utils.qtsafe_validate_twitch_oauth_token(None)
assert result is False
=======
import json
def test_qtsafe_validate_twitch_oauth_token_empty():
"""Test OAuth token validation with empty token."""
result = nowplaying.twitch.utils.qtsafe_validate_twitch_oauth_token('')
assert result is False
result = nowplaying.twitch.utils.qtsafe_validate_twitch_oauth_token(None)
assert result is False
def test_qtsafe_validate_twitch_oauth_token_malformed_json():
"""Test OAuth token validation with malformed JSON response."""
mock_response = Mock()
mock_response.json.side_effect = json.JSONDecodeError("Expecting value", "doc", 0)
with patch('requests.get', return_value=mock_response):
result = nowplaying.twitch.utils.qtsafe_validate_twitch_oauth_token('token')
assert result is False
>>>>>>> REPLACE
</suggested_fix>
## Security Issues
### Issue 1
<location> `tests/twitch/test_twitch_oauth2.py:441` </location>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (45.82%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1219 +/- ##
==========================================
- Coverage 68.25% 66.69% -1.56%
==========================================
Files 63 68 +5
Lines 10969 11522 +553
==========================================
+ Hits 7487 7685 +198
- Misses 3482 3837 +355
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @aw-was-here - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- The TwitchSettings class has ballooned with both UI wiring and OAuth logic—consider splitting UI-related code from the OAuth2 service into smaller components to improve readability and maintainability.
- The two webserver redirect handlers (twitchredirect_handler and twitchchatredirect_handler) are almost identical; extract the shared flow (state validation, template rendering, PKCE cleanup) into a common helper to reduce duplication.
- In TwitchRedemptions you fall back to a manually built SimpleOAuth for get_user_image—inject or retrieve the real OAuth2 client directly instead of duplicating attributes to keep token handling consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The TwitchSettings class has ballooned with both UI wiring and OAuth logic—consider splitting UI-related code from the OAuth2 service into smaller components to improve readability and maintainability.
- The two webserver redirect handlers (twitchredirect_handler and twitchchatredirect_handler) are almost identical; extract the shared flow (state validation, template rendering, PKCE cleanup) into a common helper to reduce duplication.
- In TwitchRedemptions you fall back to a manually built SimpleOAuth for get_user_image—inject or retrieve the real OAuth2 client directly instead of duplicating attributes to keep token handling consistent.
## Individual Comments
### Comment 1
<location> `nowplaying/twitch/constants.py:17` </location>
<code_context>
+CHAT_BOT_AUTH_SCOPES: list[AuthScope] = [
+ AuthScope.CHAT_READ,
+ AuthScope.CHAT_EDIT,
+ AuthScope.USER_BOT
+]
+
</code_context>
<issue_to_address>
Potentially non-existent AuthScope.USER_BOT used.
If 'USER_BOT' is not defined in AuthScope, this will raise an AttributeError at runtime. Please verify its existence or update the code accordingly.
</issue_to_address>
### Comment 2
<location> `nowplaying/twitch/settings.py:90` </location>
<code_context>
- if (oldchannel != newchannel) or (oldclientid != newclientid) or (
- oldsecret != newsecret) or (oldchattoken != newchattoken):
+ if (oldchannel != newchannel) or (oldclientid != newclientid) or (oldsecret != newsecret):
subprocesses.stop_twitchbot()
+ # Clean up old token storage
</code_context>
<issue_to_address>
No check for chat token changes when restarting twitchbot.
Currently, chat token changes do not trigger a bot restart. Please consider including chat token changes in the restart condition.
</issue_to_address>
### Comment 3
<location> `nowplaying/twitch/settings.py:124` </location>
<code_context>
+
+ def clear_authentication(self):
+ ''' clear stored authentication tokens '''
+ if self.oauth:
+ self.oauth.clear_stored_tokens()
+ self.update_oauth_status()
+ logging.info('Cleared Twitch OAuth2 authentication')
+
</code_context>
<issue_to_address>
clear_authentication() only clears OAuth2 tokens, not chat tokens.
Consider updating clear_authentication() to also clear chat tokens for a complete authentication reset.
Suggested implementation:
```python
def clear_authentication(self):
''' clear stored authentication tokens (OAuth2 and chat) '''
if self.oauth:
self.oauth.clear_stored_tokens()
if hasattr(self, 'chattoken') and self.chattoken:
self.chattoken.clear_stored_tokens()
self.update_oauth_status()
logging.info('Cleared Twitch OAuth2 and chat authentication')
```
If your chat token uses a different method or attribute name for clearing tokens, replace `self.chattoken.clear_stored_tokens()` with the appropriate call.
</issue_to_address>
### Comment 4
<location> `nowplaying/twitch/settings.py:312` </location>
<code_context>
+
+ logging.info('Twitch OAuth2 authentication URL copied to clipboard')
+
+ except Exception as error: # pylint: disable=broad-exception-caught
+ logging.error('Failed to generate auth URL: %s', error)
+ if hasattr(self.widget, 'oauth_status_label'):
+ self.widget.oauth_status_label.setText(f'Error generating URL: {error}')
+
</code_context>
<issue_to_address>
Broad exception handling may mask unexpected errors.
Catch only specific exceptions (such as network-related errors) to avoid hiding other issues and to aid debugging.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
except Exception as error: # pylint: disable=broad-exception-caught
logging.error('Failed to generate auth URL: %s', error)
if hasattr(self.widget, 'oauth_status_label'):
self.widget.oauth_status_label.setText(f'Error generating URL: {error}')
=======
except (OSError, ValueError) as error:
logging.error('Failed to generate auth URL: %s', error)
if hasattr(self.widget, 'oauth_status_label'):
self.widget.oauth_status_label.setText(f'Error generating URL: {error}')
except Exception as error:
logging.exception('Unexpected error during Twitch OAuth2 URL generation')
raise
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `nowplaying/twitch/chat.py:68` </location>
<code_context>
logging.error(line)
- async def _token_validation(self):
- if token := self.config.cparser.value('twitchbot/chattoken'):
- if 'oauth:' in token:
- token = token.replace('oauth:', '')
- self.config.cparser.setValue('twitchbot/chattoken', token)
- logging.debug('validating old token')
- try:
- valid = await validate_token(token)
- if valid.get('status') == 401:
- token = None
- logging.error('Old twitchbot-specific token has expired')
- except Exception as error: #pylint: disable=broad-except
- logging.error('cannot validate token: %s', error)
- token = None
- return token
-
async def run_chat(self, twitchlogin): # pylint: disable=too-many-branches, too-many-statements
</code_context>
<issue_to_address>
Token refresh logic does not handle all error cases.
Currently, when token refresh fails, the invalid token remains in the config, which may cause repeated failures. Please remove invalid tokens from the config after a failed refresh.
</issue_to_address>
### Comment 6
<location> `nowplaying/twitch/chat.py:219` </location>
<code_context>
+ await asyncio.sleep(60)
+ continue
+
+ self.chat = await Chat(self.twitch, initial_channel=[channel.strip()])
self.chat.register_event(ChatEvent.READY, self.on_twitchchat_ready)
self.chat.register_event(ChatEvent.MESSAGE, self.on_twitchchat_incoming_message)
</code_context>
<issue_to_address>
initial_channel is passed as a list, but may expect a string.
Please confirm whether Chat expects a string or a list for initial_channel to avoid potential type issues.
</issue_to_address>
### Comment 7
<location> `nowplaying/twitch/chat.py:256` </location>
<code_context>
await twitchlogin.api_logout()
async def on_twitchchat_ready(self, ready_event):
- ''' twitch chatbot has connected, now join '''
- await ready_event.chat.join_room(self.config.cparser.value('twitchbot/channel'))
+ ''' twitch chatbot has connected and auto-joined initial channels '''
+ # Channel already joined via initial_channel parameter - no manual join needed
+ pass
async def on_twitchchat_incoming_message(self, msg):
</code_context>
<issue_to_address>
on_twitchchat_ready() is now a no-op.
If auto-join makes this handler unnecessary, consider removing its registration to prevent confusion.
Suggested implementation:
```python
self.chat = await Chat(self.twitch, initial_channel=[channel.strip()])
self.chat.register_event(ChatEvent.MESSAGE, self.on_twitchchat_incoming_message)
self.chat.register_command('whatsnowplayingversion',
```
```python
async def on_twitchchat_incoming_message(self, msg):
```
</issue_to_address>
## Security Issues
### Issue 1
<location> `tests/twitch/test_twitch_oauth2.py:466` </location>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @aw-was-here - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- Great work centralizing OAuth2 flows in the new
OAuth2Client, but consider parameterizing the/oauth2/authorizeand/oauth2/tokenendpoint paths rather than hard-coding them so that service-specific differences (e.g. Twitch vs Kick) live in subclass configuration only. - The checks against
EventSubWebsocket._runninguse a protected attribute—please switch to a public API or expose anis_alive()/is_connected()method to avoid relying on internal implementation details. - Both
TwitchChatandTwitchRedemptionshave ballooned in size—splitting out connection setup, message handling, and template rendering into smaller helper classes or modules would improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Great work centralizing OAuth2 flows in the new `OAuth2Client`, but consider parameterizing the `/oauth2/authorize` and `/oauth2/token` endpoint paths rather than hard-coding them so that service-specific differences (e.g. Twitch vs Kick) live in subclass configuration only.
- The checks against `EventSubWebsocket._running` use a protected attribute—please switch to a public API or expose an `is_alive()`/`is_connected()` method to avoid relying on internal implementation details.
- Both `TwitchChat` and `TwitchRedemptions` have ballooned in size—splitting out connection setup, message handling, and template rendering into smaller helper classes or modules would improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `nowplaying/twitch/redemptions.py:138` </location>
<code_context>
- continue
+ def _is_eventsub_running(self) -> bool:
+ ''' Check if EventSub WebSocket is running '''
+ if not self.eventsub:
+ return False
+ return self.eventsub._running # pylint: disable=protected-access
- # you can either start listening before or after you started pubsub.
</code_context>
<issue_to_address>
Accessing protected member _running of EventSubWebsocket.
Accessing a protected member may lead to breakage if the upstream library changes its implementation. Consider using a public API for connection status or add error handling to mitigate this risk.
</issue_to_address>
### Comment 2
<location> `nowplaying/twitch/utils.py:179` </location>
<code_context>
+ async def save_refreshed_tokens(self, access_token: str, refresh_token: str):
</code_context>
<issue_to_address>
save_refreshed_tokens is now async, but may be called by non-async code.
Verify that user_auth_refresh_callback supports async functions; otherwise, this change may introduce runtime errors.
</issue_to_address>
### Comment 3
<location> `nowplaying/twitch/oauth2.py:115` </location>
<code_context>
+ return f'http://localhost:{port}/twitchredirect'
+
+ @staticmethod
+ def validate_token_sync( # pylint: disable=too-many-return-statements
+ token: str | None,
+ return_username: bool = False) -> str | bool | None:
+ ''' Synchronously validate Twitch OAuth token
+
+ Args:
+ token: The OAuth token to validate
+ return_username: If True, return username on success; if False, return boolean
+
+ Returns:
+ - If return_username=True: username string on success, None on failure
+ - If return_username=False: True on success, False on failure
+ '''
+ if not token:
+ return None if return_username else False
+
+ url = 'https://id.twitch.tv/oauth2/validate'
</code_context>
<issue_to_address>
validate_token_sync uses requests synchronously, which may block the UI.
Consider making this function asynchronous or running it in a background thread to prevent UI blocking when called from the main thread.
Suggested implementation:
```python
@staticmethod
def validate_token_sync( # pylint: disable=too-many-return-statements
token: str | None,
return_username: bool = False) -> str | bool | None:
''' Synchronously validate Twitch OAuth token
Args:
token: The OAuth token to validate
return_username: If True, return username on success; if False, return boolean
Returns:
- If return_username=True: username string on success, None on failure
- If return_username=False: True on success, False on failure
'''
if not token:
return None if return_username else False
url = 'https://id.twitch.tv/oauth2/validate'
headers = {'Authorization': f'OAuth {token}'}
try:
req = requests.get(url, headers=headers, timeout=10)
except Exception as error: #pylint: disable=broad-except
logging.error('Twitch token validation check failed: %s', error)
return None if return_username else False
if req.status_code != 200:
if req.status_code == 401:
return None if return_username else False
logging.error('Twitch token validation failed: HTTP %d', req.status_code)
return None if return_username else False
data = req.json()
if return_username:
return data.get('login')
return True
@staticmethod
async def validate_token_async(
token: str | None,
return_username: bool = False) -> str | bool | None:
''' Asynchronously validate Twitch OAuth token
Args:
token: The OAuth token to validate
return_username: If True, return username on success; if False, return boolean
Returns:
- If return_username=True: username string on success, None on failure
- If return_username=False: True on success, False on failure
'''
import httpx
if not token:
return None if return_username else False
url = 'https://id.twitch.tv/oauth2/validate'
headers = {'Authorization': f'OAuth {token}'}
try:
async with httpx.AsyncClient(timeout=10) as client:
resp = await client.get(url, headers=headers)
except Exception as error: #pylint: disable=broad-except
logging.error('Twitch token async validation check failed: %s', error)
return None if return_username else False
if resp.status_code != 200:
if resp.status_code == 401:
return None if return_username else False
logging.error('Twitch token async validation failed: HTTP %d', resp.status_code)
return None if return_username else False
data = resp.json()
if return_username:
return data.get('login')
return True
@staticmethod
def validate_token_in_background(
token: str | None,
return_username: bool = False,
executor=None) -> 'concurrent.futures.Future[str | bool | None]':
'''Run validate_token_sync in a background thread to avoid blocking the UI.
Returns a Future object.
'''
import concurrent.futures
import functools
if executor is None:
executor = concurrent.futures.ThreadPoolExecutor(max_workers=1)
func = functools.partial(
OAuth2.validate_token_sync, token, return_username=return_username)
return executor.submit(func)
```
- You will need to ensure `httpx` is installed in your environment for the async method.
- The new `validate_token_async` method can be called from async code, and `validate_token_in_background` can be used from sync code to avoid blocking the UI.
- If you want to use the background thread helper, you may want to manage the executor's lifecycle elsewhere in your application to avoid thread leaks.
- You may need to adjust the import path for `OAuth2` in the `validate_token_in_background` method if the class name is different.
</issue_to_address>
### Comment 4
<location> `nowplaying/oauth2.py:54` </location>
<code_context>
+ self.client_id: str = self.config.cparser.value(f'{self.config_prefix}/clientid')
+ self.client_secret: str = self.config.cparser.value(f'{self.config_prefix}/secret')
+ # Redirect URI is set dynamically by the calling code, not stored in config
+ self.redirect_uri: str = None
+
+ # PKCE parameters
</code_context>
<issue_to_address>
Type annotation for redirect_uri is inconsistent with initial value.
Since self.redirect_uri is initialized to None, update its type annotation to Optional[str] or str | None for accuracy.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Redirect URI is set dynamically by the calling code, not stored in config
self.redirect_uri: str = None
=======
# Redirect URI is set dynamically by the calling code, not stored in config
self.redirect_uri: str | None = None
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `nowplaying/oauth2.py:136` </location>
<code_context>
- ''' Exchange authorization code for access token '''
+ ''' Exchange authorization code for access token using Kick's endpoint '''
# Load PKCE parameters from config if not already set (for callback handler)
if not self.code_verifier and self.config:
- self.code_verifier = self.config.cparser.value('kick/temp_code_verifier')
+ self.code_verifier = self.config.cparser.value(
</code_context>
<issue_to_address>
Potential for race condition or stale PKCE parameters if multiple OAuth flows run simultaneously.
Storing PKCE parameters in a shared config can cause conflicts if multiple OAuth2 flows run at once. Use separate storage or namespace temp values to prevent this issue.
</issue_to_address>
### Comment 6
<location> `nowplaying/oauth2.py:378` </location>
<code_context>
+ def get_auth_url(self, token_type: str = 'main') -> str | None: # pylint: disable=unused-argument
+ ''' Generate OAuth authentication URL for specified token type '''
+ # Validate configuration
+ if not self.client_id or not self.client_secret:
+ logging.error('OAuth2 configuration incomplete')
+ return None
</code_context>
<issue_to_address>
Returning None on incomplete configuration may mask errors.
Consider raising a ValueError or providing a clearer error message to help callers distinguish configuration issues from other failures.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Validate configuration
if not self.client_id or not self.client_secret:
logging.error('OAuth2 configuration incomplete')
return None
=======
# Validate configuration
if not self.client_id or not self.client_secret:
raise ValueError('OAuth2 configuration incomplete: client_id and/or client_secret are missing')
>>>>>>> REPLACE
</suggested_fix>
### Comment 7
<location> `nowplaying/oauth2.py:362` </location>
<code_context>
+ self.config.cparser.remove(f'{self.config_prefix}/temp_code_verifier')
+ self.config.cparser.remove(f'{self.config_prefix}/temp_state')
+ self.config.save()
+ self.config.cparser.sync() # Ensure cross-process visibility
+ logging.debug('Cleaned up temporary PKCE parameters for %s', self.config_prefix)
+
</code_context>
<issue_to_address>
Reliance on cparser.sync() assumes underlying config backend supports it.
If sync() is unsupported or non-atomic, this could cause race conditions or stale reads in concurrent environments. Confirm the backend handles concurrent access safely.
</issue_to_address>
### Comment 8
<location> `nowplaying/twitch/constants.py:37` </location>
<code_context>
+from nowplaying.twitch.constants import TWITCHBOT_CHECKBOXES, TWITCH_MESSAGE_LIMIT, SPLITMESSAGETEXT
LASTANNOUNCED = {'artist': None, 'title': None}
-SPLITMESSAGETEXT = '****SPLITMESSSAGEHERE****'
-TWITCH_MESSAGE_LIMIT = 500 # Character limit for Twitch messages
-
</code_context>
<issue_to_address>
Typo in SPLITMESSAGETEXT constant: 'MESSSAGE' has three 'S's.
If unintentional, please correct the spelling to '****SPLITMESSAGEHERE****'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
SPLITMESSAGETEXT = '****SPLITMESSSAGEHERE****'
=======
SPLITMESSAGETEXT = '****SPLITMESSAGEHERE****'
>>>>>>> REPLACE
</suggested_fix>
### Comment 9
<location> `tests/twitch/test_twitch_oauth2.py:453` </location>
<code_context>
+ assert oauth1.state != oauth2.state
+
+
+def test_pkce_challenge_calculation(configured_oauth): # pylint: disable=redefined-outer-name
+ """Test PKCE code challenge calculation correctness."""
+ oauth = configured_oauth
+
+ # Set known verifier for predictable challenge
+ oauth.code_verifier = 'test_verifier_123456789'
+ oauth._generate_pkce_parameters() # pylint: disable=protected-access
+
+ # Manually calculate expected challenge
+ expected_challenge = base64.urlsafe_b64encode(
+ hashlib.sha256(oauth.code_verifier.encode('utf-8')).digest()).decode('utf-8').rstrip('=')
+
+ assert oauth.code_challenge == expected_challenge
+
+
</code_context>
<issue_to_address>
Potential issue: PKCE challenge calculation test may not be deterministic.
Since _generate_pkce_parameters() generates a new random code_verifier, the test may not be deterministic. To ensure reliable testing, either refactor the challenge calculation into a separate method or set both code_verifier and code_challenge directly in the test.
</issue_to_address>
### Comment 10
<location> `docs/output/twitchbot.md:4` </location>
<code_context>
**What's Now Playing** integrates with Twitch with channel point
-redemptions and/or chat. They may be run independently or run both at
+redemptions and chat. They may be run independently or run both at
the same time.
</code_context>
<issue_to_address>
Consider clarifying if both redemptions and chat are required or optional.
The change from 'and/or' to 'and' may imply both are required. If each can be used separately, 'and/or' is clearer.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
**What's Now Playing** integrates with Twitch with channel point
redemptions and chat. They may be run independently or run both at
the same time.
=======
**What's Now Playing** integrates with Twitch with channel point
redemptions and/or chat. Each can be used independently, or both can be run at
the same time.
>>>>>>> REPLACE
</suggested_fix>
### Comment 11
<location> `docs/output/twitchbot.md:74` </location>
<code_context>
+ * **Channel**: Your Twitch channel name (where you want to monitor/send messages)
+ * **Client ID**: From your Twitch application
+ * **Client Secret**: From your Twitch application
+ * **Redirect URI**: Will be automatically populated based on your webserver port
+
+ 4. Click "Save" to store the configuration
</code_context>
<issue_to_address>
Clarify that the Redirect URI does not need to be entered manually.
Explicitly mention that the Redirect URI is auto-populated and does not require manual input.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
* **Redirect URI**: Will be automatically populated based on your webserver port
=======
* **Redirect URI**: This field will be automatically populated based on your webserver port and does not require manual input.
>>>>>>> REPLACE
</suggested_fix>
## Security Issues
### Issue 1
<location> `tests/twitch/test_twitch_oauth2.py:466` </location>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @aw-was-here - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- Avoid directly accessing the protected
_runningattribute on EventSubWebsocket inTwitchRedemptions._is_eventsub_running—either use a publicis_connected()API or wrap it to prevent breakage when internals change. - Keep UI concerns out of the process manager—move the
QApplication.processEvents()calls inSubprocessManager.start_all_processesinto the tray or startup window logic so that the process manager stays UI-agnostic. - Unify token persistence in the new OAuth2Client so that refresh and exchange methods automatically save tokens to config, removing the need for callers to write tokens manually and reducing duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid directly accessing the protected `_running` attribute on EventSubWebsocket in `TwitchRedemptions._is_eventsub_running`—either use a public `is_connected()` API or wrap it to prevent breakage when internals change.
- Keep UI concerns out of the process manager—move the `QApplication.processEvents()` calls in `SubprocessManager.start_all_processes` into the tray or startup window logic so that the process manager stays UI-agnostic.
- Unify token persistence in the new OAuth2Client so that refresh and exchange methods automatically save tokens to config, removing the need for callers to write tokens manually and reducing duplication.
## Individual Comments
### Comment 1
<location> `nowplaying/twitch/chat.py:447` </location>
<code_context>
''' take a template, fill it in, and post it '''
- if not jinja2driver:
- jinja2driver = self.jinja2
- if not templatein:
+ # Validate inputs and setup
+ if not self._validate_template_inputs(templatein):
</code_context>
<issue_to_address>
Early return on missing templatein may silently fail.
Consider adding a warning or error log when templatein is missing to make debugging easier.
Suggested implementation:
```python
# Validate inputs and setup
if not self._validate_template_inputs(templatein):
import logging
logging.warning("No template input provided to _post_template; skipping post.")
return
```
If your class already has a logger instance (e.g., `self.log`), replace `logging.warning(...)` with `self.log.warning(...)` for consistency with your logging conventions.
</issue_to_address>
### Comment 2
<location> `nowplaying/twitch/redemptions.py:44` </location>
<code_context>
self.chat = None
- self.uuid = None
- self.pubsub = None
+ self.eventsub: EventSubWebsocket | None = None
self.requests = nowplaying.trackrequests.Requests(config=config, stopevent=stopevent)
self.widgets = None
</code_context>
<issue_to_address>
eventsub._running is a protected member.
Directly accessing a protected attribute can lead to maintenance issues if the library changes. Prefer using a public interface or request one upstream.
Suggested implementation:
```python
def is_eventsub_running(self) -> bool:
"""
Returns True if the EventSubWebsocket is running.
This method encapsulates access to the protected member _running.
If the upstream library provides a public interface in the future,
update this method accordingly.
"""
if self.eventsub is None:
return False
return getattr(self.eventsub, "_running", False)
async def callback_redemption(self, data: ChannelPointsCustomRewardRedemptionAddEvent):
''' handle the channel point redemption '''
```
Replace all usages of `self.eventsub._running` in this file (and elsewhere in the codebase, if applicable) with `self.is_eventsub_running()`. This isolates the protected member access and makes future maintenance easier.
</issue_to_address>
### Comment 3
<location> `nowplaying/twitch/utils.py:21` </location>
<code_context>
- if not user:
- return None
+ # Get user info using the OAuth2 client
+ headers = {'Authorization': f'Bearer {oauth.access_token}', 'Client-Id': oauth.client_id}
+
async with aiohttp.ClientSession() as session:
</code_context>
<issue_to_address>
No check for presence of access_token or client_id before use.
Add a check to ensure oauth.access_token and oauth.client_id are not None before using them, and log a warning or error if missing.
</issue_to_address>
### Comment 4
<location> `nowplaying/twitch/utils.py:179` </location>
<code_context>
+ logging.info('No valid Twitch tokens found. OAuth flow required.')
+ return None
+
+ async def save_refreshed_tokens(self, access_token: str, refresh_token: str):
+ ''' Save automatically refreshed tokens from TwitchAPI library '''
+ oauth_client = await self.get_oauth_client()
</code_context>
<issue_to_address>
save_refreshed_tokens does not handle None values for tokens.
Check if access_token or refresh_token is None before updating the config to prevent potential issues.
</issue_to_address>
### Comment 5
<location> `nowplaying/twitch/launch.py:44` </location>
<code_context>
- # self.redemptions.run_redemptions(self.twitchlogin, self.chat))
- # self.tasks.add(task)
- # task.add_done_callback(self.tasks.discard)
+ if self.redemptions:
+ task = self.loop.create_task(
+ self.redemptions.run_redemptions(self.chat))
</code_context>
<issue_to_address>
Redemptions are now started, but error handling is minimal.
Add error handling or logging to ensure task failures are detected and can be addressed.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if self.redemptions:
task = self.loop.create_task(
self.redemptions.run_redemptions(self.chat))
self.tasks.add(task)
task.add_done_callback(self.tasks.discard)
=======
if self.redemptions:
def log_task_exception(task):
try:
exception = task.exception()
if exception:
import logging
logging.error("Redemptions task failed", exc_info=exception)
except asyncio.CancelledError:
pass
task = self.loop.create_task(
self.redemptions.run_redemptions(self.chat))
self.tasks.add(task)
task.add_done_callback(self.tasks.discard)
task.add_done_callback(log_task_exception)
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `nowplaying/startup.py:44` </location>
<code_context>
+ self.setFixedSize(400, 200)
+
+ # Remove window decorations for splash-like appearance
+ self.setWindowFlags(Qt.WindowType.Dialog | Qt.WindowType.FramelessWindowHint
+ | Qt.WindowType.WindowStaysOnTopHint)
+
</code_context>
<issue_to_address>
Frameless window may impact accessibility and user experience.
A frameless window removes standard controls, so users may not be able to move or close it if issues occur. Please add an explicit close button or another way to dismiss the window.
Suggested implementation:
```python
from PySide6.QtWidgets import ( # pylint: disable=import-error,no-name-in-module
QDialog, QLabel, QProgressBar, QVBoxLayout, QPushButton)
```
```python
def _setup_ui(self) -> None:
"""Set up the startup window UI."""
self.setWindowTitle("Starting What's Now Playing")
self.setModal(True)
self.setFixedSize(400, 200)
# Remove window decorations for splash-like appearance
# Add a close button for accessibility
close_button = QPushButton("Close", self)
close_button.setFixedSize(60, 28)
close_button.clicked.connect(self.close)
close_button.move(self.width() - close_button.width() - 10, 10)
close_button.setToolTip("Close this window")
```
</issue_to_address>
### Comment 7
<location> `nowplaying/startup.py:171` </location>
<code_context>
+ self.progress_bar.setValue(self.progress_value)
+
+ # Force UI update
+ self.repaint()
+
+ def complete_startup(self) -> None:
</code_context>
<issue_to_address>
Consider using QApplication.processEvents() instead of repaint() for UI responsiveness.
QApplication.processEvents() will help keep the UI responsive during long operations, ensuring updates are shown promptly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Force UI update
self.repaint()
=======
# Force UI update
from PyQt5.QtWidgets import QApplication
QApplication.processEvents()
>>>>>>> REPLACE
</suggested_fix>
### Comment 8
<location> `nowplaying/twitch/oauth2.py:125` </location>
<code_context>
+ logging.warning("No token to validate")
+ return None
+
+ headers = {'Authorization': f'OAuth {token}'}
+
+ async with aiohttp.ClientSession() as session:
</code_context>
<issue_to_address>
Twitch API expects 'Bearer' instead of 'OAuth' in Authorization header.
Refer to the Twitch API docs and update the header to use 'Bearer' instead of 'OAuth' to ensure compatibility and prevent future issues.
</issue_to_address>
## Security Issues
### Issue 1
<location> `tests/twitch/test_twitch_oauth2.py:466` </location>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nowplaying/twitch/launch.py
Outdated
Comment on lines
44
to
48
| if self.redemptions: | ||
| task = self.loop.create_task( | ||
| self.redemptions.run_redemptions(self.chat)) | ||
| self.tasks.add(task) | ||
| task.add_done_callback(self.tasks.discard) |
There was a problem hiding this comment.
suggestion (bug_risk): Redemptions are now started, but error handling is minimal.
Add error handling or logging to ensure task failures are detected and can be addressed.
Suggested change
| if self.redemptions: | |
| task = self.loop.create_task( | |
| self.redemptions.run_redemptions(self.chat)) | |
| self.tasks.add(task) | |
| task.add_done_callback(self.tasks.discard) | |
| if self.redemptions: | |
| def log_task_exception(task): | |
| try: | |
| exception = task.exception() | |
| if exception: | |
| import logging | |
| logging.error("Redemptions task failed", exc_info=exception) | |
| except asyncio.CancelledError: | |
| pass | |
| task = self.loop.create_task( | |
| self.redemptions.run_redemptions(self.chat)) | |
| self.tasks.add(task) | |
| task.add_done_callback(self.tasks.discard) | |
| task.add_done_callback(log_task_exception) |
There was a problem hiding this comment.
what is it about AI systems wanting to do imports in the middle of code?
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @aw-was-here - I've reviewed your changes and they look great!
Blocking issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `nowplaying/kick/oauth2.py:22` </location>
<code_context>
- auth_url = self.get_authorization_url(scopes)
+ super().__init__(config, nowplaying.kick.constants.KICK_SERVICE_CONFIG)
+
+ async def validate_token_async(self, token: str | None = None) -> dict[str, Any] | None:
+ ''' Async validate Kick OAuth token using Kick's introspection endpoint '''
+ if not token:
</code_context>
<issue_to_address>
validate_token_async and validate_token have overlapping functionality.
Consider merging these methods to avoid code duplication and maintain consistency.
Suggested implementation:
```python
async def validate_token(self, token: str | None = None) -> dict[str, Any] | None:
'''Validate Kick OAuth token using Kick's introspection endpoint (async)'''
if not token:
token = self.access_token or self.config.cparser.value('kick/accesstoken')
if not token:
return None
url = nowplaying.kick.constants.TOKEN_INTROSPECT_ENDPOINT
headers = {'Authorization': f'Bearer {token}'}
async with aiohttp.ClientSession() as session:
```
- Remove the old `validate_token` (sync) method if it exists elsewhere in the file.
- Update all calls to `validate_token_async` and `validate_token` in the codebase to use `await validate_token(...)`.
- If you need to support both sync and async, consider using a sync wrapper that runs the async method using `asyncio.run()` or similar, but this is not recommended for aiohttp-based code.
</issue_to_address>
### Comment 2
<location> `nowplaying/startup.py:69` </location>
<code_context>
+ top_layout = QHBoxLayout()
+ top_layout.addStretch() # Push button to the right
+
+ self.close_button = QPushButton("×")
+ self.close_button.setFixedSize(20, 20)
+ self.close_button.setStyleSheet("""
</code_context>
<issue_to_address>
The close button uses a multiplication sign which may not render consistently.
Consider replacing the Unicode multiplication sign with a standard close icon or a more widely supported character for better cross-platform compatibility.
Suggested implementation:
```python
self.close_button = QPushButton("X")
```
If you want to use a standard close icon instead of the "X" character, you can replace the line with:
<code>
self.close_button = QPushButton()
self.close_button.setIcon(self.style().standardIcon(QStyle.SP_TitleBarCloseButton))
</code>
You will also need to import QStyle:
<code>
from PyQt5.QtWidgets import QStyle
</code>
Choose the approach that best fits your application's style and dependencies.
</issue_to_address>
### Comment 3
<location> `nowplaying/startup.py:33` </location>
<code_context>
+ self._center_window()
+
+ # Auto-close timer as failsafe (30 seconds max)
+ self.failsafe_timer = QTimer()
+ self.failsafe_timer.timeout.connect(self.accept)
+ self.failsafe_timer.setSingleShot(True)
</code_context>
<issue_to_address>
Failsafe timer may close the window before slow initializations complete.
Consider making the timeout configurable or notifying the user before the timer expires to handle slow initializations or network delays.
Suggested implementation:
```python
super().__init__()
self.bundledir = pathlib.Path(bundledir) if bundledir else None
self.progress_value = 0
self.max_steps = 10 # More detailed steps now
self.drag_position = None # For window dragging
self._failsafe_timeout_ms = kwargs.get('failsafe_timeout_ms', 30000) # Default 30 seconds
self._failsafe_warning_ms = kwargs.get('failsafe_warning_ms', 5000) # Warn 5 seconds before
self._setup_ui()
self._center_window()
# Label to notify user before auto-close
self.failsafe_warning_label = QLabel("")
self.failsafe_warning_label.setStyleSheet("color: red;")
self.layout().addWidget(self.failsafe_warning_label)
# Auto-close timer as failsafe (configurable)
self.failsafe_timer = QTimer()
self.failsafe_timer.timeout.connect(self.accept)
self.failsafe_timer.setSingleShot(True)
self.failsafe_timer.start(self._failsafe_timeout_ms)
# Warning timer to notify user before failsafe triggers
self.failsafe_warning_timer = QTimer()
self.failsafe_warning_timer.setSingleShot(True)
self.failsafe_warning_timer.timeout.connect(self._show_failsafe_warning)
self.failsafe_warning_timer.start(self._failsafe_timeout_ms - self._failsafe_warning_ms)
logging.debug("Startup window initialized")
```
```python
def _setup_ui(self) -> None: # pylint: disable=too-many-statements
"""Set up the startup window UI."""
self.setWindowTitle("Starting What's Now Playing - Press Escape to cancel")
self.setModal(True)
self.setFixedSize(400, 220) # Slightly taller for close button
def _show_failsafe_warning(self):
self.failsafe_warning_label.setText(
"Initialization is taking longer than expected. "
"This window will close in {} seconds.".format(self._failsafe_warning_ms // 1000)
)
```
- You may need to ensure that the main layout is accessible via `self.layout()`. If not, replace `self.layout().addWidget(self.failsafe_warning_label)` with the appropriate method to add a widget to your window's layout.
- Update the constructor signature to accept `**kwargs` or add explicit parameters for `failsafe_timeout_ms` and `failsafe_warning_ms` if you prefer.
- If you want to allow the user to cancel the auto-close, you could add a button or further logic in `_show_failsafe_warning`.
</issue_to_address>
### Comment 4
<location> `nowplaying/startup.py:179` </location>
<code_context>
+ logging.debug('No icon file found')
+ return None
+
+ def update_progress(self, step: str, progress: int | None = None) -> None:
+ """Update the progress display.
+
</code_context>
<issue_to_address>
Progress bar may exceed max_steps if update_progress is called more than max_steps times.
Consider clamping the progress value or resetting it to prevent exceeding max_steps when update_progress is called multiple times without a specified progress argument.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def update_progress(self, step: str, progress: int | None = None) -> None:
"""Update the progress display.
=======
def update_progress(self, step: str, progress: int | None = None) -> None:
"""Update the progress display.
# Clamp or reset progress to not exceed max_steps
if not hasattr(self, 'current_progress'):
self.current_progress = 0
if progress is not None:
self.current_progress = min(progress, self.max_steps)
else:
self.current_progress = min(self.current_progress + 1, self.max_steps)
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `nowplaying/twitch/oauth2.py:125` </location>
<code_context>
+ return None if return_username else False
+
+ url = 'https://id.twitch.tv/oauth2/validate'
+ headers = {'Authorization': f'OAuth {token}'}
+
+ try:
</code_context>
<issue_to_address>
Twitch token validation uses 'OAuth' instead of 'Bearer' in the Authorization header.
Using 'Bearer' aligns with OAuth2 standards and may improve compatibility with future changes.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
headers = {'Authorization': f'OAuth {token}'}
=======
headers = {'Authorization': f'Bearer {token}'}
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `tests/twitch/test_twitch_oauth2.py:453` </location>
<code_context>
+ assert oauth1.state != oauth2.state
+
+
+def test_pkce_challenge_calculation(configured_oauth): # pylint: disable=redefined-outer-name
+ """Test PKCE code challenge calculation correctness."""
+ oauth = configured_oauth
+
+ # Set known verifier for predictable challenge
+ oauth.code_verifier = 'test_verifier_123456789'
+ oauth._generate_pkce_parameters() # pylint: disable=protected-access
+
+ # Manually calculate expected challenge
+ expected_challenge = base64.urlsafe_b64encode(
+ hashlib.sha256(oauth.code_verifier.encode('utf-8')).digest()).decode('utf-8').rstrip('=')
+
+ assert oauth.code_challenge == expected_challenge
+
+
</code_context>
<issue_to_address>
Test may not be verifying PKCE challenge calculation independently.
Since _generate_pkce_parameters() overwrites code_verifier, the test may not be validating the challenge calculation as intended. Refactor to isolate and test the calculation logic directly.
Suggested implementation:
```python
def test_pkce_challenge_calculation_independent():
"""Test PKCE code challenge calculation logic independently."""
code_verifier = 'test_verifier_123456789'
# Manually calculate expected challenge
expected_challenge = base64.urlsafe_b64encode(
hashlib.sha256(code_verifier.encode('utf-8')).digest()).decode('utf-8').rstrip('=')
# Call the calculation logic directly
actual_challenge = nowplaying.twitch.oauth2.TwitchOAuth2.calculate_pkce_challenge(code_verifier)
assert actual_challenge == expected_challenge
```
You must implement the static/class method `calculate_pkce_challenge` in the `TwitchOAuth2` class in `nowplaying.twitch.oauth2`. It should accept a `code_verifier` string and return the PKCE code challenge as calculated in the test.
</issue_to_address>
### Comment 7
<location> `tests/twitch/test_twitch_utils.py:127` </location>
<code_context>
('chat', 'not_authenticated', 'returns_false'),
])
@pytest.mark.asyncio
-async def test_error_handling_scenarios( # pylint: disable=redefined-outer-name
- kick_integration_config,
</code_context>
<issue_to_address>
Missing test for image conversion failure in get_user_image.
Add a test case simulating image2png conversion failure to verify error handling in get_user_image.
</issue_to_address>
### Comment 8
<location> `tests/twitch/test_compat.py:76` </location>
<code_context>
+def test_qt_config_serialization_compatibility(test_settings): # pylint:disable=unused-argument, redefined-outer-name
</code_context>
<issue_to_address>
Test does not verify enum values after deserialization.
Please add assertions to verify that the deserialized list contains AuthScope enums with the correct values.
</issue_to_address>
### Comment 9
<location> `tests/kick/test_kick_oauth2.py:112` </location>
<code_context>
+ if redirect_uri:
+ oauth.redirect_uri = redirect_uri
if expected_error:
with pytest.raises(ValueError, match=expected_error):
</code_context>
<issue_to_address>
Missing test for invalid or malformed client IDs.
Add tests for invalid client IDs (e.g., empty, whitespace, or illegal characters) to verify robust handling.
</issue_to_address>
### Comment 10
<location> `tests/kick/test_kick_oauth2.py:234` </location>
<code_context>
assert oauth.access_token == 'new_token'
assert oauth.refresh_token == 'new_refresh'
else:
if has_refresh_token:
# HTTP error case
- mock_responses.post(f"{oauth.OAUTH_HOST}/oauth/token",
</code_context>
<issue_to_address>
Test does not cover refresh token expiry or invalidation.
Please add a test for scenarios where the refresh token is expired or invalid to verify proper error handling.
</issue_to_address>
### Comment 11
<location> `tests/kick/test_kick_settings.py:270` </location>
<code_context>
+def test_validate_token_sync_scenarios(status_code, response_data, exception_type,
</code_context>
<issue_to_address>
Test for sync token validation could include malformed response structure.
Add a test case for responses that are valid JSON but lack required fields like 'active' or 'client_id' to verify the function returns False.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
(200, {}, requests.RequestException, False), # Network error
(200, {}, ValueError, False), # JSON parsing error
])
def test_validate_token_sync_scenarios(status_code, response_data, exception_type,
expected_result):
=======
(200, {}, requests.RequestException, False), # Network error
(200, {}, ValueError, False), # JSON parsing error
(200, {"foo": "bar"}, None, False), # Malformed JSON: missing 'active' and 'client_id'
])
def test_validate_token_sync_scenarios(status_code, response_data, exception_type,
expected_result):
>>>>>>> REPLACE
</suggested_fix>
## Security Issues
### Issue 1
<location> `tests/twitch/test_twitch_oauth2.py:466` </location>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai summary |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Overhaul Twitch and Kick integration by introducing a generic OAuth2 client, refactoring chat and redemption modules to use modern OAuth2 and EventSub WebSockets, updating the settings UI for dual-token flows, and improving startup feedback and documentation
New Features:
Enhancements:
Documentation:
Tests:
Chores: