Repository: google/go-flow-levee
Pull request #70 (2020-08-27 19:18:50)

Prepare fieldtags analyzer for integration with sourcetype analyzer
This PR is a precursor to #71.

This PR prepares integration with the `sourcetype` analyzer by modifying the `fieldtags` analyzer to work with `types.Object`s. This is useful because the `sourcetype` analyzer works with `types.Var`s, which are `types.Object`s.

Working with `types.Object`s also makes the `fieldtags` analyzer more robust. Currently, it is working with strings, and in fact it contains a bug: it uses `Pkg.Name()` instead of `Pkg.Path()` when recording the string that represents a field's package. Using `types.Object`s makes the code simpler and more likely to be correct.

- [x] Tests pass
- [x] Appropriate changes to README are included in PR


Patch:
diff --git a/go.mod b/go.mod
index 34f01d8..787f951 100644
--- a/go.mod
+++ b/go.mod
@@ -2,4 +2,7 @@ module github.com/google/go-flow-levee
 
 go 1.14
 
-require golang.org/x/tools v0.0.0-20200416214402-fc959738d646
+require (
+	github.com/google/go-cmp v0.5.2
+	golang.org/x/tools v0.0.0-20200416214402-fc959738d646
+)
diff --git a/go.sum b/go.sum
index f91eab3..4d08442 100644
--- a/go.sum
+++ b/go.sum
@@ -1,3 +1,5 @@
+github.com/google/go-cmp v0.5.2 h1:X2ev0eStA3AbceY54o37/0PQ/UWqKEiiO2dKL5OPaFM=
+github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
diff --git a/internal/pkg/fieldpropagator/analyzer.go b/internal/pkg/fieldpropagator/analyzer.go
index c5f25d7..0806fbb 100644
--- a/internal/pkg/fieldpropagator/analyzer.go
+++ b/internal/pkg/fieldpropagator/analyzer.go
@@ -79,7 +79,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	return FieldPropagators(isFieldPropagator), nil
 }
 
