From aa91f9a2a784cd066fc41317e77cb5e19af827f2 Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Mon, 20 Apr 2026 01:04:04 +0000
Subject: [PATCH 01/12] security: tighten on-disk secret handling

- cache.py: chmod the SQLite cache DB to 0o600 after creation so cached
  account and transaction data isn't world/group readable on shared
  POSIX systems (the directory was already 0o700, but the file wasn't
  locked down like session.pickle / preferences.json).
- session_manager.py: refuse to load session.pickle unless it is owned
  by the current user and not group/world-writable. pickle
  deserialization of an attacker-writable file is RCE; this closes the
  gap where another process running as the same user could plant a
  malicious pickle.
- login_view.py: stop echoing raw exception messages into the login UI
  on the generic except branch. Log the traceback instead and show a
  generic "Sign-in failed" message so upstream error text can't leak
  into the UI.
---
 src/auth/login_view.py      |  8 ++++++--
 src/auth/session_manager.py | 27 +++++++++++++++++++++++++++
 src/data/cache.py           |  4 ++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/src/auth/login_view.py b/src/auth/login_view.py
index 4b60274..43b447e 100644
--- a/src/auth/login_view.py
+++ b/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:
diff --git a/src/auth/session_manager.py b/src/auth/session_manager.py
index a14137f..d873c87 100644
--- a/src/auth/session_manager.py
+++ b/src/auth/session_manager.py
@@ -1,5 +1,7 @@
 """Manages Monarch Money authentication and session persistence."""
 
+import os
+import sys
 from pathlib import Path
 
 import keyring
@@ -11,6 +13,25 @@
 SESSION_FILE = SESSION_DIR / "session.pickle"
 
 
+def _session_file_is_safe_to_load(path: Path) -> bool:
+    """Refuse to deserialize session pickle if filesystem permissions are loose.
+
+    The MonarchMoney library uses pickle, so loading an attacker-writable file
+    is RCE. On POSIX systems we require the file to be owned by the current
+    user and not group/world-writable. On Windows these checks are skipped
+    (NTFS ACLs are not reflected in stat mode bits).
+    """
+    if sys.platform == "win32":
+        return True
+    try:
+        st = path.stat()
+    except OSError:
+        return False
+    if st.st_uid != os.getuid():
+        return False
+    return not st.st_mode & 0o022
+
+
 class SessionManager:
     """Handles login, MFA, and session token persistence."""
 
@@ -47,6 +68,12 @@ async def try_restore_session(self) -> bool:
         """Attempt to restore a saved session. Returns True if successful."""
         if not SESSION_FILE.exists():
             return False
+        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))
             # Validate the session is still good by making a lightweight call
diff --git a/src/data/cache.py b/src/data/cache.py
index 03316a6..c6a8861 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -17,6 +17,10 @@ class DataCache:
     def __init__(self, db_path: Path = CACHE_DB) -> None:
         db_path.parent.mkdir(parents=True, exist_ok=True, mode=0o700)
         self._conn = sqlite3.connect(str(db_path))
