security: rebase @dylanbyars security audit onto v2.2.0 + bump to v2.2.1#24
Merged
radiosilence merged 9 commits intomainfrom Apr 18, 2026
Merged
security: rebase @dylanbyars security audit onto v2.2.0 + bump to v2.2.1#24radiosilence merged 9 commits intomainfrom
radiosilence merged 9 commits intomainfrom
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>
This was referenced 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
force-pushed
the
security-audit-v2.2.1
branch
from
264511e to
eca6986
Compare
radiosilence
added a commit
that referenced
this pull request
Apr 18, 2026
The NonceStore added in #24 was an unbounded HashMap. A misbehaving MCP client that PREVIEW'd repeatedly without ever CONFIRM'ing could grow it without limit for the life of the process. Not exploitable (local, trusted caller) but not tidy either. Now bounded: - 15-minute TTL per nonce, swept on every issue and rejected on consume - Hard cap of 256 outstanding nonces, oldest evicted when full - Seven tests: roundtrip, missing, reuse, tampered params, expired, cap eviction, expired sweep Entries now carry an `issued_at: Instant` alongside the fingerprint; `Nonce` became a struct so the map type reflects what it holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
radiosilence
added a commit
that referenced
this pull request
Apr 18, 2026
The NonceStore added in #24 was an unbounded HashMap. A misbehaving MCP client that PREVIEW'd repeatedly without ever CONFIRM'ing could grow it without limit for the life of the process. Not exploitable (local, trusted caller) but not tidy either. Now bounded: - 15-minute TTL per nonce, swept on every issue and rejected on consume - Hard cap of 256 outstanding nonces, oldest evicted when full - Seven tests: roundtrip, missing, reuse, tampered params, expired, cap eviction, expired sweep Entries now carry an `issued_at: Instant` alongside the fingerprint; `Nonce` became a struct so the map type reflects what it holds. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 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
Rebase of PR #23 from @dylanbyars onto main after v2.2.0 (contacts CRUD) landed. All 9 findings + B1 reply-all bug preserved; CHANGELOG bumped to
[2.2.1], Cargo.toml bumped to2.2.1.Why rebase (not merge-as-is)
PR #23 was based on v2.1.0 — after contacts CRUD merged, it hit trivial conflicts in
CHANGELOG.mdandCargo.toml. Same eight commits from @dylanbyars, clean rebase onto13f9ff7, plus one version-bump commit.Findings covered
commands/download.rs,util::sanitize_filename)InvalidToken(&'static str)Also picks up transitive
rustls-webpki0.103.10 → 0.103.12, killing RUSTSEC-2026-0098 and RUSTSEC-2026-0099.Local verification
cargo test— 82 passing (78 from PR security: fix path traversal, credential exposure, MCP confirmation replay, and reply-all preview #23 + 4 contacts CRUD)cargo clippy --all-targets -- -D warnings— cleancargo fmt -- --check— cleancargo audit— 0 vulns, 1 warning remaining (pasteunmaintained, transitive viarav1e→image, unavoidable)Follow-up
Open an issue to cap / TTL the
NonceStoreinmcp/graphql/types.rs— unbounded right now. Not exploitable (local MCP process, trusted caller) but worth tidying.Credit
Audit and fixes by @dylanbyars — see closed PR #23 for the original review. Rebase by the maintainer.
Test plan
🤖 Generated with Claude Code