fix: address batch quoting review findings

- reject arguments containing %VAR% references that cmd.exe would
  expand (verified: expansion happens even inside double quotes and can
  inject commands from environment values); references to undefined
  variables are left alone since cmd.exe passes them through literally
- quote ( and ) as metacharacters: an unquoted ")" inside a conda.bat
  style "if (...)" block ends the block early, silently corrupting the
  argument or aborting the script
- accept pre-quoted arguments such as '"urllib3<1.25"' again (the
  historical workaround from #312) instead of raising ValueError

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bwhe91T5gnFxoeS2WLKfM4

Author: henryiii

nox/popen.py

@@ -17,7 +17,9 @@
 __lazy_modules__ = {"contextlib", "locale"}
 
 import contextlib
+import itertools
 import locale
+import os
 import subprocess
 import sys
 from typing import IO, TYPE_CHECKING
@@ -40,20 +42,79 @@ def __dir__() -> list[str]:
 DEFAULT_INTERRUPT_TIMEOUT = 0.3
 DEFAULT_TERMINATE_TIMEOUT = 0.2
 
-_CMD_META = frozenset("&<>^|")
-
-
-def _windows_batch_command(args: Sequence[str]) -> str:
-    """Build a command line that protects the cmd.exe metacharacters ``&<>^|``.
-
-    Note that ``%`` cannot be protected at this layer: cmd.exe expands
-    ``%VAR%`` references even inside double quotes.
+_CMD_META = frozenset("&<>^|()")
+
+# Variables cmd.exe resolves dynamically or defines itself, so a reference to
+# one expands even when it is absent from the child process environment.
+_CMD_AUTO_VARS = frozenset(
+    {
+        "__APPDIR__",
+        "__CD__",
+        "CD",
+        "CMDCMDLINE",
+        "CMDEXTVERSION",
+        "COMSPEC",
+        "DATE",
+        "ERRORLEVEL",
+        "HIGHESTNUMANODENUMBER",
+        "PATHEXT",
+        "PROMPT",
+        "RANDOM",
+        "TIME",
+    }
+)
+
+
+def _expandable_reference(arg: str, env: Mapping[str, str]) -> str | None:
+    """Find a ``%VAR%`` reference that cmd.exe would expand, if there is one.
+
+    cmd.exe scans the text between each pair of ``%`` characters; when it
+    names a defined (or dynamic) variable - optionally followed by ``:``
+    modifiers - the value is substituted, even inside double quotes. An
+    undefined name is left alone and the scan resumes at the trailing ``%``.
     """
+    defined = {name.upper() for name in env}
+    percents = [index for index, char in enumerate(arg) if char == "%"]
+    for start, end in itertools.pairwise(percents):
+        name = arg[start + 1 : end].partition(":")[0].upper()
+        if name and (
+            name in defined
+            or name in _CMD_AUTO_VARS
+            # hidden variables such as the per-drive working directory "=C:"
+            or name.startswith("=")
+        ):
+            return arg[start : end + 1]
+    return None
+
+
+def _windows_batch_command(
+    args: Sequence[str], env: Mapping[str, str] | None = None
+) -> str:
+    """Build a command line that protects the cmd.exe metacharacters ``&<>^|()``.
+
+    Arguments cmd.exe would corrupt anyway are rejected: ``%VAR%`` references
+    are expanded even inside double quotes, and quote characters cannot be
+    escaped - cmd.exe knows no backslash escapes, it only counts quotes.
+    """
+    if env is None:
+        env = os.environ
 
     quoted_args = []
     for arg in args:
+        reference = _expandable_reference(arg, env)
+        if reference is not None:
+            msg = (
+                "Cannot escape argument for batch script, cmd.exe would"
+                f" expand {reference}: {arg}"
+            )
+            raise ValueError(msg)
         if _CMD_META.isdisjoint(arg):
             quoted_args.append(subprocess.list2cmdline([arg]))
+        elif len(arg) >= 2 and arg[0] == arg[-1] == '"' and '"' not in arg[1:-1]:
+            # A pre-quoted argument such as '"urllib3<1.25"' (the historical
+            # workaround): its outer quotes already protect the
+            # metacharacters, so pass it through unchanged.
+            quoted_args.append(arg)
         elif '"' in arg:
             # list2cmdline escapes embedded quotes with backslashes, but
             # cmd.exe knows no backslash escapes - it only counts quote
@@ -127,7 +188,7 @@ def popen(
 
     popen_args: Sequence[str] | str = args
     if sys.platform.startswith("win") and args[0].casefold().endswith((".bat", ".cmd")):
-        popen_args = _windows_batch_command(args)
+        popen_args = _windows_batch_command(args, env)
 
     proc = subprocess.Popen(popen_args, env=env, stdout=stdout, stderr=stderr)
 

tests/test_command.py

@@ -57,13 +57,14 @@ def _write_echo_batch(tmp_path: Path, suffix: str = ".bat") -> Path:
 
 def test_windows_batch_command() -> None:
     command = nox.popen._windows_batch_command(
-        [r"C:\Program Files\tool.cmd", "plain", "has space", "requests<99"]
+        [r"C:\Program Files\tool.cmd", "plain", "has space", "requests<99", "py(3)"]
     )
 
-    assert command == r'"C:\Program Files\tool.cmd" plain "has space" "requests<99"'
+    expected = r'"C:\Program Files\tool.cmd" plain "has space" "requests<99" "py(3)"'
+    assert command == expected
 
 
-@pytest.mark.parametrize("argument", ['"urllib3<1.25"', 'a"<b', 'a">b'])
+@pytest.mark.parametrize("argument", ['a"<b', 'a">b', '"urllib3<1.25', 'urllib3<1.25"'])
 def test_windows_batch_command_rejects_quoted_metacharacters(argument: str) -> None:
     # cmd.exe only counts quote characters, so an argument mixing quotes and
     # metacharacters cannot be escaped reliably; 'a">b' would even run
@@ -72,10 +73,62 @@ def test_windows_batch_command_rejects_quoted_metacharacters(argument: str) -> N
         nox.popen._windows_batch_command([r"C:\tool.bat", argument])
 
 
+def test_windows_batch_command_accepts_pre_quoted_argument() -> None:
+    # the historical workaround from #312: the caller quotes the constraint
+    # themselves, and the outer quotes already protect the metacharacters.
+    command = nox.popen._windows_batch_command([r"C:\tool.bat", '"urllib3<1.25"'])
+
+    assert command == r'C:\tool.bat "urllib3<1.25"'
+
+
+@pytest.mark.parametrize(
+    "argument",
+    ["%NOX_TEST_VAR%", "pre%NOX_TEST_VAR%post", "%%NOX_TEST_VAR%%", '"%NOX_TEST_VAR%"'],
+)
+def test_windows_batch_command_rejects_percent_expansion(argument: str) -> None:
+    # cmd.exe expands %VAR% references even inside double quotes, so an
+    # argument referencing a defined variable cannot be passed through
+    # faithfully; a variable value could even inject extra commands.
+    with pytest.raises(ValueError, match="would expand %NOX_TEST_VAR%"):
+        nox.popen._windows_batch_command(
+            [r"C:\tool.bat", argument], {"nox_test_var": "value"}
+        )
+
+
+@pytest.mark.parametrize("argument", ["%RANDOM%", "%__CD__%", "%=C:%", "%PATH:;=,%"])
+def test_windows_batch_command_rejects_dynamic_percent_expansion(
+    argument: str,
+) -> None:
+    # these expand even when the variable is missing from the child
+    # environment: cmd.exe resolves them dynamically or defines them itself.
+    with pytest.raises(ValueError, match="would expand"):
+        nox.popen._windows_batch_command([r"C:\tool.bat", argument], {"PATH": "C:;D:"})
+
+
+@pytest.mark.parametrize(
+    "argument", ["100%", "%%", "%undefined_variable%", "https://x.test/%20a%20b.whl"]
+)
+def test_windows_batch_command_allows_literal_percents(argument: str) -> None:
+    # cmd.exe leaves references to undefined variables alone, so these
+    # arguments survive verbatim and are safe to pass through.
+    command = nox.popen._windows_batch_command([r"C:\tool.bat", argument], {})
+
+    assert command == f"C:\\tool.bat {argument}"
+
+
 @only_on_windows
 @pytest.mark.parametrize("suffix", [".bat", ".cmd"])
 @pytest.mark.parametrize(
-    "argument", ["name&value", "name<value", "name>value", "name^value", "name|value"]
+    "argument",
+    [
+        "name&value",
+        "name<value",
+        "name>value",
+        "name^value",
+        "name|value",
+        "name(value",
+        "name)value",
+    ],
 )
 def test_run_windows_batch_metacharacter_arg(
     tmp_path: Path, suffix: str, argument: str
@@ -87,6 +140,34 @@ def test_run_windows_batch_metacharacter_arg(
     assert result.strip() == argument
 
 
+@only_on_windows
+def test_run_windows_batch_paren_arg_in_block(tmp_path: Path) -> None:
+    # conda.bat-style scripts wrap commands in if (...) blocks, where an
+    # unquoted ")" in an argument ends the block early and silently corrupts
+    # the argument (or aborts with "... was unexpected at this time").
+    batch = tmp_path / "block.bat"
+    batch.write_text(
+        f'@echo off\nif "1"=="1" (\n'
+        f'  "{PYTHON}" -c "import sys; print(sys.argv[1])" %*\n)\n',
+        encoding="utf-8",
+    )
+
+    result = nox.command.run([batch, "(pkg)"], silent=True)
+
+    assert result.strip() == "(pkg)"
+
+
+@only_on_windows
+def test_run_windows_batch_pre_quoted_arg(tmp_path: Path) -> None:
+    # the pre-quoted idiom from #312 keeps working; the child process sees
+    # the argument without the caller's quotes, which is what conda expects.
+    batch = _write_echo_batch(tmp_path)
+
+    result = nox.command.run([batch, '"urllib3<1.25"'], silent=True)
+
+    assert result.strip() == "urllib3<1.25"
+
+
 @only_on_windows
 def test_run_windows_batch_quoted_metacharacter_arg(
     tmp_path: Path, monkeypatch: pytest.MonkeyPatch