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

gosec: add G115 to default exclusions #4941

Closed
wants to merge 1 commit into from

Conversation

ldez
Copy link
Member

@ldez ldez commented Aug 22, 2024

Fixes #4935, #4939

Related to #4927

@ldez ldez added bug Something isn't working linter: update Update the linter implementation inside golangci-lint labels Aug 22, 2024
@ldez ldez requested review from alexandear and bombsimon August 22, 2024 15:51
@ldez ldez added this to the next milestone Aug 22, 2024
@ccoVeille
Copy link
Contributor

ccoVeille commented Aug 22, 2024

Thanks, it will help a lot until gosec discussion ends

Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

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

Hmm, is this a good idea? In general I really don't like exclude-use-default (all rules should be up to the user to configure) and to me that's legacy that should be removed. It's been brought up in previous discussions (such as #4323, #2239 and #456).

As far as I can tell, G115 is a valid check given it doesn't have false positives. So if this gets excluded by default and then gets fixed upstream we'd no longer warn about actual overflow and instead hide them from users that enables gosec.

And as far as I can tell from the linked issues, since we don't change existing exclusions due to backwards compatibility reasons that means that if we add this we can't remove it until a new major version.

Also, isn't this (partly) fixed by securego/gosec#1188 and more fixes are to come upstream?

@ldez
Copy link
Member Author

ldez commented Aug 22, 2024

We have no choice if we want to disable a rule inside gosec rather than using default exclusion.
I also always set it to false but at least we will reduce the noise for some users.

As far as I can tell, G115 is a valid check given it doesn't have false positives.

Currently, it has false positives: uint8 -> int64, uint8 -> int, etc.

But securego/gosec#1188 can reduce the problem.

@ldez
Copy link
Member Author

ldez commented Aug 22, 2024

Closed in favor of #4943

@ldez ldez closed this Aug 22, 2024
@ldez ldez added the won't fix This will not be worked on label Aug 22, 2024
@bombsimon
Copy link
Member

bombsimon commented Aug 22, 2024

We have no choice if we want to disable a rule inside gosec rather than using default exclusion.
I also always set it to false but at least we will reduce the noise for some users.

Alright, thanks for clarifying! I get that the noise can be annoying but it looks like gosec isn't one of the default enabled linters in golangci-lint so it's not something we need to suppress for users running without a config/default config at least. So given that the linter must be enabled by the user (specifically or with enable-all), an alternative that is easier to change could be to note this in the reference config.

I'm open to reconsidering this but if the manual bumping you just did solves the new false positives and the old ones stay the same (or will be fixed in the near future) I think it would be nice to avoid extending this list.

@ldez
Copy link
Member Author

ldez commented Aug 22, 2024

We have no choice if we want to disable a rule inside gosec rather than using default exclusion.

I want to add a more detailed explanation of this sentence:
gosec doesn't have a default configuration and doesn't allow a default configuration.
By using default exclusion, we let the user have the possibility to enable this rule without unnecessary complexity for us to handle interaction with include/exclude.

But, for now, we have another solution, so we are "safe" 😸

@ldez ldez removed this from the next milestone Aug 22, 2024
@proton-ab
Copy link
Contributor

proton-ab commented Aug 24, 2024

Quite ironic that this was marked as won't fix while you yourself recommend end-users to disable G115 (and people will - because it still ignores bound checks). In my opinion, telling users to ignore it creates more mess as users will inevitably forget to enable it back whenever its fixed upstream, while you (golangci-lint team) do follow gosec development closely (for regular updates) and could easily decide to re-enable it back once securego/gosec#1189 lands.

I really fail to see your logic in this decision.

@ldez
Copy link
Member Author

ldez commented Aug 24, 2024

I recommended disabling it as a workaround because of false positives just after the v1.60.2 release.
It was because it was the easiest solution before creating a new release of golangci-lint.

  1. v1.60.2 -> G115 is enabled by default inside gosec but there are false positives
  2. AFTER v1.60.2 and BEFORE v1.60.3, there are 2 solutions: gosec fixes or default exclusions.
    1. I suggested a workaround
    2. I created this PR to add G115 to the default exclusions
    3. I closed this PR BECAUSE gosec created a fix (Fix false positive in conversion overflow check from uint8/int8 type securego/gosec#1188)
  3. v1.60.3 -> the false positives of G115, that we are aware of, are fixed (G115: integer overflow conversion uint8 -> int64 securego/gosec#1185)

The logic in this decision is clear if you watch the chronology of the events.

while you (golangci-lint team) do follow gosec development closely (for regular updates)

There are over a hundred linters, so we can't follow every commit and every issue inside all those linters, we don't have the time to do it.

Dependabot creates PRs automatically to update linters when they create a release, we review the changes (not just the PR content: I read every linter changelogs and every linter diff).

Don't hesitate to donate to give us the time to watch every commit and every issue of the linters.

golangci-lint is a free and open-source project built by volunteers.

Open Collective backers and sponsors GitHub Sponsors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working linter: update Update the linter implementation inside golangci-lint won't fix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v1.60.2 reports integer overflow conversion int -> int32 for conversion like int32(group) with group is an int
4 participants