Restore #185: verify a Cloudinary media url is our asset

functions/src/__tests__/media-url-ownership.test.ts

@@ -0,0 +1,121 @@
+import "./setup";
+import { afterAll, beforeEach, describe, expect, it } from "vitest";
+import { admin, db } from "../platform";
+import { createPostCallable } from "../posts";
+import { updatePetCallable } from "../pets";
+import { callAs, clearRateLimits, errorCodeOf } from "./helpers";
+
+// A host allowlist proves the bytes are served by Cloudinary. It does not
+// prove they are OUR bytes. Anyone can register a free Cloudinary account, and
+// https://res.cloudinary.com/<their-cloud>/... passes a hostname check
+// unchanged — skipping the whole upload pipeline (size caps, per-user folder,
+// signature rate limit) and, worse, keeping the attacker in control of the
+// asset, so anything that survives moderation can be swapped afterwards at the
+// same URL.
+//
+// setup.ts sets CLOUDINARY_CLOUD_NAME=test-cloud, so "test-cloud" is ours.
+
+const OWNER = "media-owner";
+const PET = "media-pet";
+
+const ours = (p = "petnote/users/media-owner/photo.jpg") =>
+  `https://res.cloudinary.com/test-cloud/image/upload/v1700000000/${p}`;
+const foreignCloud =
+  "https://res.cloudinary.com/attacker-cloud/image/upload/v1700000000/petnote/users/media-owner/photo.jpg";
+const ourCloudOutsideFolder =
+  "https://res.cloudinary.com/test-cloud/image/upload/v1700000000/somewhere-else/photo.jpg";
+
+async function wipe() {
+  for (const c of ["users", "pets", "posts", "callableRateLimits", "notifications"]) {
+    const snap = await db.collection(c).get();
+    for (const d of snap.docs) await db.recursiveDelete(d.ref).catch(() => undefined);
+  }
+  const users = await admin.auth().listUsers(1000);
+  await Promise.all(
+    users.users.map((u) => admin.auth().deleteUser(u.uid).catch(() => undefined))
+  );
+}
+
+beforeEach(async () => {
+  await wipe();
+  await clearRateLimits();
+  await admin.auth().createUser({
+    uid: OWNER,
+    email: `${OWNER}@example.com`,
+    emailVerified: true,
+  });
+  await db.doc(`users/${OWNER}`).set({ displayName: OWNER });
+  await db.doc(`pets/${PET}`).set({ name: "Rex", ownerId: OWNER, species: "dog" });
+});
+afterAll(wipe);
+
+const post = (url: string) =>
+  callAs<{ id: string }>(createPostCallable, OWNER, {
+    text: "hello",
+    petId: PET,
+    media: [{ url, type: "image" }],
+  });
+
+describe("cloudinary media urls must be our own assets", () => {
+  it("accepts an asset in our cloud, under our folder", async () => {
+    const res = await post(ours());
+    const stored = (await db.doc(`posts/${res.id}`).get()).data() ?? {};
+    expect(stored.media[0].url).toBe(ours());
+  });
+
+  it("refuses an identical path in someone else's cloud", async () => {
+    // The whole attack in one line: same host, same folder, different bucket.
+    expect(await errorCodeOf(() => post(foreignCloud))).toBe("invalid-argument");
+  });
+
+  it("refuses our cloud outside the petnote folder", async () => {
+    expect(await errorCodeOf(() => post(ourCloudOutsideFolder))).toBe(
+      "invalid-argument"
+    );
+  });
+
+  it("refuses a foreign cloud in the thumbnail as well as the url", async () => {
+    // thumbUrl is validated separately and was just as exploitable.
+    expect(
+      await errorCodeOf(() =>
+        callAs(createPostCallable, OWNER, {
+          text: "hello",
+          petId: PET,
+          media: [{ url: ours(), type: "image", thumbUrl: foreignCloud }],
+        })
+      )
+    ).toBe("invalid-argument");
+  });
+
+  it("still refuses a host that was never allowed", async () => {
+    expect(
+      await errorCodeOf(() => post("https://evil.example.com/x.jpg"))
+    ).toBe("invalid-argument");
+  });
+
+  it("applies to pet avatars too, not just post media", async () => {
+    expect(
+      await errorCodeOf(() =>
+        callAs(updatePetCallable, OWNER, { petId: PET, avatarUrl: foreignCloud })
+      )
+    ).toBe("invalid-argument");
+    const ok = await callAs(updatePetCallable, OWNER, {
+      petId: PET,
+      avatarUrl: ours("petnote/users/media-owner/avatar.jpg"),
+    });
+    expect(ok).toBeTruthy();
+  });
+
+  it("leaves the non-Cloudinary avatar hosts alone", async () => {
+    // dicebear generates our default avatars and Google serves profile photos
+    // from sign-in; neither is ours to fingerprint by path, and breaking them
+    // would blank the avatar of every user who has not uploaded one.
+    const dicebear = "https://api.dicebear.com/7.x/thumbs/svg?seed=media-owner";
+    const ok = await callAs(updatePetCallable, OWNER, {
+      petId: PET,
+      avatarUrl: dicebear,
+    });
+    expect(ok).toBeTruthy();
+    expect((await db.doc(`pets/${PET}`).get()).data()?.avatarUrl).toBe(dicebear);
+  });
+});

