fix(upload): the truncation guard never fired -- it checked the wrong field
Some checks failed
Audit / cargo audit (backend) (push) Failing after 9m29s
Audit / npm audit (frontend) (push) Successful in 59s
Checks / Backend — cargo test + clippy + fmt (push) Failing after 59s
Checks / Frontend — vitest + svelte-check (push) Failing after 5m40s
Checks / Keepsake viewer — builds, self-contained, committed artifact in sync (push) Failing after 5m18s
Checks / E2E — typecheck + lint (push) Failing after 37s
E2E / Playwright E2E (chromium + webkit) (push) Failing after 8m55s
E2E / Cross-UA smoke matrix (push) Failing after 4m2s

v0.18.3 added a 400 branch to keep a guest's photo when the request body arrives
truncated, gated on `body.error !== 'bad_request'`. Its premise was that axum's
multipart rejection is a plain-text 400 with no envelope, so an envelope with
`bad_request` in it must be a considered verdict on the file.

That is false for the path that actually fails, and the branch was inert against
the exact incident it was written for. A guest on the live event lost a photo at
07:22; replaying the same truncation against the running backend at 07:50
produced a byte-identical log line:

  WARN request rejected status=400 code="bad_request"
       detail="Error parsing `multipart/form-data` request"

and this response body:

  400 application/json
  {"error":"bad_request","message":"Error parsing `multipart/form-data` request"}

A `bad_request` envelope. The guard evaluates false, the item still goes
terminal, and the blob is still purged.

The handler never reaches axum's extractor rejection: it pulls the fields itself
and wraps `MultipartError` in `AppError::BadRequest` -- bare from the field loop,
prefixed with "Datei konnte nicht gelesen werden: " from the chunk loop. Both
arrive indistinguishable BY CODE from "file too large". Axum's own plain-text
rejection does exist (no boundary in Content-Type -> "Invalid `boundary` ...")
but is a different error and not one a webview produces.

So the rule keys on the MESSAGE, and moves into an exported `isIncompleteBody`
beside `isReversibleLock`, matching how the other data-loss-critical rules in
this file are made testable. Tests transcribe the four responses captured from
the running backend, so reverting to an envelope check fails them.

Matching an upstream Display string is the weakness here and is called out in
the doc comment: an axum upgrade could reword it and silently re-open the data
loss. The durable fix is a distinct backend code (`body_incomplete`) this can
prefer once it exists -- deliberately not done now, because it means an app image
release and the backend has not needed one since v0.18.0.
This commit is contained in:
2026-08-22 08:13:40 +00:00
parent 2dd563b3ee
commit 2b57f1728e
2 changed files with 100 additions and 17 deletions

View File

