diff --git a/CHANGELOG.md b/CHANGELOG.md index 36a7370..81bef4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- **Restricted `youtube-audio` to YouTube hostnames.** The route handed a + caller-supplied URL straight to `yt_dlp`, which falls back to its generic + extractor for anything it doesn't recognize — that extractor can fetch + essentially any web page server-side, an SSRF shape with no scheme/host + restriction. The UI only ever offers "paste a YouTube URL"; the route now + rejects any URL whose scheme isn't http/https or whose host isn't + youtube.com/youtu.be (or a documented subdomain) with 400, before yt_dlp + ever sees it. - **Fixed stored XSS via unrestricted file-upload extensions.** `upload-art`, `upload-preview`, and `upload-audio` wrote uploaded bytes to disk using the client-supplied filename's extension with no allow-list, and that storage diff --git a/plugin.json b/plugin.json index 671b77e..9bae9dc 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "id": "editor", "name": "Arrangement Editor", - "version": "1.8.2", + "version": "1.8.3", "description": "Build and edit feedpak arrangements — import Guitar Pro tabs, sync audio, and tune every note.", "category": "creation", "icon": "assets/thumb.png", diff --git a/routes.py b/routes.py index 37ef3fd..17f64a9 100644 --- a/routes.py +++ b/routes.py @@ -12,6 +12,7 @@ import time import zipfile from pathlib import Path +from urllib.parse import urlparse from xml.etree import ElementTree as ET from xml.dom import minidom @@ -1043,6 +1044,28 @@ def _safe_wafont_name(name): return name if _WAFONT_NAME_RE.match(name) else None +# Hosts the /youtube-audio route is allowed to hand to yt_dlp. Whitelist +# shape, not a blacklist — see the comment on the route itself (setup()) for +# why: yt_dlp's generic extractor can fetch essentially any web page, which +# makes an unrestricted URL an SSRF vector. +_YOUTUBE_HOSTS = frozenset({ + "youtube.com", "www.youtube.com", "m.youtube.com", + "music.youtube.com", "youtu.be", "www.youtu.be", +}) + + +def _is_youtube_url(url): + """True only for an http(s) youtube.com/youtu.be URL. Pure string + validation — no network access, no DNS resolution.""" + if not isinstance(url, str): + return False + try: + parsed = urlparse(url) + except ValueError: + return False + return parsed.scheme in ("http", "https") and (parsed.hostname or "").lower() in _YOUTUBE_HOSTS + + def _gp_sync_points_to_warp_payload(sync_points): """GP8 `SyncPoint` objects → the dict shape the warp builder consumes. @@ -6714,12 +6737,25 @@ async def upload_audio(file: UploadFile = File(...)): return {"audio_url": f"{STORAGE_URL}/editor_audio_{audio_id}{ext}", "duration": _dur} # ── Download audio from YouTube ────────────────────────────────── + # + # This hands a caller-supplied URL to yt_dlp, which by default falls + # back to its *generic* extractor for anything it doesn't recognize — + # that extractor can fetch essentially any web page server-side, which + # is an SSRF shape (server-side fetch of a caller-chosen target with no + # scheme/host restriction). The UI only ever offers "paste a YouTube + # URL" (screen.html), so restricting to YouTube's own hostnames (see + # _is_youtube_url) closes that off at the one point we control, without + # needing to intercept yt_dlp's internal request handling (which also + # legitimately talks to Google's video CDN hosts, not just the URL the + # caller supplied). @app.post("/api/plugins/editor/youtube-audio") async def youtube_audio(data: dict): url = data.get("url", "").strip() if not url: return JSONResponse({"error": "No URL provided"}, 400) + if not _is_youtube_url(url): + return JSONResponse({"error": "URL must be a youtube.com or youtu.be link"}, 400) def _download(): tmp = tempfile.mkdtemp(prefix="slopsmith_yt_") diff --git a/tests/test_youtube_audio_url.py b/tests/test_youtube_audio_url.py new file mode 100644 index 0000000..7d67914 --- /dev/null +++ b/tests/test_youtube_audio_url.py @@ -0,0 +1,74 @@ +"""Tests for _is_youtube_url — the allow-list guard in front of the +/api/plugins/editor/youtube-audio route (issue #14). + +The route hands a caller-supplied URL to yt_dlp, whose generic extractor can +fetch essentially any web page server-side — an SSRF shape with no +scheme/host restriction. The UI only ever offers "paste a YouTube URL", so +_is_youtube_url restricts requests to youtube.com/youtu.be hostnames before +the URL ever reaches yt_dlp. +""" + +from routes import _is_youtube_url + + +def test_real_youtube_urls_pass(): + for url in ( + "https://www.youtube.com/watch?v=dQw4w9WgXcQ", + "https://youtube.com/watch?v=dQw4w9WgXcQ", + "https://m.youtube.com/watch?v=dQw4w9WgXcQ", + "https://music.youtube.com/watch?v=dQw4w9WgXcQ", + "https://youtu.be/dQw4w9WgXcQ", + "http://youtube.com/watch?v=dQw4w9WgXcQ", # http allowed, not just https + ): + assert _is_youtube_url(url) is True + + +def test_non_youtube_hosts_rejected(): + for url in ( + "https://example.com/video", + "https://vimeo.com/12345", + "https://169.254.169.254/latest/meta-data/", # cloud metadata endpoint + "http://127.0.0.1:8080/internal", + "http://localhost/admin", + "http://192.168.1.1/", + "https://internal-service.local/", + ): + assert _is_youtube_url(url) is False + + +def test_lookalike_hosts_rejected(): + # A generic-extractor SSRF guard must not be foolable by a hostname that + # merely *contains* "youtube.com" — only the real domain (and its + # documented subdomains) is allowed. + for url in ( + "https://youtube.com.evil.example/watch?v=1", + "https://evil-youtube.com/watch?v=1", + "https://notyoutube.com/watch?v=1", + "https://youtube.com.evil.com/", + "https://xn--youtube-com.evil.example/", + ): + assert _is_youtube_url(url) is False + + +def test_userinfo_host_confusion_rejected(): + # https://youtube.com@evil.example/ — browsers/naive parsers can be + # tricked into reading "youtube.com" as the host when it's actually + # userinfo; urlparse().hostname correctly resolves this to "evil.example", + # which must still be rejected. + assert _is_youtube_url("https://youtube.com@evil.example/watch?v=1") is False + + +def test_non_http_schemes_rejected(): + for url in ( + "file:///etc/passwd", + "ftp://youtube.com/watch?v=1", + "javascript:alert(1)", + "data:text/html,", + "gopher://youtube.com/", + ): + assert _is_youtube_url(url) is False + + +def test_malformed_and_non_string_input_rejected(): + for url in ("", "not a url", " ", None, 42, ["https://youtube.com/"]): + assert _is_youtube_url(url) is False