cmdmod enhancements for Windows #68156

xsmile · 2025-07-07T13:32:09Z

What does this PR do?

Change the behavior of the cmdmod execution module.

shell / python_shell

Respect user input for the shell parameter. Same as python_shell, it is disabled by default for all functions except cmd.shell. This prevents unnecessary calls to the shell and possibly unsafe command executions. cmd.run and other similar functions now require shell to be specified explicitly if shell functionality is required.

cmd._run

the command list / string is kept as is without altering the command behaviour. Special characters and whitespaces are handled by subprocess.list2cmdline only if required in case the shell is cmd.exe or if impersonating another user and passing the command string to CreateProcess.
run PowerShell scripts via -File instead of -Command. This makes it possible to pass arguments as a list and not having to worry about escaping characters. In contrast to the previous behavior with -Command, the arguments are not evaluated as commands. When PowerShell commands are a requirement, cmd.shell or cmd.powershell should be used instead, e.g. Windows: Using inline powershell in args with cmd.script and shell: powershell #56195 .

cmd.script

always remove the temporary script, even if it fails to be executed
automatically suggest a shell when a Windows script with a known file extension is found
allow to pass arguments as a list

cmd.powershell

do not silently fail and report success when a PowerShell command fails to run

What issues does this PR fix or reference?

#56195
#68096
#68118

Merge requirements satisfied?

Docs
Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
Tests written/updated

Commits signed with GPG?

No

welcome · 2025-07-07T13:32:12Z

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject.pdl@broadcom.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

twangboy

I have a few questions about some of the tests. Also, this needs a changelog. Additionally, please make these changes against the 3006.x branch as these issue also exist there.

tests/pytests/functional/modules/cmd/test_run_win.py

tests/pytests/unit/modules/test_cmdmod.py

twangboy

A few questions and changes. Thanks for adding a changelog.

salt/modules/cmdmod.py

salt/platform/win.py

tests/pytests/unit/modules/test_cmdmod.py

salt/utils/win_runas.py

twangboy · 2025-07-09T19:55:12Z

Got some pre-commit failures

--- a/tests/integration/modules/test_cmdmod.py
+++ b/tests/integration/modules/test_cmdmod.py
@@ -590,9 +590,7 @@ class CMDModuleTest(ModuleCase):
         self.assertEqual(out, "")
 
         # cmd.run_stderr
-        out = self.run_function(
-            "cmd.shell", ls_command, shell=shell, hide_output=True
-        )
+        out = self.run_function("cmd.shell", ls_command, shell=shell, hide_output=True)
         self.assertEqual(out, "")

xsmile · 2025-07-11T20:12:00Z

Would it do any harm to disable python_shell in the state module?

salt/salt/states/cmd.py

Line 862 in cb70ab0

cmd=name, timeout=timeout, python_shell=True, **cmd_kwargs

salt/salt/states/cmd.py

Line 1166 in cb70ab0

cmd_all = __salt__["cmd.script"](source, python_shell=True, **cmd_kwargs)

xsmile · 2025-07-17T19:11:53Z

Fixed tests and added two changes:

set shell to DEFAULT_SHELL if required, as sh instead of bash was used while running tests
revert changes to other Windows modules included with Prepend cmd when using built-in commands on Windows #67919, as quoting is now handled automatically

Thanks for the review.

xsmile requested a review from a team as a code owner July 7, 2025 13:32

twangboy requested changes Jul 7, 2025

View reviewed changes

tests/pytests/functional/modules/cmd/test_run_win.py Outdated Show resolved Hide resolved

tests/pytests/functional/modules/cmd/test_run_win.py Show resolved Hide resolved

tests/pytests/unit/modules/test_cmdmod.py Show resolved Hide resolved

twangboy added the test:full Run the full test suite label Jul 7, 2025

twangboy added this to the Sulfur v3006.14 milestone Jul 7, 2025

twangboy temporarily deployed to ci July 7, 2025 18:45 — with GitHub Actions Inactive

xsmile temporarily deployed to ci July 7, 2025 18:47 — with GitHub Actions Inactive

