fix(e5): deep-review survivors — TMDB key log leak, person-enrich rollback + timeout
Three verified deep-review findings (the 4th, the filter combobox a11y gap, is deferred to a dedicated session on the combobox architecture): - SEC (medium): TmdbClient sent the api_key as a URL query param, so an HTTP error's httpx exception string embedded the full URL incl. the key, which enrich_batch logged. _get now raises status+path only (never str(e)/the URL) and severs the secret-bearing cause with `from None`. Regression test asserts neither the status nor the network error path contains the key. - STATE (low): person_detail's lazy-enrich except now db.rollback()s, so a failed enrich_person commit can't poison the session and 500 the subsequent SELECTs (SQLAlchemy 2.x deactivates the tx on a commit-time flush failure and does not auto-reset it). Test locks that the page renders despite a poisoned enrich. - ABUSE (low): the request-path lazy person enrich now caps its TMDB client at an 8s timeout (was the 30s default) so a slow TMDB can't tie up a worker; TmdbClient gained a `timeout` param (batch keeps 30s). The fuller fix (move person enrichment off the request path) is logged for later.
This commit is contained in:
@@ -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=<secret>` 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
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user