diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py index 393eb32e..fc764eb4 100644 --- a/src/openflexure_microscope_server/stitching.py +++ b/src/openflexure_microscope_server/stitching.py @@ -78,13 +78,14 @@ def validate_command(cmd: list[str]) -> None: # Check against forbidden commands (case-insensitive) if element.lower() in FORBIDDEN_COMMANDS: raise StitcherValidationError( - f"Invalid stiching command: Forbidden element '{element}' detected." + f"Invalid stitching command: Forbidden element '{element}' detected." ) # Ensure characters are safe for a path component - if element != make_path_safe(element): + _, error_raised = make_path_safe(element) + if error_raised: raise StitcherValidationError( - f"Invalid stiching command: Element '{element}' contains unsafe characters." + f"Invalid stitching command: Element '{element}' contains unsafe characters." ) @@ -154,7 +155,8 @@ class BaseStitcher: :raises RuntimeError: if inputs are unsafe. """ - if self.images_dir != make_path_safe(self.images_dir): + _, error_raised = make_path_safe(self.images_dir) + if error_raised: raise StitcherValidationError( "Invalid directory path: Contains unsafe characters." ) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index a13cf19d..11803368 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -77,7 +77,7 @@ def requires_lock( # Compiled regular expressions for unsafe characters # Matches anything that isn't a-z, A-Z, 0-9, _, ., -, :, /, \ -_WINDOWS_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-:/\\]") +_WINDOWS_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-:/\ \\]") # Matches anything that isn't a-z, A-Z, 0-9, _, ., -, \ _POSIX_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-/]") # Matches anything that isn't a-z, A-Z, 0-9, _, ., - @@ -110,26 +110,21 @@ _WINDOWS_RESERVED_NAMES = { } -def make_path_safe(unsafe_path_string: str) -> str: - """Replace unsafe characters in a file path with underscores, preserving separators. +def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]: + """Check if a file path has any unsafe elements in it, such as + unsafe characters or reserved names for its OS. - This can be used to check that user inputs have not been concatenated into an - unsafe filepath. - - This ensures compatibility across platforms by preserving only valid characters, - 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. + The path is not coerced into a safe form because if we ask + for a file to be written to a location we shouldn't be changing + that location as we have no "consent" to write to this other location. :param unsafe_path_string: The original path string to sanitise. - :returns: A version of the input string safe to use as a file path. + :returns: The input string unchanged and a boolean indicating if + any unsafe features were found. """ # Split by separators first to sanitise components independently components = re.split(r"([/\\])", unsafe_path_string) - sanitised_components = [] unsafe_character_pattern = ( _WINDOWS_UNSAFE_PATTERN @@ -137,40 +132,52 @@ def make_path_safe(unsafe_path_string: str) -> str: else _POSIX_UNSAFE_PATTERN ) + # Ensure each warning only gets logged once per path + unsafe_relative_path = False + trailing_dots = False + trailing_whitespace = False + unsafe_characters = False + reserved_word = False + for i, component in enumerate(components): - if component in ("/", "\\"): - sanitised_components.append(component) - elif component in (".", ".."): - # Only allow '.' and '..' if they are the very first - # elements and the next element is a separator. - # This allows for relative paths like "./file" or "../file" - # but not "file./file" or "file/../file" - is_first = i == 0 - next_to_sep = (i + 1 < len(components)) and ( - components[i + 1] in ("/", "\\") - ) + # Skip separators and empty strings + if component in ("/", "\\") or not component: + continue - if is_first and next_to_sep: - sanitised_components.append(component) - else: - sanitised_components.append("_") - 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) + # 1. Check for relative paths in the wrong place + if component in (".", "..") and not unsafe_relative_path: + is_first = (i == 0) + is_second = (i == 2 and components[i-1] in ("/", "\\") and components[i-2] == "..") + if not (is_first or is_second): + LOGGER.warning(f"File path {unsafe_path_string} may be unsafe due to unexpected relative navigation.") + unsafe_relative_path = True + continue - sanitised_components.append( - _sanitise_reserved(sanitised, is_filename=False) - ) - else: - sanitised_components.append(component) + # 2. Check for trailing dots in path + if "." in component and i != len(components) - 1 and not trailing_dots: + # Check for trailing dots in the file path before the file name + # e.g.: foo/bar./file is bad, but foo/bar.file.py is fine + LOGGER.warning(f'File path {unsafe_path_string} may be unsafe due to trailing dots.') + trailing_dots = True - return "".join(sanitised_components) + # Check for trailing spaces - Windows specific + if sys.platform.startswith("win") and component.endswith(" ") and not trailing_whitespace: + LOGGER.warning(f"{unsafe_path_string} contains unsafe trailing spaces.") + trailing_whitespace = True + + # 3. Check for unsafe characters (Regex) + if unsafe_character_pattern.search(component) and not unsafe_characters: + LOGGER.warning(f"{unsafe_path_string} contains characters that may be unsafe on this platform.") + unsafe_characters = True + + # 4. Check for Reserved Names (e.g., CON, PRN, LPT1) + # Assuming _sanitise_reserved returns a different string if it's a reserved name + if _sanitise_reserved(component, is_filename=False) != component and not reserved_word: + LOGGER.warning(f"{unsafe_path_string} contains a reserved system name and may cause issues.") + reserved_word = True + + warning_raised = unsafe_relative_path or trailing_dots or trailing_whitespace or unsafe_characters or reserved_word + return unsafe_path_string, warning_raised def make_name_safe(unsafe_name_string: str) -> str: diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index dbff78e4..c6d32431 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -1,15 +1,42 @@ """Unit tests for utility functions.""" import sys - +import logging import pytest from openflexure_microscope_server.utilities import ( _WINDOWS_RESERVED_NAMES, make_name_safe, - make_path_safe, + make_path_safe ) +PATHS_WITH_DOTS = { + "a./b/c.", + "path./to/file.", + "./path./to/file." +} + +RESERVED_NAME_PATHS = { + "path/CON/file", + "path/NUL.txt/file", + "C:/AUX/test" +} + +if sys.platform.startswith("win"): + UNSAFE_CHARACTER_PATHS = { + "C:/path/asterisk/data*.csv", + "data/files/read_me_first!.txt", + "logs/system.log", + "projects/alpha#1/notes.txt", + } +else: + UNSAFE_CHARACTER_PATHS = { + "/var/log/app:backup.log", + "/etc/configs/network-settings\\v1", + "/tmp/file_with_@_symbol.txt", + "/home/user/script(1).py" + } + def test_make_name_safe_basic(): """Test basic functionality of make_name_safe.""" @@ -56,57 +83,81 @@ def test_make_name_safe_reserved_names_false_positives(): assert make_name_safe("CHILLI CON CARNE") == "chilli_con_carne" -def test_make_path_safe_basic(): +def test_make_path_safe_basic(caplog): """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" + with caplog.at_level(logging.WARNING): + if sys.platform.startswith("win"): + assert make_path_safe("C:\\path\\to/file")[0] == "C:\\path\\to/file" + assert make_path_safe("C:\\a very long path\\to/file")[0] == "C:\\a very long path\\to/file" + else: + assert make_path_safe("path/to/file")[0] == "path/to/file" + assert make_path_safe("a very long path/to/file")[0] == "a very long path/to/file" + + # No errors should be raised. These paths are safe. + assert len(caplog.records) == 0 -def test_make_path_safe_reserved_in_components(): +@pytest.mark.parametrize("path", UNSAFE_CHARACTER_PATHS) +def test_make_path_safe_unsafe_characters_in_components(caplog, path): + """Test unsafe characters within path components.""" + with caplog.at_level(logging.WARNING): + assert make_path_safe(path)[0] == path + assert f"{path} contains characters that may be unsafe on this platform." + + +@pytest.mark.parametrize("path", RESERVED_NAME_PATHS) +def test_make_path_safe_reserved_in_components(caplog, path): """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" + with caplog.at_level(logging.WARNING): + assert make_path_safe(path)[0] == path + assert f"{path} contains a reserved system name and may cause issues." -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" - - # Allow dots at the start of the path - # As this is common for hidden files on POSIX and relative paths - # And is the configuration in the manual and simulation config - # json files. - assert make_path_safe("./path/to/file") == "./path/to/file" - assert make_path_safe("./path./to /file.") == "./path/to/file" - assert make_path_safe("./openflexure/data/") == "./openflexure/data/" +def test_make_path_safe_trailing_space_in_components(caplog): + """Test trailing spaces in path components.""" + with caplog.at_level(logging.WARNING): + # Test Windows - cannot have trailingspaces + if sys.platform.startswith("win"): + assert make_path_safe("path /to /file ")[0] == "path /to /file " + assert make_path_safe("path/to /file")[0] == "path/to /file" + # Check directories can have spaces in them but not at the end + # TODO - dont want underscores, want to keep interim spaces + assert make_path_safe("path/a long path name /file")[0] == "path/a long path name /file" + assert len(caplog.records) == 3 + assert f"path /to /file contains unsafe trailing spaces." in caplog.messages + assert f"path/to /file contains unsafe trailing spaces." in caplog.messages + assert f"path/a long path name /file contains unsafe trailing spaces." in caplog.messages + else: + # Trailing spaces allows in POSIX systems + assert make_path_safe("path /to /file ")[0] == "path /to /file " + assert make_path_safe("path/to /file")[0] == "path/to /file" + # Check directories can have spaces in them but not at the end + assert make_path_safe("path/a long path name /file")[0] == "path/a long path name file" + # No warnings should be raised + assert len(caplog.records) == 0 -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" - - -def test_make_path_safe_relative(): +def test_make_path_safe_relative(caplog): """Test that relative path components are preserved.""" - # We only allow relative paths with one dot, and at the beginning of the path, - # to avoid issues with paths like "file./file" or "file../file. - assert make_path_safe("./openflexure/data/") == "./openflexure/data/" - assert make_path_safe("../openflexure/data/") == "../openflexure/data/" + # Relative imports ./foo, ../foo or ../../foo are allowed. + with caplog.at_level(logging.WARNING): + assert make_path_safe("./openflexure/data/")[0] == "./openflexure/data/" + assert make_path_safe("../openflexure/data/")[0] == "../openflexure/data/" + assert make_path_safe("../../openflexure/data/")[0] == "../../openflexure/data/" + assert make_path_safe(".")[0] == "." + assert make_path_safe("..")[0] == ".." - assert make_path_safe("path/./to/file") == "path/_/to/file" - assert make_path_safe("path/../to/file") == "path/_/to/file" - assert make_path_safe(".") == "_" - assert make_path_safe("..") == "_" + assert len(caplog.records) == 0 + + assert make_path_safe("a_bad/../relative_path")[0] == "a_bad/../relative_path" + assert len(caplog.records) == 1 + assert(f"File path a_bad/../relative_path may be unsafe due to unexpected relative navigation.") + + +@pytest.mark.parametrize("filepath", PATHS_WITH_DOTS) +def test_make_path_safe_dots_warning(caplog, filepath): + """Test that a user is warned about trailing dots in their file paths. The paths should remain unchanged.""" + with caplog.at_level(logging.WARNING): + assert make_path_safe(filepath)[0] == filepath + assert f"File path {filepath} may be unsafe due to trailing dots." in caplog.messages