refactor: Proxy and cache avatar images (Fallenbagel#907)

* refactor: proxy and cache user avatar images * fix: extract keys * fix: set avatar image URL * fix: show the correct avatar in the list of available users in advanced request * fix(s): set correct src URL for cached image * fix: remove unexpired unused image when a user changes their avatar * fix: requested changes * refactor: use 'mime' package to detmerine file extension * style: grammar * refactor: checks if the default avatar is cached to avoid creating duplicates for different users * fix: fix vulnerability * fix: fix incomplete URL substring sanitization * refactor: only cache avatar with http url protocol * fix: remove log and correctly set the if statement for the cached image component * fix: avatar images not showing on issues page * style: formatting --------- Co-authored-by: JoaquinOlivero <[email protected]>
Ruakij · Sep 19, 2024 · edfd804 · edfd804
1 parent 2b05fff
commit edfd804
Show file tree

Hide file tree

Showing 25 changed files with 241 additions and 87 deletions.
diff --git a/next.config.js b/next.config.js
@@ -10,7 +10,6 @@ module.exports = {
     remotePatterns: [
       { hostname: 'gravatar.com' },
       { hostname: 'image.tmdb.org' },
-      { hostname: '*', protocol: 'https' },
     ],
   },
   webpack(config) {

diff --git a/overseerr-api.yml b/overseerr-api.yml
@@ -2790,6 +2790,15 @@ paths:
                           imageCount:
                             type: number
                             example: 123
+                      avatar:
+                        type: object
+                        properties:
+                          size:
+                            type: number
+                            example: 123456
+                          imageCount:
+                            type: number
+                            example: 123
                   apiCaches:
                     type: array
                     items:

diff --git a/package.json b/package.json
@@ -62,6 +62,7 @@
     "formik": "^2.4.6",
     "gravatar-url": "3.1.0",
     "lodash": "4.17.21",
+    "mime": "3",
     "next": "^14.2.4",
     "node-cache": "5.1.2",
     "node-gyp": "9.3.1",
@@ -119,6 +120,7 @@
     "@types/express": "4.17.17",
     "@types/express-session": "1.17.6",
     "@types/lodash": "4.14.191",
+    "@types/mime": "3",
     "@types/node": "20.14.8",
     "@types/node-schedule": "2.1.0",
     "@types/nodemailer": "6.4.7",

diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
diff --git a/server/index.ts b/server/index.ts
@@ -19,6 +19,7 @@ import { getSettings } from '@server/lib/settings';
 import logger from '@server/logger';
 import clearCookies from '@server/middleware/clearcookies';
 import routes from '@server/routes';
+import avatarproxy from '@server/routes/avatarproxy';
 import imageproxy from '@server/routes/imageproxy';
 import { getAppVersion } from '@server/utils/appVersion';
 import restartFlag from '@server/utils/restartFlag';
@@ -202,6 +203,7 @@ app
 
     // Do not set cookies so CDNs can cache them
     server.use('/imageproxy', clearCookies, imageproxy);
+    server.use('/avatarproxy', clearCookies, avatarproxy);
 
     server.get('*', (req, res) => handle(req, res));
     server.use(

diff --git a/server/interfaces/api/settingsInterfaces.ts b/server/interfaces/api/settingsInterfaces.ts
@@ -58,7 +58,7 @@ export interface CacheItem {
 
 export interface CacheResponse {
   apiCaches: CacheItem[];
-  imageCache: Record<'tmdb', { size: number; imageCount: number }>;
+  imageCache: Record<'tmdb' | 'avatar', { size: number; imageCount: number }>;
 }
 
 export interface StatusResponse {

diff --git a/server/job/schedule.ts b/server/job/schedule.ts
@@ -227,6 +227,9 @@ export const startJobs = (): void => {
       });
       // Clean TMDB image cache
       ImageProxy.clearCache('tmdb');
+
+      // Clean users avatar image cache
+      ImageProxy.clearCache('avatar');
     }),
   });
 

diff --git a/server/lib/imageproxy.ts b/server/lib/imageproxy.ts
@@ -3,6 +3,7 @@ import type { RateLimitOptions } from '@server/utils/rateLimit';
 import rateLimit from '@server/utils/rateLimit';
 import { createHash } from 'crypto';
 import { promises } from 'fs';
