[Sprint 3] MCP Server#4
Conversation
Implement MCP server using rmcp crate with stdio transport. All 9 MCP tools for AI agent email access: mailbox_create, mailbox_list, mailbox_delete, email_list (with filters), email_read, email_mark_read, email_mark_unread, email_send (with DKIM), and email_reply (with threading). Includes 21 unit tests and 11 MCP integration tests.
uzyn
left a comment
There was a problem hiding this comment.
Sprint 3 Review -- MCP Server
Sprint Goal Assessment
Sprint 3 aims to give AI agents full email access via MCP (Model Context Protocol). The implementation adds a new src/mcp.rs module (835 lines) with all 9 MCP tools, 21 unit tests, and 11 integration tests that communicate via JSON-RPC over stdio. The sprint goal is substantively met.
Acceptance Criteria Checklist
S3.1 -- MCP Transport + Mailbox Tools
| Criterion | Status |
|---|---|
aimx mcp starts an MCP server in stdio mode |
Met -- uses rmcp crate with stdio transport |
Server responds to MCP initialize handshake correctly |
Met -- integration test mcp_initialize_handshake verifies |
mailbox_create(name) creates mailbox and returns confirmation |
Met -- delegates to existing mailbox::create_mailbox |
mailbox_list() returns all mailboxes with message counts (total and unread) |
Met -- includes both total and unread counts |
mailbox_delete(name) deletes mailbox (with appropriate safeguards) |
Met -- delegates to mailbox::delete_mailbox which blocks catchall deletion |
| Server exits cleanly when stdin closes | Met -- integration test mcp_clean_exit_on_stdin_close verifies |
Integration tests: spawn aimx mcp, send JSON-RPC requests, assert responses |
Met -- 11 integration tests via McpClient harness |
S3.2 -- Email Read + List Tools
| Criterion | Status |
|---|---|
email_list(mailbox) returns frontmatter of all emails |
Met |
email_list supports filters: unread, from, since, subject |
Met -- all four filters implemented |
email_read(mailbox, id) returns full Markdown content |
Met |
email_mark_read(mailbox, id) updates frontmatter |
Met |
email_mark_unread(mailbox, id) updates frontmatter |
Met |
| Non-existent mailbox or email ID returns clear MCP error | Met |
| Unit tests for email listing with each filter type | Met -- 8 filter tests |
| Unit tests for mark read/unread | Met -- 4 tests |
| Integration tests via MCP JSON-RPC | Met |
S3.3 -- Email Send + Reply Tools
| Criterion | Status |
|---|---|
email_send(from_mailbox, to, subject, body, attachments?) composes, DKIM-signs, and sends |
Met -- composes and delegates to send_with_transport |
email_reply(mailbox, id, body) replies with correct threading headers |
Met -- sets In-Reply-To from original message_id |
| Send/reply return confirmation with Message-ID | Met |
| Errors return clear MCP errors | Met |
| Integration tests via MCP JSON-RPC for send and reply | Partial -- error paths tested; happy-path send/reply not tested because sendmail is unavailable in CI (noted in PR description). Acceptable tradeoff. |
Bugs Found
Bug 1: Path traversal in email ID parameter (Security) 🚨
email_read, email_mark_read, email_mark_unread, email_reply, and set_read_status all construct file paths from unsanitized user input:
let filepath = mailbox_dir.join(format!("{}.md", params.id));An MCP client could pass id: "../../etc/passwd" (minus the .md extension, but similar payloads with ../ sequences) to read or overwrite files outside the mailbox directory. While the mailbox parameter is validated against config.mailboxes.contains_key(), the id parameter has no validation at all.
The mailbox module already has validate_mailbox_name() that blocks .., /, \, etc. A similar validation is needed for email IDs. Recommended fix: add a validate_email_id(id) function that rejects .., /, \, and null bytes, and call it in all tool functions that accept an id parameter.
Bug 2: Sending from catchall produces invalid From address
When email_send is called with from_mailbox: "catchall", the code does:
let from_address = &config.mailboxes[¶ms.from_mailbox].address;The catchall address is *@domain.com, which is a glob pattern, not a valid email address. This will produce a From: *@domain.com header in the outbound email, which is invalid RFC 5322 and will cause delivery failures or rejection by receiving MTAs.
Recommended fix: either (a) reject sending from catchall with a clear error, or (b) construct a valid catchall@domain.com address for the From header when sending from the catchall mailbox.
Non-Blocker Suggestions
-
Silent
sincefilter failure: If the user passes an invalid RFC 3339 datetime string to thesincefilter,parse_from_rfc3339fails silently (.ok()) and the filter is simply ignored. This could confuse users. Consider returning an error for unparseablesincevalues. -
email_replydoes not build References chain: The sprint 2 code has abuild_references()function that appends the original Message-ID to the existing References chain. However,email_replyin the MCP module only passesreply_to: Some(meta.message_id)toSendArgs, which results inReferencesbeing set to just the immediate parent's Message-ID. If the original email itself was part of a longer thread (with its ownreferencesfield in frontmatter), that chain is lost. Thebuild_referencesfunction exists insend.rsbut is marked#[allow(dead_code)]-- it should be used here. This is non-blocking because single-hop replies will still thread correctly, but multi-hop reply chains will break. -
Duplicated DKIM loading pattern: The
email_sendandemail_replytool functions have identical 8-line blocks for loading the DKIM private key and constructingdkim_info. This could be extracted into a helper method onAimxMcpServer. -
serde_jsonadded as a runtime dependency:serde_jsonis listed in[dependencies]rather than[dev-dependencies]. It is required byrmcpat runtime, so this is correct, but worth noting that the binary size will increase. Not actionable -- just an observation. -
tokiofeatures = "full": The MCP server only needs the IO-related features from tokio. Usingfeatures = ["full"]pulls in the entire tokio runtime (timers, signal handling, fs, net, etc.). Consider narrowing tofeatures = ["rt-multi-thread", "macros", "io-util", "io-std"]for a smaller binary. Non-blocking.
Test Coverage Assessment
Good coverage areas:
- 21 unit tests covering: frontmatter parsing, email listing, all filter types, combined filters, read/unread status toggling, error cases, email address extraction, mailbox unread counts, tool registration
- 11 integration tests covering: MCP handshake, tool listing, mailbox CRUD, email list+read+filter, mark read/unread, error cases (missing mailbox, missing email, catchall delete), clean exit
Gaps:
- No unit test for
email_sendoremail_replyhappy paths (understandable given sendmail dependency, but could use the existingMockTransportfromsend.rs) - No test for the catchall
from_addressbug - No test for path traversal via malicious
idparameter - No test for invalid
sincedatetime handling
Security Issues
- Path traversal via
idparameter (Bug 1 above) -- the email ID is user-supplied and concatenated into filesystem paths without sanitization. This allows reading and writing files outside the intended mailbox directory.
PRD Alignment
The implementation aligns well with the PRD requirements:
- FR-22 (stdio mode): Met
- FR-23 (mailbox tools): Met
- FR-24 (email tools with filters): Met
- FR-25 (mark read/unread): Met
- NFR-2 (no daemon): Met --
aimx mcpis a stdio session, not a long-running daemon
Summary
Verdict: Needs minor fixes
The MCP server implementation is well-structured and thorough. All 9 tools are implemented, integration tests verify the full JSON-RPC communication flow, and the code reuses existing modules cleanly. Two bugs need to be fixed before merge:
- Path traversal in email ID parameter -- security issue allowing filesystem escape via crafted ID strings. Straightforward fix: add ID validation matching the existing mailbox name validation.
- Catchall From address --
email_sendfrom catchall producesFrom: *@domain.com, which is invalid. Fix by rejecting catchall sends or synthesizing a valid address.
Both fixes are small and self-contained. Once addressed, this PR is ready to merge.
Blockers: - Validate email ID in all tool functions to prevent path traversal - Reject sending/replying from catchall mailbox (invalid From address) Non-blockers: - Return error on invalid 'since' datetime instead of silently ignoring - Use build_references() for proper References chain in email_reply - Extract DKIM key loading into shared helper function
Review Response (7b3811a)FixedBlocker 1 -- Path traversal in email ID parameter Blocker 2 -- Sending from catchall produces invalid From address Non-blocker 1 -- Silent Non-blocker 2 -- Non-blocker 3 -- Duplicated DKIM loading pattern Intentionally left as-isNon-blocker 4 -- Non-blocker 5 -- Test resultsAll 89 unit tests and 23 integration tests pass. |
Re-Review: All Blockers Resolved -- APPROVEDVerified each previously flagged issue against the actual code in commit 7b3811a: Blocker 1: Path traversal in email ID -- FIXED
Blocker 2: Catchall produces invalid From address -- FIXEDBoth Non-blocker 1: Silent
|
…robustness - Reorder validate-before-sanitize in write_common_headers so compose_message() returns an error when From/To/Subject contain CRLF, with sanitization as a safety net (Bug #1) - Sanitize In-Reply-To and References headers to prevent CRLF injection from malicious inbound Message-ID values (Bug #2) - Add upper-bound guard (999,999) on counter in create_file_atomic to prevent theoretical u32 overflow (Suggestion #4) - Replace duplicate setup_verify_step_available test with a compile-time signature check that is distinct from verify_runner_trait_mock_pass (Suggestion #3) - Update CRLF injection tests to verify error return instead of silent sanitization, add new tests for reply_to/references injection
…lans Code-review-backed fix plans for each of the 10 findings, with file:line refs, effort estimates, and a priority order. No code changes yet — this consolidates the investigation so fixes can be sequenced. - #10 DKIM mismatch: not a code bug; DNS republish + optional startup check - #9 shell injection: pass trigger vars via env, not string substitution - #8 MCP writes: route state mutations through daemon UDS - #7 claude-code hint: print `claude mcp add` command post-install - #4 send config read: move mailbox resolution to daemon side - #2 SPF: plumb envelope MAIL FROM from smtp session through ingest - #5 wildcard send: remove wildcard branch from resolve_from_mailbox - #1 mailbox create: add restart hint (or route via daemon) - #3 plan wording: clarify "compose new" in docs/manual-test.md
* docs: add manual test results with findings Full execution of docs/manual-test.md against agent.zeroshot.lol. 10 findings recorded with severity and fix direction, notably: - P0 DKIM key on disk does not match DNS TXT (root cause of outbound dkim=fail at Gmail) - P0 Shell injection in on_receive cmd template expansion - P1 MCP write ops (email_mark_read, etc.) fail when MCP runs as non-root due to root:root 0644 mailbox files * docs: add Recommended fixes section with per-finding implementation plans Code-review-backed fix plans for each of the 10 findings, with file:line refs, effort estimates, and a priority order. No code changes yet — this consolidates the investigation so fixes can be sequenced. - #10 DKIM mismatch: not a code bug; DNS republish + optional startup check - #9 shell injection: pass trigger vars via env, not string substitution - #8 MCP writes: route state mutations through daemon UDS - #7 claude-code hint: print `claude mcp add` command post-install - #4 send config read: move mailbox resolution to daemon side - #2 SPF: plumb envelope MAIL FROM from smtp session through ingest - #5 wildcard send: remove wildcard branch from resolve_from_mailbox - #1 mailbox create: add restart hint (or route via daemon) - #3 plan wording: clarify "compose new" in docs/manual-test.md
Sprint 44 (post-launch security + quick fixes) addresses findings #9, #10, #7, #1-tier-1, #3. Sprint 45 (strict outbound + MCP writes via daemon addresses #4, #5, #8 and introduces UDS MARK-READ/MARK-UNREAD verbs. Sprint 46 (mailbox CRUD via UDS) addresses #1-tier-2 with MAILBOX-CREATE/MAILBOX-DELETE verbs so daemon picks up changes live. PRD FR-18d tightened: outbound send now requires a concrete non-wildcard mailbox under config.domain; catchall is inbound-only. PRD FR-18e added to cover the new state-mutation verbs on the UDS socket. Finding #2 (SPF envelope MAIL FROM) excluded — already shipped in cd22428. EOF )
- `aimx send` no longer reads /etc/aimx/config.toml; daemon parses From: from the submitted body and resolves the sender mailbox against its in-memory Config. - Outbound rejects both foreign-domain From (ERR DOMAIN) and any From whose local part does not match an explicitly configured non-wildcard mailbox (ERR MAILBOX). Catchall is inbound-only per FR-18d (tightened). - New AIMX/1 MARK-READ / MARK-UNREAD UDS verbs with typed codec, per- mailbox tokio RwLock in a StateContext, and client wiring in `aimx mcp` so email_mark_read / email_mark_unread work without root write access to mailbox files. - New error codes PROTOCOL / NOTFOUND / IO added; legacy From-Mailbox: header silently ignored for forward-compatibility. Closes findings #4, #5, #8 from the 2026-04-17 manual test run.
Add MCP server with 9 email/mailbox tools (Sprint 3) Implement MCP server using rmcp crate with stdio transport. Provides 9 tools for AI agent email access: mailbox_create, mailbox_list, mailbox_delete, email_list (with filters), email_read, email_mark_read, email_mark_unread, email_send (with DKIM signing), and email_reply (with proper threading via References chain). Includes path traversal validation on email IDs, catchall mailbox send guard, DKIM key loading helper, and 21 unit + 23 integration tests.
…lans Code-review-backed fix plans for each of the 10 findings, with file:line refs, effort estimates, and a priority order. No code changes yet — this consolidates the investigation so fixes can be sequenced. - #10 DKIM mismatch: not a code bug; DNS republish + optional startup check - #9 shell injection: pass trigger vars via env, not string substitution - #8 MCP writes: route state mutations through daemon UDS - #7 claude-code hint: print `claude mcp add` command post-install - #4 send config read: move mailbox resolution to daemon side - #2 SPF: plumb envelope MAIL FROM from smtp session through ingest - #5 wildcard send: remove wildcard branch from resolve_from_mailbox - #1 mailbox create: add restart hint (or route via daemon) - #3 plan wording: clarify "compose new" in docs/manual-test.md
* docs: add manual test results with findings Full execution of docs/manual-test.md against agent.zeroshot.lol. 10 findings recorded with severity and fix direction, notably: - P0 DKIM key on disk does not match DNS TXT (root cause of outbound dkim=fail at Gmail) - P0 Shell injection in on_receive cmd template expansion - P1 MCP write ops (email_mark_read, etc.) fail when MCP runs as non-root due to root:root 0644 mailbox files * docs: add Recommended fixes section with per-finding implementation plans Code-review-backed fix plans for each of the 10 findings, with file:line refs, effort estimates, and a priority order. No code changes yet — this consolidates the investigation so fixes can be sequenced. - #10 DKIM mismatch: not a code bug; DNS republish + optional startup check - #9 shell injection: pass trigger vars via env, not string substitution - #8 MCP writes: route state mutations through daemon UDS - #7 claude-code hint: print `claude mcp add` command post-install - #4 send config read: move mailbox resolution to daemon side - #2 SPF: plumb envelope MAIL FROM from smtp session through ingest - #5 wildcard send: remove wildcard branch from resolve_from_mailbox - #1 mailbox create: add restart hint (or route via daemon) - #3 plan wording: clarify "compose new" in docs/manual-test.md
Sprint 44 (post-launch security + quick fixes) addresses findings #9, #10, #7, #1-tier-1, #3. Sprint 45 (strict outbound + MCP writes via daemon addresses #4, #5, #8 and introduces UDS MARK-READ/MARK-UNREAD verbs. Sprint 46 (mailbox CRUD via UDS) addresses #1-tier-2 with MAILBOX-CREATE/MAILBOX-DELETE verbs so daemon picks up changes live. PRD FR-18d tightened: outbound send now requires a concrete non-wildcard mailbox under config.domain; catchall is inbound-only. PRD FR-18e added to cover the new state-mutation verbs on the UDS socket. Finding #2 (SPF envelope MAIL FROM) excluded — already shipped in f5cebd2. EOF )
- `aimx send` no longer reads /etc/aimx/config.toml; daemon parses From: from the submitted body and resolves the sender mailbox against its in-memory Config. - Outbound rejects both foreign-domain From (ERR DOMAIN) and any From whose local part does not match an explicitly configured non-wildcard mailbox (ERR MAILBOX). Catchall is inbound-only per FR-18d (tightened). - New AIMX/1 MARK-READ / MARK-UNREAD UDS verbs with typed codec, per- mailbox tokio RwLock in a StateContext, and client wiring in `aimx mcp` so email_mark_read / email_mark_unread work without root write access to mailbox files. - New error codes PROTOCOL / NOTFOUND / IO added; legacy From-Mailbox: header silently ignored for forward-compatibility. Closes findings #4, #5, #8 from the 2026-04-17 manual test run.
Summary
aimx mcp) usingrmcpcrate with stdio transport for AI agent email accessmailbox_create,mailbox_list,mailbox_delete,email_list(with unread/from/since/subject filters),email_read,email_mark_read,email_mark_unread,email_send(with DKIM signing),email_reply(with In-Reply-To/References threading)aimx mcpas a child process and communicate via JSON-RPC over stdioStories Implemented
S3.1 — MCP Transport + Mailbox Tools
aimx mcpstarts an MCP server in stdio modeinitializehandshake correctlymailbox_create(name)creates mailbox and returns confirmationmailbox_list()returns all mailboxes with message counts (total and unread)mailbox_delete(name)deletes mailbox (with appropriate safeguards)aimx mcp, send JSON-RPC requests, assert responsesS3.2 — Email Read + List Tools
email_list(mailbox)returns frontmatter of all emails in the mailboxemail_listsupports optional filters:unread,from,since,subjectemail_read(mailbox, id)returns full Markdown content of the emailemail_mark_read(mailbox, id)updates frontmatterread: trueemail_mark_unread(mailbox, id)updates frontmatterread: falseS3.3 — Email Send + Reply Tools
email_send(from_mailbox, to, subject, body, attachments?)composes, DKIM-signs, and sendsemail_reply(mailbox, id, body)replies with correct In-Reply-To/References headersTest plan
cargo clippy -- -D warningspasses with zero warningscargo fmt -- --checkpasses