fix(security): mask backup credentials on read + unblock MFA login during maintenance
Two pre-existing bugs surfaced while reviewing #806 (kept separate per scope policy — no OIDC code here): - backup_s3_secret_key and backup_rsync_ssh_key (an SSH PRIVATE KEY) were returned in PLAINTEXT by GET /admin/backup/config and by the generic settings reads (GET /admin/settings and /admin/settings/:type — which mask the recaptcha/umami/rybbit keys but not these). All three now mask with the established bullet sentinel, and PUT /admin/backup/config skips the sentinel on write so the edit form round-trips without clobbering stored credentials (same pattern as the email/WhatsApp config endpoints) - /api/auth/admin/login/mfa was missing from the maintenance-mode allowlist: the first login step passed, the second factor got a 503 — any MFA-enrolled admin was locked out exactly while maintenance mode was on Regression tests: masking on all three read paths, sentinel round-trip preserves stored values, real rotation still writes.
This commit is contained in:
@@ -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');
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -73,6 +73,10 @@ async function maintenanceMiddleware(req, res, next) {
|
|||||||
// entries here matched nothing, which is exactly why the lockout happened).
|
// entries here matched nothing, which is exactly why the lockout happened).
|
||||||
const skipPaths = [
|
const skipPaths = [
|
||||||
'/api/auth/admin/login',
|
'/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/auth/session',
|
||||||
'/api/public/settings',
|
'/api/public/settings',
|
||||||
'/health'
|
'/health'
|
||||||
|
|||||||
@@ -29,7 +29,13 @@ router.get('/config', adminAuth, requirePermission('backup.view'), async (req, r
|
|||||||
config[setting.setting_key] = setting.setting_value;
|
config[setting.setting_key] = setting.setting_value;
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// 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);
|
res.json(config);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
errorResponse(res, error, 500, 'Failed to get backup configuration');
|
errorResponse(res, error, 500, 'Failed to get backup configuration');
|
||||||
@@ -65,6 +71,11 @@ router.put('/config', adminAuth, requirePermission('backup.create'), async (req,
|
|||||||
|
|
||||||
// Update settings
|
// Update settings
|
||||||
for (const [key, value] of Object.entries(updates)) {
|
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_')) {
|
if (key.startsWith('backup_')) {
|
||||||
await db('app_settings')
|
await db('app_settings')
|
||||||
.insert({
|
.insert({
|
||||||
|
|||||||
@@ -160,6 +160,16 @@ router.get('/', adminAuth, requirePermission('settings.view'), async (req, res)
|
|||||||
if (settingsObject.security_recaptcha_secret_key) {
|
if (settingsObject.security_recaptcha_secret_key) {
|
||||||
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
|
// Umami v2 API key (#661 Bug C) — read-write secret that authenticates
|
||||||
// outbound calls to the operator's Umami instance for the device
|
// outbound calls to the operator's Umami instance for the device
|
||||||
// breakdown. Masked on GET, same pattern as the recaptcha secret.
|
// 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) {
|
if (settingsObject.security_recaptcha_secret_key) {
|
||||||
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
|
// Umami v2 API key (#661 Bug C) — read-write secret that authenticates
|
||||||
// outbound calls to the operator's Umami instance for the device
|
// outbound calls to the operator's Umami instance for the device
|
||||||
// breakdown. Masked on GET, same pattern as the recaptcha secret.
|
// breakdown. Masked on GET, same pattern as the recaptcha secret.
|
||||||
|
|||||||
Reference in New Issue
Block a user