fix: quote display names with RFC 2822 special characters in +reply#513
fix: quote display names with RFC 2822 special characters in +reply#513richandersonio wants to merge 2 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: df6a097 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
googleworkspace-bot
added
the
area: core
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where display names with special characters were not being properly quoted, leading to email header parsing errors. The fix involves checking for RFC 2822 special characters and re-quoting the display name when necessary. The added test cases are good and cover the scenarios described. I've found one minor omission in the list of special characters that should be addressed, as detailed in the specific comment.
| format!("{} <{}>", display, email) | ||
| // Re-quote if the display name contains RFC 2822 special characters | ||
| // (commas, parens, etc.) to prevent header parsing issues. | ||
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { |
There was a problem hiding this comment.
The list of special characters to check for is missing the colon (:). According to RFC 5322, the colon is one of the specials that requires the display name to be quoted. Without this, a display name like Group: Work would not be quoted, potentially leading to it being misinterpreted as a group address syntax.
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { | |
| if display.contains(|c: char| ":,;()<>@\\\".[]".contains(c)) { |
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue with unquoted special characters in email display names, which is a valid problem. The approach of re-quoting display names containing special characters is correct. However, the implementation for escaping quotes within the display name (display.replace('"', "\\\"")) introduces a critical regression. It will corrupt display names that already contain correctly escaped quotes, leading to malformed email headers. I've left a detailed comment on the specific line with an explanation of the issue. The new tests are a good addition but should be expanded to cover display names with quotes to catch this kind of regression.
src/helpers/gmail/mod.rs
Outdated
| // Re-quote if the display name contains RFC 2822 special characters | ||
| // (commas, parens, etc.) to prevent header parsing issues. | ||
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { | ||
| format!("\"{}\" <{}>", display.replace('"', "\\\""), email) |
There was a problem hiding this comment.
The current implementation display.replace('"', "\\\"") introduces a critical regression. It incorrectly handles display names that already contain escaped quotes.
For example, if the original header has a display name like "Rich \"The Man\" Anderson", extract_display_name will return Rich \"The Man\" Anderson. Your new logic will then transform this into Rich \\\"The Man\\\" Anderson, resulting in a malformed header "Rich \\\"The Man\\\" Anderson" <email> when quoted.
This happens because .replace() acts on all " characters, without considering if they are already part of an escape sequence.
A correct implementation must handle escaping more carefully. Given that display may contain already-escaped characters, a simple string replacement is not sufficient. A more robust solution would be to iterate through the string to build a properly escaped version, or to first unescape the display string completely and then re-escape it for quoting.
I recommend rethinking this escaping logic to avoid this regression. You should also add a test case for a display name containing a quote to prevent this in the future.
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the issue of +reply failing when display names contain RFC 2822 special characters by implementing logic to re-quote such display names. The addition of new test cases specifically validates these scenarios, improving the robustness of header encoding. However, the list of special characters checked for re-quoting is incomplete, potentially leading to malformed headers for display names containing colons or periods.
| format!("{} <{}>", display, email) | ||
| // Re-quote if the display name contains RFC 2822 special characters | ||
| // (commas, parens, etc.) to prevent header parsing issues. | ||
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { |
There was a problem hiding this comment.
The list of RFC 2822 special characters used to determine if a display name needs re-quoting is incomplete. According to RFC 5322 (section 3.2.3), the characters : (colon) and . (period) are also considered 'specials' and require the display name to be quoted if they appear outside of a quoted-string context. Since extract_display_name strips surrounding quotes, a display name like My:Name or First.Last would not be re-quoted by the current logic, leading to an invalid header.
This can result in malformed email headers and potential delivery issues or incorrect parsing by email clients.
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { | |
| if display.contains(|c: char| ",;()<>@\".[]:.".contains(c)) { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where display names containing special characters were not being properly quoted, leading to header parsing errors. The implementation is robust in handling already-escaped characters within the display name. I've found one minor omission in the list of special characters that should be addressed to make the fix more complete.
| format!("{} <{}>", display, email) | ||
| // Re-quote if the display name contains RFC 2822 special characters | ||
| // (commas, parens, etc.) to prevent header parsing issues. | ||
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { |
There was a problem hiding this comment.
The list of special characters used to determine if a display name needs quoting appears to be missing the colon (:) character. According to RFC 2822 (section 3.2.1), the colon is considered a "special" character and should not appear in an unquoted atom. An unquoted display name containing a colon could lead to header parsing issues, similar to the ones this PR is fixing for commas and parentheses. It would be more robust to include it in the check.
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { | |
| if display.contains(|c: char| ",;()<>@\\\":.[]:".contains(c)) { |
When replying to emails from corporate senders with display names like "Anderson, Rich (CORP)" <email@adp.com>, the +reply command fails with "Invalid To header" (400) from the Gmail API. The root cause: encode_address_header() strips quotes from the display name via extract_display_name(), then reconstructs the address without re-quoting. When the display name contains RFC 2822 special characters (commas, parentheses), the unquoted form is ambiguous — commas split it into multiple malformed mailboxes and parentheses are interpreted as RFC 2822 comments. Fix: re-quote the display name when it contains any RFC 2822 special characters, using a single-pass character iterator that preserves already-escaped sequences and escapes bare quotes/backslashes. Fixes googleworkspace#512
richandersonio
force-pushed
the
fix/reply-display-name-quoting
branch
from
462e59f to
2aa19b9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with incorrect quoting of email display names that contain special characters, which could lead to malformed headers. The proposed solution correctly identifies when a display name needs to be quoted and handles existing escaped characters. My review points out a minor omission in the list of special characters being checked, which could cause issues in other edge cases. The overall approach and the addition of new tests are well-executed.
| format!("{} <{}>", display, email) | ||
| // Re-quote if the display name contains RFC 2822 special characters | ||
| // (commas, parens, etc.) to prevent header parsing issues. | ||
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { |
There was a problem hiding this comment.
According to RFC 2822, the list of specials characters that require a display name to be quoted also includes the colon (:). This character is currently missing from your check.
While less common in display names than commas or parentheses, omitting it could lead to incorrectly formatted headers if a name like "Location: Main Office" <main@example.com> is processed.
Please add the colon to the list of special characters to ensure full compliance and prevent potential parsing issues.
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { | |
| if display.contains(|c: char| ",;()<>@:\\\"..[]".contains(c)) { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where email display names containing special characters were not being quoted correctly, leading to invalid headers. The fix introduces logic to detect RFC 2822 special characters and then properly quote and escape the display name, including a mechanism to avoid double-escaping already-escaped characters. The new unit tests effectively cover the main scenarios. I have one suggestion to make the fix more complete.
| format!("{} <{}>", display, email) | ||
| // Re-quote if the display name contains RFC 2822 special characters | ||
| // (commas, parens, etc.) to prevent header parsing issues. | ||
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { |
There was a problem hiding this comment.
The list of special characters used to determine if a display name needs quoting is missing the colon (:). According to RFC 2822 and RFC 5322, the colon is a special character that requires the display name to be quoted if present. Without this, a display name containing a colon could result in a malformed header.
| if display.contains(|c: char| ",;()<>@\\\".[]".contains(c)) { | |
| if display.contains(|c: char| ",;()<@:\\\".[]".contains(c)) { |
|
Please resolve comments and update the special characters list |
…gration Consolidates PRs #491, #513, #517, and #502 into a single rollup: - Migrate message construction to mail-builder crate (RFC-compliant MIME) - Add --from flag to +send for send-as alias support - Add --attachment flag to +send with MIME auto-detection and path validation - Add +read helper for extracting message body/headers (text, HTML, JSON) - Serialize support for OriginalMessage and Mailbox types - Display name quoting handled natively by mail-builder
…#526) * refactor(gmail): replace hand-rolled email construction with mail-builder Replace custom MessageBuilder, RFC 2047 encoding, header sanitization, and address encoding (including #482) with the mail-builder crate (Stalwart Labs, 0 runtime deps). Each command builds a mail_builder::MessageBuilder directly. Introduce structured types throughout: - Mailbox type (parsed display name + email) replaces raw string passing - sanitize_control_chars strips ASCII control characters (CRLF, null, tab, etc.) at the parse boundary — defense-in-depth for mail-builder's structured header types, superseding sanitize_header_value, sanitize_component, and encode_address_header from #482 - OriginalMessage fields use Option<T> instead of empty-string sentinels - parse_original_message returns Result with validation (threadId, From, Message-ID) - Pre-parsed Config types (SendConfig, ForwardConfig, ReplyConfig) with Vec<Mailbox> — parse at the boundary, not downstream - parse_forward_args and parse_send_args return Result with --to validation, consistent with parse_reply_args - parse_optional_mailboxes helper normalizes Some(vec![]) to None for optional address fields (--cc, --bcc, --from) - Envelope types borrow from Config + OriginalMessage with lifetimes - Message IDs stored bare (no angle brackets), parsed once at boundary - References stored as Vec<String> instead of space-separated string - ThreadingHeaders bundles In-Reply-To + References with debug_assert for bare-ID convention - Shared CLI arg builders (common_mail_args, common_reply_args) eliminate duplicated --cc/--bcc/--html/--dry-run definitions Additional improvements: - finalize_message returns Result instead of panicking via .expect() - Mailbox::parse_list filters empty-email entries (trailing comma edge case) - format_email_link percent-encodes mailto hrefs to prevent parameter injection - Forward date handling: omits Date line when absent instead of showing empty "Date: " - Dry-run auth: log skipped auth as diagnostic instead of silently discarding errors - Restore --html tips in after_help strings (gmail_quote CSS, cid: image warnings, HTML fragment advice) lost in release PR #434 - Update execute_method call for upload_content_type parameter (#429) Delete: MessageBuilder, encode_header_value, sanitize_header_value, encode_address_header, sanitize_component, extract_email, extract_display_name, split_mailbox_list, build_references. * feat(gmail): add --from flag to +send for send-as alias support Consistent with +reply, +reply-all, and +forward which already support --from. Uses the same parse_optional_mailboxes path and apply_optional_headers plumbing. * fix: quote display names with RFC 2822 special characters in +reply When replying to emails from corporate senders with display names like "Anderson, Rich (CORP)" <email@adp.com>, the +reply command fails with "Invalid To header" (400) from the Gmail API. The root cause: encode_address_header() strips quotes from the display name via extract_display_name(), then reconstructs the address without re-quoting. When the display name contains RFC 2822 special characters (commas, parentheses), the unquoted form is ambiguous — commas split it into multiple malformed mailboxes and parentheses are interpreted as RFC 2822 comments. Fix: re-quote the display name when it contains any RFC 2822 special characters, using a single-pass character iterator that preserves already-escaped sequences and escapes bare quotes/backslashes. Fixes #512 * feat(gmail): add --attachment flag, +read helper, and mail-builder migration Consolidates PRs #491, #513, #517, and #502 into a single rollup: - Migrate message construction to mail-builder crate (RFC-compliant MIME) - Add --from flag to +send for send-as alias support - Add --attachment flag to +send with MIME auto-detection and path validation - Add +read helper for extracting message body/headers (text, HTML, JSON) - Serialize support for OriginalMessage and Mailbox types - Display name quoting handled natively by mail-builder * chore: regenerate skills [skip ci] * fix: use validate_safe_file_path for attachment path validation Addresses Gemini review: validate_safe_dir_path hardcodes '--dir' in error messages. validate_safe_file_path accepts the flag name, so errors now correctly reference '--attachment'. * refactor: make OriginalMessage.thread_id optional The Gmail API does not guarantee threadId on all message resources (e.g. drafts). Making it Option<String> prevents parse failures on valid messages and avoids requiring thread_id in helpers like +read that don't use it. * fix: use canonicalized path for attachment file operations (TOCTOU) validate_safe_file_path returns a canonicalized PathBuf. Use it for exists/is_file checks and downstream file reads instead of the original un-resolved path to prevent time-of-check/time-of-use races. * feat(gmail): add --attach flag for file attachments Add -a/--attach to +send, +reply, +reply-all, and +forward. Can be specified multiple times for multiple attachments. MIME type is auto- detected via mime_guess2. Closes #247. Send via the Gmail API upload endpoint (multipart/related with message/rfc822 media type) instead of base64-encoding into a JSON raw field. This raises the size limit from ~5MB (metadata-only endpoint) to 35MB (upload endpoint, per discovery document). Introduce UploadSource enum in the executor to consolidate upload_path, upload_content_type, and upload_bytes into a single type-safe parameter. File and Bytes variants make the two upload strategies (from disk vs. from memory) mutually exclusive by construction. Validates attachment paths (control characters, regular file, non-empty) and total size (25MB raw limit, accounting for base64 expansion of attachments within the MIME message against the 35MB API limit). Size check uses actual bytes read to avoid TOCTOU race. * chore: update changeset and fix integration with malob's attachment impl Update changeset to reflect combined work. Fix thread_id type mismatches in new tests from cherry-pick. Fix upload_path scope in main.rs. Make reject_control_chars pub(crate) for attachment validation. Co-authored-by: Malo Bourgon <mbourgon@gmail.com> * chore: regenerate skills [skip ci] * fix: restore MIME sanitization and terminal escape protection in executor Restore two security features accidentally lost during the UploadSource refactor: 1. resolve_upload_mime: restructure from early-returns to collect-then- sanitize pattern — strips control chars from user-supplied MIME types to prevent CRLF header injection. 2. Model Armor error path: restore sanitize_for_terminal on error messages to prevent terminal escape sequence injection from API responses. Co-authored-by: Malo Bourgon <mbourgon@gmail.com> * chore: remove duplicate changeset from cherry-pick gmail-attach-flag.md duplicated content already in gmail-helpers-rollup.md. Both were marked minor, which would cause a double version bump. * fix: add path traversal protection to attachment validation Replace reject_control_chars with validate_safe_file_path in parse_attachments. All file operations (metadata, read, filename extraction, MIME detection) now use the canonicalized path, preventing path traversal attacks (e.g. ../../.ssh/id_rsa) and closing TOCTOU gaps. Update tests to use CWD-relative temp directories (tempdir_in(".")) since validate_safe_file_path rejects paths outside the working directory. Co-authored-by: Malo Bourgon <mbourgon@gmail.com> * refactor: deduplicate terminal sanitizer in read.rs Replace the local sanitize_terminal_output function with the existing crate::error::sanitize_for_terminal via import alias. This eliminates code duplication and provides consistent sanitization across the codebase. The crate-wide sanitizer also correctly strips CR (carriage return) which can be abused for terminal overwrite attacks. --------- Co-authored-by: Malo Bourgon <mbourgon@gmail.com> Co-authored-by: Rich Anderson <richanderson00@gmail.com> Co-authored-by: jpoehnelt-bot <jpoehnelt-bot@users.noreply.github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@users.noreply.github.com>
4 participants
Summary
encode_address_header()when they contain RFC 2822 special characters (,;()<>@\".[])Problem
gws gmail +replyfails withInvalid To header(HTTP 400) when replying to emails where the sender's display name contains commas or parentheses — common with corporate email (Outlook/Exchange) which uses formats like"Anderson, Rich (CORP)" <email@adp.com>.The root cause:
encode_address_header()strips quotes from the display name viaextract_display_name(), then reconstructs the address without re-quoting. The unquoted comma makes it look like two malformed mailboxes, and unquoted parentheses are interpreted as RFC 2822 comments.Fix
One-line change in
encode_address_header(): check if the display name contains special characters and wrap in quotes if so.Test plan
cargo test -- test_encode_address_header— all 14 tests pass (12 existing + 2 new)test_encode_address_header_display_name_with_comma— verifies"Anderson, Rich (CORP)"stays quotedtest_encode_address_header_display_name_with_parens— verifiesRich (CORP)gets quotedFixes #512