FRE-4520: Fix security vulnerabilities in notification template system

- Fix HTML injection vulnerability with proper entity encoding
- Fix rate limit cleanup bug (count vs timestamp confusion)
- Add URL validation to prevent open redirect attacks
- Add expiration to in-memory deduplication entries
- Use Zod schema for config validation
- Add email format validation

All 29 tests passing. Ready for Code Reviewer final review.

Co-Authored-By: Paperclip <noreply@paperclip.ing>

packages/shared-notifications/SECURITY_REMEDIATION.md

@@ -0,0 +1,48 @@
+## Security Remediation Complete
+
+All 4 Medium and 2 Low severity findings have been addressed:
+
+### Medium Severity (Fixed)
+
+**1. HTML Injection via Template Variables** (`template.service.ts:168`)
+- Added `escapeHtml()` method with HTML entity encoding
+- Variables substituted in HTML context are now properly escaped
+- Handles &, <, >, ", and ' characters
+
+**2. Rate Limit Cleanup Logic Bug** (`email.service.ts:16-23`)
+- Created `RateLimitEntry` interface with `count` and `lastSentAt` fields
+- Cleanup now correctly compares timestamps instead of counts
+- Rate limiting will work effectively across cleanup cycles
+
+**3. Open Redirect via URL Template Variables** (`template.service.ts`)
+- Added `TRUSTED_DOMAINS` allowlist (shieldai.com, app.shieldai.com, api.shieldai.com)
+- Added `validateUrl()` method that validates URLs against trusted domains
+- Invalid URLs default to `/` to prevent phishing attacks
+
+**4. In-Memory Deduplication Expiration** (`notification.service.ts:62-88`)
+- Created `DeduplicationEntry` interface with `externalIds` and `expiresAt` fields
+- In-memory dedup now respects the configured window_seconds TTL
+- Prevents indefinite growth of pending deduplication sets
+
+### Low Severity (Fixed)
+
+**5. Zod Schema Validation** (`notification.config.ts`)
+- `loadNotificationConfig()` now parses through `NotificationConfigSchema.parse()`
+- Invalid configurations will throw at startup instead of runtime
+
+**6. Email Format Validation** (`email.service.ts:33`)
+- Added `EMAIL_PATTERN` regex for basic email validation
+- Invalid email formats throw before attempting to send
+
+### Test Results
+- All 29 tests passing ✅
+- No new TypeScript errors introduced
+
+### Files Modified
+- `packages/shared-notifications/src/services/template.service.ts`
+- `packages/shared-notifications/src/services/email.service.ts`
+- `packages/shared-notifications/src/services/notification.service.ts`
+- `packages/shared-notifications/src/config/notification.config.ts`
+
+### Next Action
+Ready for Code Reviewer final review before marking security review complete.

packages/shared-notifications/src/config/notification.config.ts

