Send emails with attachments - #381
Conversation
- Add export_format='pdf' option for Google Sheets in get_drive_file_download_url - Previously only supported csv and xlsx formats for Sheets - Update docstring and parameter documentation - Tested with production Google Sheet and confirmed working
- Add file path and base64 content attachment support - Auto-detect MIME types from file extensions - Support attachments in both send_gmail_message and draft_gmail_message - Accept attachments via 'path' (file path) or 'content' (base64) + 'filename' - Note: FastMCP schema generation issue prevents List[Dict] from appearing in MCP tool schema
There was a problem hiding this comment.
Pull request overview
This PR adds attachment support to Gmail operations and extends Google Sheets export capabilities to include PDF format.
Changes:
- Added attachment support for sending Gmail messages and creating drafts, supporting both file paths (auto-encoded) and base64 content
- Extended Google Sheets export to support PDF format in addition to existing XLSX and CSV options
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| gmail/gmail_tools.py | Added attachment handling logic with support for file paths and base64 content, updated function signatures and documentation for send_gmail_message and draft_gmail_message |
| gdrive/drive_tools.py | Added PDF export support for Google Sheets in get_drive_file_download_url |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| references: Optional[str] = Body( | ||
| None, description="Optional chain of Message-IDs for proper threading." | ||
| ), | ||
| attachments_json: Optional[str] = Body( |
There was a problem hiding this comment.
Parameter is named attachments_json but the actual parameter used in the function body is attachments (line 1077, 1092, 1107-1108). The parameter name should match what's used throughout the function.
| attachments_json: Optional[str] = Body( | |
| attachments: Optional[str] = Body( |
| subject (str): Email subject. | ||
| body (str): Email body content. | ||
| body_format (Literal['plain', 'html']): Email body format. Defaults to 'plain'. | ||
| attachments (List[Dict[str, str]]): Optional list of attachments. Each dict can contain: |
There was a problem hiding this comment.
Documentation describes a parameter named attachments, but the function signature declares attachments_json. These names should be consistent.
| attachments (List[Dict[str, str]]): Optional list of attachments. Each dict can contain: | |
| attachments_json (Optional[str]): Optional JSON array of attachments. Each element is a dict that can contain: |
| """ | ||
| logger.info( | ||
| f"[send_gmail_message] Invoked. Email: '{user_google_email}', Subject: '{subject}'" | ||
| f"[send_gmail_message] Invoked. Email: '{user_google_email}', Subject: '{subject}', Attachments: {len(attachments)}" |
There was a problem hiding this comment.
This will raise an error when attachments is None. The code attempts to call len() on a potentially None value. Should check if attachments exists before calling len(), e.g., len(attachments) if attachments else 0.
| f"[send_gmail_message] Invoked. Email: '{user_google_email}', Subject: '{subject}', Attachments: {len(attachments)}" | |
| f"[send_gmail_message] Invoked. Email: '{user_google_email}', Subject: '{subject}', Attachments: {len(attachments) if attachments else 0}" |
| part.add_header( | ||
| "Content-Disposition", f"attachment; filename={filename}" |
There was a problem hiding this comment.
The filename should be properly quoted and sanitized in the Content-Disposition header to prevent header injection attacks. Use f'attachment; filename=\"{filename}\"' with proper escaping or use email.utils.encode_rfc2231().
| part.add_header( | |
| "Content-Disposition", f"attachment; filename={filename}" | |
| # Sanitize filename to prevent header injection and ensure valid quoting | |
| safe_filename = (filename or "").replace("\r", "").replace("\n", "") | |
| safe_filename = safe_filename.replace("\\", "\\\\").replace('"', r"\"") | |
| part.add_header( | |
| "Content-Disposition", f'attachment; filename="{safe_filename}"' |
| attachments: List[Dict[str, str]] = Body( | ||
| default_factory=list, |
There was a problem hiding this comment.
Using default_factory=list creates a mutable default argument. This should be Body(default=None) or use Optional[List[Dict[str, str]]] with None as default, similar to the pattern used in send_gmail_message.
| attachments: List[Dict[str, str]] = Body( | |
| default_factory=list, | |
| attachments: Optional[List[Dict[str, str]]] = Body( | |
| default=None, |
|
Hi! Really looking forward to this feature. We're using google_workspace_mcp extensively for email automation via Claude Code and attachment support would be a great addition. Thanks for the implementation @songmeo! 👍 |
|
Thank you, @martinschenk. I’ve been using my fork with this feature for the past week, and it’s been working well for us. Looking forward to the code review. |
…into songmeo/main
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.warning("Skipping attachment: missing filename") | ||
| continue | ||
|
|
||
| file_data = base64.urlsafe_b64decode(content_base64) |
There was a problem hiding this comment.
Using urlsafe_b64decode for user-provided base64 content is incorrect. The Gmail API uses urlsafe_b64encode for the final message encoding, but attachment content from users should be decoded using standard base64.b64decode. Use base64.b64decode(content_base64) instead to handle standard base64-encoded attachment content.
| file_data = base64.urlsafe_b64decode(content_base64) | |
| file_data = base64.b64decode(content_base64) |
| continue | ||
|
|
||
| # Create MIME attachment | ||
| part = MIMEBase(*mime_type.split("/")) |
There was a problem hiding this comment.
If mime_type contains more than one '/' character (though uncommon, it's possible with malformed input), split('/') will produce more than 2 elements, causing MIMEBase to receive too many arguments. Use mime_type.split('/', 1) to ensure only two parts (maintype and subtype) are extracted.
| part = MIMEBase(*mime_type.split("/")) | |
| main_type, sub_type = mime_type.split("/", 1) | |
| part = MIMEBase(main_type, sub_type) |
| safe_filename = (filename or "").replace("\r", "").replace("\n", "") | ||
| safe_filename = safe_filename.replace("\\", "\\\\").replace('"', r"\"") |
There was a problem hiding this comment.
The filename sanitization logic is duplicated as separate steps. Consider consolidating into a single chain or extracting to a helper function for better maintainability, especially if this sanitization logic needs to be updated or reused.
| safe_filename = (filename or "").replace("\r", "").replace("\n", "") | |
| safe_filename = safe_filename.replace("\\", "\\\\").replace('"', r"\"") | |
| safe_filename = ( | |
| (filename or "") | |
| .replace("\r", "") | |
| .replace("\n", "") | |
| .replace("\\", "\\\\") | |
| .replace('"', r"\"") | |
| ) |
| ), | ||
| attachments: Optional[List[Dict[str, str]]] = Body( | ||
| None, | ||
| description='Optional list of attachments. Each can have: "path" (file path, auto-encodes), OR "content" (base64) + "filename". Optional "mime_type". Example: [{"path": "/path/to/file.pdf"}] or [{"filename": "doc.pdf", "content": "base64data", "mime_type": "application/pdf"}]', |
There was a problem hiding this comment.
The description should clarify that 'content' expects standard base64 encoding (not urlsafe). This helps API consumers understand the expected format for base64-encoded attachments.
Fix #375