fix(email): quoted display-name brackets can override the real sender address#151
Open
ygd58 wants to merge 1 commit into
Open
fix(email): quoted display-name brackets can override the real sender address#151ygd58 wants to merge 1 commit into
ygd58 wants to merge 1 commit into
Conversation
…sender address
extract_sender_address() picked the address from the *last* <...> pair
found anywhere in the From header. If a quoted display-name that follows
the real address happens to contain its own bracketed text, e.g.:
<alice@trusted.org> "Bob <fake@evil.com>"
the function returned the decoy fake@evil.com instead of the real sender
alice@trusted.org, because rfind('<')/rfind('>') don't know about quoting.
Since EmailPolicy::sender_visible() drives allowlist/denylist filtering
directly off this value, a crafted display-name could cause a message to
be mis-classified relative to the actual sender.
Replaced the naive last-bracket search with extract_bracketed_address(),
a single left-to-right scan that tracks whether it's inside a double-quoted
quoted-string (respecting backslash-escaped quotes) and only treats
unquoted '<'/'>' as address delimiters. This correctly resolves the real
address regardless of whether a bracketed decoy appears before or after it
in the header.
Added regression tests for: decoy after the real address, decoy before it
(previously-working direction, kept as a guard), and an escaped-quote
edge case inside the display-name.
Note on verification: this sandbox only has rustc 1.75 available via apt,
and the crate needs edition 2024 (~1.85+), so I couldn't run the full
'cargo test' suite here. I extracted the new function verbatim into a
standalone file and compiled+ran it with rustc directly against all 5
test cases (3 old passing behaviors + 2 new ones) -- all passed. Please
run 'cargo test --lib email' locally to confirm before merging.
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.
Bug:
extract_sender_address()insrc/email.rspicks the address from the last<...>pair anywhere in theFromheader (rfind('<')/rfind('>')). If a quoted display-name that happens to follow the real address contains its own bracketed text, e.g.:...the function returns the decoy
fake@evil.cominstead of the actual senderalice@trusted.org, because the last-bracket search has no concept of quoting. SinceEmailPolicy::sender_visible()drives allowlist/denylist filtering directly off this return value, a crafted display-name can cause a message to be mis-classified relative to who actually sent it.Fix: replaced the naive last-bracket search with
extract_bracketed_address()— a single left-to-right scan that tracks whether it's inside a double-quoted quoted-string (respecting backslash-escaped quotes, e.g.\") and only treats unquoted</>as address delimiters. This resolves the real address correctly regardless of whether a bracketed decoy appears before or after it.Tests added (all passing,
cargo test email::— 15 passed / 0 failed):\"doesn't prematurely flip the in-quotes state and expose a decoy bracketVerification note: I drafted this with an AI pair-programming assistant (Claude) working from a sandbox without a matching Rust toolchain for this crate's edition, so the function was first verified standalone with plain
rustcagainst all 5 cases, and then I pulled the branch and ran the actual crate's test suite locally (cargo test email::) before opening this PR — output above.No behavior change for the common case (single unquoted
<addr>pair) or for headers with no brackets at all (unchanged fallback path).