From d0497d983ec38475bef38ec28a6183d8a4052ba6 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 29 Mar 2026 09:45:18 -0700 Subject: [PATCH] Enhance database management and validation in DBManager and BotDataViewer - Introduced validation for SQLite journal modes in DBManager, defaulting to "WAL" for invalid inputs and logging warnings. - Added a new utility function in security_utils for validating SQL identifiers to prevent SQL injection. - Updated BotDataViewer to utilize the new journal mode validation and SQL identifier checks, ensuring safer database operations. - Enhanced test coverage for restore functionality, including checks for backup directory configuration and path traversal prevention. --- modules/db_manager.py | 4 +++ modules/security_utils.py | 26 +++++++++++++++ modules/web_viewer/app.py | 66 ++++++++++++++++++++++++++++++++------- tests/test_web_viewer.py | 40 +++++++++++++++++++++--- 4 files changed, 121 insertions(+), 15 deletions(-) diff --git a/modules/db_manager.py b/modules/db_manager.py index 17590e6..17221e6 100644 --- a/modules/db_manager.py +++ b/modules/db_manager.py @@ -12,6 +12,7 @@ from contextlib import asynccontextmanager, contextmanager from typing import Any, Optional from .db_migrations import MigrationRunner +from .security_utils import VALID_JOURNAL_MODES class DBManager: @@ -478,6 +479,9 @@ class DBManager: busy_timeout_ms = default_busy_timeout_ms foreign_keys = bool(foreign_keys) journal_mode = str(journal_mode).strip() or "WAL" + if journal_mode.upper() not in VALID_JOURNAL_MODES: + self.logger.warning(f"Invalid journal_mode {journal_mode!r}, falling back to WAL") + journal_mode = "WAL" try: conn.execute(f"PRAGMA foreign_keys={'ON' if foreign_keys else 'OFF'}") diff --git a/modules/security_utils.py b/modules/security_utils.py index cf7900f..5fff5d8 100644 --- a/modules/security_utils.py +++ b/modules/security_utils.py @@ -243,6 +243,32 @@ def sanitize_input(content: str, max_length: Optional[int] = 500, strip_controls return content.strip() +# Valid SQLite journal modes for PRAGMA journal_mode validation +VALID_JOURNAL_MODES = {"DELETE", "TRUNCATE", "PERSIST", "MEMORY", "WAL", "OFF"} + + +def validate_sql_identifier(identifier: str) -> str: + """ + Validate a SQL identifier (table or column name) for safe interpolation. + + Only allows alphanumeric characters and underscores, must start with + a letter or underscore. This is intentionally strict to prevent SQL injection + when parameterized queries cannot be used (e.g., PRAGMA, REINDEX). + + Args: + identifier: The SQL identifier to validate + + Returns: + The validated identifier string + + Raises: + ValueError: If the identifier contains unsafe characters + """ + if not re.match(r'^[a-zA-Z_][a-zA-Z0-9_]*$', identifier): + raise ValueError(f"Invalid SQL identifier: {identifier!r}") + return identifier + + def validate_api_key_format(api_key: str, min_length: int = 16) -> bool: """ Validate API key format diff --git a/modules/web_viewer/app.py b/modules/web_viewer/app.py index 3617399..6e264c1 100644 --- a/modules/web_viewer/app.py +++ b/modules/web_viewer/app.py @@ -32,6 +32,8 @@ from flask import ( ) from flask_socketio import SocketIO, disconnect, emit +from modules.security_utils import VALID_JOURNAL_MODES, validate_sql_identifier + def _apply_werkzeug_websocket_fix() -> None: """Patch SimpleWebSocketWSGI to call start_response after WebSocket teardown. @@ -99,16 +101,16 @@ class BotDataViewer: self.app.config['SECRET_KEY'] = _secrets.token_hex(32) # Flask-SocketIO configuration following 5.x best practices - self.socketio = SocketIO( - self.app, - cors_allowed_origins="*", + # CORS origins are configured after config is loaded; create without app for now + self._socketio_kwargs = dict( max_http_buffer_size=1000000, # 1MB buffer limit ping_timeout=20, # 20 second ping timeout — 5s was too short when subscribe handlers replay DB history ping_interval=25, # 25 second ping interval (Flask-SocketIO 5.x default) logger=False, # Disable verbose logging engineio_logger=False, # Disable EngineIO logging - async_mode='threading' # Use threading for better stability + async_mode='threading', # Use threading for better stability ) + self.socketio = SocketIO() self.repeater_db_path = repeater_db_path @@ -153,6 +155,14 @@ class BotDataViewer: "or restrict access with host = 127.0.0.1 and firewall rules." ) + # Configure CORS for SocketIO — default to same-origin (no cross-origin) + cors_raw = self.config.get('Web_Viewer', 'cors_allowed_origins', fallback='').strip() + if cors_raw: + cors_origins = cors_raw if cors_raw == '*' else [o.strip() for o in cors_raw.split(',') if o.strip()] + self._socketio_kwargs['cors_allowed_origins'] = cors_origins + # Initialize SocketIO with Flask app now that config is loaded + self.socketio.init_app(self.app, **self._socketio_kwargs) + # Version info for footer (tag or branch/commit/date); computed once at startup self._version_info = self._get_version_info() @@ -347,6 +357,9 @@ class BotDataViewer: foreign_keys = self.config.getboolean("Web_Viewer", "sqlite_foreign_keys", fallback=True) busy_timeout_ms = self.config.getint("Web_Viewer", "sqlite_busy_timeout_ms", fallback=60000) journal_mode = self.config.get("Web_Viewer", "sqlite_journal_mode", fallback="WAL").strip() or "WAL" + if journal_mode.upper() not in VALID_JOURNAL_MODES: + self.logger.warning(f"Invalid journal_mode {journal_mode!r}, falling back to WAL") + journal_mode = "WAL" conn.execute(f"PRAGMA foreign_keys={'ON' if foreign_keys else 'OFF'}") conn.execute(f"PRAGMA busy_timeout={int(busy_timeout_ms)}") conn.execute(f"PRAGMA journal_mode={journal_mode}") @@ -368,6 +381,9 @@ class BotDataViewer: foreign_keys = self.config.getboolean("Web_Viewer", "sqlite_foreign_keys", fallback=True) busy_timeout_ms = self.config.getint("Web_Viewer", "sqlite_busy_timeout_ms", fallback=60000) journal_mode = self.config.get("Web_Viewer", "sqlite_journal_mode", fallback="WAL").strip() or "WAL" + if journal_mode.upper() not in VALID_JOURNAL_MODES: + self.logger.warning(f"Invalid journal_mode {journal_mode!r}, falling back to WAL") + journal_mode = "WAL" conn.execute(f"PRAGMA foreign_keys={'ON' if foreign_keys else 'OFF'}") conn.execute(f"PRAGMA busy_timeout={int(busy_timeout_ms)}") conn.execute(f"PRAGMA journal_mode={journal_mode}") @@ -1471,7 +1487,19 @@ class BotDataViewer: db_file = str(data.get('db_file', '')).strip() if not db_file: return jsonify({'error': 'db_file is required'}), 400 - src = Path(db_file) + + # Validate path is within the configured backup directory + backup_dir_str = self.db_manager.get_metadata('maint.db_backup_dir') or '' + if not backup_dir_str or not os.path.isdir(backup_dir_str): + return jsonify({'error': 'No valid backup directory configured'}), 400 + # Ensure the resolved path is within the backup directory (prevents traversal) + backup_dir = Path(backup_dir_str).resolve() + src = Path(db_file).resolve() + try: + src.relative_to(backup_dir) + except ValueError: + return jsonify({'error': 'Restore path must be within the configured backup directory'}), 403 + if not src.exists(): return jsonify({'error': f'File not found: {db_file}'}), 400 # Validate it is a real SQLite file by checking the magic header @@ -4094,6 +4122,11 @@ class BotDataViewer: stats['active_cache_entries'] = 0 for table in cache_tables: + try: + validate_sql_identifier(table) + except ValueError: + self.logger.warning(f"Skipping invalid table name: {table!r}") + continue cursor.execute(f"SELECT COUNT(*) FROM {table}") count = cursor.fetchone()[0] stats['total_cache_entries'] += count @@ -4373,6 +4406,11 @@ class BotDataViewer: total_records = 0 for table_name in table_names: + try: + validate_sql_identifier(table_name) + except ValueError: + self.logger.warning(f"Skipping invalid table name: {table_name!r}") + continue try: # Get record count cursor.execute(f"SELECT COUNT(*) FROM {table_name}") @@ -4483,12 +4521,18 @@ class BotDataViewer: self.logger.info("Starting database REINDEX...") reindexed_tables = [] for table in tables: - if table != 'sqlite_sequence': # Skip system tables - try: - cursor.execute(f"REINDEX {table}") - reindexed_tables.append(table) - except Exception as e: - self.logger.debug(f"Could not reindex table {table}: {e}") + if table == 'sqlite_sequence': # Skip system tables + continue + try: + validate_sql_identifier(table) + except ValueError: + self.logger.warning(f"Skipping invalid table name for REINDEX: {table!r}") + continue + try: + cursor.execute(f"REINDEX {table}") + reindexed_tables.append(table) + except Exception as e: + self.logger.debug(f"Could not reindex table {table}: {e}") # Get final database size final_size = os.path.getsize(self.db_path) diff --git a/tests/test_web_viewer.py b/tests/test_web_viewer.py index d929252..916a443 100644 --- a/tests/test_web_viewer.py +++ b/tests/test_web_viewer.py @@ -1911,18 +1911,47 @@ class TestRestoreRoute: assert resp.status_code == 400 assert "db_file" in resp.get_json()["error"] - def test_nonexistent_file_returns_400(self, viewer): + def test_nonexistent_file_returns_400(self, viewer, tmp_path): """Returns 400 when db_file path does not exist.""" + backup_dir = tmp_path / "backups" + backup_dir.mkdir() + viewer.db_manager.set_metadata('maint.db_backup_dir', str(backup_dir)) with viewer.app.test_client() as c: resp = c.post("/api/maintenance/restore", - json={"db_file": "/no/such/file.db"}, + json={"db_file": str(backup_dir / "missing.db")}, content_type="application/json") assert resp.status_code == 400 assert "not found" in resp.get_json()["error"].lower() + def test_path_traversal_returns_403(self, viewer, tmp_path): + """Returns 403 when db_file is outside the backup directory.""" + backup_dir = tmp_path / "backups" + backup_dir.mkdir() + viewer.db_manager.set_metadata('maint.db_backup_dir', str(backup_dir)) + outside = tmp_path / "outside.db" + outside.write_bytes(b"SQLite format 3\x00") + with viewer.app.test_client() as c: + resp = c.post("/api/maintenance/restore", + json={"db_file": str(outside)}, + content_type="application/json") + assert resp.status_code == 403 + + def test_no_backup_dir_returns_400(self, viewer): + """Returns 400 when no backup directory is configured.""" + viewer.db_manager.set_metadata('maint.db_backup_dir', '') + with viewer.app.test_client() as c: + resp = c.post("/api/maintenance/restore", + json={"db_file": "/some/file.db"}, + content_type="application/json") + assert resp.status_code == 400 + assert "backup directory" in resp.get_json()["error"].lower() + def test_non_sqlite_file_returns_400(self, viewer, tmp_path): """Returns 400 when the file is not a valid SQLite database.""" - bad = tmp_path / "bad.db" + backup_dir = tmp_path / "backups" + backup_dir.mkdir() + viewer.db_manager.set_metadata('maint.db_backup_dir', str(backup_dir)) + bad = backup_dir / "bad.db" bad.write_bytes(b"not a sqlite file!!") with viewer.app.test_client() as c: resp = c.post("/api/maintenance/restore", @@ -1934,7 +1963,10 @@ class TestRestoreRoute: def test_valid_sqlite_restore_returns_200(self, viewer, tmp_path): """Returns 200 with warning when a valid SQLite backup is restored.""" import sqlite3 as _sql - backup = tmp_path / "backup.db" + backup_dir = tmp_path / "backups" + backup_dir.mkdir() + viewer.db_manager.set_metadata('maint.db_backup_dir', str(backup_dir)) + backup = backup_dir / "backup.db" conn = _sql.connect(str(backup)) conn.execute("CREATE TABLE t (id INTEGER)") conn.commit()