From fb76074e7c0db71053eed10561a3933e646890b6 Mon Sep 17 00:00:00 2001 From: Maurice Date: Tue, 25 Aug 2026 10:45:14 +0200 Subject: [PATCH] feat(mailer): say so when SMTP certificate verification is off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SMTP_SKIP_TLS_VERIFY (and the matching admin setting) turns off certificate checking for outgoing mail. That escape hatch stays — an install behind an internal relay with a self-signed certificate depends on it — but until now it downgraded the connection without a word, and a transport is built per send, so whoever flipped it had nothing to go on afterwards. It now warns once per process, naming the host it applies to. Once, not per mail: a line on every notification is a line nobody reads. --- .../notifications/mailer/mailer.service.ts | 24 ++- server/tests/unit/nest/mailer.service.test.ts | 146 ++++++++++++++++++ 2 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 server/tests/unit/nest/mailer.service.test.ts diff --git a/server/src/nest/notifications/mailer/mailer.service.ts b/server/src/nest/notifications/mailer/mailer.service.ts index 870bdf40f..f15937cb2 100644 --- a/server/src/nest/notifications/mailer/mailer.service.ts +++ b/server/src/nest/notifications/mailer/mailer.service.ts @@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common'; import nodemailer from 'nodemailer'; import { PASSWORD_RESET_I18N } from '@trek/shared/i18n/externalNotifications'; import { readEnv } from '../../../app-config'; -import { logError, logInfo, logDebug } from '../../audit/audit-log.logger'; +import { logError, logInfo, logDebug, logWarn } from '../../audit/audit-log.logger'; import { decrypt_api_key } from '../../common/crypto/apiKeyCrypto'; import { DatabaseService } from '../../database/database.service'; import { buildEmailHtml, buildPasswordResetHtml } from './email-html'; @@ -28,6 +28,8 @@ interface SmtpConfig { */ @Injectable() export class MailerService { + private skippedTlsWarned = false; + constructor(private readonly db: DatabaseService) {} private getAppSetting(key: string): string | null { @@ -61,15 +63,35 @@ export class MailerService { */ private createTransport(config: SmtpConfig) { const skipTls = readEnv().smtp.skipTlsVerify || this.getAppSetting('smtp_skip_tls_verify') === 'true'; + if (skipTls) this.warnOnceAboutSkippedTls(config); return nodemailer.createTransport({ host: config.host, port: config.port, secure: config.secure, auth: config.user ? { user: config.user, pass: config.pass } : undefined, + // The operator's opt-out for a relay with a self-signed certificate, off by + // default and reachable only through SMTP_SKIP_TLS_VERIFY or the matching + // admin setting. It stays because self-hosted installs behind an internal + // relay depend on it; warnOnceAboutSkippedTls is what keeps it from being a + // silent downgrade. ...(skipTls ? { tls: { rejectUnauthorized: false } } : {}), }); } + /** + * Once per process, not once per mail — a transport is built on every send, and + * a line on every notification would train the operator to scroll past it. + */ + private warnOnceAboutSkippedTls(config: SmtpConfig): void { + if (this.skippedTlsWarned) return; + this.skippedTlsWarned = true; + logWarn( + `SECURITY: SMTP certificate verification is disabled for ${config.host}:${config.port}. ` + + 'Mail — including password-reset links and the SMTP credentials — travels over a channel an ' + + 'active attacker can impersonate. Keep this on only for a relay you control on a trusted network.', + ); + } + /** Is SMTP configured at the instance level? (Independent of any one user's address.) */ isSmtpConfigured(): boolean { return !!(readEnv().smtp.host || this.getAppSetting('smtp_host')); diff --git a/server/tests/unit/nest/mailer.service.test.ts b/server/tests/unit/nest/mailer.service.test.ts new file mode 100644 index 000000000..e50a489cf --- /dev/null +++ b/server/tests/unit/nest/mailer.service.test.ts @@ -0,0 +1,146 @@ +/** + * mailer.service.test.ts + * + * Covers the SMTP transport options MailerService hands to nodemailer, and in + * particular the skip-TLS opt-out: it must stay reachable for operators behind an + * internal relay, and it must announce itself instead of downgrading quietly. + * Constructed directly (no TestingModule, repo convention). + */ + +const { testDb, dbMock } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA foreign_keys = ON'); + db.exec('PRAGMA busy_timeout = 5000'); + const mock = { + db, + closeDb: () => {}, + reinitialize: () => {}, + canAccessTrip: () => undefined, + isOwner: () => false, + }; + return { testDb: db, dbMock: mock }; +}); + +const { sendMail, createTransport } = vi.hoisted(() => { + const send = vi.fn().mockResolvedValue({ messageId: 'test' }); + return { + sendMail: send, + createTransport: vi.fn((_options: Record) => ({ sendMail: send })), + }; +}); + +vi.mock('../../../src/db/database', () => dbMock); +vi.mock('nodemailer', () => ({ default: { createTransport } })); +vi.mock('../../../src/nest/audit/audit-log.logger', () => ({ + logInfo: vi.fn(), + logDebug: vi.fn(), + logError: vi.fn(), + logWarn: vi.fn(), +})); + +import { describe, it, expect, beforeAll, beforeEach, afterAll, vi } from 'vitest'; +import { createTables } from '../../../src/db/schema'; +import { runMigrations } from '../../../src/db/migrations'; +import { resetTestDb } from '../../helpers/test-db'; +import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { logWarn } from '../../../src/nest/audit/audit-log.logger'; + +function setAppSetting(key: string, value: string): void { + testDb.prepare('INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)').run(key, value); +} + +/** The minimum that makes getSmtpConfig() return a config instead of null. */ +function configureSmtp(): void { + setAppSetting('smtp_host', 'mail.internal.example'); + setAppSetting('smtp_port', '587'); + setAppSetting('smtp_from', 'trek@example.com'); +} + +function newMailer(): MailerService { + return new MailerService(new DatabaseService(testDb)); +} + +/** The options object of the most recent nodemailer.createTransport() call. */ +function lastTransportOptions(): Record { + const calls = createTransport.mock.calls; + return calls[calls.length - 1][0] as Record; +} + +beforeAll(() => { + createTables(testDb); + runMigrations(testDb); +}); + +beforeEach(() => { + resetTestDb(testDb); + vi.clearAllMocks(); +}); + +afterAll(() => { + testDb.close(); +}); + +describe('MailerService TLS options', () => { + it('MAILER-001: leaves certificate verification on by default', async () => { + configureSmtp(); + + expect(await newMailer().sendEmail('someone@example.com', 'Subject', 'Body')).toBe(true); + + expect(lastTransportOptions()).not.toHaveProperty('tls'); + expect(logWarn).not.toHaveBeenCalled(); + }); + + it('MAILER-002: the smtp_skip_tls_verify setting turns verification off', async () => { + configureSmtp(); + setAppSetting('smtp_skip_tls_verify', 'true'); + + await newMailer().sendEmail('someone@example.com', 'Subject', 'Body'); + + expect(lastTransportOptions().tls).toEqual({ rejectUnauthorized: false }); + }); + + it('MAILER-003: anything other than "true" leaves verification on', async () => { + configureSmtp(); + setAppSetting('smtp_skip_tls_verify', 'false'); + + await newMailer().sendEmail('someone@example.com', 'Subject', 'Body'); + + expect(lastTransportOptions()).not.toHaveProperty('tls'); + expect(logWarn).not.toHaveBeenCalled(); + }); + + it('MAILER-004: skipping verification is announced, and names the host it applies to', async () => { + configureSmtp(); + setAppSetting('smtp_skip_tls_verify', 'true'); + + await newMailer().sendEmail('someone@example.com', 'Subject', 'Body'); + + expect(logWarn).toHaveBeenCalledTimes(1); + const warning = vi.mocked(logWarn).mock.calls[0][0]; + expect(warning).toContain('mail.internal.example:587'); + expect(warning).toContain('SECURITY'); + }); + + it('MAILER-005: the warning is logged once per process, not once per mail', async () => { + configureSmtp(); + setAppSetting('smtp_skip_tls_verify', 'true'); + const mailer = newMailer(); + + await mailer.sendEmail('first@example.com', 'One', 'Body'); + await mailer.sendEmail('second@example.com', 'Two', 'Body'); + await mailer.sendPasswordResetEmail('third@example.com', 'https://trek.example/reset', null); + + expect(createTransport).toHaveBeenCalledTimes(3); + expect(logWarn).toHaveBeenCalledTimes(1); + }); + + it('MAILER-006: a send with no SMTP configured builds no transport at all', async () => { + expect(await newMailer().sendEmail('someone@example.com', 'Subject', 'Body')).toBe(false); + + expect(createTransport).not.toHaveBeenCalled(); + expect(sendMail).not.toHaveBeenCalled(); + }); +});