Repository: google/go-flow-levee (go)

Issue:
Make identifying field tags configurable
Currently, we only examine for the tag `levee:"source"`.  The source tag should allow this to be user-configurable.

Pull request:
Make field tags configurable
Closes #115.

- [x] Tests pass
- [x] Appropriate changes to README are included in PR

Patch:
diff --git a/configuration/README.md b/configuration/README.md
index 68f05f8..8b4b0ca 100644
--- a/configuration/README.md
+++ b/configuration/README.md
@@ -28,6 +28,25 @@ Sources are identified via regexp according to package, type, and field names.
 }
 ```
 
+Sources may also be identified via field tags:
+```go
+type Example struct {
+	fieldName fieldType `levee:"source"` // this field will be considered a Source
+}
+```
+
+The tag `levee:"source"` is built-in. Additional tags may be identified via explicit string literals (not regexps). The following example shows how the `levee:"source"` tag could be defined if it weren't built-in:
+```json
+{
+	"FieldTags": [
+		{
+			"Key": "levee",
+			"Val": "source"
+		}
+	]
+}
+```
+
 Sinks and sanitizers are identified via regexp according to package, method, and (optional) receiver name.
 
 ```json
diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go
index 561be51..7062634 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -41,9 +41,35 @@ type Config struct {
 	Sources    []sourceMatcher
 	Sinks      []funcMatcher
 	Sanitizers []funcMatcher
+	FieldTags  []fieldTagMatcher
 	Exclude    []pathMatcher
 }
 
+type fieldTagMatcher struct {
+	Key string
+	Val string
+}
+
+func (ftm fieldTagMatcher) matches(key, val string) bool {
+	return ftm.Key == key && ftm.Val == val
+}
+
+// IsSourceFieldTag determines whether a field tag made up of a key and value
+// is a Source.
+func (c Config) IsSourceFieldTag(key, val string) bool {
+	// built in
+	if key == "levee" && val == "source" {
+		return true
+	}
+	// configured
+	for _, ft := range c.FieldTags {
+		if ft.matches(key, val) {
+			return true
+		}
+	}
+	return false
+}
+
 type pathMatcher struct {
 	PathRE regexp.Regexp
 }
diff --git a/internal/pkg/fieldtags/analyzer.go b/internal/pkg/fieldtags/analyzer.go
index bc303a0..77f4a8e 100644
--- a/internal/pkg/fieldtags/analyzer.go
+++ b/internal/pkg/fieldtags/analyzer.go
@@ -22,23 +22,13 @@ import (
 	"reflect"
 	"strings"
 
+	"github.com/google/go-flow-levee/internal/pkg/config"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/go/ssa"
 )
 
-type keyValue struct {
-	key   string
-	value string
-}
-
-type sourcePatterns []keyValue
-
-var patterns sourcePatterns = []keyValue{
-	{"levee", "source"},
-}
-
 // 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
@@ -53,7 +43,17 @@ var Analyzer = &analysis.Analyzer{
 	ResultType: reflect.TypeOf(new(ResultType)).Elem(),
 }
 
+type fieldTag struct {
+	key string
+	val string
+}
+
 func run(pass *analysis.Pass) (interface{}, error) {
+	conf, err := config.ReadConfig()
+	if err != nil {
+		return nil, err
+	}
+
 	inspectResult := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 	taggedFields := map[types.Object]bool{}
 
@@ -71,73 +71,81 @@ func run(pass *analysis.Pass) (interface{}, error) {
 			return
 		}
 		for _, f := range (*s).Fields.List {
-			if patterns.isSource(f) && len(f.Names) > 0 {
-				fNames := make([]string, len(f.Names))
-				for i, ident := range f.Names {
-					fNames[i] = ident.Name
-					taggedFields[pass.TypesInfo.ObjectOf(ident)] = true
+			tags := extractTags(f)
+			isTaggedField := false
+			for _, ft := range tags {
+				if conf.IsSourceFieldTag(ft.key, ft.val) {
+					isTaggedField = true
 				}
-				pass.Reportf(f.Pos(), "tagged field: %s", strings.Join(fNames, ", "))
 			}
+			if !isTaggedField || len(f.Names) == 0 {
+				continue
+			}
+			fNames := make([]string, len(f.Names))
+			for i, ident := range f.Names {
+				fNames[i] = ident.Name
+				taggedFields[pass.TypesInfo.ObjectOf(ident)] = true
+			}
+			pass.Reportf(f.Pos(), "tagged field: %s", strings.Join(fNames, ", "))
 		}
 	})
 
 	return ResultType(taggedFields), nil
 }
 
