Skip to content

security: download-path confinement, CRLF rejection, resource limits - #210

Closed
lucagattoni wants to merge 4 commits into
Wh1isper:mainfrom
lucagattoni:20260720_1944-safety-hardening-code
Closed

security: download-path confinement, CRLF rejection, resource limits#210
lucagattoni wants to merge 4 commits into
Wh1isper:mainfrom
lucagattoni:20260720_1944-safety-hardening-code

Conversation

@lucagattoni

Copy link
Copy Markdown
Contributor

What this delivers

Three security-hardening fixes to the email tools, rebased cleanly onto current main and with docs harmonized into the new multi-file docs/ structure (post-#205).

This supersedes #201, which was branched a week ago and now conflicts with the docs reorganization (#205) and the pyright/tooling changes (#204). Same fixes, cleanly re-homed.

Fixes

  1. download_attachment path confinement — save paths are confined to attachment_download_dir (default ~/Downloads). A save_path escaping it via .. or a symlink (including a final-component symlink) is rejected, so attacker-controlled attachment bytes can't be written over ~/.ssh/authorized_keys, a crontab, or the config file. New attachment_download_dir setting + MCP_EMAIL_SERVER_ATTACHMENT_DOWNLOAD_DIR.
  2. CRLF injection rejection — reject CR/LF in recipients, header values, and attachment paths. Closes SMTP-envelope injection via bcc (handed to aiosmtplib without being placed in a header).
  3. Resource-exhaustion ceilingspage_size ≤ 500, ≤ 100 email_ids per call, and a 25 MB attachment cap guard against self-DoS from unbounded tool inputs.

Rebase / integration notes

  • Adopted upstream's robust _decoded_payload in the extracted _find_attachment_in_message helper (upstream added that decoder after this branch was first written).
  • Typed _reject_crlf as Iterable[str | None] to satisfy the pyright check added in chore: add pyright checks and update Python tooling #204.
  • Docs distributed the idiomatic way: security.md (attachment access rewrite, HTTP-transport = not-auth, new "Message and input limits"), transports.md (unauthenticated-gateway warning under DNS-rebinding), configuration.md (setting + env rows), and corrected now-stale download_attachment guidance in tools.md/troubleshooting.md.
  • E2E: download_attachment confinement required pointing MCP_EMAIL_SERVER_ATTACHMENT_DOWNLOAD_DIR at the GreenMail test's tmp_path, or the roundtrip download is rejected.

Test plan

  • uv run pytest -q578 passed (new: test_crlf_injection.py, test_download_path_confinement.py, test_resource_limits.py)
  • uv run ruff check / ruff format --check — clean
  • uv run pyright0 errors (chore: add pyright checks and update Python tooling #204's check)
  • mkdocs build --strict — builds, no broken anchors
  • prettier --check docs/*.md — clean

Companion to #209 (permission model). The two are independent at the code level; when both land, whichever merges second needs a small app.py rebase where they both touch send_email/save_to_mailbox.

🤖 Generated with Claude Code

https://claude.ai/code/session_013pd8meRq4jSiqaLq715nSD

lucagattoni and others added 3 commits July 20, 2026 19:51
… limits (code only)

Code-only slice of PR Wh1isper#201, rebased onto current main (post-Wh1isper#204/Wh1isper#205).
- download_attachment confines save_path within attachment_download_dir
  (default ~/Downloads); paths escaping it (incl. .. or symlinks) are rejected
- reject CR/LF in recipients, header values, and attachment paths (SMTP-envelope
  injection, notably via bcc)
- resource-exhaustion ceilings on page_size, email_ids count, attachment size

Rebase notes: adopt upstream's _decoded_payload in the extracted
_find_attachment_in_message helper; type _reject_crlf as Iterable[str | None]
for the pyright check added in Wh1isper#204. Docs are re-homed separately.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pd8meRq4jSiqaLq715nSD
download_attachment now confines save paths to attachment_download_dir
(default ~/Downloads). The GreenMail E2E downloads into pytest's tmp_path, so
set MCP_EMAIL_SERVER_ATTACHMENT_DOWNLOAD_DIR to that dir or the roundtrip
download is rejected.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pd8meRq4jSiqaLq715nSD
…r#205 structure)

- security.md: rewrite 'Attachment access' for the new save-path confinement;
  sharpen 'HTTP transport security' (Host/Origin != auth); add 'Message and
  input limits' (CRLF rejection, page/id/attachment ceilings)
- configuration.md: attachment_download_dir setting + env rows
- transports.md: unauthenticated-gateway warning in 'DNS rebinding protection'
- tools.md / troubleshooting.md: correct now-stale download_attachment
  'resolves against working directory' guidance

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pd8meRq4jSiqaLq715nSD
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.7%. Comparing base (81dd623) to head (e39692f).

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #210     +/-   ##
=======================================
+ Coverage   92.5%   92.7%   +0.1%     
=======================================
  Files         10      10             
  Lines       2013    2074     +61     
  Branches     310     320     +10     
=======================================
+ Hits        1863    1923     +60     
- Misses       107     109      +2     
+ Partials      43      42      -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Close the codecov patch gap opened by extracting _find_attachment_in_message:
unit-test the helper (found / not-found / no-filename / non-multipart) and the
download_attachment attachment-size rejection, which were otherwise exercised
only by the (uncounted) GreenMail E2E.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pd8meRq4jSiqaLq715nSD
@Wh1isper

Copy link
Copy Markdown
Owner

Thank you for putting together this security hardening work and for documenting the threat model so clearly.

Since this PR was opened, #212 has landed a substantial architecture rewrite that addresses these concerns through the new application boundaries:

  • shared application validation now rejects C0/DEL control characters, including CR and LF, before provider access;
  • request, provider-work, attachment, aggregate, and serialized-result limits are centrally owned and enforced;
  • attachment writes now require explicit current policy, fresh authority checks, bounded payloads, no-follow path traversal, owner/permission checks, and race-resistant atomic writes.

The attachment path contract was also intentionally resolved differently from this PR: compatibility preserves the caller's exact destination rather than confining it under a configured download root, with the filesystem defenses above providing the security boundary.

As a result, the original issues are either covered by #212 or superseded by the accepted exact-destination design, and this branch now conflicts with the rewritten architecture. I’m closing this PR as superseded. Thank you again for the careful contribution; it helped highlight the boundaries that needed explicit treatment.

@Wh1isper Wh1isper closed this Jul 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants