From de2d13b1af90f401bf39bac82553af71a88bce87 Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 22 Sep 2026 08:10:23 +0200 Subject: [PATCH] fix(server): upload filters answer multer once on a throwing callback; positive call-graph gate test --- server/scripts/__fixtures__/sync-db-sample.ts | 23 ++++++++++++ server/src/nest/files/files.controller.ts | 13 +++++-- server/src/nest/journey/journey.controller.ts | 13 +++++-- server/src/systemNotices/service.ts | 2 +- .../tests/unit/scripts/db-call-graph.test.ts | 36 ++++++++++++++----- 5 files changed, 72 insertions(+), 15 deletions(-) create mode 100644 server/scripts/__fixtures__/sync-db-sample.ts diff --git a/server/scripts/__fixtures__/sync-db-sample.ts b/server/scripts/__fixtures__/sync-db-sample.ts new file mode 100644 index 000000000..5ed794cbf --- /dev/null +++ b/server/scripts/__fixtures__/sync-db-sample.ts @@ -0,0 +1,23 @@ +/** + * Fixture for `db-call-graph.mjs --sync --tx --root scripts/__fixtures__`: one class that + * still touches the database synchronously and opens a raw transaction, so the two gates + * CI runs (`--sync`, `--tx`) each have a known-bad case to report. `unawaited-sample.ts` + * beside it is clean under both flags. + */ + +interface RawDb { + get(sql: string, ...args: unknown[]): unknown; + transaction(fn: () => T): T; +} + +export class SyncSample { + private db: RawDb = { get: () => undefined, transaction: (fn) => fn() }; + + readSync(id: number): unknown { + return this.db.get('SELECT 1 WHERE id = ?', id); // sync DB-touching method + } + + writeInTx(): void { + this.db.transaction(() => undefined); // raw transaction site + } +} diff --git a/server/src/nest/files/files.controller.ts b/server/src/nest/files/files.controller.ts index 57aeb8a0e..f1e3f906c 100644 --- a/server/src/nest/files/files.controller.ts +++ b/server/src/nest/files/files.controller.ts @@ -58,8 +58,8 @@ export function filesUploadFileFilter(allowedTypes: AllowedFileTypesService): Op // detached async function that always answers through `cb` — multer holds // the file stream until it does (make-middleware.js says as much). A // rejection refuses the file, so the filter still fails closed. Only the - // await is wrapped in try/catch, so a throw from cb() itself is never - // re-routed into a second cb() call. + // await is wrapped in try/catch; a throw from cb() itself lands in the + // trailing .catch, which logs it and answers multer one final time. (async () => { let allowed: string[]; try { @@ -73,7 +73,14 @@ export function filesUploadFileFilter(allowedTypes: AllowedFileTypesService): Op reject(); })().catch((err: unknown) => { logError(`files upload fileFilter: ${err instanceof Error ? err.message : String(err)}`); - cb(err instanceof Error ? err : new Error(String(err))); + // cb() itself is the only thing that can reach here (every await is + // inside the try above); answer once more so multer never hangs, but a + // second throw from cb() means multer already answered — swallow it. + try { + cb(err instanceof Error ? err : new Error(String(err))); + } catch { + /* multer already answered */ + } }); }; } diff --git a/server/src/nest/journey/journey.controller.ts b/server/src/nest/journey/journey.controller.ts index a6ac63fbf..a68ff5661 100644 --- a/server/src/nest/journey/journey.controller.ts +++ b/server/src/nest/journey/journey.controller.ts @@ -88,8 +88,8 @@ export function journeyImageFileFilter(allowedTypes: AllowedFileTypesService): O // allowed-extension list is now an async read. The decision runs in a // detached async function that always answers through `cb`; a rejection // refuses the file, so the filter still fails closed. Only the await is - // wrapped in try/catch, so a throw from cb() itself is never re-routed - // into a second cb() call. + // wrapped in try/catch; a throw from cb() itself lands in the trailing + // .catch, which logs it and answers multer one final time. (async () => { let allowed: string[]; try { @@ -101,7 +101,14 @@ export function journeyImageFileFilter(allowedTypes: AllowedFileTypesService): O cb(null, true); })().catch((err: unknown) => { logError(`journey photo fileFilter: ${err instanceof Error ? err.message : String(err)}`); - cb(err instanceof Error ? err : new Error(String(err))); + // cb() itself is the only thing that can reach here (every await is + // inside the try above); answer once more so multer never hangs, but a + // second throw from cb() means multer already answered — swallow it. + try { + cb(err instanceof Error ? err : new Error(String(err))); + } catch { + /* multer already answered */ + } }); }; } diff --git a/server/src/systemNotices/service.ts b/server/src/systemNotices/service.ts index 098477159..4cf6d954b 100644 --- a/server/src/systemNotices/service.ts +++ b/server/src/systemNotices/service.ts @@ -62,7 +62,7 @@ export async function getActiveNoticesFor( // `evaluate` runs inside a .filter(), which cannot await, so the addon flags // the registry is able to ask about are resolved up front and handed to it as // a lookup. The ids come from the notices themselves, so every question the - // conditions can ask has an answer here and the fallback is unreachable. + // conditions can ask has an answer here. const addonFlags = new Map(); for (const condition of SYSTEM_NOTICES.flatMap(n => n.conditions)) { if (condition.kind === 'addonEnabled' && !addonFlags.has(condition.addonId)) { diff --git a/server/tests/unit/scripts/db-call-graph.test.ts b/server/tests/unit/scripts/db-call-graph.test.ts index 759e3f0fb..982cb3f73 100644 --- a/server/tests/unit/scripts/db-call-graph.test.ts +++ b/server/tests/unit/scripts/db-call-graph.test.ts @@ -32,10 +32,26 @@ interface UnawaitedReport { unawaitedCallCount: number; } +interface SyncDbMethod { + file: string; + class: string; + method: string; + line: number; + direct: boolean; +} + +interface TransactionSite { + file: string; + class: string; + method: string; + line: number; + receiver: string; +} + interface SyncTxReport { - syncDbMethods: unknown[]; + syncDbMethods: SyncDbMethod[]; syncDbMethodCount: number; - transactionSites: unknown[]; + transactionSites: TransactionSite[]; transactionSiteCount: number; } @@ -87,14 +103,18 @@ describe('db-call-graph.mjs', () => { expect(byMethod.good3).toBeUndefined(); }); - it('CALLGRAPH-002: --sync --tx finds nothing in the fixture tree (it touches no database) and exits 0', () => { + it('CALLGRAPH-002: --sync --tx reports exactly the sync-db fixture (two sync DB methods, one raw transaction) and exits 1', () => { const { status, report } = run(['--root', 'scripts/__fixtures__', '--sync', '--tx', '--json']); const { syncDbMethods, syncDbMethodCount, transactionSites, transactionSiteCount } = report as SyncTxReport; - expect(status).toBe(0); - expect(syncDbMethodCount).toBe(0); - expect(syncDbMethods).toEqual([]); - expect(transactionSiteCount).toBe(0); - expect(transactionSites).toEqual([]); + // Non-empty lists exit 1 — that is what makes the CI step fail on a regression. + expect(status).toBe(1); + expect(syncDbMethodCount).toBe(2); + expect(syncDbMethods.map((m) => `${m.class}.${m.method}`).sort()).toEqual(['SyncSample.readSync', 'SyncSample.writeInTx']); + expect(syncDbMethods.every((m) => m.file.endsWith('sync-db-sample.ts') && m.direct)).toBe(true); + expect(transactionSiteCount).toBe(1); + expect(transactionSites).toEqual([expect.objectContaining({ class: 'SyncSample', method: 'writeInTx', receiver: 'this.db' })]); + // The unawaited fixture beside it stays clean under both flags. + expect(syncDbMethods.some((m) => m.class === 'Sample')).toBe(false); }); });