Repository: aquasecurity/tfsec
Pull request #1093 (2021-10-01 08:05:19)

Slight rework of the nil protection
@skirkpatrick tests for a panic on rule checking

This rework uses the tests provided by @skirkpatrick but moves the nil protection into the check.
Also reworking StopOnErrors to be more intuitive



Patch:
diff --git a/cmd/tfsec/main.go b/cmd/tfsec/main.go
index 8aaacb8ee..7162d4e78 100644
--- a/cmd/tfsec/main.go
+++ b/cmd/tfsec/main.go
@@ -370,7 +370,9 @@ func getScannerOptions() []scanner.Option {
 		options = append(options, scanner.OptionWithWorkspaceName(workspace))
 	}
 
-	options = append(options, scanner.OptionIgnoreCheckErrors(!stopOnCheckError))
+	if stopOnCheckError {
+		options = append(options, scanner.OptionStopOnErrors())
+	}
 
 	var allExcludedRuleIDs []string
 	for _, exclude := range strings.Split(excludedRuleIDs, ",") {
diff --git a/internal/app/tfsec/scanner/option.go b/internal/app/tfsec/scanner/option.go
index 77d2c1fb4..ef93c2768 100644
--- a/internal/app/tfsec/scanner/option.go
+++ b/internal/app/tfsec/scanner/option.go
@@ -26,9 +26,9 @@ func OptionIncludeRules(ruleIDs []string) func(s *Scanner) {
 	}
 }
 
-func OptionIgnoreCheckErrors(ignore bool) func(s *Scanner) {
+func OptionStopOnErrors() func(s *Scanner) {
 	return func(s *Scanner) {
-		s.ignoreCheckErrors = ignore
+		s.ignoreCheckErrors = false
 	}
 }
 
diff --git a/pkg/rule/check.go b/pkg/rule/check.go
index d5ae80c74..bd7245cf0 100644
--- a/pkg/rule/check.go
+++ b/pkg/rule/check.go
@@ -7,17 +7,16 @@ import (
 	runtimeDebug "runtime/debug"
 	"strings"
 
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/debug"
 	"github.com/aquasecurity/tfsec/pkg/provider"
 
 	"github.com/aquasecurity/tfsec/pkg/result"
 
-	"github.com/aquasecurity/tfsec/internal/app/tfsec/debug"
-
 	"github.com/aquasecurity/tfsec/internal/app/tfsec/block"
 )
 
 // CheckRule the provided HCL block against the rule
-func CheckRule(r *Rule, resourceBlock block.Block, module block.Module, ignoreErrors bool) result.Set {
+func CheckRule(r *Rule, resourceBlock block.Block, module block.Module, ignoreErrors bool) (resultSet result.Set) {
 	if ignoreErrors {
 		defer func() {
 			if err := recover(); err != nil {
@@ -28,14 +27,13 @@ func CheckRule(r *Rule, resourceBlock block.Block, module block.Module, ignoreEr
 	}
 
 	var links []string
-
 	if r.Provider != provider.CustomProvider {
 		links = append(links, fmt.Sprintf("https://tfsec.dev/docs/%s/%s/%s#%s/%s", r.Provider, r.Service, r.ShortCode, r.Provider, r.Service))
 	}
 
 	links = append(links, r.Documentation.Links...)
 
-	resultSet := result.NewSet(resourceBlock).
+	resultSet = result.NewSet(resourceBlock).
 		WithRuleID(r.ID()).
 		WithLegacyRuleID(r.LegacyID).
 		WithRuleSummary(r.Documentation.Summary).


Test patch:
diff --git a/internal/app/tfsec/custom/processing_test.go b/internal/app/tfsec/custom/processing_test.go
index c6c2b0033..f47792864 100644
--- a/internal/app/tfsec/custom/processing_test.go
+++ b/internal/app/tfsec/custom/processing_test.go
@@ -391,7 +391,7 @@ func scanTerraform(t *testing.T, mainTf string) []result.Result {
 	blocks, err := parser.New(dirName, parser.OptionStopOnHCLError()).ParseDirectory()
 	assert.NoError(t, err)
 
-	return scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	return scanner.New(scanner.OptionStopOnErrors()).Scan(blocks)
 }
 
 // This function is copied from setup_test.go as it is not possible to import function from test files.
diff --git a/internal/app/tfsec/test/module_test.go b/internal/app/tfsec/test/module_test.go
index 00da4e1a7..74981c2af 100644
--- a/internal/app/tfsec/test/module_test.go
+++ b/internal/app/tfsec/test/module_test.go
@@ -70,7 +70,7 @@ resource "problem" "uhoh" {
 
 	blocks, err := parser.New(fs.RealPath("/project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, badRule.ID(), "", results)
 
 }
@@ -97,7 +97,7 @@ resource "problem" "uhoh" {
 
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, badRule.ID(), "", results)
 
 }
@@ -124,7 +124,7 @@ resource "problem" "uhoh" {
 
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, badRule.ID(), "", results)
 
 }
@@ -160,7 +160,7 @@ resource "problem" "uhoh" {
 
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, badRule.ID(), "", results)
 
 }
@@ -197,7 +197,7 @@ resource "problem" "uhoh" {
 
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, badRule.ID(), "", results)
 
 }
@@ -251,7 +251,7 @@ resource "problem" "uhoh" {
 
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, badRule.ID(), "", results)
 
 }
@@ -285,7 +285,7 @@ resource "problem" "uhoh" {
 
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, badRule.ID(), "", results)
 
 }
@@ -322,7 +322,7 @@ resource "problem" "uhoh" {
 
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, badRule.ID(), "", results)
 
 }
@@ -376,7 +376,7 @@ resource "problem" "uhoh" {
 
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, badRule.ID(), "", results)
 
 }
