fix(file_tools): refuse plain-text writes that corrupt binary documents

tests/tools/test_binary_document_write_guard.py

@@ -0,0 +1,188 @@
+"""Tests for the binary-document write guard (port of nearai/ironclaw#7109).
+
+A plain-text write can never produce a valid OOXML/OLE/ODF container, so
+write_file/patch must refuse to write text into .docx/.xlsx/.pptx (and
+friends), and must refuse to OVERWRITE an existing .pdf — while still
+allowing new-.pdf creation (raw PDF syntax is text-authorable).
+"""
+
+import json
+import zipfile
+from pathlib import Path
+
+from tools.binary_extensions import (
+    has_opaque_document_extension,
+    is_pdf_path,
+)
+from tools.file_tools import (
+    _check_binary_document_write,
+    patch_tool,
+    write_file_tool,
+)
+
+
+def _make_minimal_docx(path: Path) -> None:
+    with zipfile.ZipFile(path, "w") as z:
+        z.writestr(
+            "[Content_Types].xml",
+            '<?xml version="1.0"?><Types xmlns="http://schemas.openxmlformats.org/'
+            'package/2006/content-types"><Default Extension="xml" '
+            'ContentType="application/xml"/></Types>',
+        )
+        z.writestr(
+            "word/document.xml",
+            '<?xml version="1.0"?><w:document xmlns:w="http://schemas.'
+            'openxmlformats.org/wordprocessingml/2006/main"><w:body><w:p><w:r>'
+            "<w:t>Quarterly numbers look good.</w:t></w:r></w:p></w:body>"
+            "</w:document>",
+        )
+
+
+class TestExtensionHelpers:
+    def test_opaque_document_extensions(self):
+        for p in ("a.docx", "b.XLSX", "c.pptx", "d.doc", "e.odt", "f.ods", "g.odp",
+                  "h.docm", "i.xlsm", "j.xlsb", "k.pptm", "l.ppsx", "m.ppsm",
+                  "n.pps", "o.pot", "p.rtf", "q.epub"):
+            assert has_opaque_document_extension(p) is True, f"{p} should be opaque"
+
+    def test_non_opaque_paths(self):
+        for p in ("a.txt", "b.py", "c.pdf", "d.md", "noext", "e.csv"):
+            assert has_opaque_document_extension(p) is False
+
+    def test_is_pdf_path(self):
+        assert is_pdf_path("report.pdf") is True
+        assert is_pdf_path("report.PDF") is True
+        assert is_pdf_path("report.txt") is False
+
+
+class TestCheckBinaryDocumentWrite:
+    def test_docx_always_rejected(self, tmp_path: Path):
+        # Even a NON-existing docx is rejected — text can't be a valid container.
+        err = _check_binary_document_write(str(tmp_path / "new.docx"))
+        assert err is not None
+        assert ".docx" in err
+
+    def test_existing_pdf_rejected(self, tmp_path: Path):
+        pdf = tmp_path / "doc.pdf"
+        pdf.write_bytes(b"%PDF-1.4\n%%EOF\n")
+        err = _check_binary_document_write(str(pdf))
+        assert err is not None
+        assert "overwrite" in err.lower()
+
+    def test_new_pdf_allowed(self, tmp_path: Path):
+        assert _check_binary_document_write(str(tmp_path / "fresh.pdf")) is None
+
+    def test_plain_text_allowed(self, tmp_path: Path):
+        assert _check_binary_document_write(str(tmp_path / "notes.txt")) is None
+
+
+class TestWriteFileToolGuard:
+    def test_write_file_rejects_existing_docx(self, tmp_path: Path):
+        docx = tmp_path / "report.docx"
+        _make_minimal_docx(docx)
+        original = docx.read_bytes()
+
+        result = json.loads(write_file_tool(str(docx), "edited text"))
+
+        assert result.get("error"), "text write into .docx must be refused"
+        assert docx.read_bytes() == original, "document bytes must be untouched"
+        assert zipfile.is_zipfile(docx), "document must remain a valid container"
+
+    def test_write_file_rejects_docm(self, tmp_path: Path):
+        """Regression: .docm is extractable by read_file (anydoc) but was
+        missing from OPAQUE_DOCUMENT_EXTENSIONS in the original PR #82818.
+        Flagged by @egilewski — proven live: text write corrupted the zip."""
+        docm = tmp_path / "macro.docm"
+        _make_minimal_docx(docm)  # same OOXML zip structure
+        original = docm.read_bytes()
+
+        result = json.loads(write_file_tool(str(docm), "edited text"))
+
+        assert result.get("error"), "text write into .docm must be refused"
+        assert docm.read_bytes() == original, "document bytes must be untouched"
+        assert zipfile.is_zipfile(docm), "document must remain a valid container"
+
+    def test_write_file_rejects_new_docx(self, tmp_path: Path):
+        result = json.loads(write_file_tool(str(tmp_path / "new.docx"), "hello"))
+        assert result.get("error")
+        assert not (tmp_path / "new.docx").exists()
+
+    def test_write_file_rejects_existing_pdf_overwrite(self, tmp_path: Path):
+        pdf = tmp_path / "doc.pdf"
+        pdf.write_bytes(b"%PDF-1.4\n1 0 obj\nendobj\n%%EOF\n")
+        original = pdf.read_bytes()
+
+        result = json.loads(write_file_tool(str(pdf), "replacement text"))
+
+        assert result.get("error")
+        assert pdf.read_bytes() == original
+
+    def test_write_file_allows_new_pdf_creation(self, tmp_path: Path):
+        pdf = tmp_path / "generated.pdf"
+        result = json.loads(write_file_tool(str(pdf), "%PDF-1.4\n%%EOF\n"))
+        assert not result.get("error")
+        assert pdf.exists()
+
+    def test_write_file_plain_text_unaffected(self, tmp_path: Path):
+        target = tmp_path / "notes.txt"
+        result = json.loads(write_file_tool(str(target), "hello world"))
+        assert not result.get("error")
+        assert target.read_text() == "hello world"
+
+
+class TestPatchToolGuard:
+    def test_patch_replace_rejects_docx(self, tmp_path: Path):
+        docx = tmp_path / "report.docx"
+        _make_minimal_docx(docx)
+        original = docx.read_bytes()
+
+        result = json.loads(
+            patch_tool(mode="replace", path=str(docx),
+                       old_string="good", new_string="great")
+        )
+
+        assert result.get("error")
+        assert docx.read_bytes() == original
+
+    def test_patch_v4a_update_rejects_docx(self, tmp_path: Path):
+        docx = tmp_path / "report.docx"
+        _make_minimal_docx(docx)
+        original = docx.read_bytes()
+
+        v4a = (
+            "*** Begin Patch\n"
+            f"*** Update File: {docx}\n"
+            "@@\n"
+            "-good\n"
+            "+great\n"
+            "*** End Patch"
+        )
+        result = json.loads(patch_tool(mode="patch", patch=v4a))
+
+        assert result.get("error")
+        assert docx.read_bytes() == original
+
+    def test_patch_v4a_delete_of_docx_not_blocked_by_guard(self, tmp_path: Path):
+        # Delete doesn't write text content — the binary-document guard must
+        # not fire for it (delete may still fail/succeed for other reasons).
+        docx = tmp_path / "old.docx"
+        _make_minimal_docx(docx)
+
+        v4a = (
+            "*** Begin Patch\n"
+            f"*** Delete File: {docx}\n"
+            "*** End Patch"
+        )
+        result = json.loads(patch_tool(mode="patch", patch=v4a))
+        err = result.get("error") or ""
+        assert "binary document" not in err.lower()
+
+    def test_patch_replace_plain_text_unaffected(self, tmp_path: Path):
+        target = tmp_path / "notes.txt"
+        target.write_text("hello world")
+        result = json.loads(
+            patch_tool(mode="replace", path=str(target),
+                       old_string="world", new_string="there")
+        )
+        assert not result.get("error")
+        assert target.read_text() == "hello there"

