New Checks: XR007 and XR008: Check for os/exec.Command and os/exec.Co…

…mmandContext Usage (#234)

Reference: #233

CHANGELOG.md

@@ -1,3 +1,10 @@
+# v0.26.0
+
+FEATURES
+
+* **New Check:** `XR007`: check for `exec.Command` usage
+* **New Check:** `XR008`: check for `exec.CommandContext` usage
+
 # v0.25.0
 
 ENHANCEMENTS

README.md

@@ -212,6 +212,8 @@ Extra lint checks are not included in the `tfproviderlint` tool and must be acce
 | [XR004](xpasses/XR004) | check for `ResourceData.Set()` calls that should implement error checking with complex values | AST |
 | [XR005](xpasses/XR005) | check for `Resource` that `Description` is configured | AST |
 | [XR006](xpasses/XR006) | check for `Resource` that implements `Timeouts` for missing `Create`, `Delete`, `Read`, or `Update` implementation | AST |
+| [XR007](xpasses/XR007) | check for `os/exec.Command` usage | AST |
+| [XR008](xpasses/XR008) | check for `os/exec.CommandContext` usage | AST |
 
 ### Extra Schema Checks
 

helper/analysisutils/analyzers.go

@@ -11,6 +11,27 @@ import (
 	"golang.org/x/tools/go/analysis/passes/inspect"
 )
 
+// AvoidSelectorExprAnalyzer returns an Analyzer for *ast.SelectorExpr to avoid
+func AvoidSelectorExprAnalyzer(analyzerName string, callExprAnalyzer, selectorExprAnalyzer *analysis.Analyzer, packagePath, typeName string) *analysis.Analyzer {
+	doc := fmt.Sprintf(`check for %[2]s.%[3]s usage to avoid
+
+The %[1]s analyzer reports usage:
+
+%[2]s.%[3]s
+`, analyzerName, packagePath, typeName)
+
+	return &analysis.Analyzer{
+		Name: analyzerName,
+		Doc:  doc,
+		Requires: []*analysis.Analyzer{
+			callExprAnalyzer,
+			commentignore.Analyzer,
+			selectorExprAnalyzer,
+		},
+		Run: AvoidSelectorExprRunner(analyzerName, callExprAnalyzer, selectorExprAnalyzer, packagePath, typeName),
+	}
+}
+
 // DeprecatedReceiverMethodSelectorExprAnalyzer returns an Analyzer for deprecated *ast.SelectorExpr
 func DeprecatedReceiverMethodSelectorExprAnalyzer(analyzerName string, callExprAnalyzer, selectorExprAnalyzer *analysis.Analyzer, packagePath, typeName, methodName string) *analysis.Analyzer {
 	doc := fmt.Sprintf(`check for deprecated %[2]s.%[3]s usage

helper/analysisutils/runners.go

@@ -15,6 +15,74 @@ import (
 	"golang.org/x/tools/go/ast/inspector"
 )
 
+// AvoidSelectorExprRunner returns an Analyzer runner for *ast.SelectorExpr to avoid
+func AvoidSelectorExprRunner(analyzerName string, callExprAnalyzer, selectorExprAnalyzer *analysis.Analyzer, packagePath, typeName string) func(*analysis.Pass) (interface{}, error) {
+	return func(pass *analysis.Pass) (interface{}, error) {
+		callExprs := pass.ResultOf[callExprAnalyzer].([]*ast.CallExpr)
+		selectorExprs := pass.ResultOf[selectorExprAnalyzer].([]*ast.SelectorExpr)
+		ignorer := pass.ResultOf[commentignore.Analyzer].(*commentignore.Ignorer)
+
+		// CallExpr and SelectorExpr will overlap, so only perform one report/fix
+		reported := make(map[token.Pos]struct{})
+
+		for _, callExpr := range callExprs {
+			if ignorer.ShouldIgnore(analyzerName, callExpr) {
+				continue
+			}
+
+			pass.Report(analysis.Diagnostic{
+				Pos:     callExpr.Pos(),
+				End:     callExpr.End(),
+				Message: fmt.Sprintf("%s: avoid %s.%s", analyzerName, packagePath, typeName),
+				SuggestedFixes: []analysis.SuggestedFix{
+					{
+						Message: "Remove",
+						TextEdits: []analysis.TextEdit{
+							{
+								Pos:     callExpr.Pos(),
+								End:     callExpr.End(),
+								NewText: []byte{},
+							},
+						},
+					},
+				},
+			})
+
+			reported[callExpr.Pos()] = struct{}{}
+		}
+
+		for _, selectorExpr := range selectorExprs {
+			if ignorer.ShouldIgnore(analyzerName, selectorExpr) {
+				continue
+			}
+
+			if _, ok := reported[selectorExpr.Pos()]; ok {
+				continue
+			}
+
+			pass.Report(analysis.Diagnostic{
+				Pos:     selectorExpr.Pos(),
+				End:     selectorExpr.End(),
+				Message: fmt.Sprintf("%s: avoid %s.%s", analyzerName, packagePath, typeName),
+				SuggestedFixes: []analysis.SuggestedFix{
+					{
+						Message: "Remove",
+						TextEdits: []analysis.TextEdit{
+							{
+								Pos:     selectorExpr.Pos(),
+								End:     selectorExpr.End(),
+								NewText: []byte{},
+							},
+						},
+					},
+				},
+			})
+		}
+
+		return nil, nil
+	}
+}
+
 // DeprecatedReceiverMethodSelectorExprRunner returns an Analyzer runner for deprecated *ast.SelectorExpr
 func DeprecatedReceiverMethodSelectorExprRunner(analyzerName string, callExprAnalyzer, selectorExprAnalyzer *analysis.Analyzer, packagePath, typeName, methodName string) func(*analysis.Pass) (interface{}, error) {
 	return func(pass *analysis.Pass) (interface{}, error) {

helper/analysisutils/stdlib_analyzers.go

@@ -21,3 +21,16 @@ func StdlibFunctionCallExprAnalyzer(analyzerName string, packagePath string, fun
 		ResultType: reflect.TypeOf([]*ast.CallExpr{}),
 	}
 }
+
+// StdlibFunctionSelectorExprAnalyzer returns an Analyzer for standard library function *ast.SelectorExpr
+func StdlibFunctionSelectorExprAnalyzer(analyzerName string, packagePath string, functionName string) *analysis.Analyzer {
+	return &analysis.Analyzer{
+		Name: analyzerName,
+		Doc:  fmt.Sprintf("find %s.%s() selectors for later passes", packagePath, functionName),
+		Requires: []*analysis.Analyzer{
+			inspect.Analyzer,
+		},
+		Run:        StdlibFunctionSelectorExprRunner(packagePath, functionName),
+		ResultType: reflect.TypeOf([]*ast.SelectorExpr{}),
+	}
+}

helper/analysisutils/stdlib_runners.go

@@ -31,3 +31,26 @@ func StdlibFunctionCallExprRunner(packagePath string, functionName string) func(
 		return result, nil
 	}
 }
+
+// StdlibFunctionSelectorExprRunner returns an Analyzer runner for function *ast.SelectorExpr
+func StdlibFunctionSelectorExprRunner(packagePath string, functionName string) func(*analysis.Pass) (interface{}, error) {
+	return func(pass *analysis.Pass) (interface{}, error) {
+		inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+		nodeFilter := []ast.Node{
+			(*ast.SelectorExpr)(nil),
+		}
+		var result []*ast.SelectorExpr
+
+		inspect.Preorder(nodeFilter, func(n ast.Node) {
+			selectorExpr := n.(*ast.SelectorExpr)
+
+			if !astutils.IsStdlibPackageFunc(selectorExpr, pass.TypesInfo, packagePath, functionName) {
+				return
+			}
+
+			result = append(result, selectorExpr)
+		})
+
+		return result, nil
+	}
+}

passes/stdlib/osexeccommandcallexpr/osexeccommandcallexpr.go

@@ -0,0 +1,11 @@
+package osexeccommandcallexpr
+
+import (
+	"github.com/bflad/tfproviderlint/helper/analysisutils"
+)
+
+var Analyzer = analysisutils.StdlibFunctionCallExprAnalyzer(
+	"osexeccommandcallexpr",
+	"os/exec",
+	"Command",
+)

passes/stdlib/osexeccommandcallexpr/osexeccommandcallexpr_test.go

@@ -0,0 +1,15 @@
+package osexeccommandcallexpr
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+func TestValidateAnalyzer(t *testing.T) {
+	err := analysis.Validate([]*analysis.Analyzer{Analyzer})
+
+	if err != nil {
+		t.Fatal(err)
+	}
+}

passes/stdlib/osexeccommandcontextcallexpr/osexeccommandcontextcallexpr.go

@@ -0,0 +1,11 @@
+package osexeccommandcontextcallexpr
+
+import (
+	"github.com/bflad/tfproviderlint/helper/analysisutils"
+)
+
+var Analyzer = analysisutils.StdlibFunctionCallExprAnalyzer(
+	"osexeccommandcontextcallexpr",
+	"os/exec",
+	"CommandContext",
+)

passes/stdlib/osexeccommandcontextcallexpr/osexeccommandcontextcallexpr_test.go

@@ -0,0 +1,15 @@
+package osexeccommandcontextcallexpr
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+func TestValidateAnalyzer(t *testing.T) {
+	err := analysis.Validate([]*analysis.Analyzer{Analyzer})
+
+	if err != nil {
+		t.Fatal(err)
+	}
+}

passes/stdlib/osexeccommandcontextselectorexpr/osexeccommandcontextselectorexpr.go

@@ -0,0 +1,11 @@
+package osexeccommandcontextselectorexpr
+
+import (
+	"github.com/bflad/tfproviderlint/helper/analysisutils"
+)
+
+var Analyzer = analysisutils.StdlibFunctionSelectorExprAnalyzer(
+	"osexeccommandselectorexpr",
+	"os/exec",
+	"CommandContext",
+)

passes/stdlib/osexeccommandcontextselectorexpr/osexeccommandcontextselectorexpr_test.go

@@ -0,0 +1,15 @@
+package osexeccommandcontextselectorexpr
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+func TestValidateAnalyzer(t *testing.T) {
+	err := analysis.Validate([]*analysis.Analyzer{Analyzer})
+
+	if err != nil {
+		t.Fatal(err)
+	}
+}

passes/stdlib/osexeccommandselectorexpr/osexeccommandselectorexpr.go

@@ -0,0 +1,11 @@
+package osexeccommandselectorexpr
+
+import (
+	"github.com/bflad/tfproviderlint/helper/analysisutils"
+)
+
+var Analyzer = analysisutils.StdlibFunctionSelectorExprAnalyzer(
+	"osexeccommandselectorexpr",
+	"os/exec",
+	"Command",
+)

passes/stdlib/osexeccommandselectorexpr/osexeccommandselectorexpr_test.go

@@ -0,0 +1,15 @@
+package osexeccommandselectorexpr
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+func TestValidateAnalyzer(t *testing.T) {
+	err := analysis.Validate([]*analysis.Analyzer{Analyzer})
+
+	if err != nil {
+		t.Fatal(err)
+	}
+}

xpasses/XR007/README.md

@@ -0,0 +1,28 @@
+# XR007
+
+The XR007 analyzer reports usage of the [`os/exec.Command()`](https://pkg.go.dev/os/exec#Command) function. Providers that are using Go language based SDKs likely want to prevent any execution of other binaries for various reasons such as security and unexpected requirements (e.g. tool installation outside Terraform).
+
+## Flagged Code
+
+```go
+var sneaky = exec.Command
+
+sneaky("evilprogram")
+
+exec.Command("evilprogram")
+```
+
+## Passing Code
+
+```go
+// Not present :)
+```
+
+## Ignoring Reports
+
+Singular reports can be ignored by adding the a `//lintignore:XR007` Go code comment at the end of the offending line or on the line immediately proceding, e.g.
+
+```go
+//lintignore:XR007
+exec.Command("evilprogram")
+```

xpasses/XR007/XR007.go

@@ -0,0 +1,15 @@
+package XR007
+
+import (
+	"github.com/bflad/tfproviderlint/helper/analysisutils"
+	"github.com/bflad/tfproviderlint/passes/stdlib/osexeccommandcallexpr"
+	"github.com/bflad/tfproviderlint/passes/stdlib/osexeccommandselectorexpr"
+)
+
+var Analyzer = analysisutils.AvoidSelectorExprAnalyzer(
+	"XR007",
+	osexeccommandcallexpr.Analyzer,
+	osexeccommandselectorexpr.Analyzer,
+	"os/exec",
+	"Command",
+)

xpasses/XR007/XR007_test.go

@@ -0,0 +1,18 @@
+package XR007_test
+
+import (
+	"testing"
+
+	"github.com/bflad/tfproviderlint/xpasses/XR007"
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestXR007(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, XR007.Analyzer, "a")
+}
+
+func TestAnalyzerFixes(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.RunWithSuggestedFixes(t, testdata, XR007.Analyzer, "a")
+}

xpasses/XR007/testdata/src/a/alias.go

@@ -0,0 +1,13 @@
+package a
+
+import (
+	e "os/exec"
+)
+
+var failingAlias = e.Command // want "avoid os/exec.Command"
+
+func fAlias() {
+	e.Command("true") // want "avoid os/exec.Command"
+
+	failingAlias("true")
+}

xpasses/XR007/testdata/src/a/alias.go.golden

@@ -0,0 +1,13 @@
+package a
+
+import (
+	e "os/exec"
+)
+
+// want "avoid os/exec.Command"
+
+func fAlias() {
+	// want "avoid os/exec.Command"
+
+	failingAlias("true")
+}

xpasses/XR007/testdata/src/a/main.go

@@ -0,0 +1,22 @@
+package a
+
+import (
+	"os/exec"
+)
+
+var failing = exec.Command // want "avoid os/exec.Command"
+
+func f() {
+	// Comment ignored
+
+	//lintignore:XR007
+	exec.Command("true")
+
+	exec.Command("true") //lintignore:XR007
+
+	// Failing
+
+	exec.Command("true") // want "avoid os/exec.Command"
+
+	failing("true")
+}