Fix a path traversal out of the music library

`year` was appended to the album directory name AFTER safe() had run, so it
was the one path component reaching the filesystem unchecked:

  POST /api/request {"artist":"E","title":"p","year":"../../../../tmp/pwned"}
  -> library/E/Singles (../../../../tmp/pwned)/p.flac
  -> normalises to ../tmp/pwned)/p.flac

Any authenticated user could do it, guests included, and it wrote a real
audio file to an arbitrary path the process could reach. Existing files were
not overwritten -- tag_and_file returns early when the destination exists --
so this created new files and directories rather than clobbering.

Fixed in two layers, because the first is a property of five call sites that
must each stay correct and the second cannot be forgotten:

  1. safe_year() whitelists \d{4}, applied in enqueue_job (the choke point for
     all three request paths) and again in tag_and_file.
  2. tag_and_file resolves the destination and refuses anything that is not
     under the library root, before mkdir rather than after.

Also from the same pass:

  - Cap import at 500 pasted lines / 1000 expanded tracks. URL expansion runs
    BEFORE the quota check -- it has to, since a playlist URL is one line and
    200 tracks -- so without a cap a paste of 10,000 album URLs was 10,000
    requests to Deezer from this IP before any quota was consulted.
  - Clamp search limit (1-50), jobs limit (1-1000) and activity days (1-365).
    search limit fanned out one /album/{id} enrichment per distinct album.
  - safe_cover() requires https and <=300 chars. img-src permits data:, so
    the field was free storage for megabytes of base64 per job.
  - Truncate filename components to 150 chars; a long title used to fail the
    import with ENAMETOOLONG after the download had already completed.
  - Treat Deezer's 403 as no-results. Its WAF rejects injection-looking
    queries, which surfaced to the user as a 502 "service broken".

Add tests/: 42 security assertions and 35 authorization assertions covering
the role matrix, last-admin protection, mid-session revocation, job scoping
and OAuth CSRF. All 77 pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

app.py

@@ -40,7 +40,7 @@ from pathlib import Path
 from urllib.parse import urlencode
 
 import httpx
-from fastapi import FastAPI, HTTPException, Request
+from fastapi import FastAPI, HTTPException, Query, Request
 from fastapi.responses import (FileResponse, HTMLResponse, JSONResponse,
                                RedirectResponse)
 from fastapi.staticfiles import StaticFiles
@@ -323,11 +323,36 @@ def require_admin(request: Request) -> str:
 
 SAFE = re.compile(r'[<>:"/\\|?*\x00-\x1f]')
 NOISE = re.compile(r"[^a-z0-9]+")
+YEAR = re.compile(r"^\d{4}$")
+
+# Most filesystems cap a path component at 255 bytes. Track titles can be far
+# longer than that once a stranger has appended their own notes, and the
+# resulting ENAMETOOLONG would fail the import after the download had finished.
+MAX_COMPONENT = 150
 
 
 def safe(name: str) -> str:
     """Filesystem-safe path component. Trailing dots/spaces break on some FS."""
-    return SAFE.sub("_", name).strip(". ") or "Unknown"
+    return SAFE.sub("_", name).strip(". ")[:MAX_COMPONENT].strip() or "Unknown"
+
+
+def safe_cover(url) -> str | None:
+    """Artwork URL, stored and later rendered as an <img src>.
+
+    The CSP already stops a foreign host loading, so this is not an XSS gate —
+    it stops the field being used as free storage. `data:` is permitted by
+    img-src, so without a check any client could park megabytes of base64 in
+    the jobs table on every request.
+    """
+    u = str(url or "").strip()
+    return u if u.startswith("https://") and len(u) <= 300 else None
+
+
+def safe_year(year) -> str | None:
+    """A year, or nothing. This lands in a directory name, so it is checked
+    against a whitelist rather than merely scrubbed."""
+    y = str(year or "").strip()
+    return y if YEAR.match(y) else None
 
 
 def norm(s: str) -> str:
@@ -454,7 +479,7 @@ def slskd_client(timeout: float = 30.0) -> httpx.AsyncClient:
 PROBE_TIMEOUT = 4.0
 
 
