From 494b85d4404be2657f72abc8320209bc4512e2db Mon Sep 17 00:00:00 2001
From: Peifan Li <frank.li.oxygen@gmail.com>
Date: Sun, 4 Jan 2026 22:03:41 -0500
Subject: [PATCH] refactor: Add default timeout for thumbnail downloads

---
 .../services/downloaders/BaseDownloader.ts    |  6 ++-
 .../services/downloaders/MissAVDownloader.ts  | 42 +++++++++++-------
 .../downloaders/bilibili/bilibiliVideo.ts     | 37 +++++++++++++---
 .../services/downloaders/ytdlp/ytdlpVideo.ts  | 16 ++++++-
 backend/src/utils/ytDlpUtils.ts               | 44 ++++++++++++++++++-
 5 files changed, 119 insertions(+), 26 deletions(-)

diff --git a/backend/src/services/downloaders/BaseDownloader.ts b/backend/src/services/downloaders/BaseDownloader.ts
index 9311576..5c7a09b 100644
--- a/backend/src/services/downloaders/BaseDownloader.ts
+++ b/backend/src/services/downloaders/BaseDownloader.ts
@@ -41,6 +41,9 @@ export abstract class BaseDownloader implements IDownloader {
     options?: DownloadOptions
   ): Promise<Video>;
 
+  // Default timeout for thumbnail downloads (60 seconds)
+  protected static readonly THUMBNAIL_DOWNLOAD_TIMEOUT = 60000;
+
   /**
    * Common helper to download a thumbnail
    */
