From 4e79bfc01a78a8a996fd511942c0228b7e78992b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Thu, 5 Nov 2020 16:34:25 +0000
Subject: [PATCH] warn when a public package imports a private package (#182)

That is, a package that is built without obfuscation imports an
obfuscated package. This will result in confusing compilation error
messages, because the importer can't find the exported names from the
imported package by their non-obfuscated names:

	> ! garble build ./importer
	[stderr]
	# test/main/importer
	importer/importer.go:5:9: undefined: imported.Name
	exit status 2

Instead, detect this bad input case and provide a nice error:

	public package "test/main/importer" can't depend on obfuscated package "test/main/imported" (matched via GOPRIVATE="test/main/imported")

For now, this is by design. It also makes little sense for a public
package to import an obfuscated package in general, because the public
package would have to leak details about the private package's API and
behavior.

While at it, fix a quirk where we thought the unsafe package could be
private. It can't be, because the runtime package is always public and
it imports the runtime package:

	public package "internal/bytealg" can't depend on obfuscated package "unsafe" (matched via GOPRIVATE="*")

Instead of trying to obfuscate "unsafe" and doing nothing, simply add it
to the neverPrivate list, which is also a better name than
"privateBlacklist" (for #169).

Fixes #164.

Co-authored-by: lu4p <lu4p@pm.me>
---
 main.go                        | 31 ++++++++++++++++++++++---------
 testdata/scripts/goprivate.txt | 20 +++++++++++++++++---
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/main.go b/main.go
index 0a943870..df9cc153 100644
--- a/main.go
+++ b/main.go
@@ -183,6 +183,9 @@ type listedPackage struct {
 	ImportPath string
 	Export     string
 	Deps       []string
+
+	// TODO(mvdan): reuse this field once TOOLEXEC_IMPORTPATH is used
+	private bool
 }
 
 func listPackage(path string) (*listedPackage, error) {
@@ -412,14 +415,25 @@ func mainErr(args []string) error {
 			return err
 		}
 		anyPrivate := false
-		for path := range listedPackages {
+		for path, pkg := range listedPackages {
 			if isPrivate(path) {
+				pkg.private = true
 				anyPrivate = true
-				break
 			}
 		}
 		if !anyPrivate {
-			return fmt.Errorf("GOPRIVATE %q does not match any packages to be built", envGoPrivate)
+			return fmt.Errorf("GOPRIVATE=%q does not match any packages to be built", envGoPrivate)
+		}
+		for path, pkg := range listedPackages {
+			if pkg.private {
+				continue
+			}
+			for _, depPath := range pkg.Deps {
+				if listedPackages[depPath].private {
+					return fmt.Errorf("public package %q can't depend on obfuscated package %q (matched via GOPRIVATE=%q)",
+						path, depPath, envGoPrivate)
+				}
+			}
 		}
 
 		execPath, err := os.Executable()
@@ -631,10 +645,6 @@ func transformCompile(args []string) ([]string, error) {
 	tf.existingNames = collectNames(files)
 	tf.buildBlacklist(files)
 
-	// unsafe.Pointer is a special type that doesn't exist as a plain Go
-	// type definition, so we can't change its name.
-	tf.blacklist[types.Unsafe.Scope().Lookup("Pointer")] = struct{}{}
-
 	if envGarbleLiterals {
 		// TODO: use transformer here?
 		files = literals.Obfuscate(files, tf.info, fset, tf.blacklist)
@@ -792,14 +802,17 @@ func transformCompile(args []string) ([]string, error) {
 	return append(flags, newPaths...), nil
 }
 
-const privateBlacklist = "runtime,internal/cpu,internal/bytealg"
+// neverPrivate is a core set of std packages which we cannot obfuscate.
+// At the moment, this is the runtime package and its (few) dependencies.
+// This might change in the future.
+const neverPrivate = "runtime,internal/cpu,internal/bytealg,unsafe"
 
 // isPrivate checks if GOPRIVATE matches path.
 //
 // To allow using garble without GOPRIVATE for standalone main packages, it will
 // default to not matching standard library packages.
 func isPrivate(path string) bool {
-	if module.MatchPrefixPatterns(privateBlacklist, path) {
+	if module.MatchPrefixPatterns(neverPrivate, path) {
 		return false
 	}
 	if path == "main" || path == "command-line-arguments" || strings.HasPrefix(path, "plugin/unnamed") {
diff --git a/testdata/scripts/goprivate.txt b/testdata/scripts/goprivate.txt
index e5e4cd6e..8dff35bf 100644
--- a/testdata/scripts/goprivate.txt
+++ b/testdata/scripts/goprivate.txt
@@ -1,11 +1,15 @@
 env GOPRIVATE=match-absolutely/nothing
-! garble build -o bin ./standalone
-stderr 'does not match any packages'
+! garble build -o=out ./standalone
+stderr '^GOPRIVATE="match-absolutely/nothing" does not match any packages to be built$'
+
+env GOPRIVATE=test/main/imported
+! garble build ./importer
+stderr '^public package "test/main/importer" can''t depend on obfuscated package "test/main/imported" \(matched via GOPRIVATE="test/main/imported"\)$'
 
 [short] stop
 
 env GOPRIVATE='*'
-garble build  -o bin ./standalone
+garble build -o=out ./standalone
 
 -- go.mod --
 module test/main
@@ -13,3 +17,13 @@ module test/main
 package main
 
 func main() {}
+-- importer/importer.go --
+package importer
+
+import "test/main/imported"
+
+var _ = imported.Name
+-- imported/imported.go --
+package imported
+
+var Name = "value"