-
Notifications
You must be signed in to change notification settings - Fork 496
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
GH-768: Plugin to detect Telegram bot tokens #808
Conversation
re.compile(r'\d{8,10}:[0-9A-Za-z_-]{35}'), | ||
] | ||
|
||
def verify(self, secret: str) -> VerifiedResult: # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at other RegexBasedDetector
, it doesn't look like you need a verify
method like you would in a BasePlugin
(see github_token.py
for reference).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provides the ability to decrease false positives by making an API call to a server to check whether a PotentialSecret is indeed real. This can be extremely useful for specific secrets (e.g. RegexBasedDetector subclasses) since we are able to determine which server to contact.
verify
method is optional but really good in decreasing False-positives
. Others RegexBasedDetector
plugins also has this method implemented where there is a way to verify whether the token is still active. (stripe.py, slack.py)
In my test in local:
- without verify: will report if any string matches the plugin regex ("is_verified": false)
- with verify: will only report if the token is still active ("is_verified": true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the same as #769?
Yes, looks same as 769. Didn't see it earlier. |
@Chandra158 I don't see any update on that PR, so I'd go with this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Tests