From 15ac9cf1cffa8dd5db7c2910c3b0bed9075ca934 Mon Sep 17 00:00:00 2001 From: anis Date: Tue, 12 May 2026 19:36:26 +0200 Subject: [PATCH 01/19] fix etcdv3 user/password auth --- salt/utils/etcd_util.py | 1 + 1 file changed, 1 insertion(+) diff --git a/salt/utils/etcd_util.py b/salt/utils/etcd_util.py index 25de49cf80fa..7aa53a8d08f4 100644 --- a/salt/utils/etcd_util.py +++ b/salt/utils/etcd_util.py @@ -699,6 +699,7 @@ def __init__( # etcd3-py uses verify instead of ca_cert self.xargs["verify"] = self.xargs.pop("ca_cert", None) self.client = etcd3.Client(host=self.host, port=self.port, **self.xargs) + self.client.auth() def _maybe_decode_key(self, key, **extra_kwargs): extra_kwargs.setdefault("unicode_errors", self.unicode_errors) From 3acdcb4642e6dbbd6aa90f3296c45966a9075c1f Mon Sep 17 00:00:00 2001 From: cmcmarrow Date: Wed, 16 Aug 2023 19:35:51 -0500 Subject: [PATCH 02/19] handle_strict_undefined --- salt/utils/jinja.py | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 704b5f5997a9..17aefc50dbd5 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -734,6 +734,16 @@ def show_full_context(ctx): ) +def _handle_strict_undefined(function): + @wraps(function) + def __handle_strict_undefined(value, *args, **kwargs): + if isinstance(value, jinja2.StrictUndefined): + return value + return function(value, *args, **kwargs) + + return __handle_strict_undefined + + class SerializerExtension(Extension): ''' Yaml and Json manipulation. @@ -956,13 +966,15 @@ def __init__(self, environment): "load_json": self.load_json, "load_text": self.load_text, "dict_to_sls_yaml_params": self.dict_to_sls_yaml_params, - "combinations": itertools.combinations, - "combinations_with_replacement": itertools.combinations_with_replacement, - "compress": itertools.compress, - "permutations": itertools.permutations, - "product": itertools.product, - "zip": zip, - "zip_longest": itertools.zip_longest, + "combinations": _handle_strict_undefined(itertools.combinations), + "combinations_with_replacement": _handle_strict_undefined( + itertools.combinations_with_replacement + ), + "compress": _handle_strict_undefined(itertools.compress), + "permutations": _handle_strict_undefined(itertools.permutations), + "product": _handle_strict_undefined(itertools.product), + "zip": _handle_strict_undefined(zip), + "zip_longest": _handle_strict_undefined(itertools.zip_longest), } ) @@ -993,6 +1005,7 @@ def explore(data): return explore(data) + @_handle_strict_undefined def format_json(self, value, sort_keys=True, indent=None): json_txt = salt.utils.json.dumps( value, sort_keys=sort_keys, indent=indent @@ -1002,6 +1015,7 @@ def format_json(self, value, sort_keys=True, indent=None): except UnicodeDecodeError: return Markup(salt.utils.stringutils.to_unicode(json_txt)) + @_handle_strict_undefined def format_yaml(self, value, flow_style=True): yaml_txt = salt.utils.yaml.safe_dump( value, default_flow_style=flow_style @@ -1013,6 +1027,7 @@ def format_yaml(self, value, flow_style=True): except UnicodeDecodeError: return Markup(salt.utils.stringutils.to_unicode(yaml_txt)) + @_handle_strict_undefined def format_xml(self, value): """Render a formatted multi-line XML string from a complex Python data structure. Supports tag attributes and nested dicts/lists. @@ -1069,9 +1084,11 @@ def recurse_tree(xmliter, element=None): ).toprettyxml(indent=" ") ) + @_handle_strict_undefined def format_python(self, value): return Markup(pprint.pformat(value).strip()) + @_handle_strict_undefined def load_yaml(self, value): if isinstance(value, TemplateModule): value = str(value) @@ -1097,6 +1114,7 @@ def load_yaml(self, value): except AttributeError: raise TemplateRuntimeError(f"Unable to load yaml from {value}") + @_handle_strict_undefined def load_json(self, value): if isinstance(value, TemplateModule): value = str(value) @@ -1105,6 +1123,7 @@ def load_json(self, value): except (ValueError, TypeError, AttributeError): raise TemplateRuntimeError(f"Unable to load json from {value}") + @_handle_strict_undefined def load_text(self, value): if isinstance(value, TemplateModule): value = str(value) @@ -1231,6 +1250,7 @@ def parse_import(self, parser, converter): parser, import_node.template, f"import_{converter}", body, lineno ) + @_handle_strict_undefined def dict_to_sls_yaml_params(self, value, flow_style=False): """ .. versionadded:: 3005 From 47b3b89dd9b74dae25179af66a18a072a3425aec Mon Sep 17 00:00:00 2001 From: cmcmarrow Date: Wed, 16 Aug 2023 20:30:59 -0500 Subject: [PATCH 03/19] fix dec --- salt/utils/jinja.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 17aefc50dbd5..100c81f77fe4 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -1007,6 +1007,8 @@ def explore(data): @_handle_strict_undefined def format_json(self, value, sort_keys=True, indent=None): + if isinstance(value, jinja2.StrictUndefined): + return value json_txt = salt.utils.json.dumps( value, sort_keys=sort_keys, indent=indent ).strip() @@ -1015,8 +1017,9 @@ def format_json(self, value, sort_keys=True, indent=None): except UnicodeDecodeError: return Markup(salt.utils.stringutils.to_unicode(json_txt)) - @_handle_strict_undefined def format_yaml(self, value, flow_style=True): + if isinstance(value, jinja2.StrictUndefined): + return value yaml_txt = salt.utils.yaml.safe_dump( value, default_flow_style=flow_style ).strip() @@ -1036,6 +1039,8 @@ def format_xml(self, value): :returns: Formatted XML string rendered with newlines and indentation :rtype: str """ + if isinstance(value, jinja2.StrictUndefined): + return value def normalize_iter(value): if isinstance(value, (list, tuple)): @@ -1084,12 +1089,14 @@ def recurse_tree(xmliter, element=None): ).toprettyxml(indent=" ") ) - @_handle_strict_undefined def format_python(self, value): + if isinstance(value, jinja2.StrictUndefined): + return value return Markup(pprint.pformat(value).strip()) - @_handle_strict_undefined def load_yaml(self, value): + if isinstance(value, jinja2.StrictUndefined): + return value if isinstance(value, TemplateModule): value = str(value) try: @@ -1114,8 +1121,9 @@ def load_yaml(self, value): except AttributeError: raise TemplateRuntimeError(f"Unable to load yaml from {value}") - @_handle_strict_undefined def load_json(self, value): + if isinstance(value, jinja2.StrictUndefined): + return value if isinstance(value, TemplateModule): value = str(value) try: @@ -1123,8 +1131,9 @@ def load_json(self, value): except (ValueError, TypeError, AttributeError): raise TemplateRuntimeError(f"Unable to load json from {value}") - @_handle_strict_undefined def load_text(self, value): + if isinstance(value, jinja2.StrictUndefined): + return value if isinstance(value, TemplateModule): value = str(value) @@ -1250,7 +1259,6 @@ def parse_import(self, parser, converter): parser, import_node.template, f"import_{converter}", body, lineno ) - @_handle_strict_undefined def dict_to_sls_yaml_params(self, value, flow_style=False): """ .. versionadded:: 3005 @@ -1267,6 +1275,8 @@ def dict_to_sls_yaml_params(self, value, flow_style=False): :returns: Formatted SLS YAML string rendered with newlines and indentation """ + if isinstance(value, jinja2.StrictUndefined): + return value return self.format_yaml( [{key: val} for key, val in value.items()], flow_style=flow_style ) From 41db47c60661ce5fd6a99b96d61915246e4d8199 Mon Sep 17 00:00:00 2001 From: cmcmarrow Date: Tue, 5 Sep 2023 12:23:43 -0500 Subject: [PATCH 04/19] check for nested StrictUndefined --- salt/utils/jinja.py | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 100c81f77fe4..ba8fbfb6de4f 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -12,7 +12,7 @@ import uuid import warnings from collections import OrderedDict -from collections.abc import Hashable +from collections.abc import Hashable, Iterable, Mapping from functools import wraps from xml.dom import minidom from xml.etree.ElementTree import Element, SubElement, tostring @@ -734,10 +734,26 @@ def show_full_context(ctx): ) +def _has_strict_undefined(value): + if isinstance(value, jinja2.StrictUndefined): + return True + elif isinstance(value, Mapping): + for key, item in value.items(): + if _handle_strict_undefined(key): + return True + if _handle_strict_undefined(item): + return True + elif isinstance(value, Iterable): + for item in value: + if _handle_strict_undefined(item): + return True + return False + + def _handle_strict_undefined(function): @wraps(function) def __handle_strict_undefined(value, *args, **kwargs): - if isinstance(value, jinja2.StrictUndefined): + if _has_strict_undefined(value): return value return function(value, *args, **kwargs) @@ -1007,7 +1023,7 @@ def explore(data): @_handle_strict_undefined def format_json(self, value, sort_keys=True, indent=None): - if isinstance(value, jinja2.StrictUndefined): + if _has_strict_undefined(value): return value json_txt = salt.utils.json.dumps( value, sort_keys=sort_keys, indent=indent @@ -1018,7 +1034,7 @@ def format_json(self, value, sort_keys=True, indent=None): return Markup(salt.utils.stringutils.to_unicode(json_txt)) def format_yaml(self, value, flow_style=True): - if isinstance(value, jinja2.StrictUndefined): + if _has_strict_undefined(value): return value yaml_txt = salt.utils.yaml.safe_dump( value, default_flow_style=flow_style @@ -1039,7 +1055,7 @@ def format_xml(self, value): :returns: Formatted XML string rendered with newlines and indentation :rtype: str """ - if isinstance(value, jinja2.StrictUndefined): + if _has_strict_undefined(value): return value def normalize_iter(value): @@ -1090,12 +1106,12 @@ def recurse_tree(xmliter, element=None): ) def format_python(self, value): - if isinstance(value, jinja2.StrictUndefined): + if _has_strict_undefined(value): return value return Markup(pprint.pformat(value).strip()) def load_yaml(self, value): - if isinstance(value, jinja2.StrictUndefined): + if _has_strict_undefined(value): return value if isinstance(value, TemplateModule): value = str(value) @@ -1122,7 +1138,7 @@ def load_yaml(self, value): raise TemplateRuntimeError(f"Unable to load yaml from {value}") def load_json(self, value): - if isinstance(value, jinja2.StrictUndefined): + if _has_strict_undefined(value): return value if isinstance(value, TemplateModule): value = str(value) @@ -1132,7 +1148,7 @@ def load_json(self, value): raise TemplateRuntimeError(f"Unable to load json from {value}") def load_text(self, value): - if isinstance(value, jinja2.StrictUndefined): + if _has_strict_undefined(value): return value if isinstance(value, TemplateModule): value = str(value) @@ -1275,7 +1291,7 @@ def dict_to_sls_yaml_params(self, value, flow_style=False): :returns: Formatted SLS YAML string rendered with newlines and indentation """ - if isinstance(value, jinja2.StrictUndefined): + if _has_strict_undefined(value): return value return self.format_yaml( [{key: val} for key, val in value.items()], flow_style=flow_style From 31d474802865b91cbaecdfd253d59d7bf3bf1f23 Mon Sep 17 00:00:00 2001 From: cmcmarrow Date: Tue, 5 Sep 2023 13:38:18 -0500 Subject: [PATCH 05/19] tests --- salt/utils/jinja.py | 11 +- .../unit/utils/jinja/test_strict_undefined.py | 116 ++++++++++++++++++ 2 files changed, 122 insertions(+), 5 deletions(-) create mode 100644 tests/pytests/unit/utils/jinja/test_strict_undefined.py diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index ba8fbfb6de4f..5c1cf72f9bd0 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -12,7 +12,7 @@ import uuid import warnings from collections import OrderedDict -from collections.abc import Hashable, Iterable, Mapping +from collections.abc import Hashable, Mapping, Sequence from functools import wraps from xml.dom import minidom from xml.etree.ElementTree import Element, SubElement, tostring @@ -739,13 +739,14 @@ def _has_strict_undefined(value): return True elif isinstance(value, Mapping): for key, item in value.items(): - if _handle_strict_undefined(key): + # StrictUndefined cant be a key in dict, but still check for other mapping types + if _has_strict_undefined(key): return True - if _handle_strict_undefined(item): + if _has_strict_undefined(item): return True - elif isinstance(value, Iterable): + elif isinstance(value, Sequence) and not isinstance(value, str): for item in value: - if _handle_strict_undefined(item): + if _has_strict_undefined(item): return True return False diff --git a/tests/pytests/unit/utils/jinja/test_strict_undefined.py b/tests/pytests/unit/utils/jinja/test_strict_undefined.py new file mode 100644 index 000000000000..01ba74464a26 --- /dev/null +++ b/tests/pytests/unit/utils/jinja/test_strict_undefined.py @@ -0,0 +1,116 @@ +import math + +from jinja2 import StrictUndefined + +from salt.utils import jinja + + +def test_has_undefined(): + assert jinja._has_strict_undefined(StrictUndefined()) is True + + +def test_has_none(): + assert jinja._has_strict_undefined(None) is False + + +def test_has_bool(): + assert jinja._has_strict_undefined(True) is False + assert jinja._has_strict_undefined(False) is False + + +def test_has_int(): + for i in range(-300, 300): + assert jinja._has_strict_undefined(i) is False + assert jinja._has_strict_undefined(8231940728139704) is False + assert jinja._has_strict_undefined(-8231940728139704) is False + + +def test_has_float(): + assert jinja._has_strict_undefined(0.0) is False + assert jinja._has_strict_undefined(-0.0000000000001324) is False + assert jinja._has_strict_undefined(451452.13414) is False + assert jinja._has_strict_undefined(math.inf) is False + assert jinja._has_strict_undefined(math.nan) is False + + +def test_has_str(): + assert jinja._has_strict_undefined("") is False + assert jinja._has_strict_undefined(" ") is False + assert jinja._has_strict_undefined("\0") is False + assert jinja._has_strict_undefined("salt salt salt") is False + assert ( + jinja._has_strict_undefined('assert jinja._has_strict_undefined("") is False') + is False + ) + + +def test_hash_mapping(): + assert jinja._has_strict_undefined({}) is False + assert jinja._has_strict_undefined({True: False, False: True}) is False + assert ( + jinja._has_strict_undefined({True: False, None: True, 88: 98, "salt": 300}) + is False + ) + assert jinja._has_strict_undefined({True: StrictUndefined()}) is True + assert jinja._has_strict_undefined({True: {True: StrictUndefined()}}) is True + assert ( + jinja._has_strict_undefined( + {True: False, None: True, 88: 98, "salt": StrictUndefined()} + ) + is True + ) + + +def test_has_sequence(): + assert jinja._has_strict_undefined(()) is False + assert jinja._has_strict_undefined([]) is False + assert jinja._has_strict_undefined([None, 1, 2, 3]) is False + assert jinja._has_strict_undefined([None, 1, 2, [(None, "str")]]) is False + assert jinja._has_strict_undefined({None, 1, 2, (None, "str")}) is False + assert jinja._has_strict_undefined((StrictUndefined(),)) is True + assert ( + jinja._has_strict_undefined([None, 1, 2, [(None, StrictUndefined())]]) is True + ) + + +def test_has_iter(): + # We should not be running iters make sure iters are not called + assert jinja._has_strict_undefined(iter([])) is False + assert jinja._has_strict_undefined(iter({1: 1})) is False + assert ( + jinja._has_strict_undefined(iter([None, "\0", StrictUndefined(), False])) + is False + ) + assert jinja._has_strict_undefined(iter({1: StrictUndefined()})) is False + + +def test_full(): + assert ( + jinja._has_strict_undefined( + {1: 45, None: (0, 1, [[{"": {"": (3.2, 34.2)}}]], False)} + ) + is False + ) + assert ( + jinja._has_strict_undefined( + {1: 45, None: (0, 1, [[{"": {"": (StrictUndefined(), 34.2)}}]], False)} + ) + is True + ) + + +@jinja._handle_strict_undefined +def _handle_test_helper(value, k=34, *args, **kwargs): + return None + + +def test_handle_strict_undefined(): + assert _handle_test_helper(False) is None + assert _handle_test_helper((1, 2, 3)) is None + assert _handle_test_helper({1, 2, 3}) is None + assert _handle_test_helper([1, 2, 3, {"": None, "str": 0.0}]) is None + # Note StrictUndefined overrides __eq__ + assert isinstance(_handle_test_helper(StrictUndefined()), StrictUndefined) + assert isinstance( + _handle_test_helper([1, 2, 3, {"": None, "str": StrictUndefined()}]), list + ) From 844f381ec70498fd065e056bb6b279e1210f6b78 Mon Sep 17 00:00:00 2001 From: cmcmarrow Date: Tue, 5 Sep 2023 13:42:41 -0500 Subject: [PATCH 06/19] remove dec --- salt/utils/jinja.py | 1 - 1 file changed, 1 deletion(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 5c1cf72f9bd0..fb03aac9ceac 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -1047,7 +1047,6 @@ def format_yaml(self, value, flow_style=True): except UnicodeDecodeError: return Markup(salt.utils.stringutils.to_unicode(yaml_txt)) - @_handle_strict_undefined def format_xml(self, value): """Render a formatted multi-line XML string from a complex Python data structure. Supports tag attributes and nested dicts/lists. From 77f886015416fcc866f9072b06536ff1e64bc58f Mon Sep 17 00:00:00 2001 From: cmcmarrow Date: Wed, 6 Sep 2023 23:54:47 -0500 Subject: [PATCH 07/19] wrtie tests and catch nested errors --- changelog/64915.fixed.md | 1 + salt/utils/jinja.py | 68 +++-- .../utils/jinja/test_jinja_custom_filters.py | 267 ++++++++++++++++++ .../unit/utils/jinja/test_strict_undefined.py | 116 -------- 4 files changed, 308 insertions(+), 144 deletions(-) create mode 100644 changelog/64915.fixed.md create mode 100644 tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py delete mode 100644 tests/pytests/unit/utils/jinja/test_strict_undefined.py diff --git a/changelog/64915.fixed.md b/changelog/64915.fixed.md new file mode 100644 index 000000000000..44a599c7eea7 --- /dev/null +++ b/changelog/64915.fixed.md @@ -0,0 +1 @@ +Catch StrictUndefined in salt jinja custom filters. diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index fb03aac9ceac..2c6fe36979d3 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -734,33 +734,54 @@ def show_full_context(ctx): ) -def _has_strict_undefined(value): +def __get_strict_undefined(value, ids): + if id(value) in ids: + return [] + ids.add(id(value)) + undefined = [] if isinstance(value, jinja2.StrictUndefined): - return True + undefined.append(value) elif isinstance(value, Mapping): for key, item in value.items(): # StrictUndefined cant be a key in dict, but still check for other mapping types - if _has_strict_undefined(key): - return True - if _has_strict_undefined(item): - return True + undefined.extend(__get_strict_undefined(key, ids)) + undefined.extend(__get_strict_undefined(item, ids)) elif isinstance(value, Sequence) and not isinstance(value, str): for item in value: - if _has_strict_undefined(item): - return True - return False + undefined.extend(__get_strict_undefined(item, ids)) + return undefined + + +def _get_strict_undefined(value): + return tuple(__get_strict_undefined(value, set())) + + +def _join_strict_undefined(undefined): + return jinja2.StrictUndefined("\n".join(u._undefined_message for u in undefined)) def _handle_strict_undefined(function): @wraps(function) def __handle_strict_undefined(value, *args, **kwargs): - if _has_strict_undefined(value): - return value + undefined = _get_strict_undefined(value) + if undefined: + return _join_strict_undefined(undefined) return function(value, *args, **kwargs) return __handle_strict_undefined +def _handle_method_strict_undefined(function): + @wraps(function) + def __handle_method_strict_undefined(self, value, *args, **kwargs): + undefined = _get_strict_undefined(value) + if undefined: + return _join_strict_undefined(undefined) + return function(self, value, *args, **kwargs) + + return __handle_method_strict_undefined + + class SerializerExtension(Extension): ''' Yaml and Json manipulation. @@ -1022,10 +1043,8 @@ def explore(data): return explore(data) - @_handle_strict_undefined + @_handle_method_strict_undefined def format_json(self, value, sort_keys=True, indent=None): - if _has_strict_undefined(value): - return value json_txt = salt.utils.json.dumps( value, sort_keys=sort_keys, indent=indent ).strip() @@ -1034,9 +1053,8 @@ def format_json(self, value, sort_keys=True, indent=None): except UnicodeDecodeError: return Markup(salt.utils.stringutils.to_unicode(json_txt)) + @_handle_method_strict_undefined def format_yaml(self, value, flow_style=True): - if _has_strict_undefined(value): - return value yaml_txt = salt.utils.yaml.safe_dump( value, default_flow_style=flow_style ).strip() @@ -1047,6 +1065,7 @@ def format_yaml(self, value, flow_style=True): except UnicodeDecodeError: return Markup(salt.utils.stringutils.to_unicode(yaml_txt)) + @_handle_method_strict_undefined def format_xml(self, value): """Render a formatted multi-line XML string from a complex Python data structure. Supports tag attributes and nested dicts/lists. @@ -1055,8 +1074,6 @@ def format_xml(self, value): :returns: Formatted XML string rendered with newlines and indentation :rtype: str """ - if _has_strict_undefined(value): - return value def normalize_iter(value): if isinstance(value, (list, tuple)): @@ -1105,14 +1122,12 @@ def recurse_tree(xmliter, element=None): ).toprettyxml(indent=" ") ) + @_handle_method_strict_undefined def format_python(self, value): - if _has_strict_undefined(value): - return value return Markup(pprint.pformat(value).strip()) + @_handle_method_strict_undefined def load_yaml(self, value): - if _has_strict_undefined(value): - return value if isinstance(value, TemplateModule): value = str(value) try: @@ -1137,9 +1152,8 @@ def load_yaml(self, value): except AttributeError: raise TemplateRuntimeError(f"Unable to load yaml from {value}") + @_handle_method_strict_undefined def load_json(self, value): - if _has_strict_undefined(value): - return value if isinstance(value, TemplateModule): value = str(value) try: @@ -1147,9 +1161,8 @@ def load_json(self, value): except (ValueError, TypeError, AttributeError): raise TemplateRuntimeError(f"Unable to load json from {value}") + @_handle_method_strict_undefined def load_text(self, value): - if _has_strict_undefined(value): - return value if isinstance(value, TemplateModule): value = str(value) @@ -1275,6 +1288,7 @@ def parse_import(self, parser, converter): parser, import_node.template, f"import_{converter}", body, lineno ) + @_handle_method_strict_undefined def dict_to_sls_yaml_params(self, value, flow_style=False): """ .. versionadded:: 3005 @@ -1291,8 +1305,6 @@ def dict_to_sls_yaml_params(self, value, flow_style=False): :returns: Formatted SLS YAML string rendered with newlines and indentation """ - if _has_strict_undefined(value): - return value return self.format_yaml( [{key: val} for key, val in value.items()], flow_style=flow_style ) diff --git a/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py b/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py new file mode 100644 index 000000000000..e6c4f67be371 --- /dev/null +++ b/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py @@ -0,0 +1,267 @@ +import math + +import jinja2 +import pytest +from jinja2 import StrictUndefined + +from salt.utils import jinja + + +def test_get_undefined(): + assert len(jinja._get_strict_undefined(StrictUndefined())) == 1 + + +def test_get_none(): + assert len(jinja._get_strict_undefined(None)) == 0 + + +def test_get_bool(): + assert len(jinja._get_strict_undefined(True)) == 0 + assert len(jinja._get_strict_undefined(False)) == 0 + + +def test_get_int(): + for i in range(-300, 300): + assert len(jinja._get_strict_undefined(i)) == 0 + assert len(jinja._get_strict_undefined(8231940728139704)) == 0 + assert len(jinja._get_strict_undefined(-8231940728139704)) == 0 + + +def test_get_float(): + assert bool(jinja._get_strict_undefined(0.0)) == 0 + assert bool(jinja._get_strict_undefined(-0.0000000000001324)) == 0 + assert bool(jinja._get_strict_undefined(451452.13414)) == 0 + assert bool(jinja._get_strict_undefined(math.inf)) == 0 + assert bool(jinja._get_strict_undefined(math.nan)) == 0 + + +def test_get_str(): + assert len(jinja._get_strict_undefined("")) == 0 + assert len(jinja._get_strict_undefined(" ")) == 0 + assert len(jinja._get_strict_undefined("\0")) == 0 + assert len(jinja._get_strict_undefined("salt salt salt")) == 0 + assert ( + len( + jinja._get_strict_undefined( + 'assert jinja._has_strict_undefined("") is False' + ) + ) + == 0 + ) + + +def test_get_mapping(): + assert len(jinja._get_strict_undefined({})) == 0 + assert len(jinja._get_strict_undefined({True: False, False: True})) == 0 + assert ( + len(jinja._get_strict_undefined({True: False, None: True, 88: 98, "salt": 300})) + == 0 + ) + assert len(jinja._get_strict_undefined({True: StrictUndefined()})) == 1 + assert len(jinja._get_strict_undefined({True: {True: StrictUndefined()}})) == 1 + assert ( + len( + jinja._get_strict_undefined( + {True: False, None: True, 88: 98, "salt": StrictUndefined()} + ) + ) + == 1 + ) + + +def test_get_sequence(): + assert len(jinja._get_strict_undefined(())) == 0 + assert len(jinja._get_strict_undefined([])) == 0 + assert len(jinja._get_strict_undefined([None, 1, 2, 3])) == 0 + assert len(jinja._get_strict_undefined([None, 1, 2, [(None, "str")]])) == 0 + assert len(jinja._get_strict_undefined({None, 1, 2, (None, "str")})) == 0 + assert len(jinja._get_strict_undefined((StrictUndefined(),))) == 1 + assert ( + len(jinja._get_strict_undefined([None, 1, 2, [(None, StrictUndefined())]])) == 1 + ) + + +def test_get_iter(): + # We should not be running iters make sure iters are not called + assert len(jinja._get_strict_undefined(iter([]))) == 0 + assert len(jinja._get_strict_undefined(iter({1: 1}))) == 0 + assert ( + len(jinja._get_strict_undefined(iter([None, "\0", StrictUndefined(), False]))) + == 0 + ) + assert len(jinja._get_strict_undefined(iter({1: StrictUndefined()}))) == 0 + + +def test_full(): + assert ( + len( + jinja._get_strict_undefined( + {1: 45, None: (0, 1, [[{"": {"": (3.2, 34.2)}}]], False)} + ) + ) + == 0 + ) + assert ( + len( + jinja._get_strict_undefined( + {1: 45, None: (0, 1, [[{"": {"": (StrictUndefined(), 34.2)}}]], False)} + ) + ) + == 1 + ) + assert ( + len( + jinja._get_strict_undefined( + { + 1: 45, + None: ( + 0, + 1, + [ + [ + { + "": {"": (StrictUndefined(), 34.2)}, + 45: StrictUndefined(), + } + ] + ], + False, + ), + } + ) + ) + == 2 + ) + assert ( + len( + jinja._get_strict_undefined( + { + 1: 45, + None: ( + 0, + 1, + [ + StrictUndefined(), + [ + { + "": {"": (StrictUndefined(), 34.2)}, + 45: StrictUndefined(), + } + ], + StrictUndefined(), + ], + False, + ), + } + ) + ) + == 4 + ) + + +@jinja._handle_strict_undefined +def _handle_test_helper(value, k=34, *args, **kwargs): + return None + + +def test_handle_strict_undefined(): + assert _handle_test_helper(False) is None + assert _handle_test_helper((1, 2, 3)) is None + assert _handle_test_helper({1, 2, 3}) is None + assert _handle_test_helper([1, 2, 3, {"": None, "str": 0.0}]) is None + assert isinstance(_handle_test_helper(StrictUndefined()), StrictUndefined) + assert isinstance( + _handle_test_helper( + [1, 2, 3, {"": StrictUndefined(), "str": StrictUndefined()}] + ), + StrictUndefined, + ) + + +def _render(yaml): + return ( + jinja2.Environment( + extensions=[jinja.SerializerExtension], undefined=jinja2.StrictUndefined + ) + .from_string(yaml) + .render() + ) + + +def _render_fail(yaml): + with pytest.raises(jinja2.exceptions.UndefinedError): + _render(yaml) + + +YAML_SLS_ERROR = """ +{%- set ports = {'http': 80} %} + +huh: + test.configurable_test_state: + - changes: true + - result: true + - comment: https port is {{ ports['https'] | yaml }} +""" + + +YAML_SLS = """ +{%- set ports = {'https': 80} %} + +huh: + test.configurable_test_state: + - changes: true + - result: true + - comment: https port is {{ ports['https'] | yaml }} +""" + + +YAML_SLS_RIGHT = """ + +huh: + test.configurable_test_state: + - changes: true + - result: true + - comment: https port is 80""" + + +def test_yaml(): + _render_fail(YAML_SLS_ERROR) + assert _render(YAML_SLS) == YAML_SLS_RIGHT + + +JSON_SLS_ERROR = """ +{%- set ports = {'http': 80} %} + +huh: + test.configurable_test_state: + - changes: true + - result: true + - comment: https port is {{ [ports['https23'], ports['https']] | json }} + - comment2: https port is {{ [666, ports['https2']] | json }} + - comment3: https port is {{ [666, ports['https2']] | json }} +""" + + +JSON_SLS = """ +{%- set ports = {'https': 80} %} + +huh: + test.configurable_test_state: + - changes: true + - result: true + - comment: https port is {{ [666, ports['https']] | json }} +""" + + +JSON_SLS_RIGHT = """ + +huh: + test.configurable_test_state: + - changes: true + - result: true + - comment: https port is [666, 80]""" + + +def test_json(): + _render_fail(JSON_SLS_ERROR) + assert _render(JSON_SLS) == JSON_SLS_RIGHT diff --git a/tests/pytests/unit/utils/jinja/test_strict_undefined.py b/tests/pytests/unit/utils/jinja/test_strict_undefined.py deleted file mode 100644 index 01ba74464a26..000000000000 --- a/tests/pytests/unit/utils/jinja/test_strict_undefined.py +++ /dev/null @@ -1,116 +0,0 @@ -import math - -from jinja2 import StrictUndefined - -from salt.utils import jinja - - -def test_has_undefined(): - assert jinja._has_strict_undefined(StrictUndefined()) is True - - -def test_has_none(): - assert jinja._has_strict_undefined(None) is False - - -def test_has_bool(): - assert jinja._has_strict_undefined(True) is False - assert jinja._has_strict_undefined(False) is False - - -def test_has_int(): - for i in range(-300, 300): - assert jinja._has_strict_undefined(i) is False - assert jinja._has_strict_undefined(8231940728139704) is False - assert jinja._has_strict_undefined(-8231940728139704) is False - - -def test_has_float(): - assert jinja._has_strict_undefined(0.0) is False - assert jinja._has_strict_undefined(-0.0000000000001324) is False - assert jinja._has_strict_undefined(451452.13414) is False - assert jinja._has_strict_undefined(math.inf) is False - assert jinja._has_strict_undefined(math.nan) is False - - -def test_has_str(): - assert jinja._has_strict_undefined("") is False - assert jinja._has_strict_undefined(" ") is False - assert jinja._has_strict_undefined("\0") is False - assert jinja._has_strict_undefined("salt salt salt") is False - assert ( - jinja._has_strict_undefined('assert jinja._has_strict_undefined("") is False') - is False - ) - - -def test_hash_mapping(): - assert jinja._has_strict_undefined({}) is False - assert jinja._has_strict_undefined({True: False, False: True}) is False - assert ( - jinja._has_strict_undefined({True: False, None: True, 88: 98, "salt": 300}) - is False - ) - assert jinja._has_strict_undefined({True: StrictUndefined()}) is True - assert jinja._has_strict_undefined({True: {True: StrictUndefined()}}) is True - assert ( - jinja._has_strict_undefined( - {True: False, None: True, 88: 98, "salt": StrictUndefined()} - ) - is True - ) - - -def test_has_sequence(): - assert jinja._has_strict_undefined(()) is False - assert jinja._has_strict_undefined([]) is False - assert jinja._has_strict_undefined([None, 1, 2, 3]) is False - assert jinja._has_strict_undefined([None, 1, 2, [(None, "str")]]) is False - assert jinja._has_strict_undefined({None, 1, 2, (None, "str")}) is False - assert jinja._has_strict_undefined((StrictUndefined(),)) is True - assert ( - jinja._has_strict_undefined([None, 1, 2, [(None, StrictUndefined())]]) is True - ) - - -def test_has_iter(): - # We should not be running iters make sure iters are not called - assert jinja._has_strict_undefined(iter([])) is False - assert jinja._has_strict_undefined(iter({1: 1})) is False - assert ( - jinja._has_strict_undefined(iter([None, "\0", StrictUndefined(), False])) - is False - ) - assert jinja._has_strict_undefined(iter({1: StrictUndefined()})) is False - - -def test_full(): - assert ( - jinja._has_strict_undefined( - {1: 45, None: (0, 1, [[{"": {"": (3.2, 34.2)}}]], False)} - ) - is False - ) - assert ( - jinja._has_strict_undefined( - {1: 45, None: (0, 1, [[{"": {"": (StrictUndefined(), 34.2)}}]], False)} - ) - is True - ) - - -@jinja._handle_strict_undefined -def _handle_test_helper(value, k=34, *args, **kwargs): - return None - - -def test_handle_strict_undefined(): - assert _handle_test_helper(False) is None - assert _handle_test_helper((1, 2, 3)) is None - assert _handle_test_helper({1, 2, 3}) is None - assert _handle_test_helper([1, 2, 3, {"": None, "str": 0.0}]) is None - # Note StrictUndefined overrides __eq__ - assert isinstance(_handle_test_helper(StrictUndefined()), StrictUndefined) - assert isinstance( - _handle_test_helper([1, 2, 3, {"": None, "str": StrictUndefined()}]), list - ) From daba3a909f0d1cdb39c019a4388a6c6f19a31f84 Mon Sep 17 00:00:00 2001 From: cmcmarrow Date: Wed, 18 Oct 2023 11:31:52 -0500 Subject: [PATCH 08/19] add python tests --- .../utils/jinja/test_jinja_custom_filters.py | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py b/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py index e6c4f67be371..38abdfd53b2f 100644 --- a/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py +++ b/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py @@ -265,3 +265,40 @@ def test_yaml(): def test_json(): _render_fail(JSON_SLS_ERROR) assert _render(JSON_SLS) == JSON_SLS_RIGHT + +PYTHON_SLS_ERROR = """ +{%- set ports = {'http': 80} %} + +huh: + test.configurable_test_state: + - changes: true + - result: true + - comment: https port is {{ [ports['https23'], ports['https']] | python }} + - comment2: https port is {{ [666 + 1, ports['https2']] | python }} + - comment3: https port is {{ [666 + 1, ports['https2']] | python }} +""" + + +PYTHON_SLS = """ +{%- set ports = {'https': 80} %} + +huh: + test.configurable_test_state: + - changes: true + - result: true + - comment: https port is {{ [666 + 1, ports['https']] | python }} +""" + + +PYTHON_SLS_RIGHT = """ + +huh: + test.configurable_test_state: + - changes: true + - result: true + - comment: https port is [667, 80]""" + + +def test_python(): + _render_fail(PYTHON_SLS_ERROR) + assert _render(PYTHON_SLS) == PYTHON_SLS_RIGHT From 37d7a9078a99ff6f3748b4f2640e5e82aee77ccc Mon Sep 17 00:00:00 2001 From: twangboy Date: Mon, 18 May 2026 10:57:58 -0600 Subject: [PATCH 09/19] Rebase StrictUndefined jinja fix onto 3006.x; guard tojson filter Replay PR #65004 onto current upstream/3006.x and apply the same StrictUndefined handling to the standalone tojson filter for parity with | json. --- salt/utils/jinja.py | 39 ++++++++++--------- .../utils/jinja/test_jinja_custom_filters.py | 25 ++++++++++++ 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/salt/utils/jinja.py b/salt/utils/jinja.py index 2c6fe36979d3..a9f89d218453 100644 --- a/salt/utils/jinja.py +++ b/salt/utils/jinja.py @@ -400,25 +400,6 @@ def indent(s, width=4, first=False, blank=False, indentfirst=None): return rv -@jinja_filter("tojson") -def tojson(val, indent=None, **options): - """ - Implementation of tojson filter (only present in Jinja 2.9 and later). - Unlike the Jinja built-in filter, this allows arbitrary options to be - passed in to the underlying JSON library. - """ - options.setdefault("ensure_ascii", True) - if indent is not None: - options["indent"] = indent - return ( - salt.utils.json.dumps(val, **options) - .replace("<", "\\u003c") - .replace(">", "\\u003e") - .replace("&", "\\u0026") - .replace("'", "\\u0027") - ) - - @jinja_filter("quote") def quote(txt): """ @@ -782,6 +763,26 @@ def __handle_method_strict_undefined(self, value, *args, **kwargs): return __handle_method_strict_undefined +@jinja_filter("tojson") +@_handle_strict_undefined +def tojson(val, indent=None, **options): + """ + Implementation of tojson filter (only present in Jinja 2.9 and later). + Unlike the Jinja built-in filter, this allows arbitrary options to be + passed in to the underlying JSON library. + """ + options.setdefault("ensure_ascii", True) + if indent is not None: + options["indent"] = indent + return ( + salt.utils.json.dumps(val, **options) + .replace("<", "\\u003c") + .replace(">", "\\u003e") + .replace("&", "\\u0026") + .replace("'", "\\u0027") + ) + + class SerializerExtension(Extension): ''' Yaml and Json manipulation. diff --git a/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py b/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py index 38abdfd53b2f..159dabb9b035 100644 --- a/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py +++ b/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py @@ -188,6 +188,14 @@ def _render(yaml): ) +def _render_with_salt_filters(template_src): + env = jinja2.Environment( + extensions=[jinja.SerializerExtension], undefined=jinja2.StrictUndefined + ) + env.filters.update(jinja.jinja_filter.salt_jinja_filters) + return env.from_string(template_src).render() + + def _render_fail(yaml): with pytest.raises(jinja2.exceptions.UndefinedError): _render(yaml) @@ -266,6 +274,23 @@ def test_json(): _render_fail(JSON_SLS_ERROR) assert _render(JSON_SLS) == JSON_SLS_RIGHT + +def test_tojson(): + tojson_error = """ +{%- set ports = {'http': 80} %} +x: {{ ports['https'] | tojson }} +""" + tojson_ok = """ +{%- set ports = {'https': 80} %} +x: {{ ports['https'] | tojson }} +""" + tojson_right = """ +x: 80""" + with pytest.raises(jinja2.exceptions.UndefinedError): + _render_with_salt_filters(tojson_error) + assert _render_with_salt_filters(tojson_ok).strip() == tojson_right.strip() + + PYTHON_SLS_ERROR = """ {%- set ports = {'http': 80} %} From 9df975ec1c698f451cdeb7f7fd2f0be0adb935f6 Mon Sep 17 00:00:00 2001 From: twangboy Date: Mon, 18 May 2026 11:02:44 -0600 Subject: [PATCH 10/19] test(jinja): cover StrictUndefined yaml path under SandboxedEnvironment Production renders SLS with SandboxedEnvironment; assert the yaml filter still raises UndefinedError for missing keys in that setup. --- .../utils/jinja/test_jinja_custom_filters.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py b/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py index 159dabb9b035..b71b2a42cfb8 100644 --- a/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py +++ b/tests/pytests/unit/utils/jinja/test_jinja_custom_filters.py @@ -3,6 +3,7 @@ import jinja2 import pytest from jinja2 import StrictUndefined +from jinja2.sandbox import SandboxedEnvironment from salt.utils import jinja @@ -201,6 +202,17 @@ def _render_fail(yaml): _render(yaml) +def _render_sandboxed(template_src): + """Match production: salt.utils.templates uses SandboxedEnvironment + StrictUndefined.""" + return ( + SandboxedEnvironment( + extensions=[jinja.SerializerExtension], undefined=jinja2.StrictUndefined + ) + .from_string(template_src) + .render() + ) + + YAML_SLS_ERROR = """ {%- set ports = {'http': 80} %} @@ -237,6 +249,12 @@ def test_yaml(): assert _render(YAML_SLS) == YAML_SLS_RIGHT +def test_yaml_strict_undefined_sandboxed_environment(): + with pytest.raises(jinja2.exceptions.UndefinedError): + _render_sandboxed(YAML_SLS_ERROR) + assert _render_sandboxed(YAML_SLS) == YAML_SLS_RIGHT + + JSON_SLS_ERROR = """ {%- set ports = {'http': 80} %} From 01e33a432fdef034eb5c34323c2af5ede0088b7a Mon Sep 17 00:00:00 2001 From: twangboy Date: Mon, 18 May 2026 12:03:55 -0600 Subject: [PATCH 11/19] Fix OSError "The operation completed successfully" in CreateProcessWithTokenW `advapi32` is loaded with `use_last_error=True`, so the error code after a failed ctypes call must be read with `ctypes.get_last_error()`, not `win32api.GetLastError()`. The Windows live slot is reset to 0 by Python internals before the old code could read it, producing the misleading "The operation completed successfully" exception that prevented the child process from ever being resumed via ResumeThread. (#57848) --- changelog/57848.fixed.md | 1 + salt/platform/win.py | 5 +---- tests/pytests/unit/platform/test_win.py | 26 +++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 changelog/57848.fixed.md diff --git a/changelog/57848.fixed.md b/changelog/57848.fixed.md new file mode 100644 index 000000000000..051d5e51e308 --- /dev/null +++ b/changelog/57848.fixed.md @@ -0,0 +1 @@ +Fixed `OSError: The operation completed successfully` raised by `CreateProcessWithTokenW` on Windows when the underlying advapi32 call fails. The error code is now read from `ctypes.get_last_error()` (the ctypes-saved slot) instead of `win32api.GetLastError()` (the live Windows slot, which may be reset to 0 before it is read). diff --git a/salt/platform/win.py b/salt/platform/win.py index c61172df1dc1..b39a14bd638e 100644 --- a/salt/platform/win.py +++ b/salt/platform/win.py @@ -1165,10 +1165,7 @@ def CreateProcessWithTokenW( ctypes.byref(process_info), ) if ret == 0: - winerr = win32api.GetLastError() - exc = OSError(win32api.FormatMessage(winerr)) - exc.winerror = winerr - raise exc + raise ctypes.WinError(ctypes.get_last_error()) return process_info diff --git a/tests/pytests/unit/platform/test_win.py b/tests/pytests/unit/platform/test_win.py index 5eed38bea4ab..2b739410524b 100644 --- a/tests/pytests/unit/platform/test_win.py +++ b/tests/pytests/unit/platform/test_win.py @@ -1,5 +1,7 @@ import base64 +import ctypes import subprocess +from unittest.mock import patch import pytest @@ -120,3 +122,27 @@ def test_prepend_cmd_unquoted_payload(): assert win.prepend_cmd("cmd.exe", compound, quote_c_payload=False) == ( f"cmd.exe /c {compound}" ) + + +def test_create_process_with_token_w_raises_real_error(): + """ + When the underlying advapi32 call fails, CreateProcessWithTokenW must raise + OSError with the code from ctypes.get_last_error() (the ctypes-saved slot), + not from win32api.GetLastError() (the live Windows slot, which may already + be 0 by the time Python reads it). + + Regression: github.com/saltstack/salt/issues/57848 + """ + ERROR_ACCESS_DENIED = 5 + + def _fake_advapi32_call(*args, **kwargs): + ctypes.set_last_error(ERROR_ACCESS_DENIED) + return 0 # FALSE — failure + + with patch.object( + win.advapi32, "CreateProcessWithTokenW", side_effect=_fake_advapi32_call + ): + with pytest.raises(OSError) as exc_info: + win.CreateProcessWithTokenW(token=1, commandline="whoami") + + assert exc_info.value.winerror == ERROR_ACCESS_DENIED From aab49463ec99b9eda27b46e9bc168e101503c88e Mon Sep 17 00:00:00 2001 From: twangboy Date: Mon, 18 May 2026 12:24:18 -0600 Subject: [PATCH 12/19] Fix failing lint tests --- tests/pytests/unit/platform/test_win.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pytests/unit/platform/test_win.py b/tests/pytests/unit/platform/test_win.py index 2b739410524b..f4a7a629d28d 100644 --- a/tests/pytests/unit/platform/test_win.py +++ b/tests/pytests/unit/platform/test_win.py @@ -1,7 +1,7 @@ import base64 import ctypes import subprocess -from unittest.mock import patch +from tests.support.mock import patch import pytest From 15684ce3f56c81eb6a1e13d516a2427d79f2ac2a Mon Sep 17 00:00:00 2001 From: twangboy Date: Mon, 18 May 2026 13:13:14 -0600 Subject: [PATCH 13/19] Fix pre-commit --- tests/pytests/unit/platform/test_win.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/pytests/unit/platform/test_win.py b/tests/pytests/unit/platform/test_win.py index f4a7a629d28d..bfbb1af13f56 100644 --- a/tests/pytests/unit/platform/test_win.py +++ b/tests/pytests/unit/platform/test_win.py @@ -1,11 +1,11 @@ import base64 import ctypes import subprocess -from tests.support.mock import patch import pytest import salt.utils.platform +from tests.support.mock import patch if salt.utils.platform.is_windows(): from salt.platform import win From 4ae77e596628b795007296d4b7406b59fb16bef6 Mon Sep 17 00:00:00 2001 From: twangboy Date: Mon, 18 May 2026 15:43:01 -0600 Subject: [PATCH 14/19] Fix LGPO duplicate policy error when ADMX files share an identical path When multiple ADMX files (e.g. TerminalServer.admx and TerminalServer-Server.admx on Windows Server) define the same policy display name resolving to an identical full path, _lookup_admin_template now deduplicates the matches and returns success rather than a spurious "multiple policies" error. Closes #62732 --- changelog/62732.fixed.md | 1 + salt/modules/win_lgpo.py | 40 +++++- .../modules/win_lgpo/test_get_policy_info.py | 24 ++++ tests/pytests/unit/modules/test_win_lgpo.py | 136 ++++++++++++++++++ 4 files changed, 196 insertions(+), 5 deletions(-) create mode 100644 changelog/62732.fixed.md create mode 100644 tests/pytests/unit/modules/test_win_lgpo.py diff --git a/changelog/62732.fixed.md b/changelog/62732.fixed.md new file mode 100644 index 000000000000..a000b21955af --- /dev/null +++ b/changelog/62732.fixed.md @@ -0,0 +1 @@ +Fixed LGPO ``get_policy_info`` incorrectly returning a "multiple policies" error when duplicate ADMX policy definitions (e.g. ``TerminalServer.admx`` and ``TerminalServer-Server.admx``) resolve to the same full path. diff --git a/salt/modules/win_lgpo.py b/salt/modules/win_lgpo.py index 5fe07f6ba517..f4d12bd68e01 100644 --- a/salt/modules/win_lgpo.py +++ b/salt/modules/win_lgpo.py @@ -8563,6 +8563,7 @@ def _lookup_admin_template(policy_name, policy_class, adml_language="en-US"): ) return False, None, [], msg else: + all_paths = [] for possible_policy in admx_search_results: this_parent_list = _build_parent_list( policy_definition=possible_policy, @@ -8571,12 +8572,41 @@ def _lookup_admin_template(policy_name, policy_class, adml_language="en-US"): ) this_parent_list.reverse() this_parent_list.append(policy_name) - if suggested_policies: - suggested_policies = ", ".join( - [suggested_policies, "\\".join(this_parent_list)] + all_paths.append("\\".join(this_parent_list)) + unique_paths = list(dict.fromkeys(all_paths)) + if len(unique_paths) == 1: + # All matches resolve to the same full path (e.g. + # duplicate policy definitions across ADMX files + # like TerminalServer.admx and + # TerminalServer-Server.admx). Treat as a single + # unambiguous policy. + search_result = admx_search_results[0] + if "name" in search_result.attrib: + policy_display_name = _getFullPolicyName( + policy_item=search_result, + policy_name=search_result.attrib["name"], + return_full_policy_names=True, + adml_language=adml_language, ) - else: - suggested_policies = "\\".join(this_parent_list) + policy_aliases.append(policy_display_name) + policy_aliases.append(search_result.attrib["name"]) + full_path_list = _build_parent_list( + policy_definition=search_result, + return_full_policy_names=True, + adml_language=adml_language, + ) + full_path_list.reverse() + full_path_list.append(policy_display_name) + policy_aliases.append("\\".join(full_path_list)) + return True, search_result, policy_aliases, None + else: + for full_path in all_paths: + if suggested_policies: + suggested_policies = ", ".join( + [suggested_policies, full_path] + ) + else: + suggested_policies = full_path if suggested_policies: msg = ( 'ADML policy name "{}" is used as the display name for ' diff --git a/tests/pytests/functional/modules/win_lgpo/test_get_policy_info.py b/tests/pytests/functional/modules/win_lgpo/test_get_policy_info.py index 09de8845f236..b3a76becd43c 100644 --- a/tests/pytests/functional/modules/win_lgpo/test_get_policy_info.py +++ b/tests/pytests/functional/modules/win_lgpo/test_get_policy_info.py @@ -2,6 +2,7 @@ Unit tests for the LGPO module """ +import os import platform import pytest @@ -55,3 +56,26 @@ def test_61859(lgpo): policy_class="machine", ) assert result["message"] == expected + + +_TS_SERVER_ADMX = r"C:\Windows\PolicyDefinitions\TerminalServer-Server.admx" + + +@pytest.mark.skipif( + not os.path.exists(_TS_SERVER_ADMX), + reason="TerminalServer-Server.admx only present on Windows Server editions", +) +def test_62732_duplicate_policy_identical_paths(lgpo): + """ + On Windows Server, TerminalServer.admx and TerminalServer-Server.admx + both define "Do not allow Clipboard redirection" with the same full path. + get_policy_info should resolve it as a single unambiguous policy rather + than returning a "multiple policies" error. + """ + result = lgpo.get_policy_info( + policy_name="Do not allow Clipboard redirection", + policy_class="Machine", + ) + assert result["policy_found"] is True + assert not result["message"] + assert result["policy_name"] == "Do not allow Clipboard redirection" diff --git a/tests/pytests/unit/modules/test_win_lgpo.py b/tests/pytests/unit/modules/test_win_lgpo.py new file mode 100644 index 000000000000..cfd8beef650a --- /dev/null +++ b/tests/pytests/unit/modules/test_win_lgpo.py @@ -0,0 +1,136 @@ +""" +Unit tests for salt.modules.win_lgpo +""" + +import pytest + +import salt.modules.win_lgpo as win_lgpo +from tests.support.mock import MagicMock, patch + +pytestmark = [ + pytest.mark.windows_whitelisted, + pytest.mark.skip_unless_on_windows, +] + +_ADML_TAG = "{http://www.microsoft.com/GroupPolicy/2006/07/PolicyDefinitions}string" + + +def _make_adml(policy_id): + fake = MagicMock() + fake.tag = _ADML_TAG + fake.attrib = {"id": policy_id} + return fake + + +def _make_admx(name): + fake = MagicMock() + fake.attrib = {"name": name, "class": "Machine"} + return fake + + +def test_duplicate_policies_identical_paths_returns_success(): + """ + When multiple ADMX entries share the same display name but all resolve + to the same full path (e.g. TerminalServer.admx and + TerminalServer-Server.admx for "Do not allow Clipboard redirection"), + _lookup_admin_template should return True with policy info rather than + a "multiple policies" error. + """ + policy_name = "Do not allow Clipboard redirection" + policy_class = "Machine" + + fake_adml = _make_adml("TS_CLIENT_CLIPBOARD") + fake_policy_1 = _make_admx("TS_CLIENT_CLIPBOARD") + fake_policy_2 = _make_admx("TS_CLIENT_CLIPBOARD") + + # _build_parent_list returns innermost-first; the code reverses it + # in-place, so return a fresh list on every call. + shared_parent = [ + "Device and Resource Redirection", + "Remote Desktop Session Host", + "Remote Desktop Services", + "Windows Components", + ] + + with ( + patch.object(win_lgpo, "_get_policy_definitions", return_value=MagicMock()), + patch.object(win_lgpo, "_get_policy_resources", return_value=MagicMock()), + patch.object(win_lgpo, "ADMX_SEARCH_XPATH", MagicMock(return_value=[])), + patch.object( + win_lgpo, + "ADML_SEARCH_XPATH", + MagicMock(return_value=[fake_adml]), + ), + patch.object( + win_lgpo, + "ADMX_DISPLAYNAME_SEARCH_XPATH", + MagicMock(return_value=[fake_policy_1, fake_policy_2]), + ), + patch.object( + win_lgpo, + "_build_parent_list", + side_effect=lambda **_: list(shared_parent), + ), + patch.object(win_lgpo, "_getFullPolicyName", return_value=policy_name), + ): + found, policy_xml, aliases, msg = win_lgpo._lookup_admin_template( + policy_name, policy_class + ) + + assert found is True + assert msg is None + assert policy_xml is fake_policy_1 + assert policy_name in aliases + + +def test_duplicate_policies_distinct_paths_returns_error(): + """ + When multiple ADMX entries share the same display name but resolve to + genuinely different full paths (e.g. "Access data sources across + domains" in multiple Internet Explorer sub-trees), _lookup_admin_template + should still return False with the existing "multiple policies" error. + """ + policy_name = "Access data sources across domains" + policy_class = "Machine" + + fake_adml = _make_adml("SomePolicyId") + fake_policy_1 = _make_admx("PolicyA") + fake_policy_2 = _make_admx("PolicyB") + + parent_path_1 = ["Security Features", "Internet Explorer", "Windows Components"] + parent_path_2 = [ + "Internet Control Panel", + "Internet Explorer", + "Windows Components", + ] + + with ( + patch.object(win_lgpo, "_get_policy_definitions", return_value=MagicMock()), + patch.object(win_lgpo, "_get_policy_resources", return_value=MagicMock()), + patch.object(win_lgpo, "ADMX_SEARCH_XPATH", MagicMock(return_value=[])), + patch.object( + win_lgpo, + "ADML_SEARCH_XPATH", + MagicMock(return_value=[fake_adml]), + ), + patch.object( + win_lgpo, + "ADMX_DISPLAYNAME_SEARCH_XPATH", + MagicMock(return_value=[fake_policy_1, fake_policy_2]), + ), + patch.object( + win_lgpo, + "_build_parent_list", + side_effect=[list(parent_path_1), list(parent_path_2)], + ), + patch.object(win_lgpo, "_getFullPolicyName", return_value=policy_name), + ): + found, policy_xml, aliases, msg = win_lgpo._lookup_admin_template( + policy_name, policy_class + ) + + assert found is False + assert policy_xml is None + assert aliases == [] + assert "multiple policies" in msg + assert policy_name in msg From 6988ffae79e4e78a15afef026c1dd0317fbb661a Mon Sep 17 00:00:00 2001 From: Twangboy Date: Wed, 20 May 2026 17:16:07 -0600 Subject: [PATCH 15/19] Update bootstrap script to support Salt 3008 requirements files --- salt/cloud/deploy/bootstrap-salt.sh | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/salt/cloud/deploy/bootstrap-salt.sh b/salt/cloud/deploy/bootstrap-salt.sh index ad2115206060..e18f72a7456d 100644 --- a/salt/cloud/deploy/bootstrap-salt.sh +++ b/salt/cloud/deploy/bootstrap-salt.sh @@ -26,7 +26,7 @@ #====================================================================================================================== set -o nounset # Treat unset variables as an error -__ScriptVersion="2026.05.01" +__ScriptVersion="2026.05.20" __ScriptName="bootstrap-salt.sh" __ScriptFullName="$0" @@ -2785,8 +2785,18 @@ __install_salt_from_repo() { rm -f ${_TMP_DIR}/git/deps/* - echodebug "Installing Salt requirements from PyPi, ${_pip_cmd} install ${_USE_BREAK_SYSTEM_PACKAGES} --ignore-installed ${_PIP_INSTALL_ARGS} -r requirements/static/ci/py${_py_version}/linux.txt" - ${_pip_cmd} install ${_USE_BREAK_SYSTEM_PACKAGES} --ignore-installed ${_PIP_INSTALL_ARGS} -r "requirements/static/ci/py${_py_version}/linux.txt" + _salt_static_ci_linux_req="" + if [ -f "requirements/static/ci/py${_py_version}/linux.lock" ]; then + _salt_static_ci_linux_req="requirements/static/ci/py${_py_version}/linux.lock" + elif [ -f "requirements/static/ci/py${_py_version}/linux.txt" ]; then + _salt_static_ci_linux_req="requirements/static/ci/py${_py_version}/linux.txt" + else + echoerror "Salt static CI requirements not found: expected requirements/static/ci/py${_py_version}/linux.lock or requirements/static/ci/py${_py_version}/linux.txt" + return 1 + fi + + echodebug "Installing Salt requirements from PyPi, ${_pip_cmd} install ${_USE_BREAK_SYSTEM_PACKAGES} --ignore-installed ${_PIP_INSTALL_ARGS} -r ${_salt_static_ci_linux_req}" + ${_pip_cmd} install ${_USE_BREAK_SYSTEM_PACKAGES} --ignore-installed ${_PIP_INSTALL_ARGS} -r "${_salt_static_ci_linux_req}" # shellcheck disable=SC2181 if [ $? -ne 0 ]; then echo "Failed to install salt requirements for the version of Python ${_py_version}" From 1f53a567aaf40288b30bd1482fba43cf52037f83 Mon Sep 17 00:00:00 2001 From: co-cy Date: Wed, 6 May 2026 15:54:31 -0300 Subject: [PATCH 16/19] Distinguish corrupt vs transient errors in LoadAuth.get_tok When the eauth token backend raised any exception, the previous code either propagated it or treated it as "remove the token". For Redis or other network-backed token stores, a transient backend error (connection drop, NFS hang, full disk) would therefore log every authenticated user out -- ``rm_token`` deletes valid tokens that just briefly could not be read. Split the ``except`` into two cases: * ``SaltDeserializationError`` -- the on-disk blob is corrupt and cannot be parsed. The token is permanently unusable, so removing it is correct: leaving it around would make every subsequent ``get_tok`` for the same id keep failing. * ``OSError`` (covers ``IOError``) -- transient backend error. The token itself is fine; do NOT delete it. Return ``{}`` so the caller treats this request as not-authenticated, and the next request retries against the backend. Three behavioural tests guard each branch and the existing expired path. The IOError test fails on the previous implementation, which is the regression this change exists to prevent. Refs: #69073 --- changelog/69073.fixed.md | 1 + salt/auth/__init__.py | 45 ++++++++---- tests/pytests/unit/auth/test_auth.py | 101 +++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 15 deletions(-) create mode 100644 changelog/69073.fixed.md diff --git a/changelog/69073.fixed.md b/changelog/69073.fixed.md new file mode 100644 index 000000000000..d610309edd7b --- /dev/null +++ b/changelog/69073.fixed.md @@ -0,0 +1 @@ +``LoadAuth.get_tok`` now distinguishes between corrupt token blobs (removed from the store) and transient backend errors such as Redis connection drops or NFS hangs (token kept, request treated as not-authenticated). Previously a single backend hiccup could log every authenticated user out by deleting valid tokens. diff --git a/salt/auth/__init__.py b/salt/auth/__init__.py index 019c98229632..4e125646ed54 100644 --- a/salt/auth/__init__.py +++ b/salt/auth/__init__.py @@ -239,30 +239,45 @@ def get_tok(self, tok): Return the name associated with the token, or False if the token is not valid """ - tdata = {} try: tdata = self.tokens["{}.get_token".format(self.opts["eauth_tokens"])]( self.opts, tok ) except salt.exceptions.SaltDeserializationError: - log.warning("Failed to load token %r - removing broken/empty file.", tok) - rm_tok = True - else: - if not tdata: - return {} - rm_tok = False + # The on-disk / in-store token blob is corrupt and cannot + # be parsed. Removing it is the right call -- a corrupt + # token can never authenticate anyway, and leaving it + # around makes every subsequent ``get_tok`` for the same + # id keep failing. + log.warning( + "Token %r could not be deserialized; removing it from the " "store.", + tok, + ) + self.rm_token(tok) + return {} + except OSError: + # Transient backend error (Redis connection blip, NFS hang, + # hung disk). The token itself is fine; do NOT delete it -- + # that would log every authenticated user out on every + # backend hiccup. Return an empty dict so the caller treats + # this request as not-authenticated; the next request will + # retry against the backend and succeed once it recovers. + log.warning( + "Token store transient error reading %r; treating as " + "not-authenticated for this request without removing the " + "token from the store.", + tok, + ) + return {} + if not tdata: + return {} if tdata.get("expire", 0) < time.time(): - # If expire isn't present in the token it's invalid and needs - # to be removed. Also, if it's present and has expired - in - # other words, the expiration is before right now, it should - # be removed. - rm_tok = True - - if rm_tok: + # Expired token: drop it from the store. ``expire`` defaults + # to 0 if missing, so a malformed-but-deserializable token + # without an ``expire`` key falls into this branch too. self.rm_token(tok) return {} - return tdata def list_tokens(self): diff --git a/tests/pytests/unit/auth/test_auth.py b/tests/pytests/unit/auth/test_auth.py index 4fc3d836426f..683dd8fdd9b5 100644 --- a/tests/pytests/unit/auth/test_auth.py +++ b/tests/pytests/unit/auth/test_auth.py @@ -2,6 +2,8 @@ import salt.auth import salt.config +import salt.exceptions +from tests.support.mock import MagicMock def test_cve_2021_3244(tmp_path): @@ -31,3 +33,102 @@ def test_cve_2021_3244(tmp_path): t_data = auth.get_tok(t_data["token"]) assert not t_data assert not token_file.exists() + + +# --------------------------------------------------------------------------- +# ``LoadAuth.get_tok`` exception handling. +# +# A backend read failure must be classified by *cause*, not collapsed to +# "delete the token": +# +# * ``SaltDeserializationError`` — the stored blob cannot be parsed. The +# token is permanently unusable; remove it so subsequent reads do not +# keep failing on the same corrupt entry. +# * ``OSError`` / ``IOError`` — transient (Redis connection drop, NFS +# hang, full disk). The token itself is fine. Returning ``{}`` while +# leaving the token in place makes this request not-authenticated and +# lets the next request retry once the backend recovers. **Deleting on +# transient I/O errors logs every authenticated user out on every +# backend hiccup**, which is the regression these tests guard against. +# * Expired (deserialized successfully but past ``expire``) — remove. +# --------------------------------------------------------------------------- + + +def _make_auth(tmp_path, get_token_side_effect): + """Build a ``LoadAuth`` whose ``localfs.get_token`` is replaced by + ``get_token_side_effect``. ``rm_token`` is wrapped with a + ``MagicMock`` so the test can assert on whether the production code + chose to delete the token. The backing token directory is real so + ``rm_token`` would not blow up if it were called -- we are checking + *intent*, not state.""" + token_dir = tmp_path / "tokens" + token_dir.mkdir() + opts = { + "extension_modules": "", + "optimization_order": [0, 1, 2], + "token_expire": 60, + "keep_acl_in_token": False, + "eauth_tokens": "localfs", + "token_dir": str(token_dir), + "token_expire_user_override": False, + "external_auth": {"auto": {"foo": []}}, + } + auth = salt.auth.LoadAuth(opts) + + if callable(get_token_side_effect): + auth.tokens["localfs.get_token"] = get_token_side_effect + else: + auth.tokens["localfs.get_token"] = MagicMock(side_effect=get_token_side_effect) + + auth.tokens["localfs.rm_token"] = MagicMock() + return auth + + +def test_get_tok_returns_empty_and_keeps_token_on_io_error(tmp_path): + """Headline regression: a transient backend error (e.g. Redis + connection drop) must NOT cause the token to be deleted. The + previous implementation either propagated the exception or deleted + the token -- both wrong. Correct behaviour is to return ``{}`` and + leave the token alone so the next request can retry.""" + auth = _make_auth(tmp_path, OSError("redis connection reset")) + + result = auth.get_tok("any-token-id") + + assert result == {} + auth.tokens["localfs.rm_token"].assert_not_called() + + +def test_get_tok_removes_token_on_deserialization_error(tmp_path): + """A corrupt token blob is permanently unusable; removing it is + correct because every subsequent read would fail the same way.""" + auth = _make_auth( + tmp_path, + salt.exceptions.SaltDeserializationError("bad msgpack"), + ) + + result = auth.get_tok("corrupt-token-id") + + assert result == {} + auth.tokens["localfs.rm_token"].assert_called_once_with( + auth.opts, "corrupt-token-id" + ) + + +def test_get_tok_removes_expired_token(tmp_path): + """Expired tokens are deserializable but past their ``expire`` + timestamp. They must be removed so the store does not accumulate + dead entries.""" + expired_blob = { + "token": "expired-token-id", + "expire": time.time() - 60, + "name": "foo", + "eauth": "auto", + } + auth = _make_auth(tmp_path, lambda opts, tok: expired_blob) + + result = auth.get_tok("expired-token-id") + + assert result == {} + auth.tokens["localfs.rm_token"].assert_called_once_with( + auth.opts, "expired-token-id" + ) From 7c4d9a182426ee1a889237ef1f11e98948336110 Mon Sep 17 00:00:00 2001 From: co-cy Date: Tue, 12 May 2026 17:33:13 -0300 Subject: [PATCH 17/19] Fix lint W1404 implicit-str-concat in LoadAuth.get_tok warning CI saltpylint flagged `salt/auth/__init__.py:253` with `W1404(implicit-str-concat)`. The two adjacent string literals on that line are the result of black collapsing what was originally a two-line string onto a single line, which saltpylint reads as a missing comma. Merge them into a single string literal; the message remains under 88 columns so black leaves it alone. Refs: #69073 --- salt/auth/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/auth/__init__.py b/salt/auth/__init__.py index 4e125646ed54..8acccc8b5016 100644 --- a/salt/auth/__init__.py +++ b/salt/auth/__init__.py @@ -250,7 +250,7 @@ def get_tok(self, tok): # around makes every subsequent ``get_tok`` for the same # id keep failing. log.warning( - "Token %r could not be deserialized; removing it from the " "store.", + "Token %r could not be deserialized; removing it from the store.", tok, ) self.rm_token(tok) From f780a7bd598a3f79a422e03b1c5cfd0f7adbc6cd Mon Sep 17 00:00:00 2001 From: co-cy Date: Wed, 13 May 2026 12:50:54 -0300 Subject: [PATCH 18/19] Log exception class+message on token-read errors, traceback at DEBUG Address review feedback from @bdrx312 on PR #69074: the original `log.warning` calls in the SaltDeserializationError and OSError handlers in `LoadAuth.get_tok` did not capture the exception at all, making troubleshooting harder. Two combined changes: 1. Add the exception class+message inline via `%r` so each occurrence is one greppable line that names which subclass (e.g. `ConnectionResetError`) and its message produced the warning -- enough to triage without a full traceback. 2. Add a companion `log.debug(..., exc_info=True)` per @bdrx312's suggestion. Operators who need the full traceback for a specific intermittent failure can raise the level to DEBUG and see it. Costs nothing at the default WARNING level because logging skips exc_info formatting when the level is disabled. This avoids GBs/hour of stack frames during a flapping Redis or NFS outage (the original `exc_info=True` approach considered earlier) while keeping full diagnostic depth one log-level change away. Refs: #69073 --- salt/auth/__init__.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/salt/auth/__init__.py b/salt/auth/__init__.py index 8acccc8b5016..2b51fb94aeac 100644 --- a/salt/auth/__init__.py +++ b/salt/auth/__init__.py @@ -243,31 +243,43 @@ def get_tok(self, tok): tdata = self.tokens["{}.get_token".format(self.opts["eauth_tokens"])]( self.opts, tok ) - except salt.exceptions.SaltDeserializationError: + except salt.exceptions.SaltDeserializationError as exc: # The on-disk / in-store token blob is corrupt and cannot # be parsed. Removing it is the right call -- a corrupt # token can never authenticate anyway, and leaving it # around makes every subsequent ``get_tok`` for the same - # id keep failing. + # id keep failing. ``%r`` on the exception gives the + # operator the class and message inline (e.g. msgpack + # format error, truncated file) without spamming a full + # traceback into a hot-path WARNING; the full traceback is + # available via the companion ``log.debug`` for deeper + # investigation. log.warning( - "Token %r could not be deserialized; removing it from the store.", + "Token %r could not be deserialized (%r); removing it from the store.", tok, + exc, ) + log.debug("Token deserialization traceback:", exc_info=True) self.rm_token(tok) return {} - except OSError: + except OSError as exc: # Transient backend error (Redis connection blip, NFS hang, # hung disk). The token itself is fine; do NOT delete it -- # that would log every authenticated user out on every # backend hiccup. Return an empty dict so the caller treats # this request as not-authenticated; the next request will # retry against the backend and succeed once it recovers. + # Same logging pattern as above -- exception class + message + # at WARNING, full traceback at DEBUG so a flapping deploy + # stays diagnoseable without GB/hour of stack frames. log.warning( - "Token store transient error reading %r; treating as " + "Token store transient error reading %r (%r); treating as " "not-authenticated for this request without removing the " "token from the store.", tok, + exc, ) + log.debug("Token store transient-error traceback:", exc_info=True) return {} if not tdata: From f4b6fcae1dbc88e135a7438fabb2087ea3021125 Mon Sep 17 00:00:00 2001 From: anis Date: Thu, 21 May 2026 17:06:41 +0200 Subject: [PATCH 19/19] add changelog --- changelog/69136.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/69136.added.md diff --git a/changelog/69136.added.md b/changelog/69136.added.md new file mode 100644 index 000000000000..ab5176f23455 --- /dev/null +++ b/changelog/69136.added.md @@ -0,0 +1 @@ +fix etcdv3 module authentification when using etcd3-py lib