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

G101 false positives on basically any random string with 24 characters in it #1018

Closed
natefinch opened this issue Sep 20, 2023 · 5 comments
Closed

Comments

@natefinch
Copy link

natefinch commented Sep 20, 2023

We have a string that looks like this:

   name := "ournice-api-feat-enabled"

I couldn't for the life of me figure out why gosec was saying it looks like hardcoded credentials. Then I looked at recent changes and saw #971

The added regex is huge and arguably impossible to thoroughly test.

patternValue := "(?i)(^(.*[:;,](\\s)*)?[a-f0-9]{64}$)|(AIza[0-9A-Za-z-_]{35})|(^(.*[:;,](\\s)*)?github_pat_[a-zA-Z0-9]{22}_[a-zA-Z0-9]{59}$)|(^(.*[:;,](\\s)*)?[0-9a-zA-Z-_]{24}$)"

And that bit at the end there is the problem .... any string that happens to be 24 characters and matches [0-9a-zA-Z-_] will trigger this linter. Given that those are super common character restrictions, a ton of just random strings are going to coincidentally match, if they happen to be 24 characters long, like ENVIRONMENT_VARIABLE_ONE or an-amazon-s3-bucket-name.

This does not seem like a smart enough check, and will just result in people turning off the check, rather than actually finding hard coded credentials.

@natefinch
Copy link
Author

Closing since this was already fixed in #1009 ... but y'all should cut a new release :)

@ccojocar
Copy link
Member

Thanks for reporting this. As you already found out, it is fixed in master. I will release soon a new version.

@Corazu
Copy link

Corazu commented Sep 26, 2023

Thanks for reporting this. As you already found out, it is fixed in master. I will release soon a new version.

@ccojocar are we expecting a release anytime soon? We've got a project running into this bug pretty significantly (as well as G602 - which I had to turn off for now, but I don't want to turn G101 off completely)

@ccojocar
Copy link
Member

ccojocar commented Oct 5, 2023

@Corazu I'll release a version soon. Please could you try the master version and let me know if you face any issues. Thanks

@atc0005
Copy link

atc0005 commented Oct 5, 2023

@ccojocar

FWIW:

I checked three projects I work with that were flagged with G101 false positives. I re-tested with v2.17.0 and confirmed that the false positives were still reported.

I then tested again using the latest changes from master and did not receive those false positives.

refs:

$ go install github.com/securego/gosec/v2/cmd/gosec@master
go: downloading github.com/securego/gosec/v2 v2.17.1-0.20231005110022-d864a91884da
go: downloading github.com/securego/gosec v0.0.0-20200330112059-e030aa4f768b
go: downloading golang.org/x/tools v0.13.0
go: downloading github.com/google/uuid v1.3.1
go: downloading golang.org/x/sys v0.12.0

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

No branches or pull requests

5 participants
@ccojocar @natefinch @Corazu @atc0005 and others