-def why(e: Exception) -> str:
+def errstr(e: Exception) -> str:
     """httpx's timeout and connection errors often str() to an empty string,
     which would render as 'slskd unreachable — ' with nothing after it."""
     return (str(e) or type(e).__name__)[:120]
@@ -598,14 +623,25 @@ def tag_and_file(job: sqlite3.Row, src: Path) -> Path:
         log.warning("tagging failed for %s: %s", src.name, e)
 
     album = f"{safe(job['album'] or 'Singles')}"
-    if job["year"]:
+    year = safe_year(job["year"])
-        album += f" ({job['year']})"
+    if year:
+        album += f" ({year})"
     dest_dir = LIBRARY_DIR / safe(job["artist"]) / album
-    dest_dir.mkdir(parents=True, exist_ok=True)
 
     num = f"{int(job['track']):02d} - " if job["track"] else ""
     dest = dest_dir / f"{num}{safe(job['title'])}{src.suffix.lower()}"
 
+    # Belt and braces. Every component above is either safe()'d or matched
+    # against a whitelist, but that is a property of five call sites that must
+    # each stay correct; this is one check that cannot be forgotten. It exists
+    # because `year` was for a while appended AFTER safe() ran, so a request
+    # with year="../../../../tmp/x" wrote outside the library.
+    root = LIBRARY_DIR.resolve()
+    if not Path(os.path.normpath(dest)).resolve().is_relative_to(root):
+        raise ValueError(f"refusing to write outside the library: {dest}")
+
+    dest_dir.mkdir(parents=True, exist_ok=True)
+
     if dest.exists():
         return dest
     try:
@@ -1136,6 +1172,13 @@ async def deezer_search(q: str, limit: int = 25) -> list[dict]:
         return []
     async with httpx.AsyncClient(timeout=20) as c:
         r = await c.get(f"{DEEZER}/search", params={"q": q, "limit": limit * 2})
+        # Deezer's WAF answers 403 to anything that looks like an injection
+        # attempt -- "'; DROP TABLE" and friends. That is a statement about the
+        # query, not an upstream outage, so it belongs on the empty-results
+        # path; a 502 would tell the user the service is broken when it is not.
+        if r.status_code == 403:
+            log.info("deezer refused the query %r", q[:60])
+            return []
         if r.status_code != 200:
             raise HTTPException(502, f"deezer returned {r.status_code}")
         data = r.json()
@@ -1185,7 +1228,7 @@ async def deezer_search(q: str, limit: int = 25) -> list[dict]:
 
 
 @app.get("/api/search")
-async def search(q: str, limit: int = 25):
+async def search(q: str, limit: int = Query(25, ge=1, le=50)):
     """Track search. Returns individual songs, not albums — the whole point.
 
     Year and genre are grafted on afterwards because Deezer's track search
@@ -1315,6 +1358,13 @@ async def enqueue_job(artist: str, title: str, album=None, year=None,
                       cover=None, genre=None, duration=None) -> str:
     """Create a job and hand it to the search queue."""
     job_id = str(uuid.uuid4())
+    # The single choke point for all three request paths, so normalise here.
+    # year reaches a directory name, and artist/title/album reach filenames.
+    year = safe_year(year)
+    cover = safe_cover(cover)
+    artist, title = artist[:MAX_COMPONENT], title[:MAX_COMPONENT]
+    album = album[:MAX_COMPONENT] if album else album
+    genre = genre[:80] if genre else genre
     with db() as c:
         c.execute(
             "INSERT INTO jobs (id,state,detail,artist,title,album,year,track,"
@@ -1397,6 +1447,11 @@ class ImportBody(BaseModel):
 SPLIT = re.compile(r"\s+[-–—|]\s+|\s+by\s+", re.I)
 DEEZER_URL = re.compile(r"deezer\.com/(?:[a-z]{2}/)?(album|playlist)/(\d+)")
 
+# Generous for a human pasting an album or a year-end list, small enough that
+# one request cannot turn into thousands of upstream calls.
+MAX_IMPORT_LINES = 500
+MAX_IMPORT_TRACKS = 1000
+
 
 async def expand_url(kind: str, ident: str) -> list[str]:
     """Turn a Deezer album/playlist URL into 'Artist - Title' lines."""
@@ -1424,12 +1479,23 @@ async def import_list(body: ImportBody, request: Request):
     than silently landing the wrong song in the library.
     """
     lines, queued, skipped = [], [], []
