diff --git a/packages/shared-notifications/SECURITY_REMEDIATION.md b/packages/shared-notifications/SECURITY_REMEDIATION.md new file mode 100644 index 0000000..33a784a --- /dev/null +++ b/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. diff --git a/packages/shared-notifications/src/config/notification.config.ts b/packages/shared-notifications/src/config/notification.config.ts index a08e90f..93f242f 100644 --- a/packages/shared-notifications/src/config/notification.config.ts +++ b/packages/shared-notifications/src/config/notification.config.ts @@ -35,35 +35,39 @@ export const NotificationConfigSchema = z.object({ export type NotificationConfig = z.infer; -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); +}; diff --git a/packages/shared-notifications/src/services/email.service.ts b/packages/shared-notifications/src/services/email.service.ts index e0af717..97d1eb9 100644 --- a/packages/shared-notifications/src/services/email.service.ts +++ b/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(); + private sentCount = new Map(); 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 { 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 ', @@ -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()}`, diff --git a/packages/shared-notifications/src/services/notification.service.ts b/packages/shared-notifications/src/services/notification.service.ts index 69db6b9..f42df30 100644 --- a/packages/shared-notifications/src/services/notification.service.ts +++ b/packages/shared-notifications/src/services/notification.service.ts @@ -21,6 +21,11 @@ export interface RateLimitResult { resetInSeconds: number; } +interface DeduplicationEntry { + externalIds: Set; + 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; - private pendingDeduplication = new Map>(); + private pendingDeduplication = new Map(); private preferenceCache = new Map(); private constructor() { @@ -64,9 +69,10 @@ export class NotificationService { dedupKey: DeduplicationKey ): Promise { 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; diff --git a/packages/shared-notifications/src/services/template.service.ts b/packages/shared-notifications/src/services/template.service.ts index ecea907..e881c2a 100644 --- a/packages/shared-notifications/src/services/template.service.ts +++ b/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, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); + } + + 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);