feat(gmail): add --attachment flag to +send helper#517
feat(gmail): add --attachment flag to +send helper#517alimpolat wants to merge 5 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: f3d0fa7 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 |
googleworkspace-bot
added
the
area: core
Reads files from disk, auto-detects MIME types from extensions, and builds multipart/mixed MIME messages. Supports multiple attachments via repeated --attachment flags. Security: paths validated against traversal (../), control characters, and non-file targets (directories, devices). Closes googleworkspace#498
|
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. |
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 significantly enhances 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
|
alimpolat
force-pushed
the
feat/attachment-flag
branch
from
c467d97 to
1dd90a3
Compare
|
/gemini review |
![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 introduces a valuable feature for sending email attachments. The implementation is well-tested and includes important security validations for attachment paths. I've identified a critical security vulnerability related to symlink handling in path validation, a high-severity issue with filename sanitization that could lead to malformed emails, and a high-severity maintainability concern due to code duplication. Addressing these points will significantly improve the robustness and security of the new functionality.
| fn validate_attachment_path(path: &PathBuf) -> Result<(), GwsError> { | ||
| let path_str = path.to_string_lossy(); | ||
|
|
||
| // Reject control characters | ||
| if path_str.bytes().any(|b| b < 0x20 || b == 0x7F) { | ||
| return Err(GwsError::Validation(format!( | ||
| "--attachment path contains invalid control characters: {}", | ||
| path_str | ||
| ))); | ||
| } | ||
|
|
||
| // Reject path traversal | ||
| for component in path.components() { | ||
| if let std::path::Component::ParentDir = component { | ||
| return Err(GwsError::Validation(format!( | ||
| "--attachment path must not contain '..' traversal: {}", | ||
| path_str | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| // File must exist and be a regular file | ||
| if !path.exists() { | ||
| return Err(GwsError::Validation(format!( | ||
| "--attachment file not found: {}", | ||
| path_str | ||
| ))); | ||
| } | ||
| if !path.is_file() { | ||
| return Err(GwsError::Validation(format!( | ||
| "--attachment path is not a file: {}", | ||
| path_str | ||
| ))); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The current path validation is vulnerable to path traversal attacks using symbolic links. An attacker could create a symlink that points to a directory outside the current working directory (e.g., ln -s ../../ .) and then use it to access sensitive files (e.g., --attachment ./.ssh/id_rsa).
The validation logic checks for .. components in the provided path string but does not resolve symlinks before doing so. A safer approach is to canonicalize the path to resolve all symlinks and .. components, and then verify that the resulting absolute path is still within the current working directory.
fn validate_attachment_path(path: &PathBuf) -> Result<(), GwsError> {
let path_str = path.to_string_lossy();
// Reject control characters
if path_str.bytes().any(|b| b < 0x20 || b == 0x7F) {
return Err(GwsError::Validation(format!(
"--attachment path contains invalid control characters: {}",
path_str
)));
}
// Reject absolute paths to enforce CWD-relative access
if path.is_absolute() {
return Err(GwsError::Validation(format!(
"--attachment path must be relative: {}",
path_str
)));
}
// Resolve symlinks and normalize the path, which also checks for existence.
let canonical_path = path.canonicalize().map_err(|e| {
GwsError::Validation(format!(
"Failed to resolve attachment path '{}': {}. Ensure the file exists and is accessible.",
path_str, e
))
})?;
// Ensure the resolved path is within the current working directory.
let cwd = std::env::current_dir()
.map_err(|_| GwsError::Validation("Could not determine current working directory.".to_string()))?;
if !canonical_path.starts_with(&cwd) {
return Err(GwsError::Validation(format!(
"--attachment path '{}' resolves outside the current working directory, which is not allowed.",
path_str
)));
}
// Final check: must be a file.
if !canonical_path.is_file() {
return Err(GwsError::Validation(format!(
"--attachment path is not a file: {}",
path_str
)));
}
Ok(())
}
src/helpers/gmail/mod.rs
Outdated
| let mut headers = format!( | ||
| "To: {}\r\nSubject: {}", | ||
| encode_address_header(&sanitize_header_value(self.to)), | ||
| encode_header_value(&sanitize_header_value(self.subject)), | ||
| ); | ||
|
|
||
| if let Some(ref threading) = self.threading { | ||
| headers.push_str(&format!( | ||
| "\r\nIn-Reply-To: {}\r\nReferences: {}", | ||
| sanitize_header_value(threading.in_reply_to), | ||
| sanitize_header_value(threading.references), | ||
| )); | ||
| } | ||
|
|
||
| headers.push_str(&format!( | ||
| "\r\nMIME-Version: 1.0\r\nContent-Type: multipart/mixed; boundary=\"{boundary}\"" | ||
| )); | ||
|
|
||
| if let Some(from) = self.from { | ||
| headers.push_str(&format!( | ||
| "\r\nFrom: {}", | ||
| encode_address_header(&sanitize_header_value(from)) | ||
| )); | ||
| } | ||
|
|
||
| if let Some(cc) = self.cc { | ||
| headers.push_str(&format!( | ||
| "\r\nCc: {}", | ||
| encode_address_header(&sanitize_header_value(cc)) | ||
| )); | ||
| } | ||
|
|
||
| if let Some(bcc) = self.bcc { | ||
| headers.push_str(&format!( | ||
| "\r\nBcc: {}", | ||
| encode_address_header(&sanitize_header_value(bcc)) | ||
| )); | ||
| } |
There was a problem hiding this comment.
This block of code for constructing email headers is almost an exact duplicate of the logic in the existing build() method. This duplication makes the code harder to maintain, as any future changes to header logic (e.g., adding a new header) will need to be made in two places, increasing the risk of bugs.
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, I recommend extracting the common header-building logic into a private helper method within the MessageBuilder implementation. Both build() and build_with_attachments() can then call this shared method.
Here is an example of how you could refactor it:
impl MessageBuilder<'_> {
fn build_common_headers(&self) -> String {
let mut headers = format!(
"To: {}\r\nSubject: {}",
encode_address_header(&sanitize_header_value(self.to)),
encode_header_value(&sanitize_header_value(self.subject)),
);
if let Some(ref threading) = self.threading {
// ... append In-Reply-To/References ...
}
if let Some(from) = self.from {
// ... append From ...
}
if let Some(cc) = self.cc {
// ... append Cc ...
}
if let Some(bcc) = self.bcc {
// ... append Bcc ...
}
headers
}
pub fn build(&self, body: &str) -> String {
let mut headers = self.build_common_headers();
// ... append simple Content-Type ...
format!("{}\r\n\r\n{}", headers, body)
}
pub fn build_with_attachments(&self, body: &str, attachments: &[Attachment]) -> String {
// ...
let mut headers = self.build_common_headers();
// ... append multipart Content-Type ...
// ... build multipart body ...
}
}| message.push_str(&format!( | ||
| "--{boundary}\r\n\ | ||
| Content-Type: {mime_type}\r\n\ | ||
| Content-Transfer-Encoding: base64\r\n\ | ||
| Content-Disposition: attachment; filename=\"{filename}\"\r\n\ | ||
| \r\n\ | ||
| {encoded}\r\n", | ||
| mime_type = att.mime_type, | ||
| filename = sanitize_header_value(&att.filename), | ||
| )); |
There was a problem hiding this comment.
The current implementation of sanitize_header_value only removes CR and LF characters. A filename containing a double quote (") will break the Content-Disposition header's filename parameter, as it will be prematurely terminated. For example, a filename my"file.txt would result in a malformed header: filename="my"file.txt".
To ensure the header is correctly formatted according to RFC 2183, you should escape backslashes and double quotes within the filename.
let encoded = base64::engine::general_purpose::STANDARD.encode(&att.data);
let sanitized_filename = sanitize_header_value(&att.filename)
.replace('\\', "\\\\")
.replace('"', "\\\"");
message.push_str(&format!(
"--{boundary}\r\n\
Content-Type: {mime_type}\r\n\
Content-Transfer-Encoding: base64\r\n\
Content-Disposition: attachment; filename=\"{filename}\"\r\n\
\r\n\
{encoded}\r\n",
mime_type = att.mime_type,
filename = sanitized_filename,
));
There was a problem hiding this comment.
Code Review
This pull request adds support for sending email attachments via the gmail +send helper. The changes include new CLI flags, path validation for security, MIME type detection, and construction of multipart MIME messages. The implementation is well-tested and includes important security considerations like preventing path traversal. My review focuses on an opportunity to reduce code duplication related to the new attachment feature, which will improve long-term maintainability.
| let raw = if config.attachments.is_empty() { | ||
| MessageBuilder { | ||
| to: &config.to, | ||
| subject: &config.subject, | ||
| from: None, | ||
| cc: config.cc.as_deref(), | ||
| bcc: config.bcc.as_deref(), | ||
| threading: None, | ||
| html: config.html, | ||
| } | ||
| .build(&config.body) | ||
| } else { | ||
| let attachments = load_attachments(&config.attachments)?; | ||
| MessageBuilder { | ||
| to: &config.to, | ||
| subject: &config.subject, | ||
| from: None, | ||
| cc: config.cc.as_deref(), | ||
| bcc: config.bcc.as_deref(), | ||
| threading: None, | ||
| html: config.html, | ||
| } | ||
| .build_with_attachments(&config.body, &attachments) | ||
| }; |
There was a problem hiding this comment.
The MessageBuilder is constructed twice with identical parameters, once in the if block and once in the else block. This code duplication can be avoided by creating the MessageBuilder instance once before the conditional block. This improves maintainability and reduces the chance of introducing bugs if the builder's initialization logic needs to be updated in the future.
let builder = MessageBuilder {
to: &config.to,
subject: &config.subject,
from: None,
cc: config.cc.as_deref(),
bcc: config.bcc.as_deref(),
threading: None,
html: config.html,
};
let raw = if config.attachments.is_empty() {
builder.build(&config.body)
} else {
let attachments = load_attachments(&config.attachments)?;
builder.build_with_attachments(&config.body, &attachments)
};- Extract build_common_headers() to eliminate header duplication (DRY) - Harden path validation: canonicalize + CWD containment check to prevent symlink traversal attacks - Reject absolute attachment paths - Escape backslashes and quotes in attachment filenames (RFC 2183) - Add tests for filename escaping, absolute path rejection, valid file
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a much-needed feature for sending email attachments. The implementation is comprehensive, covering MIME message construction, path validation, and MIME type detection.
My review focuses on two key areas:
- A critical security vulnerability (TOCTOU race condition) in the attachment path validation logic that could allow reading arbitrary files.
- A high-severity issue with handling non-ASCII filenames, which can lead to malformed emails.
Addressing these points will make the feature robust and secure.
src/helpers/gmail/send.rs
Outdated
| // Validate each attachment path | ||
| for path in &attachments { | ||
| validate_attachment_path(path)?; | ||
| } |
There was a problem hiding this comment.
There is a Time-of-check to time-of-use (TOCTOU) race condition vulnerability in how attachment paths are handled.
The current implementation validates all attachment paths in parse_send_args and then, at a later time, reads the files in load_attachments. An attacker could modify a file path (e.g., replace a file with a symlink to a sensitive file like /etc/passwd) between the validation check and the read operation.
To mitigate this, the validation should be performed immediately before the file is read. This significantly reduces the window for a race condition.
Recommendation:
- Remove this validation loop from
parse_send_args. - Move the call to
validate_attachment_path(path)?inside the loop in theload_attachmentsfunction, placing it just before thestd::fs::read(path)call.
src/helpers/gmail/mod.rs
Outdated
| let safe_filename = sanitize_header_value(&att.filename) | ||
| .replace('\\', "\\\\") | ||
| .replace('"', "\\\""); |
There was a problem hiding this comment.
The current filename sanitization does not correctly handle non-ASCII characters. If a filename contains characters outside the ASCII set (e.g., 'résumé.pdf'), they will be inserted directly into the Content-Disposition header. This violates RFC standards that require headers to be ASCII, resulting in a malformed MIME part that may cause issues for email clients.
To properly support international filenames, you should use RFC 2231 encoding for the filename parameter. This involves using the filename* parameter with a specified charset and percent-encoding the filename.
For example:
Content-Disposition: attachment; filename*="UTF-8''r%C3%A9sum%C3%A9.pdf"
Please consider implementing RFC 2231 encoding for non-ASCII filenames to ensure correctness.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a new --attachment flag to the gmail +send helper, allowing users to send emails with file attachments. The implementation includes building multipart/mixed MIME messages, auto-detecting MIME types from file extensions, and validating attachment paths.
The code is well-structured and includes comprehensive tests for the new functionality. I've identified a critical security vulnerability (TOCTOU race condition) in the attachment path validation logic and a high-severity issue regarding the handling of non-ASCII filenames. My review comments provide details and suggestions for fixing these issues.
src/helpers/gmail/send.rs
Outdated
| // Validate each attachment path | ||
| for path in &attachments { | ||
| validate_attachment_path(path)?; | ||
| } |
There was a problem hiding this comment.
This validation introduces a Time-of-check to time-of-use (TOCTOU) vulnerability. The file path is validated here, but the file content is read much later in load_attachments. An attacker could replace the file with a malicious one (e.g., a symlink to a sensitive file) between the check and the read. To fix this, the validation logic should be moved into load_attachments to execute immediately before the file is read. Please remove this loop.
| fn load_attachments(paths: &[PathBuf]) -> Result<Vec<super::Attachment>, GwsError> { | ||
| let mut attachments = Vec::with_capacity(paths.len()); | ||
|
|
||
| for path in paths { | ||
| let data = std::fs::read(path).map_err(|e| { | ||
| GwsError::Validation(format!( | ||
| "Failed to read attachment '{}': {}", | ||
| path.display(), | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| let filename = path | ||
| .file_name() | ||
| .map(|n| n.to_string_lossy().to_string()) | ||
| .unwrap_or_else(|| "attachment".to_string()); | ||
|
|
||
| let mime_type = super::mime_type_from_extension(&filename).to_string(); | ||
|
|
||
| attachments.push(super::Attachment { | ||
| filename, | ||
| mime_type, | ||
| data, | ||
| }); | ||
| } | ||
|
|
||
| Ok(attachments) | ||
| } |
There was a problem hiding this comment.
To mitigate the TOCTOU vulnerability identified in parse_send_args, the path validation should be performed here, immediately before reading the file. This minimizes the time window for an attacker to tamper with the file system object being attached.
fn load_attachments(paths: &[PathBuf]) -> Result<Vec<super::Attachment>, GwsError> {
let mut attachments = Vec::with_capacity(paths.len());
for path in paths {
validate_attachment_path(path)?;
let data = std::fs::read(path).map_err(|e| {
GwsError::Validation(format!(
"Failed to read attachment '{}': {}",
path.display(),
e
))
})?;
let filename = path
.file_name()
.map(|n| n.to_string_lossy().to_string())
.unwrap_or_else(|| "attachment".to_string());
let mime_type = super::mime_type_from_extension(&filename).to_string();
attachments.push(super::Attachment {
filename,
mime_type,
data,
});
}
Ok(attachments)
}
src/helpers/gmail/mod.rs
Outdated
| let safe_filename = sanitize_header_value(&att.filename) | ||
| .replace('\\', "\\\\") | ||
| .replace('"', "\\\""); |
There was a problem hiding this comment.
The current implementation for handling attachment filenames in the Content-Disposition header does not correctly handle non-ASCII characters. Sending a filename like résumé.pdf will result in a malformed header for many email clients, as the value is not encoded according to RFC 2231/5987. This can lead to garbled or incorrect filenames for the recipient.
A robust solution involves implementing RFC 2231 encoding for the filename parameter (e.g., filename*=UTF-8''r%C3%A9sum%C3%A9.pdf).
As a simpler, more compatible (though not strictly standard for this header) alternative, you could use RFC 2047 encoding, which is already used elsewhere in this file for other headers. However, you would need a variant of encode_header_value that doesn't produce folded lines.
At a minimum, you should consider sanitizing filenames to be ASCII-only to prevent malformed headers, even if it impacts user experience for international filenames.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for sending email attachments. The implementation is well-structured, with clear separation of concerns and comprehensive tests. However, I've identified a critical Time-of-check to time-of-use (TOCTOU) security vulnerability in the attachment path validation logic. The validation and file-reading operations are separated, creating a race condition window. My review includes a critical comment with a suggested fix to mitigate this vulnerability by moving the validation to occur immediately before the file is read.
src/helpers/gmail/send.rs
Outdated
| // Validate each attachment path | ||
| for path in &attachments { | ||
| validate_attachment_path(path)?; | ||
| } |
There was a problem hiding this comment.
This validation loop introduces a Time-of-check to time-of-use (TOCTOU) race condition vulnerability. The path is validated here, but the file is read later in load_attachments. An attacker could swap the file with a symlink to a sensitive file in the time between these two operations.
To fix this, this validation should be removed from parse_send_args and moved into load_attachments to be performed immediately before reading the file. I have added another comment on load_attachments with the suggested implementation.
| fn load_attachments(paths: &[PathBuf]) -> Result<Vec<super::Attachment>, GwsError> { | ||
| let mut attachments = Vec::with_capacity(paths.len()); | ||
|
|
||
| for path in paths { | ||
| let data = std::fs::read(path).map_err(|e| { | ||
| GwsError::Validation(format!( | ||
| "Failed to read attachment '{}': {}", | ||
| path.display(), | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| let filename = path | ||
| .file_name() | ||
| .map(|n| n.to_string_lossy().to_string()) | ||
| .unwrap_or_else(|| "attachment".to_string()); | ||
|
|
||
| let mime_type = super::mime_type_from_extension(&filename).to_string(); | ||
|
|
||
| attachments.push(super::Attachment { | ||
| filename, | ||
| mime_type, | ||
| data, | ||
| }); | ||
| } | ||
|
|
||
| Ok(attachments) | ||
| } |
There was a problem hiding this comment.
To mitigate the TOCTOU vulnerability identified in parse_send_args, the path validation should be performed inside this function, immediately before reading each file. This ensures that the file being read is the same one that was validated.
This change moves the validation logic into this function.
fn load_attachments(paths: &[PathBuf]) -> Result<Vec<super::Attachment>, GwsError> {
let mut attachments = Vec::with_capacity(paths.len());
for path in paths {
// Validate the path immediately before reading to prevent TOCTOU race conditions.
validate_attachment_path(path)?;
let data = std::fs::read(path).map_err(|e| {
GwsError::Validation(format!(
"Failed to read attachment '{}': {}",
path.display(),
e
))
})?;
let filename = path
.file_name()
.map(|n| n.to_string_lossy().to_string())
.unwrap_or_else(|| "attachment".to_string());
let mime_type = super::mime_type_from_extension(&filename).to_string();
attachments.push(super::Attachment {
filename,
mime_type,
data,
});
}
Ok(attachments)
}- Move path validation into load_attachments() to minimize TOCTOU window (validate immediately before reading) - Add RFC 2231 encoding for non-ASCII filenames in Content-Disposition (filename*=UTF-8''... with ASCII fallback for older clients) - Extract encode_content_disposition() and percent_encode_filename() helper functions with full test coverage - Add tests for Swedish characters, spaces, and Unicode filenames
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new --attachment flag to the gmail +send helper, a great feature that allows users to send emails with attachments directly from the command line. The implementation is comprehensive, covering MIME message construction, automatic MIME type detection, and international filename support. The addition of security validations for attachment paths is particularly important and well-thought-out.
My review focuses on a critical security vulnerability in the file handling logic. I've identified a Time-of-check to Time-of-use (TOCTOU) race condition that could allow an attacker to bypass the path validation and read arbitrary files on the system. I've provided details on this in a specific comment. Addressing this is crucial for the security of the tool.
| validate_attachment_path(path)?; | ||
|
|
||
| let data = std::fs::read(path).map_err(|e| { | ||
| GwsError::Validation(format!( | ||
| "Failed to read attachment '{}': {}", | ||
| path.display(), | ||
| e | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
There is a Time-of-check to Time-of-use (TOCTOU) race condition vulnerability here.
An attacker could potentially bypass the path validation. Here's the scenario:
validate_attachment_path(path)is called on line 128. It verifies thatpathpoints to a safe file within the current working directory.- An attacker, in the small time window between the validation check completing and
std::fs::read(path)being called on line 130, replacespath(or one of its parent directories) with a symbolic link to a sensitive file outside the working directory (e.g.,/etc/passwd). std::fs::read(path)is executed, and it will follow the new symbolic link, reading the sensitive file.
Your validate_attachment_path function is well-written and correctly handles symlinks at the time of checking, but this race condition undermines its protection.
To mitigate this, you should ensure that the file being read is the same one that was validated. Here are a few strategies:
- Open the file without following symlinks. On Unix, you can use
std::os::unix::fs::OpenOptionsExt::custom_flagsto addO_NOFOLLOW. This is a robust solution but requires platform-specific code. - Verify file identity after opening. You can open the file, and then use its file descriptor to get its metadata (like inode number on Unix) and re-verify that it matches the metadata of the file you intended to open. This can also be platform-specific.
Given the security-sensitive nature of file access, I recommend addressing this race condition.
|
Hi! All the Gemini review feedback has been addressed across the follow-up commits:
All 160 Gmail-related tests pass locally (including 25+ new tests for attachments, path validation, MIME encoding, and non-ASCII filenames). The |
2 participants
Summary
Adds
--attachment <PATH>flag to thegmail +sendhelper, enabling email attachments without requiring manual RFC 2822/MIME construction or hitting command-line length limits.--attachmentflagsmultipart/mixedMIME message internally (no CLI argument size issues)../), control characters, directories, and missing filesUsage
Closes #498
Files Changed
src/helpers/gmail/mod.rsMessageBuilder::build_with_attachments(),Attachmentstruct,mime_type_from_extension(),--attachmentCLI arg, testssrc/helpers/gmail/send.rsSendConfig.attachments,validate_attachment_path(),load_attachments(), tests.changeset/add-attachment-flag.mdTest plan
send.rstests still pass (parse args, HTML flag, cc/bcc)load_attachments()reads file data and detects MIME typebuild_with_attachments()produces valid multipart/mixed MIMEmime_type_from_extension()common types, case insensitive, unknown fallbackgenerate_mime_boundary()uniquenesscargo test,cargo clippy, codecov/patch