fix(store): fail closed on encryption failure in save_email #236

2026-07-30T02:59:28Z

KofTwentyTwo commented

2026-07-30 02:59:28 +00:00

(Migrated from github.com)

Summary

Fixes #198 (reconciles #162). PayloadCipher::encrypt_text_at_rest swallowed any AEAD encryption failure with .unwrap_or_default(), silently persisting an empty string in place of ciphertext — a value indistinguishable from a genuinely empty body on read-back. This is a silent data-loss bug on the mail body write path.

Fix:

encrypt_text_at_rest now returns Result<String, CipherError>. The empty-input short-circuit (Ok(String::new())) is preserved — an empty body legitimately has nothing to encrypt, and is not a failure.
Added DatabaseError::Encryption(String) (mirrors the KeyProvisioning style/doc-comment) plus a From<CipherError> for DatabaseError impl.
save_email now resolves enc_plain/enc_html via .transpose()? before self.pool.begin() runs, so a cipher failure aborts the call before any SQL executes — no row is ever written with a corrupted/empty body column.
Verified (via cargo check-all across the workspace) that encrypt_text_at_rest has no other callers outside nuncio-store; the only other hit is a doc-comment reference in vault.rs.

Type-signature proof of fail-closed behavior

The PRIMARY evidence this bug can't recur is structural, not test-based: after this change there is no unwrap_or_default() / .ok() / unwrap_or(...) anywhere between PayloadCipher::encrypt_bytes and the SQL bind in save_email. Every fallible step (encrypt_bytes → encrypt_text_at_rest → the .transpose()? in save_email) is connected by ?, so a CipherError can only ever propagate as Err(DatabaseError::Encryption(_)) — it is not possible for it to compile down to a silently-substituted empty string.

Testing approach (honest, no fabricated failure)

AES-256-GCM is realistically infallible for in-memory plaintexts (failure requires exceeding ~64 GiB of data for a single nonce, which is not a scenario this codebase can hit for email bodies). Per the story brief, I deliberately did not manufacture a contrived forced-failure unit test for that unreachable path. Instead:

The existing success-path tests (text_at_rest_encryption_decryption_roundtrip, text_at_rest_wrong_key_fails_to_decrypt) were updated to the new Result signature using .expect(...) (tests are exempt from the unwrap/expect lint) and still pass.
I looked for a clean injectable failure seam into save_email (e.g. swapping the cipher via a trait) but PayloadCipher is an associated-function-only type with no room to inject a fake failure without weakening its public API for production callers, which the story brief says not to do. So this PR relies on the type-signature proof above plus the passing success-path tests, rather than a fake db.rs-level failure test.

Out of scope / follow-up

decrypt_text_at_rest still silently returns an empty string on any decode/decrypt/UTF-8 failure — that's a read-path issue (turns a corrupted/tampered ciphertext into a fabricated "empty body" instead of a decode error), distinct from this write-path data-loss bug. Left as-is per the story brief; recommend the PO file a follow-up issue to make the read path fail closed too (return Result and surface a decode error to callers instead of coercing to "").

Gate output

cargo fmt --all -- --check — clean, no diff.

cargo check-all (clippy --all-targets --workspace -D warnings) — clean, zero warnings.

cargo test-all — all green, 0 ignored:

test result: ok. 25 passed; 0 failed; 0 ignored ...   (nuncio-cal)
test result: ok. 2 passed; 0 failed; 0 ignored ...    (nuncio-cal doctest-adjacent)
test result: ok. 28 passed; 0 failed; 0 ignored ...   (nuncio-cli)
test result: ok. 4 passed; 0 failed; 0 ignored ...    (nuncio-contacts)
test result: ok. 12 passed; 0 failed; 0 ignored ...
test result: ok. 3 passed; 0 failed; 0 ignored ...
test result: ok. 52 passed; 0 failed; 0 ignored ...
test result: ok. 26 passed; 0 failed; 0 ignored ...
test result: ok. 2 passed; 0 failed; 0 ignored ...
test result: ok. 52 passed; 0 failed; 0 ignored ...
test result: ok. 3 passed; 0 failed; 0 ignored ...
test result: ok. 23 passed; 0 failed; 0 ignored ...
test result: ok. 57 passed; 0 failed; 0 ignored ...   (nuncio-store, includes cipher.rs)
test result: ok. 117 passed; 0 failed; 0 ignored ...  (nunciod unit tests)
test result: ok. 1 passed; 0 failed; 0 ignored ...    (accounts_lifecycle_e2e_test)
test result: ok. 1 passed; 0 failed; 0 ignored ...    (calendar_e2e_test)
test result: ok. 1 passed; 0 failed; 0 ignored ...    (contacts_e2e_test)
test result: ok. 1 passed; 0 failed; 0 ignored ...    (daemon_e2e_test)
test result: ok. 2 passed; 0 failed; 0 ignored ...    (filters_triage_e2e_test)
test result: ok. 6 passed; 0 failed; 0 ignored ...    (grpc_auth_test)
test result: ok. 1 passed; 0 failed; 0 ignored ...    (spine_e2e_test)

Total: 419 passed, 0 failed, 0 ignored across the workspace. The local pre-commit hook (identical fmt/clippy/test gate) also passed on this commit.

Test plan

cargo fmt --all -- --check
cargo check-all
cargo test-all (0 ignored)
Verified no other callers of encrypt_text_at_rest outside nuncio-store
Verified no unwrap_or_default/.ok()/unwrap_or remains between encrypt_bytes and the SQL bind

