diff --git a/server/src/nest/README.md b/server/src/nest/README.md index 9122f3307..fe0cc2549 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -48,14 +48,28 @@ remains as the platform underneath `@nestjs/platform-express`. wiring sheet named throughout the log below — is now `PluginRpcHostFactory` and injects two things. See `plugins/host/rpc-kit/README.md`. +- **Plugin module split (complete):** `PluginsModule` was 21 controllers, 24 domain + imports and 10 providers in one class, so `AdminModule` importing it for one + cascade-disable call inherited the whole graph. It is now `PluginsRuntimeModule` + (supervisor, capability router, hook contracts — the half with the domain imports), + `PluginContributionsModule` (the 14 read-only hook controllers, in + `plugins/contributions/`), `PluginOAuthModule` (`plugins/oauth/`) and a composition + root holding the install and delivery controllers. `AdminModule` imports the runtime + alone. Note the invariant the boot-time coverage check now depends on: importing + `PluginsRuntimeModule` must pull in EVERY domain that owns part of the wire surface, + which is why `WeatherModule` sits in its import list. - **Guards (BE-Phase 2, in progress):** the JWT verify lives in `auth/`, and `middleware/auth.ts`, `validate.ts` and `idempotency.ts` are deleted; the three addon guards are one `AddonGuard` + `@RequireAddon`; the demo-mode block is one condition. Still open: `TripAccessGuard`, default-deny, the MFA policy. -`src/services/` is down to one top-level file (backup) plus the `airtrail/` -directory. Both wait on an open decision: backup on the plugins module split and -the db-lifecycle question, airtrail on the credential handling. Note that the trip-access and +`src/services/` is down to the `airtrail/` directory, which waits on the credential +decision. Backup has moved in: `services/backupService.ts` is now +`nest/backup/backup.impl.ts`, imported only by its own domain. It stays a module of +free functions rather than becoming methods, because the restore path closes and +reinitializes the core DB handle and rewriting that shape in the same step as the move +would make a regression there impossible to bisect — the db-lifecycle question is the +LAST step of that fold, not part of it. Note that the trip-access and `canEdit` methods on the domain services are **not** dead weight waiting for a guard: their callers are overwhelmingly the `*.mcp.ts` tools, which never pass through an HTTP guard. In the five domains piloted for `TripAccessGuard`, 40 of @@ -95,7 +109,7 @@ Two shapes, and the choice is settled: from the app after `buildApp()` — the way `bootstrap.ts` hands the `/mcp` handler its registry. The declaration is **structural** (`{ backups: { createBackup } }`), never the provider class: importing `BackupService` would -pull in `src/nest/backup` → `src/services/backupService` → `src/scheduler` and +pull in `src/nest/backup` → `nest/backup/backup.impl` → `src/scheduler` and close a cycle. **`.bridge.ts` is the older shape** and still correct for the MCP @@ -164,6 +178,20 @@ strings. Where the legacy route returns a hand-written error (e.g. weather's `{ error: 'Latitude and longitude are required' }`), reproduce that exact body in the controller rather than relying on the generic `ZodValidationPipe` envelope. +## Coverage is gated per domain, as a ratchet + +`server/vitest.config.ts` carries one threshold entry per `src/nest//`, set at +that domain's measured coverage minus one point. The single repo-wide 80% it replaced +let a well-covered domain subsidise a thin one — `booking-import` sits at 50% and +`integrations` at 20%, and the aggregate still cleared the bar, so either could have +lost another ten points with the build staying green. + +Regenerate the block with `node scripts/coverage-thresholds.mjs` after a run that +RAISED coverage. Raise an entry when you improve a domain; never lower one to make a +build pass. The script reads `coverage/coverage-summary.json` rather than the text +reporter on purpose: the text reporter prints one row per DIRECTORY, so a domain with +subdirectories reads several points higher there than it actually is. + ## How to write the tests Every module ships two kinds of tests. The coverage gate (`vitest.config.ts`) diff --git a/server/src/nest/backup/backup.controller.ts b/server/src/nest/backup/backup.controller.ts index 0c95ab66d..567c3df7a 100644 --- a/server/src/nest/backup/backup.controller.ts +++ b/server/src/nest/backup/backup.controller.ts @@ -25,7 +25,7 @@ import { AdminGuard } from '../auth/admin.guard'; import { CurrentUser } from '../auth/current-user.decorator'; import { getClientIp } from '../audit/client-ip'; import { AuditService } from '../audit/audit.service'; -import { getUploadTmpDir, MAX_BACKUP_UPLOAD_SIZE } from '../../services/backupService'; +import { getUploadTmpDir, MAX_BACKUP_UPLOAD_SIZE } from './backup.impl'; const UPLOAD = { dest: getUploadTmpDir(), diff --git a/server/src/services/backupService.ts b/server/src/nest/backup/backup.impl.ts similarity index 97% rename from server/src/services/backupService.ts rename to server/src/nest/backup/backup.impl.ts index 79dd84a3c..75da93a62 100644 --- a/server/src/services/backupService.ts +++ b/server/src/nest/backup/backup.impl.ts @@ -1,23 +1,23 @@ import archiver from 'archiver'; import unzipper from 'unzipper'; import path from 'path'; -import { readEnv } from '../app-config'; +import { readEnv } from '../../app-config'; import fs from 'fs'; import Database from 'better-sqlite3'; -import { db, closeDb, reinitialize } from '../db/database'; -import * as scheduler from '../scheduler'; -import { invalidatePermissionsCache } from '../nest/permissions/permissions.bridge'; -import { pluginsCodeRoot, pluginsDataRoot } from '../nest/plugins/paths'; -import { stageExtractedPluginTrees, applyStagedRestoreNow } from '../nest/plugins/plugin-backup'; -import { snapshotAllPluginDataDbs } from '../nest/plugins/host/plugin-data.service'; +import { db, closeDb, reinitialize } from '../../db/database'; +import * as scheduler from '../../scheduler'; +import { invalidatePermissionsCache } from '../permissions/permissions.bridge'; +import { pluginsCodeRoot, pluginsDataRoot } from '../plugins/paths'; +import { stageExtractedPluginTrees, applyStagedRestoreNow } from '../plugins/plugin-backup'; +import { snapshotAllPluginDataDbs } from '../plugins/host/plugin-data.service'; // --------------------------------------------------------------------------- // Paths // --------------------------------------------------------------------------- -const dataDir = path.join(__dirname, '../../data'); +const dataDir = path.join(__dirname, '../../../data'); const backupsDir = path.join(dataDir, 'backups'); -const uploadsDir = path.join(__dirname, '../../uploads'); +const uploadsDir = path.join(__dirname, '../../../uploads'); // Compressed upload cap for restore archives. Defaults to 500 MB, raisable via // BACKUP_UPLOAD_LIMIT_MB for instances whose backups (uploads/ included) grow diff --git a/server/src/nest/backup/backup.service.ts b/server/src/nest/backup/backup.service.ts index f2c66429c..167c2c8b2 100644 --- a/server/src/nest/backup/backup.service.ts +++ b/server/src/nest/backup/backup.service.ts @@ -1,10 +1,15 @@ import { Injectable } from '@nestjs/common'; -import * as svc from '../../services/backupService'; +import * as svc from './backup.impl'; /** - * Thin Nest wrapper around the existing backup service. The zip packing/restore, - * the auto-backup scheduler settings, the filename validation, the rate-limit - * bookkeeping and the tmp-dir all reuse the legacy code unchanged. + * The backup domain's injectable face. + * + * The implementation moved here from src/services/backupService.ts with the code + * unchanged, and stays a module rather than becoming methods on this class: the zip + * packing and the restore path close and reinitialize the core DB handle, which is the + * single most dangerous sequence in the server. Rewriting its shape and moving it in + * one step would make a regression there impossible to bisect. The DB-lifecycle + * question is deliberately the LAST step of this fold, not part of the move. */ @Injectable() export class BackupService { diff --git a/server/src/scheduler.ts b/server/src/scheduler.ts index 1f174a807..a757f052e 100644 --- a/server/src/scheduler.ts +++ b/server/src/scheduler.ts @@ -62,7 +62,7 @@ function saveSettings(settings: BackupSettings): void { * * The shapes are structural on purpose: the scheduler names the capability it * wants, never the provider class. Importing BackupService here would drag in - * src/nest/backup → src/services/backupService → src/scheduler and put the + * src/nest/backup → src/nest/backup/backup.impl → src/scheduler and put the * cycle straight back — the same cycle the lazy `await import()` in runBackup * existed to dodge. * diff --git a/server/tests/e2e/backup.e2e.test.ts b/server/tests/e2e/backup.e2e.test.ts index 7238311fa..217f99c61 100644 --- a/server/tests/e2e/backup.e2e.test.ts +++ b/server/tests/e2e/backup.e2e.test.ts @@ -39,7 +39,7 @@ const { backupSvc } = vi.hoisted(() => ({ MAX_BACKUP_UPLOAD_SIZE: 1024, }, })); -vi.mock('../../src/services/backupService', () => backupSvc); +vi.mock('../../src/nest/backup/backup.impl', () => backupSvc); import { BackupModule } from '../../src/nest/backup/backup.module'; import { DatabaseModule } from '../../src/nest/database/database.module'; diff --git a/server/tests/integration/backup.test.ts b/server/tests/integration/backup.test.ts index 60149e477..a49234918 100644 --- a/server/tests/integration/backup.test.ts +++ b/server/tests/integration/backup.test.ts @@ -48,8 +48,8 @@ vi.mock('../../src/config', () => ({ vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi.fn() })); // Mock filesystem-dependent service functions to avoid real disk I/O in tests -vi.mock('../../src/services/backupService', async () => { - const actual = await vi.importActual('../../src/services/backupService'); +vi.mock('../../src/nest/backup/backup.impl', async () => { + const actual = await vi.importActual('../../src/nest/backup/backup.impl'); return { ...actual, createBackup: vi.fn().mockResolvedValue({ @@ -81,7 +81,7 @@ import { runMigrations } from '../../src/db/migrations'; import { resetTestDb, resetRateLimits } from '../helpers/test-db'; import { createAdmin, createUser } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; -import * as backupService from '../../src/services/backupService'; +import * as backupService from '../../src/nest/backup/backup.impl'; import fs from 'fs'; import path from 'path'; import os from 'os'; diff --git a/server/tests/unit/auto-backup.test.ts b/server/tests/unit/auto-backup.test.ts index 6994d509b..362d94ae2 100644 --- a/server/tests/unit/auto-backup.test.ts +++ b/server/tests/unit/auto-backup.test.ts @@ -63,7 +63,7 @@ vi.mock('../../src/config', () => ({ import path from 'node:path'; import { start, setSchedulerDeps } from '../../src/scheduler'; -import { createBackup } from '../../src/services/backupService'; +import { createBackup } from '../../src/nest/backup/backup.impl'; const liveDb = path.join(__dirname, '../../data', 'travel.db'); diff --git a/server/tests/unit/nest/backup.controller.test.ts b/server/tests/unit/nest/backup.controller.test.ts index df6eba65e..8ae3c6baf 100644 --- a/server/tests/unit/nest/backup.controller.test.ts +++ b/server/tests/unit/nest/backup.controller.test.ts @@ -7,7 +7,7 @@ vi.mock('../../../src/nest/audit/audit-log.logger', () => ({ LOG_LEVEL: 'error', // The controller imports the tmp-dir + size cap at module load. The thin // BackupService wrapper forwards every call straight into this module, so the // mock also stubs the delegated functions for the wrapper tests below. -vi.mock('../../../src/services/backupService', () => ({ +vi.mock('../../../src/nest/backup/backup.impl', () => ({ getUploadTmpDir: () => '/tmp', MAX_BACKUP_UPLOAD_SIZE: 1024, BACKUP_RATE_WINDOW: 3600000, @@ -28,7 +28,7 @@ import { BackupService as RealBackupService } from '../../../src/nest/backup/bac import { AdminGuard } from '../../../src/nest/auth/admin.guard'; import type { BackupService } from '../../../src/nest/backup/backup.service'; import type { AuditService } from '../../../src/nest/audit/audit.service'; -import * as backupSvc from '../../../src/services/backupService'; +import * as backupSvc from '../../../src/nest/backup/backup.impl'; import type { User } from '../../../src/types'; const user = { id: 1, role: 'admin', email: 'a@example.test' } as User; diff --git a/server/tests/unit/services/backupService.test.ts b/server/tests/unit/nest/backup.impl.test.ts similarity index 86% rename from server/tests/unit/services/backupService.test.ts rename to server/tests/unit/nest/backup.impl.test.ts index de02c0c4d..0f3b802c8 100644 --- a/server/tests/unit/services/backupService.test.ts +++ b/server/tests/unit/nest/backup.impl.test.ts @@ -92,7 +92,7 @@ import { backupFileExists, listBackups, updateAutoSettings, -} from '../../../src/services/backupService'; +} from '../../../src/nest/backup/backup.impl'; // --------------------------------------------------------------------------- // formatSize @@ -715,6 +715,167 @@ describe('BACKUP-038 restoreFromZip', () => { }); }); +// --------------------------------------------------------------------------- +// restoreFromZip — the per-entry extraction loop +// +// This is the part of the restore that decides what an attacker-supplied archive is +// allowed to write, and it had no tests of its own: the zip-slip refusal, the running +// decompressed-byte cap (the declared size in the central directory is +// attacker-controlled, so the real guard counts bytes as they land) and the failure +// path that leaves the process without a reopened DB. +// --------------------------------------------------------------------------- + +/** A minimal unzipper entry: emits `chunks` when its stream is piped. */ +function zipEntry(entryPath: string, chunks: Buffer[] = [Buffer.alloc(8)], type = 'File') { + return { + path: entryPath, + type, + uncompressedSize: chunks.reduce((n, c) => n + c.length, 0), + stream() { + const handlers: Record void>> = {}; + return { + on(event: string, cb: (arg?: unknown) => void) { + (handlers[event] ??= []).push(cb); + return this; + }, + destroy: vi.fn(), + // The production code pipes AFTER registering handlers, so emitting here is + // the moment every listener is in place. + pipe(out: { emit(event: string): void }) { + for (const chunk of chunks) for (const cb of handlers.data ?? []) cb(chunk); + out.emit('finish'); + }, + }; + }, + }; +} + +/** A write stream that only has to carry 'finish' back to the awaiting promise. */ +function fakeWriteStream() { + const handlers: Record void>> = {}; + return { + on(event: string, cb: () => void) { + (handlers[event] ??= []).push(cb); + return this; + }, + destroy: vi.fn(), + emit(event: string) { + for (const cb of handlers[event] ?? []) cb(); + }, + }; +} + +describe('BACKUP-061 restoreFromZip extraction', () => { + beforeEach(() => { + vi.clearAllMocks(); + fsMock.createWriteStream.mockImplementation(() => fakeWriteStream()); + fsMock.existsSync.mockReturnValue(true); + fsMock.statSync.mockReturnValue({ size: 10 }); + }); + + it('BACKUP-061a — refuses an entry whose path escapes the archive root (zip-slip)', async () => { + unzipperMock.Open.file.mockResolvedValueOnce({ files: [zipEntry('../../etc/passwd')] }); + + const result = await restoreFromZip('/data/tmp/slip.zip'); + + expect(result.success).toBe(false); + expect(result.status).toBe(400); + expect(result.error).toMatch(/escapes the archive root/i); + // Nothing of that archive is left behind, and no byte of it was written. + expect(fsMock.rmSync).toHaveBeenCalledWith(expect.stringContaining('restore-'), { recursive: true, force: true }); + expect(fsMock.createWriteStream).not.toHaveBeenCalled(); + }); + + it('BACKUP-061b — an absolute entry path is refused the same way', async () => { + unzipperMock.Open.file.mockResolvedValueOnce({ files: [zipEntry('C:/Windows/system32/evil.dll')] }); + + const result = await restoreFromZip('/data/tmp/abs.zip'); + + expect(result.success).toBe(false); + expect(result.status).toBe(400); + expect(fsMock.createWriteStream).not.toHaveBeenCalled(); + }); + + it('BACKUP-061c — directory entries are skipped rather than written', async () => { + unzipperMock.Open.file.mockResolvedValueOnce({ + files: [zipEntry('uploads/', [], 'Directory'), zipEntry('travel.db')], + }); + + // What happens after extraction is BACKUP-042..045's business; this case only + // cares that the directory entry never reached the writer. + await restoreFromZip('/data/tmp/dirs.zip').catch(() => undefined); + + expect(fsMock.createWriteStream).toHaveBeenCalledTimes(1); + }); + + it('BACKUP-061d — stops mid-stream once the ACTUAL decompressed bytes cross the cap', async () => { + // The declared size is a lie: the central directory claims 8 bytes while the + // stream delivers well past the 5 GB cap. Only the running total catches this. + const lying = zipEntry('travel.db', [Buffer.alloc(3 * 1024 * 1024 * 1024), Buffer.alloc(3 * 1024 * 1024 * 1024)]); + lying.uncompressedSize = 8; + unzipperMock.Open.file.mockResolvedValueOnce({ files: [lying] }); + + const result = await restoreFromZip('/data/tmp/liar.zip'); + + expect(result.success).toBe(false); + expect(result.status).toBe(400); + expect(result.error).toMatch(/decompressed size/i); + expect(fsMock.rmSync).toHaveBeenCalledWith(expect.stringContaining('restore-'), { recursive: true, force: true }); + }); + + it('BACKUP-061e — a stream error is not swallowed as a size refusal', async () => { + const entry = zipEntry('travel.db'); + entry.stream = () => { + const handlers: Record void>> = {}; + return { + on(event: string, cb: (arg?: unknown) => void) { + (handlers[event] ??= []).push(cb); + return this; + }, + destroy: vi.fn(), + pipe() { + for (const cb of handlers.error ?? []) cb(new Error('corrupt deflate stream')); + }, + }; + }; + unzipperMock.Open.file.mockResolvedValueOnce({ files: [entry] }); + + // A corrupt stream is NOT dressed up as a 400 "too large": it leaves the function + // as a throw, which is what makes the controller answer 500 rather than telling the + // admin their perfectly-sized backup is over the cap. + await expect(restoreFromZip('/data/tmp/corrupt.zip')).rejects.toThrow('corrupt deflate stream'); + expect(fsMock.rmSync).toHaveBeenCalledWith(expect.stringContaining('restore-'), { recursive: true, force: true }); + }); + + it('BACKUP-061f — a reopen failure after the swap reports "restart required", not success', async () => { + unzipperMock.Open.file.mockResolvedValueOnce({ files: [zipEntry('travel.db')] }); + const restored = { + prepare: vi + .fn() + .mockReturnValueOnce({ get: vi.fn().mockReturnValue({ integrity_check: 'ok' }) }) + .mockReturnValueOnce({ + all: vi.fn().mockReturnValue([{ name: 'users' }, { name: 'trips' }, { name: 'trip_members' }, { name: 'places' }, { name: 'days' }]), + }), + close: vi.fn(), + }; + DatabaseMock.mockImplementation(function () { + return restored; + }); + fsMock.existsSync.mockImplementation((path: string) => !String(path).includes('uploads')); + dbMock.reinitialize.mockImplementationOnce(() => { + throw new Error('database is locked'); + }); + + const result = await restoreFromZip('/data/tmp/ok.zip'); + + // The files already landed, so this is neither a success nor a plain failure: the + // admin has to restart, and the message has to say so. + expect(result.success).toBe(false); + expect(result.status).toBe(500); + expect(result.error).toMatch(/restart the server/i); + }); +}); + // --------------------------------------------------------------------------- // better-sqlite3 mock — hoisted by Vitest regardless of file position // --------------------------------------------------------------------------- diff --git a/server/vitest.config.ts b/server/vitest.config.ts index 4e86b1fac..c1808306b 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -64,7 +64,7 @@ export default defineConfig({ 'src/nest/atlas/**/*.ts': { statements: 91, branches: 78, functions: 96, lines: 93 }, 'src/nest/audit/**/*.ts': { statements: 92, branches: 79, functions: 99, lines: 96 }, 'src/nest/auth/**/*.ts': { statements: 93, branches: 84, functions: 95, lines: 96 }, - 'src/nest/backup/**/*.ts': { statements: 97, branches: 91, functions: 99, lines: 97 }, + 'src/nest/backup/**/*.ts': { statements: 96, branches: 92, functions: 99, lines: 98 }, 'src/nest/booking-import/**/*.ts': { statements: 49, branches: 28, functions: 58, lines: 52 }, 'src/nest/budget/**/*.ts': { statements: 91, branches: 77, functions: 97, lines: 95 }, 'src/nest/categories/**/*.ts': { statements: 99, branches: 80, functions: 99, lines: 99 }, @@ -83,7 +83,7 @@ export default defineConfig({ 'src/nest/llm-parse/**/*.ts': { statements: 91, branches: 85, functions: 85, lines: 94 }, 'src/nest/maps/**/*.ts': { statements: 92, branches: 85, functions: 95, lines: 95 }, 'src/nest/memories/**/*.ts': { statements: 88, branches: 80, functions: 95, lines: 90 }, - 'src/nest/notifications/**/*.ts': { statements: 83, branches: 71, functions: 84, lines: 86 }, + 'src/nest/notifications/**/*.ts': { statements: 83, branches: 72, functions: 85, lines: 86 }, 'src/nest/oauth/**/*.ts': { statements: 96, branches: 95, functions: 97, lines: 97 }, 'src/nest/oidc/**/*.ts': { statements: 88, branches: 83, functions: 91, lines: 92 }, 'src/nest/packing/**/*.ts': { statements: 92, branches: 83, functions: 98, lines: 97 },