Add partial context implementation for WebSocket routes + Security fix

[nicolas-rempulski]

This one started as a feature request and escalated to a security issue.

The problem arisen when we tried to authenticate user on WebSocket routes (@OPEN, @close, @message) : the authentication failed as Wisdom doesn't use Http headers on Socket to populate the Context available in Controllers (context(), session(), ....).
Several options were possible from there :

• Create a feature request on Github to use Context on WebSocket Routed ❌
• Implement the feature ourselves ❌
• Use the WebSocket body / parameters to send the user's sessionId ❌
• Create the new WebSocket() client side a few milliseconds after a successful authenticated HttpRequest to retrieve user session informations server-side ✅
• Stop using WebSo...

? Opening a WebSocket after a HttpRequest allow to retrieve credentials ? Ok, surely some VertX or Wisdom wizardry, let's go onward.

Security issue shred to light when we hit our Proxy (Nginx) timeout that disconnected our WebSockets after 5Mn of inactivyty.
To solve this issue, when the socket timed-out, we made our client re-establish the WebSocket connection immediatly...
And we tested this feature by hot restarting Nginx while several client had WebSockets opened on our server...
And Wisdom saw each client reconnecting ... as one and only user, all with the same sessionId (same session, same flash, same Context).

It appears that, when a websocket Opening, arrive in the few milliseconds after a HttpRequest, Wisdom mess the Context for the WebSocket Routes and give access to a random Context of a Request currently executed.

This PR intends to fix the security issue by providing Context for WebSocket Routed, using HttpHeaders provided by VertX. This should erase any remaining Context while handling WebSocket events.

Finally, I think there is somewhere a Context release or cleanup that is not correctly done. This did not appear before as the context is always injected in HttpRequest, thus erasing any previous one, but WebSocket, without Context handling, opened a hole in this process.

As it is deep down in Wisdom, I would really like some review before merge.

Thanks !

[cescoffier]

It looks good to me. Would have been nice to have a few tests.

[nicolas-rempulski]

If the overall process seems good, I'll try to add some tonight.

How many is a "few" at least ? 2 ? ^^

2016-10-21 16:30 GMT+02:00 Clement Escoffier [email protected]:

It looks good to me. Would have been nice to have a few tests.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#559 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABtSg_3jtfWLAH1A2qURXk0R3j4RnIb1ks5q2MxxgaJpZM4KcKZU
.

[cescoffier]

"few" means it should cover the new code ;-)

[cescoffier]

ping ?

[barjo]

pong ?