From 2fc202fb74224c8cb460576eb19f44e34e848d57 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Oct 2025 16:38:36 -0500 Subject: [PATCH] PR feedback --- src/core/adb_command.py | 5 --- src/utils/security_utils.py | 52 +++++------------------------- tests/utils/test_security_utils.py | 25 +++----------- 3 files changed, 13 insertions(+), 69 deletions(-) diff --git a/src/core/adb_command.py b/src/core/adb_command.py index 0e48b14..bedd246 100644 --- a/src/core/adb_command.py +++ b/src/core/adb_command.py @@ -10,11 +10,6 @@ from typing import Optional, Tuple, Union from .platform_tools import get_adb_binary_path -try: - from ..utils.security_utils import validate_device_id, sanitize_android_path -except ImportError: - from utils.security_utils import validate_device_id, sanitize_android_path - class ADBCommandRunner: """Handles ADB command execution and device communication.""" diff --git a/src/utils/security_utils.py b/src/utils/security_utils.py index 72913fa..1e70cdb 100644 --- a/src/utils/security_utils.py +++ b/src/utils/security_utils.py @@ -59,6 +59,8 @@ def sanitize_android_path(path: str) -> str: raise ValueError("Path contains null byte") # Check for command injection patterns + # Note: We check for shell metacharacters that could be used for command injection + # We allow spaces and most characters that are valid in Android paths dangerous_patterns = [ ';', '|', '&', '$(', '${', '`', '\n', '\r', '&&', '||', '>>', @@ -68,13 +70,10 @@ def sanitize_android_path(path: str) -> str: if pattern in path: raise ValueError(f"Path contains dangerous pattern: {pattern}") - # Validate path structure (should start with / for absolute paths on Android) - # Allow relative paths but be cautious - if not path.startswith('/') and not path.startswith('./'): - # If it's not an absolute or explicitly relative path, make it explicit - # Most Android paths should be absolute - if not re.match(r'^[a-zA-Z0-9_\-./]+$', path): - raise ValueError("Path contains invalid characters") + # Note: We allow spaces, Unicode characters, and other characters that are + # valid in Android filesystem paths. The dangerous pattern check above is + # sufficient to prevent command injection since we pass paths as arguments + # to subprocess (not through shell=True). return path @@ -109,6 +108,8 @@ def sanitize_local_path(path: str, base_dir: Optional[str] = None) -> str: raise ValueError(f"Invalid path: {e}") # If base_dir is specified, ensure the path is within it + # This check is sufficient - after normalization, if the path doesn't start + # with base_dir, it's outside the allowed directory tree if base_dir: try: base_dir_abs = os.path.normpath(os.path.abspath(base_dir)) @@ -118,15 +119,6 @@ def sanitize_local_path(path: str, base_dir: Optional[str] = None) -> str: except (ValueError, OSError) as e: raise ValueError(f"Invalid base directory: {e}") - # Check for dangerous patterns in the original path that might bypass normalization - if '..' in path: - # Verify that after normalization, we haven't moved up unexpectedly - path_depth = normalized_path.count(os.sep) - if base_dir: - base_depth = base_dir_abs.count(os.sep) - if path_depth < base_depth: - raise ValueError("Path traversal detected: attempting to access parent directories") - return normalized_path @@ -156,31 +148,3 @@ def validate_device_id(device_id: str) -> str: raise ValueError(f"Device ID contains dangerous character: {char}") return device_id - - -def escape_shell_arg(arg: str) -> str: - """Escape a shell argument for safe use in commands. - - Note: This is a defense-in-depth measure. Prefer using validated inputs - and avoiding shell=True in subprocess calls. - - Args: - arg: Argument to escape - - Returns: - Escaped argument safe for shell use - """ - # For maximum safety with subprocess, we actually want to avoid shell escaping - # and instead ensure the argument doesn't contain dangerous characters - # This function validates and returns the argument if safe - - if not arg: - return arg - - # Check for any shell metacharacters - dangerous_chars = [';', '|', '&', '$', '`', '\n', '\r', '>', '<', '(', ')', '{', '}', '[', ']', '!', '*', '?', '~'] - for char in dangerous_chars: - if char in arg: - raise ValueError(f"Argument contains shell metacharacter: {char}") - - return arg diff --git a/tests/utils/test_security_utils.py b/tests/utils/test_security_utils.py index c46bff7..7de14e4 100644 --- a/tests/utils/test_security_utils.py +++ b/tests/utils/test_security_utils.py @@ -9,7 +9,6 @@ from src.utils.security_utils import ( sanitize_android_path, sanitize_local_path, validate_device_id, - escape_shell_arg, ) @@ -54,6 +53,11 @@ class TestSanitizeAndroidPath: """Test that valid relative paths are accepted.""" assert sanitize_android_path("./folder/file.txt") == "./folder/file.txt" + def test_path_with_spaces(self): + """Test that paths with spaces are allowed.""" + assert sanitize_android_path("/sdcard/My Photos/vacation.jpg") == "/sdcard/My Photos/vacation.jpg" + assert sanitize_android_path("/sdcard/DCIM/Camera Roll/IMG_001.jpg") == "/sdcard/DCIM/Camera Roll/IMG_001.jpg" + def test_empty_path(self): """Test that empty paths are rejected.""" with pytest.raises(ValueError, match="Path cannot be empty"): @@ -152,25 +156,6 @@ class TestValidateDeviceId: validate_device_id(f"device{char}123") -class TestEscapeShellArg: - """Tests for escape_shell_arg function.""" - - def test_safe_argument(self): - """Test that safe arguments are passed through.""" - assert escape_shell_arg("safe-file_name.txt") == "safe-file_name.txt" - - def test_empty_argument(self): - """Test that empty arguments are handled.""" - assert escape_shell_arg("") == "" - - def test_shell_metacharacters(self): - """Test that shell metacharacters are rejected.""" - dangerous_chars = [';', '|', '&', '$', '`', '>', '<', '(', ')', '*'] - for char in dangerous_chars: - with pytest.raises(ValueError, match="shell metacharacter"): - escape_shell_arg(f"arg{char}value") - - class TestSecurityIntegration: """Integration tests for security utilities."""