@@ -466,7 +466,7 @@ module "something" {
 
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, "aws-iam-no-policy-wildcards", "", results)
 }
 
@@ -494,7 +494,7 @@ resource "aws_api_gateway_domain_name" "outdated_security_policy" {
 	require.NoError(t, fs.WriteTextFile("project/main.tf", example))
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, "aws-api-gateway-use-secure-tls-policy", "", results)
 }
 
@@ -522,6 +522,6 @@ resource "aws_api_gateway_domain_name" "outdated_security_policy" {
 	require.NoError(t, fs.WriteTextFile("project/main.tf", example))
 	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
 	require.NoError(t, err)
-	results := scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	results := scanner.New().Scan(blocks)
 	testutil.AssertCheckCode(t, "", "aws-api-gateway-use-secure-tls-policy", results)
 }
diff --git a/internal/app/tfsec/test/performance_test.go b/internal/app/tfsec/test/performance_test.go
index f6d47e798..dec792bdf 100644
--- a/internal/app/tfsec/test/performance_test.go
+++ b/internal/app/tfsec/test/performance_test.go
@@ -32,6 +32,6 @@ func BenchmarkCalculate(b *testing.B) {
 		if err != nil {
 			panic(err)
 		}
-		_ = scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+		_ = scanner.New().Scan(blocks)
 	}
 }
