Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(misconf): don't shift ignore rule related to code #6708

Merged
merged 2 commits into from
May 17, 2024
Merged

fix(misconf): don't shift ignore rule related to code #6708

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6f2375
fix(misconf): don't shift ignore rule related to code
nikpivkin May 16, 2024
c572cca
test: add case
nikpivkin May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 26 additions & 16 deletions pkg/iac/ignore/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"strings"
"time"

"github.com/samber/lo"
simar7 marked this conversation as resolved.
Show resolved Hide resolved

"github.com/aquasecurity/trivy/pkg/iac/types"
"github.com/aquasecurity/trivy/pkg/log"
)
Expand Down Expand Up @@ -36,23 +38,34 @@ func Parse(src, path string, parsers ...RuleSectionParser) Rules {
func parseLine(line string, rng types.Range, parsers []RuleSectionParser) []Rule {
var rules []Rule

sections := strings.Split(strings.TrimSpace(line), " ")
for _, section := range sections {
section := strings.TrimSpace(section)
section = strings.TrimLeftFunc(section, func(r rune) bool {
parts := strings.Split(strings.TrimSpace(line), " ")
parts = lo.FilterMap(parts, func(part string, _ int) (string, bool) {
part = strings.TrimSpace(part)
part = strings.TrimLeftFunc(part, func(r rune) bool {
return r == '#' || r == '/' || r == '*'
})

section, exists := hasIgnoreRulePrefix(section)
return part, part != ""
})

for i, part := range parts {
part, exists := hasIgnoreRulePrefix(part)
if !exists {
continue
}

rule, err := parseComment(section, rng, parsers)
sections, err := parseRuleSections(part, rng, parsers)
if err != nil {
log.Debug("Failed to parse rule", log.String("range", rng.String()), log.Err(err))
continue
}

rule := Rule{
rng: rng,
isStartLine: i == 0 || (len(rules) > 0 && rules[0].isStartLine),
sections: sections,
}

rules = append(rules, rule)
}

Expand All @@ -72,11 +85,8 @@ func hasIgnoreRulePrefix(s string) (string, bool) {
return "", false
}

func parseComment(input string, rng types.Range, parsers []RuleSectionParser) (Rule, error) {
rule := Rule{
rng: rng,
sections: make(map[string]any),
}
func parseRuleSections(input string, rng types.Range, parsers []RuleSectionParser) (map[string]any, error) {
sections := make(map[string]any)

parsers = append(parsers, &expiryDateParser{
rng: rng,
Expand All @@ -93,7 +103,7 @@ func parseComment(input string, rng types.Range, parsers []RuleSectionParser) (R
StringMatchParser{SectionKey: "id"},
}
if idParser.Parse(val) {
rule.sections[idParser.Key()] = idParser.Param()
sections[idParser.Key()] = idParser.Param()
}
}

Expand All @@ -103,16 +113,16 @@ func parseComment(input string, rng types.Range, parsers []RuleSectionParser) (R
}

if parser.Parse(val) {
rule.sections[parser.Key()] = parser.Param()
sections[parser.Key()] = parser.Param()
}
}
}

if _, exists := rule.sections["id"]; !exists {
return Rule{}, errors.New("rule section with the `ignore` key is required")
if _, exists := sections["id"]; !exists {
return nil, errors.New("rule section with the `ignore` key is required")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any callers of this func that we need to add a check for when we return nil here? I only see this and it seems like this is covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if comment parsing fails, we skip it https://github.com/aquasecurity/trivy/pull/6708/files#diff-c1fe13f24e97f852991501a2535948a2ff13ef4e36564313e85754d8f9c6f35eR58-R60

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I guess it is subjective as I only tend to return nil for structs that are passed around by reference. But I'm fine with this.

}

return rule, nil
return sections, nil
}

type StringMatchParser struct {
Expand Down
16 changes: 11 additions & 5 deletions pkg/iac/ignore/rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,16 @@ func (r Rules) shift() {
)

for i := len(r) - 1; i > 0; i-- {
currentIgnore, nextIgnore := r[i], r[i-1]
currentRule, prevRule := r[i], r[i-1]

if !prevRule.isStartLine {
continue
}

if currentRange == nil {
currentRange = &currentIgnore.rng
currentRange = &currentRule.rng
}
if nextIgnore.rng.GetStartLine()+1+offset == currentIgnore.rng.GetStartLine() {
if prevRule.rng.GetStartLine()+1+offset == currentRule.rng.GetStartLine() {
r[i-1].rng = *currentRange
offset++
} else {
Expand All @@ -46,8 +51,9 @@ func (r Rules) shift() {

// Rule represents a rule for ignoring vulnerabilities.
type Rule struct {
rng types.Range
sections map[string]any
rng types.Range
isStartLine bool
sections map[string]any
}

func (r Rule) ignore(m types.Metadata, ids []string, ignorers map[string]Ignorer) bool {
Expand Down
35 changes: 35 additions & 0 deletions pkg/iac/ignore/rule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,41 @@ func TestRules_Ignore(t *testing.T) {
},
shouldIgnore: false,
},
{
name: "multiple ignore rules on the same line",
src: `test #trivy:ignore:rule-1
test #trivy:ignore:rule-2
`,
args: args{
metadata: metadataWithLine(filename, 1),
ids: []string{"rule-1"},
},
shouldIgnore: true,
},
{
name: "multiple ignore rules on the same line",
src: `# trivy:ignore:rule-1
# trivy:ignore:rule-2
test #trivy:ignore:rule-3
`,
args: args{
metadata: metadataWithLine(filename, 3),
ids: []string{"rule-1"},
},
shouldIgnore: true,
},
{
name: "multiple ignore rules on the same line",
src: `# trivy:ignore:rule-1 # trivy:ignore:rule-2
# trivy:ignore:rule-3
test #trivy:ignore:rule-4
`,
args: args{
metadata: metadataWithLine(filename, 3),
ids: []string{"rule-2"},
},
shouldIgnore: true,
},
}

for _, tt := range tests {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Since we can pass a slice of ids of rules, should we create a test case as such:

		{
			name: "multiple ignore rules on the same line",
			src: `# trivy:ignore:rule-1
# trivy:ignore:rule-2
test #trivy:ignore:rule-3
`,
			args: args{
				metadata: metadataWithLine(filename, 3),
				ids:      []string{"rule-1", "rule-2", "rule-3", "non-existing-rule"},
			},
			shouldIgnore: true,
		},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done c572cca

Expand Down