tools/binary_extensions.py

@@ -40,3 +40,36 @@ def has_binary_extension(path: str) -> bool:
     if dot == -1:
         return False
     return path[dot:].lower() in BINARY_EXTENSIONS
+
+
+# Container document formats (OOXML zip / OLE compound / ODF zip / EPUB zip / RTF)
+# that a plain-text write can NEVER produce validly.  read_file auto-extracts
+# these to readable text (via anydoc for the non-built-in formats), so a model
+# that "read" report.docx and then writes the edited text back via
+# write_file/patch silently destroys the document.
+# PDF is intentionally NOT here: raw PDF syntax is text-authorable, so
+# new-file creation is legitimate — only overwrites are dangerous (handled
+# separately by the write guard).
+OPAQUE_DOCUMENT_EXTENSIONS = frozenset({
+    ".doc", ".docx", ".docm",
+    ".xls", ".xlsx", ".xlsm", ".xlsb",
+    ".ppt", ".pps", ".pot", ".pptx", ".pptm", ".ppsx", ".ppsm",
+    ".odt", ".ods", ".odp",
+    ".rtf", ".epub",
+})
+
+
+def has_opaque_document_extension(path: str) -> bool:
+    """True when the path names an opaque container document (.docx etc.).
+
+    Pure string check, no I/O.
+    """
+    dot = path.rfind(".")
+    if dot == -1:
+        return False
+    return path[dot:].lower() in OPAQUE_DOCUMENT_EXTENSIONS
+
+
+def is_pdf_path(path: str) -> bool:
+    """True when the path has a .pdf extension. Pure string check, no I/O."""
+    return path.lower().endswith(".pdf")

