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

Image url reveals bot's token #115

Closed
alcir opened this issue Jan 11, 2019 · 11 comments
Closed

Image url reveals bot's token #115

alcir opened this issue Jan 11, 2019 · 11 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation
Milestone

Comments

@alcir
Copy link

alcir commented Jan 11, 2019

Imgur is not configured.
Sending an image on Telegram, the API URL appears on IRC.
Such URL contains the token of the bot, that should be keep private.

'Untitled Image' uploaded by XXXXXX: https://api.telegram.org/file/botXXXXXXXXXXX:XXXXXXXXXXXXXXXXXX/photos/file_1.jpg

Is this the only way to use the Telegram API?
There is a way to disable images on IRC, if Imgur is not configured?

Thanks.

@xforever1313
Copy link
Member

My guess is that files are also going to be impacted by this.

I think the best way to do this is add options to disable Photos and Files being sent via URL so we don't expose the bot API token. These config items should probably be set to false by default, and have a comment with a warning about what happens if they are set to true (maybe even go so far as hiding them so its developers only). I could see someone wanting to set this to true if they are testing the bot out, or if they are in a private IRC channel and trust everyone inside of it.

The good news is this should be straight forward to do since all of the TgHandlers take in an enabled boolean. We can just pass in the true/false from the config directly into the constructor.

@jwflory jwflory added bug Something isn't working priority:high labels Jan 15, 2019
@Tjzabel Tjzabel added this to the v1.3 milestone Jan 27, 2019
@jwflory
Copy link
Member

jwflory commented Feb 2, 2019

@alcir @xforever1313 We suggested handling this issue in two ways.

Option 1

  1. Enabling Imgur support by default (with the generic API key provided by upstream library)
  2. Supplementing docs to recommend getting your own API key to replace the default

This way, the Telegram API token is never exposed by accident.

Option 2

  1. Better documenting behavior of not using Telegram (i.e. API token will be exposed and this is a risk you are comfortable taking)

What do y'all think? @RITlug/teleirc-developers-commit-access

@jwflory jwflory added documentation Improvements or additions to documentation priority:crit and removed priority:high labels Feb 2, 2019
@jwflory
Copy link
Member

jwflory commented Feb 2, 2019

Discussed in 2019-02-02 RITlug developers' meeting. This issue is targeted for the v1.3 milestone, estimated to release on March 2nd, 2019.


@Tjzabel is looking into this one. If @RITlug/teleirc-developers-commit-access have an opinion on previous options, please weigh in. 🙂 Otherwise, @Tjzabel is researching this more by the next developers' meeting on Saturday, Feb. 9th.

Edit: Personally I am in favor of Option 1. We accidentally were doing this before. I think we can strongly recommend someone to get their own API key instead of the generic default, but I don't think we have to write new code to police users' behavior.

@Tjzabel
Copy link
Member

Tjzabel commented Feb 8, 2019

So as an update, I am looking to go with option 1.

This entails enabling Imgur by default, and writing documentation suggesting to get a personal Imgur API key. However, this does not fix the issue of when someone uploads an image as a file. When uploading as a file, the API key is still shown.

In order to fix this, I think we can either:

  1. Check to see if the photo is a file, and if so, don't make it go through to IRC side.
  2. Keep it how it is, only enabling Imgur by default

@jwflory
Copy link
Member

jwflory commented Feb 8, 2019

When uploading as a file, the API key is still shown.

Ouch, I didn't know that.

I think we should add a configuration option to allow files to be sent, but it should be disabled by default (i.e. files will not be relayed over the bridge). We can provide documentation to warn a user about the risk of exposing the API key by enabling it. What do you think?

@Tjzabel
Copy link
Member

Tjzabel commented Feb 9, 2019

@jwflory hmm, so essentially it would work like this(?):

  1. User sends file over Telegram
  2. Teleirc sees message is of file type, checks to see if IRC file sending is enabled
  3. If enabled, send over file IRC side, showing API key
  4. If disabled, display message similar to: <user> shared a file on Telegram with caption: "foo"

Then, write documentation to support this suggestion. Thoughts?

@jwflory
Copy link
Member

jwflory commented Feb 9, 2019

Discussed in 2019-02-09 RITlug developers' meeting.


Agreed on Option 1 with Imgur enabled by default with docs to explain how to get an Imgur API key. @Tjzabel's PR to include a configurable feature to send an alert that a file was uploaded without exposing the sensitive URL over IRC (e.g. @Tim123 uploaded a file on Telegram with caption: "This is my white paper on why Teleirc is awesome."). The notification is enabled by default, and an admin can allow Telegram API URLs (with bot token) to go over to IRC if configured.

@Tjzabel aims to have this done by next developers' meeting on Saturday, Feb. 16th.

@Tjzabel
Copy link
Member

Tjzabel commented Feb 12, 2019

Due to unforeseen circumstances, this issue will not be completed by 16 February.

By our next meeting, I hope to at least further my research into grabbing the document caption.

@jwflory
Copy link
Member

jwflory commented Feb 14, 2019

@Tjzabel Thanks for the advance heads-up. 👍 We should still be on target if you can get this sometime next week.

@Tjzabel
Copy link
Member

Tjzabel commented Feb 14, 2019

@jwflory I should most likely be able to still get this done (at lest the implementation, not documentation) sometime next week. I will re-evaluate my workload over this weekend to see what is realistically feasible.

@Tjzabel
Copy link
Member

Tjzabel commented Feb 23, 2019

@alcir this issue has been resolved, and the newest changes have been pushed to master.

If you would like to have this new default behavior before our next milestone release, feel free to run teleirc from the master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

4 participants