From 7f54f339b27a38255ca49a4695a41c49b2dafdad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 01:38:40 +0200 Subject: [PATCH] refactor(backup): drop the retention value nothing could ever read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit E2. `loadOffhostConfig` parsed BACKUP_RETENTION_DAYS, clamped it to a minimum of one day and put it on the config object, where no caller read it. Neither annotation fits: it is not deliberately internal, because there is no internal use, and no consumer is pending. The design forbids one outright — the worker's grant excludes DeleteObject on purpose, so nothing here can act on a retention window without giving a compromised worker the ability to wipe the history. A clamp that guards nothing only read as an enforcer the worker is not. So the field and the clamp go, and the reason sits where the parse was. The variable keeps its place in the two example files and on the compose whitelist: it is the number the operator sets the bucket's own lifecycle rule to. The docs table said "defaults to 30", which implied the app had an opinion; it now says the app does not read it. --- docs/ops/backup-restore.md | 7 ++++--- src/lib/jobs/__tests__/offhost-backup.test.ts | 5 ++++- src/lib/jobs/offhost-backup.ts | 19 +++++++++---------- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/docs/ops/backup-restore.md b/docs/ops/backup-restore.md index 0ac2d7ad5..64e7854e9 100644 --- a/docs/ops/backup-restore.md +++ b/docs/ops/backup-restore.md @@ -41,7 +41,7 @@ the restore script needs no flag to tell them apart. | `BACKUP_S3_ACCESS_KEY` | yes | | | `BACKUP_S3_SECRET_KEY` | yes | | | `BACKUP_S3_REGION` | no | defaults to `auto` (Cloudflare R2) | -| `BACKUP_RETENTION_DAYS` | no | defaults to `30` | +| `BACKUP_RETENTION_DAYS` | no | not read by the app; the number for your bucket's lifecycle rule | ## Bucket permissions @@ -56,8 +56,9 @@ Cloudflare R2 the **Object Read & Write** token already covers all three. The worker never deletes a backup object: its grant covers PutObject, GetObject and AbortMultipartUpload only, so a compromised worker cannot -wipe the history. Expiry is the storage provider's lifecycle rule, set to -match `BACKUP_RETENTION_DAYS`: +wipe the history, and nothing in the app reads `BACKUP_RETENTION_DAYS`. +Expiry is the storage provider's lifecycle rule, set to match the number +you wrote there: ``` Filter: "" (all objects) diff --git a/src/lib/jobs/__tests__/offhost-backup.test.ts b/src/lib/jobs/__tests__/offhost-backup.test.ts index f507cb1f5..5a630b088 100644 --- a/src/lib/jobs/__tests__/offhost-backup.test.ts +++ b/src/lib/jobs/__tests__/offhost-backup.test.ts @@ -89,6 +89,10 @@ describe("loadOffhostConfig", () => { expect(loadOffhostConfig()).toBeNull(); }); + // No `retentionDays` here on purpose: the worker cannot enforce retention + // (DeleteObject is kept out of its grant), so the config carries no field + // for it and `BACKUP_RETENTION_DAYS` stays the operator's number for the + // bucket's own lifecycle rule. it("parses a complete config", () => { vi.stubEnv("BACKUP_S3_ENDPOINT", "https://r2.example"); vi.stubEnv("BACKUP_S3_BUCKET", "hl-backups"); @@ -102,7 +106,6 @@ describe("loadOffhostConfig", () => { expect(cfg!.endpoint).toBe("https://r2.example"); expect(cfg!.region).toBe("auto"); expect(cfg!.encryptionKey.length).toBe(32); - expect(cfg!.retentionDays).toBe(30); }); }); diff --git a/src/lib/jobs/offhost-backup.ts b/src/lib/jobs/offhost-backup.ts index 9f868623e..0f55273c0 100644 --- a/src/lib/jobs/offhost-backup.ts +++ b/src/lib/jobs/offhost-backup.ts @@ -13,7 +13,8 @@ * * Retention: the worker NEVER calls DeleteObject on backup keys. Operators * MUST configure a bucket-level lifecycle rule (e.g. expire after - * `BACKUP_RETENTION_DAYS`). This keeps the IAM grant for the worker + * `BACKUP_RETENTION_DAYS`, which this module reads nowhere — see + * `loadOffhostConfig`). This keeps the IAM grant for the worker * limited to PutObject + GetObject + AbortMultipartUpload, so a compromised * worker cannot wipe the backup history. The abort is what cleans up a run * that failed partway rather than leaving billed, unlistable parts behind; @@ -40,7 +41,6 @@ export interface OffhostBackupConfig { secretKey: string; region: string; encryptionKey: Buffer; - retentionDays: number; } export class OffhostBackupNotConfiguredError extends Error { @@ -69,13 +69,13 @@ export function loadOffhostConfig(): OffhostBackupConfig | null { const encRaw = process.env.BACKUP_ENCRYPTION_KEY; if (!endpoint || !bucket || !accessKey || !secretKey || !encRaw) return null; - const retentionDays = (() => { - const raw = process.env.BACKUP_RETENTION_DAYS; - if (!raw) return 30; - const v = parseInt(raw, 10); - return Number.isFinite(v) && v >= 1 ? v : 30; - })(); - + // `BACKUP_RETENTION_DAYS` is deliberately absent from this config. It used + // to be parsed and clamped here and then read by nobody, which read as an + // enforcer the worker is not: retention belongs to the bucket's lifecycle + // rule, and no consumer can ever appear here because DeleteObject is kept + // out of the worker's grant on purpose (see the header). The variable stays + // documented and on the compose whitelist because it is the number the + // operator sets that rule to. return { endpoint, bucket, @@ -83,7 +83,6 @@ export function loadOffhostConfig(): OffhostBackupConfig | null { secretKey, region: process.env.BACKUP_S3_REGION ?? "auto", encryptionKey: decodeBackupKey(encRaw), - retentionDays, }; }