fix(file-ops): prevent non-UTF-8 corruption and symlink data-loss
Two DATA-LOSS bugs in ShellFileOperations found in a core-tools audit, each reproduced live against current main: 1. Non-UTF-8 file content silently corrupted on read->write. The terminal env decodes stdout with errors='replace', so a latin-1/8859 file's bytes arrive as U+FFFD before _is_likely_binary inspects them. U+FFFD is 'printable', so the >30%-non-printable check never flagged it, and the agent would read the mojibake and write it back, permanently replacing the original bytes. Fix: treat a sample containing U+FFFD as binary (read-only). 2. Writing through a symlink destroyed the link and orphaned the target. The atomic temp-file + 'mv -f' swap replaced the symlink itself with a plain file; the real target was never updated. Fix: resolve the link with readlink -f/realpath first and recompute the temp dir from the resolved target so the mv stays same-filesystem atomic. Broken links fall back to the original path (no regression). Both verified with sabotage-checked regression tests (fail without the fix). Proper UTF-8 text (incl. non-ASCII) and plain-file writes are unaffected.
This commit is contained in:
parent
9d08c95464
commit
021a076880
|
|
@ -615,3 +615,61 @@ class TestAtomicWriteNewFilePermissions:
|
|||
assert result.error is None, f"write failed: {result.error}"
|
||||
assert dest.read_text() == "#!/bin/sh\necho updated\n"
|
||||
assert dest.stat().st_mode & 0o777 == 0o755
|
||||
|
||||
|
||||
class TestAtomicWriteThroughSymlink:
|
||||
"""_atomic_write must edit a symlink's target, not replace the link.
|
||||
|
||||
Regression: the temp-file + ``mv`` swap replaced the symlink itself with a
|
||||
plain file, orphaning the real target and destroying the link (data-loss).
|
||||
"""
|
||||
|
||||
def test_write_follows_symlink_and_preserves_link(self, tmp_path):
|
||||
ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path)))
|
||||
real = tmp_path / "real.txt"
|
||||
link = tmp_path / "link.txt"
|
||||
real.write_text("original\n")
|
||||
link.symlink_to(real)
|
||||
|
||||
result = ops.write_file(str(link), "newcontent\n")
|
||||
|
||||
assert result.error is None, f"write failed: {result.error}"
|
||||
# The link must survive as a symlink...
|
||||
assert link.is_symlink(), "symlink was replaced by a plain file"
|
||||
# ...and the real target must carry the new content.
|
||||
assert real.read_text() == "newcontent\n"
|
||||
assert os.path.realpath(link) == str(real)
|
||||
|
||||
def test_write_through_broken_symlink_falls_back(self, tmp_path):
|
||||
"""A broken link resolves through readlink -f and creates the target."""
|
||||
ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path)))
|
||||
target = tmp_path / "target.txt"
|
||||
link = tmp_path / "broken.lnk"
|
||||
link.symlink_to(target) # target does not exist yet
|
||||
|
||||
result = ops.write_file(str(link), "data\n")
|
||||
|
||||
assert result.error is None, f"write failed: {result.error}"
|
||||
assert target.exists()
|
||||
assert target.read_text() == "data\n"
|
||||
|
||||
|
||||
class TestReadNonUtf8IsBinary:
|
||||
"""Non-UTF-8 content must be flagged binary, not returned as lossy text.
|
||||
|
||||
Regression: the terminal env decodes stdout with errors="replace", turning
|
||||
every non-UTF-8 byte into U+FFFD before _is_likely_binary sees it. U+FFFD is
|
||||
"printable", so the non-printable ratio never caught it, and a
|
||||
read→edit→write round-trip would overwrite the original bytes with mojibake.
|
||||
"""
|
||||
|
||||
def test_replacement_char_sample_flagged_binary(self, tmp_path):
|
||||
ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path)))
|
||||
# A latin-1 file decoded with errors="replace" yields U+FFFD chars.
|
||||
lossy_sample = "caf\ufffd r\ufffdsum\ufffd\n"
|
||||
assert ops._is_likely_binary("notes.txt", lossy_sample) is True
|
||||
|
||||
def test_plain_utf8_text_not_flagged(self, tmp_path):
|
||||
ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path)))
|
||||
# Proper UTF-8 (including non-ASCII) must still read as text.
|
||||
assert ops._is_likely_binary("notes.txt", "café résumé\nsecond\n") is False
|
||||
|
|
|
|||
|
|
@ -884,6 +884,17 @@ class ShellFileOperations(FileOperations):
|
|||
|
||||
# Content analysis: >30% non-printable chars = binary
|
||||
if content_sample:
|
||||
# Undecodable bytes: the terminal env decodes stdout with
|
||||
# errors="replace", so any non-UTF-8 byte arrives here already
|
||||
# turned into U+FFFD. That char is "printable" (ord 65533), so the
|
||||
# non-printable ratio below never catches it — and returning the
|
||||
# lossy text would let a read→edit→write round-trip silently
|
||||
# overwrite the original bytes with mojibake. Treat a file whose
|
||||
# sample carries the replacement char as binary (read-only) so the
|
||||
# agent can't corrupt it. Legitimate UTF-8 text effectively never
|
||||
# contains U+FFFD.
|
||||
if "\ufffd" in content_sample[:1000]:
|
||||
return True
|
||||
non_printable = sum(1 for c in content_sample[:1000]
|
||||
if ord(c) < 32 and c not in '\n\r\t')
|
||||
return non_printable / min(len(content_sample), 1000) > 0.30
|
||||
|
|
@ -1022,11 +1033,21 @@ class ShellFileOperations(FileOperations):
|
|||
script = (
|
||||
"set -e; "
|
||||
f"d={q_parent}; t={q_path}; "
|
||||
# Follow a symlink target so we edit the file the link points at,
|
||||
# rather than replacing the symlink itself with a plain file (which
|
||||
# orphans the real target and destroys the link). Recompute the
|
||||
# temp dir from the RESOLVED target so `mv` stays same-filesystem
|
||||
# atomic. Best-effort: a broken link or missing readlink/realpath
|
||||
# falls back to the original path (pre-fix behavior, no regression).
|
||||
'if [ -L "$t" ]; then '
|
||||
'rt="$(readlink -f "$t" 2>/dev/null || realpath "$t" 2>/dev/null || true)"; '
|
||||
'[ -n "$rt" ] && { t="$rt"; d="$(dirname "$t")"; }; '
|
||||
"fi; "
|
||||
'tmp="$(mktemp -p "$d" ' + tmpl + ' 2>/dev/null '
|
||||
'|| mktemp "$d/.hermes-tmp.$$.XXXXXX" 2>/dev/null '
|
||||
'|| { tmp="$d/.hermes-tmp.$$"; : > "$tmp" && echo "$tmp"; })"; '
|
||||
'[ -n "$tmp" ] || { echo "atomic write: could not create temp file" >&2; exit 1; }; '
|
||||
"trap 'rm -f \"$tmp\"' EXIT; "
|
||||
"trap 'rm -f \\\"$tmp\\\"' EXIT; "
|
||||
# preserve mode of an existing target (best-effort, never fatal)
|
||||
'if [ -e "$t" ]; then '
|
||||
'm="$(stat -c%a "$t" 2>/dev/null || stat -f%Lp "$t" 2>/dev/null || true)"; '
|
||||
|
|
|
|||
Loading…
Reference in New Issue