From adfe18c703796595a64ce055a351108aa943514f Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:23:47 +0700 Subject: [PATCH 01/14] fix(webhook): deliver session lifecycle events and key webhook hardening (#335) --- CHANGELOG.md | 10 +++ docs/22-n8n-integration.md | 22 ++++-- src/modules/session/session.service.spec.ts | 71 ++++++++++++++++++- src/modules/session/session.service.ts | 21 +++++- src/modules/webhook/dto/webhook.dto.ts | 1 + .../webhook/utils/idempotency.util.spec.ts | 64 +++++++++++++++++ src/modules/webhook/utils/idempotency.util.ts | 28 +++++--- src/modules/webhook/webhook.service.ts | 7 +- 8 files changed, 203 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66bcc8f6..e8e6eb8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Webhook subscriptions for session lifecycle events now deliver.** `session.status`, `session.qr`, + `session.authenticated` and `session.disconnected` were accepted on subscribe but were never dispatched, so + consumers (including the n8n trigger node) waited indefinitely. They now fire from the engine lifecycle. The + n8n integration docs are corrected to the real event names (`session.authenticated`, `session.qr` — + previously documented as the non-existent `session.connected`/`session.qr_ready`, which were rejected at + registration). `group.*` events remain accepted on subscribe but are documented as reserved (not emitted + yet). + ## [0.4.2] - 2026-06-19 Bug-fix and hardening release: access-control tightening, session-lifecycle resilience, data-migration diff --git a/docs/22-n8n-integration.md b/docs/22-n8n-integration.md index dc86bc11..81bab449 100644 --- a/docs/22-n8n-integration.md +++ b/docs/22-n8n-integration.md @@ -73,13 +73,21 @@ Start workflows when WhatsApp events occur. #### Supported Events -| Event | Description | Use Case | -| ---------------------- | ------------------------- | ------------------------ | -| `message.received` | New incoming message | Auto-reply, lead capture | -| `message.sent` | Message sent successfully | Delivery confirmation | -| `session.connected` | Session authenticated | Startup notifications | -| `session.disconnected` | Session lost connection | Alert monitoring | -| `session.qr_ready` | QR code generated | Reconnection alerts | +| Event | Description | Use Case | +| ----------------------- | ----------------------------------- | ------------------------- | +| `message.received` | New incoming message | Auto-reply, lead capture | +| `message.sent` | Message sent successfully | Delivery confirmation | +| `message.ack` | Delivery/read status advanced | Read receipts | +| `message.failed` | Outgoing message failed | Failure alerting | +| `message.revoked` | Message deleted for everyone | Deletion tracking | +| `session.status` | Session status changed | Lifecycle tracking | +| `session.qr` | QR code generated | Reconnection alerts | +| `session.authenticated` | Session logged in (phone available) | Startup notifications | +| `session.disconnected` | Session lost connection | Alert monitoring | + +> **Reserved:** `group.join`, `group.leave`, and `group.update` are accepted by the +> subscription API but are not emitted yet — don't depend on them until a release notes +> them as live. #### How It Works diff --git a/src/modules/session/session.service.spec.ts b/src/modules/session/session.service.spec.ts index 938b9b49..b35ee95a 100644 --- a/src/modules/session/session.service.spec.ts +++ b/src/modules/session/session.service.spec.ts @@ -9,7 +9,7 @@ import { EngineFactory } from '../../engine/engine.factory'; import { EventsGateway } from '../events/events.gateway'; import { WebhookService } from '../webhook/webhook.service'; import { HookManager } from '../../core/hooks'; -import { IncomingMessage, EngineEventCallbacks } from '../../engine/interfaces/whatsapp-engine.interface'; +import { IncomingMessage, EngineEventCallbacks, EngineStatus } from '../../engine/interfaces/whatsapp-engine.interface'; function createMockSession(overrides: Partial = {}): Session { return { @@ -760,6 +760,75 @@ describe('SessionService', () => { expect(dispatchedEvents('message.revoked')).toHaveLength(1); expect(eventsGateway.emitMessageRevoked as jest.Mock).toHaveBeenCalledWith('sess-uuid-1', expect.anything()); }); + + // ── session lifecycle events ────────────────────────────────────── + + it('dispatches session.qr with the QR payload when the engine emits a QR code', async () => { + const callbacks = await startAndCaptureCallbacks(); + expect(typeof callbacks.onQRCode).toBe('function'); + + callbacks.onQRCode!('qr-data-abc'); + await flush(); + + const qr = dispatchedEvents('session.qr'); + expect(qr).toHaveLength(1); + expect(qr[0][0]).toBe('sess-uuid-1'); + expect(qr[0][2]).toMatchObject({ sessionId: 'sess-uuid-1', qr: 'qr-data-abc' }); + }); + + it('dispatches session.authenticated with phone/pushName when the engine reports ready', async () => { + const callbacks = await startAndCaptureCallbacks(); + expect(typeof callbacks.onReady).toBe('function'); + + callbacks.onReady!('628123', 'Alice'); + await flush(); + + const auth = dispatchedEvents('session.authenticated'); + expect(auth).toHaveLength(1); + expect(auth[0][0]).toBe('sess-uuid-1'); + expect(auth[0][2]).toMatchObject({ sessionId: 'sess-uuid-1', phone: '628123', pushName: 'Alice' }); + }); + + it('dispatches session.disconnected with the reason when the engine disconnects', async () => { + const callbacks = await startAndCaptureCallbacks(); + expect(typeof callbacks.onDisconnected).toBe('function'); + // Isolate the dispatch from the reconnect scheduler, which would otherwise leave a live timer. + jest + .spyOn(service as unknown as { scheduleReconnect: (id: string, s: unknown) => void }, 'scheduleReconnect') + .mockImplementation(() => undefined); + + callbacks.onDisconnected!('logged out'); + await flush(); + + const disc = dispatchedEvents('session.disconnected'); + expect(disc).toHaveLength(1); + expect(disc[0][0]).toBe('sess-uuid-1'); + expect(disc[0][2]).toMatchObject({ sessionId: 'sess-uuid-1', reason: 'logged out' }); + }); + + it('dispatches session.status on a session status transition', async () => { + await startAndCaptureCallbacks(); + await flush(); + + // start() transitions the session to INITIALIZING via updateStatus(). + const status = dispatchedEvents('session.status'); + expect(status.length).toBeGreaterThanOrEqual(1); + expect(status[0][0]).toBe('sess-uuid-1'); + expect(status[0][2]).toMatchObject({ sessionId: 'sess-uuid-1', status: SessionStatus.INITIALIZING }); + }); + + it('does not double-dispatch session.status when onStateChanged and a dedicated callback report the same status', async () => { + const callbacks = await startAndCaptureCallbacks(); + // wwebjs signals a QR transition via BOTH onStateChanged(QR_READY) and onQRCode → updateStatus(QR_READY) twice. + callbacks.onStateChanged!(EngineStatus.QR_READY); + callbacks.onQRCode!('qr-data-abc'); + await flush(); + + const qrStatus = dispatchedEvents('session.status').filter( + c => (c[2] as { status?: string }).status === SessionStatus.QR_READY, + ); + expect(qrStatus).toHaveLength(1); + }); }); // ── stop ────────────────────────────────────────────────────────── diff --git a/src/modules/session/session.service.ts b/src/modules/session/session.service.ts index b5e2b1b2..73431b0b 100644 --- a/src/modules/session/session.service.ts +++ b/src/modules/session/session.service.ts @@ -91,6 +91,11 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat // Reconnection state per session private reconnectStates: Map = new Map(); + // Last session.status value dispatched to webhooks per session. Some engines signal one transition + // via BOTH onStateChanged and a dedicated callback (onQRCode/onDisconnected), so this guards the + // webhook POST against firing the same status twice. Cleared on delete(). + private readonly lastDispatchedStatus = new Map(); + // Sessions currently being stopped/deleted. An in-flight executeReconnect awaits // engine init, so a stop/delete during that window could re-register an engine AFTER // teardown (orphan). stop()/delete() add the id here; executeReconnect checks it after its @@ -345,6 +350,7 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat } finally { // Always clear the teardown mark so a later recreate/start with this id isn't suppressed. this.stoppingSessions.delete(id); + this.lastDispatchedStatus.delete(id); } } @@ -411,12 +417,14 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat await this.updateStatus(id, SessionStatus.INITIALIZING); await engine.initialize({ - onQRCode: (): void => { + onQRCode: (qr: string): void => { this.logger.log('QR code generated', { sessionId: id, action: 'qr_generated', }); + void this.webhookService.dispatch(id, 'session.qr', { sessionId: id, qr }); + // Execute hook for QR event void this.hookManager.execute( 'session:qr', @@ -437,6 +445,8 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat action: 'ready', }); + void this.webhookService.dispatch(id, 'session.authenticated', { sessionId: id, phone, pushName }); + // Execute hook for ready event void this.hookManager.execute( 'session:ready', @@ -696,6 +706,8 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat action: 'disconnected', }); + void this.webhookService.dispatch(id, 'session.disconnected', { sessionId: id, reason }); + // Execute hook for disconnected event void this.hookManager.execute( 'session:disconnected', @@ -1005,6 +1017,13 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat }); // Emit real-time event to connected WebSocket clients this.eventsGateway.emitSessionStatus(id, status); + // Mirror the status change to subscribed webhooks. Some engines signal one transition via both + // onStateChanged AND a dedicated callback (onQRCode/onDisconnected), which would POST the same + // status twice — dispatch only when the status actually changed from the last one we sent. + if (this.lastDispatchedStatus.get(id) !== status) { + this.lastDispatchedStatus.set(id, status); + void this.webhookService.dispatch(id, 'session.status', { sessionId: id, status }); + } } /** diff --git a/src/modules/webhook/dto/webhook.dto.ts b/src/modules/webhook/dto/webhook.dto.ts index aab2e33e..94d7229e 100644 --- a/src/modules/webhook/dto/webhook.dto.ts +++ b/src/modules/webhook/dto/webhook.dto.ts @@ -26,6 +26,7 @@ export const WEBHOOK_EVENTS = [ 'session.qr', 'session.authenticated', 'session.disconnected', + // Reserved: accepted on subscribe but not dispatched yet (no engine emit source). 'group.join', 'group.leave', 'group.update', diff --git a/src/modules/webhook/utils/idempotency.util.spec.ts b/src/modules/webhook/utils/idempotency.util.spec.ts index 0d56c730..062b9f41 100644 --- a/src/modules/webhook/utils/idempotency.util.spec.ts +++ b/src/modules/webhook/utils/idempotency.util.spec.ts @@ -47,6 +47,70 @@ describe('Idempotency Utils', () => { expect(key).toBe('sess_sess_1_CONNECTED'); }); + it('salts session.status keys with the occurrence time so repeated transitions to the same status stay distinct', () => { + const a = generateIdempotencyKey( + 'session.status', + { sessionId: 'A', status: 'DISCONNECTED' }, + '2026-06-19T00:00:00.000Z', + ); + const b = generateIdempotencyKey( + 'session.status', + { sessionId: 'A', status: 'DISCONNECTED' }, + '2026-06-19T02:00:00.000Z', + ); + expect(a).not.toBe(b); + }); + + it('salts session.authenticated keys so re-authentication (same phone, later time) is a distinct event', () => { + const a = generateIdempotencyKey( + 'session.authenticated', + { sessionId: 'A', phone: '628', pushName: 'Me' }, + '2026-06-19T00:00:00.000Z', + ); + const b = generateIdempotencyKey( + 'session.authenticated', + { sessionId: 'A', phone: '628', pushName: 'Me' }, + '2026-06-19T01:00:00.000Z', + ); + expect(a).not.toBe(b); + }); + + it('salts session.disconnected keys so repeat disconnects with the same reason stay distinct', () => { + const a = generateIdempotencyKey( + 'session.disconnected', + { sessionId: 'A', reason: 'logged out' }, + '2026-06-19T00:00:00.000Z', + ); + const b = generateIdempotencyKey( + 'session.disconnected', + { sessionId: 'A', reason: 'logged out' }, + '2026-06-19T03:00:00.000Z', + ); + expect(a).not.toBe(b); + }); + + it('is retry-stable: the same lifecycle occurrence regenerates the same key', () => { + const at = '2026-06-19T00:00:00.000Z'; + const a = generateIdempotencyKey('session.disconnected', { sessionId: 'A', reason: 'logged out' }, at); + const b = generateIdempotencyKey('session.disconnected', { sessionId: 'A', reason: 'logged out' }, at); + expect(a).toBe(b); + }); + + it('does not salt message-event keys with the occurrence time (content-based dedup preserved)', () => { + const a = generateIdempotencyKey( + 'message.ack', + { id: 'X', status: 'read', sessionId: 'A' }, + '2026-06-19T00:00:00.000Z', + ); + const b = generateIdempotencyKey( + 'message.ack', + { id: 'X', status: 'read', sessionId: 'A' }, + '2026-06-19T09:00:00.000Z', + ); + expect(a).toBe(b); + expect(a).toBe('ack_A_X_read'); + }); + it('should generate key for group.join', () => { const key = generateIdempotencyKey('group.join', { groupId: 'grp_1', diff --git a/src/modules/webhook/utils/idempotency.util.ts b/src/modules/webhook/utils/idempotency.util.ts index ec61405a..99140480 100644 --- a/src/modules/webhook/utils/idempotency.util.ts +++ b/src/modules/webhook/utils/idempotency.util.ts @@ -24,11 +24,17 @@ function hashData(data: Record): string { * Same event with same data will produce the same key (deterministic). * * @remarks - * Keys are content-based and do NOT include timestamps. - * This ensures that replayed/retried events with identical payloads - * produce the same key for proper deduplication. + * Message keys are content-based (keyed on the unique message id), so two deliveries of the same + * logical message dedupe. Lifecycle events (session.status/authenticated/disconnected) recur with + * identical content — the same phone on every reconnect, a constant disconnect reason — so they are + * salted with `occurredAt` (captured ONCE per dispatch and reused across retries): distinct + * occurrences get distinct keys while retries of the same occurrence stay stable. + * + * @param occurredAt - ISO timestamp captured once per dispatch; salts recurring lifecycle keys. */ -export function generateIdempotencyKey(event: string, data: Record): string { +export function generateIdempotencyKey(event: string, data: Record, occurredAt?: string): string { + // Salt applied only to the recurring lifecycle keys below; message/qr keys ignore it. + const occurrence = occurredAt ? `_${occurredAt}` : ''; switch (event) { case 'message.received': case 'message.sent': @@ -51,20 +57,22 @@ export function generateIdempotencyKey(event: string, data: Record w.events.includes(event) || w.events.includes('*')); - // Generate idempotency key (same for all webhooks receiving this event) - const idempotencyKey = generateIdempotencyKey(event, { ...data, sessionId }); + // Generate idempotency key (same for all webhooks receiving this event). occurredAt is captured + // once here and reused for every retry of this dispatch, so recurring lifecycle events get a + // distinct-per-occurrence key while retries of the same event stay stable. + const occurredAt = new Date().toISOString(); + const idempotencyKey = generateIdempotencyKey(event, { ...data, sessionId }, occurredAt); // Dispatch to all matching webhooks for (const webhook of matchingWebhooks) { From 33f480a9cab522ec87d11e27b74151ab8dc5de59 Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:24:30 +0700 Subject: [PATCH 02/14] fix(security): pin outbound webhook and media fetches to validated IP (#338) --- package-lock.json | 59 ++++++++- package.json | 1 + src/common/media/load-remote-media.spec.ts | 24 ++-- src/common/media/load-remote-media.ts | 71 +++++----- src/common/security/ssrf-guard.spec.ts | 123 ++++++++++++++++++ src/common/security/ssrf-guard.ts | 86 ++++++++++-- .../adapters/whatsapp-web-js.adapter.spec.ts | 76 +++++++---- .../adapters/whatsapp-web-js.adapter.ts | 29 ++--- .../processors/webhook.processor.spec.ts | 10 +- .../queue/processors/webhook.processor.ts | 38 +++--- src/modules/webhook/webhook.service.spec.ts | 21 +-- src/modules/webhook/webhook.service.ts | 56 +++----- 12 files changed, 425 insertions(+), 169 deletions(-) diff --git a/package-lock.json b/package-lock.json index 348bcfc8..6f5afd2f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "openwa", - "version": "0.4.1", + "version": "0.4.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "openwa", - "version": "0.4.1", + "version": "0.4.2", "hasInstallScript": true, "license": "MIT", "dependencies": { @@ -41,6 +41,7 @@ "sqlite3": "^5.1.7", "tar-stream": "^3.2.0", "typeorm": "^0.3.29", + "undici": "^6.27.0", "uuid": "^14.0.0", "whatsapp-web.js": "^1.34.7" }, @@ -761,6 +762,7 @@ "integrity": "sha512-CGOfOJqWjg2qW/Mb6zNsDm+u5vFQ8DxXfbM09z69p5Z6+mE1ikP2jUXw+j42Pf1XTYED2Rni5f95npYeuwMDQA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@babel/code-frame": "^7.29.0", "@babel/generator": "^7.29.0", @@ -1266,6 +1268,7 @@ "resolved": "https://registry.npmjs.org/@bull-board/api/-/api-8.0.0.tgz", "integrity": "sha512-GYXJNJclgm9H5Tt/tmuNWfjyF2BuygMwl8xrRIfxxOTgcK4SB8zNUPveTcHGAOoKKtPDVotHNZCBRrFCcOAXMA==", "license": "MIT", + "peer": true, "dependencies": { "redis-info": "^3.1.0" }, @@ -1304,6 +1307,7 @@ "resolved": "https://registry.npmjs.org/@bull-board/ui/-/ui-8.0.0.tgz", "integrity": "sha512-X/F256CmpBj9oj+fK2wOzjygkhTtjgWnc3f/SxPiUs3rQFvgYCplJLEaURh13J+Tpq46/1drAxq4Ukt4e5LelA==", "license": "MIT", + "peer": true, "dependencies": { "@bull-board/api": "8.0.0" } @@ -1672,6 +1676,7 @@ "resolved": "https://registry.npmjs.org/@grpc/grpc-js/-/grpc-js-1.14.4.tgz", "integrity": "sha512-k9Dj3DV/itK9D06Y8f190Qgop7/Ui+D0njFV3LHMPwPT75DpXLQohE9Wmz0QElrJnzsjB7KPWiKJbOl7IPDArQ==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@grpc/proto-loader": "^0.8.0", "@js-sdsl/ordered-map": "^4.4.2" @@ -1703,6 +1708,7 @@ "resolved": "https://registry.npmjs.org/@grpc/proto-loader/-/proto-loader-0.7.15.tgz", "integrity": "sha512-tMXdRCfYVixjuFK+Hk0Q1s38gV9zDiDJfWL3h1rv4Qc39oILCu1TRTDt7+fGUI8K4G1Fj125Hx/ru3azECWTyQ==", "license": "Apache-2.0", + "peer": true, "dependencies": { "lodash.camelcase": "^4.3.0", "long": "^5.0.0", @@ -3491,6 +3497,7 @@ "resolved": "https://registry.npmjs.org/@nestjs/bull-shared/-/bull-shared-11.0.4.tgz", "integrity": "sha512-VBJcDHSAzxQnpcDfA0kt9MTGUD1XZzfByV70su0W0eDCQ9aqIEBlzWRW21tv9FG9dIut22ysgDidshdjlnczLw==", "license": "MIT", + "peer": true, "dependencies": { "tslib": "2.8.1" }, @@ -3645,6 +3652,7 @@ "resolved": "https://registry.npmjs.org/@nestjs/common/-/common-11.1.27.tgz", "integrity": "sha512-kEGSzqM2lWr4whh4Ubflw+oPZSEzxvRMu9WL+LveZploJWTjec5bBlCiRVlVzTPg2kIwBiLwWSvCCW7Wnin1gg==", "license": "MIT", + "peer": true, "dependencies": { "file-type": "21.3.4", "iterare": "1.2.1", @@ -3691,6 +3699,7 @@ "resolved": "https://registry.npmjs.org/@nestjs/core/-/core-11.1.27.tgz", "integrity": "sha512-K6DX7hcqmZdeXkv7tsPakKBRCgqL19a4mtbX4FluY0hWtFdtPKp6lbe+lb8gWPfvLdbOWr/CPScn7BSjBX+Ecg==", "license": "MIT", + "peer": true, "dependencies": { "fast-safe-stringify": "2.1.1", "iterare": "1.2.1", @@ -3750,6 +3759,7 @@ "resolved": "https://registry.npmjs.org/@nestjs/platform-express/-/platform-express-11.1.27.tgz", "integrity": "sha512-0ZFhz6H6EdGh4xQVbUNwjoAwBuz73P7FvUAl67h9CTdMqQlJDaQYJApBv8pKfVZ1fGjMCbl0m9DcC6pXaZPWSQ==", "license": "MIT", + "peer": true, "dependencies": { "cors": "2.8.6", "express": "5.2.1", @@ -3771,6 +3781,7 @@ "resolved": "https://registry.npmjs.org/@nestjs/platform-socket.io/-/platform-socket.io-11.1.27.tgz", "integrity": "sha512-xgpLzaIDGOCC6xOAtHnRAz8sqieFgGxxu3MN5ID026Jt6oeL3efp29N5QHhPr7UlqBfy/Jd02uj0POkZq6Au3Q==", "license": "MIT", + "peer": true, "dependencies": { "socket.io": "4.8.3", "tslib": "2.8.1" @@ -3983,6 +3994,7 @@ "resolved": "https://registry.npmjs.org/@nestjs/typeorm/-/typeorm-11.0.1.tgz", "integrity": "sha512-8rw/nKT0S+L+MkzgE9F2/mox7mAgsPlwfzmW9gsESN1lmQtIrVEfiiBwC2O8+guS1jBfQehJIdcdUj2OAp4VUQ==", "license": "MIT", + "peer": true, "peerDependencies": { "@nestjs/common": "^10.0.0 || ^11.0.0", "@nestjs/core": "^10.0.0 || ^11.0.0", @@ -4563,6 +4575,7 @@ "integrity": "sha512-FXx2pKgId/WyYo2jXw63kk7/+TY7u7AziEJxJAnSFzHlqTAS3Ync6SvgYAN/k4/PQpnnVuzoMuVnByKK2qp0ag==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@types/estree": "*", "@types/json-schema": "*" @@ -4682,6 +4695,7 @@ "resolved": "https://registry.npmjs.org/@types/node/-/node-25.9.3.tgz", "integrity": "sha512-603BddQMv3pUcr4U2dhujk83N2tTDVr/34wII2B6bJy6g+8WD6yUb11jszNs0gdi4PesVWl7ABt8nYMVpnLUcg==", "license": "MIT", + "peer": true, "dependencies": { "undici-types": ">=7.24.0 <7.24.7" } @@ -4907,6 +4921,7 @@ "integrity": "sha512-5B7PfA2e1NQGCnDHd/0lW7W3gvp3d59Ryw54FYO8Uswxo9f6ikw3AZV+Xj/TvpImmpsiYyUqAfhC6kJID1jF6w==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.61.0", "@typescript-eslint/types": "8.61.0", @@ -5658,6 +5673,7 @@ "integrity": "sha512-UVJyE9MttOsBQIDKw1skb9nAwQuR5wuGD3+82K6JgJlm/Y+KI92oNsMNGZCYdDsVtRHSak0pcV5Dno5+4jh9sw==", "devOptional": true, "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -5746,6 +5762,7 @@ "integrity": "sha512-fgFx7Hfoq60ytK2c7DhnF8jIvzYgOMxfugjLOSMHjLIPgenqa7S7oaagATUq99mV6IYvN2tRmC0wnTYX6iPbMw==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "fast-deep-equal": "^3.1.1", "fast-json-stable-stringify": "^2.0.0", @@ -6343,6 +6360,7 @@ "resolved": "https://registry.npmjs.org/bare-events/-/bare-events-2.8.2.tgz", "integrity": "sha512-riJjyv1/mHLIPX4RwiK+oW9/4c3TEUeORHKefKAKnZ5kyslbN+HXowtbaVEqt4IMUB7OXlfixcs6gsFeo/jhiQ==", "license": "Apache-2.0", + "peer": true, "peerDependencies": { "bare-abort-controller": "*" }, @@ -6645,6 +6663,7 @@ } ], "license": "MIT", + "peer": true, "dependencies": { "baseline-browser-mapping": "^2.10.12", "caniuse-lite": "^1.0.30001782", @@ -6735,6 +6754,7 @@ "resolved": "https://registry.npmjs.org/bullmq/-/bullmq-5.78.1.tgz", "integrity": "sha512-zD5IT+qMqbMgPFPdL9FwnZka1bz6nckM+5lXj4N0vsXqdzoVO6wizmXpwsg/4GnHmXJsL7XOKeWA64tYUdPrOA==", "license": "MIT", + "peer": true, "dependencies": { "cron-parser": "4.9.0", "ioredis": "5.10.1", @@ -7052,6 +7072,7 @@ "integrity": "sha512-Qgzu8kfBvo+cA4962jnP1KkS6Dop5NS6g7R5LFYJr4b8Ub94PPQXUksCw9PvXoeXPRRddRNC5C1JQUR2SMGtnA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "readdirp": "^4.0.1" }, @@ -7121,13 +7142,15 @@ "version": "0.5.1", "resolved": "https://registry.npmjs.org/class-transformer/-/class-transformer-0.5.1.tgz", "integrity": "sha512-SQa1Ws6hUbfC98vKGxZH3KFY0Y1lm5Zm0SY8XX9zbK7FJCyVEac3ATW0RIpwzW+oOfmHE5PMPufDG9hCfoEOMw==", - "license": "MIT" + "license": "MIT", + "peer": true }, "node_modules/class-validator": { "version": "0.15.1", "resolved": "https://registry.npmjs.org/class-validator/-/class-validator-0.15.1.tgz", "integrity": "sha512-LqoS80HBBSCVhz/3KloUly0ovokxpdOLR++Al3J3+dHXWt9sTKlKd4eYtoxhxyUjoe5+UcIM+5k9MIxyBWnRTw==", "license": "MIT", + "peer": true, "dependencies": { "@types/validator": "^13.15.3", "libphonenumber-js": "^1.11.1", @@ -7980,7 +8003,8 @@ "version": "0.0.1581282", "resolved": "https://registry.npmjs.org/devtools-protocol/-/devtools-protocol-0.0.1581282.tgz", "integrity": "sha512-nv7iKtNZQshSW2hKzYNr46nM/Cfh5SEvE2oV0/SEGgc9XupIY5ggf84Cz8eJIkBce7S3bmTAauFD6aysMpnqsQ==", - "license": "BSD-3-Clause" + "license": "BSD-3-Clause", + "peer": true }, "node_modules/dezalgo": { "version": "1.0.4", @@ -8478,6 +8502,7 @@ "integrity": "sha512-1y+7C+vi12bUK1IpZeaV3gsH9fHLBmPvYmPx42pvT/E9yG0IC8g3PUZZgp0+JLJl7ZDK0flc2gc+Aw9dpCvIsQ==", "dev": true, "license": "MIT", + "peer": true, "workspaces": [ "packages/*" ], @@ -8537,6 +8562,7 @@ "integrity": "sha512-82GZUjRS0p/jganf6q1rEO25VSoHH0hKPCTrgillPjdI/3bgBhAE1QzHrHTizjpRvy6pGAvKjDJtk2pF9NDq8w==", "dev": true, "license": "MIT", + "peer": true, "bin": { "eslint-config-prettier": "bin/cli.js" }, @@ -10048,6 +10074,7 @@ "resolved": "https://registry.npmjs.org/ioredis/-/ioredis-5.11.1.tgz", "integrity": "sha512-ehuGcf94bQXhfagULNXrJdfnWO38v070jxSx/qE87Kjzmu2fU7ro5EFAb+OPituLqgfyuQaym5DlrNydW2sJ9A==", "license": "MIT", + "peer": true, "dependencies": { "@ioredis/commands": "1.10.0", "cluster-key-slot": "1.1.1", @@ -10330,6 +10357,7 @@ "integrity": "sha512-Yi1jqNC/Oq0N4hBgNH/YvBpP1P57QqundgytzYqy3yqAa7NZPNjSoi4SGbRAXDMdBzNE6xBCi5U7RgfrvMEUVQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@jest/core": "30.4.2", "@jest/types": "30.4.1", @@ -12321,7 +12349,6 @@ "resolved": "https://registry.npmjs.org/object-hash/-/object-hash-3.0.0.tgz", "integrity": "sha512-RSn9F68PjH9HqtltsSnqYC1XXoWe9Bju5+213R98cNGttag9q9yAOTzdbsqvIa7aNm5WffBZFpWYr2aWrklWAw==", "license": "MIT", - "peer": true, "engines": { "node": ">= 6" } @@ -12710,6 +12737,7 @@ "resolved": "https://registry.npmjs.org/pg/-/pg-8.21.0.tgz", "integrity": "sha512-AUP1EYJuHraQGsVoCQVIcM7TEJVGtDzxWtGFZd8rds9d+CCXlU5Js1rYgfLNvxy9iJrpHjGrRjoi/3BT9fRyiA==", "license": "MIT", + "peer": true, "dependencies": { "pg-connection-string": "^2.13.0", "pg-pool": "^3.14.0", @@ -13039,6 +13067,7 @@ "integrity": "sha512-N2MylSdi48+5N/6S5j+maeHbUSIzzZ5uOcX5Hm4QpV8Dkb1HFjfAKTKX6yNPJQD9AhcT3ifHNB66tWTTJDi11Q==", "dev": true, "license": "MIT", + "peer": true, "bin": { "prettier": "bin/prettier.cjs" }, @@ -13754,7 +13783,8 @@ "version": "0.2.2", "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.2.2.tgz", "integrity": "sha512-urBwgfrvVP/eAyXx4hluJivBKzuEbSQs9rKWCrCkbSxNv8mxPcUZKeuoF3Uy4mJl3Lwprp6yy5/39VWigZ4K6Q==", - "license": "Apache-2.0" + "license": "Apache-2.0", + "peer": true }, "node_modules/require-directory": { "version": "2.1.1", @@ -13904,6 +13934,7 @@ "resolved": "https://registry.npmjs.org/rxjs/-/rxjs-7.8.2.tgz", "integrity": "sha512-dhKf903U/PQZY6boNNtAGdWbG85WAbjT/1xYoZIC7FAY0yWapOBQVsVrDl58W86//e1VpMNBtRV4MaXfdMySFA==", "license": "Apache-2.0", + "peer": true, "dependencies": { "tslib": "^2.1.0" } @@ -14504,6 +14535,7 @@ "integrity": "sha512-GGIyOiFaG+TUra3JIfkI/zGP8yZYLPQ0pl1bH+ODjiX57sPhrLU5sQJn1y9bDKZUFYkX1crlrPfSYt0BKKdkog==", "hasInstallScript": true, "license": "BSD-3-Clause", + "peer": true, "dependencies": { "bindings": "^1.5.0", "node-addon-api": "^7.0.0", @@ -15029,6 +15061,7 @@ "integrity": "sha512-Thbli+OlOj+iMPYFBVBfJ3OmCAnaSyNn4M1vz9T6Gka5Jt9ba/HIR56joy65tY6kx/FCF5VXNB819Y7/GUrBGA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "fast-deep-equal": "^3.1.3", "fast-uri": "^3.0.1", @@ -15397,6 +15430,7 @@ "integrity": "sha512-f0FFpIdcHgn8zcPSbf1dRevwt047YMnaiJM3u2w2RewrB+fob/zePZcrOyQoLMMO7aBIddLcQIEK5dYjkLnGrQ==", "devOptional": true, "license": "MIT", + "peer": true, "dependencies": { "@cspotcode/source-map-support": "^0.8.0", "@tsconfig/node10": "^1.0.7", @@ -15581,6 +15615,7 @@ "resolved": "https://registry.npmjs.org/typeorm/-/typeorm-0.3.30.tgz", "integrity": "sha512-8T35PzjefOdqc2ZR9mwLQj0pUGp6lQhMbK2EvVMwJVJWlaoHm0v/Q6dThNOZkFchD+0yMg8gwjKM28ePiLSXSQ==", "license": "MIT", + "peer": true, "dependencies": { "@sqltools/formatter": "^1.2.5", "ansis": "^4.2.0", @@ -15800,6 +15835,7 @@ "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", "devOptional": true, "license": "Apache-2.0", + "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -15870,6 +15906,15 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/undici": { + "version": "6.27.0", + "resolved": "https://registry.npmjs.org/undici/-/undici-6.27.0.tgz", + "integrity": "sha512-YmfV3YnEDzXRC5lZ2jWtWWHKGUm1zIt8AhesR1tens+HTNv+YZlN/dp6G727LOvMJ8xjP9Be7Y2Sdr96LDm+pg==", + "license": "MIT", + "engines": { + "node": ">=18.17" + } + }, "node_modules/undici-types": { "version": "7.24.6", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-7.24.6.tgz", @@ -16131,6 +16176,7 @@ "integrity": "sha512-wGN3qcrBQIFmQ/c0AiOAQBvrZ5lmY8vbbMv4Mxfgzqd/B6+9pXtLo73WuS1dSGXM5QYY3hZnIbvx+K1xxe6FyA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@types/eslint-scope": "^3.7.7", "@types/estree": "^1.0.8", @@ -16199,6 +16245,7 @@ "integrity": "sha512-Thbli+OlOj+iMPYFBVBfJ3OmCAnaSyNn4M1vz9T6Gka5Jt9ba/HIR56joy65tY6kx/FCF5VXNB819Y7/GUrBGA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "fast-deep-equal": "^3.1.3", "fast-uri": "^3.0.1", diff --git a/package.json b/package.json index 87f274b3..641ca9db 100644 --- a/package.json +++ b/package.json @@ -70,6 +70,7 @@ "sqlite3": "^5.1.7", "tar-stream": "^3.2.0", "typeorm": "^0.3.29", + "undici": "^6.27.0", "uuid": "^14.0.0", "whatsapp-web.js": "^1.34.7" }, diff --git a/src/common/media/load-remote-media.spec.ts b/src/common/media/load-remote-media.spec.ts index 757909b5..467a2d15 100644 --- a/src/common/media/load-remote-media.spec.ts +++ b/src/common/media/load-remote-media.spec.ts @@ -1,10 +1,16 @@ +import { fetch as undiciFetch } from 'undici'; import { loadRemoteMediaBuffer } from './load-remote-media'; import { SsrfBlockedError } from '../security/ssrf-guard'; +// Media download goes through undici's fetch (via the SSRF-pinning helper); mock it, not global fetch. +jest.mock('undici', () => { + const actual = jest.requireActual('undici'); + return { __esModule: true, ...actual, fetch: jest.fn() }; +}); + describe('loadRemoteMediaBuffer', () => { - const realFetch = global.fetch; afterEach(() => { - global.fetch = realFetch; + (undiciFetch as jest.Mock).mockReset(); delete process.env.MEDIA_DOWNLOAD_MAX_BYTES; }); @@ -28,28 +34,24 @@ describe('loadRemoteMediaBuffer', () => { }); it('blocks an internal URL via the SSRF guard before any fetch', async () => { - const fetchMock = jest.fn(); - global.fetch = fetchMock as typeof fetch; + const fetchMock = undiciFetch as jest.Mock; await expect(loadRemoteMediaBuffer('http://127.0.0.1/x.png')).rejects.toBeInstanceOf(SsrfBlockedError); expect(fetchMock).not.toHaveBeenCalled(); }); it('fetches a public URL and returns the bytes + content-type', async () => { - const fetchMock = jest - .fn() - .mockResolvedValue(fakeResponse([1, 2, 3], { 'content-type': 'image/png', 'content-length': '3' })); - global.fetch = fetchMock as typeof fetch; + const fetchMock = undiciFetch as jest.Mock; + fetchMock.mockResolvedValue(fakeResponse([1, 2, 3], { 'content-type': 'image/png', 'content-length': '3' })); const res = await loadRemoteMediaBuffer('http://8.8.8.8/x.png'); expect(res.mimetype).toBe('image/png'); expect(Array.from(res.data)).toEqual([1, 2, 3]); // Never follow redirects (a 3xx could reach an internal host the guard never validated). - expect(fetchMock).toHaveBeenCalledWith('http://8.8.8.8/x.png', expect.objectContaining({ redirect: 'error' })); + expect(fetchMock).toHaveBeenCalledWith('http://8.8.8.8/x.png', expect.objectContaining({ redirect: 'manual' })); }); it('rejects a body that exceeds the byte cap', async () => { process.env.MEDIA_DOWNLOAD_MAX_BYTES = '2'; - const fetchMock = jest.fn().mockResolvedValue(fakeResponse([1, 2, 3], { 'content-type': 'image/png' })); - global.fetch = fetchMock as typeof fetch; + (undiciFetch as jest.Mock).mockResolvedValue(fakeResponse([1, 2, 3], { 'content-type': 'image/png' })); await expect(loadRemoteMediaBuffer('http://8.8.8.8/x.png')).rejects.toThrow(/exceeds/i); }); }); diff --git a/src/common/media/load-remote-media.ts b/src/common/media/load-remote-media.ts index 4f0573d4..b57155ce 100644 --- a/src/common/media/load-remote-media.ts +++ b/src/common/media/load-remote-media.ts @@ -1,4 +1,4 @@ -import { assertSafeFetchUrl } from '../security/ssrf-guard'; +import { withSafeFetch } from '../security/ssrf-guard'; /** Default cap on a server-side media download: 50 MiB (overridable via MEDIA_DOWNLOAD_MAX_BYTES). */ const DEFAULT_MEDIA_MAX_BYTES = 50 * 1024 * 1024; @@ -13,48 +13,51 @@ function positiveIntFromEnv(name: string, fallback: number): number { /** * Fetch remote media as a Buffer for sending, with an SSRF host guard, a byte cap, and a timeout. * The guard runs BEFORE any network call, so an internal/reserved URL throws `SsrfBlockedError` - * and no outbound socket is opened. `redirect: 'error'` is used because the guard only validated - * the original host — a followed 3xx could reach an internal target. The cap is enforced while - * streaming (Content-Length may be absent or wrong) to bound memory use. + * and no outbound socket is opened. It goes through `withSafeFetch`, which pins the connection to + * the vetted IP and refuses redirects (the guard only validated the original host — a followed 3xx + * could reach an internal target). The cap is enforced while streaming (Content-Length may be absent + * or wrong) to bound memory use. * * Engine-neutral: returns raw bytes + the response content-type, so any engine adapter can use it. */ export async function loadRemoteMediaBuffer(url: string): Promise<{ data: Buffer; mimetype: string }> { - await assertSafeFetchUrl(url); - const maxBytes = positiveIntFromEnv('MEDIA_DOWNLOAD_MAX_BYTES', DEFAULT_MEDIA_MAX_BYTES); const timeoutMs = positiveIntFromEnv('MEDIA_DOWNLOAD_TIMEOUT_MS', DEFAULT_MEDIA_TIMEOUT_MS); - const response = await fetch(url, { redirect: 'error', signal: AbortSignal.timeout(timeoutMs) }); - if (!response.ok) { - throw new Error(`Media fetch failed with status ${response.status}`); - } - - const declaredLength = Number(response.headers.get('content-length') ?? ''); - if (Number.isFinite(declaredLength) && declaredLength > maxBytes) { - throw new Error(`Media exceeds the ${maxBytes}-byte limit`); - } - - const reader = response.body?.getReader(); - if (!reader) { - throw new Error('Media response has no body'); - } - - const chunks: Buffer[] = []; - let total = 0; - for (;;) { - const { done, value } = await reader.read(); - if (done) { - break; + // Always guarded (media SSRF is independent of the webhook opt-out); withSafeFetch validates the + // host, pins the connection to the vetted IP, and refuses redirects. The streaming cap runs inside + // the callback so the connection stays open for the body read and is torn down right after. + return withSafeFetch(url, { signal: AbortSignal.timeout(timeoutMs) }, async response => { + if (!response.ok) { + throw new Error(`Media fetch failed with status ${response.status}`); } - total += value.byteLength; - if (total > maxBytes) { - await reader.cancel(); + + const declaredLength = Number(response.headers.get('content-length') ?? ''); + if (Number.isFinite(declaredLength) && declaredLength > maxBytes) { throw new Error(`Media exceeds the ${maxBytes}-byte limit`); } - chunks.push(Buffer.from(value)); - } - const mimetype = (response.headers.get('content-type') ?? '').split(';')[0].trim(); - return { data: Buffer.concat(chunks), mimetype }; + const reader = response.body?.getReader(); + if (!reader) { + throw new Error('Media response has no body'); + } + + const chunks: Buffer[] = []; + let total = 0; + for (;;) { + const { done, value } = (await reader.read()) as { done: boolean; value: Uint8Array }; + if (done) { + break; + } + total += value.byteLength; + if (total > maxBytes) { + await reader.cancel(); + throw new Error(`Media exceeds the ${maxBytes}-byte limit`); + } + chunks.push(Buffer.from(value)); + } + + const mimetype = (response.headers.get('content-type') ?? '').split(';')[0].trim(); + return { data: Buffer.concat(chunks), mimetype }; + }); } diff --git a/src/common/security/ssrf-guard.spec.ts b/src/common/security/ssrf-guard.spec.ts index 42b11edf..8d07baff 100644 --- a/src/common/security/ssrf-guard.spec.ts +++ b/src/common/security/ssrf-guard.spec.ts @@ -4,7 +4,25 @@ import { assertNoRedirect, SsrfBlockedError, isSsrfProtectionEnabled, + resolveSafeFetchTarget, + pinnedLookup, + withSafeFetch, } from './ssrf-guard'; +import * as dnsPromises from 'dns/promises'; +import { fetch as undiciFetch } from 'undici'; + +// Default to the real resolver (so the localhost/real-DNS cases below behave normally); individual +// tests override a single call with mockResolvedValueOnce to simulate a specific resolution. +jest.mock('dns/promises', () => { + const actual = jest.requireActual('dns/promises'); + return { __esModule: true, ...actual, lookup: jest.fn(actual.lookup) }; +}); + +// Mock undici's fetch (keep the real Agent so withSafeFetch builds a real pinned dispatcher). +jest.mock('undici', () => { + const actual = jest.requireActual('undici'); + return { __esModule: true, ...actual, fetch: jest.fn() }; +}); describe('isBlockedAddress', () => { it.each([ @@ -56,6 +74,7 @@ describe('assertSafeFetchUrl', () => { }); it('rejects a hostname that resolves to loopback (localhost)', async () => { + (dnsPromises.lookup as jest.Mock).mockResolvedValueOnce([{ address: '127.0.0.1', family: 4 }]); await expect(assertSafeFetchUrl('http://localhost:9999/hook')).rejects.toThrow(SsrfBlockedError); }); @@ -115,6 +134,110 @@ describe('assertNoRedirect (redirect bypass)', () => { }); }); +describe('pinnedLookup (DNS-rebind defense)', () => { + it('returns the captured addresses and never re-resolves DNS (all: true)', () => { + const pinned = [{ address: '93.184.216.34', family: 4 }]; + const callback = jest.fn(); + pinnedLookup(pinned)('evil.example', { all: true }, callback); + expect(callback).toHaveBeenCalledWith(null, pinned); + }); + + it('returns the first captured address in single-result form (all: false)', () => { + const pinned = [ + { address: '93.184.216.34', family: 4 }, + { address: '93.184.216.35', family: 4 }, + ]; + const callback = jest.fn(); + pinnedLookup(pinned)('evil.example', { all: false }, callback); + expect(callback).toHaveBeenCalledWith(null, '93.184.216.34', 4); + }); +}); + +describe('resolveSafeFetchTarget', () => { + const orig = process.env.SSRF_ALLOWED_HOSTS; + afterEach(() => { + if (orig === undefined) delete process.env.SSRF_ALLOWED_HOSTS; + else process.env.SSRF_ALLOWED_HOSTS = orig; + }); + + it('returns null for a public literal IP (no hostname to rebind)', async () => { + await expect(resolveSafeFetchTarget('https://8.8.8.8/hook')).resolves.toBeNull(); + }); + + it('returns null for an allowlisted host (trusted, no pin needed)', async () => { + process.env.SSRF_ALLOWED_HOSTS = 'minio'; + await expect(resolveSafeFetchTarget('http://minio:9000/x.png')).resolves.toBeNull(); + }); + + it('throws for a blocked literal address', async () => { + await expect(resolveSafeFetchTarget('http://127.0.0.1/x')).rejects.toThrow(SsrfBlockedError); + }); + + it('returns the resolved public addresses for a hostname (the IPs to pin to)', async () => { + (dnsPromises.lookup as jest.Mock).mockResolvedValueOnce([{ address: '93.184.216.34', family: 4 }]); + await expect(resolveSafeFetchTarget('https://example.com/hook')).resolves.toEqual([ + { address: '93.184.216.34', family: 4 }, + ]); + }); + + it('throws when a hostname resolves to a blocked address', async () => { + (dnsPromises.lookup as jest.Mock).mockResolvedValueOnce([{ address: '10.0.0.5', family: 4 }]); + await expect(resolveSafeFetchTarget('https://rebind.example/hook')).rejects.toThrow(SsrfBlockedError); + }); +}); + +describe('withSafeFetch (guarded + pinned fetch)', () => { + afterEach(() => { + (undiciFetch as jest.Mock).mockReset(); + }); + + it('rejects a blocked host before performing any fetch (fail-closed)', async () => { + const use = jest.fn(); + await expect(withSafeFetch('http://127.0.0.1/hook', {}, use, { guard: true })).rejects.toThrow(SsrfBlockedError); + expect(use).not.toHaveBeenCalled(); + }); + + it('pins the connection by passing a dispatcher to fetch for a hostname target', async () => { + // The security property: for a DNS hostname the connection MUST go through a pinned dispatcher, + // else fetch re-resolves DNS independently and the rebind window reopens. Removing the pin + // (dispatcher = undefined) makes this fail. + (dnsPromises.lookup as jest.Mock).mockResolvedValueOnce([{ address: '93.184.216.34', family: 4 }]); + (undiciFetch as jest.Mock).mockResolvedValue({ status: 200, type: 'basic' }); + const use = jest.fn(() => 'used'); + + const result = await withSafeFetch('https://example.com/hook', { method: 'POST' }, use, { guard: true }); + + expect(result).toBe('used'); + expect(use).toHaveBeenCalledTimes(1); + const [url, init] = (undiciFetch as jest.Mock).mock.calls[0] as [string, { redirect: string; dispatcher: unknown }]; + expect(url).toBe('https://example.com/hook'); + expect(init.redirect).toBe('manual'); + expect(init.dispatcher).toBeDefined(); + }); + + it('refuses a redirect on the pinned path (real undici manual-redirect shape: 302/basic)', async () => { + (dnsPromises.lookup as jest.Mock).mockResolvedValueOnce([{ address: '93.184.216.34', family: 4 }]); + (undiciFetch as jest.Mock).mockResolvedValue({ status: 302, type: 'basic' }); + + await expect(withSafeFetch('https://example.com/hook', {}, jest.fn(), { guard: true })).rejects.toThrow( + SsrfBlockedError, + ); + }); + + it('skips validation and pinning entirely when guard is false (SSRF opt-out)', async () => { + (undiciFetch as jest.Mock).mockResolvedValue({ status: 200, type: 'basic' }); + const use = jest.fn(() => 'ok'); + + // An internal host would normally be blocked — with guard:false it is delivered unpinned. + const result = await withSafeFetch('http://127.0.0.1/hook', {}, use, { guard: false }); + + expect(result).toBe('ok'); + const [, init] = (undiciFetch as jest.Mock).mock.calls[0] as [string, { redirect: string; dispatcher: unknown }]; + expect(init.redirect).toBe('follow'); + expect(init.dispatcher).toBeUndefined(); + }); +}); + describe('isSsrfProtectionEnabled', () => { const orig = process.env.WEBHOOK_SSRF_PROTECT; afterEach(() => { diff --git a/src/common/security/ssrf-guard.ts b/src/common/security/ssrf-guard.ts index 94507527..d670c0c0 100644 --- a/src/common/security/ssrf-guard.ts +++ b/src/common/security/ssrf-guard.ts @@ -1,5 +1,7 @@ -import { isIPv4, isIPv6 } from 'net'; +import { isIPv4, isIPv6, type LookupFunction } from 'net'; import { lookup } from 'dns/promises'; +import { type LookupAddress, type LookupOptions } from 'dns'; +import { Agent, fetch as undiciFetch, type RequestInit, type Response } from 'undici'; /** Thrown when an outbound URL is blocked by the SSRF guard. */ export class SsrfBlockedError extends Error { @@ -119,12 +121,18 @@ export function assertNoRedirect(response: { status: number; type?: string }, ur } /** - * Resolves an outbound URL and throws SsrfBlockedError if its scheme is not - * http(s) or if the host (literal or any DNS-resolved address) is internal/reserved. - * Guards both webhook delivery and server-side media fetches. Hosts named in - * `SSRF_ALLOWED_HOSTS` are allowed through (escape-hatch for trusted internal targets). + * Validate an outbound URL and resolve its host ONCE. Throws SsrfBlockedError if the scheme is not + * http(s) or if the host (literal or any DNS-resolved address) is internal/reserved. Guards both + * webhook delivery and server-side media fetches. Hosts named in `SSRF_ALLOWED_HOSTS` are allowed + * through (escape-hatch for trusted internal targets). + * + * Returns the vetted resolved addresses so a caller can PIN the connection to them — defeating the + * DNS-rebinding window where the address validated here differs from the one `fetch` would re-resolve. + * Returns null when there is nothing to pin: an allowlisted host (trusted — deliberately left + * unpinned, since the operator opts in to whatever its DNS returns) or a literal IP (no DNS, so no + * rebind is possible — fetch connects straight to the validated literal). */ -export async function assertSafeFetchUrl(rawUrl: string): Promise { +export async function resolveSafeFetchTarget(rawUrl: string): Promise { let url: URL; try { url = new URL(rawUrl); @@ -139,14 +147,14 @@ export async function assertSafeFetchUrl(rawUrl: string): Promise { const host = url.hostname.replace(/^\[|\]$/g, ''); // strip IPv6 brackets if (getAllowedHosts().has(host.toLowerCase())) { - return; // explicitly allowlisted internal target + return null; // explicitly allowlisted internal target } if (isIPv4(host) || isIPv6(host)) { if (isBlockedAddress(host)) { throw new SsrfBlockedError(`Blocked internal address: ${host}`); } - return; + return null; // literal IP — fetch connects directly, nothing to rebind } const resolved = await lookup(host, { all: true }); @@ -158,4 +166,66 @@ export async function assertSafeFetchUrl(rawUrl: string): Promise { throw new SsrfBlockedError(`Host ${host} resolves to a blocked internal address: ${address}`); } } + return resolved; // vetted addresses — pin the connection to these +} + +/** + * Backwards-compatible assertion form: validate the URL (used at webhook registration time, where + * only the throw/no-throw outcome matters). + */ +export async function assertSafeFetchUrl(rawUrl: string): Promise { + await resolveSafeFetchTarget(rawUrl); +} + +/** + * Build a `net`-style lookup function that always returns the pre-validated addresses and never + * consults DNS — so a connection using it cannot be re-resolved to a different (internal) address. + */ +export function pinnedLookup(addresses: LookupAddress[]): LookupFunction { + // undici always invokes the lookup with an options object; `all: true` expects the address array, + // otherwise a single (address, family) pair. + const fn = (_hostname: string, options: LookupOptions, callback: (...args: unknown[]) => void): void => { + if (options.all) { + callback(null, addresses); + } else { + callback(null, addresses[0].address, addresses[0].family); + } + }; + return fn as unknown as LookupFunction; +} + +/** + * Perform an SSRF-safe fetch and hand the response to `use`, then tear down the per-request + * connection. The host is validated and resolved ONCE; the connection is pinned to the vetted IP(s) + * via an undici dispatcher so it cannot be re-resolved to an internal address between check and + * connect (DNS-rebinding TOCTOU). The original hostname is preserved for TLS SNI and the Host header, + * so virtual hosting and certificate validation are unaffected, and ALL vetted addresses are offered + * so A-record failover still works. Redirects are refused (the guard only validated the original host). + * + * `use` must read everything it needs from the response before returning — the dispatcher (and its + * sockets) is destroyed once `use` settles, so a still-streaming body would be cut off. + * + * @param opts.guard - when false (the WEBHOOK_SSRF_PROTECT opt-out), skips validation/pinning and + * performs a plain redirect-following fetch. Defaults to true (always guard). + */ +export async function withSafeFetch( + rawUrl: string, + init: RequestInit, + use: (response: Response) => Promise | T, + opts: { guard?: boolean } = {}, +): Promise { + const guard = opts.guard ?? true; + if (!guard) { + return use(await undiciFetch(rawUrl, { ...init, redirect: 'follow' })); + } + + const target = await resolveSafeFetchTarget(rawUrl); + const dispatcher = target ? new Agent({ connect: { lookup: pinnedLookup(target) } }) : undefined; + try { + const response = await undiciFetch(rawUrl, { ...init, redirect: 'manual', dispatcher }); + assertNoRedirect(response, rawUrl); + return await use(response); + } finally { + if (dispatcher) await dispatcher.destroy().catch(() => undefined); + } } diff --git a/src/engine/adapters/whatsapp-web-js.adapter.spec.ts b/src/engine/adapters/whatsapp-web-js.adapter.spec.ts index 3519d676..aeb2e0e2 100644 --- a/src/engine/adapters/whatsapp-web-js.adapter.spec.ts +++ b/src/engine/adapters/whatsapp-web-js.adapter.spec.ts @@ -9,6 +9,14 @@ import { import { EngineNotReadyError } from '../../common/errors/engine-not-ready.error'; import { EngineStatus } from '../interfaces/whatsapp-engine.interface'; import { SsrfBlockedError } from '../../common/security/ssrf-guard'; +import { fetch as undiciFetch } from 'undici'; + +// loadRemoteMedia now fetches bytes through the SSRF-pinned path (undici fetch), then builds the +// MessageMedia locally — so mock undici fetch, not MessageMedia.fromUrl. +jest.mock('undici', () => { + const actual = jest.requireActual('undici'); + return { __esModule: true, ...actual, fetch: jest.fn() }; +}); describe('wwebjsAckToDeliveryStatus (engine ack-int -> neutral DeliveryStatus boundary, #265)', () => { // Regression-locks the integer boundary the decoupling moved behaviour into, incl. the @@ -62,22 +70,57 @@ describe('extractLinkedParentJID (#201)', () => { }); }); -describe('loadRemoteMedia — media-fetch SSRF guard + cap + timeout', () => { +describe('loadRemoteMedia — routes through the SSRF-pinned media fetch', () => { let fromUrlSpy: jest.SpyInstance; + // A Response-like with a single-chunk body stream (mirrors load-remote-media.spec). + const fakeResponse = (bytes: number[], headers: Record) => ({ + ok: true, + status: 200, + headers: { get: (k: string) => headers[k.toLowerCase()] ?? null }, + body: { + getReader: () => { + let done = false; + return { + read: () => + done + ? Promise.resolve({ done: true, value: undefined }) + : ((done = true), Promise.resolve({ done: false, value: new Uint8Array(bytes) })), + cancel: () => Promise.resolve(), + }; + }, + }, + }); + beforeEach(() => { - fromUrlSpy = jest - .spyOn(MessageMedia, 'fromUrl') - .mockResolvedValue(new MessageMedia('image/png', 'ZmFrZQ==', 'x.png')); + // Spied only to assert the vulnerable fromUrl path is NEVER taken. + fromUrlSpy = jest.spyOn(MessageMedia, 'fromUrl'); + (undiciFetch as jest.Mock).mockReset(); }); afterEach(() => { fromUrlSpy.mockRestore(); + (undiciFetch as jest.Mock).mockReset(); delete process.env.SSRF_ALLOWED_HOSTS; }); + it('builds MessageMedia from the pinned fetch bytes, never via MessageMedia.fromUrl', async () => { + (undiciFetch as jest.Mock).mockResolvedValue(fakeResponse([104, 105], { 'content-type': 'image/png' })); + + const media = await loadRemoteMedia('https://8.8.8.8/x.png'); + + expect(fromUrlSpy).not.toHaveBeenCalled(); // the unpinned node-fetch path is gone + expect(media.mimetype).toBe('image/png'); + expect(media.data).toBe(Buffer.from([104, 105]).toString('base64')); + expect(undiciFetch).toHaveBeenCalledWith( + 'https://8.8.8.8/x.png', + expect.objectContaining({ redirect: 'manual' }), // pinned + redirects refused + ); + }); + it('blocks an internal/loopback URL BEFORE any fetch (no outbound socket)', async () => { await expect(loadRemoteMedia('http://127.0.0.1/x.png')).rejects.toBeInstanceOf(SsrfBlockedError); + expect(undiciFetch).not.toHaveBeenCalled(); expect(fromUrlSpy).not.toHaveBeenCalled(); }); @@ -85,28 +128,17 @@ describe('loadRemoteMedia — media-fetch SSRF guard + cap + timeout', () => { await expect(loadRemoteMedia('http://169.254.169.254/latest/meta-data/x.png')).rejects.toBeInstanceOf( SsrfBlockedError, ); - expect(fromUrlSpy).not.toHaveBeenCalled(); - }); - - it('fetches a public URL with a byte cap and an abort-timeout signal', async () => { - await loadRemoteMedia('https://8.8.8.8/x.png'); - - expect(fromUrlSpy).toHaveBeenCalledTimes(1); - const [url, options] = fromUrlSpy.mock.calls[0] as [ - string, - { reqOptions: { size: number; signal: unknown; redirect: string } }, - ]; - expect(url).toBe('https://8.8.8.8/x.png'); - expect(typeof options.reqOptions.size).toBe('number'); - expect(options.reqOptions.size).toBeGreaterThan(0); - expect(options.reqOptions.signal).toBeInstanceOf(AbortSignal); - expect(options.reqOptions.redirect).toBe('error'); // never follow redirects + expect(undiciFetch).not.toHaveBeenCalled(); }); it('honors the SSRF_ALLOWED_HOSTS escape-hatch for trusted internal media stores', async () => { process.env.SSRF_ALLOWED_HOSTS = 'minio'; - await loadRemoteMedia('http://minio:9000/bucket/x.png'); - expect(fromUrlSpy).toHaveBeenCalledTimes(1); + (undiciFetch as jest.Mock).mockResolvedValue(fakeResponse([1], { 'content-type': 'image/png' })); + + const media = await loadRemoteMedia('http://minio:9000/bucket/x.png'); + + expect(media.mimetype).toBe('image/png'); + expect(fromUrlSpy).not.toHaveBeenCalled(); }); }); diff --git a/src/engine/adapters/whatsapp-web-js.adapter.ts b/src/engine/adapters/whatsapp-web-js.adapter.ts index 4c940c3d..590b33f8 100644 --- a/src/engine/adapters/whatsapp-web-js.adapter.ts +++ b/src/engine/adapters/whatsapp-web-js.adapter.ts @@ -35,7 +35,7 @@ import { import { createLogger } from '../../common/services/logger.service'; import { EngineNotReadyError } from '../../common/errors/engine-not-ready.error'; import { MessageNotFoundError } from '../../common/errors/message-not-found.error'; -import { assertSafeFetchUrl } from '../../common/security/ssrf-guard'; +import { loadRemoteMediaBuffer } from '../../common/media/load-remote-media'; import { GroupChat, GroupMetadataRaw, @@ -47,16 +47,6 @@ import { import { buildIncomingMessageBase } from './message-mapper'; import { capInboundMedia } from './inbound-media-cap'; -/** Default cap on a server-side media download: 50 MiB (overridable via MEDIA_DOWNLOAD_MAX_BYTES). */ -const DEFAULT_MEDIA_MAX_BYTES = 50 * 1024 * 1024; -/** Default timeout for a server-side media download: 30s (overridable via MEDIA_DOWNLOAD_TIMEOUT_MS). */ -const DEFAULT_MEDIA_TIMEOUT_MS = 30_000; - -function positiveIntFromEnv(name: string, fallback: number): number { - const parsed = Number.parseInt(process.env[name] ?? '', 10); - return Number.isInteger(parsed) && parsed > 0 ? parsed : fallback; -} - /** * Map a whatsapp-web.js MessageAck integer to the neutral DeliveryStatus. * wwebjs: -1 ERROR, 0 PENDING, 1 SERVER (sent), 2 DEVICE (delivered), 3 READ, 4 PLAYED. @@ -78,16 +68,13 @@ export function wwebjsAckToDeliveryStatus(ack: number): DeliveryStatus { * existing MIME-detection behavior. */ export async function loadRemoteMedia(url: string): Promise { - await assertSafeFetchUrl(url); - return MessageMedia.fromUrl(url, { - reqOptions: { - size: positiveIntFromEnv('MEDIA_DOWNLOAD_MAX_BYTES', DEFAULT_MEDIA_MAX_BYTES), - signal: AbortSignal.timeout(positiveIntFromEnv('MEDIA_DOWNLOAD_TIMEOUT_MS', DEFAULT_MEDIA_TIMEOUT_MS)), - // Never follow redirects: the SSRF guard only validated the original host, so a - // followed 3xx could reach an internal target. node-fetch rejects on redirect. - redirect: 'error', - }, - }); + // Fetch through the SSRF-pinned path: it validates the host, pins the connection to the vetted IP + // (so a DNS rebind can't redirect it to an internal target between check and connect), caps bytes, + // and refuses redirects. We then build the MessageMedia from the returned bytes — NOT via + // MessageMedia.fromUrl, whose bundled node-fetch performs its own unpinned DNS re-resolution. + const { data, mimetype } = await loadRemoteMediaBuffer(url); + const filename = new URL(url).pathname.split('/').pop() || undefined; + return new MessageMedia(mimetype || 'application/octet-stream', data.toString('base64'), filename); } export interface WhatsAppWebJsConfig { diff --git a/src/modules/queue/processors/webhook.processor.spec.ts b/src/modules/queue/processors/webhook.processor.spec.ts index ee1558b3..600e53f1 100644 --- a/src/modules/queue/processors/webhook.processor.spec.ts +++ b/src/modules/queue/processors/webhook.processor.spec.ts @@ -4,6 +4,13 @@ import { WebhookProcessor } from './webhook.processor'; import { Webhook } from '../../webhook/entities/webhook.entity'; import { HookManager } from '../../../core/hooks'; import { WebhookJobData } from '../../webhook/webhook.service'; +import { fetch as undiciFetch } from 'undici'; + +// Delivery goes through undici's fetch (via the SSRF-pinning helper), so mock that, not global fetch. +jest.mock('undici', () => { + const actual = jest.requireActual('undici'); + return { __esModule: true, ...actual, fetch: jest.fn() }; +}); /** * Regression coverage for the production (QUEUE_ENABLED) webhook delivery path, which was @@ -45,8 +52,7 @@ describe('WebhookProcessor', () => { repo = { update: jest.fn().mockResolvedValue({ affected: 1 }) }; hookManager = { execute: jest.fn().mockResolvedValue({ continue: true, data: {} }) }; processor = new WebhookProcessor(repo as unknown as Repository, hookManager as unknown as HookManager); - mockFetch = jest.fn(); - global.fetch = mockFetch as typeof global.fetch; + mockFetch = undiciFetch as jest.Mock; process.env.WEBHOOK_SSRF_PROTECT = 'false'; // delivery-logic tests; redirect test flips it on }); diff --git a/src/modules/queue/processors/webhook.processor.ts b/src/modules/queue/processors/webhook.processor.ts index b0c70e75..08d9864b 100644 --- a/src/modules/queue/processors/webhook.processor.ts +++ b/src/modules/queue/processors/webhook.processor.ts @@ -7,7 +7,7 @@ import { QUEUE_NAMES } from '../queue-names'; import { WebhookJobData } from '../../webhook/webhook.service'; import { Webhook } from '../../webhook/entities/webhook.entity'; import { HookManager } from '../../../core/hooks'; -import { assertSafeFetchUrl, assertNoRedirect, isSsrfProtectionEnabled } from '../../../common/security/ssrf-guard'; +import { withSafeFetch, isSsrfProtectionEnabled } from '../../../common/security/ssrf-guard'; export interface WebhookJobResult { statusCode: number; @@ -48,27 +48,23 @@ export class WebhookProcessor extends WorkerHost { 'X-OpenWA-Retry-Count': String(job.attemptsMade), }; - const ssrfProtected = isSsrfProtectionEnabled(); try { - if (ssrfProtected) { - await assertSafeFetchUrl(url); - } - const response = await fetch(url, { - method: 'POST', - headers: requestHeaders, - body: JSON.stringify(payload), - signal: AbortSignal.timeout(10000), - redirect: ssrfProtected ? 'manual' : 'follow', - }); - if (ssrfProtected) { - assertNoRedirect(response, url); - } + const { status, statusText, ok } = await withSafeFetch( + url, + { + method: 'POST', + headers: requestHeaders, + body: JSON.stringify(payload), + signal: AbortSignal.timeout(10000), + }, + response => ({ status: response.status, statusText: response.statusText, ok: response.ok }), + { guard: isSsrfProtectionEnabled() }, + ); const responseTime = Date.now() - startTime; - const success = response.ok; - if (!success) { - throw new Error(`HTTP ${response.status}: ${response.statusText}`); + if (!ok) { + throw new Error(`HTTP ${status}: ${statusText}`); } // Update lastTriggeredAt on successful delivery @@ -84,7 +80,7 @@ export class WebhookProcessor extends WorkerHost { event, webhookId, deliveryId: payload.deliveryId, - statusCode: response.status, + statusCode: status, responseTime, attempt: job.attemptsMade + 1, }, @@ -96,14 +92,14 @@ export class WebhookProcessor extends WorkerHost { event, deliveryId: payload.deliveryId, idempotencyKey: payload.idempotencyKey, - statusCode: response.status, + statusCode: status, responseTime, attempt: job.attemptsMade + 1, action: 'webhook_delivered', }); return { - statusCode: response.status, + statusCode: status, success: true, responseTime, }; diff --git a/src/modules/webhook/webhook.service.spec.ts b/src/modules/webhook/webhook.service.spec.ts index 9f47d197..17e10013 100644 --- a/src/modules/webhook/webhook.service.spec.ts +++ b/src/modules/webhook/webhook.service.spec.ts @@ -4,6 +4,12 @@ jest.mock('dns/promises', () => ({ lookup: jest.fn().mockResolvedValue([{ address: '93.184.216.34', family: 4 }]), })); +// Webhook delivery goes through undici's fetch (via the SSRF-pinning helper); mock it, not global fetch. +jest.mock('undici', () => { + const actual = jest.requireActual('undici'); + return { __esModule: true, ...actual, fetch: jest.fn() }; +}); + import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; import { getQueueToken } from '@nestjs/bullmq'; @@ -11,6 +17,7 @@ import { Repository } from 'typeorm'; import { NotFoundException, BadRequestException } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import * as crypto from 'crypto'; +import { fetch as undiciFetch } from 'undici'; import { WebhookService, WebhookPayload } from './webhook.service'; import { Webhook } from './entities/webhook.entity'; import { HookManager } from '../../core/hooks'; @@ -232,10 +239,9 @@ describe('WebhookService', () => { // ── dispatch (direct mode — queue disabled) ─────────────────────── describe('dispatch (direct mode)', () => { - const mockFetch = jest.fn(); + const mockFetch = undiciFetch as jest.Mock; beforeEach(() => { - global.fetch = mockFetch as typeof global.fetch; mockFetch.mockResolvedValue({ ok: true, status: 200 }); }); @@ -340,11 +346,11 @@ describe('WebhookService', () => { (repository.update as jest.Mock).mockResolvedValue({ affected: 1 }); const captured: Record = {}; - const mockFetch = jest.fn().mockImplementation((_url: string, opts: RequestInit) => { + const mockFetch = undiciFetch as jest.Mock; + mockFetch.mockImplementation((_url: string, opts: RequestInit) => { Object.assign(captured, opts.headers as Record); return Promise.resolve({ ok: true, status: 200 }); }); - global.fetch = mockFetch as typeof global.fetch; const payload: WebhookPayload = { event: 'message.received', @@ -371,11 +377,10 @@ describe('WebhookService', () => { // ── redirect refusal ───────────────────────────────────────── describe('dispatch — redirect refusal', () => { - const mockFetch = jest.fn(); + const mockFetch = undiciFetch as jest.Mock; const origProtect = process.env.WEBHOOK_SSRF_PROTECT; beforeEach(() => { - global.fetch = mockFetch as typeof global.fetch; process.env.WEBHOOK_SSRF_PROTECT = 'true'; }); @@ -433,11 +438,11 @@ describe('WebhookService', () => { (repository.update as jest.Mock).mockResolvedValue({ affected: 1 }); const capturedHeaders: Record = {}; - const mockFetch = jest.fn().mockImplementation((_url: string, opts: RequestInit) => { + const mockFetch = undiciFetch as jest.Mock; + mockFetch.mockImplementation((_url: string, opts: RequestInit) => { Object.assign(capturedHeaders, opts.headers as Record); return Promise.resolve({ ok: true, status: 200 }); }); - global.fetch = mockFetch as typeof global.fetch; const sigPayload: WebhookPayload = { event: 'message.received', diff --git a/src/modules/webhook/webhook.service.ts b/src/modules/webhook/webhook.service.ts index b4647225..7cb42b61 100644 --- a/src/modules/webhook/webhook.service.ts +++ b/src/modules/webhook/webhook.service.ts @@ -12,7 +12,7 @@ import { QUEUE_NAMES } from '../queue/queue-names'; import { generateIdempotencyKey, generateDeliveryId } from './utils/idempotency.util'; import { assertSafeFetchUrl, - assertNoRedirect, + withSafeFetch, isSsrfProtectionEnabled, SsrfBlockedError, } from '../../common/security/ssrf-guard'; @@ -168,26 +168,13 @@ export class WebhookService { headers['X-OpenWA-Signature'] = this.generateSignature(body, webhook.secret); } - const ssrfProtected = isSsrfProtectionEnabled(); try { - if (ssrfProtected) { - await assertSafeFetchUrl(webhook.url); - } - const response = await fetch(webhook.url, { - method: 'POST', - headers, - body, - signal: AbortSignal.timeout(10000), - redirect: ssrfProtected ? 'manual' : 'follow', - }); - if (ssrfProtected) { - assertNoRedirect(response, webhook.url); - } - - return { - success: response.ok, - statusCode: response.status, - }; + return await withSafeFetch( + webhook.url, + { method: 'POST', headers, body, signal: AbortSignal.timeout(10000) }, + response => ({ success: response.ok, statusCode: response.status }), + { guard: isSsrfProtectionEnabled() }, + ); } catch (error) { return { success: false, @@ -372,24 +359,21 @@ export class WebhookService { headers['X-OpenWA-Signature'] = this.generateSignature(body, webhook.secret); } - const ssrfProtected = isSsrfProtectionEnabled(); try { - if (ssrfProtected) { - await assertSafeFetchUrl(webhook.url); - } - const response = await fetch(webhook.url, { - method: 'POST', - headers, - body, - signal: AbortSignal.timeout(this.configService.get('webhook.timeout', 10000)), - redirect: ssrfProtected ? 'manual' : 'follow', - }); - if (ssrfProtected) { - assertNoRedirect(response, webhook.url); - } + const { ok, status, statusText } = await withSafeFetch( + webhook.url, + { + method: 'POST', + headers, + body, + signal: AbortSignal.timeout(this.configService.get('webhook.timeout', 10000)), + }, + response => ({ ok: response.ok, status: response.status, statusText: response.statusText }), + { guard: isSsrfProtectionEnabled() }, + ); - if (!response.ok) { - throw new Error(`HTTP ${response.status}: ${response.statusText}`); + if (!ok) { + throw new Error(`HTTP ${status}: ${statusText}`); } // Update last triggered timestamp From b487725556789cde770e16119addac23c95d527c Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:24:30 +0700 Subject: [PATCH 03/14] fix(plugins): persist plugin enable/config and restore (#339) --- .../plugins/plugin-loader.service.spec.ts | 96 ++++++++++++++++++- src/core/plugins/plugin-loader.service.ts | 47 ++++++++- src/core/plugins/plugin-storage.service.ts | 11 ++- 3 files changed, 146 insertions(+), 8 deletions(-) diff --git a/src/core/plugins/plugin-loader.service.spec.ts b/src/core/plugins/plugin-loader.service.spec.ts index 18f0fbd4..a625aebd 100644 --- a/src/core/plugins/plugin-loader.service.spec.ts +++ b/src/core/plugins/plugin-loader.service.spec.ts @@ -25,17 +25,23 @@ describe('resolvePluginMainPath', () => { }); }); +import * as fs from 'fs'; +import * as os from 'os'; import { PluginLoaderService } from './plugin-loader.service'; import { ConfigService } from '@nestjs/config'; import { ModuleRef } from '@nestjs/core'; import { HookManager } from '../hooks'; import { PluginStorageService } from './plugin-storage.service'; -import { IPlugin, PluginManifest, PluginType } from './plugin.interfaces'; +import { IPlugin, PluginManifest, PluginStatus, PluginType } from './plugin.interfaces'; describe('PluginLoaderService.registerBuiltInPlugin config', () => { function makeLoader(): PluginLoaderService { const configService = { get: jest.fn().mockReturnValue(undefined) } as unknown as ConfigService; - const pluginStorage = {} as unknown as PluginStorageService; + const pluginStorage = { + getPluginEntry: jest.fn().mockReturnValue(undefined), + setPluginEntry: jest.fn(), + getPluginConfig: jest.fn().mockReturnValue(null), + } as unknown as PluginStorageService; return new PluginLoaderService(configService, new HookManager(), pluginStorage, {} as unknown as ModuleRef); } const manifest: PluginManifest = { @@ -59,3 +65,89 @@ describe('PluginLoaderService.registerBuiltInPlugin config', () => { expect(loader.getPlugin('cfg-test')?.config).toEqual({}); }); }); + +describe('PluginLoaderService — enable/config persistence', () => { + let tmpDir: string; + let config: ConfigService; + let storage: PluginStorageService; + let loader: PluginLoaderService; + + const manifest: PluginManifest = { + id: 'persist-test', + name: 'Persist Test', + version: '1.0.0', + type: PluginType.EXTENSION, + main: 'index.js', + }; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'owa-plugin-')); + config = { get: (k: string) => (k === 'dataDir' ? tmpDir : undefined) } as unknown as ConfigService; + storage = new PluginStorageService(config); + loader = new PluginLoaderService(config, new HookManager(), storage, {} as unknown as ModuleRef); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('creates a complete INSTALLED registry entry on register so a status write persists across a restart', () => { + loader.registerBuiltInPlugin(manifest, {}, { apiKey: 'default' }); + const entry = storage.getPluginEntry('persist-test'); + expect(entry).toMatchObject({ + id: 'persist-test', + status: PluginStatus.INSTALLED, + builtIn: true, + }); + + // The status write now lands (previously a silent no-op because no entry existed). + storage.setPluginStatus('persist-test', PluginStatus.ENABLED); + + // Durable: a fresh storage instance re-reads registry.json (simulates a restart). + expect(new PluginStorageService(config).getPluginStatus('persist-test')).toBe(PluginStatus.ENABLED); + }); + + it('keeps using live env config for a built-in across restarts (the first snapshot must not freeze it)', () => { + // Boot 1: register with one env-derived default, no operator edit. + loader.registerBuiltInPlugin(manifest, {}, { execPath: '/old/chromium', headless: true }); + + // Boot 2: env changed (e.g. operator set PUPPETEER_EXECUTABLE_PATH on a new image) → the live value wins. + const storage2 = new PluginStorageService(config); + const loader2 = new PluginLoaderService(config, new HookManager(), storage2, {} as unknown as ModuleRef); + loader2.registerBuiltInPlugin(manifest, {}, { execPath: '/new/chromium', headless: true }); + + expect(loader2.getPlugin('persist-test')?.config).toEqual({ execPath: '/new/chromium', headless: true }); + }); + + it('reports a re-registered plugin as installed after restart even if it was enabled (no boot auto-enable, no divergence)', () => { + loader.registerBuiltInPlugin(manifest, {}, {}); + storage.setPluginStatus('persist-test', PluginStatus.ENABLED); // operator enabled it + + // Restart: re-register the built-in. + const storage2 = new PluginStorageService(config); + const loader2 = new PluginLoaderService(config, new HookManager(), storage2, {} as unknown as ModuleRef); + loader2.registerBuiltInPlugin(manifest, {}, {}); + + // Runtime is INSTALLED (not auto-enabled) AND the registry agrees (no enabled/installed divergence). + expect(loader2.getPlugin('persist-test')?.status).toBe(PluginStatus.INSTALLED); + expect(storage2.getPluginStatus('persist-test')).toBe(PluginStatus.INSTALLED); + }); + + it('writes registry.json without group/other access (plugin config can hold secrets)', () => { + loader.registerBuiltInPlugin(manifest, {}, { apiKey: 'secret' }); + const mode = fs.statSync(path.join(tmpDir, 'plugins', 'registry.json')).mode & 0o777; + expect(mode & 0o077).toBe(0); + }); + + it('restores the operator config on the next load instead of resetting to the default', () => { + loader.registerBuiltInPlugin(manifest, {}, { apiKey: 'default' }); + loader.updatePluginConfig('persist-test', { apiKey: 'operator-secret' }); + expect(storage.getPluginConfig('persist-test')).toEqual({ apiKey: 'operator-secret' }); + + // Restart: re-register the built-in with its default config — the persisted operator config wins. + const storage2 = new PluginStorageService(config); + const loader2 = new PluginLoaderService(config, new HookManager(), storage2, {} as unknown as ModuleRef); + loader2.registerBuiltInPlugin(manifest, {}, { apiKey: 'default' }); + expect(loader2.getPlugin('persist-test')?.config).toEqual({ apiKey: 'operator-secret' }); + }); +}); diff --git a/src/core/plugins/plugin-loader.service.ts b/src/core/plugins/plugin-loader.service.ts index 35afc3f9..81253fee 100644 --- a/src/core/plugins/plugin-loader.service.ts +++ b/src/core/plugins/plugin-loader.service.ts @@ -121,19 +121,22 @@ export class PluginLoaderService implements OnModuleInit { throw new Error(`Plugin ${manifest.id} is already loaded`); } - // Load stored config - const storedConfig = this.pluginStorage.getPluginConfig(manifest.id); + // Load any persisted config so an operator's settings survive a restart. + const storedConfig = this.pluginStorage.getPluginConfig(manifest.id) ?? {}; const pluginInstance: PluginInstance = { manifest, status: PluginStatus.INSTALLED, - config: storedConfig ?? {}, + config: storedConfig, instance: null, loadedAt: new Date(), }; this.plugins.set(manifest.id, pluginInstance); + // Ensure a registry entry exists so later enable/disable/config writes persist. + this.ensureRegistryEntry(manifest, false); + this.logger.log(`Plugin loaded: ${manifest.name} v${manifest.version}`, { pluginId: manifest.id, type: manifest.type, @@ -143,6 +146,34 @@ export class PluginLoaderService implements OnModuleInit { return pluginInstance; } + /** + * Ensure a freshly-loaded plugin has a persisted registry entry, so later enable/disable/config + * writes (which only update an EXISTING entry) actually persist instead of silently no-op'ing. + * Creates a complete INSTALLED entry when none exists; an existing entry's persisted status/config + * is left untouched. Best-effort (saveRegistry swallows fs errors, so a disk failure never turns a + * load into a 500). Does NOT enable or run the plugin — boot never auto-executes plugin code. + */ + private ensureRegistryEntry(manifest: PluginManifest, builtIn: boolean): void { + // Reconcile the persisted entry with the freshly-loaded runtime: the runtime always loads + // INSTALLED and is never auto-enabled on boot (enabling must stay an explicit ADMIN action that + // runs the lifecycle), so the entry's status is (re)set to INSTALLED to match — a previously + // enabled plugin must be re-enabled after a restart. The operator's persisted config is preserved + // so secrets/settings survive. Best-effort: saveRegistry swallows fs errors, so a disk failure + // never turns a load into a 500. + const existing = this.pluginStorage.getPluginEntry(manifest.id); + this.pluginStorage.setPluginEntry({ + id: manifest.id, + type: manifest.type, + name: manifest.name, + version: manifest.version, + status: PluginStatus.INSTALLED, + config: existing?.config ?? {}, + builtIn, + installedAt: existing?.installedAt ?? new Date(), + updatedAt: new Date(), + }); + } + async enablePlugin(pluginId: string): Promise { const plugin = this.plugins.get(pluginId); if (!plugin) { @@ -411,16 +442,24 @@ export class PluginLoaderService implements OnModuleInit { // ============================================================================ registerBuiltInPlugin(manifest: PluginManifest, instance: IPlugin, config: Record = {}): void { + // Merge: env-derived defaults stay live each boot (so a changed .env wins), while an operator's + // persisted overrides win for the keys they actually set. Engine config is wholly env-derived + // (no persisted overrides), so it is never frozen to a first-boot snapshot. + const effectiveConfig = { ...config, ...(this.pluginStorage.getPluginConfig(manifest.id) ?? {}) }; + const pluginInstance: PluginInstance = { manifest, status: PluginStatus.INSTALLED, - config, + config: effectiveConfig, instance, loadedAt: new Date(), }; this.plugins.set(manifest.id, pluginInstance); + // Ensure a registry entry exists so later enable/disable/config writes persist. + this.ensureRegistryEntry(manifest, true); + this.logger.debug(`Built-in plugin registered: ${manifest.name}`, { pluginId: manifest.id, action: 'builtin_plugin_registered', diff --git a/src/core/plugins/plugin-storage.service.ts b/src/core/plugins/plugin-storage.service.ts index 1e9b5f30..c1fe47a5 100644 --- a/src/core/plugins/plugin-storage.service.ts +++ b/src/core/plugins/plugin-storage.service.ts @@ -39,11 +39,18 @@ export class PluginStorageService { try { const dir = path.dirname(this.registryPath); if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true }); + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); } const entries = Array.from(this.registry.values()); - fs.writeFileSync(this.registryPath, JSON.stringify(entries, null, 2)); + // Owner-only: plugin config can hold secrets (e.g. an API key). writeFileSync's mode only + // applies on CREATE, so chmod an already-existing, looser file too (best-effort). + fs.writeFileSync(this.registryPath, JSON.stringify(entries, null, 2), { mode: 0o600 }); + try { + fs.chmodSync(this.registryPath, 0o600); + } catch { + /* best-effort hardening */ + } } catch (error) { this.logger.error('Failed to save plugin registry', String(error), { action: 'registry_save_failed', From 5738f497b05179766cfdfb5a3d86e26fe0f75d48 Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:24:30 +0700 Subject: [PATCH 04/14] fix(message): persist bulk-sent messages, sanitize SSRF (#340) --- .../message/bulk-message.service.spec.ts | 108 +++++++++++++++++- src/modules/message/bulk-message.service.ts | 91 +++++++++++++-- src/modules/message/message.service.ts | 5 +- 3 files changed, 193 insertions(+), 11 deletions(-) diff --git a/src/modules/message/bulk-message.service.spec.ts b/src/modules/message/bulk-message.service.spec.ts index ce579afb..7c72b30e 100644 --- a/src/modules/message/bulk-message.service.spec.ts +++ b/src/modules/message/bulk-message.service.spec.ts @@ -1,8 +1,11 @@ import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; -import { BulkMessageService, resolveFinalBatchStatus } from './bulk-message.service'; +import { BulkMessageService, resolveFinalBatchStatus, sanitizeBatchError } from './bulk-message.service'; import { MessageBatch, BatchStatus } from './entities/message-batch.entity'; +import { MessageStatus } from './entities/message.entity'; import { SessionService } from '../session/session.service'; +import { MessageService } from './message.service'; +import { SsrfBlockedError } from '../../common/security/ssrf-guard'; /** Regression lock for the terminal-status decision (cancel-clobber + stopOnError overwrite bugs). */ describe('resolveFinalBatchStatus', () => { @@ -43,6 +46,7 @@ describe('BulkMessageService.onApplicationBootstrap', () => { BulkMessageService, { provide: getRepositoryToken(MessageBatch, 'data'), useValue: repo }, { provide: SessionService, useValue: { getEngine: jest.fn() } }, + { provide: MessageService, useValue: { saveOutgoingMessage: jest.fn() } }, ], }).compile(); service = module.get(BulkMessageService); @@ -65,3 +69,105 @@ describe('BulkMessageService.onApplicationBootstrap', () => { expect(repo.save).not.toHaveBeenCalled(); }); }); + +/** Regression lock: an SSRF block (which names the internal host/IP) must not be stored verbatim. */ +describe('sanitizeBatchError', () => { + it('replaces an SSRF block message with a generic one (no internal address leak)', () => { + const result = sanitizeBatchError( + new SsrfBlockedError('Host evil.example resolves to a blocked internal address: 169.254.169.254'), + ); + expect(result.message).not.toContain('169.254.169.254'); + expect(result.code).toBe('SEND_BLOCKED'); + }); + + it('passes through an ordinary error message under SEND_FAILED', () => { + const result = sanitizeBatchError(new Error('Session is not active')); + expect(result).toEqual({ code: 'SEND_FAILED', message: 'Session is not active' }); + }); +}); + +describe('BulkMessageService.processBatch', () => { + let service: BulkMessageService; + let repo: { findOne: jest.Mock; save: jest.Mock }; + let messageService: { saveOutgoingMessage: jest.Mock }; + let engine: { sendTextMessage: jest.Mock }; + let sessionService: { getEngine: jest.Mock; findOne: jest.Mock }; + + const makeBatch = (messageCount: number): MessageBatch => + ({ + id: 'b1', + batchId: 'bx', + sessionId: 's1', + status: BatchStatus.PENDING, + currentIndex: 0, + messages: Array.from({ length: messageCount }, (_, i) => ({ + chatId: `c${i}@c.us`, + type: 'text', + content: { text: 'hi' }, + })), + options: { delayBetweenMessages: 0, randomizeDelay: false, stopOnError: false }, + progress: { total: messageCount, sent: 0, failed: 0, pending: messageCount, cancelled: 0 }, + results: [], + }) as unknown as MessageBatch; + + beforeEach(async () => { + engine = { sendTextMessage: jest.fn().mockResolvedValue({ id: 'wa1', timestamp: 111 }) }; + sessionService = { + getEngine: jest.fn().mockReturnValue(engine), + findOne: jest.fn().mockResolvedValue({ phone: '628' }), + }; + messageService = { saveOutgoingMessage: jest.fn().mockResolvedValue(undefined) }; + repo = { findOne: jest.fn(), save: jest.fn().mockImplementation(b => Promise.resolve(b)) }; + const module: TestingModule = await Test.createTestingModule({ + providers: [ + BulkMessageService, + { provide: getRepositoryToken(MessageBatch, 'data'), useValue: repo }, + { provide: SessionService, useValue: sessionService }, + { provide: MessageService, useValue: messageService }, + ], + }).compile(); + service = module.get(BulkMessageService); + }); + + const runProcessBatch = (): Promise => + (service as unknown as { processBatch: (id: string) => Promise }).processBatch('b1'); + + it('persists every sent message so it appears in chat history / stats', async () => { + repo.findOne.mockResolvedValue(makeBatch(1)); + + await runProcessBatch(); + + expect(messageService.saveOutgoingMessage).toHaveBeenCalledWith( + 's1', + expect.objectContaining({ + waMessageId: 'wa1', + chatId: 'c0@c.us', + type: 'text', + status: MessageStatus.SENT, + }), + ); + }); + + it('stops sending when the batch is cancelled in the DB by another instance/restart', async () => { + // First load is the running batch; the cadence re-read reports a CANCELLED status. + repo.findOne.mockResolvedValueOnce(makeBatch(3)).mockResolvedValue({ status: BatchStatus.CANCELLED }); + + await runProcessBatch(); + + // Only the first message (before the cadence re-read saw CANCELLED) was sent. + expect(engine.sendTextMessage).toHaveBeenCalledTimes(1); + }); + + it('does not clobber a CANCELLED that landed after the last cadence read (final status stays CANCELLED)', async () => { + const batch = makeBatch(1); + repo.findOne + .mockResolvedValueOnce(batch) // processBatch initial load + .mockResolvedValueOnce(batch) // cadence re-read (i=0) — still PROCESSING + .mockResolvedValue({ status: BatchStatus.CANCELLED }); // FINAL pre-save re-read — cancel landed late + + await runProcessBatch(); + + const savedStatuses = (repo.save.mock.calls as [MessageBatch][]).map(c => c[0].status); + expect(savedStatuses[savedStatuses.length - 1]).toBe(BatchStatus.CANCELLED); + }); +}); diff --git a/src/modules/message/bulk-message.service.ts b/src/modules/message/bulk-message.service.ts index 103cf11b..c36b707e 100644 --- a/src/modules/message/bulk-message.service.ts +++ b/src/modules/message/bulk-message.service.ts @@ -10,8 +10,11 @@ import { BatchMessageResult, } from './entities/message-batch.entity'; import { SendBulkMessageDto } from './dto/bulk-message.dto'; +import { MessageStatus } from './entities/message.entity'; import { SessionService } from '../session/session.service'; -import { IWhatsAppEngine } from '../../engine/interfaces/whatsapp-engine.interface'; +import { MessageService } from './message.service'; +import { SsrfBlockedError } from '../../common/security/ssrf-guard'; +import { IWhatsAppEngine, MessageResult } from '../../engine/interfaces/whatsapp-engine.interface'; // Type definitions for bulk message content interface BulkMessageContent { @@ -40,6 +43,18 @@ export function resolveFinalBatchStatus( return progress.failed > 0 && progress.sent === 0 ? BatchStatus.FAILED : BatchStatus.COMPLETED; } +/** + * Build the error stored on a batch result. An SSRF block names the internal host/IP it refused, so + * it must never be persisted/returned verbatim — it would be readable via GET batch status. Map it to + * a generic, code-tagged message; ordinary errors keep their (non-sensitive) message. + */ +export function sanitizeBatchError(error: unknown): { code: string; message: string } { + if (error instanceof SsrfBlockedError) { + return { code: 'SEND_BLOCKED', message: 'Destination address is not allowed' }; + } + return { code: 'SEND_FAILED', message: error instanceof Error ? error.message : String(error) }; +} + @Injectable() export class BulkMessageService implements OnApplicationBootstrap { private readonly logger = new Logger(BulkMessageService.name); @@ -49,6 +64,7 @@ export class BulkMessageService implements OnApplicationBootstrap { @InjectRepository(MessageBatch, 'data') private readonly batchRepository: Repository, private readonly sessionService: SessionService, + private readonly messageService: MessageService, ) {} /** @@ -182,6 +198,7 @@ export class BulkMessageService implements OnApplicationBootstrap { const results: BatchMessageResult[] = batch.results || []; let stoppedOnError = false; + let cancelledByDb = false; for (let i = batch.currentIndex; i < batch.messages.length; i++) { // Check for cancellation @@ -209,17 +226,21 @@ export class BulkMessageService implements OnApplicationBootstrap { batch.progress.sent++; batch.progress.pending--; + // Persist like a single send so the message shows in chat history + stats. The engine echo + // (onMessageCreate) fires the webhook/WS but does NOT write the DB, so without this the + // bulk-sent message is invisible to the messages table. + await this.persistSentMessage(batch.sessionId, msg.chatId, msg.type, content, messageResult); + this.logger.debug(`Batch ${batch.batchId}: Sent message ${i + 1}/${batch.messages.length} to ${msg.chatId}`); } catch (error) { result.status = BatchMessageStatus.FAILED; - result.error = { - code: 'SEND_FAILED', - message: String(error), - }; + // Sanitize: an SSRF block names an internal address — never store/return/log it verbatim. + const sanitized = sanitizeBatchError(error); + result.error = sanitized; batch.progress.failed++; batch.progress.pending--; - this.logger.warn(`Batch ${batch.batchId}: Failed message ${i + 1} to ${msg.chatId}: ${String(error)}`); + this.logger.warn(`Batch ${batch.batchId}: Failed message ${i + 1} to ${msg.chatId}: ${sanitized.message}`); if (batch.options.stopOnError) { batch.status = BatchStatus.FAILED; @@ -235,6 +256,15 @@ export class BulkMessageService implements OnApplicationBootstrap { // Save progress periodically (every 10 messages or last message) if (i % 10 === 0 || i === batch.messages.length - 1) { + // Honor a cancellation issued by ANOTHER instance / after a restart — the in-memory Map only + // sees same-process cancels. Re-read the status BEFORE saving so we don't clobber a CANCELLED + // back to PROCESSING. + const fresh = await this.batchRepository.findOne({ where: { id: batch.id }, select: ['status'] }); + if (fresh?.status === BatchStatus.CANCELLED) { + cancelledByDb = true; + this.logger.log(`Batch ${batch.batchId} cancelled (DB) at index ${i}`); + break; + } await this.batchRepository.save(batch); } @@ -247,7 +277,16 @@ export class BulkMessageService implements OnApplicationBootstrap { // Final update. NOTE: `batch` still holds the in-memory PROCESSING status from the start, so a // cancellation persisted by cancelBatch would be overwritten if we saved without re-deriving it. - const cancelled = !this.processingBatches.get(batch.id); + // A cancel may also have landed AFTER the last cadence re-read (multi-replica / post-restart); the + // unconditional save below would clobber it back to a terminal non-cancelled status, so re-read + // once more here unless we already know the batch was cancelled. + if (!cancelledByDb) { + const fresh = await this.batchRepository.findOne({ where: { id: batch.id }, select: ['status'] }); + if (fresh?.status === BatchStatus.CANCELLED) { + cancelledByDb = true; + } + } + const cancelled = cancelledByDb || !this.processingBatches.get(batch.id); batch.status = resolveFinalBatchStatus(cancelled, stoppedOnError, batch.progress); if (cancelled) { // Reconcile the counters the same way cancelBatch does, so the persisted state is consistent. @@ -295,12 +334,48 @@ export class BulkMessageService implements OnApplicationBootstrap { return processValue(content) as BulkMessageContent; } + /** + * Persist a successfully-sent batch message via the shared single-send persistence path, so it + * shows up in chat history and stats like any other outgoing message. Best-effort: a persistence + * failure must never flip a message that actually went out to FAILED. + */ + private async persistSentMessage( + sessionId: string, + chatId: string, + type: string, + content: BulkMessageContent, + result: MessageResult, + ): Promise { + const media = content.image ?? content.video ?? content.audio ?? content.document; + try { + await this.messageService.saveOutgoingMessage(sessionId, { + waMessageId: result.id, + chatId, + body: content.text ?? content.caption ?? '', + type, + timestamp: result.timestamp, + status: MessageStatus.SENT, + metadata: media + ? { + media: { + mimetype: media.mimetype, + data: media.url ?? media.base64, + filename: content.document?.filename, + }, + } + : undefined, + }); + } catch (error) { + this.logger.warn(`Batch message persisted-after-send failed: ${String(error)}`); + } + } + private sendMessage( engine: IWhatsAppEngine, chatId: string, type: string, content: BulkMessageContent, - ): Promise<{ id: string }> { + ): Promise { switch (type) { case 'text': return engine.sendTextMessage(chatId, content.text || ''); diff --git a/src/modules/message/message.service.ts b/src/modules/message/message.service.ts index 24e6a5d1..4daca586 100644 --- a/src/modules/message/message.service.ts +++ b/src/modules/message/message.service.ts @@ -480,9 +480,10 @@ export class MessageService { /** * Save outgoing message to database. - * When called before sending, creates a record with PENDING status. + * When called before sending, creates a record with PENDING status; bulk send reuses this after a + * successful send (status SENT) so batch messages are persisted like single sends. */ - private async saveOutgoingMessage( + async saveOutgoingMessage( sessionId: string, data: { waMessageId?: string; From 79091ac8360d2f84949b5633bedf7e94f5d462af Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:24:31 +0700 Subject: [PATCH 05/14] fix(engine): return the real id for forwarded messages (#341) --- .../adapters/whatsapp-web-js.adapter.spec.ts | 60 +++++++++++++++++++ .../adapters/whatsapp-web-js.adapter.ts | 30 ++++++++-- src/modules/message/message.service.ts | 7 ++- 3 files changed, 90 insertions(+), 7 deletions(-) diff --git a/src/engine/adapters/whatsapp-web-js.adapter.spec.ts b/src/engine/adapters/whatsapp-web-js.adapter.spec.ts index aeb2e0e2..243bb7cc 100644 --- a/src/engine/adapters/whatsapp-web-js.adapter.spec.ts +++ b/src/engine/adapters/whatsapp-web-js.adapter.spec.ts @@ -160,6 +160,66 @@ describe('WhatsAppWebJsAdapter readiness guard (#100)', () => { }); }); +describe('WhatsAppWebJsAdapter.forwardMessage (returns the real sent id, not a synthetic fwd_ id)', () => { + const readyAdapter = (client: unknown): WhatsAppWebJsAdapter => { + const adapter = new WhatsAppWebJsAdapter({ sessionId: 's', sessionDataPath: './data/sessions', puppeteer: {} }); + (adapter as unknown as { status: EngineStatus }).status = EngineStatus.READY; + (adapter as unknown as { client: unknown }).client = client; + return adapter; + }; + + it('returns the real id of the forwarded copy fetched from the destination chat', async () => { + const forward = jest.fn().mockResolvedValue(undefined); + const sourceChat = { fetchMessages: jest.fn().mockResolvedValue([{ id: { _serialized: 'SRC1' }, forward }]) }; + const destChat = { + fetchMessages: jest.fn().mockResolvedValue([ + { id: { _serialized: 'OLD' }, timestamp: 100 }, + { id: { _serialized: 'REAL_FWD' }, timestamp: 200 }, // most recent fromMe = the forwarded copy + ]), + }; + const client = { + getChatById: jest.fn((id: string) => Promise.resolve(id === 'dest@c.us' ? destChat : sourceChat)), + }; + + const result = await readyAdapter(client).forwardMessage('src@c.us', 'dest@c.us', 'SRC1'); + + expect(forward).toHaveBeenCalledWith('dest@c.us'); + expect(result.id).toBe('REAL_FWD'); + expect(result.id).not.toMatch(/^fwd_/); + }); + + it('returns an explicit-unknown id (empty, not a real/synthetic id) when the sent copy cannot be identified', async () => { + // Empty id leaves the forward row's waMessageId unset, so no ack can mis-match it (a source/synthetic + // id could cross-drive another row's delivery status). + const forward = jest.fn().mockResolvedValue(undefined); + const sourceChat = { fetchMessages: jest.fn().mockResolvedValue([{ id: { _serialized: 'SRC1' }, forward }]) }; + const destChat = { fetchMessages: jest.fn().mockResolvedValue([]) }; + const client = { + getChatById: jest.fn((id: string) => Promise.resolve(id === 'dest@c.us' ? destChat : sourceChat)), + }; + + const result = await readyAdapter(client).forwardMessage('src@c.us', 'dest@c.us', 'SRC1'); + + expect(result.id).toBe(''); + expect(result.id).not.toMatch(/^fwd_/); + }); + + it('does not report a failure when post-forward id recovery throws (the forward already happened)', async () => { + const forward = jest.fn().mockResolvedValue(undefined); + const sourceChat = { fetchMessages: jest.fn().mockResolvedValue([{ id: { _serialized: 'SRC1' }, forward }]) }; + const client = { + getChatById: jest.fn((id: string) => + id === 'dest@c.us' ? Promise.reject(new Error('puppeteer detached')) : Promise.resolve(sourceChat), + ), + }; + + const result = await readyAdapter(client).forwardMessage('src@c.us', 'dest@c.us', 'SRC1'); + + expect(forward).toHaveBeenCalledWith('dest@c.us'); + expect(result.id).toBe(''); + }); +}); + describe('WhatsAppWebJsAdapter.resolveContactPhone (@lid -> phone, #263)', () => { // Stub a "ready" adapter with a fake client so we exercise the mapping without a real browser. const readyAdapter = (getContactLidAndPhone: jest.Mock): WhatsAppWebJsAdapter => { diff --git a/src/engine/adapters/whatsapp-web-js.adapter.ts b/src/engine/adapters/whatsapp-web-js.adapter.ts index 590b33f8..e1e3eb13 100644 --- a/src/engine/adapters/whatsapp-web-js.adapter.ts +++ b/src/engine/adapters/whatsapp-web-js.adapter.ts @@ -687,11 +687,31 @@ export class WhatsAppWebJsAdapter extends EventEmitter implements IWhatsAppEngin } await msgToForward.forward(toChatId); - // forward() returns void, so we generate a result based on original message - return { - id: `fwd_${messageId}`, - timestamp: Date.now(), - }; + + // whatsapp-web.js's forward() returns void, so BEST-EFFORT recover the REAL id of the sent copy by + // reading it back from the destination chat (the most recent outgoing message). The delivery-ack + // matcher keys on this id, so a synthetic one would leave the forward stuck at SENT; Baileys already + // returns the real id. The forward already succeeded here, so recovery must NEVER fail the operation. + // When the copy can't be identified we return an explicit-unknown id (empty): message.service then + // leaves the row's waMessageId unset so no ack can mis-match it — unlike a synthetic or source id, + // which could cross-drive another row's delivery status. Concurrent forwards to the same chat may + // mis-identify the copy — acceptable for delivery-status accuracy. + try { + const destChat = await this.client!.getChatById(toChatId); + const sentByMe = (await destChat?.fetchMessages({ limit: 5, fromMe: true })) ?? []; + let sent: (typeof sentByMe)[number] | undefined; + for (const m of sentByMe) { + if (!sent || m.timestamp > sent.timestamp) { + sent = m; + } + } + if (sent) { + return { id: sent.id._serialized, timestamp: sent.timestamp }; + } + } catch (error) { + this.logger.warn(`Forward succeeded but recovering the sent message id failed: ${String(error)}`); + } + return { id: '', timestamp: Math.floor(Date.now() / 1000) }; } // ============= Phase 3: Group Management ============= diff --git a/src/modules/message/message.service.ts b/src/modules/message/message.service.ts index 4daca586..38709adf 100644 --- a/src/modules/message/message.service.ts +++ b/src/modules/message/message.service.ts @@ -449,8 +449,11 @@ export class MessageService { try { const result = await engine.forwardMessage(dto.fromChatId, dto.toChatId, dto.messageId); - // Update with actual WhatsApp message ID and status - message.waMessageId = result.id; + // Update with actual WhatsApp message ID and status. A forward whose engine could not recover the + // sent copy's real id returns an empty id — leave waMessageId unset (NULL) so no ack mis-matches it. + if (result.id) { + message.waMessageId = result.id; + } message.status = MessageStatus.SENT; message.timestamp = result.timestamp; await this.messageRepository.save(message); From 24bc593a55aaa8c84b4ddb980a011fc6df93a162 Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:24:31 +0700 Subject: [PATCH 06/14] fix(security): harden outbound requests, IPv6 SSRF (#344) --- src/common/security/ssrf-guard.spec.ts | 13 +++++ src/common/security/ssrf-guard.ts | 50 +++++++++++++++++++ .../adapters/whatsapp-web-js.adapter.spec.ts | 14 ++++++ .../adapters/whatsapp-web-js.adapter.ts | 28 +++++++++-- .../session/dto/create-session.dto.spec.ts | 36 +++++++++++++ src/modules/session/dto/create-session.dto.ts | 16 +++++- .../translation/libretranslate.client.spec.ts | 45 ++++++++++++++++- .../translation/libretranslate.client.ts | 18 ++++++- .../extensions/translation/manifest.json | 2 +- 9 files changed, 213 insertions(+), 9 deletions(-) create mode 100644 src/modules/session/dto/create-session.dto.spec.ts diff --git a/src/common/security/ssrf-guard.spec.ts b/src/common/security/ssrf-guard.spec.ts index 8d07baff..d7e31e9c 100644 --- a/src/common/security/ssrf-guard.spec.ts +++ b/src/common/security/ssrf-guard.spec.ts @@ -41,6 +41,17 @@ describe('isBlockedAddress', () => { ['::ffff:7f00:1', 'IPv4-mapped loopback (hex)'], ['::ffff:0a00:0001', 'IPv4-mapped RFC1918 (hex, zero-padded)'], ['::ffff:a9fe:a9fe', 'IPv4-mapped cloud metadata 169.254.169.254 (hex)'], + ['64:ff9b::a9fe:a9fe', 'NAT64 of cloud metadata 169.254.169.254'], + ['64:ff9b::7f00:1', 'NAT64 of loopback 127.0.0.1'], + ['64:ff9b::127.0.0.1', 'NAT64 of loopback (dotted tail)'], + ['2002:7f00:1::', '6to4 of loopback 127.0.0.1'], + ['2002:a9fe:a9fe::', '6to4 of cloud metadata 169.254.169.254'], + ['2002:0a00:0001::', '6to4 of RFC1918 10.0.0.1'], + ['2002:7f00::', '6to4 of loopback net 127.0.0.0 (low hextet compressed away)'], + ['2002:a9fe::', '6to4 of metadata net 169.254.0.0 (compressed)'], + ['2002:c0a8::', '6to4 of RFC1918 net 192.168.0.0 (compressed)'], + ['::127.0.0.1', 'IPv4-compatible loopback (deprecated, dotted)'], + ['::a9fe:a9fe', 'IPv4-compatible cloud metadata (deprecated, hex)'], ])('blocks %s (%s)', ip => { expect(isBlockedAddress(ip)).toBe(true); }); @@ -51,6 +62,8 @@ describe('isBlockedAddress', () => { ['172.32.0.1', 'just outside 172.16/12'], ['2001:4860:4860::8888', 'public IPv6'], ['::ffff:0808:0808', 'IPv4-mapped public 8.8.8.8 (hex)'], + ['2002:0808:0808::', '6to4 of public 8.8.8.8 stays allowed'], + ['64:ff9b::0808:0808', 'NAT64 of public 8.8.8.8 stays allowed'], ])('allows %s (%s)', ip => { expect(isBlockedAddress(ip)).toBe(false); }); diff --git a/src/common/security/ssrf-guard.ts b/src/common/security/ssrf-guard.ts index d670c0c0..2e85cc8d 100644 --- a/src/common/security/ssrf-guard.ts +++ b/src/common/security/ssrf-guard.ts @@ -73,6 +73,38 @@ const BLOCKED_V4: ReadonlyArray = [ * CGNAT, multicast, IPv6 loopback/ULA/link-local, IPv4-mapped variants). * Anything that isn't a recognizable public IP is treated as blocked (fail-closed). */ +/** Two 16-bit hextets → dotted IPv4 string (for IPv4-in-IPv6 embeddings like ::ffff:, 6to4, NAT64). */ +function hextetsToV4(hi: number, lo: number): string { + return `${(hi >> 8) & 0xff}.${hi & 0xff}.${(lo >> 8) & 0xff}.${lo & 0xff}`; +} + +/** + * Expand a (possibly ::-compressed, possibly dotted-IPv4-tailed) IPv6 literal to its 8 numeric + * hextets, or null if malformed. Full expansion is required so a compressed all-zero embedded segment + * (e.g. 2002:7f00:: → 127.0.0.0) is read as 0x0000 rather than silently skipped. + */ +function expandIPv6(lower: string): number[] | null { + let s = lower; + // Fold a trailing dotted IPv4 (::a.b.c.d) into two hex hextets so the remainder is pure hex. + const dotted = s.match(/(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); + if (dotted) { + const octets = dotted.slice(1, 5).map(Number); + if (octets.some(o => o > 255)) return null; + const [a, b, c, d] = octets; + s = s.slice(0, dotted.index) + `${((a << 8) | b).toString(16)}:${((c << 8) | d).toString(16)}`; + } + const halves = s.split('::'); + if (halves.length > 2) return null; + const head = halves[0] ? halves[0].split(':') : []; + const tail = halves.length === 2 && halves[1] ? halves[1].split(':') : []; + const gap = 8 - head.length - tail.length; + if (halves.length === 1 ? head.length !== 8 : gap < 1) return null; + const parts = [...head, ...Array(Math.max(gap, 0)).fill('0'), ...tail]; + if (parts.length !== 8) return null; + const nums = parts.map(h => (/^[0-9a-f]{1,4}$/.test(h) ? parseInt(h, 16) : NaN)); + return nums.some(n => Number.isNaN(n)) ? null : nums; +} + export function isBlockedAddress(ip: string): boolean { if (isIPv4(ip)) { const n = ipv4ToInt(ip); @@ -101,6 +133,24 @@ export function isBlockedAddress(ip: string): boolean { const firstHextet = lower.split(':')[0]; if (firstHextet.startsWith('fc') || firstHextet.startsWith('fd')) return true; // ULA fc00::/7 if (/^fe[89ab]/.test(firstHextet)) return true; // link-local fe80::/10 + + // IPv6 forms that embed an IPv4 — 6to4 (2002::/16), NAT64 (64:ff9b::/96), and the deprecated + // IPv4-compatible ::/96 — are classified by the embedded address so they reach the IPv4 blocklist, + // mirroring the ::ffff: handling above. The literal is fully expanded first so a compressed all-zero + // embedded hextet (e.g. 2002:7f00:: → 127.0.0.0) is not skipped. A 6to4/NAT64/compat of a genuinely + // public IPv4 still returns false, so legitimate IPv6 delivery is unaffected. + const hextets = expandIPv6(lower); + if (hextets) { + if (hextets[0] === 0x2002) { + return isBlockedAddress(hextetsToV4(hextets[1], hextets[2])); // 6to4 + } + if (hextets[0] === 0x64 && hextets[1] === 0xff9b) { + return isBlockedAddress(hextetsToV4(hextets[6], hextets[7])); // NAT64 + } + if (hextets.slice(0, 6).every(h => h === 0) && (hextets[6] | hextets[7]) !== 0) { + return isBlockedAddress(hextetsToV4(hextets[6], hextets[7])); // IPv4-compatible ::/96 + } + } return false; } diff --git a/src/engine/adapters/whatsapp-web-js.adapter.spec.ts b/src/engine/adapters/whatsapp-web-js.adapter.spec.ts index 243bb7cc..e8f1332e 100644 --- a/src/engine/adapters/whatsapp-web-js.adapter.spec.ts +++ b/src/engine/adapters/whatsapp-web-js.adapter.spec.ts @@ -2,6 +2,7 @@ import { MessageMedia } from 'whatsapp-web.js'; import { WhatsAppWebJsAdapter, extractLinkedParentJID, + isSupportedProxyUrl, loadRemoteMedia, resolveWebVersionPin, wwebjsAckToDeliveryStatus, @@ -34,6 +35,19 @@ describe('wwebjsAckToDeliveryStatus (engine ack-int -> neutral DeliveryStatus bo }); }); +describe('isSupportedProxyUrl', () => { + it.each(['http://proxy:8080', 'https://proxy:8443', 'socks4://proxy:1080', 'socks5://user:pass@proxy:1080'])( + 'accepts %s', + url => { + expect(isSupportedProxyUrl(url)).toBe(true); + }, + ); + + it.each(['not a url', 'ftp://proxy:21', 'proxy:8080', ''])('rejects %s', url => { + expect(isSupportedProxyUrl(url)).toBe(false); + }); +}); + describe('extractLinkedParentJID (#201)', () => { it('returns null when no metadata is provided', () => { expect(extractLinkedParentJID()).toBeNull(); diff --git a/src/engine/adapters/whatsapp-web-js.adapter.ts b/src/engine/adapters/whatsapp-web-js.adapter.ts index e1e3eb13..b2b7dc77 100644 --- a/src/engine/adapters/whatsapp-web-js.adapter.ts +++ b/src/engine/adapters/whatsapp-web-js.adapter.ts @@ -60,6 +60,19 @@ export function wwebjsAckToDeliveryStatus(ack: number): DeliveryStatus { return 'pending'; } +/** + * Whether a per-session proxy URL parses to a supported scheme — defense-in-depth for a stored proxy + * that bypassed DTO validation (e.g. loaded from the DB on restart). The host is NOT SSRF-blocked: a + * per-session proxy is operator-chosen egress, and a loopback proxy sidecar is a legitimate setup. + */ +export function isSupportedProxyUrl(url: string): boolean { + try { + return ['http:', 'https:', 'socks4:', 'socks5:'].includes(new URL(url).protocol); + } catch { + return false; + } +} + /** * Fetch remote media for sending, with an SSRF host guard, a byte cap, and a timeout. * The guard runs BEFORE any network call, so an internal/reserved URL throws `SsrfBlockedError` @@ -166,12 +179,17 @@ export class WhatsAppWebJsAdapter extends EventEmitter implements IWhatsAppEngin '--disable-gpu', ]; - // Add proxy configuration if provided + // Add proxy configuration if provided — but only when the URL parses to a supported scheme, so + // a malformed/stored proxy value can't break the Chromium launch or smuggle a non-proxy scheme. if (this.config.proxy) { - puppeteerArgs.push(`--proxy-server=${this.config.proxy.url}`); - this.logger.log( - `Using proxy: ${this.config.proxy.type}://${this.config.proxy.url.replace(/:[^:@]*@/, ':***@')}`, - ); + if (isSupportedProxyUrl(this.config.proxy.url)) { + puppeteerArgs.push(`--proxy-server=${this.config.proxy.url}`); + this.logger.log( + `Using proxy: ${this.config.proxy.type}://${this.config.proxy.url.replace(/:[^:@]*@/, ':***@')}`, + ); + } else { + this.logger.warn(`Ignoring invalid proxy URL for session ${this.config.sessionId}`); + } } // Pin the WA-Web version when configured (fixes the 1.34.x "stuck at authenticating" diff --git a/src/modules/session/dto/create-session.dto.spec.ts b/src/modules/session/dto/create-session.dto.spec.ts new file mode 100644 index 00000000..c83e8eca --- /dev/null +++ b/src/modules/session/dto/create-session.dto.spec.ts @@ -0,0 +1,36 @@ +import { validateSync } from 'class-validator'; +import { plainToInstance } from 'class-transformer'; +import { CreateSessionDto } from './create-session.dto'; + +describe('CreateSessionDto proxyUrl validation', () => { + const errs = (proxyUrl: string): ReturnType => + validateSync(plainToInstance(CreateSessionDto, { name: 'my-bot', proxyUrl })); + + it.each([ + 'http://proxy.example.com:8080', + 'http://user:pass@proxy.example.com:8080', + 'https://proxy.example.com:8443', + 'socks5://proxy.example.com:1080', + 'socks4://proxy.example.com:1080', + // Single-label hosts are common in containerized setups (e.g. a `squid` service) — must validate. + 'http://localhost:8080', + 'http://squid:3128', + 'socks5://proxy:1080', + 'http://10.0.0.1:8080', + ])('accepts a valid proxy URL: %s', url => { + expect(errs(url)).toHaveLength(0); + }); + + it.each([ + 'not a url', + 'proxy.example.com:8080', // no scheme + 'ftp://proxy.example.com:21', // unsupported scheme + 'javascript:alert(1)', + ])('rejects an invalid / non-proxy-scheme proxyUrl: %s', url => { + expect(errs(url).length).toBeGreaterThan(0); + }); + + it('allows an omitted proxyUrl (optional)', () => { + expect(validateSync(plainToInstance(CreateSessionDto, { name: 'my-bot' }))).toHaveLength(0); + }); +}); diff --git a/src/modules/session/dto/create-session.dto.ts b/src/modules/session/dto/create-session.dto.ts index a3e73251..691a7a9e 100644 --- a/src/modules/session/dto/create-session.dto.ts +++ b/src/modules/session/dto/create-session.dto.ts @@ -1,5 +1,5 @@ import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; -import { IsString, IsOptional, MaxLength, MinLength, Matches, IsIn } from 'class-validator'; +import { IsString, IsOptional, MaxLength, MinLength, Matches, IsIn, IsUrl } from 'class-validator'; export class CreateSessionDto { @ApiProperty({ @@ -31,6 +31,20 @@ export class CreateSessionDto { @IsOptional() @IsString() @MaxLength(255) + // Reject a malformed/non-proxy URL at the boundary (credentialed http://user:pass@host and + // socks4/5 still validate). The host is intentionally NOT SSRF-blocked here — a per-session proxy + // is operator-chosen egress, and a loopback proxy sidecar is a legitimate setup. + // require_tld:false + allow_underscores:true so single-label container hostnames (e.g. `squid`, + // `localhost`) and IP-literal proxies validate, matching the engine's URL-parse check. + @IsUrl( + { + protocols: ['http', 'https', 'socks4', 'socks5'], + require_protocol: true, + require_tld: false, + allow_underscores: true, + }, + { message: 'proxyUrl must be a valid http(s)/socks4/socks5 URL' }, + ) proxyUrl?: string; @ApiPropertyOptional({ diff --git a/src/plugins/extensions/translation/libretranslate.client.spec.ts b/src/plugins/extensions/translation/libretranslate.client.spec.ts index f9afa159..6430dcdc 100644 --- a/src/plugins/extensions/translation/libretranslate.client.spec.ts +++ b/src/plugins/extensions/translation/libretranslate.client.spec.ts @@ -6,7 +6,50 @@ describe('LibreTranslateClient', () => { global.fetch = impl; }; - afterEach(() => jest.restoreAllMocks()); + const origAllow = process.env.SSRF_ALLOWED_HOSTS; + beforeEach(() => { + // The logic tests target host `lt`; allowlist it so the new SSRF guard lets them through. + process.env.SSRF_ALLOWED_HOSTS = 'lt'; + }); + afterEach(() => { + if (origAllow === undefined) delete process.env.SSRF_ALLOWED_HOSTS; + else process.env.SSRF_ALLOWED_HOSTS = origAllow; + jest.restoreAllMocks(); + }); + + describe('SSRF guard', () => { + it('blocks a request to an internal address when SSRF protection is on (no fetch)', async () => { + delete process.env.SSRF_ALLOWED_HOSTS; // 169.254.169.254 not allowlisted + const fetchMock = jest.fn(); + global.fetch = fetchMock; + const client = new LibreTranslateClient({ url: 'http://169.254.169.254:7001', timeoutMs: 1000 }); + await expect(client.translate('a', 'en', 'es')).rejects.toThrow(); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('does not trip the circuit breaker on a deterministic SSRF block', async () => { + delete process.env.SSRF_ALLOWED_HOSTS; + global.fetch = jest.fn(); + const client = new LibreTranslateClient({ + url: 'http://169.254.169.254:7001', + timeoutMs: 1000, + failureThreshold: 2, + }); + await expect(client.translate('a', 'en', 'es')).rejects.toThrow(); + await expect(client.translate('a', 'en', 'es')).rejects.toThrow(); + await expect(client.translate('a', 'en', 'es')).rejects.toThrow(); + expect(client.isHealthy()).toBe(true); + }); + + it('refuses to follow redirects (redirect: error) on a guarded request', async () => { + const fetchMock = jest.fn().mockResolvedValue({ ok: true, json: () => Promise.resolve({ translatedText: 'x' }) }); + global.fetch = fetchMock; + const client = new LibreTranslateClient({ url: 'http://lt:7001', timeoutMs: 1000 }); + await client.translate('a', 'en', 'es'); + const init = (fetchMock.mock.calls[0] as [string, RequestInit])[1]; + expect(init.redirect).toBe('error'); + }); + }); it('translate() posts q/source/target and returns translatedText', async () => { const fetchMock = jest.fn, [string, RequestInit?]>().mockResolvedValue({ diff --git a/src/plugins/extensions/translation/libretranslate.client.ts b/src/plugins/extensions/translation/libretranslate.client.ts index 4861cc74..b7f771dc 100644 --- a/src/plugins/extensions/translation/libretranslate.client.ts +++ b/src/plugins/extensions/translation/libretranslate.client.ts @@ -1,6 +1,7 @@ // src/modules/translation/adapters/libretranslate.client.ts import { Translator, DetectResult } from './core/ports'; import { createLogger } from '../../../common/services/logger.service'; +import { assertSafeFetchUrl, isSsrfProtectionEnabled, SsrfBlockedError } from '../../../common/security/ssrf-guard'; export interface LibreTranslateOptions { url: string; @@ -57,15 +58,25 @@ export class LibreTranslateClient implements Translator { throw new Error('LibreTranslate circuit open'); } + const url = `${this.base}${path}`; + const ssrfProtected = isSsrfProtectionEnabled(); const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), this.opts.timeoutMs); try { + // Validate the target host before sending the api_key-bearing body. Honors SSRF_ALLOWED_HOSTS, + // so the documented localhost LibreTranslate sidecar still works once it's allowlisted. + if (ssrfProtected) { + await assertSafeFetchUrl(url); + } const body = method === 'POST' ? JSON.stringify({ ...payload, api_key: this.opts.apiKey }) : undefined; - const res = await fetch(`${this.base}${path}`, { + const res = await fetch(url, { method, headers: { 'Content-Type': 'application/json' }, body, signal: controller.signal, + // Refuse redirects: the guard only validated the original host; a 3xx would re-send the + // api_key to a redirect-controlled (possibly internal) target. + redirect: ssrfProtected ? 'error' : 'follow', }); if (!res.ok) { throw new Error(`LibreTranslate ${path} -> HTTP ${res.status}`); @@ -73,6 +84,11 @@ export class LibreTranslateClient implements Translator { this.consecutiveFailures = 0; return await res.json(); } catch (err) { + // A blocked-host SSRF error is a deterministic configuration problem, not a transient upstream + // failure — don't let it trip the circuit breaker (which exists to back off a flaky server). + if (err instanceof SsrfBlockedError) { + throw err; + } this.consecutiveFailures++; if (this.consecutiveFailures >= this.failureThreshold) { this.openUntil = Date.now() + this.cooldownMs; diff --git a/src/plugins/extensions/translation/manifest.json b/src/plugins/extensions/translation/manifest.json index eb016a9f..1542ebc6 100644 --- a/src/plugins/extensions/translation/manifest.json +++ b/src/plugins/extensions/translation/manifest.json @@ -13,7 +13,7 @@ "libretranslateUrl": { "type": "string", "title": "LibreTranslate URL", - "description": "Base URL of the LibreTranslate instance (e.g. http://libretranslate:7001).", + "description": "Base URL of the LibreTranslate instance (e.g. http://libretranslate:7001). With SSRF protection on (the default), a loopback/internal URL such as http://localhost:7001 must be added to SSRF_ALLOWED_HOSTS.", "default": "http://localhost:7001", "required": true }, From a09df8c1d6c884d1406d84e19c3c9d35e9976bb2 Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:24:31 +0700 Subject: [PATCH 07/14] fix(security): secret-file perms, key pepper, allowedIps (#345) --- .../bull-board-auth.middleware.spec.ts | 25 ++++++- .../security/bull-board-auth.middleware.ts | 12 +++- src/common/utils/secret-file.spec.ts | 31 +++++++++ src/common/utils/secret-file.ts | 24 +++++++ src/main.ts | 24 +++++-- src/modules/auth/api-key-hash.spec.ts | 19 ++++++ src/modules/auth/api-key-hash.ts | 14 ++++ src/modules/auth/auth.service.spec.ts | 33 +++++++++- src/modules/auth/auth.service.ts | 12 ++-- src/modules/auth/dto/api-key.dto.ts | 5 +- .../auth/dto/is-ip-or-cidr.validator.spec.ts | 65 +++++++++++++++++++ .../auth/dto/is-ip-or-cidr.validator.ts | 29 +++++++++ 12 files changed, 279 insertions(+), 14 deletions(-) create mode 100644 src/common/utils/secret-file.spec.ts create mode 100644 src/common/utils/secret-file.ts create mode 100644 src/modules/auth/api-key-hash.spec.ts create mode 100644 src/modules/auth/api-key-hash.ts create mode 100644 src/modules/auth/dto/is-ip-or-cidr.validator.spec.ts create mode 100644 src/modules/auth/dto/is-ip-or-cidr.validator.ts diff --git a/src/common/security/bull-board-auth.middleware.spec.ts b/src/common/security/bull-board-auth.middleware.spec.ts index 098366a9..892cd021 100644 --- a/src/common/security/bull-board-auth.middleware.spec.ts +++ b/src/common/security/bull-board-auth.middleware.spec.ts @@ -1,4 +1,5 @@ import { UnauthorizedException, ForbiddenException } from '@nestjs/common'; +import { ConfigService } from '@nestjs/config'; import { Request, Response } from 'express'; import { BullBoardAuthMiddleware } from './bull-board-auth.middleware'; import { AuthService } from '../../modules/auth/auth.service'; @@ -7,6 +8,7 @@ import { ApiKeyRole } from '../../modules/auth/entities/api-key.entity'; describe('BullBoardAuthMiddleware', () => { let mw: BullBoardAuthMiddleware; let authService: { validateApiKey: jest.Mock; hasPermission: jest.Mock }; + let configService: { get: jest.Mock }; const res = {} as Response; const reqWith = (headers: Record = {}, query: Record = {}): Request => @@ -14,7 +16,8 @@ describe('BullBoardAuthMiddleware', () => { beforeEach(() => { authService = { validateApiKey: jest.fn(), hasPermission: jest.fn() }; - mw = new BullBoardAuthMiddleware(authService as unknown as AuthService); + configService = { get: jest.fn().mockReturnValue(undefined) }; // no trusted proxies by default + mw = new BullBoardAuthMiddleware(authService as unknown as AuthService, configService as unknown as ConfigService); }); it('rejects when no API key is provided', async () => { @@ -56,6 +59,26 @@ describe('BullBoardAuthMiddleware', () => { expect(authService.validateApiKey).toHaveBeenCalledWith('abc', '127.0.0.1'); }); + it('honors X-Forwarded-For only behind a configured trusted proxy (allowedIps parity with the guard)', async () => { + configService.get.mockReturnValue(['127.0.0.1']); // the socket peer is a trusted proxy + authService.validateApiKey.mockResolvedValue({ role: ApiKeyRole.ADMIN }); + authService.hasPermission.mockReturnValue(true); + + await mw.use(reqWith({ 'x-api-key': 'admin', 'x-forwarded-for': '203.0.113.5' }), res, jest.fn()); + + expect(authService.validateApiKey).toHaveBeenCalledWith('admin', '203.0.113.5'); + }); + + it('ignores a spoofed X-Forwarded-For when no trusted proxy is configured (uses the socket address)', async () => { + configService.get.mockReturnValue([]); // no trusted proxies — XFF is attacker-controlled + authService.validateApiKey.mockResolvedValue({ role: ApiKeyRole.ADMIN }); + authService.hasPermission.mockReturnValue(true); + + await mw.use(reqWith({ 'x-api-key': 'admin', 'x-forwarded-for': '203.0.113.5' }), res, jest.fn()); + + expect(authService.validateApiKey).toHaveBeenCalledWith('admin', '127.0.0.1'); + }); + it('rejects an ?apiKey query param (no key in the URL)', async () => { const next = jest.fn(); await mw.use(reqWith({}, { apiKey: 'qkey' }), res, next); diff --git a/src/common/security/bull-board-auth.middleware.ts b/src/common/security/bull-board-auth.middleware.ts index 6c95897a..0c8f4b37 100644 --- a/src/common/security/bull-board-auth.middleware.ts +++ b/src/common/security/bull-board-auth.middleware.ts @@ -1,7 +1,9 @@ import { Injectable, NestMiddleware, UnauthorizedException, ForbiddenException } from '@nestjs/common'; +import { ConfigService } from '@nestjs/config'; import { Request, Response, NextFunction } from 'express'; import { AuthService } from '../../modules/auth/auth.service'; import { ApiKeyRole } from '../../modules/auth/entities/api-key.entity'; +import { resolveClientIp } from '../utils/ip'; /** * Protects the Bull Board UI (/admin/queues). @@ -16,7 +18,10 @@ import { ApiKeyRole } from '../../modules/auth/entities/api-key.entity'; */ @Injectable() export class BullBoardAuthMiddleware implements NestMiddleware { - constructor(private readonly authService: AuthService) {} + constructor( + private readonly authService: AuthService, + private readonly configService: ConfigService, + ) {} async use(req: Request, _res: Response, next: NextFunction): Promise { try { @@ -49,6 +54,9 @@ export class BullBoardAuthMiddleware implements NestMiddleware { } private getClientIp(req: Request): string { - return req.socket?.remoteAddress || req.ip || ''; + // Mirror the ApiKeyGuard's IP model so allowedIps is enforced consistently for the queue UI: + // X-Forwarded-For is honored only behind a configured trusted proxy. + const trustedProxies = this.configService.get('security.trustedProxies') ?? []; + return resolveClientIp(req, trustedProxies); } } diff --git a/src/common/utils/secret-file.spec.ts b/src/common/utils/secret-file.spec.ts new file mode 100644 index 00000000..36843daa --- /dev/null +++ b/src/common/utils/secret-file.spec.ts @@ -0,0 +1,31 @@ +import { mkdtempSync, rmSync, statSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { writeSecretFile } from './secret-file'; + +describe('writeSecretFile', () => { + let dir: string; + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'owa-secret-')); + }); + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + const mode = (p: string): number => statSync(p).mode & 0o777; + + it('writes a new secret file owner-only (no group/other access)', () => { + const p = join(dir, 'secret'); + writeSecretFile(p, 'topsecret'); + expect(mode(p) & 0o077).toBe(0); + }); + + it('tightens an already-existing world-readable file (writeFileSync mode only applies on create)', () => { + const p = join(dir, 'legacy'); + writeFileSync(p, 'old', { mode: 0o644 }); + expect(mode(p) & 0o077).not.toBe(0); // precondition: loose + + writeSecretFile(p, 'new'); + expect(mode(p) & 0o077).toBe(0); + }); +}); diff --git a/src/common/utils/secret-file.ts b/src/common/utils/secret-file.ts new file mode 100644 index 00000000..38871995 --- /dev/null +++ b/src/common/utils/secret-file.ts @@ -0,0 +1,24 @@ +import { writeFileSync, chmodSync } from 'fs'; + +/** + * Write a secret file (e.g. the generated `.env`, the raw admin key) with owner-only permissions. + * + * `writeFileSync`'s `mode` is honored only when the file is CREATED — on an overwrite it keeps the + * existing permissions. So we chmod to 0o600 BEFORE writing too: if the file already exists with + * looser perms, the new secret content is never briefly world-readable during the rewrite. The + * post-write chmod is a backstop. Both chmods are best-effort (a mount that can't chmod, or an + * absent file on the pre-write call, shouldn't break the write — create-mode covers new files). + */ +export function writeSecretFile(filePath: string, content: string): void { + try { + chmodSync(filePath, 0o600); + } catch { + /* file not present yet, or unsupported — create-mode below covers a new file */ + } + writeFileSync(filePath, content, { mode: 0o600 }); + try { + chmodSync(filePath, 0o600); + } catch { + /* best-effort */ + } +} diff --git a/src/main.ts b/src/main.ts index a594ac3a..78a0c210 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1,5 +1,6 @@ import { NestFactory } from '@nestjs/core'; import { ValidationPipe } from '@nestjs/common'; +import { ConfigService } from '@nestjs/config'; import { SwaggerModule } from '@nestjs/swagger'; import helmet from 'helmet'; import { AppModule, DASHBOARD_DIST, dashboardServingEnabled, dashboardBuildPresent } from './app.module'; @@ -15,6 +16,7 @@ import { import { BullBoardAuthMiddleware } from './common/security/bull-board-auth.middleware'; import { AuthService } from './modules/auth/auth.service'; import { Request, Response, NextFunction, json, urlencoded } from 'express'; +import { writeSecretFile } from './common/utils/secret-file'; import * as dotenv from 'dotenv'; import * as fs from 'fs'; import * as path from 'path'; @@ -36,6 +38,18 @@ if (!fs.existsSync(dataDir)) { fs.mkdirSync(dataDir, { recursive: true }); } +// Tighten any pre-existing secret files written before per-file 0600 perms (best-effort) — the +// generated .env holds S3/DB/Redis secrets and .api-key holds the raw admin key. +for (const secret of [generatedEnvPath, path.resolve(dataDir, '.api-key')]) { + if (fs.existsSync(secret)) { + try { + fs.chmodSync(secret, 0o600); + } catch { + /* best-effort */ + } + } +} + // 2. User-managed .env (does not override real process env) if (fs.existsSync(userEnvPath)) { console.log('[Bootstrap] Loading .env from:', userEnvPath); @@ -70,7 +84,7 @@ STORAGE_PATH=./data/media # Docker Profiles: none (minimal setup) `; - fs.writeFileSync(generatedEnvPath, minimalConfig); + writeSecretFile(generatedEnvPath, minimalConfig); console.log('[Bootstrap] Created default configuration at:', generatedEnvPath); dotenv.config({ path: generatedEnvPath, override: false }); } @@ -96,8 +110,10 @@ async function bootstrap() { databaseType: process.env.DATABASE_TYPE, databasePassword: process.env.DATABASE_PASSWORD, storageType: process.env.STORAGE_TYPE, - s3AccessKey: process.env.S3_ACCESS_KEY, - s3SecretKey: process.env.S3_SECRET_KEY, + // Mirror storage.service's canonical-with-legacy fallback so the guard inspects the var the app + // actually uses (it reads S3_ACCESS_KEY_ID/S3_SECRET_ACCESS_KEY first). + s3AccessKey: process.env.S3_ACCESS_KEY_ID || process.env.S3_ACCESS_KEY, + s3SecretKey: process.env.S3_SECRET_ACCESS_KEY || process.env.S3_SECRET_KEY, apiMasterKey: process.env.API_MASTER_KEY, allowDevApiKey: process.env.ALLOW_DEV_API_KEY, }); @@ -215,7 +231,7 @@ async function bootstrap() { // @bull-board/nestjs as raw Express middleware that the global ApiKeyGuard // does not cover; registering this before app.listen() ensures it runs ahead // of the Bull Board router. Requires a valid ADMIN API key. - const bullBoardAuth = new BullBoardAuthMiddleware(app.get(AuthService)); + const bullBoardAuth = new BullBoardAuthMiddleware(app.get(AuthService), app.get(ConfigService)); app.use('/api/admin/queues', (req: Request, res: Response, next: NextFunction) => { void bullBoardAuth.use(req, res, next); }); diff --git a/src/modules/auth/api-key-hash.spec.ts b/src/modules/auth/api-key-hash.spec.ts new file mode 100644 index 00000000..7adab13c --- /dev/null +++ b/src/modules/auth/api-key-hash.spec.ts @@ -0,0 +1,19 @@ +import { createHash, createHmac } from 'crypto'; +import { hashApiKey } from './api-key-hash'; + +describe('hashApiKey', () => { + it('uses plain SHA-256 when no pepper is set (preserves existing stored hashes)', () => { + expect(hashApiKey('owa_secret')).toBe(createHash('sha256').update('owa_secret').digest('hex')); + expect(hashApiKey('owa_secret', undefined)).toBe(createHash('sha256').update('owa_secret').digest('hex')); + }); + + it('uses HMAC-SHA256 with the pepper when set, distinct from the un-peppered hash', () => { + const peppered = hashApiKey('owa_secret', 'server-pepper'); + expect(peppered).toBe(createHmac('sha256', 'server-pepper').update('owa_secret').digest('hex')); + expect(peppered).not.toBe(hashApiKey('owa_secret')); + }); + + it('is deterministic for the same key + pepper', () => { + expect(hashApiKey('k', 'p')).toBe(hashApiKey('k', 'p')); + }); +}); diff --git a/src/modules/auth/api-key-hash.ts b/src/modules/auth/api-key-hash.ts new file mode 100644 index 00000000..1c8f4f7a --- /dev/null +++ b/src/modules/auth/api-key-hash.ts @@ -0,0 +1,14 @@ +import { createHash, createHmac } from 'crypto'; + +/** + * Hash an API key for storage/lookup. With a server-side pepper (`API_KEY_PEPPER`) set, uses HMAC so + * a database leak alone can't precompute candidate hashes against a guessed/user-chosen key. Without + * a pepper it falls back to plain SHA-256 — unchanged behaviour, so existing stored hashes still + * validate. NOTE: enabling (or changing) the pepper invalidates keys hashed before it was set, so it + * is a deploy-time choice; rotate/re-issue keys when turning it on. + */ +export function hashApiKey(rawKey: string, pepper?: string): string { + return pepper + ? createHmac('sha256', pepper).update(rawKey).digest('hex') + : createHash('sha256').update(rawKey).digest('hex'); +} diff --git a/src/modules/auth/auth.service.spec.ts b/src/modules/auth/auth.service.spec.ts index 16edfdca..caede3dc 100644 --- a/src/modules/auth/auth.service.spec.ts +++ b/src/modules/auth/auth.service.spec.ts @@ -2,7 +2,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { UnauthorizedException, NotFoundException } from '@nestjs/common'; -import { createHash } from 'crypto'; +import { createHash, createHmac } from 'crypto'; import { AuthService, resolveSeedApiKey } from './auth.service'; import { ApiKey, ApiKeyRole } from './entities/api-key.entity'; @@ -407,4 +407,35 @@ describe('AuthService', () => { expect(r2.id).toBe(key.id); }); }); + + // ── API_KEY_PEPPER wiring ───────────────────────────────────────── + // Proves the service's hashing path actually reads the env var (not just the pure helper). We + // assert on the keyHash the service QUERIES findOne with, since the mock returns regardless. + describe('hashKey reads API_KEY_PEPPER', () => { + const ORIGINAL_ENV = process.env; + afterEach(() => { + process.env = ORIGINAL_ENV; + }); + + const queriedHash = async (rawKey: string): Promise => { + (repository.findOne as jest.Mock).mockResolvedValue(null); + await expect(service.validateApiKey(rawKey)).rejects.toThrow(UnauthorizedException); + const calls = (repository.findOne as jest.Mock).mock.calls as Array<[{ where: { keyHash: string } }]>; + return calls[0][0].where.keyHash; + }; + + it('hashes with HMAC-SHA256 when the pepper is set', async () => { + process.env = { ...ORIGINAL_ENV, API_KEY_PEPPER: 'server-pepper' }; + const queried = await queriedHash('owa_raw_key'); + expect(queried).toBe(createHmac('sha256', 'server-pepper').update('owa_raw_key').digest('hex')); + expect(queried).not.toBe(createHash('sha256').update('owa_raw_key').digest('hex')); + }); + + it('hashes with plain SHA-256 when the pepper is unset (existing keys keep validating)', async () => { + process.env = { ...ORIGINAL_ENV }; + delete process.env.API_KEY_PEPPER; + const queried = await queriedHash('owa_raw_key'); + expect(queried).toBe(createHash('sha256').update('owa_raw_key').digest('hex')); + }); + }); }); diff --git a/src/modules/auth/auth.service.ts b/src/modules/auth/auth.service.ts index 447679cd..8afefa75 100644 --- a/src/modules/auth/auth.service.ts +++ b/src/modules/auth/auth.service.ts @@ -1,9 +1,11 @@ import { Injectable, NotFoundException, UnauthorizedException, OnModuleInit } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; -import { createHash, randomBytes } from 'crypto'; -import { existsSync, writeFileSync, readFileSync } from 'fs'; +import { randomBytes } from 'crypto'; +import { existsSync, readFileSync } from 'fs'; import { join } from 'path'; +import { writeSecretFile } from '../../common/utils/secret-file'; +import { hashApiKey } from './api-key-hash'; import { ApiKey, ApiKeyRole } from './entities/api-key.entity'; import { CreateApiKeyDto, UpdateApiKeyDto } from './dto'; import { createLogger } from '../../common/services/logger.service'; @@ -53,9 +55,9 @@ export class AuthService implements OnModuleInit { await this.seedApiKey(displayKey, 'Default Admin Key', ApiKeyRole.ADMIN); isNewKey = true; - // Save raw key to file for startup script to read + // Save raw key to file for startup script to read (owner-only — it's the raw admin key). try { - writeFileSync(API_KEY_FILE, displayKey, 'utf-8'); + writeSecretFile(API_KEY_FILE, displayKey); } catch (err) { this.logger.warn('Could not save API key file', { error: String(err) }); } @@ -243,7 +245,7 @@ export class AuthService implements OnModuleInit { } private hashKey(rawKey: string): string { - return createHash('sha256').update(rawKey).digest('hex'); + return hashApiKey(rawKey, process.env.API_KEY_PEPPER); } private isIpAllowed(clientIp: string, allowedIps: string[]): boolean { diff --git a/src/modules/auth/dto/api-key.dto.ts b/src/modules/auth/dto/api-key.dto.ts index 1d4469cd..25b3bfa6 100644 --- a/src/modules/auth/dto/api-key.dto.ts +++ b/src/modules/auth/dto/api-key.dto.ts @@ -1,6 +1,7 @@ -import { IsString, IsOptional, IsEnum, IsArray, IsDateString, MinLength, MaxLength } from 'class-validator'; +import { IsString, IsOptional, IsEnum, IsArray, IsDateString, MinLength, MaxLength, Validate } from 'class-validator'; import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; import { ApiKeyRole } from '../entities/api-key.entity'; +import { IsIpOrCidrConstraint } from './is-ip-or-cidr.validator'; export class CreateApiKeyDto { @ApiProperty({ @@ -28,6 +29,7 @@ export class CreateApiKeyDto { @IsOptional() @IsArray() @IsString({ each: true }) + @Validate(IsIpOrCidrConstraint, { each: true }) allowedIps?: string[]; @ApiPropertyOptional({ @@ -110,6 +112,7 @@ export class UpdateApiKeyDto { @IsOptional() @IsArray() @IsString({ each: true }) + @Validate(IsIpOrCidrConstraint, { each: true }) allowedIps?: string[]; @ApiPropertyOptional() diff --git a/src/modules/auth/dto/is-ip-or-cidr.validator.spec.ts b/src/modules/auth/dto/is-ip-or-cidr.validator.spec.ts new file mode 100644 index 00000000..7584d293 --- /dev/null +++ b/src/modules/auth/dto/is-ip-or-cidr.validator.spec.ts @@ -0,0 +1,65 @@ +import { validate } from 'class-validator'; +import { plainToInstance } from 'class-transformer'; +import { isIpOrCidr } from './is-ip-or-cidr.validator'; +import { CreateApiKeyDto } from './api-key.dto'; + +describe('isIpOrCidr', () => { + it.each([ + '10.0.0.1', + '192.168.1.255', + '255.255.255.255', + '10.0.0.0/8', // the documented allowedIps example + '192.168.0.0/16', + '0.0.0.0/0', + '10.0.0.0/32', + ])('accepts IPv4 literal/CIDR %s', v => { + expect(isIpOrCidr(v)).toBe(true); + }); + + it.each([ + 'not-an-ip', + '300.0.0.1', + '10.0.0.0/33', // IPv4 prefix too long + '10.0.0.0/', // missing bits + '10.0.0.0/x', + '', + ])('rejects malformed %s', v => { + expect(isIpOrCidr(v)).toBe(false); + }); + + // IPv6 is rejected on purpose: the allowedIps matcher (auth.service) is IPv4-only, so an IPv6 + // CIDR would either lock out its own client (/128) or match every IPv6 client (/<=32). The + // validator must not bless an entry the matcher can't enforce. allowedIps is IPv4-only. + it.each(['::1', '2001:db8::1', '::/0', '2001:db8::/32', '::1/128', 'fe80::1%eth0', '::ffff:10.0.0.1'])( + 'rejects IPv6 (unsupported by the matcher) %s', + v => { + expect(isIpOrCidr(v)).toBe(false); + }, + ); +}); + +describe('CreateApiKeyDto allowedIps (@Validate each)', () => { + const errorsFor = async (allowedIps: unknown): Promise => { + const dto = plainToInstance(CreateApiKeyDto, { name: 'valid-name', allowedIps }); + const errors = await validate(dto); + // Scope to the allowedIps field so an unrelated field error can't mask the assertion. + return errors.filter(e => e.property === 'allowedIps').flatMap(e => Object.values(e.constraints ?? {})); + }; + + it('accepts an array of valid IPv4 literals and CIDRs', async () => { + expect(await errorsFor(['10.0.0.1', '10.0.0.0/8'])).toEqual([]); + }); + + it('rejects when any element is malformed (per-element each: true)', async () => { + const msgs = await errorsFor(['10.0.0.1', 'nope', '10.0.0.0/8']); + expect(msgs.length).toBeGreaterThan(0); + }); + + it('rejects an IPv6 element (matcher is IPv4-only)', async () => { + expect((await errorsFor(['2001:db8::/32'])).length).toBeGreaterThan(0); + }); + + it('rejects a non-string element', async () => { + expect((await errorsFor(['10.0.0.1', 42])).length).toBeGreaterThan(0); + }); +}); diff --git a/src/modules/auth/dto/is-ip-or-cidr.validator.ts b/src/modules/auth/dto/is-ip-or-cidr.validator.ts new file mode 100644 index 00000000..238fdd56 --- /dev/null +++ b/src/modules/auth/dto/is-ip-or-cidr.validator.ts @@ -0,0 +1,29 @@ +import { ValidatorConstraint, ValidatorConstraintInterface } from 'class-validator'; +import { isIP } from 'net'; + +/** + * Whether a string is a valid IPv4 address or IPv4 CIDR range (/0-32). + * + * IPv4-only on purpose: the allowedIps matcher (auth.service `isIpAllowed`/`ipInCidr`) is IPv4-only, + * so an IPv6 entry can't be enforced — a /128 host-lock would never match its own client, and an + * IPv6 /<=32 CIDR would match EVERY IPv6 client (an over-broad grant). The validator must not bless + * an entry the matcher can't honor, so it rejects IPv6 (`allowedIps` is documented as IPv4-only). + */ +export function isIpOrCidr(value: unknown): boolean { + if (typeof value !== 'string') return false; + const slash = value.indexOf('/'); + if (slash === -1) return isIP(value) === 4; + if (isIP(value.slice(0, slash)) !== 4) return false; + const bits = value.slice(slash + 1); + return /^\d{1,2}$/.test(bits) && Number(bits) <= 32; +} + +@ValidatorConstraint({ name: 'isIpOrCidr', async: false }) +export class IsIpOrCidrConstraint implements ValidatorConstraintInterface { + validate(value: unknown): boolean { + return isIpOrCidr(value); + } + defaultMessage(): string { + return 'each allowedIps entry must be a valid IPv4 address or IPv4 CIDR range (e.g. 10.0.0.0/8)'; + } +} From 0a56b3acd786d3b4668adc80d187a093a7467cbb Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:24:31 +0700 Subject: [PATCH 08/14] fix(storage): bound tar imports, contain storage keys (#346) --- .env.example | 4 + docs/14-migration-guide.md | 6 +- src/common/storage/storage.service.spec.ts | 92 +++++++++++++++++++ src/common/storage/storage.service.ts | 73 ++++++++++++++- src/common/utils/path-safety.spec.ts | 31 ++++++- src/common/utils/path-safety.ts | 16 ++++ .../plugins/plugin-storage.service.spec.ts | 54 +++++++++++ src/core/plugins/plugin-storage.service.ts | 23 ++++- src/modules/infra/infra.controller.spec.ts | 62 +++++++++++++ src/modules/infra/infra.controller.ts | 19 +++- 10 files changed, 369 insertions(+), 11 deletions(-) create mode 100644 src/core/plugins/plugin-storage.service.spec.ts diff --git a/.env.example b/.env.example index eeec6373..7d4bc115 100644 --- a/.env.example +++ b/.env.example @@ -166,6 +166,10 @@ WEBHOOK_SSRF_PROTECT=true # Server-side media size/time limits: # MEDIA_DOWNLOAD_MAX_BYTES=52428800 # cap remote-URL sends AND inbound media (default 50 MiB; oversized inbound media is dropped, message kept) # MEDIA_DOWNLOAD_TIMEOUT_MS=30000 # abort a slow media download (default 30s) +# Storage import/export limits (ADMIN /infra/storage/* endpoints): +# STORAGE_IMPORT_MAX_BYTES=209715200 # per-entry cap for a tar.gz import; aborts on overflow (default 200 MiB) +# STORAGE_IMPORT_MAX_ENTRIES=100000 # max entries in an import archive; aborts beyond this (default 100000) +# STORAGE_EXPORT_TTL_MS=3600000 # auto-delete an export archive after this long (default 1h) # ============================================================================= # RATE LIMITING diff --git a/docs/14-migration-guide.md b/docs/14-migration-guide.md index 96c08bfe..902f8d84 100644 --- a/docs/14-migration-guide.md +++ b/docs/14-migration-guide.md @@ -156,7 +156,9 @@ curl -s 'http://localhost:2785/api/infra/storage/files/count' \ # Step 2: Export all files as tar.gz curl -s 'http://localhost:2785/api/infra/storage/export' \ -H 'X-API-Key: YOUR_KEY' -# Response: { "message": "Storage export completed", "download": "/app/data/storage-export-xxx.tar.gz" } +# Response: { "message": "Storage export completed", "download": "/app/data/exports/storage-export-xxx.tar.gz" } +# The archive is auto-removed after STORAGE_EXPORT_TTL_MS (default 1h), so re-import it before then. +# It is written under data/ so it survives the restart in Step 4 and stays import-able. # Step 3: Change storage configuration # From: STORAGE_TYPE=local @@ -170,7 +172,7 @@ docker compose up -d curl -X POST 'http://localhost:2785/api/infra/storage/import' \ -H 'X-API-Key: YOUR_KEY' \ -H 'Content-Type: application/json' \ - -d '{"filePath": "/app/data/storage-export-xxx.tar.gz"}' + -d '{"filePath": "/app/data/exports/storage-export-xxx.tar.gz"}' ``` | Scenario | Support | Method | diff --git a/src/common/storage/storage.service.spec.ts b/src/common/storage/storage.service.spec.ts index cce9ef57..89349b78 100644 --- a/src/common/storage/storage.service.spec.ts +++ b/src/common/storage/storage.service.spec.ts @@ -88,3 +88,95 @@ describe('StorageService (local) path traversal protection', () => { expect(count).toBe(1); }); }); + +function makeLocalService(): { service: StorageService; baseDir: string; localPath: string } { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'owa-storage-')); + const localPath = path.join(baseDir, 'media'); + const configService = { + get: (key: string) => (key === 'storage.type' ? 'local' : key === 'storage.localPath' ? localPath : undefined), + } as unknown as ConfigService; + return { service: new StorageService(configService), baseDir, localPath }; +} + +describe('StorageService put/getFile containment is backend-agnostic', () => { + // Force S3 routing with a stub client so the assertion proves the guard runs BEFORE any S3 call + // (i.e. it lives in put/getFile, so the otherwise-unguarded S3 backend is contained too). + function s3Stub(service: StorageService): jest.Mock { + const sendMock = jest.fn(); + const internal = service as unknown as { storageType: string; s3Client: unknown; s3Available: boolean }; + internal.storageType = 's3'; + internal.s3Client = { send: sendMock }; + internal.s3Available = true; + return sendMock; + } + + it('putFile rejects an unsafe key before reaching the S3 backend', async () => { + const { service, baseDir } = makeLocalService(); + const sendMock = s3Stub(service); + + await expect(service.putFile('../evil', Buffer.from('x'))).rejects.toThrow(); + expect(sendMock).not.toHaveBeenCalled(); + + fs.rmSync(baseDir, { recursive: true, force: true }); + }); + + it('getFile rejects an unsafe key before reaching the S3 backend', async () => { + const { service, baseDir } = makeLocalService(); + const sendMock = s3Stub(service); + + await expect(service.getFile('../../etc/passwd')).rejects.toThrow(); + expect(sendMock).not.toHaveBeenCalled(); + + fs.rmSync(baseDir, { recursive: true, force: true }); + }); +}); + +describe('StorageService import resource caps (decompression-bomb defense)', () => { + let baseDir: string; + let localPath: string; + let service: StorageService; + + beforeEach(() => { + ({ service, baseDir, localPath } = makeLocalService()); + }); + + afterEach(() => { + fs.rmSync(baseDir, { recursive: true, force: true }); + delete process.env.STORAGE_IMPORT_MAX_BYTES; + delete process.env.STORAGE_IMPORT_MAX_ENTRIES; + }); + + it('aborts an entry that exceeds the per-entry byte cap, writing nothing', async () => { + process.env.STORAGE_IMPORT_MAX_BYTES = '8'; + const gz = await makeTarGz([{ name: 'bomb.bin', data: 'far-more-than-eight-bytes' }]); + + await expect(service.importFromStream(Readable.from(gz))).rejects.toThrow(/byte|cap|exceed|large/i); + expect(fs.existsSync(path.join(localPath, 'bomb.bin'))).toBe(false); + }); + + it('aborts when the archive exceeds the max entry count', async () => { + process.env.STORAGE_IMPORT_MAX_ENTRIES = '1'; + const gz = await makeTarGz([ + { name: 'a.txt', data: 'a' }, + { name: 'b.txt', data: 'b' }, + ]); + + await expect(service.importFromStream(Readable.from(gz))).rejects.toThrow(/entr/i); + }); + + it('aborts a large multi-chunk entry mid-stream (the payload spans several stream chunks)', async () => { + process.env.STORAGE_IMPORT_MAX_BYTES = '1024'; + // 256 KiB easily spans multiple 64 KiB stream chunks, so this proves the running accumulator + // aborts mid-stream rather than only after the whole entry is buffered. + const gz = await makeTarGz([{ name: 'big.bin', data: 'x'.repeat(256 * 1024) }]); + + await expect(service.importFromStream(Readable.from(gz))).rejects.toThrow(/byte|cap|exceed|large/i); + expect(fs.existsSync(path.join(localPath, 'big.bin'))).toBe(false); + }); + + it('imports normally within the (generous default) caps', async () => { + const gz = await makeTarGz([{ name: 'ok.txt', data: 'fine' }]); + const count = await service.importFromStream(Readable.from(gz)); + expect(count).toBe(1); + }); +}); diff --git a/src/common/storage/storage.service.ts b/src/common/storage/storage.service.ts index f0b6622b..fda78d59 100644 --- a/src/common/storage/storage.service.ts +++ b/src/common/storage/storage.service.ts @@ -15,7 +15,7 @@ import { CreateBucketCommand, } from '@aws-sdk/client-s3'; import { createLogger } from '../services/logger.service'; -import { isPathWithin } from '../utils/path-safety'; +import { isPathWithin, isSafeStorageKey } from '../utils/path-safety'; interface S3Config { endpoint?: string; @@ -25,6 +25,16 @@ interface S3Config { bucket?: string; } +/** Per-entry buffer cap for an import (200 MiB — 4× the inbound media cap). Bounds a decompression bomb. */ +const DEFAULT_IMPORT_MAX_BYTES = 200 * 1024 * 1024; +/** Max number of entries an import archive may contain. Bounds an entry-count DoS. */ +const DEFAULT_IMPORT_MAX_ENTRIES = 100_000; + +function positiveIntFromEnv(name: string, fallback: number): number { + const parsed = Number.parseInt(process.env[name] ?? '', 10); + return Number.isInteger(parsed) && parsed > 0 ? parsed : fallback; +} + @Injectable() export class StorageService { private readonly logger = createLogger('StorageService'); @@ -114,6 +124,11 @@ export class StorageService { } async getFile(filePath: string): Promise { + // Mirror putFile: getLocalFile has its own isPathWithin guard, but getS3File builds + // `media/${filePath}` with none — contain both read backends at this boundary. + if (!isSafeStorageKey(filePath)) { + throw new Error(`Refusing to read an unsafe storage key: ${filePath}`); + } if (this.storageType === 's3' && this.s3Client && this.s3Available) { return this.getS3File(filePath); } @@ -121,6 +136,11 @@ export class StorageService { } async putFile(filePath: string, data: Buffer): Promise { + // Centralized containment so BOTH backends inherit it: putLocalFile has its own isPathWithin + // guard, but putS3File builds `media/${filePath}` with none — reject a traversing key here. + if (!isSafeStorageKey(filePath)) { + throw new Error(`Refusing to store an unsafe storage key: ${filePath}`); + } if (this.storageType === 's3' && this.s3Client && this.s3Available) { return this.putS3File(filePath, data); } @@ -191,18 +211,59 @@ export class StorageService { // Import - Extract tar.gz stream to current storage // ============================================================================ + // Best-effort, NOT atomic: a single bad/traversing entry is skipped and the rest still import, and a + // resource-cap breach aborts the rest but KEEPS the entries already written (no rollback). Callers + // re-running an import is safe (putFile overwrites). A staging-dir + atomic promote would make it + // transactional, but is out of scope here. async importFromStream(inputStream: Readable): Promise { let importedCount = 0; + let entryCount = 0; + const maxEntryBytes = positiveIntFromEnv('STORAGE_IMPORT_MAX_BYTES', DEFAULT_IMPORT_MAX_BYTES); + const maxEntries = positiveIntFromEnv('STORAGE_IMPORT_MAX_ENTRIES', DEFAULT_IMPORT_MAX_ENTRIES); const extract = tar.extract(); const gunzip = createGunzip(); return new Promise((resolve, reject) => { + let settled = false; + // Abort the whole import: a per-entry overflow or too many entries is a (zip-bomb) attack, not + // a per-file skip — tear down the pipeline and reject so nothing further is buffered or written. + const fail = (err: Error): void => { + if (settled) return; + settled = true; + extract.destroy(); + reject(err); + }; + extract.on('entry', (header, stream, next) => { + if (settled) { + stream.resume(); + return; + } + if (++entryCount > maxEntries) { + stream.resume(); + fail(new Error(`Import aborted: archive exceeds the ${maxEntries}-entry limit`)); + return; + } + const chunks: Buffer[] = []; + let entryBytes = 0; + let entryAborted = false; + + stream.on('data', (chunk: Buffer) => { + if (entryAborted || settled) return; + entryBytes += chunk.length; + if (entryBytes > maxEntryBytes) { + entryAborted = true; + stream.resume(); // drain the remainder so the source can end + fail(new Error(`Import aborted: entry "${header.name}" exceeds the ${maxEntryBytes}-byte per-entry cap`)); + } else { + chunks.push(chunk); + } + }); - stream.on('data', (chunk: Buffer) => chunks.push(chunk)); stream.on('end', () => { + if (entryAborted || settled) return; const data = Buffer.concat(chunks); this.putFile(header.name, data) .then(() => { @@ -219,13 +280,15 @@ export class StorageService { }); extract.on('finish', () => { + if (settled) return; + settled = true; this.logger.log(`Import completed: ${importedCount} files`); resolve(importedCount); }); extract.on('error', (err: Error) => { this.logger.error('Import failed', String(err)); - reject(err); + fail(err); }); inputStream.pipe(gunzip).pipe(extract); @@ -264,7 +327,9 @@ export class StorageService { throw new Error(`Refusing to read outside storage root: ${filePath}`); } const fullPath = path.join(this.localPath, filePath); - return Promise.resolve(fs.readFileSync(fullPath)); + // Async read so the export loop (the only caller) yields the event loop per file instead of + // blocking it with a synchronous read for every media file. + return fs.promises.readFile(fullPath); } private putLocalFile(filePath: string, data: Buffer): Promise { diff --git a/src/common/utils/path-safety.spec.ts b/src/common/utils/path-safety.spec.ts index de83e937..4d6049af 100644 --- a/src/common/utils/path-safety.spec.ts +++ b/src/common/utils/path-safety.spec.ts @@ -1,5 +1,5 @@ import * as path from 'path'; -import { isPathWithin } from './path-safety'; +import { isPathWithin, isSafeStorageKey } from './path-safety'; describe('isPathWithin', () => { const root = path.resolve('/srv/app/data'); @@ -25,3 +25,32 @@ describe('isPathWithin', () => { expect(isPathWithin('/srv/app/data', '/srv/app/data-evil/x')).toBe(false); }); }); + +describe('isSafeStorageKey', () => { + it.each([ + 'file.jpg', + 'sessionId/messageId.jpg', + 'a/b/c.txt', + 'group:sid:123@g.us.json', // plugin/JID-style keys must survive (':' '@' '.' '-') + ])('accepts the safe relative key %s', k => { + expect(isSafeStorageKey(k)).toBe(true); + }); + + it.each([ + '../evil.txt', + 'a/../../etc/passwd', + 'media/../../../secret', + '/etc/passwd', // absolute + '..', + 'a\\..\\b', // backslash traversal (pins the split on '\\' too) + '', // empty + ])('rejects the traversing/absolute key %j', k => { + expect(isSafeStorageKey(k)).toBe(false); + }); + + it('rejects a key containing a NUL or other control character (it reaches the raw S3 object key)', () => { + expect(isSafeStorageKey(`foo${String.fromCharCode(0)}.txt`)).toBe(false); // NUL + expect(isSafeStorageKey(`bar${String.fromCharCode(9)}.txt`)).toBe(false); // tab + expect(isSafeStorageKey(`baz${String.fromCharCode(31)}.txt`)).toBe(false); // unit separator + }); +}); diff --git a/src/common/utils/path-safety.ts b/src/common/utils/path-safety.ts index ca816a95..a8461198 100644 --- a/src/common/utils/path-safety.ts +++ b/src/common/utils/path-safety.ts @@ -15,3 +15,19 @@ export function isPathWithin(root: string, target: string): boolean { const resolvedTarget = path.resolve(resolvedRoot, target); return resolvedTarget === resolvedRoot || resolvedTarget.startsWith(resolvedRoot + path.sep); } + +/** + * Returns true if `key` is a safe, contained relative storage key: a non-empty relative path with no + * `..` traversal segment. Used to validate untrusted archive entry names / object keys at the + * backend-agnostic `putFile`/`getFile` boundary so an S3 key (which has no host filesystem root to + * check against `isPathWithin`) still can't escape the intended `media/` prefix. Ordinary keys — + * including plugin/JID-style ones with `:`, `@`, `.`, `-` — are preserved. + */ +export function isSafeStorageKey(key: string): boolean { + if (typeof key !== 'string' || key.length === 0) return false; + // Reject NUL / control chars: harmless on the local FS but a NUL would reach the raw S3 object Key. + // eslint-disable-next-line no-control-regex + if (/[\u0000-\u001f]/.test(key)) return false; + if (path.isAbsolute(key)) return false; + return !key.split(/[/\\]/).includes('..'); +} diff --git a/src/core/plugins/plugin-storage.service.spec.ts b/src/core/plugins/plugin-storage.service.spec.ts new file mode 100644 index 00000000..0d0e24d0 --- /dev/null +++ b/src/core/plugins/plugin-storage.service.spec.ts @@ -0,0 +1,54 @@ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { ConfigService } from '@nestjs/config'; +import { PluginStorageService } from './plugin-storage.service'; + +describe('PluginStorageService sandboxed per-plugin storage containment', () => { + let dataDir: string; + let service: PluginStorageService; + let storage: ReturnType; + const pluginId = 'demo-plugin'; + + beforeEach(() => { + dataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'owa-plugindata-')); + const configService = { + get: (k: string) => (k === 'dataDir' ? dataDir : undefined), + } as unknown as ConfigService; + service = new PluginStorageService(configService); + storage = service.createPluginStorage(pluginId); + }); + + afterEach(() => { + fs.rmSync(dataDir, { recursive: true, force: true }); + }); + + it('round-trips a normal key', async () => { + await storage.set('state', { a: 1 }); + expect(await storage.get('state')).toEqual({ a: 1 }); + await storage.delete('state'); + expect(await storage.get('state')).toBeNull(); + }); + + it('preserves JID-style keys containing : @ . -', async () => { + await storage.set('group:sess-1:12345@g.us', { announced: true }); + expect(await storage.get('group:sess-1:12345@g.us')).toEqual({ announced: true }); + }); + + it('rejects a traversing set and writes nothing outside the plugin dir', async () => { + await expect(storage.set('../../escape', { x: 1 })).rejects.toThrow(); + expect(fs.existsSync(path.join(dataDir, 'escape.json'))).toBe(false); + expect(fs.existsSync(path.join(dataDir, 'plugins', 'escape.json'))).toBe(false); + }); + + it('refuses a traversing get WITHOUT reading the real outside file it targets', async () => { + // Place a real JSON file at the location the malicious key would resolve to + // (pluginDir/../../secret.json -> dataDir/secret.json). Containment must return null, not its content. + fs.writeFileSync(path.join(dataDir, 'secret.json'), JSON.stringify({ topsecret: true })); + expect(await storage.get('../../secret')).toBeNull(); + }); + + it('rejects a traversing delete', async () => { + await expect(storage.delete('../../escape')).rejects.toThrow(); + }); +}); diff --git a/src/core/plugins/plugin-storage.service.ts b/src/core/plugins/plugin-storage.service.ts index c1fe47a5..e0f90b75 100644 --- a/src/core/plugins/plugin-storage.service.ts +++ b/src/core/plugins/plugin-storage.service.ts @@ -3,6 +3,7 @@ import { ConfigService } from '@nestjs/config'; import * as fs from 'fs'; import * as path from 'path'; import { createLogger } from '../../common/services/logger.service'; +import { isPathWithin } from '../../common/utils/path-safety'; import { PluginStatus, PluginStorage, PluginRegistryEntry } from './plugin.interfaces'; @Injectable() @@ -131,9 +132,19 @@ export class PluginStorageService { const logger = this.logger; + // Containment: a plugin storage key must resolve INSIDE its own sandbox dir. path.join normalizes + // `..`, so a key like `../../x` would otherwise escape and clobber another plugin's data, the + // registry, or .env.generated. Reject anything that escapes; JID chars (`:`,`@`,`.`,`-`) are fine. + const resolveKeyPath = (key: string): string | null => + isPathWithin(pluginDataDir, `${key}.json`) ? path.join(pluginDataDir, `${key}.json`) : null; + return { get: (key: string): Promise => { - const filePath = path.join(pluginDataDir, `${key}.json`); + const filePath = resolveKeyPath(key); + if (!filePath) { + logger.warn(`Refusing to read plugin data with an unsafe key: ${pluginId}/${key}`); + return Promise.resolve(null); + } try { if (fs.existsSync(filePath)) { const content = fs.readFileSync(filePath, 'utf-8'); @@ -146,7 +157,10 @@ export class PluginStorageService { }, set: (key: string, value: T): Promise => { - const filePath = path.join(pluginDataDir, `${key}.json`); + const filePath = resolveKeyPath(key); + if (!filePath) { + return Promise.reject(new Error(`Unsafe plugin storage key (escapes sandbox): ${key}`)); + } try { fs.writeFileSync(filePath, JSON.stringify(value, null, 2)); return Promise.resolve(); @@ -157,7 +171,10 @@ export class PluginStorageService { }, delete: (key: string): Promise => { - const filePath = path.join(pluginDataDir, `${key}.json`); + const filePath = resolveKeyPath(key); + if (!filePath) { + return Promise.reject(new Error(`Unsafe plugin storage key (escapes sandbox): ${key}`)); + } try { if (fs.existsSync(filePath)) { fs.unlinkSync(filePath); diff --git a/src/modules/infra/infra.controller.spec.ts b/src/modules/infra/infra.controller.spec.ts index d27ecdac..079acde3 100644 --- a/src/modules/infra/infra.controller.spec.ts +++ b/src/modules/infra/infra.controller.spec.ts @@ -1,4 +1,7 @@ import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { Readable } from 'stream'; import { Reflector } from '@nestjs/core'; import { BadRequestException } from '@nestjs/common'; @@ -372,3 +375,62 @@ describe('InfraController.getConfig (#226)', () => { expect(JSON.stringify(cfg)).not.toContain('"ak"'); }); }); + +describe('InfraController.exportStorage keeps the export import-able and sweeps it', () => { + function buildController(storage: Partial<{ createExportStream: jest.Mock }>) { + return new InfraController( + {} as never, + {} as never, + {} as never, + {} as never, + {} as never, + {} as never, + storage as never, + {} as never, + ); + } + + // fs.existsSync is globally mocked in this file, so probe the real filesystem via fs.promises.access. + const exists = (p: string): Promise => + fs.promises + .access(p) + .then(() => true) + .catch(() => false); + + // Poll (don't sleep a fixed time) so the sweep assertion isn't flaky under CI load. + const waitForGone = async (p: string, timeoutMs = 3000): Promise => { + const start = Date.now(); + while (await exists(p)) { + if (Date.now() - start > timeoutMs) throw new Error(`file was not swept in time: ${p}`); + await new Promise(resolve => setTimeout(resolve, 10)); + } + }; + + let cwdSpy: jest.SpyInstance | undefined; + let cwd: string | undefined; + + afterEach(() => { + cwdSpy?.mockRestore(); + if (cwd) fs.rmSync(cwd, { recursive: true, force: true }); + cwdSpy = undefined; + cwd = undefined; + delete process.env.STORAGE_EXPORT_TTL_MS; + }); + + it('writes under data/exports (so it stays import-able + survives restart) and TTL-sweeps it', async () => { + cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'owa-cwd-')); + cwdSpy = jest.spyOn(process, 'cwd').mockReturnValue(cwd); + process.env.STORAGE_EXPORT_TTL_MS = '30'; + const createExportStream = jest.fn().mockResolvedValue(Readable.from([Buffer.from('archive-bytes')])); + const controller = buildController({ createExportStream }); + + const result = await controller.exportStorage(); + + // Import-able: under /exports — the import handler only accepts paths inside data/. + expect(result.download.startsWith(path.join(cwd, 'data', 'exports'))).toBe(true); + expect(await exists(result.download)).toBe(true); + + await waitForGone(result.download); + expect(await exists(result.download)).toBe(false); + }); +}); diff --git a/src/modules/infra/infra.controller.ts b/src/modules/infra/infra.controller.ts index 24ab0786..d42a65fb 100644 --- a/src/modules/infra/infra.controller.ts +++ b/src/modules/infra/infra.controller.ts @@ -14,6 +14,7 @@ import { ShutdownService } from '../../common/services/shutdown.service'; import { createLogger } from '../../common/services/logger.service'; import * as fs from 'fs'; import * as path from 'path'; +import { randomUUID } from 'crypto'; import * as dotenv from 'dotenv'; interface InfraStatus { @@ -858,7 +859,16 @@ export class InfraController { // Note: In production, this would return a StreamableFile // For simplicity, we'll save to a temp file and return the path const stream = await this.storageService.createExportStream(); - const exportPath = path.join(process.cwd(), 'data', `storage-export-${Date.now()}.tar.gz`); + // Keep the export INSIDE data/ (under data/exports/): the import handler only accepts paths under + // data/, and the documented backend-migration flow re-imports this file AFTER a container restart, + // so it must live on the persistent volume — the OS temp dir is wiped on restart. The original + // unbounded-accumulation leak is addressed by the TTL sweep below + a collision-proof filename + // (a per-call UUID), not by relocating off the volume. + const exportDir = path.join(process.cwd(), 'data', 'exports'); + if (!fs.existsSync(exportDir)) { + fs.mkdirSync(exportDir, { recursive: true }); + } + const exportPath = path.join(exportDir, `storage-export-${Date.now()}-${randomUUID()}.tar.gz`); const writeStream = fs.createWriteStream(exportPath); stream.pipe(writeStream); @@ -868,6 +878,13 @@ export class InfraController { writeStream.on('error', reject); }); + // Sweep the throwaway archive so repeated exports don't accumulate on the data volume. + const ttlRaw = Number.parseInt(process.env.STORAGE_EXPORT_TTL_MS ?? '', 10); + const ttlMs = Number.isInteger(ttlRaw) && ttlRaw > 0 ? ttlRaw : 60 * 60 * 1000; // default 1h + setTimeout(() => { + fs.promises.unlink(exportPath).catch(() => undefined); + }, ttlMs).unref(); + return { message: 'Storage export completed', download: exportPath, From eeaaea285632f0edd775552e73851ea2ff384842 Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:24:31 +0700 Subject: [PATCH 09/14] fix(session): reconcile late acks, serialize reactions (#348) --- src/core/hooks/hook-manager.service.spec.ts | 30 +++++ src/core/hooks/hook-manager.service.ts | 4 +- src/modules/session/session.service.spec.ts | 124 ++++++++++++++++++- src/modules/session/session.service.ts | 128 ++++++++++++++------ 4 files changed, 244 insertions(+), 42 deletions(-) diff --git a/src/core/hooks/hook-manager.service.spec.ts b/src/core/hooks/hook-manager.service.spec.ts index 8725dd5e..919999f6 100644 --- a/src/core/hooks/hook-manager.service.spec.ts +++ b/src/core/hooks/hook-manager.service.spec.ts @@ -95,6 +95,36 @@ describe('HookManager', () => { expect(res.continue).toBe(true); }); + it('discards the mutated data of a handler that also returns an error (error means discard output)', async () => { + // A handler that signals an error must NOT have its (possibly partial/corrupted) mutation applied — + // otherwise a half-transformed payload reaches persistence/webhooks/WS presented as success. + hm.register( + 'bad', + 'message:received', + async () => ({ continue: true, data: 'CORRUPTED', error: new Error('partial failure') }), + 10, + ); + + const res = await hm.execute('message:received', 'original', { source: 'test' }); + + expect(res.continue).toBe(true); + expect(res.data).toBe('original'); // the errored handler's mutation is dropped + }); + + it('discards an errored handler mutation even on the stop path (continue:false + error)', async () => { + hm.register( + 'bad', + 'message:received', + async () => ({ continue: false, data: 'CORRUPTED', error: new Error('x') }), + 10, + ); + + const res = await hm.execute('message:received', 'original', { source: 'test' }); + + expect(res.continue).toBe(false); // the chain still stops + expect(res.data).toBe('original'); // but the errored mutation is not carried out on stop + }); + it('register/unregister/hasHooks/getHookCount track registrations', () => { expect(hm.hasHooks('session:created')).toBe(false); const id = hm.register('p', 'session:created', async ctx => ({ continue: true, data: ctx.data })); diff --git a/src/core/hooks/hook-manager.service.ts b/src/core/hooks/hook-manager.service.ts index 7a7bd6f6..c825b2e1 100644 --- a/src/core/hooks/hook-manager.service.ts +++ b/src/core/hooks/hook-manager.service.ts @@ -130,7 +130,9 @@ export class HookManager { ctx.data = currentData; const result = await registration.handler(ctx); - if (result.data !== undefined) { + // A handler that reports an error discards its output: do NOT apply its (possibly partial or + // corrupted) data mutation, even though HookResult allows returning data and error together. + if (result.error === undefined && result.data !== undefined) { currentData = result.data as T; } diff --git a/src/modules/session/session.service.spec.ts b/src/modules/session/session.service.spec.ts index b35ee95a..3911233f 100644 --- a/src/modules/session/session.service.spec.ts +++ b/src/modules/session/session.service.spec.ts @@ -2,7 +2,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken, getDataSourceToken } from '@nestjs/typeorm'; import { Repository, DataSource, In } from 'typeorm'; import { NotFoundException, ConflictException, BadRequestException } from '@nestjs/common'; -import { SessionService } from './session.service'; +import { SessionService, ACK_RECONCILE_DELAY_MS } from './session.service'; import { Session, SessionStatus } from './entities/session.entity'; import { Message, MessageStatus } from '../message/entities/message.entity'; import { EngineFactory } from '../../engine/engine.factory'; @@ -641,6 +641,128 @@ describe('SessionService', () => { expect(dispatchedEvents('message.failed')).toHaveLength(0); }); + it('retries the ack update once after a delay when the row is not yet matchable (ack before commit)', async () => { + const callbacks = await startAndCaptureCallbacks(); + (messageRepository.update as jest.Mock) + .mockClear() + .mockResolvedValueOnce({ affected: 0 }) // send's 2nd save (waMessageId) not committed yet + .mockResolvedValueOnce({ affected: 1 }); // retry now matches the row + + jest.useFakeTimers(); + try { + callbacks.onMessageAck!('wa-out-1', 'delivered'); + await jest.advanceTimersByTimeAsync(0); // flush the first update's microtasks + expect(messageRepository.update as jest.Mock).toHaveBeenCalledTimes(1); + + await jest.advanceTimersByTimeAsync(ACK_RECONCILE_DELAY_MS); + expect(messageRepository.update as jest.Mock).toHaveBeenCalledTimes(2); + } finally { + jest.useRealTimers(); + } + }); + + it('does not schedule a retry when the first ack update advances a row', async () => { + const callbacks = await startAndCaptureCallbacks(); + (messageRepository.update as jest.Mock).mockClear().mockResolvedValue({ affected: 1 }); + + jest.useFakeTimers(); + try { + callbacks.onMessageAck!('wa-out-1', 'delivered'); + await jest.advanceTimersByTimeAsync(ACK_RECONCILE_DELAY_MS); + expect(messageRepository.update as jest.Mock).toHaveBeenCalledTimes(1); + } finally { + jest.useRealTimers(); + } + }); + + it('handles a rejected ack update without an unhandled rejection', async () => { + const callbacks = await startAndCaptureCallbacks(); + (messageRepository.update as jest.Mock).mockClear().mockRejectedValue(new Error('data DB down')); + + // Must not throw synchronously; the .catch keeps the rejection from escaping to the global backstop + // (a missing .catch here would surface as an unhandled rejection and fail the suite). + callbacks.onMessageAck!('wa-out-1', 'delivered'); + await flush(); + await flush(); + + expect(messageRepository.update as jest.Mock).toHaveBeenCalled(); + }); + + it('serializes concurrent reactions on the same message so neither sender is clobbered', async () => { + const callbacks = await startAndCaptureCallbacks(); + + // Simulate a real DB: each findOne returns a FRESH snapshot of the persisted row, and save + // writes it back. Without per-message serialization the two handlers read the same empty + // snapshot and the second save clobbers the first sender's reaction. + type Row = { metadata?: Record }; + const clone = (r: Row): Row => JSON.parse(JSON.stringify(r)) as Row; + let stored: Row = { metadata: {} }; + (messageRepository.findOne as jest.Mock).mockImplementation(() => Promise.resolve(clone(stored))); + (messageRepository.save as jest.Mock).mockImplementation((m: Row) => { + stored = clone(m); + return Promise.resolve(m); + }); + + callbacks.onMessageReaction!({ messageId: 'wa-1', chatId: 'c', senderId: 'alice', reaction: '👍' }); + callbacks.onMessageReaction!({ messageId: 'wa-1', chatId: 'c', senderId: 'bob', reaction: '🎉' }); + + for (let i = 0; i < 5; i++) await flush(); + + expect(stored.metadata?.reactions).toEqual({ alice: '👍', bob: '🎉' }); + }); + + it('removes a sender reaction on a cleared reaction event (delete branch)', async () => { + const callbacks = await startAndCaptureCallbacks(); + type Row = { metadata?: Record }; + const clone = (r: Row): Row => JSON.parse(JSON.stringify(r)) as Row; + let stored: Row = { metadata: { reactions: { alice: '👍', bob: '🎉' } } }; + (messageRepository.findOne as jest.Mock).mockImplementation(() => Promise.resolve(clone(stored))); + (messageRepository.save as jest.Mock).mockImplementation((m: Row) => { + stored = clone(m); + return Promise.resolve(m); + }); + + callbacks.onMessageReaction!({ messageId: 'wa-1', chatId: 'c', senderId: 'alice', reaction: '' }); + + for (let i = 0; i < 3; i++) await flush(); + + expect(stored.metadata?.reactions).toEqual({ bob: '🎉' }); // alice removed, bob preserved + }); + + it('a failed reaction write does not block a later reaction on the same message', async () => { + const callbacks = await startAndCaptureCallbacks(); + type Row = { metadata?: Record }; + const clone = (r: Row): Row => JSON.parse(JSON.stringify(r)) as Row; + let stored: Row = { metadata: {} }; + (messageRepository.findOne as jest.Mock).mockImplementation(() => Promise.resolve(clone(stored))); + (messageRepository.save as jest.Mock) + .mockRejectedValueOnce(new Error('write blip')) // alice's write fails + .mockImplementation((m: Row) => { + stored = clone(m); + return Promise.resolve(m); + }); + + callbacks.onMessageReaction!({ messageId: 'wa-1', chatId: 'c', senderId: 'alice', reaction: '👍' }); + callbacks.onMessageReaction!({ messageId: 'wa-1', chatId: 'c', senderId: 'bob', reaction: '🎉' }); + + for (let i = 0; i < 5; i++) await flush(); + + expect(stored.metadata?.reactions).toEqual({ bob: '🎉' }); // bob applied despite alice's failure + }); + + it('cleans up the per-message serialization entry after the chain drains (no leak)', async () => { + const callbacks = await startAndCaptureCallbacks(); + (messageRepository.findOne as jest.Mock).mockResolvedValue({ metadata: {} }); + (messageRepository.save as jest.Mock).mockResolvedValue(undefined); + + callbacks.onMessageReaction!({ messageId: 'wa-1', chatId: 'c', senderId: 'alice', reaction: '👍' }); + + for (let i = 0; i < 3; i++) await flush(); + + const chains = (service as unknown as { reactionChains: Map }).reactionChains; + expect(chains.size).toBe(0); + }); + it('dispatches message.received (not message.sent) on an incoming message event', async () => { const callbacks = await startAndCaptureCallbacks(); diff --git a/src/modules/session/session.service.ts b/src/modules/session/session.service.ts index 73431b0b..7560a163 100644 --- a/src/modules/session/session.service.ts +++ b/src/modules/session/session.service.ts @@ -20,6 +20,7 @@ import { ChatState, DeliveryStatus, IncomingMessage, + ReactionEvent, } from '../../engine/interfaces/whatsapp-engine.interface'; import { createLogger } from '../../common/services/logger.service'; import { EventsGateway } from '../events/events.gateway'; @@ -45,6 +46,12 @@ const RECONNECT_BASE_DELAY_MIN_MS = 1000; const RECONNECT_BASE_DELAY_MAX_MS = 300_000; const RECONNECT_MAX_ATTEMPTS_CAP = 20; const RECONNECT_DELAY_CAP_MS = 3_600_000; +/** + * Delay before retrying an ack UPDATE that matched 0 rows. A fast delivered/read ack can arrive before + * the send's 2nd save (which writes waMessageId) has committed, so the first UPDATE finds no row. One + * retry after this delay closes that race; the forward-only transition guard keeps it idempotent. + */ +export const ACK_RECONCILE_DELAY_MS = 750; const clampNumber = (n: number, min: number, max: number): number => Math.min(Math.max(n, min), max); @@ -107,6 +114,11 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat // awaited hook and orphan an engine the lifecycle could never destroy. private initializingSessions: Set = new Set(); + // Serializes the read-modify-write of a message's reactions map per `${sessionId}:${waMessageId}`, + // so two concurrent reaction events on the same message don't clobber each other (both read the + // same snapshot, both full-row save, last writer wins). Entries are deleted once their chain drains. + private reactionChains: Map> = new Map(); + constructor( @InjectRepository(Session, 'data') private readonly sessionRepository: Repository, @@ -601,25 +613,44 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat // fire-and-forget writes race-safe at the DB level. const messageStatus = deliveryStatusToMessageStatus(status); if (messageStatus) { - void this.messageRepository - // Scope by sessionId: waMessageId is unique per account/chat, not global — - // an ack on one session must never advance a same-id row in another session. - .update( - { sessionId: id, waMessageId: messageId, status: In(ackStatusTransitionFrom(messageStatus)) }, - { status: messageStatus }, - ) - .then(result => { - // affected:0 — the row was not advanced: either the send's 2nd save (which sets - // waMessageId) hasn't committed yet, or the status is already at/above the target. - if (result.affected === 0) { - this.logger.debug(`Message ack ${messageId}: no status row advanced to ${messageStatus} (${status})`, { - sessionId: id, - messageId, - status, - action: 'message_ack_noop', - }); - } + // Scope by sessionId: waMessageId is unique per account/chat, not global — an ack on one + // session must never advance a same-id row in another session. The In() guard makes the + // UPDATE forward-only (a late/out-of-order ack can't downgrade) and idempotent on retry. + const advanceAck = (): Promise => + this.messageRepository + .update( + { sessionId: id, waMessageId: messageId, status: In(ackStatusTransitionFrom(messageStatus)) }, + { status: messageStatus }, + ) + .then(result => result.affected ?? 0); + + const logNoop = (): void => + this.logger.debug(`Message ack ${messageId}: no status row advanced to ${messageStatus} (${status})`, { + sessionId: id, + messageId, + status, + action: 'message_ack_noop', }); + + const onAckError = (err: unknown): void => + this.logger.error(`Failed to advance ack for ${messageId}`, String(err)); + + void advanceAck() + .then(affected => { + if (affected > 0) return; + // affected:0 — most likely the send's 2nd save (which writes waMessageId) hasn't committed + // yet, so the row isn't matchable. Each ack is one-shot (WhatsApp won't necessarily resend), + // so retry ONCE after a short delay to close that race rather than leave it stuck at SENT. + const timer = setTimeout(() => { + void advanceAck() + .then(retried => { + if (retried === 0) logNoop(); + }) + .catch(onAckError); + }, ACK_RECONCILE_DELAY_MS); + timer.unref?.(); + }) + .catch(onAckError); } // Push the live delivery/read tick to the dashboard over the websocket (neutral status). @@ -676,28 +707,18 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat action: 'message_reaction_received', }); - void this.messageRepository - .findOne({ where: { sessionId: id, waMessageId: event.messageId } }) - .then(async msg => { - if (!msg) return; - const metadata = msg.metadata || {}; - const reactions = (metadata.reactions as Record) || {}; - - if (!event.reaction) { - delete reactions[event.senderId]; - } else { - reactions[event.senderId] = event.reaction; - } - - metadata.reactions = reactions; - msg.metadata = metadata; - await this.messageRepository.save(msg); - - this.eventsGateway.emitMessageReaction(id, { ...event, reactions }); - }) - .catch(err => { - this.logger.error(`Failed to update message reaction: ${event.messageId}`, String(err)); - }); + // Serialize per message so two concurrent reactions don't read the same snapshot and clobber + // each other on the full-row save. A prior chain's failure must not block later reactions. + const key = `${id}:${event.messageId}`; + const prior = this.reactionChains.get(key) ?? Promise.resolve(); + const next = prior.catch(() => undefined).then(() => this.applyReaction(id, event)); + this.reactionChains.set(key, next); + void next.finally(() => { + // Clean up only if no newer reaction chained after us, so the map can't leak per message. + if (this.reactionChains.get(key) === next) { + this.reactionChains.delete(key); + } + }); }, onDisconnected: (reason: string): void => { this.logger.warn(`Session disconnected: ${reason}`, { @@ -763,6 +784,33 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat }); } + /** + * Apply one reaction event to the stored message's reactions map (read-modify-write of the JSON + * column). Invoked through the per-message serialization chain in onMessageReaction, so concurrent + * reactions on the same message run sequentially and don't clobber each other. + */ + private async applyReaction(id: string, event: ReactionEvent): Promise { + try { + const msg = await this.messageRepository.findOne({ where: { sessionId: id, waMessageId: event.messageId } }); + if (!msg) return; + + const metadata = msg.metadata || {}; + const reactions = (metadata.reactions as Record) || {}; + if (!event.reaction) { + delete reactions[event.senderId]; + } else { + reactions[event.senderId] = event.reaction; + } + metadata.reactions = reactions; + msg.metadata = metadata; + await this.messageRepository.save(msg); + + this.eventsGateway.emitMessageReaction(id, { ...event, reactions }); + } catch (err) { + this.logger.error(`Failed to update message reaction: ${event.messageId}`, String(err)); + } + } + private scheduleReconnect(id: string, session: Session): void { const state = this.reconnectStates.get(id); if (!state) return; From d23dbbf90eb27af2e2e9f544d345a7b47b146411 Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:26:55 +0700 Subject: [PATCH 10/14] fix(contract): webhook timeout, bounded shutdown, 501 (#350) --- .env.example | 13 ++++-- docs/22-n8n-integration.md | 8 ++++ src/common/cache/cache.service.spec.ts | 44 +++++++++++++++++++ src/common/cache/cache.service.ts | 24 +++++++++- src/config/env.validation.spec.ts | 12 +++++ src/config/env.validation.ts | 12 +++++ .../adapters/whatsapp-web-js.adapter.ts | 13 +++--- src/modules/events/events.gateway.spec.ts | 5 ++- src/modules/events/events.gateway.ts | 8 +--- src/modules/metrics/metrics.service.spec.ts | 23 +++++++++- src/modules/metrics/metrics.service.ts | 20 ++++++++- .../processors/webhook.processor.spec.ts | 21 ++++++++- .../queue/processors/webhook.processor.ts | 5 ++- src/modules/webhook/webhook.service.spec.ts | 19 +++++++- src/modules/webhook/webhook.service.ts | 8 +++- 15 files changed, 208 insertions(+), 27 deletions(-) create mode 100644 src/common/cache/cache.service.spec.ts diff --git a/.env.example b/.env.example index 7d4bc115..ebb7d251 100644 --- a/.env.example +++ b/.env.example @@ -172,10 +172,15 @@ WEBHOOK_SSRF_PROTECT=true # STORAGE_EXPORT_TTL_MS=3600000 # auto-delete an export archive after this long (default 1h) # ============================================================================= -# RATE LIMITING -# ============================================================================= -RATE_LIMIT_TTL=60 # Time window in seconds -RATE_LIMIT_MAX=100 # Max requests per window +# RATE LIMITING (all TTLs are in MILLISECONDS) +# ============================================================================= +# The "medium" tier is the one enforced on the API; short/long are optional extra tiers. +RATE_LIMIT_MEDIUM_TTL=60000 # window in ms (default 60000 = 60s) +RATE_LIMIT_MEDIUM_LIMIT=100 # max requests per window +# RATE_LIMIT_SHORT_TTL=1000 # 1s burst window (default) +# RATE_LIMIT_SHORT_LIMIT=10 +# RATE_LIMIT_LONG_TTL=3600000 # 1h window (default) +# RATE_LIMIT_LONG_LIMIT=1000 # ============================================================================= # PLUGINS diff --git a/docs/22-n8n-integration.md b/docs/22-n8n-integration.md index 81bab449..66acd97b 100644 --- a/docs/22-n8n-integration.md +++ b/docs/22-n8n-integration.md @@ -102,6 +102,8 @@ Start workflows when WhatsApp events occur. "event": "message.received", "timestamp": "2024-01-15T10:30:00Z", "sessionId": "default", + "idempotencyKey": "a1b2c3d4e5f6...", + "deliveryId": "9f8e7d6c5b4a...", "data": { "id": "3EB0F5A2B4C...", "chatId": "628123456789@c.us", @@ -113,6 +115,12 @@ Start workflows when WhatsApp events occur. } ``` +> **Deduplication.** Every delivery includes `idempotencyKey` and `deliveryId` in the body **and** as the +> `X-OpenWA-Idempotency-Key` / `X-OpenWA-Delivery-Id` headers. `idempotencyKey` is **stable across retries** +> of the same event, while `deliveryId` is unique per HTTP attempt. Because a webhook can be retried, add a +> dedup step keyed on `idempotencyKey` (e.g. an n8n IF or "Remove Duplicates" node) so a retried delivery +> isn't processed twice. + ## Example Workflows ### 1. Auto-Reply Bot diff --git a/src/common/cache/cache.service.spec.ts b/src/common/cache/cache.service.spec.ts new file mode 100644 index 00000000..c1845977 --- /dev/null +++ b/src/common/cache/cache.service.spec.ts @@ -0,0 +1,44 @@ +import { ConfigService } from '@nestjs/config'; +import { CacheService, CACHE_QUIT_TIMEOUT_MS } from './cache.service'; + +describe('CacheService.onModuleDestroy (bounded shutdown)', () => { + const makeService = (): CacheService => { + const configService = { get: jest.fn().mockReturnValue(false) } as unknown as ConfigService; + return new CacheService(configService); + }; + const withRedis = (service: CacheService, redis: unknown): void => { + (service as unknown as { redis: unknown }).redis = redis; + }; + + it('returns immediately when there is no redis client', async () => { + await expect(makeService().onModuleDestroy()).resolves.toBeUndefined(); + }); + + it('completes via a clean quit() without force-disconnecting', async () => { + const service = makeService(); + const redis = { quit: jest.fn().mockResolvedValue('OK'), disconnect: jest.fn() }; + withRedis(service, redis); + + await service.onModuleDestroy(); + + expect(redis.quit).toHaveBeenCalledTimes(1); + expect(redis.disconnect).not.toHaveBeenCalled(); + }); + + it('force-disconnects when quit() hangs past the deadline (shutdown still completes)', async () => { + jest.useFakeTimers(); + try { + const service = makeService(); + const redis = { quit: jest.fn(() => new Promise(() => {})), disconnect: jest.fn() }; // never resolves + withRedis(service, redis); + + const done = service.onModuleDestroy(); + await jest.advanceTimersByTimeAsync(CACHE_QUIT_TIMEOUT_MS); + await done; + + expect(redis.disconnect).toHaveBeenCalledTimes(1); + } finally { + jest.useRealTimers(); + } + }); +}); diff --git a/src/common/cache/cache.service.ts b/src/common/cache/cache.service.ts index 749cd6de..13132ca9 100644 --- a/src/common/cache/cache.service.ts +++ b/src/common/cache/cache.service.ts @@ -27,6 +27,9 @@ const TTL = { SESSIONS_STATS: 15, // 15 sec }; +/** Max time to await a graceful `redis.quit()` on shutdown before force-disconnecting (see onModuleDestroy). */ +export const CACHE_QUIT_TIMEOUT_MS = 2000; + @Injectable() export class CacheService implements OnModuleDestroy { private readonly logger = createLogger('CacheService'); @@ -111,8 +114,25 @@ export class CacheService implements OnModuleDestroy { } async onModuleDestroy(): Promise { - if (this.redis) { - await this.redis.quit(); + if (!this.redis) return; + const redis = this.redis; + + // Bound the teardown: redis.quit() waits for the QUIT reply, which never arrives on a half-open / + // partitioned socket — leaving app.close() blocked until the orchestrator SIGKILLs the process. + // Force-disconnect after a short deadline so shutdown always completes. + let timer: NodeJS.Timeout | undefined; + const forceDisconnect = new Promise(resolve => { + timer = setTimeout(() => { + redis.disconnect(); + resolve(); + }, CACHE_QUIT_TIMEOUT_MS); + timer.unref(); + }); + + try { + await Promise.race([redis.quit().catch(() => undefined), forceDisconnect]); + } finally { + if (timer) clearTimeout(timer); } } diff --git a/src/config/env.validation.spec.ts b/src/config/env.validation.spec.ts index ee0ca324..4ff1818d 100644 --- a/src/config/env.validation.spec.ts +++ b/src/config/env.validation.spec.ts @@ -23,4 +23,16 @@ describe('validateEnv', () => { expect(() => validateEnv({ PORT: '70000' })).toThrow(/PORT/); expect(() => validateEnv({ PORT: '2785' })).not.toThrow(); }); + + it('rejects an ENGINE_TYPE typo instead of silently falling back to whatsapp-web.js', () => { + expect(() => validateEnv({ ENGINE_TYPE: 'bailys' })).toThrow(/ENGINE_TYPE/); + expect(() => validateEnv({ ENGINE_TYPE: 'whatsapp-web.js' })).not.toThrow(); + expect(() => validateEnv({ ENGINE_TYPE: 'baileys' })).not.toThrow(); + }); + + it('rejects a STORAGE_TYPE typo instead of silently falling back to local', () => { + expect(() => validateEnv({ STORAGE_TYPE: 'ss' })).toThrow(/STORAGE_TYPE/); + expect(() => validateEnv({ STORAGE_TYPE: 'local' })).not.toThrow(); + expect(() => validateEnv({ STORAGE_TYPE: 's3' })).not.toThrow(); + }); }); diff --git a/src/config/env.validation.ts b/src/config/env.validation.ts index de17d8b1..6d98f2ef 100644 --- a/src/config/env.validation.ts +++ b/src/config/env.validation.ts @@ -22,6 +22,18 @@ export function validateEnv(config: EnvConfig): EnvConfig { errors.push(`DATABASE_TYPE must be "sqlite" or "postgres" (got "${dbType}")`); } + // Whitelist the registered engine/storage ids so a typo fails fast at boot instead of silently + // falling back to the default (engine.factory swallows an unknown ENGINE_TYPE → legacy wwebjs; + // STORAGE_TYPE → local). Values must match the ids registered in engine.factory / configuration. + const checkEnum = (key: string, allowed: string[]): void => { + const value = str(key); + if (value !== undefined && !allowed.includes(value)) { + errors.push(`${key} must be one of ${allowed.map(v => `"${v}"`).join(', ')} (got "${value}")`); + } + }; + checkEnum('ENGINE_TYPE', ['whatsapp-web.js', 'baileys']); + checkEnum('STORAGE_TYPE', ['local', 's3']); + if (dbType === 'postgres') { for (const key of ['DATABASE_HOST', 'DATABASE_USERNAME', 'DATABASE_PASSWORD']) { if (!str(key)) { diff --git a/src/engine/adapters/whatsapp-web-js.adapter.ts b/src/engine/adapters/whatsapp-web-js.adapter.ts index b2b7dc77..90638d75 100644 --- a/src/engine/adapters/whatsapp-web-js.adapter.ts +++ b/src/engine/adapters/whatsapp-web-js.adapter.ts @@ -34,6 +34,7 @@ import { } from '../interfaces/whatsapp-engine.interface'; import { createLogger } from '../../common/services/logger.service'; import { EngineNotReadyError } from '../../common/errors/engine-not-ready.error'; +import { EngineNotSupportedError } from '../../common/errors/engine-not-supported.error'; import { MessageNotFoundError } from '../../common/errors/message-not-found.error'; import { loadRemoteMediaBuffer } from '../../common/media/load-remote-media'; import { @@ -1154,22 +1155,22 @@ export class WhatsAppWebJsAdapter extends EventEmitter implements IWhatsAppEngin this.ensureReady(); // whatsapp-web.js doesn't have native status posting // This would require using the underlying WhatsApp Web API directly - throw new Error('postTextStatus not yet implemented in whatsapp-web.js adapter'); + throw new EngineNotSupportedError('postTextStatus'); } async postImageStatus(_media: MediaInput, _caption?: string): Promise { this.ensureReady(); - throw new Error('postImageStatus not yet implemented in whatsapp-web.js adapter'); + throw new EngineNotSupportedError('postImageStatus'); } async postVideoStatus(_media: MediaInput, _caption?: string): Promise { this.ensureReady(); - throw new Error('postVideoStatus not yet implemented in whatsapp-web.js adapter'); + throw new EngineNotSupportedError('postVideoStatus'); } async deleteStatus(_statusId: string): Promise { this.ensureReady(); - throw new Error('deleteStatus not yet implemented in whatsapp-web.js adapter'); + throw new EngineNotSupportedError('deleteStatus'); } // ========== Catalog (Phase 3) ========== @@ -1198,12 +1199,12 @@ export class WhatsAppWebJsAdapter extends EventEmitter implements IWhatsAppEngin async sendProduct(_chatId: string, _productId: string, _body?: string): Promise { this.ensureReady(); - throw new Error('sendProduct not yet implemented in whatsapp-web.js adapter'); + throw new EngineNotSupportedError('sendProduct'); } async sendCatalog(_chatId: string, _body?: string): Promise { this.ensureReady(); - throw new Error('sendCatalog not yet implemented in whatsapp-web.js adapter'); + throw new EngineNotSupportedError('sendCatalog'); } /* eslint-enable @typescript-eslint/require-await, @typescript-eslint/no-unused-vars */ diff --git a/src/modules/events/events.gateway.spec.ts b/src/modules/events/events.gateway.spec.ts index 390ebbbd..61726287 100644 --- a/src/modules/events/events.gateway.spec.ts +++ b/src/modules/events/events.gateway.spec.ts @@ -1,3 +1,4 @@ +import { UnauthorizedException } from '@nestjs/common'; import { Socket } from 'socket.io'; import { EventsGateway, isSessionSubscriptionAllowed } from './events.gateway'; import { AuthService } from '../auth/auth.service'; @@ -65,8 +66,8 @@ describe('EventsGateway connection auth + subscribe re-validation', () => { expect(authService.validateApiKey).not.toHaveBeenCalled(); }); - it('rejects a connection with an invalid API key', async () => { - authService.validateApiKey.mockResolvedValue(null); + it('rejects a connection when validateApiKey throws (the real auth-failure contract)', async () => { + authService.validateApiKey.mockRejectedValue(new UnauthorizedException('Invalid API key')); const sock = makeSocket({ apiKey: 'bad' }); await gateway.handleConnection(asSocket(sock)); expect(sock.disconnect).toHaveBeenCalled(); diff --git a/src/modules/events/events.gateway.ts b/src/modules/events/events.gateway.ts index 1e164b1a..e5c2f878 100644 --- a/src/modules/events/events.gateway.ts +++ b/src/modules/events/events.gateway.ts @@ -87,13 +87,9 @@ export class EventsGateway implements OnGatewayInit, OnGatewayConnection, OnGate } try { + // validateApiKey THROWS on any failure (it never resolves to a falsy value), so the rejection + // path is the catch below — a separate `if (!validKey)` branch here was dead code. const validKey = await this.authService.validateApiKey(apiKey); - if (!validKey) { - this.logger.warn(`Client ${client.id} rejected: Invalid API key`); - client.emit('message', this.createError('UNAUTHORIZED', 'Invalid API key')); - client.disconnect(); - return; - } // Store the validated key AND the raw key — the raw key lets handleSubscribe // RE-validate on each subscription so a key revoked mid-connection is caught. diff --git a/src/modules/metrics/metrics.service.spec.ts b/src/modules/metrics/metrics.service.spec.ts index bc015b34..c5ac6e09 100644 --- a/src/modules/metrics/metrics.service.spec.ts +++ b/src/modules/metrics/metrics.service.spec.ts @@ -1,6 +1,6 @@ import { NotFoundException, UnauthorizedException } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; -import { MetricsService } from './metrics.service'; +import { MetricsService, METRICS_RENDER_TTL_MS } from './metrics.service'; import { StatsService, OverviewStats } from '../stats/stats.service'; describe('MetricsService', () => { @@ -59,5 +59,26 @@ describe('MetricsService', () => { expect(out).toContain('# TYPE openwa_messages_total counter'); expect(out.endsWith('\n')).toBe(true); }); + + it('memoizes the rendered output within the TTL (one getOverview per window)', async () => { + jest.useFakeTimers(); + try { + const config = { + get: (k: string) => (k === 'METRICS_TOKEN' ? 's3cret' : undefined), + } as unknown as ConfigService; + const getOverview = jest.fn().mockResolvedValue(overview); + const svc = new MetricsService(config, { getOverview } as unknown as StatsService); + + await svc.render(); + await svc.render(); + expect(getOverview).toHaveBeenCalledTimes(1); // 2nd scrape served from the memo, no DB work + + jest.advanceTimersByTime(METRICS_RENDER_TTL_MS + 1); + await svc.render(); + expect(getOverview).toHaveBeenCalledTimes(2); // window expired → recomputed + } finally { + jest.useRealTimers(); + } + }); }); }); diff --git a/src/modules/metrics/metrics.service.ts b/src/modules/metrics/metrics.service.ts index e99640e7..eb0fe1d9 100644 --- a/src/modules/metrics/metrics.service.ts +++ b/src/modules/metrics/metrics.service.ts @@ -12,8 +12,17 @@ import { StatsService } from '../stats/stats.service'; * is required. This keeps operational internals (session counts, failure totals) from * being exposed publicly by default on a self-hosted box. */ +/** + * How long a rendered scrape is reused before recomputing. getOverview() runs a full session scan plus + * several aggregate queries, so back-to-back scrapes (or several Prometheus replicas) would otherwise + * each pay the full DB cost. Stale-by-a-few-seconds metrics are fine for Prometheus. + */ +export const METRICS_RENDER_TTL_MS = 5000; + @Injectable() export class MetricsService { + private cachedRender: { at: number; text: string } | null = null; + constructor( private readonly config: ConfigService, private readonly statsService: StatsService, @@ -47,8 +56,13 @@ export class MetricsService { return timingSafeEqual(ab, bb); } - /** Render the current metrics in Prometheus text exposition format. */ + /** Render the current metrics in Prometheus text exposition format (memoized for a short TTL). */ async render(): Promise { + const now = Date.now(); + if (this.cachedRender && now - this.cachedRender.at < METRICS_RENDER_TTL_MS) { + return this.cachedRender.text; + } + const overview = await this.statsService.getOverview(); const mem = process.memoryUsage(); const lines: string[] = []; @@ -83,7 +97,9 @@ export class MetricsService { lines.push('# TYPE openwa_messages_failed_total counter'); lines.push(`openwa_messages_failed_total ${overview.messages.failed}`); - return lines.join('\n') + '\n'; + const text = lines.join('\n') + '\n'; + this.cachedRender = { at: now, text }; + return text; } private escapeLabel(value: string): string { diff --git a/src/modules/queue/processors/webhook.processor.spec.ts b/src/modules/queue/processors/webhook.processor.spec.ts index 600e53f1..60e7dc62 100644 --- a/src/modules/queue/processors/webhook.processor.spec.ts +++ b/src/modules/queue/processors/webhook.processor.spec.ts @@ -1,5 +1,6 @@ import { Job } from 'bullmq'; import { Repository } from 'typeorm'; +import { ConfigService } from '@nestjs/config'; import { WebhookProcessor } from './webhook.processor'; import { Webhook } from '../../webhook/entities/webhook.entity'; import { HookManager } from '../../../core/hooks'; @@ -21,6 +22,7 @@ describe('WebhookProcessor', () => { let processor: WebhookProcessor; let repo: { update: jest.Mock }; let hookManager: { execute: jest.Mock }; + let configService: { get: jest.Mock }; let mockFetch: jest.Mock; const origProtect = process.env.WEBHOOK_SSRF_PROTECT; @@ -51,7 +53,13 @@ describe('WebhookProcessor', () => { beforeEach(() => { repo = { update: jest.fn().mockResolvedValue({ affected: 1 }) }; hookManager = { execute: jest.fn().mockResolvedValue({ continue: true, data: {} }) }; - processor = new WebhookProcessor(repo as unknown as Repository, hookManager as unknown as HookManager); + configService = { get: jest.fn((key: string, def?: unknown) => (key === 'webhook.timeout' ? 25000 : def)) }; + processor = new WebhookProcessor( + repo as unknown as Repository, + hookManager as unknown as HookManager, + configService as unknown as ConfigService, + ); + // The merged delivery path uses withSafeFetch (undici), so mock undici's fetch, not global.fetch. mockFetch = undiciFetch as jest.Mock; process.env.WEBHOOK_SSRF_PROTECT = 'false'; // delivery-logic tests; redirect test flips it on }); @@ -62,6 +70,17 @@ describe('WebhookProcessor', () => { else process.env.WEBHOOK_SSRF_PROTECT = origProtect; }); + it('uses the configured WEBHOOK_TIMEOUT for the request abort signal (not a hardcoded 10s)', async () => { + const timeoutSpy = jest.spyOn(AbortSignal, 'timeout'); + mockFetch.mockResolvedValue({ ok: true, status: 200 }); + + await processor.process(makeJob()); + + expect(configService.get).toHaveBeenCalledWith('webhook.timeout', 10000); + expect(timeoutSpy).toHaveBeenCalledWith(25000); + timeoutSpy.mockRestore(); + }); + it('on success updates lastTriggeredAt and fires webhook:delivered', async () => { mockFetch.mockResolvedValue({ ok: true, status: 200 }); diff --git a/src/modules/queue/processors/webhook.processor.ts b/src/modules/queue/processors/webhook.processor.ts index 08d9864b..034db296 100644 --- a/src/modules/queue/processors/webhook.processor.ts +++ b/src/modules/queue/processors/webhook.processor.ts @@ -1,6 +1,7 @@ import { Processor, WorkerHost } from '@nestjs/bullmq'; import { Job } from 'bullmq'; import { InjectRepository } from '@nestjs/typeorm'; +import { ConfigService } from '@nestjs/config'; import { Repository } from 'typeorm'; import { createLogger } from '../../../common/services/logger.service'; import { QUEUE_NAMES } from '../queue-names'; @@ -24,6 +25,7 @@ export class WebhookProcessor extends WorkerHost { @InjectRepository(Webhook, 'data') private readonly webhookRepository: Repository, private readonly hookManager: HookManager, + private readonly configService: ConfigService, ) { super(); } @@ -55,7 +57,8 @@ export class WebhookProcessor extends WorkerHost { method: 'POST', headers: requestHeaders, body: JSON.stringify(payload), - signal: AbortSignal.timeout(10000), + // Honor WEBHOOK_TIMEOUT on the primary (queued) path too — not just the deprecated direct one. + signal: AbortSignal.timeout(this.configService.get('webhook.timeout', 10000)), }, response => ({ status: response.status, statusText: response.statusText, ok: response.ok }), { guard: isSsrfProtectionEnabled() }, diff --git a/src/modules/webhook/webhook.service.spec.ts b/src/modules/webhook/webhook.service.spec.ts index 17e10013..114b77b2 100644 --- a/src/modules/webhook/webhook.service.spec.ts +++ b/src/modules/webhook/webhook.service.spec.ts @@ -63,7 +63,8 @@ describe('WebhookService', () => { get: jest.fn().mockImplementation((key: string, def?: T): T | boolean | number => { if (key === 'queue.enabled') return false; if (key === 'webhook.retryDelay') return 100; - if (key === 'webhook.timeout') return 10000; + // Distinct from the hardcoded 10000 fallback so a regression to a literal timeout is caught. + if (key === 'webhook.timeout') return 25000; return def as T; }), }; @@ -278,12 +279,28 @@ describe('WebhookService', () => { }, }); + const timeoutSpy = jest.spyOn(AbortSignal, 'timeout'); await service.dispatch('sess-1', 'message.received', { from: '628123456789@c.us' }); expect(mockFetch).toHaveBeenCalledWith( 'https://example.com/webhook', expect.objectContaining({ method: 'POST' }), ); + // Direct delivery path honors the configured WEBHOOK_TIMEOUT, not a literal 10s. + expect(timeoutSpy).toHaveBeenCalledWith(25000); + timeoutSpy.mockRestore(); + }); + + it('test() probes the receiver using the configured WEBHOOK_TIMEOUT', async () => { + const webhook = createMockWebhook({ events: ['message.received'] }); + (repository.findOne as jest.Mock).mockResolvedValue(webhook); + const timeoutSpy = jest.spyOn(AbortSignal, 'timeout'); + + await service.test('sess-1', webhook.id); + + expect(mockFetch).toHaveBeenCalled(); + expect(timeoutSpy).toHaveBeenCalledWith(25000); + timeoutSpy.mockRestore(); }); it('should NOT dispatch to webhooks that do not match the event', async () => { diff --git a/src/modules/webhook/webhook.service.ts b/src/modules/webhook/webhook.service.ts index 7cb42b61..bea14766 100644 --- a/src/modules/webhook/webhook.service.ts +++ b/src/modules/webhook/webhook.service.ts @@ -171,7 +171,13 @@ export class WebhookService { try { return await withSafeFetch( webhook.url, - { method: 'POST', headers, body, signal: AbortSignal.timeout(10000) }, + { + method: 'POST', + headers, + body, + // Use the configured WEBHOOK_TIMEOUT (single source of truth across queued/test/direct paths). + signal: AbortSignal.timeout(this.configService.get('webhook.timeout', 10000)), + }, response => ({ success: response.ok, statusCode: response.status }), { guard: isSsrfProtectionEnabled() }, ); From 1a5c6f9fab64693f2412f47ca0e95c429a7e3dca Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:28:53 +0700 Subject: [PATCH 11/14] feat(session): force-kill a stuck session (#352) --- src/engine/adapters/baileys.adapter.ts | 6 +++ .../adapters/whatsapp-web-js.adapter.spec.ts | 40 +++++++++++++++++++ .../adapters/whatsapp-web-js.adapter.ts | 30 ++++++++++++++ .../interfaces/whatsapp-engine.interface.ts | 4 ++ .../audit/entities/audit-log.entity.ts | 1 + src/modules/session/session.controller.ts | 19 +++++++++ src/modules/session/session.service.spec.ts | 31 ++++++++++++++ src/modules/session/session.service.ts | 29 +++++++++++++- 8 files changed, 159 insertions(+), 1 deletion(-) diff --git a/src/engine/adapters/baileys.adapter.ts b/src/engine/adapters/baileys.adapter.ts index 452c7c8a..c18ad3d8 100644 --- a/src/engine/adapters/baileys.adapter.ts +++ b/src/engine/adapters/baileys.adapter.ts @@ -293,6 +293,12 @@ export class BaileysAdapter implements IWhatsAppEngine { return Promise.resolve(); } + // Baileys has no separate Chromium process to SIGKILL (destroy() already ends the socket + // synchronously), so a force-destroy is just a destroy. + forceDestroy(): Promise { + return this.destroy(); + } + // ----- Status ----- getStatus(): EngineStatus { diff --git a/src/engine/adapters/whatsapp-web-js.adapter.spec.ts b/src/engine/adapters/whatsapp-web-js.adapter.spec.ts index e8f1332e..70c18188 100644 --- a/src/engine/adapters/whatsapp-web-js.adapter.spec.ts +++ b/src/engine/adapters/whatsapp-web-js.adapter.spec.ts @@ -234,6 +234,46 @@ describe('WhatsAppWebJsAdapter.forwardMessage (returns the real sent id, not a s }); }); +describe('WhatsAppWebJsAdapter.forceDestroy (recover a wedged session, #351)', () => { + const newAdapter = (): WhatsAppWebJsAdapter => + new WhatsAppWebJsAdapter({ sessionId: 'sess-1', sessionDataPath: './data/sessions', puppeteer: {} }); + const setClient = (adapter: WhatsAppWebJsAdapter, client: unknown): void => { + (adapter as unknown as { client: unknown }).client = client; + }; + const getClient = (adapter: WhatsAppWebJsAdapter): unknown => (adapter as unknown as { client: unknown }).client; + + it('SIGKILLs only its own browser process, then best-effort destroys the client', async () => { + const kill = jest.fn(); + const destroy = jest.fn().mockResolvedValue(undefined); + const adapter = newAdapter(); + setClient(adapter, { pupBrowser: { process: () => ({ kill }) }, destroy }); + + await adapter.forceDestroy(); + + expect(kill).toHaveBeenCalledWith('SIGKILL'); + expect(destroy).toHaveBeenCalledTimes(1); + expect(getClient(adapter)).toBeNull(); + expect(adapter.getStatus()).toBe(EngineStatus.DISCONNECTED); + }); + + it('still completes when the process handle is gone and destroy() rejects (best-effort)', async () => { + const adapter = newAdapter(); + setClient(adapter, { + pupBrowser: { process: () => null }, + destroy: jest.fn().mockRejectedValue(new Error('wedged')), + }); + + await expect(adapter.forceDestroy()).resolves.toBeUndefined(); + expect(getClient(adapter)).toBeNull(); + expect(adapter.getStatus()).toBe(EngineStatus.DISCONNECTED); + }); + + it('is a no-op when there is no client', async () => { + const adapter = newAdapter(); + await expect(adapter.forceDestroy()).resolves.toBeUndefined(); + }); +}); + describe('WhatsAppWebJsAdapter.resolveContactPhone (@lid -> phone, #263)', () => { // Stub a "ready" adapter with a fake client so we exercise the mapping without a real browser. const readyAdapter = (getContactLidAndPhone: jest.Mock): WhatsAppWebJsAdapter => { diff --git a/src/engine/adapters/whatsapp-web-js.adapter.ts b/src/engine/adapters/whatsapp-web-js.adapter.ts index 90638d75..1f943106 100644 --- a/src/engine/adapters/whatsapp-web-js.adapter.ts +++ b/src/engine/adapters/whatsapp-web-js.adapter.ts @@ -449,6 +449,36 @@ export class WhatsAppWebJsAdapter extends EventEmitter implements IWhatsAppEngin } } + /** + * Force-recover a wedged session: SIGKILL THIS client's own Chromium process directly (not a + * process-wide `pkill`, which would also kill other sessions), then best-effort `client.destroy()` + * for the rest of the cleanup. Both steps are wrapped so a missing process handle or a hung destroy + * can't prevent the engine from being torn down and the status reset. + */ + async forceDestroy(): Promise { + const client = this.client; + if (!client) return; + + try { + // pupBrowser is the Puppeteer Browser; .process() is the Chromium ChildProcess (null if already gone). + const proc = ( + client as unknown as { pupBrowser?: { process?: () => { kill?: (sig: string) => void } | null } } + ).pupBrowser?.process?.(); + proc?.kill?.('SIGKILL'); + } catch (err) { + this.logger.warn('forceDestroy: failed to kill the browser process', { error: String(err) }); + } + + try { + await client.destroy(); + } catch (err) { + this.logger.warn('forceDestroy: client.destroy() failed after the kill (continuing)', { error: String(err) }); + } + + this.client = null; + this.setStatus(EngineStatus.DISCONNECTED); + } + getStatus(): EngineStatus { return this.status; } diff --git a/src/engine/interfaces/whatsapp-engine.interface.ts b/src/engine/interfaces/whatsapp-engine.interface.ts index aecf4c50..9c539f57 100644 --- a/src/engine/interfaces/whatsapp-engine.interface.ts +++ b/src/engine/interfaces/whatsapp-engine.interface.ts @@ -336,6 +336,10 @@ export interface IWhatsAppEngine { disconnect(): Promise; // Closes browser but keeps session (can reconnect without QR) logout(): Promise; // Logs out and clears session data (requires QR scan again) destroy(): Promise; + // Force-kill THIS engine's own resources immediately (e.g. SIGKILL a wedged Chromium for a stuck + // session), then best-effort graceful teardown — used to recover a session that destroy() can't. + // Each adapter kills only its own resources (never a process-wide pkill). + forceDestroy(): Promise; // Status getStatus(): EngineStatus; diff --git a/src/modules/audit/entities/audit-log.entity.ts b/src/modules/audit/entities/audit-log.entity.ts index 8eea14aa..6a10492e 100644 --- a/src/modules/audit/entities/audit-log.entity.ts +++ b/src/modules/audit/entities/audit-log.entity.ts @@ -12,6 +12,7 @@ export enum AuditAction { SESSION_CREATED = 'session_created', SESSION_STARTED = 'session_started', SESSION_STOPPED = 'session_stopped', + SESSION_FORCE_KILLED = 'session_force_killed', SESSION_DELETED = 'session_deleted', SESSION_QR_GENERATED = 'session_qr_generated', SESSION_CONNECTED = 'session_connected', diff --git a/src/modules/session/session.controller.ts b/src/modules/session/session.controller.ts index 822b0f0d..f72f9231 100644 --- a/src/modules/session/session.controller.ts +++ b/src/modules/session/session.controller.ts @@ -145,6 +145,25 @@ export class SessionController { return this.transformSession(session); } + @Post(':id/force-kill') + @RequireRole(ApiKeyRole.OPERATOR) + @ApiOperation({ summary: 'Force-kill a stuck session (SIGKILL its wedged engine, then tear it down)' }) + @ApiParam({ name: 'id', description: 'Session ID' }) + @ApiResponse({ + status: 200, + description: 'Session force-killed', + type: SessionResponseDto, + }) + @ApiResponse({ status: 404, description: 'Session not found' }) + async forceKill(@Param('id') id: string): Promise { + const session = await this.sessionService.forceKill(id); + await this.auditService.logInfo(AuditAction.SESSION_FORCE_KILLED, { + sessionId: session.id, + sessionName: session.name, + }); + return this.transformSession(session); + } + @Get(':id/qr') @RequireRole(ApiKeyRole.OPERATOR) @ApiOperation({ summary: 'Get QR code for session authentication' }) diff --git a/src/modules/session/session.service.spec.ts b/src/modules/session/session.service.spec.ts index 3911233f..dce2da7d 100644 --- a/src/modules/session/session.service.spec.ts +++ b/src/modules/session/session.service.spec.ts @@ -185,6 +185,37 @@ describe('SessionService', () => { await expect(service.delete('sess-uuid-1')).rejects.toThrow('db down'); expect(stoppingOf().has('sess-uuid-1')).toBe(false); // mark still cleared on failure }); + + it('forceKill() force-destroys the engine, reconciles the map, and marks the session stopping', async () => { + (repository.findOne as jest.Mock).mockResolvedValue(createMockSession()); + (repository.update as jest.Mock).mockResolvedValue({ affected: 1 }); + const engine = { forceDestroy: jest.fn().mockResolvedValue(undefined) }; + enginesOf().set('sess-uuid-1', engine); + + const result = await service.forceKill('sess-uuid-1'); + + expect(engine.forceDestroy).toHaveBeenCalledTimes(1); + expect(enginesOf().has('sess-uuid-1')).toBe(false); // map reconciled + // Stop-mark stays set (like stop()): it blocks an in-flight reconnect from resurrecting the + // session we just killed; a later start() clears it. + expect(stoppingOf().has('sess-uuid-1')).toBe(true); + expect(result).toBeDefined(); + }); + + it('forceKill() completes even when forceDestroy() rejects (best-effort recovery)', async () => { + (repository.findOne as jest.Mock).mockResolvedValue(createMockSession()); + (repository.update as jest.Mock).mockResolvedValue({ affected: 1 }); + const engine = { forceDestroy: jest.fn().mockRejectedValue(new Error('still wedged')) }; + enginesOf().set('sess-uuid-1', engine); + + await expect(service.forceKill('sess-uuid-1')).resolves.toBeDefined(); + expect(enginesOf().has('sess-uuid-1')).toBe(false); // map reconciled despite the failure + }); + + it('forceKill() throws NotFoundException for an unknown session', async () => { + (repository.findOne as jest.Mock).mockResolvedValue(null); + await expect(service.forceKill('nope')).rejects.toThrow(NotFoundException); + }); }); // ── create ──────────────────────────────────────────────────────── diff --git a/src/modules/session/session.service.ts b/src/modules/session/session.service.ts index 7560a163..5bec9efc 100644 --- a/src/modules/session/session.service.ts +++ b/src/modules/session/session.service.ts @@ -226,7 +226,7 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat sessionId: string, engine: IWhatsAppEngine, teardown: (e: IWhatsAppEngine) => Promise, - label: 'destroy' | 'disconnect', + label: 'destroy' | 'disconnect' | 'force-destroy', ): Promise { let timer: ReturnType | undefined; try { @@ -922,6 +922,33 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat return this.findOne(id); } + /** + * Force-recover a stuck session: SIGKILL its engine's own resources (a wedged Chromium for the + * whatsapp-web.js engine) and tear it down, even when a normal stop()/delete() can't because the + * engine is hung. Mirrors stop()'s lifecycle (stop-mark + cancel-reconnect + bounded, isolated + * teardown + Map reconciliation) but uses the engine's forceDestroy(). + */ + async forceKill(id: string): Promise { + const session = await this.findOne(id); + + // Mark as tearing down BEFORE cleanup so an in-flight reconnect can't resurrect it. + this.stoppingSessions.add(id); + this.cancelReconnect(id); + + const engine = this.engines.get(id); + if (engine) { + await this.teardownEngineSafely(id, engine, e => e.forceDestroy(), 'force-destroy'); + this.engines.delete(id); + } + + this.logger.warn(`Session force-killed: ${session.name}`, { + sessionId: id, + action: 'force_kill', + }); + await this.updateStatus(id, SessionStatus.DISCONNECTED); + return this.findOne(id); + } + async getQRCode(id: string): Promise<{ qrCode: string; status: SessionStatus }> { const session = await this.findOne(id); const engine = this.engines.get(id); From 4d2a804945f9fa1dee8748343df8407343a84915 Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:28:55 +0700 Subject: [PATCH 12/14] merge #343 --- docs/README.md | 1 + docs/examples/session-phone-number-pairing.md | 96 +++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 docs/examples/session-phone-number-pairing.md diff --git a/docs/README.md b/docs/README.md index 973acd36..e14bf568 100644 --- a/docs/README.md +++ b/docs/README.md @@ -61,6 +61,7 @@ | Example | Description | | ------- | ----------- | +| [Session Phone-Number Pairing](./examples/session-phone-number-pairing.md) | Link an existing WhatsApp account by phone number instead of scanning QR | | [Webhook Signature Verification](./examples/webhook-signature-verification.md) | Verify signed OpenWA webhook deliveries in Node.js and Python | | [n8n Appointment Booking Workflow](./examples/n8n-appointment-booking.md) | Build an appointment-booking flow with OpenWA and n8n | diff --git a/docs/examples/session-phone-number-pairing.md b/docs/examples/session-phone-number-pairing.md new file mode 100644 index 00000000..1c79b0bb --- /dev/null +++ b/docs/examples/session-phone-number-pairing.md @@ -0,0 +1,96 @@ +# Session Phone-Number Pairing + +OpenWA supports linking an existing WhatsApp account to a session by phone number as an alternative to scanning a QR code. + +This flow returns an 8-character pairing code that the user enters in WhatsApp on their phone. + +> This does **not** create or register a new WhatsApp account. It only links an existing WhatsApp account as a companion device for an OpenWA session. + +## Flow + +``` +[Create Session] + │ + ▼ +[Start Session] + │ + ▼ +[Request Pairing Code] + │ + ▼ +[Enter Code in WhatsApp] + │ + ▼ +[Session Connected] +``` + +## 1. Create a Session + +```bash +curl -X POST http://localhost:2785/api/sessions \ + -H "X-API-Key: $API_KEY" \ + -H "Content-Type: application/json" \ + -d '{ + "name": "support-bot" + }' +``` + +Save the returned session `id`. + +## 2. Start the Session + +```bash +curl -X POST http://localhost:2785/api/sessions/{sessionId}/start \ + -H "X-API-Key: $API_KEY" +``` + +The session must be started before requesting a pairing code. + +## 3. Request a Pairing Code + +```bash +curl -X POST http://localhost:2785/api/sessions/{sessionId}/pairing-code \ + -H "X-API-Key: $API_KEY" \ + -H "Content-Type: application/json" \ + -d '{ + "phoneNumber": "628123456789" + }' +``` + +`phoneNumber` must be digits only in international format: country code + number, without `+`, spaces, or dashes. + +Example values: + +| Country | Example | +| ------- | ------- | +| Indonesia | `628123456789` | +| Spain | `34612345678` | +| United States | `14155552671` | + +## Response + +```json +{ + "pairingCode": "ABCD1234", + "status": "qr_ready" +} +``` + +## 4. Enter the Code in WhatsApp + +On the phone that owns the WhatsApp account: + +1. Open WhatsApp. +2. Go to **Settings**. +3. Open **Linked Devices**. +4. Choose **Link with phone number**. +5. Enter the pairing code returned by OpenWA. + +After the code is accepted, the OpenWA session should move to a connected/ready state. + +## Troubleshooting + +- If OpenWA returns `Session is not started`, call `POST /api/sessions/{sessionId}/start` first. +- If OpenWA returns `Session is already authenticated`, the account is already linked and no pairing code is needed. +- If the phone number is rejected, send digits only in international format, without `+`, spaces, or punctuation. +- If you want to create a brand-new WhatsApp account programmatically, that is outside OpenWA's scope. OpenWA only links an existing WhatsApp account. From 1018c4b7ace77ea16b79caf1457abc71a7b4a72a Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:28:55 +0700 Subject: [PATCH 13/14] merge #351 --- dashboard/src/i18n/locales/ar.json | 15 +++++++- dashboard/src/i18n/locales/en.json | 15 +++++++- dashboard/src/i18n/locales/es.json | 15 +++++++- dashboard/src/i18n/locales/fr.json | 15 +++++++- dashboard/src/i18n/locales/he.json | 15 +++++++- dashboard/src/i18n/locales/it.json | 15 +++++++- dashboard/src/i18n/locales/te.json | 15 +++++++- dashboard/src/i18n/locales/zh-CN.json | 15 +++++++- dashboard/src/i18n/locales/zh-HK.json | 15 +++++++- dashboard/src/pages/Sessions.tsx | 54 ++++++++++++++++++++++++++- dashboard/src/services/api.ts | 1 + 11 files changed, 171 insertions(+), 19 deletions(-) diff --git a/dashboard/src/i18n/locales/ar.json b/dashboard/src/i18n/locales/ar.json index dc085535..5aaae46b 100644 --- a/dashboard/src/i18n/locales/ar.json +++ b/dashboard/src/i18n/locales/ar.json @@ -226,7 +226,8 @@ "start": "بدء", "stop": "إيقاف", "reconnect": "إعادة الاتصال", - "delete": "حذف" + "delete": "حذف", + "killStuck": "Kill Stuck" }, "toasts": { "readyTitle": "الجلسة جاهزة", @@ -241,6 +242,16 @@ "sessionId": "معرّف الجلسة", "lastActive": "آخر نشاط", "error": "خطأ" + }, + "forceKill": { + "title": "Kill Stuck Session", + "message": "Are you sure you want to force-kill session \"{{name}}\"?", + "warning": "This will forcefully terminate the WhatsApp browser process. The session may need to be re-scanned on restart.", + "confirm": "Kill Session", + "successTitle": "Session Killed", + "success": "Session has been force-killed. You can restart it now.", + "failedTitle": "Force-Kill Failed", + "failed": "Failed to force-kill the session." } }, "webhooks": { @@ -605,4 +616,4 @@ "saveFailed": "فشل الحفظ" } } -} +} \ No newline at end of file diff --git a/dashboard/src/i18n/locales/en.json b/dashboard/src/i18n/locales/en.json index a161c8b8..c7a36f37 100644 --- a/dashboard/src/i18n/locales/en.json +++ b/dashboard/src/i18n/locales/en.json @@ -226,7 +226,8 @@ "start": "Start", "stop": "Stop", "reconnect": "Reconnect", - "delete": "Delete" + "delete": "Delete", + "killStuck": "Kill Stuck" }, "toasts": { "readyTitle": "Session Ready", @@ -241,6 +242,16 @@ "sessionId": "Session ID", "lastActive": "Last Active", "error": "Error" + }, + "forceKill": { + "title": "Kill Stuck Session", + "message": "Are you sure you want to force-kill session \"{{name}}\"?", + "warning": "This will forcefully terminate the WhatsApp browser process. The session may need to be re-scanned on restart.", + "confirm": "Kill Session", + "successTitle": "Session Killed", + "success": "Session has been force-killed. You can restart it now.", + "failedTitle": "Force-Kill Failed", + "failed": "Failed to force-kill the session." } }, "webhooks": { @@ -605,4 +616,4 @@ "saveFailed": "Save Failed" } } -} +} \ No newline at end of file diff --git a/dashboard/src/i18n/locales/es.json b/dashboard/src/i18n/locales/es.json index 5ad73384..b885ca5f 100644 --- a/dashboard/src/i18n/locales/es.json +++ b/dashboard/src/i18n/locales/es.json @@ -226,7 +226,8 @@ "start": "Iniciar", "stop": "Detener", "reconnect": "Reconectar", - "delete": "Eliminar" + "delete": "Eliminar", + "killStuck": "Kill Stuck" }, "toasts": { "readyTitle": "Sesión lista", @@ -241,6 +242,16 @@ "sessionId": "ID de sesión", "lastActive": "Última actividad", "error": "Error" + }, + "forceKill": { + "title": "Kill Stuck Session", + "message": "Are you sure you want to force-kill session \"{{name}}\"?", + "warning": "This will forcefully terminate the WhatsApp browser process. The session may need to be re-scanned on restart.", + "confirm": "Kill Session", + "successTitle": "Session Killed", + "success": "Session has been force-killed. You can restart it now.", + "failedTitle": "Force-Kill Failed", + "failed": "Failed to force-kill the session." } }, "webhooks": { @@ -605,4 +616,4 @@ "saveFailed": "Error al guardar" } } -} +} \ No newline at end of file diff --git a/dashboard/src/i18n/locales/fr.json b/dashboard/src/i18n/locales/fr.json index b2ebe209..e8d82293 100644 --- a/dashboard/src/i18n/locales/fr.json +++ b/dashboard/src/i18n/locales/fr.json @@ -226,7 +226,8 @@ "start": "Démarrer", "stop": "Arrêter", "reconnect": "Reconnecter", - "delete": "Supprimer" + "delete": "Supprimer", + "killStuck": "Kill Stuck" }, "toasts": { "readyTitle": "Session prête", @@ -241,6 +242,16 @@ "sessionId": "ID de session", "lastActive": "Dernière activité", "error": "Erreur" + }, + "forceKill": { + "title": "Kill Stuck Session", + "message": "Are you sure you want to force-kill session \"{{name}}\"?", + "warning": "This will forcefully terminate the WhatsApp browser process. The session may need to be re-scanned on restart.", + "confirm": "Kill Session", + "successTitle": "Session Killed", + "success": "Session has been force-killed. You can restart it now.", + "failedTitle": "Force-Kill Failed", + "failed": "Failed to force-kill the session." } }, "webhooks": { @@ -605,4 +616,4 @@ "saveFailed": "Échec de l'enregistrement" } } -} +} \ No newline at end of file diff --git a/dashboard/src/i18n/locales/he.json b/dashboard/src/i18n/locales/he.json index bde4b580..2ada3b55 100644 --- a/dashboard/src/i18n/locales/he.json +++ b/dashboard/src/i18n/locales/he.json @@ -226,7 +226,8 @@ "start": "הפעלה", "stop": "עצירה", "reconnect": "התחברות מחדש", - "delete": "מחיקה" + "delete": "מחיקה", + "killStuck": "Kill Stuck" }, "toasts": { "readyTitle": "ה-Session מוכן", @@ -241,6 +242,16 @@ "sessionId": "מזהה Session", "lastActive": "פעילות אחרונה", "error": "שגיאה" + }, + "forceKill": { + "title": "Kill Stuck Session", + "message": "Are you sure you want to force-kill session \"{{name}}\"?", + "warning": "This will forcefully terminate the WhatsApp browser process. The session may need to be re-scanned on restart.", + "confirm": "Kill Session", + "successTitle": "Session Killed", + "success": "Session has been force-killed. You can restart it now.", + "failedTitle": "Force-Kill Failed", + "failed": "Failed to force-kill the session." } }, "webhooks": { @@ -605,4 +616,4 @@ "saveFailed": "השמירה נכשלה" } } -} +} \ No newline at end of file diff --git a/dashboard/src/i18n/locales/it.json b/dashboard/src/i18n/locales/it.json index 20b1c3ee..05b6f192 100644 --- a/dashboard/src/i18n/locales/it.json +++ b/dashboard/src/i18n/locales/it.json @@ -226,7 +226,8 @@ "start": "Avvia", "stop": "Ferma", "reconnect": "Riconnetti", - "delete": "Elimina" + "delete": "Elimina", + "killStuck": "Kill Stuck" }, "toasts": { "readyTitle": "Sessione Pronta", @@ -241,6 +242,16 @@ "sessionId": "ID Sessione", "lastActive": "Ultima Attività", "error": "Errore" + }, + "forceKill": { + "title": "Kill Stuck Session", + "message": "Are you sure you want to force-kill session \"{{name}}\"?", + "warning": "This will forcefully terminate the WhatsApp browser process. The session may need to be re-scanned on restart.", + "confirm": "Kill Session", + "successTitle": "Session Killed", + "success": "Session has been force-killed. You can restart it now.", + "failedTitle": "Force-Kill Failed", + "failed": "Failed to force-kill the session." } }, "webhooks": { @@ -605,4 +616,4 @@ "saveFailed": "Salvataggio non riuscito" } } -} +} \ No newline at end of file diff --git a/dashboard/src/i18n/locales/te.json b/dashboard/src/i18n/locales/te.json index a1cd99a2..9c4ebe91 100644 --- a/dashboard/src/i18n/locales/te.json +++ b/dashboard/src/i18n/locales/te.json @@ -226,7 +226,8 @@ "start": "ప్రారంభించు", "stop": "ఆపివేయి", "reconnect": "మళ్లీ కనెక్ట్ చేయి", - "delete": "తొలగించు" + "delete": "తొలగించు", + "killStuck": "Kill Stuck" }, "toasts": { "readyTitle": "సెషన్ సిద్ధంగా ఉంది", @@ -241,6 +242,16 @@ "sessionId": "సెషన్ ID", "lastActive": "చివరిసారి యాక్టివ్", "error": "లోపం" + }, + "forceKill": { + "title": "Kill Stuck Session", + "message": "Are you sure you want to force-kill session \"{{name}}\"?", + "warning": "This will forcefully terminate the WhatsApp browser process. The session may need to be re-scanned on restart.", + "confirm": "Kill Session", + "successTitle": "Session Killed", + "success": "Session has been force-killed. You can restart it now.", + "failedTitle": "Force-Kill Failed", + "failed": "Failed to force-kill the session." } }, "webhooks": { @@ -605,4 +616,4 @@ "saveFailed": "సేవ్ చేయడం విఫలమైంది" } } -} +} \ No newline at end of file diff --git a/dashboard/src/i18n/locales/zh-CN.json b/dashboard/src/i18n/locales/zh-CN.json index 75897bcb..a0de1dde 100644 --- a/dashboard/src/i18n/locales/zh-CN.json +++ b/dashboard/src/i18n/locales/zh-CN.json @@ -226,7 +226,8 @@ "start": "启动", "stop": "停止", "reconnect": "重新连接", - "delete": "删除" + "delete": "删除", + "killStuck": "Kill Stuck" }, "toasts": { "readyTitle": "会话就绪", @@ -241,6 +242,16 @@ "sessionId": "会话 ID", "lastActive": "上次活跃", "error": "错误" + }, + "forceKill": { + "title": "Kill Stuck Session", + "message": "Are you sure you want to force-kill session \"{{name}}\"?", + "warning": "This will forcefully terminate the WhatsApp browser process. The session may need to be re-scanned on restart.", + "confirm": "Kill Session", + "successTitle": "Session Killed", + "success": "Session has been force-killed. You can restart it now.", + "failedTitle": "Force-Kill Failed", + "failed": "Failed to force-kill the session." } }, "webhooks": { @@ -605,4 +616,4 @@ "saveFailed": "保存失败" } } -} +} \ No newline at end of file diff --git a/dashboard/src/i18n/locales/zh-HK.json b/dashboard/src/i18n/locales/zh-HK.json index 76794cea..cd176394 100644 --- a/dashboard/src/i18n/locales/zh-HK.json +++ b/dashboard/src/i18n/locales/zh-HK.json @@ -226,7 +226,8 @@ "start": "啟動", "stop": "停止", "reconnect": "重新連線", - "delete": "刪除" + "delete": "刪除", + "killStuck": "Kill Stuck" }, "toasts": { "readyTitle": "工作階段就緒", @@ -241,6 +242,16 @@ "sessionId": "工作階段 ID", "lastActive": "上次活躍", "error": "錯誤" + }, + "forceKill": { + "title": "Kill Stuck Session", + "message": "Are you sure you want to force-kill session \"{{name}}\"?", + "warning": "This will forcefully terminate the WhatsApp browser process. The session may need to be re-scanned on restart.", + "confirm": "Kill Session", + "successTitle": "Session Killed", + "success": "Session has been force-killed. You can restart it now.", + "failedTitle": "Force-Kill Failed", + "failed": "Failed to force-kill the session." } }, "webhooks": { @@ -605,4 +616,4 @@ "saveFailed": "儲存失敗" } } -} +} \ No newline at end of file diff --git a/dashboard/src/pages/Sessions.tsx b/dashboard/src/pages/Sessions.tsx index 122e74c9..f2df010f 100644 --- a/dashboard/src/pages/Sessions.tsx +++ b/dashboard/src/pages/Sessions.tsx @@ -1,6 +1,6 @@ import { useState, useEffect, useCallback, useRef } from 'react'; import { Trans, useTranslation } from 'react-i18next'; -import { Plus, QrCode, RefreshCw, Trash2, Eye, Loader2, Play, Square, X, Search, Filter } from 'lucide-react'; +import { Plus, QrCode, RefreshCw, Trash2, Eye, Loader2, Play, Square, X, Search, Filter, Skull } from 'lucide-react'; import { sessionApi, type Session } from '../services/api'; import { useDocumentTitle } from '../hooks/useDocumentTitle'; import { useToast } from '../components/Toast'; @@ -25,6 +25,7 @@ export function Sessions() { const [statusFilter, setStatusFilter] = useState('all'); const [selectedSession, setSelectedSession] = useState(null); const [deleteConfirmId, setDeleteConfirmId] = useState(null); + const [killConfirmId, setKillConfirmId] = useState(null); const fetchSessions = useCallback(async (): Promise => { try { @@ -204,6 +205,20 @@ export function Sessions() { } }; + const handleForceKill = async (id: string) => { + try { + await sessionApi.forceKill(id); + setSessions(sessions.map(s => (s.id === id ? { ...s, status: 'disconnected' } : s))); + toast.success(t('sessions.forceKill.successTitle'), t('sessions.forceKill.success')); + } catch (err) { + console.error('Failed to force-kill:', err); + toast.error(t('sessions.forceKill.failedTitle'), t('sessions.forceKill.failed')); + fetchSessions(); + } finally { + setKillConfirmId(null); + } + }; + const formatLastActive = (date?: string) => { if (!date) return t('common.never'); const diff = Date.now() - new Date(date).getTime(); @@ -462,6 +477,37 @@ export function Sessions() { )} + {killConfirmId && ( +
setKillConfirmId(null)}> +
e.stopPropagation()}> +
+

