fix(server): non-HTTP entrypoints run inside an ORM request context; permission loads fail closed on context errors

This commit is contained in:
jubnl
2026-09-22 13:25:17 +02:00
parent 86c558cdd6
commit e9391a9ae1
23 changed files with 464 additions and 36 deletions
+1 -1
View File
@@ -91,7 +91,7 @@ export async function buildApp(): Promise<INestApplication> {
// 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
+6
View File
@@ -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';
+9
View File
@@ -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,
+3
View File
@@ -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');
}
+7
View File
@@ -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;
}
@@ -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());
}
@@ -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
@@ -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<string, PermissionLevel> | null = null;
@@ -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
@@ -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() {
@@ -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<string>) => 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();
+21 -2
View File
@@ -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));
@@ -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;
+11 -3
View File
@@ -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<PluginRuntimeService> {
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,
);
}
+38 -7
View File
@@ -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<Database.Database, Promise<UnitOfWork>>();
const appSettingsPerHandle = new WeakMap<Database.Database, Promise<AppSettingsRepository>>();
// 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<Database.Database, Promise<TestOrm>>();
/**
* 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<TestOrm> {
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<Database.Database, Promise<AppSettingsR
* `UnitOfWork.transactional` opens the transaction through MikroORM on that same
* handle, so the legacy raw statements a service still issues inside the
* callback run inside it — the arrangement the Phase 1 sweep relies on. Memoised
* per handle: one ORM per test file, however many services the file builds.
* per handle via `sharedTestOrm`: one ORM per test file, however many services
* the file builds — and the SAME one `createTestAppSettingsRepo` below draws
* from, so a repository and this `UnitOfWork` resolve the same transaction.
*/
export function createTestUnitOfWork(db: Database.Database): Promise<UnitOfWork> {
// `=== 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<UnitOfWork>
* 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<AppSettingsRepository> {
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;
}
@@ -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
@@ -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';
@@ -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';
@@ -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';
@@ -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);
});
});
});
@@ -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');
+71 -1
View File
@@ -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<unknown> | 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<unknown> | 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
@@ -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<PluginRpcHost, 'dispatch'>;
child: { send: (msg: unknown) => void } | null;
rpcLimiter: RpcRateLimiter;
invocations: Map<string, number | undefined>;
}
/** `onMessage` is private; this is the narrowest structural view that reaches it
* without an `any` cast. */
interface SupervisorPrivate {
onMessage(sup: DispatchEntry, msg: RpcRequest): Promise<void>;
}
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<RpcResponse | RpcError> = [];
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
});
});