@@ -1,6 +1,7 @@
import { describe, it, expect } from 'vitest'; import { describe, it, expect } from 'vitest';
import { import {
classifyUploadStatus, classifyUploadStatus,
isIncompleteBody,
isReversibleLock, isReversibleLock,
entryToQueueItem, entryToQueueItem,
shouldAbortForStall, shouldAbortForStall,
@@ -54,6 +55,59 @@ describe('classifyUploadStatus', () => {
* reopen and the photo resumes) or PURGES it (permanent ban / quota). Getting this wrong either * reopen and the photo resumes) or PURGES it (permanent ban / quota). Getting this wrong either
* loses a photo the guest expected to survive a reopen, or lets a banned device retry forever. * loses a photo the guest expected to survive a reopen, or lets a banned device retry forever.
*/ */
/**
* Regression guard for the data loss this was written for: an iPhone guest on the live event
* got "Error parsing `multipart/form-data` request", the item went terminal, and the ONLY copy
* of the photo was purged from IndexedDB with no retry offered.
*
* The first attempt at the fix keyed on the envelope (`body.error !== 'bad_request'`) and was
* inert, because the backend wraps the multipart error in its own `bad_request` envelope. These
* cases are transcribed from real responses captured against the running backend, so they fail
* if that reasoning is ever reverted.
*/
describe('isIncompleteBody', () => {
const parseError = 'Error parsing `multipart/form-data` request';
it('the exact live failure: bad_request envelope carrying the parse error → incomplete', () => {
expect(isIncompleteBody(400, { error: 'bad_request', message: parseError, status: 400 })).toBe(
true
);
});
it('the same error raised mid-file, with the German prefix → incomplete', () => {
expect(
isIncompleteBody(400, {
error: 'bad_request',
message: `Datei konnte nicht gelesen werden: ${parseError}`,
status: 400
})
).toBe(true);
});
it('an unparseable body (plain-text rejection, proxy, WAF) → incomplete', () => {
expect(isIncompleteBody(400, null)).toBe(true);
expect(isIncompleteBody(400, undefined)).toBe(true);
});
it('a real verdict on the file → NOT incomplete, so it still purges', () => {
expect(
isIncompleteBody(400, { error: 'bad_request', message: 'Datei ist zu groß. Maximum: 500 MB.' })
).toBe(false);
expect(
isIncompleteBody(400, { error: 'bad_request', message: 'Keine Datei hochgeladen.' })
).toBe(false);
expect(
isIncompleteBody(400, { error: 'bad_request', message: 'Dateityp wird nicht unterstützt.' })
).toBe(false);
});
it('only applies to 400 — other statuses keep their own rules', () => {
expect(isIncompleteBody(413, { error: 'quota_exceeded' })).toBe(false);
expect(isIncompleteBody(403, null)).toBe(false);
expect(isIncompleteBody(500, null)).toBe(false);
});
});
describe('isReversibleLock', () => { describe('isReversibleLock', () => {
it('an `uploads_locked` code is reversible at any status (event closed / released)', () => { it('an `uploads_locked` code is reversible at any status (event closed / released)', () => {
expect(isReversibleLock(403, 'uploads_locked')).toBe(true); expect(isReversibleLock(403, 'uploads_locked')).toBe(true);

View File

@@ -701,6 +701,48 @@ export function classifyUploadStatus(status: number): UploadOutcome {
return 'transient'; return 'transient';
} }
/**
* Within the `terminal` bucket, is this 400 a TRUNCATED REQUEST rather than a verdict on the
* file? Keep the blob and retry if so. Pure + exported for the same reason as
* `isReversibleLock`: it decides whether a guest keeps their photo.
*
* Keyed on the MESSAGE, not the envelope. The obvious rule — "an app-raised 400 carries
* `bad_request`, so a 400 without it is Axum's plain-text rejection" — does not hold, and was
* verified against the running backend rather than reasoned about:
*
* stream breaks between parts → 400 application/json
* {"error":"bad_request","message":"Error parsing `multipart/…"}
* stream breaks mid-file → 400 application/json, same code, message prefixed
* "Datei konnte nicht gelesen werden: …"
* no boundary in Content-Type → 400 text/plain "Invalid `boundary` for `multipart/…"
*
* Only the third is Axum's own extractor rejection. The first two — the ones a webview or a
* dropping mobile link actually produce — never reach it: the handler pulls the fields itself
* and wraps `MultipartError` in `AppError::BadRequest` (`upload.rs` field loop and chunk loop),
* so they arrive as an ordinary `bad_request` envelope, indistinguishable by code from "file too
* large" or "caption too long". An envelope check therefore never fires for the case this
* exists to catch. Confirmed live: the log line for a real guest failure and for a synthetic
* truncation are byte-identical.
*
* Retrying is safe: nothing was parsed, so nothing was stored and no quota was charged, and
* `X-Client-Upload-Id` makes a duplicate impossible even if the server did see it.
*
* The substring is Axum's `MultipartError` Display text and is therefore an UPSTREAM contract
* this file does not own — an axum upgrade could reword it and silently re-open the data loss.
* The durable fix is a distinct backend code (e.g. `body_incomplete`) that this can prefer once
* it exists; the match is kept as the fallback because it needs no app-image release.
*/
export function isIncompleteBody(status: number, body: unknown): boolean {
if (status !== 400) return false;
const envelope = body as { error?: unknown; message?: unknown } | null | undefined;
// An unparseable body (proxy, WAF, captive portal) cannot be a considered rejection either.
if (!envelope || envelope.error !== 'bad_request') return true;
return (
typeof envelope.message === 'string' &&
envelope.message.includes('Error parsing `multipart/form-data` request')
);
}
/** /**
* Within the `terminal` bucket, decide whether a 4xx is a REVERSIBLE lock (keep the blob, * Within the `terminal` bucket, decide whether a 4xx is a REVERSIBLE lock (keep the blob,
* park retryable for a host reopen) rather than a permanent rejection (purge the blob). * park retryable for a host reopen) rather than a permanent rejection (purge the blob).
@@ -1252,23 +1294,10 @@ async function uploadItem(id: string): Promise<void> {
); );
break; break;
case 'terminal': { case 'terminal': {
// A 400 the APP raised always carries `bad_request` in a JSON envelope // A truncated body is a transport failure, not a verdict on the file, so it
// (too large, wrong type, caption too long, NUL byte). Axum's own multipart // must not purge the blob. See `isIncompleteBody` for why the envelope alone
// rejection does not: it is a PLAIN-TEXT 400 ("Error parsing // cannot decide this.
// `multipart/form-data` request"), so `body` is null here. if (isIncompleteBody(xhr.status, body)) {
//
// That distinction decides whether a guest keeps their photo. An
// unparseable 400 means the request body never arrived intact — a transport
// failure, not a verdict on the file — and it is exactly what an iOS in-app
// browser (WhatsApp) produces when it truncates an XHR upload. Classified as
// terminal, it purged the blob from IndexedDB and offered no retry, so a
// webview hiccup destroyed the only copy the guest had.
//
// Same reasoning the 403 rule below already applies to an unparseable body,
// and safe to retry: nothing was parsed, so nothing was stored and no quota
// was charged — and `X-Client-Upload-Id` makes a duplicate impossible even
// if the server did see it.
if (xhr.status === 400 && body?.error !== 'bad_request') {
settle(() => settle(() =>
reject(new NetworkError('Übertragung unvollständig — bitte erneut versuchen')) reject(new NetworkError('Übertragung unvollständig — bitte erneut versuchen'))
); );