{t('sessions.forceKill.title')}

+ +
+
+

+ s.id === killConfirmId)?.name }} + components={{ strong: }} + /> +

+

{t('sessions.forceKill.warning')}

+
+
+ + +
+
+
+ )} +
{filteredSessions.length === 0 ? (
@@ -542,6 +588,12 @@ export function Sessions() { {t('sessions.actions.delete')} )} + {canWrite && session.status === 'failed' && ( + + )}
)) diff --git a/dashboard/src/services/api.ts b/dashboard/src/services/api.ts index 34f8f8b5..b7bb1e40 100644 --- a/dashboard/src/services/api.ts +++ b/dashboard/src/services/api.ts @@ -317,6 +317,7 @@ export const sessionApi = { delete: (id: string) => request(`/sessions/${id}`, { method: 'DELETE' }), start: (id: string) => request(`/sessions/${id}/start`, { method: 'POST' }), stop: (id: string) => request(`/sessions/${id}/stop`, { method: 'POST' }), + forceKill: (id: string) => request(`/sessions/${id}/force-kill`, { method: 'POST' }), getQR: (id: string) => request<{ qrCode: string; status: string }>(`/sessions/${id}/qr`), getStats: () => request('/sessions/stats/overview'), getGroups: (id: string) => From e2e606ac454aea1df3c6f4a0db187f892c72cd0b Mon Sep 17 00:00:00 2001 From: rmyndharis Date: Fri, 19 Jun 2026 21:33:38 +0700 Subject: [PATCH 14/14] =?UTF-8?q?chore(release):=20v0.4.3=20=E2=80=94=20CH?= =?UTF-8?q?ANGELOG,=20version=20bump=20(package.json/dashboard/swagger),?= =?UTF-8?q?=20README=20+=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 69 ++++++++++++++++++++++++++++++++--- README.md | 2 +- dashboard/package.json | 2 +- docs/07-api-collection.md | 2 +- docs/13-horizontal-scaling.md | 4 +- docs/15-project-roadmap.md | 2 +- docs/README.md | 2 +- package.json | 2 +- src/config/swagger.config.ts | 2 +- 9 files changed, 72 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8e6eb8d..95d1b6e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,15 +7,72 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.4.3] - 2026-06-19 + +A security-hardening and reliability release: outbound-request and storage hardening, plugin/message persistence +fixes, delivery-status and concurrency correctness, and lifecycle robustness — including a **force-kill recovery +for stuck sessions** and its dashboard button. **No breaking changes** for a correctly-configured deployment; the +only behavior change to note is that a misconfigured `ENGINE_TYPE`/`STORAGE_TYPE` now fails fast at boot instead +of silently falling back to the default. + +### Added + +- **Force-kill a stuck session.** `POST /sessions/:id/force-kill` (OPERATOR) recovers a session whose engine is + wedged and won't respond to a normal stop/delete: the whatsapp-web.js engine **SIGKILLs its own Chromium + process directly** (never a process-wide kill that could take down other sessions), then best-effort tears the + client down; the Baileys engine ends its socket. The teardown is time-bounded and isolated, and the session is + left `DISCONNECTED` and restartable. (#352) +- **Dashboard "Kill Stuck" button.** Session cards in a `failed` state get a Kill Stuck action that confirms, + then calls the force-kill endpoint above. (#351) + +### Security + +- **Outbound webhook and media fetches are pinned to the SSRF-validated IP.** The host check and the actual + connection previously resolved DNS independently, leaving a DNS-rebinding window; the connection now reuses + the exact vetted address (preserving the hostname for TLS SNI/`Host`, with A-record failover) across webhook + delivery (direct/queued/test) and server-side media downloads. (#338) +- **IPv6 SSRF blocklist closes embedded-IPv4 gaps** (6to4 `2002::/16`, NAT64 `64:ff9b::/96`, IPv4-compatible + `::/96`); the LibreTranslate plugin client is SSRF-guarded; per-session `proxyUrl` is validated as an + `http(s)`/`socks4`/`socks5` URL. (#344) +- **Secret/auth hardening.** Generated secret files (`data/.env.generated`, `data/.api-key`) are written `0600`; + an opt-in `API_KEY_PEPPER` hashes API keys with HMAC-SHA256; `allowedIps` entries are validated as IPv4/CIDR; + the queue dashboard (Bull Board) auth uses the same trusted-proxy IP model as the API; the production + secret-guard inspects the canonical S3 variables. (#345) +- **Storage import/key hardening.** A `tar.gz` import is bounded against decompression bombs (per-entry byte cap + + entry-count cap); storage-key containment is enforced at the backend-agnostic boundary so the S3 path + inherits it; a plugin's `ctx.storage` is sandbox-contained against `..` traversal. (#346) + ### Fixed - **Webhook subscriptions for session lifecycle events now deliver.** `session.status`, `session.qr`, - `session.authenticated` and `session.disconnected` were accepted on subscribe but were never dispatched, so - consumers (including the n8n trigger node) waited indefinitely. They now fire from the engine lifecycle. The - n8n integration docs are corrected to the real event names (`session.authenticated`, `session.qr` — - previously documented as the non-existent `session.connected`/`session.qr_ready`, which were rejected at - registration). `group.*` events remain accepted on subscribe but are documented as reserved (not emitted - yet). + `session.authenticated`, `session.disconnected` were accepted on subscribe but never dispatched; they now fire + from the engine lifecycle (the n8n docs are corrected to the real event names). (#335) +- **Plugin enable/disable and configuration now persist** across restarts (they previously updated only + in-memory state while the API reported success). Plugins are not auto-enabled on boot for safety; their saved + configuration is preserved. (#339) +- **Bulk-sent messages are recorded, their errors no longer leak internal addresses, and a running batch can be + cancelled across instances.** (#340) +- **Forwarded messages on the whatsapp-web.js engine report a real WhatsApp message id**, so their delivery + status advances (the synthetic `fwd_` could never match an ack). (#341) +- **A late delivery/read receipt is no longer lost** (the ack retries once when it arrives before the send's id + is committed); **concurrent reactions no longer overwrite each other** (serialized per message); a plugin hook + that reports an error no longer has its partial output applied; a failed ack write is logged with context. (#348) +- **Storage export no longer accumulates copies on the data volume** — it writes under `data/exports/` with a + TTL sweep and an async read (instead of a synchronous read that blocked the event loop). (#346) +- **`WEBHOOK_TIMEOUT` is honored on the queued and test delivery paths** (not just the deprecated direct one); + graceful shutdown is bounded (a half-open Redis socket can't block `app.close()`); unsupported status/catalog + operations return a consistent `501`; a misconfigured `ENGINE_TYPE`/`STORAGE_TYPE` fails fast at boot. (#350) + +### Changed + +- **The `/api/metrics` scrape is memoized for a few seconds** so back-to-back scrapes don't each run a full + session scan plus aggregates; removed a dead branch in the WebSocket connect handler. (#350) + +### Documentation + +- Added a **phone-number pairing** example. (#343) +- Documented the webhook `idempotencyKey`/`deliveryId` fields (body + `X-OpenWA-*` headers) and the dedup rule; + corrected the `.env.example` rate-limit variable names (`RATE_LIMIT_MEDIUM_TTL`/`_LIMIT`, in milliseconds). (#350) ## [0.4.2] - 2026-06-19 diff --git a/README.md b/README.md index 7da20bca..d3c3cff5 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@

