From 70bd03dc1d7d30e43da3a56247d1b3ae84ac495d Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Wed, 9 Sep 2026 22:09:23 +0200 Subject: [PATCH] fix(backup): close two gaps codex round 2 found in the destination guard - The public-roots list missed the bundled fallback fonts dir (backend/assets/fonts, also mounted at /fonts, and nodejs-owned per the Dockerfile's COPY --chown so it's writable at runtime). - The comparison was case-sensitive; on a case-insensitive-but- preserving filesystem (APFS, NTFS, Docker Desktop bind mounts of either) STORAGE_PATH/UPLOADS/Logos names the same directory as uploads/logos on disk. Now compares lowercased. - database_backup_retention_days reached cleanupOldBackups unvalidated. A value <= 0 pushes the cutoff to today or the future, deleting every completed backup on the next scheduled run -- a backup.create holder achieving what backup.delete gates on the manual /cleanup route. Rejected at config-write time (400) and defensively inside cleanupOldBackups itself. - The scheduled-backup cron callback closed over retention_days from schedule-start time; a retention-only /config update (which doesn't restart the schedule) ran stale until restart. Re-reads it on every tick instead. Found by codex review, round 2. --- .../databaseBackupConfigDestination.test.js | 24 +++++++++ backend/src/routes/adminDatabaseBackup.js | 10 ++++ .../services/__tests__/databaseBackup.test.js | 53 ++++++++++++++++++- backend/src/services/databaseBackup.js | 33 ++++++++++-- 4 files changed, 113 insertions(+), 7 deletions(-) diff --git a/backend/__tests__/routes/databaseBackupConfigDestination.test.js b/backend/__tests__/routes/databaseBackupConfigDestination.test.js index 2fb6f490..8adb5160 100644 --- a/backend/__tests__/routes/databaseBackupConfigDestination.test.js +++ b/backend/__tests__/routes/databaseBackupConfigDestination.test.js @@ -95,4 +95,28 @@ describe('database backup destination-path config guard (GHSA-jw8m class, #1365) const row = await db('app_settings').where({ setting_key: 'database_backup_destination_path' }).first(); expect(JSON.parse(row.setting_value)).toBe(safePath); }); + + // A retention of 0 or less pushes cleanupOldBackups' cutoff to today or + // the future, deleting every completed backup on the next scheduled run + // — a backup.create holder achieving what backup.delete gates on /cleanup. + it.each([-1, 0])('rejects database_backup_retention_days=%s', async (bad) => { + const res = await request(app) + .put('/api/admin/database-backup/config') + .set('Authorization', `Bearer ${adminToken}`) + .send({ database_backup_retention_days: bad }); + + expect(res.status).toBe(400); + }); + + it('accepts a positive database_backup_retention_days', async () => { + const res = await request(app) + .put('/api/admin/database-backup/config') + .set('Authorization', `Bearer ${adminToken}`) + .send({ database_backup_retention_days: 90 }); + + expect(res.status).toBe(200); + + const row = await db('app_settings').where({ setting_key: 'database_backup_retention_days' }).first(); + expect(JSON.parse(row.setting_value)).toBe(90); + }); }); diff --git a/backend/src/routes/adminDatabaseBackup.js b/backend/src/routes/adminDatabaseBackup.js index ef5384aa..4bf7f56d 100644 --- a/backend/src/routes/adminDatabaseBackup.js +++ b/backend/src/routes/adminDatabaseBackup.js @@ -72,6 +72,16 @@ router.put('/config', requirePermission('backup.create'), async (req, res) => { return res.status(400).json({ error: 'Destination path must not be inside a publicly served directory' }); } + // A retention of 0 or less pushes cleanupOldBackups' cutoff to today or + // the future, deleting every completed backup on the next scheduled run + // — a backup.create holder achieving what backup.delete gates on /cleanup. + if ( + req.body.database_backup_retention_days !== undefined + && (!Number.isFinite(req.body.database_backup_retention_days) || req.body.database_backup_retention_days < 1) + ) { + return res.status(400).json({ error: 'database_backup_retention_days must be a positive number' }); + } + const updates = []; for (const [key, value] of Object.entries(req.body)) { diff --git a/backend/src/services/__tests__/databaseBackup.test.js b/backend/src/services/__tests__/databaseBackup.test.js index 5a739caa..761dbbd9 100644 --- a/backend/src/services/__tests__/databaseBackup.test.js +++ b/backend/src/services/__tests__/databaseBackup.test.js @@ -10,7 +10,7 @@ jest.mock('../emailProcessor'); jest.mock('child_process'); jest.mock('node-cron', () => ({ schedule: jest.fn(() => ({ stop: jest.fn() })) })); -const { DatabaseBackupService, startScheduledBackups, isUnderPubliclyServableRoot } = require('../databaseBackup'); +const { DatabaseBackupService, startScheduledBackups, databaseBackupService, isUnderPubliclyServableRoot } = require('../databaseBackup'); const cron = require('node-cron'); describe('DatabaseBackupService', () => { @@ -249,7 +249,14 @@ describe('DatabaseBackupService', () => { path.join(storage, 'uploads', 'logos', 'sub'), path.join(storage, 'uploads', 'favicons'), path.join(storage, 'fonts'), - path.join(storage, 'fonts', 'inter') + path.join(storage, 'fonts', 'inter'), + // Bundled fallback fonts — nodejs-owned per the Dockerfile's + // COPY --chown, and served at the same public /fonts route. + path.resolve(__dirname, '../../../assets/fonts'), + // Case-insensitive-but-preserving filesystems (APFS, NTFS, Docker + // Desktop bind mounts of either) resolve this to the same directory + // as uploads/logos even though path.resolve() never folds case. + path.join(storage, 'UPLOADS', 'Logos') ])('flags %s as publicly servable', (candidate) => { expect(isUnderPubliclyServableRoot(candidate)).toBe(true); }); @@ -311,6 +318,48 @@ describe('DatabaseBackupService', () => { expect(cron.schedule).toHaveBeenCalledWith('0 4 * * *', expect.any(Function)); }); + + it('re-reads retention on every tick instead of the value captured at schedule start (#1365)', async () => { + db.mockReturnValue({ + where: jest.fn().mockReturnThis(), + select: jest.fn().mockResolvedValue([ + { setting_key: 'database_backup_enabled', setting_value: 'true' }, + { setting_key: 'database_backup_retention_days', setting_value: JSON.stringify(30) } + ]) + }); + + await startScheduledBackups(); + const tick = cron.schedule.mock.calls[0][1]; + + // A /config update between schedule-start and this tick raised + // retention to 365 — the closed-over 30 must not be what runs. + db.mockReturnValue({ + where: jest.fn().mockReturnThis(), + select: jest.fn().mockResolvedValue([ + { setting_key: 'database_backup_enabled', setting_value: 'true' }, + { setting_key: 'database_backup_retention_days', setting_value: JSON.stringify(365) } + ]) + }); + jest.spyOn(databaseBackupService, 'backup').mockResolvedValue({ success: true }); + const cleanupSpy = jest.spyOn(databaseBackupService, 'cleanupOldBackups').mockResolvedValue(undefined); + + await tick(); + + expect(cleanupSpy).toHaveBeenCalledWith(365); + + jest.restoreAllMocks(); + }); + }); + + describe('cleanupOldBackups destructive-retention guard (#1365)', () => { + it.each([-1, 0, NaN, Infinity])('refuses retentionDays=%s without touching the database', async (bad) => { + const dbSpy = jest.fn(); + db.mockImplementation(dbSpy); + + await service.cleanupOldBackups(bad); + + expect(dbSpy).not.toHaveBeenCalled(); + }); }); describe('cleanupOldBackups', () => { diff --git a/backend/src/services/databaseBackup.js b/backend/src/services/databaseBackup.js index 58bd1e39..6a069fae 100644 --- a/backend/src/services/databaseBackup.js +++ b/backend/src/services/databaseBackup.js @@ -34,14 +34,23 @@ function getPubliclyServableRoots() { return [ path.join(storage, 'uploads', 'logos'), path.join(storage, 'uploads', 'favicons'), - path.join(storage, 'fonts') + path.join(storage, 'fonts'), + // Bundled fallback fonts (server.js mounts both at /fonts, storage wins + // on overlap but express.static falls through to this one on a miss). + // COPY --chown=nodejs:nodejs in the Dockerfile makes this nodejs-owned + // and therefore writable at runtime, not just a read-only image layer. + path.resolve(__dirname, '../../assets/fonts') ]; } function isUnderPubliclyServableRoot(candidatePath) { - const resolved = path.resolve(candidatePath); + // Lowercased comparison: on a case-insensitive-but-preserving filesystem + // (default macOS APFS, NTFS, and Docker Desktop's bind-mount passthrough + // of either) `STORAGE_PATH/UPLOADS/logos` and `.../uploads/logos` name the + // same directory on disk even though path.resolve() never folds case. + const resolved = path.resolve(candidatePath).toLowerCase(); return getPubliclyServableRoots().some((root) => { - const resolvedRoot = path.resolve(root); + const resolvedRoot = path.resolve(root).toLowerCase(); return resolved === resolvedRoot || resolved.startsWith(resolvedRoot + path.sep); }); } @@ -586,10 +595,19 @@ class DatabaseBackupService { * Clean up old backups */ async cleanupOldBackups(retentionDays = 30) { + // A zero/negative/non-finite value pushes the cutoff to today or the + // future, matching (and deleting) every completed backup — including + // the one a scheduled run just created. Defense in depth: PUT /config + // already rejects such values, but this is also reachable with + // whatever database_backup_retention_days happens to be persisted. + if (!Number.isFinite(retentionDays) || retentionDays < 1) { + logger.error(`Refusing to clean up backups with invalid retentionDays: ${retentionDays}`); + return; + } try { const cutoffDate = new Date(); cutoffDate.setDate(cutoffDate.getDate() - retentionDays); - + // Get old backup records const oldBackups = await db('database_backup_runs') .where('completed_at', '<', cutoffDate) @@ -752,7 +770,12 @@ async function startScheduledBackups() { logger.info('Starting scheduled database backup'); try { await databaseBackupService.backup(); - await databaseBackupService.cleanupOldBackups(config.database_backup_retention_days || 30); + // Re-read retention on every tick rather than closing over the value + // from schedule start — a retention-only /config update doesn't + // restart the schedule (only enabled/schedule changes do), so the + // closed-over value would otherwise run stale until next restart. + const latestConfig = await databaseBackupService.getBackupConfig(); + await databaseBackupService.cleanupOldBackups(latestConfig.database_backup_retention_days || 30); } catch (error) { logger.error('Scheduled database backup failed:', error); }