0d4c30884e
* fix(security): stop caller-chosen database backup destination (GHSA-jw8m)
POST /api/admin/database-backup/backup forwarded req.body straight into
databaseBackupService.backup(), which merges options over config:
const { destinationPath = '/backup/database', ... } = { ...config, ...options }
destinationPath is not a persistable setting — the /config allowlist only
accepts database_backup_* keys — so the request body was its only source.
The built-in `admin` role holds backup.create but neither settings.edit nor
backup.restore, so it could aim a full DB dump (admin bcrypt hashes, gallery
password hashes, encrypted SMTP creds) at the PUBLIC /uploads static mount
(server.js mounts it with no auth middleware) and then fetch it
unauthenticated. Filed low; it is a privilege escalation to unauthenticated
disclosure.
Forward only the real knobs, and only when present so absent keys can't
override config defaults via spread.
* fix(security): backup/restore hardening — restore path allowlist, gunzip bound, manifest checksum keying (GHSA-fw4c, h652, hgp8)
- adminRestore /validate + /start: constrain caller-supplied source and
manifestPath to the operator-configured backup roots — the SAME set the
restore wizard discovers from — so disaster recovery from a rescued mount
still works, with RESTORE_ALLOWED_ROOTS as an escape hatch (GHSA-fw4c).
- restoreService.decompressFile: bound the EXPANDED size and abort the
pipeline when exceeded; default 50 GB, RESTORE_MAX_DECOMPRESSED_BYTES
overrides (GHSA-h652).
- backupManifest: BACKUP_MANIFEST_KEY upgrades new manifests to a keyed
HMAC (GHSA-hgp8). Deliberately opt-in and verify-if-present — the key
cannot live in the database because the database is inside the backup, so
a mandatory HMAC would lock operators out of the exact disaster-recovery
case this exists for.
Also fixes a pre-existing bug found while testing hgp8: the checksum passed
Object.keys().sort() as JSON.stringify's second argument, which is an array
REPLACER (a property allowlist applied at every depth), not a key sorter. All
nested keys — path, size, per-file checksum — were dropped before hashing, so
the file list sat outside the integrity check entirely and a manifest path
could be rewritten to ../../etc/passwd without disturbing the digest. Now
hashes a recursively-canonicalized copy, with the legacy serialization
accepted on validation so existing backups stay restorable.
* fix(security): codex round 2 — unbreak the restore wizard, share checksum verification, guard downgrades
- adminRestore: `source` is usually a SOURCE TYPE ('local'|'s3'|'upload'),
not a path — restoreService branches on those literals. The containment
check treated it as a path, so path.resolve('local') fell outside the
backup roots and BOTH /validate and /start returned 400, blocking every
normal restore. Type tokens are now excluded from the path check.
- backupManifest: extracted verifyManifestChecksum() as the single source of
truth for the legacy/keyed fallbacks. restoreService.performPreRestoreValidation
recomputed the digest itself with the default canonical+keyed settings,
which rejected EVERY backup written before this batch. It now delegates.
- backupManifest: guard the algorithm downgrade — with a key configured, an
attacker able to rewrite the backup store could strip checksum_algorithm,
edit the manifest and recompute a plain SHA-256 that verified. Opt-in via
BACKUP_MANIFEST_REQUIRE_KEYED so pre-key backups keep restoring by default.
* fix(security): codex round 3 — close two manifest-verification fail-opens (GHSA-hgp8)
verifyManifestChecksum returned valid for a manifest with no
verification.total_checksum at all, and restoreService only called it when
that field was present. Deleting the field was therefore a complete bypass of
the keying work: no digest check, no downgrade guard, no
BACKUP_MANIFEST_REQUIRE_KEYED. Every manifest this codebase writes stamps the
field, so an absent one now fails validation, and the call site invokes the
verifier unconditionally.
Second fail-open: the strict-mode rejection of an unkeyed manifest was gated on
`&& key`, so with BACKUP_MANIFEST_REQUIRE_KEYED=true and no BACKUP_MANIFEST_KEY
configured a plain SHA-256 manifest sailed through. Strict mode is a statement
about the operator's manifests, not about the host — it is exactly the fresh
disaster-recovery box that lacks the secret. The rejection no longer depends on
a key being present.
Claude-Session: https://claude.ai/code/session_01F211U4dDbEj4zXiyKbi9me
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
121 lines
4.5 KiB
JavaScript
121 lines
4.5 KiB
JavaScript
/**
|
|
* Manual database backup must not honour a caller-supplied destination
|
|
* (GHSA-jw8m-43r2-jqrm).
|
|
*
|
|
* POST /api/admin/database-backup/backup forwarded req.body straight into
|
|
* databaseBackupService.backup(), which merges options over its config:
|
|
* const { destinationPath = '/backup/database', ... } = { ...config, ...options }
|
|
* `destinationPath` is not a persistable setting (the /config allowlist only
|
|
* accepts `database_backup_*` keys), so the request body was its ONLY source.
|
|
*
|
|
* The `admin` role holds backup.create but neither settings.edit nor
|
|
* backup.restore — so it could aim a full DB dump (bcrypt hashes, gallery
|
|
* password hashes, encrypted SMTP creds) at the PUBLIC /uploads static mount
|
|
* (server.js mounts it with no auth middleware) and fetch it unauthenticated.
|
|
*
|
|
* Pins that destinationPath from the body is ignored, while the legitimate
|
|
* knobs still pass through.
|
|
*/
|
|
|
|
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-')), 'db.sqlite',
|
|
);
|
|
process.env.JWT_SECRET = process.env.JWT_SECRET || 'dbbackup-test-secret';
|
|
|
|
// Capture what the route hands the service; never run a real backup.
|
|
const mockBackup = jest.fn(async () => ({ success: true }));
|
|
jest.mock('../../src/services/databaseBackup', () => ({
|
|
databaseBackupService: {
|
|
get isRunning() { return false; },
|
|
backup: (...args) => mockBackup(...args),
|
|
},
|
|
}));
|
|
|
|
const request = require('supertest');
|
|
const express = require('express');
|
|
const bcrypt = require('bcrypt');
|
|
const jwt = require('jsonwebtoken');
|
|
|
|
const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
|
|
|
|
describe('manual database backup destination (GHSA-jw8m)', () => {
|
|
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@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(); });
|
|
|
|
beforeEach(() => mockBackup.mockClear());
|
|
|
|
it('ignores a caller-supplied destinationPath', async () => {
|
|
const res = await request(app)
|
|
.post('/api/admin/database-backup/backup')
|
|
.set('Authorization', `Bearer ${adminToken}`)
|
|
.send({ destinationPath: '/app/storage/uploads' });
|
|
|
|
expect(res.status).toBe(200);
|
|
// Give the fire-and-forget call a tick to land.
|
|
await new Promise((resolve) => setImmediate(resolve));
|
|
expect(mockBackup).toHaveBeenCalled();
|
|
const opts = mockBackup.mock.calls[0][0];
|
|
expect(opts).not.toHaveProperty('destinationPath');
|
|
expect(JSON.stringify(opts)).not.toContain('uploads');
|
|
});
|
|
|
|
it('still forwards the legitimate backup knobs', async () => {
|
|
const res = await request(app)
|
|
.post('/api/admin/database-backup/backup')
|
|
.set('Authorization', `Bearer ${adminToken}`)
|
|
.send({ compress: false, validateIntegrity: false, destinationPath: '/tmp/evil' });
|
|
|
|
expect(res.status).toBe(200);
|
|
await new Promise((resolve) => setImmediate(resolve));
|
|
const opts = mockBackup.mock.calls[0][0];
|
|
expect(opts.compress).toBe(false);
|
|
expect(opts.validateIntegrity).toBe(false);
|
|
expect(opts).not.toHaveProperty('destinationPath');
|
|
});
|
|
|
|
it('omits absent knobs entirely so service/config defaults still apply', async () => {
|
|
const res = await request(app)
|
|
.post('/api/admin/database-backup/backup')
|
|
.set('Authorization', `Bearer ${adminToken}`)
|
|
.send({});
|
|
|
|
expect(res.status).toBe(200);
|
|
await new Promise((resolve) => setImmediate(resolve));
|
|
// An explicit `{compress: undefined}` would override config on spread —
|
|
// absent keys must simply not be present.
|
|
expect(mockBackup.mock.calls[0][0]).toEqual({});
|
|
});
|
|
});
|