From 9b8e6312410948cc789e88f483fe1aee33f2938f Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 9 Aug 2026 17:09:46 -0700 Subject: [PATCH] fix(file_tools): refuse plain-text writes that corrupt binary documents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port from nearai/ironclaw#7109: read_file auto-extracts .docx/.xlsx/.pptx (and PDF via anydoc) to readable text, so a model plausibly believes it holds the file's contents and writes the edited text back with write_file/patch — silently destroying the document container. Proven live on main: write_file over a valid .docx left a non-zip corpse, and a text write over an existing .pdf clobbered the %PDF header. - tools/binary_extensions.py: OPAQUE_DOCUMENT_EXTENSIONS + has_opaque_document_extension() + is_pdf_path() (pure string checks) - tools/file_tools.py: _check_binary_document_write() — opaque container formats (doc/docx/xls/xlsx/ppt/pptx/odt/ods/odp) always rejected; .pdf rejected only when overwriting an existing regular file (new-PDF creation stays allowed, matching the upstream split guard). Wired into write_file_tool and patch_tool (replace + V4A Update/Add headers; Delete/Move skip the guard since they write no text). - tests/tools/test_binary_document_write_guard.py: guard unit tests + end-to-end write_file/patch coverage incl. bytes-untouched assertions. --- .../tools/test_binary_document_write_guard.py | 172 ++++++++++++++++++ tools/binary_extensions.py | 29 +++ tools/file_tools.py | 64 ++++++- 3 files changed, 263 insertions(+), 2 deletions(-) create mode 100644 tests/tools/test_binary_document_write_guard.py diff --git a/tests/tools/test_binary_document_write_guard.py b/tests/tools/test_binary_document_write_guard.py new file mode 100644 index 0000000000000..3bb95d40f5e5c --- /dev/null +++ b/tests/tools/test_binary_document_write_guard.py @@ -0,0 +1,172 @@ +"""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", + '', + ) + z.writestr( + "word/document.xml", + '' + "Quarterly numbers look good." + "", + ) + + +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"): + assert has_opaque_document_extension(p) is True + + 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_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" diff --git a/tools/binary_extensions.py b/tools/binary_extensions.py index bd4bb8d1de530..17147be80f746 100644 --- a/tools/binary_extensions.py +++ b/tools/binary_extensions.py @@ -40,3 +40,32 @@ 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) that a +# plain-text write can NEVER produce validly. read_file auto-extracts these +# to readable text, 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", ".xls", ".xlsx", ".ppt", ".pptx", + ".odt", ".ods", ".odp", +}) + + +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") diff --git a/tools/file_tools.py b/tools/file_tools.py index deb55134f542a..4156be2e61ac4 100644 --- a/tools/file_tools.py +++ b/tools/file_tools.py @@ -2048,6 +2048,52 @@ 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. + """ + from tools.binary_extensions import has_opaque_document_extension, is_pdf_path + 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: @@ -2062,6 +2108,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) @@ -2146,8 +2195,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 @@ -2173,12 +2226,15 @@ def patch_tool(mode: str = "replace", path: str = None, old_string: 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 @@ -2197,6 +2253,10 @@ def patch_tool(mode: str = "replace", path: str = None, old_string: 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)