@@ -51,7 +54,7 @@ export abstract class BaseDownloader implements IDownloader {
   ): Promise<boolean> {
     try {
       logger.info("Downloading thumbnail from:", thumbnailUrl);
-      if (axiosConfig.proxy) {
+      if (axiosConfig.proxy || axiosConfig.httpAgent) {
         logger.debug("Using proxy for thumbnail download");
       }
 
@@ -62,6 +65,7 @@ export abstract class BaseDownloader implements IDownloader {
         method: "GET",
         url: thumbnailUrl,
         responseType: "stream",
+        timeout: BaseDownloader.THUMBNAIL_DOWNLOAD_TIMEOUT,
         ...axiosConfig,
       });
 
diff --git a/backend/src/services/downloaders/MissAVDownloader.ts b/backend/src/services/downloaders/MissAVDownloader.ts
index ffe267c..b7bea4b 100644
--- a/backend/src/services/downloaders/MissAVDownloader.ts
+++ b/backend/src/services/downloaders/MissAVDownloader.ts
@@ -13,6 +13,7 @@ import {
   getAxiosProxyConfig,
   getNetworkConfigFromUserConfig,
   getUserYtDlpConfig,
+  InvalidProxyError,
 } from "../../utils/ytDlpUtils";
 import * as storageService from "../storageService";
 import { Video } from "../storageService";
@@ -163,13 +164,13 @@ export class MissAVDownloader extends BaseDownloader {
 
       // 4. Select the best m3u8 URL from collected URLs
       let m3u8Url = MissAVDownloader.selectBestM3u8Url(m3u8Urls, hasFormatSort);
-      
+
       if (m3u8Url) {
         logger.info(
           `Selected m3u8 URL from ${m3u8Urls.length} candidates (format sort: ${hasFormatSort}):`,
           m3u8Url
         );
-        const alternatives = m3u8Urls.filter(u => u !== m3u8Url);
+        const alternatives = m3u8Urls.filter((u) => u !== m3u8Url);
         if (alternatives.length > 0) {
           logger.info("Alternative URLs:", alternatives);
         }
@@ -328,11 +329,7 @@ export class MissAVDownloader extends BaseDownloader {
       const args = [m3u8Url, ...flagsToArgs(flags)];
 
       // Log the full command for debugging
-      logger.info(
-        "Executing yt-dlp command:",
-        YT_DLP_PATH,
-        args.join(" ")
-      );
+      logger.info("Executing yt-dlp command:", YT_DLP_PATH, args.join(" "));
 
       try {
         await new Promise<void>((resolve, reject) => {
@@ -393,9 +390,21 @@ export class MissAVDownloader extends BaseDownloader {
       // 8. Download and save the thumbnail
       if (thumbnailUrl) {
         // Use base class method via temporary instance
-        const axiosConfig = userConfig.proxy
-          ? getAxiosProxyConfig(userConfig.proxy)
-          : {};
+        let axiosConfig = {};
+        if (userConfig.proxy) {
+          try {
+            axiosConfig = getAxiosProxyConfig(userConfig.proxy);
+          } catch (error) {
+            if (error instanceof InvalidProxyError) {
+              logger.warn(
+                "Invalid proxy configuration for thumbnail download, proceeding without proxy:",
+                error.message
+              );
+            } else {
+              throw error;
+            }
+          }
+        }
         const downloader = new MissAVDownloader();
         thumbnailSaved = await downloader.downloadThumbnail(
           thumbnailUrl,
@@ -509,7 +518,10 @@ export class MissAVDownloader extends BaseDownloader {
   }
 
   // Helper to select best m3u8 URL
-  static selectBestM3u8Url(urls: string[], hasFormatSort: boolean): string | null {
+  static selectBestM3u8Url(
+    urls: string[],
+    hasFormatSort: boolean
+  ): string | null {
     if (urls.length === 0) return null;
 
     const sortedUrls = [...urls].sort((a, b) => {
@@ -543,10 +555,8 @@ export class MissAVDownloader extends BaseDownloader {
         // BUT, given the bug report where a 240p stream was picked over a master,
         // we should probably trust the master playlist more particularly if the alternative is low quality.
         // However, if we have a high quality specific stream (e.g. 720p/1080p explicit), that might be fine.
-        
         // Let's refine: If one is surrit master, pick it. (Handled by step 1 & surrit sub-logic)
         // If neither is surrit, and one is master...
-        
         // If both are master or both are not master, compare resolution.
       }
 
@@ -559,7 +569,7 @@ export class MissAVDownloader extends BaseDownloader {
       // If we have a significant resolution difference, we might prefer the higher one
       // UNLESS one is a master playlist and the other is a low res specific one.
       // If one is master (0p detected) and other is 240p, 0p (master) should win if it's likely to contain better streams.
-      
+
       // Updated Strategy:
       // If both have resolution, compare them.
       if (aQualityNum > 0 && bQualityNum > 0) {
@@ -570,14 +580,14 @@ export class MissAVDownloader extends BaseDownloader {
       // If we are prioritizing master playlists (e.g. because of surrit or format sort), master wins.
       // If we are NOT specifically prioritizing master, we still might want to prefer it over very low res (e.g. < 480p).
       if (aIsMaster && bQualityNum > 0 && bQualityNum < 480) return -1; // Master wins over < 480p
-      if (bIsMaster && aQualityNum > 0 && aQualityNum < 480) return 1;  // Master wins over < 480p
+      if (bIsMaster && aQualityNum > 0 && aQualityNum < 480) return 1; // Master wins over < 480p
 
       // Fallback: Default to higher number (so 720p wins over 0p/master if we didn't catch it above)
       // This preserves 'best attempt' for specific high quality URLs if they exist not on surrit.
       if (aQualityNum !== bQualityNum) {
         return bQualityNum - aQualityNum;
       }
-      
+
       // Final tie-breaker: prefer master if all else equal
       if (aIsMaster && !bIsMaster) return -1;
       if (!aIsMaster && bIsMaster) return 1;
diff --git a/backend/src/services/downloaders/bilibili/bilibiliVideo.ts b/backend/src/services/downloaders/bilibili/bilibiliVideo.ts
index e0998ec..54b7319 100644
--- a/backend/src/services/downloaders/bilibili/bilibiliVideo.ts
+++ b/backend/src/services/downloaders/bilibili/bilibiliVideo.ts
@@ -12,6 +12,7 @@ import {
   getAxiosProxyConfig,
   getNetworkConfigFromUserConfig,
   getUserYtDlpConfig,
+  InvalidProxyError,
 } from "../../../utils/ytDlpUtils";
 import * as storageService from "../../storageService";
 import { Video } from "../../storageService";
@@ -258,9 +259,21 @@ export async function downloadVideo(
     let thumbnailSaved = false;
     if (thumbnailUrl) {
       // Use base class method via temporary instance
-      const axiosConfig = userConfig.proxy
-        ? getAxiosProxyConfig(userConfig.proxy)
-        : {};
+      let axiosConfig = {};
+      if (userConfig.proxy) {
+        try {
+          axiosConfig = getAxiosProxyConfig(userConfig.proxy);
+        } catch (error) {
+          if (error instanceof InvalidProxyError) {
+            logger.warn(
+              "Invalid proxy configuration for thumbnail download, proceeding without proxy:",
+              error.message
+            );
+          } else {
+            throw error;
+          }
+        }
+      }
       const downloader = new BilibiliDownloaderHelper();
       thumbnailSaved = await downloader.downloadThumbnailPublic(
         thumbnailUrl,
@@ -445,9 +458,21 @@ export async function downloadSinglePart(
         : collectionName
           ? `/subtitles/${collectionName}`
           : `/subtitles`;
-      const axiosConfig = userConfig.proxy
-        ? getAxiosProxyConfig(userConfig.proxy)
-        : {};
+      let axiosConfig = {};
+      if (userConfig.proxy) {
+        try {
+          axiosConfig = getAxiosProxyConfig(userConfig.proxy);
+        } catch (error) {
+          if (error instanceof InvalidProxyError) {
+            logger.warn(
+              "Invalid proxy configuration for subtitle download, proceeding without proxy:",
+              error.message
+            );
+          } else {
+            throw error;
+          }
+        }
+      }
       subtitles = await downloadSubtitles(
         url,
         newSafeBaseFilename,
diff --git a/backend/src/services/downloaders/ytdlp/ytdlpVideo.ts b/backend/src/services/downloaders/ytdlp/ytdlpVideo.ts
index ced2769..37938b7 100644
--- a/backend/src/services/downloaders/ytdlp/ytdlpVideo.ts
+++ b/backend/src/services/downloaders/ytdlp/ytdlpVideo.ts
@@ -14,6 +14,7 @@ import {
   getAxiosProxyConfig,
   getNetworkConfigFromUserConfig,
   getUserYtDlpConfig,
+  InvalidProxyError,
 } from "../../../utils/ytDlpUtils";
 import * as storageService from "../../storageService";
 import { Video } from "../../storageService";
@@ -305,7 +306,20 @@ export async function downloadVideo(
       let axiosConfig = {};
       
       if (downloadUserConfig.proxy) {
-        axiosConfig = getAxiosProxyConfig(downloadUserConfig.proxy);
+        try {
+          axiosConfig = getAxiosProxyConfig(downloadUserConfig.proxy);
+        } catch (error) {
+          if (error instanceof InvalidProxyError) {
+            // Log the error but continue without proxy for thumbnail
+            // Video download already succeeded, don't fail for thumbnail proxy issues
+            logger.warn(
+              "Invalid proxy configuration for thumbnail download, proceeding without proxy:",
+              error.message
+            );
+          } else {
+            throw error;
+          }
+        }
       }
 
       thumbnailSaved = await downloader.downloadThumbnailPublic(
diff --git a/backend/src/utils/ytDlpUtils.ts b/backend/src/utils/ytDlpUtils.ts
index 9314b94..01a12ec 100644
--- a/backend/src/utils/ytDlpUtils.ts
+++ b/backend/src/utils/ytDlpUtils.ts
@@ -511,10 +511,28 @@ export function getNetworkConfigFromUserConfig(
   return networkOptions;
 }
 
+/**
+ * Error thrown when proxy configuration is invalid
+ */
+export class InvalidProxyError extends Error {
+  readonly proxyUrl: string;
+  readonly originalError?: Error;
+
+  constructor(proxyUrl: string, originalError?: Error) {
+    super(`Invalid proxy URL: ${proxyUrl}`);
+    this.name = "InvalidProxyError";
+    this.proxyUrl = proxyUrl;
+    this.originalError = originalError;
+  }
+}
+
 /**
  * Helper to convert a proxy URL string into an Axios config object
  * Supports http/https/socks5 proxies with authentication
  * Format: http://user:pass@host:port or socks5://user:pass@host:port
+ *
+ * @throws {InvalidProxyError} If the proxy URL is malformed - this prevents
+ *         silent fallback to direct connection which could expose user's real IP
  */
 export function getAxiosProxyConfig(proxyUrl: string): any {
   if (!proxyUrl) return {};
@@ -523,6 +541,11 @@ export function getAxiosProxyConfig(proxyUrl: string): any {
     const url = new URL(proxyUrl);
     const protocol = url.protocol.replace(":", "");
 
+    // Validate that we have a hostname
+    if (!url.hostname) {
+      throw new InvalidProxyError(proxyUrl, new Error("Missing hostname"));
+    }
+
     // Check if this is a SOCKS proxy
     if (protocol.startsWith("socks")) {
       // Use SocksProxyAgent for SOCKS proxy support
@@ -534,6 +557,14 @@ export function getAxiosProxyConfig(proxyUrl: string): any {
       };
     }
 
+    // Validate protocol for non-SOCKS proxies
+    if (protocol !== "http" && protocol !== "https") {
+      throw new InvalidProxyError(
+        proxyUrl,
+        new Error(`Unsupported proxy protocol: ${protocol}`)
+      );
+    }
+
     // Handle HTTP/HTTPS proxies
     const isHttps = protocol === "https";
     const defaultPort = isHttps ? 443 : 80;
@@ -554,7 +585,16 @@ export function getAxiosProxyConfig(proxyUrl: string): any {
 
     return { proxy: proxyConfig };
   } catch (error) {
-    console.error("Invalid proxy URL:", proxyUrl);
-    return {};
+    // Re-throw InvalidProxyError as-is
+    if (error instanceof InvalidProxyError) {
+      throw error;
+    }
+    // Wrap other errors (like URL parsing errors) in InvalidProxyError
+    // This ensures we fail rather than silently falling back to direct connection
+    console.error("Invalid proxy URL:", proxyUrl, error);
+    throw new InvalidProxyError(
+      proxyUrl,
+      error instanceof Error ? error : new Error(String(error))
+    );
   }
 }