Files
picpeak/backend/__tests__/routes/databaseBackupConfigDestination.test.js
T
Paul Nothaft 34207456e6 fix(backup): honor the configured database-backup destination path (#1367)
* 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>
2026-09-09 22:27:26 +02:00

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