addressing final PR comments
This commit is contained in:
+17
-5
@@ -149,8 +149,11 @@ class ADBManager:
|
||||
try:
|
||||
sanitized_path = sanitize_android_path(path)
|
||||
except ValueError as e:
|
||||
# Log validation error and return empty list
|
||||
logger.warning(f"Invalid path rejected in list_files: {str(e)}")
|
||||
# Log detailed validation error
|
||||
logger.warning(
|
||||
f"Security: Path rejected in list_files() - "
|
||||
f"path='{path[:100]}', reason: {str(e)}"
|
||||
)
|
||||
return []
|
||||
|
||||
device_args = []
|
||||
@@ -160,7 +163,10 @@ class ADBManager:
|
||||
validated_device = validate_device_id(target_device)
|
||||
device_args = ["-s", validated_device]
|
||||
except ValueError as e:
|
||||
logger.warning(f"Invalid device ID rejected in list_files: {str(e)}")
|
||||
logger.warning(
|
||||
f"Security: Device ID rejected in list_files() - "
|
||||
f"device_id='{target_device}', reason: {str(e)}"
|
||||
)
|
||||
return []
|
||||
|
||||
args = device_args + ["shell", "ls", "-la", sanitized_path]
|
||||
@@ -409,7 +415,10 @@ class ADBManager:
|
||||
try:
|
||||
sanitized_path = sanitize_android_path(remote_path)
|
||||
except ValueError as e:
|
||||
logger.warning(f"Invalid path rejected in get_file_info: {str(e)}")
|
||||
logger.warning(
|
||||
f"Security: Path rejected in get_file_info() - "
|
||||
f"path='{remote_path[:100]}', reason: {str(e)}"
|
||||
)
|
||||
return None
|
||||
|
||||
device_args = []
|
||||
@@ -419,7 +428,10 @@ class ADBManager:
|
||||
validated_device = validate_device_id(target_device)
|
||||
device_args = ["-s", validated_device]
|
||||
except ValueError as e:
|
||||
logger.warning(f"Invalid device ID rejected in get_file_info: {str(e)}")
|
||||
logger.warning(
|
||||
f"Security: Device ID rejected in get_file_info() - "
|
||||
f"device_id='{target_device}', reason: {str(e)}"
|
||||
)
|
||||
return None
|
||||
|
||||
args = device_args + ["shell", "ls", "-la", sanitized_path]
|
||||
|
||||
@@ -7,6 +7,10 @@ import os
|
||||
import re
|
||||
from typing import Optional
|
||||
|
||||
# Pre-compiled regex patterns for performance
|
||||
_DANGEROUS_CHAR_PATTERN = re.compile(r'[;|&$`\n\r><(){}[\]!]')
|
||||
_DANGEROUS_PATH_PATTERN = re.compile(r'[;|&`\n\r]|\$[({]|&&|\|\||>>')
|
||||
|
||||
|
||||
def sanitize_path_component(component: str) -> str:
|
||||
"""Sanitize a single path component to prevent injection.
|
||||
@@ -27,10 +31,9 @@ def sanitize_path_component(component: str) -> str:
|
||||
if '\x00' in component:
|
||||
raise ValueError("Path component contains null byte")
|
||||
|
||||
# Check for dangerous characters using optimized regex
|
||||
# Check for dangerous characters using pre-compiled regex
|
||||
# Matches any shell metacharacters that could be used for command injection
|
||||
dangerous_pattern = r'[;|&$`\n\r><(){}[\]!]'
|
||||
match = re.search(dangerous_pattern, component)
|
||||
match = _DANGEROUS_CHAR_PATTERN.search(component)
|
||||
if match:
|
||||
raise ValueError(f"Path component contains dangerous character: {match.group()}")
|
||||
|
||||
@@ -63,13 +66,12 @@ def sanitize_android_path(path: str) -> str:
|
||||
if '\x00' in path:
|
||||
raise ValueError("Path contains null byte")
|
||||
|
||||
# Check for command injection patterns using optimized regex
|
||||
# Check for command injection patterns using pre-compiled regex
|
||||
# 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
|
||||
# Pattern matches: semicolon, pipe, ampersand, dollar-paren, dollar-brace,
|
||||
# backtick, newline, carriage return, double-ampersand, double-pipe, double-redirect
|
||||
dangerous_pattern = r'[;|&`\n\r]|\$[({]|&&|\|\||>>'
|
||||
match = re.search(dangerous_pattern, path)
|
||||
match = _DANGEROUS_PATH_PATTERN.search(path)
|
||||
if match:
|
||||
raise ValueError(f"Path contains dangerous pattern: {match.group()}")
|
||||
|
||||
@@ -81,12 +83,13 @@ def sanitize_android_path(path: str) -> str:
|
||||
return path
|
||||
|
||||
|
||||
def sanitize_local_path(path: str, base_dir: Optional[str] = None) -> str:
|
||||
def sanitize_local_path(path: str, base_dir: Optional[str] = None, allow_nonexistent: bool = True) -> str:
|
||||
"""Sanitize a local filesystem path and check for path traversal.
|
||||
|
||||
Args:
|
||||
path: Local filesystem path
|
||||
base_dir: Optional base directory to restrict path within
|
||||
allow_nonexistent: If True, allow paths that don't exist yet (uses abspath instead of realpath)
|
||||
|
||||
Returns:
|
||||
Sanitized and normalized absolute path
|
||||
@@ -105,8 +108,15 @@ def sanitize_local_path(path: str, base_dir: Optional[str] = None) -> str:
|
||||
raise ValueError("Path contains null byte")
|
||||
|
||||
# Normalize the path to resolve .. and symlinks
|
||||
# For non-existent paths, use abspath to avoid CWD resolution issues
|
||||
# For existing paths, use realpath to resolve symlinks and prevent escapes
|
||||
try:
|
||||
normalized_path = os.path.normpath(os.path.realpath(path))
|
||||
if allow_nonexistent and not os.path.exists(path):
|
||||
# Path doesn't exist yet (e.g., pull destination) - use abspath
|
||||
normalized_path = os.path.normpath(os.path.abspath(path))
|
||||
else:
|
||||
# Path exists or we're strict - use realpath to resolve symlinks
|
||||
normalized_path = os.path.normpath(os.path.realpath(path))
|
||||
except (ValueError, OSError) as e:
|
||||
raise ValueError(f"Invalid path: {e}")
|
||||
|
||||
@@ -115,7 +125,11 @@ def sanitize_local_path(path: str, base_dir: Optional[str] = None) -> str:
|
||||
# with base_dir, it's outside the allowed directory tree
|
||||
if base_dir:
|
||||
try:
|
||||
base_dir_abs = os.path.normpath(os.path.realpath(base_dir))
|
||||
# Use same resolution strategy for base_dir
|
||||
if allow_nonexistent and not os.path.exists(base_dir):
|
||||
base_dir_abs = os.path.normpath(os.path.abspath(base_dir))
|
||||
else:
|
||||
base_dir_abs = os.path.normpath(os.path.realpath(base_dir))
|
||||
# Check if the normalized path starts with the base directory
|
||||
if not normalized_path.startswith(base_dir_abs + os.sep) and normalized_path != base_dir_abs:
|
||||
raise ValueError(f"Path traversal detected: path is outside base directory")
|
||||
|
||||
Reference in New Issue
Block a user