From 13abf9c82d90c405a8a2549a0c2e96a5c261b535 Mon Sep 17 00:00:00 2001 From: twangboy Date: Fri, 21 Nov 2025 14:52:45 -0700 Subject: [PATCH 1/4] Try to handle script with spaces --- salt/modules/cmdmod.py | 1 - salt/platform/win.py | 13 +- salt/utils/win_runas.py | 17 ++- .../functional/modules/cmd/test_script.py | 133 ++++++++++++++++-- 4 files changed, 143 insertions(+), 21 deletions(-) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 2c9da4ca62fd..d33a44616f33 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -729,7 +729,6 @@ def _run( if ( python_shell is not True and shell is not None - # and not salt.utils.platform.is_windows() and not isinstance(cmd, list) ): cmd = salt.utils.args.shlex_split(cmd) diff --git a/salt/platform/win.py b/salt/platform/win.py index c28621d2feca..ae302ce90935 100644 --- a/salt/platform/win.py +++ b/salt/platform/win.py @@ -1116,7 +1116,7 @@ def grant_winsta_and_desktop(th): """ current_sid = win32security.GetTokenInformation(th, win32security.TokenUser)[0] # Add permissions for the sid to the current windows station and thread id. - # This prevents windows error 0xC0000142. + # This prevents Windows error 0xC0000142. winsta = win32process.GetProcessWindowStation() set_user_perm(winsta, WINSTA_ALL, current_sid) desktop = win32service.GetThreadDesktop(win32api.GetCurrentThreadId()) @@ -1162,8 +1162,8 @@ def CreateProcessWithTokenW( ctypes.byref(startupinfo), ctypes.byref(process_info), ) + winerr = win32api.GetLastError() if ret == 0: - winerr = win32api.GetLastError() exc = OSError(win32api.FormatMessage(winerr)) exc.winerror = winerr raise exc @@ -1341,11 +1341,12 @@ def CreateProcessWithLogonW( def prepend_cmd(win_shell, cmd): """ - Prep cmd when shell is cmd.exe. Always use a command string instead of a list to satisfy - both CreateProcess and CreateProcessWithToken. + Prep cmd when shell is cmd.exe. Always use a command string instead of a + list to satisfy both CreateProcess and CreateProcessWithToken. - cmd must be double-quoted to ensure proper handling of space characters. The first opening - quote and the closing quote are stripped automatically by the Win32 API. + cmd must be double-quoted to ensure proper handling of space characters. + The first opening quote and the closing quote are stripped automatically by + the Win32 API. """ if isinstance(cmd, (list, tuple)): args = subprocess.list2cmdline(cmd) diff --git a/salt/utils/win_runas.py b/salt/utils/win_runas.py index 00cd7d14d197..a8600c5e65c6 100644 --- a/salt/utils/win_runas.py +++ b/salt/utils/win_runas.py @@ -68,7 +68,7 @@ def split_username(username): return str(user_name), str(domain) -def create_env(user_token, inherit, timeout=1): +def create_env(username, user_token, inherit=False, timeout=1): """ CreateEnvironmentBlock might fail when we close a login session and then try to re-open one very quickly. Run the method multiple times to work @@ -79,11 +79,16 @@ def create_env(user_token, inherit, timeout=1): exc = None while True: try: - env = win32profile.CreateEnvironmentBlock(user_token, False) + profile_info_dict = {"UserName": username} + profile_handle = win32profile.LoadUserProfile(user_token, profile_info_dict) + env = win32profile.CreateEnvironmentBlock(user_token, inherit) except pywintypes.error as exc: pass else: break + finally: + win32profile.UnloadUserProfile(user_token, profile_handle) + if time.time() - start > timeout: break if env is not None: @@ -95,8 +100,8 @@ def create_env(user_token, inherit, timeout=1): def runas(cmd, username, password=None, cwd=None): """ Run a command as another user. If the process is running as an admin or - system account this method does not require a password. Other non - privileged accounts need to provide a password for the user to runas. + system account, this method does not require a password. Other + non-privileged accounts need to provide a password for the user to "runas". Commands are run in with the highest level privileges possible for the account provided. """ @@ -211,7 +216,7 @@ def runas(cmd, username, password=None, cwd=None): ) # Create the environment for the user - env = create_env(user_token, False) + env = create_env(username, user_token, inherit=False) hProcess = None try: @@ -232,7 +237,7 @@ def runas(cmd, username, password=None, cwd=None): dwProcessId = process_info.dwProcessId dwThreadId = process_info.dwThreadId - # We don't use these so let's close the handle + # We don't use these, so let's close the handle salt.platform.win.kernel32.CloseHandle(stdin_write.handle) salt.platform.win.kernel32.CloseHandle(stdout_write.handle) salt.platform.win.kernel32.CloseHandle(stderr_write.handle) diff --git a/tests/pytests/functional/modules/cmd/test_script.py b/tests/pytests/functional/modules/cmd/test_script.py index 952e9c213904..db79a9bf75f6 100644 --- a/tests/pytests/functional/modules/cmd/test_script.py +++ b/tests/pytests/functional/modules/cmd/test_script.py @@ -18,7 +18,7 @@ def account(): @pytest.fixture -def echo_script(state_tree): +def echo_script_contents(): if salt.utils.platform.is_windows(): file_name = "echo_script.bat" contents = dedent( @@ -39,12 +39,25 @@ def echo_script(state_tree): echo "a: $a, b: $b" """ ) + return file_name, contents + + +@pytest.fixture +def echo_script(state_tree, echo_script_contents): + file_name, contents = echo_script_contents with pytest.helpers.temp_file(file_name, contents, state_tree / "echo-script"): yield file_name @pytest.fixture -def pipe_script(state_tree): +def echo_script_with_space(state_tree, echo_script_contents): + file_name, contents = echo_script_contents + with pytest.helpers.temp_file(file_name, contents, state_tree / "echo script"): + yield file_name + + +@pytest.fixture +def pipe_script_contents(): if salt.utils.platform.is_windows(): file_name = "pipe_script.bat" contents = dedent( @@ -69,6 +82,12 @@ def pipe_script(state_tree): fi """ ) + return file_name, contents + + +@pytest.fixture +def pipe_script(pipe_script_contents, state_tree): + file_name, contents = pipe_script_contents with pytest.helpers.temp_file(file_name, contents, state_tree / "echo-script") as f: if not salt.utils.platform.is_windows(): current_perms = f.stat().st_mode @@ -78,6 +97,22 @@ def pipe_script(state_tree): yield f +@pytest.fixture +def pipe_script_with_space(pipe_script_contents, state_tree): + file_name, contents = pipe_script_contents + with pytest.helpers.temp_directory() as temp_path: + temp_file_path = temp_path / "dir with spaces" / file_name + temp_file_path.parent.mkdir(parents=True) + assert temp_file_path.parent.exists() + temp_file_path.write_text(contents) + if not salt.utils.platform.is_windows(): + current_perms = temp_file_path.stat().st_mode + new_perms = current_perms | stat.S_IXUSR + temp_file_path.chmod(new_perms) + temp_file_path.chmod(0o755) + yield temp_file_path + + @pytest.mark.parametrize( "args, expected", [ @@ -87,7 +122,7 @@ def pipe_script(state_tree): (["foo foo", "bar bar"], "a: foo foo, b: bar bar"), ], ) -def test_echo(modules, echo_script, args, expected): +def test_script_args(modules, echo_script, args, expected): """ Test argument processing with a batch script """ @@ -105,7 +140,25 @@ def test_echo(modules, echo_script, args, expected): (["foo foo", "bar bar"], "a: foo foo, b: bar bar"), ], ) -def test_echo_runas(modules, account, echo_script, args, expected): +def test_script_args_with_space(modules, echo_script_with_space, args, expected): + """ + Test argument processing with a batch script + """ + script = f"salt://echo script/{echo_script_with_space}" + result = modules.cmd.script(script, args=args) + assert result["stdout"] == expected + + +@pytest.mark.parametrize( + "args, expected", + [ + ("foo bar", "a: foo, b: bar"), + ('foo "bar bar"', "a: foo, b: bar bar"), + (["foo", "bar"], "a: foo, b: bar"), + (["foo foo", "bar bar"], "a: foo foo, b: bar bar"), + ], +) +def test_script_args_runas(modules, account, echo_script, args, expected): """ Test argument processing with a batch/bash script and runas """ @@ -119,7 +172,30 @@ def test_echo_runas(modules, account, echo_script, args, expected): assert result["stdout"] == expected -def test_pipe_run_python_shell_true(modules, pipe_script): +@pytest.mark.parametrize( + "args, expected", + [ + ("foo bar", "a: foo, b: bar"), + ('foo "bar bar"', "a: foo, b: bar bar"), + (["foo", "bar"], "a: foo, b: bar"), + (["foo foo", "bar bar"], "a: foo foo, b: bar bar"), + ], +) +def test_script_args_runas_with_space(modules, account, echo_script_with_space, args, expected): + """ + Test argument processing with a batch/bash script and runas + """ + script = f"salt://echo script/{echo_script_with_space}" + result = modules.cmd.script( + script, + args=args, + runas=account.username, + password=account.password, + ) + assert result["stdout"] == expected + + +def test_run_pipe_python_shell_true(modules, pipe_script): if salt.utils.platform.is_windows(): cmd = f'{str(pipe_script)} | find /c /v ""' else: @@ -128,7 +204,7 @@ def test_pipe_run_python_shell_true(modules, pipe_script): assert result == "1" -def test_pipe_run_python_shell_false(modules, pipe_script): +def test_run_pipe_python_shell_false(modules, pipe_script): if salt.utils.platform.is_windows(): cmd = f'{str(pipe_script)} | find /c /v ""' # Behavior is different on Windows, I think it has to do with how cmd @@ -141,7 +217,7 @@ def test_pipe_run_python_shell_false(modules, pipe_script): assert result == expected -def test_pipe_run_default(modules, pipe_script): +def test_run_pipe_default(modules, pipe_script): if salt.utils.platform.is_windows(): cmd = f'{str(pipe_script)} | find /c /v ""' else: @@ -153,7 +229,7 @@ def test_pipe_run_default(modules, pipe_script): assert result == "1" -def test_pipe_run_shell(modules, pipe_script): +def test_run_pipe_shell(modules, pipe_script): if salt.utils.platform.is_windows(): cmd = f'{str(pipe_script)} | find /c /v ""' shell = "cmd" @@ -165,3 +241,44 @@ def test_pipe_run_shell(modules, pipe_script): # test suite, the value is empty result = modules.cmd.run(cmd, shell=shell, __pub_jid="test") assert result == "1" + + +def test_run_spaces(modules, pipe_script_with_space): + cmd = f"{str(pipe_script_with_space)}" + result = modules.cmd.run(cmd) + assert result == "fine" + + +def test_run_spaces_runas(modules, pipe_script_with_space, account): + cmd = f"{str(pipe_script_with_space)}" + result = modules.cmd.run( + cmd, + runas=account.username, + password=account.password, + ) + assert result == "fine" + + +def test_script_pipe_spaces(modules, pipe_script_with_space): + cmd = f"{str(pipe_script_with_space)}" + if salt.utils.platform.is_windows(): + args = '| find /c /v ""' + else: + args = "| wc -l" + result = modules.cmd.script(cmd, args=args) + assert result["stdout"] == "1" + + +def test_script_pipe_spaces_runas(modules, pipe_script_with_space, account): + cmd = f"{str(pipe_script_with_space)}" + if salt.utils.platform.is_windows(): + args = '| find /c /v ""' + else: + args = "| wc -l" + result = modules.cmd.script( + cmd, + args=args, + runas=account.username, + password=account.password, + ) + assert result["stdout"] == "1" From 28d73e886b27876565018646d852ec2a41c66b77 Mon Sep 17 00:00:00 2001 From: twangboy Date: Thu, 11 Dec 2025 14:22:01 -0700 Subject: [PATCH 2/4] Try to fix failing test --- salt/modules/cmdmod.py | 6 +- salt/platform/win.py | 2 +- salt/utils/win_runas.py | 53 +++++++++++----- .../functional/modules/cmd/test_script.py | 61 +++++++++++-------- 4 files changed, 73 insertions(+), 49 deletions(-) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index d33a44616f33..35e308ac76fa 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -726,11 +726,7 @@ def _run( f"Specified cwd '{cwd}' either not absolute or does not exist" ) - if ( - python_shell is not True - and shell is not None - and not isinstance(cmd, list) - ): + if python_shell is not True and shell is not None and not isinstance(cmd, list): cmd = salt.utils.args.shlex_split(cmd) if success_retcodes is None: diff --git a/salt/platform/win.py b/salt/platform/win.py index ae302ce90935..a1c97691731b 100644 --- a/salt/platform/win.py +++ b/salt/platform/win.py @@ -1162,8 +1162,8 @@ def CreateProcessWithTokenW( ctypes.byref(startupinfo), ctypes.byref(process_info), ) - winerr = win32api.GetLastError() if ret == 0: + winerr = win32api.GetLastError() exc = OSError(win32api.FormatMessage(winerr)) exc.winerror = winerr raise exc diff --git a/salt/utils/win_runas.py b/salt/utils/win_runas.py index a8600c5e65c6..d2a2ece2c67a 100644 --- a/salt/utils/win_runas.py +++ b/salt/utils/win_runas.py @@ -9,6 +9,8 @@ import subprocess import time +import salt.platform.win +import salt.utils.path from salt.exceptions import CommandExecutionError try: @@ -77,20 +79,30 @@ def create_env(username, user_token, inherit=False, timeout=1): start = time.time() env = None exc = None - while True: - try: - profile_info_dict = {"UserName": username} - profile_handle = win32profile.LoadUserProfile(user_token, profile_info_dict) - env = win32profile.CreateEnvironmentBlock(user_token, inherit) - except pywintypes.error as exc: - pass - else: - break - finally: - win32profile.UnloadUserProfile(user_token, profile_handle) - - if time.time() - start > timeout: - break + profile_info_dict = {"UserName": username} + try: + profile_handle = win32profile.LoadUserProfile(user_token, profile_info_dict) + while True: + try: + env = win32profile.CreateEnvironmentBlock(user_token, inherit) + if env is not None: + break + except pywintypes.error as exc: + pass + else: + break + if time.time() - start > timeout: + break + except (win32api.error, pywintypes.error) as e: + msg = f"Failed to load user profile: {e}" + raise CommandExecutionError(msg) + + try: + win32profile.UnloadUserProfile(user_token, profile_handle) + except (win32api.error, pywintypes.error) as e: + msg = f"Failed to unload user profile: {e}" + raise CommandExecutionError(msg) + if env is not None: return env if exc is not None: @@ -120,10 +132,12 @@ def runas(cmd, username, password=None, cwd=None): # Elevate the token from the current process access = win32security.TOKEN_QUERY | win32security.TOKEN_ADJUST_PRIVILEGES th = win32security.OpenProcessToken(win32api.GetCurrentProcess(), access) + import salt.platform.win + salt.platform.win.elevate_token(th) # Try to impersonate the SYSTEM user. This process needs to be running as a - # user who as been granted the SeImpersonatePrivilege, Administrator + # user who has been granted the SeImpersonatePrivilege, Administrator # accounts have this permission by default. try: impersonation_token = salt.platform.win.impersonate_sid( @@ -217,6 +231,13 @@ def runas(cmd, username, password=None, cwd=None): # Create the environment for the user env = create_env(username, user_token, inherit=False) + application_name = None + # TODO: Maybe it has something to do with applicationname + # application_name = salt.utils.path.which("cmd.exe") + # application_name = cmd + # import salt.utils.args + # if salt.utils.args.shlex_split(cmd)[0].endswith((".bat", "cmd", "cmd.exe")): + # application_name = salt.utils.path.which("cmd.exe") hProcess = None try: @@ -224,7 +245,7 @@ def runas(cmd, username, password=None, cwd=None): process_info = salt.platform.win.CreateProcessWithTokenW( int(user_token), logonflags=1, - applicationname=None, + applicationname=application_name, commandline=cmd, currentdirectory=cwd, creationflags=creationflags, diff --git a/tests/pytests/functional/modules/cmd/test_script.py b/tests/pytests/functional/modules/cmd/test_script.py index db79a9bf75f6..d8e7f7714eaf 100644 --- a/tests/pytests/functional/modules/cmd/test_script.py +++ b/tests/pytests/functional/modules/cmd/test_script.py @@ -20,7 +20,6 @@ def account(): @pytest.fixture def echo_script_contents(): if salt.utils.platform.is_windows(): - file_name = "echo_script.bat" contents = dedent( """\ @echo off @@ -30,7 +29,6 @@ def echo_script_contents(): """ ) else: - file_name = "echo_script.sh" contents = dedent( """\ #!/bin/bash @@ -39,27 +37,32 @@ def echo_script_contents(): echo "a: $a, b: $b" """ ) - return file_name, contents + return contents @pytest.fixture def echo_script(state_tree, echo_script_contents): - file_name, contents = echo_script_contents - with pytest.helpers.temp_file(file_name, contents, state_tree / "echo-script"): + if salt.utils.platform.is_windows(): + file_name = "echo_script.bat" + else: + file_name = "echo_script.sh" + with pytest.helpers.temp_file(file_name, echo_script_contents, state_tree): yield file_name @pytest.fixture def echo_script_with_space(state_tree, echo_script_contents): - file_name, contents = echo_script_contents - with pytest.helpers.temp_file(file_name, contents, state_tree / "echo script"): + if salt.utils.platform.is_windows(): + file_name = "echo script space.bat" + else: + file_name = "echo script space.sh" + with pytest.helpers.temp_file(file_name, echo_script_contents, state_tree): yield file_name @pytest.fixture def pipe_script_contents(): if salt.utils.platform.is_windows(): - file_name = "pipe_script.bat" contents = dedent( """\ @echo off @@ -71,7 +74,6 @@ def pipe_script_contents(): """ ) else: - file_name = "pipe_script.sh" contents = dedent( """\ #!/bin/bash @@ -82,13 +84,16 @@ def pipe_script_contents(): fi """ ) - return file_name, contents + return contents @pytest.fixture def pipe_script(pipe_script_contents, state_tree): - file_name, contents = pipe_script_contents - with pytest.helpers.temp_file(file_name, contents, state_tree / "echo-script") as f: + if salt.utils.platform.is_windows(): + file_name = "pipe_script.bat" + else: + file_name = "pipe_script.sh" + with pytest.helpers.temp_file(file_name, pipe_script_contents, state_tree) as f: if not salt.utils.platform.is_windows(): current_perms = f.stat().st_mode new_perms = current_perms | stat.S_IXUSR @@ -99,18 +104,17 @@ def pipe_script(pipe_script_contents, state_tree): @pytest.fixture def pipe_script_with_space(pipe_script_contents, state_tree): - file_name, contents = pipe_script_contents - with pytest.helpers.temp_directory() as temp_path: - temp_file_path = temp_path / "dir with spaces" / file_name - temp_file_path.parent.mkdir(parents=True) - assert temp_file_path.parent.exists() - temp_file_path.write_text(contents) + if salt.utils.platform.is_windows(): + file_name = "pipe script space.bat" + else: + file_name = "pipe script space.sh" + with pytest.helpers.temp_file(file_name, pipe_script_contents, state_tree) as f: if not salt.utils.platform.is_windows(): - current_perms = temp_file_path.stat().st_mode + current_perms = f.stat().st_mode new_perms = current_perms | stat.S_IXUSR - temp_file_path.chmod(new_perms) - temp_file_path.chmod(0o755) - yield temp_file_path + f.chmod(new_perms) + f.chmod(0o755) + yield f @pytest.mark.parametrize( @@ -126,7 +130,7 @@ def test_script_args(modules, echo_script, args, expected): """ Test argument processing with a batch script """ - script = f"salt://echo-script/{echo_script}" + script = f"salt://{echo_script}" result = modules.cmd.script(script, args=args) assert result["stdout"] == expected @@ -144,7 +148,7 @@ def test_script_args_with_space(modules, echo_script_with_space, args, expected) """ Test argument processing with a batch script """ - script = f"salt://echo script/{echo_script_with_space}" + script = f"salt://{echo_script_with_space}" result = modules.cmd.script(script, args=args) assert result["stdout"] == expected @@ -162,7 +166,7 @@ def test_script_args_runas(modules, account, echo_script, args, expected): """ Test argument processing with a batch/bash script and runas """ - script = f"salt://echo-script/{echo_script}" + script = f"salt://{echo_script}" result = modules.cmd.script( script, args=args, @@ -181,11 +185,13 @@ def test_script_args_runas(modules, account, echo_script, args, expected): (["foo foo", "bar bar"], "a: foo foo, b: bar bar"), ], ) -def test_script_args_runas_with_space(modules, account, echo_script_with_space, args, expected): +def test_script_args_runas_with_space( + modules, account, echo_script_with_space, args, expected +): """ Test argument processing with a batch/bash script and runas """ - script = f"salt://echo script/{echo_script_with_space}" + script = f"salt://{echo_script_with_space}" result = modules.cmd.script( script, args=args, @@ -249,6 +255,7 @@ def test_run_spaces(modules, pipe_script_with_space): assert result == "fine" +###### FAILING def test_run_spaces_runas(modules, pipe_script_with_space, account): cmd = f"{str(pipe_script_with_space)}" result = modules.cmd.run( From a475ce9f97c7d1d6f2850939935bcc81455a0a0c Mon Sep 17 00:00:00 2001 From: twangboy Date: Thu, 11 Dec 2025 14:49:52 -0700 Subject: [PATCH 3/4] Gate pywin32 imports in platform.win --- salt/platform/win.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/salt/platform/win.py b/salt/platform/win.py index a1c97691731b..8345e6ffe185 100644 --- a/salt/platform/win.py +++ b/salt/platform/win.py @@ -17,13 +17,18 @@ from ctypes import wintypes # pylint: disable=3rd-party-module-not-gated -import ntsecuritycon -import psutil -import win32api -import win32con -import win32process -import win32security -import win32service +try: + import ntsecuritycon + import psutil + import win32api + import win32con + import win32process + import win32security + import win32service + + HAS_PYWIN32 = True +except ImportError: + HAS_PYWIN32 = False # pylint: enable=3rd-party-module-not-gated From fa17de136865c1b936ec118602a3edbbd3c8f3c4 Mon Sep 17 00:00:00 2001 From: twangboy Date: Thu, 11 Dec 2025 14:58:02 -0700 Subject: [PATCH 4/4] Fix gating issue with platform.win --- salt/platform/win.py | 19 +++++++------------ salt/utils/win_runas.py | 1 - 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/salt/platform/win.py b/salt/platform/win.py index 8345e6ffe185..a1c97691731b 100644 --- a/salt/platform/win.py +++ b/salt/platform/win.py @@ -17,18 +17,13 @@ from ctypes import wintypes # pylint: disable=3rd-party-module-not-gated -try: - import ntsecuritycon - import psutil - import win32api - import win32con - import win32process - import win32security - import win32service - - HAS_PYWIN32 = True -except ImportError: - HAS_PYWIN32 = False +import ntsecuritycon +import psutil +import win32api +import win32con +import win32process +import win32security +import win32service # pylint: enable=3rd-party-module-not-gated diff --git a/salt/utils/win_runas.py b/salt/utils/win_runas.py index d2a2ece2c67a..dcf0ac2e81aa 100644 --- a/salt/utils/win_runas.py +++ b/salt/utils/win_runas.py @@ -9,7 +9,6 @@ import subprocess import time -import salt.platform.win import salt.utils.path from salt.exceptions import CommandExecutionError