@@ -35,35 +35,39 @@ export const NotificationConfigSchema = z.object({
 
 export type NotificationConfig = z.infer<typeof NotificationConfigSchema>;
 
-export const loadNotificationConfig = (): NotificationConfig => ({
-  resend: {
-    apiKey: process.env.RESEND_API_KEY!,
-    baseUrl: process.env.RESEND_BASE_URL || 'https://api.resend.com',
-  },
-  fcm: {
-    privateKey: process.env.FCM_PRIVATE_KEY!,
-    projectId: process.env.FCM_PROJECT_ID!,
-    clientEmail: process.env.FCM_CLIENT_EMAIL!,
-  },
-  apns: {
-    key: process.env.APNS_KEY!,
-    keyId: process.env.APNS_KEY_ID!,
-    teamId: process.env.APNS_TEAM_ID!,
-    bundleId: process.env.APNS_BUNDLE_ID!,
-  },
-  twilio: {
-    accountSid: process.env.TWILIO_ACCOUNT_SID!,
-    authToken: process.env.TWILIO_AUTH_TOKEN!,
-    messagingServiceSid: process.env.TWILIO_MESSAGING_SERVICE_SID!,
-  },
-  rateLimits: {
-    emailPerMinute: parseInt(process.env.EMAIL_RATE_LIMIT || '60', 10),
-    smsPerMinute: parseInt(process.env.SMS_RATE_LIMIT || '30', 10),
-    pushPerMinute: parseInt(process.env.PUSH_RATE_LIMIT || '100', 10),
-    windowSeconds: parseInt(process.env.RATE_LIMIT_WINDOW_SECONDS || '60', 10),
-  },
-  redis: {
-    url: process.env.REDIS_URL || 'redis://localhost:6379',
-    dedupWindowSeconds: parseInt(process.env.DEDUP_WINDOW_SECONDS || '300', 10),
-  },
-});
+export const loadNotificationConfig = (): NotificationConfig => {
+  const config = {
+    resend: {
+      apiKey: process.env.RESEND_API_KEY!,
+      baseUrl: process.env.RESEND_BASE_URL || 'https://api.resend.com',
+    },
+    fcm: {
+      privateKey: process.env.FCM_PRIVATE_KEY!,
+      projectId: process.env.FCM_PROJECT_ID!,
+      clientEmail: process.env.FCM_CLIENT_EMAIL!,
+    },
+    apns: {
+      key: process.env.APNS_KEY!,
+      keyId: process.env.APNS_KEY_ID!,
+      teamId: process.env.APNS_TEAM_ID!,
+      bundleId: process.env.APNS_BUNDLE_ID!,
+    },
+    twilio: {
+      accountSid: process.env.TWILIO_ACCOUNT_SID!,
+      authToken: process.env.TWILIO_AUTH_TOKEN!,
+      messagingServiceSid: process.env.TWILIO_MESSAGING_SERVICE_SID!,
+    },
+    rateLimits: {
+      emailPerMinute: parseInt(process.env.EMAIL_RATE_LIMIT || '60', 10),
+      smsPerMinute: parseInt(process.env.SMS_RATE_LIMIT || '30', 10),
+      pushPerMinute: parseInt(process.env.PUSH_RATE_LIMIT || '100', 10),
+      windowSeconds: parseInt(process.env.RATE_LIMIT_WINDOW_SECONDS || '60', 10),
+    },
+    redis: {
+      url: process.env.REDIS_URL || 'redis://localhost:6379',
+      dedupWindowSeconds: parseInt(process.env.DEDUP_WINDOW_SECONDS || '300', 10),
+    },
+  };
+
+  return NotificationConfigSchema.parse(config);
+};

packages/shared-notifications/src/services/email.service.ts

@@ -7,22 +7,33 @@ import { TemplateService } from './template.service';
 const config = loadNotificationConfig();
 const resend = new Resend(config.resend.apiKey);
 
+interface RateLimitEntry {
+  count: number;
+  lastSentAt: number;
+}
+
+const EMAIL_PATTERN = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+
 export class EmailService {
   private static instance: EmailService;
-  private sentCount = new Map<string, number>();
+  private sentCount = new Map<string, RateLimitEntry>();
   private cleanupInterval: NodeJS.Timeout;
 
   private constructor() {
     this.cleanupInterval = setInterval(() => {
       const now = Date.now();
-      for (const [key, timestamp] of this.sentCount.entries()) {
-        if (now - timestamp > 60000) {
+      for (const [key, entry] of this.sentCount.entries()) {
+        if (now - entry.lastSentAt > 60000) {
           this.sentCount.delete(key);
         }
       }
     }, 60000);
   }
 
+  private validateEmail(email: string): boolean {
+    return EMAIL_PATTERN.test(email);
+  }
+
   static getInstance(): EmailService {
     if (!EmailService.instance) {
       EmailService.instance = new EmailService();
@@ -32,12 +43,16 @@ export class EmailService {
 
   async send(notification: EmailNotification): Promise<NotificationResult> {
     const rateLimitKey = `email:${notification.to}`;
-    const currentCount = this.sentCount.get(rateLimitKey) || 0;
+    const entry = this.sentCount.get(rateLimitKey) || { count: 0, lastSentAt: Date.now() };
 
-    if (currentCount >= config.rateLimits.emailPerMinute) {
+    if (entry.count >= config.rateLimits.emailPerMinute) {
       throw new Error(`Email rate limit exceeded for ${notification.to}`);
     }
 
+    if (!this.validateEmail(notification.to)) {
+      throw new Error(`Invalid email format: ${notification.to}`);
+    }
+
     try {
       const { data, error } = await resend.emails.send({
         from: notification.from || 'ShieldAI <noreply@shieldai.com>',
@@ -63,7 +78,10 @@ export class EmailService {
         };
       }
 
-      this.sentCount.set(rateLimitKey, currentCount + 1);
+      this.sentCount.set(rateLimitKey, { 
+      count: entry.count + 1, 
+      lastSentAt: Date.now() 
+    });
 
       return {
         notificationId: `email-${data?.id || Date.now()}`,

packages/shared-notifications/src/services/notification.service.ts

@@ -21,6 +21,11 @@ export interface RateLimitResult {
   resetInSeconds: number;
 }
 
+interface DeduplicationEntry {
+  externalIds: Set<string>;
+  expiresAt: number;
+}
+
 export class NotificationService {
   private static instance: NotificationService;
   private emailService: EmailService;
@@ -28,7 +33,7 @@ export class NotificationService {
   private pushService: PushService;
   private redisService: RedisService;
   private config: ReturnType<typeof loadNotificationConfig>;
-  private pendingDeduplication = new Map<string, Set<string>>();
+  private pendingDeduplication = new Map<string, DeduplicationEntry>();
   private preferenceCache = new Map<string, NotificationPreference>();
 
   private constructor() {
@@ -64,9 +69,10 @@ export class NotificationService {
     dedupKey: DeduplicationKey
   ): Promise<NotificationResult> {
     const dedupId = `${dedupKey.userId}:${dedupKey.templateId}:${dedupKey.key}`;
-    const windowSet = this.pendingDeduplication.get(dedupId);
+    const windowSeconds = dedupKey.windowSeconds || this.config.redis.dedupWindowSeconds;
+    const entry = this.pendingDeduplication.get(dedupId);
 
-    if (windowSet && windowSet.size > 0) {
+    if (entry && Date.now() < entry.expiresAt && entry.externalIds.size > 0) {
       return {
         notificationId: `dedup-${Date.now()}`,
         channel: notification.channel,
@@ -78,10 +84,11 @@ export class NotificationService {
     const result = await this.send(notification);
 
     if (result.status === 'sent') {
-      if (!windowSet) {
-        this.pendingDeduplication.set(dedupId, new Set());
-      }
-      this.pendingDeduplication.get(dedupId)!.add(result.externalId!);
+      const now = Date.now();
+      this.pendingDeduplication.set(dedupId, {
+        externalIds: new Set([result.externalId!]),
+        expiresAt: now + windowSeconds * 1000,
+      });
     }
 
     return result;

packages/shared-notifications/src/services/template.service.ts

@@ -10,6 +10,7 @@ import { AllDefaultTemplates, DEFAULT_LOCALE } from '../templates/default-templa
 
 const CACHE_TTL_MS = 300000;
 const VARIABLE_PATTERN = /\{\{(\w+)\}\}/g;
+const TRUSTED_DOMAINS = ['shieldai.com', 'app.shieldai.com', 'api.shieldai.com'];
 
 export class TemplateService {
   private static instance: TemplateService;
@@ -165,19 +166,54 @@ export class TemplateService {
       varMap.set(v.name, v);
     }
 
-    return text.replace(VARIABLE_PATTERN, (match, varName) => {
+    const isHtmlContext = text.includes('<') && text.includes('>');
+
+return text.replace(VARIABLE_PATTERN, (match, varName) => {
       const value = variables[varName];
       if (value !== undefined) {
-        return String(value);
+        const stringValue = String(value);
+        if (this.isUrlVariable(varName)) {
+          return this.validateUrl(stringValue);
+        }
+        return isHtmlContext ? this.escapeHtml(stringValue) : stringValue;
       }
       const schemaVar = varMap.get(varName);
       if (schemaVar?.defaultValue !== undefined) {
-        return schemaVar.defaultValue;
+        const defaultValue = String(schemaVar.defaultValue);
+        if (this.isUrlVariable(varName)) {
+          return this.validateUrl(defaultValue);
+        }
+        return isHtmlContext ? this.escapeHtml(defaultValue) : defaultValue;
       }
       return match;
     });
   }
 
+  private escapeHtml(value: string): string {
+    return value
+      .replace(/&/g, '&amp;')
+      .replace(/</g, '&lt;')
+      .replace(/>/g, '&gt;')
+      .replace(/"/g, '&quot;')
+      .replace(/'/g, '&#039;');
+  }
+
+  private validateUrl(url: string): string {
+    try {
+      const parsed = new URL(url);
+      if (TRUSTED_DOMAINS.includes(parsed.hostname)) {
+        return url;
+      }
+      return '/';
+    } catch {
+      return '/';
+    }
+  }
+
+  private isUrlVariable(varName: string): boolean {
+    return varName.endsWith('_url');
+  }
+
   private getCached(templateId: string, locale: string): TemplateDefinition | null {
     const cacheKey = this.getCacheKey(templateId, locale);
     const entry = this.cache.get(cacheKey);