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.

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);
+  });
 });

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)) {

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', () => {

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);
       }