refactor(backup): drop the retention value nothing could ever read

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.
This commit is contained in:
Marc-André Bombeck committed 2026-09-10 01:38:40 +02:00
1 parent a4909d3e00
commit 7f54f339b2
3 files changed
+17 -14

No files matched your search

+4 -3
View File
@@ -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)
@@ -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);
});
});
+9 -10
View File
@@ -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,
};
}