From b62adb302eb38da1554faca56d0c054aedcdb79a Mon Sep 17 00:00:00 2001 From: Ali Karbassi Date: Tue, 30 Jun 2026 22:41:45 -0500 Subject: [PATCH] Create token fallback file locked to 0600 instead of chmod-after-write The file-based session fallback wrote the long-lived Monarch token via Path.write_text() (created under the process umask, typically 0644) and only narrowed it to 0600 afterward. That leaves a window where a full-access financial credential is world-readable by other local users. Create the file already locked via os.open(..., O_CREAT, 0o600), keeping the trailing chmod to enforce 0600 on a pre-existing file. Adds a regression test that fails if creation reverts to write_text(). Fixes #75 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/monarch_mcp_server/secure_session.py | 16 +++++++++-- tests/test_secure_session.py | 35 ++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/monarch_mcp_server/secure_session.py b/src/monarch_mcp_server/secure_session.py index 4677e12..128e0b6 100644 --- a/src/monarch_mcp_server/secure_session.py +++ b/src/monarch_mcp_server/secure_session.py @@ -72,10 +72,20 @@ def __init__(self) -> None: def _save_token_file(self, token: str) -> None: _TOKEN_DIR.mkdir(parents=True, exist_ok=True) - # Write with owner-only permissions - _TOKEN_FILE.write_text(token) - _TOKEN_FILE.chmod(stat.S_IRUSR | stat.S_IWUSR) # 600 _TOKEN_DIR.chmod(stat.S_IRWXU) # 700 + # Create the file already locked to owner-only (0600) instead of + # write_text()-then-chmod, which leaves a window where the token is + # world-readable under a default umask. + fd = os.open( + _TOKEN_FILE, + os.O_WRONLY | os.O_CREAT | os.O_TRUNC, + stat.S_IRUSR | stat.S_IWUSR, + ) + with os.fdopen(fd, "w") as f: + f.write(token) + # O_CREAT honors the mode only when creating; enforce it for an + # existing file too. + _TOKEN_FILE.chmod(stat.S_IRUSR | stat.S_IWUSR) # 600 logger.info(f"✅ Token saved to {_TOKEN_FILE}") def _load_token_file(self) -> Optional[str]: diff --git a/tests/test_secure_session.py b/tests/test_secure_session.py index 5c8fce9..24003f8 100644 --- a/tests/test_secure_session.py +++ b/tests/test_secure_session.py @@ -334,3 +334,38 @@ def fake_create(**kwargs): def test_no_session_returns_none(self, storage_keyring): session, _ = storage_keyring assert session.get_authenticated_client() is None + + +class TestFileFallbackPermissions: + """The plaintext file fallback must never expose the token to other users.""" + + def test_token_file_created_locked_not_via_write_text(self, tmp_path, monkeypatch): + """The token file must be created already locked to 0600, not written + world-readable and chmod'd afterward (which leaves a race window).""" + import stat as _stat + + monkeypatch.setattr(ss_module, "_TOKEN_DIR", tmp_path / "store") + monkeypatch.setattr(ss_module, "_TOKEN_FILE", tmp_path / "store" / "token") + + # A revert to write_text()-then-chmod would trip this and fail loudly. + def _boom(*_a, **_k): + raise AssertionError("token file must not be created via write_text()") + + monkeypatch.setattr(ss_module.Path, "write_text", _boom) + + create_modes = [] + real_open = ss_module.os.open + + def _recording_open(path, flags, mode=0o777): + create_modes.append(mode) + return real_open(path, flags, mode) + + monkeypatch.setattr(ss_module.os, "open", _recording_open) + + session = ss_module.SecureMonarchSession() + session._save_token_file("super-secret-token") + + token_file = tmp_path / "store" / "token" + assert token_file.read_text() == "super-secret-token" + assert create_modes and all(m == 0o600 for m in create_modes) + assert _stat.S_IMODE(token_file.stat().st_mode) == 0o600