Document and fix command sanitizing with shlex.split

CHANGES.rst

@@ -14,6 +14,9 @@ Unreleased
     with a dedicated CI job. :pr:`3139`
 -   Fix callable ``flag_value`` being instantiated when used as a default via
     ``default=True``. :issue:`3121` :pr:`3201` :pr:`3213` :pr:`3225`
+-   Use :func:`shlex.split` to split pager and editor commands into ``argv``
+    lists for :class:`subprocess.Popen`, removing ``shell=True``.
+    :issue:`1026` :pr:`1477` :pr:`2775`
 
 Version 8.3.1
 --------------

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
+        <https://github.com/pallets/click/pull/1477#issuecomment-620231711>`_
+        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

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"),
     [