mirror of
https://github.com/alexta69/metube.git
synced 2026-09-21 21:45:04 +00:00
fix: stop resolving submitted URLs locally when a proxy will (closes #1079)
validate_url resolved every submitted hostname in the server process before yt-dlp saw it. Behind a proxy that does its own DNS -- an HTTP proxy, socks5h, socks4a, or the plain socks5 yt-dlp rewrites to socks5h -- that lookup is both wrong and harmful: it describes this host's network rather than the proxy's, and it leaks the hostname of every queued URL to the local resolver, which is the one thing a SOCKS/Tor setup exists to prevent. It also failed closed when only the proxy could resolve the name, so a container pointed at the proxy's DNS port refused every add with 'Could not resolve host'. The address check is now skipped for hostnames that the carrying proxy will resolve, and kept everywhere else: for direct fetches, for hosts excluded by no_proxy, for socks4 (which resolves locally), and for hosts written as IP literals, which need no lookup and leak nothing. Scheme validation, the localhost/metadata blocklist and the connect-time socket guard are unchanged. download_proxies mirrors YoutubeDL.proxies rather than importing it: that property is only reachable from a constructed instance, and since it decides whether a security check runs, a quiet upstream change should leave the check in place rather than silently skip it. Also makes ALLOW_PRIVATE_ADDRESSES explicit in the download-queue test config -- unset on a MagicMock it is truthy, which had validate_url bypassing every check those tests asked it to run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -5,6 +5,7 @@ from __future__ import annotations
|
||||
import copy
|
||||
import os
|
||||
import re
|
||||
import socket
|
||||
import tempfile
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
@@ -27,6 +28,9 @@ def dq_env():
|
||||
cfg.AUDIO_DOWNLOAD_DIR = dl
|
||||
cfg.TEMP_DIR = dl
|
||||
cfg.MAX_CONCURRENT_DOWNLOADS = "3"
|
||||
# Explicit: an unset attribute on a MagicMock is truthy, which would
|
||||
# make validate_url bypass every SSRF check it is asked to run.
|
||||
cfg.ALLOW_PRIVATE_ADDRESSES = False
|
||||
cfg.YTDL_OPTIONS = {}
|
||||
cfg.YTDL_OPTIONS_PRESETS = {}
|
||||
cfg.CUSTOM_DIRS = True
|
||||
@@ -164,6 +168,43 @@ async def test_add_ssrf_rejected_url_recorded_as_failed_entry(dq_env):
|
||||
notifier.completed.assert_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_add_does_not_resolve_hostname_when_proxied(dq_env):
|
||||
"""With a remote-DNS proxy configured, adding a URL must not look its host
|
||||
up here: that both leaks the hostname to the local resolver and fails closed
|
||||
when only the proxy can resolve it (issue #1079)."""
|
||||
dq_env.YTDL_OPTIONS = {"proxy": "socks5h://tor:9050"}
|
||||
notifier = AsyncMock()
|
||||
|
||||
def fake_extract(self, url, *_args, **_kwargs):
|
||||
return {"_type": "video", "id": "vid1", "title": "t", "webpage_url": url}
|
||||
|
||||
dq = DownloadQueue(dq_env, notifier)
|
||||
with patch("url_guard.socket.getaddrinfo", side_effect=AssertionError("resolved")), \
|
||||
patch.object(DownloadQueue, "_DownloadQueue__extract_info", fake_extract):
|
||||
result = await dq.add(
|
||||
"https://only-the-proxy-can-resolve.invalid/x",
|
||||
"video", "auto", "any", "best", "", "", 0, auto_start=False,
|
||||
)
|
||||
assert result["status"] == "ok"
|
||||
dq.close()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_add_resolves_hostname_when_not_proxied(dq_env):
|
||||
"""Without a proxy the address check still runs and still rejects."""
|
||||
notifier = AsyncMock()
|
||||
url = "https://internal.invalid/x"
|
||||
|
||||
dq = DownloadQueue(dq_env, notifier)
|
||||
with patch("url_guard.socket.getaddrinfo",
|
||||
return_value=[(socket.AF_INET, socket.SOCK_STREAM, socket.IPPROTO_TCP, "",
|
||||
("169.254.169.254", 0))]):
|
||||
result = await dq.add(url, "video", "auto", "any", "best", "", "", 0, auto_start=False)
|
||||
assert result["status"] == "error"
|
||||
dq.close()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_cancel_removes_from_pending(dq_env):
|
||||
notifier = AsyncMock()
|
||||
|
||||
Reference in New Issue
Block a user