New function forward_gmail_message (with tests)#455
Conversation
Extract _forward_gmail_message_impl for testability and add 8 unit tests covering text/HTML forwarding, attachments, CC/BCC, and subject handling.
jcwatson11
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a testable internal implementation for forwarding Gmail messages and introduces unit tests to validate forwarding behavior across common scenarios (plain/HTML, optional message, attachments, CC/BCC, subject prefixing).
Changes:
- Extracted forwarding logic into
_forward_gmail_message_implto enable direct unit testing. - Implemented forward construction for plain/HTML, optional prepended message, and optional attachments.
- Added 8 async unit tests covering core forward scenarios.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
gmail/gmail_tools.py |
Introduces _forward_gmail_message_impl and wires forward_gmail_message to it for testability and feature coverage (HTML + attachments). |
tests/gmail/test_forward_gmail_message.py |
Adds new pytest async unit tests for forwarding behavior using mocked Gmail service chain calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| forward_header_html = ( | ||
| '<div style="color: #777;">' | ||
| "---------- Forwarded message ---------<br/>" | ||
| f"From: {original_from}<br/>" | ||
| f"Date: {original_date}<br/>" | ||
| f"Subject: {original_subject}<br/>" | ||
| f"To: {original_to}" | ||
| "</div>" | ||
| ) |
There was a problem hiding this comment.
In the HTML forward path, header values (original_from, original_subject, etc.) and forward_message when forward_message_format == 'html' are interpolated directly into HTML without escaping/sanitization. This can break the generated markup and can also allow HTML/script injection if any of these values contain markup. Consider HTML-escaping header values, and either sanitizing forward_message (if you intend to allow HTML input) or treating all forward_message content as plain text and escaping it consistently.
| if forward_message: | ||
| if forward_message_format == "html": | ||
| user_message_html = f"<div>{forward_message}</div><br/>" |
There was a problem hiding this comment.
In the HTML forward path, header values (original_from, original_subject, etc.) and forward_message when forward_message_format == 'html' are interpolated directly into HTML without escaping/sanitization. This can break the generated markup and can also allow HTML/script injection if any of these values contain markup. Consider HTML-escaping header values, and either sanitizing forward_message (if you intend to allow HTML input) or treating all forward_message content as plain text and escaping it consistently.
| urlsafe_data = attachment_data.get("data", "") | ||
| # Convert from URL-safe to standard base64 | ||
| standard_b64 = urlsafe_data.replace("-", "+").replace("_", "/") | ||
| attachments_to_send.append( | ||
| { | ||
| "content": standard_b64, | ||
| "filename": att["filename"], | ||
| "mime_type": att["mimeType"], | ||
| } | ||
| ) |
There was a problem hiding this comment.
Gmail attachment data is URL-safe base64 and is commonly returned without padding. Replacing -/_ is not sufficient: base64.b64decode() (used later in _prepare_gmail_message) can raise Incorrect padding for unpadded input. A robust fix is to normalize by adding required = padding (length multiple of 4), or decode with base64.urlsafe_b64decode() and then re-encode to standard base64 before passing into _prepare_gmail_message.
| forward_subject = original_subject | ||
| if not forward_subject.lower().startswith("fwd:"): | ||
| forward_subject = f"Fwd: {original_subject}" |
There was a problem hiding this comment.
The double-prefix guard only checks for fwd:. Subjects often use variants like FW: / FWD / localized equivalents. With the current logic, a subject like FW: Something will become Fwd: FW: Something, which conflicts with the stated intent of avoiding double-prefixing. Consider expanding the detection (e.g., accept fw: and common fwd variants, potentially allowing optional spaces).
| import sys | ||
| import os | ||
|
|
||
| sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))) | ||
|
|
There was a problem hiding this comment.
Modifying sys.path inside tests is brittle and can mask packaging/import issues (and can behave differently depending on how tests are invoked). Prefer configuring imports via the test runner/environment (e.g., pyproject.toml/pytest settings, editable install, or a conftest.py that configures the project root once) so the tests run consistently in CI and local environments.
| import sys | |
| import os | |
| sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))) |
Summary
_forward_gmail_message_implfor testabilityTest plan
uv run pytest tests/gmail/test_forward_gmail_message.py -v)