[Keyword Plugin] Various accuracy improvements #229
Conversation
These are no longer necessary as we made quotes required in the last commit.
- 🐍 Alphabetize variables - 🐍 Replace `elif` chain with a dict - 🐍 Update QUOTES_REQUIRED_FILE_EXTENSIONS in the test
e.g. "secret": "{secret}"
- Change keyword test to not have the positives start with { and end with }
- 🐍 Make .pyi files have FileType.PYTHON
- Add FP check for '/etc/' in secret - Add FP heuristic for secrets that look like directories - Add <secret> FP heuristic for .example files - Add more to FALSE_POSITIVES dict - Make all non-quotes required filetypes skip secrets starting with a $ - Make quotes required for Terraform files
| ): | ||
| return FileType.YAML | ||
| return FileType.OTHER | ||
| _, file_extension = os.path.splitext(filename) |
There was a problem hiding this comment.
Does this handle my.weird.filename?
There was a problem hiding this comment.
Yup
>>> os.path.splitext('my.weirdo.filename')
('my.weirdo', '.filename')There was a problem hiding this comment.
Oh right, and does this work with no file extension?
There was a problem hiding this comment.
Oh it does, the get call in the next line has a default
There was a problem hiding this comment.
For posterity, it'll be '' and then get set to FileType.OTHER
(I had an internal 🤔 about setdefault vs. the .get fallback but 🤷♂ I kinda liked the way it looked.)
| 'account_password', | ||
| 'api_key', |
There was a problem hiding this comment.
Why is this in both DENYLIST and FALSE_POSITIVES?
There was a problem hiding this comment.
So this is for the case wherein silly code does magical_api_key: api_key, or something to that effect, normally in a test.
DENYLIST will catch the fact that it could be an API key from magical_api_key on the LHS, but then FALSE_POSITIVES will check the RHS and cause us not to alert on it.
| 'password', | ||
| 'password)', | ||
| 'password))', |
There was a problem hiding this comment.
Just to clarify, does this mean a word that contains password)) will be marked as a false-positive?
There was a problem hiding this comment.
The short-answer is, only if it == password)), not contains.
The FALSE_POSITIVES set is for RHS values normally resulting in false-positives, you could make your real password password)), but it's more likely that e.g. I made some mistake in not requiring quotes for that particular filetype, or haven't made a custom regex for that language when I should, this set patches over certain things like that.
| FOLLOWED_BY_EQUAL_SIGNS_OPTIONAL_BRACKETS_OPTIONAL_AT_SIGN_QUOTES_REQUIRED_REGEX = re.compile( | ||
| # e.g. my_password = "bar" | ||
| # e.g. my_password = @"bar" | ||
| # e.g. my_password[] = "bar"; |
There was a problem hiding this comment.
Based on below this is Objective-C. What does this actually do in Objective-C? (I couldn't Google it)
There was a problem hiding this comment.
@ means it is a constant
[] means it is a C style array e.g. char myString[] = "This is a C character array";
Something I do not handle yet, but can ticket, are strings like the following
password = [str1 stringByAppendingFormat:@"World"];
NSMutableString *password = [NSMutableString stringWithString:@"This string is mutable"];
Prior to this PR, we'd capture e.g. [str1 and [NSMutableString, as secrets, which was 🤐
| ) | ||
| ) or lowered_secret in FALSE_POSITIVES | ||
| or lowered_secret.count('/') >= 3 |
There was a problem hiding this comment.
Good catch, added comment in 5de43e2
🔭 ➕ ➕