Repository: google/go-flow-levee
Pull request #34 (2020-07-22 16:42:25)

Detect non variadic sinks
Fixes test TODOs involving non variadic sinks.

Please ignore the 24 extra commits. Look at the `Files changed` tab instead.

> It's a good idea to open an issue first for discussion.

- [x] Tests pass
- [x] Appropriate changes to README are included in PR


Patch:
diff --git a/configuration/README.md b/configuration/README.md
index 3dd272e..d0ceb32 100644
--- a/configuration/README.md
+++ b/configuration/README.md
@@ -28,19 +28,21 @@ Sources are identified via regexp according to package, type, and field names.
 }
 ```
 
-Sinks and sanitizers are identified via regexp according to package and method name.
+Sinks, sanitizers, and some propagators (below) are identified via regexp according to package, method, and (optional) receiver name.
 
 ```json
 {
   "Sinks": [
     {
       "PackageRE": "<package path regexp>",
+      "ReceiverRE": "<type name regexp>",
       "MethodRE": "<method name regexp>"
     }
   ],
   "Sanitizers": [
     {
       "PackageRE": "<package path regexp>",
+      "ReceiverRE": "<type name regexp>",
       "MethodRE": "<method name regexp>"
     }
   ]
@@ -69,13 +71,15 @@ Argument propagators are identified via regexp matching the fully-qualified type
   "TransformingPropagators": [
     {
       "PackageRE": "<package path regexp>",
+      "ReceiverRE": "<type name regexp>",
       "MethodRE": "<method name regexp>"
     }
   ],
   "FieldPropagators": [
     {
-      "Receiver": "<fully qualified type path regexp>",
-      "AccessorRE": "<method name regexp>"
+      "PackageRE": "<package path regexp>",
+      "ReceiverRE": "<type name regexp>",
+      "MethodRE": "<method name regexp>"
     }
   ],
   "PropagatorArgs": {
@@ -84,6 +88,9 @@ Argument propagators are identified via regexp matching the fully-qualified type
 }
 ```
 
+For matchers that accept a `ReceiverRE` regexp matcher, an unspecified string will match any (or no) receiver.
+To match only methods without any receiver (i.e., a top-level function), use the matcher `^$` to match an empty-string receiver name.
+
 ### Example configuration
 
 The following configuration could be used to identify possible instances of credential logging in Kubernetes.
diff --git a/configuration/example-config.json b/configuration/example-config.json
index cc77745..5a2b883 100644
--- a/configuration/example-config.json
+++ b/configuration/example-config.json
@@ -24,6 +24,7 @@
   "Sinks": [
     {
       "PackageRE": "k?log$",
+      "ReceiverRE": "",
       "MethodRE": "Info|Warning|Error|Fatal|Exit"
     }
   ],
@@ -31,6 +32,7 @@
   "TransformingPropagators": [
     {
       "PackageName": ".*proto$",
+      "ReceiverRE": "",
       "MethodRE": "^Clone$|^Marshal"
     }
   ],
diff --git a/internal/levee.go b/internal/levee.go
index ac32548..f4cfd7e 100644
--- a/internal/levee.go
+++ b/internal/levee.go
@@ -18,12 +18,14 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/google/go-flow-levee/internal/pkg/call"
+
 	"github.com/google/go-flow-levee/internal/pkg/config"
+	"github.com/google/go-flow-levee/internal/pkg/varargs"
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/ssa"
 
 	"github.com/google/go-flow-levee/internal/pkg/source"
-	"github.com/google/go-flow-levee/internal/pkg/varargs"
 )
 
 var Analyzer = &analysis.Analyzer{
@@ -73,13 +75,11 @@ func run(pass *analysis.Pass) (interface{}, error) {
 					}
 
 				case conf.IsSink(v):
-					// TODO Only variadic sink arguments are currently detected.
-					if sinkVarargs := varargs.New(v); sinkVarargs != nil {
-						for _, s := range sources {
-							if sinkVarargs.ReferredBy(s) && !s.IsSanitizedAt(v) {
-								report(pass, s, v)
-								break
-							}
+					c := makeCall(v)
+					for _, s := range sources {
+						if c.ReferredBy(s) && !s.IsSanitizedAt(v) {
+							report(pass, s, v)
+							break
 						}
 					}
 				}
@@ -90,6 +90,13 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	return nil, nil
 }
 
+func makeCall(c *ssa.Call) call.Call {
+	if sinkVarargs := varargs.New(c); sinkVarargs != nil {
+		return sinkVarargs
+	}
+	return call.Regular(c)
+}
+
 func report(pass *analysis.Pass, source *source.Source, sink ssa.Node) {
 	var b strings.Builder
 	b.WriteString("a source has reached a sink")
diff --git a/internal/pkg/call/call.go b/internal/pkg/call/call.go
new file mode 100644
index 0000000..df01c92
--- /dev/null
+++ b/internal/pkg/call/call.go
@@ -0,0 +1,55 @@
+// 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 call contains functionality for determining whether a function
+// call refers to a given referrer.
+package call
+
+import (
+	"golang.org/x/tools/go/ssa"
+)
+
+// A Call is a wrapper for *ssa.Call instructions, allowing Variadic and
+// non-Variadic calls to be handled polymorphically
+type Call interface {
+	// ReferredBy determines if the call is referred to by the referrer
+	ReferredBy(r Referrer) bool
+}
+
+// A Referrer is something that can be queried to determine whether it refers to
+// a given ssa Node.
+type Referrer interface {
+	RefersTo(node ssa.Node) bool
+}
+
+// A RegularCall is a wrapper around a non-variadic ssa Call.
+type RegularCall struct {
+	*ssa.Call
+}
+
+// Regular creates a RegularCall from an ssa Call.
+func Regular(c *ssa.Call) *RegularCall {
+	return &RegularCall{c}
+}
+
+// ReferredBy determines if the supplied referrer refers one of the Call's arguments.
+func (c *RegularCall) ReferredBy(r Referrer) bool {
+	for _, a := range c.Call.Call.Args {
+		if r.RefersTo(a.(ssa.Node)) {
+			return true
+		}
+	}
+
+	return false
+}
diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go
index 87e080c..117ecb2 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -38,21 +38,21 @@ func init() {
 // config contains matchers and analysis scope information
 type Config struct {
 	Sources                 []sourceMatcher
-	Sinks                   []NameMatcher
-	Sanitizers              []NameMatcher
-	FieldPropagators        []fieldPropagatorMatcher
+	Sinks                   []callMatcher
+	Sanitizers              []callMatcher
+	FieldPropagators        []callMatcher
 	TransformingPropagators []transformingPropagatorMatcher
 	PropagatorArgs          argumentPropagatorMatcher
-	Whitelist               []packageMatcher
+	Allowlist               []packageMatcher
 	AnalysisScope           []packageMatcher
 }
 
 // shouldSkip returns true for any function that is outside analysis scope,
-// that is whitelisted,
+// that is allowlisted,
 // whose containing package imports "testing"
 // or whose containing package does not import any package containing a source or a sink.
 func (c Config) shouldSkip(pkg *types.Package) bool {
-	if isTestPkg(pkg) || !c.isInScope(pkg) || c.isWhitelisted(pkg) {
+	if isTestPkg(pkg) || !c.isInScope(pkg) || c.isAllowlisted(pkg) {
 		return true
 	}
 
@@ -76,7 +76,7 @@ func (c Config) shouldSkip(pkg *types.Package) bool {
 
 func (c Config) IsSink(call *ssa.Call) bool {
 	for _, p := range c.Sinks {
-		if p.MatchMethodName(call) {
+		if p.Match(call) {
 			return true
 		}
 	}
@@ -86,7 +86,7 @@ func (c Config) IsSink(call *ssa.Call) bool {
 
 func (c Config) IsSanitizer(call *ssa.Call) bool {
 	for _, p := range c.Sanitizers {
-		if p.MatchMethodName(call) {
+		if p.Match(call) {
 			return true
 		}
 	}
@@ -152,7 +152,7 @@ func (c Config) isFieldPropagator(call *ssa.Call) bool {
 	}
 
 	for _, p := range c.FieldPropagators {
-		if p.match(call) {
+		if p.Match(call) {
 			return true
 		}
 	}
@@ -186,8 +186,8 @@ func (c Config) isTransformingPropagator(call *ssa.Call) bool {
 	return false
 }
 
-func (c Config) isWhitelisted(pkg *types.Package) bool {
-	for _, w := range c.Whitelist {
+func (c Config) isAllowlisted(pkg *types.Package) bool {
+	for _, w := range c.Allowlist {
 		if w.match(pkg) {
 			return true
 		}
@@ -268,33 +268,36 @@ func (pm packageMatcher) match(pkg *types.Package) bool {
 	return pm.PackageNameRE.MatchString(pkg.Path())
 }
 
-type NameMatcher struct {
-	PackageRE regexp.Regexp
-	TypeRE    regexp.Regexp
-	MethodRE  regexp.Regexp
+type callMatcher struct {
+	PackageRE  regexp.Regexp
+	ReceiverRE regexp.Regexp
+	MethodRE   regexp.Regexp
 }
 
-func (r NameMatcher) matchPackage(p *types.Package) bool {
+func (r callMatcher) matchPackage(p *types.Package) bool {
 	return r.PackageRE.MatchString(p.Path())
 }
 
-func (r NameMatcher) MatchMethodName(c *ssa.Call) bool {
-	if c.Call.StaticCallee() == nil || c.Call.StaticCallee().Pkg == nil {
+// Match matches methods based on package, method, and receiver regexp.
+// To explicitly match a method with no receiver (i.e., a top-level function),
+// provide the ReceiverRE regexp `^$`.
+func (r callMatcher) Match(c *ssa.Call) bool {
+	callee := c.Call.StaticCallee()
+	if callee == nil || callee.Pkg == nil {
 		return false
 	}
 
-	return r.matchPackage(c.Call.StaticCallee().Pkg.Pkg) &&
-		r.MethodRE.MatchString(c.Call.StaticCallee().Name())
-}
-
-func (r NameMatcher) matchNamedType(n *types.Named) bool {
-	if types.IsInterface(n) {
-		// In our context, both sources and sanitizers are concrete types.
+	if !r.matchPackage(callee.Pkg.Pkg) || !r.MethodRE.MatchString(callee.Name()) {
 		return false
 	}
 
-	return r.PackageRE.MatchString(n.Obj().Pkg().Path()) &&
-		r.TypeRE.MatchString(n.Obj().Name())
+	recv := c.Call.Signature().Recv()
+	var recvName string
+	if recv != nil {
+		recvName = recv.Type().String()
+	}
+
+	return r.ReceiverRE.MatchString(recvName)
 }
 
 var readFileOnce sync.Once
diff --git a/internal/pkg/source/source.go b/internal/pkg/source/source.go
index 6f448f0..dbf5e83 100644
--- a/internal/pkg/source/source.go
+++ b/internal/pkg/source/source.go
@@ -114,6 +114,10 @@ func (a *Source) compress() []ssa.Node {
 	return compressed
 }
 
+func (a *Source) RefersTo(n ssa.Node) bool {
+	return a.HasPathTo(n)
+}
+
 // HasPathTo returns true when a Node is part of declaration-use graph.
 func (a *Source) HasPathTo(n ssa.Node) bool {
 	return a.marked[n]
diff --git a/internal/pkg/varargs/varargs.go b/internal/pkg/varargs/varargs.go
index 9f8da4b..5e8e29b 100644
--- a/internal/pkg/varargs/varargs.go
+++ b/internal/pkg/varargs/varargs.go
@@ -16,6 +16,7 @@
 package varargs
 
 import (
+	"github.com/google/go-flow-levee/internal/pkg/call"
 	"golang.org/x/tools/go/ssa"
 )
 
@@ -44,7 +45,7 @@ func New(s *ssa.Call) *Varargs {
 	}
 
 	a, ok := sl.X.(*ssa.Alloc)
-	if !ok || a.Comment != "varargs" {
+	if !ok || (a.Comment != "varargs" && a.Comment != "slicelit") {
 		return nil
 	}
 
@@ -67,14 +68,10 @@ func New(s *ssa.Call) *Varargs {
 	}
 }
 
-type referrer interface {
-	HasPathTo(node ssa.Node) bool
-}
-
 // ReferredBy determines if the supplied node refers the Vararg in question via a store instruction.
-func (v *Varargs) ReferredBy(r referrer) bool {
+func (v *Varargs) ReferredBy(r call.Referrer) bool {
 	for _, s := range v.stores {
-		if r.HasPathTo(s.Val.(ssa.Node)) {
+		if r.RefersTo(s.Val.(ssa.Node)) {
 			return true
 		}
 	}


Test patch:
diff --git a/internal/pkg/call/call_test.go b/internal/pkg/call/call_test.go
new file mode 100644
index 0000000..00d9e9d
--- /dev/null
+++ b/internal/pkg/call/call_test.go
@@ -0,0 +1,113 @@
+// 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 call
+
+import (
+	"reflect"
+	"testing"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/buildssa"
+	"golang.org/x/tools/go/ssa"
+)
+
+func TestRegularCallReferredBy(t *testing.T) {
+	cases := []struct {
+		desc    string
+		pattern string
+		r       Referrer
+		want    bool
+	}{
+		{
+			"call without args",
+			"noarg",
+			fakeReferrer{true},
+			false,
+		},
+		{
+			"call with one arg that is referred to by a referrer",
+			"onearg",
+			fakeReferrer{true},
+			true,
+		},
+		{
+			"call with one arg that isn't referred to",
+			"onearg",
+			fakeReferrer{false},
+			false,
+		},
+	}
+	for _, tt := range cases {
+		t.Run(tt.desc, func(t *testing.T) {
+			dir := analysistest.TestData()
+			r := analysistest.Run(t, dir, testAnalyzer, tt.pattern)
+			call := Regular(getCall(t, r))
+			got := call.ReferredBy(tt.r)
+			if got != tt.want {
+				t.Errorf("call.ReferredBy(%v) == %v, want %v", call, got, tt.want)
+			}
+		})
+	}
+}
+
+func getCall(t *testing.T, r []*analysistest.Result) *ssa.Call {
+	t.Helper()
+	if len(r) != 1 {
+		t.Fatalf("Got %d results, want one", len(r))
+	}
+	a, ok := r[0].Result.(analyzerResult)
+	if !ok {
+		t.Fatalf("Got result of type %T, want analyzerResult", a)
+	}
+	if len(a.calls) != 1 {
+		t.Fatalf("Got %d calls, want one", len(a.calls))
+	}
+	return a.calls[0]
+}
+
+type fakeReferrer struct {
+	hasPathTo bool
+}
+
+func (f fakeReferrer) RefersTo(node ssa.Node) bool { return f.hasPathTo }
+
+var testAnalyzer = &analysis.Analyzer{
+	Name:       "call",
+	Run:        run,
+	Doc:        "test harness for call",
+	Requires:   []*analysis.Analyzer{buildssa.Analyzer},
+	ResultType: reflect.TypeOf(analyzerResult{}),
+}
+
+type analyzerResult struct {
+	calls []*ssa.Call
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	in := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA)
+	var result analyzerResult
+	for _, fn := range in.SrcFuncs {
+		for _, b := range fn.Blocks {
+			for _, i := range b.Instrs {
+				switch v := i.(type) {
+				case *ssa.Call:
+					result.calls = append(result.calls, v)
+				}
+			}
+		}
+	}
+	return result, nil
+}
diff --git a/internal/pkg/call/testdata/src/noarg/test.go b/internal/pkg/call/testdata/src/noarg/test.go
new file mode 100644
index 0000000..4c3a6e5
--- /dev/null
+++ b/internal/pkg/call/testdata/src/noarg/test.go
@@ -0,0 +1,21 @@
+// 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 test
+
+func NoArgs() {}
+
+func CallNoArgs() {
+	NoArgs()
+}
diff --git a/internal/pkg/call/testdata/src/onearg/test.go b/internal/pkg/call/testdata/src/onearg/test.go
new file mode 100644
index 0000000..ac662e6
--- /dev/null
+++ b/internal/pkg/call/testdata/src/onearg/test.go
@@ -0,0 +1,21 @@
+// 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 test
+
+func OneArg(one int) {}
+
+func CallOneArg() {
+	OneArg(0)
+}
diff --git a/internal/testdata/src/example.com/tests/sinks/tests.go b/internal/testdata/src/example.com/tests/sinks/tests.go
index 7e213c8..0c753c2 100644
--- a/internal/testdata/src/example.com/tests/sinks/tests.go
+++ b/internal/testdata/src/example.com/tests/sinks/tests.go
@@ -24,9 +24,9 @@ func TestSinks(s core.Source, writer io.Writer) {
 	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"
-	core.OneArgSink(s)            // TODO want "a source has reached a sink"
+	core.OneArgSink(s)            // want "a source has reached a sink"
 
-	core.Sink([]interface{}{s, s, s}...) // TODO want "a source has reached a sink"
+	core.Sink([]interface{}{s, s, s}...) // want "a source has reached a sink"
 	core.Sink([]interface{}{s, s, s})    // TODO want "a source has reached a sink"
 }
 
@@ -34,9 +34,9 @@ func TestSinksWithRef(s *core.Source, writer io.Writer) {
 	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"
-	core.OneArgSink(s)            // TODO want "a source has reached a sink"
+	core.OneArgSink(s)            // want "a source has reached a sink"
 
-	core.Sink([]interface{}{s, s, s}...) // TODO want "a source has reached a sink"
+	core.Sink([]interface{}{s, s, s}...) // want "a source has reached a sink"
 	core.Sink([]interface{}{s, s, s})    // TODO want "a source has reached a sink"
 }
 
@@ -59,3 +59,15 @@ func TestSinksWithInnocuousRef(innoc *core.Innocuous, writer io.Writer) {
 	core.Sink([]interface{}{innoc, innoc, innoc}...)
 	core.Sink([]interface{}{innoc, innoc, innoc})
 }
+
+// TestSinksSourceAndInnocuous covers the situations in which both a Source and
+// a non-Source value are in scope, as well as variadic calls involving both
+// Source and non-Source arguments.
+func TestSinksSourceAndInnocuous(source core.Source, innoc core.Innocuous) {
+	core.Sink(source)        // want "a source has reached a sink"
+	core.OneArgSink(source)  // want "a source has reached a sink"
+	core.Sink(innoc, source) // want "a source has reached a sink"
+	core.Sink(source, innoc) // want "a source has reached a sink"
+	core.Sink(innoc)
+	core.OneArgSink(innoc)
+}
diff --git a/internal/testdata/test-config.json b/internal/testdata/test-config.json
index 101b32d..67e634c 100644
--- a/internal/testdata/test-config.json
+++ b/internal/testdata/test-config.json
@@ -20,14 +20,15 @@
   ],
   "FieldPropagators": [
     {
-      "Receiver": "example.com/core.Source",
-      "AccessorRE": "^GetData$",
+      "PackageRE": "^example.com/core$",
+      "ReceiverRE": "Source",
+      "MethodRE": "^GetData$",
       "TODO": "Make this explicit configuration unnecessary."
     }
   ],
   "TransformingPropagators": [
   ],
   "PropagatorArgs": {},
-  "Whitelist": [],
+  "Allowlist": [],
   "AnalysisScope": []
-}
\ No newline at end of file
+}