fix(store): survive one corrupt body on bulk read paths #429

2026-08-15T02:07:57Z

KofTwentyTwo commented

2026-08-15 02:07:57 +00:00

(Migrated from github.com)

Closes #265

The problem

Since decrypt started failing closed, one unauthenticatable body ciphertext took down an entire operation. Worst of all, backfill_message_fts runs inside migrate(), so one bit-rotted row stopped the daemon booting. list_messages, list_messages_page, get_message_chunk and list_messages_for_export each failed outright over one bad row out of fifty.

Single-item vs bulk — the distinction, and why

Single-item (get_message) stays fail-closed, unchanged. It names one message. There is no other row left for the caller to receive, and a message handed back with an empty body is indistinguishable from a genuinely empty one — exactly the tampering the AEAD exists to detect. The existing test asserting this is intact; a new test re-asserts it on the same corrupted row the bulk tests use, so the two behaviours are pinned as a deliberate distinction rather than an inconsistency.

Bulk paths surface a per-item error. They are asked for a set, and failing the set over one row hides the other forty-nine just as effectively as an empty body hides the one. Each row now comes back as:

pub type MessageRead = Result<Email, UnreadableBody>;

UnreadableBody carries the message key and received_at only — never body content, which is the exact material that failed to authenticate.

I chose per-item error over silent skipping for every bulk path, for one consistent reason: the row keeps its slot. Filtering unreadable rows out would shorten a page below the query's own window, so has_more and the keyset cursor would describe a sequence the store never returned, and rows past the gap would become unreachable. That is data loss, worse than the bug being fixed. Keeping the slot makes all the pagination arithmetic identical to before, and it means a caller cannot consume the page without meeting the failure.

Every failure is also logged at warn! naming the message key.

Per path

Path	Choice
`list_messages`, `list_messages_page`	Per-item `Err`; slot retained so the keyset cursor stays exact
`get_message_chunk` (triage)	Per-item `Err`; slot retained — a shortened chunk would both declare the scan finished early and rewind the resume key
`list_messages_for_export`	Per-item `Err`; the daemon omits it from the archive and names it in the response
`backfill_message_fts` (boot)	Skip the row, `warn!` per key + an aggregate count; boot proceeds
`get_message` (single item)	Unchanged — still `Err`

Search itself never decrypts (the FTS index holds write-time plaintext), so the only search-side exposure was the backfill.

Why the backfill skips rather than indexes an empty body: an FTS entry marks a message permanently indexed. An empty one would make its body permanently unsearchable with nothing left to report it. Leaving the row out means every subsequent open re-reports it, and a repaired row indexes itself with no intervention.

Why export names rather than counts: an archive is the artifact that outlives the store. Writing the message with an empty or "body unavailable" body would put text into a file no later reader could distinguish from real mail. Reporting the ids lets the user record exactly which mail their archive does not contain.

Wire + CLI

Additive proto fields (golden descriptor.bin regenerated):

ListMessagesResponse.unreadable_message_ids
ExportResponse.unreadable_message_ids
TriageProgress.unreadable_count (cumulative over a whole-store scan — unbounded, so a count)

nuncio-cli renders all three in both text and --json. Triage never evaluates filter rules against a body that failed authentication: a filter acting on corrupted bytes is worse than one that reports it did not run.

Tests

one_corrupt_body_does_not_destroy_a_bulk_read — three messages, middle one corrupted; every bulk path returns the two good ones, names the bad one, and no returned row carries a fabricated body. Single-item read of that same key still fails closed; the good messages remain individually readable.
fts_backfill_skips_one_corrupt_body_instead_of_failing_the_open — migrate() succeeds, the intact message indexes and is searchable, the corrupt row is absent from the index (not present-with-empty-body).
list_messages_reports_a_corrupt_body_instead_of_failing_the_page — wire-level: the page carries the readable messages, names the omitted one, and GetMessage on that id is still NotFound.

Mutation-verified: degrading the decoder to an empty body fails one_corrupt_body_does_not_destroy_a_bulk_read and corrupted_body_ciphertext_fails_closed_on_read; dropping unreadable_message_ids from the wire response fails the gRPC test. Both reverted.

Gate

cargo fmt --all -- --check, cargo check-all, cargo test-all all green; RUST_TEST_THREADS=2 cargo test-all x3 with zero FAILED lines. No schema migrations, no #[allow], no secrets logged.

