Prune must delete nothing when a manifest cannot be read, and a manifest must never omit a blob #157

New Issue

2026-09-22T00:55:13+02:00

clawbot commented

2026-09-22 00:55:13 +02:00

Found by the security review #73. Severity: high (silent loss of backup data, no attacker needed).

What is wrong

collectReferencedBlobs (internal/vaultik/prune.go:184-199) downloads every manifest to learn which blobs are in use. When one fails to download or decode it logs an error and continues, so every blob referenced only by that snapshot counts as unreferenced and is deleted. The command still exits 0. snapshot create --prune, the cron line the README recommends, calls this with Force: true (internal/vaultik/snapshot.go:160), so it runs unattended.
generateBlobManifest (internal/snapshot/snapshot.go:840-855) logs and skips a blob whose lookup fails or returns nil, producing a manifest that lists fewer blobs than the snapshot needs. Prune decides by manifest alone, so that snapshot loses blobs at the next prune.
verify --deep does not catch item 2: a database blob missing from the manifest is only a WARN line ("database is authoritative", internal/vaultik/verify.go:570-576) and the result is ok.
docs/REPOSTRUCTURE.md:150-151 ("Pruning Safety" points 3 and 4) describes a latest-local versus latest-remote comparison that fails on mismatch. No such check exists anywhere in the code.

Acceptable

collectReferencedBlobs returns an error naming the remote key on the first manifest it cannot download or decode. PruneBlobs deletes nothing and returns that error, so prune, snapshot create --prune and remote nuke exit non-zero.
generateBlobManifest returns an error instead of skipping a blob.
Deep verify fails, naming the hashes, when the manifest and database blob sets differ in either direction.
docs/REPOSTRUCTURE.md "Pruning Safety" says what is true: prune keeps exactly the blobs named by the manifests it could read, deletes nothing if any manifest is unreadable, and must not run while another machine is backing up to the same destination (blobs are uploaded before the manifest and the process lock is per machine). Points 3 and 4 are removed.

Out of scope, owner question on #73: a metadata/{remote-key}/ directory that has db.zst.age but no manifest. Leave listUniqueSnapshotIDs behaviour for that case unchanged here.

Definition of done

Test: a store whose Get fails for one manifest; assert an error is returned and no blob is deleted. Same for a manifest that does not decode.
Test: a manifest that omits one blob the database needs makes verify --deep fail.
Test: a blob lookup failure makes manifest generation fail.
make fmt run over the changed markdown; no existing assertion weakened; make check green.

Line numbers are as of next at 6fcd8e1.

model: fable-5-1

Found by the security review https://git.eeqj.de/sneak/vaultik/issues/73. Severity: **high** (silent loss of backup data, no attacker needed). ## What is wrong 1. `collectReferencedBlobs` (`internal/vaultik/prune.go:184-199`) downloads every manifest to learn which blobs are in use. When one fails to download or decode it logs an error and continues, so every blob referenced only by that snapshot counts as unreferenced and is deleted. The command still exits 0. `snapshot create --prune`, the cron line the README recommends, calls this with `Force: true` (`internal/vaultik/snapshot.go:160`), so it runs unattended. 2. `generateBlobManifest` (`internal/snapshot/snapshot.go:840-855`) logs and skips a blob whose lookup fails or returns nil, producing a manifest that lists fewer blobs than the snapshot needs. Prune decides by manifest alone, so that snapshot loses blobs at the next prune. 3. `verify --deep` does not catch item 2: a database blob missing from the manifest is only a WARN line ("database is authoritative", `internal/vaultik/verify.go:570-576`) and the result is `ok`. 4. `docs/REPOSTRUCTURE.md:150-151` ("Pruning Safety" points 3 and 4) describes a latest-local versus latest-remote comparison that fails on mismatch. No such check exists anywhere in the code. ## Acceptable - `collectReferencedBlobs` returns an error naming the remote key on the first manifest it cannot download or decode. `PruneBlobs` deletes nothing and returns that error, so `prune`, `snapshot create --prune` and `remote nuke` exit non-zero. - `generateBlobManifest` returns an error instead of skipping a blob. - Deep verify fails, naming the hashes, when the manifest and database blob sets differ in either direction. - `docs/REPOSTRUCTURE.md` "Pruning Safety" says what is true: prune keeps exactly the blobs named by the manifests it could read, deletes nothing if any manifest is unreadable, and must not run while another machine is backing up to the same destination (blobs are uploaded before the manifest and the process lock is per machine). Points 3 and 4 are removed. Out of scope, owner question on https://git.eeqj.de/sneak/vaultik/issues/73: a `metadata/{remote-key}/` directory that has `db.zst.age` but no manifest. Leave `listUniqueSnapshotIDs` behaviour for that case unchanged here. ## Definition of done 1. Test: a store whose `Get` fails for one manifest; assert an error is returned and no blob is deleted. Same for a manifest that does not decode. 2. Test: a manifest that omits one blob the database needs makes `verify --deep` fail. 3. Test: a blob lookup failure makes manifest generation fail. 4. `make fmt` run over the changed markdown; no existing assertion weakened; `make check` green. Line numbers are as of `next` at `6fcd8e1`. model: fable-5-1

clawbot referenced this issue

2026-09-22 00:55:32 +02:00

Bound what a downloaded object may expand to, and escape control characters from the destination before printing #164

clawbot referenced this issue

2026-09-22 00:55:32 +02:00

Correct the security claims in the docs and comments, and write down the accepted risks #171

clawbot referenced this issue from a commit

2026-09-22 11:18:10 +02:00

Fail closed on unreadable manifests instead of losing blobs (closes #157)

clawbot referenced a pull request that will close this issue

2026-09-22 11:18:41 +02:00

Fail closed on unreadable manifests instead of losing blobs #180

clawbot commented

2026-09-22 11:19:02 +02:00

Addressed in #180.

Prune now returns an error and deletes nothing when any manifest cannot be downloaded or decoded, instead of logging, skipping, and deleting the blobs referenced only by that snapshot while exiting 0. Manifest generation now fails rather than omitting a blob whose database lookup fails or returns no row. Deep verify now fails on any manifest/database divergence instead of only warning. docs/REPOSTRUCTURE.md corrected to the real behavior. Tests cover all three failure paths.

Model: opus-4-8

Addressed in https://git.eeqj.de/sneak/vaultik/pulls/180. Prune now returns an error and deletes nothing when any manifest cannot be downloaded or decoded, instead of logging, skipping, and deleting the blobs referenced only by that snapshot while exiting 0. Manifest generation now fails rather than omitting a blob whose database lookup fails or returns no row. Deep verify now fails on any manifest/database divergence instead of only warning. `docs/REPOSTRUCTURE.md` corrected to the real behavior. Tests cover all three failure paths. Model: opus-4-8

clawbot closed this issue

2026-09-22 11:45:30 +02:00

clawbot referenced this issue from a commit

2026-09-22 11:45:32 +02:00

Fail closed on unreadable manifests instead of losing blobs (closes #157)

clawbot referenced this issue

2026-09-22 11:48:32 +02:00

next: accumulated work for the upcoming release #114

Sign in to join this conversation.