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

Handling ini files #13

Merged
merged 3 commits into from
Apr 6, 2018
Merged

Handling ini files #13

merged 3 commits into from
Apr 6, 2018

Conversation

domanchi
Copy link
Contributor

@domanchi domanchi commented Apr 5, 2018

First part of fix for #12.

I currently just assume that config files in this format end with .ini. Technically, they can end with anything, but maybe we can just keep adding to INI_FILE_EXTENSIONS?

@domanchi domanchi requested a review from KevinHock April 5, 2018 00:14
Copy link
Collaborator

@KevinHock KevinHock left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -22,6 +32,14 @@ def __init__(self, charset, limit, *args):
# TODO: Update for not just python comments?
self.ignore_regex = re.compile(r'# ?pragma: ?whitelist[ -]secret')

def analyze(self, file, filename):
# Heuristically determine whether file is an ini-formatted file.
for ext in INI_FILE_EXTENSIONS:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could maybe run file on them and see if it comes back ASCII text, might get expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ini styled formats is definitely a subset of ASCII text, so that solution may lead to false positives.

Eg.

$ file test_data/config.ini
test_data/config.ini: ASCII text
$ file test_data/baseline.file
test_data/baseline.file: ASCII text

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm kind of ambivalent, right now it's easier to exclude files than include them in the tuple, which makes me lean towards including more by default/making the false-p/false-n rate customizable. Nothing is really set in stone though, we can always change it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Let me jiggle it up a bit.

@domanchi domanchi merged commit ce9232e into master Apr 6, 2018
@domanchi domanchi deleted the handling-ini-files branch April 6, 2018 21:51
jimmyhlee94 pushed a commit to jimmyhlee94/detect-secrets that referenced this pull request Aug 19, 2021
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

2 participants