diff --git a/core/database.py b/core/database.py index d71b5c64c..a9ad90b8b 100644 --- a/core/database.py +++ b/core/database.py @@ -3,13 +3,16 @@ import logging import sqlite3 from datetime import datetime, timezone from pathlib import Path +from typing import Optional +from urllib.parse import unquote, urlparse from sqlalchemy import event, create_engine, Column, String, Text, Boolean, DateTime, Integer, ForeignKey, JSON, Index, func, text -from sqlalchemy.engine import Engine +from sqlalchemy.engine import Engine, make_url from sqlalchemy.types import TypeDecorator from sqlalchemy.ext.declarative import declarative_base, declared_attr from sqlalchemy.orm import relationship, sessionmaker, backref from src.runtime_paths import get_app_root +from core.platform_compat import safe_chmod, IS_WINDOWS logger = logging.getLogger(__name__) @@ -42,12 +45,28 @@ def _default_database_url() -> str: def _normalize_sqlite_url(url: str) -> str: - if not url.startswith("sqlite:///"): + """Resolve relative ordinary SQLite paths without rewriting URI filenames.""" + try: + parsed = make_url(url) + except Exception: return url - db_path = url.replace("sqlite:///", "", 1) - if db_path == ":memory:" or os.path.isabs(db_path): + + if parsed.get_backend_name() != "sqlite": return url - return f"sqlite:///{(Path(get_app_root()) / db_path).resolve().as_posix()}" + + db_path = parsed.database + if ( + not db_path + or db_path == ":memory:" + or str(db_path).lower().startswith("file:") + or os.path.isabs(str(db_path)) + ): + return url + + absolute_path = (Path(get_app_root()) / str(db_path)).resolve().as_posix() + return parsed.set(database=absolute_path).render_as_string( + hide_password=False + ) # Get database URL from environment, default to SQLite in DATA_DIR @@ -59,6 +78,59 @@ engine = create_engine( connect_args={"check_same_thread": False} if "sqlite" in DATABASE_URL else {} ) + +# Sidecar files SQLite can create next to the main DB. -journal is the default +# rollback journal; -wal/-shm appear once WAL is enabled. Each can hold copies of +# secret-bearing pages, so they get the same 0o600 lockdown as the DB itself. +_SQLITE_SIDECARS = ("-journal", "-wal", "-shm") + + +def _sqlite_db_path(url) -> Optional[str]: + """Return the filesystem path for a file-backed SQLite URL. + + SQLite query parameters such as ``mode=memory`` only affect filename + semantics when SQLAlchemy enables URI handling with ``uri=true``. Ordinary + file URLs must therefore remain file-backed even when they contain a query + parameter named ``mode``. + + For SQLite ``file:`` URIs, an empty authority or ``localhost`` identifies a + local path. Other authorities are retained as UNC-style paths. + """ + if url.get_backend_name() != "sqlite": + return None + + db_path = url.database + if not db_path or db_path == ":memory:": + return None + + db_path = str(db_path) + query = { + str(key).lower(): str(value).strip().lower() + for key, value in dict(getattr(url, "query", {}) or {}).items() + } + uri_enabled = query.get("uri") in {"1", "true", "yes", "on"} + is_file_uri = db_path.lower().startswith("file:") + + if not uri_enabled or not is_file_uri: + return db_path + + if ( + db_path.lower().startswith("file::memory:") + or query.get("mode") == "memory" + ): + return None + + parsed = urlparse(db_path) + fs_path = parsed.path or "" + if not fs_path or fs_path == ":memory:": + return None + + authority = parsed.netloc + if authority and authority.lower() != "localhost": + fs_path = f"//{authority}{fs_path}" + + return unquote(fs_path) + # Create session factory SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) @@ -1819,6 +1891,41 @@ def init_db(): """ _migrate_model_endpoints() Base.metadata.create_all(bind=engine) + # Lock the DB file (and any SQLite sidecars) to 0o600 — it holds bearer-token + # + bcrypt hashes and encrypted provider keys. POSIX only; safe_chmod no-ops + # on Windows (ACL-restricted profile dir) and the path helper returns None for + # Postgres / in-memory. Must stay AFTER create_all: the file is born here at + # the umask default, and nothing below resets the mode. The path comes from + # engine.url (SQLAlchemy's parsed URL), so a driver-qualified or query-tagged + # DATABASE_URL still resolves to the real file instead of slipping through. + db_path = _sqlite_db_path(engine.url) + if db_path is not None: + # Fail closed-loud on the main file: this is the only access control on + # it, so if the chmod genuinely fails (read-only FS, foreign owner) an + # operator should hear about it. safe_chmod also returns False as a + # Windows no-op, so guard on IS_WINDOWS to avoid a spurious warning there. + if not safe_chmod(db_path, 0o600) and not IS_WINDOWS: + logger.warning( + "Could not restrict %s to 0o600; it holds secrets and may be " + "world-readable. Check filesystem permissions and ownership.", + db_path, + ) + # Re-lock any sidecars present at startup. New ones inherit the main + # file's mode (now 0o600, since we set it first), and they're usually + # absent here, but a stale -wal/-shm/-journal left by an older 0o644 + # install could still expose secret pages. Absent sidecars are the + # normal case, not an error — only a failed chmod warrants a warning. + for suffix in _SQLITE_SIDECARS: + sidecar = db_path + suffix + if ( + os.path.exists(sidecar) + and not safe_chmod(sidecar, 0o600) + and not IS_WINDOWS + ): + logger.warning( + "Could not restrict %s to 0o600; it may expose DB pages.", + sidecar, + ) _migrate_add_hidden_models_column() _migrate_add_cached_models_column() _migrate_add_pinned_models_column() diff --git a/tests/test_app_db_permissions.py b/tests/test_app_db_permissions.py new file mode 100644 index 000000000..d6fad8d7e --- /dev/null +++ b/tests/test_app_db_permissions.py @@ -0,0 +1,268 @@ +import os +import sys +import subprocess +from pathlib import Path + +import pytest + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX mode bits (0o600) don't exist on Windows; safe_chmod no-ops there.", +) +def test_app_db_created_with_0600(tmp_path): + """app.db holds secrets — it must not be world-readable. + + Note: under umask 077 a fresh sqlite file is born 0600 and this would pass + even without the chmod; dev/CI umask is 022, where the chmod is what makes + it pass. No umask machinery needed — just don't read a green here as proof + on a 077 box. + + A subprocess (not in-process patching) is used deliberately: the engine + binds to DATABASE_URL at import time, so a fresh interpreter with its own + DATABASE_URL is the clean way to exercise init_db() against a real on-disk + file without rebinding the already-imported engine. + """ + db_file = tmp_path / "app.db" + env = {**os.environ, "DATABASE_URL": f"sqlite:///{db_file}"} + repo_root = Path(__file__).resolve().parents[1] + # Importing core.database runs init_db() against the temp file-backed DB. + # cwd=repo_root so `import core` resolves (the `-c` sys.path[0] is the CWD). + subprocess.run( + [sys.executable, "-c", "import core.database"], + env=env, + cwd=repo_root, + check=True, + ) + assert db_file.exists() + mode = db_file.stat().st_mode & 0o777 + assert mode == 0o600, f"expected 0o600, got 0o{mode:o}" + + # Upgrade path: an already-deployed DB sitting at 0644 must be re-corrected + # on the next startup. The chmod is unconditional (not gated on create_all + # having created the file), so this is the common path for existing installs. + db_file.chmod(0o644) + subprocess.run( + [sys.executable, "-c", "import core.database"], + env=env, + cwd=repo_root, + check=True, + ) + assert db_file.stat().st_mode & 0o777 == 0o600, "existing 0644 DB not re-locked on startup" + + +def test_normalize_sqlite_url_preserves_sqlite_uri_filename(): + """URI filenames must reach SQLAlchemy unchanged for SQLite to parse.""" + from core.database import _normalize_sqlite_url + + url = "sqlite:///file:/tmp/app.db?mode=rwc&uri=true" + assert _normalize_sqlite_url(url) == url + + +def test_sqlite_db_path_handles_driver_and_query_forms(): + """The path fed to chmod must come from SQLAlchemy's parsed URL, not a naive + replace("sqlite:///"). A driver-qualified URL (sqlite+pysqlite://) or one + carrying query args (?cache=shared) would otherwise resolve to the wrong + path and leave the real file world-readable. Pure logic — runs everywhere. + """ + from sqlalchemy.engine import make_url + + from core.database import _sqlite_db_path + + # Plain forms (relative + absolute) resolve to the file path. + assert _sqlite_db_path(make_url("sqlite:///data/app.db")) == "data/app.db" + assert _sqlite_db_path(make_url("sqlite:////abs/app.db")) == "/abs/app.db" + # A driver qualifier must not defeat detection... + assert _sqlite_db_path(make_url("sqlite+pysqlite:///data/app.db")) == "data/app.db" + # ...and query args must be stripped from the path. + assert _sqlite_db_path(make_url("sqlite:///data/app.db?cache=shared")) == "data/app.db" + assert _sqlite_db_path(make_url("sqlite+pysqlite:////abs/app.db?mode=ro")) == "/abs/app.db" + # Nothing to lock for non-file-backed or non-sqlite databases. + assert _sqlite_db_path(make_url("sqlite:///:memory:")) is None + assert _sqlite_db_path(make_url("sqlite://")) is None + assert _sqlite_db_path(make_url("postgresql+psycopg2://u:p@h/db")) is None + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX mode bits (0o600) don't exist on Windows; safe_chmod no-ops there.", +) +def test_app_db_sidecars_relocked(tmp_path): + """Stale SQLite sidecars (-wal/-shm) left by an older 0o644 install hold + copies of DB pages, so startup must re-lock them too — not just app.db. + + The default -journal is transient (SQLite deletes it after the create_all + commit), so it isn't asserted on here; -wal/-shm persist and are the real + exposure once WAL has ever been enabled. + """ + import sqlite3 + + db_file = tmp_path / "app.db" + sqlite3.connect(db_file).close() # a real, pre-existing DB ... + db_file.chmod(0o644) + sidecars = [tmp_path / f"app.db{sfx}" for sfx in ("-wal", "-shm")] + for s in sidecars: + s.write_bytes(b"") + s.chmod(0o644) + + env = {**os.environ, "DATABASE_URL": f"sqlite:///{db_file}"} + repo_root = Path(__file__).resolve().parents[1] + subprocess.run( + [sys.executable, "-c", "import core.database"], + env=env, + cwd=repo_root, + check=True, + ) + + assert db_file.stat().st_mode & 0o777 == 0o600 + for s in sidecars: + assert s.stat().st_mode & 0o777 == 0o600, f"{s.name} not re-locked on startup" + + +def test_sqlite_db_path_handles_file_uri_forms(tmp_path): + """SQLite URI filenames must chmod the real filesystem path, not the + literal file: URI string. Memory URI databases should still be skipped.""" + from sqlalchemy.engine import make_url + + from core.database import _sqlite_db_path + + db_file = tmp_path / "uri-app.db" + + assert ( + _sqlite_db_path(make_url(f"sqlite+pysqlite:///file:{db_file}?mode=rwc&uri=true")) + == str(db_file) + ) + assert ( + _sqlite_db_path(make_url(f"sqlite:///file:{db_file}?cache=shared&uri=true")) + == str(db_file) + ) + + localhost_db = tmp_path / "localhost-uri.db" + assert ( + _sqlite_db_path( + make_url( + f"sqlite+pysqlite:///file://localhost{localhost_db}" + "?mode=rwc&uri=true" + ) + ) + == str(localhost_db) + ) + + non_uri_mode_db = tmp_path / "mode-query-file.db" + assert ( + _sqlite_db_path( + make_url( + f"sqlite+pysqlite:///{non_uri_mode_db}?mode=memory" + ) + ) + == str(non_uri_mode_db) + ) + assert ( + _sqlite_db_path(make_url("sqlite+pysqlite:///file::memory:?cache=shared&uri=true")) + is None + ) + assert ( + _sqlite_db_path(make_url("sqlite+pysqlite:///file:memdb1?mode=memory&cache=shared&uri=true")) + is None + ) + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX mode bits (0o600) don't exist on Windows; safe_chmod no-ops there.", +) +def test_app_db_file_uri_created_with_0600(tmp_path): + """Import-time DB initialization must lock SQLite file: URI databases too.""" + db_file = tmp_path / "uri-app.db" + env = { + **os.environ, + "DATABASE_URL": f"sqlite+pysqlite:///file:{db_file}?mode=rwc&uri=true", + } + repo_root = Path(__file__).resolve().parents[1] + + subprocess.run( + [sys.executable, "-c", "import core.database"], + env=env, + cwd=repo_root, + check=True, + ) + + assert db_file.exists() + mode = db_file.stat().st_mode & 0o777 + assert mode == 0o600, f"expected 0o600, got 0o{mode:o}" + +@pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX mode bits (0o600) don't exist on Windows; safe_chmod no-ops there.", +) +def test_app_db_localhost_file_uri_created_with_0600(tmp_path): + """A file://localhost URI must chmod the local path SQLite opens.""" + db_file = tmp_path / "localhost-uri.db" + env = { + **os.environ, + "DATABASE_URL": ( + f"sqlite+pysqlite:///file://localhost{db_file}" + "?mode=rwc&uri=true" + ), + } + repo_root = Path(__file__).resolve().parents[1] + + subprocess.run( + [sys.executable, "-c", "import core.database"], + env=env, + cwd=repo_root, + check=True, + ) + + assert db_file.exists() + mode = db_file.stat().st_mode & 0o777 + assert mode == 0o600, f"expected 0o600, got 0o{mode:o}" + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX mode bits (0o600) don't exist on Windows; safe_chmod no-ops there.", +) +def test_app_db_non_uri_mode_query_created_with_0600(tmp_path): + """mode=memory without uri=true must not hide a real SQLite file.""" + db_file = tmp_path / "mode-query-file.db" + env = { + **os.environ, + "DATABASE_URL": f"sqlite+pysqlite:///{db_file}?mode=memory", + } + repo_root = Path(__file__).resolve().parents[1] + + subprocess.run( + [sys.executable, "-c", "import core.database"], + env=env, + cwd=repo_root, + check=True, + ) + + assert db_file.exists() + mode = db_file.stat().st_mode & 0o777 + assert mode == 0o600, f"expected 0o600, got 0o{mode:o}" + +@pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX mode bits (0o600) don't exist on Windows; safe_chmod no-ops there.", +) +def test_app_db_plain_file_uri_created_with_0600(tmp_path): + """The documented sqlite:///file: URI form must remain protected.""" + db_file = tmp_path / "plain-uri-app.db" + env = { + **os.environ, + "DATABASE_URL": f"sqlite:///file:{db_file}?mode=rwc&uri=true", + } + repo_root = Path(__file__).resolve().parents[1] + + subprocess.run( + [sys.executable, "-c", "import core.database"], + env=env, + cwd=repo_root, + check=True, + ) + + assert db_file.exists() + mode = db_file.stat().st_mode & 0o777 + assert mode == 0o600, f"expected 0o600, got 0o{mode:o}"