xsmile temporarily deployed to ci July 7, 2025 19:14 — with GitHub Actions Inactive

twangboy added needs-changelog needs-rebase Needs to be rebased, against either the current branch or a different one labels Jul 9, 2025

xsmile changed the base branch from 3007.x to 3006.x July 9, 2025 18:29

xsmile force-pushed the cmdmod branch from 9b97b48 to 1727d92 Compare July 9, 2025 18:47

twangboy requested changes Jul 9, 2025

View reviewed changes

salt/modules/cmdmod.py Show resolved Hide resolved

salt/platform/win.py Outdated Show resolved Hide resolved

tests/pytests/unit/modules/test_cmdmod.py Outdated Show resolved Hide resolved

salt/utils/win_runas.py Show resolved Hide resolved

xsmile temporarily deployed to ci July 9, 2025 19:51 — with GitHub Actions Inactive

xsmile temporarily deployed to ci July 9, 2025 20:10 — with GitHub Actions Inactive

twangboy removed needs-changelog needs-rebase Needs to be rebased, against either the current branch or a different one labels Jul 9, 2025

This was linked to issues Jul 9, 2025

[BUG] cmd.run quotation handling with Windows and Powershell #68118

Open

[BUG] cmd.run module behaves incorrectly on Windows #68096

Open

xsmile temporarily deployed to ci July 10, 2025 21:10 — with GitHub Actions Inactive

twangboy modified the milestones: Sulfur v3006.14, Sulfur v3006.15 Jul 10, 2025

xsmile force-pushed the cmdmod branch from 5b4333a to 83c3a7c Compare July 11, 2025 20:03

twangboy previously approved these changes Jul 15, 2025

View reviewed changes

xsmile had a problem deploying to ci July 15, 2025 21:21 — with GitHub Actions Failure

xsmile temporarily deployed to ci July 15, 2025 21:26 — with GitHub Actions Inactive

xsmile temporarily deployed to ci July 15, 2025 21:46 — with GitHub Actions Inactive

xsmile force-pushed the cmdmod branch from 83c3a7c to f8edf8d Compare July 17, 2025 18:56

xsmile added 9 commits July 17, 2025 21:03

cmdmod: unset shell/python_shell variables by default

9bb14ab

cmdmod: change argument processing/quoting

a86c39c

cmdmod: always remove the temporary script

dc178bf

cmdmod: set a default shell when running known script types

238dfa3

cmdmod: do not report success when a powershell command fails

c68a99b

cmdmod: update tests

e5f3eb6

cmdmod: formatting and tests

8a95808

cmdmod: changelog

04ca329

cmdmod: fix other modules

ca046d3

xsmile dismissed twangboy’s stale review via ca046d3 July 17, 2025 19:04

xsmile force-pushed the cmdmod branch from f8edf8d to ca046d3 Compare July 17, 2025 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cmdmod enhancements for Windows #68156

cmdmod enhancements for Windows #68156

Uh oh!

xsmile commented Jul 7, 2025 •

edited

Loading

Uh oh!

welcome bot commented Jul 7, 2025

Uh oh!

twangboy left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twangboy left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twangboy commented Jul 9, 2025 •

edited

Loading

Uh oh!

xsmile commented Jul 11, 2025

Uh oh!

xsmile commented Jul 17, 2025

Uh oh!

Uh oh!

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

cmdmod enhancements for Windows #68156

Are you sure you want to change the base?

cmdmod enhancements for Windows #68156

Uh oh!

Conversation

xsmile commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

What issues does this PR fix or reference?

Merge requirements satisfied?

Commits signed with GPG?

Uh oh!

welcome bot commented Jul 7, 2025

Uh oh!

twangboy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twangboy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twangboy commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xsmile commented Jul 11, 2025

Uh oh!

xsmile commented Jul 17, 2025

Uh oh!

Uh oh!

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

xsmile commented Jul 7, 2025 •

edited

Loading

twangboy commented Jul 9, 2025 •

edited

Loading