From 0dcc9136b6079873c1d2c83459fe54718a0cfbb3 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Fri, 10 Jul 2026 20:41:42 +0200 Subject: [PATCH] refactor(server): move DLC path resolution to lib/dlc_paths.py (R3 substrate) (#843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The keystone for the path-dependent routers (ws_highway, song, audio-local-path, sloppak): the "where do the song files live + safe containment" layer leaves server.py so a router can reach it. - `_resolve_dlc_path` (pure — args only) moves verbatim. - `_get_dlc_dir` reads the env-derived paths through the appstate seam (appstate.dlc_dir/dlc_dir_env/config_dir) instead of module globals, so lib/dlc_paths.py does no import-time IO. - appstate gains `dlc_dir`/`dlc_dir_env` slots (config_dir already there); server.py configures them. All three are env-derived, so a setenv+reimport fixture reconfigures them for free — ZERO setattr retargeting. - server.py RE-EXPORTS both (`from dlc_paths import _get_dlc_dir, _resolve_dlc_path`), so its 24+16 call sites AND the tests that reach `server._get_dlc_dir()` / `server._resolve_dlc_path()` directly (test_dlc_junction, test_highway_ws_*) resolve unchanged — no test edits. server.py: 9,085 -> 9,008. Verified: _resolve_dlc_path byte-identical to origin/main; _get_dlc_dir's only change is the three path identifiers -> appstate.*; pyflakes clean; route table identical (143); pytest 2407 passed (test_dlc_junction + both highway_ws suites green via re-export); packaging guard 52; eslint 0. Boot smoke: library scans (8 songs, _get_dlc_dir), a real song resolves + serves art (_resolve_dlc_path), and the highway WS reaches `ready` with notes. Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 2 +- docs/size-exemptions.md | 2 +- lib/appstate.py | 5 ++- lib/dlc_paths.py | 99 +++++++++++++++++++++++++++++++++++++++++ server.py | 83 ++-------------------------------- 5 files changed, 108 insertions(+), 83 deletions(-) create mode 100644 lib/dlc_paths.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ef67b71..319d4a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 next root-level module can't ship broken. ### Added -- **`routers/` — extracting `server.py`'s route layer, cheapest-first (R3).** Each PR moves a cohesive route group into a `fastapi.APIRouter` under `lib/routers/`, mounted with `app.include_router(...)` at its original site (FastAPI matches in registration order; the full route table stays byte-identical). Bodies are verbatim — only the decorator receiver (`@app` → `@router`) and singleton reads (`meta_db` → `appstate.meta_db`, resolved at call time) change. So far: `audio_effects` (5), `artist_aliases` (5), `loops` (3), `playlists` (12 + custom covers). The `config_dir` path constant now rides the `appstate` seam (env-derived, so the pop-and-reimport fixtures reconfigure it for free), and the shared request-field sanitizer `_clean_str` moved to `lib/reqfields.py`. The next cut is picked by a dependency-closure scan that ranks groups by how many `monkeypatch.setattr(server, …)` targets they'd drag along. +- **`routers/` — extracting `server.py`'s route layer, cheapest-first (R3).** Each PR moves a cohesive route group into a `fastapi.APIRouter` under `lib/routers/`, mounted with `app.include_router(...)` at its original site (FastAPI matches in registration order; the full route table stays byte-identical). Bodies are verbatim — only the decorator receiver (`@app` → `@router`) and singleton reads (`meta_db` → `appstate.meta_db`, resolved at call time) change. So far: `audio_effects` (5), `artist_aliases` (5), `loops` (3), `playlists` (12 + custom covers). The DLC library-path resolution (`_get_dlc_dir`, pure `_resolve_dlc_path`) moved to `lib/dlc_paths.py`, reading paths through the seam; `config_dir`/`dlc_dir`/`dlc_dir_env` now ride the `appstate` seam (env-derived, so the pop-and-reimport fixtures reconfigure it for free), and the shared request-field sanitizer `_clean_str` moved to `lib/reqfields.py`. The next cut is picked by a dependency-closure scan that ranks groups by how many `monkeypatch.setattr(server, …)` targets they'd drag along. - **`routers/` — the first extracted route module (R3).** The five audio-effects mapping endpoints move out of `server.py` into `lib/routers/audio_effects.py` as a `fastapi.APIRouter`, mounted with `app.include_router(...)` **at the point in the file diff --git a/docs/size-exemptions.md b/docs/size-exemptions.md index b2a5126..89d46f5 100644 --- a/docs/size-exemptions.md +++ b/docs/size-exemptions.md @@ -55,7 +55,7 @@ without a *signed* exemption" is unenforceable. ## Planned, NOT exempt (owned by split plans — listed so nothing falls between states) core `static/app.js` (11,852) · `static/highway.js` (4,168, whole file) · `server.py` -(9,085 — was 14,037; ratcheted by the R3 `MetadataDB` + `AudioEffectsMappingDB` +(9,008 — was 14,037; ratcheted by the R3 `MetadataDB` + `AudioEffectsMappingDB` extractions and four `routers/` modules) · `lib/metadata_db.py` (4,373 — new in R3; the `MetadataDB` class alone is 4,018 lines and is a monolith in its own right, to be split per-table once the router train diff --git a/lib/appstate.py b/lib/appstate.py index 78dcecf..43e8b19 100644 --- a/lib/appstate.py +++ b/lib/appstate.py @@ -70,8 +70,11 @@ audio_effect_mappings = None # `setattr(server, …)` in a few tests, so those slots (when added) need their # tests retargeted to appstate in the same PR. config_dir = None +dlc_dir = None # the DLC_DIR env value as a Path (Path("") if unset) +dlc_dir_env = None # the raw DLC_DIR env string, "" if unset — distinguishes + # "unset" from Path("")→"." (see dlc_paths._get_dlc_dir) -_SLOTS = frozenset({"meta_db", "audio_effect_mappings", "config_dir"}) +_SLOTS = frozenset({"meta_db", "audio_effect_mappings", "config_dir", "dlc_dir", "dlc_dir_env"}) def configure(**kwargs) -> None: diff --git a/lib/dlc_paths.py b/lib/dlc_paths.py new file mode 100644 index 0000000..0202a25 --- /dev/null +++ b/lib/dlc_paths.py @@ -0,0 +1,99 @@ +"""DLC library path resolution — where the song files live, plus safe containment. + +Extracted from ``server.py`` (R3). ``_resolve_dlc_path`` is pure and moved +verbatim. ``_get_dlc_dir`` reads the env-derived paths through the ``appstate`` +seam (``server.py`` configures ``dlc_dir``/``dlc_dir_env``/``config_dir`` at +import, fresh on every re-import), so this module does no import-time IO and the +pop-and-reimport fixtures keep working. ``server.py`` re-exports both names, so +existing ``server._get_dlc_dir`` / ``server._resolve_dlc_path`` references +(tests, other handlers) resolve unchanged. +""" + +import json +import os +from pathlib import Path + +import appstate + + +def _get_dlc_dir(cfg: dict | None = None) -> Path | None: + # Only consider DLC_DIR if the env var was non-empty. `Path("")` collapses + # to `.` and reports `.is_dir() == True`, which would silently shadow the + # config.json fallback. Checking the raw env string preserves + # `DLC_DIR=.` as a valid opt-in for cwd while keeping unset/empty out. + if appstate.dlc_dir_env and appstate.dlc_dir.is_dir(): + return appstate.dlc_dir + if cfg is None: + config_file = appstate.config_dir / "config.json" + if config_file.exists(): + try: + cfg = json.loads(config_file.read_text(encoding="utf-8")) + except Exception: + pass + if isinstance(cfg, dict): + raw = str(cfg.get("dlc_dir", "")).strip() + if raw: + p = Path(raw) + if p.is_dir(): + return p + return None + + +def _resolve_dlc_path(dlc: Path, filename: str) -> Path | None: + """Resolve `filename` under DLC_DIR and refuse anything that escapes. + + `filename` arrives from `:path` route params and can contain `..` + segments. The Sloppak and archive paths happen to fail safely later + because their loaders raise on missing/invalid files, but loose- + folder format detection (`is_loose_song`) globs and parses XML on + disk first, which lets a crafted path trigger filesystem reads + outside DLC_DIR before any guard fires. Centralise the containment + check so every filename-bound handler validates before touching the + filesystem. + + 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. + """ + 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 diff --git a/server.py b/server.py index 42db67b..7082859 100644 --- a/server.py +++ b/server.py @@ -67,6 +67,7 @@ from metadata_db import ( # its own module, the `audio_effect_mappings` singleton below stays here. from audio_effects_db import AudioEffectsMappingDB from reqfields import _clean_str +from dlc_paths import _get_dlc_dir, _resolve_dlc_path # The router seam. Imported as a module (never `from appstate import ...`) so # `appstate.configure(...)` below publishes into the same namespace routers read. # Lives in lib/ because that is the one core dir every packaging path copies. @@ -368,6 +369,8 @@ appstate.configure( meta_db=meta_db, audio_effect_mappings=audio_effect_mappings, config_dir=CONFIG_DIR, + dlc_dir=DLC_DIR, + dlc_dir_env=_DLC_DIR_ENV, ) @@ -951,89 +954,9 @@ def _library_art_response(result: Any) -> Response: raise HTTPException(status_code=500, detail="Library provider returned unsupported art data") -def _get_dlc_dir(cfg: dict | None = None) -> Path | None: - # Only consider DLC_DIR if the env var was non-empty. `Path("")` collapses - # to `.` and reports `.is_dir() == True`, which would silently shadow the - # config.json fallback. Checking the raw env string preserves - # `DLC_DIR=.` as a valid opt-in for cwd while keeping unset/empty out. - if _DLC_DIR_ENV and DLC_DIR.is_dir(): - return DLC_DIR - if cfg is None: - config_file = CONFIG_DIR / "config.json" - if config_file.exists(): - try: - cfg = json.loads(config_file.read_text(encoding="utf-8")) - except Exception: - pass - if isinstance(cfg, dict): - raw = str(cfg.get("dlc_dir", "")).strip() - if raw: - p = Path(raw) - if p.is_dir(): - return p - return None - # ── Background metadata scan ────────────────────────────────────────────────── -def _resolve_dlc_path(dlc: Path, filename: str) -> Path | None: - """Resolve `filename` under DLC_DIR and refuse anything that escapes. - - `filename` arrives from `:path` route params and can contain `..` - segments. The Sloppak and archive paths happen to fail safely later - because their loaders raise on missing/invalid files, but loose- - folder format detection (`is_loose_song`) globs and parses XML on - disk first, which lets a crafted path trigger filesystem reads - outside DLC_DIR before any guard fires. Centralise the containment - check so every filename-bound handler validates before touching the - filesystem. - - 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. - """ - 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 def _pick_smart_arrangement(