Skip to content

Remove bot token from debug stream #1095

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

Merged
merged 2 commits into from
May 4, 2020

Conversation

jacklul
Copy link
Collaborator

@jacklul jacklul commented May 4, 2020

This will remove bot token from debug http log streams, specifically from the URL path, I don't think there is any other place the token shows up?

So this:

> POST /bot123456789:AAHc2FVEadqyhkTYcUuG1RiYR4Hfgpwc/sendMessage HTTP/1.1

becomes this:

> POST /botBOT_TOKEN_REMOVED/sendMessage HTTP/1.1

Can be disabled on demand with TelegramLog::$remove_bot_token = false.
Having it enabled as default will make it safe for the people to post these logs for troubleshooting.

Regex is literally modified one from Telegram.php:172.
My original idea was to cache the token in the class to not run regex for each entry but people might be running some weird setups where they handle multiple bots in single script.

@jacklul jacklul requested a review from noplanman May 4, 2020 09:44
@noplanman
Copy link
Member

Thanks for the PR 👍

I agree with not caching the token, as that would also just add an extra variable somewhere.
Also, as this code is only executed when a request fails, it should be fine regarding performance.

Copy link
Member

@noplanman noplanman left a comment

Choose a reason for hiding this comment

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

I added a short note about the new variable to the docs.

@noplanman noplanman merged commit 2022f06 into php-telegram-bot:develop May 4, 2020
@jacklul jacklul deleted the hide_api_token branch July 27, 2020 11:42
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