+        try:
+            db_path.chmod(0o600)
+        except OSError:
+            pass  # chmod not supported on all platforms (e.g. Windows)
         self._conn.execute(
             """CREATE TABLE IF NOT EXISTS cache (
                 key TEXT PRIMARY KEY,

From 85d309760b75244c61fefbe43e382e602f652ca5 Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Mon, 20 Apr 2026 01:51:46 +0000
Subject: [PATCH 02/12] security: address PR #5 review feedback

- cache.py: pre-create the SQLite DB file via os.open with mode 0o600
  before sqlite3.connect, so there's no TOCTOU window between
  connect() and chmod() where the file sits with umask-default perms.
- session_manager.py: route every chmod(SESSION_FILE) through a small
  helper that tolerates OSError, and rewrite the safety-gate docstring
  to honestly describe what it does (cross-user defense) and doesn't
  (same-user malicious process) mitigate.
- tests: add POSIX-only regression tests: cache DB must be 0o600 even
  under a zero umask, and try_restore_session() must refuse (and
  delete) session.pickle when it's group/world-writable OR owned by a
  different uid, without ever calling load_session() on the blob.
---
 src/auth/session_manager.py   | 23 ++++++++++++++----
 src/data/cache.py             | 12 +++++++++-
 tests/test_cache.py           | 31 ++++++++++++++++++++++++
 tests/test_session_manager.py | 45 +++++++++++++++++++++++++++++++++++
 4 files changed, 105 insertions(+), 6 deletions(-)

diff --git a/src/auth/session_manager.py b/src/auth/session_manager.py
index d873c87..b7336ec 100644
--- a/src/auth/session_manager.py
+++ b/src/auth/session_manager.py
@@ -17,9 +17,14 @@ def _session_file_is_safe_to_load(path: Path) -> bool:
     """Refuse to deserialize session pickle if filesystem permissions are loose.
 
     The MonarchMoney library uses pickle, so loading an attacker-writable file
-    is RCE. On POSIX systems we require the file to be owned by the current
-    user and not group/world-writable. On Windows these checks are skipped
-    (NTFS ACLs are not reflected in stat mode bits).
+    is RCE. This gate only defends against cross-user tampering: on POSIX we
+    require the file to be 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
+    this check is skipped entirely.
     """
     if sys.platform == "win32":
         return True
@@ -32,6 +37,14 @@ def _session_file_is_safe_to_load(path: Path) -> bool:
     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
+
+
 class SessionManager:
     """Handles login, MFA, and session token persistence."""
 
@@ -93,14 +106,14 @@ 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)
         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)
         self._mm.save_session(str(SESSION_FILE))
-        SESSION_FILE.chmod(0o600)
+        _chmod_session_file()
         self._authenticated = True
 
     def logout(self) -> None:
diff --git a/src/data/cache.py b/src/data/cache.py
index c6a8861..f3d8357 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -1,6 +1,7 @@
 """Simple SQLite cache for Monarch data to support offline/fast access."""
 
 import json
+import os
 import sqlite3
 from datetime import datetime, timedelta
 from pathlib import Path
@@ -16,11 +17,20 @@ class DataCache:
 
     def __init__(self, db_path: Path = CACHE_DB) -> None:
         db_path.parent.mkdir(parents=True, exist_ok=True, mode=0o700)
+        # Create the DB file with 0o600 up front so there's no window
+        # where sqlite3.connect() creates it with the current umask
+        # (typically 0o644) before a follow-up chmod tightens it.
+        if not db_path.exists():
+            try:
+                fd = os.open(str(db_path), os.O_CREAT | os.O_WRONLY | os.O_EXCL, 0o600)
+                os.close(fd)
+            except (FileExistsError, OSError):
+                pass
         self._conn = sqlite3.connect(str(db_path))
         try:
             db_path.chmod(0o600)
         except OSError:
-            pass  # chmod not supported on all platforms (e.g. Windows)
+            pass  # chmod semantics differ across platforms (e.g. Windows)
         self._conn.execute(
             """CREATE TABLE IF NOT EXISTS cache (
                 key TEXT PRIMARY KEY,
diff --git a/tests/test_cache.py b/tests/test_cache.py
index 9e1d1ed..9d8aa32 100644
--- a/tests/test_cache.py
+++ b/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,31 @@ 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)
diff --git a/tests/test_session_manager.py b/tests/test_session_manager.py
index 39804eb..e275aa8 100644
--- a/tests/test_session_manager.py
+++ b/tests/test_session_manager.py
@@ -1,5 +1,6 @@
 """Tests for session manager (auth)."""
 
+import sys
 from pathlib import Path
 from typing import Any, cast
 from unittest.mock import AsyncMock, MagicMock, patch
@@ -95,6 +96,50 @@ async def test_restore_invalid_session(self, mock_keyring, tmp_session, tmp_path
         assert sm.is_authenticated is False
         assert not session_file.exists()
 
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits only")
+    @patch("src.auth.session_manager.keyring")
+    async def test_restore_refuses_world_writable_pickle(self, mock_keyring, tmp_session, tmp_path):
+        """A session file with group/world-write bits must NOT be unpickled
+        (defense against cross-user tampering; regression for PR #5)."""
+        session_file = tmp_path / "session.pickle"
+        session_file.write_bytes(b"fake")
+        session_file.chmod(0o666)
+
+        sm = SessionManager()
+        load_session = MagicMock()
+        cast(Any, sm._mm).load_session = load_session
+
+        assert await sm.try_restore_session() is False
+        assert sm.is_authenticated is False
+        # Unsafe file is deleted so we don't keep tripping the check.
+        assert not session_file.exists()
+        # Crucially, load_session must never have been called on the
+        # attacker-writable blob.
+        load_session.assert_not_called()
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits only")
+    @patch("src.auth.session_manager.keyring")
+    async def test_restore_refuses_foreign_owned_pickle(
+        self, mock_keyring, tmp_session, tmp_path, monkeypatch
+    ):
+        """A session file owned by a different uid must NOT be unpickled."""
+        session_file = tmp_path / "session.pickle"
+        session_file.write_bytes(b"fake")
+        session_file.chmod(0o600)
+
+        import os as _os
+
+        real_uid = _os.getuid()
+        monkeypatch.setattr("src.auth.session_manager.os.getuid", lambda: real_uid + 1)
+
+        sm = SessionManager()
+        load_session = MagicMock()
+        cast(Any, sm._mm).load_session = load_session
+
+        assert await sm.try_restore_session() is False
+        load_session.assert_not_called()
+        assert not session_file.exists()
+
 
 class TestLogin:
     @patch("src.auth.session_manager.keyring")

From 6415315c41f033e798f1c51d42316bba1a1a8a12 Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Mon, 20 Apr 2026 04:02:57 +0000
Subject: [PATCH 03/12] security: reject non-regular and symlink session files

Address the second review pass on PR #5:

- Require the session pickle path to be a regular, non-symlink file
  via lstat + S_ISREG/S_ISLNK. A directory or FIFO owned by the user
  would previously have passed the uid+mode check and then failed
  loudly in load_session (or in the unlink cleanup path, since
  Path.unlink() can't remove directories). Now we fail-closed before
  touching the file, and the Windows branch still rejects non-regular
  paths even though it skips mode bits.
- Tests: add POSIX-only regression for symlinks, and a cross-platform
  regression for a directory planted at the session path (asserts we
  never call load_session and the directory survives the cleanup
  attempt without raising).

The PR body has also been updated to describe the actual threat model
(cross-user tampering only) so the docs and the docstring agree.
---
 src/auth/session_manager.py   | 33 ++++++++++++++++++++-----------
 tests/test_session_manager.py | 37 +++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/src/auth/session_manager.py b/src/auth/session_manager.py
index b7336ec..fab191e 100644
--- a/src/auth/session_manager.py
+++ b/src/auth/session_manager.py
@@ -1,6 +1,7 @@
 """Manages Monarch Money authentication and session persistence."""
 
 import os
+import stat
 import sys
 from pathlib import Path
 
@@ -14,24 +15,34 @@
 
 
 def _session_file_is_safe_to_load(path: Path) -> bool:
-    """Refuse to deserialize session pickle if filesystem permissions are loose.
+    """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 file to be 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
-    this check is skipped entirely.
+    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.
     """
-    if sys.platform == "win32":
-        return True
     try:
-        st = path.stat()
+        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
diff --git a/tests/test_session_manager.py b/tests/test_session_manager.py
index e275aa8..0aa16b2 100644
--- a/tests/test_session_manager.py
+++ b/tests/test_session_manager.py
@@ -140,6 +140,43 @@ async def test_restore_refuses_foreign_owned_pickle(
         load_session.assert_not_called()
         assert not session_file.exists()
 
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks")
+    @patch("src.auth.session_manager.keyring")
+    async def test_restore_refuses_symlink_pickle(self, mock_keyring, tmp_session, tmp_path):
+        """Even a symlink owned by us must not be unpickled — it could
+        point at attacker-controlled content."""
+        real_target = tmp_path / "real.pickle"
+        real_target.write_bytes(b"fake")
+        session_file = tmp_path / "session.pickle"
+        session_file.symlink_to(real_target)
+
+        sm = SessionManager()
+        load_session = MagicMock()
+        cast(Any, sm._mm).load_session = load_session
+
+        assert await sm.try_restore_session() is False
+        load_session.assert_not_called()
+
+    @patch("src.auth.session_manager.keyring")
+    async def test_restore_refuses_directory_at_session_path(
+        self, mock_keyring, tmp_session, tmp_path
+    ):
+        """If the path is a directory (not a regular file), the safety
+        gate must fail-closed without calling load_session — unlink()
+        can't remove a directory, so we must never reach it."""
+        session_file = tmp_path / "session.pickle"
+        session_file.mkdir(mode=0o700)
+
+        sm = SessionManager()
+        load_session = MagicMock()
+        cast(Any, sm._mm).load_session = load_session
+
+        assert await sm.try_restore_session() is False
+        load_session.assert_not_called()
+        # Directory should still exist — unlink() can't delete it, but the
+        # OSError is swallowed so we don't crash the caller.
+        assert session_file.is_dir()
+
 
 class TestLogin:
     @patch("src.auth.session_manager.keyring")

From d9467ba6b490ff8de14f2d64861e326ecbd50fef Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Mon, 20 Apr 2026 04:28:43 +0000
Subject: [PATCH 04/12] security: narrow cache pre-create except to
 FileExistsError

Address PR #5 third review pass: the os.open pre-create block was
catching every OSError, which would silently swallow EACCES / ENOSPC
and let sqlite3.connect then create the DB with umask-default perms,
defeating the hardening. Narrow the except to FileExistsError (the
race between exists() and O_EXCL) and let anything else propagate.
---
 src/data/cache.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/data/cache.py b/src/data/cache.py
index f3d8357..be69719 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -19,12 +19,15 @@ def __init__(self, db_path: Path = CACHE_DB) -> None:
         db_path.parent.mkdir(parents=True, exist_ok=True, mode=0o700)
         # Create the DB file with 0o600 up front so there's no window
         # where sqlite3.connect() creates it with the current umask
-        # (typically 0o644) before a follow-up chmod tightens it.
+        # (typically 0o644) before a follow-up chmod tightens it. We only
+        # swallow FileExistsError (the race between exists() and O_EXCL);
+        # any other OSError (EACCES, ENOSPC, …) must propagate so we
+        # don't silently fall back to umask-default perms.
         if not db_path.exists():
             try:
                 fd = os.open(str(db_path), os.O_CREAT | os.O_WRONLY | os.O_EXCL, 0o600)
                 os.close(fd)
-            except (FileExistsError, OSError):
+            except FileExistsError:
                 pass
         self._conn = sqlite3.connect(str(db_path))
         try:

From a61d81bcb9e818bbbc505012c8b1a931010f71d5 Mon Sep 17 00:00:00 2001
From: Rex Lorenzo <rexlorenzo@gmail.com>
Date: Fri, 24 Apr 2026 00:27:16 -0700
Subject: [PATCH 05/12] security: close dangling-symlink and symlinked-db
 bypass gaps
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

PR #5 review (Copilot) flagged two remaining gaps in the on-disk
hardening:

- session_manager.try_restore_session() previously short-circuited on
  Path.exists(), which returns False for dangling symlinks. A planted
  symlink at session.pickle would therefore skip the safety gate and
  stay on disk for a subsequent save_session() to follow. Remove the
  early exists() check; the safety gate already handles missing files
  (lstat → OSError → False). Logout's exists()/unlink has the same
  failure mode — switch to unconditional unlink + FileNotFoundError.
- cache.DataCache.__init__ validated db_path with exists(), which
  follows symlinks. sqlite3.connect() + chmod would then operate on
  the symlink target. Use lstat() to reject symlinks and non-regular
  files up front, add O_NOFOLLOW to the pre-create os.open.

Added regression tests for both cases.
---
 src/auth/session_manager.py   | 14 +++++++++++---
 src/data/cache.py             | 27 +++++++++++++++++++--------
 tests/test_cache.py           | 14 ++++++++++++++
 tests/test_session_manager.py | 19 +++++++++++++++++++
 4 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/src/auth/session_manager.py b/src/auth/session_manager.py
index fab191e..45b0b11 100644
--- a/src/auth/session_manager.py
+++ b/src/auth/session_manager.py
@@ -90,8 +90,11 @@ 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():
-            return False
+        # 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()
@@ -130,5 +133,10 @@ async def login_with_mfa(self, email: str, password: str, mfa_code: str) -> None
     def logout(self) -> None:
         self._authenticated = False
         self.clear_credentials()
-        if SESSION_FILE.exists():
+        # Unconditional unlink so a dangling symlink at SESSION_FILE gets
+        # removed too — Path.exists() would report False and leave the
+        # redirect in place.
+        try:
             SESSION_FILE.unlink()
+        except FileNotFoundError:
+            pass
diff --git a/src/data/cache.py b/src/data/cache.py
index be69719..7506afe 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -3,6 +3,7 @@
 import json
 import os
 import sqlite3
+import stat
 from datetime import datetime, timedelta
 from pathlib import Path
 from typing import Any
@@ -17,18 +18,28 @@ class DataCache:
 
     def __init__(self, db_path: Path = CACHE_DB) -> None:
         db_path.parent.mkdir(parents=True, exist_ok=True, mode=0o700)
-        # Create the DB file with 0o600 up front so there's no window
-        # where sqlite3.connect() creates it with the current umask
-        # (typically 0o644) before a follow-up chmod tightens it. We only
-        # swallow FileExistsError (the race between exists() and O_EXCL);
-        # any other OSError (EACCES, ENOSPC, …) must propagate so we
-        # don't silently fall back to umask-default perms.
-        if not db_path.exists():
+        # lstat() rather than exists(): if another uid planted a symlink at
+        # db_path pointing outside the cache dir, exists() would follow it
+        # and sqlite3.connect() would end up reading/writing the target.
+        try:
+            st = db_path.lstat()
+        except FileNotFoundError:
+            # No file yet — create with 0o600 up front so there's no
+            # window where sqlite3.connect() uses umask-default perms.
+            # O_NOFOLLOW is belt-and-braces against a symlink planted
+            # between our lstat() and os.open(). Only FileExistsError
+            # is swallowed (the race); other OSErrors must propagate.
+            flags = os.O_CREAT | os.O_WRONLY | os.O_EXCL
+            if hasattr(os, "O_NOFOLLOW"):
+                flags |= os.O_NOFOLLOW
             try:
-                fd = os.open(str(db_path), os.O_CREAT | os.O_WRONLY | os.O_EXCL, 0o600)
+                fd = os.open(str(db_path), flags, 0o600)
                 os.close(fd)
             except FileExistsError:
                 pass
+        else:
+            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}")
         self._conn = sqlite3.connect(str(db_path))
         try:
             db_path.chmod(0o600)
diff --git a/tests/test_cache.py b/tests/test_cache.py
index 9d8aa32..24aed4b 100644
--- a/tests/test_cache.py
+++ b/tests/test_cache.py
@@ -77,3 +77,17 @@ def test_db_file_created_restrictive_even_with_loose_umask(self, tmp_path: Path)
                 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)
+
+        with pytest.raises(OSError, match="non-regular"):
+            DataCache(db_path=db_path)
diff --git a/tests/test_session_manager.py b/tests/test_session_manager.py
index 0aa16b2..4146731 100644
--- a/tests/test_session_manager.py
+++ b/tests/test_session_manager.py
@@ -177,6 +177,25 @@ async def test_restore_refuses_directory_at_session_path(
         # OSError is swallowed so we don't crash the caller.
         assert session_file.is_dir()
 
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks")
+    @patch("src.auth.session_manager.keyring")
+    async def test_restore_removes_dangling_symlink(self, mock_keyring, tmp_session, tmp_path):
+        """A dangling symlink at SESSION_FILE must be removed — not left
+        in place for a subsequent save_session() to follow. Path.exists()
+        returns False for dangling symlinks, so the safety gate has to
+        run unconditionally."""
+        session_file = tmp_path / "session.pickle"
+        session_file.symlink_to(tmp_path / "does-not-exist")
+
+        sm = SessionManager()
+        load_session = MagicMock()
+        cast(Any, sm._mm).load_session = load_session
+
+        assert await sm.try_restore_session() is False
+        load_session.assert_not_called()
+        assert not session_file.is_symlink()
+        assert not session_file.exists()
+
 
 class TestLogin:
     @patch("src.auth.session_manager.keyring")

From aa29a0172029eaed5dde7ff2b65e14130841bb8d Mon Sep 17 00:00:00 2001
From: Rex Lorenzo <rexlorenzo@gmail.com>
Date: Fri, 24 Apr 2026 00:41:31 -0700
Subject: [PATCH 06/12] security: close cache TOCTOU and harden session
 write/logout

Second PR #5 review pass raised three more gaps:

- cache.py had a TOCTOU: after FileExistsError from the pre-create
  os.open, we proceeded to sqlite3.connect without re-validating the
  path. A process racing us could swap the file for a symlink in
  that window. Restructured so there's always an authoritative
  lstat() check immediately before sqlite3.connect.
- session_manager.save_session was unprotected against a planted
  symlink/non-regular entry at SESSION_FILE. Added
  _prepare_session_file_for_write() that lstats and unlinks anything
  non-regular before MonarchMoney writes the pickle. Called from
  both login() and login_with_mfa().
- logout() previously only caught FileNotFoundError, so a directory
  planted at SESSION_FILE would crash logout with IsADirectoryError.
  Broadened to OSError.

Regression tests added for all three.
---
 src/auth/session_manager.py   | 29 +++++++++++++++++++++----
 src/data/cache.py             | 41 +++++++++++++++++------------------
 tests/test_cache.py           | 27 +++++++++++++++++++++++
 tests/test_session_manager.py | 35 ++++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+), 25 deletions(-)

diff --git a/src/auth/session_manager.py b/src/auth/session_manager.py
index 45b0b11..b6b09eb 100644
--- a/src/auth/session_manager.py
+++ b/src/auth/session_manager.py
@@ -56,6 +56,25 @@ def _chmod_session_file() -> None:
         pass
 
 
+def _prepare_session_file_for_write() -> None:
+    """Clear any planted symlink / non-regular file before save_session.
+
+    MonarchMoney's save_session() writes through whatever is at the path,
+    so a symlink there would redirect the pickle (and the follow-up
+    chmod) outside our intended location. If SESSION_FILE is missing or
+    already a regular file there's nothing to do; 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):
+        return
+    SESSION_FILE.unlink()
+
+
 class SessionManager:
     """Handles login, MFA, and session token persistence."""
 
@@ -119,6 +138,7 @@ 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))
         _chmod_session_file()
         self._authenticated = True
@@ -126,6 +146,7 @@ async def login(self, email: str, password: str) -> None:
     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))
         _chmod_session_file()
         self._authenticated = True
@@ -133,10 +154,10 @@ async def login_with_mfa(self, email: str, password: str, mfa_code: str) -> None
     def logout(self) -> None:
         self._authenticated = False
         self.clear_credentials()
-        # Unconditional unlink so a dangling symlink at SESSION_FILE gets
-        # removed too — Path.exists() would report False and leave the
-        # redirect in place.
+        # 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 FileNotFoundError:
+        except OSError:
             pass
diff --git a/src/data/cache.py b/src/data/cache.py
index 7506afe..f7a2950 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -18,28 +18,27 @@ class DataCache:
 
     def __init__(self, db_path: Path = CACHE_DB) -> None:
         db_path.parent.mkdir(parents=True, exist_ok=True, mode=0o700)
-        # lstat() rather than exists(): if another uid planted a symlink at
-        # db_path pointing outside the cache dir, exists() would follow it
-        # and sqlite3.connect() would end up reading/writing the target.
+        # Idempotent pre-create at 0o600 so there's no umask-default window
+        # where sqlite3.connect() creates the file with loose perms.
+        # O_NOFOLLOW rejects symlinks atomically (raises ELOOP as OSError,
+        # which propagates). FileExistsError covers the pre-existing
+        # regular-file case (legitimate prior run, or a race). Other
+        # OSErrors (EACCES, ENOSPC, ELOOP) propagate.
+        flags = os.O_CREAT | os.O_WRONLY | os.O_EXCL
+        if hasattr(os, "O_NOFOLLOW"):
+            flags |= os.O_NOFOLLOW
         try:
-            st = db_path.lstat()
-        except FileNotFoundError:
-            # No file yet — create with 0o600 up front so there's no
-            # window where sqlite3.connect() uses umask-default perms.
-            # O_NOFOLLOW is belt-and-braces against a symlink planted
-            # between our lstat() and os.open(). Only FileExistsError
-            # is swallowed (the race); other OSErrors must propagate.
-            flags = os.O_CREAT | os.O_WRONLY | os.O_EXCL
-            if hasattr(os, "O_NOFOLLOW"):
-                flags |= os.O_NOFOLLOW
-            try:
-                fd = os.open(str(db_path), flags, 0o600)
-                os.close(fd)
-            except FileExistsError:
-                pass
-        else:
-            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}")
+            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 the pre-create and connect. lstat() so a planted
+        # symlink is seen as a symlink, not followed.
+        st = db_path.lstat()
+        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}")
         self._conn = sqlite3.connect(str(db_path))
         try:
             db_path.chmod(0o600)
diff --git a/tests/test_cache.py b/tests/test_cache.py
index 24aed4b..b98bdc3 100644
--- a/tests/test_cache.py
+++ b/tests/test_cache.py
@@ -91,3 +91,30 @@ def test_db_path_refuses_symlink(self, tmp_path: Path):
 
         with pytest.raises(OSError, match="non-regular"):
             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)
diff --git a/tests/test_session_manager.py b/tests/test_session_manager.py
index 4146731..3c2f685 100644
--- a/tests/test_session_manager.py
+++ b/tests/test_session_manager.py
@@ -232,3 +232,38 @@ def test_logout(self, mock_keyring, tmp_session, tmp_path):
 
         assert sm.is_authenticated is False
         assert not session_file.exists()
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks")
+    @patch("src.auth.session_manager.keyring")
+    async def test_login_clears_planted_symlink_before_save(
+        self, mock_keyring, tmp_session, tmp_path
+    ):
+        """A symlink planted at SESSION_FILE (e.g. between logout and next
+        login) must be removed before save_session — otherwise the pickle
+        write would follow the symlink out of the intended directory."""
+        target = tmp_path / "elsewhere.pickle"
+        session_file = tmp_path / "session.pickle"
+        session_file.symlink_to(target)
+
+        sm = SessionManager()
+        mm = cast(Any, sm._mm)
+        mm.login = AsyncMock()
+        mm.save_session = MagicMock()
+
+        await sm.login("user@test.com", "pass")
+
+        assert not session_file.is_symlink()
+        assert not target.exists()
+        mm.save_session.assert_called_once()
+
+    @patch("src.auth.session_manager.keyring")
+    def test_logout_survives_directory_at_session_path(self, mock_keyring, tmp_session, tmp_path):
+        """If a directory is sitting at SESSION_FILE, unlink() raises
+        IsADirectoryError. logout() must survive that — catching OSError,
+        not just FileNotFoundError."""
+        session_file = tmp_path / "session.pickle"
+        session_file.mkdir()
+
+        sm = SessionManager()
+        sm.logout()  # must not raise
+        assert session_file.is_dir()

From 2d84e55967bbf89aee699b6bc09b312b3886705a Mon Sep 17 00:00:00 2001
From: Rex Lorenzo <rexlorenzo@gmail.com>
Date: Fri, 24 Apr 2026 00:53:11 -0700
Subject: [PATCH 07/12] security: require current-uid ownership on existing
 session/cache files

Third PR #5 review pass: the regular-file check is necessary but
not sufficient. If another uid has write access to the cache dir,
they can pre-create a regular file owned by them. save_session
would then write our pickled session into their file and the
follow-up chmod(0o600) would silently fail (not owner); the same
for sqlite3.connect on a pre-planted cache DB.

- _prepare_session_file_for_write() now requires st_uid == getuid()
  and no group/world-writable bits on POSIX before trusting an
  existing regular file. Otherwise unlink + let save_session create
  fresh.
- DataCache.__init__ raises OSError on POSIX if a regular file at
  db_path is owned by a different uid.

Windows accepts any regular file on both paths (no POSIX uid).
Regression tests cover foreign-owned session file, world-writable
session file, and foreign-owned cache DB.
---
 src/auth/session_manager.py   | 16 +++++++---
 src/data/cache.py             | 11 ++++++-
 tests/test_cache.py           | 14 +++++++++
 tests/test_session_manager.py | 59 +++++++++++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 6 deletions(-)

diff --git a/src/auth/session_manager.py b/src/auth/session_manager.py
index b6b09eb..37fb3a9 100644
--- a/src/auth/session_manager.py
+++ b/src/auth/session_manager.py
@@ -57,12 +57,15 @@ def _chmod_session_file() -> None:
 
 
 def _prepare_session_file_for_write() -> None:
-    """Clear any planted symlink / non-regular file before save_session.
+    """Clear any unsafe existing file before save_session.
 
     MonarchMoney's save_session() writes through whatever is at the path,
-    so a symlink there would redirect the pickle (and the follow-up
-    chmod) outside our intended location. If SESSION_FILE is missing or
-    already a regular file there's nothing to do; anything else gets
+    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 not group- or world-writable —
+    otherwise another uid could pre-create the file and collect our
+    next session write. 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.
     """
@@ -71,7 +74,10 @@ def _prepare_session_file_for_write() -> None:
     except FileNotFoundError:
         return
     if stat.S_ISREG(st.st_mode):
-        return
+        if sys.platform == "win32":
+            return
+        if st.st_uid == os.getuid() and not st.st_mode & 0o022:
+            return
     SESSION_FILE.unlink()
 
 
diff --git a/src/data/cache.py b/src/data/cache.py
index f7a2950..d0fc57a 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -4,6 +4,7 @@
 import os
 import sqlite3
 import stat
+import sys
 from datetime import datetime, timedelta
 from pathlib import Path
 from typing import Any
@@ -35,10 +36,18 @@ def __init__(self, db_path: Path = CACHE_DB) -> None:
         # Authoritative validation immediately before sqlite3.connect —
         # closes the TOCTOU window where a symlink could be swapped in
         # between the pre-create and connect. lstat() so a planted
-        # symlink is seen as a symlink, not followed.
+        # symlink is seen as a symlink, not followed. We also require
+        # current-uid ownership on POSIX: if the cache dir is ever
+        # writable by another uid, an attacker could pre-create a
+        # regular DB file they own, and sqlite3.connect() would dump
+        # cached data into their file (chmod(0o600) would then fail
+        # silently, not owning the file). Windows lacks POSIX uid, so
+        # skip that arm there.
         st = db_path.lstat()
         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}")
         self._conn = sqlite3.connect(str(db_path))
         try:
             db_path.chmod(0o600)
