diff --git a/lib/safepath.py b/lib/safepath.py index 1eebbdc..4442e11 100644 --- a/lib/safepath.py +++ b/lib/safepath.py @@ -26,6 +26,13 @@ def safe_join(root: Path, name: str) -> Path | None: """ if not name: return None + # Reject embedded NULs explicitly. This used to ride on `.resolve()` + # raising ValueError, but on Python 3.13 (Windows) resolve() no longer + # raises for an embedded NUL, so the byte would otherwise leak through + # containment. An explicit guard is strictly-more-rejection (no effect on + # the zip-slip / traversal contract). + if "\x00" in name: + return None safe = name.replace("\\", "/") try: root_resolved = root.resolve() diff --git a/server.py b/server.py index 5a1513a..c78355c 100644 --- a/server.py +++ b/server.py @@ -5229,10 +5229,52 @@ def _resolve_dlc_path(dlc: Path, filename: str) -> Path | None: check so every filename-bound handler validates before touching the filesystem. - Returns the validated resolved Path, or None if the path is empty - or escapes the DLC root. + Containment here is LEXICAL (normalize `.`/`..` WITHOUT following + symlinks), not `safe_join`'s `.resolve()`-based check — because users + commonly mount their song library through a directory JUNCTION/symlink + (a library shared across app installs; the desktop app's own mounts). + `.resolve()` follows that junction to its real target, sees it sits + outside DLC_DIR, and wrongly rejects every song reached through it — the + scanner's `rglob` indexes those songs, but art/load then 403/404s (broken + covers, unplayable songs). Lexical normalization still rejects the only + escapes a `:path` filename can express — `..` traversal and absolute + paths — which the traversal tests pin. `safe_join` stays strict (it is + the zip-slip / plugin-asset guard, where following a symlink out IS the + defense); the loose-folder art handler keeps its own per-file symlink + re-check for defence-in-depth. + + Returns the validated Path (not necessarily link-resolved), or None if + the filename is empty, contains a NUL, or escapes the DLC root. """ - return safe_join(dlc, filename) + if not filename: + return None + # Backslashes → forward slashes so a Windows-style `..\\x` traversal is + # rejected identically on POSIX (mirrors safe_join's normalisation). + safe = filename.replace("\\", "/") + if "\x00" in safe: + return None + # Reject drive-letter / absolute paths in BOTH conventions. A POSIX "/x" is + # caught by the containment check below (the `/` operator discards `root`), + # but a Windows drive-absolute "C:/x" is treated as a relative "C:" dir on + # POSIX and would otherwise slip in as `/C:/x` — so the contract must + # hold cross-platform (a shared library is reached from either OS). + from pathlib import PurePosixPath, PureWindowsPath + if (PurePosixPath(safe).is_absolute() + or PureWindowsPath(safe).is_absolute() + or PureWindowsPath(safe).drive): + return None + try: + root = dlc.resolve() + # normpath collapses `.`/`..`/duplicate separators purely lexically — + # it never touches the filesystem, so an in-library junction component + # is preserved (allowed) while `..`/absolute segments still escape and + # get caught by the containment check below. + candidate = Path(os.path.normpath(root / safe)) + if not candidate.is_relative_to(root): + return None + except (ValueError, OSError): + return None + return candidate _SMART_TYPE_BASE: dict[str, int] = {"Lead": 0, "Rhythm": 10, "Bass": 20} diff --git a/tests/test_dlc_junction.py b/tests/test_dlc_junction.py new file mode 100644 index 0000000..5b87fa9 --- /dev/null +++ b/tests/test_dlc_junction.py @@ -0,0 +1,92 @@ +"""Unit tests for ``server._resolve_dlc_path`` — the DLC-library containment +guard. + +It must (1) allow a library mounted through a directory JUNCTION/symlink (the +shared-library-across-installs / desktop-app case that a ``.resolve()``-based +check wrongly rejected, breaking album art + song load), while (2) still +rejecting ``..`` traversal and absolute paths — the only escapes a ``:path`` +filename can express. ``safe_join`` stays strict on purpose (zip-slip guard), +so the contrast is pinned here too. +""" + +import importlib +import os +import sys + +import pytest + + +@pytest.fixture() +def server(tmp_path, monkeypatch): + (tmp_path / "cfg").mkdir() + monkeypatch.setenv("CONFIG_DIR", str(tmp_path / "cfg")) + monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1") + sys.modules.pop("server", None) + srv = importlib.import_module("server") + try: + yield srv + finally: + conn = getattr(getattr(srv, "meta_db", None), "conn", None) + if conn is not None: + getattr(sys.modules.get("server"), "_join_background_db_threads", lambda: None)() + conn.close() + sys.modules.pop("server", None) + + +def _dlc(tmp_path): + d = tmp_path / "dlc" + d.mkdir() + return d + + +# ── still-rejected escapes (the security contract) ──────────────────────────── + +def test_dotdot_traversal_rejected(server, tmp_path): + dlc = _dlc(tmp_path) + assert server._resolve_dlc_path(dlc, "../../etc/passwd") is None + # a Windows-style backslash traversal is normalised + rejected identically + assert server._resolve_dlc_path(dlc, "..\\..\\secret") is None + assert server._resolve_dlc_path(dlc, "a/../../b") is None + + +def test_absolute_path_rejected(server, tmp_path): + dlc = _dlc(tmp_path) + assert server._resolve_dlc_path(dlc, "/etc/passwd") is None + assert server._resolve_dlc_path(dlc, "C:/Windows/system32/x") is None + + +def test_empty_and_nul_rejected(server, tmp_path): + dlc = _dlc(tmp_path) + assert server._resolve_dlc_path(dlc, "") is None + assert server._resolve_dlc_path(dlc, "a\x00b") is None + + +# ── allowed: legitimate in-library paths ────────────────────────────────────── + +def test_safe_relative_allowed(server, tmp_path): + dlc = _dlc(tmp_path) + p = server._resolve_dlc_path(dlc, "CDLC/City Pop/song.feedpak") + assert p is not None + assert p.is_relative_to(dlc.resolve()) + + +def test_junction_subfolder_allowed(server, tmp_path): + """A library mounted through a directory junction/symlink must resolve — + the case that broke album art for Christian's shared city-pop library.""" + dlc = _dlc(tmp_path) + real = tmp_path / "real_library" + real.mkdir() + (real / "song.feedpak").write_bytes(b"pack") + link = dlc / "CDLC" + try: + os.symlink(real, link, target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("symlink/junction creation not permitted on this host") + + p = server._resolve_dlc_path(dlc, "CDLC/song.feedpak") + assert p is not None, "a junctioned library subfolder was wrongly rejected" + assert p.exists(), "the resolved path should reach the file through the junction" + # Contrast: safe_join stays strict (it .resolve()s and follows the junction + # to its real target outside the root), which is correct for its zip-slip + # callers but is exactly why _resolve_dlc_path can't reuse it here. + assert server.safe_join(dlc, "CDLC/song.feedpak") is None