Repository: google/go-flow-levee
Pull request #184 (2020-11-12 14:33:47)

Handle tagged fields in wrapped structs
This PR modifies the `fieldtags` analyzer to ensure that tagged fields in wrapped structs are found:
```go
type Wrapper struct {
    Wrapped struct {
        password string `levee:"source"`
    }
}
```

Tests are added to `sourceinfer` to make sure that wrapped structs are handled properly.

- [x] Tests pass
- [x] Running against a large codebase such as [Kubernetes](https://github.com/kubernetes/kubernetes) does not error out.
- (N/A) [ ] Appropriate changes to README are included in PR


Patch:
diff --git a/internal/pkg/fieldtags/analyzer.go b/internal/pkg/fieldtags/analyzer.go
index 0b23d7c..bad0f7b 100644
--- a/internal/pkg/fieldtags/analyzer.go
+++ b/internal/pkg/fieldtags/analyzer.go
@@ -53,15 +53,11 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	taggedFields := map[types.Object]bool{}
 
 	nodeFilter := []ast.Node{
-		(*ast.TypeSpec)(nil),
+		(*ast.StructType)(nil),
 	}
 
 	inspectResult.Preorder(nodeFilter, func(n ast.Node) {
-		t, ok := n.(*ast.TypeSpec)
-		if !ok {
-			return
-		}
-		s, ok := t.Type.(*ast.StructType)
+		s, ok := n.(*ast.StructType)
 		if !ok || s.Fields == nil {
 			return
 		}


Test patch:
diff --git a/internal/pkg/fieldtags/analyzer_test.go b/internal/pkg/fieldtags/analyzer_test.go
index 262ac5e..e70bc06 100644
--- a/internal/pkg/fieldtags/analyzer_test.go
+++ b/internal/pkg/fieldtags/analyzer_test.go
@@ -44,6 +44,7 @@ func TestFieldTagsAnalysis(t *testing.T) {
 		"another",
 		"hasCustomFieldTag",
 		"hasTagWithMultipleValues",
+		"adminSecret",
 	}
 
 	var got []string
diff --git a/internal/pkg/fieldtags/testdata/src/tests/test.go b/internal/pkg/fieldtags/testdata/src/tests/test.go
index 511ff3f..4e18ccc 100644
--- a/internal/pkg/fieldtags/testdata/src/tests/test.go
+++ b/internal/pkg/fieldtags/testdata/src/tests/test.go
@@ -24,3 +24,9 @@ type Person struct {
 	spaceAfterFinalQuote     string      `key:"value" `
 	someNotTaggedField       int
 }
+
+type Nester struct {
+	Nested struct {
+		adminSecret string `levee:"source"` // want "tagged field: adminSecret"
+	}
+}
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/collections/arrays.go b/internal/pkg/levee/testdata/src/example.com/tests/collections/arrays.go
index b9e897e..eb0fbf0 100644
--- a/internal/pkg/levee/testdata/src/example.com/tests/collections/arrays.go
+++ b/internal/pkg/levee/testdata/src/example.com/tests/collections/arrays.go
@@ -43,7 +43,9 @@ func TestArrayRemainsTaintedWhenSourceIsOverwritten(s core.Source) {
 
 func TestRangeOverArray() {
 	sources := [1]core.Source{core.Source{Data: "password1234"}}
-	for _, s := range sources {
+	for i, s := range sources {
 		core.Sink(s) // want "a source has reached a sink"
+		// TODO want no diagnostic reported for string value
+		core.Sink(i) // want "a source has reached a sink"
 	}
 }
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/collections/maps.go b/internal/pkg/levee/testdata/src/example.com/tests/collections/maps.go
index 2b33f98..b3a2de4 100644
--- a/internal/pkg/levee/testdata/src/example.com/tests/collections/maps.go
+++ b/internal/pkg/levee/testdata/src/example.com/tests/collections/maps.go
@@ -71,9 +71,20 @@ func TestMapUpdateWithTaintedKeyDoesNotTaintTheValue(key core.Source, value stri
 	core.Sink(value) // want "a source has reached a sink"
 }
 
-func TestRangeOverMap() {
+func TestRangeOverMapWithSourceAsValue() {
 	m := map[string]core.Source{"secret": core.Source{Data: "password1234"}}
-	for _, s := range m {
+	for k, s := range m {
 		core.Sink(s) // want "a source has reached a sink"
+		// TODO want no diagnostic reported for string key
+		core.Sink(k) // want "a source has reached a sink"
+	}
+}
+
+func TestRangeOverMapWithSourceAsKey() {
+	m := map[core.Source]string{core.Source{Data: "password1234"}: "don't sink me"}
+	for src, str := range m {
+		core.Sink(src) // want "a source has reached a sink"
+		// TODO want no diagnostic reported for string value
+		core.Sink(str) // want "a source has reached a sink"
 	}
 }
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/collections/slices.go b/internal/pkg/levee/testdata/src/example.com/tests/collections/slices.go
index 7c6594d..ec63ebf 100644
--- a/internal/pkg/levee/testdata/src/example.com/tests/collections/slices.go
+++ b/internal/pkg/levee/testdata/src/example.com/tests/collections/slices.go
@@ -29,13 +29,17 @@ func TestSlices(s core.Source) {
 
 func TestRangeOverSlice() {
 	sources := []core.Source{core.Source{Data: "password1234"}}
-	for _, s := range sources {
+	for i, s := range sources {
 		core.Sink(s) // want "a source has reached a sink"
+		// TODO want no diagnostic reported for string value
+		core.Sink(i) // want "a source has reached a sink"
 	}
 }
 
 func TestRangeOverInterfaceSlice() {
-	for _, s := range []interface{}{core.Source{Data: "password1235"}} {
+	for i, s := range []interface{}{core.Source{Data: "password1235"}} {
 		core.Sink(s) // want "a source has reached a sink"
+		// TODO want no diagnostic reported for string value
+		core.Sink(i) // want "a source has reached a sink"
 	}
 }
diff --git a/internal/pkg/sourceinfer/testdata/src/example.com/tests/core/field.go b/internal/pkg/sourceinfer/testdata/src/example.com/tests/core/field.go
index 7eaadab..e57231c 100644
--- a/internal/pkg/sourceinfer/testdata/src/example.com/tests/core/field.go
+++ b/internal/pkg/sourceinfer/testdata/src/example.com/tests/core/field.go
@@ -48,6 +48,18 @@ type (
 		sh SourceHolder
 	}
 
+	SourceWrapperHolder struct { // want SourceWrapperHolder:"inferred source"
+		Wrapped struct {
+			s source.Source
+		}
+	}
+
+	TaggedWrapperHolder struct { // want TaggedWrapperHolder:"inferred source"
+		Wrapped struct {
+			s string `levee:"source"`
+		}
+	}
+
 	TaggedHolder struct { // want TaggedHolder:"inferred source"
 		t source.Tagged
 	}