From b96c2601af4e01341b4d2c0db494ebee4aef8f42 Mon Sep 17 00:00:00 2001 From: Kevin Deldycke Date: Wed, 4 Mar 2026 14:51:58 +0400 Subject: [PATCH] Document and fix command string sanitizing with `shlex.split` Removes last use of `shell=True` use for command invokation for defense-in-depth. Refs: #1026, #1477 and #2775 --- CHANGES.rst | 3 + src/click/_termui_impl.py | 65 ++++++++++++++++----- tests/test_termui.py | 116 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 170 insertions(+), 14 deletions(-) diff --git a/src/click/_termui_impl.py b/src/click/_termui_impl.py index ee8225c4c2..c1c230d26c 100644 --- a/src/click/_termui_impl.py +++ b/src/click/_termui_impl.py @@ -367,7 +367,21 @@ def generator(self) -> cabc.Iterator[V]: def pager(generator: cabc.Iterable[str], color: bool | None = None) -> None: - """Decide what method to use for paging through text.""" + """Decide what method to use for paging through text. + + The ``PAGER`` environment variable is split into an ``argv`` list with + :func:`shlex.split` in its default POSIX mode so that quotes are + stripped from tokens and quoted Windows paths are preserved. + + .. note:: + ``posix=False`` `was considered but rejected + `_ + because it retains quote characters in tokens. The + :func:`shlex.quote` approach was also reverted in :pr:`1543`. + + .. seealso:: + :issue:`1026`, :pr:`1477` and :pr:`2775`. + """ stdout = _default_text_stdout() # There are no standard streams attached to write to. For example, @@ -378,8 +392,7 @@ def pager(generator: cabc.Iterable[str], color: bool | None = None) -> None: if not isatty(sys.stdin) or not isatty(stdout): return _nullpager(stdout, generator, color) - # Split and normalize the pager command into parts. - pager_cmd_parts = shlex.split(os.environ.get("PAGER", ""), posix=False) + pager_cmd_parts = shlex.split(os.environ.get("PAGER", "")) if pager_cmd_parts: if WIN: if _tempfilepager(generator, pager_cmd_parts, color): @@ -411,11 +424,24 @@ def pager(generator: cabc.Iterable[str], color: bool | None = None) -> None: def _pipepager( generator: cabc.Iterable[str], cmd_parts: list[str], color: bool | None ) -> bool: - """Page through text by feeding it to another program. Invoking a - pager through this might support colors. + """Page through text by feeding it to another program. + + Invokes the pager via :class:`subprocess.Popen` with an ``argv`` list + produced by :func:`shlex.split`. The command is resolved to an absolute + path with :func:`shutil.which` as recommended by the + :mod:`subprocess` docs for Windows compatibility. + + Invoking a pager through this might support colors: if piping to + ``less`` and the user hasn't decided on colors, ``LESS=-R`` is set + automatically. + + Returns ``True`` if the command was found and executed, ``False`` + otherwise so another pager can be attempted. - Returns `True` if the command was found, `False` otherwise and thus another - pager should be attempted. + .. seealso:: + :pr:`2775` improved error handling: :exc:`BrokenPipeError` is + caught specifically, generator exceptions terminate the pager, and + ``stdin.close()`` is always called in a ``finally`` block. """ # Split the command into the invoked CLI and its parameters. if not cmd_parts: @@ -509,8 +535,13 @@ def _tempfilepager( ) -> bool: """Page through text by invoking a program on a temporary file. - Returns `True` if the command was found, `False` otherwise and thus another - pager should be attempted. + Used as the primary pager strategy on Windows (where piping to + ``more`` adds spurious ``\\r\\n``), and as a fallback on other + platforms. The command is resolved to an absolute path with + :func:`shutil.which`. + + Returns ``True`` if the command was found and executed, ``False`` + otherwise so another pager can be attempted. """ # Split the command into the invoked CLI and its parameters. if not cmd_parts: @@ -592,6 +623,15 @@ def get_editor(self) -> str: return "vi" def edit_files(self, filenames: cabc.Iterable[str]) -> None: + """Open files in the user's editor. + + The editor command is split into an ``argv`` list with + :func:`shlex.split` in POSIX mode; see :func:`pager` for rationale. + + .. seealso:: + :issue:`1026` and :pr:`1477`. + """ + import shlex import subprocess editor = self.get_editor() @@ -601,11 +641,10 @@ def edit_files(self, filenames: cabc.Iterable[str]) -> None: environ = os.environ.copy() environ.update(self.env) - exc_filename = " ".join(f'"{filename}"' for filename in filenames) - try: c = subprocess.Popen( - args=f"{editor} {exc_filename}", env=environ, shell=True + args=shlex.split(editor) + list(filenames), + env=environ, ) exit_code = c.wait() if exit_code != 0: @@ -754,8 +793,6 @@ def _translate_ch_to_exc(ch: str) -> None: if ch == "\x1a" and WIN: # Windows, Ctrl+Z raise EOFError() - return None - if sys.platform == "win32": import msvcrt diff --git a/tests/test_termui.py b/tests/test_termui.py index 8220431bb4..0883906f43 100644 --- a/tests/test_termui.py +++ b/tests/test_termui.py @@ -1,11 +1,13 @@ import platform import tempfile import time +from unittest.mock import patch import pytest import click._termui_impl from click._compat import WIN +from click._termui_impl import Editor from click.exceptions import BadParameter from click.exceptions import MissingParameter @@ -404,6 +406,120 @@ def test_edit(runner): assert reopened_file.read() == "aTest\nbTest\n" +@pytest.mark.parametrize( + ("editor_cmd", "filenames", "expected_args"), + [ + pytest.param( + "myeditor --wait --flag", + ["file1.txt", "file2.txt"], + ["myeditor", "--wait", "--flag", "file1.txt", "file2.txt"], + id="editor with args", + ), + pytest.param( + "vi", + ['file"; rm -rf / ; echo "'], + ["vi", 'file"; rm -rf / ; echo "'], + id="shell metacharacters in filename", + ), + # Issue #1026: editor path with spaces must be quoted. + pytest.param( + '"C:\\Program Files\\Sublime Text 3\\sublime_text.exe"', + ["f.txt"], + ["C:\\Program Files\\Sublime Text 3\\sublime_text.exe", "f.txt"], + id="quoted windows path with spaces (issue 1026)", + ), + # PR #1477: pager/editor command with flags, like ``less -FRSX``. + pytest.param( + "less -FRSX", + ["f.txt"], + ["less", "-FRSX", "f.txt"], + id="command with flags (pr 1477)", + ), + # Issue #1026: quoted command with ``--wait`` flag. + pytest.param( + '"my command" --option value arg', + ["f.txt"], + ["my command", "--option", "value", "arg", "f.txt"], + id="quoted command with args (issue 1026)", + ), + # PR #1477: unquoted unix path. + pytest.param( + "/usr/bin/vim", + ["f.txt"], + ["/usr/bin/vim", "f.txt"], + id="unix absolute path", + ), + # Issue #1026: macOS path with escaped space. + pytest.param( + "/Applications/Sublime\\ Text.app/Contents/SharedSupport/bin/subl", + ["f.txt"], + ["/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl", "f.txt"], + id="escaped space in unix path (issue 1026)", + ), + ], +) +def test_editor_path_normalization(editor_cmd, filenames, expected_args): + with patch("subprocess.Popen") as mock_popen: + mock_popen.return_value.wait.return_value = 0 + Editor(editor=editor_cmd).edit_files(filenames) + + mock_popen.assert_called_once() + args = mock_popen.call_args[1].get("args") or mock_popen.call_args[0][0] + assert args == expected_args + assert mock_popen.call_args[1].get("shell") is None + + +@pytest.mark.skipif(not WIN, reason="Windows-specific editor paths") +@pytest.mark.parametrize( + ("editor_cmd", "expected_cmd"), + [ + pytest.param( + "notepad", + ["notepad"], + id="plain notepad", + ), + pytest.param( + '"C:\\Program Files\\Sublime Text 3\\sublime_text.exe" --wait', + ["C:\\Program Files\\Sublime Text 3\\sublime_text.exe", "--wait"], + id="quoted path with flag", + ), + ], +) +def test_editor_windows_path_normalization(editor_cmd, expected_cmd): + """Windows-specific tests: verify ``Popen`` receives unquoted paths that + ``subprocess.list2cmdline`` can re-quote for ``CreateProcess``.""" + with patch("subprocess.Popen") as mock_popen: + mock_popen.return_value.wait.return_value = 0 + Editor(editor=editor_cmd).edit_files(["f.txt"]) + + args = mock_popen.call_args[1].get("args") or mock_popen.call_args[0][0] + assert args == expected_cmd + ["f.txt"] + assert mock_popen.call_args[1].get("shell") is None + + +def test_editor_env_passed_through(): + with patch("subprocess.Popen") as mock_popen: + mock_popen.return_value.wait.return_value = 0 + Editor(editor="vi", env={"MY_VAR": "1"}).edit_files(["f.txt"]) + + env = mock_popen.call_args[1].get("env") + assert env is not None + assert env["MY_VAR"] == "1" + + +def test_editor_failure_exception(): + with patch("subprocess.Popen") as mock_popen: + mock_popen.return_value.wait.return_value = 1 + with pytest.raises(click.ClickException, match="Editing failed"): + Editor(editor="vi").edit_files(["f.txt"]) + + +def test_editor_nonexistent_exception(): + with patch("subprocess.Popen", side_effect=OSError("not found")): + with pytest.raises(click.ClickException, match="not found"): + Editor(editor="nonexistent").edit_files(["f.txt"]) + + @pytest.mark.parametrize( ("prompt_required", "required", "args", "expect"), [