From 6d66dd0ed8d97a8c434824d04fe6df72c1291b77 Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Mon, 9 Mar 2026 15:45:32 +0000 Subject: [PATCH] Change make_path_safe to is_path_safe and update tests to reflect this --- .../stitching.py | 10 +-- .../utilities.py | 8 +- tests/unit_tests/test_utilities.py | 82 +++++++------------ 3 files changed, 38 insertions(+), 62 deletions(-) diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py index fc764eb4..49916e10 100644 --- a/src/openflexure_microscope_server/stitching.py +++ b/src/openflexure_microscope_server/stitching.py @@ -18,7 +18,7 @@ from pydantic import BaseModel import labthings_fastapi as lt -from openflexure_microscope_server.utilities import make_path_safe +from openflexure_microscope_server.utilities import is_path_safe IS_WINDOWS = os.name == "nt" @@ -68,7 +68,7 @@ def validate_command(cmd: list[str]) -> None: """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``. + executables. All of these should be allowed by ``is_path_safe``. This also checks against a blacklist of forbidden commands for defense-in-depth. @@ -82,8 +82,7 @@ def validate_command(cmd: list[str]) -> None: ) # Ensure characters are safe for a path component - _, error_raised = make_path_safe(element) - if error_raised: + if not is_path_safe(element): raise StitcherValidationError( f"Invalid stitching command: Element '{element}' contains unsafe characters." ) @@ -155,8 +154,7 @@ class BaseStitcher: :raises RuntimeError: if inputs are unsafe. """ - _, error_raised = make_path_safe(self.images_dir) - if error_raised: + if not is_path_safe(self.images_dir): 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 6c73262f..7da53cc6 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -110,7 +110,7 @@ _WINDOWS_RESERVED_NAMES = { } -def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]: +def is_path_safe(unsafe_path_string: str) -> bool: """Check if a file path has any unsafe elements in it. The path is not coerced into a safe form because if we ask @@ -119,8 +119,7 @@ def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]: :param unsafe_path_string: The original path string to sanitise. - :returns: The input string unchanged and a boolean indicating if - any unsafe features were found. + :returns: A boolean indicating if any unsafe features were found. """ # Split by separators first to sanitise components independently @@ -195,14 +194,13 @@ def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]: ) reserved_word = True - warning_raised = ( + return not ( 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 7cc17d7e..bc9076cd 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -7,12 +7,14 @@ import pytest from openflexure_microscope_server.utilities import ( _WINDOWS_RESERVED_NAMES, + is_path_safe, make_name_safe, - make_path_safe, ) PATHS_WITH_DOTS = {"a./b/c.", "path./to/file.", "./path./to/file."} +PATHS_WITH_SPACES = {"path /to /file ", "path/to /file", "path/a long path name /file"} + RESERVED_NAME_PATHS = {"path/CON/file", "path/NUL.txt/file", "C:/AUX/test"} if sys.platform.startswith("win"): @@ -76,32 +78,26 @@ 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(caplog): - """Test basic functionality of make_path_safe.""" +def test_is_path_safe_basic(caplog): + """Test basic functionality of is_path_safe.""" # Note: behavior depends on platform for separators 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" - ) + assert is_path_safe("C:\\path\\to/file") + assert is_path_safe("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" - ) + assert is_path_safe("path/to/file") + assert is_path_safe("a very long path/to/file") # No errors should be raised. These paths are safe. assert len(caplog.records) == 0 @pytest.mark.parametrize("path", UNSAFE_CHARACTER_PATHS) -def test_make_path_safe_unsafe_characters_in_components(caplog, path): +def test_is_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 not is_path_safe(path) assert ( f"{path} contains characters that may be unsafe on this platform." in caplog.messages @@ -109,68 +105,52 @@ def test_make_path_safe_unsafe_characters_in_components(caplog, path): @pytest.mark.parametrize("path", RESERVED_NAME_PATHS) -def test_make_path_safe_reserved_in_components(caplog, path): +def test_is_path_safe_reserved_in_components(caplog, path): """Test reserved names within path components.""" with caplog.at_level(logging.WARNING): - assert make_path_safe(path)[0] == path + assert not is_path_safe(path) assert ( f"{path} contains a reserved system name and may cause issues." in caplog.messages ) -def test_make_path_safe_reserved_name_components(caplog): +def test_is_path_safe_reserved_name_components(caplog): """Test reserved names are okay as part of a larger dir name.""" with caplog.at_level(logging.WARNING): - assert make_path_safe("chilli con carne/recipe")[0] == "chilli con carne/recipe" + assert is_path_safe("chilli con carne/recipe") assert len(caplog.records) == 0 -def test_make_path_safe_trailing_space_in_components(caplog): +@pytest.mark.parametrize("path", PATHS_WITH_SPACES) +def test_is_path_safe_trailing_space_in_components(caplog, path): """Test trailing spaces in path components.""" with caplog.at_level(logging.WARNING): - # Test Windows - cannot have trailingspaces + # Test Windows - cannot have trailing spaces 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 - assert ( - make_path_safe("path/a long path name /file")[0] - == "path/a long path name /file" - ) - assert len(caplog.records) == 3 - assert "path /to /file contains unsafe trailing spaces." in caplog.messages - assert "path/to /file contains unsafe trailing spaces." in caplog.messages - assert ( - "path/a long path name /file contains unsafe trailing spaces." - in caplog.messages - ) + assert not is_path_safe(path) + assert len(caplog.records) == 1 + assert f"{path} 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" - ) + assert is_path_safe(path) # No warnings should be raised assert len(caplog.records) == 0 -def test_make_path_safe_relative(caplog): +def test_is_path_safe_relative(caplog): """Test that relative path components are preserved.""" # 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 is_path_safe("./openflexure/data/") + assert is_path_safe("../openflexure/data/") + assert is_path_safe("../../openflexure/data/") + assert is_path_safe(".") + assert is_path_safe("..") assert len(caplog.records) == 0 - assert make_path_safe("a_bad/../relative_path")[0] == "a_bad/../relative_path" + assert not is_path_safe("a_bad/../relative_path") assert len(caplog.records) == 1 assert ( "File path a_bad/../relative_path may be unsafe due to unexpected relative navigation." @@ -179,10 +159,10 @@ def test_make_path_safe_relative(caplog): @pytest.mark.parametrize("filepath", PATHS_WITH_DOTS) -def test_make_path_safe_dots_warning(caplog, filepath): +def test_is_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 not is_path_safe(filepath) assert ( f"File path {filepath} may be unsafe due to trailing dots." in caplog.messages