Sprint 7 Code Review: Security Hardening + Critical Fixes

Sprint Goal Assessment

Sprint 7 aims to fix all critical and high-severity issues from the post-v1 code review audit, covering security vulnerabilities (DKIM enforcement, header injection), data loss risks (atomic file IDs), race conditions (verify timing), and PRD compliance (verify in setup). All five stories are implemented and the overall approach is solid.

Acceptance Criteria Checklist

S7.1 -- DKIM Enforcement

S7.2 -- CRLF Header Injection

S7.3 -- Atomic File ID Generation

S7.4 -- Verify Race Condition

S7.5 -- Verify in Setup

Bugs Found

Bug #1 (Non-blocker): validate_header_value is dead code after sanitization

In src/send.rs lines 77-83, write_common_headers first calls sanitize_header_value() which strips all \r and \n characters, then calls validate_header_value() on the already-sanitized values. Since sanitization removes all CR/LF, the validation check can never trigger -- it is dead code.

The S7.2 AC explicitly states: "compose_message() returns an error if a header value contains CRLF after sanitization (defense in depth)". The current implementation satisfies the "defense" idea (injection is prevented by sanitization), but the "in depth" layer (validation returning an error) is unreachable.

Recommendation: Either (a) validate first, then sanitize as a fallback, or (b) remove validate_header_value since it creates a false sense of defense-in-depth. Option (a) would be:

validate_header_value("From", &args.from)?;
let from = sanitize_header_value(&args.from);

This way, direct callers get an error, and the sanitization still protects as a safety net. Alternatively, just remove the dead validation since sanitization alone is sufficient.

Bug #2 (Non-blocker): In-Reply-To and References headers are not sanitized

In src/send.rs lines 91-98, the In-Reply-To and References headers are interpolated without CRLF sanitization:

msg.push_str(&format!("In-Reply-To: {reply_id}\r\n"));
...
msg.push_str(&format!("References: {refs}\r\n"));

These values come from args.reply_to and args.references, which in the MCP email_reply path originate from stored email frontmatter (meta.message_id, meta.references). A malicious inbound email could contain a Message-ID header with CRLF sequences, which would be stored verbatim in frontmatter and then injected into outbound headers when an agent replies.

Recommendation: Apply sanitize_header_value() to reply_id and refs before interpolation.

Test Coverage Gaps

1. S7.5 integration test gap: The setup_verify_step_available test is essentially a duplicate of verify_runner_trait_mock_pass -- it calls the mock directly rather than testing the verify step within run_setup_with_verify. The comment acknowledges this ("We can't fully test run_setup_with_verify here because it reads stdin"). Consider providing a stdin mock or extracting the verify prompt logic into a testable function.

2. No counter overflow guard in create_file_atomic: The counter (u32) increments without an upper bound. While extremely unlikely in practice (would need ~4 billion files in one day), a safety check like if counter > 999_999 { return Err(...) } would be prudent.

3. No test for CRLF in reply_to/references: Given the injection risk in Bug #2, tests for CRLF in reply_to and references fields would be valuable.

Security Issues

1. CRLF injection in In-Reply-To/References (Bug #2 above) -- Medium severity. A crafted inbound email could cause header injection in outbound replies.

2. send_with_transport still accepts Option<dkim_key> -- Low severity. The function signature still allows None for the DKIM key, meaning future callers could accidentally bypass DKIM. All current callers enforce it upstream, so this is a design note rather than a bug.

Alignment with PRD

• FR-18 (DKIM signing): Now properly enforced -- unsigned sends are rejected. Aligned.
• FR-8 (End-to-end verify in setup): Now integrated as an optional step. Aligned.
• NFR-6 (Secure defaults): DKIM enforcement improves this. CRLF sanitization is a good addition. The In-Reply-To/References gap slightly weakens it.

Summary

The sprint delivers all five stories with solid implementations. The atomic file creation (S7.3), verify race fix (S7.4), and DKIM enforcement (S7.1) are clean and correct. The CRLF sanitization (S7.2) effectively prevents the primary attack vector but has two gaps: dead validation code and unsanitized In-Reply-To/References headers.

Verdict: Needs minor fixes

The two bugs are non-blocking for the security goals (the primary injection vectors in From/To/Subject are properly handled), but Bug #2 (unsanitized In-Reply-To/References) is a genuine injection vector that should be addressed before merge given this is a security hardening sprint. Bug #1 (dead validation) is a code quality issue.

Re-review: Sprint 7 — Security Hardening + Critical Fixes

All five previously flagged issues have been satisfactorily addressed. APPROVED (posted as comment since GitHub prevents self-approval).

Issue-by-Issue Verification

1. Bug #1 (Dead validate_header_value code) — RESOLVED. Validation now runs before sanitization in write_common_headers (lines 77-79 validate raw values, lines 81-83 sanitize as safety net). All four CRLF injection tests now assert error returns instead of silent sanitization. The defense-in-depth layering is correct: validate → error on bad input, sanitize → safety net.

2. Bug #2 (Unsanitized In-Reply-To/References) — RESOLVED. sanitize_header_value() is applied to both reply_to (line 92) and references (line 95) before interpolation into headers. Two new tests (reply_to_crlf_injection_sanitized, references_crlf_injection_sanitized) confirm no header injection occurs through these fields.

3. Suggestion #3 (S7.5 test duplication) — RESOLVED. setup_verify_step_available is now a compile-time function pointer check that verifies run_setup_with_verify accepts the correct signature including &dyn VerifyRunner. This is clearly distinct from verify_runner_trait_mock_pass which tests mock behavior.

4. Suggestion #4 (No upper-bound guard on counter) — RESOLVED. MAX_COUNTER of 999,999 added to create_file_atomic with a descriptive error message when exhausted.

5. Suggestion #5 (send_with_transport accepts Option for DKIM key) — ACCEPTED AS-IS. The implementer's reasoning is sound: send_with_transport is the low-level transport layer, and DKIM enforcement is the caller's responsibility. Both run() and the MCP handlers now properly enforce DKIM (returning errors on missing keys), so all production paths are covered. Keeping Option at the transport layer maintains separation of concerns and enables clean testing.

Recommended merge commit message

[Sprint 7] Security hardening and critical fixes

- S7.1: Enforce DKIM signing on all outbound email paths (send::run,
  MCP email_send, MCP email_reply) — missing DKIM key now returns an
  error instead of silently sending unsigned messages
- S7.2: Prevent CRLF header injection in From/To/Subject (validate
  then sanitize) and In-Reply-To/References (sanitize)
- S7.3: Atomic file ID generation using OpenOptions::create_new to
  eliminate TOCTOU race in concurrent mail delivery
- S7.4: Fix verify race condition by snapshotting mailbox before
  sending test email; add missing-catchall-dir guard
- S7.5: Integrate optional end-to-end verify into setup wizard via
  VerifyRunner trait

LGTM — approving.