go/vcs: fix command injection in VCS path

Apply same change as CL 94656 did for cmd/go/internal/get, but for golang.org/x/tools/go/vcs, to help keep them in sync. It indirectly includes changes from CL 94603, since CL 94656 was rebased on top of CL 94603. Updates golang/go#23867. Helps golang/go#11490. Change-Id: I33eca1aba19f47bbe3e83d4ef9f9cc9a9c9ae975 Reviewed-on: https://go-review.googlesource.com/94899 Reviewed-by: Ian Lance Taylor <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> TryBot-Result: Gobot Gobot <[email protected]>
golang · Feb 21, 2018 · e8fdd20 · e8fdd20
1 parent db9df82
commit e8fdd20
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 2 deletions.
diff --git a/go/vcs/vcs.go b/go/vcs/vcs.go
@@ -20,6 +20,7 @@ import (
 	"errors"
 	"fmt"
 	"log"
+	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -566,8 +567,8 @@ func RepoRootForImportDynamic(importPath string, verbose bool) (*RepoRoot, error
 		}
 	}
 
-	if !strings.Contains(metaImport.RepoRoot, "://") {
-		return nil, fmt.Errorf("%s: invalid repo root %q; no scheme", urlStr, metaImport.RepoRoot)
+	if err := validateRepoRoot(metaImport.RepoRoot); err != nil {
+		return nil, fmt.Errorf("%s: invalid repo root %q: %v", urlStr, metaImport.RepoRoot, err)
 	}
 	rr := &RepoRoot{
 		VCS:  ByCmd(metaImport.VCS),
@@ -580,6 +581,19 @@ func RepoRootForImportDynamic(importPath string, verbose bool) (*RepoRoot, error
 	return rr, nil
 }
 
+// validateRepoRoot returns an error if repoRoot does not seem to be
+// a valid URL with scheme.
+func validateRepoRoot(repoRoot string) error {
+	url, err := url.Parse(repoRoot)
+	if err != nil {
+		return err
+	}
+	if url.Scheme == "" {
+		return errors.New("no scheme")
+	}
+	return nil
+}
+
 // metaImport represents the parsed <meta name="go-import"
 // content="prefix vcs reporoot" /> tags from HTML files.
 type metaImport struct {

diff --git a/go/vcs/vcs_test.go b/go/vcs/vcs_test.go
@@ -140,3 +140,47 @@ func TestParseMetaGoImports(t *testing.T) {
 		}
 	}
 }
+
+func TestValidateRepoRoot(t *testing.T) {
+	tests := []struct {
+		root string
+		ok   bool
+	}{
+		{
+			root: "",
+			ok:   false,
+		},
+		{
+			root: "http://",
+			ok:   true,
+		},
+		{
+			root: "git+ssh://",
+			ok:   true,
+		},
+		{
+			root: "http#://",
+			ok:   false,
+		},
+		{
+			root: "-config",
+			ok:   false,
+		},
+		{
+			root: "-config://",
+			ok:   false,
+		},
+	}
+
+	for _, test := range tests {
+		err := validateRepoRoot(test.root)
+		ok := err == nil
+		if ok != test.ok {
+			want := "error"
+			if test.ok {
+				want = "nil"
+			}
+			t.Errorf("validateRepoRoot(%q) = %q, want %s", test.root, err, want)
+		}
+	}
+}