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