phoenixframework / phoenix

Peace of mind from prototype to production
https://www.phoenixframework.org
MIT License
21.45k stars 2.88k forks source link

WebSockets: support passing through sec-websocket-protocol header #5904

Open feld opened 3 months ago

feld commented 3 months ago

Opening this MR to begin a wider discussion on this.

Let's start with the current state of things: in Phoenix it's not possible to access any headers of Websocket connections except the X-Foo headers if you explicitly enable it with connect_info: [:x_headers]. The docs are very clear about this limitation:

Phoenix only gives you limited access to the connection headers for security reasons...

However, this severely limits the capabilities of Phoenix. We converted Pleroma from Phoenix+Cowboy for websockets to Phoenix.Socket.Transport so we could support both Cowboy and Bandit. In the process I encountered a weird test case we had written that validated you could pass through an auth token in the Sec-WebSocket-Protocol header.

Nobody could remember at the time why this existed.

It turns out, this is actually how the Mastodon client protocol authenticates over websockets!

At first I thought this was a case of abusing this header for something it wasn't meant for. The RFC doesn't make it sound like this is the purpose of the header. So why were they doing it this way?

As it turns out, lots of applications do this. Here's one from 3 weeks ago doing it in AWS Lambda.

So how can we make sure Phoenix is a suitable framework for all types of Websocket applications/protocols as the current limitations are very narrow-sighted? I do not want to have to maintain a fork of Phoenix to support this.

lambadalambda commented 3 months ago

I second this, the current behavior is great for new applications, but if you want to re-implement somebody else's API the only way to do it seems to be to run a fork.

Maybe it's possible to expose these headers after some config setting has been set that makes it explicit that this is not something you should do if it can be avoided in any way?

feld commented 1 month ago

What is needed from me to move this along? If I can get an idea of what solution will be accepted I will gladly update the branch to include the required changes to docs, tests, etc.

mtrudel commented 1 month ago

If you want to write a handler for a 'raw' websocket (ie: without channels), you can trivially do this by calling Plug.Conn.upgrade in your controller and passing in a WebSock implementation. I gave a talk at ElixirConf EU a few years ago outlining the process: https://www.youtube.com/watch?v=usKLrYl4zlY

josevalim commented 1 month ago

My understanding is that the subprotocol is set to a static field, such as "bearer", so we do support setting subprotocols on the websocket configuration: https://hexdocs.pm/phoenix/Phoenix.Endpoint.html#socket/3-websocket-configuration

If that is not enough, we would gladly merge a PR that adds :sec_websocket_headers to connect_info and we would pass all headers starting with "sec-websocket" (as we pass x-headers). If that's really a requirement, we may even consider deprecating the subprotocols feature in the future (as folks can now easily check that themselves).

feld commented 1 month ago

I also thought that the subprotocol was meant to be a static field, but that's not how it's being used in practice for some existing services/protocols. It is indeed strange that this is being treated as a header to hold authentication tokens.

I will rework this branch to support the :sec_websocket_headers option to connect_info.