Fix issue with registry entries not getting quoted properly #404
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
| from contextlib import suppress | ||
| from functools import lru_cache, wraps | ||
| from logging import getLogger | ||
| from pathlib import Path, PurePath | ||
| from typing import Any, Callable, Iterable, Literal, Mapping, Optional, Sequence, Union | ||
| from unicodedata import normalize | ||
|
|
||
|
|
@@ -136,50 +136,140 @@ def add_xml_child(parent: XMLTree.Element, tag: str, text: Optional[str] = None) | |
|
|
||
|
|
||
| class WinLex: | ||
| # Note that there are more characters, for example parenthesis, | ||
| # but in our case parenthesis are required to be quoted for example with | ||
| # python -c "print('foo')" | ||
| _META_CHARS = (">", "<", "|", "&") | ||
|
|
||
| @classmethod | ||
| def quote_args(cls, args: Sequence[str]): | ||
| # cmd.exe /K or /C expects a single string argument and requires | ||
| # doubled-up quotes when any sub-arguments have spaces: | ||
| # https://stackoverflow.com/a/6378038/3257826 | ||
|
|
||
| if len(args) > 2 and cls._is_cmd_exe(args[0]): | ||
| # and (args[1].upper() == "/K" or args[1].upper() == "/C") | ||
| # and any(" " in arg for arg in args[2:]) | ||
| switch_index = None | ||
| for index in range(1, len(args)): | ||
| a = args[index].upper() | ||
| if a == "/C" or a == "/K": | ||
| switch_index = index | ||
| break | ||
|
|
||
| # If we found /C or /K, proceed | ||
| if switch_index is not None: | ||
| tail = args[switch_index + 1 :] | ||
| if tail and any(" " in entry for entry in tail): | ||
| # Quote the call to cmd.exe only if it needs it | ||
| cmd0 = ( | ||
| cls.ensure_pad(args[0], '"') | ||
| if cls._needs_quotes_for_cmd(args[0]) | ||
| else args[0] | ||
| ) | ||
|
|
||
| # Preserve the flags between cmd and /C or /K | ||
| pre_flags = args[1:switch_index] | ||
|
|
||
| # For the tail (the command string passed to cmd.exe), quote each | ||
| # sub-arg with " only if its needed; then join; then | ||
| # wrap the WHOLE tail in one pair of quotes. | ||
| tail_parts = [ | ||
| (cls.ensure_pad(a, '"') if cls._needs_quotes_for_cmd(a) else a) | ||
| for a in tail | ||
| ] | ||
| tail_str = " ".join(tail_parts) | ||
| return [cmd0, *pre_flags, args[switch_index], f'"{tail_str}"'] | ||
|
|
||
| # Generic quoting for non-cmd | ||
| args = [cls.quote_string(arg) for arg in args] | ||
| return args | ||
|
|
||
| @classmethod | ||
| def _is_cmd_exe(cls, s: str) -> bool: | ||
| """ | ||
| 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. | ||
|
Comment on lines
+191
to
+192
Contributor
There was a problem hiding this comment. Maybe call it
Contributor
Author
There was a problem hiding this comment. Good point, fixed! |
||
| """ | ||
| t = s.strip('"') | ||
| # Accept %COMSPEC% | ||
| if t.upper() == "%COMSPEC%": | ||
| return True | ||
| # Accept bare name or any path whose basename startswith 'cmd' and | ||
| # endswith '.exe' (or no extension) | ||
| name = PurePath(t).name.upper() | ||
| return name == "CMD" or name == "CMD.EXE" | ||
|
|
||
| @classmethod | ||
| def quote_string(cls, s: str): | ||
| """ | ||
| Quote given input if necessary. | ||
| This is based on the following rules: | ||
| * Preserve already-quoted input.. | ||
| * Quote args with spaces | ||
| * Quote path-like input with variables, for example %FOO%\\foo.exe. | ||
| * Quote positional args such as %1, %2, ..., %n where n is a positive integer. | ||
| * Don't auto-quote shell metacharacters (>, <, |, &). | ||
| * Don't auto-quote just because of '%' (it changes observable output). | ||
| """ | ||
| if s == "": | ||
| return '""' | ||
|
|
||
| # Don't quote already quoted input. | ||
| # Examples: '""', '"%1"', '"C:\\Path With Spaces"' | ||
| if len(s) >= 2 and s[0] == s[-1] == '"': | ||
| return s | ||
|
|
||
| # Don't add quotes for minus or leading space | ||
| if s[0] in ("-", " "): | ||
| return s | ||
|
|
||
| # Don't quote shell meta tokens; quoting would change meaning. | ||
| # Example: echo %FOO%> output.txt | ||
| if cls._has_shell_meta(s): | ||
|
Contributor
Author
There was a problem hiding this comment. This was added because tests broke from my initial changes (in particular it was the
Contributor
There was a problem hiding this comment. I'm trying to understand the impact of this. That would mean that
Contributor
Author
There was a problem hiding this comment. 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. |
||
| return s | ||
|
|
||
| # %1, %2, %3... | ||
| if len(s) >= 2 and s[0] == "%" and s[1:].isdigit(): | ||
| return f'"{s}"' | ||
|
|
||
| # Situation with %VAR%\\f.exe or %VAR%/f.exe | ||
| if "%" in s and ("/" in s or "\\" in s): | ||
| return f'"{s}"' | ||
|
|
||
| if " " in s: | ||
| return f'"{s}"' | ||
| return s | ||
|
|
||
| @classmethod | ||
| def _has_shell_meta(cls, a: str) -> bool: | ||
| """ | ||
| Detect shell metacharacters that must remain unquoted to preserve meaning. | ||
| """ | ||
| return any(ch in a for ch in cls._META_CHARS) | ||
|
|
||
| @classmethod | ||
| def _needs_quotes_for_cmd(cls, s: str) -> bool: | ||
| """ | ||
| Return True if input contains space. | ||
| """ | ||
| return " " in s | ||
|
|
||
| @classmethod | ||
| def ensure_pad(cls, name: str, pad: str = "_") -> str: | ||
| """ | ||
| Pad the input set via 'name' with the string set via keyword-argument 'pad'. | ||
| If pad='"', then padding is only added if the input contains a space or | ||
| a percentage sign. | ||
|
|
||
| Examples: | ||
| >>> ensure_pad('conda') | ||
| '_conda_' | ||
|
|
||
| """ | ||
| if not name: | ||
| return name | ||
| if name[0] == name[-1] == pad: | ||
| return name | ||
| else: | ||
| return "%s%s%s" % (pad, name, pad) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| ### Enhancements | ||
|
|
||
| * <news item> | ||
|
|
||
| ### Bug fixes | ||
|
|
||
| * Registry entries are now properly quoted. This resolves an issue where opening files associated with a program could fail if its path included blank spaces. (#290 via #404) | ||
|
|
||
| ### Deprecations | ||
|
|
||
| * <news item> | ||
|
|
||
| ### Docs | ||
|
|
||
| * <news item> | ||
|
|
||
| ### Other | ||
|
|
||
| * <news item> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| """ | ||
| Tests for specific functions in the utils package. | ||
| """ | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| import pytest | ||
| from conftest import PLATFORM | ||
|
|
||
| pytestmark = pytest.mark.skipif( | ||
| PLATFORM != "win", reason="This file contains tests that are Windows only." | ||
| ) | ||
|
|
||
| if TYPE_CHECKING: | ||
| from menuinst.utils import WinLex | ||
| else: | ||
| WinLex = pytest.importorskip("menuinst.utils").WinLex | ||
|
|
||
|
|
||
| def test_quote_args_1(): | ||
| """Verify that input arguments are quoted.""" | ||
| wl = WinLex() | ||
| test_str = [r"%SystemRoot%\system32\foo.exe", "/pt", "%1", "%2", "%3", "%4"] | ||
| output = wl.quote_args(test_str) | ||
| assert output == [r'"%SystemRoot%\system32\foo.exe"', "/pt", '"%1"', '"%2"', '"%3"', '"%4"'] | ||
|
|
||
|
|
||
| def test_quote_args_2(): | ||
| """Verify correct quotes with blank spaces.""" | ||
| wl = WinLex() | ||
| args = [ | ||
| r"C:\Windows\System32\notepad.exe", | ||
lrandersson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| "/pt", | ||
| r"C:\Users\Foo Bar\file.txt", | ||
| "S p a c e s", | ||
| ] | ||
| output = wl.quote_args(args) | ||
| assert output == [ | ||
| r"C:\Windows\System32\notepad.exe", | ||
| "/pt", | ||
| r'"C:\Users\Foo Bar\file.txt"', | ||
| '"S p a c e s"', | ||
| ] | ||
|
|
||
|
|
||
| def test_quote_args_3(): | ||
| """Verify special case with cmd.exe and /C.""" | ||
| args = ["cmd.exe", "/C", "echo", "Hello World"] | ||
| wl = WinLex() | ||
| output = wl.quote_args(args) | ||
| assert output == ["cmd.exe", "/C", '"echo "Hello World""'] | ||
|
|
||
|
|
||
| def test_quote_args_4(): | ||
| """Verify special case with cmd.exe and /K many words with spaces and percentage signs.""" | ||
| args = ["cmd.exe", "/K", "dir", "Hello World", "%1 %2 %foo% with spaces", "x", "y"] | ||
| wl = WinLex() | ||
| output = wl.quote_args(args) | ||
| assert output == ["cmd.exe", "/K", '"dir "Hello World" "%1 %2 %foo% with spaces" x y"'] | ||
|
|
||
|
|
||
| def test_quote_args_5(): | ||
| """Verify quotation works with mix of paths and spaces.""" | ||
| cmd = [r"C:\Program Files\App\app.exe", r"C:\Users\Me\My File.txt"] | ||
| assert " ".join(WinLex.quote_args(cmd)) == ( | ||
| r'"C:\Program Files\App\app.exe" "C:\Users\Me\My File.txt"' | ||
| ) | ||
|
|
||
|
|
||
| def test_quote_args_6(): | ||
| """Verify command with metachars quotes as expected.""" | ||
| cmd = ["cmd", "/K", "echo", "%FOO%>", "out.txt"] | ||
| line = " ".join(WinLex.quote_args(cmd)) | ||
| assert "cmd /K echo %FOO%> out.txt" == line | ||
|
|
||
|
|
||
| def test_quote_args_7(): | ||
| """Test with Powershell and spaced script path.""" | ||
| cmd = [ | ||
| r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe", | ||
| "-WindowStyle", | ||
| "hidden", | ||
| r"C:\path with spaces\script.ps1", | ||
| ] | ||
| line = " ".join(WinLex.quote_args(cmd)) | ||
| assert r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe" in line | ||
| assert r'"C:\path with spaces\script.ps1"' in line | ||
|
|
||
|
|
||
| def test_quote_args_8(): | ||
| """Verify special case with spaces, cmd.exe and /K and percentage signs.""" | ||
| args = [ | ||
| r"C:\path with spaces\cmd.exe", | ||
| "/K", | ||
| "dir", | ||
| "Hello World", | ||
| "%1 %2 %foo% with spaces", | ||
| "x", | ||
| "y", | ||
| ] | ||
| wl = WinLex() | ||
| output = wl.quote_args(args) | ||
| assert output == [ | ||
| r'"C:\path with spaces\cmd.exe"', | ||
| "/K", | ||
| '"dir "Hello World" "%1 %2 %foo% with spaces" x y"', | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("special_flag", ("/C", "/K")) | ||
| def test_quote_args_special_flag(special_flag): | ||
| """Verify special case with spaces, cmd.exe, multiple flags and percentage sign.""" | ||
| args = [ | ||
| "cmd.exe", | ||
| "/A", | ||
| "/B", | ||
| special_flag, | ||
| r"C:\Foo Bar\bin.exe", | ||
| "%1", | ||
| ] | ||
| wl = WinLex() | ||
| output = wl.quote_args(args) | ||
| assert output == [ | ||
| "cmd.exe", | ||
| "/A", | ||
| "/B", | ||
| special_flag, | ||
| '""C:\\Foo Bar\\bin.exe" %1"', | ||
| ] | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will removing the quotes be a problem for paths with spaces? I don't see a test covering that example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quotes are actually not removed, it's just that we let
WinLex.quote_argsdo the work instead of manually quoting it here, to have more of a single source of quoting. Note that we returnreturn WinLex.quote_args(command)instead ofreturn commandin this PR.