diff --git a/.changes/unreleased/fixed-20260607-134043.yaml b/.changes/unreleased/fixed-20260607-134043.yaml new file mode 100644 index 00000000..e3d83b44 --- /dev/null +++ b/.changes/unreleased/fixed-20260607-134043.yaml @@ -0,0 +1,6 @@ +kind: fixed +body: Restrict file and directory permissions on auth, config, context, and log paths to prevent local credential exposure on multi-user systems +time: 2026-06-07T13:40:43+02:00 +custom: + Author: iemejia + AuthorLink: https://github.com/iemejia diff --git a/src/fabric_cli/core/fab_auth.py b/src/fabric_cli/core/fab_auth.py index a12b890c..09516ac1 100644 --- a/src/fabric_cli/core/fab_auth.py +++ b/src/fabric_cli/core/fab_auth.py @@ -56,8 +56,9 @@ def __init__(self): self._load_env() def _save_auth(self): - with open(self.auth_file, "w") as file: - file.write(json.dumps(self._get_auth_info())) + from fabric_cli.core.fab_state_config import _write_restricted_file + + _write_restricted_file(self.auth_file, json.dumps(self._get_auth_info())) def _load_auth(self): if os.path.exists(self.auth_file) and os.stat(self.auth_file).st_size != 0: diff --git a/src/fabric_cli/core/fab_context.py b/src/fabric_cli/core/fab_context.py index 10c9da2e..27ec58d5 100644 --- a/src/fabric_cli/core/fab_context.py +++ b/src/fabric_cli/core/fab_context.py @@ -159,8 +159,11 @@ def _save_context_to_file(self) -> None: } try: - with open(self._context_file, "w") as f: - json.dump(context_data, f) + from fabric_cli.core.fab_state_config import _write_restricted_file + + _write_restricted_file( + self._context_file, json.dumps(context_data) + ) except Exception: utils_ui.print_warning( "Warning: Failed to save context file. Context persistence may not work as expected." diff --git a/src/fabric_cli/core/fab_logger.py b/src/fabric_cli/core/fab_logger.py index 9af40c27..fccf3e52 100644 --- a/src/fabric_cli/core/fab_logger.py +++ b/src/fabric_cli/core/fab_logger.py @@ -156,10 +156,21 @@ def log_debug_http_request_exception(e): def _get_log_file_path(): """Create a log file path in the user's log directory.""" log_dir = user_log_dir("fabric-cli") - os.makedirs(log_dir, exist_ok=True) + os.makedirs(log_dir, mode=0o700, exist_ok=True) + # Enforce permissions on pre-existing directories from older versions + _chmod_if_posix(log_dir, 0o700) return os.path.join(log_dir, "fabcli_debug.log") +def _chmod_if_posix(path, mode): + """Best-effort chmod with warning on failure; no-op on Windows.""" + if os.name != "nt": + try: + os.chmod(path, mode) + except OSError: + pass + + def get_logger(): """Singleton logger instance with a single file handler.""" global _logger_instance, log_file_path diff --git a/src/fabric_cli/core/fab_state_config.py b/src/fabric_cli/core/fab_state_config.py index 2e441390..817e8ac3 100644 --- a/src/fabric_cli/core/fab_state_config.py +++ b/src/fabric_cli/core/fab_state_config.py @@ -2,16 +2,31 @@ # Licensed under the MIT License. import json +import logging import os -from os.path import exists, expanduser +from os.path import expanduser from fabric_cli.core import fab_constant +_logger = logging.getLogger(__name__) + + +def _chmod_if_posix(path, mode): + """Best-effort chmod with warning on failure; no-op on Windows.""" + if os.name != "nt": + try: + os.chmod(path, mode) + except OSError as e: + _logger.warning( + "Failed to set permissions %o on %s: %s", mode, path, e + ) + def config_location(): _location = expanduser("~/.config/fab/") - if not exists(_location): - os.makedirs(_location) + os.makedirs(_location, mode=0o700, exist_ok=True) + # Enforce permissions on pre-existing directories from older versions + _chmod_if_posix(_location, 0o700) return _location @@ -29,8 +44,29 @@ def read_config(file_path) -> dict: def write_config(data): - with open(config_file, "w") as file: - json.dump(data, file, indent=4) + _write_restricted_file(config_file, json.dumps(data, indent=4)) + + +def _write_restricted_file(file_path, content): + """Write content to a file with owner-only permissions (0o600). + + Handles both new file creation and tightening permissions on + pre-existing files from older CLI versions. + """ + fd = os.open(file_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + try: + with os.fdopen(fd, "w") as file: + file.write(content) + except Exception: + # os.fdopen may fail before wrapping fd; close to avoid leak. + # If os.fdopen succeeded, fd is already closed by the with block. + try: + os.close(fd) + except OSError: + pass + raise + # Enforce permissions on pre-existing files from older versions + _chmod_if_posix(file_path, 0o600) def set_config(key, value): diff --git a/tests/test_core/test_context_persistence.py b/tests/test_core/test_context_persistence.py index 0c675239..862503f4 100644 --- a/tests/test_core/test_context_persistence.py +++ b/tests/test_core/test_context_persistence.py @@ -46,16 +46,19 @@ def mock_get_config(key): # Create a mock workspace with the tenant as parent workspace = Workspace("test_workspace", "5678", tenant, "Workspace") - # Mock json.dump to avoid actually writing to the file system - with patch("json.dump") as mock_json_dump, patch("builtins.open", MagicMock()): + # Mock _write_restricted_file to avoid actually writing to the file system + with patch( + "fabric_cli.core.fab_state_config._write_restricted_file" + ) as mock_write: # Set the context context.context = workspace - # Check that json.dump was called with the right data - mock_json_dump.assert_called_once() - args, _ = mock_json_dump.call_args - assert args[0] == {"path": workspace.path} + # Check that _write_restricted_file was called with the right data + mock_write.assert_called_once() + args, _ = mock_write.call_args + assert args[0] == temp_context_file + assert json.loads(args[1]) == {"path": workspace.path} finally: os.remove(temp_context_file) @@ -185,16 +188,18 @@ def mock_get_config(key): tenant = Tenant("test_tenant", "1234") workspace = Workspace("test_workspace", "5678", tenant, "Workspace") - # Mock json.dump to avoid actually writing to the file system - with patch("json.dump") as mock_json_dump, patch("builtins.open", MagicMock()): + # Mock _write_restricted_file to avoid actually writing to the file system + with patch( + "fabric_cli.core.fab_state_config._write_restricted_file" + ) as mock_write: # Set the context - this SHOULD trigger file save when persistence is enabled context.context = workspace - # Check that json.dump was called with the right data - mock_json_dump.assert_called_once() - args, _ = mock_json_dump.call_args - assert args[0] == {"path": workspace.path} + # Check that _write_restricted_file was called with the right data + mock_write.assert_called_once() + args, _ = mock_write.call_args + assert json.loads(args[1]) == {"path": workspace.path} finally: os.remove(temp_context_file) diff --git a/tests/test_core/test_fab_auth.py b/tests/test_core/test_fab_auth.py index 5cf2e568..55f2b826 100644 --- a/tests/test_core/test_fab_auth.py +++ b/tests/test_core/test_fab_auth.py @@ -1064,3 +1064,54 @@ def test_auth_mode_migration(tmp_path): assert ( auth.get_identity_type() == "user" ), "get_identity_type returns wrong value after migration" + + +# region security: auth file permission tests + +_skip_on_windows = pytest.mark.skipif( + os.name == "nt", reason="POSIX permission tests not applicable on Windows" +) + + +@_skip_on_windows +def test_save_auth_creates_file_with_restricted_permissions(tmp_path): + """Verify auth.json is created with mode 0o600 (owner read/write only).""" + auth = FabAuth() + auth.auth_file = os.path.join(str(tmp_path), "auth.json") + auth._auth_info = {con.IDENTITY_TYPE: "user"} + + auth._save_auth() + + assert os.path.exists(auth.auth_file) + mode = oct(os.stat(auth.auth_file).st_mode & 0o777) + assert mode == "0o600", f"auth.json has mode {mode}, expected 0o600" + + # Verify content is still correct + with open(auth.auth_file, "r") as f: + data = json.load(f) + assert data[con.IDENTITY_TYPE] == "user" + + +@_skip_on_windows +def test_save_auth_tightens_permissions_on_existing_file(tmp_path): + """Verify _save_auth() enforces 0o600 on a pre-existing permissive file.""" + auth = FabAuth() + auth.auth_file = os.path.join(str(tmp_path), "auth.json") + + # Create file with overly permissive mode (simulating old CLI version) + with open(auth.auth_file, "w") as f: + json.dump({con.IDENTITY_TYPE: "user"}, f) + os.chmod(auth.auth_file, 0o644) + + auth._auth_info = {con.IDENTITY_TYPE: "spn"} + auth._save_auth() + + mode = oct(os.stat(auth.auth_file).st_mode & 0o777) + assert mode == "0o600", f"auth.json has mode {mode} after overwrite, expected 0o600" + + with open(auth.auth_file, "r") as f: + data = json.load(f) + assert data[con.IDENTITY_TYPE] == "spn" + + +# endregion diff --git a/tests/test_core/test_fab_logger.py b/tests/test_core/test_fab_logger.py index 7ad67a36..3f96a725 100644 --- a/tests/test_core/test_fab_logger.py +++ b/tests/test_core/test_fab_logger.py @@ -249,3 +249,24 @@ def mock_log_warning(): def mock_get_log_file_path(): with patch("fabric_cli.core.fab_logger.get_log_file_path") as mock: yield mock + + +# ── Security: log directory permissions ────────────────────────────────────── + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission tests not applicable on Windows") +def test_get_log_file_path_creates_directory_with_restricted_permissions( + monkeypatch, tmp_path +): + """Verify log directory is created with mode 0o700 (owner-only).""" + log_dir = tmp_path / "fabric-cli" / "log" + monkeypatch.setattr( + logger, "user_log_dir", lambda app_name: str(log_dir) + ) + + result = logger._get_log_file_path() + assert result.endswith("fabcli_debug.log") + assert log_dir.exists() + + mode = oct(log_dir.stat().st_mode & 0o777) + assert mode == "0o700", f"Log directory has mode {mode}, expected 0o700" diff --git a/tests/test_core/test_fab_state_config.py b/tests/test_core/test_fab_state_config.py index df0b171c..e8e1dbd1 100644 --- a/tests/test_core/test_fab_state_config.py +++ b/tests/test_core/test_fab_state_config.py @@ -5,6 +5,8 @@ import os import tempfile +import pytest + import fabric_cli.core.fab_state_config as cfg from fabric_cli.core import fab_constant @@ -165,3 +167,85 @@ def test_init_defaults_preserves_user_overrides_success(monkeypatch, tmp_path): assert result[fab_constant.FAB_CACHE_ENABLED] == "false" # endregion + + +# region security: file permission tests + +_skip_on_windows = pytest.mark.skipif( + os.name == "nt", reason="POSIX permission tests not applicable on Windows" +) + + +@_skip_on_windows +def test_config_location_creates_directory_with_restricted_permissions( + monkeypatch, tmp_path +): + """Verify config directory is created with mode 0o700 (owner-only access).""" + monkeypatch.setattr( + "fabric_cli.core.fab_state_config.expanduser", + lambda path: path.replace("~", str(tmp_path)), + ) + + location = cfg.config_location() + assert os.path.isdir(location) + + mode = oct(os.stat(location).st_mode & 0o777) + assert mode == "0o700", f"Config directory has mode {mode}, expected 0o700" + + +@_skip_on_windows +def test_config_location_tightens_permissions_on_existing_directory( + monkeypatch, tmp_path +): + """Verify config_location() enforces 0o700 on a pre-existing permissive directory.""" + config_dir = tmp_path / ".config" / "fab" + config_dir.mkdir(parents=True, mode=0o755) + monkeypatch.setattr( + "fabric_cli.core.fab_state_config.expanduser", + lambda path: path.replace("~", str(tmp_path)), + ) + + cfg.config_location() + + mode = oct(config_dir.stat().st_mode & 0o777) + assert mode == "0o700", f"Config directory has mode {mode}, expected 0o700" + + +@_skip_on_windows +def test_write_config_creates_file_with_restricted_permissions(monkeypatch, tmp_path): + """Verify config files are created with mode 0o600 (owner read/write only).""" + config_file = os.path.join(str(tmp_path), "config.json") + monkeypatch.setattr(cfg, "config_file", config_file) + + cfg.write_config({"key": "value"}) + + assert os.path.exists(config_file) + mode = oct(os.stat(config_file).st_mode & 0o777) + assert mode == "0o600", f"Config file has mode {mode}, expected 0o600" + + # Verify content is still correct + data = cfg.read_config(config_file) + assert data == {"key": "value"} + + +@_skip_on_windows +def test_write_config_tightens_permissions_on_existing_file(monkeypatch, tmp_path): + """Verify write_config() enforces 0o600 on a pre-existing permissive file.""" + config_file = os.path.join(str(tmp_path), "config.json") + monkeypatch.setattr(cfg, "config_file", config_file) + + # Create file with overly permissive mode (simulating old CLI version) + with open(config_file, "w") as f: + json.dump({"old": "data"}, f) + os.chmod(config_file, 0o644) + + cfg.write_config({"new": "data"}) + + mode = oct(os.stat(config_file).st_mode & 0o777) + assert mode == "0o600", f"Config file has mode {mode} after overwrite, expected 0o600" + + data = cfg.read_config(config_file) + assert data == {"new": "data"} + + +# endregion