15cd5ede82
* fix(backup): stop ignoring the configured database-backup destination path databaseBackupService.getBackupConfig() returns the raw database_backup_*-prefixed setting keys, but backup() and startScheduledBackups() destructured unprefixed names off that object (destinationPath, compress, enabled, schedule, retentionDays, emailOnSuccess/Failure). None of those keys ever existed on the config object, so every read silently fell through to its hardcoded default. The visible symptom (reported in issue 1365): the inline database dump that runs before every file backup (default ON) always tried to create /backup/database, regardless of what an admin configured, and died with EACCES on the read-only default path — before the file backup's own (correctly wired) backup_destination_path was ever reached. The standalone scheduled database-backup runner had the same bug: config.enabled was always undefined, so it silently never started regardless of database_backup_enabled. Also fixes saveManifestToLocal's manifest-directory fallback, which hardcoded /backup instead of matching the sane getStoragePath()/backups default used everywhere else for a missing backup_destination_path. Relates to issue 1365 * fix(backup): reject a database-backup destination inside a public static mount Making database_backup_destination_path actually take effect reopens a GHSA-jw8m-43r2-jqrm-class exfiltration path: that setting is writable via PUT /api/admin/database-backup/config under backup.create alone (the built-in admin role has it without settings.edit or backup.restore), with no path validation. Before this fix the setting was silently ignored (the destructuring bug), so pointing it at the public uploads/logos or fonts mount was harmless; now that it is honored, it needed the same defense GHSA-jw8m already applies to the per-request override. Rejects the setting at both the config write (immediate 400) and, defensively, at backup() time before mkdir. Found by codex review. * 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. * fix(backup): resolve symlinks and add the all-in-one frontend dir to the destination guard Codex round 3 found two more bypasses of the public-root guard, both specific to the all-in-one image (Dockerfile.aio): - /app/frontend/dist (FRONTEND_DIR) ships nodejs-owned and is served unauthenticated as the built SPA -- missing from the protected-roots list. - /app/storage is a symlink to /data/storage (the actual STORAGE_PATH). A destination given as /app/storage/uploads/logos passed the guard's lexical path.resolve() comparison while resolving, on disk, to the exact same directory as the protected STORAGE_PATH/uploads/logos. isUnderPubliclyServableRoot now resolves symlinks in whatever prefix of each path already exists (resolveRealish) before comparing, rather than relying on path.resolve() alone. Also restores three fs.mkdir spies in the test file that were never un-spied, which silently leaked a rejected mock into any later test doing a real fs.mkdir -- exactly what the new symlink test needed to set up its fixture. Found by codex review, round 3. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
123 lines
4.9 KiB
JavaScript
123 lines
4.9 KiB
JavaScript
/**
|
|
* 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);
|
|
});
|
|
|
|
// 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);
|
|
});
|
|
});
|