mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-10 18:59:56 +00:00
refactor(server): move DLC path resolution to lib/dlc_paths.py (R3 substrate) (#843)
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
6da01c55a4
commit
0dcc9136b6
+1
-1
@@ -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.
|
next root-level module can't ship broken.
|
||||||
|
|
||||||
### Added
|
### 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
|
- **`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
|
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
|
`fastapi.APIRouter`, mounted with `app.include_router(...)` **at the point in the file
|
||||||
|
|||||||
@@ -55,7 +55,7 @@ without a *signed* exemption" is unenforceable.
|
|||||||
## Planned, NOT exempt (owned by split plans — listed so nothing falls between states)
|
## 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`
|
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) ·
|
extractions and four `routers/` modules) ·
|
||||||
`lib/metadata_db.py` (4,373 — new in R3; the `MetadataDB` class alone is 4,018 lines
|
`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
|
and is a monolith in its own right, to be split per-table once the router train
|
||||||
|
|||||||
+4
-1
@@ -70,8 +70,11 @@ audio_effect_mappings = None
|
|||||||
# `setattr(server, …)` in a few tests, so those slots (when added) need their
|
# `setattr(server, …)` in a few tests, so those slots (when added) need their
|
||||||
# tests retargeted to appstate in the same PR.
|
# tests retargeted to appstate in the same PR.
|
||||||
config_dir = None
|
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:
|
def configure(**kwargs) -> None:
|
||||||
|
|||||||
@@ -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 `<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
|
||||||
@@ -67,6 +67,7 @@ from metadata_db import (
|
|||||||
# its own module, the `audio_effect_mappings` singleton below stays here.
|
# its own module, the `audio_effect_mappings` singleton below stays here.
|
||||||
from audio_effects_db import AudioEffectsMappingDB
|
from audio_effects_db import AudioEffectsMappingDB
|
||||||
from reqfields import _clean_str
|
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
|
# The router seam. Imported as a module (never `from appstate import ...`) so
|
||||||
# `appstate.configure(...)` below publishes into the same namespace routers read.
|
# `appstate.configure(...)` below publishes into the same namespace routers read.
|
||||||
# Lives in lib/ because that is the one core dir every packaging path copies.
|
# Lives in lib/ because that is the one core dir every packaging path copies.
|
||||||
@@ -368,6 +369,8 @@ appstate.configure(
|
|||||||
meta_db=meta_db,
|
meta_db=meta_db,
|
||||||
audio_effect_mappings=audio_effect_mappings,
|
audio_effect_mappings=audio_effect_mappings,
|
||||||
config_dir=CONFIG_DIR,
|
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")
|
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 ──────────────────────────────────────────────────
|
# ── 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 `<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
|
|
||||||
|
|
||||||
|
|
||||||
def _pick_smart_arrangement(
|
def _pick_smart_arrangement(
|
||||||
|
|||||||
Reference in New Issue
Block a user