From 5c2d327bef8b50ed50424234236c90b587a6a72b Mon Sep 17 00:00:00 2001 From: Schalli Date: Thu, 8 Oct 2026 22:27:40 +0200 Subject: [PATCH] fix(nextcloud-files): CR-01 Anmeldebremse reserviert Versuche vor dem Nextcloud-Aufruf - beginPasswordAttempt zaehlt einen Passwortversuch synchron und vor jedem await in Benutzer- und Servergrenze; release gibt den Platz bei Erfolg oder Nicht-Fehlversuch frei - gleichzeitige Fehlversuche kommen nicht mehr an den Grenzen vorbei (3 je Benutzer, 8 je Server) - Verbinden und Speichern laufen je Benutzer nacheinander (auch Browser-Anmeldung), damit zwei gleichzeitige Erfolge kein ungespeichertes, nie widerrufenes App-Passwort hinterlassen - Specs mit gleichzeitigen Promise.all-Anmeldungen Co-Authored-By: Claude Opus 5.5 (1M context) --- .../nextcloud-files-account.service.spec.ts | 87 +++++++++++++ .../nextcloud-files-account.service.ts | 122 ++++++++++++------ .../nextcloud-login-guard.spec.ts | 47 +++++++ .../nextcloud-files/nextcloud-login-guard.ts | 92 +++++++++++-- 4 files changed, 301 insertions(+), 47 deletions(-) diff --git a/apps/api/src/nextcloud-files/nextcloud-files-account.service.spec.ts b/apps/api/src/nextcloud-files/nextcloud-files-account.service.spec.ts index 50dba1b..94c1253 100644 --- a/apps/api/src/nextcloud-files/nextcloud-files-account.service.spec.ts +++ b/apps/api/src/nextcloud-files/nextcloud-files-account.service.spec.ts @@ -258,6 +258,93 @@ describe('Verbinden mit Passwort', () => { }); }); +describe('Gleichzeitige Anmeldungen (CR-01)', () => { + /** Haelt alle Nextcloud-Antworten an, bis `open()` gerufen wird. */ + function gated(handler: (req: NcTransportRequest) => NcTransportResponse) { + let open!: () => void; + const opened = new Promise((resolve) => { + open = resolve; + }); + const wrapped = (async (req: NcTransportRequest) => { + await opened; + return handler(req); + }) as unknown as (req: NcTransportRequest) => NcTransportResponse; + return { wrapped, open }; + } + + it('fuenf gleichzeitige Fehlversuche desselben Benutzers: nur drei erreichen Nextcloud, zwei sind sofort 429', async () => { + const g = gated(() => reply(401)); + const s = setup({ handler: g.wrapped }); + const all = Promise.all( + [0, 1, 2, 3, 4].map(() => errOf(s.service.connectWithPassword('t1', 'u1', 'zoe', 'x'))), + ); + await new Promise((r) => setTimeout(r, 5)); + g.open(); + const results = await all; + expect(results.filter((r) => r?.status === 429)).toHaveLength(2); + expect(results.filter((r) => r?.body.code === 'credentialsOrTwoFactor')).toHaveLength(3); + expect(s.calls.filter((c) => c.url.endsWith('/core/getapppassword'))).toHaveLength(3); + // Danach bleibt der Benutzer gesperrt, ohne dass Nextcloud gefragt wird. + const after = await errOf(s.service.connectWithPassword('t1', 'u1', 'zoe', 'x')); + expect(after?.status).toBe(429); + expect(s.calls).toHaveLength(3); + }); + + it('zehn gleichzeitige Fehlversuche verschiedener Benutzer: hoechstens acht erreichen Nextcloud', async () => { + const g = gated(() => reply(401)); + const s = setup({ handler: g.wrapped }); + const all = Promise.all( + Array.from({ length: 10 }, (_, i) => + errOf(s.service.connectWithPassword('t1', `u${i}`, 'zoe', 'x')), + ), + ); + await new Promise((r) => setTimeout(r, 5)); + g.open(); + const results = await all; + expect(s.calls).toHaveLength(8); + expect(results.filter((r) => r?.status === 429)).toHaveLength(2); + }); + + it('ein gelungener Versuch gibt seinen Platz frei', async () => { + const s = setup({ handler: happy }); + for (let i = 0; i < 5; i++) await s.service.connectWithPassword('t1', 'u1', 'anna', 'geheim'); + expect(s.calls.filter((c) => c.url.endsWith('/core/getapppassword'))).toHaveLength(5); + }); + + it('zwei gleichzeitige erfolgreiche Anmeldungen: eine Zeile, das zuerst ausgestellte App-Passwort ist widerrufen', async () => { + let issued = 0; + const g = gated((req) => { + if (req.url.endsWith('/core/getapppassword')) { + issued += 1; + return reply(200, ocs({ apppassword: `app-pw-${issued}` })); + } + return happy(req); + }); + const s = setup({ handler: g.wrapped }); + const both = Promise.all([ + s.service.connectWithPassword('t1', 'u1', 'anna', 'geheim'), + s.service.connectWithPassword('t1', 'u1', 'anna', 'geheim'), + ]); + await new Promise((r) => setTimeout(r, 5)); + g.open(); + await both; + expect(issued).toBe(2); + expect(s.rows).toHaveLength(1); + const stored = s.rows[0].encryptedAppPassword; + const deletes = s.calls.filter((c) => c.method === 'DELETE'); + // Genau ein Widerruf: das Passwort, das NICHT gespeichert ist. + expect(deletes).toHaveLength(1); + const revokedSecret = Buffer.from( + deletes[0].headers.authorization.replace('Basic ', ''), + 'base64', + ) + .toString('utf8') + .split(':')[1]; + expect(stored).toBe('enc(app-pw-2)'); + expect(revokedSecret).toBe('app-pw-1'); + }); +}); + describe('App-Passwort-Hygiene (D-P)', () => { it('cloud/user scheitert nach erfolgreichem getapppassword: genau ein Widerruf, kein Upsert', async () => { const s = setup({ diff --git a/apps/api/src/nextcloud-files/nextcloud-files-account.service.ts b/apps/api/src/nextcloud-files/nextcloud-files-account.service.ts index f58f854..e782899 100644 --- a/apps/api/src/nextcloud-files/nextcloud-files-account.service.ts +++ b/apps/api/src/nextcloud-files/nextcloud-files-account.service.ts @@ -70,6 +70,8 @@ export function credentialKeyOf(encryptedAppPassword: string): string { @Injectable() export class NextcloudFilesAccountService { private readonly logger = new Logger(NextcloudFilesAccountService.name); + /** Ende der Warteschlange je Mandant und Benutzer (siehe `withUserLock`). */ + private readonly userLocks = new Map>(); constructor( private readonly prisma: PrismaService, @@ -159,38 +161,83 @@ export class NextcloudFilesAccountService { ): Promise { const baseUrl = await this.requireBaseUrl(tenantId); const scope = new URL(baseUrl).origin; - this.guard.checkPasswordAttempt(userId, scope); + // CR-01: den Versuch SYNCHRON reservieren, bevor irgendetwas wartet. Gleichzeitige + // Anfragen sehen so die laufenden Versuche und bekommen 429, statt alle an Nextcloud zu gehen. + const attempt = this.guard.beginPasswordAttempt(userId, scope); + try { + return await this.withUserLock(tenantId, userId, async () => { + const issued = await getAppPassword( + this.transport, + this.gate, + baseUrl, + loginName, + password, + ); + if (!issued.ok) { + // 401 ist doppeldeutig (falsches Passwort oder Zwei-Faktor) und zaehlt bei Nextcloud als + // Fehlanmeldung; bei einer Zeitueberschreitung kann Nextcloud ihn schon gezaehlt haben. + if (issued.kind === 'credentials' || issued.kind === 'timeout') attempt.fail(); + throw authFailureToException(issued); + } + attempt.release(); - const issued = await getAppPassword(this.transport, this.gate, baseUrl, loginName, password); - if (!issued.ok) { - // 401 ist doppeldeutig (falsches Passwort oder Zwei-Faktor) und zaehlt bei Nextcloud als Fehlanmeldung. - if (issued.kind === 'credentials') this.guard.recordFailure(userId, scope); - throw authFailureToException(issued); + const ncUser = await getCurrentUser( + this.transport, + this.gate, + baseUrl, + loginName, + issued.appPassword, + ); + if (!ncUser.ok) { + await this.revokeFresh(baseUrl, loginName, issued.appPassword); + throw authFailureToException(unexpectedCredentials(ncUser)); + } + + await this.storeAppPassword( + tenantId, + userId, + baseUrl, + loginName, + ncUser, + issued.appPassword, + 'PASSWORD', + ); + this.guard.recordSuccess(userId); + return this.getStatus(tenantId, userId); + }); + } finally { + // Jeder andere Ausgang (403, Netzfehler, gesperrt ...) ist kein Fehlversuch; nach `fail` wirkungslos. + attempt.release(); } + } - const ncUser = await getCurrentUser( - this.transport, - this.gate, - baseUrl, - loginName, - issued.appPassword, - ); - if (!ncUser.ok) { - await this.revokeFresh(baseUrl, loginName, issued.appPassword); - throw authFailureToException(unexpectedCredentials(ncUser)); + /** + * Verbindungsvorgaenge desselben Benutzers laufen nacheinander (CR-01). Ohne das + * koennten zwei gleichzeitig erfolgreiche Anmeldungen beide dieselbe alte Zeile + * widerrufen und dann nacheinander speichern — das zuerst gespeicherte frische + * App-Passwort waere ueberschrieben, nirgends abgelegt und nie widerrufen. So + * widerruft der zweite Vorgang das Passwort des ersten ganz regulaer als "altes". + */ + private async withUserLock( + tenantId: string, + userId: string, + fn: () => Promise, + ): Promise { + const key = `${tenantId}:${userId}`; + const previous = this.userLocks.get(key) ?? Promise.resolve(); + let unlock!: () => void; + const mine = new Promise((resolve) => { + unlock = resolve; + }); + const tail = previous.then(() => mine); + this.userLocks.set(key, tail); + try { + await previous; + return await fn(); + } finally { + unlock(); + if (this.userLocks.get(key) === tail) this.userLocks.delete(key); } - - await this.storeAppPassword( - tenantId, - userId, - baseUrl, - loginName, - ncUser, - issued.appPassword, - 'PASSWORD', - ); - this.guard.recordSuccess(userId); - return this.getStatus(tenantId, userId); } // --- Verbinden im Browser (Login Flow v2) ------------------------------------------------------ @@ -256,14 +303,17 @@ export class NextcloudFilesAccountService { return this.failed(authFailureToException(unexpectedCredentials(ncUser))); } try { - await this.storeAppPassword( - tenantId, - userId, - entry.baseUrl, - loginName, - ncUser, - appPassword, - 'LOGIN_FLOW', + // Dieselbe Warteschlange wie die Passwort-Anmeldung (CR-01): nie zwei Speichervorgaenge zugleich. + await this.withUserLock(tenantId, userId, () => + this.storeAppPassword( + tenantId, + userId, + entry.baseUrl, + loginName, + ncUser, + appPassword, + 'LOGIN_FLOW', + ), ); } catch (err) { return this.failed(err); diff --git a/apps/api/src/nextcloud-files/nextcloud-login-guard.spec.ts b/apps/api/src/nextcloud-files/nextcloud-login-guard.spec.ts index a4c611d..02409da 100644 --- a/apps/api/src/nextcloud-files/nextcloud-login-guard.spec.ts +++ b/apps/api/src/nextcloud-files/nextcloud-login-guard.spec.ts @@ -82,6 +82,53 @@ describe('NextcloudLoginGuard — Passwort-Fehlversuche', () => { }); }); +describe('NextcloudLoginGuard — Reservierung (CR-01)', () => { + it('laufende Versuche zaehlen sofort: der 4. gleichzeitige Versuch von u1 ist 429', () => { + const { guard } = makeGuard(); + const running = [0, 1, 2].map(() => guard.beginPasswordAttempt('u1')); + expect(running).toHaveLength(3); + const blocked = codeOf(() => guard.beginPasswordAttempt('u1')); + expect(blocked.status).toBe(429); + expect(blocked.body.code).toBe('tooManyAttempts'); + }); + + it('release gibt den Platz frei, fail behaelt ihn', () => { + const { guard } = makeGuard(); + const a = guard.beginPasswordAttempt('u1'); + const b = guard.beginPasswordAttempt('u1'); + const c = guard.beginPasswordAttempt('u1'); + a.release(); + expect(codeOf(() => guard.checkPasswordAttempt('u1')).status).toBeUndefined(); + b.fail(); + c.fail(); + // fail nach release (und umgekehrt) aendert nichts mehr + a.fail(); + b.release(); + const d = guard.beginPasswordAttempt('u1'); + expect(codeOf(() => guard.checkPasswordAttempt('u1')).status).toBe(429); + d.release(); + expect(codeOf(() => guard.checkPasswordAttempt('u1')).status).toBeUndefined(); + }); + + it('die Servergrenze gilt auch fuer gleichzeitige Versuche verschiedener Benutzer', () => { + const { guard } = makeGuard(); + for (let i = 0; i < 8; i++) guard.beginPasswordAttempt(`u${i}`, 'http://a.example'); + expect(codeOf(() => guard.beginPasswordAttempt('u9', 'http://a.example')).status).toBe(429); + }); + + it('recordSuccess laesst laufende Versuche desselben Benutzers stehen', () => { + const { guard } = makeGuard(); + guard.recordFailure('u1'); + guard.recordFailure('u1'); + guard.beginPasswordAttempt('u1'); + guard.recordSuccess('u1'); + // eine abgeschlossene Fehlanmeldung weg, der laufende Versuch zaehlt noch: 1 von 3 + guard.beginPasswordAttempt('u1'); + guard.beginPasswordAttempt('u1'); + expect(codeOf(() => guard.checkPasswordAttempt('u1')).status).toBe(429); + }); +}); + describe('NextcloudLoginGuard — Start der Browser-Anmeldung', () => { it('der 11. Start eines Benutzers binnen 10 Minuten ist 429, andere Benutzer nicht', () => { const { guard, clock } = makeGuard(); diff --git a/apps/api/src/nextcloud-files/nextcloud-login-guard.ts b/apps/api/src/nextcloud-files/nextcloud-login-guard.ts index b9cbcae..7de56ef 100644 --- a/apps/api/src/nextcloud-files/nextcloud-login-guard.ts +++ b/apps/api/src/nextcloud-files/nextcloud-login-guard.ts @@ -20,6 +20,15 @@ import { ncErrorDefault } from './nextcloud-files.types'; * Der Login Flow v2 zaehlt bei der Nextcloud nicht als Fehlanmeldung (gemessen), * bekommt deshalb nur eine eigene Startgrenze (10 je Benutzer in 10 Minuten) * und beruehrt die Fehlerzaehler nie. + * + * RESERVIEREN STATT NACHTRAGEN (CR-01): Ein Passwortversuch wird mit + * `beginPasswordAttempt` SYNCHRON und VOR dem Nextcloud-Aufruf gezaehlt — er + * belegt sofort einen Platz in der Benutzer- und in der Servergrenze. Erst + * wenn klar ist, dass Nextcloud den Versuch nicht als Fehlanmeldung zaehlt + * (Erfolg, 403, Netzfehler vor der Antwort ...), wird der Platz mit `release` + * zurueckgegeben. Wuerde erst nach der Antwort gezaehlt, kaemen beliebig viele + * gleichzeitige Anfragen an beiden Grenzen vorbei, solange keine davon + * zurueck ist. */ export const USER_FAILURE_LIMIT = 3; @@ -29,10 +38,34 @@ export const SERVER_FAILURE_WINDOW_MS = 30 * 60 * 1000; export const FLOW_START_LIMIT = 10; export const FLOW_START_WINDOW_MS = 10 * 60 * 1000; -function prune(list: number[], now: number, windowMs: number): number[] { +/** Ein gezaehlter Versuch; `inFlight` = Nextcloud hat noch nicht geantwortet. */ +interface Attempt { + at: number; + inFlight: boolean; +} + +/** Ein reservierter Passwortversuch (siehe `beginPasswordAttempt`). */ +export interface PasswordAttempt { + /** Nextcloud hat den Versuch als Fehlanmeldung gezaehlt: der Platz bleibt belegt. */ + fail(): void; + /** Kein Fehlversuch (Erfolg oder Abbruch vor der Pruefung): der Platz wird frei. */ + release(): void; +} + +function prune(list: Attempt[], now: number, windowMs: number): Attempt[] { + return list.filter((a) => now - a.at < windowMs); +} + +function pruneTimes(list: number[], now: number, windowMs: number): number[] { return list.filter((t) => now - t < windowMs); } +function removeAttempt(list: Attempt[] | undefined, attempt: Attempt): void { + if (!list) return; + const i = list.indexOf(attempt); + if (i >= 0) list.splice(i, 1); +} + function tooMany(retryAfterMs: number) { return ncErrorDefault('tooManyAttempts', { retryAfterSeconds: Math.max(1, Math.ceil(retryAfterMs / 1000)), @@ -44,15 +77,16 @@ export class NextcloudLoginGuard { /** Zeitquelle in Millisekunden; Tests ersetzen sie. */ now: () => number = () => Date.now(); - private readonly userFailures = new Map(); - private readonly serverFailures = new Map(); + private readonly userFailures = new Map(); + private readonly serverFailures = new Map(); private readonly flowStarts = new Map(); /** * Darf dieser Benutzer jetzt eine Passwort-Anmeldung versuchen? Wirft 429 * `tooManyAttempts` mit `retryAfterSeconds` (bis der aelteste gezaehlte * Fehlversuch das Fenster verlaesst). `scope` trennt Server, die verschiedene - * Nextclouds ansprechen (Standard: eine gemeinsame Sperre). + * Nextclouds ansprechen (Standard: eine gemeinsame Sperre). Laufende, + * reservierte Versuche zaehlen mit. */ checkPasswordAttempt(userId: string, scope = ''): void { const now = this.now(); @@ -63,33 +97,69 @@ export class NextcloudLoginGuard { let waitMs = 0; if (mine.length >= USER_FAILURE_LIMIT) { - waitMs = Math.max(waitMs, mine[0] + USER_FAILURE_WINDOW_MS - now); + waitMs = Math.max(waitMs, mine[0].at + USER_FAILURE_WINDOW_MS - now); } if (server.length >= SERVER_FAILURE_LIMIT) { - waitMs = Math.max(waitMs, server[0] + SERVER_FAILURE_WINDOW_MS - now); + waitMs = Math.max(waitMs, server[0].at + SERVER_FAILURE_WINDOW_MS - now); } if (waitMs > 0) throw tooMany(waitMs); } + /** + * Prueft die Grenzen UND belegt sofort einen Platz in beiden (synchron, ohne + * `await` dazwischen). Wirft 429 `tooManyAttempts`, wenn eine Grenze voll ist — + * dann wird Nextcloud gar nicht erst gefragt. + */ + beginPasswordAttempt(userId: string, scope = ''): PasswordAttempt { + this.checkPasswordAttempt(userId, scope); + const attempt: Attempt = { at: this.now(), inFlight: true }; + const mine = this.userFailures.get(userId) ?? []; + mine.push(attempt); + this.userFailures.set(userId, mine); + const server = this.serverFailures.get(scope) ?? []; + server.push(attempt); + this.serverFailures.set(scope, server); + let settled = false; + return { + fail: () => { + if (settled) return; + settled = true; + attempt.inFlight = false; + }, + release: () => { + if (settled) return; + settled = true; + removeAttempt(this.userFailures.get(userId), attempt); + removeAttempt(this.serverFailures.get(scope), attempt); + }, + }; + } + recordFailure(userId: string, scope = ''): void { const now = this.now(); + const attempt: Attempt = { at: now, inFlight: false }; const mine = prune(this.userFailures.get(userId) ?? [], now, USER_FAILURE_WINDOW_MS); - mine.push(now); + mine.push(attempt); this.userFailures.set(userId, mine); const server = prune(this.serverFailures.get(scope) ?? [], now, SERVER_FAILURE_WINDOW_MS); - server.push(now); + server.push(attempt); this.serverFailures.set(scope, server); } - /** Eine gelungene Anmeldung loescht nur die Fehlversuche dieses Benutzers. */ + /** + * Eine gelungene Anmeldung loescht nur die abgeschlossenen Fehlversuche dieses + * Benutzers; noch laufende Versuche bleiben gezaehlt. + */ recordSuccess(userId: string): void { - this.userFailures.delete(userId); + const pending = (this.userFailures.get(userId) ?? []).filter((a) => a.inFlight); + if (pending.length === 0) this.userFailures.delete(userId); + else this.userFailures.set(userId, pending); } /** Zaehlt einen Start der Browser-Anmeldung; der 11. in 10 Minuten wird abgewiesen. */ checkFlowStart(userId: string): void { const now = this.now(); - const starts = prune(this.flowStarts.get(userId) ?? [], now, FLOW_START_WINDOW_MS); + const starts = pruneTimes(this.flowStarts.get(userId) ?? [], now, FLOW_START_WINDOW_MS); if (starts.length >= FLOW_START_LIMIT) { this.flowStarts.set(userId, starts); throw tooMany(starts[0] + FLOW_START_WINDOW_MS - now);