From bb520f82cfc936fb5c0bfb1a63e3ce93db302652 Mon Sep 17 00:00:00 2001 From: WEIREN FENG Date: Sat, 5 Sep 2026 23:21:49 -0700 Subject: [PATCH] Sign only the parameters Cloudinary verifies, unbreaking uploads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Photo upload has been failing with Invalid Signature. The server signed four parameters; Cloudinary verifies three. Cloudinary signs only the upload parameters it recognises and silently drops the rest. max_file_size is not one it recognises, so including it in the signed set produced a signature that could never match. Its own error said so — the String to sign it echoed back listed folder, timestamp and upload_preset, and nothing else. This is not a regression from #185, which never touched media.ts or the client upload path. It dates from whenever max_file_size entered the signature and has been broken ever since; nobody noticed because nobody had uploaded a photo through the UI in between. The comment above the limits is what caused this. It claimed they were "enforced on the signature itself so a leaked signature can't be reused to upload a bigger file than we allow" — a reasonable-sounding guarantee that Cloudinary does not offer. Replaced with what is actually true: the limits are advisory, they exist to feed the client-side check, and the only enforceable ceiling is whatever the petnote_image_signed / petnote_video_signed upload presets have configured in the Cloudinary console. The client stops sending max_file_size too. Cloudinary ignores it, so it bought nothing except the appearance that the limit was being transmitted somewhere meaningful. The size check that produces a useful error before a doomed upload stays, and the callable still returns maxFileSize to feed it. Two tests encoded the same false premise and are corrected. One of them — "signs exactly the parameters it returns" — already carried the comment "if the handler ever signs a different set of params than it hands back, Cloudinary rejects every upload". It was right, and it still passed, because it recomputed the same wrong set the handler used. max_file_size was on both sides of that comparison and on neither side of Cloudinary's. Its list is now written as the contract with Cloudinary rather than a mirror of the implementation. NOT VERIFIED HERE: whether the upload presets actually carry a max file size in the Cloudinary console. I cannot read that from the repo. If no limit is set there, then after this change there is no server-side size ceiling at all — only the advisory client check. Co-Authored-By: Claude Opus 5 (1M context) --- functions/src/__tests__/flows.test.ts | 23 +++++++++++++++++------ functions/src/media.ts | 23 +++++++++++++++++++---- src/services/cloudinary.ts | 7 ++++--- 3 files changed, 40 insertions(+), 13 deletions(-) diff --git a/functions/src/__tests__/flows.test.ts b/functions/src/__tests__/flows.test.ts index be07b0d..32ea72e 100644 --- a/functions/src/__tests__/flows.test.ts +++ b/functions/src/__tests__/flows.test.ts @@ -296,9 +296,19 @@ describe("cloudinary upload signature", () => { expect(res.folder).toBe(`petnote/users/${ALICE}`); }); - it("signs exactly the parameters it returns", async () => { + it("signs exactly the three parameters Cloudinary verifies", async () => { // Recomputed independently here. If the handler ever signs a different set - // of params than it hands back, Cloudinary rejects every upload. + // of params than Cloudinary verifies, Cloudinary rejects every upload. + // + // That comment was already here and it was right — but the test recomputed + // the same wrong set the handler used, so it passed while every real + // upload failed. max_file_size was in both sides of the comparison and in + // neither side of Cloudinary's. A test that mirrors the implementation + // cannot catch the implementation being wrong about a third party. + // + // The list below is now the contract with Cloudinary, not a copy of what + // the handler happens to do: folder, timestamp, upload_preset. Do not add + // a parameter here without confirming Cloudinary signs it. const res = await callAs<{ signature: string; timestamp: number; uploadPreset: string; folder: string; maxFileSize: number; cloudName: string; apiKey: string; @@ -306,7 +316,6 @@ describe("cloudinary upload signature", () => { const toSign = [ `folder=${res.folder}`, - `max_file_size=${res.maxFileSize}`, `timestamp=${res.timestamp}`, `upload_preset=${res.uploadPreset}`, ].join("&"); @@ -319,9 +328,11 @@ describe("cloudinary upload signature", () => { expect(res.apiKey).toBe(process.env.CLOUDINARY_API_KEY); }); - it("binds the size limit into the signature, per resource type", async () => { - // The limit is signed so a leaked signature cannot be replayed to upload - // something larger than allowed. + it("returns the advisory size limit and preset, per resource type", async () => { + // These are returned so the client can reject an oversized file before + // starting a doomed upload. They are NOT enforced by the signature — that + // was the belief that broke uploads. The enforceable ceiling, if there is + // one, is configured on the upload preset in the Cloudinary console. const image = await callAs<{ maxFileSize: number; uploadPreset: string }>( getCloudinaryUploadSignature, ALICE, { resourceType: "image" } ); diff --git a/functions/src/media.ts b/functions/src/media.ts index ce1805d..6a98b24 100644 --- a/functions/src/media.ts +++ b/functions/src/media.ts @@ -21,9 +21,23 @@ function signCloudinaryParams(params: Record, apiSecret: string) .digest("hex"); } -// Enforced on the signature itself so a leaked signature can't be reused to -// upload a bigger file than we allow. Matches the limits surfaced to the -// client so size checks stay in sync. +// Advisory only. These feed the size check in src/services/cloudinary.ts so a +// user gets a clear error before a doomed upload starts, and they are returned +// to the client for exactly that. +// +// They are NOT a server-side ceiling. The comment that used to sit here said +// they were "enforced on the signature itself", and that claim is what led to +// max_file_size being added to the signed parameter set — which broke every +// upload. Cloudinary signs only the upload parameters it recognises and +// silently drops the rest, so signing a fourth parameter it never verifies +// produced a signature that could not match. Its own error proved it: the +// String to sign Cloudinary echoed back listed folder, timestamp and +// upload_preset, and nothing else. +// +// The real server-side ceiling is whatever the petnote_image_signed and +// petnote_video_signed upload presets have configured in the Cloudinary +// console. If a hard limit matters, it has to be set there — it cannot be +// enforced from this file. const CLOUDINARY_MAX_IMAGE_BYTES = 10 * 1024 * 1024; // 10 MB const CLOUDINARY_MAX_VIDEO_BYTES = 80 * 1024 * 1024; // 80 MB @@ -76,9 +90,10 @@ export const getCloudinaryUploadSignature = onCall( const timestamp = Math.floor(Date.now() / 1000); const folder = userFolder(callerUid); const signature = signCloudinaryParams( + // Only parameters Cloudinary actually verifies. Adding one it ignores + // makes the signature unmatchable — see the note above. { folder, - max_file_size: String(maxFileSize), timestamp: String(timestamp), upload_preset: uploadPreset, }, diff --git a/src/services/cloudinary.ts b/src/services/cloudinary.ts index 5b74724..9873e55 100644 --- a/src/services/cloudinary.ts +++ b/src/services/cloudinary.ts @@ -69,9 +69,10 @@ async function uploadToCloudinary( formData.append("signature", data.signature); formData.append("upload_preset", data.uploadPreset); formData.append("folder", data.folder); - if (data.maxFileSize) { - formData.append("max_file_size", String(data.maxFileSize)); - } + // No max_file_size here. Cloudinary ignores it on upload, so sending it + // achieved nothing except tempting the server to sign it — which is what + // broke uploads. The size check above is where the limit is applied, and it + // is advisory; the enforceable ceiling lives in the upload preset. // 90s covers a max-size video on a sluggish uplink while still killing // pathological hangs that used to leave the upload spinner stuck