Repository: google/go-flow-levee
Pull request #61 (2020-08-24 21:15:56)

Fix bug in config.IsSinkFunction
## The bug
`config.IsSinkFunction` completely disregards the function's package when determining whether an `ssa.Function` is a sink. It only looks at the function's name.

## The fix
The immediate fix is simply to verify that the function's package matches the config. However, I've also added a test for it, because I've actually run into this bug a few times now while writing tests for other packages (e.g., `levee` and `source`), and I want some confidence that it won't happen again.

## Future PR's
Since the `config` package is not tested directly right now, I intend to add more tests in future PR's in order to avoid these kinds of bugs. Having direct test coverage will also increase our ability to refactor the code safely.

- [x] Tests pass
- [x] Appropriate changes to README are included in PR

mlevesquedion: What I really want to validate is that we're checking for the package and (if needed) receiver name in addition to the function name.

I don't see how modifying the `test-config.json` would increase our confidence that the function is doing the proper checks. :thinking:  Do you think I should add more positive and negative cases for sink identification in the tests?

Patch:
diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go
index 6d3851a..76ac020 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -53,12 +53,25 @@ func (c Config) IsSink(call *ssa.Call) bool {
 }
 
 func (c Config) IsSinkFunction(f *ssa.Function) bool {
+	// according to the documentation for ssa.Function, this can happen if
+	// f is a "shared func", e.g. "wrappers and error.Error"
+	if f.Pkg == nil {
+		return false
+	}
+
+	var recvName string
+	if recv := f.Signature.Recv(); recv != nil {
+		recvName = recv.Type().String()
+	}
+
 	for _, p := range c.Sinks {
-		if p.MethodRE.MatchString(f.Name()) {
+		if !p.PackageRE.MatchString(f.Pkg.Pkg.Name()) || !p.MethodRE.MatchString(f.Name()) {
+			continue
+		}
+		if p.ReceiverRE.MatchString(recvName) {
 			return true
 		}
 	}
-
 	return false
 }
 
diff --git a/internal/pkg/sanitizer/sanitizer.go b/internal/pkg/sanitizer/sanitizer.go
index 50822be..b703d45 100644
--- a/internal/pkg/sanitizer/sanitizer.go
+++ b/internal/pkg/sanitizer/sanitizer.go
@@ -57,11 +57,5 @@ func (s Sanitizer) Dominates(target ssa.Instruction) bool {
 		return sanitizationIdx < targetIdx
 	}
 
-	for _, d := range s.Call.Block().Dominees() {
-		if target.Block() == d {
-			return true
-		}
-	}
-
 	return false
 }


Test patch:
diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go
new file mode 100644
index 0000000..0c951cf
--- /dev/null
+++ b/internal/pkg/config/config_test.go
@@ -0,0 +1,56 @@
+// 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 (
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/buildssa"
+)
+
+var testAnalyzer = &analysis.Analyzer{
+	Name:     "config",
+	Run:      runTest,
+	Doc:      "test harness for the logic related to config",
+	Requires: []*analysis.Analyzer{buildssa.Analyzer},
+}
+
+func runTest(pass *analysis.Pass) (interface{}, error) {
+	in := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA)
+
+	conf, err := ReadConfig()
+	if err != nil {
+		return nil, err
+	}
+
+	for _, f := range in.SrcFuncs {
+		if conf.IsSinkFunction(f) {
+			pass.Reportf(f.Pos(), "sink")
+		}
+	}
+
+	return nil, nil
+}
+
+func TestConfig(t *testing.T) {
+	testdata := analysistest.TestData()
+	if err := FlagSet.Set("config", filepath.Join(testdata, "test-config.json")); err != nil {
+		t.Fatal(err)
+	}
+	analysistest.Run(t, testdata, testAnalyzer, "core", "notcore")
+}
diff --git a/internal/pkg/config/testdata/src/core/core.go b/internal/pkg/config/testdata/src/core/core.go
new file mode 100644
index 0000000..38cd223
--- /dev/null
+++ b/internal/pkg/config/testdata/src/core/core.go
@@ -0,0 +1,25 @@
+// 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 core
+
+func Sink() {} // want "sink"
+
+func NotSink() {}
+
+type Sinker struct{}
+
+func (s Sinker) Do() {} // want "sink"
+
+func (s Sinker) DoNot() {}
diff --git a/internal/pkg/config/testdata/src/notcore/notcore.go b/internal/pkg/config/testdata/src/notcore/notcore.go
new file mode 100644
index 0000000..c469c1e
--- /dev/null
+++ b/internal/pkg/config/testdata/src/notcore/notcore.go
@@ -0,0 +1,25 @@
+// 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 notcore
+
+func Sink() {}
+
+func NotSink() {}
+
+type Sinker struct{}
+
+func (s Sinker) Do() {}
+
+func (s Sinker) DoNot() {}
diff --git a/internal/pkg/config/testdata/test-config.json b/internal/pkg/config/testdata/test-config.json
new file mode 100644
index 0000000..4d03234
--- /dev/null
+++ b/internal/pkg/config/testdata/test-config.json
@@ -0,0 +1,13 @@
+{
+  "Sinks": [
+    {
+      "PackageRE": "^core$",
+      "MethodRE": "^Sink$"
+    },
+    {
+      "PackageRE": "^core$",
+      "MethodRE": "^Do$",
+      "TypeRE": "^Sinker$"
+    }
+  ]
+}