fix: reject stale media confirmations

khoinguyenpham04 · khoinguyenpham04 · commit ecdf0b267055 · 2026-07-30T12:17:29.000+01:00
diff --git a/packages/core/src/api/openapi/document.ts b/packages/core/src/api/openapi/document.ts
@@ -828,7 +828,7 @@ function buildMediaPaths(maxUploadSize: number) {
 						},
 					},
 					...authErrors,
-					...standardErrors(400, 404, 500),
+					...standardErrors(400, 404, 409, 500),
 				},
 			},
 		},
diff --git a/packages/core/src/astro/routes/api/media/[id]/confirm.ts b/packages/core/src/astro/routes/api/media/[id]/confirm.ts
@@ -56,6 +56,21 @@ async function forgetUploadAttempt(repo: MediaRepository, storageKey: string): P
 	}
 }
 
+async function confirmationConflict(repo: MediaRepository, id: string): Promise<Response> {
+	const current = await repo.findById(id);
+	if (!current) {
+		return apiError("NOT_FOUND", `Media item not found: ${id}`, 404);
+	}
+	if (current.status === "ready") {
+		await forgetUploadAttempt(repo, current.storageKey);
+		return apiSuccess({ item: addUrlToMedia(current) });
+	}
+	if (current.status === "pending") {
+		return apiError("INVALID_STATE", "Media item changed during confirmation", 409);
+	}
+	return apiError("INVALID_STATE", `Media item is not pending: ${current.status}`, 400);
+}
+
 async function consumeDownload(download: DownloadResult): Promise<Uint8Array> {
 	const reader = download.body.getReader();
 	try {
@@ -148,8 +163,8 @@ export const POST: APIRoute = async ({ params, request, locals }) => {
 		if (emdash.storage) {
 			const exists = await emdash.storage.exists(existing.storageKey);
 			if (!exists) {
-				// Mark as failed
-				await repo.markFailed(id);
+				const failed = await repo.markFailed(id, existing.storageKey);
+				if (!failed) return await confirmationConflict(repo, id);
 				return apiError("FILE_NOT_FOUND", "File was not uploaded to storage", 400);
 			}
 
@@ -207,28 +222,21 @@ export const POST: APIRoute = async ({ params, request, locals }) => {
 		}
 
 		// Confirm the upload
-		const item = await repo.confirmUpload(id, {
-			size: confirmedSize,
-			width,
-			height,
-			blurhash,
-			dominantColor,
-			contentHash,
-		});
+		const item = await repo.confirmUpload(
+			id,
+			{
+				size: confirmedSize,
+				width,
+				height,
+				blurhash,
+				dominantColor,
+				contentHash,
+			},
+			existing.storageKey,
+		);
 
 		if (!item) {
-			const current = await repo.findById(id);
-			if (!current) {
-				return apiError("NOT_FOUND", `Media item not found: ${id}`, 404);
-			}
-			if (current.status === "ready") {
-				await forgetUploadAttempt(repo, current.storageKey);
-				return apiSuccess({ item: addUrlToMedia(current) });
-			}
-			if (current.status !== "pending") {
-				return apiError("INVALID_STATE", `Media item is not pending: ${current.status}`, 400);
-			}
-			return apiError("CONFIRM_FAILED", "Failed to confirm upload", 500);
+			return await confirmationConflict(repo, id);
 		}
 
 		await forgetUploadAttempt(repo, item.storageKey);
diff --git a/packages/core/src/database/repositories/media.ts b/packages/core/src/database/repositories/media.ts
@@ -277,6 +277,7 @@ export class MediaRepository {
 			dominantColor?: string;
 			contentHash?: string | null;
 		},
+		expectedStorageKey?: string,
 	): Promise<MediaItem | null> {
 		const updates: Partial<MediaRow> = {
 			status: "ready",
@@ -288,29 +289,31 @@ export class MediaRepository {
 		if (metadata?.dominantColor !== undefined) updates.dominant_color = metadata.dominantColor;
 		if (metadata?.contentHash !== undefined) updates.content_hash = metadata.contentHash;
 
-		const row = await this.db
+		let query = this.db
 			.updateTable("media")
 			.set(updates)
 			.where("id", "=", id)
-			.where("status", "=", "pending")
-			.returningAll()
-			.executeTakeFirst();
+			.where("status", "=", "pending");
+		if (expectedStorageKey !== undefined) {
+			query = query.where("storage_key", "=", expectedStorageKey);
+		}
+
+		const row = await query.returningAll().executeTakeFirst();
 
 		return row ? this.rowToItem(row) : null;
 	}
 
 	/**
 	 * Mark upload as failed
 	 */
-	async markFailed(id: string): Promise<MediaItem | null> {
-		const existing = await this.findById(id);
-		if (!existing) {
-			return null;
+	async markFailed(id: string, expectedStorageKey?: string): Promise<MediaItem | null> {
+		let query = this.db.updateTable("media").set({ status: "failed" }).where("id", "=", id);
+		if (expectedStorageKey !== undefined) {
+			query = query.where("status", "=", "pending").where("storage_key", "=", expectedStorageKey);
 		}
 
-		await this.db.updateTable("media").set({ status: "failed" }).where("id", "=", id).execute();
-
-		return this.findById(id);
+		const row = await query.returningAll().executeTakeFirst();
+		return row ? this.rowToItem(row) : null;
 	}
 
 	/**
diff --git a/packages/core/tests/integration/astro/media-stream-upload.test.ts b/packages/core/tests/integration/astro/media-stream-upload.test.ts
@@ -172,8 +172,7 @@ describe("streamed media upload fallback", () => {
 
 	it("accepts existing client hash formats for deduplication", async () => {
 		const repo = new MediaRepository(db);
-		const contentHash =
-			"sha256:ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad";
+		const contentHash = "sha256:ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad";
 		const existing = await repo.create({
 			filename: "photo.png",
 			mimeType: "image/png",
@@ -643,6 +642,154 @@ describe("streamed media upload fallback", () => {
 		expect(storage.delete).toHaveBeenCalledOnce();
 	});
 
+	it("does not confirm stale metadata after a replacement upload", async () => {
+		const repo = new MediaRepository(db);
+		const pending = await repo.createPending({
+			filename: "document.pdf",
+			mimeType: "application/pdf",
+			size: 3,
+			storageKey: "document.pdf",
+			authorId: "user-1",
+		});
+		const storage = streamingStorage();
+		const firstBytes = new Uint8Array([1, 2, 3]);
+		const replacementBytes = new Uint8Array([4, 5, 6]);
+
+		await putUpload(
+			buildContext({
+				db,
+				id: pending.id,
+				request: uploadRequest(pending.id, firstBytes, "application/pdf"),
+				storage,
+			}),
+		);
+
+		let markConfirmationReading: () => void = () => undefined;
+		const confirmationReading = new Promise<void>((resolve) => {
+			markConfirmationReading = resolve;
+		});
+		let finishConfirmation: () => void = () => undefined;
+		const confirmationCanFinish = new Promise<void>((resolve) => {
+			finishConfirmation = resolve;
+		});
+		storage.download.mockImplementationOnce(async (key: string) => {
+			const bytes = storage.objects.get(key);
+			if (!bytes) throw new EmDashStorageError("File not found", "NOT_FOUND");
+			return {
+				body: new ReadableStream<Uint8Array>({
+					cancel() {
+						markConfirmationReading();
+						return confirmationCanFinish;
+					},
+				}),
+				contentType: "application/pdf",
+				size: bytes.byteLength,
+			};
+		});
+
+		const confirm = () =>
+			postConfirm(
+				buildContext({
+					db,
+					id: pending.id,
+					request: new Request(`http://localhost/_emdash/api/media/${pending.id}/confirm`, {
+						method: "POST",
+						headers: { "Content-Type": "application/json", "X-EmDash-Request": "1" },
+						body: "{}",
+					}),
+					storage,
+				}),
+			);
+		const staleConfirmation = confirm();
+		await confirmationReading;
+
+		await putUpload(
+			buildContext({
+				db,
+				id: pending.id,
+				request: uploadRequest(pending.id, replacementBytes, "application/pdf"),
+				storage,
+			}),
+		);
+		finishConfirmation();
+
+		expect((await staleConfirmation).status).toBe(409);
+		const replacement = await repo.findById(pending.id);
+		expect(replacement).toMatchObject({
+			status: "pending",
+			contentHash: await computeContentHash(replacementBytes),
+		});
+		expect(storage.objects.get(replacement!.storageKey)).toEqual(replacementBytes);
+
+		expect((await confirm()).status).toBe(200);
+		expect(await repo.findById(pending.id)).toMatchObject({
+			status: "ready",
+			contentHash: await computeContentHash(replacementBytes),
+		});
+	});
+
+	it("does not mark a replacement upload as failed from a stale storage check", async () => {
+		const repo = new MediaRepository(db);
+		const pending = await repo.createPending({
+			filename: "document.pdf",
+			mimeType: "application/pdf",
+			size: 3,
+			storageKey: "document.pdf",
+			authorId: "user-1",
+		});
+		const storage = streamingStorage();
+		const replacementBytes = new Uint8Array([4, 5, 6]);
+
+		let markStorageChecked: () => void = () => undefined;
+		const storageChecked = new Promise<void>((resolve) => {
+			markStorageChecked = resolve;
+		});
+		let finishStorageCheck: () => void = () => undefined;
+		const storageCheckCanFinish = new Promise<void>((resolve) => {
+			finishStorageCheck = resolve;
+		});
+		storage.exists.mockImplementationOnce(async () => {
+			markStorageChecked();
+			await storageCheckCanFinish;
+			return false;
+		});
+
+		const confirm = () =>
+			postConfirm(
+				buildContext({
+					db,
+					id: pending.id,
+					request: new Request(`http://localhost/_emdash/api/media/${pending.id}/confirm`, {
+						method: "POST",
+						headers: { "Content-Type": "application/json", "X-EmDash-Request": "1" },
+						body: "{}",
+					}),
+					storage,
+				}),
+			);
+		const staleConfirmation = confirm();
+		await storageChecked;
+
+		await putUpload(
+			buildContext({
+				db,
+				id: pending.id,
+				request: uploadRequest(pending.id, replacementBytes, "application/pdf"),
+				storage,
+			}),
+		);
+		finishStorageCheck();
+
+		expect((await staleConfirmation).status).toBe(409);
+		expect(await repo.findById(pending.id)).toMatchObject({
+			status: "pending",
+			contentHash: await computeContentHash(replacementBytes),
+		});
+
+		expect((await confirm()).status).toBe(200);
+		expect(await repo.findById(pending.id)).toMatchObject({ status: "ready" });
+	});
+
 	it("publishes only one object when two uploads race", async () => {
 		const repo = new MediaRepository(db);
 		const pending = await repo.createPending({
diff --git a/packages/core/tests/integration/database/media-upload-publish.test.ts b/packages/core/tests/integration/database/media-upload-publish.test.ts
@@ -55,6 +55,32 @@ describeEachDialect("pending media upload publication", (dialect) => {
 		expect((await repo.findById(pending.id))?.storageKey).toMatch(/^attempt-[ab]\.png$/);
 	});
 
+	it("does not apply stale confirmation state after the storage key changes", async () => {
+		const pending = await repo.createPending({
+			filename: "photo.png",
+			mimeType: "image/png",
+			size: 3,
+			storageKey: "pending.png",
+		});
+		await repo.createUploadAttempt(pending.id, "replacement.png");
+		await repo.publishPendingStorageKey(
+			pending.id,
+			pending.storageKey,
+			"replacement.png",
+			"sha1:589c22335a381f122d129225f5c0ba3056ed5811",
+		);
+
+		await expect(
+			repo.confirmUpload(pending.id, { width: 100 }, pending.storageKey),
+		).resolves.toBeNull();
+		await expect(repo.markFailed(pending.id, pending.storageKey)).resolves.toBeNull();
+		expect(await repo.findById(pending.id)).toMatchObject({
+			status: "pending",
+			storageKey: "replacement.png",
+			width: null,
+		});
+	});
+
 	it("does not claim a fresh active attempt for cleanup", async () => {
 		const pending = await repo.createPending({
 			filename: "photo.png",
diff --git a/packages/core/tests/unit/api/openapi.test.ts b/packages/core/tests/unit/api/openapi.test.ts
@@ -37,6 +37,7 @@ describe("OpenAPI document generation", () => {
 		expect(paths).toContain("/_emdash/api/media/{id}/confirm");
 		expect(paths).toContain("/_emdash/api/media/{id}/upload");
 		expect(paths).toContain("/_emdash/api/admin/media-usage/repair");
+		expect(doc.paths?.["/_emdash/api/media/{id}/confirm"]?.post?.responses).toHaveProperty("409");
 	});
 
 	it("documents media usage summary opt-in parameters and read responses", () => {
