Fix issue with registry entries not getting quoted properly #404

lrandersson · 2025-10-30T14:30:06Z

Description

To resolve Filetype association in Windows registry requires double-quotes to properly handle files with spaces #290

Checklist - did you ...

Add a file to the news directory (using the template) for the next release's release notes?
Add / update necessary tests?
Add / update outdated documentation?

lrandersson · 2025-10-30T14:31:35Z

tests/test_utils.py

+    assert output == [
+        '"cmd.exe"',
+        '/C',
+        '""echo" "Hello World""'


Isn't this generated incorrectly? When I tried the generated command:

C:\Windows\System32>"cmd.exe" /C ""echo" "Hello World"" '"echo"' is not recognized as an internal or external command, operable program or batch file.

What should be generated is the following:

C:\Windows\System32>"cmd.exe" /C "echo "Hello World"" "Hello World"

I have not fixed it in this PR but something I wanted to bring up, for now the test only verifies the incorrect behavior.

I would agree with you, echo should not be surrounded by quotes.

I've fixed the issue in commit a36c3c1. I wonder however if can remove the function ensure_pad or change it. It is only used inside the method WinLex.quote_args, and only with the input argument pad = '"' (which is different from the default _). I see also that a copy of the same function exists inside the legacy code, but I would suggest:

Rename to ensure_quoted or add_quotes_if_needed.

Remove the pad keyword argument since we only use it with ".

Leave the legacy function as is.
However, not sure if this is a class imported by other menuinst users, if so, we could deprecate it.

A lot of users still use v1 shortcuts, so the legacy part needs to stay for now. I would also say to leave it as is until we have a deprecation plan.

I'm not sure if ensure_pad is the right place to make that change. Creating a special case there is a little opaque. Wouldn't quote_args be the better place, i.e., only call ensure_pad if there is a space in the string?

I agree with you. I've made some fixes now.

tests/test_utils.py

lrandersson · 2025-11-14T15:01:40Z

menuinst/utils.py

+
+        # Don't touch shell meta tokens; quoting would change meaning.
+        # Example: echo %FOO%> output.txt
+        if cls._has_shell_meta(s):


This was added because tests broke from my initial changes (in particular it was the or '%' in s in quote_string that caused it:
https://github.com/conda/menuinst/actions/runs/19366298809

I'm trying to understand the impact of this. That would mean that echo %FOO%> output.txt would be quoted differently than echo %FOO% > output.txt

There are a lot of more changes now but it works now based on the testing. I've done some additional testing locally with other workflows where menuinst is used.
There are a lot more changes now compared to before, but it's ready for another round of review.

marcoesters

This looks quite extensive. Just minor code suggestions and some remaining questions.

marcoesters · 2025-12-18T20:25:00Z

menuinst/platforms/win.py

            script = self._write_script()
            if self.metadata["terminal"]:
-                command = ["cmd", "/D", "/K", f'"{script}"']
+                command = ["cmd", "/D", "/K", str(script)]


Will removing the quotes be a problem for paths with spaces? I don't see a test covering that example.

The quotes are actually not removed, it's just that we let WinLex.quote_args do the work instead of manually quoting it here, to have more of a single source of quoting. Note that we return return WinLex.quote_args(command) instead of return command in this PR.

marcoesters · 2025-12-18T20:33:12Z

menuinst/utils.py

+        Return True if input refers to cmd.exe or %COMSPEC%. Here, 'refers' implies variants of
+        cmd, cmd.exe, <some path>/cmd and accounting for quoted input.


Maybe call it _is_cmd_exe then? _is_cmd could also be interpreted as "is (a) command"

Good point, fixed!

marcoesters · 2025-12-18T20:38:57Z

menuinst/utils.py

            len(args) > 2
-            and ("CMD.EXE" in args[0].upper() or "%COMSPEC%" in args[0].upper())
+            and cls._is_cmd(args[0])
            and (args[1].upper() == "/K" or args[1].upper() == "/C")


You are using quote_args now in a function that prepends the script with cmd.exe /D /K, so args[1] will be /D even though /K is added. Will this cause any issues? Does the check need to be more robust to parse all flags?

Good catch, I made a more generic fix here 1acbcfc, I also added another test and parametrized it to verify it works for both /C and /K.

marcoesters · 2025-12-18T20:40:54Z

menuinst/utils.py

+            return s
+
+        # Don't add quotes for minus or leading space
+        if s and s[0] in ("-", " "):


Suggested change

if s and s[0] in ("-", " "):

if s[0] in ("-", " "):

You already return if s is an empty string.

Good catch 1acbcfc

marcoesters · 2025-12-18T20:43:58Z

menuinst/utils.py

+            return f'"{s}"'
+
+        if " " in s:
            return '"%s"' % s


Suggested change

return '"%s"' % s

return f'"{s}"'

Same commit 1acbcfc

marcoesters · 2025-12-18T20:44:58Z

menuinst/utils.py

        """
+        if not name:
+            return name
        if not name or name[0] == name[-1] == pad:


Suggested change

if not name or name[0] == name[-1] == pad:

if name[0] == name[-1] == pad:

Same commit 1acbcfc

marcoesters · 2025-12-18T20:45:41Z

tests/test_utils.py

+
+
+def test_quote_args_1():
+    """Verify the input arguments are quoted."""


Suggested change

"""Verify the input arguments are quoted."""

"""Verify that input arguments are quoted."""

Same commit 1acbcfc

lrandersson requested a review from a team as a code owner October 30, 2025 14:30

conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Oct 30, 2025

conda-bot added this to 🔎 Review Oct 30, 2025

github-project-automation bot moved this to 🆕 New in 🔎 Review Oct 30, 2025

lrandersson commented Oct 30, 2025

View reviewed changes

tests/test_utils.py Show resolved Hide resolved

lrandersson requested a review from marcoesters November 14, 2025 13:38

lrandersson commented Nov 14, 2025

View reviewed changes

lrandersson added 8 commits December 11, 2025 09:38

Fix issue with registry entries not getting quoted properly

9cd3246

Add news

28e8106

Fix format

c10b98f

Add quotes

188b8b7

Update to fix issue with too many quotes

8db7f87

Review fixes

5dc18f9

Add one more test and fixes

954bdcb

Add fix for tests that broke

4a0d634

lrandersson force-pushed the dev-ra-290 branch from 4e59d0e to 4a0d634 Compare December 11, 2025 14:38

lrandersson added 9 commits December 11, 2025 13:29

Updated implementation and added lots of tests

c2c39b3

Fix issues with linter and test

efcbced

Fix typo

43f185b

Add fix

59d0b34

Linter fix

85a9e84

Test correction after last update

76f6598

Ensure test is windows only

92568c2

Add potential fix for testing

ed11e64

pre-commit fix and misc changes

e163f73

marcoesters reviewed Dec 18, 2025

View reviewed changes

Add generic fix and another test for cmd flags

1acbcfc

lrandersson force-pushed the dev-ra-290 branch from a26c392 to 1acbcfc Compare December 19, 2025 14:37

		Return True if input refers to cmd.exe or %COMSPEC%. Here, 'refers' implies variants of
		cmd, cmd.exe, <some path>/cmd and accounting for quoted input.

	if not name or name[0] == name[-1] == pad:
	if name[0] == name[-1] == pad:



		def test_quote_args_1():
		"""Verify the input arguments are quoted."""

	"""Verify the input arguments are quoted."""
	"""Verify that input arguments are quoted."""

Fix issue with registry entries not getting quoted properly #404

Are you sure you want to change the base?

Fix issue with registry entries not getting quoted properly #404

Uh oh!

Conversation

lrandersson commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist - did you ...

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrandersson Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoesters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lrandersson commented Oct 30, 2025 •

edited

Loading

lrandersson Oct 31, 2025 •

edited

Loading