feat(gmail): add --attachment flag to +send, +reply, +reply-all, +forward#395
feat(gmail): add --attachment flag to +send, +reply, +reply-all, +forward#395pae23 wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 526fa02 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
|
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 Gmail helper commands by introducing a 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 introduces file attachment support for several Gmail helper commands, which is a great feature. My review focuses on the new MIME message construction logic. I've found a critical issue where filenames with special characters are not properly escaped, which can lead to corrupted emails. I've also noted significant code duplication in the header generation logic that should be refactored to improve maintainability. The rest of the implementation looks good.
| message.push_str(&format!( | ||
| "--{}\r\n\ | ||
| Content-Type: {}; name=\"{}\"\r\n\ | ||
| Content-Disposition: attachment; filename=\"{}\"\r\n\ | ||
| Content-Transfer-Encoding: base64\r\n\ | ||
| \r\n\ | ||
| {}\r\n", | ||
| boundary, | ||
| att.content_type, | ||
| sanitize_header_value(&att.filename), | ||
| sanitize_header_value(&att.filename), | ||
| folded, | ||
| )); |
There was a problem hiding this comment.
The filename and name parameters for attachments are not correctly escaped. If a filename contains a double quote (") or a backslash (\), it will break the multipart/mixed MIME structure, leading to a corrupt email message. The sanitize_header_value function only removes newlines and does not handle characters that are special inside a quoted-string.
To fix this, you should escape backslashes and double quotes in the filename. For better readability, you could extract this logic into a helper function.
Additionally, non-ASCII characters in filenames are not handled according to RFCs, which can lead to display issues in email clients. The standard way to handle this is with RFC 2231 encoding.
| message.push_str(&format!( | |
| "--{}\r\n\ | |
| Content-Type: {}; name=\"{}\"\r\n\ | |
| Content-Disposition: attachment; filename=\"{}\"\r\n\ | |
| Content-Transfer-Encoding: base64\r\n\ | |
| \r\n\ | |
| {}\r\n", | |
| boundary, | |
| att.content_type, | |
| sanitize_header_value(&att.filename), | |
| sanitize_header_value(&att.filename), | |
| folded, | |
| )); | |
| message.push_str(&format!( | |
| "--{}\r\n\ | |
| Content-Type: {}; name=\"{}\"\r\n\ | |
| Content-Disposition: attachment; filename=\"{}\"\r\n\ | |
| Content-Transfer-Encoding: base64\r\n\ | |
| \r\n\ | |
| {}\r\n", | |
| boundary, | |
| att.content_type, | |
| sanitize_header_value(&att.filename).replace('\\', "\\\\").replace('"', "\\\""), | |
| sanitize_header_value(&att.filename).replace('\\', "\\\\").replace('"', "\\\""), | |
| folded, | |
| )); |
src/helpers/gmail/mod.rs
Outdated
| let mut headers = format!( | ||
| "To: {}\r\nSubject: {}", | ||
| 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: {}", sanitize_header_value(from))); | ||
| } | ||
|
|
||
| if let Some(cc) = self.cc { | ||
| headers.push_str(&format!("\r\nCc: {}", sanitize_header_value(cc))); | ||
| } | ||
|
|
||
| if let Some(bcc) = self.bcc { | ||
| headers.push_str(&format!("\r\nBcc: {}", sanitize_header_value(bcc))); | ||
| } |
There was a problem hiding this comment.
This block of code for building email headers is almost an exact copy of the logic in the build method (lines 375-405). This duplication makes the code harder to maintain, as any changes to header logic would need to be applied in two places, increasing the risk of bugs. Consider extracting the common header-building logic into a private helper method within impl MessageBuilder and calling it from both build and build_with_attachments.
|
I have signed the CLA - https://cla.developers.google.com/clas |
|
/gemini review |
1 similar comment
|
/gemini review |
pae23
force-pushed
the
feature/gmail-attachment-support
branch
3 times, most recently
from
b4fa490 to
3538f28
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for adding file attachments to emails via the Gmail helper commands. However, a High severity Path Traversal vulnerability has been identified in the read_attachments function, as it reads arbitrary files from the file system based on user-supplied paths without validation. This needs to be addressed by ensuring all paths are within a safe directory, consistent with existing security practices in src/validate.rs. Additionally, the method for parsing attachment paths is not robust for filenames containing commas, and the base64 encoding and line-folding logic contains an unwrap() which could be made more efficient and safer.
src/helpers/gmail/mod.rs
Outdated
| let path = std::path::Path::new(path_str); | ||
| let data = std::fs::read(path).map_err(|e| { |
There was a problem hiding this comment.
The read_attachments function reads files from paths provided directly via the --attachment CLI flag without any validation. This allows for Path Traversal, where an attacker (or a malicious prompt in an LLM-integrated environment) could read arbitrary files that the user running the CLI has access to. Given that this tool is explicitly designed to be used with LLM agents (as noted in src/validate.rs), this is a significant security risk. You should use a validation helper to ensure that all attachment paths are restricted to the current working directory or another safe location.
src/helpers/gmail/mod.rs
Outdated
| /// Read attachment files from a comma-separated list of paths. | ||
| pub(super) fn read_attachments(paths_csv: &str) -> Result<Vec<Attachment>, GwsError> { | ||
| let mut attachments = Vec::new(); | ||
| for path_str in paths_csv.split(',') { |
There was a problem hiding this comment.
Using split(',') to parse file paths is not robust, as it will incorrectly split filenames that contain commas. This prevents users from attaching such files.
A more idiomatic and robust approach for command-line tools is to allow the --attachment flag to be specified multiple times. This can be easily configured in clap by using ArgAction::Append.
This would require changes in a few places:
- Argument Parsing: Modify the
.arg()definition forattachmentininject_commandsto use.action(clap::ArgAction::Append). - Handler Logic: In
handle_send,handle_reply, etc., retrieve the paths usingmatches.get_many::<String>("attachment")which provides an iterator. - This Function: Update
read_attachmentsto accept an iterator of paths instead of a single comma-separated string.
This design change would make the feature more reliable and align better with user expectations for handling multiple file inputs.
src/helpers/gmail/mod.rs
Outdated
| let encoded = STANDARD.encode(&att.data); | ||
| // Fold base64 into 76-character lines per RFC 2045. | ||
| let folded: String = encoded | ||
| .as_bytes() | ||
| .chunks(76) | ||
| .map(|chunk| std::str::from_utf8(chunk).unwrap()) | ||
| .collect::<Vec<_>>() | ||
| .join("\r\n"); |
There was a problem hiding this comment.
This implementation for encoding and folding the base64 string has a couple of issues:
- It uses
.unwrap(), which could panic. While base64 is ASCII andfrom_utf8should not fail, it's better to avoidunwrap()in production code. - It's inefficient. It first encodes the entire file into a single large
String, then iterates over its bytes to create aVec<&str>, and finally joins them into another newString. This involves multiple allocations for large files.
A more robust and efficient approach is to use the base64 crate's display helpers for line wrapping, which streams the encoding and wrapping process without intermediate allocations.
| let encoded = STANDARD.encode(&att.data); | |
| // Fold base64 into 76-character lines per RFC 2045. | |
| let folded: String = encoded | |
| .as_bytes() | |
| .chunks(76) | |
| .map(|chunk| std::str::from_utf8(chunk).unwrap()) | |
| .collect::<Vec<_>>() | |
| .join("\r\n"); | |
| use base64::display::{Base64Display, LineWrap}; | |
| // Fold base64 into 76-character lines per RFC 2045. | |
| let folded = Base64Display::new(&att.data, &STANDARD) | |
| .with_config(LineWrap::Wrap(76, base64::display::LineEnding::CRLF)); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds file attachment support to Gmail helper commands. A security review identified a high-severity path traversal vulnerability where user-supplied attachment paths are not validated against the current working directory, which is a requirement for LLM safety in this project. Additionally, double quotes in filenames are not escaped, posing a medium-severity risk. Beyond the security concerns, the implementation could be improved by using the base64 crate's built-in line wrapping for better maintainability and performance, and by converting synchronous file I/O to asynchronous to avoid blocking the runtime.
src/helpers/gmail/mod.rs
Outdated
| let path = std::path::Path::new(path_str); | ||
| let data = std::fs::read(path).map_err(|e| { |
There was a problem hiding this comment.
This function has a high-severity path traversal vulnerability: read_attachments reads files from user-supplied paths without validation, which could allow a malicious actor to exfiltrate sensitive files. It's crucial to use crate::validate::validate_safe_dir_path to restrict attachment paths to the current working directory. Additionally, the current implementation uses synchronous file I/O (std::fs::read) within an async context, which can block the Tokio runtime. Consider refactoring to use asynchronous file I/O for better performance and to prevent blocking.
| let path = std::path::Path::new(path_str); | |
| let data = std::fs::read(path).map_err(|e| { | |
| let path = crate::validate::validate_safe_dir_path(path_str)?; | |
| let data = std::fs::read(&path).map_err(|e| { |
src/helpers/gmail/mod.rs
Outdated
| boundary, att.content_type, safe_filename, safe_filename, folded, | ||
| )); | ||
| } | ||
|
|
||
| // Closing boundary. | ||
| message.push_str(&format!("--{}--\r\n", boundary)); | ||
|
|
||
| message |
There was a problem hiding this comment.
The current implementation for folding base64-encoded lines is done manually. The base64 crate provides built-in support for MIME line wrapping (76 characters per line with CRLF), which is more idiomatic and can be more performant. Using the library's feature simplifies the code and improves maintainability.
| boundary, att.content_type, safe_filename, safe_filename, folded, | |
| )); | |
| } | |
| // Closing boundary. | |
| message.push_str(&format!("--{}--\r\n", boundary)); | |
| message | |
| let folded = base64::engine::general_purpose::STANDARD | |
| .with_config( | |
| base64::engine::LineWrap::Mime, | |
| base64::alphabet::STANDARD.into_padding(), | |
| ) | |
| .encode(&att.data); |
There was a problem hiding this comment.
Code Review
This pull request introduces a much-needed feature for adding attachments to emails via helper commands. The implementation is comprehensive, covering MIME message construction, content type detection, and argument parsing for multiple commands.
My review focuses on improving robustness and efficiency. I've identified a few high-severity issues:
- The current implementation reads entire files into memory, which poses a risk of high memory usage and crashes for large attachments.
- The fallback logic for determining attachment filenames could leak local path information.
- The Base64 line-wrapping is implemented manually, while a more efficient and idiomatic method is available in the
base64crate.
Addressing these points will make the new feature more robust and performant. The changes are otherwise well-structured and the addition of tests is great.
src/helpers/gmail/mod.rs
Outdated
| continue; | ||
| } | ||
| let path = std::path::Path::new(path_str); | ||
| let data = std::fs::read(path).map_err(|e| { |
There was a problem hiding this comment.
std::fs::read(path) reads the entire file into memory. For large attachments, this can lead to very high memory usage and potentially cause the CLI to crash with an out-of-memory error. Since a key motivation for this feature is to overcome size limits, it's important to handle large files gracefully.
A more robust solution would involve streaming the file content directly during the MIME part construction instead of pre-loading it into a Vec<u8>.
src/helpers/gmail/mod.rs
Outdated
| let filename = path | ||
| .file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .unwrap_or(path_str) | ||
| .to_string(); |
There was a problem hiding this comment.
Using unwrap_or(path_str) as a fallback for the filename is risky. If path.file_name() returns None, the entire input path string is used as the filename. This can leak local filesystem paths (e.g., /home/user/file.txt) into the email attachment's filename parameter, which is an information leak and results in an invalid filename. It's safer to fail with a validation error if a proper filename cannot be determined.
| let filename = path | |
| .file_name() | |
| .and_then(|n| n.to_str()) | |
| .unwrap_or(path_str) | |
| .to_string(); | |
| let filename = path | |
| .file_name() | |
| .and_then(|n| n.to_str()) | |
| .ok_or_else(|| GwsError::Validation(format!("Could not determine filename for path: {}", path_str)))? | |
| .to_string(); |
src/helpers/gmail/mod.rs
Outdated
| let encoded = STANDARD.encode(&att.data); | ||
| // Fold base64 into 76-character lines per RFC 2045. | ||
| let folded: String = encoded | ||
| .as_bytes() | ||
| .chunks(76) | ||
| .map(|chunk| std::str::from_utf8(chunk).unwrap()) | ||
| .collect::<Vec<_>>() | ||
| .join("\r\n"); |
There was a problem hiding this comment.
The current implementation manually folds the base64-encoded string into 76-character lines. This is less efficient and more verbose than using the built-in capabilities of the base64 crate.
The base64 crate provides the MIME engine which correctly implements RFC 2045 line wrapping (76 characters per line, terminated by \r\n). Using it will make the code more concise, idiomatic, and performant.
To apply this, you'll also need to change the use statement on line 437 from STANDARD to MIME:
use base64::engine::general_purpose::MIME;
| let encoded = STANDARD.encode(&att.data); | |
| // Fold base64 into 76-character lines per RFC 2045. | |
| let folded: String = encoded | |
| .as_bytes() | |
| .chunks(76) | |
| .map(|chunk| std::str::from_utf8(chunk).unwrap()) | |
| .collect::<Vec<_>>() | |
| .join("\r\n"); | |
| let folded = MIME.encode(&att.data); |
|
/gemini review |
pae23
force-pushed
the
feature/gmail-attachment-support
branch
from
109f6ab to
875ae35
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for adding attachments to emails via helper commands, with new logic for MIME message construction, attachment reading, and content type guessing. A comprehensive security audit found no high or critical vulnerabilities, indicating the implementation follows secure coding practices, particularly in handling user-supplied file paths and constructing MIME messages. However, an issue was identified regarding the handling of comma-separated file paths for the --attachment flag, which should be addressed.
src/helpers/gmail/mod.rs
Outdated
| Arg::new("attachment") | ||
| .long("attachment") | ||
| .help("File path to attach (may be repeated)") | ||
| .value_name("PATH") | ||
| .action(ArgAction::Append), |
There was a problem hiding this comment.
The --attachment flag is defined to accept repeated occurrences, but it doesn't handle comma-separated values as described in the pull request summary and usage examples (--attachment 'report.pdf,data.csv,fix.zip'). Currently, a comma-separated string would be treated as a single, invalid filename.
To support both repeated flags and comma-separated values, you should add value_delimiter(',') to the argument definition. It would also be good to update the help text to reflect this.
This change should be applied to the argument definitions for +send (here), +reply (around line 757), +reply-all (around line 827), and +forward (around line 906).
| Arg::new("attachment") | |
| .long("attachment") | |
| .help("File path to attach (may be repeated)") | |
| .value_name("PATH") | |
| .action(ArgAction::Append), | |
| Arg::new("attachment") | |
| .long("attachment") | |
| .help("File path(s) to attach (comma-separated, or use flag repeatedly)") | |
| .value_name("PATH") | |
| .action(ArgAction::Append) | |
| .value_delimiter(','), |
There was a problem hiding this comment.
Code Review
This pull request introduces attachment support to the Gmail helper commands. However, a critical path traversal vulnerability has been identified in the read_attachments function, allowing arbitrary files to be read and attached without proper directory validation, posing a significant information disclosure risk, especially for LLM agents. Additionally, there's a correctness issue with handling non-ASCII filenames in MIME headers, which could lead to garbled display in email clients.
src/helpers/gmail/mod.rs
Outdated
| let canonical = path.canonicalize().map_err(|e| { | ||
| GwsError::Other(anyhow::anyhow!( | ||
| "Failed to resolve attachment path '{}': {}", | ||
| path_str, | ||
| e | ||
| )) | ||
| })?; | ||
| if !canonical.is_file() { | ||
| return Err(GwsError::Other(anyhow::anyhow!( | ||
| "Attachment path '{}' is not a regular file", | ||
| path_str, | ||
| ))); | ||
| } | ||
| let data = std::fs::read(&canonical).map_err(|e| { |
There was a problem hiding this comment.
The read_attachments function is vulnerable to path traversal. Using path.canonicalize() on user-supplied paths without validating that the resulting canonical path is within a safe directory (e.g., the current working directory) allows for the reading and exfiltration of arbitrary files. This is a critical information disclosure risk, especially for a tool used by LLM agents, as it bypasses existing security models designed to restrict file access. To remediate this, ensure that the resolved attachment path is strictly confined to the current working directory.
let canonical = path.canonicalize().map_err(|e| {
GwsError::Validation(format!(
"Failed to resolve attachment path '{}': {}",
path_str,
e
))
})?;
let cwd = std::env::current_dir().map_err(|e| {
GwsError::Validation(format!("Failed to determine current directory: {}", e))
})?;
if !canonical.starts_with(&cwd) {
return Err(GwsError::Validation(format!(
"Attachment path '{}' resolves outside the current directory, which is not allowed.",
path_str
)));
}
if !canonical.is_file() {
return Err(GwsError::Validation(format!(
"Attachment path '{}' is not a regular file",
path_str,
)));
}
src/helpers/gmail/mod.rs
Outdated
| let safe_filename = escape_quoted_string(&att.filename); | ||
| message.push_str(&format!( | ||
| "--{}\r\n\ | ||
| Content-Type: {}; name=\"{}\"\r\n\ | ||
| Content-Disposition: attachment; filename=\"{}\"\r\n\ | ||
| Content-Transfer-Encoding: base64\r\n\ | ||
| \r\n\ | ||
| {}\r\n", | ||
| boundary, att.content_type, safe_filename, safe_filename, folded, | ||
| )); |
There was a problem hiding this comment.
The current implementation for attachment filenames does not correctly handle non-ASCII characters. Using non-ASCII characters directly in MIME headers is not robustly supported by all email clients and can lead to garbled or incorrect filenames for the recipient.
To ensure maximum compatibility, you should use the encoding specified in RFC 2231 for parameter values. This involves percent-encoding the filename and using the * suffix (e.g., filename*=UTF-8''...).
The suggested code below implements this by checking if the filename is ASCII. If not, it generates an RFC 2231-compliant parameter.
let mut content_type_line = format!("Content-Type: {}", att.content_type);
let mut disposition_line = "Content-Disposition: attachment".to_string();
if att.filename.is_ascii() {
let safe_filename = escape_quoted_string(&att.filename);
content_type_line.push_str(&format!("; name=\"{}\"", safe_filename));
disposition_line.push_str(&format!("; filename=\"{}\"", safe_filename));
} else {
// RFC 2231 for non-ASCII filenames.
let mut encoded = String::new();
for &byte in att.filename.as_bytes() {
match byte {
b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b'~' => encoded.push(byte as char),
_ => encoded.push_str(&format!("%{:02X}", byte)),
}
}
let rfc2231_param = format!("UTF-8''{}", encoded);
content_type_line.push_str(&format!("; name*={}", rfc2231_param));
disposition_line.push_str(&format!("; filename*={}", rfc2231_param));
}
message.push_str(&format!(
"--{}\r\n{}\r\n{}\r\nContent-Transfer-Encoding: base64\r\n\r\n{}\r\n",
boundary, content_type_line, disposition_line, folded
));
pae23
force-pushed
the
feature/gmail-attachment-support
branch
from
6561d7a to
eb62e53
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for adding attachments to emails. The implementation is well-structured, especially the MIME message construction and the security measure against path traversal. However, I've identified a couple of issues within the read_attachments function. Firstly, it doesn't handle comma-separated file paths as documented, which is a functional bug. Secondly, there's a potential information leak in how attachment filenames are determined. I've provided a single, comprehensive code suggestion to address both points, which should make the feature robust and secure.
| for path_str in paths { | ||
| let path_str = path_str.trim(); | ||
| if path_str.is_empty() { | ||
| continue; | ||
| } | ||
| let path = cwd.join(path_str); | ||
| let canonical = path.canonicalize().map_err(|e| { | ||
| GwsError::Other(anyhow::anyhow!( | ||
| "Failed to resolve attachment path '{}': {}", | ||
| path_str, | ||
| e | ||
| )) | ||
| })?; | ||
| if !canonical.starts_with(&canonical_cwd) { | ||
| return Err(GwsError::Other(anyhow::anyhow!( | ||
| "Attachment '{}' resolves to '{}' which is outside the current directory", | ||
| path_str, | ||
| canonical.display(), | ||
| ))); | ||
| } | ||
| if !canonical.is_file() { | ||
| return Err(GwsError::Other(anyhow::anyhow!( | ||
| "Attachment path '{}' is not a regular file", | ||
| path_str, | ||
| ))); | ||
| } | ||
| let data = std::fs::read(&canonical).map_err(|e| { | ||
| GwsError::Other(anyhow::anyhow!( | ||
| "Failed to read attachment '{}': {}", | ||
| path_str, | ||
| e | ||
| )) | ||
| })?; | ||
| let filename = canonical | ||
| .file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .unwrap_or(path_str) | ||
| .to_string(); | ||
| let content_type = guess_content_type(&filename).to_string(); | ||
| attachments.push(Attachment { | ||
| filename, | ||
| content_type, | ||
| data, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This loop has two issues that should be addressed:
-
Missing comma-separated path handling: The pull request description and examples indicate that the
--attachmentflag supports comma-separated paths (e.g.,--attachment file1.pdf,file2.txt). The current implementation does not split the value by commas, and instead attempts to find a file with a literal name containing commas, which will fail. -
Unsafe filename fallback: Using
.unwrap_or(path_str)can be unsafe. Ifcanonical.file_name()fails to produce a filename, it falls back to using the rawpath_str. This could leak local directory structure in the attachment's filename (e.g., ifpath_strwaspath/to/file.pdf) and is not robust. It's safer to return an error if a valid filename cannot be determined from the canonical path.
The suggested change below resolves both issues by introducing a nested loop to process comma-separated paths and by ensuring filename resolution is handled safely.
for path_list_str in paths {
for path_str in path_list_str.split(',') {
let path_str = path_str.trim();
if path_str.is_empty() {
continue;
}
let path = cwd.join(path_str);
let canonical = path.canonicalize().map_err(|e| {
GwsError::Other(anyhow::anyhow!(
"Failed to resolve attachment path '{}': {}",
path_str,
e
))
})?;
if !canonical.starts_with(&canonical_cwd) {
return Err(GwsError::Other(anyhow::anyhow!(
"Attachment '{}' resolves to '{}' which is outside the current directory",
path_str,
canonical.display(),
)));
}
if !canonical.is_file() {
return Err(GwsError::Other(anyhow::anyhow!(
"Attachment path '{}' is not a regular file",
path_str,
)));
}
let data = std::fs::read(&canonical).map_err(|e| {
GwsError::Other(anyhow::anyhow!(
"Failed to read attachment '{}': {}",
path_str,
e
))
})?;
let filename = canonical
.file_name()
.and_then(|n| n.to_str())
.ok_or_else(|| {
GwsError::Other(anyhow::anyhow!(
"Could not determine filename for attachment: {}",
canonical.display()
))
})?
.to_string();
let content_type = guess_content_type(&filename).to_string();
attachments.push(Attachment {
filename,
content_type,
data,
});
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds file attachment support to several Gmail helper commands, which is a great feature. The implementation is well-structured, introducing a MessageBuilder to handle MIME message construction. However, I've identified a high-severity security vulnerability in the file reading logic that could expose the tool to a Time-of-check to time-of-use (TOCTOU) race condition, potentially allowing it to read unintended files. My review includes a specific comment with a suggested fix for this issue.
src/helpers/gmail/mod.rs
Outdated
| if !canonical.is_file() { | ||
| return Err(GwsError::Other(anyhow::anyhow!( | ||
| "Attachment path '{}' is not a regular file", | ||
| path_str, | ||
| ))); | ||
| } | ||
| let data = std::fs::read(&canonical).map_err(|e| { | ||
| GwsError::Other(anyhow::anyhow!( | ||
| "Failed to read attachment '{}': {}", | ||
| path_str, | ||
| e | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
There is a Time-of-check to time-of-use (TOCTOU) race condition vulnerability here. The code first checks if the canonical path is a file (canonical.is_file()) and then reads the file in a separate operation (std::fs::read(&canonical)). An attacker could potentially replace the file with a symbolic link to a sensitive file (e.g., /etc/passwd) between these two operations. std::fs::read would then follow the symlink and read the sensitive file.
To mitigate this, you should open the file first to get a file handle, then get its metadata from the handle to verify it's a regular file, and finally read the contents from the same file handle. This ensures you are operating on the exact same file that you checked.
| if !canonical.is_file() { | |
| return Err(GwsError::Other(anyhow::anyhow!( | |
| "Attachment path '{}' is not a regular file", | |
| path_str, | |
| ))); | |
| } | |
| let data = std::fs::read(&canonical).map_err(|e| { | |
| GwsError::Other(anyhow::anyhow!( | |
| "Failed to read attachment '{}': {}", | |
| path_str, | |
| e | |
| )) | |
| })?; | |
| let data = (|| -> Result<Vec<u8>, anyhow::Error> { | |
| use std::io::Read; | |
| let mut file = std::fs::File::open(&canonical)?; | |
| let metadata = file.metadata()?; | |
| if !metadata.is_file() { | |
| anyhow::bail!("'{}' is not a regular file", path_str); | |
| } | |
| let mut buffer = Vec::with_capacity(metadata.len() as usize); | |
| file.read_to_end(&mut buffer)?; | |
| Ok(buffer) | |
| })() | |
| .map_err(|e| { | |
| GwsError::Other(anyhow::anyhow!( | |
| "Failed to read attachment '{}': {}", | |
| path_str, | |
| e | |
| )) | |
| })?; |
pae23
force-pushed
the
feature/gmail-attachment-support
branch
from
c1eaa8e to
5b383b6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for adding file attachments to Gmail helper commands. The implementation is well-structured, particularly the MessageBuilder refactoring to support multipart messages. However, I've identified a Time-of-check to Time-of-use (TOCTOU) security vulnerability in the file reading logic. My review includes a detailed explanation and a code suggestion to mitigate this issue.
src/helpers/gmail/mod.rs
Outdated
| let canonical = path.canonicalize().map_err(|e| { | ||
| GwsError::Other(anyhow::anyhow!( | ||
| "Failed to resolve attachment path '{}': {}", | ||
| path_str, | ||
| e | ||
| )) | ||
| })?; | ||
| if !canonical.is_file() { | ||
| return Err(GwsError::Other(anyhow::anyhow!( | ||
| "Attachment path '{}' is not a regular file", | ||
| path_str, | ||
| ))); | ||
| } | ||
| let data = std::fs::read(&canonical).map_err(|e| { | ||
| GwsError::Other(anyhow::anyhow!( | ||
| "Failed to read attachment '{}': {}", | ||
| path_str, | ||
| e | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
The current implementation of reading attachments has a Time-of-check to Time-of-use (TOCTOU) race condition vulnerability. The code first canonicalizes and checks if the path is a file, and then reads it in a separate operation (std::fs::read). An attacker with local access could replace the file with a symbolic link to a sensitive file (e.g., ~/.ssh/id_rsa) between the check and the read operation.
To mitigate this, you should open the file first to get a file handle, then perform checks on the handle's metadata, and finally read the content from the handle. This ensures that you are operating on the same file that you checked.
let mut file = std::fs::File::open(&path).map_err(|e| {
GwsError::Other(anyhow::anyhow!(
"Failed to open attachment path '{}': {}",
path_str,
e
))
})?;
let metadata = file.metadata().map_err(|e| {
GwsError::Other(anyhow::anyhow!(
"Failed to get metadata for attachment '{}': {}",
path_str,
e
))
})?;
if !metadata.is_file() {
return Err(GwsError::Other(anyhow::anyhow!(
"Attachment path '{}' is not a regular file",
path_str,
)));
}
let mut data = Vec::with_capacity(metadata.len() as usize);
use std::io::Read;
file.read_to_end(&mut data).map_err(|e| {
GwsError::Other(anyhow::anyhow!(
"Failed to read attachment '{}': {}",
path_str,
e
))
})?;
let canonical = path.canonicalize().map_err(|e| {
GwsError::Other(anyhow::anyhow!(
"Failed to resolve attachment path for filename '{}': {}",
path_str,
e
))
})?;
pae23
force-pushed
the
feature/gmail-attachment-support
branch
from
5b383b6 to
3a66b8f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-implemented feature for adding attachments to emails. The code is generally of high quality, with good attention to security details like preventing path leakage and handling non-ASCII filenames. However, I've identified a critical bug that would produce malformed MIME messages, and a high-severity security vulnerability (TOCTOU) in the file reading logic. My review includes suggestions to fix both of these issues, as addressing critical bugs and security vulnerabilities is essential for the feature's integrity and security.
| message.push_str(&format!( | ||
| "--{}\r\n\ | ||
| Content-Type: {}; {}\r\n\ | ||
| Content-Disposition: attachment; {}\r\n\ | ||
| Content-Transfer-Encoding: base64\r\n\ | ||
| \r\n\ | ||
| {}\r\n", | ||
| boundary, att.content_type, ct_name, cd_filename, folded, | ||
| )); |
There was a problem hiding this comment.
The use of a multi-line format! macro with indentation will include the leading whitespace in the resulting string. This will create malformed MIME part headers (e.g., Content-Type: ...), which will likely cause email clients to fail when parsing the attachments. The headers for a MIME part must start at the beginning of the line.
message.push_str(&format!(
"--{}\r\n
Content-Type: {}; {}\r\n
Content-Disposition: attachment; {}\r\n
Content-Transfer-Encoding: base64\r\n
\r\n
{}\r\n",
boundary, att.content_type, ct_name, cd_filename, folded,
));References
- Fixing critical bugs directly related to the feature being implemented is considered within the primary goal of a pull request, or an acceptable exception to prevent shipping broken code.
src/helpers/gmail/mod.rs
Outdated
| if !canonical.is_file() { | ||
| return Err(GwsError::Other(anyhow::anyhow!( | ||
| "Attachment path '{}' is not a regular file", | ||
| path_str, | ||
| ))); | ||
| } | ||
| let data = std::fs::read(&canonical).map_err(|e| { | ||
| GwsError::Other(anyhow::anyhow!( | ||
| "Failed to read attachment '{}': {}", | ||
| path_str, | ||
| e | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
There is a Time-of-check to time-of-use (TOCTOU) race condition vulnerability here. The code first checks if the canonical path is a file (is_file()) and then reads it using std::fs::read(). An attacker could potentially replace the file with a symbolic link to a sensitive file between the check and the read operation. This could lead to the contents of an unintended file being attached to the email.
To mitigate this, you should open the file to get a file handle, then get its metadata to verify it's a regular file, and finally read the contents from the handle. This ensures the check and the read operation are performed on the exact same file.
let data = (|| -> std::io::Result<Vec<u8>> {
let mut file = std::fs::File::open(&canonical)?;
let metadata = file.metadata()?;
if !metadata.is_file() {
return Err(std::io::Error::new(
std::io::ErrorKind::InvalidInput,
"path is not a regular file",
));
}
let mut data = Vec::with_capacity(metadata.len() as usize);
use std::io::Read;
file.read_to_end(&mut data)?;
Ok(data)
})()
.map_err(|e| {
GwsError::Other(anyhow::anyhow!(
"Failed to read attachment '{}': {}",
path_str,
e
))
})?;References
- Fixing high-severity security vulnerabilities directly related to the feature being implemented is considered within the primary goal of a pull request, or an acceptable exception to prevent shipping insecure code.
…ward Add file attachment support to Gmail helper commands. Uses multipart/mixed MIME construction with proper RFC 2045 base64 line-folding and RFC 2231 non-ASCII filename encoding. Changes from v1: - Accept absolute paths (no CWD restriction for read-only attachments) - Use only basename for filenames (no local path leakage) - Escape quotes in filenames (RFC 2045/2822 compliance) - RFC 2231 encoding for non-ASCII filenames - Use --attachment with ArgAction::Append (multiple flags, no comma parsing issues) - Respect html flag in multipart body part Content-Type - Rebased cleanly on upstream/main
pae23
force-pushed
the
feature/gmail-attachment-support
branch
from
3a66b8f to
526fa02
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a much-needed feature to add attachments to emails via the --attachment flag for +send, +reply, +reply-all, and +forward commands. The implementation is thorough, covering MIME multipart message construction, base64 encoding, and security considerations for filenames. The changes are well-tested. Additionally, the documentation in the skills/ directory should be updated to reflect this new functionality, as it currently contains outdated information.
2 participants
Summary
Adds file attachment support to Gmail helper commands (
+send,+reply,+reply-all,+forward).Currently, sending emails with attachments requires manually constructing a base64-encoded MIME message and passing it via
--json '{"raw": "..."}', which hits the Linux execve argument size limit (~128KB) for any non-trivial attachment. This PR adds a simple--attachmentflag that handles MIME construction automatically.Changes
--attachmentflag on+send,+reply,+reply-all,+forward— usesArgAction::Appendso each file gets its own--attachmentflag (avoids comma-parsing issues with filenames)Attachmentstruct andread_attachments()helper — accepts both relative and absolute pathsbuild_with_attachments()method onMessageBuilder— builds multipart/mixed MIME with sharedbuild_headers(), random boundary, RFC 2045 base64 line-folding, content-type auto-detection (25+ types)build()when no attachmentsrandandbase64cratesUsage
Test plan
cargo buildpassescargo test— all 147 gmail tests pass (10 new tests added)+sendGenerated with Claude Code