From 4e99cbe759f54854de47488e2ad0c3da857a2952 Mon Sep 17 00:00:00 2001 From: WEIREN FENG Date: Thu, 3 Sep 2026 19:42:46 -0700 Subject: [PATCH] Verify a Cloudinary media url is our asset, not just a Cloudinary url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TRUSTED_MEDIA_URL_HOSTS is ["res.cloudinary.com"], and validateTrustedHttpsUrl checked the protocol and the hostname and nothing else. A hostname proves the bytes are served by Cloudinary. It does not prove they are ours. Anyone can register a free Cloudinary account, and https://res.cloudinary.com//image/upload/... passed that check unchanged. So a client could call createPostCallable directly with a url in their own bucket — never requesting an upload signature at all — and skip the entire pipeline: the 10MB/80MB size caps, the per-user petnote/users/{uid} folder, the signature rate limit. 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 replaced afterwards at the same url, on every post, avatar, cover image and check-in that referenced it. Same primitive lets one user publish another user's asset url as their own media. Two checks now, and only for res.cloudinary.com: the first path segment is our cloud name, and the path runs through the petnote/ folder our signing callable creates. dicebear and lh3.googleusercontent.com are untouched — they serve generated default avatars and Google sign-in photos, which are not ours to fingerprint by path, and rejecting them would blank the avatar of every user who never uploaded one. NOT enforcing that the folder's uid matches the caller. Family members co-edit a pet's avatar, so uploader and writer are legitimately different people, and reuse inside our own bucket is a far smaller problem than a foreign bucket. The cloud name is read from process.env rather than CLOUDINARY_CLOUD_NAME .value(), so shared.ts does not have to import a secret param; a bound secret is an env var at runtime. The eleven callables that validate media urls now bind it. A callable that reaches the validator without it throws rather than degrading to host-only checking, because a silent downgrade would reopen the hole invisibly. isTrustedHttpsUrl, the read-path variant used by getNotificationActor on triggers as well as callables, stays host-only. It is not an authorization decision — it chooses between a stored avatar and a generated default — and ownership is enforced where the url is written. Urls stored before this are not re-checked; that needs a backfill, not a read-path change. Four new tests fail on the old validator and pass on this one. Three pass on both, as guardrails: our own cloud is still accepted, a never-allowed host is still rejected, and a dicebear avatar still works. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/__tests__/media-url-ownership.test.ts | 121 ++++++++++++++++++ functions/src/meetups.ts | 16 ++- functions/src/pets.ts | 16 ++- functions/src/places.ts | 30 ++++- functions/src/posts.ts | 9 +- functions/src/shared.ts | 68 +++++++++- functions/src/users.ts | 16 ++- 7 files changed, 259 insertions(+), 17 deletions(-) create mode 100644 functions/src/__tests__/media-url-ownership.test.ts diff --git a/functions/src/__tests__/media-url-ownership.test.ts b/functions/src/__tests__/media-url-ownership.test.ts new file mode 100644 index 0000000..c0348d8 --- /dev/null +++ b/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//... 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); + }); +}); diff --git a/functions/src/meetups.ts b/functions/src/meetups.ts index 0542b23..0ad75c2 100644 --- a/functions/src/meetups.ts +++ b/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."); diff --git a/functions/src/pets.ts b/functions/src/pets.ts index 2249688..3378527 100644 --- a/functions/src/pets.ts +++ b/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."); diff --git a/functions/src/places.ts b/functions/src/places.ts index 13b233e..45398ae 100644 --- a/functions/src/places.ts +++ b/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."); diff --git a/functions/src/posts.ts b/functions/src/posts.ts index 3fda33e..f71e2cd 100644 --- a/functions/src/posts.ts +++ b/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."); diff --git a/functions/src/shared.ts b/functions/src/shared.ts index 4311a87..b98348c 100644 --- a/functions/src/shared.ts +++ b/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//...` + * 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[] diff --git a/functions/src/users.ts b/functions/src/users.ts index 608cb78..3f351e0 100644 --- a/functions/src/users.ts +++ b/functions/src/users.ts @@ -1,7 +1,7 @@ import { createHash, randomInt } from "node:crypto"; import { onDocumentCreated, 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 { cascadeDeleteMeetup, cascadeDeletePet, @@ -283,7 +283,12 @@ export const onFamilyCreated = onDocumentCreated( } ); -export const ensureUserProfileCallable = onCall(async (request) => { +export const ensureUserProfileCallable = 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."); @@ -393,7 +398,12 @@ export const checkDisplayNameAvailabilityCallable = onCall(async (request) => { return { available: !taken, taken }; }); -export const updateUserProfileCallable = onCall(async (request) => { +export const updateUserProfileCallable = 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.");