tools/file_tools.py

@@ -12,7 +12,11 @@
 from pathlib import Path, PurePosixPath
 
 from agent.file_safety import get_read_block_error
-from tools.binary_extensions import has_binary_extension
+from tools.binary_extensions import (
+    has_binary_extension,
+    has_opaque_document_extension,
+    is_pdf_path,
+)
 from tools.file_operations import (
     ShellFileOperations,
     normalize_read_pagination,
@@ -2170,6 +2174,51 @@ def _mark_verification_stale(
         logger.debug("verification stale marker failed", exc_info=True)
 
 
+def _check_binary_document_write(filepath: str, task_id: str = "default") -> str | None:
+    """Reject text-tool writes that would corrupt a binary document.
+
+    ``read_file`` auto-extracts .docx/.xlsx/.pptx (and PDF, via anydoc) to
+    readable text, so the model plausibly believes it holds the file's
+    contents and tries to write the edited text back with write_file/patch.
+    A plain-text write can never produce a valid OOXML/OLE/ODF container, so
+    that write silently destroys the document (port of nearai/ironclaw#7109).
+
+    Rules:
+    - Opaque container formats (.doc/.docx/.xls/.xlsx/.ppt/.pptx/.odt/.ods/
+      .odp): always rejected — text bytes are never a valid document, whether
+      creating or overwriting.
+    - .pdf: rejected only when OVERWRITING an existing regular file. Raw PDF
+      syntax is text-authorable, so new-file creation stays allowed.
+    """
+    if has_opaque_document_extension(filepath):
+        ext = filepath[filepath.rfind("."):].lower()
+        return (
+            f"Refusing to write plain text to binary document '{filepath}' ({ext}). "
+            "A text write cannot produce a valid document container and would "
+            "corrupt the file (read_file showed you EXTRACTED text, not the real "
+            "bytes). Use the docx/xlsx/powerpoint skills or a library like "
+            "python-docx/openpyxl/python-pptx via the terminal to create or edit "
+            "this document."
+        )
+    if is_pdf_path(filepath):
+        try:
+            resolved = Path(_resolve_path_for_task(filepath, task_id))
+        except Exception:
+            resolved = Path(_expand_tilde(filepath))
+        try:
+            if resolved.is_file():
+                return (
+                    f"Refusing to overwrite existing PDF '{filepath}' with plain text. "
+                    "read_file showed you EXTRACTED text, not the real bytes — writing "
+                    "text back would destroy the document. Use the pdf skill or a PDF "
+                    "library via the terminal to modify it. (Creating a NEW .pdf file "
+                    "is allowed.)"
+                )
+        except OSError:
+            pass
+    return None
+
+
 def write_file_tool(path: str, content: str, task_id: str = "default",
                     cross_profile: bool = False,
                     session_id: str | None = None) -> str:
@@ -2184,6 +2233,9 @@ def write_file_tool(path: str, content: str, task_id: str = "default",
     sensitive_err = _check_sensitive_path(path, task_id)
     if sensitive_err:
         return tool_error(sensitive_err)
+    binary_doc_err = _check_binary_document_write(path, task_id)
+    if binary_doc_err:
+        return tool_error(binary_doc_err)
     protected_err = _check_protected_instruction_write([path], task_id)
     if protected_err:
         return tool_error(protected_err)
@@ -2271,8 +2323,12 @@ def patch_tool(mode: str = "replace", path: str = None, old_string: str = None,
     """
     # Check sensitive paths for both replace (explicit path) and V4A patch (extract paths)
     _paths_to_check = []
+    # Paths whose CONTENT will be text-written (Update/Add + explicit path).
+    # V4A Delete/Move don't write text, so they skip the binary-document guard.
+    _content_write_paths = []
     if path:
         _paths_to_check.append(path)
+        _content_write_paths.append(path)
     if mode == "patch" and patch:
         import re as _re
         from tools.path_security import has_traversal_component
@@ -2298,12 +2354,15 @@ def _reject_v4a_traversal(v4a_path: str) -> str | None:
         # it accepts ``***Update File:`` with no space after the asterisks
         # (patch_parser.py uses ``\*\*\*\s*Update\s+File:``). Requiring a space
         # here let a no-space header parse + apply while skipping this check.
-        for _m in _re.finditer(r'^\*\*\*\s*(?:Update|Add|Delete)\s+File:\s*(.+)$', patch, _re.MULTILINE):
-            v4a_path = _m.group(1).strip()
+        for _m in _re.finditer(r'^\*\*\*\s*(Update|Add|Delete)\s+File:\s*(.+)$', patch, _re.MULTILINE):
+            _op = _m.group(1)
+            v4a_path = _m.group(2).strip()
             _err = _reject_v4a_traversal(v4a_path)
             if _err:
                 return _err
             _paths_to_check.append(v4a_path)
+            if _op in ("Update", "Add"):
+                _content_write_paths.append(v4a_path)
         # ``*** Move File: src -> dst`` is a valid V4A op (patch_parser.py:114)
         # but was never extracted, so a Move targeting /etc/crontab skipped the
         # sensitive-path pre-check. Check BOTH endpoints, and run them through
@@ -2322,6 +2381,10 @@ def _reject_v4a_traversal(v4a_path: str) -> str | None:
             cross_warning = _check_cross_profile_path(_p, task_id)
             if cross_warning:
                 return tool_error(cross_warning)
+    for _p in _content_write_paths:
+        binary_doc_err = _check_binary_document_write(_p, task_id)
+        if binary_doc_err:
+            return tool_error(binary_doc_err)
     # One approval prompt for the whole patch: a single protected file gates
     # the ENTIRE patch (deny applies nothing — see the helper's docstring).
     protected_err = _check_protected_instruction_write(_paths_to_check, task_id)