diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py index 1f077ec6..0f6eb5e7 100644 --- a/src/openflexure_microscope_server/stitching.py +++ b/src/openflexure_microscope_server/stitching.py @@ -48,18 +48,46 @@ class StitcherValidationError(RuntimeError): """The stitcher received values that it deems unsafe to create a command from.""" +# A list of commands that are forbidden in any part of a generated CLI command. +# This provides defense-in-depth against trying to execute arbitrary shells +# or elevation tools. +_FORBIDDEN_COMMANDS = { + "sudo", + "sh", + "bash", + "python", + "python3", + "perl", + "ruby", + "php", + "nc", + "netcat", + "curl", + "wget", +} + + def validate_command(cmd: list[str]) -> None: - """Validate that the command only characters that are allowed in a path. + """Validate that the command only contains characters that are allowed in a path. The values in the commands should be numbers, commandline flags, paths, and executables. All of these should be allowed by ``make_path_safe``. + This also checks against a blacklist of forbidden commands for defense-in-depth. + :raises StitcherValidationError: if any element in the command is not safe. """ for element in cmd: + # Check against forbidden commands (case-insensitive) + if element.lower() in _FORBIDDEN_COMMANDS: + raise StitcherValidationError( + f"Invalid stiching command: Forbidden element '{element}' detected." + ) + + # Ensure characters are safe for a path component if element != make_path_safe(element): raise StitcherValidationError( - "Invalid stiching command: Contains unsafe characters." + f"Invalid stiching command: Element '{element}' contains unsafe characters." ) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index d6df4df9..c93bf6ed 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -83,6 +83,32 @@ _POSIX_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-/]") # Matches anything that isn't a-z, A-Z, 0-9, _, ., - _NAME_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-]") +# Windows reserved names (case-insensitive) +_WINDOWS_RESERVED_NAMES = { + "CON", + "PRN", + "AUX", + "NUL", + "COM1", + "COM2", + "COM3", + "COM4", + "COM5", + "COM6", + "COM7", + "COM8", + "COM9", + "LPT1", + "LPT2", + "LPT3", + "LPT4", + "LPT5", + "LPT6", + "LPT7", + "LPT8", + "LPT9", +} + def make_path_safe(unsafe_path_string: str) -> str: """Replace unsafe characters in a file path with underscores, preserving separators. @@ -94,16 +120,41 @@ def make_path_safe(unsafe_path_string: str) -> str: including path separators (forward slash on POSIX, and forward slash/backslash/colon on Windows). + Additionally, it sanitises each path component for Windows reserved names and + trailing dots/spaces to ensure maximum cross-platform compatibility. + :param unsafe_path_string: The original path string to sanitise. :returns: A version of the input string safe to use as a file path. """ + # Split by separators first to sanitise components independently + components = re.split(r"([/\\])", unsafe_path_string) + sanitised_components = [] + unsafe_character_pattern = ( _WINDOWS_UNSAFE_PATTERN if sys.platform.startswith("win") else _POSIX_UNSAFE_PATTERN ) - return unsafe_character_pattern.sub("_", unsafe_path_string) + + for component in components: + if component in ("/", "\\"): + sanitised_components.append(component) + elif component: + # 1. Strip trailing dots and spaces + # 2. Apply unsafe character regex + # 3. Check for reserved names + sanitised = component.rstrip(". ") + if not sanitised: + sanitised = "_" + else: + sanitised = unsafe_character_pattern.sub("_", sanitised) + + sanitised_components.append(_sanitise_reserved(sanitised)) + else: + sanitised_components.append(component) + + return "".join(sanitised_components) def make_name_safe(unsafe_name_string: str) -> str: @@ -112,11 +163,37 @@ def make_name_safe(unsafe_name_string: str) -> str: This excludes all path separators, ensuring the result is safe to use as a standalone filename or identifier component. + This also handles Windows reserved names and trailing dots/spaces. + :param unsafe_name_string: The original name string to sanitise. :returns: A version of the input string safe to use as a file name or identifier. """ - return _NAME_UNSAFE_PATTERN.sub("_", unsafe_name_string) + # 1. Strip trailing dots and spaces + # 2. Apply unsafe character regex + # 3. Check for reserved names + name = unsafe_name_string.rstrip(". ") + if not name: + return "_" + + name = _NAME_UNSAFE_PATTERN.sub("_", name) + return _sanitise_reserved(name) + + +def _sanitise_reserved(name: str) -> str: + """Check a name against Windows reserved names. + + :param name: The component to check. + :returns: The sanitised component. + """ + # Check for Windows reserved names. + # These are reserved even with an extension (e.g. NUL.txt). + # We check the part before the first dot. + base_name = name.split(".", maxsplit=1)[0].upper() + if base_name in _WINDOWS_RESERVED_NAMES: + return f"{name}_" + + return name class VersionData(BaseModel): diff --git a/tests/unit_tests/test_stitching_validation.py b/tests/unit_tests/test_stitching_validation.py new file mode 100644 index 00000000..0223847d --- /dev/null +++ b/tests/unit_tests/test_stitching_validation.py @@ -0,0 +1,49 @@ +"""Unit tests for stitching command validation.""" + +import pytest + +from openflexure_microscope_server.stitching import ( + StitcherValidationError, + validate_command, +) + + +def test_validate_command_success(): + """Test valid commands pass validation.""" + validate_command(["openflexure-stitch", "--stitching_mode", "all", "path/to/scan"]) + validate_command(["--resize", "0.5", "8192"]) + + +def test_validate_command_forbidden(): + """Test forbidden commands raise error.""" + with pytest.raises( + StitcherValidationError, match="Forbidden element 'sudo' detected" + ): + validate_command(["sudo", "rm", "-rf", "/"]) + + with pytest.raises( + StitcherValidationError, match="Forbidden element 'python' detected" + ): + validate_command(["python", "-c", "print('hello')"]) + + with pytest.raises( + StitcherValidationError, match="Forbidden element 'sh' detected" + ): + validate_command(["sh", "-c", "whoami"]) + + +def test_validate_command_unsafe_chars(): + """Test elements with unsafe characters raise error.""" + with pytest.raises(StitcherValidationError, match="contains unsafe characters"): + validate_command(["openflexure-stitch", "path;rm -rf /"]) + + with pytest.raises(StitcherValidationError, match="contains unsafe characters"): + validate_command(["openflexure-stitch", "path&echo hello"]) + + +def test_validate_command_case_insensitive(): + """Test forbidden command check is case-insensitive.""" + with pytest.raises( + StitcherValidationError, match="Forbidden element 'SUDO' detected" + ): + validate_command(["SUDO", "ls"]) diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py new file mode 100644 index 00000000..a9972a44 --- /dev/null +++ b/tests/unit_tests/test_utilities.py @@ -0,0 +1,71 @@ +"""Unit tests for utility functions.""" + +import sys + +from openflexure_microscope_server.utilities import make_name_safe, make_path_safe + + +def test_make_name_safe_basic(): + """Test basic functionality of make_name_safe.""" + assert make_name_safe("normal_name") == "normal_name" + assert make_name_safe("name with spaces") == "name_with_spaces" + assert make_name_safe("name/with/slashes") == "name_with_slashes" + assert make_name_safe("name\\with\\backslashes") == "name_with_backslashes" + + +def test_make_name_safe_trailing_chars(): + """Test that trailing dots and spaces are removed.""" + assert make_name_safe("name.") == "name" + assert make_name_safe("name ") == "name" + assert make_name_safe("name. ") == "name" + assert make_name_safe("name .") == "name" + assert make_name_safe(".") == "_" + assert make_name_safe(" ") == "_" + assert make_name_safe(". ") == "_" + + +def test_make_name_safe_reserved_names(): + """Test Windows reserved names.""" + assert make_name_safe("CON") == "CON_" + assert make_name_safe("con") == "con_" + assert make_name_safe("NUL.txt") == "NUL.txt_" + assert make_name_safe("COM1") == "COM1_" + assert make_name_safe("LPT9.tar.gz") == "LPT9.tar.gz_" + # Check that names STARTING with reserved names but not followed by . or end are safe + assert make_name_safe("CONSTANT") == "CONSTANT" + assert make_name_safe("CON2") == "CON2" + + +def test_make_path_safe_basic(): + """Test basic functionality of make_path_safe.""" + # Note: behavior depends on platform for separators + if sys.platform.startswith("win"): + assert make_path_safe("C:\\path\\to/file") == "C:\\path\\to/file" + else: + # On POSIX, \ is not a separator and might be replaced by _ if not in allowed pattern + # The regex for POSIX is [^a-zA-Z0-9_.\-/] + # And : is also not allowed + assert make_path_safe("path/to/file") == "path/to/file" + + +def test_make_path_safe_reserved_in_components(): + """Test reserved names within path components.""" + assert make_path_safe("path/CON/file") == "path/CON_/file" + assert make_path_safe("path/NUL.txt/file") == "path/NUL.txt_/file" + + if sys.platform.startswith("win"): + assert make_path_safe("C:/AUX/test") == "C:/AUX_/test" + else: + assert make_path_safe("C:/AUX/test") == "C_/AUX_/test" + + +def test_make_path_safe_trailing_in_components(): + """Test trailing chars in path components.""" + assert make_path_safe("path /to. /file ") == "path/to/file" + assert make_path_safe("path./to /file.") == "path/to/file" + + +def test_make_path_safe_separators(): + """Test that separators are preserved and components sanitised.""" + assert make_path_safe("a/b\\c") == "a/b\\c" + assert make_path_safe("a./b /c.") == "a/b/c"