Merge pull request #807 from PicPeak/fix/settings-secret-exposure-mfa-maintenance

fix(security): mask backup credentials on read + unblock MFA login during maintenance

backend/__tests__/integration/backupSecretMasking.test.js

@@ -0,0 +1,108 @@
+/**
+ * Backup credential exposure regression tests.
+ *
+ * The generic settings reads (GET /admin/settings, GET /admin/settings/:type)
+ * masked the recaptcha/umami/rybbit keys but returned backup_s3_secret_key
+ * and backup_rsync_ssh_key (an SSH PRIVATE KEY) in plaintext to any
+ * settings.view holder; GET /admin/backup/config returned them too. Both now
+ * mask, and PUT /admin/backup/config skips the mask sentinel so the edit
+ * form round-trips without clobbering stored credentials.
+ */
+
+const request = require('supertest');
+const express = require('express');
+
+const { bootCrmDb } = require('./helpers/crmDb');
+
+jest.mock('../../src/middleware/auth', () => ({
+  adminAuth: (req, _res, next) => {
+    req.admin = { id: 1, username: 'test-admin' };
+    next();
+  },
+}));
+jest.mock('../../src/middleware/permissions', () => ({
+  requirePermission: () => (_req, _res, next) => next(),
+}));
+
+describe('backup credential masking', () => {
+  let db;
+  let cleanup;
+  let app;
+
+  beforeAll(async () => {
+    ({ db, cleanup } = await bootCrmDb());
+
+    // Upsert: several backup_* keys are pre-seeded by the backup migrations.
+    const seed = [
+      { setting_key: 'backup_destination_type', setting_value: JSON.stringify('s3'), setting_type: 'backup' },
+      { setting_key: 'backup_s3_endpoint', setting_value: JSON.stringify('https://s3.example.com'), setting_type: 'backup' },
+      { setting_key: 'backup_s3_bucket', setting_value: JSON.stringify('backups'), setting_type: 'backup' },
+      { setting_key: 'backup_s3_access_key', setting_value: JSON.stringify('AKIAEXAMPLE'), setting_type: 'backup' },
+      { setting_key: 'backup_s3_secret_key', setting_value: JSON.stringify('super-secret-s3-key'), setting_type: 'backup' },
+      { setting_key: 'backup_rsync_ssh_key', setting_value: JSON.stringify('-----BEGIN OPENSSH PRIVATE KEY-----abc'), setting_type: 'backup' },
+    ];
+    for (const row of seed) {
+      await db('app_settings').insert(row).onConflict('setting_key').merge();
+    }
+
+    app = express();
+    app.use(express.json());
+    app.use('/api/admin/backup', require('../../src/routes/adminBackup'));
+    app.use('/api/admin/settings', require('../../src/routes/adminSettings'));
+  }, 120000);
+
+  afterAll(async () => {
+    if (cleanup) await cleanup();
+  });
+
+  it('masks the credentials in GET /admin/backup/config', async () => {
+    const res = await request(app).get('/api/admin/backup/config').expect(200);
+    expect(res.body.backup_s3_secret_key).toBe('••••••••');
+    expect(res.body.backup_rsync_ssh_key).toBe('••••••••');
+    // Non-secret fields stay readable for the form.
+    expect(res.body.backup_s3_bucket).toBe('backups');
+  });
+
+  it('masks the credentials in the generic GET /admin/settings/:type read', async () => {
+    const res = await request(app).get('/api/admin/settings/backup').expect(200);
+    expect(res.body.backup_s3_secret_key).toBe('••••••••');
+    expect(res.body.backup_rsync_ssh_key).toBe('••••••••');
+  });
+
+  it('masks the credentials in the generic GET /admin/settings read', async () => {
+    const res = await request(app).get('/api/admin/settings').expect(200);
+    expect(res.body.backup_s3_secret_key).toBe('••••••••');
+    expect(res.body.backup_rsync_ssh_key).toBe('••••••••');
+  });
+
+  it('PUT /admin/backup/config keeps the stored secret when the sentinel round-trips', async () => {
+    await request(app)
+      .put('/api/admin/backup/config')
+      .send({
+        backup_destination_type: 's3',
+        backup_s3_endpoint: 'https://s3.example.com',
+        backup_s3_bucket: 'renamed-bucket',
+        backup_s3_access_key: 'AKIAEXAMPLE',
+        backup_s3_secret_key: '••••••••',
+        backup_rsync_ssh_key: '••••••••',
+      })
+      .expect(200);
+
+    const secret = await db('app_settings').where({ setting_key: 'backup_s3_secret_key' }).first();
+    expect(JSON.parse(secret.setting_value)).toBe('super-secret-s3-key');
+    const sshKey = await db('app_settings').where({ setting_key: 'backup_rsync_ssh_key' }).first();
+    expect(JSON.parse(sshKey.setting_value)).toBe('-----BEGIN OPENSSH PRIVATE KEY-----abc');
+    const bucket = await db('app_settings').where({ setting_key: 'backup_s3_bucket' }).first();
+    expect(JSON.parse(bucket.setting_value)).toBe('renamed-bucket');
+  });
+
+  it('PUT /admin/backup/config stores a genuinely new secret', async () => {
+    await request(app)
+      .put('/api/admin/backup/config')
+      .send({ backup_s3_secret_key: 'rotated-s3-key' })
+      .expect(200);
+
+    const secret = await db('app_settings').where({ setting_key: 'backup_s3_secret_key' }).first();
+    expect(JSON.parse(secret.setting_value)).toBe('rotated-s3-key');
+  });
+});

