diff --git a/server/src/bootstrap.ts b/server/src/bootstrap.ts index 66690d8b1..4a40200e6 100644 --- a/server/src/bootstrap.ts +++ b/server/src/bootstrap.ts @@ -91,7 +91,7 @@ export async function buildApp(): Promise { // boot succeeds, the gateway logs as registered, every test passes, and no // browser can connect. Callers take the server from getHttpServer() below. boundHttpServer = nodeHttp.createServer(instance); - app.useWebSocketAdapter(new TrekWsAdapter(boundHttpServer)); + app.useWebSocketAdapter(new TrekWsAdapter(boundHttpServer, orm)); // ConfigModule.forRoot's load factories already ran inside NestFactory.create, // so the boot-stable snapshot is resolvable here, BEFORE app.init() — this is // the one bridge that lets the pre-init Express layer consume the validated diff --git a/server/src/demo/demo-seed.ts b/server/src/demo/demo-seed.ts index 13ded7430..dad7d4365 100644 --- a/server/src/demo/demo-seed.ts +++ b/server/src/demo/demo-seed.ts @@ -6,6 +6,12 @@ import { DEMO_PASS } from '../nest/common/demo'; // touches the database happens inside the functions. import { saveBaseline, hasBaseline } from './demo-reset'; +// D6 (task-2-review.md's controller ruling / non-HTTP caller table): this runs +// from `runSchemaBootstrap` before `app.init()`, outside any request context, but +// it is all raw `better-sqlite3` through the `db` param passed in — no +// InjectRepository/global-EM read — so there is nothing to wrap yet. The domain +// phase that gives demo seeding a repository read must wrap it in +// withRequestContext then. function seedDemoData(db: Database.Database): { adminId: number; demoId: number } { const ADMIN_USER = readEnv().demo.adminUser; const ADMIN_EMAIL = readEnv().demo.adminEmailRaw || 'admin@trek.app'; diff --git a/server/src/nest-mcp/registry.ts b/server/src/nest-mcp/registry.ts index 5717d9d60..1243cc3f4 100644 --- a/server/src/nest-mcp/registry.ts +++ b/server/src/nest-mcp/registry.ts @@ -283,6 +283,15 @@ export class McpRegistry { return this.accessPolicy(access, ctx); } + // D6: this callback is invoked from inside McpTransportController's @Post/@Get/ + // @Delete handlers (mcp-transport.controller.ts), which are ordinary Nest routes — + // `@Public()` only exempts the auth guard, and the raw-body parser exemption in + // bootstrap.ts only exempts Express's JSON/urlencoded parsers. Neither opts /mcp out + // of `@mikro-orm/nestjs`'s per-request middleware (registerRequestContext, on by + // default and not overridden in mikro-orm.config.ts), which forks the EntityManager + // before any controller method runs. So a tool handler here already runs inside a + // request context; do not wrap it in withRequestContext (task-2-review.md's + // non-HTTP caller table confirms this path is already covered). private attachTool( registrar: LooseRegistrar, options: ToolOptions, diff --git a/server/src/nest/admin/demo-reset.job.ts b/server/src/nest/admin/demo-reset.job.ts index 333a205ab..847fd518d 100644 --- a/server/src/nest/admin/demo-reset.job.ts +++ b/server/src/nest/admin/demo-reset.job.ts @@ -21,6 +21,9 @@ export class DemoResetJob implements OnApplicationBootstrap { onApplicationBootstrap(): void { if (!this.registrar.isEnabled() || !this.runtimeEnv.isDemoMode()) return; + // D6: resetDemoUser is all raw better-sqlite3 (see demo-reset.ts) — no + // repository read today — but the tick already runs inside a request context + // regardless, via CronRegistrarService's one wrapper around every onTick. this.registrar.register('demo-reset', '0 * * * *', () => void this.tick(), { timezone: 'none' }); logInfo('Demo hourly reset scheduled'); } diff --git a/server/src/nest/backup/backup.impl.ts b/server/src/nest/backup/backup.impl.ts index 727b62b5c..94371f472 100644 --- a/server/src/nest/backup/backup.impl.ts +++ b/server/src/nest/backup/backup.impl.ts @@ -507,6 +507,11 @@ export async function restoreFromZip(storage: StorageService, zipPath: string): // still holds the pre-restore state. Any request using a cached // permission would decide against the wrong grants until the // next restart. Dropping the cache forces a fresh read. + // D6: no repository read happens on this path today — invalidation is a + // plain function, not a DB call — so no withRequestContext is owed here yet. + // The domain phase that gives this restore path a repository read (Plan 3's + // admin/backup cluster) must wrap it then; see task-2-review.md's non-HTTP + // caller table. invalidatePermissionsCache(); } @@ -573,6 +578,8 @@ export async function restoreFromZip(storage: StorageService, zipPath: string): // stale anyway. Invalidating here too costs nothing and guarantees // we never serve cached permissions that don't match the DB state // we leave the process in after a failed restore. + // D6: same no-repository-read note as the other invalidatePermissionsCache() + // call above — nothing to wrap yet. try { invalidatePermissionsCache(); } catch { /* best-effort */ } throw err; } diff --git a/server/src/nest/common/idempotency-cleanup.job.ts b/server/src/nest/common/idempotency-cleanup.job.ts index 612098d27..6bac3a95d 100644 --- a/server/src/nest/common/idempotency-cleanup.job.ts +++ b/server/src/nest/common/idempotency-cleanup.job.ts @@ -18,6 +18,11 @@ export class IdempotencyCleanupJob implements OnApplicationBootstrap { onApplicationBootstrap(): void { if (!this.registrar.isEnabled()) return; + // D6: `tick()` still reads/writes through raw `DatabaseService.prepare` (see + // `purgeExpiredIdempotencyKeys`), but the tick itself already runs inside a + // request context — CronRegistrarService.register wraps every job's onTick in + // one place, so this needs no wrapper of its own even once it gains a + // repository read. this.registrar.register('idempotency-cleanup', '0 3 * * *', () => this.tick()); } diff --git a/server/src/nest/mcp-shared/mcp-tool-guards.service.ts b/server/src/nest/mcp-shared/mcp-tool-guards.service.ts index f4d585261..87d7e66c7 100644 --- a/server/src/nest/mcp-shared/mcp-tool-guards.service.ts +++ b/server/src/nest/mcp-shared/mcp-tool-guards.service.ts @@ -5,7 +5,8 @@ import { RealtimeService } from '../realtime/realtime.service'; /** * The impure MCP tool guards that used to live as plain functions in - * src/mcp/tools/_shared.ts, reaching the db Proxy, permissions.bridge and the + * src/mcp/tools/_shared.ts, reaching the db Proxy, a permissions.bridge + * instance (no longer exists — see auth.service.ts:116) and the * src/websocket stub as module globals. The @McpController domain classes are * ordinary Nest providers, so they inject this instead. The pure result * helpers (noAccess/permissionDenied/adminRequired and the diff --git a/server/src/nest/permissions/permissions-cache.ts b/server/src/nest/permissions/permissions-cache.ts index 6557b6a09..40e756b57 100644 --- a/server/src/nest/permissions/permissions-cache.ts +++ b/server/src/nest/permissions/permissions-cache.ts @@ -3,12 +3,13 @@ import type { PermissionLevel } from './permissions.service'; /** * The permissions cache — module state on purpose, in its own home (the * oauth.pending-codes.ts precedent) so every path that must see one shared - * cache actually does: the container PermissionsService singleton, the - * permissions.bridge instance the MCP _shared helper reaches, and the plain - * backup restore path (backup.impl.ts), which is free functions by design and - * has nothing to hang an injected service off. A restore rewrites - * app_settings under the request path's feet; invalidating here is what keeps - * every reader consistent afterwards. + * cache actually does: the container PermissionsService singleton (the MCP + * _shared helper reaches it through injection, not a separate bridge + * instance — permissions.bridge no longer exists, see auth.service.ts:116), + * and the plain backup restore path (backup.impl.ts), which is free + * functions by design and has nothing to hang an injected service off. A + * restore rewrites app_settings under the request path's feet; invalidating + * here is what keeps every reader consistent afterwards. */ let cache: Map | null = null; diff --git a/server/src/nest/permissions/permissions.service.ts b/server/src/nest/permissions/permissions.service.ts index 763b51f2a..e870d28ff 100644 --- a/server/src/nest/permissions/permissions.service.ts +++ b/server/src/nest/permissions/permissions.service.ts @@ -1,5 +1,6 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; +import { ValidationError } from '@mikro-orm/core'; import { UnitOfWork } from '../database/unit-of-work'; import { logError } from '../audit/audit-log.logger'; import { AppSettings } from '../../db/entities/AppSettings.entity'; @@ -67,12 +68,13 @@ export const PERMISSION_ACTIONS: PermissionAction[] = [ const ACTIONS_MAP = new Map(PERMISSION_ACTIONS.map(a => [a.key, a])); // The in-memory cache is deliberately MODULE-scoped, not instance state, and -// lives in ./permissions-cache: two service instances exist at runtime (the -// container singleton and the permissions.bridge instance for -// out-of-container consumers), and the backup restore path — plain functions, -// no DI — must flush the same cache the request path reads. Both instances -// wrap the same shared connection, so a single cache is also the correct -// data shape. +// lives in ./permissions-cache: the container's PermissionsService singleton +// is not the only reader — the backup restore path (backup.impl.ts) is plain +// functions, no DI, and must flush the same cache the request path reads. +// Both reach the same shared connection, so a single cache is also the +// correct data shape. (permissions.bridge, once a second out-of-container +// instance, was replaced by injection — see auth.service.ts:116 — and no +// longer exists.) @Injectable() export class PermissionsService { @@ -88,6 +90,10 @@ export class PermissionsService { try { const rows = await this.appSettings.findByKeyPrefix('perm_'); for (const row of rows) { + // A typing artefact, not a parity change: AppSettingsRow types both + // columns nullable, but `key` can never actually be null here (a NULL + // key never matches `LIKE 'perm_%'`), and skipping a null `value` is + // exactly the legacy `allowedLevels.includes(null) === false` skip. if (row.key == null || row.value == null) continue; const actionKey = row.key.replace('perm_', ''); const action = ACTIONS_MAP.get(actionKey); @@ -100,6 +106,16 @@ export class PermissionsService { } } } catch (e) { + // A MikroORM ValidationError here (typically cannotUseGlobalContext) is not + // a DB failure — it means this call reached the repository outside any + // request context (D6: a non-HTTP entrypoint that isn't wrapped in + // withRequestContext, or a bug in one that is). Serving defaults for that + // is fail-OPEN: an admin who tightened a flag from its default gets the + // looser default back, silently, for as long as the misuse persists + // (task-2-review.md C3). A programming error rethrows instead of degrading; + // the same rule applies to any later "log and serve defaults" branch this + // plan adds (AddonsService's flag reads are Task 4's, not touched here). + if (e instanceof ValidationError) throw e; // Under the legacy better-sqlite3 path a "no such table" error meant // first-boot init racing this read, before the table existed, and was // swallowed silently. Under the ORM, migrations run to completion in @@ -107,6 +123,10 @@ export class PermissionsService { // this service — there is no window left where app_settings can be // missing — so that message match is dropped and every failure here is // now a real, loggable DB error; defaults are still served either way. + // One residual window this doesn't cover: backup.impl.ts's restore swaps + // the database file under a live process, so a concurrent load can still + // see a missing/mid-swap table and will now log here — behaviour + // (defaults, uncached) is unchanged, so it is log noise only. const msg = e instanceof Error ? e.message : String(e); logError(`Permissions load failed: ${msg}`); // Serve defaults for THIS call, but do not install them: a half-built diff --git a/server/src/nest/plugins/plugin-runtime.service.ts b/server/src/nest/plugins/plugin-runtime.service.ts index 97ff27a45..5b26bb6a1 100644 --- a/server/src/nest/plugins/plugin-runtime.service.ts +++ b/server/src/nest/plugins/plugin-runtime.service.ts @@ -1,5 +1,6 @@ import { Injectable, type OnApplicationBootstrap, type OnModuleDestroy } from '@nestjs/common'; import semver from 'semver'; +import { MikroORM } from '@mikro-orm/core'; import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; import { pluginsEnabled } from './kill-switch'; @@ -130,7 +131,9 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // plugin's ctx.plugins.call / ctx.events.emit resolve through callPlugin/ // emitPluginEvent below (which own the dependency-edge authorization). The // arrow reads this.hostFactory lazily at spawn time, so the field-initializer - // ordering (it runs before the constructor params are assigned) is safe. + // ordering (it runs before the constructor params are assigned) is safe. `orm` + // (D6, task-2-review.md C3) needs the same laziness — it is read once per RPC + // dispatch, well after the constructor has run, not at field-init time. private readonly supervisor = new PluginSupervisor((id, granted) => { if (!this.hostFactory) throw new Error('PluginRpcHostFactory not provided — tests that activate plugins must pass one'); return this.hostFactory.create(id, granted, this); @@ -158,7 +161,7 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes void this.pruneErrorLog(id).catch(() => { /* retention is best-effort */ }); } catch { /* DB unavailable — a log line must never crash the host */ } }, - }); + }, {}, () => this.orm); // Filesystem watchers for dev-linked plugins (id -> watcher), so a rebuild of the // author's source auto-reloads. Empty unless dev-link is used. @@ -188,6 +191,12 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // test instance that exercises a transaction must pass one; setOperatorEgressHosts // refuses rather than writing outside a transaction (same shape as hostFactory). private readonly uow?: UnitOfWork, + // D6 (task-2-review.md C3): the supervisor wraps every plugin RPC dispatch in a + // request context built from this. Nest always injects it (MikroOrmCoreModule is + // global); a hand-built test instance that dispatches through a real + // PermissionsService/repository must pass one or those reads fail closed with + // cannotUseGlobalContext instead of running. + private readonly orm?: MikroORM, ) {} private get db() { diff --git a/server/src/nest/plugins/supervisor/plugin-supervisor.ts b/server/src/nest/plugins/supervisor/plugin-supervisor.ts index 673c88337..32a096935 100644 --- a/server/src/nest/plugins/supervisor/plugin-supervisor.ts +++ b/server/src/nest/plugins/supervisor/plugin-supervisor.ts @@ -1,6 +1,7 @@ import fs from 'node:fs'; import { randomUUID } from 'node:crypto'; import { fork, type ChildProcess } from 'node:child_process'; +import type { EntityManager } from '@mikro-orm/core'; import { readEnv } from '../../../app-config'; import { resolveChildEntry, pluginCodeDir, pluginRealCodeDir, pluginPermissionArgs, ensurePluginModuleType } from '../paths'; import { HOOK_PERMISSION, USER_DATA_PERMISSION, EVENTS_PERMISSION, type Envelope, type RpcError, type RpcRequest } from '../protocol/envelope'; @@ -8,6 +9,7 @@ import type { PluginRpcHost } from '../host/rpc-host'; import { scheduleJobs, stopJobs, type ScheduledJob } from '../host/plugin-jobs'; import { SNAPSHOT_GRANT, type PluginEventMeta } from '../../../plugin-event-sink'; import { RpcRateLimiter, DEFAULT_RPC_LIMIT, TokenBucket, DEFAULT_LOG_LIMIT } from '../host/rate-limit'; +import { withRequestContext } from '../../database/request-context'; export interface PluginRouteInfo { i: number; @@ -113,6 +115,16 @@ export class PluginSupervisor { private readonly createRpcHost: (id: string, granted: ReadonlySet) => PluginRpcHost, private readonly hooks: SupervisorHooks = {}, tuning: SupervisorTuning = {}, + // D6: every `ctx.*` call from a plugin child arrives over IPC, not an HTTP + // request, so nothing has forked an EntityManager for it. A THUNK, not a value: + // PluginRuntimeService's own `orm` field is not assigned yet when its `supervisor` + // field initializer runs (constructor params are assigned after field + // initializers — see that file's comment), so this is read lazily, once per + // dispatch. Optional only so the hand-built doubles in + // supervisor-lifecycle.test.ts / provider-hook-grant.test.ts / + // event-subscriptions.test.ts (none of which reach a repository) don't need one; + // PluginRuntimeService always passes the real MikroORM (OrmModule is global). + private readonly resolveOrm?: () => { em: EntityManager } | undefined, ) { this.tuning = { ...DEFAULTS, ...tuning }; } @@ -550,7 +562,16 @@ export class PluginSupervisor { const inv = req.params as { _inv?: unknown } | undefined; const actingUserId = typeof inv?._inv === 'string' ? sup.invocations.get(inv._inv) : undefined; try { - const res = await sup.rpcHost.dispatch(req, actingUserId); + // The single choke point every plugin RPC passes through (D6): wrap it here, + // not inside rpc-host.ts::dispatch, so the context also spans plugin-guards' + // trip/permission reads and any *.rpc.ts handler's repository calls, not just + // dispatch's own audit write. Without this, PermissionsService.checkPermission + // ran outside any context, threw on a cold cache, and the catch served + // defaults — fail-open on a tightened flag (task-2-review.md C3). + const orm = this.resolveOrm?.(); + const res = orm + ? await withRequestContext(orm, () => sup.rpcHost.dispatch(req, actingUserId)) + : await sup.rpcHost.dispatch(req, actingUserId); sup.child?.send(res); } finally { sup.rpcLimiter.release(); diff --git a/server/src/nest/realtime/trek-ws.adapter.ts b/server/src/nest/realtime/trek-ws.adapter.ts index ff9a29cf3..5899b5f1e 100644 --- a/server/src/nest/realtime/trek-ws.adapter.ts +++ b/server/src/nest/realtime/trek-ws.adapter.ts @@ -4,9 +4,11 @@ import type { Server as HttpServer } from 'node:http'; import type { MessageMappingProperties } from '@nestjs/websockets'; import { WebSocketServer } from 'ws'; import type { Observable } from 'rxjs'; +import type { EntityManager } from '@mikro-orm/core'; import { readEnv } from '../../app-config'; import { setServer, type TrekWebSocket } from './ws-state'; import { logError } from '../audit/audit-log.logger'; +import { withRequestContext } from '../database/request-context'; // Per-connection message rate limiting. It lives in the adapter, not in the // gateway's handlers, because the original counted EVERY inbound frame before @@ -98,7 +100,15 @@ export class TrekWsAdapter extends WsAdapter { * server lands on the socket this process actually listens on. Given the app, * it would reach for Nest's own internal server, which buildApp never uses. */ - constructor(httpServer: HttpServer) { + constructor( + httpServer: HttpServer, + // D6 (task-2-review.md's controller ruling): a WS message handler has no HTTP + // request behind it, so every `@SubscribeMessage` dispatch is wrapped here — + // ONE wrapper at bindMessageHandlers below, not one per handler. Optional only + // for trek-ws-adapter.test.ts, which never reaches a repository; bootstrap.ts + // always passes the real MikroORM. + private readonly orm?: { em: EntityManager }, + ) { super(httpServer as unknown as INestApplicationContext); this.httpServerRef = httpServer; } @@ -203,7 +213,16 @@ export class TrekWsAdapter extends WsAdapter { // The whole message is the payload: TREK's frames are flat, so `tripId` // sits beside `type` rather than under a `data` key. - transform(handler.callback(message, socket)).subscribe({ + // + // The context has to wrap the CALL, not just the subscribe(): an async + // handler's body runs synchronously up to its first await the instant it is + // invoked (before transform()/subscribe() ever run), and RequestContext.create + // is AsyncLocalStorage.run — it only covers what executes inside this + // synchronous frame, after which the async chain carries it on its own. + const result = this.orm + ? withRequestContext(this.orm, () => handler.callback(message, socket)) + : handler.callback(message, socket); + transform(result).subscribe({ next: (response) => { if (response !== undefined && socket.readyState === 1) { socket.send(JSON.stringify(response)); diff --git a/server/src/nest/scheduling/cron-registrar.service.ts b/server/src/nest/scheduling/cron-registrar.service.ts index c11b910c7..0b18e59cd 100644 --- a/server/src/nest/scheduling/cron-registrar.service.ts +++ b/server/src/nest/scheduling/cron-registrar.service.ts @@ -1,8 +1,10 @@ -import { Injectable, type OnApplicationShutdown } from '@nestjs/common'; +import { Injectable, Optional, type OnApplicationShutdown } from '@nestjs/common'; import { SchedulerRegistry } from '@nestjs/schedule'; import { CronJob } from 'cron'; +import { MikroORM } from '@mikro-orm/core'; import { readEnv } from '../../app-config'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; +import { withRequestContext } from '../database/request-context'; /** * The one way TREK code schedules a cron. Job providers register here from @@ -35,6 +37,18 @@ export class CronRegistrarService implements OnApplicationShutdown { constructor( private readonly registry: SchedulerRegistry, private readonly runtimeEnv: RuntimeEnvService, + // D6 (task-2-review.md's controller ruling): a cron tick has no HTTP request + // behind it, so every job's onTick is wrapped in a request context here — the + // ONE place, rather than in each of the 14 `*.job.ts` providers. `@Optional()` + // (not just a `?`, which is TS-only and does not tell Nest's DI the provider + // may be absent) because SchedulingModule is deliberately NOT @Global (see + // this module's own docstring): the e2e suites boot partial graphs — one + // domain module + SchedulingModule, no MikroOrmModule.forRoot — so a hard + // MikroORM dependency here would fail every one of them at compile(), the + // same class of break `@InjectRepository` would cause. Production (buildApp) + // always has it; a hand-built test instance that registers a job touching a + // repository must pass one, same as the unit test doubles below. + @Optional() private readonly orm?: MikroORM, ) {} /** False under NODE_ENV=test — the single gate keeping the suites timer-free. */ @@ -56,7 +70,9 @@ export class CronRegistrarService implements OnApplicationShutdown { this.unregister(name); if (!this.isEnabled()) return false; const timeZone = opts?.timezone === 'none' ? undefined : readEnv().app.tz || 'UTC'; - const job = CronJob.from({ cronTime: expression, onTick, start: true, timeZone }); + const orm = this.orm; + const wrappedTick = orm ? () => withRequestContext(orm, () => onTick()) : onTick; + const job = CronJob.from({ cronTime: expression, onTick: wrappedTick, start: true, timeZone }); this.registry.addCronJob(name, job); this.names.add(name); return true; diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 1b09102d3..e9a82e282 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -72,7 +72,7 @@ import { PlacePhotoCacheService } from '../../src/nest/place-photos/place-photo- import { TrekPhotosRepository } from '../../src/nest/photos/trek-photos.repository'; import { RuntimeEnvService } from '../../src/nest/app-config/runtime-env.service'; import { makeStorageFixture } from './storage-fixture'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from './test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, sharedTestOrm } from './test-uow'; import { createTestOrm } from './test-orm'; import { AppSettings } from '../../src/db/entities/AppSettings.entity'; import type { AppSettingsRepository } from '../../src/db/repositories/AppSettings.repository'; @@ -159,14 +159,22 @@ export async function createPluginRpcHostFactory(dbs: DatabaseService): Promise< /** A PluginRuntimeService constructed the way Nest would: with a real host factory. */ export async function createPluginRuntime(dbs: DatabaseService, registry?: PluginRegistryService): Promise { - const orm = await createTestOrm(dbs.connection); + const auditOrm = await createTestOrm(dbs.connection); + // The SAME shared ORM `createPluginRpcHostFactory` built `permissions` from + // (via createTestAppSettingsRepo/createTestUnitOfWork) — not auditOrm above, + // which is its own separate MikroORM.init just for AuditService. Passing THIS + // one as PluginRuntimeService's last arg is what makes D6's request-context + // wrapper (task-2-review.md C3) fork from the em PermissionsService's + // repository actually resolves through. + const orm = await sharedTestOrm(dbs.connection); return new PluginRuntimeService( dbs, - new AuditService(orm.repo(AuditLog) as AuditLogRepository, orm.repo(Users) as UsersRepository), + new AuditService(auditOrm.repo(AuditLog) as AuditLogRepository, auditOrm.repo(Users) as UsersRepository), new AddonsService(dbs), new PluginUserSettingsService(dbs), registry, await createPluginRpcHostFactory(dbs), await createTestUnitOfWork(dbs.connection), + orm.orm, ); } diff --git a/server/tests/helpers/test-uow.ts b/server/tests/helpers/test-uow.ts index e0e231141..1c2dcaeb5 100644 --- a/server/tests/helpers/test-uow.ts +++ b/server/tests/helpers/test-uow.ts @@ -1,12 +1,40 @@ import { Module, type DynamicModule } from '@nestjs/common'; import type Database from 'better-sqlite3'; import { UnitOfWork } from '../../src/nest/database/unit-of-work'; -import { createTestOrm } from './test-orm'; +import { createTestOrm, type TestOrm } from './test-orm'; import { AppSettings } from '../../src/db/entities/AppSettings.entity'; import type { AppSettingsRepository } from '../../src/db/repositories/AppSettings.repository'; const perHandle = new WeakMap>(); const appSettingsPerHandle = new WeakMap>(); +// ONE MikroORM per handle, shared by createTestUnitOfWork and +// createTestAppSettingsRepo (task-2-review.md I2): each used to call +// createTestOrm(db) independently, which opened a SECOND MikroORM.init over the +// same better-sqlite3 handle — two identity maps, two Kysely clients, and +// (the functional bug) `UnitOfWork.transactional` opening its transaction on +// ORM A's EntityManager while a repository built from ORM B resolved a +// DIFFERENT EntityManager, so `TransactionContext` never saw it: a repository +// write inside `uow.transactional(...)` silently ran outside the transaction. +// Both functions below now derive from this single `t.em`, so a repository +// resolved from `t.repo(X)` and a `UnitOfWork` built from `t.em` share the same +// context-resolving EntityManager, exactly like Nest's real DI graph. +const ormPerHandle = new WeakMap>(); +/** + * Exported so a caller that needs the raw `MikroORM` instance itself — not a + * repository or a `UnitOfWork` derived from it — can still share it: e.g. + * `plugin-host.ts`'s `createPluginRuntime` passes `(await sharedTestOrm(db)).orm` + * to `PluginRuntimeService` so `PluginSupervisor`'s D6 request-context wrapper + * (task-2-review.md C3) forks from the SAME `em` `PermissionsService`'s + * repository resolves through — a wrapper built from a different ORM instance + * would fork a context a repository bound to this one never sees. + */ +export function sharedTestOrm(db: Database.Database): Promise { + const existing = ormPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = createTestOrm(db); + ormPerHandle.set(db, pending); + return pending; +} /** * The `UnitOfWork` a hand-constructed service needs, bound to the suite's own @@ -15,14 +43,16 @@ const appSettingsPerHandle = new WeakMap { // `=== undefined` rather than a truthiness test: a Promise is always truthy, // which is exactly what no-misused-promises/checksConditionals rejects. const existing = perHandle.get(db); if (existing !== undefined) return existing; - const pending = createTestOrm(db).then((t) => new UnitOfWork(t.em)); + const pending = sharedTestOrm(db).then((t) => new UnitOfWork(t.em)); perHandle.set(db, pending); return pending; } @@ -32,14 +62,15 @@ export function createTestUnitOfWork(db: Database.Database): Promise * mirroring `SettingsService`'s own `t.repo(AppSettings)` in its unit test), bound to * the suite's own better-sqlite3 handle the same way `createTestUnitOfWork` is. * - * Memoised per handle for the same reason: many suites build several services (and - * `PermissionsService` specifically, being 20+ domains' guard dependency) off the - * same test DB, and each would otherwise open its own `MikroORM.init`. + * Memoised per handle via the same `sharedTestOrm` `createTestUnitOfWork` uses — + * not a second `createTestOrm` call — so this repository and that `UnitOfWork` + * resolve through the identical `EntityManager` and a `uow.transactional(...)` + * write this repository makes is actually inside the open transaction. */ export function createTestAppSettingsRepo(db: Database.Database): Promise { const existing = appSettingsPerHandle.get(db); if (existing !== undefined) return existing; - const pending = createTestOrm(db).then((t) => t.repo(AppSettings) as AppSettingsRepository); + const pending = sharedTestOrm(db).then((t) => t.repo(AppSettings) as AppSettingsRepository); appSettingsPerHandle.set(db, pending); return pending; } diff --git a/server/tests/unit/mcp/tools-airtrail.test.ts b/server/tests/unit/mcp/tools-airtrail.test.ts index 9df4d4467..d6172724d 100644 --- a/server/tests/unit/mcp/tools-airtrail.test.ts +++ b/server/tests/unit/mcp/tools-airtrail.test.ts @@ -48,7 +48,6 @@ import { ADDON_IDS } from '../../../src/addons'; import { AirtrailClient, AirtrailRequestError, type AirtrailFlightRaw } from '../../../src/nest/integrations/airtrail.client'; import { AirtrailImportService } from '../../../src/nest/integrations/airtrail-import.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; // The permissions cache is module-scoped, so a write through any instance is diff --git a/server/tests/unit/mcp/tools-roadtrip.test.ts b/server/tests/unit/mcp/tools-roadtrip.test.ts index 0c6f0af20..a3ce90b9b 100644 --- a/server/tests/unit/mcp/tools-roadtrip.test.ts +++ b/server/tests/unit/mcp/tools-roadtrip.test.ts @@ -46,7 +46,6 @@ import { createMcpHarness, parseToolResult, type McpHarness } from '../../helper import { ADDON_IDS } from '../../../src/addons'; import { addTripMember } from '../../helpers/factories'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import { DEMO_EMAIL_PRIMARY } from '../../../src/nest/common/demo'; import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; diff --git a/server/tests/unit/mcp/tools-transit.test.ts b/server/tests/unit/mcp/tools-transit.test.ts index a96e3dcd6..a9da54554 100644 --- a/server/tests/unit/mcp/tools-transit.test.ts +++ b/server/tests/unit/mcp/tools-transit.test.ts @@ -2,7 +2,6 @@ import { runMigrations } from '../../../src/db/migrations'; import { createTables } from '../../../src/db/schema'; import { invalidatePermissionsCache } from '../../../src/nest/permissions/permissions-cache'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import { addTripMember, createDay, createTrip, createUser } from '../../helpers/factories'; import { createMcpHarness, parseToolResult, type McpHarness } from '../../helpers/mcp-harness'; import { resetTestDb } from '../../helpers/test-db'; diff --git a/server/tests/unit/nest/accommodations.service.test.ts b/server/tests/unit/nest/accommodations.service.test.ts index 6a70c8962..866a142fa 100644 --- a/server/tests/unit/nest/accommodations.service.test.ts +++ b/server/tests/unit/nest/accommodations.service.test.ts @@ -40,7 +40,6 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createTrip, createDay, createPlace, createDayAccommodation, createDayAssignment, addTripMember } from '../../helpers/factories'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { AccommodationsService } from '../../../src/nest/accommodations/accommodations.service'; diff --git a/server/tests/unit/nest/cron-registrar.service.test.ts b/server/tests/unit/nest/cron-registrar.service.test.ts index 2e2a4e0ac..707a921aa 100644 --- a/server/tests/unit/nest/cron-registrar.service.test.ts +++ b/server/tests/unit/nest/cron-registrar.service.test.ts @@ -36,6 +36,9 @@ vi.mock('cron', () => ({ import { SchedulerRegistry } from '@nestjs/schedule'; import { CronRegistrarService } from '../../../src/nest/scheduling/cron-registrar.service'; import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; +import { createSnapshotTestDb } from '../../helpers/db-mock'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; +import { Users } from '../../../src/db/entities/Users.entity'; function makeRegistrar(isTest: boolean) { const registry = new SchedulerRegistry(); @@ -150,4 +153,54 @@ describe('CronRegistrarService', () => { expect(registry.getCronJobs().size).toBe(0); expect(registrar.jobCount).toBe(0); }); + + describe('D6 request context (task-2-review.md C3 ruling)', () => { + // Global context disallowed on purpose — the production setting, like + // tests/unit/nest/database/request-context.test.ts — so a repository read + // with no wrapper around it genuinely throws, the way it would outside any + // HTTP request in production. + const testDb = createSnapshotTestDb(); + let t: TestOrm; + + beforeEach(async () => { + t = await createTestOrm(testDb, { allowGlobalContext: false }); + }); + + afterEach(async () => { + await t.close(); + }); + + it('CRONREG-010 — without MikroORM injected, a repository read inside onTick throws cannotUseGlobalContext', async () => { + const registry = new SchedulerRegistry(); + const registrar = new CronRegistrarService(registry, { isTest: () => false } as RuntimeEnvService); // no orm arg + let caught: unknown; + registrar.register('job', '0 2 * * *', async () => { + try { + await t.orm.em.find(Users, {}); + } catch (e) { + caught = e; + } + }); + await h.jobs[0].onTick(); + expect(caught).toBeInstanceOf(Error); + expect(String((caught as Error).message)).toMatch(/global (EntityManager|context)/i); + }); + + it('CRONREG-011 — with MikroORM injected, the SAME repository read inside onTick succeeds — the wrapper is load-bearing', async () => { + const registry = new SchedulerRegistry(); + const registrar = new CronRegistrarService(registry, { isTest: () => false } as RuntimeEnvService, t.orm); + let result: unknown; + let caught: unknown; + registrar.register('job', '0 2 * * *', async () => { + try { + result = await t.orm.em.find(Users, {}); + } catch (e) { + caught = e; + } + }); + await h.jobs[0].onTick(); + expect(caught).toBeUndefined(); + expect(Array.isArray(result)).toBe(true); + }); + }); }); diff --git a/server/tests/unit/nest/permissions.service.test.ts b/server/tests/unit/nest/permissions.service.test.ts index 704604f0c..7f1ecb631 100644 --- a/server/tests/unit/nest/permissions.service.test.ts +++ b/server/tests/unit/nest/permissions.service.test.ts @@ -17,6 +17,7 @@ * hand-rolled DatabaseService/all() stub. */ import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; +import { ValidationError } from '@mikro-orm/core'; // ── DB setup ────────────────────────────────────────────────────────────────── @@ -239,6 +240,26 @@ describe('load failures', () => { svc.invalidatePermissionsCache(); // don't leak this test's cache to later tests }); + it('PERM-SVC-025: a MikroORM ValidationError (context misuse) is a programming error and propagates, uncached — it does not get the "log and serve defaults" treatment', async () => { + // The concrete case this protects (task-2-review.md C3): checkPermission + // reached from a non-HTTP entrypoint outside any request context throws + // exactly this on a cold cache. Swallowing it and serving defaults was + // fail-open — an admin's tightened flag silently reverted to its looser + // default for as long as the misuse persisted. loadPermissions must rethrow + // instead of degrading. + const spy = vi + .spyOn(appSettings, 'findByKeyPrefix') + .mockRejectedValueOnce(ValidationError.cannotUseGlobalContext()); + svc.invalidatePermissionsCache(); + + await expect(svc.getPermissionLevel('trip_edit')).rejects.toThrow(ValidationError); + expect(logError).not.toHaveBeenCalled(); + expect(getPermissionsCache()).toBe(null); + + spy.mockRestore(); + svc.invalidatePermissionsCache(); + }); + it('PERM-SVC-023: an all-skipped save writes nothing and leaves the cache untouched', async () => { expect(await svc.getPermissionLevel('trip_edit')).toBe('trip_owner'); // prime the cache testDb.prepare('INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)').run('perm_trip_edit', 'trip_member'); diff --git a/server/tests/unit/nest/trek-ws-adapter.test.ts b/server/tests/unit/nest/trek-ws-adapter.test.ts index a89788ac7..f91097107 100644 --- a/server/tests/unit/nest/trek-ws-adapter.test.ts +++ b/server/tests/unit/nest/trek-ws-adapter.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterAll } from 'vitest'; /** * TrekWsAdapter: the wire protocol and the per-socket flood guard. @@ -34,6 +34,12 @@ vi.mock('../../../src/app-config', async (importOriginal) => { import { TrekWsAdapter } from '../../../src/nest/realtime/trek-ws.adapter'; import { getServer } from '../../../src/nest/realtime/ws-state'; import type { Server as HttpServer } from 'node:http'; +import { createSnapshotTestDb } from '../../helpers/db-mock'; +import { createTestOrm } from '../../helpers/test-orm'; +import { Users } from '../../../src/db/entities/Users.entity'; + +const testDb = createSnapshotTestDb(); +afterAll(() => testDb.close()); type MessageListener = (buffer: Buffer) => void; @@ -112,6 +118,70 @@ describe('TrekWsAdapter wire protocol', () => { }); }); +describe('TrekWsAdapter D6 request context (task-2-review.md C3 ruling)', () => { + // Global context disallowed on purpose — the production setting, like + // tests/unit/nest/database/request-context.test.ts — so a repository read + // with no wrapper around it genuinely throws, the way it would outside any + // HTTP request in production. A `@SubscribeMessage` handler has exactly that + // shape: dispatched from bindMessageHandlers below, not from an Express + // request, so nothing has forked an EntityManager for it unless the ONE + // wrapper there (not per-handler) does it. + it('WSAD-040: without MikroORM passed to the adapter, a repository read inside a handler throws cannotUseGlobalContext', async () => { + const t = await createTestOrm(testDb, { allowGlobalContext: false }); + try { + let caught: unknown; + const ad = new TrekWsAdapter({} as HttpServer); // no orm + const socket = fakeSocket(); + let captured: Promise | undefined; + const capture = (v: unknown) => { + captured = v instanceof Promise ? v : Promise.resolve(v); + return { subscribe: () => {} } as never; + }; + ad.bindMessageHandlers( + socket as never, + [{ message: 'join', callback: async () => { + try { await t.orm.em.find(Users, {}); } catch (e) { caught = e; } + } }] as never, + capture, + ); + socket.emit('message', frame({ type: 'join', tripId: 1 })); + await captured; + expect(caught).toBeInstanceOf(Error); + expect(String((caught as Error).message)).toMatch(/global (EntityManager|context)/i); + } finally { + await t.close(); + } + }); + + it('WSAD-041: with MikroORM passed to the adapter, the SAME repository read inside a handler succeeds — the wrapper is load-bearing', async () => { + const t = await createTestOrm(testDb, { allowGlobalContext: false }); + try { + let result: unknown; + let caught: unknown; + const ad = new TrekWsAdapter({} as HttpServer, t.orm); + const socket = fakeSocket(); + let captured: Promise | undefined; + const capture = (v: unknown) => { + captured = v instanceof Promise ? v : Promise.resolve(v); + return { subscribe: () => {} } as never; + }; + ad.bindMessageHandlers( + socket as never, + [{ message: 'join', callback: async () => { + try { result = await t.orm.em.find(Users, {}); } catch (e) { caught = e; } + } }] as never, + capture, + ); + socket.emit('message', frame({ type: 'join', tripId: 1 })); + await captured; + expect(caught).toBeUndefined(); + expect(Array.isArray(result)).toBe(true); + } finally { + await t.close(); + } + }); +}); + describe('TrekWsAdapter flood guard', () => { it('WSAD-010: counts EVERY frame, including the ones no handler wants', () => { // The guard sits before parsing on purpose. Counting only the frames that diff --git a/server/tests/unit/plugins/request-context.test.ts b/server/tests/unit/plugins/request-context.test.ts new file mode 100644 index 000000000..50c0067e2 --- /dev/null +++ b/server/tests/unit/plugins/request-context.test.ts @@ -0,0 +1,132 @@ +/** + * D6 regression (task-2-review.md C3): a plugin's `ctx.*` call arrives over a + * child-process `message` event, not an HTTP request, so nothing has forked an + * EntityManager for it. `PluginSupervisor.onMessage` (plugin-supervisor.ts:~553) + * is the single choke point every plugin RPC dispatch passes through, and it now + * wraps `sup.rpcHost.dispatch(...)` in `withRequestContext` there. + * + * The reviewer's probe, reproduced here through the REAL call chain + * (PluginSupervisor.onMessage → PluginGuards.canCreateAs → + * PermissionsService.checkPermission → AppSettingsRepository.findByKeyPrefix), + * not just `checkPermission` in isolation: with `perm_trip_create` tightened + * from its 'everybody' default to 'admin', a role-'user' actor's check must + * come back `false`. Before the fix (dispatch outside any context, and + * loadPermissions serving defaults on any error) it came back `true` — a + * fail-open security regression. After BOTH this round's fixes — the wrapper + * here, and `loadPermissions` rethrowing a MikroORM `ValidationError` instead + * of degrading (permissions.service.ts) — an unwrapped dispatch fails CLOSED + * (a HOST_ERROR envelope), never silently `true`. + * + * Global context is disallowed on purpose (`{ allowGlobalContext: false }`), + * like `tests/unit/nest/database/request-context.test.ts` — the production + * setting — NOT the `createTestOrm` default (`true`) that `test-uow.ts`'s + * shared helpers use to let ordinary unit tests call a repository with no + * request wrapper at all: that default would hide this exact bug. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { createSnapshotTestDb } from '../../helpers/db-mock'; +import { createUser } from '../../helpers/factories'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { AddonsService } from '../../../src/nest/addons/addons.service'; +import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; +import { PluginGuards } from '../../../src/nest/plugins/host/plugin-guards.service'; +import { PluginSupervisor } from '../../../src/nest/plugins/supervisor/plugin-supervisor'; +import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; +import { RpcRateLimiter, DEFAULT_RPC_LIMIT } from '../../../src/nest/plugins/host/rate-limit'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; +import { AppSettings } from '../../../src/db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { PluginRpcHost } from '../../../src/nest/plugins/host/rpc-host'; +import type { RpcRequest, RpcResponse, RpcError } from '../../../src/nest/plugins/protocol/envelope'; +import type { EntityManager } from '@mikro-orm/core'; + +/** The slice of Supervisor's private `Supervised` shape a 'req' dispatch touches. */ +interface DispatchEntry { + rpcHost: Pick; + child: { send: (msg: unknown) => void } | null; + rpcLimiter: RpcRateLimiter; + invocations: Map; +} +/** `onMessage` is private; this is the narrowest structural view that reaches it + * without an `any` cast. */ +interface SupervisorPrivate { + onMessage(sup: DispatchEntry, msg: RpcRequest): Promise; +} + +const testDb = createSnapshotTestDb(); + +let t: TestOrm; +let permissions: PermissionsService; +let guards: PluginGuards; +let userId: number; + +beforeAll(async () => { + t = await createTestOrm(testDb, { allowGlobalContext: false }); + const dbs = new DatabaseService(testDb); + permissions = new PermissionsService(t.repo(AppSettings) as AppSettingsRepository, new UnitOfWork(t.em)); + guards = new PluginGuards(dbs, permissions, new AddonsService(dbs)); + userId = createUser(testDb, { role: 'user' }).user.id; + // An admin has tightened trip_create from its 'everybody' default. + testDb.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES ('perm_trip_create', 'admin')").run(); +}); + +afterAll(async () => { + await t.close(); + testDb.close(); +}); + +/** + * Wires one supervisor whose fake rpcHost.dispatch calls straight into + * PluginGuards.canCreateAs — the same chain the review traced + * (plugin-guards.service.ts:74,84 → PermissionsService.checkPermission) — and + * envelope-shapes the result exactly like the real PluginRpcHost.handle() + * catch (rpc-host.ts): a thrown error becomes a HOST_ERROR response, never an + * unhandled rejection. `resolveOrm` is the exact 4th constructor arg + * PluginRuntimeService passes in production (plugin-runtime.service.ts). + */ +function makeSupervisor(resolveOrm?: () => { em: EntityManager } | undefined) { + const sent: Array = []; + const dispatch: PluginRpcHost['dispatch'] = async (req, actingUserId) => { + try { + const result = await guards.canCreateAs('trip_create', actingUserId as number); + return { k: 'res', id: req.id, ok: true, result }; + } catch (e) { + return { k: 'res', id: req.id, ok: false, error: { code: 'HOST_ERROR', message: e instanceof Error ? e.message : String(e) } }; + } + }; + const createRpcHost = () => ({ dispatch }) as unknown as PluginRpcHost; + const supervisor = new PluginSupervisor(createRpcHost, {}, {}, resolveOrm) as unknown as SupervisorPrivate; + const sup: DispatchEntry = { + rpcHost: { dispatch }, + child: { send: (msg) => sent.push(msg as RpcResponse | RpcError) }, + rpcLimiter: new RpcRateLimiter(DEFAULT_RPC_LIMIT, Date.now()), + invocations: new Map([['inv-1', userId]]), + }; + return { supervisor, sup, sent }; +} + +describe('PluginSupervisor request context (D6, C3)', () => { + it('CTX-PLUGIN-001: unwrapped, the RPC path fails CLOSED on a cold cache — never the old fail-open `true`', async () => { + permissions.invalidatePermissionsCache(); + const { supervisor, sup, sent } = makeSupervisor(); // no resolveOrm — the bug's exact reproduction + + await supervisor.onMessage(sup, { k: 'req', id: 'r1', method: 'trips.create', params: { _inv: 'inv-1' } }); + + expect(sent).toHaveLength(1); + const res = sent[0]; + expect(res.ok).toBe(false); + if (res.ok === false) expect(res.error.message).toMatch(/global (EntityManager|context)/i); + }); + + it('CTX-PLUGIN-002: wrapped in withRequestContext, the same dispatch enforces the stored override — a "user" role is refused', async () => { + permissions.invalidatePermissionsCache(); + const { supervisor, sup, sent } = makeSupervisor(() => t.orm); + + await supervisor.onMessage(sup, { k: 'req', id: 'r2', method: 'trips.create', params: { _inv: 'inv-1' } }); + + expect(sent).toHaveLength(1); + const res = sent[0]; + expect(res.ok).toBe(true); + if (res.ok) expect(res.result).toBe(false); // role 'user' against a stored 'admin' override + }); +});