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

gosec: exclude G601 and G113 #4736

Closed
wants to merge 1 commit into from
Closed

gosec: exclude G601 and G113 #4736

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 41 additions & 24 deletions pkg/golinters/gosec/gosec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,19 +150,6 @@ func toGosecConfig(settings *config.GoSecSettings) gosec.Config {
return conf
}

func convertScoreToString(score issue.Score) string {
switch score {
case issue.Low:
return "low"
case issue.Medium:
return "medium"
case issue.High:
return "high"
default:
return ""
}
}

// based on https://github.com/securego/gosec/blob/47bfd4eb6fc7395940933388550b547538b4c946/config.go#L52-L62
func convertGosecGlobals(globalOptionFromConfig any, conf gosec.Config) {
globalOptionMap, ok := globalOptionFromConfig.(map[string]any)
Expand All @@ -179,17 +166,35 @@ func convertGosecGlobals(globalOptionFromConfig any, conf gosec.Config) {
func gosecRuleFilters(includes, excludes []string) []rules.RuleFilter {
var filters []rules.RuleFilter

if len(includes) > 0 {
filters = append(filters, rules.NewRuleFilter(false, includes...))
cleanIncludes := cleanRules(includes)
Copy link
Member

Choose a reason for hiding this comment

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

Should we really remove the possibility to enable them even if a user wants to? I get we remove them from default rules if no config is provided but if explicitly enabled by a config I would expect them to be enabled, the extra time isn't necessarily a blocker for everyone.

Copy link
Member Author

Choose a reason for hiding this comment

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

the extra time isn't necessarily a blocker for everyone.

We are talking about a duration multiplied by at least 4 (milliseconds becomes seconds).

Should we really remove the possibility to enable them even if a user wants to?

We cannot really play with "default" because the current problem impacts users who already explicitly enable those rules.

This fix is a temporary fix, we will create the real fix when gosec allows us to handle the Go version.

Copy link
Member Author

@ldez ldez May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you feel uncomfortable with this PR, we can downgrade to v2.19.0.
But technically, the v2.19.0 contains a bug with how to handle the Go version on the same rules.

Copy link
Member

Choose a reason for hiding this comment

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

We are talking about a duration multiplied by at least 4 (milliseconds becomes seconds).

Yeah I saw your table but to me 10s isn't a blocker in my CI e.g. so if I was using this check I would be annoyed it wasn't up to me to opt in.

This fix is a temporary fix, we will create the real fix when gosec allows us to handle the Go version.

Got that, I guess it's not the end of the world and for people who reeeally care they can probably stick to an older version of golangci-lint.


if len(cleanIncludes) > 0 {
filters = append(filters, rules.NewRuleFilter(false, cleanIncludes...))
}

if len(excludes) > 0 {
filters = append(filters, rules.NewRuleFilter(true, excludes...))
cleanExcludes := cleanRules(excludes)
cleanExcludes = append(cleanExcludes, "G601", "G113")

if len(cleanExcludes) > 0 {
filters = append(filters, rules.NewRuleFilter(true, cleanExcludes...))
}

return filters
}

// code borrowed from https://github.com/securego/gosec/blob/69213955dacfd560562e780f723486ef1ca6d486/cmd/gosec/main.go#L264-L276
func filterIssues(issues []*issue.Issue, severity, confidence issue.Score) []*issue.Issue {
res := make([]*issue.Issue, 0)

for _, i := range issues {
if i.Severity >= severity && i.Confidence >= confidence {
res = append(res, i)
}
}

return res
}

// code borrowed from https://github.com/securego/gosec/blob/69213955dacfd560562e780f723486ef1ca6d486/cmd/gosec/main.go#L250-L262
func convertToScore(str string) (issue.Score, error) {
str = strings.ToLower(str)
Expand All @@ -205,15 +210,27 @@ func convertToScore(str string) (issue.Score, error) {
}
}

// code borrowed from https://github.com/securego/gosec/blob/69213955dacfd560562e780f723486ef1ca6d486/cmd/gosec/main.go#L264-L276
func filterIssues(issues []*issue.Issue, severity, confidence issue.Score) []*issue.Issue {
res := make([]*issue.Issue, 0)
func convertScoreToString(score issue.Score) string {
switch score {
case issue.Low:
return "low"
case issue.Medium:
return "medium"
case issue.High:
return "high"
default:
return ""
}
}

for _, i := range issues {
if i.Severity >= severity && i.Confidence >= confidence {
res = append(res, i)
func cleanRules(ruleNames []string) []string {
var cleanRuleNames []string
for _, ruleName := range ruleNames {
if ruleName == "G601" || ruleName == "G113" {
continue
}
cleanRuleNames = append(cleanRuleNames, ruleName)
}

return res
return cleanRuleNames
}
Loading