fix(file_tools): refuse plain-text writes that corrupt binary documents
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.
This commit is contained in:
Teknium
parent
3bd844edf1
commit
9b8e631241
|
|
@ -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",
|
||||
'<?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"):
|
||||
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"
|
||||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Reference in New Issue