Potential leak when applications don't read from the connection

[propertyscott]

The readme says:

nhooyr/websocket also responds to pings, pongs and close frames in a separate goroutine so that your application doesn't always need to read from the connection unless it expects a data message.

If an adversary sends a data message to an application that's not expecting a data message, then the read loop blocks forever.

[propertyscott]

A failed write unblocks the goroutine, but it might be some time before the application writes to the connection.

[nhooyr]

🤔

I don't think there's anything we can do here, its just normal connection behaviour. If the application isn't reading data messages from the connection, we can't read it for the application, we have to block until the application reads or the connection is closed (whether explicitly, via a failed write or a failed ping/pong).

All applications that are not reading from the connection should have a timeout anyway on how long the connection will be kept up, otherwise they're buggy. I guess this should be made more clear as right now it sort of sounds like you can just rely on c.Context() to tell if you 100% if the connection is alive or not.

What would you have it do?

Most I can think of is setting a timeout on the readLoop waiting for the application code to read. Maybe 1 minute would be reasonable? I had considered this when writing but it seemed too magical but I suppose a little magic is better than risking a security issue for the users of this package.

Or maybe we should ping/pong in the background by default? Maybe every 2 minutes? Its very cheap so I can't imagine any user of the package objecting.

[propertyscott]

The issue can be viewed as a documentation problem. Require applications to receive messages, even when the application is not interested. Add a helper function to do this.

[propertyscott]

This is something that we noticed while evaluating the package. Our application always reads the connection, so this has no impact on us should we decide to use the package.

[nhooyr]

Sounds good, thanks for the report.

[nhooyr]

The issue can be viewed as a documentation problem. Require applications to receive messages, even when the application is not interested. Add a helper function to do this.

It makes the a little API fragile to require that as if you miss that single line, you introduce a vulnerability into your code. I think it's best the readLoop time out after 2 minutes if the application doesn't read the incoming message. It's nice to not even have to think about it. Does that seem reasonable to you as well?

Regarding the ping solution I proposed, we can't ping for package users because we can't read pongs until the package user reads the data message.

[propertyscott]

I do not think that the package should try to hide the detail that the connection must be read.

We ended our evaluation of the package. Thank you for responding to my issues.

[nhooyr]

I agree, will fix this tomorrow night.

Thank you for the high quality issue reports.