|
| 1 | +From b96c2601af4e01341b4d2c0db494ebee4aef8f42 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Kevin Deldycke <kevin@deldycke.com> |
| 3 | +Date: Wed, 4 Mar 2026 14:51:58 +0400 |
| 4 | +Subject: [PATCH] Document and fix command string sanitizing with `shlex.split` |
| 5 | + |
| 6 | +Removes last use of `shell=True` use for command invokation for defense-in-depth. |
| 7 | +Refs: #1026, #1477 and #2775 |
| 8 | + |
| 9 | +Modified to apply to Azure Linux. |
| 10 | +Refactored code in `_pipepager` and `_tempfilepager` to accept a list of command parts instead of a string, |
| 11 | +and added `shlex.split` to `pager` and `Editor.edit_file` to split the command string into parts. |
| 12 | +Refactored tests as per the existing `Editor.edit_file` API usage. |
| 13 | + |
| 14 | +Upstream Patch reference: https://github.com/pallets/click/commit/b96c2601af4e01341b4d2c0db494ebee4aef8f42.patch |
| 15 | +--- |
| 16 | + src/click/_termui_impl.py | 102 +++++++++++++++++++++++++-------- |
| 17 | + tests/test_termui.py | 115 ++++++++++++++++++++++++++++++++++++++ |
| 18 | + 2 files changed, 193 insertions(+), 24 deletions(-) |
| 19 | + |
| 20 | +diff --git a/src/click/_termui_impl.py b/src/click/_termui_impl.py |
| 21 | +index f744657..3610619 100644 |
| 22 | +--- a/src/click/_termui_impl.py |
| 23 | ++++ b/src/click/_termui_impl.py |
| 24 | +@@ -358,7 +358,21 @@ class ProgressBar(t.Generic[V]): |
| 25 | + |
| 26 | + |
| 27 | + def pager(generator: t.Iterable[str], color: t.Optional[bool] = None) -> None: |
| 28 | +- """Decide what method to use for paging through text.""" |
| 29 | ++ """Decide what method to use for paging through text. |
| 30 | ++ |
| 31 | ++ The ``PAGER`` environment variable is split into an ``argv`` list with |
| 32 | ++ :func:`shlex.split` in its default POSIX mode so that quotes are |
| 33 | ++ stripped from tokens and quoted Windows paths are preserved. |
| 34 | ++ |
| 35 | ++ .. note:: |
| 36 | ++ ``posix=False`` `was considered but rejected |
| 37 | ++ <https://github.com/pallets/click/pull/1477#issuecomment-620231711>`_ |
| 38 | ++ because it retains quote characters in tokens. The |
| 39 | ++ :func:`shlex.quote` approach was also reverted in :pr:`1543`. |
| 40 | ++ |
| 41 | ++ .. seealso:: |
| 42 | ++ :issue:`1026`, :pr:`1477` and :pr:`2775`. |
| 43 | ++ """ |
| 44 | + stdout = _default_text_stdout() |
| 45 | + |
| 46 | + # There are no standard streams attached to write to. For example, |
| 47 | +@@ -368,17 +382,18 @@ def pager(generator: t.Iterable[str], color: t.Optional[bool] = None) -> None: |
| 48 | + |
| 49 | + if not isatty(sys.stdin) or not isatty(stdout): |
| 50 | + return _nullpager(stdout, generator, color) |
| 51 | +- pager_cmd = (os.environ.get("PAGER", None) or "").strip() |
| 52 | +- if pager_cmd: |
| 53 | ++ import shlex |
| 54 | ++ pager_cmd_parts = shlex.split(os.environ.get("PAGER", "")) |
| 55 | ++ if pager_cmd_parts: |
| 56 | + if WIN: |
| 57 | +- return _tempfilepager(generator, pager_cmd, color) |
| 58 | +- return _pipepager(generator, pager_cmd, color) |
| 59 | ++ return _tempfilepager(generator, pager_cmd_parts, color) |
| 60 | ++ return _pipepager(generator, pager_cmd_parts, color) |
| 61 | + if os.environ.get("TERM") in ("dumb", "emacs"): |
| 62 | + return _nullpager(stdout, generator, color) |
| 63 | + if WIN or sys.platform.startswith("os2"): |
| 64 | +- return _tempfilepager(generator, "more <", color) |
| 65 | ++ return _tempfilepager(generator, ["more"], color) |
| 66 | + if hasattr(os, "system") and os.system("(less) 2>/dev/null") == 0: |
| 67 | +- return _pipepager(generator, "less", color) |
| 68 | ++ return _pipepager(generator, ["less"], color) |
| 69 | + |
| 70 | + import tempfile |
| 71 | + |
| 72 | +@@ -392,26 +407,38 @@ def pager(generator: t.Iterable[str], color: t.Optional[bool] = None) -> None: |
| 73 | + os.unlink(filename) |
| 74 | + |
| 75 | + |
| 76 | +-def _pipepager(generator: t.Iterable[str], cmd: str, color: t.Optional[bool]) -> None: |
| 77 | +- """Page through text by feeding it to another program. Invoking a |
| 78 | +- pager through this might support colors. |
| 79 | ++def _pipepager(generator: t.Iterable[str], cmd_parts: t.List[str], color: t.Optional[bool]) -> None: |
| 80 | ++ """Page through text by feeding it to another program. |
| 81 | ++ |
| 82 | ++ Invokes the pager via subprocess with an argv list. If piping to |
| 83 | ++ ``less`` and the user hasn't decided on colors, ``LESS=-R`` is set |
| 84 | ++ automatically. |
| 85 | + """ |
| 86 | + import subprocess |
| 87 | ++ import shutil |
| 88 | + |
| 89 | + env = dict(os.environ) |
| 90 | + |
| 91 | + # If we're piping to less we might support colors under the |
| 92 | + # condition that |
| 93 | +- cmd_detail = cmd.rsplit("/", 1)[-1].split() |
| 94 | +- if color is None and cmd_detail[0] == "less": |
| 95 | +- less_flags = f"{os.environ.get('LESS', '')}{' '.join(cmd_detail[1:])}" |
| 96 | +- if not less_flags: |
| 97 | +- env["LESS"] = "-R" |
| 98 | +- color = True |
| 99 | +- elif "r" in less_flags or "R" in less_flags: |
| 100 | +- color = True |
| 101 | +- |
| 102 | +- c = subprocess.Popen(cmd, shell=True, stdin=subprocess.PIPE, env=env) |
| 103 | ++ if cmd_parts: |
| 104 | ++ cmd_name = os.path.basename(cmd_parts[0]) |
| 105 | ++ if color is None and cmd_name == "less": |
| 106 | ++ less_flags = f"{os.environ.get('LESS', '')}{' '.join(cmd_parts[1:])}" |
| 107 | ++ if not less_flags: |
| 108 | ++ env["LESS"] = "-R" |
| 109 | ++ color = True |
| 110 | ++ elif "r" in less_flags or "R" in less_flags: |
| 111 | ++ color = True |
| 112 | ++ |
| 113 | ++ # Resolve command to absolute path for Windows compatibility |
| 114 | ++ if cmd_parts: |
| 115 | ++ resolved = shutil.which(cmd_parts[0]) or cmd_parts[0] |
| 116 | ++ args = [resolved] + cmd_parts[1:] |
| 117 | ++ else: |
| 118 | ++ args = cmd_parts |
| 119 | ++ |
| 120 | ++ c = subprocess.Popen(args=args, stdin=subprocess.PIPE, env=env) |
| 121 | + stdin = t.cast(t.BinaryIO, c.stdin) |
| 122 | + encoding = get_best_encoding(stdin) |
| 123 | + try: |
| 124 | +@@ -443,10 +470,21 @@ def _pipepager(generator: t.Iterable[str], cmd: str, color: t.Optional[bool]) -> |
| 125 | + |
| 126 | + |
| 127 | + def _tempfilepager( |
| 128 | +- generator: t.Iterable[str], cmd: str, color: t.Optional[bool] |
| 129 | ++ generator: t.Iterable[str], cmd_parts: t.List[str], color: t.Optional[bool] |
| 130 | + ) -> None: |
| 131 | +- """Page through text by invoking a program on a temporary file.""" |
| 132 | ++ """Page through text by invoking a program on a temporary file. |
| 133 | ++ |
| 134 | ++ Used as the primary pager strategy on Windows (where piping to |
| 135 | ++ ``more`` adds spurious ``\r\n``), and as a fallback on other |
| 136 | ++ platforms. The command is resolved to an absolute path with |
| 137 | ++ :func:`shutil.which`. |
| 138 | ++ |
| 139 | ++ Returns ``True`` if the command was found and executed, ``False`` |
| 140 | ++ otherwise so another pager can be attempted. |
| 141 | ++ """ |
| 142 | + import tempfile |
| 143 | ++ import subprocess |
| 144 | ++ import shutil |
| 145 | + |
| 146 | + fd, filename = tempfile.mkstemp() |
| 147 | + # TODO: This never terminates if the passed generator never terminates. |
| 148 | +@@ -457,7 +495,14 @@ def _tempfilepager( |
| 149 | + with open_stream(filename, "wb")[0] as f: |
| 150 | + f.write(text.encode(encoding)) |
| 151 | + try: |
| 152 | +- os.system(f'{cmd} "{filename}"') |
| 153 | ++ resolved = shutil.which(cmd_parts[0]) if cmd_parts else None |
| 154 | ++ if not resolved: |
| 155 | ++ return |
| 156 | ++ args = [resolved] + cmd_parts[1:] + [filename] |
| 157 | ++ subprocess.Popen(args=args).wait() |
| 158 | ++ except OSError: |
| 159 | ++ # Command not found |
| 160 | ++ pass |
| 161 | + finally: |
| 162 | + os.close(fd) |
| 163 | + os.unlink(filename) |
| 164 | +@@ -501,6 +546,15 @@ class Editor: |
| 165 | + return "vi" |
| 166 | + |
| 167 | + def edit_file(self, filename: str) -> None: |
| 168 | ++ """Open files in the user's editor. |
| 169 | ++ |
| 170 | ++ The editor command is split into an ``argv`` list with |
| 171 | ++ :func:`shlex.split` in POSIX mode; see :func:`pager` for rationale. |
| 172 | ++ |
| 173 | ++ .. seealso:: |
| 174 | ++ :issue:`1026` and :pr:`1477`. |
| 175 | ++ """ |
| 176 | ++ import shlex |
| 177 | + import subprocess |
| 178 | + |
| 179 | + editor = self.get_editor() |
| 180 | +@@ -511,7 +565,7 @@ class Editor: |
| 181 | + environ.update(self.env) |
| 182 | + |
| 183 | + try: |
| 184 | +- c = subprocess.Popen(f'{editor} "{filename}"', env=environ, shell=True) |
| 185 | ++ c = subprocess.Popen(args=shlex.split(editor) + [filename], env=environ) |
| 186 | + exit_code = c.wait() |
| 187 | + if exit_code != 0: |
| 188 | + raise ClickException( |
| 189 | +diff --git a/tests/test_termui.py b/tests/test_termui.py |
| 190 | +index 7cfa939..e6390d6 100644 |
| 191 | +--- a/tests/test_termui.py |
| 192 | ++++ b/tests/test_termui.py |
| 193 | +@@ -1,10 +1,12 @@ |
| 194 | + import platform |
| 195 | + import time |
| 196 | ++from unittest.mock import patch |
| 197 | + |
| 198 | + import pytest |
| 199 | + |
| 200 | + import click._termui_impl |
| 201 | + from click._compat import WIN |
| 202 | ++from click._termui_impl import Editor |
| 203 | + |
| 204 | + |
| 205 | + class FakeClock: |
| 206 | +@@ -368,6 +370,119 @@ def test_fast_edit(runner): |
| 207 | + result = click.edit("a\nb", editor="sed -i~ 's/$/Test/'") |
| 208 | + assert result == "aTest\nbTest\n" |
| 209 | + |
| 210 | ++@pytest.mark.parametrize( |
| 211 | ++ ("editor_cmd", "filename", "expected_args"), |
| 212 | ++ [ |
| 213 | ++ pytest.param( |
| 214 | ++ "myeditor --wait --flag", |
| 215 | ++ "file1.txt", |
| 216 | ++ ["myeditor", "--wait", "--flag", "file1.txt"], |
| 217 | ++ id="editor with args", |
| 218 | ++ ), |
| 219 | ++ pytest.param( |
| 220 | ++ "vi", |
| 221 | ++ 'file"; rm -rf / ; echo "', |
| 222 | ++ ["vi", 'file"; rm -rf / ; echo "'], |
| 223 | ++ id="shell metacharacters in filename", |
| 224 | ++ ), |
| 225 | ++ # Issue #1026: editor path with spaces must be quoted. |
| 226 | ++ pytest.param( |
| 227 | ++ '"C:\\Program Files\\Sublime Text 3\\sublime_text.exe"', |
| 228 | ++ "f.txt", |
| 229 | ++ ["C:\\Program Files\\Sublime Text 3\\sublime_text.exe", "f.txt"], |
| 230 | ++ id="quoted windows path with spaces (issue 1026)", |
| 231 | ++ ), |
| 232 | ++ # PR #1477: pager/editor command with flags, like ``less -FRSX``. |
| 233 | ++ pytest.param( |
| 234 | ++ "less -FRSX", |
| 235 | ++ "f.txt", |
| 236 | ++ ["less", "-FRSX", "f.txt"], |
| 237 | ++ id="command with flags (pr 1477)", |
| 238 | ++ ), |
| 239 | ++ # Issue #1026: quoted command with ``--wait`` flag. |
| 240 | ++ pytest.param( |
| 241 | ++ '"my command" --option value arg', |
| 242 | ++ "f.txt", |
| 243 | ++ ["my command", "--option", "value", "arg", "f.txt"], |
| 244 | ++ id="quoted command with args (issue 1026)", |
| 245 | ++ ), |
| 246 | ++ # PR #1477: unquoted unix path. |
| 247 | ++ pytest.param( |
| 248 | ++ "/usr/bin/vim", |
| 249 | ++ "f.txt", |
| 250 | ++ ["/usr/bin/vim", "f.txt"], |
| 251 | ++ id="unix absolute path", |
| 252 | ++ ), |
| 253 | ++ # Issue #1026: macOS path with escaped space. |
| 254 | ++ pytest.param( |
| 255 | ++ "/Applications/Sublime\\ Text.app/Contents/SharedSupport/bin/subl", |
| 256 | ++ "f.txt", |
| 257 | ++ ["/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl", "f.txt"], |
| 258 | ++ id="escaped space in unix path (issue 1026)", |
| 259 | ++ ), |
| 260 | ++ ], |
| 261 | ++) |
| 262 | ++def test_editor_path_normalization(editor_cmd, filename, expected_args): |
| 263 | ++ with patch("subprocess.Popen") as mock_popen: |
| 264 | ++ mock_popen.return_value.wait.return_value = 0 |
| 265 | ++ Editor(editor=editor_cmd).edit_file(filename) |
| 266 | ++ |
| 267 | ++ mock_popen.assert_called_once() |
| 268 | ++ args = mock_popen.call_args[1].get("args") or mock_popen.call_args[0][0] |
| 269 | ++ assert args == expected_args |
| 270 | ++ assert mock_popen.call_args[1].get("shell") is None |
| 271 | ++ |
| 272 | ++ |
| 273 | ++@pytest.mark.skipif(not WIN, reason="Windows-specific editor paths") |
| 274 | ++@pytest.mark.parametrize( |
| 275 | ++ ("editor_cmd", "expected_cmd"), |
| 276 | ++ [ |
| 277 | ++ pytest.param( |
| 278 | ++ "notepad", |
| 279 | ++ ["notepad"], |
| 280 | ++ id="plain notepad", |
| 281 | ++ ), |
| 282 | ++ pytest.param( |
| 283 | ++ '"C:\\Program Files\\Sublime Text 3\\sublime_text.exe" --wait', |
| 284 | ++ ["C:\\Program Files\\Sublime Text 3\\sublime_text.exe", "--wait"], |
| 285 | ++ id="quoted path with flag", |
| 286 | ++ ), |
| 287 | ++ ], |
| 288 | ++) |
| 289 | ++def test_editor_windows_path_normalization(editor_cmd, expected_cmd): |
| 290 | ++ """Windows-specific tests: verify ``Popen`` receives unquoted paths that |
| 291 | ++ ``subprocess.list2cmdline`` can re-quote for ``CreateProcess``.""" |
| 292 | ++ with patch("subprocess.Popen") as mock_popen: |
| 293 | ++ mock_popen.return_value.wait.return_value = 0 |
| 294 | ++ Editor(editor=editor_cmd).edit_file("f.txt") |
| 295 | ++ |
| 296 | ++ args = mock_popen.call_args[1].get("args") or mock_popen.call_args[0][0] |
| 297 | ++ assert args == expected_cmd + ["f.txt"] |
| 298 | ++ assert mock_popen.call_args[1].get("shell") is None |
| 299 | ++ |
| 300 | ++ |
| 301 | ++def test_editor_env_passed_through(): |
| 302 | ++ with patch("subprocess.Popen") as mock_popen: |
| 303 | ++ mock_popen.return_value.wait.return_value = 0 |
| 304 | ++ Editor(editor="vi", env={"MY_VAR": "1"}).edit_file("f.txt") |
| 305 | ++ |
| 306 | ++ env = mock_popen.call_args[1].get("env") |
| 307 | ++ assert env is not None |
| 308 | ++ assert env["MY_VAR"] == "1" |
| 309 | ++ |
| 310 | ++ |
| 311 | ++def test_editor_failure_exception(): |
| 312 | ++ with patch("subprocess.Popen") as mock_popen: |
| 313 | ++ mock_popen.return_value.wait.return_value = 1 |
| 314 | ++ with pytest.raises(click.ClickException, match="Editing failed"): |
| 315 | ++ Editor(editor="vi").edit_file("f.txt") |
| 316 | ++ |
| 317 | ++ |
| 318 | ++def test_editor_nonexistent_exception(): |
| 319 | ++ with patch("subprocess.Popen", side_effect=OSError("not found")): |
| 320 | ++ with pytest.raises(click.ClickException, match="not found"): |
| 321 | ++ Editor(editor="nonexistent").edit_file("f.txt") |
| 322 | ++ |
| 323 | + |
| 324 | + @pytest.mark.parametrize( |
| 325 | + ("prompt_required", "required", "args", "expect"), |
| 326 | +-- |
| 327 | +2.43.0 |
| 328 | + |
0 commit comments