backend/src/middleware/maintenance.js

@@ -73,6 +73,10 @@ async function maintenanceMiddleware(req, res, next) {
   // entries here matched nothing, which is exactly why the lockout happened).
   const skipPaths = [
     '/api/auth/admin/login',
+    // The second factor is part of the same login — without this, any
+    // MFA-enrolled admin gets a 503 on the verify step and cannot sign in
+    // at all while maintenance mode is on.
+    '/api/auth/admin/login/mfa',
     '/api/auth/session',
     '/api/public/settings',
     '/health'

backend/src/routes/adminBackup.js

@@ -30,6 +30,12 @@ router.get('/config', adminAuth, requirePermission('backup.view'), async (req, r
       }
     });
 
+    // Never return the stored credentials — mask like the email/WhatsApp
+    // config endpoints do. The PUT below skips the mask sentinel, so the
+    // form round-trips without clobbering the real values.
+    if (config.backup_s3_secret_key) config.backup_s3_secret_key = '••••••••';
+    if (config.backup_rsync_ssh_key) config.backup_rsync_ssh_key = '••••••••';
+
     res.json(config);
   } catch (error) {
     errorResponse(res, error, 500, 'Failed to get backup configuration');
@@ -65,6 +71,11 @@ router.put('/config', adminAuth, requirePermission('backup.create'), async (req,
     
     // Update settings
     for (const [key, value] of Object.entries(updates)) {
+      // An unchanged secret round-trips as the GET mask sentinel — keep the
+      // stored value instead of overwriting it with bullets.
+      if (value === '••••••••') {
+        continue;
+      }
       if (key.startsWith('backup_')) {
         await db('app_settings')
           .insert({

backend/src/routes/adminSettings.js

@@ -160,6 +160,16 @@ router.get('/', adminAuth, requirePermission('settings.view'), async (req, res)
     if (settingsObject.security_recaptcha_secret_key) {
       settingsObject.security_recaptcha_secret_key = '••••••••';
     }
+    // Backup credentials — the S3 secret key and the rsync SSH PRIVATE KEY
+    // were returned in plaintext to any settings.view holder. Same masking
+    // pattern as the recaptcha/umami/rybbit keys; the dedicated
+    // /admin/backup/config endpoints handle the edit round-trip.
+    if (settingsObject.backup_s3_secret_key) {
+      settingsObject.backup_s3_secret_key = '••••••••';
+    }
+    if (settingsObject.backup_rsync_ssh_key) {
+      settingsObject.backup_rsync_ssh_key = '••••••••';
+    }
     // Umami v2 API key (#661 Bug C) — read-write secret that authenticates
     // outbound calls to the operator's Umami instance for the device
     // breakdown. Masked on GET, same pattern as the recaptcha secret.
@@ -410,6 +420,16 @@ router.get('/:type', adminAuth, requirePermission('settings.view'), async (req,
     if (settingsObject.security_recaptcha_secret_key) {
       settingsObject.security_recaptcha_secret_key = '••••••••';
     }
+    // Backup credentials — the S3 secret key and the rsync SSH PRIVATE KEY
+    // were returned in plaintext to any settings.view holder. Same masking
+    // pattern as the recaptcha/umami/rybbit keys; the dedicated
+    // /admin/backup/config endpoints handle the edit round-trip.
+    if (settingsObject.backup_s3_secret_key) {
+      settingsObject.backup_s3_secret_key = '••••••••';
+    }
+    if (settingsObject.backup_rsync_ssh_key) {
+      settingsObject.backup_rsync_ssh_key = '••••••••';
+    }
     // Umami v2 API key (#661 Bug C) — read-write secret that authenticates
     // outbound calls to the operator's Umami instance for the device
     // breakdown. Masked on GET, same pattern as the recaptcha secret.