## Summary Fixes #198 (reconciles #162). `PayloadCipher::encrypt_text_at_rest` swallowed any AEAD encryption failure with `.unwrap_or_default()`, silently persisting an **empty string** in place of ciphertext — a value indistinguishable from a genuinely empty body on read-back. This is a silent data-loss bug on the mail body write path. **Fix:** - `encrypt_text_at_rest` now returns `Result<String, CipherError>`. The empty-input short-circuit (`Ok(String::new())`) is preserved — an empty body legitimately has nothing to encrypt, and is not a failure. - Added `DatabaseError::Encryption(String)` (mirrors the `KeyProvisioning` style/doc-comment) plus a `From<CipherError> for DatabaseError` impl. - `save_email` now resolves `enc_plain`/`enc_html` via `.transpose()?` **before** `self.pool.begin()` runs, so a cipher failure aborts the call before any SQL executes — no row is ever written with a corrupted/empty body column. - Verified (via `cargo check-all` across the workspace) that `encrypt_text_at_rest` has no other callers outside `nuncio-store`; the only other hit is a doc-comment reference in `vault.rs`. ## Type-signature proof of fail-closed behavior The PRIMARY evidence this bug can't recur is structural, not test-based: after this change there is **no** `unwrap_or_default()` / `.ok()` / `unwrap_or(...)` anywhere between `PayloadCipher::encrypt_bytes` and the SQL bind in `save_email`. Every fallible step (`encrypt_bytes` → `encrypt_text_at_rest` → the `.transpose()?` in `save_email`) is connected by `?`, so a `CipherError` can only ever propagate as `Err(DatabaseError::Encryption(_))` — it is not possible for it to compile down to a silently-substituted empty string. ## Testing approach (honest, no fabricated failure) AES-256-GCM is realistically infallible for in-memory plaintexts (failure requires exceeding ~64 GiB of data for a single nonce, which is not a scenario this codebase can hit for email bodies). Per the story brief, I deliberately did **not** manufacture a contrived forced-failure unit test for that unreachable path. Instead: - The existing success-path tests (`text_at_rest_encryption_decryption_roundtrip`, `text_at_rest_wrong_key_fails_to_decrypt`) were updated to the new `Result` signature using `.expect(...)` (tests are exempt from the `unwrap`/`expect` lint) and still pass. - I looked for a clean injectable failure seam into `save_email` (e.g. swapping the cipher via a trait) but `PayloadCipher` is an associated-function-only type with no room to inject a fake failure without weakening its public API for production callers, which the story brief says not to do. So this PR relies on the type-signature proof above plus the passing success-path tests, rather than a fake `db.rs`-level failure test. ## Out of scope / follow-up `decrypt_text_at_rest` still silently returns an empty string on any decode/decrypt/UTF-8 failure — that's a **read-path** issue (turns a corrupted/tampered ciphertext into a fabricated "empty body" instead of a decode error), distinct from this write-path data-loss bug. Left as-is per the story brief; recommend the PO file a follow-up issue to make the read path fail closed too (return `Result` and surface a decode error to callers instead of coercing to `""`). ## Gate output `cargo fmt --all -- --check` — clean, no diff. `cargo check-all` (clippy `--all-targets --workspace -D warnings`) — clean, zero warnings. `cargo test-all` — all green, 0 ignored: ``` test result: ok. 25 passed; 0 failed; 0 ignored ... (nuncio-cal) test result: ok. 2 passed; 0 failed; 0 ignored ... (nuncio-cal doctest-adjacent) test result: ok. 28 passed; 0 failed; 0 ignored ... (nuncio-cli) test result: ok. 4 passed; 0 failed; 0 ignored ... (nuncio-contacts) test result: ok. 12 passed; 0 failed; 0 ignored ... test result: ok. 3 passed; 0 failed; 0 ignored ... test result: ok. 52 passed; 0 failed; 0 ignored ... test result: ok. 26 passed; 0 failed; 0 ignored ... test result: ok. 2 passed; 0 failed; 0 ignored ... test result: ok. 52 passed; 0 failed; 0 ignored ... test result: ok. 3 passed; 0 failed; 0 ignored ... test result: ok. 23 passed; 0 failed; 0 ignored ... test result: ok. 57 passed; 0 failed; 0 ignored ... (nuncio-store, includes cipher.rs) test result: ok. 117 passed; 0 failed; 0 ignored ... (nunciod unit tests) test result: ok. 1 passed; 0 failed; 0 ignored ... (accounts_lifecycle_e2e_test) test result: ok. 1 passed; 0 failed; 0 ignored ... (calendar_e2e_test) test result: ok. 1 passed; 0 failed; 0 ignored ... (contacts_e2e_test) test result: ok. 1 passed; 0 failed; 0 ignored ... (daemon_e2e_test) test result: ok. 2 passed; 0 failed; 0 ignored ... (filters_triage_e2e_test) test result: ok. 6 passed; 0 failed; 0 ignored ... (grpc_auth_test) test result: ok. 1 passed; 0 failed; 0 ignored ... (spine_e2e_test) ``` Total: 419 passed, 0 failed, 0 ignored across the workspace. The local pre-commit hook (identical fmt/clippy/test gate) also passed on this commit. ## Test plan - [x] `cargo fmt --all -- --check` - [x] `cargo check-all` - [x] `cargo test-all` (0 ignored) - [x] Verified no other callers of `encrypt_text_at_rest` outside `nuncio-store` - [x] Verified no `unwrap_or_default`/`.ok()`/`unwrap_or` remains between `encrypt_bytes` and the SQL bind

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#236