From 2dcef2832c3fa630673bbe6b3cb2300d3e085cb0 Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Fri, 10 Jul 2026 13:33:41 -0400 Subject: [PATCH] Fix TypeError in file clean when a require ID contains "file" _gen_keep_files filtered requisites with `"file" in comp`, which for a bare-string requisite ID degraded to a substring match. Any ID containing the substring "file" then hit `comp["file"]` and raised "TypeError: string indices must be integers". Guard the membership test with an isinstance check so only dict requisites are considered; bare strings are ignored instead of crashing. Fixes #53692 and Fixes #61042 --- changelog/53692.fixed.md | 1 + changelog/61042.fixed.md | 1 + salt/states/file.py | 4 +- .../unit/states/file/test_find_keep_files.py | 80 +++++++++++++++++++ 4 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 changelog/53692.fixed.md create mode 100644 changelog/61042.fixed.md diff --git a/changelog/53692.fixed.md b/changelog/53692.fixed.md new file mode 100644 index 000000000000..4df1fb67555b --- /dev/null +++ b/changelog/53692.fixed.md @@ -0,0 +1 @@ +Fixed a TypeError in file.recurse/file.directory with clean when a require requisite is a bare state ID string containing the substring "file"; such requisites are now ignored instead of crashing. diff --git a/changelog/61042.fixed.md b/changelog/61042.fixed.md new file mode 100644 index 000000000000..8b2b7ac5f79c --- /dev/null +++ b/changelog/61042.fixed.md @@ -0,0 +1 @@ +Fixed _gen_keep_files so the require filter only matches dict requisites; a bare-string requisite ID containing "file" no longer raises "string indices must be integers". diff --git a/salt/states/file.py b/salt/states/file.py index 96bc5cab9141..9b6bdf2e5337 100644 --- a/salt/states/file.py +++ b/salt/states/file.py @@ -634,7 +634,9 @@ def _process(name): keep = set() if isinstance(require, list): - required_files = [comp for comp in require if "file" in comp] + required_files = [ + comp for comp in require if isinstance(comp, dict) and "file" in comp + ] for comp in required_files: for low in __lowstate__: # A requirement should match either the ID and the name of diff --git a/tests/pytests/unit/states/file/test_find_keep_files.py b/tests/pytests/unit/states/file/test_find_keep_files.py index 6854bd3e61ee..f94b2358361f 100644 --- a/tests/pytests/unit/states/file/test_find_keep_files.py +++ b/tests/pytests/unit/states/file/test_find_keep_files.py @@ -3,6 +3,7 @@ import pytest import salt.states.file as filestate +from tests.support.mock import patch log = logging.getLogger(__name__) @@ -81,3 +82,82 @@ def test__find_keep_files_darwin(): actual = sorted(list(keep)) expected = [] assert actual == expected + + +def test__gen_keep_files_bare_string_requisite_53692(): + """ + A bare-string requisite ID that happens to contain the substring "file" + must not crash _gen_keep_files. This is the file.recurse(clean=True) + reproducer from #53692: the require list holds a plain state ID string + (written as ``- p_files_recurse_test_recurse_one``) instead of the dict + form ``- file: ``. Before the fix the "file" in comp membership test + degraded to a substring match, then comp["file"] indexed a str with a str + and raised ``TypeError: string indices must be integers``. + """ + # require here mirrors a state's ``require`` requisite list as passed from + # the file.recurse / file.directory clean handlers; the bare string is the + # requisite ID form ``- p_files_recurse_test_recurse_one``. + lowstate = [ + { + "name": "/test1", + "__id__": "p_files_recurse_test_recurse_one", + "fun": "recurse", + } + ] + with patch.object(filestate, "__lowstate__", lowstate, create=True): + keep = filestate._gen_keep_files("/test2", ["p_files_recurse_test_recurse_one"]) + assert keep == [] + + +def test__gen_keep_files_bare_string_requisite_61042(): + """ + Same crash as #53692 via the #61042 MCVE: a bare-string requisite ID + ``aaa_file`` (contains "file") passed to file.recurse(clean=True) must be + ignored, not raise TypeError. + """ + # Bare requisite ID form ``- aaa_file`` from the #61042 reproducer. + lowstate = [{"name": "/srv/aaa", "__id__": "aaa_file", "fun": "managed"}] + with patch.object(filestate, "__lowstate__", lowstate, create=True): + keep = filestate._gen_keep_files("/srv/target", ["aaa_file"]) + assert keep == [] + + +def test__gen_keep_files_dict_requisite_not_regressed_53692(): + """ + Inverse / must-not-regress for #53692: a normal dict requisite + ``{"file": }`` that matches a low state must still contribute its file + to the keep list. Passes with and without the fix, proving the isinstance + guard does not disturb the supported dict requisite path. + """ + lowstate = [{"name": "/nonexistent/kept", "__id__": "kept_id", "fun": "managed"}] + with patch.object(filestate, "__lowstate__", lowstate, create=True): + with patch("os.path.isdir", return_value=False): + keep = filestate._gen_keep_files("/parent", [{"file": "kept_id"}]) + assert keep == ["/nonexistent/kept"] + + +def test__gen_keep_files_dict_requisite_not_regressed_61042(): + """ + Inverse / must-not-regress for #61042: the dict requisite form + ``{"file": "bbb"}`` still retains the required file. Passes before and + after the fix. + """ + lowstate = [{"name": "/nonexistent/bbb", "__id__": "bbb", "fun": "managed"}] + with patch.object(filestate, "__lowstate__", lowstate, create=True): + with patch("os.path.isdir", return_value=False): + keep = filestate._gen_keep_files("/parent", [{"file": "bbb"}]) + assert keep == ["/nonexistent/bbb"] + + +def test__gen_keep_files_bare_string_without_file_ignored(): + """ + Peripheral coverage: a bare-string requisite that does NOT contain the + substring "file" was, and remains, silently ignored by _gen_keep_files + (it never matches the dict-key check). This documents that the fix only + changes the crashing substring-match case and preserves the pre-existing + drop of bare-string requisites. Passes with and without the fix. + """ + lowstate = [{"name": "/test1", "__id__": "aaa", "fun": "managed"}] + with patch.object(filestate, "__lowstate__", lowstate, create=True): + keep = filestate._gen_keep_files("/test2", ["aaa"]) + assert keep == []