Merge pull request #774 from /issues/772

fix: windows filename chars

Author: taylorwilsdon

core/attachment_storage.py

@@ -8,6 +8,7 @@
 import base64
 import logging
 import os
+import re
 import uuid
 from pathlib import Path
 from typing import NamedTuple, Optional, Dict
@@ -25,12 +26,38 @@
     Path(os.getenv("WORKSPACE_ATTACHMENT_DIR", _default_dir)).expanduser().resolve()
 )
 
+_WINDOWS_RESERVED_FILENAME_CHARS = re.compile(r'[<>:"/\\|?*\x00-\x1f]')
+_WINDOWS_RESERVED_NAMES = {
+    "CON",
+    "PRN",
+    "AUX",
+    "NUL",
+    *(f"COM{i}" for i in range(1, 10)),
+    *(f"LPT{i}" for i in range(1, 10)),
+}
+
 
 def _ensure_storage_dir() -> None:
     """Create the storage directory on first use, not at import time."""
     STORAGE_DIR.mkdir(parents=True, exist_ok=True, mode=0o700)
 
 
+def sanitize_attachment_filename(filename: Optional[str]) -> str:
+    """Return a filesystem-safe attachment filename."""
+    if not filename:
+        return "attachment"
+
+    sanitized = _WINDOWS_RESERVED_FILENAME_CHARS.sub("_", filename).rstrip(" .")
+    if not sanitized:
+        return "attachment"
+
+    stem = sanitized.split(".", 1)[0]
+    if stem.upper() in _WINDOWS_RESERVED_NAMES:
+        sanitized = f"_{sanitized}"
+
+    return sanitized
+
+
 class SavedAttachment(NamedTuple):
     """Result of saving an attachment: provides both the UUID and the absolute file path."""
 
@@ -76,8 +103,10 @@ def save_attachment(
 
         # Determine file extension from filename or mime type
         extension = ""
+        safe_filename = sanitize_attachment_filename(filename)
+
         if filename:
-            extension = Path(filename).suffix
+            extension = Path(safe_filename).suffix
         elif mime_type:
             # Basic mime type to extension mapping
             mime_to_ext = {
@@ -93,8 +122,8 @@ def save_attachment(
 
         # Use original filename if available, with UUID suffix for uniqueness
         if filename:
-            stem = Path(filename).stem
-            ext = Path(filename).suffix
+            stem = Path(safe_filename).stem
+            ext = Path(safe_filename).suffix
             save_name = f"{stem}_{file_id[:8]}{ext}"
         else:
             save_name = f"{file_id}{extension}"
@@ -134,7 +163,8 @@ def save_attachment(
         expires_at = datetime.now() + timedelta(seconds=self.expiration_seconds)
         self._metadata[file_id] = {
             "file_path": str(file_path),
-            "filename": filename or f"attachment{extension}",
+            "filename": save_name,
+            "original_filename": filename,
             "mime_type": mime_type or "application/octet-stream",
             "size": len(file_bytes),
             "created_at": datetime.now(),

gmail/gmail_tools.py

@@ -1794,11 +1794,13 @@ async def get_gmail_attachment_content(
         result = storage.save_attachment(
             base64_data=base64_data, filename=filename, mime_type=mime_type
         )
+        saved_filename = Path(result.path).name
 
         result_lines = [
             "Attachment downloaded successfully!",
             f"Message ID: {message_id}",
             f"Filename: {filename or 'unknown'}",
+            f"Saved filename: {saved_filename}",
             f"Size: {size_kb:.1f} KB ({size_bytes} bytes)",
         ]
 

tests/gmail/test_attachment_fix.py

@@ -82,6 +82,55 @@ def test_save_attachment_uses_binary_mode(isolated_storage):
     )
 
 
+@pytest.mark.parametrize(
+    ("filename", "expected_prefix"),
+    [
+        ("RE: Foo/Bar?.eml", "RE_ Foo_Bar_"),
+        ("FW: Client\\Matter*.eml", "FW_ Client_Matter_"),
+        ("CON.txt", "_CON_"),
+        ("report. ", "report_"),
+        ("...   ", "attachment_"),
+    ],
+)
+def test_save_attachment_sanitizes_windows_reserved_filenames(
+    isolated_storage, filename, expected_prefix
+):
+    """Attachment filenames should be safe on Windows and POSIX filesystems."""
+    payload = b"safe filename check"
+    b64_data = base64.urlsafe_b64encode(payload).decode()
+
+    result = isolated_storage.save_attachment(b64_data, filename=filename)
+    saved_name = os.path.basename(result.path)
+
+    assert saved_name.startswith(expected_prefix)
+    assert not any(char in saved_name for char in '<>:"/\\|?*')
+    assert saved_name == saved_name.rstrip(". ")
+
+    with open(result.path, "rb") as f:
+        saved_bytes = f.read()
+
+    assert saved_bytes == payload
+
+
+def test_save_attachment_metadata_filename_matches_saved_file(isolated_storage):
+    """Attachment metadata should report the on-disk filename."""
+    payload = b"metadata filename check"
+    b64_data = base64.urlsafe_b64encode(payload).decode()
+
+    result = isolated_storage.save_attachment(
+        b64_data, filename="RE: Foo?.eml", mime_type="message/rfc822"
+    )
+    saved_name = os.path.basename(result.path)
+    metadata = isolated_storage.get_attachment_metadata(result.file_id)
+
+    assert metadata["filename"] == saved_name
+    assert metadata["original_filename"] == "RE: Foo?.eml"
+    assert metadata["filename"].startswith("RE_ Foo_")
+    assert metadata["filename"].endswith(".eml")
+    assert ":" not in metadata["filename"]
+    assert "?" not in metadata["filename"]
+
+
 @pytest.mark.parametrize(
     "payload",
     [

tests/gmail/test_get_gmail_attachment_content.py

@@ -134,6 +134,34 @@ async def test_default_call_omits_base64_content(isolated_attachment_env):
     assert "standard base64" not in result
 
 
+@pytest.mark.asyncio
+async def test_download_response_reports_sanitized_saved_filename(
+    isolated_attachment_env,
+):
+    """Windows-reserved filename characters should be sanitized before saving."""
+    payload = b"attached email bytes"
+    mock_service = _build_mock_service(
+        payload, filename="RE: Foo?.eml", mime_type="message/rfc822"
+    )
+
+    result = await _unwrap(get_gmail_attachment_content)(
+        service=mock_service,
+        message_id="msg-1",
+        attachment_id="att-123",
+        user_google_email="user@example.com",
+    )
+
+    assert "Filename: RE: Foo?.eml" in result
+    assert "Saved filename: RE_ Foo_" in result
+
+    saved_files = list(isolated_attachment_env.iterdir())
+    assert len(saved_files) == 1
+    assert saved_files[0].name.startswith("RE_ Foo_")
+    assert ":" not in saved_files[0].name
+    assert "?" not in saved_files[0].name
+    assert saved_files[0].read_bytes() == payload
+
+
 @pytest.mark.asyncio
 @pytest.mark.parametrize(
     ("payload", "filename", "mime_type"),