Skip to content

Commit

Permalink
Refactor how ignored issues are tracked
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
ccojocar committed Oct 13, 2023
1 parent f338a98 commit 0ec6cd9
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 65 deletions.
76 changes: 40 additions & 36 deletions analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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{}
}

Expand Down Expand Up @@ -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)
}
}
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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{
Expand All @@ -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
}
Expand Down
21 changes: 1 addition & 20 deletions analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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())
Expand Down
4 changes: 2 additions & 2 deletions cmd/tlsconfig/tlsconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
16 changes: 11 additions & 5 deletions issue/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
4 changes: 2 additions & 2 deletions testutils/pkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 0ec6cd9

Please sign in to comment.