feat(webapp): delete the previous profile photo after a new one is stored

Author: kathiekiwi

apps/webapp/app/routes/resources.account.avatar.ts

@@ -2,6 +2,7 @@ import { json, type ActionFunctionArgs } from "@remix-run/node";
 import { updateUserAvatarUrl } from "~/models/user.server";
 import { requireUser } from "~/services/session.server";
 import {
+  deleteStaleUserAvatar,
   isAvatarUploadRejection,
   parseAvatarUpload,
   uploadUserAvatar,
@@ -20,9 +21,13 @@ export async function action({ request }: ActionFunctionArgs) {
     return json({ error: upload.error }, { status: upload.status });
   }
 
-  const { avatarUrl } = await uploadUserAvatar({ userId: user.id, ...upload });
+  const previousAvatarUrl = user.avatarUrl;
+
+  const { avatarUrl, filename } = await uploadUserAvatar({ userId: user.id, ...upload });
 
   await updateUserAvatarUrl({ id: user.id, avatarUrl });
 
+  await deleteStaleUserAvatar({ previousAvatarUrl, userId: user.id, filename });
+
   return json({ avatarUrl });
 }

apps/webapp/app/services/userAvatar.server.ts

@@ -1,4 +1,5 @@
 import { createHash } from "node:crypto";
