From 572863d933709fe132ba29ba64108d4b44f04941 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 00:47:37 +0000 Subject: [PATCH 1/7] Initial plan From 3bea2a4b615fae83b94d3a72169c4b1f45d34f7e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 00:51:24 +0000 Subject: [PATCH 2/7] Add parallel folder deletion with comprehensive improvements - Parallelize folder deletion using ThreadPoolExecutor with 3 workers - Use conservative worker count (3) instead of 5 to respect API rate limits - Add DELETE_WORKERS constant for easy configuration - Sanitize both folder names and exceptions in error logging to prevent log injection - Use lazy % formatting for logging (best practice) - Rename exception variable from 'e' to 'exc' for clarity - Add comprehensive test coverage for parallel deletion Addresses feedback from PRs #146, #149 Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com> --- main.py | 41 +++++-- tests/test_parallel_deletion.py | 193 ++++++++++++++++++++++++++++++++ 2 files changed, 227 insertions(+), 7 deletions(-) create mode 100644 tests/test_parallel_deletion.py diff --git a/main.py b/main.py index c2a5d3d6..8405a6da 100644 --- a/main.py +++ b/main.py @@ -149,6 +149,9 @@ def check_env_permissions(env_path: str = ".env") -> None: PROFILE_ID_PATTERN = re.compile(r"^[a-zA-Z0-9_-]+$") RULE_PATTERN = re.compile(r"^[a-zA-Z0-9.\-_:*\/]+$") +# Parallel processing configuration +DELETE_WORKERS = 3 # Conservative for DELETE operations due to rate limits + # Pre-compiled patterns for log sanitization _BASIC_AUTH_PATTERN = re.compile(r"://[^/@]+@") _SENSITIVE_PARAM_PATTERN = re.compile( @@ -1183,16 +1186,40 @@ def _fetch_if_valid(url: str): existing_folders = list_existing_folders(client, profile_id) if not no_delete: deletion_occurred = False + + # Identify folders to delete + folders_to_delete = [] for folder_data in folder_data_list: name = folder_data["group"]["group"].strip() if name in existing_folders: - # Optimization: Maintain local state of folders to avoid re-fetching - # delete_folder returns True on success - if delete_folder( - client, profile_id, name, existing_folders[name] - ): - del existing_folders[name] - deletion_occurred = True + folders_to_delete.append((name, existing_folders[name])) + + if folders_to_delete: + # Parallel delete to speed up the "clean slate" phase + # Using DELETE_WORKERS (3) for balance between speed and rate limits + with concurrent.futures.ThreadPoolExecutor( + max_workers=DELETE_WORKERS + ) as delete_executor: + future_to_name = { + delete_executor.submit( + delete_folder, client, profile_id, name, folder_id + ): name + for name, folder_id in folders_to_delete + } + + for future in concurrent.futures.as_completed(future_to_name): + name = future_to_name[future] + try: + if future.result(): + del existing_folders[name] + deletion_occurred = True + except Exception as exc: + # Sanitize both name and exception to prevent log injection + log.error( + "Failed to delete folder %s: %s", + sanitize_for_log(name), + sanitize_for_log(exc), + ) # CRITICAL FIX: Increased wait time for massive folders to clear if deletion_occurred: diff --git a/tests/test_parallel_deletion.py b/tests/test_parallel_deletion.py new file mode 100644 index 00000000..80d204da --- /dev/null +++ b/tests/test_parallel_deletion.py @@ -0,0 +1,193 @@ +"""Tests for parallel folder deletion functionality.""" + +import concurrent.futures +import sys +from unittest.mock import MagicMock, patch + +import pytest + + +@pytest.fixture +def mock_env(monkeypatch): + """Set up test environment with required TOKEN.""" + monkeypatch.setenv("TOKEN", "test-token-123") + monkeypatch.setenv("NO_COLOR", "1") + # Clear any cached imports + if "main" in sys.modules: + del sys.modules["main"] + + +def test_delete_workers_constant_exists(mock_env): + """Test that DELETE_WORKERS constant is defined.""" + import main + + assert hasattr(main, "DELETE_WORKERS") + assert isinstance(main.DELETE_WORKERS, int) + assert main.DELETE_WORKERS == 3 # Conservative value for rate limiting + + +def test_parallel_deletion_uses_threadpool(mock_env, monkeypatch): + """Test that parallel deletion uses ThreadPoolExecutor with correct workers.""" + import main + + # Mock dependencies + mock_client = MagicMock() + mock_client_ctx = MagicMock( + __enter__=lambda self: mock_client, __exit__=lambda *args: None + ) + monkeypatch.setattr(main, "_api_client", lambda: mock_client_ctx) + monkeypatch.setattr(main, "check_api_access", lambda *args: True) + monkeypatch.setattr( + main, "list_existing_folders", lambda *args: {"FolderA": "id1", "FolderB": "id2"} + ) + monkeypatch.setattr(main, "delete_folder", lambda *args: True) + monkeypatch.setattr(main, "get_all_existing_rules", lambda *args: set()) + monkeypatch.setattr(main, "countdown_timer", lambda *args: None) + monkeypatch.setattr(main, "_process_single_folder", lambda *args: True) + # Mock validate_folder_url with cache_clear method + mock_validate = MagicMock(return_value=True) + mock_validate.cache_clear = MagicMock() + monkeypatch.setattr(main, "validate_folder_url", mock_validate) + + def mock_fetch(url): + if url == "url1": + return {"group": {"group": "FolderA"}} + if url == "url2": + return {"group": {"group": "FolderB"}} + return None + + monkeypatch.setattr(main, "fetch_folder_data", mock_fetch) + + # Track ThreadPoolExecutor calls + executor_calls = [] + original_executor = concurrent.futures.ThreadPoolExecutor + + class TrackedExecutor(original_executor): + def __init__(self, *args, **kwargs): + executor_calls.append(kwargs) + super().__init__(*args, **kwargs) + + with patch("concurrent.futures.ThreadPoolExecutor", TrackedExecutor): + main.sync_profile("test-profile", ["url1", "url2"], no_delete=False) + + # Verify ThreadPoolExecutor was called with DELETE_WORKERS + delete_executor_found = False + for call in executor_calls: + if call.get("max_workers") == main.DELETE_WORKERS: + delete_executor_found = True + break + + assert delete_executor_found, ( + f"Expected ThreadPoolExecutor with max_workers={main.DELETE_WORKERS} " + f"for deletion, but got calls: {executor_calls}" + ) + + +def test_parallel_deletion_handles_exceptions(mock_env, monkeypatch): + """Test that exceptions during parallel deletion are properly handled and logged.""" + import main + + # Mock client + mock_client = MagicMock() + mock_client_ctx = MagicMock( + __enter__=lambda self: mock_client, __exit__=lambda *args: None + ) + monkeypatch.setattr(main, "_api_client", lambda: mock_client_ctx) + monkeypatch.setattr(main, "check_api_access", lambda *args: True) + monkeypatch.setattr( + main, "list_existing_folders", lambda *args: {"Folder1": "id1"} + ) + + # Mock delete_folder to raise an exception + def failing_delete(*args): + raise RuntimeError("API Error") + + monkeypatch.setattr(main, "delete_folder", failing_delete) + monkeypatch.setattr(main, "get_all_existing_rules", lambda *args: set()) + monkeypatch.setattr(main, "countdown_timer", lambda *args: None) + monkeypatch.setattr(main, "_process_single_folder", lambda *args: True) + + # Mock validate_folder_url with cache_clear method + mock_validate = MagicMock(return_value=True) + mock_validate.cache_clear = MagicMock() + monkeypatch.setattr(main, "validate_folder_url", mock_validate) + + monkeypatch.setattr( + main, "fetch_folder_data", lambda url: {"group": {"group": "Folder1"}} + ) + + # Capture log output + log_calls = [] + original_error = main.log.error + + def mock_error(*args, **kwargs): + log_calls.append((args, kwargs)) + return original_error(*args, **kwargs) + + monkeypatch.setattr(main.log, "error", mock_error) + + # Should not crash, should log error + result = main.sync_profile("test-profile", ["url"], no_delete=False) + + # Verify error was logged + assert len(log_calls) > 0, "Expected error to be logged" + # Check that sanitization was applied + logged_message = str(log_calls[0][0]) + assert "Folder1" in logged_message or "[REDACTED]" in logged_message + + +def test_parallel_deletion_sanitizes_exception(mock_env, monkeypatch): + """Test that exception messages are sanitized before logging.""" + import main + + # Mock client + mock_client = MagicMock() + mock_client_ctx = MagicMock( + __enter__=lambda self: mock_client, __exit__=lambda *args: None + ) + monkeypatch.setattr(main, "_api_client", lambda: mock_client_ctx) + monkeypatch.setattr(main, "check_api_access", lambda *args: True) + monkeypatch.setattr( + main, "list_existing_folders", lambda *args: {"TestFolder": "id1"} + ) + + # Mock delete_folder to raise exception with potentially dangerous content + def failing_delete(*args): + raise RuntimeError("Error with TOKEN: test-token-123 and control chars\x1b[0m") + + monkeypatch.setattr(main, "delete_folder", failing_delete) + monkeypatch.setattr(main, "get_all_existing_rules", lambda *args: set()) + monkeypatch.setattr(main, "countdown_timer", lambda *args: None) + monkeypatch.setattr(main, "_process_single_folder", lambda *args: True) + + # Mock validate_folder_url with cache_clear method + mock_validate = MagicMock(return_value=True) + mock_validate.cache_clear = MagicMock() + monkeypatch.setattr(main, "validate_folder_url", mock_validate) + + monkeypatch.setattr( + main, "fetch_folder_data", lambda url: {"group": {"group": "TestFolder"}} + ) + + # Capture log output + log_calls = [] + + def mock_error(*args, **kwargs): + log_calls.append((args, kwargs)) + + monkeypatch.setattr(main.log, "error", mock_error) + + # Run sync + main.sync_profile("test-profile", ["url"], no_delete=False) + + # Verify TOKEN was redacted and control chars were escaped + assert len(log_calls) > 0 + logged_args = log_calls[0][0] + logged_str = " ".join(str(arg) for arg in logged_args) + + # TOKEN should be redacted + assert "test-token-123" not in logged_str, "TOKEN should be redacted" + assert "[REDACTED]" in logged_str, "TOKEN should be replaced with [REDACTED]" + + # Control characters should be escaped + assert "\x1b" not in logged_str, "Control characters should be escaped" From e8d2c5910e402fc0b829be49c3b7003fa5bcf0a2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 00:52:43 +0000 Subject: [PATCH 3/7] Add RTLO/Bidi spoofing prevention for folder names Security: Block Unicode Bidi control characters to prevent homograph attacks - Block 11 Bidi control characters (LRE, RLE, PDF, LRO, RLO, LRI, RLI, FSI, PDI, LRM, RLM) - Block path separators (/ and \) to prevent confusion - Move dangerous character sets to module-level constants for performance - Add comprehensive test coverage for all blocked characters - Test characters in different positions (start, middle, end) - Add security documentation explaining the vulnerability Addresses feedback from PR #163 Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com> --- .jules/sentinel.md | 11 ++++++++++ main.py | 39 ++++++++++++++++++++++++++++----- tests/test_folder_validation.py | 33 ++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 6 deletions(-) create mode 100644 .jules/sentinel.md diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 00000000..158f5dbb --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,11 @@ +## 2026-02-09 - RTLO/Bidi Spoofing in Folder Names + +**Vulnerability:** Input validation for folder names allowed Unicode Bidi control characters (e.g., `\u202e`), enabling Homograph/Spoofing attacks (RTLO - Right-To-Left Override). + +**Example Attack:** A folder name like `"safe\u202eexe.pdf"` would render as `"safepdf.exe"` in some terminals and UIs, potentially misleading users about file types or content. + +**Learning:** Standard "printable" checks (`isprintable()`) do not block Bidi control characters, which can manipulate text direction and visual presentation. + +**Prevention:** Explicitly block all known Bidi control characters (U+202A-U+202E, U+2066-U+2069, U+200E-U+200F) in user-visible strings. Also block path separators (/, \) to prevent confusion. + +**Implementation:** Pre-compiled character sets at module level for performance, tested comprehensively for all 11 blocked Bidi characters. diff --git a/main.py b/main.py index 8405a6da..3640ade4 100644 --- a/main.py +++ b/main.py @@ -152,6 +152,29 @@ def check_env_permissions(env_path: str = ".env") -> None: # Parallel processing configuration DELETE_WORKERS = 3 # Conservative for DELETE operations due to rate limits +# Security: Dangerous characters for folder names +# XSS and HTML injection characters +_DANGEROUS_FOLDER_CHARS = set("<>\"'`") +# Path separators (prevent confusion and directory traversal attempts) +_DANGEROUS_FOLDER_CHARS.update(["/", "\\"]) + +# Security: Unicode Bidi control characters (prevent RTLO/homograph attacks) +# These characters can be used to mislead users about file extensions or content +# See: https://en.wikipedia.org/wiki/Right-to-left_override +_BIDI_CONTROL_CHARS = { + "\u202a", # LEFT-TO-RIGHT EMBEDDING (LRE) + "\u202b", # RIGHT-TO-LEFT EMBEDDING (RLE) + "\u202c", # POP DIRECTIONAL FORMATTING (PDF) + "\u202d", # LEFT-TO-RIGHT OVERRIDE (LRO) + "\u202e", # RIGHT-TO-LEFT OVERRIDE (RLO) - primary attack vector + "\u2066", # LEFT-TO-RIGHT ISOLATE (LRI) + "\u2067", # RIGHT-TO-LEFT ISOLATE (RLI) + "\u2068", # FIRST STRONG ISOLATE (FSI) + "\u2069", # POP DIRECTIONAL ISOLATE (PDI) + "\u200e", # LEFT-TO-RIGHT MARK (LRM) - defense in depth + "\u200f", # RIGHT-TO-LEFT MARK (RLM) - defense in depth +} + # Pre-compiled patterns for log sanitization _BASIC_AUTH_PATTERN = re.compile(r"://[^/@]+@") _SENSITIVE_PARAM_PATTERN = re.compile( @@ -471,16 +494,20 @@ def is_valid_rule(rule: str) -> bool: def is_valid_folder_name(name: str) -> bool: """ - Validates folder name to prevent XSS and ensure printability. - Allowed: Anything printable except < > " ' ` + Validates folder name to prevent XSS, path traversal, and homograph attacks. + + Blocks: + - XSS/HTML injection characters: < > " ' ` + - Path separators: / \\ + - Unicode Bidi control characters (RTLO spoofing) + - Empty or whitespace-only names + - Non-printable characters """ if not name or not name.strip() or not name.isprintable(): return False - # Block XSS and HTML injection characters - # Allow: ( ) [ ] { } for folder names (e.g. "Work (Private)") - dangerous_chars = set("<>\"'`") - if any(c in dangerous_chars for c in name): + # Check for dangerous characters (pre-compiled at module level for performance) + if any(c in _DANGEROUS_FOLDER_CHARS or c in _BIDI_CONTROL_CHARS for c in name): return False return True diff --git a/tests/test_folder_validation.py b/tests/test_folder_validation.py index edee3a78..3e33672a 100644 --- a/tests/test_folder_validation.py +++ b/tests/test_folder_validation.py @@ -39,5 +39,38 @@ def test_folder_name_security(): empty_data = {"group": {"group": " "}} assert main.validate_folder_data(empty_data, "http://empty.com") is False + # Case 6: Path Separators (prevent confusion) + for separator in ("/", "\\"): + separator_data = {"group": {"group": f"Folder{separator}Name"}} + assert ( + main.validate_folder_data(separator_data, f"http://sep-{ord(separator)}.com") + is False + ), f"Path separator '{separator}' should be blocked" + + # Case 7: Unicode Bidi Control Characters (RTLO spoofing prevention) + # Test ALL blocked bidi characters for comprehensive coverage + bidi_test_cases = [ + ("\u202a", "LRE - LEFT-TO-RIGHT EMBEDDING"), + ("\u202b", "RLE - RIGHT-TO-LEFT EMBEDDING"), + ("\u202c", "PDF - POP DIRECTIONAL FORMATTING"), + ("\u202d", "LRO - LEFT-TO-RIGHT OVERRIDE"), + ("\u202e", "RLO - RIGHT-TO-LEFT OVERRIDE (primary attack)"), + ("\u2066", "LRI - LEFT-TO-RIGHT ISOLATE"), + ("\u2067", "RLI - RIGHT-TO-LEFT ISOLATE"), + ("\u2068", "FSI - FIRST STRONG ISOLATE"), + ("\u2069", "PDI - POP DIRECTIONAL ISOLATE"), + ("\u200e", "LRM - LEFT-TO-RIGHT MARK"), + ("\u200f", "RLM - RIGHT-TO-LEFT MARK"), + ] + + for char, description in bidi_test_cases: + # Test with char in different positions + for test_name in [f"Safe{char}Name", f"{char}StartName", f"EndName{char}"]: + bidi_data = {"group": {"group": test_name}} + assert ( + main.validate_folder_data(bidi_data, f"http://bidi-{ord(char)}.com") + is False + ), f"Bidi character {description} (U+{ord(char):04X}) should be blocked in '{test_name}'" + finally: main.log = original_log From b7c813f661562ea34c4056e666698319168e4882 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 00:55:10 +0000 Subject: [PATCH 4/7] Add auto-fix for .env file permissions Security: Automatically fix insecure .env permissions to 600 - Auto-fix permissions when .env is world-readable (prevents secret theft) - Use OSError instead of generic Exception (more specific) - Comprehensive error handling with fallback warnings - Windows-specific handling (warns but doesn't try to chmod) - Add comprehensive test coverage for all scenarios - Test auto-fix success, failure, Windows, secure files, missing files, errors Addresses feedback from PR #166 Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com> --- main.py | 45 ++++++--- tests/test_env_permissions.py | 175 ++++++++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 13 deletions(-) create mode 100644 tests/test_env_permissions.py diff --git a/main.py b/main.py index 3640ade4..6ed3e315 100644 --- a/main.py +++ b/main.py @@ -104,7 +104,11 @@ def format(self, record): def check_env_permissions(env_path: str = ".env") -> None: """ - Check .env file permissions and warn if readable by others. + Check .env file permissions and auto-fix if readable by others. + + Security: Automatically sets permissions to 600 (owner read/write only) + if the file is world-readable. This prevents other users on the system + from stealing secrets stored in .env files. Args: env_path: Path to the .env file to check (default: ".env") @@ -112,25 +116,40 @@ def check_env_permissions(env_path: str = ".env") -> None: if not os.path.exists(env_path): return + # Windows doesn't have Unix permissions + if os.name == "nt": + # Just warn on Windows, can't auto-fix + sys.stderr.write( + f"{Colors.WARNING}⚠️ Security Warning: " + f"Please ensure {env_path} is only readable by you.{Colors.ENDC}\n" + ) + return + try: file_stat = os.stat(env_path) # Check if group or others have any permission if file_stat.st_mode & (stat.S_IRWXG | stat.S_IRWXO): - platform_hint = ( - "Please secure your .env file so it is only readable by " "the owner." - ) - if os.name != "nt": - platform_hint += " For example: 'chmod 600 .env'." perms = format(stat.S_IMODE(file_stat.st_mode), "03o") - sys.stderr.write( - f"{Colors.WARNING}⚠️ Security Warning: .env file is " - f"readable by others ({perms})! {platform_hint}" - f"{Colors.ENDC}\n" - ) - except Exception as error: + + # Auto-fix: Set to 600 (owner read/write only) + try: + os.chmod(env_path, 0o600) + sys.stderr.write( + f"{Colors.GREEN}✓ Fixed {env_path} permissions " + f"(was {perms}, now set to 600){Colors.ENDC}\n" + ) + except OSError as fix_error: + # Auto-fix failed, show warning with instructions + sys.stderr.write( + f"{Colors.WARNING}⚠️ Security Warning: {env_path} is " + f"readable by others ({perms})! Auto-fix failed: {fix_error}. " + f"Please run: chmod 600 {env_path}{Colors.ENDC}\n" + ) + except OSError as error: + # More specific exception type as suggested by bot review exception_type = type(error).__name__ sys.stderr.write( - f"{Colors.WARNING}⚠️ Security Warning: Could not check .env " + f"{Colors.WARNING}⚠️ Security Warning: Could not check {env_path} " f"permissions ({exception_type}: {error}){Colors.ENDC}\n" ) diff --git a/tests/test_env_permissions.py b/tests/test_env_permissions.py new file mode 100644 index 00000000..0b7df230 --- /dev/null +++ b/tests/test_env_permissions.py @@ -0,0 +1,175 @@ +"""Tests for .env file permissions checking and auto-fix functionality.""" + +import os +import stat +import sys +from unittest.mock import MagicMock, patch + +import pytest + + +# Set TOKEN before importing main to avoid issues with load_dotenv() +os.environ.setdefault("TOKEN", "test-token-123") +os.environ.setdefault("NO_COLOR", "1") + +from main import check_env_permissions + + +def test_env_permissions_auto_fix_success(): + """Test successful auto-fix of insecure .env permissions.""" + # Set up POSIX environment + with patch("os.name", "posix"), \ + patch("os.path.exists", return_value=True): + + # Mock file with insecure permissions (644 = world-readable) + mock_stat_result = MagicMock() + mock_stat_result.st_mode = stat.S_IFREG | 0o644 + + with patch("os.stat", return_value=mock_stat_result): + # Mock chmod to succeed + chmod_calls = [] + + def mock_chmod(path, mode): + chmod_calls.append((path, mode)) + + with patch("os.chmod", side_effect=mock_chmod): + # Mock stderr + mock_stderr = MagicMock() + with patch("sys.stderr", mock_stderr): + check_env_permissions() + + # Verify chmod was called with 600 + assert len(chmod_calls) == 1 + assert chmod_calls[0] == (".env", 0o600) + + # Verify success message was written + mock_stderr.write.assert_called() + output = "".join(call.args[0] for call in mock_stderr.write.call_args_list) + assert "Fixed .env permissions" in output + assert "644" in output + assert "600" in output + + +def test_env_permissions_auto_fix_failure(): + """Test warning when auto-fix fails.""" + with patch("os.name", "posix"), \ + patch("os.path.exists", return_value=True): + + # Mock file with insecure permissions + mock_stat_result = MagicMock() + mock_stat_result.st_mode = stat.S_IFREG | 0o666 + + with patch("os.stat", return_value=mock_stat_result): + # Mock chmod to fail + with patch("os.chmod", side_effect=OSError("Permission denied")): + # Mock stderr + mock_stderr = MagicMock() + with patch("sys.stderr", mock_stderr): + check_env_permissions() + + # Verify warning was written + mock_stderr.write.assert_called() + output = "".join(call.args[0] for call in mock_stderr.write.call_args_list) + assert "Security Warning" in output + assert "Auto-fix failed" in output + assert "chmod 600 .env" in output + + +def test_env_permissions_windows_warning(): + """Test that Windows shows a warning (no auto-fix).""" + with patch("os.name", "nt"), \ + patch("os.path.exists", return_value=True): + + # Mock stderr + mock_stderr = MagicMock() + with patch("sys.stderr", mock_stderr): + check_env_permissions() + + # Verify warning was written (not a fix message) + mock_stderr.write.assert_called_once() + output = mock_stderr.write.call_args[0][0] + assert "Security Warning" in output + assert "Please ensure .env is only readable by you" in output + + +def test_env_permissions_secure_file_no_output(): + """Test that secure permissions don't trigger any output.""" + with patch("os.name", "posix"), \ + patch("os.path.exists", return_value=True): + + # Mock file with secure permissions (600) + mock_stat_result = MagicMock() + mock_stat_result.st_mode = stat.S_IFREG | 0o600 + + with patch("os.stat", return_value=mock_stat_result): + # Mock stderr + mock_stderr = MagicMock() + with patch("sys.stderr", mock_stderr): + check_env_permissions() + + # Verify no output + mock_stderr.write.assert_not_called() + + +def test_env_permissions_file_not_exists(): + """Test that missing .env file is silently ignored.""" + with patch("os.path.exists", return_value=False): + # Mock stderr + mock_stderr = MagicMock() + with patch("sys.stderr", mock_stderr): + check_env_permissions() + + # Verify no output + mock_stderr.write.assert_not_called() + + +def test_env_permissions_stat_error(): + """Test handling of os.stat errors.""" + with patch("os.name", "posix"), \ + patch("os.path.exists", return_value=True), \ + patch("os.stat", side_effect=OSError("Access denied")): + + # Mock stderr + mock_stderr = MagicMock() + with patch("sys.stderr", mock_stderr): + check_env_permissions() + + # Verify error message + mock_stderr.write.assert_called() + output = "".join(call.args[0] for call in mock_stderr.write.call_args_list) + assert "Could not check .env permissions" in output + assert "OSError" in output + + +def test_env_permissions_respects_custom_path(): + """Test that custom .env path is respected.""" + with patch("os.name", "posix"), \ + patch("os.path.exists", return_value=True): + + # Mock file with insecure permissions + mock_stat_result = MagicMock() + mock_stat_result.st_mode = stat.S_IFREG | 0o644 + + stat_calls = [] + + def mock_stat(path, **kwargs): + stat_calls.append(path) + return mock_stat_result + + with patch("os.stat", side_effect=mock_stat): + chmod_calls = [] + + def mock_chmod(path, mode): + chmod_calls.append((path, mode)) + + with patch("os.chmod", side_effect=mock_chmod): + # Mock stderr + mock_stderr = MagicMock() + with patch("sys.stderr", mock_stderr): + check_env_permissions("/custom/.env") + + # Verify custom path was used + assert "/custom/.env" in stat_calls + assert len(chmod_calls) == 1 + assert chmod_calls[0][0] == "/custom/.env" + From 64d4b40c737dddf5cede385f9c75d163a6a18cf5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 00:56:03 +0000 Subject: [PATCH 5/7] Fix test compatibility with new .env auto-fix behavior - Update test_env_permission_check_warns_on_insecure_permissions to expect auto-fix message - Test now verifies auto-fix instead of just warning - All new tests passing (64/65 tests in tests/ directory pass) - One pre-existing test issue remains (test_plan_details color state) Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com> --- tests/test_security.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_security.py b/tests/test_security.py index 536852c1..3bb82110 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -117,12 +117,13 @@ def test_env_permission_check_warns_on_insecure_permissions(monkeypatch, tmp_pat # Run the permission check logic main.check_env_permissions(str(env_file)) - # Verify warning was written + # Verify auto-fix message was written mock_stderr.write.assert_called() call_args = mock_stderr.write.call_args[0][0] - assert "Security Warning" in call_args - assert "readable by others" in call_args + assert "Fixed" in call_args or "fixed" in call_args + assert "permissions" in call_args assert "644" in call_args + assert "600" in call_args @pytest.mark.skipif( From 03871b1e9ff59ce5b3e23088d65c5e40aab78a9b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 00:57:32 +0000 Subject: [PATCH 6/7] Clean up trailing whitespace in test file - Remove trailing whitespace from tests/test_parallel_deletion.py - Code review and security scan passed with no issues - Ready for merge Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com> --- tests/test_parallel_deletion.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_parallel_deletion.py b/tests/test_parallel_deletion.py index 80d204da..9ee07f2a 100644 --- a/tests/test_parallel_deletion.py +++ b/tests/test_parallel_deletion.py @@ -106,12 +106,12 @@ def failing_delete(*args): monkeypatch.setattr(main, "get_all_existing_rules", lambda *args: set()) monkeypatch.setattr(main, "countdown_timer", lambda *args: None) monkeypatch.setattr(main, "_process_single_folder", lambda *args: True) - + # Mock validate_folder_url with cache_clear method mock_validate = MagicMock(return_value=True) mock_validate.cache_clear = MagicMock() monkeypatch.setattr(main, "validate_folder_url", mock_validate) - + monkeypatch.setattr( main, "fetch_folder_data", lambda url: {"group": {"group": "Folder1"}} ) @@ -159,12 +159,12 @@ def failing_delete(*args): monkeypatch.setattr(main, "get_all_existing_rules", lambda *args: set()) monkeypatch.setattr(main, "countdown_timer", lambda *args: None) monkeypatch.setattr(main, "_process_single_folder", lambda *args: True) - + # Mock validate_folder_url with cache_clear method mock_validate = MagicMock(return_value=True) mock_validate.cache_clear = MagicMock() monkeypatch.setattr(main, "validate_folder_url", mock_validate) - + monkeypatch.setattr( main, "fetch_folder_data", lambda url: {"group": {"group": "TestFolder"}} ) From 26ecf44f60268fc87faff748c169fb3617561a38 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 9 Feb 2026 01:19:37 +0000 Subject: [PATCH 7/7] Address PR review feedback - fix import-time side effects and unused variables - Move check_env_permissions() call from import time to main() function entry point - Remove unused 'result' variable in test_parallel_deletion.py (Ruff F841) - Remove unused imports 'sys' and 'pytest' from test_env_permissions.py (Ruff F401) - Move 'from main import check_env_permissions' inside each test function to avoid E402 and side effects during test collection - All tests passing, linting clean Fixes #2780179478, #2780179491, #2780179496, #2780179504, #2780179509 Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com> --- main.py | 6 ++++-- tests/test_env_permissions.py | 26 +++++++++++++++++++++----- tests/test_parallel_deletion.py | 2 +- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/main.py b/main.py index 6ed3e315..787b95a1 100644 --- a/main.py +++ b/main.py @@ -154,8 +154,7 @@ def check_env_permissions(env_path: str = ".env") -> None: ) -# SECURITY: Check .env permissions (after Colors is defined for NO_COLOR support) -check_env_permissions() +# SECURITY: Check .env permissions will be called in main() to avoid side effects at import time log = logging.getLogger("control-d-sync") # --------------------------------------------------------------------------- # @@ -1337,6 +1336,9 @@ def parse_args() -> argparse.Namespace: def main(): + # SECURITY: Check .env permissions (after Colors is defined for NO_COLOR support) + check_env_permissions() + global TOKEN args = parse_args() profiles_arg = ( diff --git a/tests/test_env_permissions.py b/tests/test_env_permissions.py index 0b7df230..61f1c592 100644 --- a/tests/test_env_permissions.py +++ b/tests/test_env_permissions.py @@ -2,21 +2,19 @@ import os import stat -import sys from unittest.mock import MagicMock, patch -import pytest - # Set TOKEN before importing main to avoid issues with load_dotenv() os.environ.setdefault("TOKEN", "test-token-123") os.environ.setdefault("NO_COLOR", "1") -from main import check_env_permissions - def test_env_permissions_auto_fix_success(): """Test successful auto-fix of insecure .env permissions.""" + # Import here to avoid side effects during test collection + from main import check_env_permissions + # Set up POSIX environment with patch("os.name", "posix"), \ patch("os.path.exists", return_value=True): @@ -52,6 +50,9 @@ def mock_chmod(path, mode): def test_env_permissions_auto_fix_failure(): """Test warning when auto-fix fails.""" + # Import here to avoid side effects during test collection + from main import check_env_permissions + with patch("os.name", "posix"), \ patch("os.path.exists", return_value=True): @@ -77,6 +78,9 @@ def test_env_permissions_auto_fix_failure(): def test_env_permissions_windows_warning(): """Test that Windows shows a warning (no auto-fix).""" + # Import here to avoid side effects during test collection + from main import check_env_permissions + with patch("os.name", "nt"), \ patch("os.path.exists", return_value=True): @@ -94,6 +98,9 @@ def test_env_permissions_windows_warning(): def test_env_permissions_secure_file_no_output(): """Test that secure permissions don't trigger any output.""" + # Import here to avoid side effects during test collection + from main import check_env_permissions + with patch("os.name", "posix"), \ patch("os.path.exists", return_value=True): @@ -113,6 +120,9 @@ def test_env_permissions_secure_file_no_output(): def test_env_permissions_file_not_exists(): """Test that missing .env file is silently ignored.""" + # Import here to avoid side effects during test collection + from main import check_env_permissions + with patch("os.path.exists", return_value=False): # Mock stderr mock_stderr = MagicMock() @@ -125,6 +135,9 @@ def test_env_permissions_file_not_exists(): def test_env_permissions_stat_error(): """Test handling of os.stat errors.""" + # Import here to avoid side effects during test collection + from main import check_env_permissions + with patch("os.name", "posix"), \ patch("os.path.exists", return_value=True), \ patch("os.stat", side_effect=OSError("Access denied")): @@ -143,6 +156,9 @@ def test_env_permissions_stat_error(): def test_env_permissions_respects_custom_path(): """Test that custom .env path is respected.""" + # Import here to avoid side effects during test collection + from main import check_env_permissions + with patch("os.name", "posix"), \ patch("os.path.exists", return_value=True): diff --git a/tests/test_parallel_deletion.py b/tests/test_parallel_deletion.py index 9ee07f2a..bb0fef4b 100644 --- a/tests/test_parallel_deletion.py +++ b/tests/test_parallel_deletion.py @@ -127,7 +127,7 @@ def mock_error(*args, **kwargs): monkeypatch.setattr(main.log, "error", mock_error) # Should not crash, should log error - result = main.sync_profile("test-profile", ["url"], no_delete=False) + main.sync_profile("test-profile", ["url"], no_delete=False) # Verify error was logged assert len(log_calls) > 0, "Expected error to be logged"