security: download-path confinement, CRLF rejection, resource limits, HTTP-transport warning - #201
Closed
lucagattoni wants to merge 6 commits into
Closed
Conversation
download_attachment wrote attacker-controlled bytes to an arbitrary save_path (Path(save_path).write_bytes with parent mkdir -p), gated only by the enable_attachment_download flag — a prompt-injected agent could overwrite ~/.ssh/authorized_keys, crontabs, or the config file once the feature was enabled. Add attachment_download_dir (env: MCP_EMAIL_SERVER_ATTACHMENT_DOWNLOAD_DIR, default ~/Downloads). save_path is now resolved and must stay within that root; the parent is canonicalised with realpath first, so a symlink component pointing outside the root is rejected too. Relative paths resolve under the root. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GHu48yxodNvMb8vsuU5NSu
Header injection was already blocked by the stdlib email serializer, but bcc values go straight to the SMTP envelope (aiosmtplib writes RCPT TO raw, no CRLF check — verified against 4.0.2), so a newline in a recipient could inject extra SMTP commands/recipients. Reject CR/LF at the tool boundary for recipients/cc/bcc, the reply/threading header values, and attachment paths, in both send_email and save_to_mailbox, before dispatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GHu48yxodNvMb8vsuU5NSu
- list_emails_metadata: page >= 1, page_size in [1, 500] (was unbounded) - get_emails_content: email_ids capped at 1..100 per call (was unbounded) - attachments: 25 MiB cap per file on both send (stat before read) and download (checked before write), preventing multi-GB reads into memory Constraints are enforced by the FastMCP validation layer and advertised in each tool's input schema. Extracted the attachment-search loop from download_attachment into a helper to keep it under the complexity limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GHu48yxodNvMb8vsuU5NSu
Final adversarial review found a real bypass of the download_attachment confinement: _resolve_download_path canonicalised only the parent, so a symlink planted AT the target path (final component) inside the download root redirected the write outside it (verified PoC wrote bytes to a sibling dir). The existing symlink test only covered an intermediate component, giving false confidence. - Canonicalise the FULL candidate with realpath so a final-component symlink is resolved and caught; reject a path that names the root dir or has no filename (avoids a later IsADirectoryError). - Handler write now uses O_NOFOLLOW + owner-only mode, refusing a symlink swapped into the final component after the check (TOCTOU defence). - Correct the MAX_ATTACHMENT_BYTES docstring: on download it bounds the disk write, not the in-memory fetch. - Add an aggregate outbound cap (count + total bytes, stat'd before read). - CRLF-check subject too (clean error instead of relying on the stdlib HeaderParseError). Regression tests added for the final-component symlink escape, empty/dir save_path, subject CRLF, and the aggregate caps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GHu48yxodNvMb8vsuU5NSu
Ports the safety-hardening doc changes (originally authored against docs/installation.md, which lands in the permission-scopes PR) onto the current README structure: a prominent unauthenticated-gateway warning on the HTTP transport section, the 0.0.0.0 Compose caution, the download-path confinement note, and the MCP_EMAIL_SERVER_ATTACHMENT_DOWNLOAD_DIR row. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pd8meRq4jSiqaLq715nSD
…lution Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pd8meRq4jSiqaLq715nSD
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Jul 20, 2026
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this delivers
Five security-hardening fixes to the email tools, plus the docs that describe them. Independent of the permission-scopes PR (#200) at the code level — branched off current
main.Fixes
download_attachmentpath confinement (dd47c6e) — save paths are confined toattachment_download_dir(default~/Downloads). Asave_pathescaping it via..or an absolute path is rejected, so attacker-controlled attachment bytes can't be written over~/.ssh/authorized_keys, a crontab, or the config file. Newattachment_download_dirsetting +MCP_EMAIL_SERVER_ATTACHMENT_DOWNLOAD_DIR.e5fd74e) — reject CR/LF in recipients, header values, and attachment paths. Closes SMTP-envelope injection viabcc(passed raw to aiosmtplib, never header-serialized).e718855) — prominent docs warning thatsse/streamable-httphave no auth;Host/Originvalidation is anti-DNS-rebinding, not access control. A non-loopback bind publishes an unauthenticated email gateway.40efcd1) —MAX_PAGE_SIZE=500,MAX_EMAIL_IDS_PER_CALL=100, and attachment-size bounds guard against self-DoS from unbounded tool inputs.3d0a70a) —_resolve_download_pathnowrealpaths the full candidate (final component included), so a symlink planted at the target path can't redirect the write outside root. Plus adversarial-review gap closures.Test plan
uv run pytest -q— 559 passed (new:test_download_path_confinement.py,test_crlf_injection.py,test_resource_limits.py)uv run ruff check/ruff format --check— cleanprettier --check README.md— cleanNote on overlap with #200
The safety code is fully independent of #200. The docs overlap: #200 moves the HTTP-transport section from README into
docs/installation.md, while this PR edits that section in README (its current home). Whichever merges second will need a trivial docs rebase — the code merges cleanly either way.Opened as draft for review of the reconciliation (permission-scope enforcement calls were dropped from the cherry-picks since they belong to #200).
🤖 Generated with Claude Code
https://claude.ai/code/session_013pd8meRq4jSiqaLq715nSD