add exclude-regex-patterns #458
Conversation
- mimics the functionality of the exclude-entropy-patterns option, but for regex scans. this will help reduce false positives such as env variables for user info auth in URLs - note that the scope is forced to "line" and not configurable due to the way the regex scan is done compared to the entropy scan - will follow up with changelog update after creating PR - a few unrelated changes were necessary pre-commit linters to pass
jpaskhay
requested review from
sushantmimani,
rbailey-godaddy and
emayuri-godaddy
|
welp, guess that unit test failure wasn't related to my dev environment 😄 . will troubleshoot a bit more and hopefully have a fix soon |
| reason No String A plaintext reason the exclusion has been added | ||
| match-type No String ("search" or "match") Whether to perform a `search or match`_ regex operation | ||
| ============ ======== ============================ ============================================================== | ||
|
|
There was a problem hiding this comment.
You addressed this in the PR description, but it would be worth throwing in a line something like "The pattern is always tested against the entire line." (Or any other words that would explain why scope is a thing just above for entropy exclusions, but isn't here.)
tartufo/util.py
Outdated
| @@ -242,7 +257,7 @@ def _style_func(msg: str, *_: Any, **__: Any) -> str: | |||
| style_ok = style_error = style_warning = partial(_style_func) | |||
|
|
|||
|
|
|||
| def fail(msg: str, ctx: click.Context, code: int = 1) -> NoReturn: | |||
| def fail(msg: str, ctx: click.Context, code: int = 1) -> None: | |||
There was a problem hiding this comment.
I haven't gotten to looking at the unit test failures yet, but I was surprised to see this return type change. What is the motivation for this? Isn't it still true that fail() never returns control to the caller?
There was a problem hiding this comment.
i was getting a mypy failure on the pre-commit hook with:
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1
tartufo/util.py:261: error: Implicit return in function which does not return
i can try reverting this to see if it complains in CICD. this change as is has caused another side-effect linting issue, which i was surprised wasn't flagged locally in pre-commit
There was a problem hiding this comment.
I think the mypy failure you're getting now is due to the return-value change:
tartufo/commands/update_signatures.py:273: error: Incompatible return value type (got "Optional[GitRepoScanner]", expected "GitRepoScanner") [return-value]
It used to be that scanner was known not to be None by L273 because of the test at L250 combined with the knowledge that control would not return from the call to util.fail() on L252. Now mypy assumes control will return and that consequently scanner could still be None -- thus the error.
There was a problem hiding this comment.
yup, exactly. was just posting the previous mypy failure which led me to the NoReturn -> None change. wrapping up wordsmithing the documentation change and i'll push up a revert for this line along with it.
Co-authored-by: Scott Bailey <72747501+rbailey-godaddy@users.noreply.github.com>
…rt type hint change
|
i'm at a loss on the unit test failure. i did some troubleshooting locally, and the the way it is getting patched seems proper (same w/ the various other patches in |
Force cache flush before calling function with @lru_cache decorator; this ensures the return actually reflects the environment constructed by the unit test and not something left over from an unrelated test.
This mimics the functionality of the
exclude-entropy-patternsoption, but for regex scans. It is heavily based off #192 and other subsequent work around that feature. This will help reduce false positives such as env variables for user info auth in URLs.Note that the
scopeis forced tolineand not configurable due to the way the regex scan is done compared to the entropy scan.A few unrelated formatting changes were necessary for
pre-commitlinters to pass.I ran into issues trying to get the development environment setup properly (mainly poetry version differences). The new tests that were added passed, but I was running into 2 unrelated test failures -- not sure if related to the dev setup issues.
To help us get this pull request reviewed and merged quickly, please be sure to include the following items:
PR Type
What kind of change does this PR introduce?
Backward Compatibility
Is this change backward compatible with the most recently released version? Does it introduce changes which might change the user experience in any way? Does it alter the API in any way?
Issue Linking
What's new?
exclude-regex-patternsoption