Directly exec ExternalTokenHelper rather than using a SHELL (#29653)

* [OT] use `new` builtin for visual clarity

`new(ExternalTokenHelper)` is a lot easier to parse than
`(*ExternalTokenHelper)(nil)`

* add `Args` field to `ExternalTokenHelper`

This will be used to store any extra command arguments and allows
`BinaryPath` to hold *just* the binary path.

* remove shell invocation

Since `BinPath` no longer has to hold any additional arguments we can
execute the command directly without inoking the shell first.

* update `testExternalTokenHelper` to make use of the new `Args` field

* updated `ExternalTokenHelper` documentation

* Add changelog entry for token_helper without shell

Currently using 0.txt until we have a PR id.

* Rename 0.txt to 29653.txt

We got a PR ID, so fix the changelog file

---------

Co-authored-by: Roosevelt Burden <rburden@grantstreet.com>
Co-authored-by: Roosevelt Burden <roosevelt.burden@grantstreet.com>

api/tokenhelper/helper_external.go

@@ -9,7 +9,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"runtime"
 	"strings"
 )
 
@@ -40,14 +39,14 @@ func ExternalTokenHelperPath(path string) (string, error) {
 	return path, nil
 }
 
-var _ TokenHelper = (*ExternalTokenHelper)(nil)
+var _ TokenHelper = new(ExternalTokenHelper)
 
 // ExternalTokenHelper should only be used in a dev mode. For all other cases,
 // InternalTokenHelper should be used.
 // ExternalTokenHelper is the struct that has all the logic for storing and retrieving
 // tokens from the token helper. The API for the helpers is simple: the
-// BinaryPath is executed within a shell with environment Env. The last argument
-// appended will be the operation, which is:
+// BinaryPath is executed directly with arguments Args and environment Env.
+// The last argument appended to Args will be the operation, which is:
 //
 //   - "get" - Read the value of the token and write it to stdout.
 //   - "store" - Store the value of the token which is on stdin. Output
@@ -58,6 +57,7 @@ var _ TokenHelper = (*ExternalTokenHelper)(nil)
 // exit code then the stderr will be made part of the error value.
 type ExternalTokenHelper struct {
 	BinaryPath string
+	Args       []string
 	Env        []string
 }
 
@@ -109,28 +109,13 @@ func (h *ExternalTokenHelper) Path() string {
 }
 
 func (h *ExternalTokenHelper) cmd(op string) (*exec.Cmd, error) {
-	script := strings.ReplaceAll(h.BinaryPath, "\\", "\\\\") + " " + op
-	cmd, err := execScript(script)
-	if err != nil {
-		return nil, err
-	}
+	binPath := strings.ReplaceAll(h.BinaryPath, "\\", "\\\\")
+
+	args := make([]string, len(h.Args))
+	copy(args, h.Args)
+	args = append(args, op)
+
+	cmd := exec.Command(binPath, args...)
 	cmd.Env = h.Env
 	return cmd, nil
 }
-
-// execScript returns a command to execute a script
-func execScript(script string) (*exec.Cmd, error) {
-	var shell, flag string
-	if runtime.GOOS == "windows" {
-		shell = "cmd"
-		flag = "/C"
-	} else {
-		shell = "/bin/sh"
-		flag = "-c"
-	}
-	if other := os.Getenv("SHELL"); other != "" {
-		shell = other
-	}
-	cmd := exec.Command(shell, flag, script)
-	return cmd, nil
-}

api/tokenhelper/helper_external_test.go

@@ -8,7 +8,6 @@ import (
 	"io"
 	"os"
 	"runtime"
-	"strings"
 	"testing"
 )
 
@@ -57,16 +56,8 @@ func TestExternalTokenHelper(t *testing.T) {
 }
 
 func testExternalTokenHelper() *ExternalTokenHelper {
-	return &ExternalTokenHelper{BinaryPath: helperPath("helper"), Env: helperEnv()}
-}
-
-func helperPath(s ...string) string {
-	cs := []string{"-test.run=TestExternalTokenHelperProcess", "--"}
-	cs = append(cs, s...)
-	return fmt.Sprintf(
-		"%s %s",
-		os.Args[0],
-		strings.Join(cs, " "))
+	args := []string{"-test.run=TestExternalTokenHelperProcess", "--", "helper"}
+	return &ExternalTokenHelper{BinaryPath: os.Args[0], Args: args, Env: helperEnv()}
 }
 
 func helperEnv() []string {

changelog/29653.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+api/tokenhelper: Exec token_helper without a shell
+```