security: tighten on-disk secret handling

src/auth/login_view.py

@@ -1,5 +1,6 @@
 """Login view with email/password and MFA support."""
 
+import logging
 from collections.abc import Callable
 from typing import Any
 
@@ -8,6 +9,8 @@
 
 from src.auth.session_manager import SessionManager
 
+logger = logging.getLogger(__name__)
+
 
 class LoginView(ft.Column):
     """Login form with email, password, and optional MFA fields."""
@@ -213,8 +216,9 @@ async def _handle_login(self, e: ft.Event[ft.Button]) -> None:
             self.status_text.color = ft.Colors.RED_400
             await self.password_field.focus()
 
-        except Exception as ex:
-            self.status_text.value = f"Error: {ex}"
+        except Exception:
+            logger.exception("Unexpected error during login")
+            self.status_text.value = "Sign-in failed. Please try again."
             self.status_text.color = ft.Colors.RED_400
 
         finally:

src/auth/session_manager.py

@@ -1,5 +1,8 @@
 """Manages Monarch Money authentication and session persistence."""
 
+import os
+import stat
+import sys
 from pathlib import Path
 
 import keyring
@@ -11,6 +14,82 @@
 SESSION_FILE = SESSION_DIR / "session.pickle"
 
 
+def _session_file_is_safe_to_load(path: Path) -> bool:
+    """Refuse to deserialize session pickle if filesystem metadata is loose.
+
+    The MonarchMoney library uses pickle, so loading an attacker-writable file
+    is RCE. This gate only defends against cross-user tampering: on POSIX we
+    require the path to be a non-symlink regular file owned by the current
+    uid and not group- or world-writable. It does NOT mitigate a malicious
+    process running as the same user — that attacker can write a 0o600 file
+    owned by the user that will pass this check. Removing pickle from the
+    persistence format (or adding a keyring-backed HMAC over the blob) is the
+    only real fix for that threat model. On Windows, mode bits don't reflect
+    NTFS ACLs, so permission checks are skipped, but we still require a
+    regular (non-symlink) file.
+    """
+    try:
+        st = path.lstat()
+    except OSError:
+        return False
+    if stat.S_ISLNK(st.st_mode):
+        # Reject symlinks outright: a symlink owned by us could point at
+        # an attacker-controlled pickle, and the mode bits on the link
+        # itself aren't meaningful.
+        return False
+    if not stat.S_ISREG(st.st_mode):
+        # Directories, FIFOs, sockets, devices — none of these can be
+        # safely unpickled, and unlink() won't even clean up directories.
+        return False
+    if sys.platform == "win32":
+        return True
+    if st.st_uid != os.getuid():
+        return False
+    return not st.st_mode & 0o022
+
+
+def _chmod_session_file() -> None:
+    """Tighten session file perms to 0o600, tolerating non-POSIX filesystems."""
+    try:
+        SESSION_FILE.chmod(0o600)
+    except OSError:
+        pass
+
+
+def _prepare_session_file_for_write() -> None:
+    """Clear any unsafe existing file before save_session.
+
+    MonarchMoney's save_session() writes through whatever is at the path,
+    so a planted filesystem object there can redirect or expose the
+    pickle. An existing regular file is kept only when it's safe: on
+    POSIX, owned by the current uid and with no group or world
+    permission bits set at all (mask 0o077) — otherwise another uid
+    could pre-create the file or leave it group/world-readable before
+    our next save writes secrets into it, and _chmod_session_file()'s
+    0o600 tighten wouldn't close that read window in time. On Windows
+    we accept any regular file because POSIX uid/mode bits don't map
+    to NTFS ACLs. Anything else gets unlinked, and if unlink fails
+    (e.g. a directory was planted) the OSError propagates and save
+    fails closed.
+    """
+    try:
+        st = SESSION_FILE.lstat()
+    except FileNotFoundError:
+        return
+    if stat.S_ISREG(st.st_mode):
+        if sys.platform == "win32":
+            return
+        # Reject any group/other perms (0o077), not just writable bits.
+        # Otherwise a pre-existing 0o644 file (e.g. from a stale login
+        # with a loose umask) leaves a world-readable window between
+        # save_session() writing the pickle and _chmod_session_file()
+        # tightening it. Safer to unlink + let save_session create a
+        # fresh file that hits 0o600 immediately via our chmod.
+        if st.st_uid == os.getuid() and not st.st_mode & 0o077:
+            return
+    SESSION_FILE.unlink()
+
+
 class SessionManager:
     """Handles login, MFA, and session token persistence."""
 
@@ -45,7 +124,16 @@ def clear_credentials(self) -> None:
 
     async def try_restore_session(self) -> bool:
         """Attempt to restore a saved session. Returns True if successful."""
