From e606ebd9b902f4adde925b7c926f5326613539cb Mon Sep 17 00:00:00 2001 From: Maurice <61554723+mauriceboe@users.noreply.github.com> Date: Thu, 6 Aug 2026 22:44:59 +0200 Subject: [PATCH] chore(server): pin the WebSocket envelope, dedupe the rate limiter (#1803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(nest): register LlmParseModule in AppModule The module was only in the container because BookingImportModule and PluginsModule happen to import it, so /api/admin/llm/local depended on those two keeping that import. Register it where every other module is registered. * refactor(nest): give the rate limiter one home and one instance RateLimitService keeps its counters in a Map on the instance, but it was listed in the providers of auth, oauth, transit and trip-invite, so each of them got its own. Nothing is broken today — the bucket names in use (login/mfa/forgot/reset, oauth_*, transit_*, trip_invite) don't overlap — but two modules picking the same name would have quietly allowed twice the configured attempts. Move it to nest/common, where it belongs now that four domains use it, and hand it out through a small module they import. Not @Global: the e2e suites build a container around one domain module, so a global AppModule never pulled in isn't there at all. transit.mcp.ts keeps its own instance on purpose — it keys by userId under mcp_transit_* while the REST path keys by IP. * test(ws): pin the wire envelope before touching the transport Every frame on the socket is flat: {type, tripId, ...payload} out, {type, tripId} in. Nest's WsAdapter dispatches on message.event and answers {event, data}, so moving this onto a gateway without a custom serialiser would leave join silently unhandled — no welcome.socketId to send back as X-Socket-Id, and every client would then receive its own writes. Nothing catches that today: the 100+ tests that touch realtime mock src/websocket and never look at the bytes. WS-032 sends the exact shape the stock adapter would and asserts the server ignores it. * docs(nest): drop the parts of the README that no longer exist strangler.ts, DEFAULT_NEST_PREFIXES/NEST_PREFIXES and tests/parity/ are all gone — every request goes through Nest now — but the README still walks a new contributor through adding a prefix and writing a parity test against the Express route. Also spell out that the 80% gate is an average over src/nest/**, not a floor per file, so a large untested module can sit behind the well-covered small ones. --- server/src/nest/README.md | 33 +-- server/src/nest/app.module.ts | 3 +- .../src/nest/auth/auth-public.controller.ts | 2 +- server/src/nest/auth/auth.controller.ts | 2 +- server/src/nest/auth/auth.module.ts | 6 +- server/src/nest/auth/passkey.controller.ts | 2 +- server/src/nest/common/rate-limit.module.ts | 21 ++ .../{auth => common}/rate-limit.service.ts | 0 server/src/nest/oauth/oauth-api.controller.ts | 2 +- .../src/nest/oauth/oauth-public.controller.ts | 2 +- server/src/nest/oauth/oauth.module.ts | 6 +- server/src/nest/transit/transit.controller.ts | 2 +- server/src/nest/transit/transit.mcp.ts | 2 +- server/src/nest/transit/transit.module.ts | 6 +- .../trip-invite/trip-invite.controller.ts | 2 +- .../nest/trip-invite/trip-invite.module.ts | 6 +- server/tests/helpers/test-db.ts | 2 +- server/tests/unit/nest/auth-guard.test.ts | 2 +- .../tests/unit/nest/auth.controller.test.ts | 2 +- .../tests/unit/nest/oauth.controller.test.ts | 2 +- server/tests/unit/nest/oauth.service.test.ts | 5 +- server/tests/websocket/wire-envelope.test.ts | 261 ++++++++++++++++++ 22 files changed, 326 insertions(+), 45 deletions(-) create mode 100644 server/src/nest/common/rate-limit.module.ts rename server/src/nest/{auth => common}/rate-limit.service.ts (100%) create mode 100644 server/tests/websocket/wire-envelope.test.ts diff --git a/server/src/nest/README.md b/server/src/nest/README.md index 616d374d7..223ffda14 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -15,11 +15,10 @@ server/src/nest//.controller.ts # same routes/verbs/params/s server/src/nest//.module.ts # registered in app.module.ts ``` -Add the prefix to `DEFAULT_NEST_PREFIXES` in `strangler.ts` to route it to Nest -(operators can override at runtime via the `NEST_PREFIXES` env var — instant -rollback, no redeploy). Trip-scoped mounts use a pattern prefix with a `:param` -segment (e.g. `/api/trips/:tripId/packing`); the matcher routes only that nested -mount to Nest and leaves the sibling trip routes (days, places, ...) on Express. +Register the module in `app.module.ts` and it is live. There is no routing gate to +flip any more: **every request goes through Nest**. The `strangler.ts` matcher and +its `DEFAULT_NEST_PREFIXES` / `NEST_PREFIXES` escape hatch are gone — Express only +remains as the platform underneath `@nestjs/platform-express`. ## Migrated so far @@ -37,7 +36,6 @@ mount to Nest and leaves the sibling trip routes (days, places, ...) on Express. - `common/idempotency.interceptor.ts` — global `APP_INTERCEPTOR` replaying the client's `X-Idempotency-Key` on mutations, mirroring the legacy `applyIdempotency` middleware so retried writes don't double-apply. -- `strangler.ts` — supports both static prefixes and `:param` pattern prefixes. - `app-config/` — the `@nestjs/config` binding (`AppConfigModule`, global). Never read `process.env` in a module (ESLint enforces this): inject a boot-stable namespace via its `registerAs` token (`@Inject(mcpConfig.KEY) … ConfigType<…>`) @@ -66,21 +64,21 @@ the controller rather than relying on the generic `ZodValidationPipe` envelope. ## How to write the tests -Every module ships three kinds of tests; the coverage gate (`vitest.config.ts`, -scoped to `src/nest/**`) requires ≥80%. +Every module ships two kinds of tests. The coverage gate (`vitest.config.ts`) +requires ≥80% over `src/nest/**` — note that it is an **average across the whole +tree**, not a per-file floor, so a large untested module can hide behind the +well-covered small ones. 1. **Service / controller unit spec** — `tests/unit/nest/.controller.test.ts`. Instantiate the controller with a mocked service; assert status codes, the exact `{ error }` bodies, and that inputs are forwarded correctly (defaults, coercion). See `weather.controller.test.ts`. -2. **Parity test** — `tests/parity/.parity.test.ts`. Mock the shared service - identically for both apps, then fire the same request at the Express route and the - Nest controller with the `expectParity()` harness (`tests/parity/parity.ts`) and - assert identical status + body. This is the gate before flipping the toggle. - See `weather.parity.test.ts`. + Parity against the behaviour being replaced belongs here too — assert the exact + status codes and bodies the old path produced. There is no separate + `tests/parity/` directory; it was removed along with the routing toggle. -3. **e2e** — `tests/e2e/.e2e.test.ts`. Boot the Nest module against a temp +2. **e2e** — `tests/e2e/.e2e.test.ts`. Boot the Nest module against a temp in-memory SQLite db via the shared harness (`tests/e2e/harness.ts`: `createTempDb`/`seedUser`/`sessionCookie`), exercising the **real** `JwtAuthGuard` end-to-end (401 without cookie, 200 with a signed session). Mock external I/O @@ -89,10 +87,9 @@ scoped to `src/nest/**`) requires ≥80%. ## Definition of Done (per module) Contract in `@trek/shared` → service ported 1:1 → controller with identical routes → -validation/error parity → unit + parity + e2e tests over the gate → prefix toggled to -Nest → parity verified on the demo DB → **then** decommission the old Express -route/service (separate step, after the toggle is confirmed in prod) → frontend points -at the typed contract (Frontend Track). +validation/error parity → unit + e2e tests, with the old behaviour asserted in the +unit test → module registered in `app.module.ts` → **then** decommission the legacy +service (separate step) → frontend points at the typed contract (Frontend Track). ## Migrating a legacy `src/services/*` service into its Nest module (recipe) diff --git a/server/src/nest/app.module.ts b/server/src/nest/app.module.ts index c9ab8d500..54511383c 100644 --- a/server/src/nest/app.module.ts +++ b/server/src/nest/app.module.ts @@ -39,6 +39,7 @@ import { FeedsModule } from './feeds/feeds.module'; import { SettingsModule } from './settings/settings.module'; import { BackupModule } from './backup/backup.module'; import { BookingImportModule } from './booking-import/booking-import.module'; +import { LlmParseModule } from './llm-parse/llm-parse.module'; import { AuthModule } from './auth/auth.module'; import { OidcModule } from './oidc/oidc.module'; import { OauthModule } from './oauth/oauth.module'; @@ -59,7 +60,7 @@ import { IdempotencyInterceptor } from './common/idempotency.interceptor'; * migrated. */ @Module({ - imports: [AppConfigModule, DatabaseModule, RealtimeModule, McpModule.forRoot({ accessPolicy: trekMcpAccessPolicy, validateAccess: trekMcpValidateAccess }), WeatherModule, HelpModule, AirportsModule, ConfigModule, SystemNoticesModule, MapsModule, CategoriesModule, TagsModule, NotificationsModule, AtlasModule, VacayModule, PackingModule, TodoModule, BudgetModule, ReservationsModule, DaysModule, AssignmentsModule, PlacesModule, TripsModule, CollabModule, FilesModule, PhotosModule, MemoriesModule, AirtrailModule, JourneyModule, CollectionsModule, ShareModule, TripInviteModule, TransitModule, FeedsModule, SettingsModule, BackupModule, AuthModule, OidcModule, OauthModule, AdminModule, AddonsModule, AuditModule, PermissionsModule, PluginsModule, BookingImportModule], + imports: [AppConfigModule, DatabaseModule, RealtimeModule, McpModule.forRoot({ accessPolicy: trekMcpAccessPolicy, validateAccess: trekMcpValidateAccess }), WeatherModule, HelpModule, AirportsModule, ConfigModule, SystemNoticesModule, MapsModule, CategoriesModule, TagsModule, NotificationsModule, AtlasModule, VacayModule, PackingModule, TodoModule, BudgetModule, ReservationsModule, DaysModule, AssignmentsModule, PlacesModule, TripsModule, CollabModule, FilesModule, PhotosModule, MemoriesModule, AirtrailModule, JourneyModule, CollectionsModule, ShareModule, TripInviteModule, TransitModule, FeedsModule, SettingsModule, BackupModule, AuthModule, OidcModule, OauthModule, AdminModule, AddonsModule, AuditModule, PermissionsModule, PluginsModule, BookingImportModule, LlmParseModule], controllers: [HealthController], providers: [ HealthService, diff --git a/server/src/nest/auth/auth-public.controller.ts b/server/src/nest/auth/auth-public.controller.ts index 837cf532e..faf9d010b 100644 --- a/server/src/nest/auth/auth-public.controller.ts +++ b/server/src/nest/auth/auth-public.controller.ts @@ -2,7 +2,7 @@ import { Body, Controller, Get, HttpCode, HttpException, Param, Post, Req, Res, import type { Request, Response } from 'express'; import { AuthService } from './auth.service'; import { RegisterDto, LoginDto, ForgotPasswordDto, ResetPasswordDto, MfaVerifyLoginDto } from './auth.dto'; -import { RateLimitService } from './rate-limit.service'; +import { RateLimitService } from '../common/rate-limit.service'; import { OptionalJwtGuard } from './optional-jwt.guard'; import { getClientIp } from '../audit/client-ip'; import { AuditService } from '../audit/audit.service'; diff --git a/server/src/nest/auth/auth.controller.ts b/server/src/nest/auth/auth.controller.ts index 6097fbae2..f44edb345 100644 --- a/server/src/nest/auth/auth.controller.ts +++ b/server/src/nest/auth/auth.controller.ts @@ -34,7 +34,7 @@ import { McpTokenCreateDto, ResourceTokenDto, } from './auth.dto'; -import { RateLimitService } from './rate-limit.service'; +import { RateLimitService } from '../common/rate-limit.service'; import { JwtAuthGuard } from './jwt-auth.guard'; import { CurrentUser } from './current-user.decorator'; import { getClientIp } from '../audit/client-ip'; diff --git a/server/src/nest/auth/auth.module.ts b/server/src/nest/auth/auth.module.ts index 2cf2a241e..8e36c87b8 100644 --- a/server/src/nest/auth/auth.module.ts +++ b/server/src/nest/auth/auth.module.ts @@ -1,10 +1,10 @@ +import { RateLimitModule } from '../common/rate-limit.module'; import { Module } from '@nestjs/common'; import { AuthPublicController } from './auth-public.controller'; import { AuthController } from './auth.controller'; import { PasskeyController } from './passkey.controller'; import { AuthService } from './auth.service'; import { PasskeyService } from './passkey.service'; -import { RateLimitService } from './rate-limit.service'; import { AuditModule } from '../audit/audit.module'; import { PermissionsModule } from '../permissions/permissions.module'; import { AtlasModule } from '../atlas/atlas.module'; @@ -22,9 +22,9 @@ import { AtlasModule } from '../atlas/atlas.module'; * everything outside the container goes through auth.bridge.ts. */ @Module({ - imports: [AuditModule, PermissionsModule, AtlasModule], + imports: [RateLimitModule, AuditModule, PermissionsModule, AtlasModule], controllers: [AuthPublicController, AuthController, PasskeyController], - providers: [AuthService, PasskeyService, RateLimitService], + providers: [AuthService, PasskeyService], exports: [AuthService, PasskeyService], }) export class AuthModule {} diff --git a/server/src/nest/auth/passkey.controller.ts b/server/src/nest/auth/passkey.controller.ts index a85c87e6b..4993ff77f 100644 --- a/server/src/nest/auth/passkey.controller.ts +++ b/server/src/nest/auth/passkey.controller.ts @@ -1,6 +1,6 @@ import { Body, Controller, Delete, Get, HttpCode, HttpException, Param, Patch, Post, Req, Res, UseGuards } from '@nestjs/common'; import type { Request, Response } from 'express'; -import { RateLimitService } from './rate-limit.service'; +import { RateLimitService } from '../common/rate-limit.service'; import { JwtAuthGuard } from './jwt-auth.guard'; import { PasskeyEnabledGuard } from './passkey-enabled.guard'; import { CurrentUser } from './current-user.decorator'; diff --git a/server/src/nest/common/rate-limit.module.ts b/server/src/nest/common/rate-limit.module.ts new file mode 100644 index 000000000..5b78e29f2 --- /dev/null +++ b/server/src/nest/common/rate-limit.module.ts @@ -0,0 +1,21 @@ +import { Module } from '@nestjs/common'; +import { RateLimitService } from './rate-limit.service'; + +/** + * One module owning the limiter, imported by every consumer. + * + * It matters because the counters live in memory on the instance: declaring the + * service in each consuming module's `providers` hands every one of them its own + * map, so the same bucket name used from two modules would silently allow twice + * the configured attempts. The buckets in use today happen not to overlap, which + * is why nothing is broken — but it is one copy-pasted name away from being wrong. + * + * Deliberately not `@Global()`: the e2e suites build a container around a single + * domain module, where a global that AppModule never pulled in simply isn't there. + * An explicit import also states the dependency where it is actually used. + */ +@Module({ + providers: [RateLimitService], + exports: [RateLimitService], +}) +export class RateLimitModule {} diff --git a/server/src/nest/auth/rate-limit.service.ts b/server/src/nest/common/rate-limit.service.ts similarity index 100% rename from server/src/nest/auth/rate-limit.service.ts rename to server/src/nest/common/rate-limit.service.ts diff --git a/server/src/nest/oauth/oauth-api.controller.ts b/server/src/nest/oauth/oauth-api.controller.ts index 21c1d1b27..d4bbe8547 100644 --- a/server/src/nest/oauth/oauth-api.controller.ts +++ b/server/src/nest/oauth/oauth-api.controller.ts @@ -1,7 +1,7 @@ import { Body, Controller, Delete, Get, HttpCode, HttpException, Param, Post, Query, Req, Res, UseGuards } from '@nestjs/common'; import type { Request, Response } from 'express'; import { OauthService } from './oauth.service'; -import { RateLimitService } from '../auth/rate-limit.service'; +import { RateLimitService } from '../common/rate-limit.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CookieAuthGuard } from '../auth/cookie-auth.guard'; import { OptionalJwtGuard } from '../auth/optional-jwt.guard'; diff --git a/server/src/nest/oauth/oauth-public.controller.ts b/server/src/nest/oauth/oauth-public.controller.ts index e5e6c03ec..b5ade12e5 100644 --- a/server/src/nest/oauth/oauth-public.controller.ts +++ b/server/src/nest/oauth/oauth-public.controller.ts @@ -1,7 +1,7 @@ import { Controller, Get, Headers, HttpCode, Post, Req, Res } from '@nestjs/common'; import type { Request, Response } from 'express'; import { OauthService } from './oauth.service'; -import { RateLimitService } from '../auth/rate-limit.service'; +import { RateLimitService } from '../common/rate-limit.service'; import { getClientIp } from '../audit/client-ip'; import { logWarn } from '../audit/audit-log.logger'; import { AuditService } from '../audit/audit.service'; diff --git a/server/src/nest/oauth/oauth.module.ts b/server/src/nest/oauth/oauth.module.ts index 518bb4168..fb3dfe722 100644 --- a/server/src/nest/oauth/oauth.module.ts +++ b/server/src/nest/oauth/oauth.module.ts @@ -1,8 +1,8 @@ +import { RateLimitModule } from '../common/rate-limit.module'; import { Module } from '@nestjs/common'; import { OauthPublicController } from './oauth-public.controller'; import { OauthApiController } from './oauth-api.controller'; import { OauthService } from './oauth.service'; -import { RateLimitService } from '../auth/rate-limit.service'; import { AuditModule } from '../audit/audit.module'; import { AddonsModule } from '../addons/addons.module'; @@ -13,8 +13,8 @@ import { AddonsModule } from '../addons/addons.module'; * strangler lists /oauth/token, /oauth/userinfo, /oauth/revoke explicitly. */ @Module({ - imports: [AuditModule, AddonsModule], + imports: [RateLimitModule, AuditModule, AddonsModule], controllers: [OauthPublicController, OauthApiController], - providers: [OauthService, RateLimitService], + providers: [OauthService], }) export class OauthModule {} diff --git a/server/src/nest/transit/transit.controller.ts b/server/src/nest/transit/transit.controller.ts index 36fd62360..7f6293f7d 100644 --- a/server/src/nest/transit/transit.controller.ts +++ b/server/src/nest/transit/transit.controller.ts @@ -1,7 +1,7 @@ import { Controller, Get, HttpException, Query, Req, UseGuards } from '@nestjs/common'; import type { Request } from 'express'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; -import { RateLimitService } from '../auth/rate-limit.service'; +import { RateLimitService } from '../common/rate-limit.service'; import { TransitService } from './transit.service'; const RL_WINDOW = 15 * 60 * 1000; diff --git a/server/src/nest/transit/transit.mcp.ts b/server/src/nest/transit/transit.mcp.ts index bb0d2c7df..b1b64f824 100644 --- a/server/src/nest/transit/transit.mcp.ts +++ b/server/src/nest/transit/transit.mcp.ts @@ -14,7 +14,7 @@ import { transitPlaceSchema, } from './transit-itinerary.helpers'; import { hasTripPermission, noAccess, permissionDenied, safeBroadcast } from '../../mcp/tools/_shared'; -import { RateLimitService } from '../auth/rate-limit.service'; +import { RateLimitService } from '../common/rate-limit.service'; import { DatabaseService } from '../database/database.service'; import { DaysService } from '../days/days.service'; import { ReservationsService } from '../reservations/reservations.service'; diff --git a/server/src/nest/transit/transit.module.ts b/server/src/nest/transit/transit.module.ts index 0bb3fcce5..205615b16 100644 --- a/server/src/nest/transit/transit.module.ts +++ b/server/src/nest/transit/transit.module.ts @@ -1,8 +1,8 @@ +import { RateLimitModule } from '../common/rate-limit.module'; import { Module } from '@nestjs/common'; import { TransitController } from './transit.controller'; import { TransitService } from './transit.service'; import { TransitMcp } from './transit.mcp'; -import { RateLimitService } from '../auth/rate-limit.service'; import { DaysModule } from '../days/days.module'; import { ReservationsModule } from '../reservations/reservations.module'; import { AuthModule } from '../auth/auth.module'; @@ -14,9 +14,9 @@ import { AuthModule } from '../auth/auth.module'; */ @Module({ // DaysModule + ReservationsModule: TransitMcp's create_transit_journey injects both. - imports: [DaysModule, ReservationsModule, AuthModule], + imports: [RateLimitModule, DaysModule, ReservationsModule, AuthModule], controllers: [TransitController], - providers: [TransitService, TransitMcp, RateLimitService], + providers: [TransitService, TransitMcp], exports: [TransitService], }) export class TransitModule {} diff --git a/server/src/nest/trip-invite/trip-invite.controller.ts b/server/src/nest/trip-invite/trip-invite.controller.ts index 808d2a648..ee3269493 100644 --- a/server/src/nest/trip-invite/trip-invite.controller.ts +++ b/server/src/nest/trip-invite/trip-invite.controller.ts @@ -4,7 +4,7 @@ import type { User } from '../../types'; import { TripInviteService } from './trip-invite.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; -import { RateLimitService } from '../auth/rate-limit.service'; +import { RateLimitService } from '../common/rate-limit.service'; import { TripInviteLinkCreateDto } from './trip-invite.dto'; import { getClientIp } from '../audit/client-ip'; import { AuditService } from '../audit/audit.service'; diff --git a/server/src/nest/trip-invite/trip-invite.module.ts b/server/src/nest/trip-invite/trip-invite.module.ts index cf170b936..502ef80e3 100644 --- a/server/src/nest/trip-invite/trip-invite.module.ts +++ b/server/src/nest/trip-invite/trip-invite.module.ts @@ -1,13 +1,13 @@ +import { RateLimitModule } from '../common/rate-limit.module'; import { Module } from '@nestjs/common'; import { TripInviteLinkController, TripInviteController } from './trip-invite.controller'; import { TripInviteService } from './trip-invite.service'; -import { RateLimitService } from '../auth/rate-limit.service'; import { PermissionsModule } from '../permissions/permissions.module'; import { AuditModule } from '../audit/audit.module'; @Module({ - imports: [PermissionsModule, AuditModule], + imports: [RateLimitModule, PermissionsModule, AuditModule], controllers: [TripInviteLinkController, TripInviteController], - providers: [TripInviteService, RateLimitService], + providers: [TripInviteService], }) export class TripInviteModule {} diff --git a/server/tests/helpers/test-db.ts b/server/tests/helpers/test-db.ts index 37c48285f..8262f8029 100644 --- a/server/tests/helpers/test-db.ts +++ b/server/tests/helpers/test-db.ts @@ -21,7 +21,7 @@ import type { INestApplication } from '@nestjs/common'; import { createTables } from '../../src/db/schema'; import { runMigrations } from '../../src/db/migrations'; import { AuthPublicController } from '../../src/nest/auth/auth-public.controller'; -import type { RateLimitService } from '../../src/nest/auth/rate-limit.service'; +import type { RateLimitService } from '../../src/nest/common/rate-limit.service'; // Tables to clear on reset, child-before-parent to be safe (FK checks are OFF during reset). // Keep in sync with schema.ts + migrations.ts. Intentionally excluded: categories, addons, diff --git a/server/tests/unit/nest/auth-guard.test.ts b/server/tests/unit/nest/auth-guard.test.ts index 9ff9a4f8f..372971cb8 100644 --- a/server/tests/unit/nest/auth-guard.test.ts +++ b/server/tests/unit/nest/auth-guard.test.ts @@ -13,7 +13,7 @@ import { OptionalJwtGuard } from '../../../src/nest/auth/optional-jwt.guard'; import { AdminGuard } from '../../../src/nest/auth/admin.guard'; import { PasskeyEnabledGuard } from '../../../src/nest/auth/passkey-enabled.guard'; import { PasskeyController } from '../../../src/nest/auth/passkey.controller'; -import { RateLimitService } from '../../../src/nest/auth/rate-limit.service'; +import { RateLimitService } from '../../../src/nest/common/rate-limit.service'; // AuditService is constructor-injected since the auditLog DI migration; the // wrapper keeps the historical construction sites positional. diff --git a/server/tests/unit/nest/auth.controller.test.ts b/server/tests/unit/nest/auth.controller.test.ts index d81150e06..467922802 100644 --- a/server/tests/unit/nest/auth.controller.test.ts +++ b/server/tests/unit/nest/auth.controller.test.ts @@ -8,7 +8,7 @@ vi.mock('../../../src/services/demo', () => ({ isDemoEmail: vi.fn(() => false) } import { AuthPublicController } from '../../../src/nest/auth/auth-public.controller'; import { AuthController } from '../../../src/nest/auth/auth.controller'; -import { RateLimitService } from '../../../src/nest/auth/rate-limit.service'; +import { RateLimitService } from '../../../src/nest/common/rate-limit.service'; import type { AuthService } from '../../../src/nest/auth/auth.service'; import type { AuditService } from '../../../src/nest/audit/audit.service'; import { isDemoEmail } from '../../../src/services/demo'; diff --git a/server/tests/unit/nest/oauth.controller.test.ts b/server/tests/unit/nest/oauth.controller.test.ts index dd2beec30..f01ae1520 100644 --- a/server/tests/unit/nest/oauth.controller.test.ts +++ b/server/tests/unit/nest/oauth.controller.test.ts @@ -10,7 +10,7 @@ const getClientIpMock = vi.mocked(getClientIp); import { OauthPublicController } from '../../../src/nest/oauth/oauth-public.controller'; import { OauthApiController } from '../../../src/nest/oauth/oauth-api.controller'; -import { RateLimitService } from '../../../src/nest/auth/rate-limit.service'; +import { RateLimitService } from '../../../src/nest/common/rate-limit.service'; import type { OauthService } from '../../../src/nest/oauth/oauth.service'; import type { User } from '../../../src/types'; diff --git a/server/tests/unit/nest/oauth.service.test.ts b/server/tests/unit/nest/oauth.service.test.ts index fe7c13cb5..b2c89b638 100644 --- a/server/tests/unit/nest/oauth.service.test.ts +++ b/server/tests/unit/nest/oauth.service.test.ts @@ -166,11 +166,12 @@ describe('OauthModule', () => { const { OauthPublicController } = await import('../../../src/nest/oauth/oauth-public.controller'); const { OauthApiController } = await import('../../../src/nest/oauth/oauth-api.controller'); const { OauthService: Svc } = await import('../../../src/nest/oauth/oauth.service'); - const { RateLimitService } = await import('../../../src/nest/auth/rate-limit.service'); const controllers = Reflect.getMetadata('controllers', OauthModule); const providers = Reflect.getMetadata('providers', OauthModule); expect(controllers).toEqual([OauthPublicController, OauthApiController]); - expect(providers).toEqual([Svc, RateLimitService]); + // RateLimitService is deliberately absent: it comes from the global + // RateLimitModule so all consumers share one set of counters. + expect(providers).toEqual([Svc]); }); }); diff --git a/server/tests/websocket/wire-envelope.test.ts b/server/tests/websocket/wire-envelope.test.ts new file mode 100644 index 000000000..09711dfbe --- /dev/null +++ b/server/tests/websocket/wire-envelope.test.ts @@ -0,0 +1,261 @@ +/** + * WebSocket wire-envelope tests. Covers WS-030 to WS-036. + * + * These pin the *shape* of every frame that crosses the socket, not the behaviour + * behind it — connection.test.ts already covers the behaviour. + * + * The reason they exist: the plan is to move this transport onto a Nest gateway + * (@nestjs/websockets). Nest's stock WsAdapter dispatches on `message.event` and + * answers `{ event, data }`, while TREK speaks a flat `{ type, tripId, ...payload }` + * of its own. Swapping the adapter in without a custom serialiser would leave every + * `join` silently unhandled: no `welcome.socketId` to echo back as X-Socket-Id, so + * every client would receive its own writes back and drag-and-drop would jump. + * + * None of that shows up in the existing suite, because the 100+ tests that touch + * realtime mock `src/websocket` and never look at the bytes. So: if a change makes + * these fail, it is a breaking change for every deployed client. + */ +import { describe, it, expect, vi, beforeAll, afterAll, beforeEach } from 'vitest'; +import http from 'http'; +import WebSocket from 'ws'; + +const { testDb, dbMock } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA foreign_keys = ON'); + db.exec('PRAGMA busy_timeout = 5000'); + const mock = { + db, + closeDb: () => {}, + reinitialize: () => {}, + getPlaceWithTags: () => null, + canAccessTrip: (tripId: any, userId: number) => + db.prepare(`SELECT t.id, t.user_id FROM trips t LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL)`).get(userId, tripId, userId), + isOwner: (tripId: any, userId: number) => + !!db.prepare('SELECT id FROM trips WHERE id = ? AND user_id = ?').get(tripId, userId), + }; + return { testDb: db, dbMock: mock }; +}); + +vi.mock('../../src/db/database', () => dbMock); +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', +})); + +import type { INestApplication } from '@nestjs/common'; +import { buildApp } from '../../src/bootstrap'; +import { createTables } from '../../src/db/schema'; +import { runMigrations } from '../../src/db/migrations'; +import { resetTestDb } from '../helpers/test-db'; +import { createUser, createTrip } from '../helpers/factories'; +import { setupWebSocket, broadcast, broadcastToUser } from '../../src/websocket'; +import { createEphemeralToken } from '../../src/services/ephemeralTokens'; + +let server: http.Server; +let wsUrl: string; +let nestApp: INestApplication; + +beforeAll(async () => { + createTables(testDb); + runMigrations(testDb); + nestApp = await buildApp(); + server = http.createServer(nestApp.getHttpAdapter().getInstance()); + setupWebSocket(server); + await new Promise(resolve => server.listen(0, resolve)); + const addr = server.address() as { port: number }; + wsUrl = `ws://127.0.0.1:${addr.port}/ws`; +}); + +afterAll(async () => { + await new Promise((resolve, reject) => server.close(err => err ? reject(err) : resolve())); + await nestApp.close(); + testDb.close(); +}); + +beforeEach(() => resetTestDb(testDb)); + +/** Minimal buffered client — same idea as connection.test.ts, kept local. */ +class Frames { + private buffer: any[] = []; + private waiters: Array<(m: any) => void> = []; + constructor(private ws: WebSocket) { + ws.on('message', (data) => { + const msg = JSON.parse(data.toString()); + const waiter = this.waiters.shift(); + if (waiter) waiter(msg); else this.buffer.push(msg); + }); + } + next(timeoutMs = 3000): Promise { + if (this.buffer.length) return Promise.resolve(this.buffer.shift()); + return new Promise((resolve, reject) => { + const waiter = (m: any) => { clearTimeout(timer); resolve(m); }; + // Drop the waiter on timeout, otherwise it stays queued and swallows the + // next frame — which matters here because nextOrNull() times out by design. + const timer = setTimeout(() => { + const idx = this.waiters.indexOf(waiter); + if (idx !== -1) this.waiters.splice(idx, 1); + reject(new Error('Message timeout')); + }, timeoutMs); + this.waiters.push(waiter); + }); + } + /** Resolves to null when nothing arrives — used to assert silence. */ + async nextOrNull(timeoutMs = 400): Promise { + try { return await this.next(timeoutMs); } catch { return null; } + } + send(msg: object) { this.ws.send(JSON.stringify(msg)); } + close() { this.ws.close(); } +} + +function connect(token: string): Promise { + return new Promise((resolve, reject) => { + const ws = new WebSocket(`${wsUrl}?token=${encodeURIComponent(token)}`); + const frames = new Frames(ws); + ws.once('open', () => resolve(frames)); + ws.once('error', reject); + }); +} + +async function connectAndJoin(userId: number, tripId: number) { + const frames = await connect(createEphemeralToken(userId, 'ws')!); + await frames.next(); // welcome + frames.send({ type: 'join', tripId }); + await frames.next(); // joined + return frames; +} + +describe('WS wire envelope', () => { + it('WS-030 — the welcome frame carries socketId at the top level', async () => { + const { user } = createUser(testDb); + const frames = await connect(createEphemeralToken(user.id, 'ws')!); + try { + const msg = await frames.next(); + expect(Object.keys(msg).sort()).toEqual(['socketId', 'type']); + expect(msg.type).toBe('welcome'); + expect(typeof msg.socketId).toBe('number'); + // X-Socket-Id is read straight off this field (client/src/api/websocket.ts). + // Nesting it under `data` would silently disable echo suppression. + expect(msg).not.toHaveProperty('data'); + expect(msg).not.toHaveProperty('event'); + } finally { + frames.close(); + } + }); + + it('WS-031 — join and leave are answered flat, keyed by type', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const frames = await connect(createEphemeralToken(user.id, 'ws')!); + try { + await frames.next(); + frames.send({ type: 'join', tripId: trip.id }); + const joined = await frames.next(); + expect(Object.keys(joined).sort()).toEqual(['tripId', 'type']); + expect(joined).toEqual({ type: 'joined', tripId: trip.id }); + + frames.send({ type: 'leave', tripId: trip.id }); + const left = await frames.next(); + expect(left).toEqual({ type: 'left', tripId: trip.id }); + } finally { + frames.close(); + } + }); + + it('WS-032 — the server dispatches on `type`, not on `event`', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const frames = await connect(createEphemeralToken(user.id, 'ws')!); + try { + await frames.next(); + // This is the exact shape Nest's stock WsAdapter would send. It must not + // join the room — if it ever does, the two protocols have been mixed. + frames.send({ event: 'join', data: { tripId: trip.id } }); + expect(await frames.nextOrNull()).toBeNull(); + + // And the real shape still works afterwards. + frames.send({ type: 'join', tripId: trip.id }); + expect((await frames.next()).type).toBe('joined'); + } finally { + frames.close(); + } + }); + + it('WS-033 — a trip broadcast spreads its payload flat next to type and tripId', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const frames = await connectAndJoin(user.id, trip.id); + try { + broadcast(trip.id, 'place_created', { place: { id: 7 }, source: 'test' }); + const msg = await frames.next(); + expect(msg).toEqual({ type: 'place_created', tripId: trip.id, place: { id: 7 }, source: 'test' }); + // Not { event, data } — the payload keys sit at the top level. + expect(msg).not.toHaveProperty('data'); + expect(msg).not.toHaveProperty('event'); + expect(msg).not.toHaveProperty('payload'); + } finally { + frames.close(); + } + }); + + it('WS-034 — a payload key cannot be shadowed by the envelope', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const frames = await connectAndJoin(user.id, trip.id); + try { + // The spread runs last, so payload wins over the envelope's tripId. Pinning + // this because it decides what a re-implementation has to reproduce. + broadcast(trip.id, 'day_updated', { tripId: 999, day: { id: 1 } }); + const msg = await frames.next(); + expect(msg.tripId).toBe(999); + expect(msg.type).toBe('day_updated'); + } finally { + frames.close(); + } + }); + + it('WS-035 — a user broadcast is the payload verbatim, with no tripId added', async () => { + const { user } = createUser(testDb); + const frames = await connect(createEphemeralToken(user.id, 'ws')!); + try { + await frames.next(); + broadcastToUser(user.id, { type: 'trip_invitation', invite: { id: 3 } }); + const msg = await frames.next(); + expect(msg).toEqual({ type: 'trip_invitation', invite: { id: 3 } }); + expect(msg).not.toHaveProperty('tripId'); + } finally { + frames.close(); + } + }); + + it('WS-036 — the sender is excluded by socket id, not by connection identity', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const a = await connect(createEphemeralToken(user.id, 'ws')!); + const b = await connect(createEphemeralToken(user.id, 'ws')!); + try { + const aWelcome = await a.next(); + await b.next(); + a.send({ type: 'join', tripId: trip.id }); + await a.next(); + b.send({ type: 'join', tripId: trip.id }); + await b.next(); + + // Same user, two sockets: the one named by X-Socket-Id stays silent, the + // other still gets the event. That is what stops a client echoing its own + // writes while a second tab keeps updating. + broadcast(trip.id, 'place_created', { place: { id: 1 } }, aWelcome.socketId); + expect(await a.nextOrNull()).toBeNull(); + expect((await b.next()).type).toBe('place_created'); + } finally { + a.close(); + b.close(); + } + }); +});