fix: sanitize bugzilla attachment paths

pyproject.toml

@@ -31,6 +31,7 @@ dependencies = [
   "autobisect>=9.2,<10",
   "bugsy @ git+https://github.com/MozillaSecurity/Bugsy.git",
   "fuzzfetch>=16.1,<17",
+  "pathvalidate>=3,<4",
   "typing-extensions>=4.2,<5",
 ]
 urls.Homepage = "https://github.com/MozillaSecurity/bugmon"

src/bugmon/bugmon.py

@@ -27,6 +27,7 @@
     PernoscoCreds,
     get_pernosco_trace,
     is_pernosco_available,
+    sanitize_attachment_path,
     submit_pernosco,
 )
 
@@ -501,15 +502,22 @@ def fetch_attachments(self, unpack: Optional[bool] = True) -> None:
                 try:
                     with zipfile.ZipFile(io.BytesIO(data)) as z:
                         for filename in z.namelist():
-                            if (self.test_dir / filename).exists():
-                                log.warning("Duplicate filename: %s", filename)
-                            z.extract(filename, self.test_dir)
+                            # Skip directory entries
+                            if filename.endswith("/"):
+                                continue
+                            dest = sanitize_attachment_path(self.test_dir, filename)
+                            if dest.exists():
+                                log.warning("Duplicate filename: %s", dest)
+                            dest.parent.mkdir(parents=True, exist_ok=True)
+                            dest.write_bytes(z.read(filename))
                 except zipfile.BadZipFile as e:
                     log.warning("Failed to decompress attachment: %s", e)
                     continue
 
             else:
-                Path(self.test_dir, attachment.file_name).write_bytes(data)
+                dest = sanitize_attachment_path(self.test_dir, attachment.file_name)
+                dest.parent.mkdir(parents=True, exist_ok=True)
+                dest.write_bytes(data)
 
     def needs_bisect(self) -> bool:
         """Helper function to determine eligibility for 'bisect'"""

src/bugmon/utils.py

@@ -8,7 +8,7 @@
 import tempfile
 import zipfile
 from contextlib import contextmanager
-from pathlib import Path, PurePosixPath
+from pathlib import Path, PurePosixPath, PureWindowsPath
 from urllib.parse import urlparse
 
 try:
@@ -18,6 +18,7 @@
     from typing_extensions import TypedDict
 
 import requests
+from pathvalidate import sanitize_filename
 from requests.adapters import HTTPAdapter, Retry
 from requests.models import Response
 
@@ -36,6 +37,46 @@
 log = logging.getLogger(__name__)
 
 
+def sanitize_attachment_path(base_dir: Path, file_name: str) -> Path:
+    """Build a sanitized destination path for an attachment within base_dir.
+
+    Each path component is sanitized individually with "universal" rules so
+    that characters grizzly's TestCase loader rejects (e.g. ':') are stripped
+    everywhere - including the leading component, which sanitize_filepath would
+    otherwise preserve as a Windows drive letter. Subdirectory structure is
+    kept, while paths containing traversal ('..'), current-dir ('.'), or
+    absolute ('/') components raise ValueError.
+
+    :param base_dir: Directory the attachment should be written to.
+    :param file_name: Original attachment or archive member name.
+    :return: Sanitized path within base_dir.
+    :raises ValueError: If file_name is empty, absolute, contains path traversal,
+        or has a component without a valid sanitized name.
+    """
+
+    displayed_name = repr(file_name)
+    if len(displayed_name) > 100:
+        displayed_name = f"{displayed_name[:97]}..."
+
+    windows_path = PureWindowsPath(file_name)
+    raw_parts = [part for part in file_name.replace("\\", "/").split("/") if part]
+    if (
+        windows_path.drive
+        or windows_path.root
+        or not raw_parts
+        or any(part in (".", "..") for part in raw_parts)
+    ):
+        raise ValueError(f"Unsafe attachment path: {displayed_name}")
+
+    # Sanitization can normalize names like ".. " into "..", so the sanitized
+    # components must be re-checked before they are used.
+    parts = [sanitize_filename(part, platform="universal") for part in raw_parts]
+    if any(part in ("", ".", "..") for part in parts):
+        raise ValueError(f"Unsafe attachment path: {displayed_name}")
+
+    return base_dir.joinpath(*parts)
+
+
 class PernoscoCreds(TypedDict):
     """Interface representing required pernosco creds"""
 

tests/test_bugmon.py

@@ -2,6 +2,10 @@
 # This Source Code Form is subject to the terms of the Mozilla Public License,
 # v. 2.0. If a copy of the MPL was not distributed with this file, You can
 # obtain one at http://mozilla.org/MPL/2.0/.
+import base64
+import io
+import zipfile
+
 import pytest
 from autobisect.bisect import BisectionResult
 from fuzzfetch import Platform
