idoco / intergram

Free live chat widget linked to your Telegram messenger
https://www.intergram.xyz
Mozilla Public License 2.0
1.4k stars 358 forks source link

All replies from Telegram are send to all chat clients #91

Closed adriaandotcom closed 3 years ago

adriaandotcom commented 4 years ago

I think this is very bad in the current code of the this app.

If you reply from Telegram the websocket will send to all chats that reply.

idoco commented 4 years ago

If you reply directly to a specific message, only that visitor will receive that response.

This is also the default mode when adding the bot to a group.

You can quite easily fork this repository and change this default behavior for your website.

adriaandotcom commented 4 years ago

I mean all messages end up at all chat clients. Meaning if you have 5 active users using your chat feature and you response to one of them the other users can access those messages when they inspect the web socket.

Here is the problem:

https://github.com/idoco/intergram/blob/df1a4128c57b283dfb44766db9ba0fc29ba54d6b/server.js#L29-L35

You are emitting with io.emit() to all clients here. Use socket.emit() instead.

https://stackoverflow.com/questions/32674391/io-emit-vs-socket-emit:

socket.emit('message', "this is a test"); //sending to sender-client only
io.emit('message', "this is a test"); //sending to all clients, include sender

Here is an implemented fix from @squarecat: https://github.com/squarecat/squarechat/commit/918ed1f7f7dac7edd518a64f2e6fb2ecbd81e0fc

anubra266 commented 4 years ago

@idoco This should be implemented please.

idoco commented 4 years ago

Thanks @adriaanvanrossum this requires immediate fixing.

This will need a slightly bigger refactor than just replacing io with the connection. In the code section you linked to, I will need to get the right connection (socket) to send the message to. maybe I can manage a map between chatId + "-" + userId and the relevant socket, and emit only to that socket.

If someone is willing to help with this, I'd love to merge their changes

anubra266 commented 4 years ago

I couldn't work on this project. Most of the dependencies wanted an old version of NodeJs. And majority of my projects use newer versions. Is this suppose to happen?

adriaandotcom commented 4 years ago

Maybe this can be of help. Thanks to @Jivings

Here the code buffers the message when user is not online: https://github.com/squarecat/squarechat/commit/8d0f3ba42984ed082737cafe46afd4cb803b00ef#diff-78c12f5adc1848d13b1c6f07055d996e And here they use part of that code to send to only one socket: https://github.com/squarecat/squarechat/commit/918ed1f7f7dac7edd518a64f2e6fb2ecbd81e0fc

Here is my own implementation: https://github.com/simpleanalytics/chat/commit/f19e80c7d1cc96aa3fbe6ffe5897cb8f9e1ff5e4#diff-78c12f5adc1848d13b1c6f07055d996e

Jivings commented 4 years ago

Let me know if you want me to submit https://github.com/squarecat/squarechat/commit/918ed1f7f7dac7edd518a64f2e6fb2ecbd81e0fc as a PR since I think it alone fixes the most serious issue.

adriaandotcom commented 3 years ago

Isn't this issue super important @idoco?

idoco commented 3 years ago

This should be fixed now. Please review the fix here.

The proposed solution didn't work because we don't have the socket in context when the message is sent from the admin back to the client.

Sorry for the delay. But without external help, it takes me a long time to sit down and fix complex issues.

adriaandotcom commented 3 years ago

Glad you figured it out! Thanks for solving this issue for future users. 🙏