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

Add support for #excluding specific rules #142

Merged
merged 14 commits into from Mar 9, 2018

Conversation

jonmcclintock
Copy link
Contributor

Create the ability to exclude specific rules, rather than all of them (with "#nosec"). Works like:

            cmd := exec.Command("sh", "-c", os.Getenv("FOO")) // #exclude !G001

You can specify an arbitrary number of exclusions, and they have the same scoping semantics as "#nosec". You can also add comments to explain your exclusions:

            cmd := exec.Command("sh", "-c", os.Getenv("FOO")) // #exclude !G001: Doesn't apply here

@jonmcclintock
Copy link
Contributor Author

Have you had a chance to take a look at this?

@gcmurphy
Copy link
Member

Sorry I've not been getting many cycles to OSS work. I'd like to get #146 merged before this lands.

Awesome work though.

@jonmcclintock
Copy link
Contributor Author

jonmcclintock commented Dec 13, 2017 via email

@gcmurphy
Copy link
Member

gcmurphy commented Feb 8, 2018

@jonmcclintock not sure if i pinged at you about the rebasing this. if you don't have time I can look at doing it.

@ccojocar
Copy link
Member

I am also interested in this feature. I think we need only to exclude the rules which produce false positive results. Do you see any use case which requires specific inclusion? Typically all rules are enabled.

What do you think about this syntax since there is already #nosec directive:

#sec !G101,!G102

The build constrains in Go could be a source of inspiration: https://golang.org/pkg/go/build/

@jonmcclintock
Copy link
Contributor Author

jonmcclintock commented Feb 16, 2018 via email

analyzer.go Outdated
matches := re.FindAllStringSubmatch(group.Text(), -1)

// Find the rule IDs to ignore.
ignores := make([]string, 0)

Choose a reason for hiding this comment

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

can probably use "var ignores []string" instead

@jonmcclintock
Copy link
Contributor Author

Finally got around to rebasing this. Sorry for the delay.

@jonmcclintock
Copy link
Contributor Author

@cosmincojocar I'm not wed to "exclude" by any means, and "sec" may make more sense. I'll leave it to @gcmurphy to figure out what fits in with his vision of things...

@gcmurphy
Copy link
Member

gcmurphy commented Mar 8, 2018

This is an awesome change and thanks for getting back to it. I'm wondering if it is worth tweaking it a little bit so it is more of an extension to the #nosec directive.

Effectively: #nosec = Ignore all rules
And: #nosec G101, G202 = ignore specifically these rules.

@jonmcclintock thoughts? Would that be a difficult change to make? If so I'm happy to merge this as is and iterate on it.

@jonmcclintock
Copy link
Contributor Author

Wouldn't be that hard at all, just a couple of quick changes here:

https://github.com/GoASTScanner/gas/pull/142/files#diff-21c43657b6fe53d9635ac2f42896ee61R152

-Jon

@jonmcclintock
Copy link
Contributor Author

Pushed an updated PR with the changed exclusion syntax.

@gcmurphy gcmurphy merged commit 429ac07 into securego:master Mar 9, 2018
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

4 participants