functions/src/meetups.ts

@@ -1,7 +1,7 @@
 import { onDocumentDeleted } from "firebase-functions/v2/firestore";
 import { onCall, HttpsError } from "firebase-functions/v2/https";
 import { onSchedule } from "firebase-functions/v2/scheduler";
-import { admin, db } from "./platform";
+import { admin, db, CLOUDINARY_CLOUD_NAME } from "./platform";
 import { assertActorNotDeleting, getNotificationActor } from "./notifications";
 import {
   assertRateLimit,
@@ -225,7 +225,12 @@ export const onParticipantDeleted = onDocumentDeleted(
   }
 );
 
-export const createMeetupCallable = onCall(async (request) => {
+export const createMeetupCallable = onCall(
+  // Binds the cloud name so validateTrustedHttpsUrl can confirm a
+  // res.cloudinary.com url is OUR asset and not a free account someone
+  // else controls. Without it the validator throws rather than degrade.
+  { secrets: [CLOUDINARY_CLOUD_NAME] },
+  async (request) => {
   const callerAuth = request.auth;
   const callerUid = callerAuth?.uid;
   if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in.");
@@ -421,7 +426,12 @@ export const createMeetupCallable = onCall(async (request) => {
   return { id: meetupRef.id };
 });
 
-export const updateMeetupCallable = onCall(async (request) => {
+export const updateMeetupCallable = onCall(
+  // Binds the cloud name so validateTrustedHttpsUrl can confirm a
+  // res.cloudinary.com url is OUR asset and not a free account someone
+  // else controls. Without it the validator throws rather than degrade.
+  { secrets: [CLOUDINARY_CLOUD_NAME] },
+  async (request) => {
   const callerUid = request.auth?.uid;
   if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in.");
 

functions/src/pets.ts

@@ -1,5 +1,5 @@
 import { onCall, HttpsError } from "firebase-functions/v2/https";
-import { admin, db } from "./platform";
+import { admin, db, CLOUDINARY_CLOUD_NAME } from "./platform";
 import { cascadeDeletePet } from "./cleanup";
 import { assertActorNotDeleting, getNotificationActor } from "./notifications";
 import {
@@ -188,7 +188,12 @@ export async function getAccessiblePet(
   return canAccess ? petData : null;
 }
 
-export const createPetCallable = onCall(async (request) => {
+export const createPetCallable = onCall(
+  // Binds the cloud name so validateTrustedHttpsUrl can confirm a
+  // res.cloudinary.com url is OUR asset and not a free account someone
+  // else controls. Without it the validator throws rather than degrade.
+  { secrets: [CLOUDINARY_CLOUD_NAME] },
+  async (request) => {
   const callerUid = request.auth?.uid;
   if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in.");
 
@@ -247,7 +252,12 @@ export const createPetCallable = onCall(async (request) => {
   return { id: petRef.id };
 });
 
-export const updatePetCallable = onCall(async (request) => {
+export const updatePetCallable = onCall(
+  // Binds the cloud name so validateTrustedHttpsUrl can confirm a
+  // res.cloudinary.com url is OUR asset and not a free account someone
+  // else controls. Without it the validator throws rather than degrade.
+  { secrets: [CLOUDINARY_CLOUD_NAME] },
+  async (request) => {
   const callerUid = request.auth?.uid;
   if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in.");
 

functions/src/places.ts

@@ -1,7 +1,7 @@
 import { createHash } from "node:crypto";
 import { onDocumentCreated, onDocumentDeleted } from "firebase-functions/v2/firestore";
 import { onCall, HttpsError } from "firebase-functions/v2/https";
-import { admin, db } from "./platform";
+import { admin, db, CLOUDINARY_CLOUD_NAME } from "./platform";
 import { assertActorNotDeleting, getNotificationActor } from "./notifications";
 import { deleteCollectionPath } from "./cleanup";
 import {
@@ -478,7 +478,12 @@ export const onLocationDeleted = onDocumentDeleted(
   }
 );
 
-export const addPlaceCallable = onCall(async (request) => {
+export const addPlaceCallable = onCall(
+  // Binds the cloud name so validateTrustedHttpsUrl can confirm a
+  // res.cloudinary.com url is OUR asset and not a free account someone
+  // else controls. Without it the validator throws rather than degrade.
+  { secrets: [CLOUDINARY_CLOUD_NAME] },
+  async (request) => {
   const callerAuth = request.auth;
   const callerUid = callerAuth?.uid;
   if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in.");
@@ -542,7 +547,12 @@ export const addPlaceCallable = onCall(async (request) => {
   return { locationId, alreadyExisted };
 });
 
-export const addLocationPhotosCallable = onCall(async (request) => {
+export const addLocationPhotosCallable = onCall(
+  // Binds the cloud name so validateTrustedHttpsUrl can confirm a
+  // res.cloudinary.com url is OUR asset and not a free account someone
+  // else controls. Without it the validator throws rather than degrade.
+  { secrets: [CLOUDINARY_CLOUD_NAME] },
+  async (request) => {
   const callerAuth = request.auth;
   const callerUid = callerAuth?.uid;
   if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in.");
@@ -585,7 +595,12 @@ export const addLocationPhotosCallable = onCall(async (request) => {
   return { success: true };
 });
 
-export const submitReviewCallable = onCall(async (request) => {
+export const submitReviewCallable = onCall(
+  // Binds the cloud name so validateTrustedHttpsUrl can confirm a
+  // res.cloudinary.com url is OUR asset and not a free account someone
+  // else controls. Without it the validator throws rather than degrade.
+  { secrets: [CLOUDINARY_CLOUD_NAME] },
+  async (request) => {
   const callerAuth = request.auth;
   const callerUid = callerAuth?.uid;
   if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in.");
@@ -728,7 +743,12 @@ export const submitReviewCallable = onCall(async (request) => {
   return { id: reviewId };
 });
 
-export const checkInCallable = onCall(async (request) => {
+export const checkInCallable = onCall(
+  // Binds the cloud name so validateTrustedHttpsUrl can confirm a
+  // res.cloudinary.com url is OUR asset and not a free account someone
+  // else controls. Without it the validator throws rather than degrade.
+  { secrets: [CLOUDINARY_CLOUD_NAME] },
+  async (request) => {
   const callerAuth = request.auth;
   const callerUid = callerAuth?.uid;
   if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in.");

functions/src/posts.ts

@@ -1,6 +1,6 @@
 import { onDocumentWritten } from "firebase-functions/v2/firestore";
 import { onCall, HttpsError } from "firebase-functions/v2/https";
-import { admin, db } from "./platform";
+import { admin, db, CLOUDINARY_CLOUD_NAME } from "./platform";
 import { cascadeDeletePost, deleteQueryDocs } from "./cleanup";
 import { assertActorNotDeleting, getNotificationActor } from "./notifications";
 import {
@@ -137,7 +137,12 @@ export const onPostWritten = onDocumentWritten("posts/{postId}", async (event) =
   });
 });
 
-export const createPostCallable = onCall(async (request) => {
+export const createPostCallable = onCall(
+  // Binds the cloud name so validateTrustedHttpsUrl can confirm a
+  // res.cloudinary.com url is OUR asset and not a free account someone
+  // else controls. Without it the validator throws rather than degrade.
+  { secrets: [CLOUDINARY_CLOUD_NAME] },
+  async (request) => {
   const callerAuth = request.auth;
   const callerUid = callerAuth?.uid;
   if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in.");

functions/src/shared.ts

@@ -1,5 +1,5 @@
 import { HttpsError } from "firebase-functions/v2/https";
-import { admin, db } from "./platform";
+import { admin, db, CLOUDINARY_FOLDER } from "./platform";
 
 export const FIRESTORE_BATCH_LIMIT = 450;
 export const LOCATION_PHOTO_PREVIEW_LIMIT = 30;
@@ -155,6 +155,56 @@ export function validateRatingScore(
   return value;
 }
 
+export const CLOUDINARY_HOST = "res.cloudinary.com";
+
+/**
+ * Rejects a res.cloudinary.com URL that is not one of OUR assets.
+ *
+ * The hostname alone proves nothing. Anyone can register a free Cloudinary
+ * account in under a minute, and `https://res.cloudinary.com/<their-cloud>/...`
+ * passes a host allowlist unchanged. That let a client skip the upload
+ * pipeline entirely — the size caps, the per-user folder, the signature rate
+ * limit — by never asking for a signature at all.
+ *
+ * The worse half is ownership. An asset in someone else's cloud stays under
+ * their control, so anything that passed a moderation pass could be swapped
+ * for something else afterwards, at the same URL, on every post, avatar, cover
+ * image and check-in that referenced it.
+ *
+ * Two checks: the cloud name (this is our bucket) and the folder (this is an
+ * asset our signing callable created — see userFolder() in media.ts, which
+ * puts everything under petnote/users/{uid}/).
+ *
+ * Deliberately NOT checking that the folder's uid matches the caller. Family
+ * members co-edit a pet's avatar, so the uploader and the writer are not
+ * always the same person, and cross-user reuse inside our own bucket is a much
+ * smaller problem than a foreign bucket.
+ */
+function assertOwnCloudinaryAsset(parsed: URL, fieldName: string): void {
+  // Read from the environment rather than CLOUDINARY_CLOUD_NAME.value() so
+  // this module does not have to import a secret param that every caller
+  // would then need to bind; a bound secret IS an env var at runtime.
+  const cloudName = process.env.CLOUDINARY_CLOUD_NAME;
+  if (!cloudName) {
+    // Loud, not lenient. A callable that validates media URLs without the
+    // cloud name available is a deploy misconfiguration, and quietly falling
+    // back to host-only checking would reopen the hole without anyone seeing.
+    throw new HttpsError(
+      "internal",
+      "Media URL validation is misconfigured on the server."
+    );
+  }
+  if (
+    !parsed.pathname.startsWith(`/${cloudName}/`) ||
+    !parsed.pathname.includes(`/${CLOUDINARY_FOLDER}/`)
+  ) {
+    throw new HttpsError(
+      "invalid-argument",
+      `${fieldName} must point at an asset uploaded through PetNote.`
+    );
+  }
+}
+
 export function validateTrustedHttpsUrl(
   value: string,
   fieldName: string,
@@ -172,9 +222,25 @@ export function validateTrustedHttpsUrl(
       `${fieldName} must use a trusted HTTPS host.`
     );
   }
+  // Only Cloudinary needs this. The other allowed hosts (dicebear, Google
+  // profile photos) serve generated or third-party avatars we do not own and
+  // cannot fingerprint by path.
+  if (parsed.hostname === CLOUDINARY_HOST) {
+    assertOwnCloudinaryAsset(parsed, fieldName);
+  }
   return value;
 }
 
+/**
+ * Read-path variant: decides whether a STORED url is still safe to hand back.
+ *
+ * Intentionally host-only, unlike validateTrustedHttpsUrl above. This runs in
+ * getNotificationActor, on triggers as well as callables, where the cloud name
+ * is not bound — and it is not an authorization decision, it only chooses
+ * between a stored avatar and a generated default. Ownership is enforced when
+ * the url is WRITTEN. Urls stored before that enforcement existed are not
+ * re-checked here; that would need a backfill, not a read-path change.
+ */
 export function isTrustedHttpsUrl(
   value: unknown,
   allowedHosts: readonly string[]