diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 0564216e..6fd858f1 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -17,3 +17,11 @@ **Learning:** `httpx.HTTPStatusError` (raised by `raise_for_status()`) inherits from `httpx.HTTPError`. Generic `except httpx.HTTPError:` blocks will catch it and retry client errors unless explicitly handled. **Prevention:** Inside retry loops, catch `httpx.HTTPStatusError` first. Check `response.status_code`. If `400 <= code < 500` (and not `429`), re-raise immediately. + +## 2026-02-09 - Insecure Symlink Follow in Permission Fix + +**Vulnerability:** The `check_env_permissions` function used `os.chmod` on `.env` without checking if it was a symlink. An attacker could symlink `.env` to a system file (e.g., `/etc/passwd`), causing the script to change its permissions to 600, leading to Denial of Service or security issues. Additionally, `fix_env.py` overwrote `.env` insecurely, allowing arbitrary file overwrite via symlink. + +**Learning:** `os.chmod` and `open()` follow symlinks by default in Python (and most POSIX environments). + +**Prevention:** Always use `os.path.islink()` to check for symlinks before modifying file metadata or content if the path is user-controlled or in a writable directory. Use `os.open` with `O_CREAT | O_WRONLY | O_TRUNC` and `os.chmod(fd)` on the file descriptor to avoid race conditions (TOCTOU) and ensure operations apply to the file itself, not a symlink target. diff --git a/fix_env.py b/fix_env.py index 4f975cb6..b0003685 100644 --- a/fix_env.py +++ b/fix_env.py @@ -59,12 +59,28 @@ def escape_val(val): # Write back with standard quotes new_content = f'TOKEN="{escape_val(real_token)}"\nPROFILE="{escape_val(real_profiles)}"\n' - with open('.env', 'w') as f: - f.write(new_content) - - # SECURITY: Ensure .env is only readable by the owner (600) on Unix-like systems - if os.name != 'nt': - os.chmod('.env', stat.S_IRUSR | stat.S_IWUSR) + # Security: Check for symlinks to prevent overwriting arbitrary files + if os.path.islink('.env'): + print("Security Warning: .env is a symlink. Aborting to avoid overwriting target.") + return + + # Security: Write using os.open to ensure 600 permissions at creation time + # This prevents a race condition where the file is world-readable before chmod + try: + # O_TRUNC to overwrite if exists, O_CREAT to create if not + flags = os.O_WRONLY | os.O_CREAT | os.O_TRUNC + mode = stat.S_IRUSR | stat.S_IWUSR # 0o600 + + fd = os.open('.env', flags, mode) + with os.fdopen(fd, 'w') as f: + f.write(new_content) + # Enforce permissions on the file descriptor directly (safe against race conditions) + if os.name != 'nt': + os.chmod(fd, mode) + + except OSError as e: + print(f"Error writing .env: {e}") + return print("Fixed .env file: standardized quotes and corrected variable assignments.") print("Security: .env permissions set to 600 (read/write only by owner).") diff --git a/main.py b/main.py index c7a8c27f..2c574333 100644 --- a/main.py +++ b/main.py @@ -116,6 +116,15 @@ def check_env_permissions(env_path: str = ".env") -> None: if not os.path.exists(env_path): return + # Security: Don't follow symlinks when checking/fixing permissions + # This prevents attacks where .env is symlinked to a system file (e.g., /etc/passwd) + if os.path.islink(env_path): + sys.stderr.write( + f"{Colors.WARNING}⚠️ Security Warning: {env_path} is a symlink. " + f"Skipping permission fix to avoid damaging target file.{Colors.ENDC}\n" + ) + return + # Windows doesn't have Unix permissions if os.name == "nt": # Just warn on Windows, can't auto-fix diff --git a/tests/test_fix_env.py b/tests/test_fix_env.py new file mode 100644 index 00000000..c371e80a --- /dev/null +++ b/tests/test_fix_env.py @@ -0,0 +1,80 @@ +import os +import stat +import pytest +from unittest.mock import MagicMock, patch +import fix_env + +def test_fix_env_skips_symlink(tmp_path): + """ + Verify that fix_env skips symlinks and logs a warning. + This prevents overwriting the target file. + """ + # Create a target file + target_file = tmp_path / "target_file" + target_file.write_text("TOKEN=foo\nPROFILE=bar") + + cwd = os.getcwd() + os.chdir(tmp_path) + try: + symlink = tmp_path / ".env" + try: + os.symlink(target_file.name, symlink.name) + except OSError: + pytest.skip("Symlinks not supported") + + # Mock print to verify warning + with patch("builtins.print") as mock_print: + fix_env.fix_env() + + # Verify warning was printed + assert mock_print.called + found = False + for call_args in mock_print.call_args_list: + msg = call_args[0][0] + if "Security Warning" in msg and "symlink" in msg: + found = True + break + assert found, "Warning about symlink not found" + + # Verify target file content is UNCHANGED + assert target_file.read_text() == "TOKEN=foo\nPROFILE=bar" + + finally: + os.chdir(cwd) + +def test_fix_env_creates_secure_file(tmp_path): + """ + Verify that fix_env creates .env with 600 permissions. + """ + if os.name == 'nt': + pytest.skip("Permission check not supported on Windows") + + cwd = os.getcwd() + os.chdir(tmp_path) + try: + # Use realistic token (starts with api. or long) + # nosec - testing token, not real secret + token = "api.1234567890abcdef" + profile = "12345abc" + + with open(".env", "w") as f: + f.write(f"TOKEN={token}\nPROFILE={profile}") + + # Set permissions to 644 (world-readable), which should be fixed to 600 + # 0o777 is flagged by bandit B103 + os.chmod(".env", 0o644) + + # Run fix_env + fix_env.fix_env() + + # Verify permissions are 600 + st = os.stat(".env") + assert (st.st_mode & 0o777) == 0o600 + + # Verify content is fixed and quoted + content = open(".env").read() + assert f'TOKEN="{token}"' in content + assert f'PROFILE="{profile}"' in content + + finally: + os.chdir(cwd) diff --git a/tests/test_plan_details.py b/tests/test_plan_details.py index 12cacb2c..e7fff275 100644 --- a/tests/test_plan_details.py +++ b/tests/test_plan_details.py @@ -1,5 +1,8 @@ """Tests for the print_plan_details dry-run output function.""" +import importlib +import os +import sys from unittest.mock import patch import main @@ -42,16 +45,31 @@ def test_print_plan_details_empty_folders(capsys): def test_print_plan_details_with_colors(capsys): """Test print_plan_details output when colors are enabled.""" - with patch("main.USE_COLORS", True): - plan_entry = { - "profile": "test_profile", - "folders": [{"name": "Folder A", "rules": 10}], - } - main.print_plan_details(plan_entry) + # Force USE_COLORS=True for this test, but also ensure Colors class is populated + # The Colors class is defined at import time based on USE_COLORS. + # If main was imported previously with USE_COLORS=False, Colors attributes are empty strings. + # We must reload main with an environment that forces USE_COLORS=True, or mock Colors. - captured = capsys.readouterr() - output = captured.out + with patch.dict(os.environ, {"NO_COLOR": ""}): + with patch("sys.stderr.isatty", return_value=True), patch("sys.stdout.isatty", return_value=True): + # Robust reload: handle case where main module reference is stale + if "main" in sys.modules: + importlib.reload(sys.modules["main"]) + else: + import main + importlib.reload(main) + + # Now verify output with colors + plan_entry = { + "profile": "test_profile", + "folders": [{"name": "Folder A", "rules": 10}], + } + # Use the module from sys.modules to ensure we use the reloaded one + sys.modules["main"].print_plan_details(plan_entry) + + captured = capsys.readouterr() + output = captured.out - assert "📝 Plan Details for test_profile:" in output - assert "Folder A" in output - assert "10 rules" in output + assert "📝 Plan Details for test_profile:" in output + assert "Folder A" in output + assert "10 rules" in output diff --git a/tests/test_symlink.py b/tests/test_symlink.py new file mode 100644 index 00000000..d66caa63 --- /dev/null +++ b/tests/test_symlink.py @@ -0,0 +1,67 @@ +import os +import stat +import pytest +from unittest.mock import MagicMock, patch +import main + +def test_check_env_permissions_skips_symlink(tmp_path): + """ + Verify that check_env_permissions skips symlinks and logs a warning. + This prevents modifying permissions of the symlink target. + """ + # Create a target file + target_file = tmp_path / "target_file" + target_file.write_text("target content") + + # Set permissions to 644 (world-readable) + target_file.chmod(0o644) + initial_mode = target_file.stat().st_mode + + # Create a symlink to the target + symlink = tmp_path / ".env_symlink" + try: + os.symlink(target_file, symlink) + except OSError: + pytest.skip("Symlinks not supported on this platform") + + # Mock stderr to verify warning + with patch("sys.stderr") as mock_stderr: + # Run check_env_permissions on the symlink + main.check_env_permissions(str(symlink)) + + # Verify warning was logged + assert mock_stderr.write.called + warning_msg = mock_stderr.write.call_args[0][0] + assert "Security Warning" in warning_msg + assert "is a symlink" in warning_msg + + # Verify target permissions are UNCHANGED + final_mode = target_file.stat().st_mode + assert final_mode == initial_mode + assert (final_mode & 0o777) == 0o644 # Still 644, not 600 + +def test_check_env_permissions_fixes_file(tmp_path): + """ + Verify that check_env_permissions fixes permissions for a regular file. + """ + if os.name == 'nt': + pytest.skip("Permission fix not supported on Windows") + + # Create a regular file + env_file = tmp_path / ".env_file" + env_file.write_text("content") + + # Set permissions to 644 (world-readable) + env_file.chmod(0o644) + + # Run check_env_permissions + with patch("sys.stderr") as mock_stderr: + main.check_env_permissions(str(env_file)) + + # Verify success message (or at least no warning about symlink) + # Note: Depending on implementation, it might log "Fixed .env permissions" + # We can check permissions directly. + + # Verify permissions are fixed to 600 + final_mode = env_file.stat().st_mode + assert (final_mode & 0o777) == 0o600