Fix file states raising UnicodeDecodeError on encoding mismatch

changelog/50903.fixed.md

@@ -0,0 +1 @@
+Fixed the file.comment, file.append, and file.prepend states failing with UnicodeDecodeError when the target file contains bytes that are not valid in the system encoding.

salt/states/file.py

@@ -6306,15 +6306,15 @@ def comment(name, regex, char="#", backup=".bak", ignore_missing=False):
 
     with salt.utils.files.fopen(name, "rb") as fp_:
         slines = fp_.read()
-        slines = slines.decode(__salt_system_encoding__)
+        slines = slines.decode(__salt_system_encoding__, errors="replace")
         slines = slines.splitlines(True)
 
     # Perform the edit
     __salt__["file.comment_line"](name, regex, char, True, backup)
 
     with salt.utils.files.fopen(name, "rb") as fp_:
         nlines = fp_.read()
-        nlines = nlines.decode(__salt_system_encoding__)
+        nlines = nlines.decode(__salt_system_encoding__, errors="replace")
         nlines = nlines.splitlines(True)
 
     # Check the result
@@ -6643,7 +6643,7 @@ def append(
 
     with salt.utils.files.fopen(name, "rb") as fp_:
         slines = fp_.read()
-        slines = slines.decode(__salt_system_encoding__)
+        slines = slines.decode(__salt_system_encoding__, errors="replace")
         slines = slines.splitlines()
 
     append_lines = []
@@ -6689,7 +6689,7 @@ def append(
 
     with salt.utils.files.fopen(name, "rb") as fp_:
         nlines = fp_.read()
-        nlines = nlines.decode(__salt_system_encoding__)
+        nlines = nlines.decode(__salt_system_encoding__, errors="replace")
         nlines = nlines.splitlines()
 
     if slines != nlines:
@@ -6928,7 +6928,7 @@ def prepend(
 
     with salt.utils.files.fopen(name, "rb") as fp_:
         slines = fp_.read()
-        slines = slines.decode(__salt_system_encoding__)
+        slines = slines.decode(__salt_system_encoding__, errors="replace")
         slines = slines.splitlines(True)
 
     count = 0
@@ -6976,7 +6976,7 @@ def prepend(
         with salt.utils.files.fopen(name, "rb") as fp_:
             # read as many lines of target file as length of user input
             contents = fp_.read()
-            contents = contents.decode(__salt_system_encoding__)
+            contents = contents.decode(__salt_system_encoding__, errors="replace")
             contents = contents.splitlines(True)
             target_head = contents[0 : len(preface)]
             target_lines = []
@@ -6995,7 +6995,7 @@ def prepend(
 
     with salt.utils.files.fopen(name, "rb") as fp_:
         nlines = fp_.read()
-        nlines = nlines.decode(__salt_system_encoding__)
+        nlines = nlines.decode(__salt_system_encoding__, errors="replace")
         nlines = nlines.splitlines(True)
 
     if slines != nlines:

tests/pytests/unit/states/file/test_append.py

@@ -0,0 +1,89 @@
+import builtins
+
+import pytest
+
+import salt.states.file as filestate
+import salt.utils.files
+from tests.support.mock import MagicMock, patch
+
+
+@pytest.fixture
+def configure_loader_modules():
+    return {
+        filestate: {
+            "__env__": "base",
+            "__salt__": {},
+            "__opts__": {"test": False, "cachedir": ""},
+            "__instance_id__": "",
+            "__low__": {},
+            "__utils__": {},
+        }
+    }
+
+
+def test_append_file_encoding_mismatch(tmp_path):
+    """
+    file.append must not raise UnicodeDecodeError when the target file
+    contains bytes that are not valid in the system encoding. The decoded
+    contents are only used to build the diff, so undecodable bytes should
+    be tolerated rather than aborting the state.
+
+    Regression test for #50903.
+    """
+    name = tmp_path / "bugfile"
+    # 0xed is not valid ASCII and not valid UTF-8 on its own
+    name.write_bytes(b"abc\xedxyz\n")
+
+    salt_mock = {
+        "file.search": MagicMock(return_value=False),
+        "file.append": MagicMock(return_value=None),
+    }
+    utils_mock = {"files.is_text": MagicMock(return_value=True)}
+
+    with patch.object(builtins, "__salt_system_encoding__", "ascii"), patch.dict(
+        filestate.__salt__, salt_mock
+    ), patch.dict(filestate.__utils__, utils_mock):
+        result = filestate.append(name=str(name), text="cheese")
+
+    assert result["result"] is True
+    salt_mock["file.append"].assert_called_once()
+
+
+def test_append_clean_encoding_unaffected_50903(tmp_path):
+    """
+    Guard against overcorrection of the #50903 fix: decoding with
+    errors="replace" must not change behaviour for files that decode
+    cleanly in the system encoding. The diff must still be generated,
+    contain the original non-ASCII text unmangled, and hold no U+FFFD
+    replacement characters. This test passes with and without the fix.
+    """
+    name = tmp_path / "cleanfile"
+    # Valid UTF-8 content that decodes cleanly with the utf-8 system encoding
+    name.write_bytes("h\u00e9llo\n".encode("utf-8"))
+
+    def fake_append(fname, args=None):
+        # Perform the real append so the state's second read sees a change
+        with salt.utils.files.fopen(fname, "a", encoding="utf-8") as fp_:
+            for line in args:
+                fp_.write(line + "\n")
+
+    salt_mock = {
+        "file.search": MagicMock(return_value=False),
+        "file.append": MagicMock(side_effect=fake_append),
+    }
+    utils_mock = {"files.is_text": MagicMock(return_value=True)}
+
+    # Production callers (the state compiler running an SLS file.append)
+    # pass name and text; __salt_system_encoding__ is the locale-derived
+    # builtin read by the decode sites, so it is patched rather than passed.
+    with patch.object(builtins, "__salt_system_encoding__", "utf-8"), patch.dict(
+        filestate.__salt__, salt_mock
+    ), patch.dict(filestate.__utils__, utils_mock):
+        result = filestate.append(name=str(name), text="cheese")
+
+    assert result["result"] is True
+    assert result["comment"] == "Appended 1 lines"
+    diff = result["changes"]["diff"]
+    assert "+cheese" in diff
+    assert "h\u00e9llo" in diff
+    assert "\ufffd" not in diff

tests/pytests/unit/states/file/test_comment.py

@@ -1,3 +1,4 @@
+import builtins
 import logging
 import os
 
@@ -115,6 +116,38 @@ def test_comment():
                     assert filestate.comment(name, regex) == ret
 
 
+def test_comment_file_encoding_mismatch_50903(tmp_path):
+    """
+    file.comment must not raise UnicodeDecodeError when the target file
+    contains bytes that are not valid in the system encoding. The decoded
+    contents are only used to build the diff, so undecodable bytes should
+    be tolerated rather than aborting the state.
+
+    Regression test for #50903.
+    """
+    name = tmp_path / "fstab"
+    # 0xed is not valid ASCII and not valid UTF-8 on its own
+    name.write_bytes(b"bind 127.0.0.1\nabc\xedxyz\n")
+
+    salt_mock = {
+        # First search: uncommented pattern found; second: commented after edit
+        "file.search": MagicMock(side_effect=[True, True]),
+        "file.comment_line": MagicMock(return_value=True),
+    }
+
+    # Production callers (the state compiler running an SLS file.comment)
+    # pass name and regex; __salt_system_encoding__ is the locale-derived
+    # builtin read by the decode sites, so it is patched rather than passed.
+    with patch.object(builtins, "__salt_system_encoding__", "ascii"), patch.dict(
+        filestate.__salt__, salt_mock
+    ):
+        result = filestate.comment(str(name), "^bind 127.0.0.1")
+
+    assert result["result"] is True
+    assert result["comment"] == "Commented lines successfully"
+    salt_mock["file.comment_line"].assert_called_once()
+
+
 # 'uncomment' function tests: 1
 def test_uncomment():
     """

tests/pytests/unit/states/file/test_prepend.py

@@ -1,3 +1,4 @@
+import builtins
 import logging
 import os
 
@@ -114,3 +115,34 @@ def test_prepend():
                             comt = "Prepended 1 lines"
                             ret.update({"comment": comt, "result": True, "changes": {}})
                             assert filestate.prepend(name, text=text) == ret
+
+
+def test_prepend_file_encoding_mismatch_50903(tmp_path):
+    """
+    file.prepend must not raise UnicodeDecodeError when the target file
+    contains bytes that are not valid in the system encoding. The decoded
+    contents are only used to build the diff, so undecodable bytes should
+    be tolerated rather than aborting the state.
+
+    Regression test for #50903.
+    """
+    name = tmp_path / "motd"
+    # 0xed is not valid ASCII and not valid UTF-8 on its own
+    name.write_bytes(b"abc\xedxyz\n")
+
+    salt_mock = {
+        "file.search": MagicMock(return_value=False),
+        "file.prepend": MagicMock(return_value=True),
+    }
+
+    # Production callers (the state compiler running an SLS file.prepend)
+    # pass name and text; __salt_system_encoding__ is the locale-derived
+    # builtin read by the decode sites, so it is patched rather than passed.
+    with patch.object(builtins, "__salt_system_encoding__", "ascii"), patch.dict(
+        filestate.__salt__, salt_mock
+    ):
+        result = filestate.prepend(name=str(name), text="Trust no one")
+
+    assert result["result"] is True
+    assert result["comment"] == "Prepended 1 lines"
+    salt_mock["file.prepend"].assert_called_once()