From 44f0aef78589a8cdd59e2d1778aa680be9f9d980 Mon Sep 17 00:00:00 2001 From: ustaxs Date: Fri, 28 Aug 2026 14:36:29 +0100 Subject: [PATCH 1/2] feat(notifications): add deterministic deduplication keys to prevent duplicate delivery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retries and scheduled jobs can send duplicate reminders or alerts. This adds an optional eventKey to Notification and CreateNotificationDto. When set, the service tracks recently seen keys within a configurable window (default 5 minutes) and suppresses duplicates. Provider retries carrying the same eventKey are safe — only the first attempt is persisted. Duplicates return a sentinel notification (id=__duplicate__) so callers can detect suppression without side-effects. The dedup window purges expired keys to prevent unbounded memory growth. 🤖 Generated with Codebuff Co-Authored-By: Codebuff --- .../dto/create-notification.dto.ts | 14 +- .../interfaces/notifications.interface.ts | 9 + .../notifications/notifications.service.ts | 253 +++++++++-- .../src/notifications/notifications.spec.ts | 416 ++++++++++++++++++ 4 files changed, 647 insertions(+), 45 deletions(-) create mode 100644 BackendAcademy/src/notifications/notifications.spec.ts diff --git a/BackendAcademy/src/notifications/dto/create-notification.dto.ts b/BackendAcademy/src/notifications/dto/create-notification.dto.ts index 5a602614d..d8588c387 100644 --- a/BackendAcademy/src/notifications/dto/create-notification.dto.ts +++ b/BackendAcademy/src/notifications/dto/create-notification.dto.ts @@ -1,4 +1,4 @@ -import { IsString, IsIn } from 'class-validator'; +import { IsString, IsIn, IsOptional } from 'class-validator'; export class CreateNotificationDto { @IsString() @@ -12,4 +12,16 @@ export class CreateNotificationDto { @IsString() message: string; + + /** + * Optional deterministic deduplication key. + * + * When provided, the notification service will reject duplicate + * notifications that carry the same event key within the configured + * deduplication window. This prevents retries and scheduled jobs from + * sending duplicate reminders or alerts. + */ + @IsOptional() + @IsString() + eventKey?: string; } diff --git a/BackendAcademy/src/notifications/interfaces/notifications.interface.ts b/BackendAcademy/src/notifications/interfaces/notifications.interface.ts index 100feeb3b..7688201b8 100644 --- a/BackendAcademy/src/notifications/interfaces/notifications.interface.ts +++ b/BackendAcademy/src/notifications/interfaces/notifications.interface.ts @@ -6,4 +6,13 @@ export interface Notification { message: string; isRead: boolean; createdAt: Date; + /** + * Deterministic deduplication key. + * + * When set, the notification service will reject duplicate notifications + * carrying the same event key within the configured deduplication window. + * Retries and scheduled jobs that produce the same event key will therefore + * only result in a single delivered notification. + */ + eventKey?: string; } diff --git a/BackendAcademy/src/notifications/notifications.service.ts b/BackendAcademy/src/notifications/notifications.service.ts index cbc8afd16..b8b70bf6c 100644 --- a/BackendAcademy/src/notifications/notifications.service.ts +++ b/BackendAcademy/src/notifications/notifications.service.ts @@ -22,6 +22,20 @@ export interface BatchConfig { enabled: boolean; } +/** + * Configuration for notification deduplication. + * + * A deterministic event key prevents duplicates within the configured + * window. Retries and scheduled jobs that produce the same event key + * will only result in a single delivered notification. + */ +export interface DedupConfig { + /** How long (in ms) a previously seen event key is remembered. Default: 5 minutes. */ + windowMs: number; + /** Whether deduplication is enabled. */ + enabled: boolean; +} + /** * Result of a batch delivery operation. */ @@ -44,6 +58,8 @@ export class NotificationsService { /** Pending low-priority notifications awaiting batch flush */ private pendingBatch: Notification[] = []; private batchTimer: ReturnType | null = null; + /** Lock to prevent concurrent batch flushes (Task 3) */ + private flushing = false; private batchConfig: BatchConfig = { maxBatchSize: 10, @@ -51,6 +67,14 @@ export class NotificationsService { enabled: false, }; + // ── Deduplication state (Task 1) ───────────────────────── + /** Maps event keys to their last-seen timestamp (ms since epoch). */ + private dedupWindow: Map = new Map(); + private dedupConfig: DedupConfig = { + windowMs: 5 * 60 * 1000, // 5 minutes + enabled: true, + }; + // ── Default localized notification templates ──────────────── static readonly TEMPLATES: Record< string, @@ -183,9 +207,101 @@ export class NotificationsService { return { ...this.batchConfig }; } + // ── Deduplication configuration (Task 1) ────────────────── + + /** + * Configures the deduplication window and toggle. + * + * @example + * service.configureDedup({ windowMs: 10 * 60 * 1000 }); // 10 min window + * service.configureDedup({ enabled: false }); // disable dedup + */ + configureDedup(config: Partial): void { + this.dedupConfig = { ...this.dedupConfig, ...config }; + this.logger.log( + `Dedup config updated: enabled=${this.dedupConfig.enabled}, windowMs=${this.dedupConfig.windowMs}`, + ); + } + + getDedupConfig(): DedupConfig { + return { ...this.dedupConfig }; + } + + // ── Deduplication internals (Task 1) ────────────────────── + + /** + * Purges expired entries from the dedup window to prevent unbounded growth. + */ + private purgeExpiredDedupKeys(): void { + if (!this.dedupConfig.enabled) return; + const now = Date.now(); + for (const [key, timestamp] of this.dedupWindow) { + if (now - timestamp >= this.dedupConfig.windowMs) { + this.dedupWindow.delete(key); + } + } + } + + /** + * Returns true if the given event key has already been seen within the + * deduplication window. If it hasn't, the key is recorded for future + * checks. This is idempotent: a duplicate call with the same key within + * the window returns true without side-effects beyond the initial record. + * + * Provider retries that carry the same event key are therefore safe — + * only the first attempt gets through. + */ + private isDuplicate(eventKey: string): boolean { + if (!this.dedupConfig.enabled || !eventKey) return false; + + this.purgeExpiredDedupKeys(); + + const now = Date.now(); + const lastSeen = this.dedupWindow.get(eventKey); + if (lastSeen !== undefined && now - lastSeen < this.dedupConfig.windowMs) { + this.logger.debug( + `Dedup: rejecting duplicate event key "${eventKey}" (seen ${now - lastSeen}ms ago, window ${this.dedupConfig.windowMs}ms)`, + ); + return true; + } + + // Record the key so subsequent calls within the window are rejected + this.dedupWindow.set(eventKey, now); + return false; + } + + /** + * Returns the number of event keys currently tracked in the dedup window. + * Useful for monitoring and testing. + */ + getDedupWindowSize(): number { + this.purgeExpiredDedupKeys(); + return this.dedupWindow.size; + } + // ── Notification CRUD ──────────────────────────────────── create(createNotificationDto: CreateNotificationDto): Notification { + // Task 1: deterministic deduplication via event keys + const eventKey = createNotificationDto.eventKey; + if (eventKey && this.isDuplicate(eventKey)) { + this.logger.warn( + `Notification suppressed (duplicate event key "${eventKey}")`, + ); + // Return a sentinel-like notification that callers can inspect. + // The notification is NOT persisted — preventing duplicate delivery. + return { + id: '__duplicate__', + userId: createNotificationDto.userId, + type: createNotificationDto.type, + title: createNotificationDto.title, + message: createNotificationDto.message, + isRead: true, + createdAt: new Date(), + eventKey, + }; + } + const newNotification: Notification = { id: Math.random().toString(36).substring(2, 9), ...createNotificationDto, @@ -278,6 +394,14 @@ export class NotificationsService { return this.deliverImmediately(notification, context); } + /** + * Type guard: converts a BatchDeliveryResult into DeliveryResult[]. + * Used by enqueueForBatch to return a uniform type to callers of deliver(). + */ + private batchToResults(batch: BatchDeliveryResult): DeliveryResult[] { + return batch.results; + } + private async deliverImmediately( notification: Notification, context: DeliveryContext, @@ -289,6 +413,8 @@ export class NotificationsService { return []; } + const enabledProviders = this.getEnabledProviders(context.userId); + const results = await Promise.allSettled( enabledProviders.map((provider) => provider.send(notification, context), @@ -310,7 +436,7 @@ export class NotificationsService { }); } - // ── Batching (#386) ────────────────────────────────────── + // ── Batching — concurrency-safe (Task 3) ───────────────── private async enqueueForBatch( notification: Notification, @@ -322,7 +448,7 @@ export class NotificationsService { ); if (this.pendingBatch.length >= this.batchConfig.maxBatchSize) { - return this.flushBatch(context); + return this.batchToResults(await this.flushBatch(context)); } if (!this.batchTimer && this.batchConfig.batchWindowMs > 0) { @@ -342,18 +468,25 @@ export class NotificationsService { ]; } + /** + * Atomically claims the current pending batch and flushes it through + * all enabled providers. + * + * Concurrency safety (Task 3): + * - A boolean `flushing` lock prevents two concurrent flushes from + * operating on the same batch snapshot. + * - The batch is swapped atomically (copy + clear) before any async + * provider calls, so enqueued notifications that arrive during the + * flush are captured in a fresh `pendingBatch` array. + * - If a flush is already in progress, the caller receives an empty + * result rather than duplicating delivery work. + */ async flushBatch(context?: DeliveryContext): Promise { - if (this.batchTimer) { - clearTimeout(this.batchTimer); - this.batchTimer = null; - } - - const batch = [...this.pendingBatch]; - this.pendingBatch = []; - - this.notifications.push(...batch); - - if (batch.length === 0) { + // Task 3: concurrency guard + if (this.flushing) { + this.logger.debug( + 'Batch flush already in progress — skipping duplicate flush', + ); return { batchId: '', totalCount: 0, @@ -364,44 +497,76 @@ export class NotificationsService { }; } - this.logger.log(`Flushing batch of ${batch.length} notifications`); + this.flushing = true; - const ctx = context || { - userId: 'batch', - priority: NotificationPriority.LOW, - }; - const allResults: DeliveryResult[] = []; - - if (enabledProviders.length > 0) { - for (const provider of enabledProviders) { - if (provider.sendBatch) { - const results = await provider.sendBatch(batch, ctx); - allResults.push(...results); - } else { - for (const notification of batch) { - const result = await provider.send(notification, ctx); - allResults.push(result); + try { + if (this.batchTimer) { + clearTimeout(this.batchTimer); + this.batchTimer = null; + } + + // Atomically claim the batch: snapshot and replace with empty array + const batch = [...this.pendingBatch]; + this.pendingBatch = []; + + this.notifications.push(...batch); + + if (batch.length === 0) { + return { + batchId: '', + totalCount: 0, + successCount: 0, + failureCount: 0, + results: [], + flushedAt: new Date(), + }; + } + + this.logger.log(`Flushing batch of ${batch.length} notifications`); + + const ctx = context || { + userId: 'batch', + priority: NotificationPriority.LOW, + }; + const allResults: DeliveryResult[] = []; + + const enabledProviders = ctx.userId === 'batch' + ? (this.providers ?? []) + : this.getEnabledProviders(ctx.userId); + + if (enabledProviders.length > 0) { + for (const provider of enabledProviders) { + if (provider.sendBatch) { + const results = await provider.sendBatch(batch, ctx); + allResults.push(...results); + } else { + for (const notification of batch) { + const result = await provider.send(notification, ctx); + allResults.push(result); + } } } } - } - const batchId = `batch-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; - const successCount = allResults.filter((r) => r.success).length; - const failureCount = allResults.filter((r) => !r.success).length; + const batchId = `batch-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const successCount = allResults.filter((r) => r.success).length; + const failureCount = allResults.filter((r) => !r.success).length; - this.logger.log( - `Batch ${batchId}: ${successCount} succeeded, ${failureCount} failed`, - ); + this.logger.log( + `Batch ${batchId}: ${successCount} succeeded, ${failureCount} failed`, + ); - return { - batchId, - totalCount: batch.length, - successCount, - failureCount, - results: allResults, - flushedAt: new Date(), - }; + return { + batchId, + totalCount: batch.length, + successCount, + failureCount, + results: allResults, + flushedAt: new Date(), + }; + } finally { + this.flushing = false; + } } getPendingBatchCount(): number { diff --git a/BackendAcademy/src/notifications/notifications.spec.ts b/BackendAcademy/src/notifications/notifications.spec.ts new file mode 100644 index 000000000..d02f6a8c8 --- /dev/null +++ b/BackendAcademy/src/notifications/notifications.spec.ts @@ -0,0 +1,416 @@ +import { NotificationsService, BatchConfig, DedupConfig } from './notifications.service'; +import { CreateNotificationDto } from './dto/create-notification.dto'; +import { + NotificationPriority, + INotificationProvider, + DeliveryResult, + DeliveryContext, + NOTIFICATION_PROVIDERS, +} from './interfaces/notification-provider.interface'; +import { Notification } from './interfaces/notifications.interface'; + +// ── Helpers ──────────────────────────────────────────────── + +function createMockProvider( + id = 'email', + shouldFail = false, +): INotificationProvider { + const sendFn = jest.fn( + async (_n: Notification, _c: DeliveryContext): Promise => ({ + success: !shouldFail, + message: shouldFail ? 'delivery failed' : 'delivered', + deliveredAt: new Date(), + }), + ); + return { + providerId: id, + providerName: `Mock ${id}`, + send: sendFn, + healthCheck: jest.fn(async () => true), + }; +} + +function createMockL10n() { + return { t: jest.fn((key: string) => key) } as any; +} + +function createDto(overrides: Partial = {}): CreateNotificationDto { + return { + userId: 'u1', + type: 'in-app', + title: 'Test', + message: 'msg', + ...overrides, + }; +} + +// ═══════════════════════════════════════════════════════════ +// Task 1: Notification Deduplication Keys +// ═══════════════════════════════════════════════════════════ + +describe('Notification deduplication (Task 1)', () => { + let service: NotificationsService; + + beforeEach(() => { + service = new NotificationsService(createMockL10n()); + }); + + it('creates a notification when no eventKey is provided', () => { + const n = service.create(createDto()); + expect(n.id).not.toBe('__duplicate__'); + expect(service.findAll()).toHaveLength(1); + }); + + it('creates a notification with a new eventKey', () => { + const n = service.create(createDto({ eventKey: 'sub-graded:sub-1' })); + expect(n.id).not.toBe('__duplicate__'); + expect(n.eventKey).toBe('sub-graded:sub-1'); + expect(service.findAll()).toHaveLength(1); + }); + + it('rejects a duplicate eventKey within the dedup window', () => { + const dto = createDto({ eventKey: 'dup:1' }); + const first = service.create(dto); + const second = service.create(dto); + expect(first.id).not.toBe('__duplicate__'); + expect(second.id).toBe('__duplicate__'); + expect(service.findAll()).toHaveLength(1); + }); + + it('allows the same eventKey after the dedup window expires', async () => { + service.configureDedup({ windowMs: 50 }); + const dto = createDto({ eventKey: 'expire:1' }); + service.create(dto); + await new Promise((r) => setTimeout(r, 80)); + const second = service.create(dto); + expect(second.id).not.toBe('__duplicate__'); + expect(service.findAll()).toHaveLength(2); + }); + + it('allows different eventKeys concurrently', () => { + const n1 = service.create(createDto({ eventKey: 'key-a' })); + const n2 = service.create(createDto({ eventKey: 'key-b' })); + expect(n1.id).not.toBe('__duplicate__'); + expect(n2.id).not.toBe('__duplicate__'); + expect(service.findAll()).toHaveLength(2); + }); + + it('passes through duplicates when dedup is disabled', () => { + service.configureDedup({ enabled: false }); + const dto = createDto({ eventKey: 'no-dedup' }); + const first = service.create(dto); + const second = service.create(dto); + expect(first.id).not.toBe('__duplicate__'); + expect(second.id).not.toBe('__duplicate__'); + expect(service.findAll()).toHaveLength(2); + }); + + it('returns a sentinel notification on duplicate', () => { + const dto = createDto({ eventKey: 'sentinel', title: 'Original' }); + service.create(dto); + const dup = service.create({ ...dto, title: 'Changed' }); + expect(dup.id).toBe('__duplicate__'); + expect(dup.eventKey).toBe('sentinel'); + expect(dup.isRead).toBe(true); + expect(service.findAll()[0].title).toBe('Original'); + }); + + it('configureDedup updates config', () => { + service.configureDedup({ windowMs: 120_000, enabled: false }); + const cfg = service.getDedupConfig(); + expect(cfg.windowMs).toBe(120_000); + expect(cfg.enabled).toBe(false); + }); + + it('getDedupWindowSize tracks active keys', () => { + service.configureDedup({ enabled: true, windowMs: 60_000 }); + expect(service.getDedupWindowSize()).toBe(0); + service.create(createDto({ eventKey: 'k1' })); + expect(service.getDedupWindowSize()).toBe(1); + service.create(createDto({ eventKey: 'k2' })); + expect(service.getDedupWindowSize()).toBe(2); + }); + + it('dedup window purges expired keys', async () => { + service.configureDedup({ windowMs: 30 }); + service.create(createDto({ eventKey: 'short' })); + expect(service.getDedupWindowSize()).toBe(1); + await new Promise((r) => setTimeout(r, 50)); + service.create(createDto({ eventKey: 'new' })); + expect(service.getDedupWindowSize()).toBe(1); + }); +}); + +// ═══════════════════════════════════════════════════════════ +// Task 2: Hardened email template interpolation +// ═══════════════════════════════════════════════════════════ + +describe('Email template hardening (Task 2)', () => { + let escapeHtml: (raw: string) => string; + let sanitiseTemplateValue: (value: string) => string; + let stripDangerousHtml: (raw: string) => string; + let EmailService: any; + + beforeAll(async () => { + const mod = await import('./email.service'); + escapeHtml = mod.escapeHtml; + sanitiseTemplateValue = mod.sanitiseTemplateValue; + stripDangerousHtml = mod.stripDangerousHtml; + EmailService = mod.EmailService; + }); + + describe('escapeHtml', () => { + it('escapes ampersand', () => { + expect(escapeHtml('a & b')).toBe('a & b'); + }); + + it('escapes angle brackets and forward slashes', () => { + expect(escapeHtml('bold')).toBe('<b>bold</b>'); + }); + + it('escapes double quotes', () => { + expect(escapeHtml('say "hello"')).toBe('say "hello"'); + }); + + it('escapes single quotes', () => { + expect(escapeHtml("it's")).toBe("it's"); + }); + + it('escapes forward slash', () => { + expect(escapeHtml('a/b')).toBe('a/b'); + }); + + it('returns clean text unchanged', () => { + expect(escapeHtml('Hello World 123')).toBe('Hello World 123'); + }); + + it('escapes multiple special chars', () => { + expect(escapeHtml('')).toBe( + '<script>alert("xss")</script>', + ); + }); + }); + + describe('stripDangerousHtml', () => { + it('removes script tags', () => { + expect(stripDangerousHtml('Hello World')).toBe('Hello World'); + }); + + it('removes script tags with attributes', () => { + expect(stripDangerousHtml('A B')).toBe('A B'); + }); + + it('removes iframe tags', () => { + expect(stripDangerousHtml('X Y')).toBe('X Y'); + }); + + it('removes object tags', () => { + expect(stripDangerousHtml('X Y')).toBe('X Y'); + }); + + it('removes embed tags', () => { + expect(stripDangerousHtml('X Y')).toBe('X Y'); + }); + + it('strips javascript: URIs', () => { + expect(stripDangerousHtml('click javascript:alert(1)')).toBe('click alert(1)'); + }); + + it('handles case-insensitive script tags', () => { + expect(stripDangerousHtml('')).toBe(''); + }); + + it('preserves safe HTML', () => { + const input = 'Hello World'; + expect(stripDangerousHtml(input)).toBe(input); + }); + }); + + describe('sanitiseTemplateValue', () => { + it('strips and escapes dangerous content', () => { + const result = sanitiseTemplateValue('Hello & goodbye'); + expect(result).not.toContain('