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 whitelist directive regexes to match against inline comment syntaxes in more languages #105

Merged
merged 5 commits into from
Dec 21, 2018
Merged

add whitelist directive regexes to match against inline comment syntaxes in more languages #105

merged 5 commits into from
Dec 21, 2018

Conversation

joshuarli
Copy link
Contributor

Also:

  • improved the base regex to account for leading whitespace before the inline comment start character, and trailing whitespace until EOL.
  • some docs tweaks

The only suitable and pre-existing place I could find was in tests/plugins/high_entropy_strings_test.py. Ideally, tests should be moved to (and should only exist exclusively in) tests/plugins/base_test.py. The redundant logic to detect whether or not to ignore a file should also be removed from YamlFileParser and the HighEntropyStringsPlugin. The latter should be refactored so that the logic in analyze_string is run by BasePlugin's analyze.

@joshuarli
Copy link
Contributor Author

joshuarli commented Dec 21, 2018

Ah, actually the logic was being run twice - once in HighEntropyStringsPlugin.analyze_string and another in HighEntropyStringsPlugin.analyze's self._analyze_yaml_file. The former can be removed without consequence.

@joshuarli
Copy link
Contributor Author

So what remains here is that the IniFileParser should implement something like YamlFileParser's get_ignored_lines. And a follow-up to that would be to make a base parser class for those to consolidate that logic. Then more thought should be put into language-specific parsers, and then I think the catch-all list of whitelist directive regexes should instead be split up into those parsers. That would be a bit of initial boilerplate, but it would make tests neater and make language support more explicit and modular.

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.

Great stuff

@KevinHock KevinHock merged commit 7fdc3f1 into Yelp:master Dec 21, 2018
@joshuarli joshuarli deleted the more-whitelist-pragma-regexes branch December 21, 2018 21:36
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

3 participants