Closes #265 ## The problem Since decrypt started failing closed, one unauthenticatable body ciphertext took down an entire operation. Worst of all, `backfill_message_fts` runs inside `migrate()`, so one bit-rotted row stopped the daemon booting. `list_messages`, `list_messages_page`, `get_message_chunk` and `list_messages_for_export` each failed outright over one bad row out of fifty. ## Single-item vs bulk — the distinction, and why **Single-item (`get_message`) stays fail-closed, unchanged.** It names one message. There is no other row left for the caller to receive, and a message handed back with an empty body is indistinguishable from a genuinely empty one — exactly the tampering the AEAD exists to detect. The existing test asserting this is intact; a new test re-asserts it on the same corrupted row the bulk tests use, so the two behaviours are pinned as a deliberate distinction rather than an inconsistency. **Bulk paths surface a per-item error.** They are asked for a *set*, and failing the set over one row hides the other forty-nine just as effectively as an empty body hides the one. Each row now comes back as: ```rust pub type MessageRead = Result<Email, UnreadableBody>; ``` `UnreadableBody` carries the message key and `received_at` only — never body content, which is the exact material that failed to authenticate. I chose per-item error over silent skipping for every bulk path, for one consistent reason: **the row keeps its slot.** Filtering unreadable rows out would shorten a page below the query's own window, so `has_more` and the keyset cursor would describe a sequence the store never returned, and rows past the gap would become unreachable. That is data loss, worse than the bug being fixed. Keeping the slot makes all the pagination arithmetic identical to before, and it means a caller cannot consume the page without meeting the failure. Every failure is also logged at `warn!` naming the message key. ## Per path | Path | Choice | |---|---| | `list_messages`, `list_messages_page` | Per-item `Err`; slot retained so the keyset cursor stays exact | | `get_message_chunk` (triage) | Per-item `Err`; slot retained — a shortened chunk would both declare the scan finished early and rewind the resume key | | `list_messages_for_export` | Per-item `Err`; the daemon omits it from the archive and **names** it in the response | | `backfill_message_fts` (boot) | Skip the row, `warn!` per key + an aggregate count; boot proceeds | | `get_message` (single item) | **Unchanged — still `Err`** | Search itself never decrypts (the FTS index holds write-time plaintext), so the only search-side exposure was the backfill. **Why the backfill skips rather than indexes an empty body:** an FTS entry marks a message permanently indexed. An empty one would make its body permanently unsearchable with nothing left to report it. Leaving the row out means every subsequent open re-reports it, and a repaired row indexes itself with no intervention. **Why export names rather than counts:** an archive is the artifact that outlives the store. Writing the message with an empty or `"body unavailable"` body would put text into a file no later reader could distinguish from real mail. Reporting the ids lets the user record exactly which mail their archive does not contain. ## Wire + CLI Additive proto fields (golden `descriptor.bin` regenerated): - `ListMessagesResponse.unreadable_message_ids` - `ExportResponse.unreadable_message_ids` - `TriageProgress.unreadable_count` (cumulative over a whole-store scan — unbounded, so a count) `nuncio-cli` renders all three in both text and `--json`. Triage never evaluates filter rules against a body that failed authentication: a filter acting on corrupted bytes is worse than one that reports it did not run. ## Tests - `one_corrupt_body_does_not_destroy_a_bulk_read` — three messages, middle one corrupted; every bulk path returns the two good ones, names the bad one, and no returned row carries a fabricated body. Single-item read of that same key still fails closed; the good messages remain individually readable. - `fts_backfill_skips_one_corrupt_body_instead_of_failing_the_open` — `migrate()` succeeds, the intact message indexes and is searchable, the corrupt row is absent from the index (not present-with-empty-body). - `list_messages_reports_a_corrupt_body_instead_of_failing_the_page` — wire-level: the page carries the readable messages, names the omitted one, and `GetMessage` on that id is still `NotFound`. **Mutation-verified:** degrading the decoder to an empty body fails `one_corrupt_body_does_not_destroy_a_bulk_read` and `corrupted_body_ciphertext_fails_closed_on_read`; dropping `unreadable_message_ids` from the wire response fails the gRPC test. Both reverted. ## Gate `cargo fmt --all -- --check`, `cargo check-all`, `cargo test-all` all green; `RUST_TEST_THREADS=2 cargo test-all` x3 with zero `FAILED` lines. No schema migrations, no `#[allow]`, no secrets logged.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#429