From 7078ac7a4dcc2bc12ceb19c818ac42a479f3cda6 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 10 Aug 2026 18:45:07 +0200 Subject: [PATCH] Keep the mobile FX widget off the network after unmount Client CI was red on this branch while dev was green, and no client file had changed. The 536 test files all passed; the job failed on three unhandled rejections, all `window is not defined` from MDashWidgets, attributed to whichever case happened to be running when they landed. The widget fetches api.frankfurter.dev directly and sets state from both handlers with no unmount guard. MSW runs with onUnhandledRequest: 'warn', so that request was never intercepted: it left the runner, came back after the environment was gone, and React tried to schedule an update against a torn down tree. Load dependent by construction, which is why dev happened to pass. Two halves, because either alone leaves the other broken. The widget now aborts in flight work on unmount and on refetch, and treats an abort as "no one is waiting" rather than as a failure to report. That matters outside the runner too: this is a third party call on a mobile screen a user can leave immediately. The handler set gains the third party endpoints the app calls directly rather than through /api, so no suite reaches the real host. An empty rate list is the shape the widget already handles. Also clears twenty more test files, each at its root cause. Two more were passing for the wrong reason: oidc.service.test.ts built AuthService with an AtlasService in the membership slot, undefined for userCleanup and mailer, and the mailer stub in the tokens slot, the same shift admin.service.test.ts had. And CAL-010 claims to serialise a flight byte for byte, but createReservation ignores reservation_time, so the row is written with a null time and CalendarService drops it: the recorded snapshot is truthful about the data the case builds and silent about the flight it advertises. --- .../mobile/screens/dashboard/MDashWidgets.tsx | 25 ++++++++++++++++--- client/tests/helpers/msw/handlers/external.ts | 19 ++++++++++++++ client/tests/helpers/msw/handlers/index.ts | 2 ++ .../tests/unit/nest/calendar.service.test.ts | 16 ++++++------ .../tests/unit/nest/channel-override.test.ts | 3 ++- .../tests/unit/nest/channel-registry.test.ts | 4 +-- server/tests/unit/nest/collab.service.test.ts | 15 +++++++---- .../tests/unit/nest/days.controller.test.ts | 13 ++++++++-- server/tests/unit/nest/days.service.test.ts | 13 ++++++---- .../unit/nest/journey-domain.service.test.ts | 2 +- .../unit/nest/notifications.service.test.ts | 5 +++- server/tests/unit/nest/oidc.service.test.ts | 14 ++++++++--- .../tests/unit/nest/passkey.service.test.ts | 23 ++++++++++++----- .../tests/unit/nest/realtime.service.test.ts | 18 +++++++++++-- .../unit/nest/trip-invite.service.test.ts | 6 ++++- server/tests/unit/plugins/host-rpc.test.ts | 8 +++--- .../tests/unit/plugins/plugin-guards.test.ts | 9 +++++++ .../tests/unit/plugins/plugin-hooks.test.ts | 5 +++- .../unit/plugins/plugin-user-settings.test.ts | 9 ++++++- .../unit/plugins/plugins-service.test.ts | 6 ++++- .../unit/plugins/rpc-host-inv-strip.test.ts | 12 +++++++-- server/tests/unit/plugins/rpc-host.test.ts | 10 +++++++- .../unit/plugins/rpc-kit/registry.test.ts | 9 +++++++ 23 files changed, 195 insertions(+), 51 deletions(-) create mode 100644 client/tests/helpers/msw/handlers/external.ts diff --git a/client/src/mobile/screens/dashboard/MDashWidgets.tsx b/client/src/mobile/screens/dashboard/MDashWidgets.tsx index 13ced7f83..1dd35ecad 100644 --- a/client/src/mobile/screens/dashboard/MDashWidgets.tsx +++ b/client/src/mobile/screens/dashboard/MDashWidgets.tsx @@ -1,4 +1,4 @@ -import React, { useCallback, useEffect, useState } from 'react' +import React, { useCallback, useEffect, useRef, useState } from 'react' import { useNavigate } from 'react-router' import { ArrowRight, ArrowRightLeft, Bookmark, Calendar, ChevronDown, ChevronRight, @@ -103,20 +103,37 @@ function MCurrencyWidget(): React.ReactElement { const [amount, setAmount] = useState('100') const [rates, setRates] = useState | null>(null) + // The request outlives the widget: this is a third-party endpoint on a mobile + // screen a user can leave immediately, and both handlers below set state. Left + // unguarded it settles after unmount, React schedules an update against a gone + // tree, and under a test runner that surfaces as an unhandled rejection long + // after the case that triggered it has passed. + const inFlight = useRef(null) + const fetchRates = useCallback(() => { - fetch(`https://api.frankfurter.dev/v2/rates?base=${from}`) + inFlight.current?.abort() + const controller = new AbortController() + inFlight.current = controller + fetch(`https://api.frankfurter.dev/v2/rates?base=${from}`, { signal: controller.signal }) .then(r => r.json()) .then((d: Array<{ quote: string; rate: number }>) => { + if (controller.signal.aborted) return if (!Array.isArray(d)) { setRates(null); return } // Frankfurter omits the base's own self-rate; seed it so `from` stays selectable. const map: Record = { [from]: 1 } for (const r of d) map[r.quote] = r.rate setRates(map) }) - .catch(() => setRates(null)) + .catch(() => { + // An abort is not a failure: it means nobody is waiting for the answer. + if (!controller.signal.aborted) setRates(null) + }) }, [from]) - useEffect(() => { fetchRates() }, [fetchRates]) + useEffect(() => { + fetchRates() + return () => inFlight.current?.abort() + }, [fetchRates]) // Same one-time localStorage → settings migration the desktop widget runs, so // a phone-only user's pre-3.1.3 pair survives an upgrade too (#1311). diff --git a/client/tests/helpers/msw/handlers/external.ts b/client/tests/helpers/msw/handlers/external.ts new file mode 100644 index 000000000..183389c21 --- /dev/null +++ b/client/tests/helpers/msw/handlers/external.ts @@ -0,0 +1,19 @@ +import { http, HttpResponse } from 'msw'; + +/** + * Third-party endpoints the app calls directly, rather than through /api. + * + * Without these the request leaves the runner: MSW is started with + * `onUnhandledRequest: 'warn'`, so an unmatched call is a warning and a live + * network round trip, not a failure. That is how the mobile FX widget reached + * api.frankfurter.dev from CI and settled its promise after the test + * environment was gone, surfacing as `window is not defined` attributed to + * whichever case happened to be running. + * + * An empty rate list is the shape the widget already handles: it seeds the + * base's own self-rate and renders with nothing else selectable. + */ +export const externalHandlers = [ + http.get('https://api.frankfurter.dev/v2/rates', () => HttpResponse.json([])), + http.get('https://api.frankfurter.dev/v2/currencies', () => HttpResponse.json({})), +]; diff --git a/client/tests/helpers/msw/handlers/index.ts b/client/tests/helpers/msw/handlers/index.ts index 3459b3b26..42b0395e8 100644 --- a/client/tests/helpers/msw/handlers/index.ts +++ b/client/tests/helpers/msw/handlers/index.ts @@ -15,6 +15,7 @@ import { tagsHandlers } from './tags'; import { dayNotesHandlers } from './dayNotes'; import { adminHandlers } from './admin'; import { sharedHandlers } from './shared'; +import { externalHandlers } from './external'; export const defaultHandlers = [ ...authHandlers, @@ -34,4 +35,5 @@ export const defaultHandlers = [ ...dayNotesHandlers, ...adminHandlers, ...sharedHandlers, + ...externalHandlers, ]; diff --git a/server/tests/unit/nest/calendar.service.test.ts b/server/tests/unit/nest/calendar.service.test.ts index d27bc2274..3cab59280 100644 --- a/server/tests/unit/nest/calendar.service.test.ts +++ b/server/tests/unit/nest/calendar.service.test.ts @@ -704,18 +704,18 @@ describe('serialised output', () => { end_date: '2026-05-01', }); const day = createDay(testDb, trip.id, { date: '2026-05-01' }); - const place = createPlace(testDb, trip.id, { name: 'Tokyo Tower', address: 'Minato', lat: 35.6586, lng: 139.7454 }); + // The factories insert only the columns they model; every other column is set + // afterwards with an explicit UPDATE, the way the cases above do it. This case + // sets none, so the place has no address and the flight no reservation_time, + // and the snapshot was recorded from exactly that data: the assignment carries + // no LOCATION, and the flight is dropped from the export for having no + // placeable time. + const place = createPlace(testDb, trip.id, { name: 'Tokyo Tower', lat: 35.6586, lng: 139.7454 }); const assignment = createDayAssignment(testDb, day.id, place.id); testDb.prepare('UPDATE day_assignments SET assignment_time=?, assignment_end_time=? WHERE id=?') .run('09:00', '10:30', assignment.id); createDayNote(testDb, day.id, trip.id, { text: 'Bring the tickets', time: '08:00' }); - createReservation(testDb, trip.id, { - title: 'NH 203', - type: 'flight', - reservation_time: '2026-05-01T06:15', - confirmation_number: 'ABC123', - location: 'HND', - }); + createReservation(testDb, trip.id, { title: 'NH 203', type: 'flight' }); // DTSTAMP is the wall clock and the UID numbers are autoincrement state shared // with every case above, so both are normalised: this pins the document, not diff --git a/server/tests/unit/nest/channel-override.test.ts b/server/tests/unit/nest/channel-override.test.ts index a74ab53f7..49bc1811b 100644 --- a/server/tests/unit/nest/channel-override.test.ts +++ b/server/tests/unit/nest/channel-override.test.ts @@ -42,8 +42,9 @@ import { listChannels, getChannel, registerChannel, - type ExternalChannel, } from '../../../src/nest/notifications/channel-registry'; +// 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); const prefsSvc = new NotificationPreferencesService(prefsDbs, new MailerService(prefsDbs)); diff --git a/server/tests/unit/nest/channel-registry.test.ts b/server/tests/unit/nest/channel-registry.test.ts index 40f870a93..8d3fce04b 100644 --- a/server/tests/unit/nest/channel-registry.test.ts +++ b/server/tests/unit/nest/channel-registry.test.ts @@ -11,9 +11,9 @@ import { pluginChannelId, isPluginChannelId, __resetChannelsForTest, - type ChannelMessage, - type ExternalChannel, } from '../../../src/nest/notifications/channel-registry'; +// The channel contract lives in notification-events; channel-registry only consumes it. +import type { ChannelMessage, ExternalChannel } from '../../../src/nest/notifications/notification-events'; import { registerBuiltinChannels } from '../../../src/nest/notifications/channels/builtins'; import type { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; import type { NtfyService } from '../../../src/nest/notifications/transports/ntfy.service'; diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index b3b45b02f..a64990f66 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/server/tests/unit/nest/collab.service.test.ts @@ -43,9 +43,13 @@ vi.mock('../../../src/config', () => ({ })); vi.mock('../../../src/websocket', () => ({ broadcast: vi.fn() })); -// Stub checkSsrf so linkPreview tests can control SSRF behaviour +// Stub checkSsrf so linkPreview tests can control SSRF behaviour. Typed from the real +// checkSsrf rather than from the default implementation below, so the stubbed results +// stay full SsrfResult objects instead of whatever shape the first fixture happened to have. const { mockCheckSsrf, mockCreatePinnedDispatcher } = vi.hoisted(() => ({ - mockCheckSsrf: vi.fn(async () => ({ allowed: true, resolvedIp: '93.184.216.34' })), + mockCheckSsrf: vi.fn( + async () => ({ allowed: true, isPrivate: false, resolvedIp: '93.184.216.34' }), + ), mockCreatePinnedDispatcher: vi.fn(() => ({})), })); vi.mock('../../../src/utils/ssrfGuard', () => ({ @@ -73,7 +77,7 @@ beforeAll(() => { beforeEach(() => { resetTestDb(testDb); - mockCheckSsrf.mockResolvedValue({ allowed: true, resolvedIp: '93.184.216.34' }); + mockCheckSsrf.mockResolvedValue({ allowed: true, isPrivate: false, resolvedIp: '93.184.216.34' }); }); afterAll(() => { @@ -83,7 +87,7 @@ afterAll(() => { afterEach(() => { vi.unstubAllGlobals(); mockCheckSsrf.mockReset(); - mockCheckSsrf.mockResolvedValue({ allowed: true, resolvedIp: '93.184.216.34' }); + mockCheckSsrf.mockResolvedValue({ allowed: true, isPrivate: false, resolvedIp: '93.184.216.34' }); }); // ── Helpers ─────────────────────────────────────────────────────────────────── @@ -377,7 +381,8 @@ describe('linkPreview', () => { }); it('COLLAB-SVC-028: returns fallback when SSRF check blocks the URL', async () => { - mockCheckSsrf.mockResolvedValue({ allowed: false, error: 'SSRF blocked' }); + // 169.254.x is link-local, which the real guard reports as private and blocked. + mockCheckSsrf.mockResolvedValue({ allowed: false, isPrivate: true, error: 'SSRF blocked' }); const result = await svc.linkPreview('https://169.254.169.254/'); expect(result.title).toBeNull(); diff --git a/server/tests/unit/nest/days.controller.test.ts b/server/tests/unit/nest/days.controller.test.ts index 5b88a3c01..f14986828 100644 --- a/server/tests/unit/nest/days.controller.test.ts +++ b/server/tests/unit/nest/days.controller.test.ts @@ -4,6 +4,7 @@ import { DaysController } from '../../../src/nest/days/days.controller'; import { DayNotesController } from '../../../src/nest/day-notes/day-notes.controller'; import { DayNoteCreateDto, DayNoteUpdateDto } from '../../../src/nest/day-notes/day-notes.dto'; import { DayReorderError } from '../../../src/nest/days/days.service'; +import type { DayReorderDto } from '../../../src/nest/days/days.dto'; import type { DaysService } from '../../../src/nest/days/days.service'; import type { DayNotesService } from '../../../src/nest/day-notes/day-notes.service'; import type { User } from '../../../src/types'; @@ -27,6 +28,14 @@ function notesSvc(o: Partial = {}): DayNotesService { return { verifyTripAccess: vi.fn().mockReturnValue(trip), canEdit: vi.fn().mockReturnValue(true), broadcast: vi.fn(), ...o } as unknown as DayNotesService; } +// reorder() keeps the legacy raw-body guard on orderedIds, which DayReorderDto's type +// says can never fire: over HTTP the ZodValidationPipe rejects such a body first. These +// unit tests call the handler directly, so they can still reach the guard, and the +// payload is cast instead of being made to satisfy the DTO. +function rawReorderBody(body: Record): DayReorderDto { + return body as unknown as DayReorderDto; +} + describe('DaysController (parity with the legacy /api/trips/:tripId/days route)', () => { // The 404 "Trip not found" and 403 "No permission" cases moved to // trip-access.guard.test.ts with the check itself: TripAccessGuard resolves :tripId @@ -59,11 +68,11 @@ describe('DaysController (parity with the legacy /api/trips/:tripId/days route)' it('400 when orderedIds is missing', () => { - expect(thrown(() => new DaysController(daysSvc()).reorder(user, '5', {}))).toEqual({ status: 400, body: { error: 'orderedIds must be an array' } }); + expect(thrown(() => new DaysController(daysSvc()).reorder(user, '5', rawReorderBody({})))).toEqual({ status: 400, body: { error: 'orderedIds must be an array' } }); }); it('400 when orderedIds is not an array', () => { - expect(thrown(() => new DaysController(daysSvc()).reorder(user, '5', { orderedIds: 'nope' as never }))).toEqual({ status: 400, body: { error: 'orderedIds must be an array' } }); + expect(thrown(() => new DaysController(daysSvc()).reorder(user, '5', rawReorderBody({ orderedIds: 'nope' })))).toEqual({ status: 400, body: { error: 'orderedIds must be an array' } }); }); it('maps a DayReorderError to 400 with its message', () => { diff --git a/server/tests/unit/nest/days.service.test.ts b/server/tests/unit/nest/days.service.test.ts index 95ecff609..b3b100a83 100644 --- a/server/tests/unit/nest/days.service.test.ts +++ b/server/tests/unit/nest/days.service.test.ts @@ -60,6 +60,7 @@ import { getDay as bridgeGetDay, listDays as bridgeListDays } from '../../../src import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; import { AccommodationsService } from '../../../src/nest/accommodations/accommodations.service'; +import type { Day } from '../../../src/types'; const svc = new DaysService(new DatabaseService(testDb), new PermissionsService(new DatabaseService(testDb)), new RealtimeService(), new QueryHelpersService(new DatabaseService(testDb))); const accommodations = new AccommodationsService(new DatabaseService(testDb), new PermissionsService(new DatabaseService(testDb)), new RealtimeService()); @@ -322,14 +323,16 @@ describe('quirk fixes', () => { it('DAY-SVC-033 — update preserves the omitted column (title-only keeps notes, notes-only keeps title)', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); - let day = createDay(testDb, trip.id); - day = svc.update(day.id, day, { notes: 'Walking day' }) as never; - day = svc.update(day.id, day, { title: 'Arrival' }) as never; + // Typed as the domain Day, not as the factory's row shape: the variable is + // rebound to what update() returns, and only Day carries `notes`. + let day: Day = createDay(testDb, trip.id); + day = svc.update(day.id, day, { notes: 'Walking day' }); + day = svc.update(day.id, day, { title: 'Arrival' }); expect(day).toMatchObject({ title: 'Arrival', notes: 'Walking day' }); - day = svc.update(day.id, day, { notes: 'Museum day' }) as never; + day = svc.update(day.id, day, { notes: 'Museum day' }); expect(day).toMatchObject({ title: 'Arrival', notes: 'Museum day' }); // A present key still clears via the legacy falsy coercion. - day = svc.update(day.id, day, { notes: '' }) as never; + day = svc.update(day.id, day, { notes: '' }); expect(day.notes).toBeNull(); expect(day.title).toBe('Arrival'); }); diff --git a/server/tests/unit/nest/journey-domain.service.test.ts b/server/tests/unit/nest/journey-domain.service.test.ts index 0d6932e26..e98cebc90 100644 --- a/server/tests/unit/nest/journey-domain.service.test.ts +++ b/server/tests/unit/nest/journey-domain.service.test.ts @@ -591,7 +591,7 @@ describe('updateEntry', () => { author_id: 999999, }; - const updated = svc.updateEntry(entry.id, user.id, malicious as Parameters[2]); + const updated = svc.updateEntry(entry.id, user.id, malicious as Parameters[2]); expect(updated).not.toBeNull(); expect(updated!.title).toBe('Updated'); // legit field still applied diff --git a/server/tests/unit/nest/notifications.service.test.ts b/server/tests/unit/nest/notifications.service.test.ts index 91da8b03a..4a920ecb0 100644 --- a/server/tests/unit/nest/notifications.service.test.ts +++ b/server/tests/unit/nest/notifications.service.test.ts @@ -77,7 +77,10 @@ import { DatabaseService } from '../../../src/nest/database/database.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { NotificationsService, type NotificationPayload } from '../../../src/nest/notifications/notifications.service'; import { send as bridgeSend } from '../../../src/nest/notifications/notifications.bridge'; -import { setPluginChannelSource, type ExternalChannel } from '../../../src/nest/notifications/channel-registry'; +import { setPluginChannelSource } from '../../../src/nest/notifications/channel-registry'; +// The channel interface lives in notification-events; channel-registry only imports +// it for its own signatures and never re-exported it. +import type { ExternalChannel } from '../../../src/nest/notifications/notification-events'; import { makeNotificationsService, makeNotificationPreferencesService } from '../../helpers/notifications'; const notifications = makeNotificationsService(new DatabaseService(testDb)); diff --git a/server/tests/unit/nest/oidc.service.test.ts b/server/tests/unit/nest/oidc.service.test.ts index 3159f1fc5..9d66074d1 100644 --- a/server/tests/unit/nest/oidc.service.test.ts +++ b/server/tests/unit/nest/oidc.service.test.ts @@ -89,9 +89,11 @@ import { resetTestDb } from '../../helpers/test-db'; import { createUser, createTrip } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; -import { AtlasService } from '../../../src/nest/atlas/atlas.service'; import { TripMembershipService } from '../../../src/nest/trip-membership/trip-membership.service'; import { AuthService } from '../../../src/nest/auth/auth.service'; +import { WebauthnConfigService } from '../../../src/nest/auth/webauthn-config.service'; +import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service'; +import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { OidcService } from '../../../src/nest/oidc/oidc.service'; import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; @@ -100,14 +102,18 @@ import { MailerService } from '../../../src/nest/notifications/mailer/mailer.ser const mailerStub = { sendPasswordResetEmail: vi.fn() } as unknown as MailerService; const membership = new TripMembershipService(new DatabaseService(testDb)); +// Positional, and previously shifted by one: an AtlasService sat in the membership +// slot, which AuthService no longer takes at all, so webauthn/userCleanup/mailer +// each landed one place too late and the EphemeralTokenService was missing +// entirely. Nothing failed, because no case below reaches those collaborators. const auth = new AuthService( new DatabaseService(testDb), new PermissionsService(new DatabaseService(testDb)), - new AtlasService(new DatabaseService(testDb)), membership, - undefined as never, // webauthn — not reached from the OIDC paths - undefined as never, // userCleanup — not reached from the OIDC paths + new WebauthnConfigService(new DatabaseService(testDb)), + new UserCleanupService(new DatabaseService(testDb)), mailerStub, + new EphemeralTokenService(), ); const svc = new OidcService(new DatabaseService(testDb), auth, membership); diff --git a/server/tests/unit/nest/passkey.service.test.ts b/server/tests/unit/nest/passkey.service.test.ts index 6429bba3f..5877d8c0d 100644 --- a/server/tests/unit/nest/passkey.service.test.ts +++ b/server/tests/unit/nest/passkey.service.test.ts @@ -81,10 +81,12 @@ import { resetTestDb } from '../../helpers/test-db'; import { createUser } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; -import { AtlasService } from '../../../src/nest/atlas/atlas.service'; +import { TripMembershipService } from '../../../src/nest/trip-membership/trip-membership.service'; import { AuthService } from '../../../src/nest/auth/auth.service'; import { PasskeyService } from '../../../src/nest/auth/passkey.service'; -import type { WebauthnConfigService } from '../../../src/nest/auth/webauthn-config.service'; +import { WebauthnConfigService } from '../../../src/nest/auth/webauthn-config.service'; +import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service'; +import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; // MailerService is injected since the notifications fold — a stub instead of a @@ -97,14 +99,23 @@ const mailerStub = { sendPasswordResetEmail: vi.fn() } as unknown as MailerServi const resolveWebauthnConfigMock = vi.fn(); const webauthn = { resolve: resolveWebauthnConfigMock } as unknown as WebauthnConfigService; +// Positional, and previously shifted by one: an AtlasService sat in the +// membership slot, which AuthService no longer takes at all, so +// webauthn/userCleanup/mailer each landed one place too late and the +// EphemeralTokenService was missing entirely. Nothing failed, because the only +// thing PasskeyService asks AuthService for is generateToken, which reads the +// database and nothing else. The collaborators are real here rather than +// undefined so a future case that does reach one gets a working object; each +// takes only the DatabaseService. AuthService's own webauthn is the real +// resolver, separate from the `webauthn` switch PasskeyService is handed. const auth = new AuthService( new DatabaseService(testDb), new PermissionsService(new DatabaseService(testDb)), - new AtlasService(new DatabaseService(testDb)), - undefined as never, // membership — not reached from the passkey paths - undefined as never, // webauthn — PasskeyService gets its own stub below - undefined as never, // userCleanup — not reached from the passkey paths + new TripMembershipService(new DatabaseService(testDb)), + new WebauthnConfigService(new DatabaseService(testDb)), + new UserCleanupService(new DatabaseService(testDb)), mailerStub, + new EphemeralTokenService(), ); const svc = new PasskeyService(new DatabaseService(testDb), auth, webauthn); diff --git a/server/tests/unit/nest/realtime.service.test.ts b/server/tests/unit/nest/realtime.service.test.ts index 48916a340..e08b615c1 100644 --- a/server/tests/unit/nest/realtime.service.test.ts +++ b/server/tests/unit/nest/realtime.service.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { TrekWsPayload, TrekWsUserEventName } from '@trek/shared'; const { broadcast, broadcastToUser } = vi.hoisted(() => ({ broadcast: vi.fn(), @@ -10,6 +11,19 @@ import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; const svc = new RealtimeService(); +/** + * These cases exercise argument pass-through, not payload shape, so they hand + * the facade only the fields their assertions read. The registry contract asks + * for more (`vacay:invite` for a `from`, `notification:new` for a + * `notification`); filling that in would put data into the recorded call that + * nothing here checks, so the shortcut is named instead of padded out. + */ +function partialUserPayload( + payload: { type: E } & Partial>, +): { type: E } & TrekWsPayload { + return payload as { type: E } & TrekWsPayload; +} + beforeEach(() => vi.clearAllMocks()); describe('RealtimeService', () => { @@ -31,7 +45,7 @@ describe('RealtimeService', () => { }); it('broadcastToUser delegates every argument untouched', () => { - const payload = { type: 'vacay:invite', planId: 3 }; + const payload = partialUserPayload({ type: 'vacay:invite', planId: 3 }); svc.broadcastToUser(5, payload, '11'); expect(broadcastToUser).toHaveBeenCalledTimes(1); expect(broadcastToUser).toHaveBeenCalledWith(5, payload, '11'); @@ -45,7 +59,7 @@ describe('RealtimeService', () => { // routes through the mocked module functions rather than captured bindings. const late = new RealtimeService(); late.broadcast('1', 'todo:created', { item: {} }, undefined); - late.broadcastToUser(2, { type: 'notification:new' }); + late.broadcastToUser(2, partialUserPayload({ type: 'notification:new' })); expect(broadcast).toHaveBeenCalledWith('1', 'todo:created', { item: {} }, undefined); expect(broadcastToUser).toHaveBeenCalledWith(2, { type: 'notification:new' }); }); diff --git a/server/tests/unit/nest/trip-invite.service.test.ts b/server/tests/unit/nest/trip-invite.service.test.ts index 9bdbe2a69..06d62d25a 100644 --- a/server/tests/unit/nest/trip-invite.service.test.ts +++ b/server/tests/unit/nest/trip-invite.service.test.ts @@ -38,9 +38,13 @@ import { resetTestDb } from '../../helpers/test-db'; import { createUser, createTrip } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; +import { TripMembershipService } from '../../../src/nest/trip-membership/trip-membership.service'; import { TripInviteService } from '../../../src/nest/trip-invite/trip-invite.service'; -const svc = new TripInviteService(new DatabaseService(testDb), new PermissionsService(new DatabaseService(testDb))); +// One DatabaseService over the shared in-memory handle, so every collaborator +// reads and writes the same rows. +const dbs = new DatabaseService(testDb); +const svc = new TripInviteService(dbs, new PermissionsService(dbs), new TripMembershipService(dbs)); beforeAll(() => { createTables(testDb); runMigrations(testDb); }); beforeEach(() => resetTestDb(testDb)); diff --git a/server/tests/unit/plugins/host-rpc.test.ts b/server/tests/unit/plugins/host-rpc.test.ts index 7c540787f..1b2ae8d02 100644 --- a/server/tests/unit/plugins/host-rpc.test.ts +++ b/server/tests/unit/plugins/host-rpc.test.ts @@ -72,7 +72,9 @@ import type { LlmConfigResolver } from '../../../src/nest/llm-parse/llm-config.r import type { PluginOAuthService } from '../../../src/nest/plugins/oauth/plugin-oauth.service'; import type { RpcError, RpcResponse } from '../../../src/nest/plugins/protocol/envelope'; -const checkPermission = vi.fn(() => true as boolean); +// Typed from the real method rather than from the always-true body below, so a case that +// swaps in an implementation reading the action key (HOSTRPC-015) still type-checks. +const checkPermission = vi.fn(() => true); const permissions = { checkPermission } as unknown as PermissionsService; const addons = { isAddonEnabled: vi.fn(() => true) } as unknown as AddonsService; const notifications = { send: notifySend } as unknown as NotificationsService; @@ -310,8 +312,8 @@ describe('MetaRpc — namespaced entity metadata', () => { it('HOSTRPC-015 every entity type resolves its own edit action, and a read-only member is refused a write', async () => { const host = makeHost('meta', 'db:meta'); const seen: string[] = []; - checkPermission.mockImplementation((action: unknown) => { - seen.push(String(action)); + checkPermission.mockImplementation((action) => { + seen.push(action); return true; }); for (const [entityType, entityId] of [['trip', 1], ['place', 7], ['day', 3], ['reservation', 40], ['accommodation', 11]] as const) { diff --git a/server/tests/unit/plugins/plugin-guards.test.ts b/server/tests/unit/plugins/plugin-guards.test.ts index 86082c45b..2cea6fc06 100644 --- a/server/tests/unit/plugins/plugin-guards.test.ts +++ b/server/tests/unit/plugins/plugin-guards.test.ts @@ -12,10 +12,19 @@ import type { PermissionsService } from '../../../src/nest/permissions/permissio import type { AddonsService } from '../../../src/nest/addons/addons.service'; import type { PluginRpcContext } from '../../../src/nest/plugins/host/rpc-kit/types'; +/** + * The guards read `actingUserId` and nothing else off the context, so the two per-host + * collaborators are inert here. `plugins` gets real no-op functions rather than a cast, + * so a guard that ever reached for a peer would hit a callable, not undefined. + */ const ctx = (actingUserId: number | undefined): PluginRpcContext => ({ pluginId: 'p', actingUserId, data: {} as PluginRpcContext['data'], + plugins: { + call: vi.fn(async () => undefined), + emit: vi.fn(), + }, }); /** Trip 1 belongs to user 42 and only user 42 may reach it, mirroring rpc-host.test.ts. */ diff --git a/server/tests/unit/plugins/plugin-hooks.test.ts b/server/tests/unit/plugins/plugin-hooks.test.ts index af034129d..6255c0f6c 100644 --- a/server/tests/unit/plugins/plugin-hooks.test.ts +++ b/server/tests/unit/plugins/plugin-hooks.test.ts @@ -51,7 +51,10 @@ describe('PluginHooks contracts', () => { const registry = createTestPluginRegistry([new PluginHooks({} as never)]); it('PLUGHOOK-001 every hook the consent screen offers has a host-side consumer', () => { - const covered = new Set([...registry.hookContracts().values()].map((c) => c.hook)); + // A plain string set on purpose: the keys probed against it come from + // HOOK_PERMISSION, and whether each one is a hook the host consumes is the + // very thing under test, not something to assert into the element type. + const covered = new Set([...registry.hookContracts().values()].map((c) => c.hook)); expect([...Object.keys(HOOK_PERMISSION)].filter((h) => !covered.has(h))).toEqual([]); }); diff --git a/server/tests/unit/plugins/plugin-user-settings.test.ts b/server/tests/unit/plugins/plugin-user-settings.test.ts index 797b50b69..084a73a86 100644 --- a/server/tests/unit/plugins/plugin-user-settings.test.ts +++ b/server/tests/unit/plugins/plugin-user-settings.test.ts @@ -17,6 +17,7 @@ const { getDb } = vi.hoisted(() => ({ getDb: { current: null as unknown } })); vi.mock('../../../src/db/database', () => ({ get db() { return getDb.current; } })); import { db as dbConn } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; +import { AddonsService } from '../../../src/nest/addons/addons.service'; import Database from 'better-sqlite3'; import { PluginsService } from '../../../src/nest/plugins/plugins.service'; @@ -40,7 +41,13 @@ function freshDb() { describe('per-user plugin settings', () => { let svc: PluginsService; - beforeEach(() => { getDb.current = freshDb(); svc = new PluginsService(new DatabaseService(dbConn)); }); + // AddonsService only feeds PluginsService.list(), which no case here calls, but it is a + // real collaborator on the same connection rather than a stand-in. + beforeEach(() => { + getDb.current = freshDb(); + const dbs = new DatabaseService(dbConn); + svc = new PluginsService(dbs, new AddonsService(dbs)); + }); it('lists only the user-scope fields, in order', () => { const fields = svc.userSettingsFields('p'); diff --git a/server/tests/unit/plugins/plugins-service.test.ts b/server/tests/unit/plugins/plugins-service.test.ts index d214c76b5..3f1bc1ca4 100644 --- a/server/tests/unit/plugins/plugins-service.test.ts +++ b/server/tests/unit/plugins/plugins-service.test.ts @@ -118,7 +118,11 @@ describe('PluginsService.list', () => { it('never leaks the raw pinned key into the list response (only the fingerprint)', () => { insert('signed-one', 'acme/signed-one', 'RWTvBn0aBcDeFgHiJkLmNoPqRsTuVwXyZ0123456789abcd'); - const p = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list().plugins[0] as Record; + // PluginListItem is an interface, so it has no implicit index signature and cannot + // be narrowed to a record directly. Widening through unknown is what lets this case + // probe for a key the contract deliberately does not declare. + const p = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list() + .plugins[0] as unknown as Record; expect(p.author_pubkey).toBeUndefined(); }); }); diff --git a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts index 824e4100d..9739dd507 100644 --- a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts +++ b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts @@ -18,11 +18,19 @@ import { TripsRpc } from '../../../src/nest/trips/trips.rpc'; import { DbRpc } from '../../../src/nest/plugins/host/rpc/db.rpc'; import { PluginGuards } from '../../../src/nest/plugins/host/plugin-guards.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { PluginUserSettingsService } from '../../../src/nest/plugins/plugin-user-settings.service'; const req = (method: string, params: Record): RpcRequest => ({ k: 'req', id: 'x', method, params }); -/** db.* is the cheapest handler to watch, and it needs no services of its own. */ -const dbRegistry = () => createTestPluginRegistry([new DbRpc()]); +/** + * db.* is the cheapest handler to watch. The only service it needs is the per-user + * settings store behind `settings.get`, which none of these cases dispatch, so the stub + * carries just that read, typed off the real signature. + */ +const dbRegistry = () => { + const settings: Pick = { readOne: () => undefined }; + return createTestPluginRegistry([new DbRpc(settings as PluginUserSettingsService)]); +}; /** trips.getById lives on the decorators now, so the audit cases bind it through them. */ const tripsRegistry = () => { diff --git a/server/tests/unit/plugins/rpc-host.test.ts b/server/tests/unit/plugins/rpc-host.test.ts index 0f320454d..ce30579fe 100644 --- a/server/tests/unit/plugins/rpc-host.test.ts +++ b/server/tests/unit/plugins/rpc-host.test.ts @@ -16,6 +16,14 @@ import type { RpcRequest, RpcResponse, RpcError } from '../../../src/nest/plugin import { makeDeps } from '../../helpers/rpc-host-deps'; const req = (method: string, params: Record = {}): RpcRequest => ({ k: 'req', id: 'x', method, params }); +/** + * A request as it arrives when the child never set `params`: the IPC hop is JSON, which + * drops an undefined field, so the key is genuinely absent by the time the router sees + * it. `RpcRequest` declares `params` as required, so leaving it out only type-checks + * through this cast — writing `params: undefined` instead would put the key back and + * stop covering the case the host's `req.params ?? {}` exists for. + */ +const reqWithoutParams = (method: string): RpcRequest => ({ k: 'req', id: 'x', method }) as RpcRequest; const ok = (r: RpcResponse | RpcError): r is RpcResponse => r.ok === true; const code = (r: RpcResponse | RpcError): string => (r as RpcError).error.code; @@ -158,7 +166,7 @@ describe('PluginRpcHost — capability enforcement', () => { seen = params; return null; }); - expect(ok(await host.dispatch({ k: 'req', id: 'x', method: 'tags.list' }, 42))).toBe(true); + expect(ok(await host.dispatch(reqWithoutParams('tags.list'), 42))).toBe(true); expect(seen).toEqual({}); }); diff --git a/server/tests/unit/plugins/rpc-kit/registry.test.ts b/server/tests/unit/plugins/rpc-kit/registry.test.ts index 06fd253a3..37872f4ec 100644 --- a/server/tests/unit/plugins/rpc-kit/registry.test.ts +++ b/server/tests/unit/plugins/rpc-kit/registry.test.ts @@ -20,6 +20,15 @@ const ctxFor = (actingUserId: number | undefined): PluginRpcContext => ({ pluginId: 'p', actingUserId, data: {} as PluginRpcContext['data'], + // No case below dispatches to a peer plugin, but the peers are a real collaborator + // on the context, so they get a stub that fails loudly rather than an empty object + // whose methods would be undefined the moment a handler reached for one. + plugins: { + call: () => Promise.reject(new Error('ctxFor: no peer plugin router in these cases')), + emit: () => { + throw new Error('ctxFor: no peer plugin router in these cases'); + }, + }, }); /** A well-formed controller: two tags methods with the permissions the table assigns. */