From 1dd90a3a84f7dcd77cc0ddc2df819b67eb0dc5e0 Mon Sep 17 00:00:00 2001 From: Alim Polat Date: Tue, 17 Mar 2026 00:58:45 +0100 Subject: [PATCH 1/5] feat(gmail): add --attachment flag to +send helper 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 #498 --- .changeset/add-attachment-flag.md | 5 + src/helpers/gmail/mod.rs | 204 +++++++++++++++++++++- src/helpers/gmail/send.rs | 270 +++++++++++++++++++++++++++--- 3 files changed, 457 insertions(+), 22 deletions(-) create mode 100644 .changeset/add-attachment-flag.md diff --git a/.changeset/add-attachment-flag.md b/.changeset/add-attachment-flag.md new file mode 100644 index 00000000..35955f16 --- /dev/null +++ b/.changeset/add-attachment-flag.md @@ -0,0 +1,5 @@ +--- +"@googleworkspace/cli": minor +--- + +Add `--attachment` flag to `gmail +send` helper for sending emails with file attachments. Supports multiple files via repeated flags, auto-detects MIME types from extensions, and validates paths against traversal attacks. diff --git a/src/helpers/gmail/mod.rs b/src/helpers/gmail/mod.rs index 999d65ce..0b7dde61 100644 --- a/src/helpers/gmail/mod.rs +++ b/src/helpers/gmail/mod.rs @@ -580,6 +580,153 @@ impl MessageBuilder<'_> { format!("{}\r\n\r\n{}", headers, body) } + + /// Build an RFC 2822 multipart/mixed message with file attachments. + /// + /// The text (or HTML) body becomes the first MIME part; each attachment + /// is base64-encoded as a subsequent part. + pub fn build_with_attachments( + &self, + body: &str, + attachments: &[Attachment], + ) -> String { + if attachments.is_empty() { + return self.build(body); + } + + debug_assert!( + !self.to.is_empty(), + "MessageBuilder: `to` must not be empty" + ); + + let boundary = generate_mime_boundary(); + + 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)) + )); + } + + // Body part + let body_content_type = if self.html { + "text/html; charset=utf-8" + } else { + "text/plain; charset=utf-8" + }; + + let mut message = format!( + "{headers}\r\n\r\n\ + --{boundary}\r\n\ + Content-Type: {body_content_type}\r\n\ + \r\n\ + {body}\r\n" + ); + + // Attachment parts + for att in attachments { + let encoded = base64::engine::general_purpose::STANDARD.encode(&att.data); + 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), + )); + } + + // Closing boundary + message.push_str(&format!("--{boundary}--\r\n")); + message + } +} + +/// A file attachment ready to be included in a MIME message. +pub(super) struct Attachment { + pub filename: String, + pub mime_type: String, + pub data: Vec, +} + +/// Generate a unique MIME boundary string. +fn generate_mime_boundary() -> String { + use rand::Rng; + let mut rng = rand::thread_rng(); + let random: u128 = rng.gen(); + format!("gws_{random:032x}") +} + +/// Guess MIME type from a file extension. Falls back to +/// `application/octet-stream` for unknown extensions. +pub(super) fn mime_type_from_extension(filename: &str) -> &'static str { + let ext = filename + .rsplit('.') + .next() + .unwrap_or("") + .to_ascii_lowercase(); + match ext.as_str() { + "pdf" => "application/pdf", + "doc" => "application/msword", + "docx" => "application/vnd.openxmlformats-officedocument.wordprocessingml.document", + "xls" => "application/vnd.ms-excel", + "xlsx" => "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + "ppt" => "application/vnd.ms-powerpoint", + "pptx" => "application/vnd.openxmlformats-officedocument.presentationml.presentation", + "txt" => "text/plain", + "csv" => "text/csv", + "html" | "htm" => "text/html", + "json" => "application/json", + "xml" => "application/xml", + "zip" => "application/zip", + "gz" | "gzip" => "application/gzip", + "tar" => "application/x-tar", + "png" => "image/png", + "jpg" | "jpeg" => "image/jpeg", + "gif" => "image/gif", + "svg" => "image/svg+xml", + "webp" => "image/webp", + "mp3" => "audio/mpeg", + "mp4" => "video/mp4", + "wav" => "audio/wav", + "ics" => "text/calendar", + "eml" => "message/rfc822", + _ => "application/octet-stream", + } } /// Build the References header value. Returns just the message ID when there @@ -734,6 +881,13 @@ impl Helper for GmailHelper { .help("Treat --body as HTML content (default is plain text)") .action(ArgAction::SetTrue), ) + .arg( + Arg::new("attachment") + .long("attachment") + .help("Attach a file (can be repeated for multiple files)") + .action(ArgAction::Append) + .value_name("PATH"), + ) .arg( Arg::new("dry-run") .long("dry-run") @@ -745,12 +899,13 @@ impl Helper for GmailHelper { EXAMPLES: gws gmail +send --to alice@example.com --subject 'Hello' --body 'Hi Alice!' gws gmail +send --to alice@example.com --subject 'Hello' --body 'Hi!' --cc bob@example.com - gws gmail +send --to alice@example.com --subject 'Hello' --body 'Hi!' --bcc secret@example.com + gws gmail +send --to alice@example.com --subject 'Report' --body 'See attached.' --attachment ./report.pdf + gws gmail +send --to alice@example.com --subject 'Docs' --body 'Files attached.' --attachment a.pdf --attachment b.pdf gws gmail +send --to alice@example.com --subject 'Hello' --body 'Bold text' --html TIPS: - Handles RFC 2822 formatting and base64 encoding automatically. - For attachments, use the raw API instead: gws gmail users messages send --json '...'", + Handles RFC 2822 formatting, MIME encoding, and base64 automatically. + File MIME types are auto-detected from extensions (PDF, DOCX, PNG, etc.).", ), ); @@ -1938,4 +2093,47 @@ mod tests { <alice@example.com>" ); } + + // --- MIME type detection tests --- + + #[test] + fn test_mime_type_from_extension_common_types() { + assert_eq!(mime_type_from_extension("report.pdf"), "application/pdf"); + assert_eq!( + mime_type_from_extension("doc.docx"), + "application/vnd.openxmlformats-officedocument.wordprocessingml.document" + ); + assert_eq!(mime_type_from_extension("image.png"), "image/png"); + assert_eq!(mime_type_from_extension("photo.jpg"), "image/jpeg"); + assert_eq!(mime_type_from_extension("data.csv"), "text/csv"); + assert_eq!(mime_type_from_extension("archive.zip"), "application/zip"); + } + + #[test] + fn test_mime_type_from_extension_case_insensitive() { + assert_eq!(mime_type_from_extension("FILE.PDF"), "application/pdf"); + assert_eq!(mime_type_from_extension("IMAGE.PNG"), "image/png"); + assert_eq!(mime_type_from_extension("Doc.DOCX"), + "application/vnd.openxmlformats-officedocument.wordprocessingml.document"); + } + + #[test] + fn test_mime_type_from_extension_unknown_fallback() { + assert_eq!( + mime_type_from_extension("file.xyz"), + "application/octet-stream" + ); + assert_eq!( + mime_type_from_extension("noextension"), + "application/octet-stream" + ); + } + + #[test] + fn test_generate_mime_boundary_is_unique() { + let b1 = generate_mime_boundary(); + let b2 = generate_mime_boundary(); + assert_ne!(b1, b2); + assert!(b1.starts_with("gws_")); + } } diff --git a/src/helpers/gmail/send.rs b/src/helpers/gmail/send.rs index 6dcdf68c..bbd99a80 100644 --- a/src/helpers/gmail/send.rs +++ b/src/helpers/gmail/send.rs @@ -1,21 +1,36 @@ use super::*; +use std::path::PathBuf; pub(super) async fn handle_send( doc: &crate::discovery::RestDescription, matches: &ArgMatches, ) -> Result<(), GwsError> { - let config = parse_send_args(matches); + let config = parse_send_args(matches)?; - let raw = 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); + 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) + }; super::send_raw_email(doc, matches, &raw, None, None).await } @@ -27,22 +42,110 @@ pub(super) struct SendConfig { pub cc: Option, pub bcc: Option, pub html: bool, + pub attachments: Vec, } -fn parse_send_args(matches: &ArgMatches) -> SendConfig { - SendConfig { +fn parse_send_args(matches: &ArgMatches) -> Result { + let attachments: Vec = matches + .get_many::("attachment") + .unwrap_or_default() + .map(|s| PathBuf::from(s.trim())) + .collect(); + + // Validate each attachment path + for path in &attachments { + validate_attachment_path(path)?; + } + + Ok(SendConfig { to: matches.get_one::("to").unwrap().to_string(), subject: matches.get_one::("subject").unwrap().to_string(), body: matches.get_one::("body").unwrap().to_string(), cc: parse_optional_trimmed(matches, "cc"), bcc: parse_optional_trimmed(matches, "bcc"), html: matches.get_flag("html"), + attachments, + }) +} + +/// Validate that an attachment path is safe to read. +/// +/// Rejects paths with control characters, null bytes, or path traversal +/// segments (`..`). The file must exist and be a regular file (not a +/// directory or device). +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(()) +} + +/// Read attachment files from disk and prepare them for MIME encoding. +fn load_attachments(paths: &[PathBuf]) -> Result, 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) } #[cfg(test)] mod tests { use super::*; + use std::fs; + use tempfile::tempdir; fn make_matches_send(args: &[&str]) -> ArgMatches { let cmd = Command::new("test") @@ -51,7 +154,12 @@ mod tests { .arg(Arg::new("body").long("body")) .arg(Arg::new("cc").long("cc")) .arg(Arg::new("bcc").long("bcc")) - .arg(Arg::new("html").long("html").action(ArgAction::SetTrue)); + .arg(Arg::new("html").long("html").action(ArgAction::SetTrue)) + .arg( + Arg::new("attachment") + .long("attachment") + .action(ArgAction::Append), + ); cmd.try_get_matches_from(args).unwrap() } @@ -66,12 +174,13 @@ mod tests { "--body", "Body", ]); - let config = parse_send_args(&matches); + let config = parse_send_args(&matches).unwrap(); assert_eq!(config.to, "me@example.com"); assert_eq!(config.subject, "Hi"); assert_eq!(config.body, "Body"); assert!(config.cc.is_none()); assert!(config.bcc.is_none()); + assert!(config.attachments.is_empty()); } #[test] @@ -89,7 +198,7 @@ mod tests { "--bcc", "secret@example.com", ]); - let config = parse_send_args(&matches); + let config = parse_send_args(&matches).unwrap(); assert_eq!(config.cc.unwrap(), "carol@example.com"); assert_eq!(config.bcc.unwrap(), "secret@example.com"); @@ -107,7 +216,7 @@ mod tests { "--bcc", "", ]); - let config = parse_send_args(&matches); + let config = parse_send_args(&matches).unwrap(); assert!(config.cc.is_none()); assert!(config.bcc.is_none()); } @@ -124,7 +233,7 @@ mod tests { "Bold", "--html", ]); - let config = parse_send_args(&matches); + let config = parse_send_args(&matches).unwrap(); assert!(config.html); // Default is false @@ -137,7 +246,7 @@ mod tests { "--body", "Plain", ]); - let config = parse_send_args(&matches); + let config = parse_send_args(&matches).unwrap(); assert!(!config.html); } @@ -158,4 +267,127 @@ mod tests { assert!(raw.contains("To: bob@example.com")); assert!(raw.contains("

