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

fix(telegram_bot_token): Regex must match just bot tokens #878

Merged
merged 5 commits into from
Oct 7, 2024

Conversation

jfagoagas
Copy link
Contributor

@jfagoagas jfagoagas commented Sep 3, 2024

  • Please check if the PR fulfils these requirements
  • Tests for the changes have been added
  • Docs have been added / updated
  • All CI checks are green
(venv) ➜  time python -m pytest tests/plugins/telegram_token_test.py
================================================================================ test session starts =================================================================================
platform darwin -- Python 3.11.9, pytest-7.4.3, pluggy-1.5.0
rootdir: /private/tmp/detect-secrets
plugins: xdist-3.6.1
collected 6 items

tests/plugins/telegram_token_test.py ......                                                                                                                                    [100%]

================================================================================= 6 passed in 0.05s ==================================================================================
python -m pytest tests/plugins/telegram_token_test.py  0,21s user 0,04s system 96% cpu 0,262 total
  • What kind of change does this PR introduce?

This PR fixes a bug with the TelegramBotToken since some AWS ARNs were matched against it due to the regex configured in the deny_list.

  • What is the current behaviour?

The following AWS ARN arn:aws:sns:aaa:111122223333:aaaaaaaaaaaaaaaaaaassssssdddddddddddd matches against the TelegramBotToken when the verify option is not enabled, but it should not be needed to be verified since it is not.

This match because the regex is not enough restrictive from the beginning and the end.

  • What is the new behaviour (if this is a feature change)?
    Add the start-of-line ^ and end-of-line $ character to reduce false positives.

  • Does this PR introduce a breaking change?

No.

  • Other information:

@jfagoagas
Copy link
Contributor Author

Pinning @lorenzodb1 @KevinHock @domanchi to see if this can get a review soon. Thanks in advance!!

@@ -15,7 +15,7 @@ class TelegramBotTokenDetector(RegexBasedDetector):

denylist = [
# refs https://core.telegram.org/bots/api#authorizing-your-bot
re.compile(r'\d{8,10}:[0-9A-Za-z_-]{35}'),
re.compile(r'^\d{8,10}:[0-9A-Za-z_-]{35}$'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the Telegram docs, I found that

The token looks something like 123456:ABC-DEF1234ghIkl-zyx57W2v1u123ew11

which I assume means there's a specific format in terms of which characters will be upper case and which will be lower case. If that's correct, would you mind making the regex more accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @lorenzodb1 I'm not sure if that's accurate since they also states this in their documentation

The token is a string, like 110201543:AAHdqTcvCH1vGWJxfSeofSAs0K5PALDsaw, which is required to authorize the bot and send requests to the Bot API. Keep your token secure and store it safely, it can be used by anyone to control your bot.

https://core.telegram.org/bots/features#creating-a-new-bot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfagoagas jfagoagas requested a review from lorenzodb1 October 7, 2024 20:31
Copy link
Contributor

@lorenzodb1 lorenzodb1 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@lorenzodb1 lorenzodb1 merged commit 4627207 into Yelp:master Oct 7, 2024
15 checks passed
@jfagoagas jfagoagas deleted the fix_telegram_bot_token branch October 7, 2024 20:48
@jfagoagas
Copy link
Contributor Author

Hello @lorenzodb1 is there any ETA to have this included in a new release? Is something we do really need and we are trying not to vendor/fork the repository.

Thank you!

@jfagoagas
Copy link
Contributor Author

@lorenzodb1 @KevinHock @domanchi Any updates for a new release? Thanks!

@lorenzodb1
Copy link
Contributor

@jfagoagas we're planning to make a release soon. Either @jpdakran or myself will make one. Thank you for your patience.

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.

2 participants