diff --git a/internal/app/tfsec/test/scanner_test.go b/internal/app/tfsec/test/scanner_test.go
new file mode 100644
index 000000000..c98c018b6
--- /dev/null
+++ b/internal/app/tfsec/test/scanner_test.go
@@ -0,0 +1,157 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/testutil"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/block"
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/parser"
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/scanner"
+	"github.com/aquasecurity/tfsec/pkg/provider"
+	"github.com/aquasecurity/tfsec/pkg/result"
+	"github.com/aquasecurity/tfsec/pkg/rule"
+	"github.com/aquasecurity/tfsec/pkg/severity"
+)
+
+var panicRule = rule.Rule{
+	LegacyID:  "EXA001",
+	Provider:  provider.AWSProvider,
+	Service:   "service",
+	ShortCode: "abc",
+	Documentation: rule.RuleDocumentation{
+		Summary:     "A stupid example check for a test.",
+		Impact:      "You will look stupid",
+		Resolution:  "Don't do stupid stuff",
+		Explanation: "Panic should not be set.",
+		BadExample: []string{`
+resource "problem" "x" {
+panic = true
+}
+`},
+		GoodExample: []string{`
+resource "problem" "x" {
+
+}
+`},
+		Links: nil,
+	},
+	RequiredTypes:   []string{"resource"},
+	RequiredLabels:  []string{"problem"},
+	DefaultSeverity: severity.High,
+	CheckFunc: func(set result.Set, resourceBlock block.Block, _ block.Module) {
+		if resourceBlock.GetAttribute("panic").IsTrue() {
+			panic("This is fine")
+		}
+	},
+}
+
+func Test_PanicInCheckNotAllowed(t *testing.T) {
+
+	scanner.RegisterCheckRule(panicRule)
+	defer scanner.DeregisterCheckRule(panicRule)
+
+	fs, err := testutil.NewFilesystem()
+	require.NoError(t, err)
+	defer fs.Close()
+
+	require.NoError(t, fs.WriteTextFile("project/main.tf", `
+resource "problem" "this" {
+	panic = true
+}
+`))
+
+	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
+	require.NoError(t, err)
+	results := scanner.New().Scan(blocks)
+	testutil.AssertCheckCode(t, "", panicRule.ID(), results)
+}
+
+func Test_PanicInCheckAllowed(t *testing.T) {
+
+	scanner.RegisterCheckRule(panicRule)
+	defer scanner.DeregisterCheckRule(panicRule)
+
+	fs, err := testutil.NewFilesystem()
+	require.NoError(t, err)
+	defer fs.Close()
+
+	require.NoError(t, fs.WriteTextFile("project/main.tf", `
+resource "problem" "this" {
+	panic = true
+}
+`))
+
+	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
+	require.NoError(t, err)
+	scan := scanner.New(scanner.OptionStopOnErrors())
+	assert.Panics(t, func() {
+		scan.Scan(blocks)
+	})
+}
+
+func Test_PanicInCheckIncludePassed(t *testing.T) {
+
+	scanner.RegisterCheckRule(panicRule)
+	defer scanner.DeregisterCheckRule(panicRule)
+
+	fs, err := testutil.NewFilesystem()
+	require.NoError(t, err)
+	defer fs.Close()
+
+	require.NoError(t, fs.WriteTextFile("project/main.tf", `
+resource "problem" "this" {
+	panic = true
+}
+`))
+
+	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
+	require.NoError(t, err)
+	results := scanner.New(scanner.OptionIncludePassed()).Scan(blocks)
+	testutil.AssertCheckCode(t,  panicRule.ID(), "", results)
+}
+
+func Test_PanicNotInCheckNotIncludePassed(t *testing.T) {
+
+	scanner.RegisterCheckRule(panicRule)
+	defer scanner.DeregisterCheckRule(panicRule)
+
+	fs, err := testutil.NewFilesystem()
+	require.NoError(t, err)
+	defer fs.Close()
+
+	require.NoError(t, fs.WriteTextFile("project/main.tf", `
+resource "problem" "this" {
+	panic = true
+}
+`))
+
+	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
+	require.NoError(t, err)
+	results := scanner.New().Scan(blocks)
+	testutil.AssertCheckCode(t, "",  panicRule.ID(), results)
+}
+
+func Test_PanicNotInCheckNotIncludePassedStopOnError(t *testing.T) {
+
+	scanner.RegisterCheckRule(panicRule)
+	defer scanner.DeregisterCheckRule(panicRule)
+
+	fs, err := testutil.NewFilesystem()
+	require.NoError(t, err)
+	defer fs.Close()
+
+	require.NoError(t, fs.WriteTextFile("project/main.tf", `
+resource "problem" "this" {
+	panic = true
+}
+`))
+
+	blocks, err := parser.New(fs.RealPath("project/"), parser.OptionStopOnHCLError()).ParseDirectory()
+	require.NoError(t, err)
+
+	scanner := scanner.New(scanner.OptionStopOnErrors())
+	assert.Panics(t, func() { _ = scanner.Scan(blocks)})
+}
\ No newline at end of file
diff --git a/internal/app/tfsec/testutil/util.go b/internal/app/tfsec/testutil/util.go
index 3d647608b..d6a68de90 100644
--- a/internal/app/tfsec/testutil/util.go
+++ b/internal/app/tfsec/testutil/util.go
@@ -20,7 +20,7 @@ import (
 
 func ScanHCL(source string, t *testing.T, additionalOptions ...scanner.Option) []result.Result {
 	modules := CreateModulesFromSource(source, ".tf", t)
-	scanner := scanner.New(scanner.OptionIgnoreCheckErrors(false))
+	scanner := scanner.New(scanner.OptionStopOnErrors())
 	for _, opt := range additionalOptions {
 		opt(scanner)
 	}
@@ -29,7 +29,7 @@ func ScanHCL(source string, t *testing.T, additionalOptions ...scanner.Option) [
 
 func ScanJSON(source string, t *testing.T) []result.Result {
 	blocks := CreateModulesFromSource(source, ".tf.json", t)
-	return scanner.New(scanner.OptionIgnoreCheckErrors(false)).Scan(blocks)
+	return scanner.New(scanner.OptionStopOnErrors()).Scan(blocks)
 }
 
 func CreateModulesFromSource(source string, ext string, t *testing.T) []block.Module {