-    for raw in body.text.splitlines():
+    # Capped before expansion, not after. Each URL line costs an upstream call,
+    # and the quota check below cannot run until expansion has finished — so
+    # without a cap here a paste of 10,000 album URLs is 10,000 requests to
+    # Deezer from this IP before anyone's quota is consulted.
+    raw_lines = body.text.splitlines()
+    if len(raw_lines) > MAX_IMPORT_LINES:
+        raise HTTPException(413, f"paste at most {MAX_IMPORT_LINES} lines at a time "
+                                 f"(got {len(raw_lines)})")
+    for raw in raw_lines:
         line = raw.strip().lstrip("-•*0123456789. )").strip()
         if not line:
             continue
         m = DEEZER_URL.search(line)
         lines.extend(await expand_url(*m.groups()) if m else [line])
+        if len(lines) > MAX_IMPORT_TRACKS:
+            raise HTTPException(413, f"that expands to more than "
+                                     f"{MAX_IMPORT_TRACKS} tracks")
 
     # After expansion — a single playlist URL can be 200 tracks, and the point
     # of the check is the real number, not the number of lines pasted.
@@ -1445,7 +1511,7 @@ async def import_list(body: ImportBody, request: Request):
             # version -- 6x the popularity -- sat further down the list unseen.
             hits = await deezer_search(SPLIT.sub(" ", line), limit=10)
         except Exception as e:
-            skipped.append({"line": line, "why": str(e)[:120]})
+            skipped.append({"line": line, "why": errstr(e)})
             continue
         if not hits:
             skipped.append({"line": line, "why": "no match on Deezer"})
@@ -1465,7 +1531,8 @@ async def import_list(body: ImportBody, request: Request):
 
 
 @app.get("/api/jobs")
