fix: only hand the Docutils source to a Python interpreter

Fixes #1137

Signed-off-by: Joseph Kato <joseph@jdkato.io>

Author: jdkato

internal/lint/rst.go

@@ -82,12 +82,22 @@ var (
 	rstDirect []string // <python> -c <server>, or nil
 )
 
+// rePython matches the names Python is installed under -- `python`, `python3`,
+// `python3.12`, `pypy3` -- and nothing else.
+var rePython = regexp.MustCompile(`(?i)^(?:python|pypy)[0-9.]*(?:\.exe)?$`)
+
 // rstInterpreter finds the Python that can import Docutils.
 //
 // It is not necessarily the `python3` on PATH: rst2html is installed with a
 // shebang naming the interpreter of the environment Docutils was installed
 // into, and on a machine with several Pythons the one on PATH often cannot
 // import it. So the script is asked which interpreter it runs on.
+//
+// The answer is only used when it names Python. rst2html is not always the
+// script it looks like: a version manager can put a shell wrapper on PATH under
+// that name, and the wrapper's shebang names its own shell. Passing the server
+// source to a shell runs its first line, `import sys`, as a command -- which is
+// a real program on a machine with ImageMagick. See #1137.
 func rstInterpreter(exe string) string {
 	resolved, err := filepath.EvalSymlinks(exe)
 	if err != nil {
@@ -111,46 +121,67 @@ func rstInterpreter(exe string) string {
 	}
 
 	// `#!/usr/bin/env python3` names the interpreter in the second field.
-	if filepath.Base(fields[0]) == "env" {
+	python := fields[0]
+	if filepath.Base(python) == "env" {
 		if len(fields) < 2 {
 			return ""
 		}
-		return system.Which([]string{fields[1]})
+		python = fields[1]
+	}
+
+	if !rePython.MatchString(filepath.Base(python)) {
+		return ""
+	} else if !filepath.IsAbs(python) {
+		return system.Which([]string{python})
 	}
 
-	return fields[0]
+	return python
 }
 
 // rstFastPath returns the argv prefix for converting through a long-lived
 // interpreter, or nil when that could not be established.
 func rstFastPath(exe string) []string {
 	rstOnce.Do(func() {
-		python := rstInterpreter(exe)
-		if python == "" {
-			return
-		}
+		rstDirect = rstProbe(exe)
+	})
 
-		candidate := []string{python, "-c", rstServer}
+	return rstDirect
+}
 
-		// Trust it only after a document has made the round trip. The probe
-		// carries a non-ASCII character on purpose: a mismatched default
-		// encoding is what broke the first version of the AsciiDoc pool, and
-		// an ASCII-only probe would have passed anyway.
-		probe, err := startExtProc(candidate, rstArgs)
-		if err != nil {
-			return
-		}
-		defer probe.close()
+// rstProbe establishes the argv for a long-lived interpreter, or nil.
+func rstProbe(exe string) []string {
+	python := rstInterpreter(exe)
+	if python == "" {
+		// Either there was no shebang to read or it named something other than
+		// Python. A Python on PATH is the next guess: where the script is a
+		// wrapper, that wrapper forwards to the interpreter it would have used
+		// anyway. The probe below is what decides whether the guess can do the
+		// work, so a wrong one costs the pool, not correctness.
+		python = system.Which([]string{"python3", "python"})
+	}
 
-		got, err := probe.convert("naïve body\n")
-		if err != nil || !strings.Contains(got, "naïve body") {
-			return
-		}
+	if python == "" {
+		return nil
+	}
 
-		rstDirect = candidate
-	})
+	candidate := []string{python, "-c", rstServer}
 
-	return rstDirect
+	// Trust it only after a document has made the round trip. The probe
+	// carries a non-ASCII character on purpose: a mismatched default
+	// encoding is what broke the first version of the AsciiDoc pool, and
+	// an ASCII-only probe would have passed anyway.
+	probe, err := startExtProc(candidate, rstArgs)
+	if err != nil {
+		return nil
+	}
+	defer probe.close()
+
+	got, err := probe.convert("naïve body\n")
+	if err != nil || !strings.Contains(got, "naïve body") {
+		return nil
+	}
+
+	return candidate
 }
 
 func (l *Linter) lintRST(f *core.File) error {

internal/lint/rst_interp_test.go

@@ -0,0 +1,110 @@
+package lint
+
+import (
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/errata-ai/vale/v3/internal/system"
+)
+
+// write creates an executable script and returns its path.
+func write(t *testing.T, dir, name, body string) string {
+	t.Helper()
+
+	path := filepath.Join(dir, name)
+	if err := os.WriteFile(path, []byte(body), 0o755); err != nil { //nolint:gosec
+		t.Fatal(err)
+	}
+	return path
+}
+
+// The interpreter is read out of a shebang, which is a claim the file makes
+// about itself rather than something Vale can verify. Anything but Python has
+// to come back empty: the caller's next move is to hand it the Docutils server
+// source, and a shell reads that as a script. See #1137.
+func TestRSTInterpreterOnlyNamesPython(t *testing.T) {
+	dir := t.TempDir()
+
+	tests := []struct {
+		name    string
+		shebang string
+		python  bool
+	}{
+		{"pyenv shim", "#!/usr/bin/env bash\nset -e\nexec pyenv exec rst2html \"$@\"\n", false},
+		{"sh wrapper", "#!/bin/sh\nexec /usr/bin/rst2html \"$@\"\n", false},
+		{"env perl", "#!/usr/bin/env perl\n", false},
+		{"env with no argument", "#!/usr/bin/env\n", false},
+		{"no shebang", "not a script\n", false},
+		{"empty file", "", false},
+		{"absolute python", "#!/usr/bin/python3\n", true},
+		{"versioned python", "#!/usr/local/bin/python3.12\n", true},
+		{"pypy", "#!/usr/bin/pypy3\n", true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := rstInterpreter(write(t, dir, "rst2html", tt.shebang))
+
+			if !tt.python {
+				if got != "" {
+					t.Errorf("rstInterpreter = %q; want \"\"", got)
+				}
+				return
+			}
+			if !rePython.MatchString(filepath.Base(got)) {
+				t.Errorf("rstInterpreter = %q; want a Python", got)
+			}
+		})
+	}
+}
+
+// `#!/usr/bin/env python3` names the interpreter in the second field, and that
+// field is a bare name -- it has to be resolved on PATH, not returned as-is.
+func TestRSTInterpreterResolvesEnvPython(t *testing.T) {
+	if system.Which([]string{"python3", "python"}) == "" {
+		t.Skip("no python on PATH")
+	}
+
+	got := rstInterpreter(write(t, t.TempDir(), "rst2html", "#!/usr/bin/env python3\n"))
+	if got != "" && !filepath.IsAbs(got) {
+		t.Errorf("rstInterpreter = %q; want an absolute path", got)
+	}
+}
+
+// The failure in #1137 was not a wrong answer -- the probe caught that and Vale
+// fell back to spawning rst2html per file, so the linting was correct. It was
+// what running the probe did on the way: `bash -c` on the Docutils source runs
+// its first line, `import sys`, as a shell command, and `import` is
+// ImageMagick's screenshot tool.
+//
+// So this asserts on the side effect, not the return value. `import` here
+// records that it ran.
+func TestRSTProbeRunsNothingThroughAShell(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("no shebang handling on Windows")
+	} else if system.Which([]string{"bash"}) == "" {
+		t.Skip("bash not installed")
+	}
+
+	dir := t.TempDir()
+	sentinel := filepath.Join(dir, "ran")
+
+	// A wrapper of the shape a version manager installs: a Bourne-Again shell
+	// script under the name of the tool it forwards to.
+	shim := write(t, dir, "rst2html", "#!/usr/bin/env bash\nset -e\nexit 0\n")
+
+	// Every word on the source's first line is a command a shell would try.
+	for _, name := range []string{"import", "sys"} {
+		write(t, dir, name, "#!/bin/sh\necho ran > "+sentinel+"\n")
+	}
+
+	t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	rstProbe(shim)
+
+	if _, err := os.Stat(sentinel); err == nil {
+		t.Error("the Docutils source was executed by a shell")
+	}
+}