From decc0e1a12d58ebee8358bc568d85bb7929ce8be Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 25 Aug 2026 08:06:50 +0200 Subject: [PATCH] fix(plugins): boot enabled plugins after the rpc-registry scan, not before MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PluginRuntimeService booted enabled plugins from onModuleInit, but Nest fires same-module onModuleInit hooks in providers-array order, where the runtime precedes PluginRpcRegistryService — so every boot-activated plugin's rpc host was bound from a still-empty registry and its first RPC (typically db.migrate in onLoad) came back PERMISSION_DENIED on every cold boot, until an admin deactivated and reactivated it by hand. Move the boot reconcile to onApplicationBootstrap, which is guaranteed to run after every module's onModuleInit, registry scan included. Also fix the denial message, which interpolated the plugin id where the permission belonged ('db.migrate requires a permission "featured-guides" was not granted'); it now names both the missing permission and the plugin. --- server/src/nest/plugins/host/rpc-host.ts | 3 +- .../nest/plugins/plugin-runtime.service.ts | 14 +- .../plugins/boot-registry-order.test.ts | 142 ++++++++++++++++++ .../plugins/plugin-runtime.test.ts | 14 +- server/tests/unit/plugins/rpc-host.test.ts | 9 +- 5 files changed, 169 insertions(+), 13 deletions(-) create mode 100644 server/tests/integration/plugins/boot-registry-order.test.ts diff --git a/server/src/nest/plugins/host/rpc-host.ts b/server/src/nest/plugins/host/rpc-host.ts index c685b4ebd..4d8b5c33b 100644 --- a/server/src/nest/plugins/host/rpc-host.ts +++ b/server/src/nest/plugins/host/rpc-host.ts @@ -1,5 +1,6 @@ import { KNOWN_METHODS, + METHOD_PERMISSION, type KnownMethod, type RpcError, type RpcRequest, @@ -119,7 +120,7 @@ export class PluginRpcHost { req.id, known ? 'PERMISSION_DENIED' : 'UNKNOWN_METHOD', known - ? `${req.method} requires a permission "${this.pluginId}" was not granted` + ? `${req.method} requires the "${(METHOD_PERMISSION as Record)[req.method]}" permission, which was not granted to plugin "${this.pluginId}"` : `unknown method ${req.method}`, ); } diff --git a/server/src/nest/plugins/plugin-runtime.service.ts b/server/src/nest/plugins/plugin-runtime.service.ts index 561860651..b0e8bb138 100644 --- a/server/src/nest/plugins/plugin-runtime.service.ts +++ b/server/src/nest/plugins/plugin-runtime.service.ts @@ -1,4 +1,4 @@ -import { Injectable, type OnModuleInit, type OnModuleDestroy } from '@nestjs/common'; +import { Injectable, type OnApplicationBootstrap, type OnModuleDestroy } from '@nestjs/common'; import semver from 'semver'; import { DatabaseService } from '../database/database.service'; import { pluginsEnabled } from './kill-switch'; @@ -121,7 +121,7 @@ export class PluginDependencyError extends Error { */ @Injectable() -export class PluginRuntimeService implements OnModuleInit, OnModuleDestroy { +export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDestroy { // The rpc-host factory is bound to `this` as the inter-plugin router, so a // plugin's ctx.plugins.call / ctx.events.emit resolve through callPlugin/ // emitPluginEvent below (which own the dependency-edge authorization). The @@ -182,7 +182,15 @@ export class PluginRuntimeService implements OnModuleInit, OnModuleDestroy { return this.dbs.connection; } - onModuleInit(): void { + // onApplicationBootstrap, NOT onModuleInit: boot activation builds each plugin's + // rpc host synchronously, and the host snapshots PluginRpcRegistryService at + // construction (bindInto). Same-module onModuleInit hooks fire in providers-array + // declaration order, where this service precedes the registry — so booting from + // onModuleInit bound every host to a still-empty registry and every enabled + // plugin's first RPC after a restart came back PERMISSION_DENIED (pinned by + // tests/integration/plugins/boot-registry-order.test.ts). onApplicationBootstrap + // is guaranteed to run after EVERY module's onModuleInit, registry scan included. + onApplicationBootstrap(): void { if (!pluginsEnabled()) return; // If a restore staged plugin trees, swap them into place NOW — before we open any // plugin DB below. This is where a restored backup's plugin data/code actually diff --git a/server/tests/integration/plugins/boot-registry-order.test.ts b/server/tests/integration/plugins/boot-registry-order.test.ts new file mode 100644 index 000000000..d14532cc9 --- /dev/null +++ b/server/tests/integration/plugins/boot-registry-order.test.ts @@ -0,0 +1,142 @@ +/** + * Boot-order regression (#plugins): the RPC registry must be scanned before the + * runtime auto-activates the plugins an admin had enabled. + * + * The bug this pins: PluginRuntimeService booted enabled plugins from + * onModuleInit, and Nest fires same-module onModuleInit hooks in providers-array + * declaration order — where the runtime service is listed BEFORE + * PluginRpcRegistryService. Each boot activation builds its PluginRpcHost + * synchronously (bindInto snapshots the registry at construction), so every host + * was bound from a STILL-EMPTY registry: the plugin's first RPC on activation + * (typically ctx.db.migrate in onLoad) came back PERMISSION_DENIED and the plugin + * landed in error state on every cold boot, until an admin deactivated and + * reactivated it by hand. The fix moves the boot reconcile to + * onApplicationBootstrap, which Nest guarantees runs after EVERY module's + * onModuleInit — so this suite assembles a real Nest module mirroring the + * production declaration order (runtime provider first, registry scan later) and + * asserts the plugin still comes up clean. + */ +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { Test } from '@nestjs/testing'; +import type { TestingModule } from '@nestjs/testing'; + +const { testDb } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec(`CREATE TABLE plugins ( + id TEXT PRIMARY KEY, status TEXT, enabled INTEGER DEFAULT 0, version TEXT, api_version INTEGER DEFAULT 1, permissions TEXT DEFAULT '[]', operator_egress INTEGER DEFAULT 0, granted_permissions TEXT DEFAULT '', + config TEXT DEFAULT '{}', dependencies TEXT DEFAULT '{}', capabilities TEXT DEFAULT '{}', last_error TEXT, updated_at TEXT, + trek_range TEXT DEFAULT '>=3.0.0', + source_repo TEXT, author_pubkey TEXT, update_block_code TEXT, update_block_detail TEXT, update_block_version TEXT); + CREATE TABLE plugin_error_log (id INTEGER PRIMARY KEY AUTOINCREMENT, plugin_id TEXT, level TEXT, message TEXT, ts TEXT); + CREATE TABLE plugin_settings_fields (plugin_id TEXT, field_key TEXT, scope TEXT, secret INTEGER); + CREATE TABLE settings (user_id INTEGER, key TEXT, value TEXT); + CREATE TABLE plugin_entity_metadata (id INTEGER PRIMARY KEY AUTOINCREMENT, plugin_id TEXT, entity_type TEXT, entity_id INTEGER, key TEXT, value TEXT, updated_at TEXT); + CREATE TABLE plugin_user_config (plugin_id TEXT, user_id INTEGER, field_key TEXT, value TEXT, PRIMARY KEY (plugin_id, user_id, field_key)); + CREATE TABLE plugin_meta_migrations (plugin_id TEXT, migration_id TEXT, PRIMARY KEY (plugin_id, migration_id)); + CREATE TABLE plugin_capability_audit (id INTEGER PRIMARY KEY AUTOINCREMENT, plugin_id TEXT, acting_user_id INTEGER, method TEXT, resource TEXT, code TEXT, ts TEXT, prev_hash TEXT, hash TEXT); + CREATE TABLE plugin_scheduled_tasks (id INTEGER PRIMARY KEY AUTOINCREMENT, plugin_id TEXT NOT NULL, name TEXT NOT NULL, due_at INTEGER NOT NULL, payload TEXT NOT NULL DEFAULT 'null', every_ms INTEGER, created_at TEXT DEFAULT (datetime('now')), UNIQUE(plugin_id, name)); + CREATE TABLE plugin_user_erasure_queue (id INTEGER PRIMARY KEY AUTOINCREMENT, plugin_id TEXT NOT NULL, user_id INTEGER NOT NULL, created_at TEXT DEFAULT (datetime('now')), UNIQUE(plugin_id, user_id)); + CREATE TABLE addons (id TEXT PRIMARY KEY, enabled INTEGER DEFAULT 0); + CREATE TABLE audit_log ( + id INTEGER PRIMARY KEY AUTOINCREMENT, created_at DATETIME DEFAULT CURRENT_TIMESTAMP, + user_id INTEGER, action TEXT NOT NULL, resource TEXT, details TEXT, ip TEXT);`); + return { testDb: db }; +}); +vi.mock('../../../src/db/database', () => ({ db: testDb, canAccessTrip: () => undefined })); +vi.mock('../../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi.fn() })); + +import { db as dbConn } from '../../../src/db/database'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { AuditService } from '../../../src/nest/audit/audit.service'; +import { AddonsService } from '../../../src/nest/addons/addons.service'; +import { PluginRuntimeService } from '../../../src/nest/plugins/plugin-runtime.service'; +import { PluginUserSettingsService } from '../../../src/nest/plugins/plugin-user-settings.service'; +import { PluginRpcHostFactory } from '../../../src/nest/plugins/host/plugin-rpc-host.factory'; +import { PluginRpcRegistry } from '../../../src/nest/plugins/host/rpc-kit/registry'; +import type { PluginRpcRegistryService } from '../../../src/nest/plugins/host/rpc-kit/registry.service'; +import { DbRpc } from '../../../src/nest/plugins/host/rpc/db.rpc'; + +let codeRoot: string; +let dataRoot: string; +let mod: TestingModule; + +beforeAll(() => { + codeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'trekplug-boot-code-')); + dataRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'trekplug-boot-data-')); + process.env.TREK_PLUGINS_DIR = codeRoot; + process.env.TREK_PLUGINS_DATA_DIR = dataRoot; + process.env.TREK_PLUGINS_ENABLED = 'true'; + + // The exact shape the bug bit: a plugin that runs its own-db migration as the + // first thing its onLoad does — what every db-backed plugin's activation looks like. + const dir = path.join(codeRoot, 'migrator', 'server'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, 'index.js'), + `module.exports = { async onLoad(ctx) { await ctx.db.migrate('001', 'CREATE TABLE t (n INTEGER)'); } };`, + ); + // The row a container recreate leaves behind: enabled, already consented to + // db:own. Seeded 'inactive' (not the 'active' a real shutdown leaves) so the + // poll below only terminates on a status the THIS-boot supervisor wrote. + testDb + .prepare("INSERT INTO plugins (id, status, enabled, permissions, granted_permissions, config) VALUES ('migrator','inactive',1,'[\"db:own\"]','[\"db:own\"]','{}')") + .run(); +}); + +afterAll(async () => { + await mod?.close(); + delete process.env.TREK_PLUGINS_DIR; + delete process.env.TREK_PLUGINS_DATA_DIR; + delete process.env.TREK_PLUGINS_ENABLED; + fs.rmSync(codeRoot, { recursive: true, force: true }); + fs.rmSync(dataRoot, { recursive: true, force: true }); +}); + +describe('plugin boot vs registry scan ordering', () => { + it('BOOT-REG-001 a plugin enabled before a restart activates cleanly even though the registry scan runs in a LATER provider\'s onModuleInit', async () => { + const dbs = new DatabaseService(dbConn); + const userSettings = new PluginUserSettingsService(dbs); + // Empty at construction — exactly what PluginRpcRegistryService is before its + // own onModuleInit scan has run. + const registry = new PluginRpcRegistry(); + const hostFactory = new PluginRpcHostFactory(dbs, registry as unknown as PluginRpcRegistryService); + + mod = await Test.createTestingModule({ + providers: [ + // Mirrors plugins-runtime.module.ts: the runtime service is DECLARED BEFORE + // the provider that populates the registry, so Nest fires its onModuleInit + // (if it had one) first. The boot reconcile must therefore not live there. + { + provide: PluginRuntimeService, + useFactory: () => new PluginRuntimeService(dbs, new AuditService(dbs), new AddonsService(dbs), userSettings, undefined, hostFactory), + }, + { + provide: 'REGISTRY_SCAN', + useFactory: () => ({ + onModuleInit() { + registry.register(new DbRpc(userSettings)); + }, + }), + }, + ], + }).compile(); + await mod.init(); + + // Boot activation is fire-and-forget; wait for the supervisor to settle the row. + const runtime = mod.get(PluginRuntimeService); + let row = { status: 'starting', last_error: null as string | null }; + for (let i = 0; i < 100; i++) { + row = testDb.prepare("SELECT status, last_error FROM plugins WHERE id='migrator'").get() as typeof row; + if (row.status === 'active' || row.status === 'error') break; + await new Promise((r) => setTimeout(r, 50)); + } + + expect(row.last_error).toBeNull(); + expect(row.status).toBe('active'); + expect(runtime.isActive('migrator')).toBe(true); + }); +}); diff --git a/server/tests/integration/plugins/plugin-runtime.test.ts b/server/tests/integration/plugins/plugin-runtime.test.ts index 2c7fdf329..0ce24c5ba 100644 --- a/server/tests/integration/plugins/plugin-runtime.test.ts +++ b/server/tests/integration/plugins/plugin-runtime.test.ts @@ -148,9 +148,9 @@ describe('PluginRuntimeService (M2 end-to-end)', () => { // first activate is covered by the 'counter' happy-path test above.) }); - it('onModuleInit is a no-op when the runtime is disabled', () => { + it('onApplicationBootstrap is a no-op when the runtime is disabled', () => { process.env.TREK_PLUGINS_ENABLED = 'false'; - expect(() => createPluginRuntime(new DatabaseService(dbConn)).onModuleInit()).not.toThrow(); + expect(() => createPluginRuntime(new DatabaseService(dbConn)).onApplicationBootstrap()).not.toThrow(); process.env.TREK_PLUGINS_ENABLED = 'true'; }); @@ -246,26 +246,26 @@ describe('PluginRuntimeService (M2 end-to-end)', () => { expect(testDb.prepare("SELECT COUNT(*) c FROM plugin_user_erasure_queue WHERE plugin_id='counter' AND user_id=9").get()).toMatchObject({ c: 0 }); }); - it('onModuleInit boots every ENABLED plugin — even one left in error state', async () => { + it('onApplicationBootstrap boots every ENABLED plugin — even one left in error state', async () => { fs.mkdirSync(path.join(codeRoot, 'booter', 'server'), { recursive: true }); fs.writeFileSync(path.join(codeRoot, 'booter', 'server', 'index.js'), 'module.exports = { async onLoad() {} };'); // status='error' from a previous crash, but enabled=1 → must still boot testDb.prepare("INSERT INTO plugins (id, status, enabled, granted_permissions, config) VALUES ('booter','error',1,'[]','{}')").run(); const rt = createPluginRuntime(new DatabaseService(dbConn)); - rt.onModuleInit(); // fire-and-forget spawn + rt.onApplicationBootstrap(); // fire-and-forget spawn for (let i = 0; i < 40 && !rt.isActive('booter'); i++) await new Promise((r) => setTimeout(r, 50)); expect(rt.isActive('booter')).toBe(true); await rt.deactivate('booter'); }); - it('onModuleInit does NOT boot a disabled plugin', async () => { + it('onApplicationBootstrap does NOT boot a disabled plugin', async () => { fs.mkdirSync(path.join(codeRoot, 'sleeper', 'server'), { recursive: true }); fs.writeFileSync(path.join(codeRoot, 'sleeper', 'server', 'index.js'), 'module.exports = { async onLoad() {} };'); testDb.prepare("INSERT INTO plugins (id, status, enabled, granted_permissions, config) VALUES ('sleeper','inactive',0,'[]','{}')").run(); const rt = createPluginRuntime(new DatabaseService(dbConn)); - rt.onModuleInit(); + rt.onApplicationBootstrap(); await new Promise((r) => setTimeout(r, 300)); expect(rt.isActive('sleeper')).toBe(false); }); @@ -701,7 +701,7 @@ describe('TREK host-version gating', () => { process.env.APP_VERSION = '4.0.0'; seedRanged('boot-broken', '>=3.2.0 <4.0.0', 1); const rt = createPluginRuntime(new DatabaseService(dbConn)); - expect(() => rt.onModuleInit()).not.toThrow(); + expect(() => rt.onApplicationBootstrap()).not.toThrow(); for (let i = 0; i < 40; i++) { const row = testDb.prepare("SELECT enabled FROM plugins WHERE id='boot-broken'").get() as { enabled: number }; if (row.enabled === 0) break; diff --git a/server/tests/unit/plugins/rpc-host.test.ts b/server/tests/unit/plugins/rpc-host.test.ts index ce30579fe..d7b4f6cd1 100644 --- a/server/tests/unit/plugins/rpc-host.test.ts +++ b/server/tests/unit/plugins/rpc-host.test.ts @@ -76,9 +76,14 @@ describe('PluginRpcHost — capability enforcement', () => { expect((res as RpcError).error.message).toBe('unknown method fs.readFile'); }); - it('RPCHOST-003 a denied known method names the plugin, so the log says who asked', async () => { + it('RPCHOST-003 a denied known method names the missing permission AND the plugin', async () => { + // The old template interpolated the plugin id where the permission belonged + // ('tags.list requires a permission "p" was not granted'), so an operator read + // their plugin's NAME as an unknown permission. The message must name both. const res = await makeHost([]).dispatch(req('tags.list'), 42); - expect((res as RpcError).error.message).toBe('tags.list requires a permission "p" was not granted'); + expect((res as RpcError).error.message).toBe( + 'tags.list requires the "db:read:tags" permission, which was not granted to plugin "p"', + ); }); it('RPCHOST-004 with no permissions at all, every permissioned method is denied', async () => {