-async def jobs(request: Request, limit: int = 200, mine: bool = False):
+async def jobs(request: Request, limit: int = Query(200, ge=1, le=1000),
+               mine: bool = False):
     """Own requests by default; admins see everyone's.
 
     Scoped rather than global because on a shared instance one person's
@@ -1646,7 +1713,7 @@ async def admin_delete_user(username: str, request: Request):
 
 
 @app.get("/api/admin/activity")
-async def admin_activity(request: Request, days: int = 30):
+async def admin_activity(request: Request, days: int = Query(30, ge=1, le=365)):
     require_admin(request)
     days = max(1, min(days, 365))
     since = (datetime.now(timezone.utc) - timedelta(days=days)).strftime("%Y-%m-%d")
@@ -1785,7 +1852,7 @@ async def admin_system(request: Request):
             else:
                 slskd["error"] = f"HTTP {r.status_code}"
     except Exception as e:
-        slskd["error"] = why(e)
+        slskd["error"] = errstr(e)
     out["slskd"] = slskd
     return out
 
@@ -1799,7 +1866,7 @@ async def health():
             detail["slskd"] = r.status_code
             ok = ok and r.status_code == 200
         except Exception as e:
-            detail["slskd"] = why(e)
+            detail["slskd"] = errstr(e)
             ok = False
     detail["library_writable"] = os.access(LIBRARY_DIR, os.W_OK)
     detail["downloads_readable"] = os.access(DOWNLOAD_DIR, os.R_OK)

tests/test_authz.py

@@ -0,0 +1,152 @@
+"""Authorization matrix with MUSE_AUTH=github actually enabled.
+
+Sessions are inserted straight into the DB rather than driven through OAuth,
+so this exercises the guard, not GitHub. Run standalone: it starts its own
+server on :5042 with its own database.
+"""
+import hashlib, os, secrets, signal, sqlite3, subprocess, sys, time
+from datetime import datetime, timedelta, timezone
+import httpx
+
+HERE = os.path.dirname(os.path.abspath(__file__))
+ROOT = os.path.dirname(HERE)
+DB = os.path.join(ROOT, "dev", "authz.db")
+BASE = "http://127.0.0.1:5042"
+ok = fail = 0
+
+
+def check(name, cond, detail=""):
+    global ok, fail
+    if cond:
+        ok += 1; print(f"  PASS  {name}")
+    else:
+        fail += 1; print(f"  FAIL  {name}  {detail}")
+
+
+for f in (DB, DB + "-wal", DB + "-shm"):
+    if os.path.exists(f):
+        os.remove(f)
+
+env = {**os.environ,
+       "MUSE_AUTH": "github,google",
+       "MUSE_DB": DB,
+       "MUSE_BASE_URL": "https://muse.example.com",
+       "MUSE_GITHUB_CLIENT_ID": "x", "MUSE_GITHUB_CLIENT_SECRET": "y",
+       "MUSE_GOOGLE_CLIENT_ID": "x", "MUSE_GOOGLE_CLIENT_SECRET": "y",
+       "MUSE_ALLOWED_USERS": "bootstrap",
+       "MUSE_LIBRARY_DIR": os.path.join(ROOT, "dev", "library"),
+       "MUSE_DOWNLOAD_DIR": os.path.join(ROOT, "dev", "downloads"),
+       "MUSE_COOKIE_SECURE": "false", "SLSKD_URL": "http://127.0.0.1:1"}
+srv = subprocess.Popen([os.path.join(ROOT, ".venv/bin/uvicorn"), "app:app",
+                        "--host", "127.0.0.1", "--port", "5042"],
+                       cwd=ROOT, env=env,
+                       stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+try:
+    for _ in range(60):
+        try:
+            if httpx.get(BASE + "/healthz", timeout=1).status_code == 200:
+                break
+        except Exception:
+            time.sleep(0.3)
+    else:
+        print("server never came up"); sys.exit(1)
+
+    def seed(username, role, disabled=0):
+        tok = secrets.token_urlsafe(32)
+        h = hashlib.sha256(tok.encode()).hexdigest()
+        exp = (datetime.now(timezone.utc) + timedelta(days=1)).isoformat()
+        d = sqlite3.connect(DB)
+        d.execute("INSERT OR REPLACE INTO users (username,provider,role,disabled) "
+                  "VALUES (?,'github',?,?)", (username, role, disabled))
+        d.execute("INSERT INTO sessions (token_hash,username,expires_at) VALUES (?,?,?)",
+                  (h, username, exp))
+        d.commit(); d.close()
+        return httpx.Client(base_url=BASE, timeout=20,
+                            cookies={"muse_session": tok})
+
+    admin = seed("adm", "admin")
+    user = seed("usr", "user")
+    guest = seed("gst", "guest")
+    anon = httpx.Client(base_url=BASE, timeout=20)
+
+    print("\n=== admin routes by role ===")
+    for path in ("/api/admin/users", "/api/admin/system", "/api/admin/activity"):
+        check(f"admin  {path}", admin.get(path).status_code == 200)
+        check(f"user   {path} -> 403", user.get(path).status_code == 403)
+        check(f"guest  {path} -> 403", guest.get(path).status_code == 403)
+        check(f"anon   {path} -> 401", anon.get(path).status_code == 401)
+
+    print("\n=== mutating admin routes ===")
+    check("user cannot invite",
+          user.post("/api/admin/users",
+                    json={"username": "x", "provider": "github"}).status_code == 403)
+    check("user cannot patch",
+          user.patch("/api/admin/users/gst", json={"role": "admin"}).status_code == 403)
+    check("user cannot delete",
+          user.request("DELETE", "/api/admin/users/gst").status_code == 403)
+
+    print("\n=== last-admin protection ===")
+    check("demote last admin -> 409",
+          admin.patch("/api/admin/users/adm", json={"role": "user"}).status_code == 409)
+    check("disable last admin -> 409",
+          admin.patch("/api/admin/users/adm", json={"disabled": True}).status_code == 409)
+    check("delete last admin -> 409",
+          admin.request("DELETE", "/api/admin/users/adm").status_code == 409)
+
+    print("\n=== revocation takes effect mid-session ===")
+    check("guest works before", guest.get("/api/me").json()["user"] == "gst")
+    admin.patch("/api/admin/users/gst", json={"disabled": True})
+    check("guest 401 after disable", guest.get("/api/jobs").status_code == 401)
+
+    print("\n=== job scoping ===")
+    for cl, who in ((user, "usr"),):
+        cl.post("/api/request", json={"artist": "A", "title": "T"})
+    admin.post("/api/request", json={"artist": "B", "title": "U"})
+    time.sleep(0.5)
+    ujobs = user.get("/api/jobs").json()["jobs"]
+    ajobs = admin.get("/api/jobs").json()["jobs"]
+    check("user sees only own", all(j["requested_by"] == "usr" for j in ujobs), str(ujobs))
+    check("admin sees all", len(ajobs) > len(ujobs))
+    other = [j for j in ajobs if j["requested_by"] != "usr"]
+    if other:
+        check("cross-user retry -> 404",
+              user.post(f"/api/retry/{other[0]['id']}").status_code == 404)
+
+    print("\n=== unauthenticated surface ===")
+    check("anon /api/search -> 401", anon.get("/api/search?q=x").status_code == 401)
+    check("anon / redirects to login",
+          anon.get("/", follow_redirects=False).status_code == 302)
+    check("anon /_next bundle is NOT public",
+          anon.get("/_next/static/x.js", follow_redirects=False).status_code == 302)
+    check("anon /healthz is public", anon.get("/healthz").status_code == 200)
+
+    print("\n=== OAuth CSRF ===")
+    check("callback with no state -> 400",
+          anon.get("/auth/callback?code=x", follow_redirects=False).status_code == 400)
+    check("callback with unmatched state -> 400",
+          anon.get("/auth/callback?code=x&state=zzz",
+                   follow_redirects=False).status_code == 400)
+    c2 = httpx.Client(base_url=BASE, timeout=20,
+                      cookies={"muse_oauth_state": "github|realnonce"})
+    check("callback with wrong nonce -> 400",
+          c2.get("/auth/callback?code=x&state=wrong",
+                 follow_redirects=False).status_code == 400)
+    c3 = httpx.Client(base_url=BASE, timeout=20,
+                      cookies={"muse_oauth_state": "evil|n"})
+    check("callback with unknown provider -> 400",
+          c3.get("/auth/callback?code=x&state=n",
+                 follow_redirects=False).status_code == 400)
+    r = anon.get("/auth/login?provider=github", follow_redirects=False)
+    check("login sets state cookie", "muse_oauth_state" in r.headers.get("set-cookie", ""))
+    check("state cookie is httponly", "httponly" in r.headers.get("set-cookie", "").lower())
+    check("login redirects to github",
+          r.headers.get("location", "").startswith("https://github.com/login/oauth"))
+    check("unknown provider -> 404",
+          anon.get("/auth/login?provider=evil",
+                   follow_redirects=False).status_code == 404)
+
+    print(f"\n{ok} passed, {fail} failed")
+finally:
+    srv.send_signal(signal.SIGINT)
+    srv.wait(timeout=10)
+sys.exit(1 if fail else 0)

tests/test_security.py

@@ -0,0 +1,114 @@
+"""Security regression suite. Run against a dev.sh instance on :5041.
+
+Covers the classes that matter for an internet-facing request front door:
+path traversal into the library, authz on every admin route, session
+revocation, OAuth CSRF, injection, and resource amplification.
+"""
+import subprocess, sys, time, os, signal, sqlite3, json
+import httpx
+
+BASE = "http://127.0.0.1:5041"
+ok = fail = 0
+
+
+def check(name, cond, detail=""):
+    global ok, fail
+    if cond:
+        ok += 1
+        print(f"  PASS  {name}")
+    else:
+        fail += 1
+        print(f"  FAIL  {name}  {detail}")
+
+
+def section(t):
+    print(f"\n=== {t} ===")
+
+
+# ── path traversal ────────────────────────────────────────────────────────
+section("path traversal into the library")
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+os.environ.update(MUSE_AUTH="off", MUSE_DB="dev/muse.db",
+                  MUSE_LIBRARY_DIR="dev/library", MUSE_DOWNLOAD_DIR="dev/downloads")
+import app  # noqa: E402
+from pathlib import Path  # noqa: E402
+
+for probe in ["../../etc", "..", ".", "/etc/passwd", "....//....//x",
+              "a\x00b", "con", " . . ", "x" * 400]:
+    s = app.safe(probe)
+    check(f"safe({probe[:18]!r})", "/" not in s and "\\" not in s
+          and s not in ("", ".", "..") and len(s) <= app.MAX_COMPONENT, f"-> {s!r}")
+
+for bad in ["../../tmp/x", "1993/../../x", "19933", "", "abc", None]:
+    y = app.safe_year(bad)
+    check(f"safe_year({bad!r})", y is None, f"-> {y!r}")
+check("safe_year('1993')", app.safe_year("1993") == "1993")
+check("safe_year(' 1993 ') trims", app.safe_year("  1993  ") == "1993")
+
+for bad in ["data:image/svg+xml;base64," + "A" * 5000, "javascript:alert(1)",
+            "http://evil/x.png", "x" * 400]:
+    check(f"safe_cover({bad[:22]!r})", app.safe_cover(bad) is None)
+check("safe_cover(deezer)", app.safe_cover("https://e-cdns-images.dzcdn.net/x.jpg"))
+
+# containment holds even if the field validators are bypassed
+src = Path("dev/downloads/fake.flac")
+src.parent.mkdir(parents=True, exist_ok=True)
+src.write_bytes(b"x")
+real_safe_year = app.safe_year
+app.safe_year = lambda y: (str(y) if y else None)
+row = {"album": None, "year": "../../../../../../tmp/muse-pwned", "artist": "E",
+       "title": "p", "track": None}
+try:
+    app.tag_and_file(row, src)
+    check("containment blocks escape", False, "wrote outside the library")
+except ValueError:
+    check("containment blocks escape", True)
+except Exception as e:
+    check("containment blocks escape", False, f"wrong error {type(e).__name__}: {e}")
+app.safe_year = real_safe_year
+check("nothing landed in /tmp", not Path("/tmp/muse-pwned").exists())
+
+# ── live HTTP ─────────────────────────────────────────────────────────────
+c = httpx.Client(base_url=BASE, timeout=30)
+
+section("resource amplification")
+r = c.post("/api/import", json={"text": "\n".join(["a"] * 5000)})
+check("import line cap", r.status_code == 413, f"got {r.status_code}")
+r = c.get("/api/search", params={"q": "x", "limit": 100000})
+check("search limit clamped", r.status_code == 422, f"got {r.status_code}")
+r = c.get("/api/jobs", params={"limit": -1})
+check("jobs limit clamped", r.status_code == 422, f"got {r.status_code}")
+r = c.get("/api/admin/activity", params={"days": 99999})
+check("activity days clamped", r.status_code == 422, f"got {r.status_code}")
+
+section("injection")
+for payload in ["'; DROP TABLE users;--", "' OR '1'='1", "\x00", "%00",
+                "Don't Stop Me Now", "Guns N' Roses"]:
+    r = c.get("/api/search", params={"q": payload})
+    check(f"search survives {payload[:16]!r}", r.status_code == 200,
+          f"got {r.status_code}")
+r = c.post("/api/admin/users", json={"username": "'; DROP TABLE users;--",
+                                     "provider": "github", "role": "user"})
+with app.db() as d:
+    still = d.execute("SELECT COUNT(*) n FROM users").fetchone()["n"]
+check("users table intact after SQLi attempt", still > 0)
+c.request("DELETE", "/api/admin/users/'; DROP TABLE users;--")
+
+section("security headers")
+r = c.get("/")
+h = r.headers
+check("CSP present", "content-security-policy" in h)
+check("CSP has no unsafe-inline for script",
+      "'unsafe-inline'" not in h.get("content-security-policy", "").split("style-src")[0])
+check("nosniff", h.get("x-content-type-options") == "nosniff")
+check("frame deny", h.get("x-frame-options") == "DENY")
+check("referrer", h.get("referrer-policy") == "no-referrer")
+check("img-src is not blanket https:",
+      " https: " not in h.get("content-security-policy", ""))
+
+section("health endpoint leaks nothing")
+r = c.get("/healthz")
+check("healthz is bare ok", r.json() == {"ok": True}, r.text[:80])
+
+print(f"\n{ok} passed, {fail} failed")
+sys.exit(1 if fail else 0)