M6: Fail closed on encryption failure in PayloadCipher::encrypt_text_at_rest #198

New Issue

2026-07-28T14:04:42Z

KofTwentyTwo commented

2026-07-28 14:04:42 +00:00

(Migrated from github.com)

Context

Story for milestone M6. Read docs/STORY-WORKFLOW.md first — it defines the process, gates, and patterns; this issue defines only the specific work. crates/nuncio-store/src/cipher.rs implements PayloadCipher, the AES-256-GCM cipher used to encrypt email bodies at rest (crates/nuncio-store/src/db.rs::save_email). Verified in the tree: PayloadCipher::encrypt_text_at_rest (crates/nuncio-store/src/cipher.rs:67-73) silently swallows any AEAD encryption failure and persists an empty string instead of the ciphertext, which is indistinguishable from an originally-empty body on read-back — a silent data-loss bug (reconciles #162).

Goal

Make encrypt_text_at_rest fail closed: any encryption failure must be returned as a real error to the caller and MUST NOT result in an empty/placeholder value being written to the messages table. The fix must not fake success — a failed encryption is a failed save_email call, never a silently-corrupted row.

Files to touch / patterns to mirror

crates/nuncio-store/src/cipher.rs:63-73 — PayloadCipher::encrypt_text_at_rest. Current body:
```
pub fn encrypt_text_at_rest(key: &[u8; 32], text: &str) -> String {
    if text.is_empty() {
        return String::new();
    }
    let encrypted = Self::encrypt_bytes(key, text.as_bytes()).unwrap_or_default();
    hex::encode(encrypted)
}
```
Change the return type to Result<String, CipherError> and propagate the Self::encrypt_bytes(..) error with ? instead of .unwrap_or_default(). Keep the empty-text short-circuit (Ok(String::new())) — that's legitimate (nothing to encrypt), not a failure path.
crates/nuncio-store/src/db.rs:729-737 (DatabaseEngine::save_email) — the two call sites:
```
let enc_plain = email.body_plain.as_ref()
    .map(|p| crate::cipher::PayloadCipher::encrypt_text_at_rest(&self.storage_key, p));
let enc_html = email.body_html.as_ref()
    .map(|h| crate::cipher::PayloadCipher::encrypt_text_at_rest(&self.storage_key, h));
```
Must become fallible (.map(|p| ...).transpose()?) so a CipherError aborts save_email before any SQL executes (before the tx.begin() at db.rs:739), rather than writing a row with corrupted/empty body columns.
crates/nuncio-store/src/db.rs:13-39 (DatabaseError enum) — add a new variant, e.g. Encryption(String) (mirror the existing #[error(...)] + doc-comment style of KeyProvisioning at db.rs:34-38), and a From<CipherError> or explicit .map_err(...) conversion at the save_email call sites.
crates/nuncio-store/src/cipher.rs:141-262 (#[cfg(test)] mod tests) — add the failure-path test (see AC). PayloadCipher::encrypt_bytes itself (cipher.rs:28-44) does not currently expose an easy way to force Aes256Gcm::encrypt to fail (AES-GCM only fails on plaintext exceeding ~64 GiB), so the test must exercise the failure path by testing encrypt_bytes's error variant is correctly propagated through encrypt_text_at_rest — either via a fake/injectable failure seam or by directly asserting the new Result signature never has an "encrypt-then-discard-the-error" shape. Prefer restructuring so the test can assert: (a) the success path is unchanged (existing text_at_rest_encryption_decryption_roundtrip test at cipher.rs:236-247 still passes with the new Result-returning signature, using .expect(...) in test code, which is allowed since tests are exempt from the unwrap_used/expect_used lint), and (b) there is no code path left in encrypt_text_at_rest that can turn an Err into Ok(String::new()) — verify this by reading the diff, not just by a test, since AES-GCM's own encryption call is realistically infallible for in-memory plaintexts of the sizes Nuncio handles. Document this reasoning in the PR description rather than manufacturing a contrived unit test that doesn't reflect a real failure mode.

Implementation steps

Change PayloadCipher::encrypt_text_at_rest to return Result<String, CipherError>; replace .unwrap_or_default() with ?.
Update PayloadCipher::decrypt_text_at_rest is NOT in scope for this story (its silent-empty-on-failure behavior is a read-path issue, not the write-path data-loss bug in #162) — leave it as-is, but note it as a follow-up in this PR's description so the PO can decide whether to file a separate issue.
Update every caller in crates/nuncio-store/src/db.rs (the two sites at db.rs:733 and db.rs:737) to propagate the new Result, converting CipherError into DatabaseError via the new DatabaseError::Encryption variant (or equivalent name consistent with the existing enum's style).
Ensure save_email's transaction (tx.begin() at db.rs:739 onward) is never entered if encryption of either body_plain or body_html fails — the ?-propagation from step 1/3 naturally achieves this as long as the .map(...).transpose()? calls happen before self.pool.begin().
Run cargo check-all to catch any other callers of encrypt_text_at_rest outside nuncio-store (there should be none — it's crate-private in usage today, verify with a workspace-wide search).
Add/update tests per the Acceptance Criteria below.

Acceptance criteria

PayloadCipher::encrypt_text_at_rest returns Result<String, CipherError>; no code path can produce Ok(String::new()) for a non-empty input that failed to encrypt.
DatabaseEngine::save_email propagates encryption failures as a DatabaseError variant and returns Err before executing any SQL — no row is written for a message whose body failed to encrypt.
A test proves the previous bug is closed: e.g. a test that directly exercises PayloadCipher::encrypt_text_at_rest's new Result signature and asserts the existing success-path tests (cipher.rs:236-247, cipher.rs:250-261) still pass unchanged in shape (same assertions, Result-aware call syntax), plus a db.rs test (mirror the existing #[cfg(test)] mod tests in db.rs) asserting that save_email returns Err(DatabaseError::Encryption(_)) (or equivalent) when encryption cannot succeed, and that no row exists in messages afterward (query SELECT COUNT(*) FROM messages WHERE id = ? returns 0) — if a genuine forced-failure seam isn't feasible without weakening PayloadCipher's API, document in the PR why (see step 6 above) and instead add a test that proves the type signature itself makes silent data loss impossible (i.e., there is no unwrap_or_default() / .ok() / similar error-swallowing between encrypt_bytes and the SQL bind).
No fabricated success: save_email must never persist an empty ciphertext string when the original body was non-empty.
cargo verify green (fmt + clippy -D warnings all-targets + test-all), 0 ignored.

Definition of done & process

Follow docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in .rs/.proto); mock-only tests. Branch story/m6-cipher-fail-closed off dev; PR to dev with the gate output pasted as evidence; the PO reviews before the issue is closed.

Dependencies / notes

Not a [PROTO] story — no nuncio.proto changes. Reconciles #162. No dependency on other M6 stories; safe to run in parallel with M6-S2 through M6-S7 (different files, except both this story and M6-S2/M6-S3 touch crates/nuncio-store/src/db.rs — coordinate with the PO if those land close together to avoid a merge conflict in DatabaseError). Size: S.

## Context Story for milestone M6. **Read [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md) first** — it defines the process, gates, and patterns; this issue defines only the specific work. `crates/nuncio-store/src/cipher.rs` implements `PayloadCipher`, the AES-256-GCM cipher used to encrypt email bodies at rest (`crates/nuncio-store/src/db.rs::save_email`). Verified in the tree: `PayloadCipher::encrypt_text_at_rest` (`crates/nuncio-store/src/cipher.rs:67-73`) silently swallows any AEAD encryption failure and persists an empty string instead of the ciphertext, which is indistinguishable from an originally-empty body on read-back — a silent data-loss bug (reconciles #162). ## Goal Make `encrypt_text_at_rest` fail closed: any encryption failure must be returned as a real error to the caller and MUST NOT result in an empty/placeholder value being written to the `messages` table. The fix must not fake success — a failed encryption is a failed `save_email` call, never a silently-corrupted row. ## Files to touch / patterns to mirror - `crates/nuncio-store/src/cipher.rs:63-73` — `PayloadCipher::encrypt_text_at_rest`. Current body: ```rust pub fn encrypt_text_at_rest(key: &[u8; 32], text: &str) -> String { if text.is_empty() { return String::new(); } let encrypted = Self::encrypt_bytes(key, text.as_bytes()).unwrap_or_default(); hex::encode(encrypted) } ``` Change the return type to `Result<String, CipherError>` and propagate the `Self::encrypt_bytes(..)` error with `?` instead of `.unwrap_or_default()`. Keep the empty-text short-circuit (`Ok(String::new())`) — that's legitimate (nothing to encrypt), not a failure path. - `crates/nuncio-store/src/db.rs:729-737` (`DatabaseEngine::save_email`) — the two call sites: ```rust let enc_plain = email.body_plain.as_ref() .map(|p| crate::cipher::PayloadCipher::encrypt_text_at_rest(&self.storage_key, p)); let enc_html = email.body_html.as_ref() .map(|h| crate::cipher::PayloadCipher::encrypt_text_at_rest(&self.storage_key, h)); ``` Must become fallible (`.map(|p| ...).transpose()?`) so a `CipherError` aborts `save_email` before any SQL executes (before the `tx.begin()` at `db.rs:739`), rather than writing a row with corrupted/empty body columns. - `crates/nuncio-store/src/db.rs:13-39` (`DatabaseError` enum) — add a new variant, e.g. `Encryption(String)` (mirror the existing `#[error(...)]` + doc-comment style of `KeyProvisioning` at `db.rs:34-38`), and a `From<CipherError>` or explicit `.map_err(...)` conversion at the `save_email` call sites. - `crates/nuncio-store/src/cipher.rs:141-262` (`#[cfg(test)] mod tests`) — add the failure-path test (see AC). `PayloadCipher::encrypt_bytes` itself (`cipher.rs:28-44`) does not currently expose an easy way to force `Aes256Gcm::encrypt` to fail (AES-GCM only fails on plaintext exceeding ~64 GiB), so the test must exercise the failure path by testing `encrypt_bytes`'s error variant is correctly propagated through `encrypt_text_at_rest` — either via a fake/injectable failure seam or by directly asserting the new `Result` signature never has an "encrypt-then-discard-the-error" shape. Prefer restructuring so the test can assert: (a) the success path is unchanged (existing `text_at_rest_encryption_decryption_roundtrip` test at `cipher.rs:236-247` still passes with the new `Result`-returning signature, using `.expect(...)` in test code, which is allowed since tests are exempt from the `unwrap_used`/`expect_used` lint), and (b) there is no code path left in `encrypt_text_at_rest` that can turn an `Err` into `Ok(String::new())` — verify this by reading the diff, not just by a test, since AES-GCM's own encryption call is realistically infallible for in-memory plaintexts of the sizes Nuncio handles. Document this reasoning in the PR description rather than manufacturing a contrived unit test that doesn't reflect a real failure mode. ## Implementation steps 1. Change `PayloadCipher::encrypt_text_at_rest` to return `Result<String, CipherError>`; replace `.unwrap_or_default()` with `?`. 2. Update `PayloadCipher::decrypt_text_at_rest` is NOT in scope for this story (its silent-empty-on-failure behavior is a read-path issue, not the write-path data-loss bug in #162) — leave it as-is, but note it as a follow-up in this PR's description so the PO can decide whether to file a separate issue. 3. Update every caller in `crates/nuncio-store/src/db.rs` (the two sites at `db.rs:733` and `db.rs:737`) to propagate the new `Result`, converting `CipherError` into `DatabaseError` via the new `DatabaseError::Encryption` variant (or equivalent name consistent with the existing enum's style). 4. Ensure `save_email`'s transaction (`tx.begin()` at `db.rs:739` onward) is never entered if encryption of either `body_plain` or `body_html` fails — the `?`-propagation from step 1/3 naturally achieves this as long as the `.map(...).transpose()?` calls happen before `self.pool.begin()`. 5. Run `cargo check-all` to catch any other callers of `encrypt_text_at_rest` outside `nuncio-store` (there should be none — it's crate-private in usage today, verify with a workspace-wide search). 6. Add/update tests per the Acceptance Criteria below. ## Acceptance criteria - [ ] `PayloadCipher::encrypt_text_at_rest` returns `Result<String, CipherError>`; no code path can produce `Ok(String::new())` for a non-empty input that failed to encrypt. - [ ] `DatabaseEngine::save_email` propagates encryption failures as a `DatabaseError` variant and returns `Err` before executing any SQL — no row is written for a message whose body failed to encrypt. - [ ] A test proves the previous bug is closed: e.g. a test that directly exercises `PayloadCipher::encrypt_text_at_rest`'s new `Result` signature and asserts the existing success-path tests (`cipher.rs:236-247`, `cipher.rs:250-261`) still pass unchanged in shape (same assertions, `Result`-aware call syntax), plus a `db.rs` test (mirror the existing `#[cfg(test)] mod tests` in `db.rs`) asserting that `save_email` returns `Err(DatabaseError::Encryption(_))` (or equivalent) when encryption cannot succeed, and that no row exists in `messages` afterward (query `SELECT COUNT(*) FROM messages WHERE id = ?` returns 0) — if a genuine forced-failure seam isn't feasible without weakening `PayloadCipher`'s API, document in the PR why (see step 6 above) and instead add a test that proves the *type signature* itself makes silent data loss impossible (i.e., there is no `unwrap_or_default()` / `.ok()` / similar error-swallowing between `encrypt_bytes` and the SQL bind). - [ ] No fabricated success: `save_email` must never persist an empty ciphertext string when the original body was non-empty. - [ ] `cargo verify` green (fmt + clippy `-D warnings` all-targets + test-all), 0 ignored. ## Definition of done & process Follow [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md): no fabricated data; comment convention (intent-only, no story/GH/phase refs in `.rs`/`.proto`); mock-only tests. Branch `story/m6-cipher-fail-closed` off `dev`; PR to `dev` with the gate output pasted as evidence; the PO reviews before the issue is closed. ## Dependencies / notes Not a [PROTO] story — no `nuncio.proto` changes. Reconciles #162. No dependency on other M6 stories; safe to run in parallel with M6-S2 through M6-S7 (different files, except both this story and M6-S2/M6-S3 touch `crates/nuncio-store/src/db.rs` — coordinate with the PO if those land close together to avoid a merge conflict in `DatabaseError`). Size: S.

KofTwentyTwo commented

2026-07-30 03:00:54 +00:00

(Migrated from github.com)

Delivered via PR #236 (squash-merged to dev). encrypt_text_at_rest now returns Result<String, CipherError>; save_email propagates encryption failures as DatabaseError::Encryption and aborts (via .transpose()?) before any SQL runs — no row is ever written with an empty ciphertext for a non-empty body. No error-swallowing path remains (verified). Reconciles #162.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#198