From a4909d3e0064f982922c8b2a4378ff94e7b01845 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 01:31:17 +0200 Subject: [PATCH 1/2] docs(env): say exactly which object the off-host worker deletes E1. Two halves of one correction. `.env.example` still said backup objects older than BACKUP_RETENTION_DAYS are pruned, which no code does, so a self-hoster reading that file alone still expected the worker to keep the bucket tidy. It now carries the same sentence as the production example. And that sentence overreached in the other direction: "the worker never deletes an object" is not true of `_healthcheck/.bin`, the 1-byte key the Test-connection button writes and then deletes. The delete swallows its own failure, so an operator who granted exactly the three documented actions leaves one orphan per press and is never told. Both files now say "a BACKUP object", and name the one key and what happens without DeleteObject on that prefix. --- .env.example | 10 +++++++++- .env.production.example | 9 ++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/.env.example b/.env.example index 3c02cfbec..31cee47cc 100644 --- a/.env.example +++ b/.env.example @@ -207,12 +207,20 @@ API_TOKEN_HMAC_KEY="REPLACE-WITH-OUTPUT-OF-openssl-rand-hex-32" # the backup silently. The backup runs at 02:30 Europe/Berlin via pg-boss cron. # Restore with `pnpm tsx scripts/restore-backup.ts `. # See docs/ops/backup-restore.md for the format spec + step-by-step. +# +# The worker never deletes a BACKUP object -- its grant is PutObject, GetObject +# and AbortMultipartUpload -- so BACKUP_RETENTION_DAYS is the number you set +# the bucket's own lifecycle rule to, not something the worker enforces. The +# one key it does delete is its own connection test's `_healthcheck/.bin`, +# and that delete is best-effort: without DeleteObject on that prefix every +# Test-connection press leaves one 1-byte orphan behind, silently. +# # BACKUP_S3_ENDPOINT="https://.r2.cloudflarestorage.com" # BACKUP_S3_BUCKET="healthlog-backups" # BACKUP_S3_ACCESS_KEY="" # BACKUP_S3_SECRET_KEY="" # BACKUP_S3_REGION="auto" # "auto" for Cloudflare R2; real region for AWS S3 -# BACKUP_RETENTION_DAYS="30" # objects older than this are pruned (best-effort; prefer a bucket lifecycle rule) +# BACKUP_RETENTION_DAYS="30" # the number for your bucket lifecycle rule (see above) # BACKUP_ENCRYPTION_KEY="" # 64-hex-char or base64 32-byte; SEPARATE from ENCRYPTION_KEY diff --git a/.env.production.example b/.env.production.example index 4a56a53b6..27486de2b 100644 --- a/.env.production.example +++ b/.env.production.example @@ -315,9 +315,12 @@ API_TOKEN_HMAC_KEY="" # BACKUP_S3_REGION="auto" # # How long backup objects are meant to live, in days. Default 30, minimum 1. -# The worker never deletes an object -- its grant is PutObject, GetObject and -# AbortMultipartUpload -- so this number is what you set the bucket's own -# lifecycle rule to. See docs/ops/backup-restore.md. +# The worker never deletes a BACKUP object -- its grant is PutObject, GetObject +# and AbortMultipartUpload -- so this number is what you set the bucket's own +# lifecycle rule to. The one key it does delete is its own connection test's +# `_healthcheck/.bin`, and that delete is best-effort: without DeleteObject +# on that prefix every Test-connection press leaves one 1-byte orphan behind, +# silently. See docs/ops/backup-restore.md. # BACKUP_RETENTION_DAYS="30" 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 2/2] 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, }; }