From 1b6dc2d25f1640b6c67c467d36cdb31fff539fa0 Mon Sep 17 00:00:00 2001 From: chodeus Date: Thu, 13 Aug 2026 19:42:07 +0800 Subject: [PATCH] refactor(api): media_api queries move behind named interface methods (#523) * refactor(api): media_api queries move behind named interface methods All 11 raw-SQL sites route through domain-named owners (12 new methods; media_edit_history and poster_collections gain their owning interfaces), restoring the DBWorker table allowlist these calls bypassed. Genuine-only non-200 response docs ride along. path_safety.resolve_confined() gives path confinement one documented owner (the recurring py/path-injection FP anchor) and the regression-test import is single-style again. * fix(codeql): normalize resolve_confined via os.path.realpath py/path-injection models Path.resolve() as a filesystem sink, so the confinement owner minted alert 312 itself; os.path.realpath is the same normalization outside the sink model. Docstring to the one-line cap. Alert 313: pass ChubConfig directly instead of a lambda wrapper. * fix(review): collection key, id recovery, purge truth, deterministic pages get_by_title_and_instance carries library_name (unique-key member; IS ? matches the NULL-library row). create_collection returns its insert id so the tag flow can't adopt a concurrent namesake's row. delete_by_ids chunks at 500 and the purge route reports rows actually deleted. Shared is_missing_value predicate owns the empty-field rule. find_low_rated, find_incomplete_metadata and get_edit_history gain unique tiebreakers (one metadata PUT writes several rows with one timestamp). find_by_tag matches the quoted JSON element, not any substring. Chunk test reads the build's real variable limit instead of assuming the compile default. --- backend/api/media_api.py | 235 +++--------- backend/api/posters.py | 11 +- backend/util/database/__init__.py | 12 +- backend/util/database/collection_cache.py | 13 + backend/util/database/media_cache.py | 206 ++++++++++ backend/util/database/poster_cache.py | 30 ++ backend/util/path_safety.py | 13 +- tests/test_media_api_queries.py | 448 ++++++++++++++++++++++ tests/test_path_safety.py | 35 ++ tests/test_regression_review_2026.py | 5 +- 10 files changed, 816 insertions(+), 192 deletions(-) create mode 100644 tests/test_media_api_queries.py diff --git a/backend/api/media_api.py b/backend/api/media_api.py index dbb8b3f3..2c40e41d 100644 --- a/backend/api/media_api.py +++ b/backend/api/media_api.py @@ -29,7 +29,12 @@ ) from backend.util.arr import create_arr_client from backend.util.config import ConfigError, load_config -from backend.util.database import ChubDB, escape_like +from backend.util.database import ( + INCOMPLETE_METADATA_FIELDS, + NEVER_POPULATED_FIELDS, + ChubDB, + is_missing_value, +) from backend.util.ssrf_guard import is_safe_url, safe_external_get router = APIRouter( @@ -754,7 +759,9 @@ async def delete_collection( } } }, - } + }, + 400: {"description": "Missing 'title' or 'instance_name'"}, + 413: {"description": "Request body too large"}, }, ) async def create_collection( @@ -809,10 +816,8 @@ async def create_collection( db.collection.upsert(record, instance_name) # Fetch the created/updated record - created = db.collection.execute_query( - "SELECT * FROM collections_cache WHERE title=? AND instance_name=?", - (title, instance_name), - fetch_one=True, + created = db.collection.get_by_title_and_instance( + title, instance_name, record["library_name"] ) return ok("Collection created successfully", {"collection": created}) @@ -1169,23 +1174,12 @@ async def get_low_rated( logger: Any = Depends(get_logger), db: ChubDB = Depends(get_database), ) -> JSONResponse: + """Return media rated below `max_rating`, lowest first.""" try: limit = max(1, min(limit, 500)) offset = max(0, offset) - clauses = ["rating IS NOT NULL", "rating != ''", "CAST(rating AS REAL) < ?"] - params: list = [float(max_rating)] - if asset_type: - clauses.append("asset_type=?") - params.append(asset_type) - where = "WHERE " + " AND ".join(clauses) - rows = ( - db.worker.execute_query( - f"SELECT * FROM media_cache {where} " - "ORDER BY CAST(rating AS REAL) ASC LIMIT ? OFFSET ?", - tuple(params) + (limit, offset), - fetch_all=True, - ) - or [] + rows = db.media.find_low_rated( + max_rating, limit=limit, offset=offset, asset_type=asset_type ) return ok( f"Found {len(rows)} low-rated items", @@ -1200,26 +1194,11 @@ async def get_low_rated( ) -# Fields the ARR normalize layer never populates for a given asset_type, so -# flagging them as "missing" is a false positive. Radarr has no tvdbId, -# Sonarr has no tmdbId, Lidarr (artist) uses MusicBrainz IDs and leaves -# tmdb/tvdb/imdb + rating/runtime/language/edition as None by design. -_NEVER_POPULATED_FIELDS = { - "movie": {"tvdb_id"}, - "show": {"tmdb_id"}, - "artist": { - "tmdb_id", - "tvdb_id", - "imdb_id", - "rating", - "runtime", - "language", - "edition", - }, -} - - -@router.get("/incomplete-metadata", summary="List media with missing key metadata") +@router.get( + "/incomplete-metadata", + summary="List media with missing key metadata", + responses={400: {"description": "No valid fields requested"}}, +) async def get_incomplete_metadata( fields: str = "rating,studio,language,genre", limit: int = 200, @@ -1227,20 +1206,13 @@ async def get_incomplete_metadata( logger: Any = Depends(get_logger), db: ChubDB = Depends(get_database), ) -> JSONResponse: + """Return rows missing expected metadata, each annotated with what it lacks.""" try: - allowed = { - "rating", - "studio", - "language", - "genre", - "runtime", - "edition", - "tmdb_id", - "tvdb_id", - "imdb_id", - "year", - } - requested = [f.strip() for f in fields.split(",") if f.strip() in allowed] + requested = [ + f.strip() + for f in fields.split(",") + if f.strip() in INCOMPLETE_METADATA_FIELDS + ] if not requested: return error( "No valid fields requested", @@ -1249,74 +1221,20 @@ async def get_incomplete_metadata( ) limit = max(1, min(limit, 1000)) offset = max(0, offset) - # INTEGER columns can't be empty-string; compare to NULL/0 instead. - int_cols = {"tmdb_id", "tvdb_id", "runtime"} - - # Build a per-asset-type WHERE: for each known asset_type, only - # include requested fields that type can actually populate. A row - # matches if its asset_type has at least one null/empty expected - # field. Rows with unrecognised asset_types fall through a catch-all - # that uses the raw requested list. - subclauses = [] - params: list = [] - known_types = ("movie", "show", "artist") - for atype in known_types: - never = _NEVER_POPULATED_FIELDS.get(atype, set()) - effective = [f for f in requested if f not in never] - if not effective: - continue - field_clauses = [] - for f in effective: - if f in int_cols: - field_clauses.append(f"({f} IS NULL OR {f} = 0)") - else: - field_clauses.append(f"({f} IS NULL OR {f} = '')") - subclauses.append(f"(asset_type = ? AND ({' OR '.join(field_clauses)}))") - params.append(atype) - - # Fallback for any asset_type outside the known set — apply the full - # requested list as before. Keeps forward-compat if we add types. - unknown_field_clauses = [] - for f in requested: - if f in int_cols: - unknown_field_clauses.append(f"({f} IS NULL OR {f} = 0)") - else: - unknown_field_clauses.append(f"({f} IS NULL OR {f} = '')") - unknown_placeholders = ",".join(["?"] * len(known_types)) - subclauses.append( - f"(asset_type NOT IN ({unknown_placeholders}) " - f"AND ({' OR '.join(unknown_field_clauses)}))" - ) - params.extend(known_types) - - where = " OR ".join(subclauses) - rows = ( - db.worker.execute_query( - f"SELECT * FROM media_cache WHERE {where} " - "ORDER BY title ASC LIMIT ? OFFSET ?", - (*params, limit, offset), - fetch_all=True, - ) - or [] - ) + rows = db.media.find_incomplete_metadata(requested, limit=limit, offset=offset) # Compute per-row `missing` server-side using the same expected-field # map, so the UI doesn't have to replicate the logic. items = [] for r in rows: row = dict(r) - never = _NEVER_POPULATED_FIELDS.get(row.get("asset_type") or "", set()) + never = NEVER_POPULATED_FIELDS.get(row.get("asset_type") or "", set()) missing = [] for f in requested: if f in never: continue - val = row.get(f) - if f in int_cols: - if val is None or val == 0: - missing.append(f) - else: - if val is None or val == "": - missing.append(f) + if is_missing_value(f, row.get(f)): + missing.append(f) row["missing"] = missing items.append(row) @@ -1379,14 +1297,7 @@ def get_orphaned_cache( try: config = load_config() live = _live_arr_ids_by_instance(config, logger) - rows = ( - db.worker.execute_query( - "SELECT id, title, asset_type, instance_name, arr_id, folder, root_folder " - "FROM media_cache WHERE arr_id IS NOT NULL AND instance_name IS NOT NULL", - fetch_all=True, - ) - or [] - ) + rows = db.media.get_arr_linked() orphaned = [ dict(r) for r in rows @@ -1608,12 +1519,17 @@ def _resolve(mid): ) -@router.post("/orphaned/purge", summary="Delete orphaned cache rows by id") +@router.post( + "/orphaned/purge", + summary="Delete orphaned cache rows by id", + responses={400: {"description": "No ids provided"}}, +) async def purge_orphaned_cache( body: OrphanedPurgeRequest, logger: Any = Depends(get_logger), db: ChubDB = Depends(get_database), ) -> JSONResponse: + """Delete the given media cache rows (the orphaned-cache view's purge action).""" try: if not body.ids: return error( @@ -1621,15 +1537,11 @@ async def purge_orphaned_cache( code="NO_IDS", status_code=400, ) - placeholders = ",".join("?" for _ in body.ids) - db.worker.execute_query( - f"DELETE FROM media_cache WHERE id IN ({placeholders})", - tuple(body.ids), - ) - logger.info(f"Purged {len(body.ids)} orphaned cache row(s)") + purged = db.media.delete_by_ids(body.ids) + logger.info(f"Purged {purged} orphaned cache row(s)") return ok( - f"Purged {len(body.ids)} row(s) from cache", - {"purged": len(body.ids)}, + f"Purged {purged} row(s) from cache", + {"purged": purged}, ) except Exception as e: logger.error(f"Error purging orphaned cache: {e}", exc_info=True) @@ -1727,6 +1639,7 @@ async def get_media_item( }, 400: {"description": "No valid fields provided"}, 404: {"description": "Media item not found"}, + 413: {"description": "Request body too large"}, }, ) async def update_media_metadata( @@ -1805,18 +1718,8 @@ async def update_media_metadata( if old_value == new_value: continue try: - db.worker.execute_query( - "INSERT INTO media_edit_history " - "(media_id, edited_at, edited_by, field, old_value, new_value) " - "VALUES (?, ?, ?, ?, ?, ?)", - ( - media_id, - now_iso, - edited_by, - field, - None if old_value is None else str(old_value), - None if new_value is None else str(new_value), - ), + db.media.record_edit( + media_id, now_iso, edited_by, field, old_value, new_value ) except Exception as audit_err: logger.debug(f"Audit insert failed ({field}): {audit_err}") @@ -2050,6 +1953,10 @@ def _delete_from_arr() -> None: description="Create a new poster_collection containing every poster_cache " "row whose matching media_cache entry has the given tag in its tags field. " "Name defaults to the tag name.", + responses={ + 400: {"description": "Missing 'tag'"}, + 413: {"description": "Request body too large"}, + }, ) async def generate_collection_from_tag( request: Request, @@ -2066,18 +1973,7 @@ async def generate_collection_from_tag( return error("tag required", code="TAG_REQUIRED", status_code=400) name = (payload.get("name") or tag).strip() - # media_cache.tags is stored as serialized JSON in most paths, so we - # match with LIKE on the raw string — good enough for a low-cardinality - # tag list, and avoids requiring JSON1 extension support. - media_rows = ( - db.worker.execute_query( - "SELECT id, tmdb_id, tvdb_id, imdb_id, season_number, title, year " - "FROM media_cache WHERE tags LIKE ? ESCAPE '\\'", - (f"%{escape_like(tag)}%",), - fetch_all=True, - ) - or [] - ) + media_rows = db.media.find_by_tag(tag) if not media_rows: return ok( f"No media tagged '{tag}'", @@ -2107,31 +2003,13 @@ async def generate_collection_from_tag( from datetime import datetime as _dt created_at = _dt.utcnow().isoformat() - db.worker.execute_query( - "INSERT INTO poster_collections (name, description, created_at) VALUES (?, ?, ?)", - (name, f"Auto-generated from tag '{tag}'", created_at), - ) - # Fetch new collection id - row = db.worker.execute_query( - "SELECT id FROM poster_collections WHERE name=? ORDER BY id DESC LIMIT 1", - (name,), - fetch_one=True, + coll_id = db.poster.create_collection( + name, f"Auto-generated from tag '{tag}'", created_at ) - coll_id = row["id"] if row else None - if coll_id is None: - return error( - "Could not determine new collection id", - code="COLLECTION_CREATE_ERROR", - status_code=500, - ) for pid in poster_ids: try: - db.worker.execute_query( - "INSERT OR IGNORE INTO poster_collection_items " - "(collection_id, poster_id) VALUES (?, ?)", - (coll_id, pid), - ) + db.poster.add_collection_item(coll_id, pid) except Exception as ins_err: logger.debug(f"Skipping poster_id={pid}: {ins_err}") @@ -2159,16 +2037,9 @@ async def get_media_history( logger: Any = Depends(get_logger), db: ChubDB = Depends(get_database), ) -> JSONResponse: + """Return a media item's metadata-edit audit trail, newest first.""" try: - rows = ( - db.worker.execute_query( - "SELECT * FROM media_edit_history WHERE media_id=? " - "ORDER BY edited_at DESC LIMIT ?", - (media_id, max(1, min(limit, 500))), - fetch_all=True, - ) - or [] - ) + rows = db.media.get_edit_history(media_id, max(1, min(limit, 500))) return ok( f"Retrieved {len(rows)} history entries for media {media_id}", {"history": [dict(r) for r in rows]}, diff --git a/backend/api/posters.py b/backend/api/posters.py index ec8a2de3..14a2fcb6 100644 --- a/backend/api/posters.py +++ b/backend/api/posters.py @@ -2632,7 +2632,7 @@ async def preview_poster_file( # Restrict the served file to configured allowed roots — otherwise # an authenticated caller could point at arbitrary dirs (/etc, /root). from backend.util.config import load_config - from backend.util.path_safety import is_path_allowed + from backend.util.path_safety import is_path_allowed, resolve_confined try: config = load_config() @@ -2654,11 +2654,10 @@ async def preview_poster_file( # Assets Search grid passes item.file straight through, with # item.folder in location as an owner label, not a root). # Validate the file path itself against allowed roots instead - # of demanding that `location` is a root. - # Authorize the RESOLVED path: a symlink inside an allowed root - # must not serve whatever it points at outside the roots. - file_path = path_obj.resolve() - if not is_path_allowed(str(file_path), config): + # of demanding that `location` is a root. resolve_confined + # authorizes the RESOLVED path (symlink escapes included). + file_path = resolve_confined(path, config) + if file_path is None: return error( "Access denied - path outside allowed directory", code="PATH_TRAVERSAL_DENIED", diff --git a/backend/util/database/__init__.py b/backend/util/database/__init__.py index 4e5c82b1..ed3279d3 100644 --- a/backend/util/database/__init__.py +++ b/backend/util/database/__init__.py @@ -12,7 +12,13 @@ from .db_base import DatabaseBase, escape_like from .holiday import HolidayStatus from .media_asset_matches import MediaAssetMatches -from .media_cache import MediaCache +from .media_cache import ( + INCOMPLETE_METADATA_FIELDS, + INCOMPLETE_METADATA_INT_FIELDS, + NEVER_POPULATED_FIELDS, + MediaCache, + is_missing_value, +) from .plex_cache import PlexCache from .poster_cache import PosterCache from .run_state import RunState @@ -414,6 +420,10 @@ def my_db_operation(db): "MediaCache", "MediaAssetMatches", "WebhookCache", + "INCOMPLETE_METADATA_FIELDS", + "INCOMPLETE_METADATA_INT_FIELDS", + "NEVER_POPULATED_FIELDS", + "is_missing_value", "escape_like", "with_database", ] diff --git a/backend/util/database/collection_cache.py b/backend/util/database/collection_cache.py index dbca5833..16bc25f8 100755 --- a/backend/util/database/collection_cache.py +++ b/backend/util/database/collection_cache.py @@ -123,6 +123,19 @@ def get_by_id(self, id: int) -> Optional[dict]: "SELECT * FROM collections_cache WHERE id=?", (id,), fetch_one=True ) + def get_by_title_and_instance( + self, title: str, instance_name: str, library_name: Optional[str] = None + ) -> Optional[dict]: + """Return one collection row for a title within an instance, or None.""" + # library_name is part of the unique key (see upsert's ON CONFLICT) — + # `IS ?` so a None argument matches the NULL-library row. + return self.execute_query( + "SELECT * FROM collections_cache " + "WHERE title=? AND instance_name=? AND library_name IS ?", + (title, instance_name, library_name), + fetch_one=True, + ) + def get_all(self) -> list: """Return all records from collections_cache as a list of dicts.""" return ( diff --git a/backend/util/database/media_cache.py b/backend/util/database/media_cache.py index f94b0656..383698cc 100755 --- a/backend/util/database/media_cache.py +++ b/backend/util/database/media_cache.py @@ -69,6 +69,53 @@ "SUM(CASE WHEN asset_type='artist' AND monitored=1 THEN 1 ELSE 0 END)" ) +# Columns find_incomplete_metadata may test, and the subset stored as INTEGER +# (they can't hold '', so they compare against NULL/0 instead). +INCOMPLETE_METADATA_FIELDS = frozenset( + { + "rating", + "studio", + "language", + "genre", + "runtime", + "edition", + "tmdb_id", + "tvdb_id", + "imdb_id", + "year", + } +) +INCOMPLETE_METADATA_INT_FIELDS = frozenset({"tmdb_id", "tvdb_id", "runtime"}) + + +def is_missing_value(field: str, value) -> bool: + """True when `value` is missing for `field` (INT fields: None/0; else None/'').""" + if field in INCOMPLETE_METADATA_INT_FIELDS: + return value is None or value == 0 + return value is None or value == "" + + +# Fields the ARR normalize layer never populates for a given asset_type, so +# flagging them as "missing" is a false positive. Radarr has no tvdbId, +# Sonarr has no tmdbId, Lidarr (artist) uses MusicBrainz IDs and leaves +# tmdb/tvdb/imdb + rating/runtime/language/edition as None by design. +NEVER_POPULATED_FIELDS = { + "movie": {"tvdb_id"}, + "show": {"tmdb_id"}, + "artist": { + "tmdb_id", + "tvdb_id", + "imdb_id", + "rating", + "runtime", + "language", + "edition", + }, +} +# asset_types with their own expected-field rules; anything else matches +# find_incomplete_metadata's catch-all clause. +_KNOWN_ASSET_TYPES = ("movie", "show", "artist") + class MediaCache(DatabaseBase): """ @@ -613,6 +660,26 @@ def delete_by_id(self, id: int) -> None: """Delete a single record by its unique integer ID.""" self.execute_query("DELETE FROM media_cache WHERE id=?", (id,)) + def delete_by_ids(self, ids: List[int]) -> int: + """Delete records by integer ID; returns rows deleted.""" + # No round trip for an empty list (SQLite accepts `IN ()` and deletes + # nothing) — and the empty case must never fall through to a bare DELETE. + if not ids: + return 0 + # Chunked: one placeholder per id would blow SQLITE_MAX_VARIABLE_NUMBER. + deleted = 0 + for start in range(0, len(ids), 500): + chunk = ids[start : start + 500] + placeholders = ",".join("?" for _ in chunk) + deleted += ( + self.execute_query( + f"DELETE FROM media_cache WHERE id IN ({placeholders})", + tuple(chunk), + ) + or 0 + ) + return deleted + def get_by_keys( self, asset_type: str, @@ -1502,6 +1569,145 @@ def find_folder_collisions(self, asset_type: Optional[str] = None) -> list: collisions.sort(key=lambda c: c["count"], reverse=True) return collisions + def find_low_rated( + self, + max_rating: float, + limit: int = 100, + offset: int = 0, + asset_type: Optional[str] = None, + ) -> list: + """Return rows whose rating casts numerically below `max_rating`, lowest first.""" + clauses = ["rating IS NOT NULL", "rating != ''", "CAST(rating AS REAL) < ?"] + params: list = [float(max_rating)] + if asset_type: + clauses.append("asset_type=?") + params.append(asset_type) + where = "WHERE " + " AND ".join(clauses) + return ( + self.execute_query( + f"SELECT * FROM media_cache {where} " + "ORDER BY CAST(rating AS REAL) ASC, id ASC LIMIT ? OFFSET ?", + tuple(params) + (limit, offset), + fetch_all=True, + ) + or [] + ) + + @staticmethod + def _empty_field_clauses(fields: List[str]) -> str: + """OR-joined "is null or blank" test for each field, INTEGER-aware.""" + # SQL mirror of is_missing_value — keep in sync. + return " OR ".join( + f"({f} IS NULL OR {f} = 0)" + if f in INCOMPLETE_METADATA_INT_FIELDS + else f"({f} IS NULL OR {f} = '')" + for f in fields + ) + + def find_incomplete_metadata( + self, fields: List[str], limit: int = 200, offset: int = 0 + ) -> list: + """Return rows missing any of `fields` that their asset_type can populate.""" + # Field names are interpolated into SQL — drop anything off the + # column allow-list before building the clause. + requested = [f for f in fields if f in INCOMPLETE_METADATA_FIELDS] + if not requested: + return [] + + # One clause per known asset_type, using only the fields that type can + # populate; a catch-all keeps unrecognised types matching on the lot. + subclauses = [] + params: list = [] + for atype in _KNOWN_ASSET_TYPES: + never = NEVER_POPULATED_FIELDS.get(atype, set()) + effective = [f for f in requested if f not in never] + if not effective: + continue + subclauses.append( + f"(asset_type = ? AND ({self._empty_field_clauses(effective)}))" + ) + params.append(atype) + + unknown_placeholders = ",".join(["?"] * len(_KNOWN_ASSET_TYPES)) + subclauses.append( + f"(asset_type NOT IN ({unknown_placeholders}) " + f"AND ({self._empty_field_clauses(requested)}))" + ) + params.extend(_KNOWN_ASSET_TYPES) + + where = " OR ".join(subclauses) + return ( + self.execute_query( + f"SELECT * FROM media_cache WHERE {where} " + "ORDER BY title ASC, id ASC LIMIT ? OFFSET ?", + (*params, limit, offset), + fetch_all=True, + ) + or [] + ) + + def get_arr_linked(self) -> list: + """Return the identity/location fields of every row sourced from an *arr.""" + return ( + self.execute_query( + "SELECT id, title, asset_type, instance_name, arr_id, folder, " + "root_folder FROM media_cache " + "WHERE arr_id IS NOT NULL AND instance_name IS NOT NULL", + fetch_all=True, + ) + or [] + ) + + def find_by_tag(self, tag: str) -> list: + """Return the identifier fields of every row whose tags contain `tag`.""" + # tags is written with json.dumps, so match the quoted element — a bare + # substring would also hit tags that merely start with `tag`. + return ( + self.execute_query( + "SELECT id, tmdb_id, tvdb_id, imdb_id, season_number, title, year " + "FROM media_cache WHERE tags LIKE ? ESCAPE '\\'", + (f'%"{escape_like(tag)}"%',), + fetch_all=True, + ) + or [] + ) + + def record_edit( + self, + media_id: int, + edited_at: str, + edited_by: str, + field: str, + old_value: Any, + new_value: Any, + ) -> None: + """Append one field's old→new change to the media edit audit trail.""" + self.execute_query( + "INSERT INTO media_edit_history " + "(media_id, edited_at, edited_by, field, old_value, new_value) " + "VALUES (?, ?, ?, ?, ?, ?)", + ( + media_id, + edited_at, + edited_by, + field, + None if old_value is None else str(old_value), + None if new_value is None else str(new_value), + ), + ) + + def get_edit_history(self, media_id: int, limit: int = 100) -> list: + """Return one media item's edit audit trail, newest first.""" + return ( + self.execute_query( + "SELECT * FROM media_edit_history WHERE media_id=? " + "ORDER BY edited_at DESC, id DESC LIMIT ?", + (media_id, limit), + fetch_all=True, + ) + or [] + ) + def sync_for_instance( self, instance_name: str, diff --git a/backend/util/database/poster_cache.py b/backend/util/database/poster_cache.py index 529222cd..a2af1286 100755 --- a/backend/util/database/poster_cache.py +++ b/backend/util/database/poster_cache.py @@ -690,3 +690,33 @@ def get_candidates_by_prefix( sql += " ORDER BY priority DESC, id DESC" return self.execute_query(sql, params, fetch_all=True, conn=conn) or [] + + # --- poster_collections: user-curated sets of poster_cache rows --- + + def create_collection( + self, name: str, description: Optional[str], created_at: str + ) -> int: + """Insert a poster collection and return its new id.""" + return self.execute_query( + "INSERT INTO poster_collections (name, description, created_at) " + "VALUES (?, ?, ?)", + (name, description, created_at), + last_row_id=True, + ) + + def get_collection_id_by_name(self, name: str) -> Optional[int]: + """Id of the most recently created collection with this name, or None.""" + row = self.execute_query( + "SELECT id FROM poster_collections WHERE name=? ORDER BY id DESC LIMIT 1", + (name,), + fetch_one=True, + ) + return row["id"] if row else None + + def add_collection_item(self, collection_id: int, poster_id: int) -> None: + """Add a poster to a collection; an already-present pair is ignored.""" + self.execute_query( + "INSERT OR IGNORE INTO poster_collection_items " + "(collection_id, poster_id) VALUES (?, ?)", + (collection_id, poster_id), + ) diff --git a/backend/util/path_safety.py b/backend/util/path_safety.py index 2cbba117..c578fac5 100644 --- a/backend/util/path_safety.py +++ b/backend/util/path_safety.py @@ -7,7 +7,7 @@ import os from pathlib import Path -from typing import List +from typing import List, Optional from backend.util.config import ChubConfig @@ -238,3 +238,14 @@ def is_path_allowed(path: str, config: ChubConfig) -> bool: continue return False + + +def resolve_confined(path: str, config: ChubConfig) -> Optional[Path]: + """Resolve *path* and return it only when the resolved target is inside an allowed root, else None.""" + if not path or not isinstance(path, str): + return None + try: + resolved = os.path.realpath(os.path.expanduser(path)) + except (ValueError, OSError): + return None + return Path(resolved) if is_path_allowed(resolved, config) else None diff --git a/tests/test_media_api_queries.py b/tests/test_media_api_queries.py new file mode 100644 index 00000000..350b31d2 --- /dev/null +++ b/tests/test_media_api_queries.py @@ -0,0 +1,448 @@ +"""Tests for the media_api query methods that moved into the DB interfaces. + +Two layers: the new MediaCache / CollectionCache / PosterCache methods against a +real temp database, then the handlers whose contract nothing else pinned. +""" + +import os +import sqlite3 +import sys + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) + +from backend.util.config import ChubConfig, ConfigError # noqa: E402 +from backend.util.database import ChubDB # noqa: E402 + + +class _StubLog: + """Swallows every log call; get_adapter returns itself.""" + + def __getattr__(self, _): + """Any log method is a no-op.""" + return lambda *a, **k: None + + def get_adapter(self, *_a, **_kw): + """Adapters are the same sink.""" + return self + + +@pytest.fixture +def db(tmp_path): + """A ChubDB backed by a real (temporary) sqlite file.""" + with ChubDB(_StubLog(), db_path=str(tmp_path / "chub.db")) as database: + yield database + + +def _client(db): + """Mount the media router on a bare app carrying main.py's ConfigError handler.""" + import backend.api.main as apimain + import backend.api.media_api as media_api + + app = FastAPI() + app.state.logger = _StubLog() + app.state.db = db + app.add_exception_handler(ConfigError, apimain.handle_config_error) + app.include_router(media_api.router) + return TestClient(app, raise_server_exceptions=False) + + +def _seed(db, title, asset_type="movie", instance_name="radarr", **fields): + """Upsert one media row and return its integer id.""" + item = {"title": title, "normalized_title": title.lower(), "year": 2021, **fields} + db.media.upsert(item, asset_type, "radarr", instance_name) + row = db.media.execute_query( + "SELECT id FROM media_cache WHERE title=? AND instance_name=?", + (title, instance_name), + fetch_one=True, + ) + return row["id"] + + +# --- MediaCache.find_low_rated --------------------------------------------- + + +def test_find_low_rated_orders_and_excludes_blank_ratings(db): + """Only numerically-rated rows below the threshold, worst first.""" + _seed(db, "Bad", rating="2.5") + _seed(db, "Worse", rating="1.0") + _seed(db, "Good", rating="9.0") + _seed(db, "Unrated") # rating NULL + _seed(db, "Blank", rating="") # `or None` in upsert stores NULL + + rows = db.media.find_low_rated(5.0) + assert [r["title"] for r in rows] == ["Worse", "Bad"] + + +def test_find_low_rated_filters_by_asset_type_and_paginates(db): + """asset_type narrows the set; limit/offset walk it in rating order.""" + _seed(db, "Cheap Movie", rating="1.0") + _seed(db, "Cheap Show", asset_type="show", rating="2.0") + + assert [r["title"] for r in db.media.find_low_rated(5.0, asset_type="show")] == [ + "Cheap Show" + ] + page = db.media.find_low_rated(5.0, limit=1, offset=1) + assert [r["title"] for r in page] == ["Cheap Show"] + + +# --- MediaCache.find_incomplete_metadata ----------------------------------- + + +def test_find_incomplete_metadata_skips_never_populated_fields(db): + """A movie can't have a tvdb_id, so its absence must not flag the row.""" + _seed(db, "Movie", tvdb_id=None) + _seed(db, "Show", asset_type="show", tvdb_id=None) + + titles = {r["title"] for r in db.media.find_incomplete_metadata(["tvdb_id"])} + assert titles == {"Show"} + + +def test_find_incomplete_metadata_catch_all_covers_unknown_types(db): + """An asset_type outside the known set is matched on the raw field list.""" + _seed(db, "Album", asset_type="album", tvdb_id=None) + + titles = {r["title"] for r in db.media.find_incomplete_metadata(["tvdb_id"])} + assert titles == {"Album"} + + +def test_find_incomplete_metadata_rejects_fields_off_the_allow_list(db): + """Field names reach SQL by interpolation — anything unknown is dropped.""" + _seed(db, "Movie") + + assert db.media.find_incomplete_metadata(["bogus_column"]) == [] + assert db.media.find_incomplete_metadata(["rating; DROP TABLE media_cache--"]) == [] + # The table is still there, and a valid request still works. + assert [r["title"] for r in db.media.find_incomplete_metadata(["studio"])] == [ + "Movie" + ] + + +def test_find_incomplete_metadata_orders_by_title_and_paginates(db): + """Rows come back title-ascending so limit/offset paginate deterministically.""" + for title in ("Charlie", "Alpha", "Bravo"): + _seed(db, title) + + page1 = db.media.find_incomplete_metadata(["studio"], limit=2) + page2 = db.media.find_incomplete_metadata(["studio"], offset=2) + assert [r["title"] for r in page1] == ["Alpha", "Bravo"] + assert [r["title"] for r in page2] == ["Charlie"] + + +# --- MediaCache.get_arr_linked --------------------------------------------- + + +def test_get_arr_linked_needs_both_arr_id_and_instance(db): + """Rows without an arr_id can't be checked against a live *arr.""" + _seed(db, "Linked", arr_id=7) + _seed(db, "Unlinked") + + rows = db.media.get_arr_linked() + assert [r["title"] for r in rows] == ["Linked"] + assert set(rows[0]) == { + "id", + "title", + "asset_type", + "instance_name", + "arr_id", + "folder", + "root_folder", + } + + +# --- MediaCache.find_by_tag ------------------------------------------------ + + +def test_find_by_tag_escapes_like_metacharacters(db): + """`_` in a tag must be literal, not a single-character wildcard.""" + _seed(db, "Dune", tags=["4K_UHD"]) + _seed(db, "Sicario", tags=["4KxUHD"]) + + rows = db.media.find_by_tag("4K_UHD") + assert [r["title"] for r in rows] == ["Dune"] + assert set(rows[0]) == { + "id", + "tmdb_id", + "tvdb_id", + "imdb_id", + "season_number", + "title", + "year", + } + + +def test_find_by_tag_ignores_tags_the_request_is_a_prefix_of(db): + """A tag that prefixes another one matches only its own row.""" + _seed(db, "Dune", tags=["4K_UHD"]) + _seed(db, "Sicario", tags=["4K"]) + + assert [r["title"] for r in db.media.find_by_tag("4K")] == ["Sicario"] + + +# --- MediaCache.delete_by_ids ---------------------------------------------- + + +def test_delete_by_ids_removes_only_the_listed_rows(db): + """The unlisted row survives.""" + doomed = _seed(db, "Doomed") + keeper = _seed(db, "Keeper") + + assert db.media.delete_by_ids([doomed]) == 1 + assert db.media.get_by_id(doomed) is None + assert db.media.get_by_id(keeper) is not None + + +def test_delete_by_ids_no_ops_on_an_empty_list(db): + """An empty list must not degrade into a whole-table DELETE.""" + keeper = _seed(db, "Keeper") + + assert db.media.delete_by_ids([]) == 0 + assert db.media.get_by_id(keeper) is not None + + +def test_delete_by_ids_chunks_past_the_sqlite_variable_limit(db): + """Exceed this build's variable limit, so an unchunked single statement would raise.""" + with db.media.get_connection() as conn: + limit = conn.getlimit(sqlite3.SQLITE_LIMIT_VARIABLE_NUMBER) + real = [_seed(db, t) for t in ("One", "Two", "Three")] + absent = list(range(100_000, 100_000 + limit)) + + assert db.media.delete_by_ids(real + absent) == 3 + + +# --- MediaCache.record_edit / get_edit_history ------------------------------ + + +def test_edit_history_round_trips_newest_first(db): + """History reads back newest-first and honours the limit.""" + mid = _seed(db, "Dune") + db.media.record_edit(mid, "2026-01-01T00:00:00", "dean", "rating", "5", "6") + db.media.record_edit(mid, "2026-02-01T00:00:00", "dean", "studio", None, "A24") + + rows = db.media.get_edit_history(mid) + assert [r["field"] for r in rows] == ["studio", "rating"] + assert rows[0]["old_value"] is None + assert rows[0]["new_value"] == "A24" + assert len(db.media.get_edit_history(mid, limit=1)) == 1 + + +def test_record_edit_stringifies_values_but_keeps_none(db): + """Non-text values are stored as TEXT; None stays a genuine NULL.""" + mid = _seed(db, "Dune") + db.media.record_edit(mid, "2026-01-01T00:00:00", "dean", "runtime", 90, None) + + row = db.media.get_edit_history(mid)[0] + assert row["old_value"] == "90" + assert row["new_value"] is None + + +def test_edit_history_is_scoped_to_one_media_id(db): + """Another item's edits never leak into this item's trail.""" + a = _seed(db, "Dune") + b = _seed(db, "Sicario") + db.media.record_edit(a, "2026-01-01T00:00:00", "dean", "rating", "5", "6") + db.media.record_edit(b, "2026-01-01T00:00:00", "dean", "rating", "1", "2") + + assert len(db.media.get_edit_history(a)) == 1 + + +# --- CollectionCache.get_by_title_and_instance ------------------------------ + + +def test_get_collection_by_title_and_instance_is_instance_scoped(db): + """The same collection title in another instance is a different row.""" + for instance in ("plex1", "plex2"): + db.collection.upsert({"title": "Marvel", "library_name": "Movies"}, instance) + + row = db.collection.get_by_title_and_instance("Marvel", "plex2", "Movies") + assert row["instance_name"] == "plex2" + assert db.collection.get_by_title_and_instance("Marvel", "plex3", "Movies") is None + + +def test_collection_lookup_distinguishes_libraries(db): + """library_name completes the unique key, so the lookup must carry it.""" + for library in ("Movies", "4K Movies", None): + db.collection.upsert({"title": "Marvel", "library_name": library}, "plex1") + + lookup = db.collection.get_by_title_and_instance + assert lookup("Marvel", "plex1", "4K Movies")["library_name"] == "4K Movies" + assert lookup("Marvel", "plex1", "Movies")["library_name"] == "Movies" + # No library requested still resolves the NULL-library row. + assert lookup("Marvel", "plex1")["library_name"] is None + + +# --- PosterCache poster_collections ---------------------------------------- + + +def test_poster_collection_create_and_resolve_id(db): + """create_collection returns each new row's id; an unknown name resolves to None.""" + first = db.poster.create_collection("Halloween", "spooky", "2026-01-01T00:00:00") + second = db.poster.create_collection("Xmas", "merry", "2026-01-02T00:00:00") + + assert first == db.poster.get_collection_id_by_name("Halloween") + assert second == db.poster.get_collection_id_by_name("Xmas") + assert second != first + assert db.poster.get_collection_id_by_name("Nope") is None + + +def test_poster_collection_id_lookup_prefers_the_newest_duplicate(db): + """Names aren't unique — the lookup must return the most recent row.""" + db.poster.create_collection("Halloween", "first", "2026-01-01T00:00:00") + first = db.poster.get_collection_id_by_name("Halloween") + db.poster.create_collection("Halloween", "second", "2026-02-01T00:00:00") + + assert db.poster.get_collection_id_by_name("Halloween") > first + + +def test_add_collection_item_ignores_a_duplicate_pair(db): + """The UNIQUE(collection_id, poster_id) pair is inserted once, not raised on.""" + db.poster.create_collection("Halloween", None, "2026-01-01T00:00:00") + coll_id = db.poster.get_collection_id_by_name("Halloween") + + db.poster.add_collection_item(coll_id, 42) + db.poster.add_collection_item(coll_id, 42) + + rows = db.poster.execute_query( + "SELECT poster_id FROM poster_collection_items WHERE collection_id=?", + (coll_id,), + fetch_all=True, + ) + assert [r["poster_id"] for r in rows] == [42] + + +# --- Route contracts not covered elsewhere --------------------------------- + + +def test_low_rated_route_returns_sorted_items(db): + """GET /low-rated echoes the clamped paging and sorts worst-first.""" + _seed(db, "Bad", rating="2.5") + _seed(db, "Worse", rating="1.0") + + body = _client(db).get("/api/media/low-rated?max_rating=5").json() + assert [i["title"] for i in body["data"]["items"]] == ["Worse", "Bad"] + assert body["data"]["limit"] == 100 and body["data"]["offset"] == 0 + + +def test_low_rated_route_forwards_its_filters(db): + """asset_type and limit reach the query, not just the echoed response.""" + _seed(db, "Cheap Movie", rating="1.0") + _seed(db, "Cheap Show", asset_type="show", rating="2.0") + client = _client(db) + + filtered = client.get("/api/media/low-rated?max_rating=5&asset_type=show").json() + assert [i["title"] for i in filtered["data"]["items"]] == ["Cheap Show"] + paged = client.get("/api/media/low-rated?max_rating=5&limit=1").json() + assert [i["title"] for i in paged["data"]["items"]] == ["Cheap Movie"] + + +def test_incomplete_metadata_route_rejects_unknown_fields(db): + """A fields list with nothing valid in it is a 400, not an empty 200.""" + resp = _client(db).get("/api/media/incomplete-metadata?fields=nope") + assert resp.status_code == 400 + assert resp.json()["error_code"] == "INVALID_FIELDS" + + +def test_incomplete_metadata_route_annotates_missing_per_row(db): + """`missing` lists only the fields that row's asset_type should populate.""" + _seed(db, "Movie", studio="A24") + + body = ( + _client(db) + .get("/api/media/incomplete-metadata?fields=studio,tvdb_id,language") + .json() + ) + item = body["data"]["items"][0] + assert item["missing"] == ["language"] # studio set, tvdb_id never populated + assert body["data"]["fields_checked"] == ["studio", "tvdb_id", "language"] + + +def test_orphaned_route_reports_rows_missing_from_their_arr(db, monkeypatch): + """A cached arr_id the live instance no longer lists is orphaned.""" + import backend.api.media_api as media_api + + _seed(db, "Gone", arr_id=9) + _seed(db, "Present", arr_id=5) + monkeypatch.setattr(media_api, "load_config", ChubConfig) + monkeypatch.setattr( + media_api, "_live_arr_ids_by_instance", lambda *_a: {"radarr": {5}} + ) + + body = _client(db).get("/api/media/orphaned").json() + assert [i["title"] for i in body["data"]["items"]] == ["Gone"] + + +def test_orphaned_purge_route_deletes_and_guards_empty_ids(db): + """POST /orphaned/purge deletes the listed rows; an empty list is a 400.""" + client = _client(db) + doomed = _seed(db, "Doomed") + keeper = _seed(db, "Keeper") + + assert client.post("/api/media/orphaned/purge", json={"ids": []}).status_code == 400 + resp = client.post("/api/media/orphaned/purge", json={"ids": [doomed, 999999]}) + assert resp.status_code == 200 + # The absent id must not inflate the count — `purged` is what was deleted. + assert resp.json()["data"]["purged"] == 1 + assert db.media.get_by_id(doomed) is None + assert db.media.get_by_id(keeper) is not None + + +def test_metadata_update_writes_one_audit_row_per_changed_field(db): + """Only fields whose value actually changed reach the audit trail.""" + client = _client(db) + mid = _seed(db, "Dune", rating="5.0") + + resp = client.put( + f"/api/media/{mid}/metadata", + json={"rating": "5.0", "studio": "A24", "language": "en"}, + ) + assert resp.status_code == 200 + + history = client.get(f"/api/media/{mid}/history").json()["data"]["history"] + assert {h["field"] for h in history} == {"studio", "language"} + assert {h["media_id"] for h in history} == {mid} + + +def test_collection_from_tag_route_fills_the_poster_collection(db): + """A tagged media row whose id matches a poster lands in the new collection.""" + _seed(db, "Dune", tags=["4K"], tmdb_id=438631) + db.poster.upsert( + { + "title": "Dune", + "normalized_title": "dune", + "year": 2021, + "tmdb_id": 438631, + "tvdb_id": None, + "imdb_id": None, + "season_number": None, + "folder": "Owner", + "file": "/src/Dune.png", + "asset_type": "movie", + } + ) + + body = _client(db).post("/api/media/collections/from-tag", json={"tag": "4K"}) + assert body.status_code == 200 + data = body.json()["data"] + assert data["poster_count"] == 1 + assert data["collection_id"] == db.poster.get_collection_id_by_name("4K") + items = db.poster.execute_query( + "SELECT poster_id FROM poster_collection_items WHERE collection_id=?", + (data["collection_id"],), + fetch_all=True, + ) + assert len(items) == 1 + + +def test_create_collection_route_reads_back_the_created_row(db): + """POST /collections returns the row it just upserted.""" + resp = _client(db).post( + "/api/media/collections", + json={"title": "Marvel", "instance_name": "plex1", "year": 2012}, + ) + assert resp.status_code == 200 + collection = resp.json()["data"]["collection"] + assert collection["title"] == "Marvel" + assert collection["instance_name"] == "plex1" diff --git a/tests/test_path_safety.py b/tests/test_path_safety.py index 94b617ff..d431240e 100644 --- a/tests/test_path_safety.py +++ b/tests/test_path_safety.py @@ -6,6 +6,7 @@ get_allowed_roots, get_browse_roots, is_path_allowed, + resolve_confined, ) @@ -235,3 +236,37 @@ def test_get_allowed_roots_includes_gdrive_list(empty_config, tmp_path): ] roots = get_allowed_roots(empty_config) assert any(str(r) == str(location.resolve()) for r in roots) + + +# --- resolve_confined --- + + +def test_resolve_confined_returns_the_resolved_path_inside_a_root(config_with_roots): + """An in-root path comes back resolved, ready to serve.""" + config, tmp_path = config_with_roots + inside = tmp_path / "posters_src" / "movie.jpg" + inside.write_text("x") + + assert resolve_confined(str(inside), config) == inside.resolve() + + +def test_resolve_confined_denies_a_symlink_escaping_the_roots(config_with_roots): + """The RESOLVED target is what's authorized, not the link's own location.""" + config, tmp_path = config_with_roots + secret = tmp_path / "secret.jpg" + secret.write_text("top-secret") + link = tmp_path / "posters_src" / "innocent.jpg" + link.symlink_to(secret) + + # The link itself sits inside an allowed root; its target does not. + assert resolve_confined(str(link), config) is None + + +def test_resolve_confined_denies_traversal_and_unusable_input(config_with_roots): + """`..` escapes, empty strings and non-strings all fail closed.""" + config, tmp_path = config_with_roots + + escape = str(tmp_path / "posters_src" / ".." / "x.jpg") + assert resolve_confined(escape, config) is None + assert resolve_confined("", config) is None + assert resolve_confined(None, config) is None # type: ignore[arg-type] diff --git a/tests/test_regression_review_2026.py b/tests/test_regression_review_2026.py index b827bd4d..84ba88e9 100644 --- a/tests/test_regression_review_2026.py +++ b/tests/test_regression_review_2026.py @@ -597,7 +597,8 @@ def test_duplicates_skips_malformed_exclude_group_members(db, monkeypatch): # 21. preview_poster_file must authorize the RESOLVED path — a symlink inside an # allowed root must not serve whatever it points at outside the roots. def test_poster_preview_denies_a_symlink_escaping_the_roots(tmp_path, monkeypatch): - from backend.api.posters import preview_poster_file + """A link inside an allowed root must not serve its target outside them.""" + import backend.api.posters as posters root = tmp_path / "posters" root.mkdir() @@ -623,6 +624,6 @@ def __getattr__(self, _n): return lambda *a, **k: None resp = asyncio.run( - preview_poster_file(location=str(root), path=str(link), logger=_Log()) + posters.preview_poster_file(location=str(root), path=str(link), logger=_Log()) ) assert getattr(resp, "status_code", 200) == 403, "symlink escape was served"