Merge pull request #1559 from Eman-Yousaf/fix/path-security-separator-parity

fix(paths): treat / as a separator on Windows so stored sub-paths resolve

CHANGELOG.md

@@ -31,6 +31,7 @@ the frozen-backend fallback mirror it for their toolchains.
 - The OmniVoice guide now covers combining style attributes with a reference clip (consistent instruct stabilizes cloning; the reference wins conflicts), inline pronunciation control (pinyin / CMU phonemes), and corrects the claim that the default engine can't do voice design — it can, from attributes (#1565)
 
 ### Fixed
+- Stored artifact subpaths now resolve after moving a data directory between Windows, macOS, Linux, and Docker, while traversal and symlink escapes remain blocked (#1559) — thanks @Eman-Yousaf!
 - A remote browser hitting an API-key-configured server's admin 403 now gets the API-key login form instead of endless console 403s, while desktop and PIN-only/no-key servers keep the plain loopback error so guests are never offered a login no key can satisfy (#1568) — thanks @paoloantinori!
 - The crash-isolated ASR sidecar and its download preflight now agree on which model to load — setting the shared faster-whisper model variable applies to both variants instead of the sidecar quietly using a different one (#1556)
 - "Ready" now requires the deep health probe (a working database-backed route), not just the identity probe — a backend whose install broke underneath can no longer be announced up while every real request fails (#1548)

backend/core/path_security.py

@@ -17,6 +17,13 @@ _WINDOWS_RESERVED_NAMES = frozenset({"CON", "PRN", "AUX", "NUL"}) | frozenset(
     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."""
@@ -52,11 +59,10 @@ def resolve_within(root: os.PathLike[str] | str, value: os.PathLike[str] | str)
     raw = os.fspath(value) if value is not None else ""
     if not isinstance(raw, str) or not raw:
         raise UnsafePath("path is empty")
-    # Treat both separator families as structural on every host. Otherwise a
-    # Windows traversal string is an innocent-looking filename when validated
-    # on Linux (and can become dangerous after persisted data is moved).
-    if os.sep != "\\" and ("\\" in raw or bool(ntpath.splitdrive(raw)[0])):
-        raise UnsafePath("path uses a foreign separator or drive")
+    # Treat both separator families as structural on every host while still
+    # rejecting Windows drive paths before rebuilding relative components.
+    if os.sep != "\\" and bool(ntpath.splitdrive(raw)[0]):
+        raise UnsafePath("path uses a drive")
     root_path = Path(root).expanduser().resolve(strict=False)
     root_text = str(root_path)
     if os.path.isabs(raw):
@@ -69,7 +75,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)

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"]
+
+
+@pytest.mark.parametrize("stored_path", ["sub/voice.wav", r"sub\voice.wav"])
+def test_resolve_within_reads_a_stored_subpath(tmp_path, stored_path):
+    """A persisted sub-path resolves to the same file on Windows and POSIX.
+
+    Rows written on Windows, POSIX, or Docker must resolve identically after
+    the same data directory is opened on another supported host.
+    """
+    from core.path_security import resolve_within
+    root = tmp_path / "root"
+    (root / "sub").mkdir(parents=True)
+    assert resolve_within(root, stored_path) == 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")