From 1d84c738d8e7df46246b0f896fceee36afe20813 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 16:29:03 +0200 Subject: [PATCH] test: repair four stale backend suites All four asserted contracts the product has since moved past. No genuine product bugs behind any of them; assertions were tightened, not loosened. adminAuth (3 tests): never mounted errorHandler, so ConflictError/ ValidationError arrived as empty Express defaults. The route also checks username before email, so the "email conflict" fixture was hitting the username branch. Mount the handler, fix the fixture, match the real response shapes. backupService.enhanced (12 tests): three stacked drifts -- the db mock had no .returning(), so every runBackup threw at the insert; ensureDatabaseDumpForBackup now lazily requires ./databaseBackup inside the run, which fails under mock-fs; and the rsync path moved from exec(shell string) to spawnAsync('rsync', args) with an isHostAllowed SSRF preflight. Also updates getBackupStatus to its current shape (frontend aliases, nextScheduledRun null when no schedule is enabled, #871). adminSettings.logo: POST /logo gained requirePermission('settings.edit'); the hand-rolled db mock returns a bare Promise from select(), so the permission lookup threw a TypeError into a 500. Mock the permissions middleware alongside the already-mocked auth. crmMintPaths (2 tests): macOS-only. The expected prefix was realpath'd while the services persist under the raw STORAGE_PATH -- identical on Linux CI (/var vs /private/var only diverges on macOS), which is why it passed there. The comment justifying the realpath referenced process.cwd() behaviour the services no longer have. Refs testplan REPORT.md #22 (Part 1.2.01). --- backend/__tests__/adminSettings.logo.test.js | 5 + .../integration/crmMintPaths.test.js | 11 +- .../services/backupService.enhanced.test.js | 155 ++++++++++++------ .../src/routes/__tests__/adminAuth.test.js | 24 ++- 4 files changed, 139 insertions(+), 56 deletions(-) diff --git a/backend/__tests__/adminSettings.logo.test.js b/backend/__tests__/adminSettings.logo.test.js index 16eb8911..0ff1c17d 100644 --- a/backend/__tests__/adminSettings.logo.test.js +++ b/backend/__tests__/adminSettings.logo.test.js @@ -110,6 +110,11 @@ describe('Admin settings logo upload flow', () => { } })); + jest.doMock('../src/middleware/permissions', () => ({ + requirePermission: () => (req, res, next) => next(), + userHasAnyPermission: jest.fn().mockResolvedValue(true) + })); + jest.doMock('../src/services/publicSiteService', () => ({ clearPublicSiteCache: jest.fn(), getDefaultPublicSitePayload: jest.fn(), diff --git a/backend/__tests__/integration/crmMintPaths.test.js b/backend/__tests__/integration/crmMintPaths.test.js index 74536a9c..92e19d6a 100644 --- a/backend/__tests__/integration/crmMintPaths.test.js +++ b/backend/__tests__/integration/crmMintPaths.test.js @@ -148,10 +148,15 @@ async function seedCustomerSignedContract() { beforeAll(async () => { ({ db, cleanup, tmpDir } = await bootCrmDb()); // Business-doc PDFs (quotes/invoices/contracts) persist under - // `process.cwd()/storage/business-docs/...` — chdir into the temp dir - // so every test artifact lands isolated and gets cleaned up. + // `getStoragePath()/business-docs/...`, and safePath also allows a + // `process.cwd()/storage/business-docs/...` root — chdir into the temp + // dir so every test artifact lands isolated and gets cleaned up. process.chdir(tmpDir); - storageRoot = path.join(fs.realpathSync(tmpDir), 'storage', 'business-docs'); + // Mirror what the services store: the raw STORAGE_PATH bootCrmDb + // exported, NOT a symlink-resolved variant. On macOS os.tmpdir() is + // /var/... while realpath is /private/var/..., so canonicalizing here + // would make every stored path fail the prefix check. + storageRoot = path.join(process.env.STORAGE_PATH, 'business-docs'); // Fail-fast on the pre-existing logActivity-inside-transaction // deadlock: createContract and createStorno call logActivity() from diff --git a/backend/__tests__/services/backupService.enhanced.test.js b/backend/__tests__/services/backupService.enhanced.test.js index e2349727..a300b9d2 100644 --- a/backend/__tests__/services/backupService.enhanced.test.js +++ b/backend/__tests__/services/backupService.enhanced.test.js @@ -11,8 +11,27 @@ jest.mock('../../src/services/emailProcessor'); jest.mock('node-cron'); jest.mock('../../src/services/backupManifest'); jest.mock('../../src/services/storage/s3Storage'); +// runBackup lazily requires this from inside the run — resolve+register it +// here so the require doesn't hit the (mock-fs'd) filesystem mid-backup. +jest.mock('../../src/services/databaseBackup', () => ({ + databaseBackupService: { + backup: jest.fn() + } +})); +// Same deal for the rsync path's lazy requires. +jest.mock('../../src/utils/safeExec', () => ({ + spawnAsync: jest.fn(), + spawnToFile: jest.fn(), + spawnFromFile: jest.fn() +})); +jest.mock('../../src/utils/networkValidation', () => ({ + isHostAllowed: jest.fn().mockResolvedValue(true) +})); const backupService = require('../../src/services/backupService'); +const { databaseBackupService } = require('../../src/services/databaseBackup'); +const { spawnAsync } = require('../../src/utils/safeExec'); +const { isHostAllowed } = require('../../src/utils/networkValidation'); const { db } = require('../../src/database/db'); const logger = require('../../src/utils/logger'); const { queueEmail } = require('../../src/services/emailProcessor'); @@ -20,6 +39,20 @@ const cron = require('node-cron'); const backupManifest = require('../../src/services/backupManifest'); const S3StorageAdapter = require('../../src/services/storage/s3Storage'); +// `runBackup` opens the run row with `db('backup_runs').insert(...).returning('id')`, +// so the insert mock has to be awaitable AND carry a `.returning()`. +const insertResult = (value) => { + const thenable = Promise.resolve(value); + thenable.returning = jest.fn().mockResolvedValue(value); + return thenable; +}; + +// Every runBackup goes through ensureDatabaseDumpForBackup, which stats the +// DB dump on disk and refuses to continue without it — seed it into every +// mock-fs tree. +const DB_DUMP_PATH = '/backup/db-dump.sql'; +const mockStorage = (tree) => mockFs({ [DB_DUMP_PATH]: Buffer.from('database dump'), ...tree }); + describe('Enhanced Backup Service Tests', () => { let mockDb; let mockS3Client; @@ -36,7 +69,7 @@ describe('Enhanced Backup Service Tests', () => { orderBy: jest.fn().mockReturnThis(), limit: jest.fn().mockReturnThis(), first: jest.fn(), - insert: jest.fn(), + insert: jest.fn(() => insertResult([1])), update: jest.fn(), delete: jest.fn() }; @@ -75,6 +108,19 @@ describe('Enhanced Backup Service Tests', () => { logger.error = jest.fn(); logger.warn = jest.fn(); logger.debug = jest.fn(); + + // The inline DB dump and its on-disk verification run on every backup and + // throw when no dump is available — give both a passing default so each + // test can focus on the destination path it actually covers. + databaseBackupService.backup.mockResolvedValue({ path: DB_DUMP_PATH, size: 13 }); + isHostAllowed.mockResolvedValue(true); + jest.spyOn(backupService, 'getDatabaseBackupInfo').mockResolvedValue({ + type: 'sqlite', + backupFile: DB_DUMP_PATH, + size: 13, + checksum: 'abc123', + hasChanged: false + }); }); afterEach(() => { @@ -132,7 +178,7 @@ describe('Enhanced Backup Service Tests', () => { describe('S3 Backup Functionality', () => { beforeEach(() => { // Mock file system - mockFs({ + mockStorage({ '/storage/events/active/event1': { 'photo1.jpg': Buffer.from('photo1 content'), 'photo2.jpg': Buffer.from('photo2 content') @@ -165,12 +211,12 @@ describe('Enhanced Backup Service Tests', () => { mockDb.select.mockResolvedValue([]); mockDb.where.mockReturnThis(); mockDb.first.mockResolvedValue(null); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); jest.spyOn(backupService, 'getDatabaseBackupInfo').mockResolvedValue({ type: 'sqlite', - backupFile: null, + backupFile: DB_DUMP_PATH, hasChanged: true }); @@ -202,7 +248,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -240,7 +286,7 @@ describe('Enhanced Backup Service Tests', () => { }); mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -265,7 +311,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); jest.spyOn(backupService, 'getDatabaseBackupInfo').mockResolvedValue({ @@ -277,7 +323,7 @@ describe('Enhanced Backup Service Tests', () => { }); // Mock database backup file - mockFs({ + mockStorage({ '/storage/events/active': {}, '/backup/db-backup.sql': Buffer.from('database backup content') }); @@ -301,7 +347,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -326,12 +372,12 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - mockFs({ + mockStorage({ '/storage/events/active/event1': { 'photo1.jpg': Buffer.from('photo1 content') }, @@ -365,7 +411,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([2]); + mockDb.insert.mockReturnValue(insertResult([2])); mockDb.first.mockImplementation(() => Promise.resolve(lastBackup)); mockDb.orderBy.mockReturnThis(); mockDb.where.mockReturnThis(); @@ -373,7 +419,7 @@ describe('Enhanced Backup Service Tests', () => { jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - mockFs({ + mockStorage({ '/storage/events/active': {}, '/backup': {} }); @@ -395,7 +441,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -406,7 +452,7 @@ describe('Enhanced Backup Service Tests', () => { }; backupManifest.generateManifest.mockResolvedValue(manifest); - mockFs({ + mockStorage({ '/storage/events/active': {}, '/storage/temp': {} }); @@ -431,12 +477,12 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - mockFs({ + mockStorage({ '/storage/events/active/event1': { 'photo1.jpg': Buffer.from('photo1 content') }, @@ -461,28 +507,27 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - - // Mock exec for rsync - const { exec } = require('child_process'); - const mockExec = jest.fn((cmd, callback) => { - callback(null, { stdout: 'Number of files transferred: 1\nTotal file size: 1024 bytes' }); + + // rsync is spawned argv-style (no shell) — assert that shape, not the + // legacy `exec('rsync ...')` string. + spawnAsync.mockResolvedValue({ + stdout: 'Number of files transferred: 1\nTotal file size: 1024 bytes' }); - exec.mockImplementation(mockExec); - - mockFs({ + + mockStorage({ '/storage/events/active': {} }); - + await backupService.runBackup(); - - expect(mockExec).toHaveBeenCalledWith( - expect.stringContaining('rsync'), - expect.any(Function) - ); + + expect(spawnAsync).toHaveBeenCalledWith('rsync', expect.any(Array)); + const [, rsyncArgs] = spawnAsync.mock.calls[0]; + expect(rsyncArgs).toContain('-avz'); + expect(rsyncArgs[rsyncArgs.length - 1]).toBe('backup@backup.example.com:/remote/backup'); }); }); @@ -497,7 +542,7 @@ describe('Enhanced Backup Service Tests', () => { }; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.first.mockResolvedValue(null); jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); @@ -514,7 +559,7 @@ describe('Enhanced Backup Service Tests', () => { return originalCreateReadStream(path); }); - mockFs({ + mockStorage({ '/storage/events/active': { 'error.jpg': Buffer.from('content'), 'good.jpg': Buffer.from('content') @@ -546,7 +591,7 @@ describe('Enhanced Backup Service Tests', () => { ]; mockDb.select.mockResolvedValue([]); - mockDb.insert.mockResolvedValue([1]); + mockDb.insert.mockReturnValue(insertResult([1])); mockDb.where.mockReturnThis(); jest.spyOn(backupService, 'getBackupConfig') @@ -555,7 +600,13 @@ describe('Enhanced Backup Service Tests', () => { // Force an error jest.spyOn(backupService, 'getFilesToBackup').mockRejectedValue(new Error('Storage error')); - + + // The DB-dump verification runs first and would throw its own error — + // give it a tree so 'Storage error' is what actually surfaces. + mockStorage({ + '/storage/events/active': {} + }); + // Mock admin users query db.mockImplementation((table) => { if (table === 'admin_users') { @@ -588,7 +639,7 @@ describe('Enhanced Backup Service Tests', () => { jest.spyOn(backupService, 'getBackupConfig').mockResolvedValue(config); - mockFs({ + mockStorage({ '/storage/events/active': {}, '/backup': {} }); @@ -675,20 +726,32 @@ describe('Enhanced Backup Service Tests', () => { ]; mockDb.limit.mockResolvedValue(recentRuns); - + // getBackupStatus also reads the backup config to compute the next run; + // an unscheduled/disabled backup legitimately yields null (#871). + mockDb.select.mockResolvedValue([ + { setting_key: 'backup_enabled', setting_value: 'true' }, + { setting_key: 'backup_schedule', setting_value: '"daily"' } + ]); + backupManifest.validateManifest.mockImplementation(() => true); - + const status = await backupService.getBackupStatus(); - + + // Runs are returned with a `created_at` alias for the frontend. + const run = { ...recentRuns[0], created_at: recentRuns[0].started_at }; + expect(status).toEqual({ isRunning: false, isHealthy: true, - lastRun: expect.objectContaining({ - ...recentRuns[0], - manifestValid: true - }), - recentRuns: recentRuns, - nextScheduledRun: expect.any(String) + lastRun: { ...run, manifestValid: true }, + lastBackup: { ...run, manifestValid: true }, + lastSuccessfulBackup: run, + zombieRuns: [], + recentRuns: [run], + recentBackups: [run], + totalBackups: 1, + nextScheduledRun: expect.any(String), + nextBackup: expect.any(String) }); }); diff --git a/backend/src/routes/__tests__/adminAuth.test.js b/backend/src/routes/__tests__/adminAuth.test.js index 934fd464..8ce68347 100644 --- a/backend/src/routes/__tests__/adminAuth.test.js +++ b/backend/src/routes/__tests__/adminAuth.test.js @@ -36,11 +36,13 @@ jest.mock('../../middleware/auth', () => ({ const { db, logActivity } = require('../../database/db'); const adminAuthRouter = require('../adminAuth'); +const { errorHandler } = require('../../middleware/errorHandler'); describe('adminAuth profile updates', () => { const app = express(); app.use(express.json()); app.use('/auth/admin', adminAuthRouter); + app.use(errorHandler); beforeEach(() => { jest.clearAllMocks(); @@ -55,8 +57,8 @@ describe('adminAuth profile updates', () => { }; db.__setImplementations( - buildChain({ firstResult: null }), // email check buildChain({ firstResult: null }), // username check + buildChain({ firstResult: null }), // email check buildChain({ updateResult: 1 }), // update buildChain({ firstResult: updatedUser }), // fetch updated user ); @@ -66,18 +68,22 @@ describe('adminAuth profile updates', () => { .send({ username: updatedUser.username, email: updatedUser.email }) .expect(200); - expect(response.body).toEqual({ user: updatedUser }); + expect(response.body).toEqual({ + message: 'Admin profile updated successfully', + user: updatedUser + }); expect(logActivity).toHaveBeenCalledWith( 'admin_profile_updated', - { admin_id: 1, updated_fields: ['username', 'email'] }, + { username: updatedUser.username, email: updatedUser.email }, null, - { type: 'admin', id: 1, name: updatedUser.username } + { type: 'admin', id: 1, name: 'admin' } ); }); it('rejects email conflicts', async () => { db.__setImplementations( - buildChain({ firstResult: { id: 2 } }) + buildChain({ firstResult: null }), // username check + buildChain({ firstResult: { id: 2 } }), // email check ); const response = await request(app) @@ -85,7 +91,11 @@ describe('adminAuth profile updates', () => { .send({ username: 'newadmin', email: 'taken@example.com' }) .expect(409); - expect(response.body).toEqual({ error: 'Email is already in use by another admin' }); + expect(response.body).toEqual({ + error: 'Email address is already in use', + code: 'CONFLICT', + field: 'email' + }); }); it('validates input', async () => { @@ -94,6 +104,6 @@ describe('adminAuth profile updates', () => { .send({ username: '', email: 'not-an-email' }) .expect(400); - expect(response.body.errors).toBeDefined(); + expect(response.body.details).toBeDefined(); }); });