-        if not SESSION_FILE.exists():
+        # No early `SESSION_FILE.exists()` check — it returns False for
+        # dangling symlinks, which would skip the safety gate and leave a
+        # planted symlink in place for a later `save_session()` to follow.
+        # The safety gate handles missing files correctly (lstat → OSError
+        # → False); unlink() below handles "not there" via OSError.
+        if not _session_file_is_safe_to_load(SESSION_FILE):
+            try:
+                SESSION_FILE.unlink()
+            except OSError:
+                pass
             return False
         try:
             self._mm.load_session(str(SESSION_FILE))
@@ -65,19 +153,26 @@ async def try_restore_session(self) -> bool:
     async def login(self, email: str, password: str) -> None:
         """Login with email/password. Raises RequireMFAException if MFA needed."""
         await self._mm.login(email=email, password=password)
+        _prepare_session_file_for_write()
         self._mm.save_session(str(SESSION_FILE))
-        SESSION_FILE.chmod(0o600)
+        _chmod_session_file()
         self._authenticated = True
 
     async def login_with_mfa(self, email: str, password: str, mfa_code: str) -> None:
         """Login with email/password/MFA code."""
         await self._mm.multi_factor_authenticate(email, password, mfa_code)
+        _prepare_session_file_for_write()
         self._mm.save_session(str(SESSION_FILE))
-        SESSION_FILE.chmod(0o600)
+        _chmod_session_file()
         self._authenticated = True
 
     def logout(self) -> None:
         self._authenticated = False
         self.clear_credentials()
-        if SESSION_FILE.exists():
+        # Best-effort cleanup. Catching OSError (not just FileNotFoundError)
+        # so a planted directory at SESSION_FILE doesn't crash logout —
+        # unlink() can't remove dirs and raises IsADirectoryError.
+        try:
             SESSION_FILE.unlink()
+        except OSError:
+            pass

src/data/cache.py

@@ -1,7 +1,10 @@
 """Simple SQLite cache for Monarch data to support offline/fast access."""
 
 import json
+import os
 import sqlite3
+import stat
+import sys
 from datetime import datetime, timedelta
 from pathlib import Path
 from typing import Any
@@ -16,7 +19,60 @@ class DataCache:
 
     def __init__(self, db_path: Path = CACHE_DB) -> None:
         db_path.parent.mkdir(parents=True, exist_ok=True, mode=0o700)
+        # Idempotent pre-create at 0o600 so there's no umask-default
+        # window where sqlite3.connect() creates the file with loose
+        # perms. Only run this path when O_NOFOLLOW is available: without
+        # it, O_CREAT could in theory follow a planted symlink and
+        # create/affect the target before the lstat gate below sees it.
+        # On platforms without O_NOFOLLOW (effectively Windows), skip
+        # the pre-create — we can't express "create without following
+        # symlinks", and the umask-default window doesn't carry the same
+        # meaning there (NTFS ACLs, not POSIX mode bits).
+        # FileExistsError covers the pre-existing regular-file case
+        # (legitimate prior run, or a race). Other OSErrors (EACCES,
+        # ENOSPC, ELOOP from a symlink) propagate.
+        if hasattr(os, "O_NOFOLLOW"):
+            flags = os.O_CREAT | os.O_WRONLY | os.O_EXCL | os.O_NOFOLLOW
+            try:
+                fd = os.open(str(db_path), flags, 0o600)
+                os.close(fd)
+            except FileExistsError:
+                pass
+        # Authoritative validation immediately before sqlite3.connect —
+        # closes the TOCTOU window where a symlink could be swapped in
+        # between pre-create and connect. lstat() so a planted symlink
+        # is seen as a symlink, not followed. On Windows (no pre-create)
+        # the file may not exist yet — let sqlite3.connect create it.
+        try:
+            st = db_path.lstat()
+        except FileNotFoundError:
+            st = None
+        if st is not None:
+            if stat.S_ISLNK(st.st_mode) or not stat.S_ISREG(st.st_mode):
+                raise OSError(f"refusing to use non-regular cache DB path: {db_path}")
+            if sys.platform != "win32" and st.st_uid != os.getuid():
+                raise OSError(f"refusing to use cache DB not owned by current uid: {db_path}")
+        # Tighten perms BEFORE sqlite3.connect — a pre-existing 0o644 DB
+        # would otherwise be read/written with loose perms in the window
+        # before our chmod. For a just-pre-created file this is a no-op
+        # (os.open already set 0o600). On platforms where we skipped the
+        # pre-create (no O_NOFOLLOW) and the DB doesn't exist yet, this
+        # chmod will FileNotFoundError — swallowed — and sqlite3.connect
+        # below creates the file with umask defaults; the post-connect
+        # chmod catches that case. chmod() semantics vary on Windows,
+        # hence the broad except.
+        try:
+            db_path.chmod(0o600)
+        except OSError:
+            pass
         self._conn = sqlite3.connect(str(db_path))
