diff --git a/CHANGELOG.md b/CHANGELOG.md index fdd7116a..43c08272 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,32 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **CLI migration commands for the main (auth/audit) connection.** The app runs the main connection as a separate + always-SQLite connection, but the migration CLI only managed the data connection. New `migration:run:main`, + `migration:generate:main`, `migration:show:main`, and `migration:revert:main` scripts (plus `:prod` variants) manage + it — needed when `MAIN_DATABASE_SYNCHRONIZE=false` disables boot auto-migration. Purely additive. (#364) + +### Changed + +- **`PUT /settings` now returns `501 Not Implemented` instead of a misleading `200`.** Settings are derived from + environment variables and consumed at boot (and `ConfigService` is immutable at runtime), so the previous handler + mutated an in-memory copy and reported success while persisting and applying nothing. The endpoint is now honest + about being read-only; `GET /settings` and the ADMIN guard are unchanged, and no dashboard flow uses the write. (#364) + ### Fixed +- **Baileys reconnect no longer leaks the previous socket.** An internal (transient-drop) reconnect overwrote the live + socket without tearing the old one down, leaking its WebSocket and event listeners on every reconnect. The previous + socket is now detached and ended before its replacement is created. (#364) +- **Engine sessions keep operator config when the engine plugin fails to enable.** The engine config blob is now also + supplied at plugin construction, so `sessionDataPath`/`executablePath`/`authDir` still apply if a plugin fails to + enable before its `onLoad` runs (they previously dropped silently to defaults). The healthy path is unchanged. (#364) +- **Template names are unique per session.** A composite unique index makes resolve-by-name deterministic and rejects + duplicate names with `409 Conflict`; a migration losslessly de-duplicates any pre-existing collisions (keeps the + earliest, renames the rest) before adding the index. The `{{var}}`/`{var}` template-syntax split is unchanged and + still tracked in #69. (#364) - **Container no longer crashes on browser-cleanup paths when `ps` is missing.** The production image is based on `node:22-slim`, which omits the `ps` binary; cleanup code that shells out to `ps` (e.g. process-tree kills) fails with `spawn ps ENOENT`, and that unhandled child-process error can take down the whole Node runtime. The image now diff --git a/package.json b/package.json index c5eb2f0f..30a48308 100644 --- a/package.json +++ b/package.json @@ -36,7 +36,14 @@ "migration:revert": "npm run typeorm -- migration:revert -d src/database/data-source.ts", "migration:revert:prod": "npm run typeorm:prod -- migration:revert -d dist/database/data-source.js", "migration:show": "npm run typeorm -- migration:show -d src/database/data-source.ts", - "migration:show:prod": "npm run typeorm:prod -- migration:show -d dist/database/data-source.js" + "migration:show:prod": "npm run typeorm:prod -- migration:show -d dist/database/data-source.js", + "migration:generate:main": "npm run typeorm -- migration:generate src/database/migrations-main/$npm_config_name -d src/database/data-source-main.ts", + "migration:run:main": "npm run typeorm -- migration:run -d src/database/data-source-main.ts", + "migration:run:main:prod": "npm run typeorm:prod -- migration:run -d dist/database/data-source-main.js", + "migration:revert:main": "npm run typeorm -- migration:revert -d src/database/data-source-main.ts", + "migration:revert:main:prod": "npm run typeorm:prod -- migration:revert -d dist/database/data-source-main.js", + "migration:show:main": "npm run typeorm -- migration:show -d src/database/data-source-main.ts", + "migration:show:main:prod": "npm run typeorm:prod -- migration:show -d dist/database/data-source-main.js" }, "dependencies": { "@aws-sdk/client-s3": "^3.1068.0", diff --git a/src/common/transformers/date.transformer.spec.ts b/src/common/transformers/date.transformer.spec.ts new file mode 100644 index 00000000..92f606a8 --- /dev/null +++ b/src/common/transformers/date.transformer.spec.ts @@ -0,0 +1,33 @@ +import { DateTransformer } from './date.transformer'; + +// DateTransformer.to() resolves the DATA connection dialect from the global DATABASE_TYPE. +// Characterization tests lock the round-trip behavior (previously untested). +describe('DateTransformer (cross-DB date round-trip)', () => { + const original = process.env.DATABASE_TYPE; + afterEach(() => { + if (original === undefined) delete process.env.DATABASE_TYPE; + else process.env.DATABASE_TYPE = original; + }); + + it('from(): parses ISO strings to Date, passes a Date through, maps null to null', () => { + const parsed = DateTransformer.from('2026-06-20T10:00:00.000Z') as Date; + expect(parsed).toBeInstanceOf(Date); + expect(parsed.toISOString()).toBe('2026-06-20T10:00:00.000Z'); + + const now = new Date(); + expect(DateTransformer.from(now)).toBe(now); + expect(DateTransformer.from(null)).toBeNull(); + }); + + it('to(): stores an ISO string on SQLite and a native Date on Postgres; null stays null', () => { + const d = new Date('2026-06-20T10:00:00.000Z'); + + process.env.DATABASE_TYPE = 'sqlite'; + expect(DateTransformer.to(d)).toBe('2026-06-20T10:00:00.000Z'); + + process.env.DATABASE_TYPE = 'postgres'; + expect(DateTransformer.to(d)).toBe(d); + + expect(DateTransformer.to(null)).toBeNull(); + }); +}); diff --git a/src/common/transformers/date.transformer.ts b/src/common/transformers/date.transformer.ts index 1be1d0da..89d6595b 100644 --- a/src/common/transformers/date.transformer.ts +++ b/src/common/transformers/date.transformer.ts @@ -4,6 +4,10 @@ import { ValueTransformer } from 'typeorm'; * Cross-database date transformer. * - SQLite stores as ISO string TEXT, transformer converts to/from Date * - PostgreSQL stores as native timestamp, driver returns Date directly + * + * DATA CONNECTION ONLY. `to()` resolves the dialect from the global `DATABASE_TYPE`. Pair it + * only with data-connection entities; the always-SQLite MAIN connection (auth, audit) must not use + * it (see column-types.ts for the full rationale). */ export const DateTransformer: ValueTransformer = { from: (value: string | Date | null): Date | null => { diff --git a/src/common/utils/column-types.spec.ts b/src/common/utils/column-types.spec.ts new file mode 100644 index 00000000..366a759c --- /dev/null +++ b/src/common/utils/column-types.spec.ts @@ -0,0 +1,28 @@ +import { jsonColumnType, dateColumnType } from './column-types'; + +// These helpers resolve the DATA connection dialect from the global DATABASE_TYPE env var. +// Characterization tests lock the resolved type strings so any future change to the resolution is +// caught (the helpers were previously untested). +describe('cross-DB column type helpers (data connection dialect)', () => { + const original = process.env.DATABASE_TYPE; + afterEach(() => { + if (original === undefined) delete process.env.DATABASE_TYPE; + else process.env.DATABASE_TYPE = original; + }); + + it('resolves Postgres types when DATABASE_TYPE=postgres', () => { + process.env.DATABASE_TYPE = 'postgres'; + expect(jsonColumnType()).toBe('jsonb'); + expect(dateColumnType()).toBe('timestamp'); + }); + + it('resolves SQLite types when DATABASE_TYPE is sqlite or unset', () => { + process.env.DATABASE_TYPE = 'sqlite'; + expect(jsonColumnType()).toBe('simple-json'); + expect(dateColumnType()).toBe('text'); + + delete process.env.DATABASE_TYPE; + expect(jsonColumnType()).toBe('simple-json'); + expect(dateColumnType()).toBe('text'); + }); +}); diff --git a/src/common/utils/column-types.ts b/src/common/utils/column-types.ts index f9cab145..42b2d62b 100644 --- a/src/common/utils/column-types.ts +++ b/src/common/utils/column-types.ts @@ -6,6 +6,13 @@ * * PostgreSQL has native `jsonb` and `timestamp` types with better * indexing and query performance. + * + * DATA CONNECTION ONLY. These resolve the dialect of the *data* connection from the global + * `DATABASE_TYPE` env var. Use them only on entities bound to the data connection. Entities on the + * MAIN connection (auth, audit) are ALWAYS SQLite — it is hardcoded `type: 'sqlite'` in + * app.module.ts regardless of DATABASE_TYPE — so they must hardcode `simple-json` / `datetime` + * (see audit-log.entity.ts) and must NOT call these helpers, or a Postgres deployment would emit a + * `jsonb`/`timestamp` column on the always-SQLite main DB. */ const isPostgres = (): boolean => process.env.DATABASE_TYPE === 'postgres'; diff --git a/src/config/configuration.ts b/src/config/configuration.ts index a6c852d1..5505b131 100644 --- a/src/config/configuration.ts +++ b/src/config/configuration.ts @@ -24,7 +24,10 @@ export default () => ({ database: './data/main.sqlite', // Schema management for the auth/audit DB. Default ON (zero-config first boot). // Set MAIN_DATABASE_SYNCHRONIZE=false to manage schema via the main-owned migrations - // instead (migrationsRun then creates api_keys/audit_logs). + // instead (migrationsRun then creates api_keys/audit_logs). When disabled, run the + // main-connection migrations explicitly with `npm run migration:run:main` (or + // `migration:run:main:prod` for the compiled image) — the plain `migration:run` only + // manages the data connection. synchronize: process.env.MAIN_DATABASE_SYNCHRONIZE !== 'false', logging: process.env.DATABASE_LOGGING === 'true', }, diff --git a/src/database/data-source-main.spec.ts b/src/database/data-source-main.spec.ts new file mode 100644 index 00000000..9e4ca0bf --- /dev/null +++ b/src/database/data-source-main.spec.ts @@ -0,0 +1,21 @@ +import mainDataSource from './data-source-main'; + +// The app runs the MAIN connection (auth + audit) as a separate always-SQLite connection. The +// default data-source.ts CLI only manages the DATA connection's migrations, so this standalone +// DataSource exists so the CLI can run/generate the main-owned migrations too. +describe('main CLI DataSource', () => { + it('targets the always-SQLite main connection', () => { + expect(mainDataSource.options.type).toBe('sqlite'); + }); + + it('uses the main-owned migrations dir, not the data migrations dir', () => { + const migrations = (mainDataSource.options.migrations as string[]).join(' '); + expect(migrations).toContain('migrations-main'); + }); + + it('covers the auth and audit entities (the main connection owns them)', () => { + const entities = (mainDataSource.options.entities as string[]).join(' '); + expect(entities).toContain('auth'); + expect(entities).toContain('audit'); + }); +}); diff --git a/src/database/data-source-main.ts b/src/database/data-source-main.ts new file mode 100644 index 00000000..45df8a2d --- /dev/null +++ b/src/database/data-source-main.ts @@ -0,0 +1,32 @@ +import { DataSource } from 'typeorm'; +import { config } from 'dotenv'; + +// Load environment variables (mirrors data-source.ts). +config(); + +/** + * Standalone TypeORM CLI DataSource for the MAIN connection (auth + audit). + * + * The app runs the main connection as a separate, ALWAYS-SQLite connection (app.module.ts), distinct + * from the pluggable data connection. The default data-source.ts CLI only manages the data + * connection's migrations, so without this the CLI could not run/generate the main-owned migrations + * (migrations-main) — which matters the moment boot auto-migration is turned off + * (MAIN_DATABASE_SYNCHRONIZE=false), where the schema must be managed via the CLI instead. + * + * Mirrors the runtime main connection exactly: SQLite at ./data/main.sqlite, auth/audit entities, + * migrations-main. synchronize is always false here — the CLI manages schema via migrations. + * + * Usage: `npm run migration:run:main` (dev) / `migration:run:main:prod` (compiled). + */ +const mainDataSource = new DataSource({ + type: 'sqlite', + // Hardcoded to match the runtime main path (configuration.ts), so the CLI and the app never target + // different main databases. + database: './data/main.sqlite', + entities: [__dirname + '/../modules/auth/**/*.entity{.ts,.js}', __dirname + '/../modules/audit/**/*.entity{.ts,.js}'], + migrations: [__dirname + '/migrations-main/*{.ts,.js}'], + synchronize: false, + logging: process.env.DATABASE_LOGGING === 'true', +}); + +export default mainDataSource; diff --git a/src/database/migrations/1781100000000-AddTemplateNameUnique.ts b/src/database/migrations/1781100000000-AddTemplateNameUnique.ts new file mode 100644 index 00000000..ec589f4a --- /dev/null +++ b/src/database/migrations/1781100000000-AddTemplateNameUnique.ts @@ -0,0 +1,42 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * Enforces one template name per session (issue #69): resolve-by-name was nondeterministic because + * nothing stopped two templates in a session from sharing a name. + * + * Before adding the composite UNIQUE index, any pre-existing (sessionId, name) collisions are + * deduplicated LOSSLESSLY — the earliest row keeps the name, the rest are renamed to + * `-dup-` (id is a UUID, so the new name cannot collide; the name is left-trimmed so the + * result fits the varchar(100) column on PostgreSQL). No row is ever deleted. + * + * Hand-authored because `synchronize` is disabled for the `data` connection on PostgreSQL (and may + * be disabled on SQLite via DATABASE_SYNCHRONIZE=false). Idempotent: a re-run finds no duplicates + * and skips the existing index. + */ +export class AddTemplateNameUnique1781100000000 implements MigrationInterface { + name = 'AddTemplateNameUnique1781100000000'; + + public async up(queryRunner: QueryRunner): Promise { + if (!(await queryRunner.hasTable('templates'))) return; + + // Keep the earliest row per (sessionId, name) — createdAt ASC, id ASC as a stable tiebreak — + // and rename every other member of the group. substr(name,1,59)+'-dup-'+id(36) is <= 100 chars, + // so it never overflows the varchar(100) "name" column on PostgreSQL. + await queryRunner.query( + `UPDATE "templates" SET "name" = substr("name", 1, 59) || '-dup-' || "id" ` + + `WHERE "id" <> (` + + `SELECT t2."id" FROM "templates" t2 ` + + `WHERE t2."sessionId" = "templates"."sessionId" AND t2."name" = "templates"."name" ` + + `ORDER BY t2."createdAt" ASC, t2."id" ASC LIMIT 1)`, + ); + + await queryRunner.query( + `CREATE UNIQUE INDEX IF NOT EXISTS "IDX_templates_session_name" ON "templates" ("sessionId", "name")`, + ); + } + + public async down(queryRunner: QueryRunner): Promise { + // Only the index is reversible; the lossless renames are intentionally left in place. + await queryRunner.query(`DROP INDEX IF EXISTS "IDX_templates_session_name"`); + } +} diff --git a/src/database/migrations/__tests__/1781100000000-AddTemplateNameUnique.spec.ts b/src/database/migrations/__tests__/1781100000000-AddTemplateNameUnique.spec.ts new file mode 100644 index 00000000..0e72ec20 --- /dev/null +++ b/src/database/migrations/__tests__/1781100000000-AddTemplateNameUnique.spec.ts @@ -0,0 +1,78 @@ +import { DataSource } from 'typeorm'; +import { AddTemplateNameUnique1781100000000 } from '../1781100000000-AddTemplateNameUnique'; + +describe('AddTemplateNameUnique migration', () => { + let ds: DataSource; + + beforeEach(async () => { + ds = new DataSource({ type: 'sqlite', database: ':memory:' }); + await ds.initialize(); + // Minimal templates table mirroring the AddTemplates sqlite schema. + await ds.query( + `CREATE TABLE "templates" ("id" varchar PRIMARY KEY NOT NULL, "sessionId" varchar NOT NULL, ` + + `"name" varchar(100) NOT NULL, "body" text NOT NULL, "header" text, "footer" text, ` + + `"createdAt" datetime NOT NULL, "updatedAt" datetime NOT NULL)`, + ); + }); + + afterEach(async () => { + await ds.destroy(); + }); + + const insert = (id: string, sessionId: string, name: string, createdAt: string): Promise => + ds.query( + `INSERT INTO "templates" ("id","sessionId","name","body","header","footer","createdAt","updatedAt") ` + + `VALUES (?, ?, ?, 'body', NULL, NULL, ?, ?)`, + [id, sessionId, name, createdAt, createdAt], + ); + + it('creates the composite unique index and is idempotent', async () => { + const runner = ds.createQueryRunner(); + const migration = new AddTemplateNameUnique1781100000000(); + + await migration.up(runner); + const index = (await runner.query( + `SELECT name FROM sqlite_master WHERE type='index' AND name='IDX_templates_session_name'`, + )) as unknown[]; + expect(index).toHaveLength(1); + + // Re-run must not throw (idempotent). + await expect(migration.up(runner)).resolves.toBeUndefined(); + await runner.release(); + }); + + it('deduplicates pre-existing (sessionId, name) collisions losslessly, keeping the earliest', async () => { + await insert('id-early', 'sess-1', 'welcome', '2026-01-01T00:00:00.000Z'); + await insert('id-late', 'sess-1', 'welcome', '2026-02-01T00:00:00.000Z'); + await insert('id-other', 'sess-1', 'promo', '2026-01-01T00:00:00.000Z'); + + const runner = ds.createQueryRunner(); + await new AddTemplateNameUnique1781100000000().up(runner); + + // No rows lost — all three bodies survive. + const all = (await runner.query(`SELECT id, name FROM "templates" ORDER BY id`)) as { id: string; name: string }[]; + expect(all).toHaveLength(3); + + const byId = Object.fromEntries(all.map(r => [r.id, r.name])); + expect(byId['id-early']).toBe('welcome'); // earliest keeps the clean name + expect(byId['id-late']).toBe('welcome-dup-id-late'); // later duplicate renamed losslessly + expect(byId['id-other']).toBe('promo'); // unrelated row untouched + + // The unique index now rejects a fresh duplicate. + await expect( + runner.query( + `INSERT INTO "templates" ("id","sessionId","name","body","header","footer","createdAt","updatedAt") ` + + `VALUES ('id-new','sess-1','welcome','body',NULL,NULL,'2026-03-01T00:00:00.000Z','2026-03-01T00:00:00.000Z')`, + ), + ).rejects.toThrow(); + + await runner.release(); + }); + + it('is a no-op when the templates table does not exist', async () => { + await ds.query(`DROP TABLE "templates"`); + const runner = ds.createQueryRunner(); + await expect(new AddTemplateNameUnique1781100000000().up(runner)).resolves.toBeUndefined(); + await runner.release(); + }); +}); diff --git a/src/engine/adapters/baileys.adapter.spec.ts b/src/engine/adapters/baileys.adapter.spec.ts index 81c19471..207cec85 100644 --- a/src/engine/adapters/baileys.adapter.spec.ts +++ b/src/engine/adapters/baileys.adapter.spec.ts @@ -10,6 +10,10 @@ class FakeSock extends EventEmitter { on: (event: string, handler: (arg: unknown) => void) => { this.emitter.on(event, handler); }, + // Mirrors the real Baileys typed event emitter, which exposes removeAllListeners(event). + removeAllListeners: (event: string) => { + this.emitter.removeAllListeners(event); + }, }; public emitter = new EventEmitter(); public user: { id: string; name?: string } | undefined; @@ -355,6 +359,68 @@ describe('BaileysAdapter lifecycle hardening — I4 reconnect backoff', () => { }); }); +describe('BaileysAdapter reconnect socket teardown (no leak)', () => { + // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion + const baileys = () => jest.requireMock('@whiskeysockets/baileys') as { default: jest.Mock }; + + const fireRecoverableClose = (): void => { + fakeSock.fire('connection.update', { + connection: 'close', + lastDisconnect: { error: { output: { statusCode: 515 } } }, + }); + }; + + const initWithRealTimers = async (): Promise => { + fakeSock.user = undefined; + fakeSock.resetEmitter(); + jest.clearAllMocks(); + const adapter = newAdapter(); + await adapter.initialize(noopCallbacks({})); + return adapter; + }; + + afterEach(() => { + jest.useRealTimers(); + }); + + it('ends the previous socket when an internal reconnect replaces it', async () => { + const adapter = await initWithRealTimers(); + jest.useFakeTimers(); + fakeSock.end.mockClear(); // only count end() calls originating from the reconnect path + + fireRecoverableClose(); + await jest.runAllTimersAsync(); // reconnect runs connectInner → must tear down the old socket first + + // Before the fix, end() is only called by disconnect/logout/destroy — never on reconnect, + // so the prior socket + its listeners leak on every transient drop. + expect(fakeSock.end).toHaveBeenCalledTimes(1); + expect(adapter.getStatus()).not.toBe(EngineStatus.FAILED); + }); + + it('tearing down the previous socket does not trigger a spurious second reconnect', async () => { + const adapter = await initWithRealTimers(); + jest.useFakeTimers(); + baileys().default.mockClear(); + + // Real Baileys end() synchronously emits a connection.update {connection:'close'} before it + // detaches its own listener. If our handler is still attached when end() runs (wrong teardown + // order), that synthetic close re-enters handleConnectionUpdate and schedules a 2nd reconnect. + fakeSock.end.mockImplementationOnce(() => { + fakeSock.fire('connection.update', { + connection: 'close', + lastDisconnect: { error: { output: { statusCode: 515 } } }, + }); + }); + + fireRecoverableClose(); + await jest.runAllTimersAsync(); + + // Exactly one legitimate reconnect — the synthetic close from end() must land on zero listeners. + expect(baileys().default).toHaveBeenCalledTimes(1); + expect(adapter.getStatus()).not.toBe(EngineStatus.FAILED); + }); +}); + describe('BaileysAdapter capability gating', () => { it('throws EngineNotSupportedError for still-gated methods (e.g. getChatHistory)', async () => { const adapter = newAdapter(); diff --git a/src/engine/adapters/baileys.adapter.ts b/src/engine/adapters/baileys.adapter.ts index c18ad3d8..c9f39193 100644 --- a/src/engine/adapters/baileys.adapter.ts +++ b/src/engine/adapters/baileys.adapter.ts @@ -134,6 +134,30 @@ export class BaileysAdapter implements IWhatsAppEngine { return; } + // An internal reconnect (transient drop) overwrites this.sock WITHOUT going through + // disconnect/logout/destroy, so the previous socket's WebSocket and the 9 ev listeners we + // register below would leak on every reconnect. Tear the prior socket down first. Detach OUR + // connection.update listener BEFORE end(): Baileys' own end() synchronously emits a synthetic + // connection.update {connection:'close'}, which — if still wired — would re-enter + // handleConnectionUpdate and schedule a spurious second reconnect. + const previous = this.sock; + if (previous) { + try { + previous.ev.removeAllListeners('connection.update'); + previous.ev.removeAllListeners('creds.update'); + previous.ev.removeAllListeners('messages.upsert'); + previous.ev.removeAllListeners('messages.update'); + previous.ev.removeAllListeners('contacts.upsert'); + previous.ev.removeAllListeners('contacts.update'); + previous.ev.removeAllListeners('chats.upsert'); + previous.ev.removeAllListeners('chats.update'); + previous.ev.removeAllListeners('messaging-history.set'); + previous.end(undefined); + } catch { + // end() may already have run from Baileys' own close handler — a safe no-op. + } + } + const sock = b.default({ auth: state, version, diff --git a/src/engine/engine.factory.ts b/src/engine/engine.factory.ts index 9ea9397d..59199d5f 100644 --- a/src/engine/engine.factory.ts +++ b/src/engine/engine.factory.ts @@ -33,6 +33,12 @@ export class EngineFactory implements OnModuleInit { } private async registerBuiltInEngines(): Promise { + // The engine config sub-tree (engine.* from configuration.ts) as an opaque blob. Supplied BOTH + // to registerBuiltInPlugin (becomes context.config when onLoad runs) AND to each plugin's + // constructor (A fallback so createEngine still has operator config if enablePlugin fails + // before onLoad — otherwise sessionDataPath/executablePath/authDir would silently drop to defaults). + const engineConfig = this.configService.get>('engine') ?? {}; + // Register WhatsApp-web.js as built-in plugin const wwjsManifest: PluginManifest = { id: 'whatsapp-web.js', @@ -44,10 +50,8 @@ export class EngineFactory implements OnModuleInit { provides: ['whatsapp-engine'], }; - const wwjsPlugin = new WhatsAppWebJsPlugin(); - // Supply the engine config sub-tree (engine.* from configuration.ts) as an opaque blob; - // the plugin reads its own namespace (puppeteer.*, sessionDataPath) from context.config. - this.pluginLoader.registerBuiltInPlugin(wwjsManifest, wwjsPlugin, this.configService.get('engine') ?? {}); + const wwjsPlugin = new WhatsAppWebJsPlugin(engineConfig); + this.pluginLoader.registerBuiltInPlugin(wwjsManifest, wwjsPlugin, engineConfig); // Register Baileys as a second built-in engine plugin. Same opaque engine blob; the plugin // reads only its own namespace (baileys.authDir) from context.config. @@ -62,8 +66,8 @@ export class EngineFactory implements OnModuleInit { }; this.pluginLoader.registerBuiltInPlugin( baileysManifest, - new BaileysPlugin(this.baileysMessageStore), - this.configService.get('engine') ?? {}, + new BaileysPlugin(this.baileysMessageStore, engineConfig), + engineConfig, ); // Auto-enable the configured engine diff --git a/src/modules/session/session.service.spec.ts b/src/modules/session/session.service.spec.ts index dce2da7d..f1119849 100644 --- a/src/modules/session/session.service.spec.ts +++ b/src/modules/session/session.service.spec.ts @@ -575,6 +575,21 @@ describe('SessionService', () => { expect(sent[0][0]).toBe('sess-uuid-1'); }); + it('does NOT persist an outgoing (message_create) self-message to the messages table', async () => { + // Contract lock: message_create also fires for API sends (already persisted by the REST send + // path), so a naive save here would double-persist. Phone-composed sends are therefore + // webhooked/emitted but not mirrored to local history; safe persistence (unique index + dedup) + // is a separate enhancement. This guards against the omission silently changing. + const callbacks = await startAndCaptureCallbacks(); + + callbacks.onMessageCreate!(makeMessage({ id: 'wa-out-2', from: 'me@c.us', to: 'peer@c.us', fromMe: true })); + await flush(); + + expect(dispatchedEvents('message.sent')).toHaveLength(1); // it IS webhooked/emitted + expect(messageRepository.create).not.toHaveBeenCalled(); // but NOT persisted + expect(messageRepository.save).not.toHaveBeenCalled(); + }); + it('scopes the ack status UPDATE by sessionId, not just waMessageId', async () => { const callbacks = await startAndCaptureCallbacks(); expect(typeof callbacks.onMessageAck).toBe('function'); diff --git a/src/modules/session/session.service.ts b/src/modules/session/session.service.ts index 5bec9efc..c06c795b 100644 --- a/src/modules/session/session.service.ts +++ b/src/modules/session/session.service.ts @@ -591,7 +591,13 @@ export class SessionService implements OnModuleDestroy, OnModuleInit, OnApplicat return; } - // Dispatch to webhooks with potentially modified message + // NOTE: unlike onMessage (incoming), this path intentionally does NOT mirror the message + // to the `messages` table. message_create ALSO fires for API-originated sends, which the + // REST send path already persists — saving here would double-persist them. Safe + // persistence of phone-composed sends needs a unique (sessionId, waMessageId) index + + // de-dup and is tracked as a separate enhancement; until then this path only webhooks/ + // emits. So local message history reflects API sends + all inbound, but not sends + // composed on a linked phone. void this.webhookService.dispatch(id, 'message.sent', finalMessage); // Emit real-time event to WebSocket clients (as message.sent, not message.received) this.eventsGateway.emitMessageSent(id, finalMessage); diff --git a/src/modules/settings/settings.controller.spec.ts b/src/modules/settings/settings.controller.spec.ts new file mode 100644 index 00000000..cc3ed576 --- /dev/null +++ b/src/modules/settings/settings.controller.spec.ts @@ -0,0 +1,34 @@ +import { NotImplementedException } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { ConfigService } from '@nestjs/config'; +import { SettingsController } from './settings.controller'; +import { REQUIRED_ROLE_KEY } from '../auth/decorators/auth.decorators'; +import { ApiKeyRole } from '../auth/entities/api-key.entity'; + +// ConfigService stub: return the supplied default for every key. +const configStub = { + get: (_key: string, def?: T): T | undefined => def, +} as unknown as ConfigService; + +describe('SettingsController', () => { + it('GET /settings returns the environment-derived settings', () => { + const settings = new SettingsController(configStub).get(); + expect(settings).toHaveProperty('general'); + expect(settings).toHaveProperty('api'); + expect(settings).toHaveProperty('notifications'); + }); + + // The previous PUT mutated an in-memory field and returned 200 'updated' while persisting + // nothing and applying nothing to the runtime — a false success. Settings are env-derived and + // read-only at runtime, so the write path must say so (501) rather than fake success. + it('PUT /settings is read-only and throws 501 instead of a false-success 200', () => { + const controller = new SettingsController(configStub); + expect(() => controller.update({})).toThrow(NotImplementedException); + }); + + it('PUT /settings still requires the ADMIN role', () => { + const proto = SettingsController.prototype as unknown as Record; + const role = new Reflector().get(REQUIRED_ROLE_KEY, proto.update); + expect(role).toBe(ApiKeyRole.ADMIN); + }); +}); diff --git a/src/modules/settings/settings.controller.ts b/src/modules/settings/settings.controller.ts index d0c13cb9..e467930f 100644 --- a/src/modules/settings/settings.controller.ts +++ b/src/modules/settings/settings.controller.ts @@ -1,4 +1,4 @@ -import { Controller, Get, Put, Body } from '@nestjs/common'; +import { Controller, Get, Put, NotImplementedException } from '@nestjs/common'; import { ApiTags, ApiOperation, ApiResponse } from '@nestjs/swagger'; import { ConfigService } from '@nestjs/config'; import { RequireRole } from '../auth/decorators/auth.decorators'; @@ -61,24 +61,20 @@ export class SettingsController { @Put() @RequireRole(ApiKeyRole.ADMIN) - @ApiOperation({ summary: 'Update application settings' }) - @ApiResponse({ status: 200, description: 'Settings updated' }) - update(@Body() newSettings: Partial): Settings { - if (newSettings.general) { - this.settings.general = { - ...this.settings.general, - ...newSettings.general, - }; - } - if (newSettings.api) { - this.settings.api = { ...this.settings.api, ...newSettings.api }; - } - if (newSettings.notifications) { - this.settings.notifications = { - ...this.settings.notifications, - ...newSettings.notifications, - }; - } - return this.settings; + @ApiOperation({ summary: 'Settings are read-only at runtime (environment-derived)' }) + @ApiResponse({ + status: 501, + description: 'Settings are derived from environment configuration and cannot be changed at runtime', + }) + update(): never { + // Every Settings field is derived from environment variables and consumed at boot / + // decorator-evaluation time (ThrottlerModule.forRootAsync, port, webhook timeout, DB logging), + // and ConfigService is immutable at runtime — so a runtime write cannot actually take effect. + // The previous handler mutated an in-memory copy and returned 200 'updated' while persisting + // nothing and applying nothing: a false success. Be honest instead of pretending it worked. + throw new NotImplementedException( + 'Settings are derived from environment configuration and are read-only at runtime. ' + + 'Change the corresponding environment variable and restart the service.', + ); } } diff --git a/src/modules/template/entities/template.entity.ts b/src/modules/template/entities/template.entity.ts index 12ed27ff..9fdcb470 100644 --- a/src/modules/template/entities/template.entity.ts +++ b/src/modules/template/entities/template.entity.ts @@ -6,9 +6,13 @@ import { UpdateDateColumn, ManyToOne, JoinColumn, + Index, } from 'typeorm'; import { Session } from '../../session/entities/session.entity'; +// One template name per session: makes resolve-by-name deterministic and rejects duplicates. +// Mirrored by the AddTemplateNameUnique migration for non-synchronize (Postgres / opted-out) DBs. +@Index('IDX_templates_session_name', ['sessionId', 'name'], { unique: true }) @Entity('templates') export class Template { @PrimaryGeneratedColumn('uuid') diff --git a/src/modules/template/template.service.spec.ts b/src/modules/template/template.service.spec.ts index f20015f6..a9a0a97f 100644 --- a/src/modules/template/template.service.spec.ts +++ b/src/modules/template/template.service.spec.ts @@ -1,7 +1,7 @@ import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; -import { NotFoundException } from '@nestjs/common'; +import { ConflictException, NotFoundException } from '@nestjs/common'; import { TemplateService } from './template.service'; import { Template } from './entities/template.entity'; import { Session } from '../session/entities/session.entity'; @@ -120,13 +120,18 @@ describe('TemplateService', () => { expect(result).toBe(template); }); - it('should resolve by templateName', async () => { + it('should resolve by templateName deterministically (earliest first)', async () => { const template = createMockTemplate(); (repository.findOne as jest.Mock).mockResolvedValue(template); const result = await service.resolve('sess-1', { templateName: 'order-confirmation' }); - expect(repository.findOne).toHaveBeenCalledWith({ where: { name: 'order-confirmation', sessionId: 'sess-1' } }); + // Order by createdAt ASC so resolve-by-name is deterministic even before the unique index is + // enforced (and after, there is only one row anyway). + expect(repository.findOne).toHaveBeenCalledWith({ + where: { name: 'order-confirmation', sessionId: 'sess-1' }, + order: { createdAt: 'ASC' }, + }); expect(result).toBe(template); }); @@ -180,6 +185,31 @@ describe('TemplateService', () => { await expect(service.delete('sess-1', 'missing')).rejects.toThrow(NotFoundException); }); }); + + // ── name uniqueness (one name per session) ──────────────────────── + + describe('name uniqueness', () => { + const uniqueViolation = (): Error => + Object.assign(new Error('SQLITE_CONSTRAINT: UNIQUE constraint failed: templates.sessionId, templates.name'), { + code: 'SQLITE_CONSTRAINT', + }); + + it('translates a duplicate-name violation on create into 409 Conflict', async () => { + (repository.save as jest.Mock).mockRejectedValueOnce(uniqueViolation()); + await expect(service.create('sess-1', { name: 'dup', body: 'x' })).rejects.toThrow(ConflictException); + }); + + it('translates a duplicate-name violation on update into 409 Conflict', async () => { + (repository.findOne as jest.Mock).mockResolvedValue(createMockTemplate()); + (repository.save as jest.Mock).mockRejectedValueOnce(uniqueViolation()); + await expect(service.update('sess-1', 'tpl-uuid-1', { name: 'dup' })).rejects.toThrow(ConflictException); + }); + + it('rethrows a non-uniqueness DB error unchanged', async () => { + (repository.save as jest.Mock).mockRejectedValueOnce(new Error('disk I/O error')); + await expect(service.create('sess-1', { name: 'x', body: 'y' })).rejects.toThrow('disk I/O error'); + }); + }); }); // ── renderTemplate (shared utility) ─────────────────────────────────── diff --git a/src/modules/template/template.service.ts b/src/modules/template/template.service.ts index 705fe8ed..9d31bb20 100644 --- a/src/modules/template/template.service.ts +++ b/src/modules/template/template.service.ts @@ -1,10 +1,22 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { ConflictException, Injectable, NotFoundException } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { Template } from './entities/template.entity'; import { CreateTemplateDto, UpdateTemplateDto } from './dto'; import { createLogger } from '../../common/services/logger.service'; +/** True for a UNIQUE-constraint violation across both the SQLite and PostgreSQL drivers. */ +function isUniqueViolation(err: unknown): boolean { + const e = err as { code?: string; message?: string; driverError?: { code?: string } }; + return ( + e?.code === '23505' || // PostgreSQL unique_violation + e?.driverError?.code === '23505' || + e?.code === 'SQLITE_CONSTRAINT' || + e?.driverError?.code === 'SQLITE_CONSTRAINT' || + (typeof e?.message === 'string' && /unique constraint/i.test(e.message)) + ); +} + @Injectable() export class TemplateService { private readonly logger = createLogger('TemplateService'); @@ -23,9 +35,16 @@ export class TemplateService { footer: dto.footer ?? null, }); - const saved = await this.templateRepository.save(template); - this.logger.log('Template created', { sessionId, templateId: saved.id, name: saved.name }); - return saved; + try { + const saved = await this.templateRepository.save(template); + this.logger.log('Template created', { sessionId, templateId: saved.id, name: saved.name }); + return saved; + } catch (err) { + if (isUniqueViolation(err)) { + throw new ConflictException(`A template named '${dto.name}' already exists for this session`); + } + throw err; + } } async findBySession(sessionId: string): Promise { @@ -55,7 +74,12 @@ export class TemplateService { } if (templateName) { - const template = await this.templateRepository.findOne({ where: { name: templateName, sessionId } }); + // Order by createdAt ASC so resolution is deterministic if more than one row shares a name + // (possible only on a DB predating the unique index); the migration keeps the earliest too. + const template = await this.templateRepository.findOne({ + where: { name: templateName, sessionId }, + order: { createdAt: 'ASC' }, + }); if (!template) { throw new NotFoundException(`Template with name '${templateName}' not found`); } @@ -73,7 +97,14 @@ export class TemplateService { if (dto.header !== undefined) template.header = dto.header; if (dto.footer !== undefined) template.footer = dto.footer; - return this.templateRepository.save(template); + try { + return await this.templateRepository.save(template); + } catch (err) { + if (isUniqueViolation(err)) { + throw new ConflictException(`A template named '${template.name}' already exists for this session`); + } + throw err; + } } async delete(sessionId: string, id: string): Promise { diff --git a/src/plugins/engines/baileys/index.spec.ts b/src/plugins/engines/baileys/index.spec.ts index 38d55f2a..83ce98f2 100644 --- a/src/plugins/engines/baileys/index.spec.ts +++ b/src/plugins/engines/baileys/index.spec.ts @@ -63,4 +63,24 @@ describe('BaileysPlugin.createEngine (opaque config)', () => { plugin.createEngine({ sessionId: 'sess-1' }); expect(BaileysAdapter).toHaveBeenCalledWith(expect.objectContaining({ sessionId: 'sess-1', messageStore: store })); }); + + it('Uses the constructor-supplied engine config when onLoad never ran (enable-failure path)', () => { + const plugin = new BaileysPlugin(undefined, { baileys: { authDir: '/op/baileys' } }); + plugin.createEngine({ sessionId: 'sess-3' }); + expect(BaileysAdapter).toHaveBeenCalledWith( + expect.objectContaining({ sessionId: 'sess-3', authDir: '/op/baileys' }), + ); + }); + + it('Prefers context.config over the constructor blob on the healthy enable path', () => { + const plugin = new BaileysPlugin(undefined, { baileys: { authDir: '/ctor/baileys' } }); + void plugin.onLoad({ + config: { baileys: { authDir: '/context/baileys' } }, + logger: { log: jest.fn() }, + } as unknown as PluginContext); + plugin.createEngine({ sessionId: 'sess-4' }); + expect(BaileysAdapter).toHaveBeenCalledWith( + expect.objectContaining({ sessionId: 'sess-4', authDir: '/context/baileys' }), + ); + }); }); diff --git a/src/plugins/engines/baileys/index.ts b/src/plugins/engines/baileys/index.ts index 21e7859c..fe1dab13 100644 --- a/src/plugins/engines/baileys/index.ts +++ b/src/plugins/engines/baileys/index.ts @@ -12,7 +12,13 @@ export class BaileysPlugin implements IEnginePlugin { type = PluginType.ENGINE as const; private context?: PluginContext; - constructor(private readonly messageStore?: BaileysMessageStore) {} + // RegisteredConfig (the engine config blob) is also supplied at construction so createEngine + // has operator config even if enablePlugin fails before onLoad runs (this.context stays unset). The + // healthy path still prefers context.config (it carries any persisted-override merge). + constructor( + private readonly messageStore?: BaileysMessageStore, + private readonly registeredConfig?: Record, + ) {} onLoad(context: PluginContext): Promise { this.context = context; @@ -38,7 +44,7 @@ export class BaileysPlugin implements IEnginePlugin { // Baileys' own config namespace, read from the opaque per-engine blob the factory supplies via // context.config (the `engine` sub-tree in configuration.ts). Per-call config carries only // engine-neutral fields (sessionId, proxy). - const engineConfig = (this.context?.config ?? {}) as { baileys?: { authDir?: string } }; + const engineConfig = (this.context?.config ?? this.registeredConfig ?? {}) as { baileys?: { authDir?: string } }; const authDir = engineConfig.baileys?.authDir ?? './data/baileys'; return new BaileysAdapter({ diff --git a/src/plugins/engines/whatsapp-web-js/index.spec.ts b/src/plugins/engines/whatsapp-web-js/index.spec.ts index 5dd732c5..1c3a0f36 100644 --- a/src/plugins/engines/whatsapp-web-js/index.spec.ts +++ b/src/plugins/engines/whatsapp-web-js/index.spec.ts @@ -48,4 +48,35 @@ describe('WhatsAppWebJsPlugin.createEngine (opaque config)', () => { }), ); }); + + it('Uses the constructor-supplied engine config when onLoad never ran (enable-failure path)', () => { + // EngineFactory now also passes the engine blob at construction. If enablePlugin fails before + // onLoad runs, this.context is never set — the ctor blob must still supply operator config + // instead of silently degrading to defaults (which dropped sessionDataPath/executablePath). + const plugin = new WhatsAppWebJsPlugin({ + sessionDataPath: '/op/sessions', + puppeteer: { headless: false, args: ['--flag'], executablePath: '/usr/bin/chromium' }, + }); + + plugin.createEngine({ sessionId: 'sess-3' }); + + expect(WhatsAppWebJsAdapter).toHaveBeenCalledWith( + expect.objectContaining({ + sessionId: 'sess-3', + sessionDataPath: '/op/sessions', + puppeteer: { headless: false, args: ['--flag'], executablePath: '/usr/bin/chromium' }, + }), + ); + }); + + it('Prefers context.config over the constructor blob on the healthy enable path', () => { + const plugin = new WhatsAppWebJsPlugin({ sessionDataPath: '/ctor/path' }); + withContext(plugin, { sessionDataPath: '/context/path' }); + + plugin.createEngine({ sessionId: 'sess-4' }); + + expect(WhatsAppWebJsAdapter).toHaveBeenCalledWith( + expect.objectContaining({ sessionId: 'sess-4', sessionDataPath: '/context/path' }), + ); + }); }); diff --git a/src/plugins/engines/whatsapp-web-js/index.ts b/src/plugins/engines/whatsapp-web-js/index.ts index d77a2cd2..d8c6d76a 100644 --- a/src/plugins/engines/whatsapp-web-js/index.ts +++ b/src/plugins/engines/whatsapp-web-js/index.ts @@ -11,6 +11,11 @@ export class WhatsAppWebJsPlugin implements IEnginePlugin { type = PluginType.ENGINE as const; private context?: PluginContext; + // The engine config blob is also supplied at construction so createEngine has operator + // config even if enablePlugin fails before onLoad runs (which would leave this.context unset). + // The healthy path still prefers context.config (it carries any persisted-override merge). + constructor(private readonly registeredConfig?: Record) {} + onLoad(context: PluginContext): Promise { this.context = context; context.logger.log('WhatsApp-web.js engine plugin loaded'); @@ -35,7 +40,7 @@ export class WhatsAppWebJsPlugin implements IEnginePlugin { // Browser config is this engine's OWN namespace, read from the opaque per-engine blob the // factory supplies via context.config (the `engine` sub-tree in configuration.ts). The // per-call config carries only engine-neutral fields (sessionId, proxy). - const engineConfig = (this.context?.config ?? {}) as { + const engineConfig = (this.context?.config ?? this.registeredConfig ?? {}) as { sessionDataPath?: string; puppeteer?: { headless?: boolean; args?: string[]; executablePath?: string }; };