From c6687b2574da981200e6799749255a0bed60193c Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 20:59:07 +0200 Subject: [PATCH] refactor(notifications): reminder crons, mailer, ntfy and webhook transports through repositories --- .../db/repositories/TodoItems.repository.ts | 96 +++++++++++++++++++ .../src/db/repositories/Trips.repository.ts | 41 ++++++++ .../src/db/repositories/Users.repository.ts | 16 ++++ .../notifications/mailer/mailer.module.ts | 13 +++ .../notifications/mailer/mailer.service.ts | 41 ++++---- .../notifications/notifications.module.ts | 26 +++-- .../notifications/reminder-jobs.service.ts | 91 +++++++++++------- .../storage-health-notifier.service.ts | 65 +++++++++---- .../notifications/transports/ntfy.service.ts | 41 +++++--- .../transports/webhook.service.ts | 22 +++-- server/tests/helpers/mcp-test-controllers.ts | 4 +- server/tests/helpers/notifications.ts | 23 +++-- ...ge-health-notifier-request-context.test.ts | 80 ++++++++++++++++ server/tests/unit/nest/admin.service.test.ts | 3 +- .../nest/admin.version-notification.test.ts | 3 +- .../tests/unit/nest/channel-override.test.ts | 5 +- server/tests/unit/nest/mailer.service.test.ts | 12 ++- .../notification-preferences.service.test.ts | 14 ++- .../unit/nest/notification-transports.test.ts | 73 ++++++++++++-- .../unit/nest/reminder-jobs.service.test.ts | 24 ++++- 20 files changed, 555 insertions(+), 138 deletions(-) create mode 100644 server/tests/integration/storage-health-notifier-request-context.test.ts diff --git a/server/src/db/repositories/TodoItems.repository.ts b/server/src/db/repositories/TodoItems.repository.ts index 291309b21..77faaf928 100644 --- a/server/src/db/repositories/TodoItems.repository.ts +++ b/server/src/db/repositories/TodoItems.repository.ts @@ -2,6 +2,7 @@ import type { TodoItems } from '../entities/TodoItems.entity'; import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; import { presenceSet } from './_shared/presence-set'; +import { columnRef, currentTimestamp, nowMinusHours } from '../dialect/sql-functions'; /** A bare `todo_items` row — every scalar column, incl. the two `persist(false)` relation mirrors (`trip_id`, `assigned_user_id`). */ export interface TodoItemRow { @@ -159,4 +160,99 @@ export class TodoItemsRepository extends TrekRepository { .executeTakeFirstOrThrow(); return Number(result.insertId); } + + // --------------------------------------------------------------------------- + // Plan 3f Task 4 (`ReminderJobsService#todoTick`) — additive. `this.qb()`, + // not Kysely: RJ5's dedup window needs `nowMinusHours` (a raw MikroORM + // `RawQueryFragment` from `sql-functions.ts`, which composes with a + // QueryBuilder/native filter but not with Kysely — the Kysely `sql` tag is + // banned under `src/db/repositories/**`), and the join needs `columnRef` + // to read `trip_id`/`assigned_user_id` (both `persist(false)` mirrors of + // the real `trip`/`assignedUser` relations — a bare select silently drops + // them, per `RoadtripDayBoundariesRepository`'s documented trap). `ti.trip` + // is ALSO the join target here, so it is never itself selected (that + // aliases off the JOINED row's own PK instead of the FK scalar — + // `PlaceRatingsRepository.listForPlaces`'s documented trap); `t.user` is + // safe to select bare (its target, `users`, is not separately joined in + // this query), aliasing to the physical `user_id` column per + // `TripsRepository.findAccessible`'s precedent. + // --------------------------------------------------------------------------- + + /** + * RJ4 (`reminder-jobs.service.ts#todoTick`) — restructured per Task 0's R9 + * ruling. Legacy: `SELECT ti.id, ti.trip_id, ti.name, ti.due_date, + * ti.assigned_user_id, t.title AS trip_title, t.user_id AS trip_owner_id + * FROM todo_items ti JOIN trips t ON t.id = ti.trip_id WHERE ti.checked = 0 + * AND ti.due_date IS NOT NULL AND ti.due_date <> '' AND date(ti.due_date) + * <= date('now', '+' || ? || ' days') AND date(ti.due_date) >= date('now') + * AND (ti.reminded_at IS NULL OR ti.reminded_at <= datetime('now', '-20 + * hours'))`. + * + * The `date('now', '+' || ? || ' days')`/`date('now')` bounds are + * JS-computed by the caller (`todayDate`/`cutoffDate`, both `Date.UTC`- + * based `YYYY-MM-DD` text — matching SQLite's own UTC `date('now')`, per + * the same reasoning RJ3's restructuring documents) and bound directly + * against `ti.due_date` (documented elsewhere as always canonical + * `YYYY-MM-DD` text, so `date(ti.due_date)`'s normalization is a no-op for + * well-formed rows and the wrapping is dropped). The `datetime('now', + * '-20 hours')` dedup bound stays genuinely in SQL, via `nowMinusHours` + * (Task 0's R9 helper) — RJ5's brief explicitly calls for it, not a JS + * equivalent. + */ + async listDueForReminder(todayDate: string, cutoffDate: string): Promise { + const platform = this.getEntityManager().getPlatform(); + const rows = await this.qb('ti') + .join('ti.trip', 't') + .select(['ti.id', columnRef(platform, 'ti.trip_id'), 'ti.name', 'ti.due_date', columnRef(platform, 'ti.assigned_user_id'), 't.title', 't.user']) + .andWhere({ checked: 0 }) + .andWhere({ due_date: { $ne: null } }) + .andWhere({ due_date: { $ne: '' } }) + .andWhere({ due_date: { $gte: todayDate } }) + .andWhere({ due_date: { $lte: cutoffDate } }) + .andWhere({ $or: [{ reminded_at: null }, { reminded_at: { $lte: nowMinusHours(platform, 20) } }] }) + .execute('all', false); + return rows.map((row) => ({ + id: row.id, + trip_id: row.trip_id, + name: row.name, + due_date: row.due_date, + assigned_user_id: row.assigned_user_id, + trip_title: row.title, + trip_owner_id: row.user_id, + })); + } + + /** + * RJ5 (`reminder-jobs.service.ts#todoTick`, looped) — `UPDATE todo_items + * SET reminded_at = CURRENT_TIMESTAMP WHERE id = ?`. Stays AFTER the + * notification send in call order (plan3f-inputs.md correction #8 — a + * documented, not-fixed-by-this-plan at-least-once ordering, unlike + * `notifications.service.ts#respond`'s deliberately claim-then-act shape). + */ + async markReminded(id: number): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { reminded_at: currentTimestamp(platform) }); + } +} + +/** {@link TodoItemsRepository.listDueForReminder}'s raw QB row — before the `trip_title`/`trip_owner_id` rename. */ +interface TodoReminderQueryRow { + id: number; + trip_id: number; + name: string; + due_date: string; + assigned_user_id: number | null; + title: string; + user_id: number; +} + +/** {@link TodoItemsRepository.listDueForReminder}'s output row. */ +export interface TodoReminderRow { + id: number; + trip_id: number; + name: string; + due_date: string; + assigned_user_id: number | null; + trip_title: string; + trip_owner_id: number; } diff --git a/server/src/db/repositories/Trips.repository.ts b/server/src/db/repositories/Trips.repository.ts index 7360cb8c3..7672dba08 100644 --- a/server/src/db/repositories/Trips.repository.ts +++ b/server/src/db/repositories/Trips.repository.ts @@ -735,6 +735,47 @@ export class TripsRepository extends TrekRepository { .executeTakeFirst(); return row?.currency; } + + // --------------------------------------------------------------------------- + // Plan 3f Task 4 (`ReminderJobsService`) — additive. + // --------------------------------------------------------------------------- + + /** + * RJ2 (`reminder-jobs.service.ts`'s boot banner, inside `runOnBoot`) — + * `SELECT COUNT(*) as c FROM trips WHERE reminder_days > 0 AND start_date + * IS NOT NULL`. + */ + async countActiveWithReminders(): Promise { + return this.count({ reminder_days: { $gt: 0 }, start_date: { $ne: null } }); + } + + /** + * RJ3 (`reminder-jobs.service.ts#tripTick`) — restructured per Task 0's R9 + * ruling: the legacy statement concatenated a per-row COLUMN + * (`t.reminder_days`) into a `date('now', '+' || t.reminder_days || ' + * days')` modifier, which no bound-parameter or JS-constant-spelled helper + * can express. This narrows the SQL read to the same WHERE minus the date + * comparison (`SELECT t.id, t.title, t.user_id, t.reminder_days, + * t.start_date FROM trips t WHERE t.reminder_days > 0 AND t.start_date IS + * NOT NULL`); the caller (`ReminderJobsService.tripTick`) does the + * per-row date-equality check in JS against `start_date`, per R9's exact + * verified shape. + */ + async listReminderCandidates(): Promise { + return await this.qb('t') + .select(['t.id', 't.title', 't.user', 't.reminder_days', 't.start_date']) + .andWhere({ reminder_days: { $gt: 0 }, start_date: { $ne: null } }) + .execute('all', false); + } +} + +/** {@link TripsRepository.listReminderCandidates}'s row — `t.user` selected bare (not joined here), so it aliases to the physical `user_id` column per `findAccessible`'s documented precedent. */ +export interface TripReminderCandidateRow { + id: number; + title: string; + user_id: number; + reminder_days: number; + start_date: string; } /** `PublicApiService.listTrips`/`getTrip`'s 8-column trip projection. */ diff --git a/server/src/db/repositories/Users.repository.ts b/server/src/db/repositories/Users.repository.ts index 5e3a1e291..c5e9fce4c 100644 --- a/server/src/db/repositories/Users.repository.ts +++ b/server/src/db/repositories/Users.repository.ts @@ -1228,6 +1228,22 @@ export class UsersRepository extends TrekRepository { const row = await this.findOne({ id }, { fields: ['username', 'email'] }); return row ? { username: row.username, email: row.email } : undefined; } + + // --------------------------------------------------------------------- + // Plan 3f Task 4 (MS2 — mailer.service.ts#getUserEmail) — additive. + // --------------------------------------------------------------------- + + /** + * MS2 — `SELECT email FROM users WHERE id = ? AND COALESCE(is_guest, 0) = 0` + * — defense-in-depth (#1362 class): a guest's synthetic email must never be + * emailed. `is_guest: 0` is exact parity for the `COALESCE(is_guest, 0) = 0` + * guard (`countNonGuest`'s docstring above: the column is `NOT NULL DEFAULT + * 0`, so no row can store `NULL` there). + */ + async getEmailNonGuest(id: number): Promise { + const row = await this.findOne({ id, is_guest: 0 }, { fields: ['email'] }); + return row?.email ?? null; + } } /** diff --git a/server/src/nest/notifications/mailer/mailer.module.ts b/server/src/nest/notifications/mailer/mailer.module.ts index 594915a81..f24fae69a 100644 --- a/server/src/nest/notifications/mailer/mailer.module.ts +++ b/server/src/nest/notifications/mailer/mailer.module.ts @@ -1,5 +1,9 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { MailerService } from './mailer.service'; +import { Users } from '../../../db/entities/Users.entity'; +import { Settings } from '../../../db/entities/Settings.entity'; +import { AppSettings } from '../../../db/entities/AppSettings.entity'; /** * Outgoing SMTP. A leaf module on purpose: AuthModule imports it for the @@ -7,8 +11,17 @@ import { MailerService } from './mailer.service'; * channel. Since NotificationsModule already imports AuthModule (the demo gate * in NotificationsMcp), the mailer living in the notifications domain would * close a hard cycle — this module is what keeps it a DAG without a forwardRef. + * + * `MikroOrmModule.forFeature([Users, Settings, AppSettings])` (Plan 3f Task + * 4) registers `UsersRepository`/`SettingsRepository`/`AppSettingsRepository` + * for `MailerService`'s `@InjectRepository`-free constructor-injected + * repositories — this module is the ONLY place `MailerService` is a + * provider (`AuthModule`/`NotificationsModule` both just import this module + * and reuse the exported singleton), so no other module needs the + * registration. */ @Module({ + imports: [MikroOrmModule.forFeature([Users, Settings, AppSettings])], providers: [MailerService], exports: [MailerService], }) diff --git a/server/src/nest/notifications/mailer/mailer.service.ts b/server/src/nest/notifications/mailer/mailer.service.ts index 4e6715b3b..f1b077c26 100644 --- a/server/src/nest/notifications/mailer/mailer.service.ts +++ b/server/src/nest/notifications/mailer/mailer.service.ts @@ -1,10 +1,16 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import nodemailer from 'nodemailer'; import { PASSWORD_RESET_I18N } from '@trek/shared/i18n/externalNotifications'; import { readEnv } from '../../../app-config'; 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 { AppSettingsRepository } from '../../../db/repositories/AppSettings.repository'; +import { SettingsRepository } from '../../../db/repositories/Settings.repository'; +import { UsersRepository } from '../../../db/repositories/Users.repository'; +import { AppSettings } from '../../../db/entities/AppSettings.entity'; +import { Settings } from '../../../db/entities/Settings.entity'; +import { Users } from '../../../db/entities/Users.entity'; import { buildEmailHtml, buildPasswordResetHtml } from './email-html'; import { describeSmtpFailure, describeSmtpGap, parseSmtpPort, type SmtpTarget } from './smtp-diagnostics'; @@ -54,21 +60,21 @@ const TEST_SOCKET_TIMEOUT_MS = 30_000; export class MailerService { private skippedTlsWarned = false; - constructor(private readonly db: DatabaseService) {} - - private async getAppSetting(key: string): Promise { - return this.db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', key)?.value || null; - } + constructor( + @InjectRepository(Users) private readonly users: UsersRepository, + @InjectRepository(Settings) private readonly settings: SettingsRepository, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + ) {} /** Env wins over the admin panel, per field. Read fresh on every send. */ private async readSmtpSettings() { const smtpEnv = readEnv().smtp; return { - host: smtpEnv.host || (await this.getAppSetting('smtp_host')), - port: smtpEnv.port || (await this.getAppSetting('smtp_port')), - user: smtpEnv.user || (await this.getAppSetting('smtp_user')), - pass: smtpEnv.pass || decrypt_api_key(await this.getAppSetting('smtp_pass')) || '', - from: smtpEnv.from || (await this.getAppSetting('smtp_from')), + host: smtpEnv.host || (await this.appSettings.getValue('smtp_host')), + port: smtpEnv.port || (await this.appSettings.getValue('smtp_port')), + user: smtpEnv.user || (await this.appSettings.getValue('smtp_user')), + pass: smtpEnv.pass || decrypt_api_key(await this.appSettings.getValue('smtp_pass')) || '', + from: smtpEnv.from || (await this.appSettings.getValue('smtp_from')), }; } @@ -96,7 +102,7 @@ export class MailerService { * one method is the only change, and it keeps the freshness property visible. */ private async createTransport(config: SmtpConfig, socketTimeoutMs: number = SOCKET_TIMEOUT_MS) { - const skipTls = readEnv().smtp.skipTlsVerify || (await this.getAppSetting('smtp_skip_tls_verify')) === 'true'; + const skipTls = readEnv().smtp.skipTlsVerify || (await this.appSettings.getValue('smtp_skip_tls_verify')) === 'true'; if (skipTls) this.warnOnceAboutSkippedTls(config); return nodemailer.createTransport({ host: config.host, @@ -131,20 +137,17 @@ export class MailerService { /** Is SMTP configured at the instance level? (Independent of any one user's address.) */ async isSmtpConfigured(): Promise { - return !!(readEnv().smtp.host || (await this.getAppSetting('smtp_host'))); + return !!(readEnv().smtp.host || (await this.appSettings.getValue('smtp_host'))); } async getUserEmail(userId: number): Promise { // Defense-in-depth (#1362): a guest's synthetic email must never be emailed. - return this.db.get<{ email: string }>( - 'SELECT email FROM users WHERE id = ? AND COALESCE(is_guest, 0) = 0', userId, - )?.email || null; + return await this.users.getEmailNonGuest(userId); } async getUserLanguage(userId: number): Promise { - return this.db.get<{ value: string }>( - "SELECT value FROM settings WHERE user_id = ? AND key = 'language'", userId, - )?.value || 'en'; + const row = await this.settings.getOne(userId, 'language'); + return row?.value || 'en'; } /** diff --git a/server/src/nest/notifications/notifications.module.ts b/server/src/nest/notifications/notifications.module.ts index ae3682486..51d744064 100644 --- a/server/src/nest/notifications/notifications.module.ts +++ b/server/src/nest/notifications/notifications.module.ts @@ -15,6 +15,9 @@ import { StorageModule } from '../storage/storage.module'; import { Notifications } from '../../db/entities/Notifications.entity'; import { NotificationChannelPreferences } from '../../db/entities/NotificationChannelPreferences.entity'; import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { Settings } from '../../db/entities/Settings.entity'; +import { Trips } from '../../db/entities/Trips.entity'; +import { TodoItems } from '../../db/entities/TodoItems.entity'; /** Notifications domain (L6 leaf module). Registered in AppModule. * AuthModule feeds NotificationsMcp's demo gate; MailerModule carries SMTP, @@ -27,15 +30,26 @@ import { AppSettings } from '../../db/entities/AppSettings.entity'; * in-container consumers (AdminController's dev test send and preferences tab, * the plugin RPC surface, HostSurfaceRpc). * MikroOrmModule.forFeature([Notifications, NotificationChannelPreferences, - * AppSettings]) (Plan 3f Task 3) registers NotificationsRepository/ + * AppSettings, Settings, Trips, TodoItems]) (Plan 3f Task 3, extended by + * Task 4) registers NotificationsRepository/ * NotificationChannelPreferencesRepository/AppSettingsRepository for * NotificationsService/NotificationPreferencesService's @InjectRepository - * constructor params — the TodoModule/PermissionsModule precedent. Only - * this module needs the registration: NotificationsService/ - * NotificationPreferencesService are providers here alone, every other - * module injects the exported singleton rather than declaring its own. */ + * constructor params, plus SettingsRepository (NtfyService/WebhookService) + * and TripsRepository/TodoItemsRepository (ReminderJobsService) — the + * TodoModule/PermissionsModule precedent. Only this module needs the + * registration: WebhookService/NtfyService/ReminderJobsService/ + * NotificationsService/NotificationPreferencesService are providers here + * alone, every other module injects the exported singleton (or, for + * MailerService, goes through MailerModule's own forFeature) rather than + * declaring its own. */ @Module({ - imports: [MikroOrmModule.forFeature([Notifications, NotificationChannelPreferences, AppSettings]), AuthModule, MailerModule, SchedulingModule, StorageModule], + imports: [ + MikroOrmModule.forFeature([Notifications, NotificationChannelPreferences, AppSettings, Settings, Trips, TodoItems]), + AuthModule, + MailerModule, + SchedulingModule, + StorageModule, + ], controllers: [NotificationsController, AdminNotificationPreferencesController], providers: [ NotificationsService, diff --git a/server/src/nest/notifications/reminder-jobs.service.ts b/server/src/nest/notifications/reminder-jobs.service.ts index c5dd3fa3c..716e1987a 100644 --- a/server/src/nest/notifications/reminder-jobs.service.ts +++ b/server/src/nest/notifications/reminder-jobs.service.ts @@ -1,6 +1,12 @@ import { Injectable, type OnApplicationBootstrap } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { logInfo, logError } from '../audit/audit-log.logger'; -import { DatabaseService } from '../database/database.service'; +import { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { TripsRepository } from '../../db/repositories/Trips.repository'; +import { TodoItemsRepository } from '../../db/repositories/TodoItems.repository'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { Trips } from '../../db/entities/Trips.entity'; +import { TodoItems } from '../../db/entities/TodoItems.entity'; import { NotificationsService } from './notifications.service'; import { CronRegistrarService } from '../scheduling/cron-registrar.service'; @@ -11,6 +17,14 @@ import { CronRegistrarService } from '../scheduling/cron-registrar.service'; * app-tz and read their enable gate from app_settings per tick, so toggling * notify_trip_reminder / notify_todo_due takes effect at the next run without * a restart. + * + * Plan 3f Task 4: converted off raw SQL onto `AppSettingsRepository` (the + * shared `getValue(key)` — RJ1, one of six identical `app_settings` reads + * across this plan, per R4/the inventory's own duplication note — never a + * bespoke local wrapper), `TripsRepository` (RJ2/RJ3) and `TodoItemsRepository` + * (RJ4/RJ5). RJ3/RJ4's column-concatenated `date()` modifiers restructure to + * plain JS date arithmetic per Task 0's verified R9 shape (`Date.UTC`-based, + * matching SQLite's own UTC `date('now')`). */ // Each todo is reminded at most once per ~24 h (tracked via @@ -21,15 +35,13 @@ const TODO_REMINDER_LEAD_DAYS = 3; @Injectable() export class ReminderJobsService implements OnApplicationBootstrap { constructor( - private readonly db: DatabaseService, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Trips) private readonly trips: TripsRepository, + @InjectRepository(TodoItems) private readonly todoItems: TodoItemsRepository, private readonly notifications: NotificationsService, private readonly registrar: CronRegistrarService, ) {} - private async getSetting(key: string): Promise { - return this.db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', key)?.value; - } - async onApplicationBootstrap(): Promise { if (!this.registrar.isEnabled()) return; @@ -39,17 +51,17 @@ export class ReminderJobsService implements OnApplicationBootstrap { // safe if this dependency graph goes repository-backed later). await this.registrar.runOnBoot('reminder-jobs-boot', async () => { try { - const reminderEnabled = (await this.getSetting('notify_trip_reminder')) !== 'false'; - const channelsRaw = (await this.getSetting('notification_channels')) || (await this.getSetting('notification_channel')) || 'none'; + const reminderEnabled = (await this.appSettings.getValue('notify_trip_reminder')) !== 'false'; + const channelsRaw = (await this.appSettings.getValue('notification_channels')) || (await this.appSettings.getValue('notification_channel')) || 'none'; const activeChannels = channelsRaw === 'none' ? [] : channelsRaw.split(',').map(c => c.trim()); if (!reminderEnabled) { logInfo('Trip reminders: disabled in settings'); } else { - const tripCount = this.db.get<{ c: number }>('SELECT COUNT(*) as c FROM trips WHERE reminder_days > 0 AND start_date IS NOT NULL')?.c ?? 0; + const tripCount = await this.trips.countActiveWithReminders(); logInfo(`Trip reminders: enabled via [${activeChannels.join(',')}]${tripCount > 0 ? `, ${tripCount} trip(s) with active reminders` : ''}`); } - if ((await this.getSetting('notify_todo_due')) !== 'false') { + if ((await this.appSettings.getValue('notify_todo_due')) !== 'false') { logInfo(`Todo due reminders: enabled (lead ${TODO_REMINDER_LEAD_DAYS}d)`); } else { logInfo('Todo due reminders: disabled in settings'); @@ -66,14 +78,22 @@ export class ReminderJobsService implements OnApplicationBootstrap { /** Daily check for trips starting exactly reminder_days from now. */ async tripTick(): Promise { try { - if ((await this.getSetting('notify_trip_reminder')) === 'false') return; + if ((await this.appSettings.getValue('notify_trip_reminder')) === 'false') return; - const trips = this.db.all<{ id: number; title: string; user_id: number; reminder_days: number }>(` - SELECT t.id, t.title, t.user_id, t.reminder_days FROM trips t - WHERE t.reminder_days > 0 - AND t.start_date IS NOT NULL - AND t.start_date = date('now', '+' || t.reminder_days || ' days') - `); + // RJ3 (Task 0's R9 ruling): the legacy statement concatenated a + // per-row column (`t.reminder_days`) into a `date('now', '+' || + // t.reminder_days || ' days')` modifier — no bound-parameter or + // JS-constant-spelled helper can express that. Read the narrower + // candidate set (reminder_days set, start_date set) and do the + // per-row date-equality check in JS instead. `Date.UTC(...)` matches + // SQLite's own `date('now')`, which is UTC, not server-local time. + const candidates = await this.trips.listReminderCandidates(); + const todayUtc = new Date(); + const trips = candidates.filter((t) => { + const target = new Date(Date.UTC(todayUtc.getUTCFullYear(), todayUtc.getUTCMonth(), todayUtc.getUTCDate())); + target.setUTCDate(target.getUTCDate() + t.reminder_days); + return t.start_date === target.toISOString().slice(0, 10); + }); for (const trip of trips) { await this.notifications.send({ event: 'trip_reminder', actorId: null, scope: 'trip', targetId: trip.id, params: { trip: trip.title, tripId: String(trip.id) } }).catch(() => {}); @@ -90,26 +110,22 @@ export class ReminderJobsService implements OnApplicationBootstrap { /** Daily check for unchecked todos due inside the lead window. */ async todoTick(): Promise { try { - if ((await this.getSetting('notify_todo_due')) === 'false') return; + if ((await this.appSettings.getValue('notify_todo_due')) === 'false') return; - // Select unchecked todos with a due date inside the lead window - // that haven't been reminded in the last 24 hours. `due_date` is - // stored as a YYYY-MM-DD text; SQLite date() handles it directly. - const todos = this.db.all<{ - id: number; trip_id: number; name: string; due_date: string; - assigned_user_id: number | null; trip_title: string; trip_owner_id: number; - }>(` - SELECT ti.id, ti.trip_id, ti.name, ti.due_date, ti.assigned_user_id, - t.title AS trip_title, t.user_id AS trip_owner_id - FROM todo_items ti - JOIN trips t ON t.id = ti.trip_id - WHERE ti.checked = 0 - AND ti.due_date IS NOT NULL - AND ti.due_date <> '' - AND date(ti.due_date) <= date('now', '+' || ? || ' days') - AND date(ti.due_date) >= date('now') - AND (ti.reminded_at IS NULL OR ti.reminded_at <= datetime('now', '-20 hours')) - `, TODO_REMINDER_LEAD_DAYS); + // RJ4 (Task 0's R9 ruling): `date('now', '+' || ? || ' days')` + // concatenates a bound parameter into the modifier — resolved in JS + // the same way RJ3 is (`Date.UTC`-based, matching SQLite's UTC + // `date('now')`). `due_date` is documented as always canonical + // `YYYY-MM-DD` text, so a direct text-range bind reproduces + // `date(ti.due_date) <= / >= ...` for well-formed rows. + const todayUtc = new Date(); + const today = new Date(Date.UTC(todayUtc.getUTCFullYear(), todayUtc.getUTCMonth(), todayUtc.getUTCDate())); + const todayDate = today.toISOString().slice(0, 10); + const cutoff = new Date(today); + cutoff.setUTCDate(cutoff.getUTCDate() + TODO_REMINDER_LEAD_DAYS); + const cutoffDate = cutoff.toISOString().slice(0, 10); + + const todos = await this.todoItems.listDueForReminder(todayDate, cutoffDate); for (const todo of todos) { const targetScope: 'user' | 'trip' = todo.assigned_user_id ? 'user' : 'trip'; @@ -126,7 +142,8 @@ export class ReminderJobsService implements OnApplicationBootstrap { due: todo.due_date, }, }).catch(() => {}); - this.db.run('UPDATE todo_items SET reminded_at = CURRENT_TIMESTAMP WHERE id = ?', todo.id); + // RJ5 stays AFTER the send, unchanged (plan3f-inputs.md correction #8). + await this.todoItems.markReminded(todo.id); } if (todos.length > 0) { diff --git a/server/src/nest/notifications/storage-health-notifier.service.ts b/server/src/nest/notifications/storage-health-notifier.service.ts index edc8435a6..1c8901269 100644 --- a/server/src/nest/notifications/storage-health-notifier.service.ts +++ b/server/src/nest/notifications/storage-health-notifier.service.ts @@ -1,4 +1,6 @@ -import { Injectable, Logger, OnApplicationBootstrap } from '@nestjs/common'; +import { Injectable, Logger, OnApplicationBootstrap, Optional } from '@nestjs/common'; +import { MikroORM } from '@mikro-orm/core'; +import { withRequestContext } from '../database/request-context'; import { StorageEventsService } from '../storage/storage-events.service'; import { NotificationsService } from './notifications.service'; import { ReplicaFailureDebouncer } from './replica-failure-debouncer'; @@ -10,6 +12,22 @@ const DEBOUNCE_WINDOW_MS = 60 * 60 * 1000; * Lives HERE (not in StorageModule) because NotificationsModule → AuthModule * → StorageModule — the reverse import would cycle. Subscribes at bootstrap; * the debouncer keeps an outage from becoming a notification storm. + * + * Plan 3f Task 4 (R3): Task 0's emitter trace found one ambiguous path — the + * "Sync now" backfill sweep (`StorageJobsService.startBackfill`) detaches + * from its originating admin request (`void driver.backfill(...)`, + * fire-and-forget), so a failure it reports could reach this listener long + * after that request's own EntityManager fork was meant to be done with. No + * emitter was found with NO context at all, but wrapping here is cheap + * insurance regardless (Task 0's own recommendation): the listener now forks + * its OWN fresh request context on every invocation — the same + * `withRequestContext` shape `CronRegistrarService.register`'s wrapped tick + * uses — independent of whatever context (if any) was active at the call + * site that triggered the failure. `orm` is `@Optional()` (matching + * `CronRegistrarService`'s own reasoning): a hand-built test double that + * constructs this service directly, with no MikroORM, still gets the exact + * pre-Task-4 behavior (send called without a context wrapper) rather than + * failing to compile or throwing. */ @Injectable() export class StorageHealthNotifierService implements OnApplicationBootstrap { @@ -19,31 +37,38 @@ export class StorageHealthNotifierService implements OnApplicationBootstrap { constructor( private readonly events: StorageEventsService, private readonly notifications: NotificationsService, + @Optional() private readonly orm?: MikroORM, ) {} onApplicationBootstrap(): void { this.events.onReplicaFailure((failure) => { const suppressed = this.debouncer.admit(failure.backend); if (suppressed === null) return; - void this.notifications - .send({ - event: 'replica_failure', - actorId: null, - scope: 'admin', - targetId: 0, - params: { - backend: failure.backend, - key: failure.key, - op: failure.op, - error: failure.error, - suppressed: String(suppressed), - }, - }) - .catch((err: unknown) => { - this.logger.error( - `replica_failure notification failed: ${err instanceof Error ? err.message : String(err)}`, - ); - }); + const sendNotification = () => + this.notifications + .send({ + event: 'replica_failure', + actorId: null, + scope: 'admin', + targetId: 0, + params: { + backend: failure.backend, + key: failure.key, + op: failure.op, + error: failure.error, + suppressed: String(suppressed), + }, + }) + .catch((err: unknown) => { + this.logger.error( + `replica_failure notification failed: ${err instanceof Error ? err.message : String(err)}`, + ); + }); + if (this.orm) { + void withRequestContext(this.orm, sendNotification); + } else { + void sendNotification(); + } }); } } diff --git a/server/src/nest/notifications/transports/ntfy.service.ts b/server/src/nest/notifications/transports/ntfy.service.ts index 0e0fe146f..cec1ff7a1 100644 --- a/server/src/nest/notifications/transports/ntfy.service.ts +++ b/server/src/nest/notifications/transports/ntfy.service.ts @@ -1,10 +1,17 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { logDebug, logError, logInfo } from '../../audit/audit-log.logger'; import { decrypt_api_key } from '../../common/crypto/apiKeyCrypto'; -import { DatabaseService } from '../../database/database.service'; +import { AppSettingsRepository } from '../../../db/repositories/AppSettings.repository'; +import { SettingsRepository } from '../../../db/repositories/Settings.repository'; +import { AppSettings } from '../../../db/entities/AppSettings.entity'; +import { Settings } from '../../../db/entities/Settings.entity'; import { safeFetchFollow, SsrfBlockedError } from '../../../utils/ssrfGuard'; import type { NotifEventType } from '../notification-events'; +/** NS2's three per-user ntfy keys, read together off `SettingsRepository.getForUser`. */ +const NTFY_USER_KEYS = new Set(['ntfy_topic', 'ntfy_server', 'ntfy_token']); + export interface NtfyConfig { server: string | null; topic: string | null; @@ -107,19 +114,24 @@ function encodeHeaderValue(value: string): string { /** ntfy push: the per-user and admin topic config, and the header-based POST. */ @Injectable() export class NtfyService { - constructor(private readonly db: DatabaseService) {} - - private async getAppSetting(key: string): Promise { - return this.db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', key)?.value || null; - } + constructor( + @InjectRepository(Settings) private readonly settings: SettingsRepository, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + ) {} + /** + * NS2. R4's fold ruling: `getForUser` (the existing, shared + * `SettingsRepository`'s method) followed by a JS filter to the three + * `ntfy_*` keys — no new batch method on the repository. `rows.length === + * 0` gates on the FILTERED (ntfy_*) row count, matching the legacy `IN + * (...)`-scoped statement's own `rows.length === 0` check exactly (a row + * present with a NULL value still counts as present — only its absence + * from the settings table returns null here). + */ async getUserNtfyConfig(userId: number): Promise { - const rows = this.db.all<{ key: string; value: string }>( - "SELECT key, value FROM settings WHERE user_id = ? AND key IN ('ntfy_topic', 'ntfy_server', 'ntfy_token')", - userId, - ); + const rows = (await this.settings.getForUser(userId)).filter((r) => NTFY_USER_KEYS.has(r.key)); if (rows.length === 0) return null; - const map: Record = {}; + const map: Record = {}; for (const r of rows) map[r.key] = r.value; return { topic: map['ntfy_topic'] || null, @@ -128,10 +140,11 @@ export class NtfyService { }; } + /** NS1 — three of the plan's six identical `app_settings` reads, R4's shared `AppSettingsRepository.getValue(key)`. */ async getAdminNtfyConfig(): Promise { - const topic = (await this.getAppSetting('admin_ntfy_topic')) || null; - const server = (await this.getAppSetting('admin_ntfy_server')) || null; - const rawToken = (await this.getAppSetting('admin_ntfy_token')) || null; + const topic = (await this.appSettings.getValue('admin_ntfy_topic')) || null; + const server = (await this.appSettings.getValue('admin_ntfy_server')) || null; + const rawToken = (await this.appSettings.getValue('admin_ntfy_token')) || null; return { topic, server, diff --git a/server/src/nest/notifications/transports/webhook.service.ts b/server/src/nest/notifications/transports/webhook.service.ts index d921bd4e0..294d27dd9 100644 --- a/server/src/nest/notifications/transports/webhook.service.ts +++ b/server/src/nest/notifications/transports/webhook.service.ts @@ -1,7 +1,11 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { logDebug, logError, logInfo } from '../../audit/audit-log.logger'; import { decrypt_api_key } from '../../common/crypto/apiKeyCrypto'; -import { DatabaseService } from '../../database/database.service'; +import { AppSettingsRepository } from '../../../db/repositories/AppSettings.repository'; +import { SettingsRepository } from '../../../db/repositories/Settings.repository'; +import { AppSettings } from '../../../db/entities/AppSettings.entity'; +import { Settings } from '../../../db/entities/Settings.entity'; import { safeFetchFollow, SsrfBlockedError } from '../../../utils/ssrfGuard'; /** @@ -44,19 +48,21 @@ export function buildWebhookBody( /** Outgoing webhooks: the per-user and the admin-global URL, and the POST itself. */ @Injectable() export class WebhookService { - constructor(private readonly db: DatabaseService) {} + constructor( + @InjectRepository(Settings) private readonly settings: SettingsRepository, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + ) {} + /** WH1 — R4's fold ruling: the shared, already-populated `SettingsRepository.getOne(userId, key)`. */ async getUserWebhookUrl(userId: number): Promise { - const value = this.db.get<{ value: string }>( - "SELECT value FROM settings WHERE user_id = ? AND key = 'webhook_url'", userId, - )?.value || null; + const row = await this.settings.getOne(userId, 'webhook_url'); + const value = row?.value || null; return value ? decrypt_api_key(value) : null; } + /** WH2 — one of the plan's six identical `app_settings` reads, R4's shared `AppSettingsRepository.getValue(key)`. */ async getAdminWebhookUrl(): Promise { - const value = this.db.get<{ value: string }>( - 'SELECT value FROM app_settings WHERE key = ?', 'admin_webhook_url', - )?.value || null; + const value = (await this.appSettings.getValue('admin_webhook_url')) || null; return value ? decrypt_api_key(value) : null; } diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 73d47a798..e286b3ab1 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -152,6 +152,7 @@ import { import { AppSettings } from '../../src/db/entities/AppSettings.entity'; import { AuditLog } from '../../src/db/entities/AuditLog.entity'; import { Users } from '../../src/db/entities/Users.entity'; +import { Settings } from '../../src/db/entities/Settings.entity'; import { InviteTokens } from '../../src/db/entities/InviteTokens.entity'; import { McpTokens } from '../../src/db/entities/McpTokens.entity'; import { OauthTokens } from '../../src/db/entities/OauthTokens.entity'; @@ -177,6 +178,7 @@ export async function createMcpTestRegistry(): Promise { const appSettings = (await createTestOrm(dbService.connection)).repo(AppSettings); const auditLogRepo = mcpOrm.repo(AuditLog); const usersRepo = mcpOrm.repo(Users); + const settingsRepo = mcpOrm.repo(Settings); const inviteTokensRepo = mcpOrm.repo(InviteTokens); const mcpTokensRepoForAuth = mcpOrm.repo(McpTokens); const oauthTokensRepo = mcpOrm.repo(OauthTokens); @@ -197,7 +199,7 @@ export async function createMcpTestRegistry(): Promise { new TripMembershipService(await createTestTripsRepo(dbService.connection), await createTestTripMembersRepo(dbService.connection)), new WebauthnConfigService(appSettings), new UserCleanupService(dbService, budgetService, await createTestUnitOfWork(dbService.connection), usersRepo, await createTestBudgetItemsRepo(dbService.connection)), - new MailerService(dbService), + new MailerService(usersRepo, settingsRepo, appSettings), new EphemeralTokenService(), new AllowedFileTypesService(appSettings), await createTestUnitOfWork(dbService.connection), appSettings, usersRepo, inviteTokensRepo, mcpTokensRepoForAuth, oauthTokensRepo, webauthnCredentialsRepoForAuth, passwordResetTokensRepo, diff --git a/server/tests/helpers/notifications.ts b/server/tests/helpers/notifications.ts index 353674333..96b4722cd 100644 --- a/server/tests/helpers/notifications.ts +++ b/server/tests/helpers/notifications.ts @@ -5,7 +5,7 @@ import { NotificationPreferencesService } from '../../src/nest/notifications/not import { NotificationsService } from '../../src/nest/notifications/notifications.service'; import { NtfyService } from '../../src/nest/notifications/transports/ntfy.service'; import { WebhookService } from '../../src/nest/notifications/transports/webhook.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from './test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo, createTestUsersRepo } from './test-uow'; import { createTestNotificationsRepo, createTestNotificationChannelPreferencesRepo } from './notifications-repos'; /** @@ -23,18 +23,27 @@ import { createTestNotificationsRepo, createTestNotificationChannelPreferencesRe * `realtime`) is unchanged, so every caller that only goes through this * function (`mcp-test-controllers.ts`, `plugin-host.ts`, both test-only) needs * no edit of its own. + * + * Plan 3f Task 4: `MailerService`/`WebhookService`/`NtfyService` no longer + * take a `DatabaseService` either — `MailerService` takes + * `UsersRepository`/`SettingsRepository`/`AppSettingsRepository`, + * `WebhookService`/`NtfyService` take `SettingsRepository`/ + * `AppSettingsRepository`, all resolved through the same memoised + * `test-uow.ts` factories bound to `dbs.connection`. */ export async function makeNotificationsService(dbs: DatabaseService, realtime = new RealtimeService()): Promise { - const mailer = new MailerService(dbs); - const uow = await createTestUnitOfWork(dbs.connection); + const usersRepo = await createTestUsersRepo(dbs.connection); + const settingsRepo = await createTestSettingsRepo(dbs.connection); const appSettings = await createTestAppSettingsRepo(dbs.connection); + const mailer = new MailerService(usersRepo, settingsRepo, appSettings); + const uow = await createTestUnitOfWork(dbs.connection); const channelPrefsRepo = await createTestNotificationChannelPreferencesRepo(dbs.connection); const notificationsRepo = await createTestNotificationsRepo(dbs.connection); return new NotificationsService( realtime, mailer, - new WebhookService(dbs), - new NtfyService(dbs), + new WebhookService(settingsRepo, appSettings), + new NtfyService(settingsRepo, appSettings), new NotificationPreferencesService(mailer, uow, appSettings, channelPrefsRepo), uow, notificationsRepo, @@ -43,9 +52,11 @@ export async function makeNotificationsService(dbs: DatabaseService, realtime = /** The preferences half on its own, over the same connection. */ export async function makeNotificationPreferencesService(dbs: DatabaseService): Promise { + const usersRepo = await createTestUsersRepo(dbs.connection); + const settingsRepo = await createTestSettingsRepo(dbs.connection); const appSettings = await createTestAppSettingsRepo(dbs.connection); const channelPrefsRepo = await createTestNotificationChannelPreferencesRepo(dbs.connection); - return new NotificationPreferencesService(new MailerService(dbs), await createTestUnitOfWork(dbs.connection), appSettings, channelPrefsRepo); + return new NotificationPreferencesService(new MailerService(usersRepo, settingsRepo, appSettings), await createTestUnitOfWork(dbs.connection), appSettings, channelPrefsRepo); } /** diff --git a/server/tests/integration/storage-health-notifier-request-context.test.ts b/server/tests/integration/storage-health-notifier-request-context.test.ts new file mode 100644 index 000000000..514d938cc --- /dev/null +++ b/server/tests/integration/storage-health-notifier-request-context.test.ts @@ -0,0 +1,80 @@ +/** + * Storage-health-notifier request-context ratchet (Plan 3f Task 4, R3). + * + * Task 0's emitter trace (`task-0-report.md`) found one ambiguous path: the + * "Sync now" backfill sweep (`StorageJobsService.startBackfill`) detaches + * from its originating admin request (`void driver.backfill(...)`, + * fire-and-forget), so a `replica_failure` event it reports can reach + * `StorageHealthNotifierService`'s listener long after the triggering + * request's own EntityManager fork was meant to be done with. No emitter was + * found with NO request context at all, but per Task 0's own recommendation + * ("cheap insurance worth taking regardless"), the listener now forks its + * OWN fresh request context on every invocation via `withRequestContext` — + * the same shape `CronRegistrarService.register`'s wrapped tick uses. + * + * This test fires the event from a BARE context — plain top-level test code, + * no HTTP request, no `withRequestContext` wrapper around the emit itself — + * driving the REAL listener registration through a real `buildApp()` boot + * (not a bare method call on a hand-built double), and asserts the listener + * never logs `cannotUseGlobalContext` / "global EntityManager" — the same + * regression class `boot-sweeps-request-context.test.ts` guards for the + * seven cron boot sweeps. + */ +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import type { INestApplication } from '@nestjs/common'; + +vi.mock('../../src/db/database', async () => { + const { createSnapshotTestDb, buildDbMock } = await import('../helpers/db-mock'); + return buildDbMock(createSnapshotTestDb()); +}); +vi.mock('../../src/config', () => ({ + JWT_SECRET: 'test-jwt-secret-for-trek-testing-only', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, + SESSION_DURATION: '24h', + SESSION_DURATION_MS: 86400000, + SESSION_DURATION_SECONDS: 86400, + DEFAULT_LANGUAGE: 'en', +})); +vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi.fn(), getOnlineUserIds: vi.fn(() => []) })); + +import { db as testDb } from '../../src/db/database'; +import { buildApp } from '../../src/bootstrap'; +import { StorageEventsService } from '../../src/nest/storage/storage-events.service'; + +describe('StorageHealthNotifierService listener runs inside its own request context, outside any active one', () => { + let app: INestApplication; + + beforeAll(async () => { + app = await buildApp(); + }); + + afterAll(async () => { + await app.close(); + testDb.close(); + }); + + it('STORAGE-HEALTH-CTX-001: emitting replica_failure from a bare (non-request) context never logs cannotUseGlobalContext', async () => { + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + // The listener is already registered for real, from buildApp()'s own + // Nest lifecycle (onApplicationBootstrap runs during app.init()) — no + // hand-called registration here, per rule 12/13 ("drive the real + // listener registration, not a bare method call"). + const events = app.get(StorageEventsService); + events.emitReplicaFailure({ backend: 'ctx-ratchet', key: 'k', op: 'put', error: 'timeout', at: Date.now() }); + + // Let the fire-and-forget async dispatch (withRequestContext(...).then(...)) + // settle before asserting. + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); + + const suspicious = errSpy.mock.calls + .map((args) => args.map(String).join(' ')) + .filter((line) => /cannotUseGlobalContext|global EntityManager/i.test(line)); + expect(suspicious).toEqual([]); + } finally { + errSpy.mockRestore(); + } + }); +}); diff --git a/server/tests/unit/nest/admin.service.test.ts b/server/tests/unit/nest/admin.service.test.ts index 421701289..732a0f663 100644 --- a/server/tests/unit/nest/admin.service.test.ts +++ b/server/tests/unit/nest/admin.service.test.ts @@ -94,6 +94,7 @@ import { createTestPasswordResetTokensRepo, createTestTripsRepo, createTestTripMembersRepo, + createTestSettingsRepo, } from '../../helpers/test-uow'; import { budgetRepoArgs } from '../../helpers/budget-repos'; import { createTestBudgetItemsRepo } from '../../helpers/files-repos'; @@ -117,7 +118,7 @@ beforeAll(async () => { permissions = new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)); userCleanup = new UserCleanupService(dbs, new BudgetService(dbs, permissions, new ExchangeRatesService(), realtime, await createTestUnitOfWork(dbs.connection), ...(await budgetRepoArgs(dbs.connection))), await createTestUnitOfWork(dbs.connection), await createTestUsersRepo(dbs.connection), await createTestBudgetItemsRepo(dbs.connection)); auth = new AuthService( - permissions, new TripMembershipService(await createTestTripsRepo(dbs.connection), await createTestTripMembersRepo(dbs.connection)), webauthn, userCleanup, new MailerService(dbs), new EphemeralTokenService(), new AllowedFileTypesService(await createTestAppSettingsRepo(dbs.connection)), await createTestUnitOfWork(dbs.connection), + permissions, new TripMembershipService(await createTestTripsRepo(dbs.connection), await createTestTripMembersRepo(dbs.connection)), webauthn, userCleanup, new MailerService(await createTestUsersRepo(dbs.connection), await createTestSettingsRepo(dbs.connection), await createTestAppSettingsRepo(dbs.connection)), new EphemeralTokenService(), new AllowedFileTypesService(await createTestAppSettingsRepo(dbs.connection)), await createTestUnitOfWork(dbs.connection), await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection), await createTestInviteTokensRepo(dbs.connection), await createTestMcpTokensRepo(dbs.connection), await createTestOauthTokensRepo(dbs.connection), await createTestWebauthnCredentialsRepo(dbs.connection), await createTestPasswordResetTokensRepo(dbs.connection), ); diff --git a/server/tests/unit/nest/admin.version-notification.test.ts b/server/tests/unit/nest/admin.version-notification.test.ts index 000a89c5b..a4be9aac2 100644 --- a/server/tests/unit/nest/admin.version-notification.test.ts +++ b/server/tests/unit/nest/admin.version-notification.test.ts @@ -72,6 +72,7 @@ import { createTestPasswordResetTokensRepo, createTestTripsRepo, createTestTripMembersRepo, + createTestSettingsRepo, } from '../../helpers/test-uow'; import { budgetRepoArgs } from '../../helpers/budget-repos'; import { createTestBudgetItemsRepo } from '../../helpers/files-repos'; @@ -95,7 +96,7 @@ beforeAll(async () => { permissions = new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)); userCleanup = new UserCleanupService(dbs, new BudgetService(dbs, permissions, new ExchangeRatesService(), realtime, await createTestUnitOfWork(dbs.connection), ...(await budgetRepoArgs(dbs.connection))), await createTestUnitOfWork(dbs.connection), await createTestUsersRepo(dbs.connection), await createTestBudgetItemsRepo(dbs.connection)); auth = new AuthService( - permissions, new TripMembershipService(await createTestTripsRepo(dbs.connection), await createTestTripMembersRepo(dbs.connection)), webauthn, userCleanup, new MailerService(dbs), new EphemeralTokenService(), new AllowedFileTypesService(await createTestAppSettingsRepo(dbs.connection)), await createTestUnitOfWork(dbs.connection), + permissions, new TripMembershipService(await createTestTripsRepo(dbs.connection), await createTestTripMembersRepo(dbs.connection)), webauthn, userCleanup, new MailerService(await createTestUsersRepo(dbs.connection), await createTestSettingsRepo(dbs.connection), await createTestAppSettingsRepo(dbs.connection)), new EphemeralTokenService(), new AllowedFileTypesService(await createTestAppSettingsRepo(dbs.connection)), await createTestUnitOfWork(dbs.connection), await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection), await createTestInviteTokensRepo(dbs.connection), await createTestMcpTokensRepo(dbs.connection), await createTestOauthTokensRepo(dbs.connection), await createTestWebauthnCredentialsRepo(dbs.connection), await createTestPasswordResetTokensRepo(dbs.connection), ); diff --git a/server/tests/unit/nest/channel-override.test.ts b/server/tests/unit/nest/channel-override.test.ts index 712a86e55..7e0bd6981 100644 --- a/server/tests/unit/nest/channel-override.test.ts +++ b/server/tests/unit/nest/channel-override.test.ts @@ -68,7 +68,7 @@ import { DatabaseService } from '../../../src/nest/database/database.service'; import { createPluginRuntime } from '../../helpers/plugin-host'; import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; import { NotificationPreferencesService } from '../../../src/nest/notifications/notification-preferences.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; import { createTestNotificationChannelPreferencesRepo } from '../../helpers/notifications-repos'; import { @@ -80,7 +80,6 @@ import { // The registry consumes ExternalChannel but does not re-export it; it is declared here. import type { ExternalChannel } from '../../../src/nest/notifications/notification-events'; -const prefsDbs = new DatabaseService(testDb); let prefsSvc: NotificationPreferencesService; // An arrow forwarder rather than `.bind(prefsSvc)`: under `strictBindCallApply: false` // a bound alias is typed `any`, which hides a missing `await` from tsc and the lint @@ -93,7 +92,7 @@ beforeAll(async () => { createTables(testDb); runMigrations(testDb); prefsSvc = new NotificationPreferencesService( - new MailerService(prefsDbs), + new MailerService(await createTestUsersRepo(testDb), await createTestSettingsRepo(testDb), await createTestAppSettingsRepo(testDb)), await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestNotificationChannelPreferencesRepo(testDb), diff --git a/server/tests/unit/nest/mailer.service.test.ts b/server/tests/unit/nest/mailer.service.test.ts index 55d0707bb..9227178de 100644 --- a/server/tests/unit/nest/mailer.service.test.ts +++ b/server/tests/unit/nest/mailer.service.test.ts @@ -48,8 +48,11 @@ 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 { logError, logInfo, logWarn } from '../../../src/nest/audit/audit-log.logger'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; +import { Users } from '../../../src/db/entities/Users.entity'; +import { Settings } from '../../../src/db/entities/Settings.entity'; +import { AppSettings } from '../../../src/db/entities/AppSettings.entity'; function setAppSetting(key: string, value: string): void { testDb.prepare('INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)').run(key, value); @@ -62,8 +65,10 @@ function configureSmtp(): void { setAppSetting('smtp_from', 'trek@example.com'); } +let testOrm: TestOrm; + function newMailer(): MailerService { - return new MailerService(new DatabaseService(testDb)); + return new MailerService(testOrm.repo(Users), testOrm.repo(Settings), testOrm.repo(AppSettings)); } /** The options object of the most recent nodemailer.createTransport() call. */ @@ -72,9 +77,10 @@ function lastTransportOptions(): Record { return calls[calls.length - 1][0] as Record; } -beforeAll(() => { +beforeAll(async () => { createTables(testDb); runMigrations(testDb); + testOrm = await createTestOrm(testDb); }); beforeEach(() => { diff --git a/server/tests/unit/nest/notification-preferences.service.test.ts b/server/tests/unit/nest/notification-preferences.service.test.ts index b0c9895c8..1473de459 100644 --- a/server/tests/unit/nest/notification-preferences.service.test.ts +++ b/server/tests/unit/nest/notification-preferences.service.test.ts @@ -36,19 +36,18 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createAdmin, setAppSetting, setNotificationChannels, disableNotificationPref } from '../../helpers/factories'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; import { NotificationPreferencesService } from '../../../src/nest/notifications/notification-preferences.service'; import { registerBuiltinChannels } from '../../../src/nest/notifications/channels/builtins'; import { NtfyService } from '../../../src/nest/notifications/transports/ntfy.service'; import { WebhookService } from '../../../src/nest/notifications/transports/webhook.service'; import { __resetChannelsForTest } from '../../../src/nest/notifications/channel-registry'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; import { createTestNotificationChannelPreferencesRepo } from '../../helpers/notifications-repos'; -const dbs = new DatabaseService(testDb); -const mailer = new MailerService(dbs); -registerBuiltinChannels({ mailer, webhook: new WebhookService(dbs), ntfy: new NtfyService(dbs) }); +// Built in beforeAll: MailerService/WebhookService/NtfyService now take +// repositories, resolved async through test-uow.ts's memoised factories. +let mailer: MailerService; // Constructed in beforeAll: the service now takes a UnitOfWork, which is async to build. let svc: NotificationPreferencesService; @@ -70,6 +69,11 @@ const isWebhookConfigured = (...a: Parameters) => sv beforeAll(async () => { createTables(testDb); runMigrations(testDb); + const usersRepo = await createTestUsersRepo(testDb); + const settingsRepo = await createTestSettingsRepo(testDb); + const appSettingsRepoForChannels = await createTestAppSettingsRepo(testDb); + mailer = new MailerService(usersRepo, settingsRepo, appSettingsRepoForChannels); + registerBuiltinChannels({ mailer, webhook: new WebhookService(settingsRepo, appSettingsRepoForChannels), ntfy: new NtfyService(settingsRepo, appSettingsRepoForChannels) }); svc = new NotificationPreferencesService( mailer, await createTestUnitOfWork(testDb), diff --git a/server/tests/unit/nest/notification-transports.test.ts b/server/tests/unit/nest/notification-transports.test.ts index 6a48ac339..606382b99 100644 --- a/server/tests/unit/nest/notification-transports.test.ts +++ b/server/tests/unit/nest/notification-transports.test.ts @@ -1,4 +1,5 @@ -import { describe, it, expect, vi, afterEach, afterAll, beforeEach } from 'vitest'; +import { describe, it, expect, vi, afterEach, afterAll, beforeAll, beforeEach } from 'vitest'; +import Database from 'better-sqlite3'; vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ get: vi.fn(() => undefined), all: vi.fn(() => []) }) }, @@ -45,20 +46,38 @@ vi.mock('../../../src/utils/ssrfGuard', () => { }; }); -import { DatabaseService } from '../../../src/nest/database/database.service'; import { getEventText, buildEmailHtml } from '../../../src/nest/notifications/mailer/email-html'; import { WebhookService, buildWebhookBody } from '../../../src/nest/notifications/transports/webhook.service'; import { NtfyService, resolveNtfyUrl, resolveAdminNtfyUrl, resolveNtfyToken, type NtfyConfig } from '../../../src/nest/notifications/transports/ntfy.service'; - -// The transports are providers now; the db stub below is the same one the -// module-level `db` mock used to supply, handed in instead of imported. -const stubDb = { prepare: () => ({ get: () => undefined, all: () => [], run: () => undefined }) } as unknown as ConstructorParameters[0]; -const webhookSvc = new WebhookService(new DatabaseService(stubDb)); -const ntfySvc = new NtfyService(new DatabaseService(stubDb)); -const sendWebhook = webhookSvc.sendWebhook.bind(webhookSvc); -const sendNtfy = ntfySvc.sendNtfy.bind(ntfySvc); import { checkSsrf } from '../../../src/utils/ssrfGuard'; import { logError } from '../../../src/nest/audit/audit-log.logger'; +import { createTables } from '../../../src/db/schema'; +import { runMigrations } from '../../../src/db/migrations'; +import { createTestSettingsRepo, createTestAppSettingsRepo } from '../../helpers/test-uow'; + +// The transports are providers now, taking SettingsRepository/ +// AppSettingsRepository instead of DatabaseService — a real, throwaway +// in-memory DB (never read by any case in this file: every case here drives +// sendWebhook/sendNtfy directly, never the config getters, except the +// GHSA-7pqc-fj3c-9346 case below, which resolves real configs through the +// same repositories) replaces the STUB-DB fixture the fake `DatabaseService` +// connection used to supply. +let webhookSvc: WebhookService; +let ntfySvc: NtfyService; +let sendWebhook: WebhookService['sendWebhook']; +let sendNtfy: NtfyService['sendNtfy']; + +beforeAll(async () => { + const transportsDb = new Database(':memory:'); + createTables(transportsDb); + runMigrations(transportsDb); + const settingsRepo = await createTestSettingsRepo(transportsDb); + const appSettingsRepo = await createTestAppSettingsRepo(transportsDb); + webhookSvc = new WebhookService(settingsRepo, appSettingsRepo); + ntfySvc = new NtfyService(settingsRepo, appSettingsRepo); + sendWebhook = webhookSvc.sendWebhook.bind(webhookSvc); + sendNtfy = ntfySvc.sendNtfy.bind(ntfySvc); +}); afterEach(() => { vi.unstubAllEnvs(); @@ -397,6 +416,40 @@ describe('resolveNtfyToken', () => { }); }); +// R2 (Task 0's report, verbatim spec): drives the REAL end-to-end path — +// resolve the URL and token exactly the way production does, then call the +// real sendNtfy — rather than unit-testing resolveNtfyToken in isolation +// (the block above). Mutation-proved: temporarily swap isOperatorNtfyServer's +// target-based URL comparison for a role-based check (`return true` / +// `ctx.user.role === 'admin'`-shaped) and this test's second assertion goes +// red (the admin token would be attached to the non-operator send too). +describe('GHSA-7pqc-fj3c-9346: ntfy token is only attached when the target is the operator\'s own server', () => { + const adminCfg: NtfyConfig = { server: 'https://ntfy.operator.example', topic: 'ops', token: 'operator-secret-token' }; + // The "user token" is deliberately null in both fixtures, to isolate the + // admin-token-leak path specifically — if the user had their own token, + // resolveNtfyToken would return it first and the admin-scope branch would + // never be reached, per its own precedence. + const operatorTargetUser: NtfyConfig = { server: null, topic: 'user-topic', token: null }; // rides the operator's own server + const nonOperatorTargetUser: NtfyConfig = { server: 'https://ntfy.attacker.example', topic: 'user-topic', token: null }; // a different, user-chosen server + const payload = { event: 'trip_reminder', title: 'Trip reminder', body: 'Your trip starts soon' }; + + beforeEach(() => { + (globalThis.fetch as ReturnType).mockClear(); + vi.mocked(checkSsrf).mockResolvedValue({ allowed: true, isPrivate: false, resolvedIp: '1.2.3.4' }); + }); + + it('attaches the admin token on an operator-server send, and withholds it on a non-operator-server send', async () => { + const mockFetch = globalThis.fetch as unknown as ReturnType; + mockFetch.mockResolvedValue({ ok: true, text: async () => '' } as never); + + await sendNtfy(resolveNtfyUrl(adminCfg, operatorTargetUser)!, resolveNtfyToken(adminCfg, operatorTargetUser), payload); + expect(mockFetch.mock.calls[0][1].headers['Authorization']).toBe('Bearer operator-secret-token'); + + await sendNtfy(resolveNtfyUrl(adminCfg, nonOperatorTargetUser)!, resolveNtfyToken(adminCfg, nonOperatorTargetUser), payload); + expect(mockFetch.mock.calls[1][1].headers['Authorization']).toBeUndefined(); + }); +}); + describe('resolveNtfyUrl', () => { const adminCfg: NtfyConfig = { server: 'https://ntfy.sh', topic: 'admin-topic', token: null }; diff --git a/server/tests/unit/nest/reminder-jobs.service.test.ts b/server/tests/unit/nest/reminder-jobs.service.test.ts index 6f9616167..95879951a 100644 --- a/server/tests/unit/nest/reminder-jobs.service.test.ts +++ b/server/tests/unit/nest/reminder-jobs.service.test.ts @@ -33,11 +33,15 @@ vi.mock('../../../src/config', () => ({ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { createUser, createTrip, createTodoItem, setAppSetting, setNotificationChannels } from '../../helpers/factories'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import { ReminderJobsService } from '../../../src/nest/notifications/reminder-jobs.service'; import { notificationsStub } from '../../helpers/notifications'; import type { NotificationsService } from '../../../src/nest/notifications/notifications.service'; import type { CronRegistrarService } from '../../../src/nest/scheduling/cron-registrar.service'; +import { createTestAppSettingsRepo, createTestTripsRepo } from '../../helpers/test-uow'; +import { createTestTodoItemsRepo } from '../../helpers/todo-repos'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { TripsRepository } from '../../../src/db/repositories/Trips.repository'; +import type { TodoItemsRepository } from '../../../src/db/repositories/TodoItems.repository'; interface Registered { name: string; @@ -45,6 +49,10 @@ interface Registered { onTick: () => Promise | void; } +let appSettingsRepo: AppSettingsRepository; +let tripsRepo: TripsRepository; +let todoItemsRepo: TodoItemsRepository; + function makeJobs(overrides: { notifications?: NotificationsService } = {}) { const registered: Registered[] = []; const registrar = { @@ -62,7 +70,9 @@ function makeJobs(overrides: { notifications?: NotificationsService } = {}) { }; const send = vi.fn().mockResolvedValue(undefined); const svc = new ReminderJobsService( - new DatabaseService(testDb), + appSettingsRepo, + tripsRepo, + todoItemsRepo, overrides.notifications ?? notificationsStub(send), registrar as unknown as CronRegistrarService, ); @@ -76,9 +86,12 @@ function tripWithReminder(userId: number, days: number, title = 'Lisbon'): numbe return trip.id; } -beforeAll(() => { +beforeAll(async () => { createTables(testDb); runMigrations(testDb); + appSettingsRepo = await createTestAppSettingsRepo(testDb); + tripsRepo = await createTestTripsRepo(testDb); + todoItemsRepo = await createTestTodoItemsRepo(testDb); }); beforeEach(() => { @@ -172,8 +185,11 @@ describe('trip reminder tick', () => { it('RJOB-007 — a failing tick is contained to the check-failed log line', async () => { const broken = { send: vi.fn() } as unknown as NotificationsService; + const throwGone = () => { throw new Error('db gone'); }; const svc = new ReminderJobsService( - { get: () => { throw new Error('db gone'); }, all: () => { throw new Error('db gone'); }, run: () => { throw new Error('db gone'); } } as unknown as DatabaseService, + { getValue: throwGone } as unknown as AppSettingsRepository, + { listReminderCandidates: throwGone, countActiveWithReminders: throwGone } as unknown as TripsRepository, + { listDueForReminder: throwGone, markReminded: throwGone } as unknown as TodoItemsRepository, broken, { isEnabled: () => true, register: () => true, unregister: () => {} } as unknown as CronRegistrarService, );