diff --git a/functions/src/__tests__/flows.test.ts b/functions/src/__tests__/flows.test.ts index 32ea72e..f7fe5d9 100644 --- a/functions/src/__tests__/flows.test.ts +++ b/functions/src/__tests__/flows.test.ts @@ -1,6 +1,6 @@ import "./setup"; import { afterAll, beforeEach, describe, expect, it } from "vitest"; -import { admin, db } from "../platform"; +import { CLOUDINARY_CLOUD_NAME, admin, db } from "../platform"; import { ensureUserProfileCallable, deleteUserAccount } from "../users"; import { createPostCallable, createCommentCallable } from "../posts"; import { createPetCallable } from "../pets"; @@ -324,7 +324,7 @@ describe("cloudinary upload signature", () => { .digest("hex"); expect(res.signature).toBe(expected); - expect(res.cloudName).toBe(process.env.CLOUDINARY_CLOUD_NAME); + expect(res.cloudName).toBe(CLOUDINARY_CLOUD_NAME); expect(res.apiKey).toBe(process.env.CLOUDINARY_API_KEY); }); diff --git a/functions/src/__tests__/media-url-ownership.test.ts b/functions/src/__tests__/media-url-ownership.test.ts index c0348d8..64060be 100644 --- a/functions/src/__tests__/media-url-ownership.test.ts +++ b/functions/src/__tests__/media-url-ownership.test.ts @@ -1,6 +1,6 @@ import "./setup"; import { afterAll, beforeEach, describe, expect, it } from "vitest"; -import { admin, db } from "../platform"; +import { admin, db, CLOUDINARY_CLOUD_NAME } from "../platform"; import { createPostCallable } from "../posts"; import { updatePetCallable } from "../pets"; import { callAs, clearRateLimits, errorCodeOf } from "./helpers"; @@ -13,17 +13,18 @@ import { callAs, clearRateLimits, errorCodeOf } from "./helpers"; // 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. +// The cloud name is a plain constant in platform.ts, so these tests exercise +// the exact value production uses rather than an environment stand-in. const OWNER = "media-owner"; const PET = "media-pet"; +const CLOUD = CLOUDINARY_CLOUD_NAME; const ours = (p = "petnote/users/media-owner/photo.jpg") => - `https://res.cloudinary.com/test-cloud/image/upload/v1700000000/${p}`; + `https://res.cloudinary.com/${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"; +const ourCloudOutsideFolder = `https://res.cloudinary.com/${CLOUD}/image/upload/v1700000000/somewhere-else/photo.jpg`; async function wipe() { for (const c of ["users", "pets", "posts", "callableRateLimits", "notifications"]) { diff --git a/functions/src/__tests__/setup.ts b/functions/src/__tests__/setup.ts index 6e439d6..a197a7d 100644 --- a/functions/src/__tests__/setup.ts +++ b/functions/src/__tests__/setup.ts @@ -11,7 +11,11 @@ process.env.GCLOUD_PROJECT ||= "petnote-test"; // defineSecret(...).value() reads process.env, so the Cloudinary signature // tests can supply values without a real Secret Manager. These are obviously // fake and exist only so the signing path can be exercised end to end. -process.env.CLOUDINARY_CLOUD_NAME ||= "test-cloud"; +// +// CLOUDINARY_CLOUD_NAME is deliberately NOT here. It is a plain constant in +// platform.ts, not a secret, so tests read the same value production does — +// which is the point: a test can no longer pass because the environment +// happened to supply something the deployed function would not have. process.env.CLOUDINARY_API_KEY ||= "test-api-key"; process.env.CLOUDINARY_API_SECRET ||= "test-api-secret"; process.env.FIREBASE_CONFIG ||= JSON.stringify({ diff --git a/functions/src/media.ts b/functions/src/media.ts index e01054a..ae06dbb 100644 --- a/functions/src/media.ts +++ b/functions/src/media.ts @@ -64,7 +64,7 @@ function userFolder(callerUid: string): string { export const getCloudinaryUploadSignature = onCall( { - secrets: [CLOUDINARY_CLOUD_NAME, CLOUDINARY_API_KEY, CLOUDINARY_API_SECRET], + secrets: [CLOUDINARY_API_KEY, CLOUDINARY_API_SECRET], }, async (request) => { const callerUid = request.auth?.uid; @@ -90,11 +90,12 @@ export const getCloudinaryUploadSignature = onCall( throw new HttpsError("invalid-argument", "resourceType must be 'image' or 'video'."); } - const cloudName = CLOUDINARY_CLOUD_NAME.value(); + const cloudName = CLOUDINARY_CLOUD_NAME; const apiKey = CLOUDINARY_API_KEY.value(); const apiSecret = CLOUDINARY_API_SECRET.value(); - if (!cloudName || !apiKey || !apiSecret) { + // cloudName is a constant now, so only the two real secrets can be missing. + if (!apiKey || !apiSecret) { throw new HttpsError("failed-precondition", "Cloudinary secrets are not configured."); } @@ -140,7 +141,7 @@ export const getCloudinaryUploadSignature = onCall( // destroyed because the prefix won't match. export const deleteCloudinaryAssetsCallable = onCall( { - secrets: [CLOUDINARY_CLOUD_NAME, CLOUDINARY_API_KEY, CLOUDINARY_API_SECRET], + secrets: [CLOUDINARY_API_KEY, CLOUDINARY_API_SECRET], }, async (request) => { const callerUid = request.auth?.uid; @@ -191,10 +192,11 @@ export const deleteCloudinaryAssetsCallable = onCall( validated.push({ publicId, resourceType }); } - const cloudName = CLOUDINARY_CLOUD_NAME.value(); + const cloudName = CLOUDINARY_CLOUD_NAME; const apiKey = CLOUDINARY_API_KEY.value(); const apiSecret = CLOUDINARY_API_SECRET.value(); - if (!cloudName || !apiKey || !apiSecret) { + // cloudName is a constant now, so only the two real secrets can be missing. + if (!apiKey || !apiSecret) { throw new HttpsError("failed-precondition", "Cloudinary secrets are not configured."); } diff --git a/functions/src/meetups.ts b/functions/src/meetups.ts index 0ad75c2..0542b23 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, CLOUDINARY_CLOUD_NAME } from "./platform"; +import { admin, db } from "./platform"; import { assertActorNotDeleting, getNotificationActor } from "./notifications"; import { assertRateLimit, @@ -225,12 +225,7 @@ export const onParticipantDeleted = onDocumentDeleted( } ); -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) => { +export const createMeetupCallable = onCall(async (request) => { const callerAuth = request.auth; const callerUid = callerAuth?.uid; if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in."); @@ -426,12 +421,7 @@ export const createMeetupCallable = onCall( return { id: meetupRef.id }; }); -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) => { +export const updateMeetupCallable = onCall(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 3378527..2249688 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, CLOUDINARY_CLOUD_NAME } from "./platform"; +import { admin, db } from "./platform"; import { cascadeDeletePet } from "./cleanup"; import { assertActorNotDeleting, getNotificationActor } from "./notifications"; import { @@ -188,12 +188,7 @@ export async function getAccessiblePet( return canAccess ? petData : null; } -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) => { +export const createPetCallable = onCall(async (request) => { const callerUid = request.auth?.uid; if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in."); @@ -252,12 +247,7 @@ export const createPetCallable = onCall( return { id: petRef.id }; }); -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) => { +export const updatePetCallable = onCall(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 44502cb..46cf5a9 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, CLOUDINARY_CLOUD_NAME } from "./platform"; +import { admin, db } from "./platform"; import { assertActorNotDeleting, getNotificationActor } from "./notifications"; import { deleteCollectionPath } from "./cleanup"; import { @@ -483,12 +483,7 @@ export const onLocationDeleted = onDocumentDeleted( } ); -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) => { +export const addPlaceCallable = onCall(async (request) => { const callerAuth = request.auth; const callerUid = callerAuth?.uid; if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in."); @@ -552,12 +547,7 @@ export const addPlaceCallable = onCall( return { locationId, alreadyExisted }; }); -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) => { +export const addLocationPhotosCallable = onCall(async (request) => { const callerAuth = request.auth; const callerUid = callerAuth?.uid; if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in."); @@ -600,12 +590,7 @@ export const addLocationPhotosCallable = onCall( return { success: true }; }); -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) => { +export const submitReviewCallable = onCall(async (request) => { const callerAuth = request.auth; const callerUid = callerAuth?.uid; if (!callerUid) throw new HttpsError("unauthenticated", "Must be logged in."); @@ -748,12 +733,7 @@ export const submitReviewCallable = onCall( return { id: reviewId }; }); -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) => { +export const checkInCallable = onCall(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/platform.ts b/functions/src/platform.ts index 5caa2ce..82b59f0 100644 --- a/functions/src/platform.ts +++ b/functions/src/platform.ts @@ -11,12 +11,25 @@ if (admin.apps.length === 0) { } const db = admin.firestore(); -const CLOUDINARY_CLOUD_NAME = defineSecret("CLOUDINARY_CLOUD_NAME"); const CLOUDINARY_API_KEY = defineSecret("CLOUDINARY_API_KEY"); const CLOUDINARY_API_SECRET = defineSecret("CLOUDINARY_API_SECRET"); const GEOAPIFY_API_KEY = defineSecret("GEOAPIFY_API_KEY"); const CLOUDINARY_FOLDER = "petnote"; +// Not a secret, and it was a mistake to store it as one. The cloud name is the +// first path segment of every image URL the app serves — it is public by +// construction, and anyone who has loaded a single photo has it. +// +// Treating it as a secret invented a failure mode with no upside: any callable +// that validates a media URL had to remember `secrets: [CLOUDINARY_CLOUD_NAME]`, +// and one that forgot passed CI — the emulator drives handlers through .run(), +// which bypasses secret mounting entirely, and setup.ts sets the variable +// directly — then threw in production on the first upload. Eleven callables +// carried that binding purely to read a value that was never confidential. +// +// The API key and secret stay in Secret Manager. Those are the credentials. +const CLOUDINARY_CLOUD_NAME = "dgeunvmmn"; + export { admin, db, diff --git a/functions/src/posts.ts b/functions/src/posts.ts index f71e2cd..3fda33e 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, CLOUDINARY_CLOUD_NAME } from "./platform"; +import { admin, db } from "./platform"; import { cascadeDeletePost, deleteQueryDocs } from "./cleanup"; import { assertActorNotDeleting, getNotificationActor } from "./notifications"; import { @@ -137,12 +137,7 @@ export const onPostWritten = onDocumentWritten("posts/{postId}", async (event) = }); }); -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) => { +export const createPostCallable = onCall(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 b98348c..cffe8e6 100644 --- a/functions/src/shared.ts +++ b/functions/src/shared.ts @@ -1,5 +1,10 @@ import { HttpsError } from "firebase-functions/v2/https"; -import { admin, db, CLOUDINARY_FOLDER } from "./platform"; +import { + admin, + db, + CLOUDINARY_CLOUD_NAME, + CLOUDINARY_FOLDER, +} from "./platform"; export const FIRESTORE_BATCH_LIMIT = 450; export const LOCATION_PHOTO_PREVIEW_LIMIT = 30; @@ -181,19 +186,11 @@ export const CLOUDINARY_HOST = "res.cloudinary.com"; * 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." - ); - } + // A plain constant now, not a secret param read out of the environment. + // There is no longer a way for a caller to reach this function without the + // cloud name available, so the "misconfigured" branch that used to guard + // that case is gone with it. + const cloudName = CLOUDINARY_CLOUD_NAME; if ( !parsed.pathname.startsWith(`/${cloudName}/`) || !parsed.pathname.includes(`/${CLOUDINARY_FOLDER}/`) diff --git a/functions/src/users.ts b/functions/src/users.ts index 3f351e0..608cb78 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, CLOUDINARY_CLOUD_NAME } from "./platform"; +import { admin, db } from "./platform"; import { cascadeDeleteMeetup, cascadeDeletePet, @@ -283,12 +283,7 @@ export const onFamilyCreated = onDocumentCreated( } ); -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) => { +export const ensureUserProfileCallable = onCall(async (request) => { const callerUid = request.auth?.uid; if (!callerUid) { throw new HttpsError("unauthenticated", "Must be logged in."); @@ -398,12 +393,7 @@ export const checkDisplayNameAvailabilityCallable = onCall(async (request) => { return { available: !taken, taken }; }); -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) => { +export const updateUserProfileCallable = onCall(async (request) => { const callerUid = request.auth?.uid; if (!callerUid) { throw new HttpsError("unauthenticated", "Must be logged in.");