+import { logger } from "~/services/logger.server";
 import {
   AVATAR_EXTENSIONS,
   type AvatarContentType,
@@ -98,7 +99,60 @@ export async function uploadUserAvatar({
   const { client, objectKey } = requireAvatarObjectStore();
   await client.putObject(objectKey(path), data, contentType);
 
-  return { avatarUrl: buildUserAvatarUrl(userId, filename) };
+  return { filename, avatarUrl: buildUserAvatarUrl(userId, filename) };
+}
+
+const AVATAR_URL_REGEX = /^\/resources\/account\/avatar\/([^/]+)\/([^/]+)$/;
+
+/**
+ * Undefined unless the stored URL is this user's own avatar route and names a different object:
+ * an OAuth avatar elsewhere is not ours to delete, and the same content hash is the same file.
+ */
+export function resolveStaleAvatarObjectPath({
+  previousAvatarUrl,
+  userId,
+  filename,
+}: {
+  previousAvatarUrl: string | null;
+  userId: string;
+  filename: string;
+}): string | undefined {
+  const match = previousAvatarUrl?.match(AVATAR_URL_REGEX);
+
+  if (!match) {
+    return undefined;
+  }
+
+  const [, previousUserId, previousFilename] = match;
+
+  if (previousUserId !== userId || previousFilename === filename) {
+    return undefined;
+  }
+
+  return resolveUserAvatarObjectPath(previousUserId, previousFilename);
+}
+
+export async function deleteStaleUserAvatar(options: {
+  previousAvatarUrl: string | null;
+  userId: string;
+  filename: string;
+}) {
+  const path = resolveStaleAvatarObjectPath(options);
+
+  if (!path) {
+    return;
+  }
+
+  try {
+    const { client, objectKey } = requireAvatarObjectStore();
+    await client.deleteObject(objectKey(path));
+  } catch (error) {
+    logger.warn("Failed to delete the previous avatar", {
+      userId: options.userId,
+      path,
+      error: error instanceof Error ? error.message : String(error),
+    });
+  }
 }
 
 export function presignUserAvatarUrl(objectPath: string) {

apps/webapp/app/v3/objectStoreClient.server.ts

@@ -1,5 +1,10 @@
 import { AwsClient } from "aws4fetch";
-import { GetObjectCommand, PutObjectCommand, S3Client } from "@aws-sdk/client-s3";
+import {
+  DeleteObjectCommand,
+  GetObjectCommand,
+  PutObjectCommand,
+  S3Client,
+} from "@aws-sdk/client-s3";
 import { getSignedUrl } from "@aws-sdk/s3-request-presigner";
 
 /**
@@ -19,6 +24,7 @@ interface IObjectStoreClient {
     contentType: string
   ): Promise<string>;
   getObject(key: string): Promise<string>;
+  deleteObject(key: string): Promise<void>;
   presign(key: string, method: "PUT" | "GET", expiresIn: number): Promise<string>;
 }
 
@@ -76,6 +82,13 @@ class Aws4FetchClient implements IObjectStoreClient {
     return response.text();
   }
 
+  async deleteObject(key: string): Promise<void> {
+    const response = await this.awsClient.fetch(this.buildUrl(key), { method: "DELETE" });
+    if (!response.ok) {
+      throw new Error(`Failed to delete from object store: ${response.statusText}`);
+    }
+  }
+
   async presign(key: string, method: "PUT" | "GET", expiresIn: number): Promise<string> {
     const url = new URL(this.config.baseUrl);
     url.pathname = normalizeObjectStoreLogicalKeyPathname(key);
@@ -151,6 +164,12 @@ class AwsSdkClient implements IObjectStoreClient {
     return response.Body.transformToString();
   }
 
+  async deleteObject(key: string): Promise<void> {
+    await this.s3Client.send(
+      new DeleteObjectCommand({ Bucket: this.config.bucket, Key: this.toS3ObjectKey(key) })
+    );
+  }
+
   async presign(key: string, method: "PUT" | "GET", expiresIn: number): Promise<string> {
     const s3Key = this.toS3ObjectKey(key);
     const command =
@@ -221,6 +240,10 @@ export class ObjectStoreClient implements IObjectStoreClient {
     return this.impl.getObject(key);
   }
 
+  deleteObject(key: string): Promise<void> {
+    return this.impl.deleteObject(key);
+  }
+
   presign(key: string, method: "PUT" | "GET", expiresIn: number): Promise<string> {
     return this.impl.presign(key, method, expiresIn);
   }

apps/webapp/test/userAvatar.test.ts

@@ -4,6 +4,7 @@ import {
   buildUserAvatarUrl,
   isAvatarUploadRejection,
   parseAvatarUpload,
+  resolveStaleAvatarObjectPath,
   resolveUserAvatarObjectPath,
 } from "~/services/userAvatar.server";
 import { MAX_AVATAR_SIZE_IN_BYTES } from "~/utils/avatarLimits";
@@ -112,3 +113,48 @@ describe("parseAvatarUpload", () => {
     expect(isAvatarUploadRejection(await parseAvatarUpload(form))).toBe(false);
   });
 });
+
+describe("resolveStaleAvatarObjectPath", () => {
+  const previous = filenameFor([1, 2, 3]);
+  const next = filenameFor([4, 5, 6]);
+
+  it("derives the old object from the stored URL", () => {
+    expect(
+      resolveStaleAvatarObjectPath({
+        previousAvatarUrl: buildUserAvatarUrl(USER_ID, previous),
+        userId: USER_ID,
+        filename: next,
+      })
+    ).toBe(`avatars/${USER_ID}/${previous}`);
+  });
+
+  it("keeps the object when the content hash is unchanged", () => {
+    expect(
+      resolveStaleAvatarObjectPath({
+        previousAvatarUrl: buildUserAvatarUrl(USER_ID, previous),
+        userId: USER_ID,
+        filename: previous,
+      })
+    ).toBeUndefined();
+  });
+
+  it.each([
+    ["no previous avatar", null],
+    ["an OAuth avatar hosted elsewhere", "https://avatars.githubusercontent.com/u/1?v=4"],
+    [
+      "an absolute URL onto our own path",
+      `https://evil.test/resources/account/avatar/${USER_ID}/${previous}`,
+    ],
+    ["another user's avatar", `/resources/account/avatar/usr_other/${previous}`],
+    [
+      "a filename that is not content-addressed",
+      `/resources/account/avatar/${USER_ID}/../../secret.png`,
+    ],
+    ["a deeper path", `/resources/account/avatar/${USER_ID}/${previous}/extra`],
+    ["an unrelated app path", "/resources/account/photo"],
+  ])("leaves %s alone", (_case, previousAvatarUrl) => {
+    expect(
+      resolveStaleAvatarObjectPath({ previousAvatarUrl, userId: USER_ID, filename: next })
+    ).toBeUndefined();
+  });
+});