maybeEncryptArchive swallows plaintext unlink failure — plaintext may persist on disk #48

New issue

Closed

opened 2026-08-14 03:03:46 +00:00 by Mark · 0 comments

Mark commented

2026-08-14 03:03:46 +00:00

Owner

maybeEncryptArchive in backend/src/core/backup/encryption.ts:281 deletes the plaintext archive with .catch(() => undefined). If the post-encryption unlink fails (permissions, FS error, container restart mid-cleanup), the plaintext backup silently persists on disk next to the .enc file — a defense-in-depth gap for a feature whose entire purpose is to not have plaintext on disk.

Expected: on unlink failure, log at warn/error level and surface a non-fatal warning to the caller / UI so the operator knows plaintext may still exist (option (a), recommended over fail-closed (b) since (b) would break the backup on any filesystem hiccup). .catch(() => undefined) is unacceptable.

Scope: change maybeEncryptArchive (and any helper it delegates the unlink to) in Mark/docker_backup, add a unit/integration test that simulates unlink failure (point the post-encrypt path at a read-only dir, or mock the fs call to reject), and confirm the warning is visible in the job status / logs. Callers: backend/src/core/backup/incremental/execute.ts:151 and backend/src/core/backup/delta.ts:164. No dedicated encryption test file exists yet (backend/tests/core/backup/ has checksum/compressor/executor/verification).

Source: kanban card t_066d040b, decomposed from the deployment-gap probe (issue #47). Non-blocking remediation relative to the #20 redeploy; can land in a follow-up PR.

maybeEncryptArchive in backend/src/core/backup/encryption.ts:281 deletes the plaintext archive with `.catch(() => undefined)`. If the post-encryption unlink fails (permissions, FS error, container restart mid-cleanup), the plaintext backup silently persists on disk next to the `.enc` file — a defense-in-depth gap for a feature whose entire purpose is to not have plaintext on disk. Expected: on unlink failure, log at warn/error level and surface a non-fatal warning to the caller / UI so the operator knows plaintext may still exist (option (a), recommended over fail-closed (b) since (b) would break the backup on any filesystem hiccup). `.catch(() => undefined)` is unacceptable. Scope: change maybeEncryptArchive (and any helper it delegates the unlink to) in Mark/docker_backup, add a unit/integration test that simulates unlink failure (point the post-encrypt path at a read-only dir, or mock the fs call to reject), and confirm the warning is visible in the job status / logs. Callers: backend/src/core/backup/incremental/execute.ts:151 and backend/src/core/backup/delta.ts:164. No dedicated encryption test file exists yet (backend/tests/core/backup/ has checksum/compressor/executor/verification). Source: kanban card t_066d040b, decomposed from the deployment-gap probe (issue #47). Non-blocking remediation relative to the #20 redeploy; can land in a follow-up PR.