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

Issue:
Enforce strict config unmarshalling
In order to minimize the risk of configuration errors, we would like the unmarshalling of the configuration to be strict. Specifically, it should disallow unknown fields in matchers. This avoids errors where a typo in a key name leads to an incorrect configuration. We don't want typos to lead to silent failures, e.g. a matcher is broken and the user thinks their code is safe when it is not.

There are already two test cases covering this behavior to some extent, in `config/matcher_test.go`:

```go
		{
			desc: "Unmarshaling is strict",
			yaml: `
Blahblah: foo
PackageRE: bar`,
			shouldErrorOnLoad: false, // TODO true
		},
```

(It is the same case, in the tests for two different matchers.)

Pull request:
Add validation to config fields.
Fixes #206 

Add validation for config yaml fields.
Notice that we have both upper case (e.g. "Key") and lower case (e.g. "key") in our unit tests and sample files. The validation will allow both.

- [x] Tests pass
- [x] Running against a large codebase such as [Kubernetes](https://github.com/kubernetes/kubernetes) does not error out.
- [ ] Appropriate changes to README are included in PR

Patch:
diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go
index 5e045a5..e564ba5 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -29,15 +29,20 @@ import (
 	"github.com/google/go-flow-levee/internal/pkg/config/regexp"
 )
 
-// FlagSet should be used by analyzers to reuse -config flag.
-var FlagSet flag.FlagSet
-var configFile string
+var (
+	// FlagSet should be used by analyzers to reuse -config flag.
+	FlagSet                    flag.FlagSet
+	configFile                 string
+	validFuncMatcherFields     []string = []string{"package", "packagere", "receiver", "receiverre", "method", "methodre"}
+	validSourceMatcherFields   []string = []string{"package", "packagere", "type", "typere", "field", "fieldre"}
+	validFieldTagMatcherFields []string = []string{"key", "val", "value"}
+)
 
 func init() {
 	FlagSet.StringVar(&configFile, "config", "config.yaml", "path to analysis configuration file")
 }
 
-// config contains matchers and analysis scope information
+// Config contains matchers and analysis scope information.
 type Config struct {
 	Sources    []sourceMatcher
 	Sinks      []funcMatcher
@@ -46,11 +51,6 @@ type Config struct {
 	Exclude    []funcMatcher
 }
 
-type fieldTagMatcher struct {
-	Key string
-	Val string
-}
-
 // IsSourceFieldTag determines whether a field tag made up of a key and value
 // is a Source.
 func (c Config) IsSourceFieldTag(tag string) bool {
@@ -85,6 +85,7 @@ func (c Config) IsExcluded(path, recv, name string) bool {
 	return false
 }
 
+// IsSink determines whether a function is a sink.
 func (c Config) IsSink(path, recv, name string) bool {
 	for _, p := range c.Sinks {
 		if p.MatchFunction(path, recv, name) {
@@ -94,6 +95,7 @@ func (c Config) IsSink(path, recv, name string) bool {
 	return false
 }
 
+// IsSanitizer determines whether a function is a sanitizer.
 func (c Config) IsSanitizer(path, recv, name string) bool {
 	for _, p := range c.Sanitizers {
 		if p.MatchFunction(path, recv, name) {
@@ -103,6 +105,7 @@ func (c Config) IsSanitizer(path, recv, name string) bool {
 	return false
 }
 
+// IsSourceType determines whether a type is a source to analyze.
 func (c Config) IsSourceType(path, name string) bool {
 	for _, p := range c.Sources {
 		if p.MatchType(path, name) {
@@ -112,6 +115,7 @@ func (c Config) IsSourceType(path, name string) bool {
 	return false
 }
 
+// IsSourceField determines whether a field contains secret.
 func (c Config) IsSourceField(path, typeName, fieldName string) bool {
 	for _, p := range c.Sources {
 		if p.MatchField(path, typeName, fieldName) {
@@ -137,7 +141,32 @@ func (vacuousMatcher) MatchString(string) bool {
 	return true
 }
 
-// Returns the first non-nil matcher.  If all are nil, returns a vacuousMatcher.
+type fieldTagMatcher struct {
+	Key string
+	Val string
+}
+
+type rawFieldTagMatcher struct {
+	Key string
+	Val string
+}
+
+func (ft *fieldTagMatcher) UnmarshalJSON(bytes []byte) error {
+	if err := validateFieldNames(&bytes, "fieldTagMatcher", validFieldTagMatcherFields); err != nil {
+		return err
+	}
+
+	raw := rawFieldTagMatcher{}
+	if err := json.Unmarshal(bytes, &raw); err != nil {
+		return err
+	}
+	ft.Key = raw.Key
+	ft.Val = raw.Val
+	return nil
+}
+
+// Returns the first non-nil matcher.
+// If all are nil, returns a vacuousMatcher.
 func matcherFrom(lm *literalMatcher, r *regexp.Regexp) stringMatcher {
 	switch {
 	case lm != nil:
@@ -169,20 +198,24 @@ type rawSourceMatcher struct {
 }
 
 func (s *sourceMatcher) UnmarshalJSON(bytes []byte) error {
+	if err := validateFieldNames(&bytes, "sourceMatcher", validSourceMatcherFields); err != nil {
+		return err
+	}
+
 	raw := rawSourceMatcher{}
 	if err := json.Unmarshal(bytes, &raw); err != nil {
 		return err
 	}
 
-	// validation: do not double-specify any attribute with literal and regexp
+	// Only one of literal and regexp field can be specified.
 	if raw.Package != nil && raw.PackageRE != nil {
-		return fmt.Errorf("expected only one of Package, PackageRE to be configured")
+		return fmt.Errorf("expected only one of Package, PackageRE in config definition for a source matcher")
 	}
 	if raw.Type != nil && raw.TypeRE != nil {
-		return fmt.Errorf("expected only one of Type, TypeRE to be configured")
+		return fmt.Errorf("expected only one of Type, TypeRE in config definition for a source matcher")
 	}
 	if raw.Field != nil && raw.FieldRE != nil {
-		return fmt.Errorf("expected only one of Field, FieldRE to be configured")
+		return fmt.Errorf("expected only one of Field, FieldRE in config definition for a source matcher")
 	}
 
 	*s = sourceMatcher{
@@ -218,20 +251,24 @@ type rawFuncMatcher struct {
 }
 
 func (fm *funcMatcher) UnmarshalJSON(bytes []byte) error {
+	if err := validateFieldNames(&bytes, "funcMatcher", validFuncMatcherFields); err != nil {
+		return err
+	}
+
 	raw := rawFuncMatcher{}
 	if err := json.Unmarshal(bytes, &raw); err != nil {
 		return err
 	}
 
-	// validation: do not double-specify any attribute with literal and regexp
+	// Only one of literal and regexp field can be specified.
 	if raw.Package != nil && raw.PackageRE != nil {
-		return fmt.Errorf("expected at most one of Package, PackageRE to be configured")
+		return fmt.Errorf("expected only one of Package, PackageRE in config definition for a function matcher")
 	}
 	if raw.Receiver != nil && raw.ReceiverRE != nil {
-		return fmt.Errorf("expected at most one of Receiver, ReceiverRE to be configured")
+		return fmt.Errorf("expected only one of Receiver, ReceiverRE in config definition for a function matcher")
 	}
 	if raw.Method != nil && raw.MethodRE != nil {
-		return fmt.Errorf("expected at most one of Method, MethodRE to be configured")
+		return fmt.Errorf("expected only one of Method, MethodRE in config definition for a function matcher")
 	}
 
 	*fm = funcMatcher{
@@ -250,6 +287,7 @@ var readFileOnce sync.Once
 var readConfigCached *Config
 var readConfigCachedErr error
 
+// ReadConfig reads, parses, and validates config file.
 func ReadConfig() (*Config, error) {
 	readFileOnce.Do(func() {
 		c := new(Config)
@@ -267,3 +305,22 @@ func ReadConfig() (*Config, error) {
 	})
 	return readConfigCached, readConfigCachedErr
 }
+
+func validateFieldNames(bytes *[]byte, matcherType string, validFields []string) error {
+	rawMap := make(map[string]interface{})
+	if err := json.Unmarshal(*bytes, &rawMap); err != nil {
+		return err
+	}
+	for label := range rawMap {
+		matched := false
+		for _, valid := range validFields {
+			if strings.EqualFold(label, valid) {
+				matched = true
+			}
+		}
+		if !matched {
+			return fmt.Errorf("%v is not a valid config field for %s, expect one of: %v", label, matcherType, validFields)
+		}
+	}
+	return nil
+}


Test patch:
diff --git a/internal/pkg/config/matcher_test.go b/internal/pkg/config/matcher_test.go
index e3b9c7c..a484b0c 100644
--- a/internal/pkg/config/matcher_test.go
+++ b/internal/pkg/config/matcher_test.go
@@ -21,50 +21,43 @@ import (
 	"sigs.k8s.io/yaml"
 )
 
-func TestFuncMatcherUnmarshaling(t *testing.T) {
+func TestFuncMatcherUnmarshalErrorCases(t *testing.T) {
 	testCases := []struct {
-		desc, yaml        string
-		shouldErrorOnLoad bool
+		desc, yaml string
 	}{
 		{
 			desc: "Unmarshaling is strict",
 			yaml: `
 Blahblah: foo
 PackageRE: bar`,
-			shouldErrorOnLoad: false, // TODO(#206): true
 		},
 		{
 			desc: "Malformed YAML errors gracefully",
 			yaml: `
 PackageRE: "No ending quote`,
-			shouldErrorOnLoad: true,
 		},
 		{
 			desc: "Malformed regexp errors gracefully",
 			yaml: `
 PackageRE: "(?:NoEndingParen"`,
-			shouldErrorOnLoad: true,
 		},
 		{
 			desc: "Do not permit both Package and PackageRE",
 			yaml: `
 Package: foo
 PackageRE: bar`,
-			shouldErrorOnLoad: true,
 		},
 		{
 			desc: "Do not permit both Receiver and ReceiverRE",
 			yaml: `
 Receiver: foo
 ReceiverRE: bar`,
-			shouldErrorOnLoad: true,
 		},
 		{
 			desc: "Do not permit both Field and FieldRE",
 			yaml: `
 Method: foo
 MethodRE: bar`,
-			shouldErrorOnLoad: true,
 		},
 	}
 
@@ -73,8 +66,8 @@ MethodRE: bar`,
 			fm := funcMatcher{}
 			err := yaml.UnmarshalStrict([]byte(tc.yaml), &fm)
 
-			if (err != nil) != tc.shouldErrorOnLoad {
-				t.Errorf("error expectation = %v, but got err=%v", tc.shouldErrorOnLoad, err)
+			if err == nil {
+				t.Error("got err = nil, want error")
 			}
 		})
 	}
@@ -144,60 +137,53 @@ Method: bar`,
 		t.Run(tc.desc, func(t *testing.T) {
 			fm := funcMatcher{}
 			if err := yaml.UnmarshalStrict([]byte(tc.yaml), &fm); err != nil {
-				t.Errorf("Unexpected error unmarshalling funcMatcher: %v", err)
+				t.Errorf("unexpected error unmarshalling funcMatcher: %v", err)
 			}
 
 			if tc.shouldMatch != fm.MatchFunction(tc.path, tc.recv, tc.name) {
-				t.Errorf("MatchFunction(%q, %q, %q) = %v; got %v", tc.path, tc.recv, tc.name, tc.shouldMatch, !tc.shouldMatch)
+				t.Errorf("MatchFunction(%q, %q, %q) got %v, want %v; ", tc.path, tc.recv, tc.name, !tc.shouldMatch, tc.shouldMatch)
 			}
 		})
 	}
 }
 
-func TestSourceMatcherUnmarshaling(t *testing.T) {
+func TestSourceMatcherUnmarshalingErrorCases(t *testing.T) {
 	testCases := []struct {
-		desc, yaml        string
-		shouldErrorOnLoad bool
+		desc, yaml string
 	}{
 		{
 			desc: "Unmarshaling is strict",
 			yaml: `
 Blahblah: foo
 PackageRE: bar`,
-			shouldErrorOnLoad: false, // TODO(#206): true
 		},
 		{
 			desc: "Malformed YAML errors gracefully",
 			yaml: `
 PackageRE: "No ending quote`,
-			shouldErrorOnLoad: true,
 		},
 		{
 			desc: "Malformed regexp errors gracefully",
 			yaml: `
 PackageRE: "(?:NoEndingParen"`,
-			shouldErrorOnLoad: true,
 		},
 		{
 			desc: "Do not permit both Package and PackageRE",
 			yaml: `
 Package: foo
 PackageRE: bar`,
-			shouldErrorOnLoad: true,
 		},
 		{
 			desc: "Do not permit both Type and TypeRE",
 			yaml: `
 Type: foo
 TypeRE: bar`,
-			shouldErrorOnLoad: true,
 		},
 		{
 			desc: "Do not permit both Field and FieldRE",
 			yaml: `
 Field: foo
 FieldRE: bar`,
-			shouldErrorOnLoad: true,
 		},
 	}
 
@@ -206,8 +192,8 @@ FieldRE: bar`,
 			sm := sourceMatcher{}
 			err := yaml.UnmarshalStrict([]byte(tc.yaml), &sm)
 
-			if (err != nil) != tc.shouldErrorOnLoad {
-				t.Errorf("error expectation = %v, but got err=%v", tc.shouldErrorOnLoad, err)
+			if err == nil {
+				t.Errorf("got err = nil, want err != nil")
 			}
 		})
 	}
@@ -287,10 +273,42 @@ Field: qux`,
 			}
 
 			if tc.shouldMatchType != sm.MatchType(tc.path, tc.typ) {
-				t.Errorf("MatchType(%q, %q) = %v; got %v", tc.path, tc.typ, tc.shouldMatchType, !tc.shouldMatchType)
+				t.Errorf("MatchType(%q, %q) got %v, want %v", tc.path, tc.typ, !tc.shouldMatchType, tc.shouldMatchType)
 			}
 			if tc.shouldMatchField != sm.MatchField(tc.path, tc.typ, tc.fieldName) {
-				t.Errorf("MatchField(%q, %q, %q) = %v; got %v", tc.path, tc.typ, tc.fieldName, tc.shouldMatchType, !tc.shouldMatchType)
+				t.Errorf("MatchField(%q, %q, %q) got %v, want %v", tc.path, tc.typ, tc.fieldName, !tc.shouldMatchType, tc.shouldMatchType)
+			}
+		})
+	}
+}
+
+func TestFieldTagMatcherUnmarshalling(t *testing.T) {
+	testCases := []struct {
+		desc, yaml string
+		wantErr    bool
+	}{
+		{
+			desc: "unknown field is not allowed",
+			yaml: `
+Key: good
+UnknownField: bad`,
+			wantErr: true,
+		}, {
+			desc: "valid field tag config",
+			yaml: `
+key: good
+val: AlsoGood`,
+			wantErr: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			ftm := fieldTagMatcher{}
+			err := yaml.UnmarshalStrict([]byte(tc.yaml), &ftm)
+
+			if (err != nil) != tc.wantErr {
+				t.Errorf("got err = %v, expect err = %v", err, tc.wantErr)
 			}
 		})
 	}
@@ -362,7 +380,7 @@ func TestMatcherTypes(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			if tc.matcher.MatchString(tc.s) != tc.shouldMatch {
-				t.Errorf("Expected matcher (%T) %v to return MatchString(%q) == %v, got %v", tc.matcher, tc.matcher, tc.s, tc.shouldMatch, !tc.shouldMatch)
+				t.Errorf("matcher (%T) %v returned MatchString(%q) == %v, want %v, ", tc.matcher, tc.matcher, tc.s, !tc.shouldMatch, tc.shouldMatch)
 			}
 		})
 	}