-func analyzeBlocks(pass *analysis.Pass, conf *config.Config, tf fieldtags.TaggedFields, meth *ssa.Function) {
+func analyzeBlocks(pass *analysis.Pass, conf *config.Config, tf fieldtags.ResultType, meth *ssa.Function) {
 	// Function does not return anything
 	if res := meth.Signature.Results(); res == nil || (*res).Len() == 0 {
 		return
@@ -97,13 +97,13 @@ func analyzeBlocks(pass *analysis.Pass, conf *config.Config, tf fieldtags.Tagged
 	}
 }
 
-func analyzeResults(pass *analysis.Pass, conf *config.Config, tf fieldtags.TaggedFields, meth *ssa.Function, results []ssa.Value) {
+func analyzeResults(pass *analysis.Pass, conf *config.Config, tf fieldtags.ResultType, meth *ssa.Function, results []ssa.Value) {
 	for _, r := range results {
 		fa, ok := fieldAddr(r)
 		if !ok {
 			continue
 		}
-		if conf.IsSourceFieldAddr(fa) || tf.IsSource(fa) {
+		if conf.IsSourceFieldAddr(fa) || tf.IsSourceFieldAddr(fa) {
 			pass.ExportObjectFact(meth.Object(), &isFieldPropagator{})
 		}
 	}
diff --git a/internal/pkg/fieldtags/analyzer.go b/internal/pkg/fieldtags/analyzer.go
index 93bac7a..bc303a0 100644
--- a/internal/pkg/fieldtags/analyzer.go
+++ b/internal/pkg/fieldtags/analyzer.go
@@ -17,14 +17,11 @@
 package fieldtags
 
 import (
-	"fmt"
 	"go/ast"
 	"go/types"
 	"reflect"
 	"strings"
 
-	"github.com/google/go-flow-levee/internal/pkg/utils"
-
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
@@ -42,16 +39,9 @@ var patterns sourcePatterns = []keyValue{
 	{"levee", "source"},
 }
 
-// TaggedFields is named type representing a slice of TaggedFields.
-type TaggedFields []TaggedField
-
-// ResultType is a slice of TaggedFields.
-type ResultType = TaggedFields
-
-// A TaggedField contains the necessary information to identify a tagged struct field.
-type TaggedField struct {
-	pkg, typ, field string
-}
+// ResultType is a map from types.Object to bool.
+// It can be used to determine whether a field is a tagged Source field.
+type ResultType map[types.Object]bool
 
 var Analyzer = &analysis.Analyzer{
 	Name: "fieldtags",
@@ -65,7 +55,7 @@ var Analyzer = &analysis.Analyzer{
 
 func run(pass *analysis.Pass) (interface{}, error) {
 	inspectResult := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	var taggedFields TaggedFields
+	taggedFields := map[types.Object]bool{}
 
 	nodeFilter := []ast.Node{
 		(*ast.TypeSpec)(nil),
@@ -82,17 +72,17 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		}
 		for _, f := range (*s).Fields.List {
 			if patterns.isSource(f) && len(f.Names) > 0 {
-				tf := TaggedField{
-					pkg:   pass.Pkg.Name(),
-					typ:   t.Name.Name,
-					field: f.Names[0].Name,
+				fNames := make([]string, len(f.Names))
+				for i, ident := range f.Names {
+					fNames[i] = ident.Name
+					taggedFields[pass.TypesInfo.ObjectOf(ident)] = true
 				}
-				taggedFields = append(taggedFields, tf)
-				pass.Reportf(f.Pos(), "tagged field: %s", tf)
+				pass.Reportf(f.Pos(), "tagged field: %s", strings.Join(fNames, ", "))
 			}
 		}
 	})
-	return taggedFields, nil
+
+	return ResultType(taggedFields), nil
 }
 
 func (sp *sourcePatterns) isSource(field *ast.Field) bool {
@@ -150,32 +140,14 @@ func (sp *sourcePatterns) isSource(field *ast.Field) bool {
 	return false
 }
 
-// IsSource determines whether a FieldAddr is a source, that is whether it refers to a field previously identified as a source.
-func (t TaggedFields) IsSource(f *ssa.FieldAddr) bool {
-	n, ok := namedType(f)
-	if !ok {
-		return false
-	}
-	fName := utils.FieldName(f)
-	for _, tf := range t {
-		if tf.matches(n.String(), fName) {
-			return true
-		}
-	}
-	return false
-}
-
-func namedType(f *ssa.FieldAddr) (*types.Named, bool) {
-	structType := f.X.Type()
-	deref := utils.Dereference(structType)
-	named, ok := deref.(*types.Named)
-	return named, ok
-}
-
-func (t TaggedField) matches(typeName, fieldName string) bool {
-	return fmt.Sprintf("%s.%s", t.pkg, t.typ) == typeName && t.field == fieldName
+// IsSourceFieldAddr determines whether a ssa.FieldAddr is a source, that is whether it refers to a field previously identified as a source.
+func (r ResultType) IsSourceFieldAddr(fa *ssa.FieldAddr) bool {
+	// incantation plundered from the docstring for ssa.FieldAddr.Field
+	field := fa.X.Type().Underlying().(*types.Pointer).Elem().Underlying().(*types.Struct).Field(fa.Field)
+	return r.IsSource(field)
 }
 
-func (t TaggedField) String() string {
-	return fmt.Sprintf("%s.%s.%s", t.pkg, t.typ, t.field)
+// IsSource determines whether a types.Var is a source, that is whether it refers to a field previously identified as a source.
+func (r ResultType) IsSource(field *types.Var) bool {
+	return r[(types.Object)(field)]
 }
diff --git a/internal/pkg/source/source.go b/internal/pkg/source/source.go
index f5dff48..d33c7f9 100644
--- a/internal/pkg/source/source.go
+++ b/internal/pkg/source/source.go
@@ -106,9 +106,10 @@ func (a *Source) visitReferrers(referrers *[]ssa.Instruction) {
 		case *ssa.Call:
 			// This is to avoid attaching calls where the source is the receiver, ex:
 			// core.Sinkf("Source id: %v", wrapper.Source.GetID())
-			if v.Call.Signature().Recv() != nil {
+			if recv := v.Call.Signature().Recv(); recv != nil && a.config.IsSource(utils.Dereference(recv.Type())) {
 				continue
 			}
+
 			if a.config.IsSanitizer(v) {
 				a.sanitizers = append(a.sanitizers, &sanitizer.Sanitizer{Call: v})
 			}


Test patch:
diff --git a/internal/pkg/fieldtags/analyzer_test.go b/internal/pkg/fieldtags/analyzer_test.go
index 40abfec..2885548 100644
--- a/internal/pkg/fieldtags/analyzer_test.go
+++ b/internal/pkg/fieldtags/analyzer_test.go
@@ -17,11 +17,33 @@ package fieldtags
 import (
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"golang.org/x/tools/go/analysis/analysistest"
 )
 
 func TestFieldTagsAnalysis(t *testing.T) {
 	testdata := analysistest.TestData()
 
-	analysistest.Run(t, testdata, Analyzer, "tests")
+	results := analysistest.Run(t, testdata, Analyzer, "tests")
+
+	if len(results) != 1 {
+		t.Fatalf("expected 1 result, got %d", len(results))
+	}
+
+	want := []string{
+		"password",
+		"creds",
+		"secret",
+		"another",
+	}
+
+	var got []string
+	for o := range results[0].Result.(ResultType) {
+		got = append(got, o.Name())
+	}
+
+	if diff := cmp.Diff(want, got, cmpopts.SortSlices(func(a, b string) bool { return a < b })); diff != "" {
+		t.Errorf("Tagged Fields diff (-want +got):\n%s", diff)
+	}
 }
diff --git a/internal/pkg/fieldtags/testdata/src/tests/test.go b/internal/pkg/fieldtags/testdata/src/tests/test.go
index f635d8e..9974403 100644
--- a/internal/pkg/fieldtags/testdata/src/tests/test.go
+++ b/internal/pkg/fieldtags/testdata/src/tests/test.go
@@ -15,9 +15,9 @@
 package fieldtags
 
 type Person struct {
-	password             string      `levee:"source"`               // want "tagged field: fieldtags.Person.password"
-	secret               string      `json:"secret" levee:"source"` // want "tagged field: fieldtags.Person.secret"
-	another              interface{} "levee:\"source\""             // want "tagged field: fieldtags.Person.another"
+	password, creds      string      `levee:"source"`               // want "tagged field: password, creds"
+	secret               string      `json:"secret" levee:"source"` // want "tagged field: secret"
+	another              interface{} "levee:\"source\""             // want "tagged field: another"
 	name                 string      `some_key:"non_secret"`
 	spaceAfterFinalQuote string      `key:"value" `
 	someNotTaggedField   int
diff --git a/internal/pkg/levee/testdata/src/example.com/core/sink.go b/internal/pkg/levee/testdata/src/example.com/core/sink.go
index a4d3b1b..e220f81 100644
--- a/internal/pkg/levee/testdata/src/example.com/core/sink.go
+++ b/internal/pkg/levee/testdata/src/example.com/core/sink.go
@@ -16,6 +16,10 @@ package core
 
 import "io"
 
+type Sinker struct{}
+
+func (s Sinker) Sink(args ...interface{}) {}
+
 func Sink(args ...interface{}) {}
 
 func Sinkf(format string, args ...interface{}) {}
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/sinks/tests.go b/internal/pkg/levee/testdata/src/example.com/tests/sinks/tests.go
index 18f30f1..fbef18e 100644
--- a/internal/pkg/levee/testdata/src/example.com/tests/sinks/tests.go
+++ b/internal/pkg/levee/testdata/src/example.com/tests/sinks/tests.go
@@ -21,6 +21,7 @@ import (
 )
 
 func TestSinks(s core.Source, writer io.Writer) {
+	core.Sinker{}.Sink(s)         // want "a source has reached a sink"
 	core.Sink(s)                  // want "a source has reached a sink"
 	core.Sinkf("a source: %v", s) // want "a source has reached a sink"
 	core.FSinkf(writer, s)        // want "a source has reached a sink"
@@ -28,6 +29,7 @@ func TestSinks(s core.Source, writer io.Writer) {
 }
 
 func TestSinksWithRef(s *core.Source, writer io.Writer) {
+	core.Sinker{}.Sink(s)         // want "a source has reached a sink"
 	core.Sink(s)                  // want "a source has reached a sink"
 	core.Sinkf("a source: %v", s) // want "a source has reached a sink"
 	core.FSinkf(writer, s)        // want "a source has reached a sink"
@@ -35,6 +37,7 @@ func TestSinksWithRef(s *core.Source, writer io.Writer) {
 }
 
 func TestSinksInnocuous(innoc core.Innocuous, writer io.Writer) {
+	core.Sinker{}.Sink(innoc)
 	core.Sink(innoc)
 	core.Sinkf("a source: %v", innoc)
 	core.FSinkf(writer, innoc)
@@ -45,6 +48,7 @@ func TestSinksInnocuous(innoc core.Innocuous, writer io.Writer) {
 }
 
 func TestSinksWithInnocuousRef(innoc *core.Innocuous, writer io.Writer) {
+	core.Sinker{}.Sink(innoc)
 	core.Sink(innoc)
 	core.Sinkf("a source: %v", innoc)
 	core.FSinkf(writer, innoc)