From 0ec6cd95d7bf02aef4ec2786e884868e0044875b Mon Sep 17 00:00:00 2001 From: Cosmin Cojocar Date: Fri, 13 Oct 2023 14:04:21 +0200 Subject: [PATCH] Refactor how ignored issues are tracked Track ignored issues using file location instead of a AST node. There are issues linked to a different AST node than the original node used to start the scan. Signed-off-by: Cosmin Cojocar --- analyzer.go | 76 ++++++++++++++++++++------------------ analyzer_test.go | 21 +---------- cmd/tlsconfig/tlsconfig.go | 4 +- issue/issue.go | 16 +++++--- testutils/pkg.go | 4 +- 5 files changed, 56 insertions(+), 65 deletions(-) diff --git a/analyzer.go b/analyzer.go index 07e1a4785b..5981c9a860 100644 --- a/analyzer.go +++ b/analyzer.go @@ -57,6 +57,12 @@ const aliasOfAllRules = "*" var generatedCodePattern = regexp.MustCompile(`^// Code generated .* DO NOT EDIT\.$`) +// ignoreLocation keeps the location of an ignored rule +type ignoreLocation struct { + file string + line string +} + // The Context is populated with data parsed from the source code as it is scanned. // It is passed through to all rule functions as they are called. Rules may use // this data in conjunction with the encountered AST node. @@ -69,7 +75,7 @@ type Context struct { Root *ast.File Imports *ImportTracker Config Config - Ignores []map[string][]issue.SuppressionInfo + Ignores map[ignoreLocation]map[string][]issue.SuppressionInfo PassedValues map[string]interface{} } @@ -360,7 +366,7 @@ func (gosec *Analyzer) CheckAnalyzers(pkg *packages.Package) { if result != nil { if passIssues, ok := result.([]*issue.Issue); ok { for _, iss := range passIssues { - gosec.updateIssues(iss, false, []issue.SuppressionInfo{}) + gosec.updateIssues(iss) } } } @@ -521,10 +527,8 @@ func (gosec *Analyzer) ignore(n ast.Node) map[string]issue.SuppressionInfo { // Visit runs the gosec visitor logic over an AST created by parsing go code. // Rule methods added with AddRule will be invoked as necessary. func (gosec *Analyzer) Visit(n ast.Node) ast.Visitor { - ignores, ok := gosec.updateIgnoredRules(n) - if !ok { - return gosec - } + // Update any potentially ignored rules at the node location + gosec.updateIgnoredRules(n) // Using ast.File instead of ast.ImportSpec, so that we can track all imports at once. switch i := n.(type) { @@ -533,56 +537,55 @@ func (gosec *Analyzer) Visit(n ast.Node) ast.Visitor { } for _, rule := range gosec.ruleset.RegisteredFor(n) { - suppressions, ignored := gosec.updateSuppressions(rule.ID(), ignores) issue, err := rule.Match(n, gosec.context) if err != nil { file, line := GetLocation(n, gosec.context) file = path.Base(file) gosec.logger.Printf("Rule error: %v => %s (%s:%d)\n", reflect.TypeOf(rule), err, file, line) } - gosec.updateIssues(issue, ignored, suppressions) + gosec.updateIssues(issue) } return gosec } -func (gosec *Analyzer) updateIgnoredRules(n ast.Node) (map[string][]issue.SuppressionInfo, bool) { - if n == nil { - if len(gosec.context.Ignores) > 0 { - gosec.context.Ignores = gosec.context.Ignores[1:] - } - return nil, false - } - // Get any new rule exclusions. +func (gosec *Analyzer) updateIgnoredRules(n ast.Node) { ignoredRules := gosec.ignore(n) - - // Now create the union of exclusions. - ignores := map[string][]issue.SuppressionInfo{} - if len(gosec.context.Ignores) > 0 { - for k, v := range gosec.context.Ignores[0] { - ignores[k] = v + if len(ignoredRules) > 0 { + if gosec.context.Ignores == nil { + gosec.context.Ignores = make(map[ignoreLocation]map[string][]issue.SuppressionInfo) + } + line := issue.GetLine(gosec.context.FileSet.File(n.Pos()), n) + ignoreLocation := ignoreLocation{ + file: gosec.context.FileSet.File(n.Pos()).Name(), + line: line, + } + current, ok := gosec.context.Ignores[ignoreLocation] + if !ok { + current = map[string][]issue.SuppressionInfo{} } + for r, s := range ignoredRules { + if current[r] == nil { + current[r] = []issue.SuppressionInfo{} + } + current[r] = append(current[r], s) + } + gosec.context.Ignores[ignoreLocation] = current } +} - for ruleID, suppression := range ignoredRules { - ignores[ruleID] = append(ignores[ruleID], suppression) +func (gosec *Analyzer) getSuppressionsAtLineInFile(file string, line string, id string) ([]issue.SuppressionInfo, bool) { + ignores, ok := gosec.context.Ignores[ignoreLocation{file: file, line: line}] + if !ok { + ignores = make(map[string][]issue.SuppressionInfo) } - // Push the new set onto the stack. - gosec.context.Ignores = append([]map[string][]issue.SuppressionInfo{ignores}, gosec.context.Ignores...) - - return ignores, true -} - -func (gosec *Analyzer) updateSuppressions(id string, ignores map[string][]issue.SuppressionInfo) ([]issue.SuppressionInfo, bool) { - // Check if all rules are ignored. + // Check if the rule was specifically suppressed at this location. generalSuppressions, generalIgnored := ignores[aliasOfAllRules] - // Check if the specific rule is ignored ruleSuppressions, ruleIgnored := ignores[id] - ignored := generalIgnored || ruleIgnored suppressions := append(generalSuppressions, ruleSuppressions...) - // Track external suppressions. + // Track external suppressions of this rule. if gosec.ruleset.IsRuleSuppressed(id) { ignored = true suppressions = append(suppressions, issue.SuppressionInfo{ @@ -593,8 +596,9 @@ func (gosec *Analyzer) updateSuppressions(id string, ignores map[string][]issue. return suppressions, ignored } -func (gosec *Analyzer) updateIssues(issue *issue.Issue, ignored bool, suppressions []issue.SuppressionInfo) { +func (gosec *Analyzer) updateIssues(issue *issue.Issue) { if issue != nil { + suppressions, ignored := gosec.getSuppressionsAtLineInFile(issue.File, issue.Line, issue.RuleID) if gosec.showIgnored { issue.NoSec = ignored } diff --git a/analyzer_test.go b/analyzer_test.go index 937a2e3364..d5f45c886f 100644 --- a/analyzer_test.go +++ b/analyzer_test.go @@ -743,25 +743,6 @@ var _ = Describe("Analyzer", func() { Expect(issues[0].Suppressions[0].Justification).To(Equal("")) }) - It("should track multiple suppressions if the violation is suppressed by both #nosec and #nosec RuleList", func() { - sample := testutils.SampleCodeG101[0] - source := sample.Code[0] - analyzer.LoadRules(rules.Generate(false, rules.NewRuleFilter(false, "G101")).RulesInfo()) - - nosecPackage := testutils.NewTestPackage() - defer nosecPackage.Close() - nosecSource := strings.Replace(source, "}", "} //#nosec G101 -- Justification", 1) - nosecSource = strings.Replace(nosecSource, "func", "//#nosec\nfunc", 1) - nosecPackage.AddFile("pwd.go", nosecSource) - err := nosecPackage.Build() - Expect(err).ShouldNot(HaveOccurred()) - err = analyzer.Process(buildTags, nosecPackage.Path) - Expect(err).ShouldNot(HaveOccurred()) - issues, _, _ := analyzer.Report() - Expect(issues).To(HaveLen(sample.Errors)) - Expect(issues[0].Suppressions).To(HaveLen(2)) - }) - It("should not report an error if the rule is not included", func() { sample := testutils.SampleCodeG101[0] source := sample.Code[0] @@ -807,7 +788,7 @@ var _ = Describe("Analyzer", func() { nosecPackage := testutils.NewTestPackage() defer nosecPackage.Close() - nosecSource := strings.Replace(source, "}", "} //#nosec G101 -- Justification", 1) + nosecSource := strings.Replace(source, "password := \"f62e5bcda4fae4f82370da0c6f20697b8f8447ef\"", "password := \"f62e5bcda4fae4f82370da0c6f20697b8f8447ef\" //#nosec G101 -- Justification", 1) nosecPackage.AddFile("pwd.go", nosecSource) err := nosecPackage.Build() Expect(err).ShouldNot(HaveOccurred()) diff --git a/cmd/tlsconfig/tlsconfig.go b/cmd/tlsconfig/tlsconfig.go index 325c883bd4..096fecd2d4 100644 --- a/cmd/tlsconfig/tlsconfig.go +++ b/cmd/tlsconfig/tlsconfig.go @@ -187,7 +187,7 @@ func main() { } outputPath := filepath.Join(dir, *outputFile) - if err := os.WriteFile(outputPath, src, 0o644); err != nil { + if err := os.WriteFile(outputPath, src, 0o644); err != nil /*#nosec G306*/ { log.Fatalf("Writing output: %s", err) - } //#nosec G306 + } } diff --git a/issue/issue.go b/issue/issue.go index db4d630fab..1000b20423 100644 --- a/issue/issue.go +++ b/issue/issue.go @@ -178,11 +178,7 @@ func codeSnippetEndLine(node ast.Node, fobj *token.File) int64 { // New creates a new Issue func New(fobj *token.File, node ast.Node, ruleID, desc string, severity, confidence Score) *Issue { name := fobj.Name() - start, end := fobj.Line(node.Pos()), fobj.Line(node.End()) - line := strconv.Itoa(start) - if start != end { - line = fmt.Sprintf("%d-%d", start, end) - } + line := GetLine(fobj, node) col := strconv.Itoa(fobj.Position(node.Pos()).Column) var code string @@ -217,3 +213,13 @@ func (i *Issue) WithSuppressions(suppressions []SuppressionInfo) *Issue { i.Suppressions = suppressions return i } + +// GetLine returns the line number of a given ast.Node +func GetLine(fobj *token.File, node ast.Node) string { + start, end := fobj.Line(node.Pos()), fobj.Line(node.End()) + line := strconv.Itoa(start) + if start != end { + line = fmt.Sprintf("%d-%d", start, end) + } + return line +} diff --git a/testutils/pkg.go b/testutils/pkg.go index 2ad07965b5..6dcf79ec03 100644 --- a/testutils/pkg.go +++ b/testutils/pkg.go @@ -53,9 +53,9 @@ func (p *TestPackage) write() error { return nil } for filename, content := range p.Files { - if e := os.WriteFile(filename, []byte(content), 0o644); e != nil { + if e := os.WriteFile(filename, []byte(content), 0o644); e != nil /* #nosec G306 */ { return e - } //#nosec G306 + } } p.onDisk = true return nil