mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-09-11 11:24:29 +00:00
Compare commits
2
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
cb3d1f2714 | ||
|
|
0a45e89777 |
@@ -26,6 +26,13 @@ def safe_join(root: Path, name: str) -> Path | None:
|
|||||||
"""
|
"""
|
||||||
if not name:
|
if not name:
|
||||||
return None
|
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("\\", "/")
|
safe = name.replace("\\", "/")
|
||||||
try:
|
try:
|
||||||
root_resolved = root.resolve()
|
root_resolved = root.resolve()
|
||||||
|
|||||||
@@ -5229,10 +5229,52 @@ def _resolve_dlc_path(dlc: Path, filename: str) -> Path | None:
|
|||||||
check so every filename-bound handler validates before touching the
|
check so every filename-bound handler validates before touching the
|
||||||
filesystem.
|
filesystem.
|
||||||
|
|
||||||
Returns the validated resolved Path, or None if the path is empty
|
Containment here is LEXICAL (normalize `.`/`..` WITHOUT following
|
||||||
or escapes the DLC root.
|
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}
|
_SMART_TYPE_BASE: dict[str, int] = {"Lead": 0, "Rhythm": 10, "Bass": 20}
|
||||||
|
|||||||
@@ -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
|
||||||
Reference in New Issue
Block a user