fix(library): serve art/load for songs mounted through a library junction (#766)

* fix(library): serve art/load songs mounted through a library junction

A song library mounted through a directory JUNCTION/symlink subfolder (a
library shared across app installs; the desktop app's own mounts) had broken
album art and couldn't load: the scanner's rglob follows the junction and
indexes the songs, but _resolve_dlc_path (via safe_join's .resolve()) followed
the junction to its real target, saw it outside DLC_DIR, and rejected every
song reached through it → 403 on /art, 404 on /art/candidates, broken covers.

- _resolve_dlc_path now uses LEXICAL containment (os.path.normpath, no symlink
  following) so an in-library junction is allowed, while `..` traversal and
  absolute paths are still rejected (the traversal tests pin this).
- safe_join is left STRICT (.resolve()-based) — it is the zip-slip / plugin-
  asset / avatar guard, where following a symlink out IS the defense — but
  gains an explicit NUL guard (on Python 3.13/Windows resolve() no longer
  raises on an embedded NUL, so the byte was leaking through; strictly-more-
  rejection, no effect on the zip-slip contract).

Tests: test_dlc_junction (junction allowed; `..`/absolute/NUL rejected; the
safe_join-stays-strict contrast). Existing traversal/safepath/art-candidates
suites stay green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(library): reject Windows drive-letter paths in _resolve_dlc_path

The new test_absolute_path_rejected pins 'C:/Windows/system32/x' → None, but on
POSIX a drive-letter path isn't absolute, so Path(dlc)/'C:/…' becomes the
contained relative dir '<dlc>/C:/…' and slipped through the lexical containment
check (red on the Linux CI). Not an escape, but the traversal contract should
hold cross-platform (a shared library is reached from either OS). Reject a path
that is absolute or drive-qualified in either POSIX or Windows semantics before
the containment check. Legitimate relative/junction paths are unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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()

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 `<root>/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}

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