From fe7b61c3c1a41dca3812779b9bc9e400299ff62b Mon Sep 17 00:00:00 2001 From: Nick Szaunig Date: Mon, 12 Oct 2026 12:00:03 +0200 Subject: [PATCH] =?UTF-8?q?fix(api-keys):=20g=C3=BCltige=20Schl=C3=BCssel?= =?UTF-8?q?=20nie=20durch=20die=20Fehlversuchs-Sperre=20abweisen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Befunde aus dem Sicherheitsreview des Branches: - MEDIUM: Die Sperre lief vor der Schlüsselsuche und zählte auch falsch aufgebaute Tokens. Wer die gemeinsame Adresse (Cluster-Ausgang, Proxy) nutzt oder per X-Forwarded-For vortäuscht, hätte ERPNext und den Shop 15 Minuten aussperren können. Jetzt: erst Suche über den Hash; ein gültiger, nicht widerrufener, nicht abgelaufener Schlüssel kommt immer durch; 429 nur für unbekannte, widerrufene und abgelaufene Schlüssel; falsch aufgebaute Tokens sofort 401 ohne Zählung. Beim Start warnt ApiCore, wenn TRUST_PROXY in Produktion fehlt (im Cluster gesetzt). - LOW: Bei vollem Speicher verdrängt der Limiter zuerst nicht gesperrte Einträge, gesperrte erst zuletzt. - LOW: Bewegungs-Cursor mit Jahr außerhalb 2000-2100 ergab 500, jetzt 400. - LOW/INFO: Vertrag legt fest, dass ERPNext Produkte und Chargen bei jedem Lauf vollständig neu liest (Voll-Spiegel) und Bewegungen inkrementell; remainingQuantity ist live, Bewegungen kommen 5 Minuten verzögert. Co-Authored-By: Claude Opus 5.5 (1M context) --- ARCHITECTURE.md | 2 +- .../src/api-keys/api-keys.service.spec.ts | 68 ++++++++++---- .../src/api-keys/api-keys.service.ts | 93 ++++++++++--------- .../api-keys/failed-attempt-limiter.spec.ts | 35 +++++++ .../src/api-keys/failed-attempt-limiter.ts | 25 +++-- ApiCore-Backend/src/app.config.spec.ts | 53 +++++++++++ ApiCore-Backend/src/app.config.ts | 14 ++- .../src/erp-export/erp-cursor.spec.ts | 23 +++++ ApiCore-Backend/src/erp-export/erp-cursor.ts | 11 ++- ApiCore-Backend/test/erp-export.e2e-spec.ts | 47 +++++++++- docs/erp-inventar-export.md | 10 +- 11 files changed, 300 insertions(+), 81 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 12197e8..d07a9bc 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -143,7 +143,7 @@ Grundlage: OWASP API Security Top 10 (2023) und OWASP ASVS Level 2. Umsetzung un | Sitzungen | Refresh-Rotation atomar, Diebstahlerkennung, max. 7 Tage; Einmal-Tokens für SSO-Abschluss und MFA-Schritt | | Step-up | Backup-Import, MFA ein/aus verlangen aktuelles Passwort (+ TOTP) | | Autorisierung | fail-closed `PermissionsGuard`; keine Rechteausweitung über Benutzer-/Rollenpflege; Suche nur über lesbare Objektarten | -| Maschinen-Zugriff | API-Schlüssel nur als Hash (Suche über den Hash, kein Vergleich von Geheimnissen), Scope-Allowlist (`shop-sync:read`, `shop-sync:write`, `erp-inventory:read`), IP-Allowlist, widerrufbar, befristbar; Ersteller kann nur eigene Rechte vergeben; nach 20 abgelehnten Schlüsseln in 15 min je IP (IPv6: je /64) `429`; Logs nur mit Schlüssel-Präfix | +| Maschinen-Zugriff | API-Schlüssel nur als Hash (Suche über den Hash, kein Vergleich von Geheimnissen), Scope-Allowlist (`shop-sync:read`, `shop-sync:write`, `erp-inventory:read`), IP-Allowlist, widerrufbar, befristbar; Ersteller kann nur eigene Rechte vergeben; nach 20 ungültigen Schlüsseln in 15 min je IP (IPv6: je /64) `429` für weitere ungültige – gültige Schlüssel nie, falsch aufgebaute Tokens zählen nicht; Warnung beim Start, wenn `TRUST_PROXY` in Produktion fehlt; Logs nur mit Schlüssel-Präfix | | Ausgehende Aufrufe | Webhooks: Ziel-IP beim Verbindungsaufbau geprüft (kein DNS-Rebinding), nur Port 80/443, https in Produktion, 5 s Timeout, Signatur über Zeitstempel+ID+Body | | Transport/Antworten | `Cache-Control: no-store` (außer öffentliche Chargen-Seite), CSP `default-src 'none'`, `frame-ancestors 'none'`, keine ETags; Produktion startet nur mit https-`FRONTEND_ORIGIN` und sicheren Cookies | | Rate-Limits | Throttler vor der Authentifizierung (100/min, Login/MFA/Refresh/Logout 10/min, öffentliche Chargen 30/min); Client-IP über `TRUST_PROXY` | diff --git a/ApiCore-Backend/src/api-keys/api-keys.service.spec.ts b/ApiCore-Backend/src/api-keys/api-keys.service.spec.ts index d24d77e..0d3cc93 100644 --- a/ApiCore-Backend/src/api-keys/api-keys.service.spec.ts +++ b/ApiCore-Backend/src/api-keys/api-keys.service.spec.ts @@ -135,44 +135,78 @@ describe('ApiKeysService.authenticate', () => { ).rejects.toBeInstanceOf(ForbiddenException); }); - it(`sperrt eine Adresse nach ${API_KEY_FAILURE_LIMIT.maxFailures} Fehlversuchen mit 429, ohne die Datenbank zu fragen`, async () => { - for (let i = 0; i < API_KEY_FAILURE_LIMIT.maxFailures; i++) { - await expect(service.authenticate(TOKEN, '203.0.113.7')).resolves.toBe( - null, - ); + async function failFrom(ip: string, times: number): Promise { + for (let i = 0; i < times; i++) { + // Unbekannter, aber richtig aufgebauter Schlüssel. + await expect(service.authenticate(OTHER_TOKEN, ip)).resolves.toBe(null); } - prisma.apiKey.findUnique.mockClear(); - prisma.apiKey.findUnique.mockResolvedValue(storedKey()); + } - // Gesperrt ist gesperrt - auch ein richtiger Schlüssel verrät nichts mehr. - const blocked = service.authenticate(TOKEN, '203.0.113.7'); + it(`sperrt unbekannte Schlüssel einer Adresse nach ${API_KEY_FAILURE_LIMIT.maxFailures} Fehlversuchen mit 429`, async () => { + await failFrom('203.0.113.7', API_KEY_FAILURE_LIMIT.maxFailures); + + const blocked = service.authenticate(OTHER_TOKEN, '203.0.113.7'); await expect(blocked).rejects.toBeInstanceOf(HttpException); + await expect( + service.authenticate(OTHER_TOKEN, '203.0.113.7'), + ).rejects.toMatchObject({ status: 429 }); + expect(loggedText()).toContain('203.0.113.7'); + + // Widerrufene Schlüssel sind ebenfalls ungültig und bleiben gesperrt. + prisma.apiKey.findUnique.mockResolvedValueOnce( + storedKey({ revokedAt: new Date('2026-10-02T00:00:00.000Z') }), + ); await expect( service.authenticate(TOKEN, '203.0.113.7'), ).rejects.toMatchObject({ status: 429 }); - expect(prisma.apiKey.findUnique).not.toHaveBeenCalled(); - expect(loggedText()).toContain('203.0.113.7'); // Andere Adressen sind nicht betroffen. - await expect(service.authenticate(TOKEN, '198.51.100.1')).resolves.toEqual({ + await expect( + service.authenticate(OTHER_TOKEN, '198.51.100.1'), + ).resolves.toBe(null); + }); + + it('weist einen gültigen Schlüssel nie wegen der Sperre ab - sonst könnte jeder ERPNext oder den Shop aussperren', async () => { + await failFrom('203.0.113.7', API_KEY_FAILURE_LIMIT.maxFailures); + await expect( + service.authenticate(OTHER_TOKEN, '203.0.113.7'), + ).rejects.toMatchObject({ status: 429 }); + prisma.apiKey.findUnique.mockResolvedValueOnce(storedKey()); + + await expect(service.authenticate(TOKEN, '203.0.113.7')).resolves.toEqual({ id: 'key-1', scopes: ['erp-inventory:read'], }); }); + it('zählt fehlerhaft aufgebaute Schlüssel nicht als Fehlversuch', async () => { + for (let i = 0; i < API_KEY_FAILURE_LIMIT.maxFailures * 2; i++) { + await expect(service.authenticate('ack_x', '203.0.113.7')).resolves.toBe( + null, + ); + } + // Weder gesperrt noch eine Datenbankabfrage. + expect(prisma.apiKey.findUnique).not.toHaveBeenCalled(); + await expect( + service.authenticate(OTHER_TOKEN, '203.0.113.7'), + ).resolves.toBe(null); + }); + it('zählt IPv6-Adressen aus demselben /64 gemeinsam (wie der Throttler)', async () => { // Wechselnde Adressen im eigenen /64 umgehen die Sperre nicht. for (let i = 1; i <= API_KEY_FAILURE_LIMIT.maxFailures; i++) { - await service.authenticate(TOKEN, `2001:db8:1:2::${i.toString(16)}`); + await service.authenticate( + OTHER_TOKEN, + `2001:db8:1:2::${i.toString(16)}`, + ); } await expect( - service.authenticate(TOKEN, '2001:db8:1:2::ffff'), + service.authenticate(OTHER_TOKEN, '2001:db8:1:2::ffff'), ).rejects.toMatchObject({ status: 429 }); // Ein anderes /64 ist frei. - prisma.apiKey.findUnique.mockResolvedValueOnce(storedKey()); await expect( - service.authenticate(TOKEN, '2001:db8:1:3::1'), - ).resolves.toMatchObject({ id: 'key-1' }); + service.authenticate(OTHER_TOKEN, '2001:db8:1:3::1'), + ).resolves.toBe(null); }); }); diff --git a/ApiCore-Backend/src/api-keys/api-keys.service.ts b/ApiCore-Backend/src/api-keys/api-keys.service.ts index cbde071..f45c21e 100644 --- a/ApiCore-Backend/src/api-keys/api-keys.service.ts +++ b/ApiCore-Backend/src/api-keys/api-keys.service.ts @@ -39,12 +39,13 @@ const LAST_USED_RESOLUTION_MS = 5 * 60 * 1000; const LOGGABLE_PREFIX_LENGTH = 12; /** - * Failed key attempts per client IP before that IP is refused with 429 for - * the rest of the window - the same budget as the shop's ERP export. A key - * holds 256 random bits, so this is not what keeps it safe; it keeps a - * misconfigured or hostile client from hammering the key lookup, and the - * block applies to every key from that address, so a correct key cannot be - * told apart from a wrong one while it lasts. + * Unknown, revoked or expired keys per client network before that network + * gets 429 for further invalid keys for the rest of the window - the same + * budget as the shop's ERP export. It slows down guessing and a + * misconfigured client; a valid key is never refused by it. Otherwise + * anyone able to send requests from (or forge) a shared address - the + * cluster egress, a proxy - could lock ERPNext or the shop out, and a key + * holds 256 random bits, so letting it through costs nothing. */ export const API_KEY_FAILURE_LIMIT = { maxFailures: 20, @@ -52,7 +53,7 @@ export const API_KEY_FAILURE_LIMIT = { maxTrackedSources: 10_000, } as const; -type RejectReason = 'malformed' | 'unknown' | 'revoked' | 'expired'; +type RejectReason = 'unknown' | 'revoked' | 'expired'; /** What a request authenticated by an API key may do - it is not a user. */ export interface ApiKeyPrincipal { @@ -199,19 +200,40 @@ export class ApiKeysService { /** * Resolves a presented key to what it may do, or null for anything that - * must not pass: malformed, unknown, revoked or expired. Each of those - * counts as a failed attempt of the client IP; an IP over the limit gets - * 429 before any lookup. + * must not pass: malformed, unknown, revoked or expired. * * The key is found by the SHA-256 of the presented token through a unique * index - never by comparing secrets byte by byte, so response timing says - * nothing about the stored key. + * nothing about the stored key. Order matters: the lookup comes first, and + * only an invalid key counts against (or is stopped by) the failure limit + * of the client network. A malformed token is not a guess at a key and is + * refused without lookup and without counting. */ async authenticate( token: string, clientIp?: string, ): Promise { - const source = clientIp ?? 'unknown'; + if (!TOKEN_PATTERN.test(token)) { + return null; + } + const apiKey = await this.prisma.apiKey.findUnique({ + where: { keyHash: hashToken(token) }, + }); + const now = new Date(); + const reason = rejectReason(apiKey, now); + if (reason === null && apiKey) { + return this.accept(apiKey, clientIp, now); + } + this.recordRejection(token, reason ?? 'unknown', clientIp); + return null; + } + + /** Counts an invalid key; 429 once the client network is over the limit. */ + private recordRejection( + token: string, + reason: RejectReason, + clientIp: string | undefined, + ): void { // Counted per client network like the throttler (IPv6 /64), so rotating // addresses inside one's own prefix does not reset the budget. const subject = clientIp ? rateLimitSubject(clientIp) : 'unknown'; @@ -228,46 +250,22 @@ export class ApiKeysService { HttpStatus.TOO_MANY_REQUESTS, ); } - const result = await this.resolve(token, clientIp); - if ('principal' in result) { - return result.principal; - } // Only the public prefix is ever logged, never the key itself. - const prefix = - result.reason === 'malformed' - ? '(malformed)' - : token.slice(0, LOGGABLE_PREFIX_LENGTH); this.logger.warn( - `API key rejected (${result.reason}): ${prefix} from ${source}`, + `API key rejected (${reason}): ${token.slice(0, LOGGABLE_PREFIX_LENGTH)} from ${clientIp ?? 'unknown'}`, ); if (this.failures.recordFailure(subject)) { this.logger.warn( - `API key attempts from ${subject} blocked for ${API_KEY_FAILURE_LIMIT.windowMs / 60_000} minutes after ${API_KEY_FAILURE_LIMIT.maxFailures} failures`, + `Invalid API keys from ${subject} refused for ${API_KEY_FAILURE_LIMIT.windowMs / 60_000} minutes after ${API_KEY_FAILURE_LIMIT.maxFailures} failures`, ); } - return null; } - private async resolve( - token: string, + private async accept( + apiKey: ApiKey, clientIp: string | undefined, - ): Promise<{ principal: ApiKeyPrincipal } | { reason: RejectReason }> { - if (!TOKEN_PATTERN.test(token)) { - return { reason: 'malformed' }; - } - const apiKey = await this.prisma.apiKey.findUnique({ - where: { keyHash: hashToken(token) }, - }); - const now = new Date(); - if (!apiKey) { - return { reason: 'unknown' }; - } - if (apiKey.revokedAt) { - return { reason: 'revoked' }; - } - if (apiKey.expiresAt && apiKey.expiresAt <= now) { - return { reason: 'expired' }; - } + now: Date, + ): Promise { if (!isIpAllowed(clientIp, apiKey.allowedIps)) { // The key itself is right: say so, so the admin finds the cause - a // thief learns nothing new, they already hold the key. @@ -290,12 +288,17 @@ export class ApiKeysService { } // Intersect with today's allowlist: narrowing it later also narrows // keys issued before. - return { - principal: { id: apiKey.id, scopes: apiKey.scopes.filter(isApiKeyScope) }, - }; + return { id: apiKey.id, scopes: apiKey.scopes.filter(isApiKeyScope) }; } } +function rejectReason(apiKey: ApiKey | null, now: Date): RejectReason | null { + if (!apiKey) return 'unknown'; + if (apiKey.revokedAt) return 'revoked'; + if (apiKey.expiresAt && apiKey.expiresAt <= now) return 'expired'; + return null; +} + /** The key from an `Authorization: Bearer ack_…` header, or null. */ export function extractApiKeyToken( authorization: string | undefined, diff --git a/ApiCore-Backend/src/api-keys/failed-attempt-limiter.spec.ts b/ApiCore-Backend/src/api-keys/failed-attempt-limiter.spec.ts index bda3cfb..4fbcd69 100644 --- a/ApiCore-Backend/src/api-keys/failed-attempt-limiter.spec.ts +++ b/ApiCore-Backend/src/api-keys/failed-attempt-limiter.spec.ts @@ -74,4 +74,39 @@ describe('FailedAttemptLimiter', () => { small.recordFailure('c'); expect(small.trackedSources()).toBe(2); }); + it('verdrängt bei voller Liste zuerst nicht gesperrte Quellen', () => { + const small = new FailedAttemptLimiter({ + maxFailures: 2, + windowMs: 60_000, + maxTrackedSources: 2, + clock: () => now, + }); + small.recordFailure('angreifer'); + small.recordFailure('angreifer'); // gesperrt, ältester Eintrag + small.recordFailure('harmlos'); // ein Fehlversuch, nicht gesperrt + small.recordFailure('neu'); + + expect(small.trackedSources()).toBe(2); + // Die Sperre bleibt, obwohl sie der älteste Eintrag ist. + expect(small.isBlocked('angreifer')).toBe(true); + // Verdrängt wurde der nicht gesperrte Eintrag. + expect(small.recordFailure('harmlos')).toBe(false); + }); + + it('verdrängt gesperrte Quellen erst, wenn alle gesperrt sind (älteste zuerst)', () => { + const small = new FailedAttemptLimiter({ + maxFailures: 1, + windowMs: 60_000, + maxTrackedSources: 2, + clock: () => now, + }); + small.recordFailure('a'); + small.recordFailure('b'); + small.recordFailure('c'); + + expect(small.trackedSources()).toBe(2); + expect(small.isBlocked('a')).toBe(false); + expect(small.isBlocked('b')).toBe(true); + expect(small.isBlocked('c')).toBe(true); + }); }); diff --git a/ApiCore-Backend/src/api-keys/failed-attempt-limiter.ts b/ApiCore-Backend/src/api-keys/failed-attempt-limiter.ts index 1da300c..662152d 100644 --- a/ApiCore-Backend/src/api-keys/failed-attempt-limiter.ts +++ b/ApiCore-Backend/src/api-keys/failed-attempt-limiter.ts @@ -69,13 +69,26 @@ export class FailedAttemptLimiter { this.windows.delete(source); } } - // Immer noch voll: den ältesten Eintrag verdrängen (Map behält die - // Einfügereihenfolge). Lieber eine Zählung verlieren als unbegrenzt wachsen. + // Immer noch voll: lieber eine Zählung verlieren als unbegrenzt wachsen. + // Zuerst den ältesten nicht gesperrten Eintrag verdrängen - sonst könnte + // ein Angreifer mit vielen Adressen seine eigene Sperre hinausschieben. + // Nur wenn alle gesperrt sind, geht der älteste gesperrte (die Map behält + // die Einfügereihenfolge). if (this.windows.size >= this.options.maxTrackedSources) { - for (const oldest of this.windows.keys()) { - this.windows.delete(oldest); - break; - } + const victim = + this.oldestWhere( + (window) => window.failures < this.options.maxFailures, + ) ?? this.oldestWhere(() => true); + if (victim !== undefined) this.windows.delete(victim); } } + + private oldestWhere( + matches: (window: Window) => boolean, + ): string | undefined { + for (const [source, window] of this.windows) { + if (matches(window)) return source; + } + return undefined; + } } diff --git a/ApiCore-Backend/src/app.config.spec.ts b/ApiCore-Backend/src/app.config.spec.ts index 2a81592..458be14 100644 --- a/ApiCore-Backend/src/app.config.spec.ts +++ b/ApiCore-Backend/src/app.config.spec.ts @@ -65,3 +65,56 @@ describe('configureApp trust proxy logging', () => { log.mockRestore(); }); }); + +describe('configureApp trust proxy warning', () => { + const originalProxy = process.env.TRUST_PROXY; + const originalEnv = process.env.NODE_ENV; + afterEach(() => { + if (originalProxy === undefined) delete process.env.TRUST_PROXY; + else process.env.TRUST_PROXY = originalProxy; + if (originalEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = originalEnv; + }); + + function configure(): jest.SpyInstance { + const warn = jest.spyOn(Logger.prototype, 'warn').mockImplementation(); + jest.spyOn(Logger.prototype, 'log').mockImplementation(); + const app = { + set: jest.fn(), + useBodyParser: jest.fn(), + use: jest.fn(), + useGlobalFilters: jest.fn(), + enableCors: jest.fn(), + setGlobalPrefix: jest.fn(), + enableVersioning: jest.fn(), + useGlobalPipes: jest.fn(), + }; + configureApp(app as unknown as NestExpressApplication); + return warn; + } + + function trustProxyWarnings(warn: jest.SpyInstance): unknown[][] { + const calls = warn.mock.calls as unknown[][]; + return calls.filter(([m]) => String(m).includes('TRUST_PROXY')); + } + + afterEach(() => jest.restoreAllMocks()); + + it('warnt in Produktion, wenn TRUST_PROXY fehlt', () => { + process.env.NODE_ENV = 'production'; + delete process.env.TRUST_PROXY; + expect(trustProxyWarnings(configure())).toHaveLength(1); + }); + + it('warnt nicht, wenn TRUST_PROXY gesetzt ist', () => { + process.env.NODE_ENV = 'production'; + process.env.TRUST_PROXY = 'loopback, 10.42.0.0/16'; + expect(trustProxyWarnings(configure())).toHaveLength(0); + }); + + it('warnt nicht in der Entwicklung', () => { + process.env.NODE_ENV = 'development'; + delete process.env.TRUST_PROXY; + expect(trustProxyWarnings(configure())).toHaveLength(0); + }); +}); diff --git a/ApiCore-Backend/src/app.config.ts b/ApiCore-Backend/src/app.config.ts index f04f2b8..04ceddc 100644 --- a/ApiCore-Backend/src/app.config.ts +++ b/ApiCore-Backend/src/app.config.ts @@ -50,9 +50,17 @@ export function configureApp(app: NestExpressApplication): void { // to work correctly rather than rate-limiting all traffic together. const trustProxy = trustProxySetting(); app.set('trust proxy', trustProxy); - new Logger('Bootstrap').log( - describeTrustProxy(trustProxy, !!process.env.TRUST_PROXY?.trim()), - ); + const bootstrapLogger = new Logger('Bootstrap'); + const trustProxyExplicit = !!process.env.TRUST_PROXY?.trim(); + bootstrapLogger.log(describeTrustProxy(trustProxy, trustProxyExplicit)); + if (!trustProxyExplicit && process.env.NODE_ENV === 'production') { + // Behind Cloudflare and Traefik one hop is wrong: req.ip becomes the + // proxy, so rate limits, API-key failure limits and IP allowlists act + // on a shared address instead of the client. + bootstrapLogger.warn( + 'TRUST_PROXY is not set in production - client IPs for rate limits and API-key allowlists are likely the proxy address; set it as in deploy/k8s/backend/configmap.example.yaml', + ); + } // No ETags: they let caches and proxies revalidate (and keep) personal // responses that are meant to be no-store. app.set('etag', false); diff --git a/ApiCore-Backend/src/erp-export/erp-cursor.spec.ts b/ApiCore-Backend/src/erp-export/erp-cursor.spec.ts index d6fec96..76ad922 100644 --- a/ApiCore-Backend/src/erp-export/erp-cursor.spec.ts +++ b/ApiCore-Backend/src/erp-export/erp-cursor.spec.ts @@ -64,7 +64,30 @@ describe('ERP-Cursor', () => { 'bewegungen', raw({ k: 'bewegungen', ts: '2026-10-01', id: 'c1' }), ], + [ + 'Jahr jenseits des Datenbankbereichs', + 'bewegungen', + raw({ k: 'bewegungen', ts: '+275760-09-13T00:00:00.000Z', id: 'c1' }), + ], + [ + 'Jahr vor 2000', + 'bewegungen', + raw({ k: 'bewegungen', ts: '1999-12-31T23:59:59.999Z', id: 'c1' }), + ], + [ + 'Jahr nach 2100', + 'bewegungen', + raw({ k: 'bewegungen', ts: '2101-01-01T00:00:00.000Z', id: 'c1' }), + ], ] as const)('lehnt ab: %s', (_label, kind, cursor) => { expect(() => decodeCursor(cursor, kind)).toThrow(BadRequestException); }); + + it.each(['2000-01-01T00:00:00.000Z', '2100-12-31T23:59:59.999Z'])( + 'nimmt den Rand des erlaubten Bereichs an: %s', + (ts) => { + const cursor = encodeCursor({ k: 'bewegungen', ts, id: 'c1' }); + expect(decodeCursor(cursor, 'bewegungen').ts).toBe(ts); + }, + ); }); diff --git a/ApiCore-Backend/src/erp-export/erp-cursor.ts b/ApiCore-Backend/src/erp-export/erp-cursor.ts index 9d23374..e9d20e3 100644 --- a/ApiCore-Backend/src/erp-export/erp-cursor.ts +++ b/ApiCore-Backend/src/erp-export/erp-cursor.ts @@ -23,6 +23,11 @@ type CursorOf = Extract; // etwas anderes bedeuten könnten. const ID_PATTERN = /^[A-Za-z0-9_-]{1,64}$/; +// Zeitstempel eines Bewegungs-Cursors: nur plausible Jahre. JavaScript +// kennt Jahre bis +275760, die Datenbankabfrage nicht - das wäre ein 500. +const MIN_CURSOR_YEAR = 2000; +const MAX_CURSOR_YEAR = 2100; + const FIELDS: Record = { produkte: ['id', 'k'], chargen: ['id', 'k', 't'], @@ -55,7 +60,11 @@ function parse(raw: string): Record { function isCanonicalTimestamp(value: unknown): value is string { if (typeof value !== 'string') return false; const date = new Date(value); - return !Number.isNaN(date.getTime()) && date.toISOString() === value; + if (Number.isNaN(date.getTime()) || date.toISOString() !== value) { + return false; + } + const year = date.getUTCFullYear(); + return year >= MIN_CURSOR_YEAR && year <= MAX_CURSOR_YEAR; } /** Prüft den Cursor für genau diesen Endpunkt; sonst 400. */ diff --git a/ApiCore-Backend/test/erp-export.e2e-spec.ts b/ApiCore-Backend/test/erp-export.e2e-spec.ts index 3f43674..6f08ec6 100644 --- a/ApiCore-Backend/test/erp-export.e2e-spec.ts +++ b/ApiCore-Backend/test/erp-export.e2e-spec.ts @@ -577,6 +577,22 @@ describe('ERP-Inventar-Export (e2e)', () => { await get(`bewegungen?nach=${productCursor}`, erpKey).expect(400); }); + it('lehnt einen Bewegungs-Cursor mit unmöglichem Jahr ab (400 statt 500)', async () => { + const cursor = encodeCursor({ + k: 'bewegungen', + ts: '+275760-09-13T00:00:00.000Z', + id: movementIds.hofladen, + }); + await get(`bewegungen?nach=${cursor}`, erpKey).expect(400); + }); + + it('verträgt ein fernes seit (leere Seite statt Fehler)', async () => { + const body = ( + await get('bewegungen?seit=9999-12-31T00:00:00Z', erpKey).expect(200) + ).body as PageBody; + expect(body.items).toEqual([]); + }); + it('lehnt seit und nach zusammen ab (400)', async () => { const cursor = encodeCursor({ k: 'bewegungen', @@ -645,7 +661,7 @@ describe('ERP-Inventar-Export (e2e)', () => { }); describe('Fehlversuche', () => { - it('sperrt eine Adresse nach zu vielen falschen Schlüsseln mit 429, andere nicht', async () => { + it('sperrt nach zu vielen unbekannten Schlüsseln weitere ungültige mit 429, andere Adressen nicht', async () => { const attacker = '203.0.113.50'; const wrong = `ack_abcd1234_${'Y'.repeat(43)}`; for (let i = 0; i < API_KEY_FAILURE_LIMIT.maxFailures; i++) { @@ -656,9 +672,32 @@ describe('ERP-Inventar-Export (e2e)', () => { statusCode: 429, message: expect.stringContaining('Fehlversuche') as string, }); - // Auch ein richtiger Schlüssel verrät von dort nichts mehr. - await get('produkte', erpKey, attacker).expect(429); - await get('produkte', erpKey, '203.0.113.51').expect(200); + await get('produkte', wrong, '203.0.113.51').expect(401); + }); + + it('lässt einen gültigen Schlüssel auch von einer gesperrten Adresse durch', async () => { + // Sonst könnte jeder, der die gemeinsame Adresse (Cluster-Ausgang, + // Proxy) nutzt oder fälscht, ERPNext oder den Shop aussperren. + const shared = '203.0.113.60'; + const wrong = `ack_abcd1234_${'X'.repeat(43)}`; + for (let i = 0; i < API_KEY_FAILURE_LIMIT.maxFailures; i++) { + await get('produkte', wrong, shared).expect(401); + } + await get('produkte', wrong, shared).expect(429); + await get('produkte', erpKey, shared).expect(200); + await request(server()) + .get('/api/v1/shop-sync/stock') + .set('Authorization', `Bearer ${shopKey}`) + .set('X-Forwarded-For', shared) + .expect(200); + }); + + it('zählt fehlerhaft aufgebaute Schlüssel nicht', async () => { + const ip = '203.0.113.70'; + for (let i = 0; i < API_KEY_FAILURE_LIMIT.maxFailures + 5; i++) { + await get('produkte', 'ack_x', ip).expect(401); + } + await get('produkte', `ack_abcd1234_${'W'.repeat(43)}`, ip).expect(401); }); }); }); diff --git a/docs/erp-inventar-export.md b/docs/erp-inventar-export.md index 9cce421..93678cd 100644 --- a/docs/erp-inventar-export.md +++ b/docs/erp-inventar-export.md @@ -16,7 +16,7 @@ Der ERPNext-Importer wird genau nach diesem Dokument gebaut. Änderungen am Vert | Speicherung | ApiCore speichert nur den SHA-256-Hash. Gesucht wird über den Hash (eindeutiger Index), nie durch Vergleich des Geheimnisses. Der Schlüssel selbst wird genau einmal beim Anlegen angezeigt. | | Abschalten | Jederzeit widerrufbar (wirkt sofort), optional mit Ablaufdatum. | | Absender | Optional nur von bestimmten IP-Adressen/Netzen (Allowlist am Schlüssel). | -| Fehlversuche | Nach 20 abgelehnten Schlüsseln (fehlerhaft, unbekannt, widerrufen, abgelaufen) innerhalb von 15 Minuten antwortet ApiCore dieser IP-Adresse (bei IPv6 dem ganzen /64-Netz) mit `429`, auch für richtige Schlüssel. Zusätzlich gilt das allgemeine Limit von 100 Anfragen je Minute und IP. | +| Fehlversuche | Nach 20 ungültigen Schlüsseln (unbekannt, widerrufen, abgelaufen) innerhalb von 15 Minuten bekommt diese IP-Adresse (bei IPv6 das ganze /64-Netz) für weitere ungültige Schlüssel `429`. Ein gültiger Schlüssel wird davon nie abgewiesen, auch nicht von einer gesperrten Adresse – sonst könnte jeder, der dieselbe Adresse nutzt oder vortäuscht, ERPNext oder den Shop aussperren. Nicht wie ein Schlüssel aufgebaute Tokens (`ack_…` im falschen Format) bekommen sofort `401` und zählen nicht. Zusätzlich gilt das allgemeine Limit von 100 Anfragen je Minute und IP. | | Protokoll | Abgelehnte Schlüssel werden mit Grund, Präfix (`ack_abcd1234`) und IP protokolliert, nie mit dem Schlüssel. Query-Strings landen nicht im Log. | | Trennung | Eigener Schlüssel nur für ERPNext. Der Wabenhain-Schlüssel (`shop-sync:*`) kommt an diese Endpunkte nicht heran und umgekehrt. | @@ -57,7 +57,9 @@ Benutzer mit dem Recht `erp-inventory:read` (z. B. die Rolle Administrator) kön Ablauf: ohne `nach` beginnen, dann mit `nach=` weiterlesen, solange `hasMore` `true` ist. -`produkte` und `chargen` sind Momentaufnahmen und werden in jedem Lauf vollständig gelesen. Ändert sich zwischen zwei Seiten ein Bestand, gleicht der nächste Lauf das aus. Bei `bewegungen` speichert der Importer den letzten `nextCursor` und liest im nächsten Lauf dort weiter. +**Voll-Spiegel für `produkte` und `chargen` (verbindlich):** ERPNext liest beide Listen in **jedem** Lauf vollständig neu, beginnend ohne `nach`, und speichert für sie keinen Cursor über den Lauf hinaus. Die Kennungen (cuid) sind nicht streng monoton: ein während eines Durchlaufs neu angelegter Artikel oder eine neue Charge kann in diesem Durchlauf fehlen und erscheint im nächsten. Ändert sich zwischen zwei Seiten ein Bestand, gleicht ebenfalls der nächste Lauf das aus. Eine bereits bekannte Charge kann so nicht verloren gehen: fehlt sie in einem vollständigen Durchlauf, ist sie gelöscht (siehe Abschnitt 4). + +**Inkrementell für `bewegungen`:** Der Importer speichert den letzten `nextCursor` und liest im nächsten Lauf dort weiter. Die Sortierung nach (`createdAt`, `id`) zusammen mit der Wartezeit (Abschnitt 5) sorgt dafür, dass keine Bewegung übersprungen wird. ## 3. `GET /produkte` – Artikel @@ -102,7 +104,7 @@ Erst alle Abfüllungen (`BOTTLING`), dann alle Herstellungschargen (`PRODUCTION` Hinweise für den Abgleich: -- `remainingQuantity` ist der Sollwert für den Bestandsabgleich (Stock Reconciliation) im Lager „Imkerei (ApiCore)“. Er enthält alle Bewegungen bis zum Abruf, auch die der letzten Minuten, die `/bewegungen` noch zurückhält. +- `remainingQuantity` ist der Sollwert für den Bestandsabgleich (Stock Reconciliation) im Lager „Imkerei (ApiCore)“. Er ist **live**: er enthält alle Bewegungen bis zum Abruf. `/bewegungen` liefert dagegen nur Bewegungen, die älter als 5 Minuten sind. Eine Buchung der letzten Minuten steckt also schon im Rest, ihre Bewegung kommt erst im nächsten Lauf. Der Importer darf deshalb den Rest nicht aus den gelieferten Bewegungen nachrechnen und mit `remainingQuantity` vergleichen; Sollwert ist immer `remainingQuantity`, die Bewegungen dienen nur der Einordnung (Vertrauensschrank, Eigenbedarf). - Wird `initialQuantity` in ApiCore korrigiert, ändert sich der Rest sofort mit. - Eine irrtümlich angelegte Herstellungscharge ohne Bewegungen kann in ApiCore gelöscht werden; sie fehlt dann in der Liste und ist im Spiegel auf 0 zu setzen. Abfüllungen und Chargen mit Bewegungen werden nie gelöscht. @@ -181,7 +183,7 @@ Fehlerantworten sind JSON mit `statusCode` und `message` (bei Validierungsfehler | `400` | `limit` außerhalb 1–500 oder keine Ganzzahl; `nach` kein gültiger Cursor oder von einem anderen Endpunkt; `seit` kein ISO-Zeitpunkt; `seit` und `nach` zusammen; `jahr` außerhalb 1900–2100; unbekannter Parameter | Programmfehler – Schritt abbrechen, im Cockpit melden | | `401` | Schlüssel fehlt, ist fehlerhaft, unbekannt, widerrufen oder abgelaufen | Schritt abbrechen; Hinweis „ApiCore-Schlüssel prüfen“ | | `403` | Schlüssel ohne `erp-inventory:read`, oder IP-Adresse nicht in der Allowlist des Schlüssels | Schritt abbrechen; Hinweis „Recht bzw. Adressen am Schlüssel prüfen“ | -| `429` | zu viele Fehlversuche oder Anfragen von dieser Adresse | Schritt abbrechen, im nächsten Lauf erneut | +| `429` | ungültiger Schlüssel von einer Adresse mit zu vielen Fehlversuchen, oder allgemeines Anfragelimit überschritten | Schritt abbrechen, im nächsten Lauf erneut | | `5xx` / keine Antwort | Störung in ApiCore | Schritt abbrechen, im nächsten Lauf erneut | Ein fehlgeschlagener Abruf ist nie „leer“: der Importer setzt dann keine Bestände auf 0 und schiebt keinen Cursor weiter. Die anderen Schritte des Abgleich-Jobs laufen weiter.