diff --git a/server/src/nest/backup/backup.controller.ts b/server/src/nest/backup/backup.controller.ts index 9071e46a7..953776dd3 100644 --- a/server/src/nest/backup/backup.controller.ts +++ b/server/src/nest/backup/backup.controller.ts @@ -28,6 +28,7 @@ import { AutoBackupSettingsDto } from './backup.dto'; import { getClientIp } from '../audit/client-ip'; import { AuditService } from '../audit/audit.service'; import { getUploadTmpDir, MAX_BACKUP_UPLOAD_SIZE } from './backup.impl'; +import { StorageNotFoundError } from '../storage/storage.types'; import { ManagedForbidden, isManagedBlocked, MANAGED_FORBIDDEN_ERROR } from '../common/managed'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; @@ -91,7 +92,16 @@ export class BackupController { if (!(await this.backup.backupFileExists(filename))) { throw new HttpException({ error: 'Backup not found' }, 404); } - res.download(this.backup.backupFilePath(filename), filename); + try { + await this.backup.sendBackupToResponse(filename, res); + } catch (err) { + // The pre-check above owns the normal miss; this only covers a delete + // racing between the check and the send. + if (err instanceof StorageNotFoundError) { + throw new HttpException({ error: 'Backup not found' }, 404); + } + throw err; + } } @ManagedForbidden('a restore replaces database and uploads, and the operator owns the recovery point') diff --git a/server/src/nest/backup/backup.impl.ts b/server/src/nest/backup/backup.impl.ts index c362b6009..90043aad4 100644 --- a/server/src/nest/backup/backup.impl.ts +++ b/server/src/nest/backup/backup.impl.ts @@ -10,6 +10,7 @@ import { invalidatePermissionsCache } from '../permissions/permissions-cache'; import { pluginsCodeRoot, pluginsDataRoot } from '../plugins/paths'; import { stageExtractedPluginTrees, applyStagedRestoreNow } from '../plugins/plugin-backup'; import { snapshotAllPluginDataDbs } from '../plugins/host/plugin-data.service'; +import type { Response } from 'express'; import type { StorageService } from '../storage/storage.service'; // --------------------------------------------------------------------------- @@ -89,6 +90,17 @@ export function backupFileExists(storage: StorageService, filename: string): Pro return storage.exists('backups', filename); } +/** + * The codebase's only res.download becomes the storage equivalent: root-relative + * sendFile via sendToResponse, with res.download's attachment header rebuilt by + * hand (filenames are regex-gated ASCII — no encoding cases). + */ +export function sendBackupToResponse(storage: StorageService, filename: string, res: Response): Promise { + return storage.sendToResponse('backups', filename, res, { + disposition: `attachment; filename="${filename}"`, + }); +} + // --------------------------------------------------------------------------- // Rate limiter state (shared across requests) // --------------------------------------------------------------------------- diff --git a/server/src/nest/backup/backup.service.ts b/server/src/nest/backup/backup.service.ts index 95335cd7a..d57cfbcc8 100644 --- a/server/src/nest/backup/backup.service.ts +++ b/server/src/nest/backup/backup.service.ts @@ -1,4 +1,5 @@ import { Injectable } from '@nestjs/common'; +import type { Response } from 'express'; import { StorageService } from '../storage/storage.service'; import * as svc from './backup.impl'; @@ -24,6 +25,7 @@ export class BackupService { isValidBackupFilename(filename: string) { return svc.isValidBackupFilename(filename); } backupFilePath(filename: string) { return svc.backupFilePath(filename); } backupFileExists(filename: string) { return svc.backupFileExists(this.storage, filename); } + sendBackupToResponse(filename: string, res: Response) { return svc.sendBackupToResponse(this.storage, filename, res); } checkRateLimit(key: string, maxAttempts: number, windowMs: number) { return svc.checkRateLimit(key, maxAttempts, windowMs); } get rateWindow() { return svc.BACKUP_RATE_WINDOW; } diff --git a/server/tests/integration/backup.test.ts b/server/tests/integration/backup.test.ts index b8d1b8cda..6f61cfdb2 100644 --- a/server/tests/integration/backup.test.ts +++ b/server/tests/integration/backup.test.ts @@ -85,6 +85,7 @@ import { resetTestDb, resetRateLimits } from '../helpers/test-db'; import { createAdmin, createUser } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; import * as backupService from '../../src/nest/backup/backup.impl'; +import { DEFAULT_BACKUPS_ROOT } from '../../src/nest/storage/storage-paths'; import fs from 'fs'; import path from 'path'; import os from 'os'; @@ -200,31 +201,36 @@ describe('Backup security', () => { // --------------------------------------------------------------------------- describe('Backup download', () => { - let tmpFile: string; + // Serving goes through the real StorageService now (sendBackupToResponse is + // NOT overridden in the impl mock), so the fixture must live in the real + // backups root. Only this one file is cleaned up — never the whole dir. + const downloadFixture = 'backup-2026-04-06T12-00-00.zip'; + const downloadFixturePath = path.join(DEFAULT_BACKUPS_ROOT, downloadFixture); beforeEach(() => { - // Create a real temporary file that Express can stream back - tmpFile = path.join(os.tmpdir(), `test-backup-${Date.now()}.zip`); - fs.writeFileSync(tmpFile, 'fake zip content'); - vi.mocked(backupService.backupFileExists).mockReturnValue(true); - vi.mocked(backupService.backupFilePath).mockReturnValue(tmpFile); + vi.mocked(backupService.backupFileExists).mockResolvedValue(true); }); afterAll(() => { - try { fs.unlinkSync(tmpFile); } catch {} + try { fs.unlinkSync(downloadFixturePath); } catch {} }); it('BACKUP-INT-001 — GET /backup/download/:filename returns 200 with content-disposition', async () => { const { user: admin } = createAdmin(testDb); - const filename = 'backup-2026-04-06T12-00-00.zip'; + // (Re)write the fixture immediately before the request that serves it. + fs.mkdirSync(DEFAULT_BACKUPS_ROOT, { recursive: true }); + fs.writeFileSync(downloadFixturePath, 'fake zip content'); const res = await request(app) - .get(`/api/backup/download/${filename}`) + .get(`/api/backup/download/${downloadFixture}`) .set('Cookie', authCookie(admin.id)); expect(res.status).toBe(200); expect(res.headers['content-disposition']).toMatch(/attachment/i); - expect(res.headers['content-disposition']).toContain(filename); + expect(res.headers['content-disposition']).toContain(downloadFixture); + // superagent has no parser for application/zip, so assert the byte count + // rather than the (unbuffered) body. + expect(res.headers['content-length']).toBe(String('fake zip content'.length)); }); it('BACKUP-INT-002 — GET /backup/download/:filename returns 400 for invalid filename', async () => { diff --git a/server/tests/unit/nest/backup.controller.test.ts b/server/tests/unit/nest/backup.controller.test.ts index 1ad96a9de..0526636e6 100644 --- a/server/tests/unit/nest/backup.controller.test.ts +++ b/server/tests/unit/nest/backup.controller.test.ts @@ -18,10 +18,12 @@ vi.mock('../../../src/nest/backup/backup.impl', () => ({ isValidBackupFilename: vi.fn().mockReturnValue(true), backupFilePath: vi.fn().mockReturnValue('/data/backups/svc.zip'), backupFileExists: vi.fn().mockReturnValue(true), + sendBackupToResponse: vi.fn().mockResolvedValue(undefined), checkRateLimit: vi.fn().mockReturnValue(true), })); import { BackupController } from '../../../src/nest/backup/backup.controller'; +import { StorageNotFoundError } from '../../../src/nest/storage/storage.types'; import { BackupService as RealBackupService } from '../../../src/nest/backup/backup.service'; import { AdminGuard } from '../../../src/nest/auth/admin.guard'; import type { BackupService } from '../../../src/nest/backup/backup.service'; @@ -56,6 +58,7 @@ function svc(o: Partial = {}): BackupService { isValidBackupFilename: vi.fn().mockReturnValue(true), backupFilePath: vi.fn().mockReturnValue('/b/x.zip'), backupFileExists: vi.fn().mockReturnValue(true), + sendBackupToResponse: vi.fn().mockResolvedValue(undefined), checkRateLimit: vi.fn().mockReturnValue(true), rateWindow: 3600000, ...o, @@ -106,12 +109,22 @@ describe('BackupController', () => { expect(writeAudit).toHaveBeenCalledWith(expect.objectContaining({ action: 'backup.create', resource: 'b.zip' })); }); - it('GET /download 400 invalid / 404 missing, else res.download', async () => { - const res = { download: vi.fn() } as unknown as Response; + it('GET /download 400 invalid / 404 missing, else serves through storage', async () => { + const res = {} as unknown as Response; expect(await thrownAsync(() => bc(svc({ isValidBackupFilename: vi.fn().mockReturnValue(false) })).download('x', res))).toEqual({ status: 400, body: { error: 'Invalid filename' } }); expect(await thrownAsync(() => bc(svc({ backupFileExists: vi.fn().mockResolvedValue(false) })).download('x.zip', res))).toEqual({ status: 404, body: { error: 'Backup not found' } }); - await bc(svc()).download('x.zip', res); - expect(res.download).toHaveBeenCalledWith('/b/x.zip', 'x.zip'); + const sendBackupToResponse = vi.fn().mockResolvedValue(undefined); + await bc(svc({ sendBackupToResponse } as Partial)).download('x.zip', res); + expect(sendBackupToResponse).toHaveBeenCalledWith('x.zip', res); + }); + + it('GET /download maps a StorageNotFoundError raced past the pre-check to the same 404', async () => { + const res = {} as unknown as Response; + const sendBackupToResponse = vi.fn().mockRejectedValue(new StorageNotFoundError('x.zip')); + expect(await thrownAsync(() => bc(svc({ sendBackupToResponse } as Partial)).download('x.zip', res))).toEqual({ status: 404, body: { error: 'Backup not found' } }); + // A non-miss failure is NOT swallowed into the 404 envelope. + const boom = vi.fn().mockRejectedValue(new Error('io')); + await expect(bc(svc({ sendBackupToResponse: boom } as Partial)).download('x.zip', res)).rejects.toThrow('io'); }); it('POST /restore maps the service status, else audits', async () => { @@ -224,6 +237,10 @@ describe('BackupService (wrapper)', () => { expect(wrapper.backupFileExists('svc.zip')).toBe(true); expect(backupSvc.backupFileExists).toHaveBeenCalledWith(storage, 'svc.zip'); + const fakeRes = {} as Response; + await wrapper.sendBackupToResponse('svc.zip', fakeRes); + expect(backupSvc.sendBackupToResponse).toHaveBeenCalledWith(storage, 'svc.zip', fakeRes); + expect(wrapper.checkRateLimit('ip', 3, 1000)).toBe(true); expect(backupSvc.checkRateLimit).toHaveBeenCalledWith('ip', 3, 1000); }); diff --git a/server/tests/unit/nest/backup.impl.test.ts b/server/tests/unit/nest/backup.impl.test.ts index b3859209e..0a458153c 100644 --- a/server/tests/unit/nest/backup.impl.test.ts +++ b/server/tests/unit/nest/backup.impl.test.ts @@ -77,6 +77,7 @@ import { backupFilePath, backupFileExists, listBackups, + sendBackupToResponse, } from '../../../src/nest/backup/backup.impl'; import type { StorageService } from '../../../src/nest/storage/storage.service'; @@ -933,6 +934,38 @@ describe('BACKUP-040 backupFileExists', () => { }); }); +// --------------------------------------------------------------------------- +// sendBackupToResponse +// --------------------------------------------------------------------------- + +describe('BACKUP-062 sendBackupToResponse', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('BACKUP-062a — serves through storage with the res.download attachment header', async () => { + const storage = stubStorage(); + const res = { setHeader: vi.fn() } as unknown as import('express').Response; + + await sendBackupToResponse(storage, 'backup-2026-01-01T00-00-00.zip', res); + + expect(storage.sendToResponse).toHaveBeenCalledOnce(); + expect(storage.sendToResponse).toHaveBeenCalledWith( + 'backups', + 'backup-2026-01-01T00-00-00.zip', + res, + { disposition: 'attachment; filename="backup-2026-01-01T00-00-00.zip"' }, + ); + }); + + it('BACKUP-062b — propagates a storage failure (the controller owns the miss contract)', async () => { + const storage = stubStorage({ sendToResponse: vi.fn(async () => { throw new Error('missing'); }) }); + const res = {} as import('express').Response; + + await expect(sendBackupToResponse(storage, 'backup-x.zip', res)).rejects.toThrow('missing'); + }); +}); + // --------------------------------------------------------------------------- // listBackups // ---------------------------------------------------------------------------