diff --git a/tests/test_cache.py b/tests/test_cache.py
index b98bdc3..823df97 100644
--- a/tests/test_cache.py
+++ b/tests/test_cache.py
@@ -118,3 +118,17 @@ def racy_open(*_args, **_kwargs):
             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)
diff --git a/tests/test_session_manager.py b/tests/test_session_manager.py
index 3c2f685..10a62f9 100644
--- a/tests/test_session_manager.py
+++ b/tests/test_session_manager.py
@@ -267,3 +267,62 @@ def test_logout_survives_directory_at_session_path(self, mock_keyring, tmp_sessi
         sm = SessionManager()
         sm.logout()  # must not raise
         assert session_file.is_dir()
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX uid")
+    @patch("src.auth.session_manager.keyring")
+    async def test_login_unlinks_foreign_owned_regular_file(
+        self, mock_keyring, tmp_session, tmp_path, monkeypatch
+    ):
+        """An attacker-owned regular file at SESSION_FILE must be unlinked
+        before save_session, so the next pickle write isn't captured by a
+        file owned by another uid (who could pre-create it in a
+        loosely-permissioned parent dir)."""
+        import os as _os
+
+        session_file = tmp_path / "session.pickle"
+        session_file.write_bytes(b"attacker content")
+        session_file.chmod(0o600)
+        real_uid = _os.getuid()
+        monkeypatch.setattr("src.auth.session_manager.os.getuid", lambda: real_uid + 1)
+
+        sm = SessionManager()
+        mm = cast(Any, sm._mm)
+        mm.login = AsyncMock()
+
+        def fake_save(path: str) -> None:
+            with open(path, "wb") as f:
+                f.write(b"our session")
+
+        mm.save_session = MagicMock(side_effect=fake_save)
+
+        await sm.login("user@test.com", "pass")
+
+        assert session_file.read_bytes() == b"our session"
+        mm.save_session.assert_called_once()
+
+    @pytest.mark.skipif(sys.platform == "win32", reason="POSIX mode bits")
+    @patch("src.auth.session_manager.keyring")
+    async def test_login_unlinks_world_writable_regular_file(
+        self, mock_keyring, tmp_session, tmp_path
+    ):
+        """Same class: a world-writable regular file is suspect even if
+        we own it (any other process could write into it) — must be
+        unlinked before save, so save_session produces a fresh 0o600
+        file."""
+        session_file = tmp_path / "session.pickle"
+        session_file.write_bytes(b"suspect")
+        session_file.chmod(0o666)
+
+        sm = SessionManager()
+        mm = cast(Any, sm._mm)
+        mm.login = AsyncMock()
+
+        def fake_save(path: str) -> None:
+            with open(path, "wb") as f:
+                f.write(b"fresh")
+
+        mm.save_session = MagicMock(side_effect=fake_save)
+
+        await sm.login("user@test.com", "pass")
+
+        assert session_file.read_bytes() == b"fresh"

From 753aec04e68bb614ffb2d0c924b1ef1addeef660 Mon Sep 17 00:00:00 2001
From: Rex Lorenzo <rexlorenzo@gmail.com>
Date: Fri, 24 Apr 2026 01:03:28 -0700
Subject: [PATCH 08/12] security: close pre-chmod read windows on session and
 cache files

Fourth PR #5 review pass:

- _prepare_session_file_for_write tightened: any existing regular
  file must also have no group/other bits set (mask 0o077, not just
  the 0o022 writable bits). A legacy 0o644 pickle left behind under
  a loose umask would otherwise leak the next save_session write
  to world-readers until _chmod_session_file() tightens it.
- cache.py chmod moved to *before* sqlite3.connect so a pre-existing
  DB with loose perms (or a connect() that errors before the post-
  chmod) can't be read/written with world-readable mode. For the
  just-pre-created case (os.open already set 0o600) the extra chmod
  is a no-op.
---
 src/auth/session_manager.py | 8 +++++++-
 src/data/cache.py           | 9 +++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/auth/session_manager.py b/src/auth/session_manager.py
index 37fb3a9..76238c1 100644
--- a/src/auth/session_manager.py
+++ b/src/auth/session_manager.py
@@ -76,7 +76,13 @@ def _prepare_session_file_for_write() -> None:
     if stat.S_ISREG(st.st_mode):
         if sys.platform == "win32":
             return
-        if st.st_uid == os.getuid() and not st.st_mode & 0o022:
+        # 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()
 
diff --git a/src/data/cache.py b/src/data/cache.py
index d0fc57a..0aec829 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -48,11 +48,16 @@ def __init__(self, db_path: Path = CACHE_DB) -> None:
             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}")
