Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

feld
Copy link

@feld feld commented Aug 20, 2024

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
Copy link

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
Copy link
Author

feld commented Oct 21, 2024

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
Copy link
Member

mtrudel commented Oct 21, 2024

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
Copy link
Member

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
Copy link
Author

feld commented Oct 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants