gh-133545: Also quote arguments containing &<>^| on Windows

[jhohm]

In addition to space, tab, and empty string, quote arguments containing &<>^| on Windows.

Resolves #133545.

• Issue: subprocess.Popen doesn't properly escape < or > for windows batch files #133545

[gpshead]

looping in @zooba for windows specific experience as it behaves differently than others perhaps do. could this break real use cases that we just lack coverage for when launching processes?

[zooba]

Those characters only need to be quoted when launching cmd.exe, and quoting them for other executables could change their behaviour. So basically, if shell=True then we should do it.

We decided a while back that explicitly launching cmd.exe or a batch file without shell=True shouldn't automatically do shell quoting (as part of that 10.0 CVE in Rust that isn't a security vulnerability 🙃) - users get to opt into shell-style quoting.

I'm pretty sure this change as it stands will affect all executables. We should only check for those extra characters in user-provided arguments when shell=True and allow them unquoted when the developer has specified them (so they can run a command like run(["set", ">env.txt"], shell=True)). Which I think ultimately means we shouldn't be trying to detect them at all.

Perhaps there's a middle ground somewhere, but I'm not sure where it is. But I'm pretty sure this PR isn't it - we definitely need to know about the caller's intent before modifying their arguments here.

[jefsayshi]

I agree that the special characters should not be quoted when shell=True

Also I believe that this test needs to have shell=True or be updated to match the pattern of the other tests

[jefsayshi]

I think the main issue is that currently it is either quoting/escaping too much or too little. I would expect at least one (preferably two) of these test cases would pass, but they can't because redirections aren't quoted automatically and if we try to quote them then those quotes are escaped. This happens regardless if shell is True or False.

    def test_redirect(self):
        with tempfile.NamedTemporaryFile(suffix=".bat", delete_on_close=False) as f:
            f.write(b"@ECHO Off\necho %*\n")
            f.close()
            p = subprocess.run([f.file.name, "hello<world"], capture_output=True)

        self.assertIn(b"hello<world", p.stdout.strip())

    def test_redirect_quoted(self):
        with tempfile.NamedTemporaryFile(suffix=".bat", delete_on_close=False) as f:
            f.write(b"@ECHO Off\necho %*\n")
            f.close()
            p = subprocess.run([f.file.name, "\"hello<world\""], capture_output=True)

        self.assertIn(b"hello<world", p.stdout.strip())
        self.assertNotIn(b"\\\"", p.stdout.strip())

    def test_redirect_shell(self):
        with tempfile.NamedTemporaryFile(suffix=".bat", delete_on_close=False) as f:
            f.write(b"@ECHO Off\necho %*\n")
            f.close()
            p = subprocess.run([f.file.name, "hello<world"], capture_output=True, shell=True)

        self.assertIn(b"hello<world", p.stdout.strip())

    def test_redirect_quoted_shell(self):
        with tempfile.NamedTemporaryFile(suffix=".bat", delete_on_close=False) as f:
            f.write(b"@ECHO Off\necho %*\n")
            f.close()
            p = subprocess.run([f.file.name, "\"hello<world\""], capture_output=True, shell=True)

        self.assertIn(b"hello<world", p.stdout.strip())
        self.assertNotIn(b"\\\"", p.stdout.strip())

[github-actions]

This PR is stale because it has been open for 30 days with no activity.