🛡️ Sentinel: [HIGH] Fix Symlink Vulnerability in .env Handling

.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.

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).")

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

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)

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_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

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