+import mime from 'mime/lite';
 import path, { join } from 'path';
 
 type ImageResponse = {
@@ -11,7 +12,7 @@ type ImageResponse = {
     curRevalidate: number;
     isStale: boolean;
     etag: string;
-    extension: string;
+    extension: string | null;
     cacheKey: string;
     cacheMiss: boolean;
   };
@@ -27,29 +28,45 @@ class ImageProxy {
     let deletedImages = 0;
     const cacheDirectory = path.join(baseCacheDirectory, key);
 
-    const files = await promises.readdir(cacheDirectory);
-
-    for (const file of files) {
-      const filePath = path.join(cacheDirectory, file);
-      const stat = await promises.lstat(filePath);
-
-      if (stat.isDirectory()) {
-        const imageFiles = await promises.readdir(filePath);
-
-        for (const imageFile of imageFiles) {
-          const [, expireAtSt] = imageFile.split('.');
-          const expireAt = Number(expireAtSt);
-          const now = Date.now();
+    try {
+      const files = await promises.readdir(cacheDirectory);
 
-          if (now > expireAt) {
-            await promises.rm(path.join(filePath, imageFile));
-            deletedImages += 1;
+      for (const file of files) {
+        const filePath = path.join(cacheDirectory, file);
+        const stat = await promises.lstat(filePath);
+
+        if (stat.isDirectory()) {
+          const imageFiles = await promises.readdir(filePath);
+
+          for (const imageFile of imageFiles) {
+            const [, expireAtSt] = imageFile.split('.');
+            const expireAt = Number(expireAtSt);
+            const now = Date.now();
+
+            if (now > expireAt) {
+              await promises.rm(path.join(filePath), {
+                recursive: true,
+              });
+              deletedImages += 1;
+            }
           }
         }
       }
+    } catch (e) {
+      if (e.code === 'ENOENT') {
+        logger.error('Directory not found', {
+          label: 'Image Cache',
+          message: e.message,
+        });
+      } else {
+        logger.error('Failed to read directory', {
+          label: 'Image Cache',
+          message: e.message,
+        });
+      }
     }
 
-    logger.info(`Cleared ${deletedImages} stale image(s) from cache`, {
+    logger.info(`Cleared ${deletedImages} stale image(s) from cache '${key}'`, {
       label: 'Image Cache',
     });
   }
@@ -69,33 +86,49 @@ class ImageProxy {
   }
 
   private static async getDirectorySize(dir: string): Promise<number> {
-    const files = await promises.readdir(dir, {
-      withFileTypes: true,
-    });
+    try {
+      const files = await promises.readdir(dir, {
+        withFileTypes: true,
+      });
 
-    const paths = files.map(async (file) => {
-      const path = join(dir, file.name);
+      const paths = files.map(async (file) => {
+        const path = join(dir, file.name);
 
-      if (file.isDirectory()) return await ImageProxy.getDirectorySize(path);
+        if (file.isDirectory()) return await ImageProxy.getDirectorySize(path);
 
-      if (file.isFile()) {
-        const { size } = await promises.stat(path);
+        if (file.isFile()) {
+          const { size } = await promises.stat(path);
 
-        return size;
-      }
+          return size;
+        }
 
-      return 0;
-    });
+        return 0;
+      });
+
+      return (await Promise.all(paths))
+        .flat(Infinity)
+        .reduce((i, size) => i + size, 0);
+    } catch (e) {
+      if (e.code === 'ENOENT') {
+        return 0;
+      }
+    }
 
-    return (await Promise.all(paths))
-      .flat(Infinity)
-      .reduce((i, size) => i + size, 0);
+    return 0;
   }
 
   private static async getImageCount(dir: string) {
-    const files = await promises.readdir(dir);
+    try {
+      const files = await promises.readdir(dir);
 
-    return files.length;
+      return files.length;
+    } catch (e) {
+      if (e.code === 'ENOENT') {
+        return 0;
+      }
+    }
+
+    return 0;
   }
 
   private fetch: typeof fetch;
@@ -147,6 +180,27 @@ class ImageProxy {
     return imageResponse;
   }
 
+  public async clearCachedImage(path: string) {
+    // find cacheKey
+    const cacheKey = this.getCacheKey(path);
+
+    try {
+      const directory = join(this.getCacheDirectory(), cacheKey);
+      const files = await promises.readdir(directory);
+
+      await promises.rm(directory, { recursive: true });
+
+      logger.info(`Cleared ${files[0]} from cache 'avatar'`, {
+        label: 'Image Cache',
+      });
+    } catch (e) {
+      logger.error('Failed to clear cached image', {
+        label: 'Image Cache',
+        message: e.message,
+      });
+    }
+  }
+
   private async get(cacheKey: string): Promise<ImageResponse | null> {
     try {
       const directory = join(this.getCacheDirectory(), cacheKey);
@@ -187,16 +241,25 @@ class ImageProxy {
       const directory = join(this.getCacheDirectory(), cacheKey);
       const href =
         this.baseUrl +
-        (this.baseUrl.endsWith('/') ? '' : '/') +
+        (this.baseUrl.length > 0
+          ? this.baseUrl.endsWith('/')
+            ? ''
+            : '/'
+          : '') +
         (path.startsWith('/') ? path.slice(1) : path);
       const response = await this.fetch(href);
       const arrayBuffer = await response.arrayBuffer();
       const buffer = Buffer.from(arrayBuffer);
 
-      const extension = path.split('.').pop() ?? '';
-      const maxAge = Number(
+      const extension = mime.getExtension(
+        response.headers.get('content-type') ?? ''
+      );
+
+      let maxAge = Number(
         (response.headers.get('cache-control') ?? '0').split('=')[1]
       );
+
+      if (!maxAge) maxAge = 86400;
       const expireAt = Date.now() + maxAge * 1000;
       const etag = (response.headers.get('etag') ?? '').replace(/"/g, '');
 
@@ -232,7 +295,7 @@ class ImageProxy {
 
   private async writeToCacheDir(
     dir: string,
-    extension: string,
+    extension: string | null,
     maxAge: number,
     expireAt: number,
     buffer: Buffer,