diff --git a/backend/app/plex/tmdb.py b/backend/app/plex/tmdb.py index d53277f0..466884bc 100644 --- a/backend/app/plex/tmdb.py +++ b/backend/app/plex/tmdb.py @@ -65,13 +65,15 @@ class TmdbNotConfigured(TmdbError): class TmdbClient: """Bound to the configured api_key + language. Use as a context manager so the httpx client closes.""" - def __init__(self, db: Session): + def __init__(self, db: Session, timeout: float = 30.0): self.key = sysconfig.get_str(db, "tmdb_api_key") if not self.key: raise TmdbNotConfigured("TMDB API key is not configured") self.base = settings.tmdb_api_base.rstrip("/") self.language = sysconfig.get_str(db, "tmdb_language") or settings.tmdb_language - self._http = httpx.Client(timeout=30.0) + # `timeout` is caller-tunable so the request-path lazy person enrich can cap how long it may + # block a worker thread (the batch job keeps the generous default). + self._http = httpx.Client(timeout=timeout) self._last = 0.0 def __enter__(self) -> "TmdbClient": @@ -97,13 +99,17 @@ class TmdbClient: try: r = self._http.get(f"{self.base}{path}", params=p) except httpx.HTTPError as e: - raise TmdbError(str(e)) from e + # NEVER put str(e)/the URL in the message: the request URL carries `?api_key=` in + # its query string, and this message is logged (enrich_batch). Report the type + path only. + raise TmdbError(f"TMDB request error on {path}: {type(e).__name__}") from None if r.status_code == 404: return None try: r.raise_for_status() except httpx.HTTPStatusError as e: - raise TmdbError(str(e)) from e + # Same reason: the HTTPStatusError string embeds the full URL incl. the api_key. Sever the + # cause (`from None`) so no traceback can resurface it, and log only status + path. + raise TmdbError(f"TMDB {path} returned {e.response.status_code}") from None try: return r.json() except ValueError as e: @@ -447,7 +453,10 @@ def enrich_person(db: Session, person: PlexPerson) -> bool: if not person.tmdb_person_id or not sysconfig.get_str(db, "tmdb_api_key"): return False try: - with TmdbClient(db) as tmdb: + # This runs on the request thread (first person-page open), so cap the outbound TMDB call with + # a short timeout — a slow/hanging TMDB must not tie up a worker for the default 30s. (The fuller + # fix — moving person enrichment off the request path — is logged in ROADMAP under E5.) + with TmdbClient(db, timeout=8.0) as tmdb: payload = tmdb.fetch_person(person.tmdb_person_id) except TmdbError: return False diff --git a/backend/app/routes/plex/people.py b/backend/app/routes/plex/people.py index 43df9c4f..040a4081 100644 --- a/backend/app/routes/plex/people.py +++ b/backend/app/routes/plex/people.py @@ -165,6 +165,10 @@ def person_detail( try: plex_tmdb.enrich_person(db, person) except Exception: # noqa: BLE001 — enrichment is optional; the filmography still renders + # A failed enrich_person commit leaves the session in a needs-rollback state; without this + # the following SELECTs would raise PendingRollbackError and 500 the page (SQLAlchemy 2.x + # deactivates the transaction on a commit-time flush failure and does NOT auto-reset it). + db.rollback() log.warning("person bio enrich failed for %s", name, exc_info=True) movie_lib_ids, show_lib_ids = _content_libs(db) diff --git a/backend/tests/test_plex_person_bio.py b/backend/tests/test_plex_person_bio.py index 34382e0c..1fa1edcf 100644 --- a/backend/tests/test_plex_person_bio.py +++ b/backend/tests/test_plex_person_bio.py @@ -66,7 +66,7 @@ def test_enrich_person_persists_and_stamps(db, monkeypatch): db.add(person) db.commit() monkeypatch.setattr(plex_tmdb.sysconfig, "get_str", lambda d, k: "key" if k == "tmdb_api_key" else "") - monkeypatch.setattr(plex_tmdb, "TmdbClient", lambda d: _FakeTmdb(PAYLOAD)) + monkeypatch.setattr(plex_tmdb, "TmdbClient", lambda d, timeout=None: _FakeTmdb(PAYLOAD)) assert plex_tmdb.enrich_person(db, person) is True assert person.biography == "A celebrated actor." assert person.tmdb_enriched_at is not None @@ -100,6 +100,36 @@ def _get(db, user, monkeypatch, path): app.dependency_overrides.clear() +def test_person_detail_survives_poisoned_enrich(db, monkeypatch): + # If the lazy enrich_person leaves the session in a failed-transaction state (e.g. a commit-time + # flush error), person_detail's except must roll it back so the following SELECTs still run — the + # page renders despite the enrich failure ("never fail the page"). Without the db.rollback() fix the + # poisoned session would raise PendingRollbackError on the next query and 500. + from sqlalchemy import text + + lib = PlexLibrary(plex_key="9", title="Movies", kind="movie", enabled=True) + db.add(lib) + db.flush() + # Un-enriched person (tmdb_person_id set + tmdb_enriched_at None) → triggers the lazy enrich path. + db.add(PlexPerson(name="Poison P", tmdb_person_id=777)) + user = User(email="poison@example.com") + db.add(user) + db.commit() + + def _poison_then_raise(db_, person_): + try: + db_.execute(text("SELECT 1 / 0")) # aborts the transaction (like a failed enrich commit) + except Exception: # noqa: BLE001 — swallow so the tx stays poisoned, then raise like the real path + pass + raise RuntimeError("enrich commit blew up") + + monkeypatch.setattr(plex_tmdb, "enrich_person", _poison_then_raise) + + resp = _get(db, user, monkeypatch, "/api/plex/person/Poison P") + assert resp.status_code == 200, resp.text # page still renders (session was rolled back) + assert resp.json()["biography"] is None # enrich failed, so no bio + + def test_person_detail_serves_bio_and_resolves_known_for(db, monkeypatch): lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) db.add(lib) diff --git a/backend/tests/test_plex_tmdb.py b/backend/tests/test_plex_tmdb.py index 1f654efb..05eb9062 100644 --- a/backend/tests/test_plex_tmdb.py +++ b/backend/tests/test_plex_tmdb.py @@ -5,6 +5,9 @@ recommendations to library rows; the whole thing no-ops without an API key). The DB tests use the `db` fixture (real Postgres) — skipped when none is reachable.""" from datetime import datetime, timezone +import httpx +import pytest + from app.models import PlexItem, PlexLibrary, PlexPerson, PlexShow from app.plex import tmdb @@ -198,6 +201,44 @@ def test_serve_block_none_until_enriched_and_resolves_recs(db): assert recs[999]["in_library"] is False and recs[999]["rating_key"] is None +def test_get_never_leaks_api_key_in_error_messages(monkeypatch): + # The api_key rides in the request URL query string, so an error message built from the httpx + # exception (which embeds that URL) would leak the secret to logs (enrich_batch logs it). Both the + # HTTP-status and the network error paths must report status/path only — never the key. + monkeypatch.setattr( + tmdb.sysconfig, "get_str", lambda d, k: "SECRETKEY" if k == "tmdb_api_key" else "en-US" + ) + client = tmdb.TmdbClient(None) + req = httpx.Request( + "GET", "https://api.themoviedb.org/3/movie/1?api_key=SECRETKEY&language=en-US" + ) + + class _StatusHttp: + def get(self, url, params=None): + return httpx.Response(401, request=req) # non-404 → raise_for_status → HTTPStatusError + + def close(self): + pass + + client._http = _StatusHttp() + with pytest.raises(tmdb.TmdbError) as ei: + client._get("/movie/1") + assert "SECRETKEY" not in str(ei.value) and "401" in str(ei.value) + + class _NetHttp: + def get(self, url, params=None): + raise httpx.ConnectError("connect failed", request=req) + + def close(self): + pass + + client._http = _NetHttp() + with pytest.raises(tmdb.TmdbError) as ei2: + client._get("/movie/1") + assert "SECRETKEY" not in str(ei2.value) + client.close() + + def test_run_enrich_noops_without_key(db): # No tmdb_api_key configured (default "") → clean no-op, never an error. out = tmdb.run_enrich(db)