Refactor properly the package error parsing and cover all test cases

Signed-off-by: Cosmin Cojocar <[email protected]>

Author: ccojocar

analyzer.go

@@ -111,7 +111,7 @@ func (gosec *Analyzer) Process(buildTags []string, packagePaths ...string) error
 		}
 		for _, pkg := range pkgs {
 			if pkg.Name != "" {
-				err := gosec.parseErrors(pkg)
+				err := gosec.ParseErrors(pkg)
 				if err != nil {
 					return fmt.Errorf("parsing errors in pkg %q: %v", pkg.Name, err)
 				}
@@ -185,39 +185,34 @@ func (gosec *Analyzer) check(pkg *packages.Package) {
 	}
 }
 
-func (gosec *Analyzer) parseErrors(pkg *packages.Package) error {
+// ParseErrors parses the errors from given package
+func (gosec *Analyzer) ParseErrors(pkg *packages.Package) error {
 	if len(pkg.Errors) == 0 {
 		return nil
 	}
 	for _, pkgErr := range pkg.Errors {
-		// infoErr contains information about the error
-		// at index 0 is the file path
-		// at index 1 is the line; index 2 is for column
-		// at index 3 is the actual error
-		infoErr := strings.Split(pkgErr.Error(), ":")
-		filePath := infoErr[0]
+		parts := strings.Split(pkgErr.Pos, ":")
+		file := parts[0]
 		var err error
-		var line, column int
-		var errorMsg string
-		if len(infoErr) > 3 {
-			if line, err = strconv.Atoi(infoErr[1]); err != nil {
+		var line int
+		if len(parts) > 1 {
+			if line, err = strconv.Atoi(parts[1]); err != nil {
 				return fmt.Errorf("parsing line: %v", err)
 			}
-			if column, err = strconv.Atoi(infoErr[2]); err != nil {
+		}
+		var column int
+		if len(parts) > 2 {
+			if column, err = strconv.Atoi(parts[2]); err != nil {
 				return fmt.Errorf("parsing column: %v", err)
 			}
-			errorMsg = strings.TrimSpace(infoErr[3])
-		} else if len(infoErr) > 1 {
-			errorMsg = strings.TrimSpace(infoErr[1])
-		} else {
-			return fmt.Errorf("cannot parse error %q", infoErr)
 		}
-		newErr := NewError(line, column, errorMsg)
-		if errSlice, ok := gosec.errors[filePath]; ok {
-			gosec.errors[filePath] = append(errSlice, *newErr)
+		msg := strings.TrimSpace(pkgErr.Msg)
+		newErr := NewError(line, column, msg)
+		if errSlice, ok := gosec.errors[file]; ok {
+			gosec.errors[file] = append(errSlice, *newErr)
 		} else {
 			errSlice = []Error{}
-			gosec.errors[filePath] = append(errSlice, *newErr)
+			gosec.errors[file] = append(errSlice, *newErr)
 		}
 	}
 	return nil

analyzer_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/securego/gosec"
 	"github.com/securego/gosec/rules"
+	"golang.org/x/tools/go/packages"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -234,58 +235,185 @@ var _ = Describe("Analyzer", func() {
 			err = analyzer.Process(buildTags, pkg.Path)
 			Expect(err).ShouldNot(HaveOccurred())
 		})
+
 		It("should report an error when the package is empty", func() {
 			analyzer.LoadRules(rules.Generate().Builders())
 			pkg := testutils.NewTestPackage()
 			defer pkg.Close()
 			err := analyzer.Process(buildTags, pkg.Path)
 			Expect(err).Should(HaveOccurred())
 		})
-	})
 
-	It("should be possible to overwrite nosec comments, and report issues", func() {
-		// Rule for MD5 weak crypto usage
-		sample := testutils.SampleCodeG401[0]
-		source := sample.Code[0]
-
-		// overwrite nosec option
-		nosecIgnoreConfig := gosec.NewConfig()
-		nosecIgnoreConfig.SetGlobal(gosec.Nosec, "true")
-		customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, logger)
-		customAnalyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders())
-
-		nosecPackage := testutils.NewTestPackage()
-		defer nosecPackage.Close()
-		nosecSource := strings.Replace(source, "h := md5.New()", "h := md5.New() // #nosec", 1)
-		nosecPackage.AddFile("md5.go", nosecSource)
-		err := nosecPackage.Build()
-		Expect(err).ShouldNot(HaveOccurred())
-		err = customAnalyzer.Process(buildTags, nosecPackage.Path)
-		Expect(err).ShouldNot(HaveOccurred())
-		nosecIssues, _, _ := customAnalyzer.Report()
-		Expect(nosecIssues).Should(HaveLen(sample.Errors))
+		It("should be possible to overwrite nosec comments, and report issues", func() {
+			// Rule for MD5 weak crypto usage
+			sample := testutils.SampleCodeG401[0]
+			source := sample.Code[0]
 
-	})
+			// overwrite nosec option
+			nosecIgnoreConfig := gosec.NewConfig()
+			nosecIgnoreConfig.SetGlobal(gosec.Nosec, "true")
+			customAnalyzer := gosec.NewAnalyzer(nosecIgnoreConfig, tests, logger)
+			customAnalyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders())
 
-	It("should be able to analyze Go test package", func() {
-		customAnalyzer := gosec.NewAnalyzer(nil, true, logger)
-		customAnalyzer.LoadRules(rules.Generate().Builders())
-		pkg := testutils.NewTestPackage()
-		defer pkg.Close()
-		pkg.AddFile("foo.go", `
+			nosecPackage := testutils.NewTestPackage()
+			defer nosecPackage.Close()
+			nosecSource := strings.Replace(source, "h := md5.New()", "h := md5.New() // #nosec", 1)
+			nosecPackage.AddFile("md5.go", nosecSource)
+			err := nosecPackage.Build()
+			Expect(err).ShouldNot(HaveOccurred())
+			err = customAnalyzer.Process(buildTags, nosecPackage.Path)
+			Expect(err).ShouldNot(HaveOccurred())
+			nosecIssues, _, _ := customAnalyzer.Report()
+			Expect(nosecIssues).Should(HaveLen(sample.Errors))
+
+		})
+
+		It("should be able to analyze Go test package", func() {
+			customAnalyzer := gosec.NewAnalyzer(nil, true, logger)
+			customAnalyzer.LoadRules(rules.Generate().Builders())
+			pkg := testutils.NewTestPackage()
+			defer pkg.Close()
+			pkg.AddFile("foo.go", `
 			package foo
 			func foo(){
 			}`)
-		pkg.AddFile("foo_test.go", `
+			pkg.AddFile("foo_test.go", `
 			package foo_test
 			import "testing"
 			func TestFoo(t *testing.T){
 			}`)
-		err := pkg.Build()
-		Expect(err).ShouldNot(HaveOccurred())
-		err = customAnalyzer.Process(buildTags, pkg.Path)
-		Expect(err).ShouldNot(HaveOccurred())
-		_, metrics, _ := customAnalyzer.Report()
-		Expect(metrics.NumFiles).To(Equal(3))
+			err := pkg.Build()
+			Expect(err).ShouldNot(HaveOccurred())
+			err = customAnalyzer.Process(buildTags, pkg.Path)
+			Expect(err).ShouldNot(HaveOccurred())
+			_, metrics, _ := customAnalyzer.Report()
+			Expect(metrics.NumFiles).To(Equal(3))
+		})
+	})
+
+	Context("when parsing errors from a package", func() {
+
+		It("should return no error when the error list is empty", func() {
+			pkg := &packages.Package{}
+			err := analyzer.ParseErrors(pkg)
+			Expect(err).ShouldNot(HaveOccurred())
+		})
+
+		It("should properly parse the errors", func() {
+			pkg := &packages.Package{
+				Errors: []packages.Error{
+					packages.Error{
+						Pos: "file:1:2",
+						Msg: "build error",
+					},
+				},
+			}
+			err := analyzer.ParseErrors(pkg)
+			Expect(err).ShouldNot(HaveOccurred())
+			_, _, errors := analyzer.Report()
+			Expect(len(errors)).To(Equal(1))
+			for _, ferr := range errors {
+				Expect(len(ferr)).To(Equal(1))
+				Expect(ferr[0].Line).To(Equal(1))
+				Expect(ferr[0].Column).To(Equal(2))
+				Expect(ferr[0].Err).Should(MatchRegexp(`build error`))
+			}
+		})
+
+		It("should properly parse the errors without line and column", func() {
+			pkg := &packages.Package{
+				Errors: []packages.Error{
+					packages.Error{
+						Pos: "file",
+						Msg: "build error",
+					},
+				},
+			}
+			err := analyzer.ParseErrors(pkg)
+			Expect(err).ShouldNot(HaveOccurred())
+			_, _, errors := analyzer.Report()
+			Expect(len(errors)).To(Equal(1))
+			for _, ferr := range errors {
+				Expect(len(ferr)).To(Equal(1))
+				Expect(ferr[0].Line).To(Equal(0))
+				Expect(ferr[0].Column).To(Equal(0))
+				Expect(ferr[0].Err).Should(MatchRegexp(`build error`))
+			}
+		})
+
+		It("should properly parse the errors without column", func() {
+			pkg := &packages.Package{
+				Errors: []packages.Error{
+					packages.Error{
+						Pos: "file",
+						Msg: "build error",
+					},
+				},
+			}
+			err := analyzer.ParseErrors(pkg)
+			Expect(err).ShouldNot(HaveOccurred())
+			_, _, errors := analyzer.Report()
+			Expect(len(errors)).To(Equal(1))
+			for _, ferr := range errors {
+				Expect(len(ferr)).To(Equal(1))
+				Expect(ferr[0].Line).To(Equal(0))
+				Expect(ferr[0].Column).To(Equal(0))
+				Expect(ferr[0].Err).Should(MatchRegexp(`build error`))
+			}
+		})
+
+		It("should return error when line cannot be parsed", func() {
+			pkg := &packages.Package{
+				Errors: []packages.Error{
+					packages.Error{
+						Pos: "file:line",
+						Msg: "build error",
+					},
+				},
+			}
+			err := analyzer.ParseErrors(pkg)
+			Expect(err).Should(HaveOccurred())
+		})
+
+		It("should return error when column cannot be parsed", func() {
+			pkg := &packages.Package{
+				Errors: []packages.Error{
+					packages.Error{
+						Pos: "file:1:column",
+						Msg: "build error",
+					},
+				},
+			}
+			err := analyzer.ParseErrors(pkg)
+			Expect(err).Should(HaveOccurred())
+		})
+
+		It("should append  error to the same file", func() {
+			pkg := &packages.Package{
+				Errors: []packages.Error{
+					packages.Error{
+						Pos: "file:1:2",
+						Msg: "error1",
+					},
+					packages.Error{
+						Pos: "file:3:4",
+						Msg: "error2",
+					},
+				},
+			}
+			err := analyzer.ParseErrors(pkg)
+			Expect(err).ShouldNot(HaveOccurred())
+			_, _, errors := analyzer.Report()
+			Expect(len(errors)).To(Equal(1))
+			for _, ferr := range errors {
+				Expect(len(ferr)).To(Equal(2))
+				Expect(ferr[0].Line).To(Equal(1))
+				Expect(ferr[0].Column).To(Equal(2))
+				Expect(ferr[0].Err).Should(MatchRegexp(`error1`))
+				Expect(ferr[1].Line).To(Equal(3))
+				Expect(ferr[1].Column).To(Equal(4))
+				Expect(ferr[1].Err).Should(MatchRegexp(`error2`))
+			}
+		})
 	})
 })