phoenixframework / websock_adapter Goto Github PK

Hello there 👋🏼

In #14 (released as 0.5.5), a change was made to the way websocket connections are validated (I understand it to be that upgrade MUST be a part of the Connection header now). Although this change is aligned with the spec, it caused a notable change in our application's error rate immediately after deploy.

Unfortunately, we are completely unsure when a client would send Connection: Keep-Alive to that endpoint, but it happens often. (Do you know of any reason why a socket connection initiated by phoenix.js might do that?) We are certain that it is a websocket client making the erroneous request — not a rogue crawler or anything like that — because the request includes our authorization token and vsn param. A small sample size of requests have shown Chrome on Windows as the user agent (but the sample size is much too small to be a significant indicator).

Because we can't pinpoint the source of the erroneous request, we also can't know whether the request was being served successfully previously. The WebSockAdapter.UpgradeError doesn't appear to replace any older errors in our tracing, so the outcome of these requests before the change is unclear.

To make this actionable, here are two points:

• Could you please consider changes like the introduction of WebSockAdapter.UpgradeError to be breaking changes in the future? Despite the closer alignment with the spec, it was effectively a breaking change for our application.
• Are you aware of any situation in which a websocket client (ex. phoenix.js) would send Connection: Keep-Alive to /socket/websocket?

Thanks for your help!

To help mitigate mtrudel/bandit#149, we should be validating WebSocket upgrade requests against RFC6455§4.2.1 at the time the upgrade is requested, and not deferring to the underlying server's checks (which will be run after the Plug life cycle completes). The checks required are already implemented at https://github.com/mtrudel/bandit/blob/main/lib/bandit/websocket/handshake.ex#L15-L34 for reference.

There are a couple of ways we can approach this:

1. Add these checks directly inline in WebSockAdapter.upgrade/4. This has the upside of structurally preventing invalid upgrades, but the downside that it's hard to surface the error up to the caller without breaking an existing API (unless we return it in :private or similar).
2. Add a WebSockAdapter.validate_upgrade/1 which takes a Plug.Conn and returns any errors for the caller to process. We'd also have to tee up a separate PR to Phoenix to make use of this validation in Phoenix.Transport.WebSocket.
3. Do # 2, but adding the validation function into a new Plug.WebSocketHelper module. The bonus here is that I can hoist up Bandit's existing checks for this and DRY it all up.

Comments / preferences welcome (@chrismccord, relevant to your interests).

Congrats on the release. On Elixir 1.14, OTP 25, Phoenix 1.7-rc.0 I'm seeing at compilation:

warning: @behaviour :cowboy_websocket does not exist (in module WebSockAdapter.CowboyAdapter)
  lib/websock/cowboy_adapter.ex:1: WebSockAdapter.CowboyAdapter (module)

And of course then warnings for all the callback implementations.

Currently Bandit and this Cowboy adapter, only allow for sending of "WebSocket Close" codes 1000 and 1002. Or in the case of cowboy adapter they drop/ignore close codes entirely.

The WebSocket specification allows any code from with registered values here. As well as a short UTF8 Binary message along with close codes.

I previously did some work to handle the case where the user is force disconnected by sending a status code 3000, which is registered unauthorized. This allowed the socket.js to not retry connections if they are booted by the server purposely. While maintaining some level of backwards of compatibility.

I don't see any reason why would should limit to 1000 and 1002. With cowboy we could handle the reply {:stop, {:close, CODE, REASON}, state} correctly. In Bandit it looks like this is a known thing but it is simply not being used by the websocket close function.

I Post all this because maybe I'm missing something?

Why limit the close codes? Why drop/ignore close messages? I can do the work to enable it on your end and phoenix's end but I wanted to gut check myself.

The version in the mix.exs are quite restrictive:

{:websock, "~> 0.4.3"},
{:plug, "~> 1.14.0"},
{:bandit, "~> 0.5.9", optional: true},

Currently bandit 0.6.0 won't work without specifying override: true.

Can we switch it to go up to the next major version (e.g. {:bandit, "~> 0.5"}) for all three?