Hello world

")); } + + #[test] + fn test_parse_send_args_with_attachments() { + let dir = tempdir().unwrap(); + let file1 = dir.path().join("doc.pdf"); + let file2 = dir.path().join("image.png"); + fs::write(&file1, b"fake pdf").unwrap(); + fs::write(&file2, b"fake png").unwrap(); + + let f1 = file1.to_string_lossy().to_string(); + let f2 = file2.to_string_lossy().to_string(); + + let matches = make_matches_send(&[ + "test", + "--to", + "me@example.com", + "--subject", + "Hi", + "--body", + "See attached", + "--attachment", + &f1, + "--attachment", + &f2, + ]); + let config = parse_send_args(&matches).unwrap(); + assert_eq!(config.attachments.len(), 2); + } + + #[test] + fn test_validate_attachment_path_rejects_traversal() { + let path = PathBuf::from("../../etc/passwd"); + let result = validate_attachment_path(&path); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("traversal"), "got: {msg}"); + } + + #[test] + fn test_validate_attachment_path_rejects_missing_file() { + let path = PathBuf::from("nonexistent_file_12345.pdf"); + let result = validate_attachment_path(&path); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("not found"), "got: {msg}"); + } + + #[test] + fn test_validate_attachment_path_rejects_directory() { + let dir = tempdir().unwrap(); + let path = dir.path().to_path_buf(); + let result = validate_attachment_path(&path); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("not a file"), "got: {msg}"); + } + + #[test] + fn test_validate_attachment_path_rejects_control_chars() { + let path = PathBuf::from("file\x01name.pdf"); + let result = validate_attachment_path(&path); + assert!(result.is_err()); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("control characters"), "got: {msg}"); + } + + #[test] + fn test_load_attachments() { + let dir = tempdir().unwrap(); + let file = dir.path().join("report.pdf"); + fs::write(&file, b"PDF content here").unwrap(); + + let attachments = load_attachments(&[file]).unwrap(); + assert_eq!(attachments.len(), 1); + assert_eq!(attachments[0].filename, "report.pdf"); + assert_eq!(attachments[0].mime_type, "application/pdf"); + assert_eq!(attachments[0].data, b"PDF content here"); + } + + #[test] + fn test_build_with_attachments() { + let attachment = super::super::Attachment { + filename: "test.pdf".to_string(), + mime_type: "application/pdf".to_string(), + data: b"fake pdf data".to_vec(), + }; + + let raw = MessageBuilder { + to: "bob@example.com", + subject: "With attachment", + from: None, + cc: None, + bcc: None, + threading: None, + html: false, + } + .build_with_attachments("See attached.", &[attachment]); + + assert!(raw.contains("Content-Type: multipart/mixed; boundary=")); + assert!(raw.contains("Content-Type: text/plain; charset=utf-8")); + assert!(raw.contains("See attached.")); + assert!(raw.contains("Content-Disposition: attachment; filename=\"test.pdf\"")); + assert!(raw.contains("Content-Type: application/pdf")); + assert!(raw.contains("Content-Transfer-Encoding: base64")); + } + + #[test] + fn test_build_with_empty_attachments_falls_back() { + let raw = MessageBuilder { + to: "bob@example.com", + subject: "No attachments", + from: None, + cc: None, + bcc: None, + threading: None, + html: false, + } + .build_with_attachments("Plain body.", &[]); + + // Should fall back to simple message (no multipart) + assert!(raw.contains("Content-Type: text/plain; charset=utf-8")); + assert!(!raw.contains("multipart")); + } } From 50df01eb3264d1624a44cafb8ce6185b393d4310 Mon Sep 17 00:00:00 2001 From: Alim Polat Date: Tue, 17 Mar 2026 01:18:41 +0100 Subject: [PATCH 2/5] fix: address code review feedback - 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 --- src/helpers/gmail/mod.rs | 79 +++++++------------- src/helpers/gmail/send.rs | 151 +++++++++++++++++++++++++++----------- 2 files changed, 135 insertions(+), 95 deletions(-) diff --git a/src/helpers/gmail/mod.rs b/src/helpers/gmail/mod.rs index 0b7dde61..2d249691 100644 --- a/src/helpers/gmail/mod.rs +++ b/src/helpers/gmail/mod.rs @@ -523,8 +523,9 @@ pub(super) struct MessageBuilder<'a> { } impl MessageBuilder<'_> { - /// Build the complete RFC 2822 message (headers + blank line + body). - pub fn build(&self, body: &str) -> String { + /// Build the common RFC 2822 headers shared by both simple and multipart + /// messages: To, Subject, threading, From, Cc, Bcc. + fn build_common_headers(&self) -> String { debug_assert!( !self.to.is_empty(), "MessageBuilder: `to` must not be empty" @@ -546,15 +547,6 @@ impl MessageBuilder<'_> { )); } - let content_type = if self.html { - "text/html; charset=utf-8" - } else { - "text/plain; charset=utf-8" - }; - headers.push_str(&format!( - "\r\nMIME-Version: 1.0\r\nContent-Type: {content_type}" - )); - if let Some(from) = self.from { headers.push_str(&format!( "\r\nFrom: {}", @@ -578,6 +570,22 @@ impl MessageBuilder<'_> { )); } + headers + } + + /// Build the complete RFC 2822 message (headers + blank line + body). + pub fn build(&self, body: &str) -> String { + let mut headers = self.build_common_headers(); + + let content_type = if self.html { + "text/html; charset=utf-8" + } else { + "text/plain; charset=utf-8" + }; + headers.push_str(&format!( + "\r\nMIME-Version: 1.0\r\nContent-Type: {content_type}" + )); + format!("{}\r\n\r\n{}", headers, body) } @@ -594,52 +602,13 @@ impl MessageBuilder<'_> { return self.build(body); } - debug_assert!( - !self.to.is_empty(), - "MessageBuilder: `to` must not be empty" - ); - let boundary = generate_mime_boundary(); - - 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), - )); - } + let mut headers = self.build_common_headers(); 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)) - )); - } - // Body part let body_content_type = if self.html { "text/html; charset=utf-8" @@ -658,6 +627,12 @@ impl MessageBuilder<'_> { // Attachment parts for att in attachments { let encoded = base64::engine::general_purpose::STANDARD.encode(&att.data); + // Escape backslashes and quotes per RFC 2183 to prevent + // malformed Content-Disposition headers from filenames like: + // my"file.txt → my\"file.txt + let safe_filename = sanitize_header_value(&att.filename) + .replace('\\', "\\\\") + .replace('"', "\\\""); message.push_str(&format!( "--{boundary}\r\n\ Content-Type: {mime_type}\r\n\ @@ -666,7 +641,7 @@ impl MessageBuilder<'_> { \r\n\ {encoded}\r\n", mime_type = att.mime_type, - filename = sanitize_header_value(&att.filename), + filename = safe_filename, )); } diff --git a/src/helpers/gmail/send.rs b/src/helpers/gmail/send.rs index bbd99a80..08b09129 100644 --- a/src/helpers/gmail/send.rs +++ b/src/helpers/gmail/send.rs @@ -7,29 +7,21 @@ pub(super) async fn handle_send( ) -> Result<(), GwsError> { let config = parse_send_args(matches)?; + 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() { - 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) + builder.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) + builder.build_with_attachments(&config.body, &attachments) }; super::send_raw_email(doc, matches, &raw, None, None).await @@ -70,9 +62,9 @@ fn parse_send_args(matches: &ArgMatches) -> Result { /// Validate that an attachment path is safe to read. /// -/// Rejects paths with control characters, null bytes, or path traversal -/// segments (`..`). The file must exist and be a regular file (not a -/// directory or device). +/// Rejects absolute paths, paths with control characters, and paths that +/// resolve (after following symlinks) to a location outside the current +/// working directory. The file must exist and be a regular file. fn validate_attachment_path(path: &PathBuf) -> Result<(), GwsError> { let path_str = path.to_string_lossy(); @@ -84,24 +76,39 @@ fn validate_attachment_path(path: &PathBuf) -> Result<(), GwsError> { ))); } - // 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 - ))); - } + // Reject absolute paths — force CWD-relative access + if path.is_absolute() { + return Err(GwsError::Validation(format!( + "--attachment path must be relative: {}", + path_str + ))); } - // File must exist and be a regular file - if !path.exists() { + // Resolve symlinks and normalize the path (also checks 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(|e| { + GwsError::Validation(format!("Failed to determine current directory: {e}")) + })?; + let canonical_cwd = cwd.canonicalize().map_err(|e| { + GwsError::Validation(format!("Failed to canonicalize current directory: {e}")) + })?; + + if !canonical_path.starts_with(&canonical_cwd) { return Err(GwsError::Validation(format!( - "--attachment file not found: {}", + "--attachment path '{}' resolves outside the current directory", path_str ))); } - if !path.is_file() { + + // Must be a regular file (not a directory or device) + if !canonical_path.is_file() { return Err(GwsError::Validation(format!( "--attachment path is not a file: {}", path_str @@ -297,28 +304,44 @@ mod tests { } #[test] - fn test_validate_attachment_path_rejects_traversal() { - let path = PathBuf::from("../../etc/passwd"); + fn test_validate_attachment_path_rejects_missing_file() { + let path = PathBuf::from("nonexistent_file_12345.pdf"); let result = validate_attachment_path(&path); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); - assert!(msg.contains("traversal"), "got: {msg}"); + assert!( + msg.contains("Failed to resolve") || msg.contains("not found"), + "got: {msg}" + ); } #[test] - fn test_validate_attachment_path_rejects_missing_file() { - let path = PathBuf::from("nonexistent_file_12345.pdf"); + fn test_validate_attachment_path_rejects_absolute() { + let path = if cfg!(windows) { + PathBuf::from("C:\\Windows\\System32\\notepad.exe") + } else { + PathBuf::from("/etc/passwd") + }; let result = validate_attachment_path(&path); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); - assert!(msg.contains("not found"), "got: {msg}"); + assert!(msg.contains("must be relative"), "got: {msg}"); } #[test] + #[serial_test::serial] fn test_validate_attachment_path_rejects_directory() { let dir = tempdir().unwrap(); - let path = dir.path().to_path_buf(); - let result = validate_attachment_path(&path); + let canonical_dir = dir.path().canonicalize().unwrap(); + let sub = canonical_dir.join("subdir"); + fs::create_dir(&sub).unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + + let result = validate_attachment_path(&PathBuf::from("subdir")); + std::env::set_current_dir(&saved_cwd).unwrap(); + assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!(msg.contains("not a file"), "got: {msg}"); @@ -333,6 +356,23 @@ mod tests { assert!(msg.contains("control characters"), "got: {msg}"); } + #[test] + #[serial_test::serial] + fn test_validate_attachment_path_accepts_valid_file() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + let file = canonical_dir.join("valid.pdf"); + fs::write(&file, b"data").unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + + let result = validate_attachment_path(&PathBuf::from("valid.pdf")); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_ok(), "expected Ok, got: {result:?}"); + } + #[test] fn test_load_attachments() { let dir = tempdir().unwrap(); @@ -373,6 +413,31 @@ mod tests { assert!(raw.contains("Content-Transfer-Encoding: base64")); } + #[test] + fn test_build_with_attachments_escapes_filename_quotes() { + let attachment = super::super::Attachment { + filename: "my\"file.pdf".to_string(), + mime_type: "application/pdf".to_string(), + data: b"data".to_vec(), + }; + + let raw = MessageBuilder { + to: "bob@example.com", + subject: "Test", + from: None, + cc: None, + bcc: None, + threading: None, + html: false, + } + .build_with_attachments("Body.", &[attachment]); + + assert!( + raw.contains("filename=\"my\\\"file.pdf\""), + "quotes in filename should be escaped: {raw}" + ); + } + #[test] fn test_build_with_empty_attachments_falls_back() { let raw = MessageBuilder { From f688f05bbccf7f0d34298adc29b3caed2886d86f Mon Sep 17 00:00:00 2001 From: Alim Polat Date: Tue, 17 Mar 2026 01:24:54 +0100 Subject: [PATCH 3/5] chore: retrigger CLA check From 1bc29171d754ce16dd27991813d06bd6452fdbfb Mon Sep 17 00:00:00 2001 From: Alim Polat Date: Tue, 17 Mar 2026 07:11:48 +0100 Subject: [PATCH 4/5] fix: use relative paths in attachment test to match validation --- src/helpers/gmail/send.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/helpers/gmail/send.rs b/src/helpers/gmail/send.rs index 08b09129..36b8b6a4 100644 --- a/src/helpers/gmail/send.rs +++ b/src/helpers/gmail/send.rs @@ -276,15 +276,15 @@ mod tests { } #[test] + #[serial_test::serial] fn test_parse_send_args_with_attachments() { let dir = tempdir().unwrap(); - let file1 = dir.path().join("doc.pdf"); - let file2 = dir.path().join("image.png"); - fs::write(&file1, b"fake pdf").unwrap(); - fs::write(&file2, b"fake png").unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + fs::write(canonical_dir.join("doc.pdf"), b"fake pdf").unwrap(); + fs::write(canonical_dir.join("image.png"), b"fake png").unwrap(); - let f1 = file1.to_string_lossy().to_string(); - let f2 = file2.to_string_lossy().to_string(); + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); let matches = make_matches_send(&[ "test", @@ -295,11 +295,13 @@ mod tests { "--body", "See attached", "--attachment", - &f1, + "doc.pdf", "--attachment", - &f2, + "image.png", ]); let config = parse_send_args(&matches).unwrap(); + std::env::set_current_dir(&saved_cwd).unwrap(); + assert_eq!(config.attachments.len(), 2); } From f3d0fa7ad9811e970d641227f23e7cb5767d2e2c Mon Sep 17 00:00:00 2001 From: Alim Polat Date: Tue, 17 Mar 2026 07:20:47 +0100 Subject: [PATCH 5/5] fix: address remaining review feedback - 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 --- src/helpers/gmail/mod.rs | 114 +++++++++++++++++++++++++++++++++++--- src/helpers/gmail/send.rs | 60 +++++++++++++++++--- 2 files changed, 159 insertions(+), 15 deletions(-) diff --git a/src/helpers/gmail/mod.rs b/src/helpers/gmail/mod.rs index 2d249691..e69b55da 100644 --- a/src/helpers/gmail/mod.rs +++ b/src/helpers/gmail/mod.rs @@ -627,21 +627,15 @@ impl MessageBuilder<'_> { // Attachment parts for att in attachments { let encoded = base64::engine::general_purpose::STANDARD.encode(&att.data); - // Escape backslashes and quotes per RFC 2183 to prevent - // malformed Content-Disposition headers from filenames like: - // my"file.txt → my\"file.txt - let safe_filename = sanitize_header_value(&att.filename) - .replace('\\', "\\\\") - .replace('"', "\\\""); + let disposition = encode_content_disposition(&att.filename); 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\ + {disposition}\r\n\ \r\n\ {encoded}\r\n", mime_type = att.mime_type, - filename = safe_filename, )); } @@ -704,6 +698,55 @@ pub(super) fn mime_type_from_extension(filename: &str) -> &'static str { } } +/// Build a Content-Disposition header for an attachment. +/// +/// For ASCII-only filenames, uses the simple `filename="..."` form with +/// backslash/quote escaping per RFC 2183. +/// +/// For filenames containing non-ASCII characters, adds an RFC 2231/5987 +/// `filename*` parameter with UTF-8 percent-encoding so that international +/// filenames display correctly in email clients. +fn encode_content_disposition(filename: &str) -> String { + let sanitized = sanitize_header_value(filename); + let is_ascii = sanitized.bytes().all(|b| b.is_ascii() && b != b'\0'); + + if is_ascii { + // Simple form: escape backslashes and quotes + let escaped = sanitized.replace('\\', "\\\\").replace('"', "\\\""); + format!("Content-Disposition: attachment; filename=\"{escaped}\"") + } else { + // RFC 2231: filename*=UTF-8''percent-encoded-name + // Also include a plain ASCII fallback for older clients. + let ascii_fallback = sanitized + .chars() + .map(|c| if c.is_ascii() && c != '"' && c != '\\' { c } else { '_' }) + .collect::(); + let encoded = percent_encode_filename(&sanitized); + format!( + "Content-Disposition: attachment; filename=\"{ascii_fallback}\"; \ + filename*=UTF-8''{encoded}" + ) + } +} + +/// Percent-encode a filename for RFC 2231 `filename*` parameter. +/// Encodes all non-ASCII bytes and RFC 5987 attr-char special characters. +fn percent_encode_filename(s: &str) -> String { + let mut out = String::with_capacity(s.len() * 3); + for byte in s.bytes() { + // RFC 5987 attr-char allows: ALPHA DIGIT ! # $ & + - . ^ _ ` | ~ + if byte.is_ascii_alphanumeric() + || matches!(byte, b'!' | b'#' | b'$' | b'&' | b'+' | b'-' + | b'.' | b'^' | b'_' | b'`' | b'|' | b'~') + { + out.push(byte as char); + } else { + out.push_str(&format!("%{:02X}", byte)); + } + } + out +} + /// Build the References header value. Returns just the message ID when there /// are no prior references, or appends it to the existing chain. pub(super) fn build_references(original_references: &str, original_message_id: &str) -> String { @@ -2111,4 +2154,59 @@ mod tests { assert_ne!(b1, b2); assert!(b1.starts_with("gws_")); } + + // --- Content-Disposition encoding tests --- + + #[test] + fn test_encode_content_disposition_ascii() { + let header = encode_content_disposition("report.pdf"); + assert_eq!( + header, + "Content-Disposition: attachment; filename=\"report.pdf\"" + ); + } + + #[test] + fn test_encode_content_disposition_escapes_quotes() { + let header = encode_content_disposition("my\"file.pdf"); + assert!(header.contains("filename=\"my\\\"file.pdf\""), "got: {header}"); + } + + #[test] + fn test_encode_content_disposition_escapes_backslash() { + let header = encode_content_disposition("path\\file.pdf"); + assert!(header.contains("filename=\"path\\\\file.pdf\""), "got: {header}"); + } + + #[test] + fn test_encode_content_disposition_non_ascii_uses_rfc2231() { + let header = encode_content_disposition("résumé.pdf"); + // Should have both ASCII fallback and RFC 2231 filename* + assert!(header.contains("filename=\"r_sum_.pdf\""), "missing ASCII fallback: {header}"); + assert!(header.contains("filename*=UTF-8''r%C3%A9sum%C3%A9.pdf"), "missing RFC 2231: {header}"); + } + + #[test] + fn test_encode_content_disposition_swedish_chars() { + let header = encode_content_disposition("ärende_åtgärd.pdf"); + assert!(header.contains("filename*=UTF-8''"), "should use RFC 2231 for Swedish chars: {header}"); + assert!(header.contains("%C3%A4"), "should encode ä: {header}"); + assert!(header.contains("%C3%A5"), "should encode å: {header}"); + } + + #[test] + fn test_percent_encode_filename_ascii() { + assert_eq!(percent_encode_filename("report.pdf"), "report.pdf"); + } + + #[test] + fn test_percent_encode_filename_spaces() { + assert_eq!(percent_encode_filename("my report.pdf"), "my%20report.pdf"); + } + + #[test] + fn test_percent_encode_filename_unicode() { + let encoded = percent_encode_filename("résumé.pdf"); + assert_eq!(encoded, "r%C3%A9sum%C3%A9.pdf"); + } } diff --git a/src/helpers/gmail/send.rs b/src/helpers/gmail/send.rs index 36b8b6a4..c243dc9d 100644 --- a/src/helpers/gmail/send.rs +++ b/src/helpers/gmail/send.rs @@ -44,10 +44,8 @@ fn parse_send_args(matches: &ArgMatches) -> Result { .map(|s| PathBuf::from(s.trim())) .collect(); - // Validate each attachment path - for path in &attachments { - validate_attachment_path(path)?; - } + // Path validation is deferred to load_attachments() to minimize the + // TOCTOU window — validate immediately before reading. Ok(SendConfig { to: matches.get_one::("to").unwrap().to_string(), @@ -119,10 +117,16 @@ fn validate_attachment_path(path: &PathBuf) -> Result<(), GwsError> { } /// Read attachment files from disk and prepare them for MIME encoding. +/// +/// Validation is performed immediately before reading each file to minimize +/// the TOCTOU (time-of-check to time-of-use) window. fn load_attachments(paths: &[PathBuf]) -> Result, GwsError> { let mut attachments = Vec::with_capacity(paths.len()); for path in paths { + // Validate right before reading to minimize TOCTOU window + validate_attachment_path(path)?; + let data = std::fs::read(path).map_err(|e| { GwsError::Validation(format!( "Failed to read attachment '{}': {}", @@ -376,12 +380,18 @@ mod tests { } #[test] + #[serial_test::serial] fn test_load_attachments() { let dir = tempdir().unwrap(); - let file = dir.path().join("report.pdf"); - fs::write(&file, b"PDF content here").unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + fs::write(canonical_dir.join("report.pdf"), b"PDF content here").unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + + let attachments = load_attachments(&[PathBuf::from("report.pdf")]).unwrap(); + std::env::set_current_dir(&saved_cwd).unwrap(); - let attachments = load_attachments(&[file]).unwrap(); assert_eq!(attachments.len(), 1); assert_eq!(attachments[0].filename, "report.pdf"); assert_eq!(attachments[0].mime_type, "application/pdf"); @@ -440,6 +450,42 @@ mod tests { ); } + #[test] + fn test_build_with_attachments_non_ascii_filename() { + let attachment = super::super::Attachment { + filename: "résumé.pdf".to_string(), + mime_type: "application/pdf".to_string(), + data: b"data".to_vec(), + }; + + let raw = MessageBuilder { + to: "bob@example.com", + subject: "Test", + from: None, + cc: None, + bcc: None, + threading: None, + html: false, + } + .build_with_attachments("Body.", &[attachment]); + + // Should have RFC 2231 filename* parameter + assert!( + raw.contains("filename*=UTF-8''"), + "non-ASCII filename should use RFC 2231 encoding: {raw}" + ); + // Should have ASCII fallback + assert!( + raw.contains("filename=\"r_sum_.pdf\""), + "should include ASCII fallback filename: {raw}" + ); + // Should have percent-encoded UTF-8 + assert!( + raw.contains("r%C3%A9sum%C3%A9.pdf"), + "should percent-encode non-ASCII bytes: {raw}" + ); + } + #[test] fn test_build_with_empty_attachments_falls_back() { let raw = MessageBuilder {