fix: address PR #102 review feedback (GRO-145)

- factories.ts: add photoKey/photoUploadedAt null defaults to buildPet (TS regression fix)
- s3.ts: lazy singleton S3Client to avoid re-instantiation per call
- routes/pets.ts: server-side 5MB file size limit, explicit content-type allowlist (drops image/svg+xml etc), validate confirm key ownership against pets/${petId}/ prefix, delete old S3 object on re-upload, fix RBAC comment on DELETE photo
- PetPhotoUpload.tsx: bypass canvas resize for GIFs (preserves animation), pass fileSizeBytes in upload-url request
- Add PetPhotoDisplay.test.tsx: 7 tests covering fetch states, placeholder, refetch on petId change, custom size
- Add PetPhotoUpload.test.tsx: 8 tests covering idle state, type validation, upload flow, progress, GIF bypass
- Update petPhotos.test.ts: add SVG rejection, 5MB limit, key ownership, and old-photo deletion tests (18 total)

Co-Authored-By: Paperclip <noreply@paperclip.ing>

apps/api/src/__tests__/petPhotos.test.ts

@@ -101,7 +101,7 @@ describe("POST /pets/:petId/photo/upload-url", () => {
     const res = await app.request(`/pets/${PET_ID}/photo/upload-url`, {
       method: "POST",
       headers: { "Content-Type": "application/json" },
-      body: JSON.stringify({ contentType: "image/jpeg" }),
+      body: JSON.stringify({ contentType: "image/jpeg", fileSizeBytes: 1024 }),
     });
     expect(res.status).toBe(200);
     const body = (await res.json()) as { uploadUrl: string; key: string };
@@ -115,7 +115,27 @@ describe("POST /pets/:petId/photo/upload-url", () => {
     const res = await app.request(`/pets/${PET_ID}/photo/upload-url`, {
       method: "POST",
       headers: { "Content-Type": "application/json" },
-      body: JSON.stringify({ contentType: "application/pdf" }),
+      body: JSON.stringify({ contentType: "application/pdf", fileSizeBytes: 1024 }),
+    });
+    expect(res.status).toBe(400);
+  });
+
+  it("rejects image/svg+xml with 400 (allowlist enforcement)", async () => {
+    const app = buildApp(MANAGER);
+    const res = await app.request(`/pets/${PET_ID}/photo/upload-url`, {
+      method: "POST",
+      headers: { "Content-Type": "application/json" },
+      body: JSON.stringify({ contentType: "image/svg+xml", fileSizeBytes: 1024 }),
+    });
+    expect(res.status).toBe(400);
+  });
+
+  it("rejects fileSizeBytes over 5 MB with 400", async () => {
+    const app = buildApp(MANAGER);
+    const res = await app.request(`/pets/${PET_ID}/photo/upload-url`, {
+      method: "POST",
+      headers: { "Content-Type": "application/json" },
+      body: JSON.stringify({ contentType: "image/jpeg", fileSizeBytes: 6 * 1024 * 1024 }),
     });
     expect(res.status).toBe(400);
   });
@@ -126,7 +146,7 @@ describe("POST /pets/:petId/photo/upload-url", () => {
     const res = await app.request(`/pets/${PET_ID}/photo/upload-url`, {
       method: "POST",
       headers: { "Content-Type": "application/json" },
-      body: JSON.stringify({ contentType: "image/jpeg" }),
+      body: JSON.stringify({ contentType: "image/jpeg", fileSizeBytes: 1024 }),
     });
     expect(res.status).toBe(404);
   });
@@ -136,7 +156,7 @@ describe("POST /pets/:petId/photo/upload-url", () => {
     const res = await app.request(`/pets/${PET_ID}/photo/upload-url`, {
       method: "POST",
       headers: { "Content-Type": "application/json" },
-      body: JSON.stringify({ contentType: "image/png" }),
+      body: JSON.stringify({ contentType: "image/png", fileSizeBytes: 1024 }),
     });
     expect(res.status).toBe(200);
   });
@@ -177,6 +197,34 @@ describe("POST /pets/:petId/photo/confirm", () => {
     });
     expect(res.status).toBe(404);
   });
+
+  it("returns 400 when key does not belong to the pet", async () => {
+    const app = buildApp(MANAGER);
+    const res = await app.request(`/pets/${PET_ID}/photo/confirm`, {
+      method: "POST",
+      headers: { "Content-Type": "application/json" },
+      body: JSON.stringify({ key: "pets/other-pet-id/1700000000000.jpg" }),
+    });
+    expect(res.status).toBe(400);
+    const body = (await res.json()) as { error: string };
+    expect(body.error).toMatch(/invalid key/i);
+  });
+
+  it("deletes old photo from storage when re-uploading", async () => {
+    const { deleteObject } = await import("../lib/s3.js");
+    const oldKey = `pets/${PET_ID}/old.jpg`;
+    dbPetRow = { ...dbPetRow!, photoKey: oldKey };
+
+    const app = buildApp(MANAGER);
+    const res = await app.request(`/pets/${PET_ID}/photo/confirm`, {
+      method: "POST",
+      headers: { "Content-Type": "application/json" },
+      body: JSON.stringify({ key: PHOTO_KEY }),
+    });
+
+    expect(res.status).toBe(200);
+    expect(deleteObject).toHaveBeenCalledWith(oldKey);
+  });
 });
 
 // ─── DELETE /:petId/photo ────────────────────────────────────────────────────