+        # Second chmod so a freshly-created-by-sqlite DB (the no-O_NOFOLLOW
+        # first-run case) lands on 0o600 rather than whatever the umask
+        # produced. No-op when the pre-connect chmod already succeeded.
+        try:
+            db_path.chmod(0o600)
+        except OSError:
+            pass
         self._conn.execute(
             """CREATE TABLE IF NOT EXISTS cache (
                 key TEXT PRIMARY KEY,

tests/test_cache.py

@@ -1,5 +1,8 @@
 """Tests for the data cache."""
 
+import os
+import stat
+import sys
 import time
 from collections.abc import Iterator
 from pathlib import Path
@@ -46,3 +49,90 @@ def test_stores_complex_types(self, cache: DataCache):
         data = [{"id": 1, "name": "test"}, {"id": 2, "values": [1, 2, 3]}]
         cache.set("list", data)
         assert cache.get("list") == data
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits only")
+    def test_db_file_has_restrictive_permissions(self, tmp_path: Path):
+        """The cache DB must be 0o600 so other local users can't read
+        cached balances / transactions (regression for PR #5 review)."""
+        db_path = tmp_path / "perms.db"
+        c = DataCache(db_path=db_path)
+        try:
+            mode = stat.S_IMODE(db_path.stat().st_mode)
+            assert mode == 0o600, f"expected 0o600, got {oct(mode)}"
+        finally:
+            c.close()
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits only")
+    def test_db_file_created_restrictive_even_with_loose_umask(self, tmp_path: Path):
+        """Simulate a loose umask (0o000 → world-writable default) and
+        confirm the pre-create step still lands on 0o600 from the start."""
+        old_umask = os.umask(0)
+        try:
+            db_path = tmp_path / "umask.db"
+            c = DataCache(db_path=db_path)
+            try:
+                mode = stat.S_IMODE(db_path.stat().st_mode)
+                assert mode == 0o600
+            finally:
+                c.close()
+        finally:
+            os.umask(old_umask)
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks")
+    def test_db_path_refuses_symlink(self, tmp_path: Path):
+        """A symlink planted at db_path must be rejected outright —
+        otherwise sqlite3.connect() would follow it and we'd chmod a
+        file outside the cache dir."""
+        target = tmp_path / "elsewhere" / "victim.db"
+        target.parent.mkdir()
+        target.write_bytes(b"")
+        db_path = tmp_path / "cache.db"
+        db_path.symlink_to(target)
+
+        # Match OSError generally (not the specific message): O_NOFOLLOW and
+        # O_EXCL can race on different platforms — macOS raises EEXIST
+        # (caught, then the lstat gate raises "non-regular"), Linux can
+        # raise ELOOP from os.open directly. Both are correct rejections.
+        with pytest.raises(OSError):
+            DataCache(db_path=db_path)
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks")
+    def test_db_path_revalidates_after_file_exists(
+        self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+    ):
+        """Simulate the TOCTOU race: os.open raises FileExistsError (a file
+        was there), but by the time we sqlite3.connect the path could be
+        a symlink. The post-create lstat re-check must catch that."""
+        target = tmp_path / "elsewhere" / "victim.db"
+        target.parent.mkdir()
+        target.write_bytes(b"")
+        db_path = tmp_path / "cache.db"
+
+        real_open = os.open
+
+        def racy_open(*_args, **_kwargs):
+            # Swap in a symlink between pre-create and the re-validation.
+            # Real attack would be an inter-process race; this monkeypatch
+            # reproduces the same end state deterministically.
+            db_path.symlink_to(target)
+            raise FileExistsError
+
+        monkeypatch.setattr("src.data.cache.os.open", racy_open)
+        with pytest.raises(OSError, match="non-regular"):
+            DataCache(db_path=db_path)
+        # Restore so pytest's teardown doesn't trip.
+        monkeypatch.setattr("src.data.cache.os.open", real_open)
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX uid")
+    def test_db_path_refuses_foreign_owned(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch):
+        """If a regular file at db_path is owned by another uid, refuse to
+        use it — another uid with dir-write access could pre-create a DB
+        owned by them, and sqlite3.connect() would dump cached data into
+        their file (chmod would silently no-op for a non-owned file)."""
+        db_path = tmp_path / "cache.db"
+        db_path.write_bytes(b"")  # regular file, owned by us
+        real_uid = os.getuid()
+        monkeypatch.setattr("src.data.cache.os.getuid", lambda: real_uid + 1)
+
+        with pytest.raises(OSError, match="not owned by current uid"):
+            DataCache(db_path=db_path)