Isnt this "unsafe"?

[sinnbeck]

I have read through the source, and it seems that all chat is done on a single private channel called 'private-chatify'. On this channel, all messages are sent. That would also mean that if someone were to open developer tools (f12), select Network and then WS (Websockets), they would be able to see all messages sent? Yes the package does make sure you only see you own chat messages, but as that is done using Javascript, anyone can read ALL messages.

[munafio]

It may be "unsafe" as you said! because I tried to make this package as simple as I can for other developers so I used the simplest techniques with this package .. and also I may not noticed that!!.. but, I'll re-develop it completely with better, secure, easy and clean code .. and this will be fixed in the next releases
Many Thanks for you.

[sinnbeck]

No worries. I was just looking at the source as I am building something similar (using pusher with laravel echo). I am joining a personal private channel ('private-direct.' + user.id), which I then push to, using laravels own broadcasting (using your implementation should probably be able to do the same) :)

Something like

send() method on controller

Chatify::push('private-chatify.' . $request['id'], 'messaging', [
                'from_id' => Auth::user()->id,
                'to_id' => $request['id'],
                'message' => Chatify::messageCard($messageData, 'default')
            ]);

and js

// subscribe to the channel
var channel = pusher.subscribe('private-chatify.' + auth_id);

[munafio]

This will be solved as your suggestion and maybe with some changes in a better way..
Thanks to you.

[laurencei]

@munafio, what is the eta for v2 that you mentioned?

p.s. seems like an amazing package - great work.

[munafio]

@laurencei I can not tell you right now, BUT! As soon as possible will be there😁

[elliters]

This has been a year now, is this get solved ?

[munafio]

This has been a year now, is this get solved ?

Yes, since v1.2.x as I can remember.

[stayallive]

This package is still using a single socket channel as of today for realtime so be cautious using this, the Pusher authentication endpoint also authorizes any channel for any authenticated user so be careful exposing that if you use other private channels with more sensitive data.

chatify/src/Http/Controllers/MessagesController.php

Lines 177 to 182 in d49fcc3

	// send to user using pusher
	Chatify::push('private-chatify', 'messaging', [
	'from_id' => Auth::user()->id,
	'to_id' => $request['id'],
	'message' => Chatify::messageCard($messageData, 'default')
	]);

[aneesdev]

@munafio it's 2022 and you till haven't done anything sadly

[munafio]

@munafio it's 2022 and you till haven't done anything sadly

Hi 👋
Sorry for that, but the reason behind it is that there is no support for my work to keep up with 🤷🏻‍♂️ also because of my own business... but the next days I will work on it to fix this security issue.
Hope to be released very soon.. till then, you can do/customize whatever you want since Chatify is fully customizable ❤️
Regards

[stayallive]

Hey @munafio, it's not that you have to fix it (you don't owe anyone anything, or at least that is how open source should work...), but it's good that if you are aware of a security problem that you at least educate users looking at the project, pinning this issue is already enough. That way people can make their own decisions and fixes when needed. So thanks for educating future readers! ❤️

[munafio]

Hey everyone, 👋🏻
I've just released a new version (v1.5.3) with a fix to this critical issue, now a multiple sockets been used and each user has his own channel which is totally separated from other channels.
This is an urgent fix, in the next releases will enhance it more if needed.
Regards ❤️

[hdk-pd]

Thank you for taking the time to address this.
However, I fail to see how this fixes the issue. Yes, it uses multiple channels now, but that just changes the issue without fixing it. From what I've seen, you just suffixed the channel with a public, known ID. I even have to know that ID to send a message to someone, it is part of the request. Additionally, the two commits e734553 e981c71 do not implement any authorization on the channel. As a result, you can still read all messages. Please correct me if I missed something, I'd be happy if I did.

[munafio]

Thank you for taking the time to address this.

However, I fail to see how this fixes the issue. Yes, it uses multiple channels now, but that just changes the issue without fixing it. From what I've seen, you just suffixed the channel with a public, known ID. I even have to know that ID to send a message to someone, it is part of the request. Additionally, the two commits e734553 e981c71 do not implement any authorization on the channel. As a result, you can still read all messages. Please correct me if I missed something, I'd be happy if I did.

The channels are private and even if you know the id, you can not show all the messages of all users that making conversations, only the one you are contacting with which is normal.. this was the main issue.

[hdk-pd]

Thank you for your time again.

Your private channel needs authorization by default, yes. But I can solely find this one single check, which would a user not get access granted to a channel: https://github.com/munafio/chatify/blob/master/src/Http/Controllers/MessagesController.php#L41
This only checks if you are authenticated at all (and not authorized as the comment // check if user authorized states), if so, it just happily passes along every request parameter you throw at it, this includes any private channel you like. Doesn't it? I didn't check this part too in-depth, but isn't the suffix just the AUTO INCREMENTAL user id?

Edit: This is actually what stayallive mentioned in his comment before too #29 (comment). Using this package does not only expose all messages for the very same package, it opens all doors to listen to any private channel in Pusher the credentials are valid for. The explanation of "trying to make everything as simple as possible with the most simple techniques", "not having the support to fix it" and just closing the issue and all of this are starting to make me think this might be a backdoor.

[munafio]

@hdk-pd That's correct, It's checking if a user is authenticated not authorized (my bad for the comment state) but then Pusher will do handle the auth for you and you can see that in the next line.. you can read about it at their official docs..

[hdk-pd]

Pusher can not authorize your users. How should Pusher know which user is allowed to access a certain channel? That is why your endpoint in the code line I sent in my last comment even exists. It would be obsolete otherwise.

Honestly, I think the Pusher docs are misleading.
The examples do not show the part that you also miss in your code: checking if the user is actually allowed to listen to the specific channel. Only the Laravel example really checks the user id. The rest pretty much does what you are doing, just check if the user is logged in and then grant access to whatever channel they want to access.
Check the documentation for creating a custom authorization handler that returns a valid token that allows you to listen to a channel. It reads:

You would first check that the user (authenticated via cookies or whatever) has permission to access channel private-foobar

You are not doing this. You are checking if the user is any user (in other words: authenticated, which is not equal to authorized).

[munafio]

@hdk-pd That's right, I'll work on it to let only the authorized users as you mentioned.. it was Pusher mistake as mine and will be fixed. thank you so much for your explanation and your time.

And I'll keep you updated.

[hdk-pd]

Thank you for your time to follow along with me. I raised the concern in the Pusher documentation repository to see if a made up example scenario with proper authorization could be integrated in the examples I linked before.

[munafio]

A new version been released just now (v1.5.4), fixing connection auth issue.
Now, the package uses multi-socket connection with authorization for the users accessing/connecting a channel.. with this way, it should be fixed now.