apps/api/src/lib/s3.ts

@@ -6,16 +6,21 @@ import {
 } from "@aws-sdk/client-s3";
 import { getSignedUrl } from "@aws-sdk/s3-request-presigner";
 
+let s3Instance: S3Client | null = null;
+
 function getS3Client(): S3Client {
-  return new S3Client({
-    endpoint: process.env.S3_ENDPOINT,
-    region: process.env.S3_REGION ?? "us-east-1",
-    credentials: {
-      accessKeyId: process.env.S3_ACCESS_KEY_ID ?? "",
-      secretAccessKey: process.env.S3_SECRET_ACCESS_KEY ?? "",
-    },
-    forcePathStyle: true, // required for Ceph RGW
-  });
+  if (!s3Instance) {
+    s3Instance = new S3Client({
+      endpoint: process.env.S3_ENDPOINT,
+      region: process.env.S3_REGION ?? "us-east-1",
+      credentials: {
+        accessKeyId: process.env.S3_ACCESS_KEY_ID ?? "",
+        secretAccessKey: process.env.S3_SECRET_ACCESS_KEY ?? "",
+      },
+      forcePathStyle: true, // required for Ceph RGW
+    });
+  }
+  return s3Instance;
 }
 
 function getBucket(): string {
@@ -26,6 +31,7 @@ function getBucket(): string {
 export async function getPresignedUploadUrl(
   key: string,
   contentType: string,
+  sizeBytes: number,
   expiresIn = 900
 ): Promise<string> {
   const client = getS3Client();
@@ -33,6 +39,7 @@ export async function getPresignedUploadUrl(
     Bucket: getBucket(),
     Key: key,
     ContentType: contentType,
+    ContentLength: sizeBytes,
   });
   return getSignedUrl(client, command, { expiresIn });
 }

apps/api/src/routes/pets.ts

@@ -99,12 +99,22 @@ petsRouter.delete("/:id", async (c) => {
 
 // ─── Photo routes ──────────────────────────────────────────────────────────────
 
+const ALLOWED_CONTENT_TYPES = new Set([
+  "image/jpeg",
+  "image/png",
+  "image/webp",
+  "image/gif",
+]);
+
+const MAX_PHOTO_SIZE = 5 * 1024 * 1024; // 5 MB
+
 const uploadUrlSchema = z.object({
-  contentType: z
-    .string()
-    .refine((v) => v.startsWith("image/"), {
-      message: "contentType must be an image/* MIME type",
-    }),
+  contentType: z.string().refine((v) => ALLOWED_CONTENT_TYPES.has(v), {
+    message: "contentType must be one of: image/jpeg, image/png, image/webp, image/gif",
+  }),
+  fileSizeBytes: z.number().int().positive().max(MAX_PHOTO_SIZE, {
+    message: "File must not exceed 5 MB",
+  }),
 });
 
 const confirmSchema = z.object({
@@ -122,14 +132,14 @@ petsRouter.post(
   async (c) => {
     const db = getDb();
     const petId = c.req.param("petId");
-    const { contentType } = c.req.valid("json");
+    const { contentType, fileSizeBytes } = c.req.valid("json");
 
     const [pet] = await db.select().from(pets).where(eq(pets.id, petId));
     if (!pet) return c.json({ error: "Pet not found" }, 404);
 
     const ext = contentType.split("/")[1] ?? "jpg";
     const key = `pets/${petId}/${Date.now()}.${ext}`;
-    const uploadUrl = await getPresignedUploadUrl(key, contentType);
+    const uploadUrl = await getPresignedUploadUrl(key, contentType, fileSizeBytes);
 
     return c.json({ uploadUrl, key });
   }
@@ -148,6 +158,19 @@ petsRouter.post(
     const petId = c.req.param("petId");
     const { key } = c.req.valid("json");
 
+    // Validate that the key belongs to this pet to prevent key hijacking
+    if (!key.startsWith(`pets/${petId}/`)) {
+      return c.json({ error: "Invalid key" }, 400);
+    }
+
+    const [pet] = await db.select().from(pets).where(eq(pets.id, petId));
+    if (!pet) return c.json({ error: "Pet not found" }, 404);
+
+    // Delete the previous photo from storage to avoid orphaned objects
+    if (pet.photoKey) {
+      await deleteObject(pet.photoKey);
+    }
+
     const [row] = await db
       .update(pets)
       .set({ photoKey: key, photoUploadedAt: new Date(), updatedAt: new Date() })
@@ -162,7 +185,7 @@ petsRouter.post(
 /**
  * DELETE /:petId/photo
  * Removes the photo from object storage and clears the DB record.
- * Manager-only (write-destructive operation).
+ * All staff roles (manager, receptionist, groomer) may call this.
  */
 petsRouter.delete("/:petId/photo", async (c) => {
   const db = getDb();