diff --git a/server/src/db/repositories/AuditLog.repository.ts b/server/src/db/repositories/AuditLog.repository.ts index d0056ace5..a4f1ca4ca 100644 --- a/server/src/db/repositories/AuditLog.repository.ts +++ b/server/src/db/repositories/AuditLog.repository.ts @@ -1,4 +1,4 @@ -import type { AuditLog } from '../entities/AuditLog.entity'; +import { AuditLog } from '../entities/AuditLog.entity'; import { type AssertRowKeys } from './_shared/rows'; import { EntityRepository } from '@mikro-orm/sql'; @@ -19,14 +19,8 @@ export interface AuditLogRow { const _auditLogRowKeys: AssertRowKeys = true; -/** The column set `insert` writes; `id`/`created_at` are generated. */ -export interface NewAuditLogRow { - user_id: number | null; - action: string; - resource: string | null; - details: string | null; - ip: string | null; -} +/** The column set `insertEntry` writes; `id`/`created_at` are generated. */ +export type NewAuditLogRow = Omit; // Reserved for Plan 3i (`admin.service.ts`'s paginated read of the same // table): `listPage(limit, offset): Promise` (a `qb()` @@ -41,9 +35,19 @@ export class AuditLogRepository extends EntityRepository { * * Best-effort, fire-and-forget, exactly like the caller * (`AuditService.writeAudit`, wrapped in an outer try/catch that never - * throws): no `refresh`, nothing read back — the generated `id` and - * `created_at` (left to the column's `DEFAULT CURRENT_TIMESTAMP`) are - * never used afterward. + * throws): no `refresh`, nothing the caller reads — MikroORM appends + * `RETURNING id, created_at` for the defaults, but `insertEntry` resolves + * `void`. + * + * A native `em.insert()`, not `create()` + `flush()`: `flush()` commits + * the *whole* unit of work of the request's `EntityManager`, not just this + * row — with ~98 call sites for `writeAudit`, one of them firing before + * every MCP tool handler runs, an unrelated pending change elsewhere in + * the same request would be committed early by an audit write, and the + * outer try/catch that makes this best-effort would swallow whatever that + * flush raised. `em.insert()` fires a single native INSERT with no side + * effects on the context/identity map — the same fire-and-forget + * semantics as the legacy `dbs.run(INSERT …)` autocommit statement. * * Named `insertEntry`, not `insert`: `EntityRepository` (`@mikro-orm/core`) * already declares a native `insert(data, options?): Promise>` @@ -53,13 +57,12 @@ export class AuditLogRepository extends EntityRepository { * inserts `createDay`/`createNote` rather than `create`. */ async insertEntry(entry: NewAuditLogRow): Promise { - const row = this.create({ + await this.getEntityManager().insert(AuditLog, { user: entry.user_id, action: entry.action, resource: entry.resource, details: entry.details, ip: entry.ip, }); - await this.getEntityManager().persist(row).flush(); } } diff --git a/server/tests/integration/mcp.test.ts b/server/tests/integration/mcp.test.ts index 816e4c45d..7670cb16c 100644 --- a/server/tests/integration/mcp.test.ts +++ b/server/tests/integration/mcp.test.ts @@ -40,7 +40,7 @@ import { OauthService } from '../../src/nest/oauth/oauth.service'; import { DatabaseService } from '../../src/nest/database/database.service'; import { AddonsService } from '../../src/nest/addons/addons.service'; import { AuditService } from '../../src/nest/audit/audit.service'; -import { createTestOrm } from '../helpers/test-orm'; +import { createTestOrm, type TestOrm } from '../helpers/test-orm'; import { AuditLog } from '../../src/db/entities/AuditLog.entity'; import type { AuditLogRepository } from '../../src/db/repositories/AuditLog.repository'; import { Users } from '../../src/db/entities/Users.entity'; @@ -70,11 +70,12 @@ const EXPECTED_CHALLENGE = let nestApp: INestApplication; let app: Application; +let t: TestOrm; beforeAll(async () => { nestApp = await buildApp(); app = nestApp.getHttpAdapter().getInstance(); - const t = await createTestOrm(testDb); + t = await createTestOrm(testDb); oauthSvc = new OauthService(oauthDbs, new AddonsService(oauthDbs), new AuditService(t.repo(AuditLog) as AuditLogRepository, t.repo(Users) as UsersRepository)); }); @@ -86,6 +87,7 @@ beforeEach(() => { afterAll(async () => { closeMcpSessions(); await nestApp.close(); + await t.close(); testDb.close(); }); diff --git a/server/tests/integration/oauth-authorize.test.ts b/server/tests/integration/oauth-authorize.test.ts index a3b6a7027..9b0065704 100644 --- a/server/tests/integration/oauth-authorize.test.ts +++ b/server/tests/integration/oauth-authorize.test.ts @@ -44,7 +44,7 @@ import { OauthService } from '../../src/nest/oauth/oauth.service'; import { DatabaseService } from '../../src/nest/database/database.service'; import { AddonsService } from '../../src/nest/addons/addons.service'; import { AuditService } from '../../src/nest/audit/audit.service'; -import { createTestOrm } from '../helpers/test-orm'; +import { createTestOrm, type TestOrm } from '../helpers/test-orm'; import { AuditLog } from '../../src/db/entities/AuditLog.entity'; import type { AuditLogRepository } from '../../src/db/repositories/AuditLog.repository'; import { Users } from '../../src/db/entities/Users.entity'; @@ -59,6 +59,7 @@ let containerSideOauth: OauthService; let nestApp: INestApplication; let app: Application; +let t: TestOrm; function makePkce() { const verifier = crypto.randomBytes(32).toString('base64url'); @@ -85,7 +86,7 @@ async function registerClient(redirectUri = 'https://client.example.com/cb', sco beforeAll(async () => { nestApp = await buildApp(); app = nestApp.getHttpAdapter().getInstance(); - const t = await createTestOrm(testDb); + t = await createTestOrm(testDb); containerSideOauth = new OauthService(oauthDbs, new AddonsService(oauthDbs), new AuditService(t.repo(AuditLog) as AuditLogRepository, t.repo(Users) as UsersRepository)); }); @@ -97,6 +98,7 @@ beforeEach(() => { afterAll(async () => { await nestApp.close(); + await t.close(); testDb.close(); }); diff --git a/server/tests/integration/oauth.test.ts b/server/tests/integration/oauth.test.ts index cc99f9cd0..3fbac0e69 100644 --- a/server/tests/integration/oauth.test.ts +++ b/server/tests/integration/oauth.test.ts @@ -41,7 +41,7 @@ import { OauthService } from '../../src/nest/oauth/oauth.service'; import { DatabaseService } from '../../src/nest/database/database.service'; import { AddonsService } from '../../src/nest/addons/addons.service'; import { AuditService } from '../../src/nest/audit/audit.service'; -import { createTestOrm } from '../helpers/test-orm'; +import { createTestOrm, type TestOrm } from '../helpers/test-orm'; import { AuditLog } from '../../src/db/entities/AuditLog.entity'; import type { AuditLogRepository } from '../../src/db/repositories/AuditLog.repository'; import { Users } from '../../src/db/entities/Users.entity'; @@ -62,6 +62,7 @@ const getUserByAccessToken = (...args: Parameters { nestApp = await buildApp(); app = nestApp.getHttpAdapter().getInstance(); - const t = await createTestOrm(testDb); + t = await createTestOrm(testDb); containerSideOauth = new OauthService(oauthDbs, new AddonsService(oauthDbs), new AuditService(t.repo(AuditLog) as AuditLogRepository, t.repo(Users) as UsersRepository)); }); @@ -95,6 +96,7 @@ beforeEach(() => { afterAll(async () => { await nestApp.close(); + await t.close(); testDb.close(); }); diff --git a/server/tests/integration/plugins/boot-registry-order.test.ts b/server/tests/integration/plugins/boot-registry-order.test.ts index dd68b1f4e..4439a619d 100644 --- a/server/tests/integration/plugins/boot-registry-order.test.ts +++ b/server/tests/integration/plugins/boot-registry-order.test.ts @@ -59,7 +59,7 @@ import { PluginRpcHostFactory } from '../../../src/nest/plugins/host/plugin-rpc- 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'; -import { createTestOrm } from '../../helpers/test-orm'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; import { AuditLog } from '../../../src/db/entities/AuditLog.entity'; import type { AuditLogRepository } from '../../../src/db/repositories/AuditLog.repository'; import { Users } from '../../../src/db/entities/Users.entity'; @@ -68,6 +68,7 @@ import type { UsersRepository } from '../../../src/db/repositories/Users.reposit let codeRoot: string; let dataRoot: string; let mod: TestingModule; +let t: TestOrm | undefined; beforeAll(() => { codeRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'trekplug-boot-code-')); @@ -94,6 +95,7 @@ beforeAll(() => { afterAll(async () => { await mod?.close(); + await t?.close(); delete process.env.TREK_PLUGINS_DIR; delete process.env.TREK_PLUGINS_DATA_DIR; delete process.env.TREK_PLUGINS_ENABLED; @@ -109,7 +111,7 @@ describe('plugin boot vs registry scan ordering', () => { // own onModuleInit scan has run. const registry = new PluginRpcRegistry(); const hostFactory = new PluginRpcHostFactory(dbs, registry as unknown as PluginRpcRegistryService); - const t = await createTestOrm(dbConn); + t = await createTestOrm(dbConn); const auditLogRepo = t.repo(AuditLog) as AuditLogRepository; const usersRepo = t.repo(Users) as UsersRepository; diff --git a/server/tests/unit/db/repositories/AuditLog.repository.test.ts b/server/tests/unit/db/repositories/AuditLog.repository.test.ts index dfb0795c4..e5825396a 100644 --- a/server/tests/unit/db/repositories/AuditLog.repository.test.ts +++ b/server/tests/unit/db/repositories/AuditLog.repository.test.ts @@ -4,6 +4,7 @@ import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; import { createUser } from '../../../helpers/factories'; import { AuditLog } from '../../../../src/db/entities/AuditLog.entity'; +import { AppSettings } from '../../../../src/db/entities/AppSettings.entity'; import type { AuditLogRepository } from '../../../../src/db/repositories/AuditLog.repository'; import { DB_TIMESTAMP_RE } from '../../../../src/db/types'; @@ -24,8 +25,8 @@ describe('AuditLogRepository', () => { await auditLog.insertEntry({ user_id: user.id, action: 'trip.create', resource: 'trip', details: '{"title":"Rome"}', ip: '1.2.3.4' }); const row = testDb.prepare('SELECT * FROM audit_log').get() as { id: number; created_at: string }; expect(row).toStrictEqual({ - id: row.id, - created_at: row.created_at, + id: expect.any(Number), + created_at: expect.stringMatching(DB_TIMESTAMP_RE), user_id: user.id, action: 'trip.create', resource: 'trip', @@ -61,4 +62,17 @@ describe('AuditLogRepository', () => { auditLog.insertEntry({ user_id: user.id, action: 'user.login', resource: null, details: null, ip: null }), ).resolves.toBeUndefined(); }); + + it('AUDITREPO-006: a native insert, not a unit-of-work flush — another pending entity on the same EM stays unwritten', async () => { + const { user } = createUser(testDb); + // Left pending on purpose: `t.em.create` stages a change without flushing it. + t.em.create(AppSettings, { key: 'pending-during-audit-insert', value: 'should-not-be-written' }); + await auditLog.insertEntry({ user_id: user.id, action: 'trip.create', resource: 'trip', details: null, ip: null }); + + const pendingCount = (testDb.prepare('SELECT COUNT(*) AS n FROM app_settings WHERE key = ?').get('pending-during-audit-insert') as { n: number }).n; + expect(pendingCount).toBe(0); + + const auditRow = testDb.prepare('SELECT action FROM audit_log').get() as { action: string }; + expect(auditRow.action).toBe('trip.create'); + }); }); diff --git a/server/tests/unit/nest/oauth.service.test.ts b/server/tests/unit/nest/oauth.service.test.ts index 9525cce0d..9d0d2c69d 100644 --- a/server/tests/unit/nest/oauth.service.test.ts +++ b/server/tests/unit/nest/oauth.service.test.ts @@ -65,7 +65,7 @@ import type { AddonsService } from '../../../src/nest/addons/addons.service'; import { getMcpSafeUrl } from '../../../src/app-config'; import { ADDON_IDS } from '../../../src/addons'; import { MAX_PENDING_CODES, sweepPendingCodes } from '../../../src/nest/oauth/oauth.pending-codes'; -import { createTestOrm } from '../../helpers/test-orm'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; import { AuditLog } from '../../../src/db/entities/AuditLog.entity'; import type { AuditLogRepository } from '../../../src/db/repositories/AuditLog.repository'; import { Users } from '../../../src/db/entities/Users.entity'; @@ -78,6 +78,7 @@ const addonsStub = { isAddonEnabled } as unknown as AddonsService; let svc: OauthService; let auditLogRepo: AuditLogRepository; let usersRepo: UsersRepository; +let t: TestOrm; // Legacy free-function names delegating to the service, so the moved cases below // read exactly as they did before the fold. Arrow wrappers rather than `.bind`: @@ -105,7 +106,7 @@ const isConsentSufficient = (...args: Parameters { createTables(testDb); runMigrations(testDb); - const t = await createTestOrm(testDb); + t = await createTestOrm(testDb); auditLogRepo = t.repo(AuditLog) as AuditLogRepository; usersRepo = t.repo(Users) as UsersRepository; svc = new OauthService(dbs, addonsStub, new AuditService(auditLogRepo, usersRepo)); @@ -120,7 +121,8 @@ beforeEach(() => { isAddonEnabled.mockReturnValue(true); }); -afterAll(() => { +afterAll(async () => { + await t.close(); testDb.close(); }); diff --git a/server/tests/unit/plugins/settings-isolation.test.ts b/server/tests/unit/plugins/settings-isolation.test.ts index 20aa211ad..0817c01e4 100644 --- a/server/tests/unit/plugins/settings-isolation.test.ts +++ b/server/tests/unit/plugins/settings-isolation.test.ts @@ -9,7 +9,7 @@ * field with such a name reported as configured for a user who had configured nothing — * which for a notification channel meant being dispatched to everyone with no credentials. */ -import { describe, it, expect, vi, beforeAll, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; const { testDb, dbMock } = vi.hoisted(() => { const Database = require('better-sqlite3'); @@ -28,7 +28,7 @@ import { runMigrations } from '../../../src/db/migrations'; import { createUser } from '../../helpers/factories'; import { PluginUserSettingsService } from '../../../src/nest/plugins/plugin-user-settings.service'; import { parseManifest, ManifestError } from '../../../src/nest/plugins/install/manifest'; -import { createTestOrm } from '../../helpers/test-orm'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; import { AuditLog } from '../../../src/db/entities/AuditLog.entity'; import type { AuditLogRepository } from '../../../src/db/repositories/AuditLog.repository'; import { Users } from '../../../src/db/entities/Users.entity'; @@ -37,6 +37,11 @@ import type { UsersRepository } from '../../../src/db/repositories/Users.reposit const userSettings = () => new PluginUserSettingsService(new DatabaseService(dbConn)); let uid: number; +let t: TestOrm | undefined; + +afterAll(async () => { + await t?.close(); +}); function declareField(pluginId: string, key: string, opts: { required?: boolean; secret?: boolean } = {}) { testDb.prepare( @@ -131,7 +136,7 @@ describe('a plugin channel label is bounded by the host', () => { VALUES ('loud', 'Loud', 'active', 1, '1.0.0', '[]', '[]', ?, '{}')`, ).run(JSON.stringify({ notificationChannel: { title: '🎉'.repeat(5) + 'A'.repeat(500) } })); - const t = await createTestOrm(dbConn); + t = await createTestOrm(dbConn); const rt = new PluginRuntimeService( new DatabaseService(dbConn), new AuditService(t.repo(AuditLog) as AuditLogRepository, t.repo(Users) as UsersRepository), diff --git a/server/tests/unit/services/airtrailTestConnection.test.ts b/server/tests/unit/services/airtrailTestConnection.test.ts index 6063b675f..50b5b6570 100644 --- a/server/tests/unit/services/airtrailTestConnection.test.ts +++ b/server/tests/unit/services/airtrailTestConnection.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeAll, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; // Avoid any real DNS/network from the SSRF guard during saveSettings and the probe. vi.mock('../../../src/utils/ssrfGuard', () => ({ @@ -12,7 +12,7 @@ import { AirtrailService } from '../../../src/nest/integrations/airtrail.service import type { AirtrailClient } from '../../../src/nest/integrations/airtrail.client'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { AuditService } from '../../../src/nest/audit/audit.service'; -import { createTestOrm } from '../../helpers/test-orm'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; import { AuditLog } from '../../../src/db/entities/AuditLog.entity'; import type { AuditLogRepository } from '../../../src/db/repositories/AuditLog.repository'; import { Users } from '../../../src/db/entities/Users.entity'; @@ -22,9 +22,10 @@ import type { UsersRepository } from '../../../src/db/repositories/Users.reposit // stub; the credential handling around it runs against the real row. const listFlights = vi.fn(); let svc: AirtrailService; +let t: TestOrm; beforeAll(async () => { - const t = await createTestOrm(db); + t = await createTestOrm(db); svc = new AirtrailService( new DatabaseService(db), new AuditService(t.repo(AuditLog) as AuditLogRepository, t.repo(Users) as UsersRepository), @@ -32,6 +33,10 @@ beforeAll(async () => { ); }); +afterAll(async () => { + await t.close(); +}); + const MASK = '••••••••'; beforeEach(() => { diff --git a/server/tests/unit/services/airtrailWriteback.test.ts b/server/tests/unit/services/airtrailWriteback.test.ts index e09449007..9062d2c0c 100644 --- a/server/tests/unit/services/airtrailWriteback.test.ts +++ b/server/tests/unit/services/airtrailWriteback.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeAll } from 'vitest'; +import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; // Avoid any real DNS/network from the SSRF guard during saveSettings. vi.mock('../../../src/utils/ssrfGuard', () => ({ @@ -12,7 +12,7 @@ import { AirtrailService } from '../../../src/nest/integrations/airtrail.service import { AirtrailClient } from '../../../src/nest/integrations/airtrail.client'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { AuditService } from '../../../src/nest/database/../audit/audit.service'; -import { createTestOrm } from '../../helpers/test-orm'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; import { AuditLog } from '../../../src/db/entities/AuditLog.entity'; import type { AuditLogRepository } from '../../../src/db/repositories/AuditLog.repository'; import { Users } from '../../../src/db/entities/Users.entity'; @@ -24,9 +24,10 @@ let svc: AirtrailService; let getConnectionSettings: (...args: Parameters) => ReturnType; let isAirtrailWriteEnabled: (...args: Parameters) => ReturnType; let saveSettings: (...args: Parameters) => ReturnType; +let t: TestOrm; beforeAll(async () => { - const t = await createTestOrm(db); + t = await createTestOrm(db); svc = new AirtrailService( new DatabaseService(db), new AuditService(t.repo(AuditLog) as AuditLogRepository, t.repo(Users) as UsersRepository), @@ -37,6 +38,10 @@ beforeAll(async () => { saveSettings = (...args) => svc.saveSettings(...args); }); +afterAll(async () => { + await t.close(); +}); + describe('airtrail writeback opt-in persistence (#1240)', () => { it('defaults the writeback opt-in to off for a new user', async () => { const { user } = createUser(db);