k1nq/TREK
1
0
Fork 0
mirror of https://github.com/mauriceboe/TREK.git synced 2026-10-21 14:36:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(audit): audit rows are native inserts, not a unit-of-work flush; closed test ORMs

Browse Source
This commit is contained in:
jubnl
2026-09-22 12:56:03 +02:00
parent b0905f9867
commit 86c558cdd6
10 changed files with 78 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/db/repositories/AuditLog.repository.ts
+17 -14
View File
@@ -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<AuditLogRow, AuditLog> = 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<AuditLogRow, 'id' | 'created_at'>;
// Reserved for Plan 3i (`admin.service.ts`'s paginated read of the same
// table): `listPage(limit, offset): Promise<AuditLogPageRow[]>` (a `qb()`
@@ -41,9 +35,19 @@ export class AuditLogRepository extends EntityRepository<AuditLog> {
*
* 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<Primary<Entity>>`
@@ -53,13 +57,12 @@ export class AuditLogRepository extends EntityRepository<AuditLog> {
* inserts `createDay`/`createNote` rather than `create`.
*/
async insertEntry(entry: NewAuditLogRow): Promise<void> {
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();
}
}
server/tests/integration/mcp.test.ts
+4 -2
View File
@@ -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();
});
server/tests/integration/oauth-authorize.test.ts
+4 -2
View File
@@ -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();
});
server/tests/integration/oauth.test.ts
+4 -2
View File
@@ -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<OauthService['getUserByAccessT
let nestApp: INestApplication;
let app: Application;
let t: TestOrm;
// PKCE helpers
function makePkce() {
@@ -83,7 +84,7 @@ function setMcpEnabled(enabled: boolean) {
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));
});
@@ -95,6 +96,7 @@ beforeEach(() => {
afterAll(async () => {
await nestApp.close();
await t.close();
testDb.close();
});
server/tests/integration/plugins/boot-registry-order.test.ts
+4 -2
View File
@@ -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;
server/tests/unit/db/repositories/AuditLog.repository.test.ts
+16 -2
View File
@@ -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');
});
});
server/tests/unit/nest/oauth.service.test.ts
+5 -3
View File
@@ -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<OauthService['isConsentSufficie
beforeAll(async () => {
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();
});
server/tests/unit/plugins/settings-isolation.test.ts
+8 -3
View File
@@ -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),
server/tests/unit/services/airtrailTestConnection.test.ts
+8 -3
View File
@@ -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(() => {
server/tests/unit/services/airtrailWriteback.test.ts
+8 -3
View File
@@ -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<AirtrailService['getConnectionSettings']>) => ReturnType<AirtrailService['getConnectionSettings']>;
let isAirtrailWriteEnabled: (...args: Parameters<AirtrailService['isAirtrailWriteEnabled']>) => ReturnType<AirtrailService['isAirtrailWriteEnabled']>;
let saveSettings: (...args: Parameters<AirtrailService['saveSettings']>) => ReturnType<AirtrailService['saveSettings']>;
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);