Websocket handling: client side pings kill ws-connection.

[someone-somenet-org]

It seems that Mattermost's websocket handling of client side pings is broken since the 5.33.x update.

Instead of responding to ping frames (which is a MUST) MM now just silently drops the websocket connection.

I can reliably reproduce the loss of connection and work around the issue by disabling said client ping logic.

It also seems to affect matterhorn a mattermost terminal client.

Summary

Websocket handling of ping frames is broken with MM 5.33.1.

Steps to reproduce

For example use python3 websockets lib.

import asyncio, pprint, json, websockets, logging
logging.basicConfig(level = logging.DEBUG, format = "%(levelname)s::%(name)s::%(message)s")

async def ws_run():
  async with websockets.connect("ws://localhost:8065/api/v4/websocket", extra_headers={"Authorization": "Bearer ..."}) as websocket:
    while True:
    pprint.pprint(json.loads(await websocket.recv()))

loop = asyncio.new_event_loop()
loop.run_until_complete(ws_run())

• Connect to the websocket of a MM instance.
• Events come in.
• See outogoing ping
• No events come it anymore.
• Watch your connection die.

Counter-example: (added ping_interval=None to connect)

import asyncio, pprint, json, websockets, logging
logging.basicConfig(level = logging.DEBUG, format = "%(levelname)s::%(name)s::%(message)s")

async def ws_run():
  async with websockets.connect("ws://localhost:8065/api/v4/websocket", ping_interval=None, extra_headers={"Authorization": "Bearer ..."}) as websocket:
    while True:
    pprint.pprint(json.loads(await websocket.recv()))

loop = asyncio.new_event_loop()
loop.run_until_complete(ws_run())

• Connect to the websocket of a MM instance.
• Watch events come in.
• Watch your connection being happy.

Expected behavior

MM responding to client side ping frames as required by the RFC: https://tools.ietf.org/html/rfc6455#section-5.5.2

Observed behavior (that appears unintentional)

Websocket connection is silently dropped after a ping request from client.

[attzonko]

We are also seeing this issue as it affects the mattermost driver and anyone using that driver.

[amyblais]

Opened a ticket for investigation https://mattermost.atlassian.net/browse/MM-34158.

[elyscape]

This appears to have been caused inadvertently by #16620, which changed the websocket library in use. The old library was fairly high level and had default handling of ping messages:

Connections handle received ping messages by calling the handler function set with the SetPingHandler method. The default ping handler sends a pong message to the peer.

Connections handle received pong messages by calling the handler function set with the SetPongHandler method. The default pong handler does nothing. If an application sends ping messages, then the application should set a pong handler to receive the corresponding pong.

The new implementation is lower level and doesn't handle this for users, requiring them to implement this functionality themselves. This should be done here:
https://github.com/mattermost/mattermost-server/blob/00dda9fa065f09b54e65cd3a3fca23b3865e0332/app/web_conn.go#L248-L273
As you can see, close and pong messages are handled, but ping events are not. As such, ping events will fall through to the default handler, which logs an error and closes the connection. Adding a case for ws.OpPing to this switch block should be sufficient to resolve this issue.

[agnivade]

Thank you @someone-somenet-org and @attzonko for the report, and thank you @elyscape for the analysis!

The Mattermost server wasn't really explicitly handling pings from clients. It seems to be a default behavior from the old implementation which other clients seemed to rely on. The client is only meant to respond to ping messages with a pong, as our official Go websocket client (https://pkg.go.dev/github.com/mattermost/mattermost-server/v5/model#WebSocketClient) does.

The workaround is to disable making explicit pings and only respond to pongs from the server.

In any case, closing the connection is incorrect behavior, and we will be fixing this. Thanks everyone.

[attzonko]

@agnivade From the spec, it seems that a ping frame can come from either endpoint (client or server). Any endpoint receiving a ping frame must respond with a pong frame. So, are you suggesting that the server will no longer respond with a pong frame going forward, or am I misunderstanding your response?

[agnivade]

Sorry for any confusion there. The fix will be to respond with a pong frame. But until then, the workaround is to not make explicit pings.

For a better solution, I would suggest not to send ping frames at all, as the server already does that.

[attzonko]

OK, that makes sense. However, it is not very practical for us to disable the client-side ping, since the WebSocket connection is handled by a library twice removed from our Bot framework, and the intermediate library does not provide a way to disable the client pings. I'll try and work with those owners to get that functionality added, however from their standpoint the server is the one that is not adhering to the spec by not responding to the ping.

[agnivade]

We have reverted the feature, and it's already merged to master. So I will close the issue. A dot release for 5.33.2 should be out soon. Thanks everyone!