From ee05131add2030df42915b8bee29f943db77d272 Mon Sep 17 00:00:00 2001 From: Schalli Date: Thu, 8 Oct 2026 22:41:20 +0200 Subject: [PATCH] fix(nextcloud-files): WR-04/05/06, IN-01 Ersetzen und Abschluss von Uploads abgesichert - WR-04: die Entity-Tag-Pruefung beim Ersetzen laeuft im Zusammenbau unmittelbar vor dem MOVE (gemessen gegen Nextcloud 34: If-Match auf dem MOVE wird gegen .file geprueft und ergibt immer 412, ist also nicht nutzbar); weicht der Tag ab -> changedMeanwhile - WR-05: completeUpload merkt den Zustand synchron vor dem ersten await; ein zweiter oder gleichzeitiger Abschluss liefert das gemerkte Ergebnis (auch denselben Fehler) und baut nie zweimal zusammen; nur Fehler vor dem MOVE duerfen erneut versucht werden - WR-06: 412 mit replaceEtag (PUT oder Zusammenbau) ist changedMeanwhile, nicht nameTaken - IN-01: replaceEtag muss die Form eines Entity-Tags haben (sonst 400) Co-Authored-By: Claude Opus 5.5 (1M context) --- .../dto/nextcloud-files-transfer.dto.ts | 11 ++ .../nextcloud-files-transfer.service.spec.ts | 125 ++++++++++++++++++ .../nextcloud-files-transfer.service.ts | 122 ++++++++++++----- 3 files changed, 227 insertions(+), 31 deletions(-) diff --git a/apps/api/src/nextcloud-files/dto/nextcloud-files-transfer.dto.ts b/apps/api/src/nextcloud-files/dto/nextcloud-files-transfer.dto.ts index eb6e5c1..35ce2c8 100644 --- a/apps/api/src/nextcloud-files/dto/nextcloud-files-transfer.dto.ts +++ b/apps/api/src/nextcloud-files/dto/nextcloud-files-transfer.dto.ts @@ -8,6 +8,7 @@ import { IsNotEmpty, IsOptional, IsString, + Matches, Max, MaxLength, Min, @@ -26,6 +27,13 @@ import { /** Hoechster Unix-Zeitstempel (Sekunden), den eine Aenderungszeit haben darf (Jahr 2100). */ export const MAX_MTIME_SECONDS = 4102444800; +/** + * Form eines Entity-Tags (IN-01): optional `W/`, optional in Anfuehrungszeichen, sonst nur + * sichtbare ASCII-Zeichen. Der Wert geht als `If-Match`-Kopfzeile an Nextcloud; CR, LF oder + * andere Steuerzeichen liessen den Aufruf sonst scheitern und als "nicht erreichbar" enden. + */ +export const ETAG_PATTERN = /^(W\/)?"?[\x21\x23-\x7e]{1,128}"?$/; + export class StartUploadDto { @IsString() @IsNotEmpty() @@ -40,6 +48,7 @@ export class StartUploadDto { @IsOptional() @IsString() @MaxLength(200) + @Matches(ETAG_PATTERN) replaceEtag?: string; } @@ -62,6 +71,7 @@ export class CompleteUploadDto { @IsOptional() @IsString() @MaxLength(200) + @Matches(ETAG_PATTERN) replaceEtag?: string; } @@ -88,6 +98,7 @@ export class UploadQueryDto { @IsOptional() @IsString() @MaxLength(200) + @Matches(ETAG_PATTERN) replaceEtag?: string; } diff --git a/apps/api/src/nextcloud-files/nextcloud-files-transfer.service.spec.ts b/apps/api/src/nextcloud-files/nextcloud-files-transfer.service.spec.ts index 4aebf58..36b2b42 100644 --- a/apps/api/src/nextcloud-files/nextcloud-files-transfer.service.spec.ts +++ b/apps/api/src/nextcloud-files/nextcloud-files-transfer.service.spec.ts @@ -278,6 +278,24 @@ describe('NextcloudFilesTransferService — putSingle / putChunk', () => { expect(body).toMatchObject({ code: 'nameTaken', existing: { etag: '"e9"' } }); }); + it('putSingle mit replaceEtag: 412 -> 409 changedMeanwhile, nicht nameTaken (WR-06)', async () => { + const { service, calls } = setup((req) => + req.method === 'PUT' ? { status: 412 } : { status: 207, text: statXml('e10') }, + ); + const { status, body } = parts( + await caught( + service.putSingle('t1', 'u1', rawReq(5), { + path: '/Projekte/a.txt', + size: 5, + replaceEtag: '"e9"', + }), + ), + ); + expect(status).toBe(409); + expect(body).toMatchObject({ code: 'changedMeanwhile', existing: { etag: '"e10"' } }); + expect(calls[0].headers['if-match']).toBe('"e9"'); + }); + it('putChunk: Stueck 3 -> .../uploads/anna//00003 mit Destination und OC-Total-Length', async () => { const { service, calls } = setup(); const req = rawReq(CHUNK); @@ -562,6 +580,113 @@ describe('NextcloudFilesTransferService — Zusammenbau', () => { }); }); +describe('NextcloudFilesTransferService — Abschluss ist idempotent, Ersetzen geprueft (WR-04/05/06)', () => { + const dto = { path: '/Projekte/a.txt', size: 30000000 }; + + it('zwei gleichzeitige Abschluesse mit replaceEtag: ein PROPFIND, ein MOVE, beide ohne Fehler', async () => { + let releaseMove: (() => void) | undefined; + const { service, calls } = setup((req) => { + if (req.method === 'PROPFIND') return { status: 207, text: statXml('e1') }; + return new Promise((resolve) => { + releaseMove = () => resolve({ status: 204 }); + }); + }); + const first = service.completeUpload('t1', 'u1', UP, { ...dto, replaceEtag: '"e1"' }); + const second = service.completeUpload('t1', 'u1', UP, { ...dto, replaceEtag: '"e1"' }); + await new Promise((r) => setTimeout(r, 5)); + releaseMove?.(); + const results = await Promise.all([first, second]); + expect(results).toContainEqual({ state: 'done' }); + expect(results.every((r) => r.state === 'done' || r.state === 'assembling')).toBe(true); + expect(calls.map((c) => c.method)).toEqual(['PROPFIND', 'MOVE']); + expect(service.uploadState('t1', 'u1', UP)).toEqual({ state: 'done' }); + // Ein dritter Abschluss danach: das gemerkte Ergebnis, kein neuer Aufruf. + expect(await service.completeUpload('t1', 'u1', UP, { ...dto, replaceEtag: '"e1"' })).toEqual({ + state: 'done', + }); + expect(calls).toHaveLength(2); + }); + + it('kein If-Match auf dem MOVE (Nextcloud prueft es gegen .file); die Pruefung laeuft unmittelbar davor', async () => { + const { service, calls } = setup((req) => + req.method === 'PROPFIND' ? { status: 207, text: statXml('e1') } : { status: 204 }, + ); + await service.completeUpload('t1', 'u1', UP, { ...dto, replaceEtag: '"e1"' }); + const move = calls.find((c) => c.method === 'MOVE'); + expect(move?.headers['if-match']).toBeUndefined(); + expect(move?.headers.overwrite).toBe('T'); + }); + + it('ein Fehler nach dem MOVE wird bei einem erneuten Abschluss gleich gemeldet, ohne zweiten MOVE', async () => { + const { service, calls } = setup(() => ({ status: 507 })); + const e1 = parts(await caught(service.completeUpload('t1', 'u1', UP, dto))); + const e2 = parts(await caught(service.completeUpload('t1', 'u1', UP, dto))); + expect(e1).toMatchObject({ status: 507, body: { code: 'quotaExceeded' } }); + expect(e2).toEqual(e1); + expect(calls.filter((c) => c.method === 'MOVE')).toHaveLength(1); + }); + + it('ein Fehler VOR dem MOVE (Pruefung nicht erreichbar) darf mit einem neuen Abschluss wiederholt werden', async () => { + let down = true; + const { service, calls } = setup((req) => { + if (req.method === 'PROPFIND') { + if (down) throw Object.assign(new Error('x'), { code: 'ECONNRESET' }); + return { status: 207, text: statXml('e1') }; + } + return { status: 204 }; + }); + const e = parts( + await caught(service.completeUpload('t1', 'u1', UP, { ...dto, replaceEtag: '"e1"' })), + ); + expect(e.body.code).toBe('nextcloudUnavailable'); + down = false; + expect(await service.completeUpload('t1', 'u1', UP, { ...dto, replaceEtag: '"e1"' })).toEqual({ + state: 'done', + }); + expect(calls.filter((c) => c.method === 'MOVE')).toHaveLength(1); + }); + + it('MOVE 412 beim Ersetzen: changedMeanwhile statt nameTaken, Upload-Ordner geloescht', async () => { + const { service, calls } = setup((req) => { + if (req.method === 'MOVE') return { status: 412 }; + if (req.method === 'PROPFIND') return { status: 207, text: statXml('e1') }; + return { status: 204 }; + }); + const { status, body } = parts( + await caught(service.completeUpload('t1', 'u1', UP, { ...dto, replaceEtag: '"e1"' })), + ); + expect(status).toBe(409); + expect(body.code).toBe('changedMeanwhile'); + expect(calls.some((c) => c.method === 'DELETE')).toBe(true); + }); +}); + +describe('Eingaben: replaceEtag (IN-01)', () => { + it('nur sichtbare ASCII-Zeichen in der Form eines Entity-Tags', async () => { + const { plainToInstance } = await import('class-transformer'); + const { validate } = await import('class-validator'); + const { StartUploadDto, UploadQueryDto, CompleteUploadDto } = await import( + './dto/nextcloud-files-transfer.dto' + ); + const ok = ['"1645b02b08e573cfed4dda2749761446"', 'W/"abc"', 'abc123']; + const bad = ['"a\r\nX-Evil: 1"', 'a b', '"ä"', '', 'x'.repeat(140)]; + for (const etag of ok) { + const errs = await validate( + plainToInstance(StartUploadDto, { path: '/a.txt', size: 1, replaceEtag: etag }), + ); + expect(errs, etag).toHaveLength(0); + } + for (const etag of bad) { + for (const cls of [StartUploadDto, CompleteUploadDto, UploadQueryDto]) { + const errs = await validate( + plainToInstance(cls as never, { path: '/a.txt', size: 1, replaceEtag: etag }) as object, + ); + expect(errs.length, `${cls.name} ${JSON.stringify(etag)}`).toBeGreaterThan(0); + } + } + }); +}); + describe('NextcloudFilesTransferService — Herunterladen', () => { const FILE = { status: 200, diff --git a/apps/api/src/nextcloud-files/nextcloud-files-transfer.service.ts b/apps/api/src/nextcloud-files/nextcloud-files-transfer.service.ts index 7ef82d4..627c447 100644 --- a/apps/api/src/nextcloud-files/nextcloud-files-transfer.service.ts +++ b/apps/api/src/nextcloud-files/nextcloud-files-transfer.service.ts @@ -1,12 +1,12 @@ import { randomUUID } from 'node:crypto'; import type { Readable } from 'node:stream'; -import { Inject, Injectable } from '@nestjs/common'; +import { HttpException, Inject, Injectable } from '@nestjs/common'; import { NEXTCLOUD_FILES_CHUNK_SIZE, NEXTCLOUD_FILES_MAX_CHUNKS } from '@tessera/shared'; import type { Response } from 'express'; import { NextcloudCallGate } from './nextcloud-call-gate'; import * as dav from './nextcloud-dav'; import * as transfer from './nextcloud-dav-transfer'; -import { type NcSession, ncErrorDefault } from './nextcloud-files.types'; +import { type NcSession, ncError, ncErrorDefault } from './nextcloud-files.types'; import { NextcloudFilesAccountService } from './nextcloud-files-account.service'; import { type NcResult, @@ -92,6 +92,11 @@ interface AssemblyRecord { state: 'assembling' | 'done' | 'failed'; code?: string; message?: string; + /** HTTP-Status und Zusatzfelder des Fehlers, damit ein wiederholter Abschluss ihn gleich meldet. */ + status?: number; + extra?: Record; + /** Der Fehler kam, bevor der MOVE an Nextcloud ging: ein neuer Abschluss darf es erneut versuchen. */ + retryable?: boolean; finishedAt?: number; } @@ -302,9 +307,17 @@ export class NextcloudFilesTransferService { guard.settle(); } if (!NextcloudFilesTransferService.ok(result)) { - const existing = - result.ok && result.status === 412 ? await this.existingOf(session, segments) : undefined; - return this.fail(tenantId, userId, result, existing); + if (result.ok && result.status === 412) { + const existing = await this.existingOf(session, segments); + // WR-06: mit `replaceEtag` heisst 412 "die Version hat sich inzwischen geaendert" (If-Match + // passt nicht mehr), nicht "Name vergeben" — sonst fragte die Oberflaeche erneut nach + // "Ersetzen" und ersetzte dann eine Version, die der Benutzer nie gesehen hat. + if (query.replaceEtag) { + throw ncErrorDefault('changedMeanwhile', existing ? { existing } : undefined); + } + return this.fail(tenantId, userId, result, existing); + } + return this.fail(tenantId, userId, result); } return { path: pathOf(segments), size: length }; } @@ -370,6 +383,14 @@ export class NextcloudFilesTransferService { } } + /** + * Abschluss eines Uploads in Stuecken. IDEMPOTENT (WR-05): Der Zustand wird synchron, vor + * dem ersten `await`, unter Mandant, Benutzer und Upload-Kennung gemerkt. Ein zweiter oder + * gleichzeitiger Abschluss (Wiederholung nach einem Netzfehler, zweiter Tab) liefert das + * gemerkte Ergebnis — `assembling`, `done` oder denselben Fehler — und baut nie ein zweites + * Mal zusammen. Nur ein Fehler, der VOR dem MOVE entstand (Nextcloud hat also nichts + * veraendert), darf mit einem neuen Abschluss erneut versucht werden. + */ async completeUpload( tenantId: string, userId: string, @@ -382,50 +403,41 @@ export class NextcloudFilesTransferService { const mtime = NextcloudFilesTransferService.mtimeOf(dto.mtime); const session = await this.session(tenantId, userId); + // Ab hier kein `await` bis der Zustand gemerkt ist. this.prune(); const key = this.key(tenantId, userId, uploadId); const known = this.assemblies.get(key); - if (known?.state === 'assembling') return { state: 'assembling' }; - if (known?.state === 'done') return { state: 'done' }; - - // Ersetzen: unmittelbar vor dem Zusammenbau noch einmal pruefen, ob noch die Version - // da ist, die der Benutzer ersetzen wollte. - if (dto.replaceEtag) { - const probe = await dav.stat(this.transport, this.gate, session, segments); - if (!probe.ok) return this.fail(tenantId, userId, probe); - if (probe.status === 404 || (NextcloudFilesTransferService.ok(probe) && !probe.entry)) { - throw ncErrorDefault('changedMeanwhile'); - } - if (!NextcloudFilesTransferService.ok(probe)) return this.fail(tenantId, userId, probe); - if (probe.entry?.etag !== dto.replaceEtag) { - throw ncErrorDefault( - 'changedMeanwhile', - probe.entry ? { existing: viewOf(probe.entry) } : undefined, - ); - } - } + if (known && !(known.state === 'failed' && known.retryable)) return replay(known); const record: AssemblyRecord = { state: 'assembling' }; this.assemblies.set(key, record); + const progress = { moveSent: false }; // Der Zusammenbau gehoert nicht zur Anfrage: er laeuft weiter, auch wenn der Browser // aufgibt, und haelt sein Ergebnis in `record` fest. - const job = this.assemble(tenantId, userId, session, uploadId, segments, { + const job = this.assemble(tenantId, userId, session, uploadId, segments, progress, { totalLength: total, mtime, - replace: Boolean(dto.replaceEtag), + replaceEtag: dto.replaceEtag, }).then( () => { record.state = 'done'; record.finishedAt = Date.now(); }, (err: unknown) => { - const body = (err as { getResponse?: () => unknown }).getResponse?.() as - | { code?: string; message?: string } - | undefined; + const body = + err instanceof HttpException + ? (err.getResponse() as { code?: string; message?: string } & Record) + : undefined; record.state = 'failed'; record.code = body?.code ?? 'nextcloudError'; record.message = body?.message ?? ncErrorDefault('nextcloudError').message; + record.status = err instanceof HttpException ? err.getStatus() : 502; + if (body) { + const { code: _c, message: _m, ...extra } = body; + record.extra = extra; + } + record.retryable = !progress.moveSent; record.finishedAt = Date.now(); throw err; }, @@ -452,21 +464,56 @@ export class NextcloudFilesTransferService { session: NcSession, uploadId: string, segments: readonly string[], - opts: { totalLength: number; mtime?: number; replace: boolean }, + progress: { moveSent: boolean }, + opts: { totalLength: number; mtime?: number; replaceEtag?: string }, ): Promise { + const { replaceEtag } = opts; + if (replaceEtag) { + // Ersetzen (WR-04): unmittelbar vor dem MOVE pruefen, ob noch genau die Version da ist, + // die der Benutzer ersetzen wollte. Ein `If-Match` auf dem MOVE geht NICHT: Nextcloud + // wertet es beim Zusammenbau gegen die Quelle `.file` aus, nicht gegen das Ziel + // (gemessen gegen Nextcloud 34: jedes If-Match, auch das richtige, ergibt 412). Es bleibt + // nur das kurze Fenster zwischen dieser Pruefung und dem MOVE. + const probe = await dav.stat(this.transport, this.gate, session, segments); + if (!probe.ok) return this.fail(tenantId, userId, probe); + if (probe.status === 404 || (NextcloudFilesTransferService.ok(probe) && !probe.entry)) { + throw ncErrorDefault('changedMeanwhile'); + } + if (!NextcloudFilesTransferService.ok(probe)) return this.fail(tenantId, userId, probe); + if (probe.entry?.etag !== replaceEtag) { + throw ncErrorDefault( + 'changedMeanwhile', + probe.entry ? { existing: viewOf(probe.entry) } : undefined, + ); + } + } + + progress.moveSent = true; const result = await transfer.uploadAssemble( this.transport, this.gate, session, uploadId, segments, - opts, + { totalLength: opts.totalLength, mtime: opts.mtime, replace: Boolean(replaceEtag) }, ); + // Gesperrter Ursprung oder schon toter Zugang: der MOVE ging gar nicht erst raus. + if ( + !result.ok && + (result.kind === 'paused' || + (result.kind === 'credential-dead' && result.status === undefined)) + ) { + progress.moveSent = false; + } if (NextcloudFilesTransferService.ok(result)) return; if (result.ok && result.status === 412) { // Das Ziel ist inzwischen vergeben (gemessen: der Upload-Ordner BLEIBT) -> aufraeumen. const existing = await this.existingOf(session, segments); await transfer.uploadAbort(this.transport, this.gate, session, uploadId).catch(() => {}); + // WR-06: beim Ersetzen heisst 412 "inzwischen geaendert", nicht "Name vergeben". + if (replaceEtag) { + throw ncErrorDefault('changedMeanwhile', existing ? { existing } : undefined); + } return this.fail(tenantId, userId, result, existing); } return this.fail(tenantId, userId, result); @@ -591,3 +638,16 @@ export class NextcloudFilesTransferService { function viewOf(entry: NcEntry): ExistingView { return { etag: entry.etag, size: entry.size, mtime: entry.mtime }; } + +/** Gemerktes Ergebnis eines Abschlusses erneut melden (WR-05). */ +function replay(known: AssemblyRecord): { state: 'done' | 'assembling' } { + if (known.state === 'done') return { state: 'done' }; + if (known.state === 'assembling') return { state: 'assembling' }; + const code = (known.code ?? 'nextcloudError') as Parameters[0]; + throw ncError( + code, + known.status ?? 502, + known.message ?? ncErrorDefault('nextcloudError').message, + known.extra, + ); +}