diff --git a/app.py b/app.py index 91da4cf..8784d2a 100644 --- a/app.py +++ b/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 . + + 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"]: - album += f" ({job['year']})" + year = safe_year(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) diff --git a/tests/test_authz.py b/tests/test_authz.py new file mode 100644 index 0000000..a3ae763 --- /dev/null +++ b/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) diff --git a/tests/test_security.py b/tests/test_security.py new file mode 100644 index 0000000..7b3d4b6 --- /dev/null +++ b/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)