diff --git a/lib/routers/playlists.py b/lib/routers/playlists.py index 4d8adafa..f517fc25 100644 --- a/lib/routers/playlists.py +++ b/lib/routers/playlists.py @@ -6,6 +6,9 @@ ``reqfields``. See ``appstate.py``. """ +import logging +import os +import tempfile from pathlib import Path from fastapi import APIRouter @@ -14,6 +17,8 @@ import appstate from reqfields import _clean_str +log = logging.getLogger("feedBack.server") + router = APIRouter() # Cache policy for the custom-cover file response: revalidate every time so a @@ -204,15 +209,40 @@ async def api_set_playlist_cover(pid: int, data: dict): return JSONResponse({"error": "Invalid base64"}, status_code=400) cover = _playlist_cover_path(pid) cover.parent.mkdir(parents=True, exist_ok=True) + # Decode/validate the image — a bad payload is a CLIENT error (400), and the + # message stays generic so it can't echo internals. try: from PIL import Image img = Image.open(io.BytesIO(img_data)).convert("RGB") img.thumbnail((640, 640)) # covers stay small - tmp = cover.with_suffix(".png.tmp") - img.save(str(tmp), "PNG") + except Exception: + return JSONResponse({"error": "Invalid image"}, status_code=400) + # Persist. A save/replace failure is a SERVER error (500, logged, no + # filesystem detail leaked) — the pre-split handler mislabeled these as 400 + # and echoed the exception. A unique temp name in the cover dir (not a shared + # `{pid}.png.tmp`) means two concurrent uploads can't clobber each other's + # temp file; the atomic replace publishes. Re-check the playlist still exists + # just before publishing so a delete that raced the decode above can't leave + # an orphan cover — cheap belt-and-braces; FeedBack is single-user + # (Principle I), so a full per-playlist lock would be for a race the + # deployment model precludes. + tmp = None + try: + # mkstemp is inside the try too: an unwritable dir / full disk raises + # here, and that's the same class of persistence failure as save/replace. + fd, tmp_name = tempfile.mkstemp(prefix=f".{pid}.", suffix=".png.tmp", dir=str(cover.parent)) + tmp = Path(tmp_name) + with os.fdopen(fd, "wb") as f: + img.save(f, "PNG") + if appstate.meta_db.get_playlist(pid) is None: + tmp.unlink(missing_ok=True) + return JSONResponse({"error": "not found"}, status_code=404) tmp.replace(cover) - except Exception as e: - return JSONResponse({"error": f"Invalid image: {e}"}, status_code=400) + except Exception: + if tmp is not None: + tmp.unlink(missing_ok=True) + log.exception("playlist cover save failed (pid=%s)", pid) + return JSONResponse({"error": "could not save cover"}, status_code=500) return {"ok": True, "cover_url": _playlist_cover_url(pid)} diff --git a/tests/test_playlist_cover_errors.py b/tests/test_playlist_cover_errors.py new file mode 100644 index 00000000..641acf05 --- /dev/null +++ b/tests/test_playlist_cover_errors.py @@ -0,0 +1,107 @@ +"""Playlist cover upload: client vs server errors, no temp litter, no leak. + +The pre-split handler caught decode AND persistence failures in one `except`, +returned 400 for both, and echoed the exception (`Invalid image: {e}`) — so a +disk/permission failure was mislabeled as a client error and could leak a +filesystem path. These pin the split. +""" + +import base64 +import io + +import pytest +from fastapi.testclient import TestClient +from PIL import Image + +import appstate +from metadata_db import MetadataDB +from routers import playlists + + +@pytest.fixture() +def client(tmp_path): + prev = (appstate.meta_db, appstate.config_dir) + db = MetadataDB(tmp_path) + appstate.configure(meta_db=db, config_dir=tmp_path) + app_ = __import__("fastapi").FastAPI() + app_.include_router(playlists.router) + try: + yield TestClient(app_), tmp_path + finally: + db.conn.close() + appstate.configure(meta_db=prev[0], config_dir=prev[1]) + + +def _png_b64(): + buf = io.BytesIO() + Image.new("RGB", (8, 8), (10, 20, 30)).save(buf, "PNG") + return base64.b64encode(buf.getvalue()).decode() + + +def _make_playlist(client): + return client.post("/api/playlists", json={"name": "P"}).json()["id"] + + +def test_valid_cover_saves_and_leaves_no_temp(client): + c, tmp_path = client + pid = _make_playlist(c) + r = c.post(f"/api/playlists/{pid}/cover", json={"image": _png_b64()}) + assert r.status_code == 200 + cover_dir = tmp_path / "playlist_covers" + assert (cover_dir / f"{pid}.png").exists() + # The atomic-publish temp file must not linger. + assert not list(cover_dir.glob("*.tmp")) + + +def test_undecodable_image_is_a_400_without_leaking(client): + c, _ = client + pid = _make_playlist(c) + # valid base64, not a valid image + r = c.post(f"/api/playlists/{pid}/cover", json={"image": base64.b64encode(b"not an image").decode()}) + assert r.status_code == 400 + body = r.json()["error"] + assert body == "Invalid image" # generic — no exception detail echoed + assert "playlist_covers" not in body # no filesystem path leak + + +def test_save_failure_is_a_500_not_a_400(client, monkeypatch): + """A persistence failure (here: Image.save raising) must be a logged 500, + not a 400 — the whole point of the decode/persist split. Negative-checks + against the pre-fix behavior, which returned 400 for exactly this.""" + c, tmp_path = client + pid = _make_playlist(c) + payload = _png_b64() # build BEFORE patching save + + def boom(self, fp, *a, **k): + raise OSError("disk full") + + monkeypatch.setattr(Image.Image, "save", boom) + r = c.post(f"/api/playlists/{pid}/cover", json={"image": payload}) + + assert r.status_code == 500 + assert "disk full" not in r.json()["error"] # no internal detail + assert not list((tmp_path / "playlist_covers").glob("*.tmp")) # temp cleaned up + + +def test_temp_creation_failure_is_a_500(client, monkeypatch): + """mkstemp raising (unwritable dir / full disk) must hit the same logged 500 + path as a save failure, not escape as an unhandled server error.""" + import tempfile as _tempfile + + c, _ = client + pid = _make_playlist(c) + payload = _png_b64() + + def boom(*a, **k): + raise OSError("read-only file system") + + monkeypatch.setattr(_tempfile, "mkstemp", boom) + r = c.post(f"/api/playlists/{pid}/cover", json={"image": payload}) + assert r.status_code == 500 + assert "read-only" not in r.json()["error"] + + +def test_upload_to_missing_playlist_is_404(client): + c, _ = client + r = c.post("/api/playlists/9999/cover", json={"image": _png_b64()}) + assert r.status_code == 404