Refactor bot server setup to use Waitress for production; fallback to Flask dev server for local development. Added timeout to HTTP requests in Fun and Test cogs. Improved error handling for missing environment variables. Enhanced secret key management in Flask app. Added request timeout configuration. Introduced new experimental features including user profile and balance cards, and a Tic-Tac-Toe game with Minimax AI. Addressed various database and security issues, and improved code quality across multiple files.
This commit is contained in:
@@ -0,0 +1,93 @@
|
||||
## Major problems found
|
||||
|
||||
### 1. Database / SQL issues
|
||||
- sql_commands.py
|
||||
- `execute_query()` returns `copy(cursor)` after closing the cursor/connection, which is invalid and likely breaks fetch operations.
|
||||
- `insert()` parses SQL text to build `ON DUPLICATE KEY UPDATE`; this is brittle and unsafe for many query forms.
|
||||
- `delete()` builds SQL using f-strings with `table_name` and column names from caller input, so it can be SQL-injection vulnerable if those values are not controlled.
|
||||
- `create_table_if_not_exists()` also uses unescaped table/schema names via f-string.
|
||||
- `bulk_insert()` assumes params are list of dicts and rebuilds SQL based on keys; this is fragile and can break on mixed column orders.
|
||||
- `DatabaseManager` is instantiated in many modules/cogs, creating multiple separate connection pools instead of reusing one singleton.
|
||||
|
||||
- bank_functions.py
|
||||
- `db = DatabaseManager("economy")` is created at import time and may use incorrect/duplicate env loading.
|
||||
- `bank_data()` treats `fetch_one()` result inconsistently; if it returns `{}` instead of `None`, account creation may not happen.
|
||||
- `update_money()` uses `insert(..., overwrite=True)` to update balances, which is a weird and brittle upsert pattern.
|
||||
- `record_transaction()` calls `db.insert("transactions", ...)` without a proper query skeleton.
|
||||
|
||||
- npc_memory.py
|
||||
- Uses `sqlite3.connect('npc_memory.db')` with a hardcoded path and no thread-safety handling.
|
||||
- Single SQLite connection is reused without controlling concurrency or isolation.
|
||||
|
||||
### 2. Secret/configuration handling
|
||||
- app.py
|
||||
- `app.secret_key = os.getenv("SECRET_KEY")` can be `None`, breaking Flask session security.
|
||||
- Required env-vars are checked, but `DISCORD_BOT_TOKEN` is loaded and never used; config is inconsistent with actual bot startup.
|
||||
- mail.py
|
||||
- Loads email credentials from env and may fail silently if missing.
|
||||
- bot.py
|
||||
- Prints the bot token with `print(token)`; this is a secret leak risk.
|
||||
- Multiple files call `load_dotenv()` repeatedly instead of once at startup.
|
||||
|
||||
### 3. Bot startup / architecture
|
||||
- bot.py
|
||||
- `iterate_prefix("Vamoc")` and `iterate_prefix("!V")` generate all uppercase/lowercase permutations, which is extremely inefficient and unnecessary.
|
||||
- `self.iterate_prefix("Vamoc")+self.iterate_prefix("!V")` creates huge prefixes list and prints it.
|
||||
- A Flask dev server is started in a daemon thread with `app.run(...)`, which is not suitable for production and may cause shutdown issues.
|
||||
- `token` missing path handling is minimal and should use explicit error handling.
|
||||
|
||||
- app.py
|
||||
- Uses Flask dev server, OAuth flows, and session storage without clear production readiness.
|
||||
- Raw `requests` calls to Discord API are used without timeouts in some cases.
|
||||
|
||||
### 4. Code quality / maintainability
|
||||
- economy.py
|
||||
- `from utils.bank_functions import *` is bad practice and obscures imports.
|
||||
- Duplicate `from discord.ext import commands` import.
|
||||
- Many methods have weak validation and inconsistent return semantics.
|
||||
- `check_transfer()` has odd parameter names and does not cleanly separate logic from reply behavior.
|
||||
- `daily` command uses inconsistent datetime handling and stores timestamp as float in DB without schema enforcement.
|
||||
|
||||
- sql_commands.py
|
||||
- Uses `pandas` only for `fetch_as_dataframe`, which is heavy for a bot and likely unnecessary.
|
||||
- `pool_reset_session` config uses `bool(os.getenv(...))`, which returns `True` for any non-empty string, not only `"True"`.
|
||||
|
||||
- README.md
|
||||
- The documented repo structure does not match actual workspace structure.
|
||||
- It claims SQLite usage while sql_commands.py uses MySQL.
|
||||
- It references files/paths that are not present (e.g. `extras/`, `LICENSE`).
|
||||
|
||||
### 5. Dependency / packaging
|
||||
- requirements.txt
|
||||
- Includes large packages like `numpy`, `pandas`, and `mysql-connector` even though the bot only needs lightweight DB access and small utilities.
|
||||
- `mysql-connector==2.2.9` is very old; upgrade or use `mysql-connector-python` / `mysqlclient`.
|
||||
|
||||
### 6. Security / robustness
|
||||
- app.py
|
||||
- OAuth redirect URL and query parameters are concatenated without URL encoding.
|
||||
- Session data stores access tokens directly, which is sensitive.
|
||||
- `get_user_transactions()` builds ORDER BY using string interpolation; it is validated but could be improved with safer mapping.
|
||||
|
||||
- mail.py
|
||||
- HTML email is generated via large f-strings; this is hard to maintain and could be moved to a template.
|
||||
- Many files mix business logic and I/O without error boundaries, so failures can cascade.
|
||||
|
||||
### 7. Other issues / enhancements
|
||||
- Several experimental files under Experimantal and implementing use `input()` and are not production-ready.
|
||||
- npc_memory.py and sql_commands.py should expose a shared data access layer rather than ad-hoc DB wrappers in each module.
|
||||
- Missing tests, linting, formatting, and structured error logging across the repo.
|
||||
- app.py has duplicated code patterns for Discord API requests and could benefit from helper functions.
|
||||
- bot.py and bot_development.py are almost duplicates; consolidate startup logic.
|
||||
|
||||
## Recommended enhancements
|
||||
- Centralize configuration and database initialization.
|
||||
- Use a shared singleton `DatabaseManager` or dependency injection.
|
||||
- Add strong type hints and remove wildcard imports.
|
||||
- Replace Flask dev thread with proper web hosting or decouple the web interface.
|
||||
- Validate all environment variables at startup.
|
||||
- Add command-level checks and better error messages.
|
||||
- Remove or replace heavy dependencies if they are not needed.
|
||||
- Add tests for DB operations, command behavior, and web routes.
|
||||
- Clean up README to match actual repository state.
|
||||
|
||||
> Note: this is a high-level audit, not a full code rewrite. If you want, I can now create a prioritized issue list or fix the worst bugs first.
|
||||
Reference in New Issue
Block a user