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.

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

security fixes go part #964

Merged

Conversation

ivolzhevbt
Copy link
Contributor

continuation of #963

this part switches to go modules and fixes

vulnerability severity description
G304 MEDIUM While input is not controlled by user input it's an equal effort to sanitize input or suppress warning I went with sanitizing input
G703 MEDIUM The warning is about uncatched error from deferring closing a file. While nothing can be done if this will error, it's clearer to explicitly ignore it

Thank you for the maintaining vscode-kubernetes-tools and please let me know what do you think about it?

if err != nil {
panic(err)
}
defer f.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

While I understand the reasoning behind the change, this is the standard convention, and it performs exactly the same as your proposed change. This convention is described in the Effective Go guide, which is referred to when styling conventions are called into question.

This document gives tips for writing clear, idiomatic Go code. It augments the language specification, the Tour of Go, and How to Write Go Code, all of which you should read first.

I would suggest that Salus should be updated to follow the standard convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bacongobbler thanks for your feedback
Salus uses gosec under the hood, and I'm quite sure they might be not very cooperative regarding this rule.
at least their recommendation securego/gosec#512 here proposes to explicitly handle errors.

will silencing this particular warning be considered?

@itowlson
Copy link
Collaborator

itowlson commented Jul 6, 2021

Talked to @bacongobbler and he's happy with the revised PR so will merge - thanks!

@itowlson itowlson merged commit b88c7d9 into vscode-kubernetes-tools:master Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants