fix(security): address notification system security audit findings

- SSRF: guard sendWebhook() with checkSsrf() + createPinnedAgent() to block requests to loopback, link-local, private network, and cloud metadata endpoints - XSS: escape subject, body, and ctaHref in buildEmailHtml() via escapeHtml() to prevent HTML injection through user-controlled params (actor, preview, etc.) - Encrypt webhook URLs at rest: apply maybe_encrypt_api_key on save (settingsService for user URLs, authService for admin URL) and decrypt_api_key on read in getUserWebhookUrl() / getAdminWebhookUrl() - Log failed channel dispatches: inspect Promise.allSettled() results and log rejections via logError instead of silently dropping them - Log admin webhook failures: replace fire-and-forget .catch(() => {}) with .catch(err => logError(...)) and await the call - Migration 69: guard against missing notification_preferences table on fresh installs - Migration 70: drop the now-unused notification_preferences table - Refactor: extract applyUserChannelPrefs() helper to deduplicate setPreferences / setAdminPreferences logic - Tests: add SEC-016 (XSS, 5 cases) and SEC-017 (SSRF, 6 cases) test suites; mock ssrfGuard in notificationService tests
2026-04-05 03:36:22 +02:00
parent 6df8b2555d
commit 7b37d337c1
8 changed files with 237 additions and 46 deletions
--- a/server/src/db/migrations.ts
+++ b/server/src/db/migrations.ts
@@ -563,8 +563,11 @@ function runMigrations(db: Database.Database): void {
        CREATE INDEX IF NOT EXISTS idx_ncp_user ON notification_channel_preferences(user_id);
      `);

-      // Migrate data from old notification_preferences table
-      const oldPrefs = db.prepare('SELECT * FROM notification_preferences').all() as Array<Record<string, number>>;
+      // Migrate data from old notification_preferences table (may not exist on fresh installs)
+      const tableExists = (db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name='notification_preferences'").get() as { name: string } | undefined) != null;
+      const oldPrefs: Array<Record<string, number>> = tableExists
+        ? db.prepare('SELECT * FROM notification_preferences').all() as Array<Record<string, number>>
+        : [];
      const eventCols: Record<string, string> = {
        trip_invite: 'notify_trip_invite',
        booking_change: 'notify_booking_change',
@@ -602,6 +605,10 @@ function runMigrations(db: Database.Database): void {
          SELECT 'notification_channels', value FROM app_settings WHERE key = 'notification_channel';
      `);
    },
+    // Migration 70: Drop the old notification_preferences table (data migrated to notification_channel_preferences in migration 69)
+    () => {
+      db.exec('DROP TABLE IF EXISTS notification_preferences;');
+    },
  ];

  if (currentVersion < migrations.length) {
--- a/server/src/services/authService.ts
+++ b/server/src/services/authService.ts
@@ -716,6 +716,7 @@ export function updateAppSettings(
      }
      if (key === 'smtp_pass' && val === '••••••••') continue;
      if (key === 'smtp_pass') val = encrypt_api_key(val);
+      if (key === 'admin_webhook_url' && val) val = maybe_encrypt_api_key(val) ?? val;
      db.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)").run(key, val);
    }
  }