@@ -11,6 +15,16 @@
 from bugmon.exceptions import BugmonException
 
 
+def _attachment(file_name, data, creation_time="2020-06-30T12:40:45Z"):
+    attachment = type("Attachment", (), {})()
+    attachment.file_name = file_name
+    attachment.data = base64.b64encode(data).decode()
+    attachment.creation_time = creation_time
+    attachment.content_type = "text/plain"
+    attachment.is_obsolete = False
+    return attachment
+
+
 def test_bugmon_need_info_on_bisect_fix(mocker, bugmon, build):
     """Test that the assignee is NI'd when the testcase no longer reproduces"""
     mocker.patch.object(bugmon, "detect_config", return_value=True)
@@ -46,6 +60,41 @@ def test_bugmon_throws_without_pernosco_submit(
         BugMonitor(bugsy, bug, working_dir, pernosco_creds, False)
 
 
+def test_bugmon_fetch_attachments_preserves_nested_paths(mocker, bugmon):
+    """Verify valid ZIP and regular attachments preserve nested paths."""
+    archive_data = io.BytesIO()
+    with zipfile.ZipFile(archive_data, "w") as archive:
+        archive.writestr("nested/test.js", b"nested")
+        archive.writestr("nested/", b"")
+
+    attachments = [
+        _attachment("attachments.zip", archive_data.getvalue()),
+        _attachment("other/regular.js", b"regular", "2020-07-01T12:40:45Z"),
+    ]
+    mocker.patch("bugmon.bug.EnhancedBug.get_attachments", return_value=attachments)
+
+    bugmon.fetch_attachments()
+
+    assert (bugmon.test_dir / "nested/test.js").read_bytes() == b"nested"
+    assert (bugmon.test_dir / "other/regular.js").read_bytes() == b"regular"
+
+
+def test_bugmon_fetch_attachments_rejects_unsafe_path(mocker, bugmon):
+    """Verify unsafe attachment paths raise instead of being normalized."""
+    archive_data = io.BytesIO()
+    with zipfile.ZipFile(archive_data, "w") as archive:
+        archive.writestr("../escaped.js", b"escaped")
+
+    attachments = [_attachment("attachments.zip", archive_data.getvalue())]
+    mocker.patch("bugmon.bug.EnhancedBug.get_attachments", return_value=attachments)
+
+    with pytest.raises(ValueError, match="Unsafe attachment path"):
+        bugmon.fetch_attachments()
+
+    assert not (bugmon.test_dir / "escaped.js").exists()
+    assert not (bugmon.test_dir.parent / "escaped.js").exists()
+
+
 def test_bugmon_no_need_pernosco_with_pernosco_failed(bugmon):
     """Test that bugmon does not attempt to record a pernosco session when the
     pernosco-failed status command is present"""

tests/test_utils.py

@@ -36,6 +36,51 @@ def test_get_pernosco_trace_none_match(tmp_path):
     assert utils.get_pernosco_trace(tmp_path) is None
 
 
+@pytest.mark.parametrize(
+    "file_name, expected",
+    [
+        ("test.js", "test.js"),
+        ("subdir/test.js", "subdir/test.js"),
+        ("foo:bar.js", "foobar.js"),
+        (r"subdir\\test.js", "subdir/test.js"),
+    ],
+)
+def test_sanitize_attachment_path(tmp_path, file_name, expected):
+    """Verify attachment paths are sanitized and remain relative."""
+    assert utils.sanitize_attachment_path(tmp_path, file_name) == tmp_path / expected
+
+
+@pytest.mark.parametrize(
+    "file_name",
+    [
+        "",
+        ".",
+        "..",
+        "../..",
+        "../outside.js",
+        ".. /outside.js",
+        "..\x00/outside.js",
+        "nested/./test.js",
+        "nested/../test.js",
+        "/absolute/path.js",
+        "C:\\absolute\\path.js",
+    ],
+)
+def test_sanitize_attachment_path_rejects_unsafe_paths(tmp_path, file_name):
+    """Verify unsafe paths raise instead of being normalized."""
+    with pytest.raises(ValueError, match="Unsafe attachment path"):
+        utils.sanitize_attachment_path(tmp_path, file_name)
+
+
+def test_sanitize_attachment_path_limits_error_length(tmp_path):
+    """Verify unsafe filenames cannot produce excessively long errors."""
+    with pytest.raises(ValueError) as exc_info:
+        utils.sanitize_attachment_path(tmp_path, f"../{'x' * 1000}.js")
+
+    assert len(str(exc_info.value)) <= 125
+    assert str(exc_info.value).endswith("...")
+
+
 def test_has_pernosco_creds_all(pernosco_creds):
     """Verify that has_pernosco_creds returns True when all creds present"""
     assert utils.has_pernosco_creds(pernosco_creds) is True