diff --git a/server/scripts/size-baseline.json b/server/scripts/size-baseline.json index f84666d63..1d8837261 100644 --- a/server/scripts/size-baseline.json +++ b/server/scripts/size-baseline.json @@ -15,7 +15,7 @@ "src/nest/journey/journey-domain.service.ts": 1804, "src/nest/maps/maps.service.ts": 3252, "src/nest/places/places.service.ts": 2022, - "src/nest/plugins/plugin-runtime.service.ts": 1472, + "src/nest/plugins/plugin-runtime.service.ts": 1463, "src/nest/plugins/runtime/plugin-sdk.ts": 1094, "src/nest/reservations/reservations.service.ts": 1248, "src/nest/trips/trips.service.ts": 1014, diff --git a/server/src/nest/plugins/oauth/plugin-oauth.service.ts b/server/src/nest/plugins/oauth/plugin-oauth.service.ts index b6fc7a0c3..ab1fa0465 100644 --- a/server/src/nest/plugins/oauth/plugin-oauth.service.ts +++ b/server/src/nest/plugins/oauth/plugin-oauth.service.ts @@ -14,6 +14,7 @@ import { PluginOauthState } from '../../../db/entities/PluginOauthState.entity'; import type { PluginOauthStateRepository } from '../../../db/repositories/PluginOauthState.repository'; import { PluginSettingsFields } from '../../../db/entities/PluginSettingsFields.entity'; import type { PluginSettingsFieldsRepository } from '../../../db/repositories/PluginSettingsFields.repository'; +import { UnitOfWork } from '../../database/unit-of-work'; /** * Host-brokered outbound OAuth (#plugins). A plugin becomes an OAuth *client* of a @@ -79,6 +80,7 @@ export class PluginOAuthService { @InjectRepository(PluginOauthTokens) private readonly tokensRepo: PluginOauthTokensRepository, @InjectRepository(PluginOauthState) private readonly stateRepo: PluginOauthStateRepository, @InjectRepository(PluginSettingsFields) private readonly settingsFieldsRepo: PluginSettingsFieldsRepository, + private readonly uow: UnitOfWork, ) {} /** The plugin's decrypted OAuth provider config from its INSTANCE settings, or null @@ -127,9 +129,11 @@ export class PluginOAuthService { const challenge = b64url(crypto.createHash('sha256').update(verifier).digest()); const state = b64url(crypto.randomBytes(24)); - // Drop this user's stale states for the plugin, then store the fresh one. - await this.stateRepo.deleteForUser(pluginId, userId); // PO3 - await this.stateRepo.insertState(state, pluginId, userId, verifier, nowMs); // PO4 + // Drop this user's stale states for the plugin and store the fresh one, together. + await this.uow.transactional(async () => { + await this.stateRepo.deleteForUser(pluginId, userId); // PO3 + await this.stateRepo.insertState(state, pluginId, userId, verifier, nowMs); // PO4 + }); authorize.searchParams.set('response_type', 'code'); authorize.searchParams.set('client_id', cfg.clientId); @@ -141,7 +145,11 @@ export class PluginOAuthService { return authorize.toString(); } - /** Complete the callback: verify state, exchange the code, store the tokens. */ + /** + * Complete the callback: verify state, exchange the code, store the tokens. + * @txIndependent the state is consumed before the token exchange (network I/O), + * and the tokens are stored after it. + */ async completeCallback(pluginId: string, userId: number, code: string, state: string, nowMs: number): Promise { // PO5+PO6+PO7, atomically — the state is consumed (deleted) by this ONE call // regardless of outcome, same as the legacy SELECT-then-unconditional-DELETE @@ -191,8 +199,10 @@ export class PluginOAuthService { } async disconnect(pluginId: string, userId: number): Promise { - await this.tokensRepo.deleteForUser(pluginId, userId); // PO9 - await this.stateRepo.deleteForUser(pluginId, userId); // PO10 + await this.uow.transactional(async () => { + await this.tokensRepo.deleteForUser(pluginId, userId); // PO9 + await this.stateRepo.deleteForUser(pluginId, userId); // PO10 + }); } // --- internals --- diff --git a/server/src/nest/plugins/plugin-runtime.service.ts b/server/src/nest/plugins/plugin-runtime.service.ts index a8b4fc696..8e5567fdf 100644 --- a/server/src/nest/plugins/plugin-runtime.service.ts +++ b/server/src/nest/plugins/plugin-runtime.service.ts @@ -243,18 +243,9 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // registry reads a recipient's own settings on every dispatch, so an absent one // would be a TypeError at send time rather than a missing-provider error at boot. private readonly userSettings: PluginUserSettingsService, - // Plan 3j Task 2 — every table this file's own statements (PR1-PR53) touch, now - // injected as repositories instead of raw prepared-statement calls on `this.db`. All 14 are - // REQUIRED (not `@Optional()`): virtually every method on this class reaches - // `plugins` or one of its satellite tables, so a hand-built test instance needs - // real ones regardless (`sharedTestOrm(...).repo(Entity)`, the same shape - // `PluginGuards`' `UsersRepository` injection (Task 1) already established for - // this plan). `PluginOauthTokens`/`PluginOauthState`/`PluginMetaMigrations`/ - // `PluginCapabilityAudit` are used ONLY by this file's own uninstall cascade and - // GDPR export — Task 3/Task 4 add their own, disjoint methods to the same four - // repositories for their own files. `Settings`/`NotificationChannelPreferences` - // are cross-domain (3f) — additive methods only (`deleteByKeyPrefix`/ - // `deleteAllForChannel`), never their own per-user CRUD surface. + // Every table this class touches, all required: a hand-built test instance needs + // real ones (`sharedTestOrm(...).repo(Entity)`). `Settings` and + // `NotificationChannelPreferences` are cross-domain and used for deletes only. @InjectRepository(Plugins) private readonly plugins: PluginsRepository, @InjectRepository(PluginErrorLog) private readonly pluginErrorLog: PluginErrorLogRepository, @InjectRepository(PluginScheduledTasks) private readonly pluginScheduledTasks: PluginScheduledTasksRepository, @@ -453,7 +444,8 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes /** Fire every scheduled task that is due on an ACTIVE plugin; re-arm recurring * ones, delete one-shots. The row is re-armed/deleted BEFORE the fire so a crash * mid-callback can't double-fire; an inactive plugin's tasks are left untouched so - * they run on the next sweep after it reactivates. Never throws. */ + * they run on the next sweep after it reactivates. Never throws. + * @txIndependent each task's claim is its own atomic statement, followed by its delivery. */ private async fireDueScheduled(): Promise { if (!pluginsEnabled()) return; try { @@ -465,10 +457,8 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes const due = await this.pluginScheduledTasks.findDueForPlugins(now, active); for (const t of due) { if (!this.supervisor.isActive(t.plugin_id)) continue; // leave for a later sweep - // Plan 3j Task 2 deviation (named, task-2-report.md — same spirit as R-uninstall's - // egress-host transaction): the claim is now atomic (`due_at <= now` guarded in - // the repository) — a second, overlapping tick's own claim on the SAME row loses - // and skips delivery entirely, rather than double-firing (RACE-SCHED-001). + // The claim is atomic (`due_at <= now` guarded in the repository): an overlapping + // tick's claim on the same row loses and skips delivery (RACE-SCHED-001). const claimed = t.every_ms ? await this.pluginScheduledTasks.rearm(t.id, now + t.every_ms, now) : await this.pluginScheduledTasks.deleteById(t.id, now); @@ -1088,16 +1078,10 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes } } - /** Stop the plugin, remove its code, and optionally delete all its data. */ /** - * R-uninstall: the 9-table cascade converts with the SAME statement order and the SAME - * (non-)transactional scope the legacy code had — NOT wrapped in `uow.transactional`. - * Nine independent best-effort deletes, several inside their own try/catch tolerating an - * absent table on a slimmed test schema (`plugin_actions`, `plugin_egress_hosts`) — - * `plugins`/`plugin_settings_fields`/`plugin_scheduled_tasks`/`plugin_error_log`/ - * `plugin_entity_metadata`/`plugin_user_config`/`plugin_oauth_tokens`/`plugin_oauth_state`/ - * `plugin_meta_migrations`/`plugin_capability_audit`/`plugin_user_erasure_queue` do NOT - * tolerate an absent table, unchanged from the legacy statement order and shape. + * Stop the plugin, remove its code, and optionally delete all its data. The row + * deletes keep the legacy order; `plugin_actions` and `plugin_egress_hosts` still + * tolerate an absent table (a slimmed test schema), the others do not. */ async uninstall(id: string, deleteData: boolean): Promise { await this.supervisor.disable(id); @@ -1106,6 +1090,14 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // Code always goes; the DB metadata + fields go so it disappears from the UI. // Link-safe: a dev-linked plugin only drops the symlink, never the author's source. removePluginCodeEntry(pluginCodeDir(id)); + // Every row about the plugin goes in one transaction: a failure halfway used to + // leave a plugin without its row but with its settings, grants or tasks, which a + // later plugin reusing the id would inherit. The data dir goes after the commit. + await this.uow.transactional(() => this.dropPluginRows(id, deleteData)); + if (deleteData) removePluginData(id); + } + + private async dropPluginRows(id: string, deleteData: boolean): Promise { await this.plugins.deleteById(id); await this.pluginSettingsFields.deleteAllForPlugin(id); try { await this.pluginActions.deleteAllForPlugin(id); } catch { /* table absent */ } @@ -1122,7 +1114,6 @@ export class PluginRuntimeService implements OnApplicationBootstrap, OnModuleDes // user's opt-outs and the admin's enablement. await this.retireNotificationChannel(id); if (deleteData) { - removePluginData(id); await this.pluginErrorLog.deleteAllForPlugin(id); await this.settingsRepo.deleteByKeyPrefix(`plugin:${id}:`); await this.pluginEntityMetadata.deleteAllForPlugin(id); diff --git a/server/src/nest/plugins/registry/registry.service.ts b/server/src/nest/plugins/registry/registry.service.ts index 26f6fa3c5..dce7e292f 100644 --- a/server/src/nest/plugins/registry/registry.service.ts +++ b/server/src/nest/plugins/registry/registry.service.ts @@ -512,19 +512,23 @@ export class PluginRegistryService { fs.rmSync(dest, { recursive: true, force: true }); fs.renameSync(pluginRoot, dest); - // 7. register INACTIVE (record provenance) - await discoverPlugins(this.discoveryRepos); - await this.plugins.setInstallProvenance(id, entry.repo, ver.commitSha, ver.sha256, entry.reviewedAt ?? null); - // Pin the author key on first successful install of a signed plugin (TOFU) — - // and, after a re-trust, re-pin to the new key the admin blessed. Only ever set - // to a key the artifact just verified under; NEVER cleared to NULL, because a - // NULL pin re-opens the "was never signed" path that accepts an unsigned update. - if (entry.authorPublicKey) { - await this.plugins.setAuthorPubkey(id, entry.authorPublicKey); - } - // The plugin is now on new code that passed every check — whatever refusal was - // recorded before no longer describes reality. - await clearUpdateBlock(this.plugins, id); + // 7. register INACTIVE (record provenance), the row and its provenance in one + // write, so a failure never leaves a registered plugin without its pinned key. + const repos = this.discoveryRepos; + await repos.uow.transactional(async () => { + await discoverPlugins(repos); + await this.plugins.setInstallProvenance(id, entry.repo, ver.commitSha, ver.sha256, entry.reviewedAt ?? null); + // Pin the author key on first successful install of a signed plugin (TOFU) — + // and, after a re-trust, re-pin to the new key the admin blessed. Only ever set + // to a key the artifact just verified under; NEVER cleared to NULL, because a + // NULL pin re-opens the "was never signed" path that accepts an unsigned update. + if (entry.authorPublicKey) { + await this.plugins.setAuthorPubkey(id, entry.authorPublicKey); + } + // The plugin is now on new code that passed every check — whatever refusal was + // recorded before no longer describes reality. + await clearUpdateBlock(this.plugins, id); + }); return { id, version: ver.version, trekRangeBypassed }; } finally { fs.rmSync(staging, { recursive: true, force: true }); diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index a95bf2c8b..b4e1573e0 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -284,7 +284,7 @@ export async function createPluginRpcHostParts( // moved off DatabaseService onto AddonsRepository. const llmConfig = new LlmConfigResolver(new SettingsService(await createTestUnitOfWork(db), appSettings, await createTestSettingsRepo(db)), (await sharedTestOrm(db)).repo(Addons), addons); const pluginOrm = await sharedTestOrm(db); - const oauth = new PluginOAuthService(pluginOrm.repo(Plugins), pluginOrm.repo(PluginOauthTokens), pluginOrm.repo(PluginOauthState), pluginOrm.repo(PluginSettingsFields)); + const oauth = new PluginOAuthService(pluginOrm.repo(Plugins), pluginOrm.repo(PluginOauthTokens), pluginOrm.repo(PluginOauthState), pluginOrm.repo(PluginSettingsFields), await createTestUnitOfWork(db)); const accommodations = new AccommodationsService( permissions, realtime, assignments, await createTestUnitOfWork(db), await createTestTripsRepo(db), diff --git a/server/tests/integration/plugins/plugin-runtime.task2-conversion.test.ts b/server/tests/integration/plugins/plugin-runtime.task2-conversion.test.ts index d86487d84..74fea082a 100644 --- a/server/tests/integration/plugins/plugin-runtime.task2-conversion.test.ts +++ b/server/tests/integration/plugins/plugin-runtime.task2-conversion.test.ts @@ -2,8 +2,8 @@ * Plan 3j Task 2 — mutation/parity proofs the brief names explicitly, on top of the * pre-existing (and still-green) `plugin-runtime.test.ts`/`plugins-service.test.ts` * suites: R-install-gates' three named accept+refuse pairs (PR17/PR26/PR28), the - * 9-table uninstall cascade's preserved NON-transactional partial-failure shape - * (R-uninstall), the egress-host DELETE+loop-INSERT's new transaction rollback proof + * uninstall cascade's all-or-nothing rollback (R-uninstall, one transaction since + * the transaction sweep), the egress-host DELETE+loop-INSERT's transaction rollback proof * (R-uninstall's ONE named exception), PS7/PS9/PS11's triple-duplicate SELECT * collapsing onto one `PluginUserConfigRepository.findConfig` method, and a two-tick * race for the scheduler sweep + the GDPR erasure drain. @@ -255,8 +255,8 @@ describe('Plan 3j Task 2 — R-install-gates named accept+refuse pairs (PR17/PR2 }); }); -describe('Plan 3j Task 2 — R-uninstall: the 9-table cascade stays non-transactional (concurrency-pin)', () => { - it('CASCADE-PARTIAL-001: a failure partway through the cascade leaves EARLIER deletes committed and LATER ones never attempted — the CURRENT legacy shape, not hardened to all-or-nothing', async () => { +describe('R-uninstall: the cascade is one transaction', () => { + it('CASCADE-PARTIAL-001: a failure partway through the cascade rolls every earlier delete back, and the data directory stays', async () => { const rt = await buildRuntime(); const id = 'cascade-partial'; seedPlugin(id); @@ -272,18 +272,21 @@ describe('Plan 3j Task 2 — R-uninstall: the 9-table cascade stays non-transact const errorLogRepo = (rt as any).pluginErrorLog; const spy = vi.spyOn(errorLogRepo, 'deleteAllForPlugin').mockRejectedValueOnce(new Error('simulated mid-cascade crash')); + const dataDir = path.join(dataRoot, id); + fs.mkdirSync(dataDir, { recursive: true }); + await expect(rt.uninstall(id, true)).rejects.toThrow(/simulated mid-cascade crash/); spy.mockRestore(); - // EVERYTHING before the simulated crash point is already gone — no transaction - // rolled it back, exactly matching the legacy sequential-statement shape. - expect(testDb.prepare("SELECT COUNT(*) c FROM plugins WHERE id=?").get(id)).toMatchObject({ c: 0 }); - expect(testDb.prepare("SELECT COUNT(*) c FROM plugin_settings_fields WHERE plugin_id=?").get(id)).toMatchObject({ c: 0 }); - expect(testDb.prepare("SELECT COUNT(*) c FROM plugin_scheduled_tasks WHERE plugin_id=?").get(id)).toMatchObject({ c: 0 }); - // The row the mock made deleteAllForPlugin THROW for is untouched (never committed)... - expect(testDb.prepare("SELECT COUNT(*) c FROM plugin_error_log WHERE plugin_id=?").get(id)).toMatchObject({ c: 1 }); - // ...and everything AFTER it in cascade order never even ran. - expect(testDb.prepare("SELECT COUNT(*) c FROM plugin_entity_metadata WHERE plugin_id=?").get(id)).toMatchObject({ c: 1 }); + // Everything before the simulated crash point was rolled back with it: a half + // uninstall used to leave settings, tasks and metadata for a later plugin that + // reuses the id to inherit. + for (const table of ['plugin_settings_fields', 'plugin_scheduled_tasks', 'plugin_error_log', 'plugin_entity_metadata']) { + expect(testDb.prepare(`SELECT COUNT(*) c FROM ${table} WHERE plugin_id=?`).get(id)).toMatchObject({ c: 1 }); + } + expect(testDb.prepare("SELECT COUNT(*) c FROM plugins WHERE id=?").get(id)).toMatchObject({ c: 1 }); + // The data directory goes only after the rows commit. + expect(fs.existsSync(dataDir)).toBe(true); }); }); diff --git a/server/tests/unit/plugins/plugin-oauth.service.test.ts b/server/tests/unit/plugins/plugin-oauth.service.test.ts index 832fce8b0..db807e306 100644 --- a/server/tests/unit/plugins/plugin-oauth.service.test.ts +++ b/server/tests/unit/plugins/plugin-oauth.service.test.ts @@ -35,6 +35,7 @@ import Database from 'better-sqlite3'; import { createSnapshotTestDb } from '../../helpers/db-mock'; import { PluginOAuthService } from '../../../src/nest/plugins/oauth/plugin-oauth.service'; import { sharedTestOrm } from '../../helpers/test-uow'; +import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; import { Plugins } from '../../../src/db/entities/Plugins.entity'; import { PluginOauthTokens } from '../../../src/db/entities/PluginOauthTokens.entity'; import { PluginOauthState } from '../../../src/db/entities/PluginOauthState.entity'; @@ -64,7 +65,7 @@ const NOW = 1_700_000_000_000; */ async function makeOauthService(): Promise { const orm = await sharedTestOrm(getDb.current as Database.Database); - return new PluginOAuthService(orm.repo(Plugins), orm.repo(PluginOauthTokens), orm.repo(PluginOauthState), orm.repo(PluginSettingsFields)); + return new PluginOAuthService(orm.repo(Plugins), orm.repo(PluginOauthTokens), orm.repo(PluginOauthState), orm.repo(PluginSettingsFields), new UnitOfWork(orm.em)); } describe('PluginOAuthService', () => { @@ -113,6 +114,15 @@ describe('PluginOAuthService', () => { expect((rows.prepare('SELECT COUNT(*) c FROM plugin_oauth_state WHERE user_id = 42').get() as { c: number }).c).toBe(1); }); + it('startConnect drops the old state and stores the new one together: a failed store keeps the old one', async () => { + const first = new URL(await svc.startConnect('p', 42, NOW)).searchParams.get('state')!; + const orm = await sharedTestOrm(getDb.current as Database.Database); + vi.spyOn(orm.repo(PluginOauthState), 'insertState').mockRejectedValueOnce(new Error('disk full')); + await expect(svc.startConnect('p', 42, NOW)).rejects.toThrow('disk full'); + const rows = getDb.current as unknown as InstanceType; + expect(rows.prepare('SELECT state FROM plugin_oauth_state WHERE user_id = 42').all()).toEqual([{ state: first }]); + }); + it('rejects a non-https / loopback / metadata / internal authorize endpoint', async () => { getDb.current = freshDb({ ...CFG, oauth_authorize_url: 'http://provider.example/authorize' }); await expect((await makeOauthService()).startConnect('p', 42, NOW)).rejects.toThrow(/https/);