-
-
Notifications
You must be signed in to change notification settings - Fork 10
refactor: code refactoring and remove legacy code and components #26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…fetching, replacing credential-derived tokens and direct login.
…fetching, replacing credential-derived tokens and direct login.
…e token salt, and streamline catalog refresh logic.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @TimilsinaBimal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major refactoring of the authentication and user management system, moving away from Watchly-specific credentials and encryption towards a more streamlined approach using Stremio's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a major refactoring of the authentication and user data storage mechanism, simplifying the flow to rely on Stremio's authKey. It also includes a significant UI overhaul for a cleaner user experience. While the simplification is a good goal, I've found several critical issues. The most important is a security regression where sensitive user data (authKey) is now stored in plaintext in Redis. Additionally, there are several places where a None check is missing, which will lead to unhandled exceptions and 500 errors. There's also a user experience regression for new users. I've provided suggestions to fix these issues.
I am having trouble creating individual review comments. Click here to see my feedback.
app/api/endpoints/catalogs.py (45)
The get_user_data method can return None if the token is invalid, but there is no check for this. If credentials is None, the subsequent call to credentials.get("settings", {}) on line 48 will raise an AttributeError, causing a 500 Internal Server Error. You should add a check to handle this case gracefully.
credentials = await token_store.get_user_data(token)
if not credentials:
raise HTTPException(status_code=401, detail="Invalid or expired token. Please reconfigure the addon.")app/api/endpoints/catalogs.py (99)
The get_user_data method can return None if the token is invalid. This is not handled, and a None value for credentials will be passed to refresh_catalogs_for_credentials, causing an AttributeError on line 102. You should add a check to ensure credentials is not None before proceeding.
credentials = await token_store.get_user_data(token)
if not credentials:
raise HTTPException(status_code=401, detail="Invalid or expired token. Please reconfigure the addon.")app/api/endpoints/manifest.py (61)
The get_user_data method can return None for an invalid token. This is not handled, and the call to credentials.get("settings") on line 63 will raise an AttributeError, causing a 500 error. This error will then be cached by alru_cache, causing persistent failures for this token. You should add a check for None and return an empty list to prevent this.
credentials = await token_store.get_user_data(token)
if not credentials:
return []
app/services/catalog_updater.py (19)
The function refresh_catalogs_for_credentials is called from update_catalogs in catalogs.py where credentials can be None. However, this function does not handle the case where credentials is None, which will lead to an AttributeError on the next line. A check should be added at the beginning of this function to handle this possibility gracefully.
async def refresh_catalogs_for_credentials(token: str, credentials: dict[str, Any] | None) -> bool:
if not credentials:
logger.warning(f"[{token}] Attempted to refresh catalogs with no credentials.")
return False
app/services/token_store.py (41-57)
This refactoring removes the encryption of user data before storing it in Redis. The previous implementation used Fernet encryption to protect the payload, which included the sensitive Stremio authKey. Storing this key in plaintext is a significant security regression. If the Redis instance is ever exposed, all user authKeys will be compromised, allowing unauthorized access to their Stremio accounts. It is critical to re-implement encryption for the stored payload.
app/api/endpoints/tokens.py (121-128)
The initial catalog refresh for newly created accounts has been removed. This is a significant regression in user experience, as new users will see empty catalogs until the next scheduled background job runs. The previous implementation correctly triggered a refresh upon account creation. This logic should be restored to provide a good first-time user experience.
try:
token = await token_store.store_user_data(user_id, payload_to_store)
logger.info(f"[{token}] Account {'created' if is_new_account else 'updated'} for user {user_id}")
except RuntimeError as exc:
raise HTTPException(status_code=500, detail="Server configuration error.") from exc
except (redis_exceptions.RedisError, OSError) as exc:
raise HTTPException(status_code=503, detail="Storage temporarily unavailable.") from exc
if is_new_account:
try:
# Trigger initial catalog refresh for new users
from app.services.catalog_updater import refresh_catalogs_for_credentials
await refresh_catalogs_for_credentials(token, payload_to_store)
except Exception as exc:
logger.error(f"[{token}] Initial catalog refresh failed: {exc}", exc_info=True)
# Clean up the created token if refresh fails
await token_store.delete_token(token=token)
raise HTTPException(
status_code=502,
detail="Credentials verified, but Watchly couldn't refresh your catalogs. Please try again.",
) from excapp/core/settings.py (30-33)
This Credentials model is not used anywhere in the codebase. Additionally, a similar, also unused, model exists in the new app/models/token.py file. To avoid confusion and code duplication, this unused model and the app/models/token.py file should be removed.
app/models/token.py (1-11)
This new file and the models within it (UserSettings, Credentials) appear to be unused and redundant. UserSettings is already defined in app/core/settings.py, and the Credentials model here is also unused. To avoid confusion and maintain a clean codebase, this file should be removed.
…kens across endpoints.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant and valuable refactoring by removing legacy code, simplifying API endpoints, and improving security by encrypting the Stremio authKey at rest. The move to store user settings in Redis instead of the URL is a great architectural improvement. However, I've identified a critical bug that prevents new user registration. Additionally, there's a security regression where sensitive user IDs are being logged in plain text. I've also noted some areas for improvement regarding code duplication and potential user experience degradation. Please review the detailed comments.
I am having trouble creating individual review comments. Click here to see my feedback.
app/api/endpoints/tokens.py (96-97)
This check introduces a critical bug that prevents new users from registering. For a new user, existing_data will be None, which will cause this HTTPException to be raised. The previous logic correctly handled this by using is_new_account = not existing_data. This block should be removed to restore the ability to create new accounts.
app/services/catalog_updater.py (129)
The raw token (key) is being logged here. This is a security risk. Please redact sensitive information in logs.
app/api/endpoints/catalogs.py (91)
The raw token (user ID) is being logged in this exception message, which poses a security risk by exposing PII. Please ensure all sensitive information, like tokens, is redacted before being logged.
app/api/endpoints/catalogs.py (103)
The raw token (user ID) is being logged here. This is a security risk as it exposes PII. Please redact sensitive information in logs.
app/api/endpoints/catalogs.py (43)
The raw token, which corresponds to the user ID, is being logged. This is a security risk as it exposes Personally Identifiable Information (PII) in the logs. The redact_token utility was removed in this PR, but a similar mechanism should be used for logging. Please consider re-introducing a utility to mask sensitive tokens in logs.
app/api/endpoints/tokens.py (125)
The raw token (user ID) is being logged, which is a security risk as it can expose PII. Please redact sensitive information before logging.
app/services/catalog_updater.py (132)
The raw token (key) is being logged in this error message. This is a security risk. Please redact sensitive information in logs.
app/api/endpoints/tokens.py (202)
The raw token (user ID) is being logged upon deletion. This is a security risk. Please ensure tokens are redacted in all log messages.
app/models/token.py (1-11)
This new file duplicates the UserSettings and Credentials models already defined in app/core/settings.py. The UserSettings model here is an empty placeholder, which is very confusing and will likely lead to import issues and inconsistencies. Please remove this file and use the models from app/core/settings.py as the single source of truth.
app/services/catalog_updater.py (52)
The raw token (user ID) is being logged. This is a security risk as it exposes PII. Please redact sensitive information in logs.
app/services/catalog_updater.py (121)
The raw token (key) is being logged here. This is a security risk. Please redact sensitive information in logs.
app/api/endpoints/manifest.py (91-147)
The logic for handling the manifest has been significantly simplified by removing support for multiple endpoint variations. However, the _manifest_handler function now contains a broad except Exception block when fetching user settings. While it correctly raises a 401, it's generally better to catch more specific exceptions to avoid masking unrelated issues. For instance, a JSONDecodeError from pydantic or a specific Redis error could be caught.
app/api/endpoints/tokens.py (131-144)
The logic for refreshing catalogs immediately after creating/updating a token has been removed. This also removes the important rollback mechanism that would delete a token for a new user if the initial catalog refresh failed. While background updates will handle this eventually, new users will have a poor experience, seeing no catalogs until the next scheduled job. Was this removal intentional? If not, please consider restoring this immediate feedback loop.
app/services/token_store.py (22)
The instance attribute self._cipher is initialized in _get_cipher but is no longer declared in __init__. It's a good practice to declare all instance attributes in the __init__ method for clarity and to avoid potential AttributeError issues. Please add self._cipher: Fernet | None = None to the __init__ method.
app/services/token_store.py (137)
The descriptive docstring for this method was removed. For better code maintainability and readability, it's good practice to keep docstrings that explain what methods do. Please consider adding it back.
Example:
"""Iterate over all stored payloads, yielding key and payload."""…for enhanced security.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant and valuable refactoring, primarily focused on improving security and simplifying the codebase. Key improvements include removing legacy URL patterns that exposed settings, encrypting sensitive authKey values in storage, and strengthening key derivation with PBKDF2. The codebase is also cleaner with the removal of unused utility functions and the deprecation of username/password authentication in favor of authKey.
My review focuses on a few areas to ensure the refactoring is complete and robust. I've identified a critical functional regression where immediate catalog refreshes upon token creation have been removed. I've also pointed out a potential unhandled exception case in the new token decryption logic, along with some minor code cleanup opportunities and a suggestion to improve cryptographic practices.
I am having trouble creating individual review comments. Click here to see my feedback.
app/api/endpoints/tokens.py (131-145)
The logic to refresh user catalogs immediately after creating or updating a token has been removed. This is a significant functional regression. Previously, refresh_catalogs_for_credentials was called, ensuring that any changes to user settings (like catalog preferences or genre exclusions) were reflected immediately. Without this, users will have to wait for the next scheduled background job, which could be hours away, leading to a poor user experience. This logic, including the rollback mechanism for new accounts on failure, should be restored.
app/services/token_store.py (117-124)
The try...except block in get_user_data only catches json.JSONDecodeError. If self.decrypt_token fails (e.g., if TOKEN_SALT is changed or the data in Redis is corrupted), it will raise a cryptography.fernet.InvalidToken exception, which is not handled here. This will result in an unhandled exception and a 500 Internal Server Error for the user.
It's better to catch this specific exception, log the error, and return None.
Note: You will need to re-import InvalidToken from cryptography.fernet for the suggestion to work:
from cryptography.fernet import Fernet, InvalidToken
try:
data = json.loads(data_raw)
if data.get("authKey"):
data["authKey"] = self.decrypt_token(data["authKey"])
self._payload_cache[token] = data
return data
except (json.JSONDecodeError, InvalidToken):
logger.warning(f"Failed to decode or decrypt data for token {redact_token(token)}")
return None
app/api/endpoints/catalogs.py (21-25)
This if not token: check is redundant because token is a required path parameter in the route @router.get("/{token}/catalog/{type}/{id}.json"). FastAPI ensures that this function will not be called if the token is missing from the URL, so token will always have a value here. This check can be safely removed to simplify the code.
app/api/endpoints/manifest.py (101-103)
The check if not token: is redundant. The token is a required path parameter in the route defined for _manifest_handler's caller (@router.get("/{token}/manifest.json")). FastAPI will handle missing path parameters with a 404 Not Found response, so this check is unnecessary and can be removed.
app/models/token.py (1-11)
This new file appears to be unused throughout the application. It also defines a UserSettings model as an empty class (pass), which could cause confusion with the complete UserSettings model defined in app/core/settings.py. To avoid clutter and potential confusion for future development, this file should be removed.
app/services/token_store.py (47-52)
The PBKDF2HMAC key derivation function is configured with an empty salt (salt=b""). While the primary defense here is the secrecy of TOKEN_SALT, using a non-empty, static salt is a recommended cryptographic practice. It provides an additional layer of security against pre-computation attacks at virtually no cost. Consider using a hardcoded, non-empty byte string for the salt.
kdf = PBKDF2HMAC(
algorithm=hashes.SHA256(),
length=32,
salt=b"watchly-token-salt", # Use a non-empty, static salt
iterations=200_000,
)
…for enhanced security.
* feat: add option to rename, disable, reorder catalogs * feat: new recommendation system based on weights * feat: dynamic row generator * feat: add dynamic row generator with country flag * feat: add watched.loved row * feat: new ui with stremio auth button * feat: add languages selection button in ui for tmdb meta * feat: add option to enable rpdb api key for posters * feat: ui improvements * chore: add vercel app env * chore: improve recommendation weights and refactor * chore: remove unnecessary country flags * add workflows to bump version * chore: bump version to 1.0.0-rc.1 * chore: bump version to 1.0.0-rc.2 * chore: use redis token from config * fix: priotrize keywords for row generation than genre * chore: bump version to v1.0.0-rc.3 * feat: add option to delete account * refactor: update token deletion API to use DELETE / and implement deep cloning for catalog data in the frontend, removing conditional name handling. * opt: improve recommendations using larger candidate pool and better similarity methods * feat: Add genre exclusion UI, store excluded genres in user settings,and apply them during catalog generation (#21) * feat: Add genre exclusion UI, store excluded genres in user settings, and apply them during catalog generation. * refactor: simplify filtering recommendations by excluded genres using list comprehension * refactor: streamline genre exclusion logic for similarity recommendations * feat: Rework UI into a multi-step setup wizard with account management and section navigation (#22) * feat: Rework UI into a multi-step setup wizard with Watchly account management and section navigation * critical: check if user already exists * refactor: remove disabled class from source code button * feat: add translation service to translate catalog names to user language (#23) * feat: add option to rename, enable disable catalogs (#24) * feat: Implement user ID-based token management and Stremio user info fetching, replacing credential-derived tokens and direct login. (#25) * refactor: code refactoring and remove legacy code and components (#26) * feat: add option to rename, enable disable catalogs * feat: add option to rename, enable disable catalogs * feat: Implement user ID-based token management and Stremio user info fetching, replacing credential-derived tokens and direct login. * feat: Implement user ID-based token management and Stremio user info fetching, replacing credential-derived tokens and direct login. * feat: Refactor token management to use a dedicated token model, remove token salt, and streamline catalog refresh logic. * feat: Encrypt auth keys in token store and enforce 401 for invalid tokens across endpoints. * feat: Introduce token redaction utility and apply it to log messages for enhanced security. * feat: Introduce token redaction utility and apply it to log messages for enhanced security. * fernet client management * feat: add migration script to migrate old tokens to new system * feat: check and only update tokens that are installed from the same system * opt: give more weight to items watched recently * docs: add contributing guidelines and acknowledgements * opt: optimize translation service * chore: bump version to v1.0.0-rc.6 * feat: add gemini service to generate catalog names (#29) * Refactor services for improved async handling and error resilience (#30) * refactor: fetch loved and liked items at once and use that * Refactor services for improved async handling and error resilience * chore: bump version to v1.0.0 * feat: better bare row name generation (#32)
No description provided.