make the handling of import paths more robust

First, make isPrivate panic on malformed import paths, since that should
never happen. This catches the errors that some users had run into with
packages like gopkg.in/yaml.v2 and github.com/satori/go.uuid:

	panic: malformed import path "gopkg.in/garbletest%2ev2": invalid char '%'

This seems to trigger when a module path contains a dot after the first
element, *and* that module is fetched via the proxy. This results in the
toolchain URL-encoding the second dot, and garble ends up seeing that
encoded path.

We reproduce this behavior with a fake gopkg.in module added to the test
module proxy. Using yaml.v2 directly would have been easier, but it's
pretty large. Note that we tried a replace directive, but that does not
trigger the URL-encoding bug.

Also note that we do not obfuscate the gopkg.in package; that's fine, as
the isPrivate path validity check catches the bug either way.

For now, make initImport use url.PathUnescape to work around this issue.
The underlying bug is likely in either the goobj2 fork, or in the
upstream Go toolchain itself.

hashImport also gives a better error if it cannot find a package now,
rather than just an "empty seed" panic.

Finally, the sanity check in isPrivate unearthed the fact that we do not
support garbling test packages at all, since they were invalid paths
which never matched GOPRIVATE. Add an explicit check and TODO about
that.

Fixes #224.
Fixes #228.

import_obfuscation.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net/url"
 	"os"
 	"path/filepath"
 	"sort"
@@ -203,15 +204,27 @@ func obfuscateImports(objPath string, importMap goobj2.ImportMap) (garbledObj st
 			}
 
 			initImport := func(imp string) string {
-				if !isPrivate(imp) {
+				// Due to an apparent bug in goobj2, in some rare
+				// edge cases like gopkg.in/yaml.v2 we end up with
+				// URL-escaped paths, like "gopkg.in/yaml%2ev2".
+				// Unescape them to use isPrivate and hashImport.
+				// TODO: Investigate and fix this in goobj2.
+				unesc, err := url.PathUnescape(imp)
+				if err != nil {
+					panic(err)
+				}
+
+				if !isPrivate(unesc) {
+					// If the path isn't private, leave the
+					// original path as-is, even if escaped.
 					return imp
 				}
 
-				privPaths, privNames := explodeImportPath(imp)
+				privPaths, privNames := explodeImportPath(unesc)
 				privImports.privatePaths = append(privImports.privatePaths, privPaths...)
 				privImports.privateNames = append(privImports.privateNames, privNames...)
 
-				return hashImport(imp, nil)
+				return hashImport(unesc, nil)
 			}
 
 			for i := range am.Imports {
@@ -433,6 +446,16 @@ func hashImport(pkg string, seed []byte) string {
 			pkgPath = buildInfo.firstImport
 		}
 		seed = buildInfo.imports[pkgPath].actionID
+
+		// If we can't find a seed, provide a more helpful panic than hashWith's.
+		if len(seed) == 0 {
+			var imports []string
+			for path := range buildInfo.imports {
+				imports = append(imports, path)
+			}
+			sort.Strings(imports)
+			panic(fmt.Sprintf("could not find package path %s; imports:\n%s", pkgPath, strings.Join(imports, "\n")))
+		}
 	}
 
 	return hashWith(seed, pkg)

main.go

@@ -718,6 +718,18 @@ var runtimeRelated = map[string]bool{
 // To allow using garble without GOPRIVATE for standalone main packages, it will
 // default to not matching standard library packages.
 func isPrivate(path string) bool {
+	// isPrivate is used in lots of places, so use it as a way to sanity
+	// check that none of our package paths are invalid.
+	// This can happen if we end up with an escaped or corrupted path.
+	// TODO: Do we want to support obfuscating test packages?
+	// It is a bit tricky as their import paths are confusing, such as
+	// "test/bar.test" and "test/bar [test/bar.test]".
+	if strings.HasSuffix(path, ".test") || strings.HasSuffix(path, ".test]") {
+		return false
+	}
+	if err := module.CheckImportPath(path); err != nil {
+		panic(err)
+	}
 	if runtimeRelated[path] {
 		return false
 	}

testdata/mod/gopkg.in_garbletest.v2_v2.999.0.txt

@@ -0,0 +1,12 @@
+module gopkg.in/garbletest.v2@v2.999.0
+
+-- .mod --
+module gopkg.in/garbletest.v2
+
+go 1.15
+-- .info --
+{"Version":"v2.999.0","Time":"2020-11-17T15:46:20Z"}
+-- apic.go --
+package garbletest
+
+func Test() { println("this is the dummy garbletest.v2 package") }

testdata/scripts/imports.txt

@@ -47,10 +47,15 @@ module test/main
 
 go 1.15
 
-require rsc.io/quote v1.5.2
+require (
+	rsc.io/quote v1.5.2
+	gopkg.in/garbletest.v2 v2.999.0
+)
 -- go.sum --
 golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c h1:pvCbr/wm8HzDD3fVywevekufpn6tCGPY3spdHeZJEsw=
 golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
+gopkg.in/garbletest.v2 v2.999.0 h1:wiZfOKGiXX7DoYVgbNvnTaCjqElrpZQSvKg0HYouw/o=
+gopkg.in/garbletest.v2 v2.999.0/go.mod h1:MF1BPTBjmDdc9x86+9UMLL9pAH2eMFPHvltohOvlGEw=
 rsc.io/quote v1.5.2 h1:3fEykkD9k7lYzXqCYrwGAf7iNhbk4yCjHmKBN9td4L0=
 rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0=
 rsc.io/sampler v1.3.0 h1:HLGR/BgEtI3r0uymSP/nl2uPLsUnNJX8toRyhfpBTII=
@@ -66,6 +71,7 @@ import (
 	"test/main/imported"
 
 	"rsc.io/quote"
+	garbletest "gopkg.in/garbletest.v2"
 )
 
 func main() {
@@ -91,6 +97,7 @@ func main() {
 	}
 
 	fmt.Println(quote.Go())
+	garbletest.Test()
 }
 
 func printfWithoutPackage(format string, v interface{}) {

testdata/scripts/syntax.txt

@@ -1,4 +1,4 @@
-env GOPRIVATE='test/main,private.source/*'
+env GOPRIVATE='test/main,private.source'
 
 garble build
 exec ./main$exe
@@ -17,7 +17,7 @@ binsubstr main$exe 'globalVar' # 'globalType' only matches on go < 1.15
 ! binsubstr main$exe 'localName' 'globalConst' 'remoteIntReturn' 'intReturn'
 
 -- extra/go.mod --
-module "private.source/extra"
+module private.source/extra
 
 go 1.15
 -- extra/extra.go --