From 579f2e0a2ef1ce807559270bd51290b6c100a5b8 Mon Sep 17 00:00:00 2001 From: Eman-Yousaf <180714048+Eman-Yousaf@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:39:09 +0500 Subject: [PATCH] fix(paths): treat / as a separator on Windows so stored sub-paths resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolve_within split candidate paths on os.sep alone. Windows accepts / as a real separator but os.sep is \ there, so a persisted sub-path such as "job_123/out.mp4" stayed a single component, failed the basename-equality check, and raised UnsafePath — while the identical value split cleanly and resolved on POSIX. A data directory written on Linux or by the Docker deployment and then opened by the Windows desktop app hit exactly that. Split on both separator families instead, which is what the comment above the split already states the code intends. This is not a loosening: every component still goes through the same basename / "." / ".." / empty rejection, and the commonpath containment check and symlink resolution below are unchanged. POSIX behaviour is unchanged too — a backslash is already rejected there as a foreign separator before the split runs. This also restores real coverage of the symlink-escape guard on Windows. test_resolve_within_rejects_symlink_escape asserts through "link/secret.wav", which previously raised at component validation before reaching the containment check it exists to cover, so it passed for the wrong reason. It now matches on the reason. --- backend/core/path_security.py | 9 ++++++- tests/test_filesystem_boundaries.py | 40 ++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/backend/core/path_security.py b/backend/core/path_security.py index f465b09f2..c4aa8d330 100644 --- a/backend/core/path_security.py +++ b/backend/core/path_security.py @@ -17,6 +17,13 @@ f"{prefix}{number}" for prefix in ("COM", "LPT") for number in range(1, 10) ) +# Both separator families, so a stored sub-path splits into the same components +# on every host. Windows accepts ``/`` as a real separator, so splitting on +# ``os.sep`` alone left ``"job/out.mp4"`` as a single component there while the +# identical value split cleanly on POSIX. POSIX input never reaches this with a +# backslash — it is rejected as a foreign separator before the split. +_PATH_SEPARATORS = re.compile(r"[\\/]") + class UnsafePath(ValueError): """Raised when a path crosses its allowed filesystem boundary.""" @@ -69,7 +76,7 @@ def resolve_within(root: os.PathLike[str] | str, value: os.PathLike[str] | str) # containment proof explicit to static analysis, this rejects empty, # dot, parent, drive, and separator-bearing components before Path sees # any persisted/request-derived string. - parts = raw.split(os.sep) + parts = _PATH_SEPARATORS.split(raw) clean_parts: list[str] = [] for part in parts: clean = os.path.basename(part) diff --git a/tests/test_filesystem_boundaries.py b/tests/test_filesystem_boundaries.py index ce69e5fed..7ba50c8e3 100644 --- a/tests/test_filesystem_boundaries.py +++ b/tests/test_filesystem_boundaries.py @@ -52,6 +52,41 @@ def test_resolve_within_accepts_relative_and_existing_absolute_paths(tmp_path): assert resolve_within(root, item) == item +def test_stored_subpaths_split_on_both_separator_families(): + """The component split is host-independent. + + Asserted on the splitter itself, not through ``resolve_within``: the + Python suite runs on Linux only, where ``os.sep`` splitting already + handled ``/``. A behavioural test would pass here whether or not the + Windows path is fixed, so it would not guard the regression. + """ + from core.path_security import _PATH_SEPARATORS + assert _PATH_SEPARATORS.split("sub/voice.wav") == ["sub", "voice.wav"] + assert _PATH_SEPARATORS.split(r"sub\voice.wav") == ["sub", "voice.wav"] + + +def test_resolve_within_reads_a_stored_subpath(tmp_path): + """A persisted sub-path resolves to the same file on Windows and POSIX. + + Windows accepts ``/`` as a real separator, so a row written by a Linux + host (or a Docker deployment) must resolve there exactly as it does on + POSIX instead of being rejected as one unsafe component. + """ + from core.path_security import resolve_within + root = tmp_path / "root" + (root / "sub").mkdir(parents=True) + assert resolve_within(root, "sub/voice.wav") == root / "sub" / "voice.wav" + + +def test_resolve_within_rejects_traversal_through_either_separator(tmp_path): + """Splitting on both separators must not open a traversal path.""" + from core.path_security import UnsafePath, resolve_within + root = tmp_path / "root" + (root / "sub").mkdir(parents=True) + with pytest.raises(UnsafePath): + resolve_within(root, "sub/../../secret.wav") + + def test_resolve_within_rejects_parent_and_absolute_escape(tmp_path): from core.path_security import UnsafePath, resolve_within root = tmp_path / "root" @@ -76,7 +111,10 @@ def test_resolve_within_rejects_symlink_escape(tmp_path): (root / "link").symlink_to(outside, target_is_directory=True) except OSError: pytest.skip("symlink creation is unavailable on this host") - with pytest.raises(UnsafePath): + # Match the reason, not just the type: when ``/`` was not treated as a + # separator on Windows this call failed at component validation instead, + # so the containment check below it was never exercised there. + with pytest.raises(UnsafePath, match="escapes its allowed root"): resolve_within(root, "link/secret.wav")