diff --git a/backend/__tests__/routes/databaseBackupConfigDestination.test.js b/backend/__tests__/routes/databaseBackupConfigDestination.test.js new file mode 100644 index 00000000..2fb6f490 --- /dev/null +++ b/backend/__tests__/routes/databaseBackupConfigDestination.test.js @@ -0,0 +1,98 @@ +/** + * PUT /api/admin/database-backup/config must reject a + * database_backup_destination_path that resolves inside a publicly served + * directory (GHSA-jw8m-43r2-jqrm class, #1365). + * + * Before #1365, database_backup_destination_path was silently ignored by + * databaseBackupService.backup() (a destructuring bug always fell back to + * the hardcoded /backup/database), so this setting being freely writable by + * any backup.create holder — the built-in `admin` role has it without + * settings.edit or backup.restore — was harmless. Making the setting + * actually take effect reopens the exact exfiltration path GHSA-jw8m fixed + * for the per-request override, through the persisted setting instead. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-dbbackup-config-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'dbbackup-config-test-secret'; +process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-dbbackup-storage-')); + +const request = require('supertest'); +const express = require('express'); +const bcrypt = require('bcrypt'); +const jwt = require('jsonwebtoken'); + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +describe('database backup destination-path config guard (GHSA-jw8m class, #1365)', () => { + let db; let cleanup; let app; let adminToken; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + + const role = await db('roles').where({ name: 'admin' }).first(); + const r = await db('admin_users').insert({ + username: 'limited-admin', + email: 'limited-admin-config@example.com', + password_hash: await bcrypt.hash('Passw0rd!', 4), + role_id: role.id, + is_active: 1, + created_at: new Date(), + updated_at: new Date(), + }).returning('id'); + const id = r[0]?.id ?? r[0]; + adminToken = jwt.sign( + { id, username: 'limited-admin', type: 'admin', role: 'admin', loginTime: Date.now() }, + process.env.JWT_SECRET, + { expiresIn: '1h', issuer: 'picpeak-auth' }, + ); + + app = express(); + app.use(express.json()); + app.use('/api/admin/database-backup', require('../../src/routes/adminDatabaseBackup')); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + it('rejects a destination inside the public uploads/logos mount', async () => { + const res = await request(app) + .put('/api/admin/database-backup/config') + .set('Authorization', `Bearer ${adminToken}`) + .send({ database_backup_destination_path: path.join(process.env.STORAGE_PATH, 'uploads', 'logos') }); + + expect(res.status).toBe(400); + + // The seeded default must survive untouched — the rejected value never lands. + const row = await db('app_settings').where({ setting_key: 'database_backup_destination_path' }).first(); + expect(JSON.parse(row.setting_value)).toBe('/backup/database'); + }); + + it('rejects a destination inside the public fonts mount', async () => { + const res = await request(app) + .put('/api/admin/database-backup/config') + .set('Authorization', `Bearer ${adminToken}`) + .send({ database_backup_destination_path: path.join(process.env.STORAGE_PATH, 'fonts') }); + + expect(res.status).toBe(400); + }); + + it('accepts a destination outside any public mount', async () => { + const safePath = path.join(process.env.STORAGE_PATH, 'db-backups'); + const res = await request(app) + .put('/api/admin/database-backup/config') + .set('Authorization', `Bearer ${adminToken}`) + .send({ database_backup_destination_path: safePath }); + + expect(res.status).toBe(200); + + const row = await db('app_settings').where({ setting_key: 'database_backup_destination_path' }).first(); + expect(JSON.parse(row.setting_value)).toBe(safePath); + }); +}); diff --git a/backend/src/routes/adminDatabaseBackup.js b/backend/src/routes/adminDatabaseBackup.js index b4114091..ef5384aa 100644 --- a/backend/src/routes/adminDatabaseBackup.js +++ b/backend/src/routes/adminDatabaseBackup.js @@ -2,7 +2,7 @@ const express = require('express'); const router = express.Router(); const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); -const { databaseBackupService } = require('../services/databaseBackup'); +const { databaseBackupService, isUnderPubliclyServableRoot } = require('../services/databaseBackup'); const { db } = require('../database/db'); const logger = require('../utils/logger'); const { getPagination } = require('../utils/routeHelpers'); @@ -60,7 +60,18 @@ router.put('/config', requirePermission('backup.create'), async (req, res) => { 'database_backup_email_on_failure', 'database_backup_email_on_success' ]; - + + // A backup.create holder (the built-in `admin` role has it without + // settings.edit or backup.restore) could otherwise point backups at a + // public static mount and fetch the dump unauthenticated — see + // isUnderPubliclyServableRoot's comment (GHSA-jw8m-43r2-jqrm class). + if ( + typeof req.body.database_backup_destination_path === 'string' + && isUnderPubliclyServableRoot(req.body.database_backup_destination_path) + ) { + return res.status(400).json({ error: 'Destination path must not be inside a publicly served directory' }); + } + 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 d66121c0..fded51ab 100644 --- a/backend/src/services/__tests__/databaseBackup.test.js +++ b/backend/src/services/__tests__/databaseBackup.test.js @@ -1,5 +1,6 @@ const { db } = require('../../database/db'); const fs = require('fs').promises; +const path = require('path'); const crypto = require('crypto'); // Mock dependencies @@ -9,7 +10,7 @@ jest.mock('../emailProcessor'); jest.mock('child_process'); jest.mock('node-cron', () => ({ schedule: jest.fn(() => ({ stop: jest.fn() })) })); -const { DatabaseBackupService, startScheduledBackups } = require('../databaseBackup'); +const { DatabaseBackupService, startScheduledBackups, isUnderPubliclyServableRoot } = require('../databaseBackup'); const cron = require('node-cron'); describe('DatabaseBackupService', () => { @@ -231,6 +232,58 @@ describe('DatabaseBackupService', () => { }); }); + describe('isUnderPubliclyServableRoot (GHSA-jw8m class, #1365)', () => { + const originalStoragePath = process.env.STORAGE_PATH; + const storage = '/tmp/picpeak-test-storage'; + + beforeEach(() => { + process.env.STORAGE_PATH = storage; + }); + + afterAll(() => { + if (originalStoragePath === undefined) { + delete process.env.STORAGE_PATH; + } else { + process.env.STORAGE_PATH = originalStoragePath; + } + }); + + it.each([ + path.join(storage, 'uploads', 'logos'), + path.join(storage, 'uploads', 'logos', 'sub'), + path.join(storage, 'uploads', 'favicons'), + path.join(storage, 'fonts'), + path.join(storage, 'fonts', 'inter') + ])('flags %s as publicly servable', (candidate) => { + expect(isUnderPubliclyServableRoot(candidate)).toBe(true); + }); + + it.each([ + path.join(storage, 'backups'), + path.join(storage, 'uploads', 'contracts', 'signed'), + path.join(storage, 'uploads', 'transfers', '123'), + '/data/db-backups' + ])('does not flag %s', (candidate) => { + expect(isUnderPubliclyServableRoot(candidate)).toBe(false); + }); + + it('backup() refuses a destination inside a publicly servable root without ever calling mkdir', async () => { + const publicPath = path.join(storage, 'uploads', 'logos'); + db.mockReturnValue({ + where: jest.fn().mockReturnThis(), + select: jest.fn().mockResolvedValue([ + { setting_key: 'database_backup_destination_path', setting_value: JSON.stringify(publicPath) } + ]) + }); + + const mkdirSpy = jest.spyOn(fs, 'mkdir'); + + await expect(service.backup({})).rejects.toThrow('publicly served directory'); + + expect(mkdirSpy).not.toHaveBeenCalled(); + }); + }); + describe('startScheduledBackups (#1365)', () => { // Same key-mismatch bug as backup(): getBackupConfig() returns // database_backup_*-prefixed keys, but this read `config.enabled` / diff --git a/backend/src/services/databaseBackup.js b/backend/src/services/databaseBackup.js index 937a8c60..b388676c 100644 --- a/backend/src/services/databaseBackup.js +++ b/backend/src/services/databaseBackup.js @@ -28,6 +28,36 @@ const packageJson = require('../../package.json'); // createSQLiteBackup below. const FACE_TABLES = ['photo_faces', 'event_people', 'event_people_merge_dismissals']; +function getStoragePath() { + return process.env.STORAGE_PATH || path.join(__dirname, '../../../storage'); +} + +// Public, unauthenticated static mounts (server.js) that must never become a +// backup destination — a dump landing there is downloadable by anyone who +// learns or guesses the filename, GHSA-jw8m-43r2-jqrm's exact class. Before +// #1365, `database_backup_destination_path` was silently ignored (a +// destructuring bug always fell back to the hardcoded /backup/database), so +// this setting being freely writable by any backup.create holder — the +// built-in `admin` role has it without settings.edit or backup.restore — was +// harmless. Making the setting actually take effect reopens that exact +// exfiltration path unless it's rejected here too. +function getPubliclyServableRoots() { + const storage = getStoragePath(); + return [ + path.join(storage, 'uploads', 'logos'), + path.join(storage, 'uploads', 'favicons'), + path.join(storage, 'fonts') + ]; +} + +function isUnderPubliclyServableRoot(candidatePath) { + const resolved = path.resolve(candidatePath); + return getPubliclyServableRoots().some((root) => { + const resolvedRoot = path.resolve(root); + return resolved === resolvedRoot || resolved.startsWith(resolvedRoot + path.sep); + }); +} + /** * Database Backup Service * Supports both SQLite and PostgreSQL with proper escaping, @@ -396,6 +426,12 @@ class DatabaseBackupService { ...options }; + if (isUnderPubliclyServableRoot(destinationPath)) { + throw new Error( + `Refusing to write a database backup to a publicly served directory: ${destinationPath}` + ); + } + // Create backup directory await fs.mkdir(destinationPath, { recursive: true }); @@ -822,5 +858,6 @@ module.exports = { databaseBackupService, startScheduledBackups, stopScheduledBackups, + isUnderPubliclyServableRoot, DatabaseBackupService // Export class for testing }; \ No newline at end of file