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.");