From ac04e27e34587f161fd04067e4147132bd40bc5a Mon Sep 17 00:00:00 2001 From: fabi Date: Wed, 12 Aug 2026 23:11:57 +0200 Subject: [PATCH] fix(upload): a retry after release returns the stored photo instead of refusing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The idempotency key was only readable as a multipart FIELD, and a field cannot be read until the body is being parsed — which happens after the lock/release pre-flight. So the replay was unreachable in exactly the case it exists for: the photo commits → the response is lost on the way back (the flaky-wifi failure the key was added for) → the host releases the gallery at the end of the night → the phone's retry answers `gallery_released`. The guest is told a photo that is sitting in the gallery was never sent. And the remedy the client offers is destructive: `open_event` clears `export_released_at` AND bumps `export_epoch`, retiring the whole keepsake generation and forcing a multi-GB rebuild on a 2-vCPU box at midnight — to re-send a photo that was never missing. Several guests on one flaky evening make this likely to happen at least once. The key is now also sent as `X-Client-Upload-Id`, which arrives with the request line, so the answer is knowable before anything is decided about locks. The multipart field stays for the concurrent case and as a fallback. Placed ahead of the hourly rate limiter too, which was the same mistake one layer up: a 40-photo burst with two retries apiece exhausted the guest's hour on uploads that had all committed the first time. The body is still drained rather than abandoned — replying before reading it makes the proxy see a broken pipe and turn a clean 200 into a 502. The spec carries its own control: a DIFFERENT photo is asserted to still be refused with `gallery_released` after the release, so the replay cannot be green merely because the gate was open. --- backend/src/handlers/upload.rs | 43 ++++++++++ e2e/helpers/upload-client.ts | 12 ++- .../02-upload/retry-after-release.spec.ts | 85 +++++++++++++++++++ frontend/src/lib/upload-queue.ts | 11 +++ 4 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 e2e/specs/02-upload/retry-after-release.spec.ts diff --git a/backend/src/handlers/upload.rs b/backend/src/handlers/upload.rs index 1728d0b..da77274 100644 --- a/backend/src/handlers/upload.rs +++ b/backend/src/handlers/upload.rs @@ -168,8 +168,51 @@ const ALLOWED_MEDIA: &[(&str, &str)] = &[ pub async fn upload( State(state): State, auth: AuthUser, + headers: axum::http::HeaderMap, mut multipart: Multipart, ) -> Result<(StatusCode, Json), AppError> { + // REPLAY FIRST — before the rate limit, before the ban check, before the lock/release gate. + // + // The idempotency key also arrives as a multipart FIELD, and there is a replay for it further + // down; but a field cannot be read until the body is being parsed, which is after every gate + // below. So the field's replay was unreachable in exactly the situation it matters most: + // + // the photo committed, the response was lost on the way back (the flaky-wifi failure this + // whole mechanism exists for), the host released the gallery at the end of the night, and + // the phone's retry then answered `gallery_released` — telling the guest a photo that is + // ALREADY IN THE GALLERY had not been sent. + // + // And the remedy that error suggests is destructive: `open_event` clears `export_released_at` + // and BUMPS `export_epoch`, retiring the whole keepsake generation and forcing a multi-GB + // rebuild on a 2-vCPU box at midnight — to re-send a photo that was never missing. + // + // A header arrives with the request line, so the answer is knowable before anything is + // decided. Charging the hourly rate limit for a retry of an already-stored photo was the same + // mistake one layer up: a 40-photo burst with two retries each exhausted the hour for uploads + // that committed the first time. + // + // The body is still DRAINED rather than abandoned — see `drain_multipart`: replying before + // reading the body makes the proxy see a broken pipe and turn a clean 200 into a 502. + if let Some(cid) = headers + .get("x-client-upload-id") + .and_then(|v| v.to_str().ok()) + .and_then(|v| Uuid::parse_str(v.trim()).ok()) + && let Some(existing) = + Upload::find_by_client_upload_id(&state.pool, auth.user_id, cid).await? + { + drain_multipart(multipart).await; + let uploader_name = User::find_by_id(&state.pool, auth.user_id) + .await? + .map(|u| u.display_name) + .unwrap_or_default(); + tracing::info!( + client_upload_id = %cid, upload_id = %existing.id, + "upload retry replayed from the header key, before the lock/release gate" + ); + let dto = replay_upload_dto(&state, &existing, &uploader_name).await; + return Ok((StatusCode::OK, Json(dto))); + } + // Rate limit: N uploads per hour per user. Gated by master + per-endpoint toggles. let rate_limits_on = config::get_bool(&state.config_cache, "rate_limits_enabled", true).await; let upload_rate_on = config::get_bool(&state.config_cache, "upload_rate_enabled", true).await; diff --git a/e2e/helpers/upload-client.ts b/e2e/helpers/upload-client.ts index 6fb535c..6d0ffa3 100644 --- a/e2e/helpers/upload-client.ts +++ b/e2e/helpers/upload-client.ts @@ -7,6 +7,7 @@ import { BASE } from './env'; * * Field shape matches [backend/src/handlers/upload.rs]: * - file (binary; carries filename + content_type in the part headers) + * - client_upload_id (uuid, optional; also sent as the X-Client-Upload-Id header) * - caption (text, optional) * - hashtags (CSV text, optional) */ @@ -17,6 +18,12 @@ export type UploadOptions = { contentType?: string; caption?: string; hashtags?: string; + /** + * Idempotency key. Sent BOTH as the `X-Client-Upload-Id` header and as the multipart field, + * exactly as the real client does — the header is what lets the server replay a stored upload + * before it evaluates the lock/release gate, and the field covers the concurrent case. + */ + clientUploadId?: string; }; export async function uploadRaw( @@ -29,9 +36,12 @@ export async function uploadRaw( form.append('file', blob as any, opts.filename ?? 'upload.bin'); if (opts.caption !== undefined) form.append('caption', opts.caption); if (opts.hashtags !== undefined) form.append('hashtags', opts.hashtags); + if (opts.clientUploadId !== undefined) form.append('client_upload_id', opts.clientUploadId); + const headers: Record = { Authorization: `Bearer ${token}` }; + if (opts.clientUploadId !== undefined) headers['X-Client-Upload-Id'] = opts.clientUploadId; return fetch(`${BASE}/api/v1/upload`, { method: 'POST', - headers: { Authorization: `Bearer ${token}` }, + headers, body: form as any, }); } diff --git a/e2e/specs/02-upload/retry-after-release.spec.ts b/e2e/specs/02-upload/retry-after-release.spec.ts new file mode 100644 index 0000000..a4d49ae --- /dev/null +++ b/e2e/specs/02-upload/retry-after-release.spec.ts @@ -0,0 +1,85 @@ +/** + * A retry of a photo that was ALREADY STORED must return that photo — even after the gallery has + * been released. + * + * The idempotency key arrives as a multipart field as well, and there is a replay for it in the + * handler; but a field cannot be read until the body is being parsed, which happens after the + * lock/release pre-flight. So the replay was unreachable in precisely the case that matters: + * + * the photo commits → the response is lost on the way back (the flaky-wifi failure the key + * exists for) → the host releases the gallery at the end of the night → the phone retries → + * `gallery_released`. + * + * The guest is then told a photo that is sitting in the gallery was never sent, and the remedy the + * client offers — ask the hosts to reopen — bumps `export_epoch`, retiring the whole keepsake and + * forcing a rebuild, to re-send something that was never missing. + * + * The key is now also sent as `X-Client-Upload-Id`, which arrives with the request line. + */ +import { test, expect } from '../../fixtures/test'; +import { uploadRaw } from '../../helpers/upload-client'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { BASE } from '../../helpers/env'; + +const SLUG = 'e2e-test-event'; + +function sample(): Buffer { + return readFileSync(join(process.cwd(), 'fixtures', 'media', 'sample.jpg')); +} + +test.describe('Upload — a retry after release replays instead of refusing', () => { + test('the stored photo comes back, and the guest is not told to reopen the gallery', async ({ + guest, + db, + }) => { + const g = await guest('WiederholerWilli'); + const key = crypto.randomUUID(); + + // 1. The upload commits. In the real failure the guest never sees this response. + const first = await uploadRaw(g.jwt, sample(), { + filename: 'a.jpg', + contentType: 'image/jpeg', + clientUploadId: key, + }); + expect(first.status).toBe(201); + const original = await first.json(); + + // 2. The host releases the gallery — the end-of-event action every queue runs into. + await db.setExportReleased(SLUG, true); + + // A DIFFERENT photo must still be refused: this is the control that proves the release is + // actually in effect, so the replay below is not just "the gate was open all along". + const stranger = await uploadRaw(g.jwt, sample(), { + filename: 'b.jpg', + contentType: 'image/jpeg', + clientUploadId: crypto.randomUUID(), + }); + expect(stranger.status, 'a genuinely new upload must still be refused after release').toBe(403); + expect((await stranger.json()).error).toBe('gallery_released'); + + // 3. The phone retries the FIRST photo. It is already in the gallery, so the honest answer is + // the stored row — not "the gallery is closed". + const retry = await uploadRaw(g.jwt, sample(), { + filename: 'a.jpg', + contentType: 'image/jpeg', + clientUploadId: key, + }); + expect( + retry.status, + 'a retry of an already-stored photo must be replayed, not refused with gallery_released' + ).toBe(200); + const replayed = await retry.json(); + expect(replayed.id, 'the replay must return the ORIGINAL upload, not a new one').toBe( + original.id + ); + + // 4. And no second row was created — the whole point of the key. + const feed = await fetch(`${BASE}/api/v1/feed?limit=100`, { + headers: { Authorization: `Bearer ${g.jwt}` }, + }); + const items: any[] = (await feed.json()).uploads ?? []; + const mine = items.filter((u) => u.id === original.id); + expect(mine.length, 'the photo must appear exactly once').toBe(1); + }); +}); diff --git a/frontend/src/lib/upload-queue.ts b/frontend/src/lib/upload-queue.ts index d8c66fc..4bd6b4f 100644 --- a/frontend/src/lib/upload-queue.ts +++ b/frontend/src/lib/upload-queue.ts @@ -1127,6 +1127,17 @@ async function uploadItem(id: string): Promise { const xhr = new XMLHttpRequest(); xhr.open('POST', '/api/v1/upload'); xhr.setRequestHeader('Authorization', `Bearer ${token}`); + // The SAME idempotency key as the multipart field above, in a header. + // + // The field alone cannot be read until the body is being parsed, which is after the + // release/lock pre-flight — so a retry of a photo that was already stored got + // `gallery_released` instead of its original row, and the guest was told a photo that + // IS in the gallery had not been sent. The only remedy on offer (ask the hosts to + // reopen) bumps the export epoch and destroys the released keepsake. + // + // A header arrives with the request line, so the server can replay before it decides + // anything about locks. The field stays for the concurrent case and as the fallback. + xhr.setRequestHeader('X-Client-Upload-Id', entry.id); // Wall-clock backstop only — generous enough that a slow-but-alive LTE upload is // never killed by it. See MIN/MAX_UPLOAD_TIMEOUT_MS. xhr.timeout = Math.min(