-func (sp *sourcePatterns) isSource(field *ast.Field) bool {
+func extractTags(field *ast.Field) (tags []fieldTag) {
 	if field.Tag == nil {
-		return false
+		return
 	}
 
-	tag := field.Tag.Value
+	wholeTag := field.Tag.Value
 
 	// TODO: consider refactoring this logic into a regex matcher
 	i := 1 // skip initial quote
 	j := 1
-	for j < len(tag) {
-		for j < len(tag) && tag[j] != ':' {
+	for j < len(wholeTag) {
+		for j < len(wholeTag) && wholeTag[j] != ':' {
 			j++
 		}
-		key := tag[i:j]
+		key := wholeTag[i:j]
 		if key == "" {
-			return false
+			return
 		}
 
 		i = j + 1 // skip colon
-		if i >= len(tag) {
-			return false
+		if i >= len(wholeTag) {
+			return
 		}
-		if tag[i] == '\\' {
+		if wholeTag[i] == '\\' {
 			i++
 		}
 		i++ // skip quote
 
 		j = i
-		for j < len(tag) && tag[j] != '"' {
+		for j < len(wholeTag) && wholeTag[j] != '"' {
 			// skip escape character
-			if tag[j] == '\\' {
+			if wholeTag[j] == '\\' {
 				j++
 			}
 			j++
 		}
-		value := tag[i:j]
+		value := wholeTag[i:j]
+		// remove trailing escaped quote if present
+		value = strings.TrimSuffix(value, `\"`)
 		if value == "" {
-			return false
+			return
 		}
 
-		for _, p := range *sp {
-			// value may be a list of comma separated values
-			if p.key == key && strings.Contains(value, p.value) {
-				return true
-			}
+		// value may be a list of comma separated values
+		for _, v := range strings.Split(value, ",") {
+			tags = append(tags, fieldTag{key: key, val: v})
 		}
 
 		i = j + 2 // skip closing quote and space
 		j = i
 	}
 
-	return false
+	return tags
 }
 
 // IsSourceFieldAddr determines whether a ssa.FieldAddr is a source, that is whether it refers to a field previously identified as a source.


Test patch:
diff --git a/internal/pkg/config/fieldtags_test.go b/internal/pkg/config/fieldtags_test.go
new file mode 100644
index 0000000..f1b73ad
--- /dev/null
+++ b/internal/pkg/config/fieldtags_test.go
@@ -0,0 +1,71 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config
+
+import (
+	"testing"
+)
+
+func TestFieldTagsIdentification(t *testing.T) {
+	if err := FlagSet.Set("config", "testdata/test-config.json"); err != nil {
+		t.Error(err)
+	}
+
+	config, err := ReadConfig()
+	if err != nil {
+		t.Error(err)
+	}
+
+	cases := []struct {
+		desc string
+		key  string
+		val  string
+		want bool
+	}{
+		{
+			"built-in field tag",
+			"levee",
+			"source",
+			true,
+		},
+		{
+			"custom field tag",
+			"example",
+			"sensitive",
+			true,
+		},
+		{
+			"different tag key",
+			"notexample",
+			"sensitive",
+			false,
+		},
+		{
+			"different tag value",
+			"example",
+			"notsensitive",
+			false,
+		},
+	}
+
+	for _, tt := range cases {
+		t.Run(tt.desc, func(t *testing.T) {
+			got := config.IsSourceFieldTag(tt.key, tt.val)
+			if got != tt.want {
+				t.Errorf("config.IsSourceFieldTag(%q, %q) == %v, want %v", tt.key, tt.val, got, tt.want)
+			}
+		})
+	}
+}
diff --git a/internal/pkg/config/testdata/test-config.json b/internal/pkg/config/testdata/test-config.json
index 418bddc..6929d63 100644
--- a/internal/pkg/config/testdata/test-config.json
+++ b/internal/pkg/config/testdata/test-config.json
@@ -10,7 +10,13 @@
       "ReceiverRE": "^Sinker$"
     }
   ],
-    "Exclude": [
+  "FieldTags": [
+  	{
+  		"Key": "example",
+  		"Val": "sensitive"
+  	}
+  ],
+  "Exclude": [
     {
       "PathRE": "^example.com/exclusion.Foo$"
     },
diff --git a/internal/pkg/fieldtags/analyzer_test.go b/internal/pkg/fieldtags/analyzer_test.go
index 2885548..598f353 100644
--- a/internal/pkg/fieldtags/analyzer_test.go
+++ b/internal/pkg/fieldtags/analyzer_test.go
@@ -15,16 +15,22 @@
 package fieldtags
 
 import (
+	"path/filepath"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/google/go-flow-levee/internal/pkg/config"
 	"golang.org/x/tools/go/analysis/analysistest"
 )
 
 func TestFieldTagsAnalysis(t *testing.T) {
 	testdata := analysistest.TestData()
 
+	if err := config.FlagSet.Set("config", filepath.Join(testdata, "test-config.json")); err != nil {
+		t.Error(err)
+	}
+
 	results := analysistest.Run(t, testdata, Analyzer, "tests")
 
 	if len(results) != 1 {
@@ -36,6 +42,8 @@ func TestFieldTagsAnalysis(t *testing.T) {
 		"creds",
 		"secret",
 		"another",
+		"hasCustomFieldTag",
+		"hasTagWithMultipleValues",
 	}
 
 	var got []string
diff --git a/internal/pkg/fieldtags/testdata/src/tests/test.go b/internal/pkg/fieldtags/testdata/src/tests/test.go
index 9974403..4e244df 100644
--- a/internal/pkg/fieldtags/testdata/src/tests/test.go
+++ b/internal/pkg/fieldtags/testdata/src/tests/test.go
@@ -15,10 +15,12 @@
 package fieldtags
 
 type Person struct {
-	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
+	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"
+	hasCustomFieldTag        string      `example:"sensitive"`          // want "tagged field: hasCustomFieldTag"
+	hasTagWithMultipleValues string      `levee:"value0,source,value2"` // want "tagged field: hasTagWithMultipleValues"
+	name                     string      `some_key:"non_secret"`
+	spaceAfterFinalQuote     string      `key:"value" `
+	someNotTaggedField       int
 }
diff --git a/internal/pkg/fieldtags/testdata/test-config.json b/internal/pkg/fieldtags/testdata/test-config.json
new file mode 100644
index 0000000..71eb141
--- /dev/null
+++ b/internal/pkg/fieldtags/testdata/test-config.json
@@ -0,0 +1,8 @@
+{
+	"FieldTags": [
+		{
+			"Key": "example",
+			"Val": "sensitive"
+		}
+	]
+}