search

voryx / Thruway

PHP Client and Router Library for Autobahn and WAMP (Web Application Messaging Protocol) for Real-Time Application Messaging
MIT License
676 stars 118 forks source link

Invalid roles in Welcome message #280

Closed mikesoule closed 6 years ago

mikesoule commented 6 years ago

The RFC only allows for broker and dealer roles in the Welcome message but other roles are being added to the message via the constructor. See: https://github.com/wamp-proto/wamp-proto/blob/master/rfc/draft-oberstet-hybi-crossbar-wamp.txt#L1685

This causes the Python Autobahn Welcome message class to throw a protocol error when receiving a Welcome from Thruway. See: https://github.com/crossbario/autobahn-python/blob/master/autobahn/wamp/message.py#L812

I'm actually not sure if Autobahn Python is being too strict here of if Thruway should be enforcing roles per the RFC but I'm guessing the issue is with Thruway.

If we agree that Thruway should filter the roles in messages, I'd be happy to issue a pull request with a fix. Please specify if you would like it addressed only in the Welcome class or in the DetailsTrait.

One option is to add a method DetailsTrait::setAllowedRoles(array $roles = []) which could be called from each message constructor where the RFC specifies restricted roles. Then DetailsTrait::setDetails() could filter out invalid roles via its allowedRoles property.

mbonneau commented 6 years ago

@mikesoule - Thanks for this pickup. Interoperability with other WAMP clients and routers is important. I do think that the crossbar message implementations are a little strict as the RFC does not seem to explicitly limit the router roles to those two and would cause future extensions to not announce themselves in the roles section. This is really besides the point though as interop with crossbar is higher priority at the moment (I generally consider crossbar/autobahn to be a reference implementation).

Right now the roles are being picked up by the roles themselves by watching for the SendWelcomeMessageEvent.

At some point this could be restructured, but for the time being filtering these would acceptable. This could be handled in the processSendWelcome method on the Realm which is able to modify the message before it goes out.

A PR with that solution would be greatly appreciated. If you are not able to get to this in the next day or so please let me know and I will see if I can set aside some time as I would like to get this fixed as soon as possible.

mikesoule commented 6 years ago

Thanks @mbonneau. Grabbing a cup of coffee and I'll get right on it.

mikesoule commented 6 years ago

@mbonneau I submitted the pull request. Let me know if any further changes are needed. https://github.com/voryx/Thruway/pull/281

mbonneau commented 6 years ago

Hi @mikesoule 0.5.2 has been released with the fix. Thanks again for your help.

mikesoule commented 6 years ago

@mbonneau It looks like my fix did not completely solve the problem. I ran into this issue again in AnonymousAuthenticator. The reason is because the authenticator sends a welcome message directly to the session rather than via the Realm.

A quick fix would be for me to add the role filter to the WelcomeMessage constructor but I still can't be sure that covers all corner cases. The setDetails() method seems to be the single point of entry for setting and filtering roles but that's a trait so trying to hook into that method could get ugly.

Thoughts?

mbonneau commented 6 years ago

@mikesoule Fixing it in the setDetails method of the WelcomeMessage is appropriate so that we catch all use cases. If you are willing, a PR would be appreciated.

Moving forward to the future I have filed this issue: https://github.com/voryx/ThruwayCommon/issues/1