From 98e71207c5f6bcba4a7a1122da95188ffc84be95 Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Thu, 5 Mar 2026 13:03:17 +0000 Subject: [PATCH 01/20] Add forbidden windows commands to stitching and utilities to check for file name sanitation. Unit tests also added --- .../stitching.py | 32 +++++++- .../utilities.py | 81 ++++++++++++++++++- tests/unit_tests/test_stitching_validation.py | 49 +++++++++++ tests/unit_tests/test_utilities.py | 71 ++++++++++++++++ 4 files changed, 229 insertions(+), 4 deletions(-) create mode 100644 tests/unit_tests/test_stitching_validation.py create mode 100644 tests/unit_tests/test_utilities.py 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" From 48f6e1b369ceb75e502478fcfeb59ff8df17032f Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Thu, 5 Mar 2026 13:35:21 +0000 Subject: [PATCH 02/20] Allow python and python3 as stitching commands. They are allowed. Move stitching tests into one file --- .../stitching.py | 2 - tests/unit_tests/test_stitching.py | 23 +++++++++ tests/unit_tests/test_stitching_validation.py | 49 ------------------- 3 files changed, 23 insertions(+), 51 deletions(-) delete mode 100644 tests/unit_tests/test_stitching_validation.py diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py index 0f6eb5e7..bf5da43c 100644 --- a/src/openflexure_microscope_server/stitching.py +++ b/src/openflexure_microscope_server/stitching.py @@ -55,8 +55,6 @@ _FORBIDDEN_COMMANDS = { "sudo", "sh", "bash", - "python", - "python3", "perl", "ruby", "php", diff --git a/tests/unit_tests/test_stitching.py b/tests/unit_tests/test_stitching.py index 349d975c..5ae8842e 100644 --- a/tests/unit_tests/test_stitching.py +++ b/tests/unit_tests/test_stitching.py @@ -21,6 +21,7 @@ from openflexure_microscope_server.stitching import ( PreviewStitcher, StitcherValidationError, StitchingSettings, + validate_command, ) from ..shared_utils.lt_test_utils import LabThingsTestEnv @@ -32,6 +33,28 @@ THIS_DIR: str = os.path.dirname(os.path.realpath(__file__)) MOCK_STITCHER: str = os.path.join(THIS_DIR, "mock_stitching", "mock-stitch.py") +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 'sh' detected"): + validate_command(["sh", "-c", "whoami"]) + + with pytest.raises( + StitcherValidationError, match="Forbidden element 'SUDO' detected" + ): + validate_command(["SUDO", "ls"]) + + def test_base_stitcher(): """Test the logic in BaseStitcher. diff --git a/tests/unit_tests/test_stitching_validation.py b/tests/unit_tests/test_stitching_validation.py deleted file mode 100644 index 0223847d..00000000 --- a/tests/unit_tests/test_stitching_validation.py +++ /dev/null @@ -1,49 +0,0 @@ -"""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"]) From fa9ea778127f338a973854d75c26939a2b787708 Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Thu, 5 Mar 2026 13:42:55 +0000 Subject: [PATCH 03/20] Ruff check --- tests/unit_tests/test_stitching.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit_tests/test_stitching.py b/tests/unit_tests/test_stitching.py index 5ae8842e..34d7fe7d 100644 --- a/tests/unit_tests/test_stitching.py +++ b/tests/unit_tests/test_stitching.py @@ -46,7 +46,9 @@ def test_validate_command_forbidden(): ): validate_command(["sudo", "rm", "-rf", "/"]) - with pytest.raises(StitcherValidationError, match="Forbidden element 'sh' detected"): + with pytest.raises( + StitcherValidationError, match="Forbidden element 'sh' detected" + ): validate_command(["sh", "-c", "whoami"]) with pytest.raises( From 35efba19b847ec8574ab619b6e2f6c86c662ec6d Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Thu, 5 Mar 2026 13:55:05 +0000 Subject: [PATCH 04/20] Use pytest parametrize for better coverage of forbidden commands and reserved names --- .../stitching.py | 35 +++++++++---------- tests/unit_tests/test_stitching.py | 19 +++++----- tests/unit_tests/test_utilities.py | 31 +++++++++++----- 3 files changed, 49 insertions(+), 36 deletions(-) diff --git a/src/openflexure_microscope_server/stitching.py b/src/openflexure_microscope_server/stitching.py index bf5da43c..393eb32e 100644 --- a/src/openflexure_microscope_server/stitching.py +++ b/src/openflexure_microscope_server/stitching.py @@ -29,6 +29,22 @@ STITCH_TILE_SIZE = 8192 DEFAULT_OVERLAP = 0.1 DEFAULT_RESIZE = 0.5 +# 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", + "perl", + "ruby", + "php", + "nc", + "netcat", + "curl", + "wget", +} + class StitchingSettings(BaseModel): """The data needed to stitch a scan.""" @@ -48,23 +64,6 @@ 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", - "perl", - "ruby", - "php", - "nc", - "netcat", - "curl", - "wget", -} - - def validate_command(cmd: list[str]) -> None: """Validate that the command only contains characters that are allowed in a path. @@ -77,7 +76,7 @@ def validate_command(cmd: list[str]) -> None: """ for element in cmd: # Check against forbidden commands (case-insensitive) - if element.lower() in _FORBIDDEN_COMMANDS: + if element.lower() in FORBIDDEN_COMMANDS: raise StitcherValidationError( f"Invalid stiching command: Forbidden element '{element}' detected." ) diff --git a/tests/unit_tests/test_stitching.py b/tests/unit_tests/test_stitching.py index 34d7fe7d..ca5b9a08 100644 --- a/tests/unit_tests/test_stitching.py +++ b/tests/unit_tests/test_stitching.py @@ -16,6 +16,7 @@ from pydantic import BaseModel import labthings_fastapi as lt from openflexure_microscope_server.stitching import ( + FORBIDDEN_COMMANDS, BaseStitcher, FinalStitcher, PreviewStitcher, @@ -39,22 +40,20 @@ def test_validate_command_success(): validate_command(["--resize", "0.5", "8192"]) -def test_validate_command_forbidden(): +@pytest.mark.parametrize("cmd_name", FORBIDDEN_COMMANDS) +def test_validate_command_forbidden(cmd_name): """Test forbidden commands raise error.""" + # As the main command with pytest.raises( - StitcherValidationError, match="Forbidden element 'sudo' detected" + StitcherValidationError, match=f"Forbidden element '{cmd_name}' detected" ): - validate_command(["sudo", "rm", "-rf", "/"]) + validate_command([cmd_name, "some_arg"]) + # As an argument (case-insensitive) with pytest.raises( - StitcherValidationError, match="Forbidden element 'sh' detected" + StitcherValidationError, match=f"Forbidden element '{cmd_name.upper()}' detected" ): - validate_command(["sh", "-c", "whoami"]) - - with pytest.raises( - StitcherValidationError, match="Forbidden element 'SUDO' detected" - ): - validate_command(["SUDO", "ls"]) + validate_command(["safe-command", cmd_name.upper()]) def test_base_stitcher(): diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index a9972a44..dc8e3ae1 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -2,10 +2,17 @@ import sys -from openflexure_microscope_server.utilities import make_name_safe, make_path_safe +import pytest + +from openflexure_microscope_server.utilities import ( + _WINDOWS_RESERVED_NAMES, + 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" @@ -24,16 +31,24 @@ def test_make_name_safe_trailing_chars(): assert make_name_safe(". ") == "_" -def test_make_name_safe_reserved_names(): +@pytest.mark.parametrize("name", _WINDOWS_RESERVED_NAMES) +def test_make_name_safe_reserved_names(name): """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 + # Base name should be sanitized + assert make_name_safe(name) == f"{name}_" + # Case-insensitive + assert make_name_safe(name.lower()) == f"{name.lower()}_" + # With extension + assert make_name_safe(f"{name}.txt") == f"{name}.txt_" + # Multiple extensions + assert make_name_safe(f"{name}.tar.gz") == f"{name}.tar.gz_" + + +def test_make_name_safe_reserved_names_false_positives(): + """Test that names containing but not equal to reserved names are safe.""" assert make_name_safe("CONSTANT") == "CONSTANT" assert make_name_safe("CON2") == "CON2" + assert make_name_safe("ICON") == "ICON" def test_make_path_safe_basic(): From f52ce85b3e19b3638c60145de797351dfd19abdb Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Thu, 5 Mar 2026 14:04:33 +0000 Subject: [PATCH 05/20] Ruff formatting --- tests/unit_tests/test_stitching.py | 3 ++- tests/unit_tests/test_utilities.py | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit_tests/test_stitching.py b/tests/unit_tests/test_stitching.py index ca5b9a08..ead2276b 100644 --- a/tests/unit_tests/test_stitching.py +++ b/tests/unit_tests/test_stitching.py @@ -51,7 +51,8 @@ def test_validate_command_forbidden(cmd_name): # As an argument (case-insensitive) with pytest.raises( - StitcherValidationError, match=f"Forbidden element '{cmd_name.upper()}' detected" + StitcherValidationError, + match=f"Forbidden element '{cmd_name.upper()}' detected", ): validate_command(["safe-command", cmd_name.upper()]) diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index dc8e3ae1..bd7f4ce3 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -12,7 +12,6 @@ from openflexure_microscope_server.utilities import ( 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" From 7a17c92d3ee073f34c599e6d425d9e1a75ffe82f Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Thu, 5 Mar 2026 15:13:42 +0000 Subject: [PATCH 06/20] Apply suggestions from code review of branch 523-improve-validation-checks Co-authored-by: Julian Stirling --- tests/unit_tests/test_utilities.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index bd7f4ce3..73b08a1f 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -48,6 +48,7 @@ def test_make_name_safe_reserved_names_false_positives(): assert make_name_safe("CONSTANT") == "CONSTANT" assert make_name_safe("CON2") == "CON2" assert make_name_safe("ICON") == "ICON" + assert make_name_safe("CHILLI CON CARNE") == "CHILLI CON CARNE" def test_make_path_safe_basic(): From 713df248ffc1eb8d2d5df0255488ff17895f3577 Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Fri, 6 Mar 2026 09:53:41 +0000 Subject: [PATCH 07/20] Add underscores to fix unit test --- tests/unit_tests/test_utilities.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index 73b08a1f..52f17c3f 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -48,7 +48,7 @@ def test_make_name_safe_reserved_names_false_positives(): assert make_name_safe("CONSTANT") == "CONSTANT" assert make_name_safe("CON2") == "CON2" assert make_name_safe("ICON") == "ICON" - assert make_name_safe("CHILLI CON CARNE") == "CHILLI CON CARNE" + assert make_name_safe("CHILLI CON CARNE") == "CHILLI_CON_CARNE" def test_make_path_safe_basic(): From e5429d6cda89f1ec2bce9454d93d48d1367f906c Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Fri, 6 Mar 2026 14:07:07 +0000 Subject: [PATCH 08/20] Change make_path_safe to allow single dot relative imports, but no dots anywhere else. Added new unit tests for this. --- .../utilities.py | 16 +++++++++++++- tests/unit_tests/test_utilities.py | 22 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index c93bf6ed..c4b328d4 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -137,9 +137,23 @@ def make_path_safe(unsafe_path_string: str) -> str: else _POSIX_UNSAFE_PATTERN ) - for component in components: + for i, component in enumerate(components): if component in ("/", "\\"): sanitised_components.append(component) + elif component == ".": + # Only allow '.' if its the very first + # element and the next element is a separator. + # This allows for relative paths like "./file" + # but not "file./file" + is_first = i == 0 + next_to_sep = (i + 1 < len(components)) and ( + components[i + 1] in ("/", "\\") + ) + + 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 diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index 52f17c3f..84c25dc3 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -79,8 +79,30 @@ def test_make_path_safe_trailing_in_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_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(): + """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/" + 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("..") == "_" From 4395d4001c265ff9e040da18d34fed8d9b16fd73 Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Fri, 6 Mar 2026 14:13:00 +0000 Subject: [PATCH 09/20] Allow relative imports with two dots at start of path. Update unit tests. --- src/openflexure_microscope_server/utilities.py | 10 +++++----- tests/unit_tests/test_utilities.py | 9 ++++----- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index c4b328d4..5fa1db8f 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -140,11 +140,11 @@ def make_path_safe(unsafe_path_string: str) -> str: for i, component in enumerate(components): if component in ("/", "\\"): sanitised_components.append(component) - elif component == ".": - # Only allow '.' if its the very first - # element and the next element is a separator. - # This allows for relative paths like "./file" - # but not "file./file" + 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 ("/", "\\") diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index 84c25dc3..52b94fad 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -95,13 +95,12 @@ def test_make_path_safe_separators(): def test_make_path_safe_relative(): - """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 - """ + """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/" - assert make_path_safe("../openflexure/data/") == "_/openflexure/data/" assert make_path_safe("path/./to/file") == "path/_/to/file" assert make_path_safe("path/../to/file") == "path/_/to/file" assert make_path_safe(".") == "_" From 35f7adcf07207dd0328f92c7a44763acf31922cf Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Fri, 6 Mar 2026 14:16:42 +0000 Subject: [PATCH 10/20] Codespell --- src/openflexure_microscope_server/utilities.py | 2 +- tests/unit_tests/test_config_utilities.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index 5fa1db8f..2f6c90a4 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -382,7 +382,7 @@ def merge_patch( :param target: The target object :param patch: The patch to be applied - :param enforce_dict: Boolean, set True enfoces that the target and patch are both + :param enforce_dict: Boolean, set True enforces that the target and patch are both dictionaries. """ if enforce_dict and not (isinstance(target, dict) and isinstance(patch, dict)): diff --git a/tests/unit_tests/test_config_utilities.py b/tests/unit_tests/test_config_utilities.py index cd72b7d0..4fb4b91c 100644 --- a/tests/unit_tests/test_config_utilities.py +++ b/tests/unit_tests/test_config_utilities.py @@ -37,7 +37,7 @@ from openflexure_microscope_server import utilities def test_merge_patch(target, patch, outcome, err_if_enforce): """Check that merge_patch returns the expected outcome for each target and patch. - These test cases are specified in Appedix A of IETF RFC 7396. + These test cases are specified in Appendix A of IETF RFC 7396. """ assert utilities.merge_patch(target, patch) == outcome From 2c2f97cb5627b760a014f9340e9b6626b5b76695 Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Fri, 6 Mar 2026 15:37:01 +0000 Subject: [PATCH 11/20] Force file names to be lower case to avoid Windows issues around case sensitivity when checking if a file already exists. Path names can be uppercase as these will be defined on a users system and pre-exist. Files are created by us so we can force sanitation --- .../utilities.py | 18 +++++++---- tests/unit_tests/test_scan_directories.py | 30 +++++++++++++++++++ tests/unit_tests/test_utilities.py | 19 +++++++----- 3 files changed, 55 insertions(+), 12 deletions(-) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index 2f6c90a4..a13cf19d 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -164,7 +164,9 @@ def make_path_safe(unsafe_path_string: str) -> str: else: sanitised = unsafe_character_pattern.sub("_", sanitised) - sanitised_components.append(_sanitise_reserved(sanitised)) + sanitised_components.append( + _sanitise_reserved(sanitised, is_filename=False) + ) else: sanitised_components.append(component) @@ -194,20 +196,26 @@ def make_name_safe(unsafe_name_string: str) -> str: return _sanitise_reserved(name) -def _sanitise_reserved(name: str) -> str: +def _sanitise_reserved(name: str, is_filename: bool = True) -> str: """Check a name against Windows reserved names. :param name: The component to check. - :returns: The sanitised component. + :param is_filename: Whether the component is a filename. + If True, names will be forced into lowercase. + :returns: The sanitised component, in lower case. """ # 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 f"{name.lower()}_" if is_filename else f"{name}_" - return name + # We force names to be lowercase to avoid issues on + # Windows where files with the same name + # but different case can cause problems when checking if + # a file already exists. + return name.lower() if is_filename else name class VersionData(BaseModel): diff --git a/tests/unit_tests/test_scan_directories.py b/tests/unit_tests/test_scan_directories.py index 603587a3..80121a32 100644 --- a/tests/unit_tests/test_scan_directories.py +++ b/tests/unit_tests/test_scan_directories.py @@ -247,6 +247,36 @@ def test_scan_sequence_and_listing(caplog): assert len(caplog.records) == 0 +def test_scan_sequence_case_sensitive(): + """Check created scans are added in order and listed correctly, even when cases are different.""" + _clear_scan_dir() + scan_dir_manager = ScanDirectoryManager(BASE_SCAN_DIR) + + # Create some scan data and mark it as successful to get an end date. + scan_data = fake_active_scan_data() + scan_data.set_final_data(result="Success") + # Make 4 scans + scan_dir = scan_dir_manager.new_scan_dir("fake_scan") + scan_dir.save_scan_data(scan_data) + scan_dir = scan_dir_manager.new_scan_dir("FAKE_SCAN") + scan_dir.save_scan_data(scan_data) + scan_dir = scan_dir_manager.new_scan_dir("Fake_Scan") + scan_dir.save_scan_data(scan_data) + scan_dir = scan_dir_manager.new_scan_dir("FAKE_scan") + scan_dir.save_scan_data(scan_data) + scan_dir = scan_dir_manager.new_scan_dir("fAkE_sCaN") + scan_dir.save_scan_data(scan_data) + + # Check they exist and are numbered sequentially + all_scans = scan_dir_manager.all_scans + assert_unique_of_length(all_scans, 5) + assert "fake_scan_0001" in all_scans + assert "fake_scan_0002" in all_scans + assert "fake_scan_0003" in all_scans + assert "fake_scan_0004" in all_scans + assert "fake_scan_0005" in all_scans + + def test_scan_name_non_sequential(): """Check new scan has the correct name if the directories are not sequential.""" _clear_scan_dir() diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index 52b94fad..dbff78e4 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -17,6 +17,7 @@ def test_make_name_safe_basic(): 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" + assert make_name_safe("sPoNgEmoCk") == "spongemock" def test_make_name_safe_trailing_chars(): @@ -34,21 +35,25 @@ def test_make_name_safe_trailing_chars(): def test_make_name_safe_reserved_names(name): """Test Windows reserved names.""" # Base name should be sanitized - assert make_name_safe(name) == f"{name}_" + assert make_name_safe(name) == f"{name.lower()}_" # Case-insensitive assert make_name_safe(name.lower()) == f"{name.lower()}_" # With extension - assert make_name_safe(f"{name}.txt") == f"{name}.txt_" + assert make_name_safe(f"{name}.txt") == f"{name.lower()}.txt_" # Multiple extensions - assert make_name_safe(f"{name}.tar.gz") == f"{name}.tar.gz_" + assert make_name_safe(f"{name}.tar.gz") == f"{name.lower()}.tar.gz_" def test_make_name_safe_reserved_names_false_positives(): """Test that names containing but not equal to reserved names are safe.""" - assert make_name_safe("CONSTANT") == "CONSTANT" - assert make_name_safe("CON2") == "CON2" - assert make_name_safe("ICON") == "ICON" - assert make_name_safe("CHILLI CON CARNE") == "CHILLI_CON_CARNE" + assert make_name_safe("CONSTANT") == "constant" + assert make_name_safe("CON2") == "con2" + assert make_name_safe("ICON") == "icon" + assert make_name_safe("icon") == "icon" + assert make_name_safe("iCon") == "icon" + assert make_name_safe("iCOn") == "icon" + assert make_name_safe("icOn") == "icon" + assert make_name_safe("CHILLI CON CARNE") == "chilli_con_carne" def test_make_path_safe_basic(): From 349485d43bf655b685e5be3e68b89700172643f1 Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Fri, 6 Mar 2026 15:52:50 +0000 Subject: [PATCH 12/20] Remove multi-underscores from allowed names --- .../scan_directories.py | 3 ++ tests/unit_tests/test_scan_directories.py | 29 ++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/openflexure_microscope_server/scan_directories.py b/src/openflexure_microscope_server/scan_directories.py index 295c36dd..4f2e5215 100644 --- a/src/openflexure_microscope_server/scan_directories.py +++ b/src/openflexure_microscope_server/scan_directories.py @@ -353,6 +353,9 @@ class ScanDirectoryManager: For more explanation on the scan naming see `new_scan_dir` """ scan_name = make_name_safe(scan_name) + + # Strip all trailing underscores from the base name + scan_name = scan_name.strip("_") # A regex with the scan name and a group for the numbers scan_regex = re.compile( "^" + scan_name + "_([0-9]{" + str(SCAN_ZERO_PAD_DIGITS) + "})$" diff --git a/tests/unit_tests/test_scan_directories.py b/tests/unit_tests/test_scan_directories.py index 80121a32..0fc157fc 100644 --- a/tests/unit_tests/test_scan_directories.py +++ b/tests/unit_tests/test_scan_directories.py @@ -167,7 +167,7 @@ def test_bad_scan_names(): scan_dir = scan_dir_manager.new_scan_dir("fake scan") assert scan_dir.name == "fake_scan_0001" scan_dir = scan_dir_manager.new_scan_dir("fake scan;rm -rf /;") - assert scan_dir.name == "fake_scan_rm_-rf____0001" + assert scan_dir.name == "fake_scan_rm_-rf_0001" def test_scan_sequence_and_listing(caplog): @@ -277,6 +277,33 @@ def test_scan_sequence_case_sensitive(): assert "fake_scan_0005" in all_scans +def test_scan_names_have_single_underscores(): + """Check created scans have only one underscore in them, not two.""" + _clear_scan_dir() + scan_dir_manager = ScanDirectoryManager(BASE_SCAN_DIR) + + # Create some scan data and mark it as successful to get an end date. + scan_data = fake_active_scan_data() + scan_data.set_final_data(result="Success") + # Make 4 scans + scan_dir = scan_dir_manager.new_scan_dir("fake_scan_") + scan_dir.save_scan_data(scan_data) + scan_dir = scan_dir_manager.new_scan_dir("fake_scan__") + scan_dir.save_scan_data(scan_data) + scan_dir = scan_dir_manager.new_scan_dir("fake_scan_______") + scan_dir.save_scan_data(scan_data) + scan_dir = scan_dir_manager.new_scan_dir("fake_scan") + scan_dir.save_scan_data(scan_data) + + # Check they exist and are numbered sequentially + all_scans = scan_dir_manager.all_scans + assert_unique_of_length(all_scans, 4) + assert "fake_scan_0001" in all_scans + assert "fake_scan_0002" in all_scans + assert "fake_scan_0003" in all_scans + assert "fake_scan_0004" in all_scans + + def test_scan_name_non_sequential(): """Check new scan has the correct name if the directories are not sequential.""" _clear_scan_dir() From 14b3e5a62708033fe05ecd27a16e39473b6fc7dd Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Sat, 7 Mar 2026 18:17:43 +0000 Subject: [PATCH 13/20] Apply suggestions from code review of branch 523-improve-validation-checks Co-authored-by: Julian Stirling --- src/openflexure_microscope_server/utilities.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index a13cf19d..db75786d 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -201,7 +201,8 @@ def _sanitise_reserved(name: str, is_filename: bool = True) -> str: :param name: The component to check. :param is_filename: Whether the component is a filename. - If True, names will be forced into lowercase. + If True, names will be forced into lowercase. + :returns: The sanitised component, in lower case. """ # Check for Windows reserved names. From f67699fbc8dd6d9d7daf86351f17fb2e3ed1273f Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Mon, 9 Mar 2026 13:54:43 +0000 Subject: [PATCH 14/20] Raise warnings if there are unsafe file path elements rather than coercing file paths. Stitching still raises an error if your file name is unsafe --- .../stitching.py | 10 +- .../utilities.py | 95 ++++++------ tests/unit_tests/test_utilities.py | 141 ++++++++++++------ 3 files changed, 153 insertions(+), 93 deletions(-) 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 From 968237bec09719ae2e84f0c8bf451e898a8f26db Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Mon, 9 Mar 2026 14:02:15 +0000 Subject: [PATCH 15/20] Ruff checks --- .../utilities.py | 47 +++++++++--- tests/unit_tests/test_utilities.py | 71 ++++++++++++------- 2 files changed, 81 insertions(+), 37 deletions(-) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index d16d494b..10215af8 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -111,8 +111,7 @@ _WINDOWS_RESERVED_NAMES = { 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. + """Check if a file path has any unsafe elements in it. 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 @@ -124,6 +123,7 @@ def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]: any unsafe features were found. """ # Split by separators first to sanitise components independently + components = re.split(r"([/\\])", unsafe_path_string) unsafe_character_pattern = ( @@ -146,10 +146,16 @@ def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]: # 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] == "..") + 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.") + LOGGER.warning( + f"File path {unsafe_path_string} may be unsafe due to unexpected relative navigation." + ) unsafe_relative_path = True continue @@ -157,26 +163,45 @@ def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]: 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.') + LOGGER.warning( + f"File path {unsafe_path_string} may be unsafe due to trailing dots." + ) trailing_dots = True # Check for trailing spaces - Windows specific - if sys.platform.startswith("win") and component.endswith(" ") and not trailing_whitespace: + 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.") + 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.") + 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 + warning_raised = ( + unsafe_relative_path + or trailing_dots + or trailing_whitespace + or unsafe_characters + or reserved_word + ) return unsafe_path_string, warning_raised diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index c6d32431..6df07dec 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -1,26 +1,19 @@ """Unit tests for utility functions.""" -import sys import logging +import sys + 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." -} +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" -} +RESERVED_NAME_PATHS = {"path/CON/file", "path/NUL.txt/file", "C:/AUX/test"} if sys.platform.startswith("win"): UNSAFE_CHARACTER_PATHS = { @@ -34,7 +27,7 @@ else: "/var/log/app:backup.log", "/etc/configs/network-settings\\v1", "/tmp/file_with_@_symbol.txt", - "/home/user/script(1).py" + "/home/user/script(1).py", } @@ -89,10 +82,16 @@ def test_make_path_safe_basic(caplog): 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 ( + 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" + 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 @@ -103,7 +102,10 @@ 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." + assert ( + f"{path} contains characters that may be unsafe on this platform." + in caplog.messages + ) @pytest.mark.parametrize("path", RESERVED_NAME_PATHS) @@ -111,29 +113,40 @@ def test_make_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 f"{path} contains a reserved system name and may cause issues." + assert ( + f"{path} contains a reserved system name and may cause issues." + in caplog.messages + ) 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 + # 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 ( + 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 + 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 + ) 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 ( + 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 @@ -152,7 +165,10 @@ def test_make_path_safe_relative(caplog): 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.") + assert ( + "File path a_bad/../relative_path may be unsafe due to unexpected relative navigation." + in caplog.messages + ) @pytest.mark.parametrize("filepath", PATHS_WITH_DOTS) @@ -160,4 +176,7 @@ 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 + assert ( + f"File path {filepath} may be unsafe due to trailing dots." + in caplog.messages + ) From b1d47c8aaeb69ce9132372753d1b4a33b2a36d1c Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Mon, 9 Mar 2026 14:09:00 +0000 Subject: [PATCH 16/20] Add final test case for path checking --- tests/unit_tests/test_utilities.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index 6df07dec..a1256e4b 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -119,6 +119,13 @@ def test_make_path_safe_reserved_in_components(caplog, path): ) +def test_make_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 len(caplog.records) == 0 + + def test_make_path_safe_trailing_space_in_components(caplog): """Test trailing spaces in path components.""" with caplog.at_level(logging.WARNING): From f50b3d5cfa7c5d1725652f3bde3d9994eef3e47c Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Mon, 9 Mar 2026 14:29:24 +0000 Subject: [PATCH 17/20] Allow spaces in linux paths --- src/openflexure_microscope_server/utilities.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index 10215af8..6c73262f 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -76,10 +76,10 @@ def requires_lock( # Compiled regular expressions for unsafe characters -# Matches anything that isn't a-z, A-Z, 0-9, _, ., -, :, /, \ +# Matches anything that isn't a-z, A-Z, 0-9, _, ., -, :, /, \, space _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, _, ., -, \, space +_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_.\-]") From ada75857061cb880b3d627fcab70bfdba47a05fc Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Mon, 9 Mar 2026 14:40:51 +0000 Subject: [PATCH 18/20] Fix typos --- tests/unit_tests/test_utilities.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index a1256e4b..f17bfa3f 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -25,7 +25,7 @@ if sys.platform.startswith("win"): else: UNSAFE_CHARACTER_PATHS = { "/var/log/app:backup.log", - "/etc/configs/network-settings\\v1", + "/etc/configs/network-settings\v1", "/tmp/file_with_@_symbol.txt", "/home/user/script(1).py", } @@ -152,7 +152,7 @@ def test_make_path_safe_trailing_space_in_components(caplog): # 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" + == "path/a long path name /file" ) # No warnings should be raised assert len(caplog.records) == 0 From 3776828699fa44f69d352b073768f00b711076a7 Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Mon, 9 Mar 2026 14:52:23 +0000 Subject: [PATCH 19/20] Fix final typo --- tests/unit_tests/test_utilities.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit_tests/test_utilities.py b/tests/unit_tests/test_utilities.py index f17bfa3f..7cc17d7e 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -151,7 +151,7 @@ def test_make_path_safe_trailing_space_in_components(caplog): 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] + make_path_safe("path/a long path name /file")[0] == "path/a long path name /file" ) # No warnings should be raised From 6d66dd0ed8d97a8c434824d04fe6df72c1291b77 Mon Sep 17 00:00:00 2001 From: Beth Probert Date: Mon, 9 Mar 2026 15:45:32 +0000 Subject: [PATCH 20/20] 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