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

CheckAnalyzers doesn't seem to respect exclude or nosec (e.g. G602) #1175

Closed
imirkin opened this issue Jul 25, 2024 · 2 comments · Fixed by #1180
Closed

CheckAnalyzers doesn't seem to respect exclude or nosec (e.g. G602) #1175

imirkin opened this issue Jul 25, 2024 · 2 comments · Fixed by #1180

Comments

@imirkin
Copy link

imirkin commented Jul 25, 2024

I'm getting false positives with rule G602 and v2.20.0. I can't seem to find a way of supressing them. Adding to -exclude on the cmdline doesn't work, nor does adding // #nosec G602 comments.

As an aside, the code is fine... it's like

if len(x) == 1 {
  ...
} else if len(x) == 2 {
  if x[0].foo() == bar {
    return Pair{x[0], x[1]}
  } else if x[0].baz() == bar {
    return Pair{x[1], x[0]}
  }
} else {
  ...
}

and it generates errors for each of the 4 lines inside the len(x) == 2 block. But the fact that it's not excludable is most worrying.

Downgrading to v2.17.0 is not an option as I now get a panic when it is built with Go 1.22 (upgrading from Go 1.20).

@imirkin
Copy link
Author

imirkin commented Aug 1, 2024

Do you have any advice for how to fix this? The "analyzers" don't appear to be rules, and thus don't get the filtering benefits that rules do. Should a new section be added for analyzers separate from rules for tracking them? Should they all become part of the same list?

Rgvs added a commit to Rgvs/gosec that referenced this issue Aug 16, 2024
* This change does not exclude analyzers for inline comment
* Changed the expected issues count for G103, G109. Previously G115 has been included
* show analyzers IDs(G115, G602) in gosec usage help
* See securego#1175
Rgvs added a commit to Rgvs/gosec that referenced this issue Aug 19, 2024
* This change does not exclude analyzers for inline comment
* Changed the expected issues count for G103, G109 samples for test. Previously G115 has been included in the issue count
* Show analyzers IDs(G115, G602) in gosec usage help
* See securego#1175
ccojocar pushed a commit that referenced this issue Aug 20, 2024
* This change does not exclude analyzers for inline comment
* Changed the expected issues count for G103, G109 samples for test. Previously G115 has been included in the issue count
* Show analyzers IDs(G115, G602) in gosec usage help
* See #1175
@imirkin
Copy link
Author

imirkin commented Aug 20, 2024

@ccojocar The change added support for excluding globally, but not per-line/block #nosec comments. I think that's also important to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants