Tighten SSRF checks and remote download cleanup

app/services/downloader.py

@@ -29,14 +29,16 @@ class Downloader:
         current_url = url
         timeout = httpx.Timeout(self.settings.download_timeout_seconds, connect=30.0)
         headers = {"User-Agent": "MediaAPI/1.0"}
-        async with httpx.AsyncClient(timeout=timeout, headers=headers) as client:
+        # Do not silently route a security-sensitive fetch through ambient
+        # HTTP(S)_PROXY settings that can change the destination semantics.
+        async with httpx.AsyncClient(
+            timeout=timeout, headers=headers, follow_redirects=False, trust_env=False
+        ) as client:
             for _ in range(6):
                 await self.validate_url(current_url)
                 destination: Path | None = None
                 try:
-                    async with client.stream(
-                        "GET", current_url, follow_redirects=False
-                    ) as response:
+                    async with client.stream("GET", current_url, follow_redirects=False) as response:
                         if response.status_code in {301, 302, 303, 307, 308}:
                             location = response.headers.get("location")
                             if not location:
@@ -45,21 +47,18 @@ class Downloader:
                             continue
                         response.raise_for_status()
                         declared = self._content_length(response.headers.get("content-length"))
-                        if declared and declared > self.settings.max_upload_size:
+                        if declared is not None and declared > self.settings.max_upload_size:
                             raise InputError("Remote media exceeds MAX_UPLOAD_SIZE")
                         name = self.validator.safe_filename(
                             filename or self._response_filename(response, current_url),
                             "download.bin",
                         )
-                        unique_name = f"{uuid4().hex}_{name}"
-                        destination = destination_dir / unique_name
+                        destination = destination_dir / f"{uuid4().hex}_{name}"
                         size = 0
                         async with aiofiles.open(destination, "wb") as output:
                             async for chunk in response.aiter_bytes(1024 * 1024):
                                 size += len(chunk)
                                 if size > self.settings.max_upload_size:
-                                    await output.close()
-                                    destination.unlink(missing_ok=True)
                                     raise InputError("Remote media exceeds MAX_UPLOAD_SIZE")
                                 await output.write(chunk)
                         mime_type = response.headers.get("content-type", "").split(";", 1)[0]
@@ -88,21 +87,43 @@ class Downloader:
             raise DownloadError("Too many redirects while downloading media")
 
     async def validate_url(self, value: str) -> None:
-        parsed = urlparse(value)
-        if parsed.scheme not in {"http", "https"} or not parsed.hostname:
-            raise InputError("Only HTTP and HTTPS media URLs are supported")
-        if parsed.username or parsed.password:
-            raise InputError("URLs containing credentials are not allowed")
+        try:
+            parsed = urlparse(value)
+            if (
+                parsed.scheme.lower() not in {"http", "https"}
+                or not parsed.hostname
+                or parsed.username is not None
+                or parsed.password is not None
+                or any(char.isspace() for char in parsed.hostname)
+            ):
+                raise InputError("Only credential-free HTTP and HTTPS media URLs are supported")
+            port = parsed.port
+        except ValueError as exc:
+            raise InputError("Media URL contains an invalid host or port") from exc
+
         if self.settings.allow_private_urls:
             return
+
+        target_port = port or (443 if parsed.scheme.lower() == "https" else 80)
         try:
+            # Reject the entire hostname if any resolved address is non-global.
+            # This blocks mixed public/private DNS answers rather than trusting
+            # the first address returned by the resolver.
             records = await asyncio.to_thread(
-                socket.getaddrinfo, parsed.hostname, parsed.port or 443, type=socket.SOCK_STREAM
+                socket.getaddrinfo,
+                parsed.hostname,
+                target_port,
+                type=socket.SOCK_STREAM,
             )
-        except socket.gaierror as exc:
+        except (socket.gaierror, OSError) as exc:
             raise DownloadError("The remote hostname could not be resolved") from exc
+        if not records:
+            raise DownloadError("The remote hostname has no usable address")
         for record in records:
-            address = ipaddress.ip_address(record[4][0])
+            try:
+                address = ipaddress.ip_address(record[4][0].split("%", 1)[0])
+            except (ValueError, IndexError, TypeError) as exc:
+                raise DownloadError("The remote hostname resolved to an invalid address") from exc
             if not address.is_global:
                 raise InputError("Private, loopback, and link-local URLs are not allowed")
 
@@ -117,7 +138,10 @@ class Downloader:
 
     @staticmethod
     def _content_length(value: str | None) -> int | None:
+        if value is None:
+            return None
         try:
-            return int(value) if value else None
-        except ValueError:
+            parsed = int(value)
+        except (TypeError, ValueError):
             return None
+        return parsed if parsed >= 0 else None