-        self._conn = sqlite3.connect(str(db_path))
+        # Tighten perms BEFORE sqlite3.connect — a pre-existing 0o644 DB
+        # would otherwise be read/written with loose perms in the window
+        # before a post-connect chmod. For a just-pre-created file this
+        # is a no-op (os.open already set 0o600). chmod() semantics vary
+        # on Windows, hence the swallow.
         try:
             db_path.chmod(0o600)
         except OSError:
-            pass  # chmod semantics differ across platforms (e.g. Windows)
+            pass
+        self._conn = sqlite3.connect(str(db_path))
         self._conn.execute(
             """CREATE TABLE IF NOT EXISTS cache (
                 key TEXT PRIMARY KEY,

From 3880fc29ae8cd804deabd4461dd4a5350412e21b Mon Sep 17 00:00:00 2001
From: Rex Lorenzo <rexlorenzo@gmail.com>
Date: Fri, 24 Apr 2026 01:14:34 -0700
Subject: [PATCH 09/12] security: align docstring; skip cache pre-create
 without O_NOFOLLOW
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fifth PR #5 review pass — both changes narrow:

- _prepare_session_file_for_write docstring drifted when the mask
  tightened from 0o022 to 0o077 last commit. Updated to match: "no
  group or world permission bits set at all", with the why-of-the
  read-window spelled out.
- cache.py: only run the O_CREAT|O_EXCL pre-create when O_NOFOLLOW
  is available. Without it, O_CREAT could in principle follow a
  planted symlink before the lstat gate catches it. In practice
  this is Windows-only (O_NOFOLLOW is a POSIX feature), where
  creating symlinks requires admin and POSIX mode bits don't map
  to NTFS ACLs, so skipping the pre-create is both safer and
  cheaper. The post-check now tolerates FileNotFoundError so
  sqlite3.connect can create the file itself on that path.
---
 src/auth/session_manager.py | 15 ++++++----
 src/data/cache.py           | 55 +++++++++++++++++++------------------
 2 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/src/auth/session_manager.py b/src/auth/session_manager.py
index 76238c1..95f785c 100644
--- a/src/auth/session_manager.py
+++ b/src/auth/session_manager.py
@@ -62,12 +62,15 @@ def _prepare_session_file_for_write() -> None:
     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 not group- or world-writable —
-    otherwise another uid could pre-create the file and collect our
-    next session write. 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.
+    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()
diff --git a/src/data/cache.py b/src/data/cache.py
index 0aec829..1dd75a4 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -19,35 +19,38 @@ 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.
-        # O_NOFOLLOW rejects symlinks atomically (raises ELOOP as OSError,
-        # which propagates). FileExistsError covers the pre-existing
-        # regular-file case (legitimate prior run, or a race). Other
-        # OSErrors (EACCES, ENOSPC, ELOOP) propagate.
-        flags = os.O_CREAT | os.O_WRONLY | os.O_EXCL
+        # 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. Platforms without O_NOFOLLOW are effectively Windows, where
+        # creating symlinks requires admin and the umask-default window
+        # doesn't carry the same meaning (NTFS ACLs, not POSIX mode).
+        # 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_NOFOLLOW
-        try:
-            fd = os.open(str(db_path), flags, 0o600)
-            os.close(fd)
-        except FileExistsError:
-            pass
+            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 the pre-create and connect. lstat() so a planted
-        # symlink is seen as a symlink, not followed. We also require
-        # current-uid ownership on POSIX: if the cache dir is ever
-        # writable by another uid, an attacker could pre-create a
-        # regular DB file they own, and sqlite3.connect() would dump
-        # cached data into their file (chmod(0o600) would then fail
-        # silently, not owning the file). Windows lacks POSIX uid, so
-        # skip that arm there.
-        st = db_path.lstat()
-        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}")
+        # 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 a post-connect chmod. For a just-pre-created file this

From e86070b1969c5bf923951498d14f5dfe73626666 Mon Sep 17 00:00:00 2001
From: Rex Lorenzo <rexlorenzo@gmail.com>
Date: Fri, 24 Apr 2026 08:40:37 -0700
Subject: [PATCH 10/12] test: relax cache symlink-rejection assertion to
 OSError

O_EXCL and O_NOFOLLOW interact differently across POSIX kernels:
macOS raises EEXIST first (caught as FileExistsError, then the
lstat gate raises \"non-regular\"), Linux may raise ELOOP directly
from os.open. Both are correct rejections, so the test shouldn't
depend on which specific errno wins. Assert OSError without
message matching.
---
 tests/test_cache.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/test_cache.py b/tests/test_cache.py
index 823df97..b390e64 100644
--- a/tests/test_cache.py
+++ b/tests/test_cache.py
@@ -89,7 +89,11 @@ def test_db_path_refuses_symlink(self, tmp_path: Path):
         db_path = tmp_path / "cache.db"
         db_path.symlink_to(target)
 
-        with pytest.raises(OSError, match="non-regular"):
+        # 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")

From b83b6262d5a9a0bc8860e7c79bb8cc2d1dc7cee6 Mon Sep 17 00:00:00 2001
From: Rex Lorenzo <rexlorenzo@gmail.com>
Date: Fri, 24 Apr 2026 08:50:35 -0700
Subject: [PATCH 11/12] docs: reword cache.py O_NOFOLLOW-absent branch comment
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Avoid implying non-admin Windows users can't create symlinks —
Windows 10+ Developer Mode enables it. The actual reason we skip
pre-create on those platforms is the missing O_NOFOLLOW primitive
and the different permission model (NTFS ACLs), not symlink
availability.
---
 src/data/cache.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/data/cache.py b/src/data/cache.py
index 1dd75a4..9eceff3 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -21,12 +21,13 @@ 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. Platforms without O_NOFOLLOW are effectively Windows, where
-        # creating symlinks requires admin and the umask-default window
-        # doesn't carry the same meaning (NTFS ACLs, not POSIX mode).
+        # 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.

From 4cf7afe9fd71d10267ff524a797b8eea11f4a8ef Mon Sep 17 00:00:00 2001
From: Rex Lorenzo <rexlorenzo@gmail.com>
Date: Fri, 24 Apr 2026 09:47:35 -0700
Subject: [PATCH 12/12] security: chmod cache DB after sqlite3.connect too

On platforms without O_NOFOLLOW (effectively Windows) we skip the
pre-create, so if the DB file doesn't exist yet the chmod-before-
connect is a no-op (FileNotFoundError, swallowed) and sqlite3
creates the file with umask-default perms. A second chmod after
connect catches that first-run case on those platforms; it's a
no-op when the pre-connect chmod already tightened a pre-existing
file.
---
 src/data/cache.py | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/src/data/cache.py b/src/data/cache.py
index 9eceff3..9575442 100644
--- a/src/data/cache.py
+++ b/src/data/cache.py
@@ -54,14 +54,25 @@ def __init__(self, db_path: Path = CACHE_DB) -> None:
                 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 a post-connect chmod. For a just-pre-created file this
-        # is a no-op (os.open already set 0o600). chmod() semantics vary
-        # on Windows, hence the swallow.
+        # 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,