From fe409ccefbefd5f2b8783c2e83a5ce66348c2b44 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Tue, 2 Jun 2026 14:43:03 +0100 Subject: [PATCH 1/4] Allow dots within filepaths if not trailing. --- .../utilities.py | 136 +++++++++--------- tests/unit_tests/test_utilities.py | 23 ++- 2 files changed, 87 insertions(+), 72 deletions(-) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index 7da53cc6..d57b7b43 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -9,6 +9,7 @@ import tomllib from datetime import datetime from functools import wraps from importlib.metadata import version +from pathlib import PurePath from typing import ( Any, Callable, @@ -80,6 +81,10 @@ def requires_lock( _WINDOWS_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_.\-/ ]") + +UNSAFE_PATTERN = ( + _WINDOWS_UNSAFE_PATTERN if sys.platform.startswith("win") else _POSIX_UNSAFE_PATTERN +) # Matches anything that isn't a-z, A-Z, 0-9, _, ., - _NAME_UNSAFE_PATTERN = re.compile(r"[^a-zA-Z0-9_.\-]") @@ -121,86 +126,83 @@ def is_path_safe(unsafe_path_string: str) -> bool: :returns: A boolean indicating if any unsafe features were found. """ - # Split by separators first to sanitise components independently - - components = re.split(r"([/\\])", unsafe_path_string) - - unsafe_character_pattern = ( - _WINDOWS_UNSAFE_PATTERN - if sys.platform.startswith("win") - else _POSIX_UNSAFE_PATTERN + return all( + [ + _path_normalised(unsafe_path_string), + _no_trailing_dots_or_whitespaces(unsafe_path_string), + _no_unsafe_characters(unsafe_path_string), + _no_reserved_name(unsafe_path_string), + ] ) - # 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): - # Skip separators and empty strings - if component in ("/", "\\") or not component: +def _path_normalised(unsafe_path_string: str) -> bool: + """Check that the path starts normalised. + + Warn and return false if . or .. is not at the start of the path. + """ + # Ensure correct path separators before normalisation: + if sys.platform.startswith("win"): + unsafe_path_string = unsafe_path_string.replace("/", "\\") + normalised = os.path.normpath(unsafe_path_string) + + # Allow starting with ./ (or .\ on Windows) + if unsafe_path_string.startswith("." + os.path.sep): + normalised = "." + os.path.sep + normalised + + # Allow trailing path separator + if unsafe_path_string.endswith(os.path.sep): + normalised += os.path.sep + + if normalised != unsafe_path_string: + LOGGER.warning( + f"File path {unsafe_path_string} may be unsafe due to unexpected relative " + "navigation." + ) + return False + return True + + +def _no_trailing_dots_or_whitespaces(unsafe_path_string: str) -> bool: + """Disallow trailing dots or whitespace.""" + for component in PurePath(unsafe_path_string).parts: + if component == "..": continue - - # 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 - - # 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 + bad_trailing_chars = ". " if sys.platform.startswith("win") else "." + if component.rstrip(bad_trailing_chars) != component: LOGGER.warning( - f"File path {unsafe_path_string} may be unsafe due to trailing dots." + f"File path {unsafe_path_string} may be unsafe due to trailing dots " + "or whitespace." ) - trailing_dots = True + return False + # All passed + return True - # 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: +def _no_unsafe_characters(unsafe_path_string: str) -> bool: + """Disallow unsafe characters.""" + for component in PurePath(unsafe_path_string).parts: + if UNSAFE_PATTERN.search(component): LOGGER.warning( - f"{unsafe_path_string} contains characters that may be unsafe on this platform." + f"{unsafe_path_string} contains characters that may be unsafe on this " + "platform." ) - unsafe_characters = True + return False + # All passed + return 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 - ): + +def _no_reserved_name(unsafe_path_string: str) -> bool: + """Disallow use of Windows reserved names (e.g., CON, PRN, LPT1).""" + for component in PurePath(unsafe_path_string).parts: + if _sanitise_reserved(component, is_filename=False) != component: LOGGER.warning( - f"{unsafe_path_string} contains a reserved system name and may cause issues." + f"{unsafe_path_string} contains a reserved system name and may cause " + "issues." ) - reserved_word = True - - return not ( - unsafe_relative_path - or trailing_dots - or trailing_whitespace - or unsafe_characters - or reserved_word - ) + return False + # All passed + return True 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 bc9076cd..6351d9d3 100644 --- a/tests/unit_tests/test_utilities.py +++ b/tests/unit_tests/test_utilities.py @@ -11,7 +11,9 @@ from openflexure_microscope_server.utilities import ( make_name_safe, ) -PATHS_WITH_DOTS = {"a./b/c.", "path./to/file.", "./path./to/file."} +BAD_PATHS_WITH_DOTS = {"a./b/c.", "path./to/file.", "./path./to/file."} + +GOOD_PATHS_WITH_DOTS = {"a.a/b/.c", "path.to/file.file.file", "./path.to/file.files"} PATHS_WITH_SPACES = {"path /to /file ", "path/to /file", "path/a long path name /file"} @@ -130,7 +132,10 @@ def test_is_path_safe_trailing_space_in_components(caplog, path): if sys.platform.startswith("win"): assert not is_path_safe(path) assert len(caplog.records) == 1 - assert f"{path} contains unsafe trailing spaces." in caplog.messages + assert ( + f"File path {path} may be unsafe due to trailing dots or whitespace." + in caplog.messages + ) else: # Trailing spaces allows in POSIX systems assert is_path_safe(path) @@ -158,12 +163,20 @@ def test_is_path_safe_relative(caplog): ) -@pytest.mark.parametrize("filepath", PATHS_WITH_DOTS) -def test_is_path_safe_dots_warning(caplog, filepath): +@pytest.mark.parametrize("filepath", BAD_PATHS_WITH_DOTS) +def test_is_path_safe_with_trailing_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 not is_path_safe(filepath) assert ( - f"File path {filepath} may be unsafe due to trailing dots." + f"File path {filepath} may be unsafe due to trailing dots or whitespace." in caplog.messages ) + + +@pytest.mark.parametrize("filepath", GOOD_PATHS_WITH_DOTS) +def test_is_path_safe_allowed_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 is_path_safe(filepath) + assert len(caplog.records) == 0 From 0cb37abffe3fa8f6138082898f0e3d958044256a Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Tue, 2 Jun 2026 15:02:56 +0100 Subject: [PATCH 2/4] Path checking: Ensure windows outputs original path when warning --- 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 d57b7b43..558fac66 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -141,6 +141,7 @@ def _path_normalised(unsafe_path_string: str) -> bool: Warn and return false if . or .. is not at the start of the path. """ + original_path = unsafe_path_string # Ensure correct path separators before normalisation: if sys.platform.startswith("win"): unsafe_path_string = unsafe_path_string.replace("/", "\\") @@ -156,7 +157,7 @@ def _path_normalised(unsafe_path_string: str) -> bool: if normalised != unsafe_path_string: LOGGER.warning( - f"File path {unsafe_path_string} may be unsafe due to unexpected relative " + f"File path {original_path} may be unsafe due to unexpected relative " "navigation." ) return False From ef497b8fdd0705d1cd889a14d51b2c7f4e2830a6 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Fri, 5 Jun 2026 10:26:20 +0000 Subject: [PATCH 3/4] Add comments about PurePath usage --- src/openflexure_microscope_server/utilities.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index 558fac66..d468ac80 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -166,6 +166,9 @@ def _path_normalised(unsafe_path_string: str) -> bool: def _no_trailing_dots_or_whitespaces(unsafe_path_string: str) -> bool: """Disallow trailing dots or whitespace.""" + # Use PurePath from Python's standard library to iterate over each element + # in the path as separated by path separators. For example: + # "foo/bar/tada.py" -> ("foo", "bar", "tada.py") for component in PurePath(unsafe_path_string).parts: if component == "..": continue From 8367c40b9070da6091057094df5d9c2d1a496c52 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Fri, 5 Jun 2026 11:32:25 +0100 Subject: [PATCH 4/4] rename unvalidated paths from unsafe to unvalidated --- .../utilities.py | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/openflexure_microscope_server/utilities.py b/src/openflexure_microscope_server/utilities.py index d468ac80..d5c47e6b 100644 --- a/src/openflexure_microscope_server/utilities.py +++ b/src/openflexure_microscope_server/utilities.py @@ -115,47 +115,47 @@ _WINDOWS_RESERVED_NAMES = { } -def is_path_safe(unsafe_path_string: str) -> bool: +def is_path_safe(unvalidated_path: 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 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. + :param unvalidated_path: The original path string to sanitise. :returns: A boolean indicating if any unsafe features were found. """ return all( [ - _path_normalised(unsafe_path_string), - _no_trailing_dots_or_whitespaces(unsafe_path_string), - _no_unsafe_characters(unsafe_path_string), - _no_reserved_name(unsafe_path_string), + _path_normalised(unvalidated_path), + _no_trailing_dots_or_whitespaces(unvalidated_path), + _no_unsafe_characters(unvalidated_path), + _no_reserved_name(unvalidated_path), ] ) -def _path_normalised(unsafe_path_string: str) -> bool: +def _path_normalised(unvalidated_path: str) -> bool: """Check that the path starts normalised. Warn and return false if . or .. is not at the start of the path. """ - original_path = unsafe_path_string + original_path = unvalidated_path # Ensure correct path separators before normalisation: if sys.platform.startswith("win"): - unsafe_path_string = unsafe_path_string.replace("/", "\\") - normalised = os.path.normpath(unsafe_path_string) + unvalidated_path = unvalidated_path.replace("/", "\\") + normalised = os.path.normpath(unvalidated_path) # Allow starting with ./ (or .\ on Windows) - if unsafe_path_string.startswith("." + os.path.sep): + if unvalidated_path.startswith("." + os.path.sep): normalised = "." + os.path.sep + normalised # Allow trailing path separator - if unsafe_path_string.endswith(os.path.sep): + if unvalidated_path.endswith(os.path.sep): normalised += os.path.sep - if normalised != unsafe_path_string: + if normalised != unvalidated_path: LOGGER.warning( f"File path {original_path} may be unsafe due to unexpected relative " "navigation." @@ -164,18 +164,18 @@ def _path_normalised(unsafe_path_string: str) -> bool: return True -def _no_trailing_dots_or_whitespaces(unsafe_path_string: str) -> bool: +def _no_trailing_dots_or_whitespaces(unvalidated_path: str) -> bool: """Disallow trailing dots or whitespace.""" # Use PurePath from Python's standard library to iterate over each element # in the path as separated by path separators. For example: # "foo/bar/tada.py" -> ("foo", "bar", "tada.py") - for component in PurePath(unsafe_path_string).parts: + for component in PurePath(unvalidated_path).parts: if component == "..": continue bad_trailing_chars = ". " if sys.platform.startswith("win") else "." if component.rstrip(bad_trailing_chars) != component: LOGGER.warning( - f"File path {unsafe_path_string} may be unsafe due to trailing dots " + f"File path {unvalidated_path} may be unsafe due to trailing dots " "or whitespace." ) return False @@ -183,12 +183,12 @@ def _no_trailing_dots_or_whitespaces(unsafe_path_string: str) -> bool: return True -def _no_unsafe_characters(unsafe_path_string: str) -> bool: +def _no_unsafe_characters(unvalidated_path: str) -> bool: """Disallow unsafe characters.""" - for component in PurePath(unsafe_path_string).parts: + for component in PurePath(unvalidated_path).parts: if UNSAFE_PATTERN.search(component): LOGGER.warning( - f"{unsafe_path_string} contains characters that may be unsafe on this " + f"{unvalidated_path} contains characters that may be unsafe on this " "platform." ) return False @@ -196,12 +196,12 @@ def _no_unsafe_characters(unsafe_path_string: str) -> bool: return True -def _no_reserved_name(unsafe_path_string: str) -> bool: +def _no_reserved_name(unvalidated_path: str) -> bool: """Disallow use of Windows reserved names (e.g., CON, PRN, LPT1).""" - for component in PurePath(unsafe_path_string).parts: + for component in PurePath(unvalidated_path).parts: if _sanitise_reserved(component, is_filename=False) != component: LOGGER.warning( - f"{unsafe_path_string} contains a reserved system name and may cause " + f"{unvalidated_path} contains a reserved system name and may cause " "issues." ) return False