--- a/server/src/services/notificationPreferencesService.ts
+++ b/server/src/services/notificationPreferencesService.ts
@@ -173,6 +173,27 @@ function setAdminGlobalPref(event: NotifEventType, channel: 'email' | 'webhook',

 // ── Preferences update ─────────────────────────────────────────────────────

+// ── Shared helper for per-user channel preference upserts ─────────────────
+
+function applyUserChannelPrefs(
+  userId: number,
+  prefs: Partial<Record<string, Partial<Record<string, boolean>>>>,
+  upsert: ReturnType<typeof db.prepare>,
+  del: ReturnType<typeof db.prepare>
+): void {
+  for (const [eventType, channels] of Object.entries(prefs)) {
+    if (!channels) continue;
+    for (const [channel, enabled] of Object.entries(channels)) {
+      if (enabled) {
+        // Remove explicit row — default is enabled
+        del.run(userId, eventType, channel);
+      } else {
+        upsert.run(userId, eventType, channel, 0);
+      }
+    }
+  }
+}
+
 /**
 * Bulk-update preferences from the matrix UI.
 * Inserts disabled rows (enabled=0) and removes rows that are enabled (default).
@@ -187,20 +208,7 @@ export function setPreferences(
  const del = db.prepare(
    'DELETE FROM notification_channel_preferences WHERE user_id = ? AND event_type = ? AND channel = ?'
  );
-
-  db.transaction(() => {
-    for (const [eventType, channels] of Object.entries(prefs)) {
-      if (!channels) continue;
-      for (const [channel, enabled] of Object.entries(channels)) {
-        if (enabled) {
-          // Remove explicit row — default is enabled
-          del.run(userId, eventType, channel);
-        } else {
-          upsert.run(userId, eventType, channel, 0);
-        }
-      }
-    }
-  })();
+  db.transaction(() => applyUserChannelPrefs(userId, prefs, upsert, del))();
 }

 /**
@@ -219,24 +227,33 @@ export function setAdminPreferences(
    'DELETE FROM notification_channel_preferences WHERE user_id = ? AND event_type = ? AND channel = ?'
  );

-  db.transaction(() => {
-    for (const [eventType, channels] of Object.entries(prefs)) {
-      if (!channels) continue;
-      for (const [channel, enabled] of Object.entries(channels)) {
-        if (ADMIN_GLOBAL_CHANNELS.includes(channel as NotifChannel)) {
-          // Global setting — stored in app_settings
-          setAdminGlobalPref(eventType as NotifEventType, channel as 'email' | 'webhook', enabled);
-        } else {
-          // Per-user (inapp)
-          if (enabled) {
-            del.run(userId, eventType, channel);
-          } else {
-            upsert.run(userId, eventType, channel, 0);
-          }
-        }
+  // Split global (email/webhook) from per-user (inapp) prefs
+  const globalPrefs: Partial<Record<string, Partial<Record<string, boolean>>>> = {};
+  const userPrefs: Partial<Record<string, Partial<Record<string, boolean>>>> = {};
+
+  for (const [eventType, channels] of Object.entries(prefs)) {
+    if (!channels) continue;
+    for (const [channel, enabled] of Object.entries(channels)) {
+      if (ADMIN_GLOBAL_CHANNELS.includes(channel as NotifChannel)) {
+        if (!globalPrefs[eventType]) globalPrefs[eventType] = {};
+        globalPrefs[eventType]![channel] = enabled;
+      } else {
+        if (!userPrefs[eventType]) userPrefs[eventType] = {};
+        userPrefs[eventType]![channel] = enabled;
      }
    }
-  })();
+  }
+
+  // Apply global prefs outside the transaction (they write to app_settings)
+  for (const [eventType, channels] of Object.entries(globalPrefs)) {
+    if (!channels) continue;
+    for (const [channel, enabled] of Object.entries(channels)) {
+      setAdminGlobalPref(eventType as NotifEventType, channel as 'email' | 'webhook', enabled);
+    }
+  }
+
+  // Apply per-user (inapp) prefs in a transaction
+  db.transaction(() => applyUserChannelPrefs(userId, userPrefs, upsert, del))();
 }

 // ── SMTP availability helper (for authService) ─────────────────────────────
--- a/server/src/services/notificationService.ts
+++ b/server/src/services/notificationService.ts
@@ -1,5 +1,5 @@
 import { db } from '../db/database';
-import { logDebug } from './auditLog';
+import { logDebug, logError } from './auditLog';
 import {
  getActiveChannels,
  isEnabledForEvent,
@@ -264,7 +264,12 @@ export async function send(payload: NotificationPayload): Promise<void> {
      }
    }

-    await Promise.allSettled(promises);
+    const results = await Promise.allSettled(promises);
+    for (const result of results) {
+      if (result.status === 'rejected') {
+        logError(`notificationService.send channel dispatch failed event=${event} recipient=${recipientId}: ${result.reason}`);
+      }
+    }
  }));

  // ── Admin webhook (scope: admin) — global, respects global pref ──────
@@ -272,7 +277,9 @@ export async function send(payload: NotificationPayload): Promise<void> {
    const adminWebhookUrl = getAdminWebhookUrl();
    if (adminWebhookUrl) {
      const { title, body } = getEventText('en', event, params);
-      sendWebhook(adminWebhookUrl, { event, title, body, link: fullLink }).catch(() => {});
+      await sendWebhook(adminWebhookUrl, { event, title, body, link: fullLink }).catch((err: unknown) => {
+        logError(`notificationService.send admin webhook failed event=${event}: ${err instanceof Error ? err.message : err}`);
+      });
    }
  }
 }
--- a/server/src/services/notifications.ts
+++ b/server/src/services/notifications.ts
@@ -3,6 +3,7 @@ import fetch from 'node-fetch';
 import { db } from '../db/database';
 import { decrypt_api_key } from './apiKeyCrypto';
 import { logInfo, logDebug, logError } from './auditLog';
+import { checkSsrf, createPinnedAgent } from '../utils/ssrfGuard';

 // ── Types ──────────────────────────────────────────────────────────────────

@@ -17,6 +18,17 @@ interface SmtpConfig {
  secure: boolean;
 }

+// ── HTML escaping ──────────────────────────────────────────────────────────
+
+function escapeHtml(str: string): string {
+  return str
+    .replace(/&/g, '&amp;')
+    .replace(/</g, '&lt;')
+    .replace(/>/g, '&gt;')
+    .replace(/"/g, '&quot;')
+    .replace(/'/g, '&#39;');
+}
+
 // ── Settings helpers ───────────────────────────────────────────────────────

 function getAppSetting(key: string): string | null {
@@ -54,11 +66,13 @@ export function getUserLanguage(userId: number): string {
 }

 export function getUserWebhookUrl(userId: number): string | null {
-  return (db.prepare("SELECT value FROM settings WHERE user_id = ? AND key = 'webhook_url'").get(userId) as { value: string } | undefined)?.value || null;
+  const value = (db.prepare("SELECT value FROM settings WHERE user_id = ? AND key = 'webhook_url'").get(userId) as { value: string } | undefined)?.value || null;
+  return value ? decrypt_api_key(value) : null;
 }

 export function getAdminWebhookUrl(): string | null {
-  return getAppSetting('admin_webhook_url') || null;
+  const value = getAppSetting('admin_webhook_url') || null;
+  return value ? decrypt_api_key(value) : null;
 }

 // ── Email i18n strings ─────────────────────────────────────────────────────
@@ -226,7 +240,9 @@ export function getEventText(lang: string, event: NotifEventType, params: Record
 export function buildEmailHtml(subject: string, body: string, lang: string, navigateTarget?: string): string {
  const s = I18N[lang] || I18N.en;
  const appUrl = getAppUrl();
-  const ctaHref = navigateTarget ? `${appUrl}${navigateTarget}` : (appUrl || '');
+  const ctaHref = escapeHtml(navigateTarget ? `${appUrl}${navigateTarget}` : (appUrl || ''));
+  const safeSubject = escapeHtml(subject);
+  const safeBody = escapeHtml(body);

  return `<!DOCTYPE html>
 <html>
@@ -243,9 +259,9 @@ export function buildEmailHtml(subject: string, body: string, lang: string, navi
        </td></tr>
        <!-- Content -->
        <tr><td style="padding: 32px 32px 16px;">
-          <h1 style="margin: 0 0 8px; font-size: 18px; font-weight: 700; color: #111827; line-height: 1.3;">${subject}</h1>
+          <h1 style="margin: 0 0 8px; font-size: 18px; font-weight: 700; color: #111827; line-height: 1.3;">${safeSubject}</h1>
          <div style="width: 32px; height: 3px; background: #111827; border-radius: 2px; margin-bottom: 20px;"></div>
-          <p style="margin: 0; font-size: 14px; color: #4b5563; line-height: 1.7; white-space: pre-wrap;">${body}</p>
+          <p style="margin: 0; font-size: 14px; color: #4b5563; line-height: 1.7; white-space: pre-wrap;">${safeBody}</p>
        </td></tr>
        <!-- CTA -->
        ${appUrl ? `<tr><td style="padding: 8px 32px 32px; text-align: center;">
@@ -328,12 +344,20 @@ export function buildWebhookBody(url: string, payload: { event: string; title: s
 export async function sendWebhook(url: string, payload: { event: string; title: string; body: string; tripName?: string; link?: string }): Promise<boolean> {
  if (!url) return false;

+  const ssrf = await checkSsrf(url);
+  if (!ssrf.allowed) {
+    logError(`Webhook blocked by SSRF guard event=${payload.event} url=${url} reason=${ssrf.error}`);
+    return false;
+  }
+
  try {
+    const agent = createPinnedAgent(ssrf.resolvedIp!, new URL(url).protocol);
    const res = await fetch(url, {
      method: 'POST',
      headers: { 'Content-Type': 'application/json' },
      body: buildWebhookBody(url, payload),
      signal: AbortSignal.timeout(10000),
+      agent,
    });

    if (!res.ok) {
--- a/server/src/services/settingsService.ts
+++ b/server/src/services/settingsService.ts
@@ -1,4 +1,7 @@
 import { db } from '../db/database';
+import { maybe_encrypt_api_key } from './apiKeyCrypto';
+
+const ENCRYPTED_SETTING_KEYS = new Set(['webhook_url']);

 export function getUserSettings(userId: number): Record<string, unknown> {
  const rows = db.prepare('SELECT key, value FROM settings WHERE user_id = ?').all(userId) as { key: string; value: string }[];
@@ -13,12 +16,17 @@ export function getUserSettings(userId: number): Record<string, unknown> {
  return settings;
 }

+function serializeValue(key: string, value: unknown): string {
+  const raw = typeof value === 'object' ? JSON.stringify(value) : String(value !== undefined ? value : '');
+  if (ENCRYPTED_SETTING_KEYS.has(key)) return maybe_encrypt_api_key(raw) ?? raw;
+  return raw;
+}
+
 export function upsertSetting(userId: number, key: string, value: unknown) {
-  const serialized = typeof value === 'object' ? JSON.stringify(value) : String(value !== undefined ? value : '');
  db.prepare(`
    INSERT INTO settings (user_id, key, value) VALUES (?, ?, ?)
    ON CONFLICT(user_id, key) DO UPDATE SET value = excluded.value
-  `).run(userId, key, serialized);
+  `).run(userId, key, serializeValue(key, value));
 }

 export function bulkUpsertSettings(userId: number, settings: Record<string, unknown>) {
@@ -29,8 +37,7 @@ export function bulkUpsertSettings(userId: number, settings: Record<string, unkn
  db.exec('BEGIN');
  try {
    for (const [key, value] of Object.entries(settings)) {
-      const serialized = typeof value === 'object' ? JSON.stringify(value) : String(value !== undefined ? value : '');
-      upsert.run(userId, key, serialized);
+      upsert.run(userId, key, serializeValue(key, value));
    }
    db.exec('COMMIT');
  } catch (err) {
--- a/server/tests/unit/services/notificationService.test.ts
+++ b/server/tests/unit/services/notificationService.test.ts
@@ -49,6 +49,10 @@ vi.mock('nodemailer', () => ({

 vi.mock('node-fetch', () => ({ default: fetchMock }));
 vi.mock('../../../src/websocket', () => ({ broadcastToUser: broadcastMock }));
+vi.mock('../../../src/utils/ssrfGuard', () => ({
+  checkSsrf: vi.fn(async () => ({ allowed: true, isPrivate: false, resolvedIp: '1.2.3.4' })),
+  createPinnedAgent: vi.fn(() => ({})),
+}));

 import { createTables } from '../../../src/db/schema';
 import { runMigrations } from '../../../src/db/migrations';
--- a/server/tests/unit/services/notifications.test.ts
+++ b/server/tests/unit/services/notifications.test.ts
@@ -1,4 +1,4 @@
-import { describe, it, expect, vi, afterEach } from 'vitest';
+import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest';

 vi.mock('../../../src/db/database', () => ({
  db: { prepare: () => ({ get: vi.fn(() => undefined), all: vi.fn(() => []) }) },
@@ -18,7 +18,15 @@ vi.mock('../../../src/services/auditLog', () => ({
 vi.mock('nodemailer', () => ({ default: { createTransport: vi.fn(() => ({ sendMail: vi.fn() })) } }));
 vi.mock('node-fetch', () => ({ default: vi.fn() }));

-import { getEventText, buildEmailHtml, buildWebhookBody } from '../../../src/services/notifications';
+// ssrfGuard is mocked per-test in the SSRF describe block; default passes all
+vi.mock('../../../src/utils/ssrfGuard', () => ({
+  checkSsrf: vi.fn(async () => ({ allowed: true, isPrivate: false, resolvedIp: '1.2.3.4' })),
+  createPinnedAgent: vi.fn(() => ({})),
+}));
+
+import { getEventText, buildEmailHtml, buildWebhookBody, sendWebhook } from '../../../src/services/notifications';
+import { checkSsrf } from '../../../src/utils/ssrfGuard';
+import { logError } from '../../../src/services/auditLog';

 afterEach(() => {
  vi.unstubAllEnvs();
@@ -193,3 +201,119 @@ describe('buildEmailHtml', () => {
    expect(unknown).toContain('notifications enabled in TREK');
  });
 });
+
+// ── SEC: XSS escaping in buildEmailHtml ──────────────────────────────────────
+
+describe('buildEmailHtml XSS prevention (SEC-016)', () => {
+  it('escapes HTML special characters in subject', () => {
+    const html = buildEmailHtml('<script>alert(1)</script>', 'Body', 'en');
+    expect(html).not.toContain('<script>');
+    expect(html).toContain('&lt;script&gt;');
+  });
+
+  it('escapes HTML special characters in body', () => {
+    const html = buildEmailHtml('Subject', '<img src=x onerror=alert(1)>', 'en');
+    expect(html).toContain('&lt;img');
+    expect(html).not.toContain('<img src=x');
+  });
+
+  it('escapes double quotes in subject to prevent attribute injection', () => {
+    const html = buildEmailHtml('He said "hello"', 'Body', 'en');
+    expect(html).toContain('&quot;');
+    expect(html).not.toContain('"hello"');
+  });
+
+  it('escapes ampersands in body', () => {
+    const html = buildEmailHtml('Subject', 'a & b', 'en');
+    expect(html).toContain('&amp;');
+    expect(html).not.toMatch(/>[^<]*a & b[^<]*</);
+  });
+
+  it('escapes user-controlled actor and preview in collab_message body', () => {
+    const { body } = getEventText('en', 'collab_message', {
+      trip: 'MyTrip',
+      actor: '<evil>',
+      preview: '<script>xss()</script>',
+    });
+    const html = buildEmailHtml('Subject', body, 'en');
+    expect(html).not.toContain('<evil>');
+    expect(html).not.toContain('<script>');
+    expect(html).toContain('&lt;evil&gt;');
+    expect(html).toContain('&lt;script&gt;');
+  });
+});
+
+// ── SEC: SSRF protection in sendWebhook ──────────────────────────────────────
+
+describe('sendWebhook SSRF protection (SEC-017)', () => {
+  const payload = { event: 'test', title: 'T', body: 'B' };
+
+  beforeEach(() => {
+    vi.mocked(logError).mockClear();
+  });
+
+  it('allows a public URL and calls fetch', async () => {
+    const mockFetch = (await import('node-fetch')).default as unknown as ReturnType<typeof vi.fn>;
+    mockFetch.mockResolvedValueOnce({ ok: true, text: async () => '' } as never);
+    vi.mocked(checkSsrf).mockResolvedValueOnce({ allowed: true, isPrivate: false, resolvedIp: '1.2.3.4' });
+
+    const result = await sendWebhook('https://example.com/hook', payload);
+    expect(result).toBe(true);
+    expect(mockFetch).toHaveBeenCalled();
+  });
+
+  it('blocks loopback address and returns false', async () => {
+    vi.mocked(checkSsrf).mockResolvedValueOnce({
+      allowed: false, isPrivate: true, resolvedIp: '127.0.0.1',
+      error: 'Requests to loopback and link-local addresses are not allowed',
+    });
+
+    const result = await sendWebhook('http://localhost/secret', payload);
+    expect(result).toBe(false);
+    expect(vi.mocked(logError)).toHaveBeenCalledWith(expect.stringContaining('SSRF'));
+  });
+
+  it('blocks cloud metadata endpoint (169.254.169.254) and returns false', async () => {
+    vi.mocked(checkSsrf).mockResolvedValueOnce({
+      allowed: false, isPrivate: true, resolvedIp: '169.254.169.254',
+      error: 'Requests to loopback and link-local addresses are not allowed',
+    });
+
+    const result = await sendWebhook('http://169.254.169.254/latest/meta-data', payload);
+    expect(result).toBe(false);
+    expect(vi.mocked(logError)).toHaveBeenCalledWith(expect.stringContaining('SSRF'));
+  });
+
+  it('blocks private network addresses and returns false', async () => {
+    vi.mocked(checkSsrf).mockResolvedValueOnce({
+      allowed: false, isPrivate: true, resolvedIp: '192.168.1.1',
+      error: 'Requests to private/internal network addresses are not allowed',
+    });
+
+    const result = await sendWebhook('http://192.168.1.1/hook', payload);
+    expect(result).toBe(false);
+    expect(vi.mocked(logError)).toHaveBeenCalledWith(expect.stringContaining('SSRF'));
+  });
+
+  it('blocks non-HTTP protocols', async () => {
+    vi.mocked(checkSsrf).mockResolvedValueOnce({
+      allowed: false, isPrivate: false,
+      error: 'Only HTTP and HTTPS URLs are allowed',
+    });
+
+    const result = await sendWebhook('file:///etc/passwd', payload);
+    expect(result).toBe(false);
+  });
+
+  it('does not call fetch when SSRF check blocks the URL', async () => {
+    const mockFetch = (await import('node-fetch')).default as unknown as ReturnType<typeof vi.fn>;
+    mockFetch.mockClear();
+    vi.mocked(checkSsrf).mockResolvedValueOnce({
+      allowed: false, isPrivate: true, resolvedIp: '127.0.0.1',
+      error: 'blocked',
+    });
+
+    await sendWebhook('http://localhost/secret', payload);
+    expect(mockFetch).not.toHaveBeenCalled();
+  });
+});