Repository navigation
Fix/security review 2026 09 - #296
Merged
Merged
Conversation
… text With SocketType.starttls or plain the IMAP and SMTP connect paths used to authenticate over the unencrypted connection whenever the server did not advertise STARTTLS, so a MITM only had to strip the capability to obtain the credentials. Both paths now issue CAPABILITY/EHLO first, refuse to authenticate unless plainNoStartTls was chosen explicitly, and discard the pre-TLS capabilities after the upgrade (RFC 3501 6.2.1, RFC 3207 4.2). The TLS upgrade now also verifies the certificate against the configured host name. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The DATA payload was only escaped with a non-overlapping replaceAll of CRLF.CRLF, so two consecutive lines consisting of a single period ended the DATA phase early and turned the rest of the message into SMTP commands on the authenticated session. Lines like '.NET' also lost their leading period. Every line starting with a period is now dot-stuffed and the end-of-data marker is appended exactly once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Header values are now sanitised of CR/LF in the Header constructor, so a subject, display name, file name or custom header can no longer start another header or terminate the header section. MailAddress.encode() renders display names as RFC 5322 quoted-strings with quoted-pairs (the Q encoder now also encodes '"' and '\'), and refuses email addresses containing control characters, white space, angle brackets or list separators. The SMTP DATA and BDAT commands validate every MAIL FROM / RCPT TO address with the same rule instead of interpolating it verbatim, and a missing sender is an error instead of 'MAIL FROM:<null>'. mailto links drop addresses that fail this check. The address parser honours quoted-pairs and indexes by UTF-16 code unit, so an escaped quote can neither hide an extra recipient nor crash on emoji. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…er section The folded-Bcc fix from #294 only covered the DATA command; the BDAT command, which MailClient prefers whenever a server announces CHUNKING, still matched a single physical line and leaked every folded Bcc recipient to all other recipients. Both paths now share removeBccHeader, which matches the header name case-insensitively and never touches the body, so a body line starting with 'Bcc:' is no longer removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Both the text and the binary multipart splitter matched '--boundary' anywhere, so a text line containing it introduced a part that other clients and scanners do not see (RFC 2046 5.1.1 requires the preceding CRLF). Delimiters now need a line start, tolerate transport padding and reject boundaries that are only a prefix of the real one. The binary splitter no longer hides small trailing parts or drops the last part when the closing delimiter is missing, message/rfc822 parts without a header separator no longer throw, and nesting is capped at 50 levels with children skipping the redundant line-ending normalisation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tokens MailAccount, MailServerConfig and OauthToken rendered their full JSON, so any log line or exception message that interpolated one of them contained the password or the OAuth access and refresh tokens. toString() now redacts those values; toJson() is unchanged for persistence. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…andling HttpHelper closes its client, bounds header wait and body download with the timeout, caps the body at 1 MiB, fails instead of hanging on stream errors and no longer follows https-to-http redirects. The email address is query-encoded before it is sent to autoconfig.<domain>, a result obtained over the plain http fallback is flagged via ClientConfig.isFromInsecureSource, forceSslConnection now also updates the generic preferred servers the account is actually built from, UsernameType.emailLocalPart returns the local part instead of the domain, MX host parsing no longer depends on a trailing dot and the probe sockets opened during discovery are destroyed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…connection
A parser exception used to escape the socket callback, leaving the
pending command incomplete and every later command queued forever.
Tagged-response parsing now fails the command with an ImapException,
untagged parse errors are logged and skipped, and errors of the response
reader itself fail all pending tasks and close the connection. The
reader accepts only digit-only literal sizes, caps a literal at 128 MiB
(configurable), adds an empty data line for {0} so the rest of the
response is not mistaken for literal data, and is iterative instead of
recursive. Quoted strings resolve quoted-pairs including \\, partial
fetch items like BODY[1]<0> keep their range attached, the token
separator before a literal is restored for LIST/STATUS parsing, message
ranges are capped at 16M entries and 2^32-1, FETCH/BODYSTRUCTURE
parsing uses tryParse and bounds checks, and completed tasks are removed
from the task map.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
connectToServer() never completed when the server closed the socket or stayed silent before its greeting; it now fails, also when the greeting does not arrive within the connect timeout. SmtpClient and PopClient fail the command awaiting a response when the connection is lost, when the client disconnects or when writing to the socket fails (the write futures were dropped before), and PopClient fails a command whose reply cannot be parsed instead of leaving it pending. QUIT no longer disconnects from inside the response handler; the clients disconnect after the command completed. Event stream controllers stay open so a reconnected client can still report events, and ImapClient.disconnect() fails its pending tasks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
POP3: the terminator of a multi-line reply was searched backwards over the whole buffer on every chunk and required a leading CRLF, so LIST or UIDL on an empty mailbox (+OK CRLF . CRLF) never completed and large messages were scanned in quadratic time. The reader now scans only new bytes with an incremental matcher, recognizes the terminator right after the status line and consumes it completely instead of leaving a stray LF. SMTP: all lines available in a chunk were treated as one complete reply, so a multi-line EHLO reply that arrived in two chunks was truncated and its remainder was attributed to the next command. Lines are now buffered until the final 'NNN text' line (RFC 5321 4.2), which also makes a single-line EHLO reply complete. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Both commands base64-encoded String.codeUnits, so an umlaut was sent as Latin-1 and any character outside Latin-1 threw an ArgumentError. RFC 4616 requires UTF-8, which CRAM-MD5 and XOAUTH2 already used. Also corrects the CRAM-MD5 log labels that claimed to be XOAUTH2. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d BDAT The send commands ignored the reply codes of their intermediate steps: the message payload was sent even when DATA was refused, which made the server interpret the message as commands, and a rejected recipient was ignored unless it happened to be the last one, so a message was partially delivered while the caller saw a success. Both the DATA and the BDAT command now stop on a rejected MAIL FROM, remember the first rejected recipient or a refused DATA/chunk reply, abort the transaction with RSET so the connection stays usable, and fail with the rejecting reply. BDAT sends SMTPUTF8 only when the server announces it and together with BODY=8BITMIME instead of replacing it, and the debug print is gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… afterwards MailClient.reconnect() and resume() ran the retry loop while holding the incoming lock; the loop called MailClient.listMailboxes(), which takes the same non-reentrant lock, so a reconnect without previously listed mailboxes hung forever, and disconnect() could never abort the loop because it needs the lock as well. The loop now runs outside the lock and lists mailboxes directly. The retry interval grows exponentially up to the maximum instead of jumping to it, and an aborted reconnect resets the reconnecting flag so events are not buffered and dropped forever. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The POP poll compared the message counts the wrong way round, so new messages were never detected, every poll opened a new session without ending the previous one, retrieved messages carried no sequence id and therefore could not be deleted or re-fetched, undeleting sent RSET followed by DELE again and store() always threw. OAuth accounts sent the access token as the PASS argument; PopClient now offers authenticateWithOAuth2() using AUTH XOAUTH2 and OauthAuthentication uses it. PopClient also accepts a SecurityContext like the other clients, and exceptions from the polling timer are logged instead of surfacing as unhandled errors. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…consistent The refreshed token only copied the access token and expiry, so a refresh token rotated by the provider was discarded and the next refresh failed. Concurrent callers could also refresh the same token twice, and the SMTP connection was authenticated without checking the expiry first. The refresh is now serialized, keeps the rotated refresh token and runs before the outgoing connection is authenticated. OauthToken defines equality and hashCode, and the authentications combine their fields with Object.hash so equal objects share a hash code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
LOGIN interpolated user name and password into a quoted string without escaping, so credentials containing '"', '\', non-ASCII characters or line breaks produced malformed or injectable commands. LOGIN now uses RFC 3501 quoted strings with escaping and falls back to literals for anything that cannot be quoted. Mailbox names are quoted with escaping everywhere (SELECT/EXAMINE/DELETE/RENAME/SUBSCRIBE, GETMETADATA) and rejected when they contain line breaks, SETMETADATA declares the literal size over the bytes that are actually sent, search terms escape backslashes and use a literal for control characters, literal data in SEARCH/SORT commands may contain line feeds, and the OAUTHBEARER authorization identity escapes ',' and '='. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ht parts The quoted-printable text decoder indexed past the end on truncated input and used int.parse on non-hex data, the base64 decoder threw on any non-alphabet character or bad padding, and both were reached without a guard from file name and header decoding; they are now lenient as RFC 2045 requires, and an undecodable encoded word is kept verbatim. Binary bodies with 7bit/8bit/binary transfer encoding are returned byte for byte instead of going through a UTF-8 round trip that rejected or corrupted non-ASCII bytes, and quoted-printable attachments are decoded on the binary path. getPartWithContentId() compares the normalised content id, the text part lookup skips attached messages and text attachments, and header parameters are split at unquoted semicolons only, trimmed and decoded according to RFC 2231. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…folding Date headers rendered half-hour zones west of Greenwich as '-03-30' and UTC as '-0000' (RFC 5322 reserves that for 'no zone information'); the offset is now rendered as signed hours and unsigned minutes with '+0000' for UTC. Two-digit years follow the RFC 5322 4.3 rule (50-99 are 19xx), three-digit years get 1900 added and absurd years are rejected. The Q and B header encoders mixed rune and UTF-16 indices, so display names with astral characters lost their closing '?=' or produced replacement characters and lone surrogates. Header folding no longer breaks a line inside an encoded word. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…igner Message-IDs and multipart boundaries are generated from Random.secure() where available. Plus-alias matching required only a prefix match, so 'ad+min@example.com' was treated as 'admin@example.com'; it now requires the exact base address. The DKIM signer hashed only the first 72 UTF-16 code units of the body (throwing for shorter bodies and leaving the rest unsigned), signed only the From header and duplicated the text part because buildMimeMessage() was not idempotent; it now hashes the whole canonicalized body, signs From/To/Cc/Subject/Date/Message-ID/ MIME-Version/Content-Type in h= order, and building twice keeps a single text part. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
deleteMessages(), its copy-to-trash fallback and the move fallback issued a plain STORE for UID sequences, so the message with that sequence number was flagged as deleted and, with expunge, permanently removed while the intended message survived; every IMAP-fetched message carries a UID, so this was the normal path. They now use UID STORE and, where UIDPLUS is available, UID EXPUNGE limited to the affected messages. Switching mailboxes and the undo paths use UNSELECT instead of CLOSE when the server supports it, so messages that were deleted with canUndo are no longer expunged as a side effect. unselectMailbox() also passes the matching type argument so its parser no longer fails at runtime. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The first response line was only cleared in sendCommand, so after a multi-step command wrote its follow-up the stale line was dispatched again and the real reply stayed buffered for the next command. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The new message count was stored before the retrieve loop, so a failed retrieve made the next poll skip all remaining new messages. The count now advances with every loaded message. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The send commands validated the envelope in their constructors and threw ArgumentError synchronously, which escaped the SmtpException handler of the mail client. The client now checks the envelope first and throws an SmtpException, so MailClient.sendMessage fails with a MailException. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The mail client reuses its IMAP, POP and SMTP clients. A reply that was received only partially when the connection was lost stayed buffered and corrupted the parsing of the first replies of the next connection. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
connect() with connection information created a greeting completer that nobody awaited, so a socket closing before the greeting raised an unhandled asynchronous error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Protocol errors were reported as 'connection lost: ImapException: protocol error: ...' and an intentional disconnect as 'connection lost: client disconnected'. Pending tasks now fail with the plain message. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
'utf8' is not the registered IANA name; decoding still accepts both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
RFC 2047 section 5 forbids an encoded-word inside a quoted-string, and strict receivers showed the raw encoded text as the display name. The quoted-printable header encoder gained a phrase mode that always encodes and only leaves letters, digits and !*+-/ unencoded; MailAddress.encode() uses it for non-ASCII names and keeps quoted-strings for ASCII names. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
allMatches(text, from) already yields the prefix match first, so the matchAsPrefix call and the first-match helper were redundant. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The security-critical decision was implemented twice with hand-copied messages. One helper now serves IMAP, POP and SMTP; a failing POP STLS is thereby also reported as MailException. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Both clients carried the same logic for failing a command whose write failed. ClientBase.writeOrFail() now owns it and uses createClientError, the clients only keep their own command bookkeeping. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <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
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.
Changes from code review