From 239349a07086552eb02ef584cd1c60d50aebbc91 Mon Sep 17 00:00:00 2001 From: WEIREN FENG Date: Sun, 6 Sep 2026 15:16:58 -0700 Subject: [PATCH] Make the Cloudinary cloud name a constant, not a secret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cloud name is the first path segment of every image URL the app serves. It is public by construction — anyone who has loaded a single photo already has it — and storing it in Secret Manager protected nothing. What it did instead was invent a failure mode. Because it was a secret param, every callable that validates a media URL had to remember `secrets: [CLOUDINARY_CLOUD_NAME]`, and one that forgot would pass CI and throw in production on the first upload. CI cannot catch that: the emulator drives handlers through .run(), which bypasses secret mounting entirely, and setup.ts set the variable directly, so the tests saw a value the deployed function would not have had. #185's own description flagged this and had to derive the list of eleven bindings by grepping call paths rather than by testing it. All eleven bindings are gone. The value lives in platform.ts next to CLOUDINARY_FOLDER, and shared.ts reads it directly instead of reaching into process.env — which also removes the "misconfigured" branch that existed only to handle a caller arriving without the binding. There is no longer a way to arrive without it. CLOUDINARY_API_KEY and CLOUDINARY_API_SECRET stay in Secret Manager. Those are the credentials, and media.ts keeps binding them. The truthiness check there now covers only those two, since the cloud name cannot be empty. setup.ts stops setting CLOUDINARY_CLOUD_NAME, and the tests compare against the constant. That is the part that matters beyond tidiness: a test can no longer pass because the environment supplied something production would not have. Functions 80/80, build / lint / typecheck:test clean. Co-Authored-By: Claude Opus 5 (1M context) --- functions/src/__tests__/flows.test.ts | 4 +-- .../src/__tests__/media-url-ownership.test.ts | 11 +++---- functions/src/__tests__/setup.ts | 6 +++- functions/src/media.ts | 14 +++++---- functions/src/meetups.ts | 16 ++-------- functions/src/pets.ts | 16 ++-------- functions/src/places.ts | 30 ++++--------------- functions/src/platform.ts | 15 +++++++++- functions/src/posts.ts | 9 ++---- functions/src/shared.ts | 25 +++++++--------- functions/src/users.ts | 16 ++-------- 11 files changed, 62 insertions(+), 100 deletions(-) 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.");