security: fix path traversal, credential exposure, MCP confirmation replay, and reply-all preview#23
Closed
dylanbyars wants to merge 8 commits intoradiosilence:mainfrom
Closed
security: fix path traversal, credential exposure, MCP confirmation replay, and reply-all preview#23dylanbyars wants to merge 8 commits intoradiosilence:mainfrom
dylanbyars wants to merge 8 commits intoradiosilence:mainfrom
Conversation
The email sender controls attachment.name, but download passed it
straight into Path::new(out_dir).join(). Two escape paths:
attachment.name = "../../../etc/cron.d/pwn"
→ Path::join resolves relative traversal beneath out_dir
attachment.name = "/etc/cron.d/pwn"
→ Path::join silently replaces out_dir when the joined
segment is absolute
Any email sender could write arbitrary files when the recipient ran
`fastmail-cli download`.
Introduces util::sanitize_filename(raw, fallback) which:
- splits on both '/' and '\\' and keeps only the final segment
(covers Unix-style '../foo', absolute '/etc/foo', and
Windows-style 'C:\\evil\\foo' alike)
- strips NUL and other control characters
- trims leading/trailing dots and whitespace
- replaces Windows-reserved stems (CON, PRN, NUL, COM1-9,
LPT1-9) with the fallback, preventing cross-platform
footguns
- caps at 200 chars while preserving the extension
The download site now writes via OpenOptions::create_new(true),
which uses O_EXCL on Unix and CREATE_NEW on Windows. This both
refuses silent overwrite and closes a TOCTOU where an attacker
pre-creates a symlink at the target path.
Tests: 10 new cases covering each rejection path and the fallback.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
list_addressbooks() built its URL with format!("/dav/addressbooks/
user/{}/", self.username). The raw username came straight from
config or the FASTMAIL_USERNAME env var and went into the URL path
with no encoding.
For the typical case (emails like user@domain.tld) this works —
'@' and '.' are valid URL path characters. But if the username is
ever misconfigured to contain '/', '?', '#', or '%', the URL is
malformed and the request targets a different endpoint on the
CardDAV server.
Fix: add percent-encoding = "2" and run the username through
utf8_percent_encode with a PATH_SEGMENT set that escapes all
controls and segment delimiters.
Drive-by: the adjacent contacts.sort_by() was flagged by clippy's
unnecessary-sort-by lint on Rust 1.95. Rewritten as sort_by_key so
CI's `cargo clippy -- -D warnings` stays green.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Config::save() had two weaknesses: TOCTOU on permissions fs::write(&path, content)?; fs::set_permissions(&path, 0o600)?; Between the write and the chmod the file existed with the default umask mode — typically 0o644 — so another local user or process could read the token before perms were tightened. Same pattern applied to the parent directory. Symlink-follow fs::write also follows symlinks, so a hostile program with write access to the config dir could pre-create a symlink at config.toml pointing elsewhere and the token would be written through the link. Fix: write to a sibling config.toml.tmp opened via OpenOptions with .create_new(true) + .mode(0o600), then rename() atomically over the target. create_new uses O_EXCL on Unix and CREATE_NEW on Windows, so the write refuses to race with a pre-existing path (including a symlink). Before rename, symlink_metadata() is checked on the target — if it's already a symlink we refuse to overwrite it and error out loudly rather than redirect the token. Parent dir is now created with DirBuilder::mode(0o700) so the dir's mode is right from the first open(2) call; a follow-up set_permissions covers the case where the dir already existed with a wider mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously the `auth` subcommand took the token as a positional argument: fastmail-cli auth fmu1-your-token-here which exposed the token to: - `ps auxw` (visible to every local user) - shell history files (~/.zsh_history, ~/.bash_history) - the process environment inherited by child processes - any terminal-multiplexer log The token is now Option<String>. When omitted the command reads a single line from stdin (trimmed). On a TTY it prints a prompt to stderr; when stdin is piped (CI, scripts, paste into a here-string) it reads silently. # interactive — prompts on stderr, line read from tty fastmail-cli auth # script / piped — no prompt, just reads echo "$FASTMAIL_TOKEN" | fastmail-cli auth The positional form still works for backward-compatibility, but the help text steers users toward stdin. README/CHANGELOG updated in a follow-up commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two small hardenings in the JMAP client:
URL template substitution (M1)
download_blob/upload_blob built URLs by chaining str::replace:
.replace("{accountId}", account_id)
.replace("{blobId}", blob_id)
.replace("{name}", "attachment")
.replace("{type}", "application/octet-stream")
If any substituted value ever contained another placeholder —
e.g. a blob_id literally spelled "{name}" — it would bleed into
the next step and produce a surprising URL. Today all template
inputs come from trusted sources (Fastmail session data or
hardcoded strings), so this is defense-in-depth rather than a
live bug. But it's the kind of bug that's painful to notice if
the trust boundary ever shifts.
Replaced with apply_url_template(tmpl, &[(key, value), ...]),
a single-pass helper that never re-scans substituted content.
Unknown placeholders are preserved literally. 5 unit tests
cover: basic substitution, no-cascade, unknown placeholders,
no-op, unterminated braces.
InvalidToken variant (M4)
Error::InvalidToken(String) invited a future contributor to
stuff the token itself into the variant for "better debug
output" — which would then show up in tracing output and the
Output::error JSON.
Narrowed to InvalidToken(&'static str) so construction sites
can only pass compile-time literals. All four existing
callsites already used literals, so this is a type-level
constraint that prevents regression, not a behaviour change.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The MCP sendEmail / replyToEmail / forwardEmail mutations enforce a
PREVIEW -> CONFIRM flow to stop an LLM from sending without showing
the user what it's about to send. That confirmation was a
DefaultHasher-based hash of the compose params:
token = hash(to, subject, body)
which is a *signature*, not a *nonce*: calling CONFIRM with the
same params always produces the same token, so the "confirmation"
check collapsed to "does the caller know the params" — trivially
satisfied by anyone calling the mutation at all.
The check it was meant to enforce was "the caller previously called
PREVIEW for these exact params". That requires server-side state
tied to the PREVIEW call.
Introduce NonceStore (tokio::sync::Mutex<HashMap<String, String>>)
injected into the GraphQL schema as context data:
PREVIEW
- generate a uuid::Uuid::new_v4() as the nonce
- store nonce -> fingerprint(params) in NonceStore
- return the nonce as confirmation_token
CONFIRM / DRAFT
- look up and REMOVE the nonce from NonceStore (one-shot)
- compare stored fingerprint vs fingerprint(submitted params)
- reject on missing nonce (PREVIEW not called, or re-used)
- reject on fingerprint mismatch (params tampered between
PREVIEW and CONFIRM, which a prompt-injected agent might
try)
This still isn't a proof that a human approved — an LLM can call
PREVIEW and immediately CONFIRM without human intervention — but
it is now a genuine state-transition check rather than a
tautology, and it makes param-tampering attacks observable.
Adds uuid = "1" with the "v4" feature as a direct dep. Cargo.lock
was already pulling uuid in transitively via kreuzberg -> cfb, so
no new crate enters the build, only a new direct reference and the
v4 feature.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- README: expand the Authentication section to cover the new stdin flow (interactive prompt + pipe form), keep the positional form documented for backward compat, and mention the atomic-write / symlink-refuse behaviour so users aren't surprised. - CHANGELOG: new [Unreleased] block with a Security subsection listing each finding (C1, C2, H1, H2, H3, M1, M3, M4) in the same voice as prior entries, plus a Changed note that `auth` arg is now Option<String>. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The MCP replyToEmail mutation's PREVIEW path built recipients as: let to_addrs: Vec<EmailAddress> = original.from.clone().unwrap_or_default(); and used the raw user-supplied `cc` unchanged, never consulting reply_all. Calling PREVIEW with `all: true` would therefore display only the original sender in To and whatever the user explicitly passed as CC — not what the send path would actually produce. The send path in JmapClient::reply_email correctly expanded reply-all, so PREVIEW and CONFIRM diverged for the same arguments. Two knock-on effects: 1. Preview lies — the user cannot see the true recipient list before confirming a reply-all. 2. Duplicate-send — if a user tried to work around (1) by passing the missing recipients as explicit `cc`, those addresses would *also* be expanded into To by the send path at CONFIRM time, producing duplicate delivery. Observed live: original email `To: [Sher, Dylan, Anne, Leon]`, reply-all preview showed `To: Paul, CC: (none)`, "fixing" by adding Sher/Anne/Leon via cc would have sent `To: Paul,Sher,Anne,Leon` AND `Cc: Sher,Anne,Leon`. Fix: extracted `jmap::expand_reply_recipients` as a pure function used by both the preview path (via a new `resolve_my_email` helper on JmapClient so preview can filter out the sending identity without re-implementing the lookup) and the send path in reply_email. The helper also deduplicates by lowercase email and strips from CC anything already in To — closing the duplicate-send window regardless of how the two paths evolve. Tests: 9 new unit tests covering plain reply (no expansion), reply-all basic expansion, me-filtering (case-insensitive), dedup of overlapping user CC and reply-all-expanded To (the exact bug report scenario), dedup of duplicates in original recipients, preview-without-identity falling back to dedup-only, and recipient ordering preservation. 78 total passing, fmt/clippy clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
radiosilence
added a commit
that referenced
this pull request
Apr 18, 2026
Security patch release bundling the v2.1.0 audit fixes (PR #23) from @dylanbyars on top of v2.2.0 contacts CRUD. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
Owner
|
Rebased onto main (v2.2.0 contacts CRUD landed in the meantime) and bumped to v2.2.1 in #24 — all 8 original commits preserved with authorship intact. Closing this PR in favour of #24; credit and CHANGELOG note you as the audit author. Thanks @dylanbyars — really solid work. |
radiosilence
added a commit
that referenced
this pull request
Apr 18, 2026
Security patch release bundling the v2.1.0 audit fixes (PR #23) from @dylanbyars on top of v2.2.0 contacts CRUD. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
radiosilence
added a commit
that referenced
this pull request
Apr 18, 2026
…2.1 (#24) * fix: sanitize attachment filenames to prevent path traversal The email sender controls attachment.name, but download passed it straight into Path::new(out_dir).join(). Two escape paths: attachment.name = "../../../etc/cron.d/pwn" → Path::join resolves relative traversal beneath out_dir attachment.name = "/etc/cron.d/pwn" → Path::join silently replaces out_dir when the joined segment is absolute Any email sender could write arbitrary files when the recipient ran `fastmail-cli download`. Introduces util::sanitize_filename(raw, fallback) which: - splits on both '/' and '\\' and keeps only the final segment (covers Unix-style '../foo', absolute '/etc/foo', and Windows-style 'C:\\evil\\foo' alike) - strips NUL and other control characters - trims leading/trailing dots and whitespace - replaces Windows-reserved stems (CON, PRN, NUL, COM1-9, LPT1-9) with the fallback, preventing cross-platform footguns - caps at 200 chars while preserving the extension The download site now writes via OpenOptions::create_new(true), which uses O_EXCL on Unix and CREATE_NEW on Windows. This both refuses silent overwrite and closes a TOCTOU where an attacker pre-creates a symlink at the target path. Tests: 10 new cases covering each rejection path and the fallback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: URL-encode CardDAV username in addressbook paths list_addressbooks() built its URL with format!("/dav/addressbooks/ user/{}/", self.username). The raw username came straight from config or the FASTMAIL_USERNAME env var and went into the URL path with no encoding. For the typical case (emails like user@domain.tld) this works — '@' and '.' are valid URL path characters. But if the username is ever misconfigured to contain '/', '?', '#', or '%', the URL is malformed and the request targets a different endpoint on the CardDAV server. Fix: add percent-encoding = "2" and run the username through utf8_percent_encode with a PATH_SEGMENT set that escapes all controls and segment delimiters. Drive-by: the adjacent contacts.sort_by() was flagged by clippy's unnecessary-sort-by lint on Rust 1.95. Rewritten as sort_by_key so CI's `cargo clippy -- -D warnings` stays green. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: atomic 0o600 token file write with symlink guard Config::save() had two weaknesses: TOCTOU on permissions fs::write(&path, content)?; fs::set_permissions(&path, 0o600)?; Between the write and the chmod the file existed with the default umask mode — typically 0o644 — so another local user or process could read the token before perms were tightened. Same pattern applied to the parent directory. Symlink-follow fs::write also follows symlinks, so a hostile program with write access to the config dir could pre-create a symlink at config.toml pointing elsewhere and the token would be written through the link. Fix: write to a sibling config.toml.tmp opened via OpenOptions with .create_new(true) + .mode(0o600), then rename() atomically over the target. create_new uses O_EXCL on Unix and CREATE_NEW on Windows, so the write refuses to race with a pre-existing path (including a symlink). Before rename, symlink_metadata() is checked on the target — if it's already a symlink we refuse to overwrite it and error out loudly rather than redirect the token. Parent dir is now created with DirBuilder::mode(0o700) so the dir's mode is right from the first open(2) call; a follow-up set_permissions covers the case where the dir already existed with a wider mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: read API token from stdin when `auth` is called without one Previously the `auth` subcommand took the token as a positional argument: fastmail-cli auth fmu1-your-token-here which exposed the token to: - `ps auxw` (visible to every local user) - shell history files (~/.zsh_history, ~/.bash_history) - the process environment inherited by child processes - any terminal-multiplexer log The token is now Option<String>. When omitted the command reads a single line from stdin (trimmed). On a TTY it prints a prompt to stderr; when stdin is piped (CI, scripts, paste into a here-string) it reads silently. # interactive — prompts on stderr, line read from tty fastmail-cli auth # script / piped — no prompt, just reads echo "$FASTMAIL_TOKEN" | fastmail-cli auth The positional form still works for backward-compatibility, but the help text steers users toward stdin. README/CHANGELOG updated in a follow-up commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: single-pass URL template + tighten InvalidToken variant Two small hardenings in the JMAP client: URL template substitution (M1) download_blob/upload_blob built URLs by chaining str::replace: .replace("{accountId}", account_id) .replace("{blobId}", blob_id) .replace("{name}", "attachment") .replace("{type}", "application/octet-stream") If any substituted value ever contained another placeholder — e.g. a blob_id literally spelled "{name}" — it would bleed into the next step and produce a surprising URL. Today all template inputs come from trusted sources (Fastmail session data or hardcoded strings), so this is defense-in-depth rather than a live bug. But it's the kind of bug that's painful to notice if the trust boundary ever shifts. Replaced with apply_url_template(tmpl, &[(key, value), ...]), a single-pass helper that never re-scans substituted content. Unknown placeholders are preserved literally. 5 unit tests cover: basic substitution, no-cascade, unknown placeholders, no-op, unterminated braces. InvalidToken variant (M4) Error::InvalidToken(String) invited a future contributor to stuff the token itself into the variant for "better debug output" — which would then show up in tracing output and the Output::error JSON. Narrowed to InvalidToken(&'static str) so construction sites can only pass compile-time literals. All four existing callsites already used literals, so this is a type-level constraint that prevents regression, not a behaviour change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: use random one-shot nonces for MCP compose confirmation The MCP sendEmail / replyToEmail / forwardEmail mutations enforce a PREVIEW -> CONFIRM flow to stop an LLM from sending without showing the user what it's about to send. That confirmation was a DefaultHasher-based hash of the compose params: token = hash(to, subject, body) which is a *signature*, not a *nonce*: calling CONFIRM with the same params always produces the same token, so the "confirmation" check collapsed to "does the caller know the params" — trivially satisfied by anyone calling the mutation at all. The check it was meant to enforce was "the caller previously called PREVIEW for these exact params". That requires server-side state tied to the PREVIEW call. Introduce NonceStore (tokio::sync::Mutex<HashMap<String, String>>) injected into the GraphQL schema as context data: PREVIEW - generate a uuid::Uuid::new_v4() as the nonce - store nonce -> fingerprint(params) in NonceStore - return the nonce as confirmation_token CONFIRM / DRAFT - look up and REMOVE the nonce from NonceStore (one-shot) - compare stored fingerprint vs fingerprint(submitted params) - reject on missing nonce (PREVIEW not called, or re-used) - reject on fingerprint mismatch (params tampered between PREVIEW and CONFIRM, which a prompt-injected agent might try) This still isn't a proof that a human approved — an LLM can call PREVIEW and immediately CONFIRM without human intervention — but it is now a genuine state-transition check rather than a tautology, and it makes param-tampering attacks observable. Adds uuid = "1" with the "v4" feature as a direct dep. Cargo.lock was already pulling uuid in transitively via kreuzberg -> cfb, so no new crate enters the build, only a new direct reference and the v4 feature. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: note stdin auth in README and log security fixes in CHANGELOG - README: expand the Authentication section to cover the new stdin flow (interactive prompt + pipe form), keep the positional form documented for backward compat, and mention the atomic-write / symlink-refuse behaviour so users aren't surprised. - CHANGELOG: new [Unreleased] block with a Security subsection listing each finding (C1, C2, H1, H2, H3, M1, M3, M4) in the same voice as prior entries, plus a Changed note that `auth` arg is now Option<String>. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: reply-all preview divergence + duplicate-recipient risk The MCP replyToEmail mutation's PREVIEW path built recipients as: let to_addrs: Vec<EmailAddress> = original.from.clone().unwrap_or_default(); and used the raw user-supplied `cc` unchanged, never consulting reply_all. Calling PREVIEW with `all: true` would therefore display only the original sender in To and whatever the user explicitly passed as CC — not what the send path would actually produce. The send path in JmapClient::reply_email correctly expanded reply-all, so PREVIEW and CONFIRM diverged for the same arguments. Two knock-on effects: 1. Preview lies — the user cannot see the true recipient list before confirming a reply-all. 2. Duplicate-send — if a user tried to work around (1) by passing the missing recipients as explicit `cc`, those addresses would *also* be expanded into To by the send path at CONFIRM time, producing duplicate delivery. Observed live: original email `To: [Sher, Dylan, Anne, Leon]`, reply-all preview showed `To: Paul, CC: (none)`, "fixing" by adding Sher/Anne/Leon via cc would have sent `To: Paul,Sher,Anne,Leon` AND `Cc: Sher,Anne,Leon`. Fix: extracted `jmap::expand_reply_recipients` as a pure function used by both the preview path (via a new `resolve_my_email` helper on JmapClient so preview can filter out the sending identity without re-implementing the lookup) and the send path in reply_email. The helper also deduplicates by lowercase email and strips from CC anything already in To — closing the duplicate-send window regardless of how the two paths evolve. Tests: 9 new unit tests covering plain reply (no expansion), reply-all basic expansion, me-filtering (case-insensitive), dedup of overlapping user CC and reply-all-expanded To (the exact bug report scenario), dedup of duplicates in original recipients, preview-without-identity falling back to dedup-only, and recipient ordering preservation. 78 total passing, fmt/clippy clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: bump version to 2.2.1 Security patch release bundling the v2.1.0 audit fixes (PR #23) from @dylanbyars on top of v2.2.0 contacts CRUD. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Dylan Byars <dylanbyars@oncolens.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Thanks! Happy to contribute. Loving this project so far and look forward to more improvements |
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
3 participants
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.
Summary
Security audit against
v2.1.0surfaced nine issues across download, config, auth, JMAP, CardDAV, and MCP. While testing the fixes end-to-end, I also hit an exploitable logic bug in the MCP reply-all preview path that caused duplicate email delivery under a plausible user workaround. Everything is addressed here, one commit per finding (or paired where fixes share surface area) so each is individually reviewable and revertable.Two findings are exploitable in the wild:
fastmail-cli downloadI'd suggest cutting a point release once this lands.
Findings
commands/download.rsattachment.name; write viaOpenOptions::create_new(true)(O_EXCL)carddav/mod.rs/dav/addressbooks/user/{}/config.rsmode(0o600)config.rscommands/auth.rs,main.rsjmap/mod.rsutil.rsmcp/graphql/*error.rsInvalidTokennow holds&'static strmcp/graphql/mutation.rs,jmap/mod.rsexpand_reply_recipients; preview + send share one computed list; helper also dedupesFull per-issue writeups are in
CHANGELOG.md.Backward compatibility
fastmail-cli auth YOUR_TOKENstill works (positional arg nowOption<String>)JmapClient::reply_emailsignature changed:reply_all: bool→to: Vec<EmailAddress>. Both in-repo callers updated. Internal Rust API only — no user-visible CLI or MCP change.percent-encoding = "2";uuidalready transitive, just enablesv4feature.Test plan
cargo fmt -- --checkcleancargo clippy -- -D warningscleancargo test— 78 passing (up from 59 onmain)Suggested follow-up (not in this PR)
src/jmap/mod.rsis now 1989 lines, an outlier vs the rest of the repo. Natural split:client.rs/mail.rs/compose.rs/blob.rs/masked.rs. Kept out of this PR to keep the security changes reviewable.🤖 Generated with Claude Code