Reject a metadata database truncated to the age header and nonce #198

Merged
clawbot merged 1 commits from issue-152-truncated-metadata-db into next 2026-09-22 18:11:58 +02:00
Collaborator

Fixes #152.

An object holding exactly the age header plus its 16-byte nonce decrypts without error: age.Decrypt succeeds, and on the first read the age stream's io.ErrUnexpectedEOF is mapped by the zstd decoder to a clean io.EOF at frame start. blobgen.Reader.Read then returned zero bytes and no error, so a truncated stream was indistinguishable from a valid empty one. Blobs are caught by the Close-time hash check; the metadata export (db.zst.age) was not, so restore built a fresh schema on the empty file and reported success.

Two production changes, per the issue's Acceptable section:

  1. blobgen.Reader.Read, on EOF, reads once more from the age reader and surfaces io.ErrUnexpectedEOF unless that read is (0, io.EOF), so a legitimately empty payload still round-trips to empty.
  2. materializeSnapshotDB rejects a zero-length decrypted database (errEmptySnapshotDB) before OpenReadOnly applies a schema.

Tests now pin each change to its own error, so reverting either fails a test:

  • The restore-level truncation test asserts io.ErrUnexpectedEOF. Reverting the reader change drops restore to the identity check and fails it.
  • A new test feeds a complete but empty age stream (header, nonce, final tag over zero bytes) through blobgen and asserts errEmptySnapshotDB, the branch the truncation input never reaches.
  • The internal/blobgen truncation test is unchanged.

Model: opus-4-8

Fixes https://git.eeqj.de/sneak/vaultik/issues/152. An object holding exactly the age header plus its 16-byte nonce decrypts without error: `age.Decrypt` succeeds, and on the first read the age stream's `io.ErrUnexpectedEOF` is mapped by the zstd decoder to a clean `io.EOF` at frame start. `blobgen.Reader.Read` then returned zero bytes and no error, so a truncated stream was indistinguishable from a valid empty one. Blobs are caught by the Close-time hash check; the metadata export (`db.zst.age`) was not, so restore built a fresh schema on the empty file and reported success. Two production changes, per the issue's Acceptable section: 1. `blobgen.Reader.Read`, on EOF, reads once more from the age reader and surfaces `io.ErrUnexpectedEOF` unless that read is `(0, io.EOF)`, so a legitimately empty payload still round-trips to empty. 2. `materializeSnapshotDB` rejects a zero-length decrypted database (`errEmptySnapshotDB`) before `OpenReadOnly` applies a schema. Tests now pin each change to its own error, so reverting either fails a test: - The restore-level truncation test asserts `io.ErrUnexpectedEOF`. Reverting the reader change drops restore to the identity check and fails it. - A new test feeds a complete but empty age stream (header, nonce, final tag over zero bytes) through `blobgen` and asserts `errEmptySnapshotDB`, the branch the truncation input never reaches. - The `internal/blobgen` truncation test is unchanged. Model: opus-4-8
clawbot added the needs-review label 2026-09-22 17:13:33 +02:00
clawbot self-assigned this 2026-09-22 17:13:33 +02:00
Author
Collaborator

FAIL -- needs-rework.

  1. internal/vaultik/restore_truncated_db_test.go, TestRestoreRejectsTruncatedMetadataDB (the closing require.Error): the test passes even with both of this PR's production changes reverted. I verified by reverting the blobgen.Reader.Read probe and the errEmptySnapshotDB check together and running it — restore still errors, because the pre-existing identity check (errSnapshotDBMismatch, restore.go:713, not touched by this PR) already rejects the empty database this truncation yields. A restore test that returns an error regardless of whether the fix is present does not guard the fix; a future regression removing both additions would not be caught here. Acceptable: assert the specific error the fix produces for a truncated object — require.ErrorIs(err, io.ErrUnexpectedEOF) — so that reverting the reader change (which drops the failure back to errSnapshotDBMismatch) makes the test fail.

  2. internal/vaultik/restore.go:765-768, the written == 0 -> errEmptySnapshotDB branch (the second half of the issue's Acceptable section): it has no test. The new restore test feeds a stream cut at header+nonce, which the reader now rejects during the copy (io.ErrUnexpectedEOF) before written == 0 is ever reached; I confirmed by reverting only this branch (reader fix intact) — the restore test still passes. No test supplies a validly-encrypted empty database (header + nonce + final tag), the only input that exercises this branch. Reverting the branch fails no test. Acceptable: add a test that writes a fully-encrypted empty db.zst.age and asserts restore fails with errEmptySnapshotDB.

  3. Commit message body is ~168 words, over the ~120-word guideline for a commit body. Acceptable: trim the explanation toward ~120 words; the longer form can stay in the PR description.

The reader fix itself is correct and is properly guarded — the internal/blobgen truncation test fails when the reader change is reverted, and the genuinely-empty blob still round-trips. make check is green on the rebased head.

Model: opus-4-8

FAIL -- needs-rework. 1. `internal/vaultik/restore_truncated_db_test.go`, `TestRestoreRejectsTruncatedMetadataDB` (the closing `require.Error`): the test passes even with both of this PR's production changes reverted. I verified by reverting the `blobgen.Reader.Read` probe and the `errEmptySnapshotDB` check together and running it — restore still errors, because the pre-existing identity check (`errSnapshotDBMismatch`, `restore.go:713`, not touched by this PR) already rejects the empty database this truncation yields. A restore test that returns an error regardless of whether the fix is present does not guard the fix; a future regression removing both additions would not be caught here. Acceptable: assert the specific error the fix produces for a truncated object — `require.ErrorIs(err, io.ErrUnexpectedEOF)` — so that reverting the reader change (which drops the failure back to `errSnapshotDBMismatch`) makes the test fail. 2. `internal/vaultik/restore.go:765-768`, the `written == 0` -> `errEmptySnapshotDB` branch (the second half of the issue's Acceptable section): it has no test. The new restore test feeds a stream cut at header+nonce, which the reader now rejects during the copy (`io.ErrUnexpectedEOF`) before `written == 0` is ever reached; I confirmed by reverting only this branch (reader fix intact) — the restore test still passes. No test supplies a validly-encrypted empty database (header + nonce + final tag), the only input that exercises this branch. Reverting the branch fails no test. Acceptable: add a test that writes a fully-encrypted empty `db.zst.age` and asserts restore fails with `errEmptySnapshotDB`. 3. Commit message body is ~168 words, over the ~120-word guideline for a commit body. Acceptable: trim the explanation toward ~120 words; the longer form can stay in the PR description. The reader fix itself is correct and is properly guarded — the `internal/blobgen` truncation test fails when the reader change is reverted, and the genuinely-empty blob still round-trips. `make check` is green on the rebased head. Model: opus-4-8
clawbot added needs-rework and removed needs-review labels 2026-09-22 17:45:39 +02:00
clawbot added 1 commit 2026-09-22 17:52:35 +02:00
An object holding just the age header and its 16-byte nonce decrypts
without error: the truncated read surfaces as io.ErrUnexpectedEOF at the
age layer, which the zstd decoder maps to a clean EOF at frame start.
blobgen then reported zero bytes and no error, so a truncated stream was
indistinguishable from a valid empty one, and the metadata database
export slipped through — restore built a fresh schema on the empty file
and reported success.

blobgen.Reader.Read now, on EOF, reads once more from the age reader and
surfaces io.ErrUnexpectedEOF unless that read is (0, io.EOF), the state a
genuine end leaves. downloadSnapshotDB additionally rejects a zero-length
decrypted database before any schema is built.

Model: opus-4-8
clawbot force-pushed issue-152-truncated-metadata-db from 03f126edc1 to cf0f08586d 2026-09-22 17:52:35 +02:00 Compare
Author
Collaborator

Rework addressing the three findings on #198. No production code changed; the reader fix and the written == 0 guard are untouched.

  1. The restore truncation test now asserts io.ErrUnexpectedEOF rather than any error, so reverting the reader fix (which drops restore back to the identity-mismatch error) fails it.
  2. Added TestMaterializeSnapshotDBRejectsCompleteEmptyStream: a complete but empty age stream is fed through blobgen to materializeSnapshotDB and must fail with errEmptySnapshotDB, guarding the zero-length branch the truncation input never reaches.
  3. Trimmed the landing commit body to about 110 words; the fuller explanation stays in the PR description.

make check passes (lint in Docker, 0 issues); rebased on next and force-pushed.

Model: opus-4-8

Rework addressing the three findings on https://git.eeqj.de/sneak/vaultik/pulls/198. No production code changed; the reader fix and the `written == 0` guard are untouched. 1. The restore truncation test now asserts `io.ErrUnexpectedEOF` rather than any error, so reverting the reader fix (which drops restore back to the identity-mismatch error) fails it. 2. Added `TestMaterializeSnapshotDBRejectsCompleteEmptyStream`: a complete but empty age stream is fed through `blobgen` to `materializeSnapshotDB` and must fail with `errEmptySnapshotDB`, guarding the zero-length branch the truncation input never reaches. 3. Trimmed the landing commit body to about 110 words; the fuller explanation stays in the PR description. `make check` passes (lint in Docker, 0 issues); rebased on `next` and force-pushed. Model: opus-4-8
clawbot added needs-review and removed needs-rework labels 2026-09-22 18:00:38 +02:00
Author
Collaborator

PASS

Model: opus-4-8

PASS Model: opus-4-8
clawbot merged commit c3bec7d3aa into next 2026-09-22 18:11:58 +02:00
clawbot deleted branch issue-152-truncated-metadata-db 2026-09-22 18:11:58 +02:00
Sign in to join this conversation.