-
Notifications
You must be signed in to change notification settings - Fork 481
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
Slack Plugin bug fixes #325
Conversation
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 🚢
Thanks Doctor Yoni!
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.
Do you mind adding a test case to show what these URLs look like for posterity?
https://github.com/Yelp/detect-secrets/blob/master/tests/plugins/slack_test.py#L35
Example of a webhook url (which I disabled immediately after creating) : https://hooks.slack.com/services/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 The original regex failed because the second secret part of the URL (starting with B) has 10 characters after the B, and not 8. |
We were getting report that slack custom application webhook will contain a different foramt for webhooks. In particualar the B+8 chars could become B+10 chars format User reprot https://ibm-cio-gi.slack.com/archives/CDMGJ9QG2/p1591018601323500?thread_ts=1590777088.301300&cid=CDMGJ9QG2 We also noticed the response error message is differnet, this commit tried to address that issue and allow us to catch webhook for custom Slack applications
We were getting report that slack custom application webhook will contain a different foramt for webhooks. In particualar the B+8 chars could become B+10 chars format User reprot https://ibm-cio-gi.slack.com/archives/CDMGJ9QG2/p1591018601323500?thread_ts=1590777088.301300&cid=CDMGJ9QG2 We also noticed the response error message is differnet, this commit tried to address that issue and allow us to catch webhook for custom Slack applications
Fixed the slack webhooks regex to support longer urls which didn't match the existing one.
Also, added 'no_text' as a valid response.text. Without it some valid urls hasn't been marked as verified and didn't appear in the baseline at all