CI - Version + Version License Node NestJS diff --git a/dashboard/package.json b/dashboard/package.json index 6f7f066f..13e8fdcf 100644 --- a/dashboard/package.json +++ b/dashboard/package.json @@ -1,7 +1,7 @@ { "name": "dashboard", "private": true, - "version": "0.4.1", + "version": "0.4.3", "type": "module", "scripts": { "dev": "vite", diff --git a/docs/07-api-collection.md b/docs/07-api-collection.md index 82d43e18..fc3fae43 100644 --- a/docs/07-api-collection.md +++ b/docs/07-api-collection.md @@ -71,7 +71,7 @@ curl -H "X-API-Key: $API_KEY" \ ```json { "status": "ok", - "version": "0.4.0", + "version": "0.4.3", "uptime": 86400, "timestamp": "2026-02-02T10:30:00Z", "checks": { diff --git a/docs/13-horizontal-scaling.md b/docs/13-horizontal-scaling.md index 480e7b95..1ed1b368 100644 --- a/docs/13-horizontal-scaling.md +++ b/docs/13-horizontal-scaling.md @@ -105,7 +105,7 @@ version: '3.8' services: openwa: - image: ghcr.io/rmyndharis/openwa:0.4.0 + image: ghcr.io/rmyndharis/openwa:0.4.3 deploy: replicas: 1 # MUST stay 1 until session-claim is implemented — multiple replicas on one session volume corrupt WhatsApp auth (H1/H11) update_config: @@ -263,7 +263,7 @@ spec: spec: containers: - name: openwa - image: ghcr.io/rmyndharis/openwa:0.4.0 + image: ghcr.io/rmyndharis/openwa:0.4.3 ports: - containerPort: 2785 name: http diff --git a/docs/15-project-roadmap.md b/docs/15-project-roadmap.md index 081bf7e8..587de275 100644 --- a/docs/15-project-roadmap.md +++ b/docs/15-project-roadmap.md @@ -491,7 +491,7 @@ v0.1.0 Release Package: ## 15.6 Future Roadmap (v0.3.0+) > **Note:** Version 0.1.0 is the initial stable release including all features from Phases 1-3. -> Versions 0.1.7–0.2.10, 0.3.0, and 0.4.0 have since shipped (see the CHANGELOG); v1.0.0 +> Versions 0.1.7 through 0.4.3 have since shipped (see the CHANGELOG); v1.0.0 > onward is forward-looking. ```mermaid diff --git a/docs/README.md b/docs/README.md index e14bf568..69285639 100644 --- a/docs/README.md +++ b/docs/README.md @@ -16,7 +16,7 @@

- Version + Version License Node NestJS diff --git a/package.json b/package.json index 641ca9db..c5eb2f0f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "openwa", - "version": "0.4.2", + "version": "0.4.3", "description": "Open Source WhatsApp API Gateway - Free, Self-Hosted HTTP API for WhatsApp", "author": "Yudhi Armyndharis & OpenWA Contributors", "private": true, diff --git a/src/config/swagger.config.ts b/src/config/swagger.config.ts index f29d7aad..7527df10 100644 --- a/src/config/swagger.config.ts +++ b/src/config/swagger.config.ts @@ -14,7 +14,7 @@ export function createSwaggerConfig(): Omit { new DocumentBuilder() .setTitle('OpenWA API') .setDescription('Open Source WhatsApp API Gateway - Free, Self-Hosted HTTP API') - .setVersion('0.4.2') + .setVersion('0.4.3') .addApiKey({ type: 'apiKey', name: 'X-API-Key', in: 'header' }, API_KEY_SECURITY_SCHEME) // Apply the scheme globally so Swagger UI sends the key with every request // (mirrors the global ApiKeyGuard). Without this, "Authorize" is cosmetic.