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

Provide a mechanism to validate upgrade requests before issuing upgrades #11

Closed
mtrudel opened this issue Jun 16, 2023 · 0 comments · Fixed by #14
Closed

Provide a mechanism to validate upgrade requests before issuing upgrades #11

mtrudel opened this issue Jun 16, 2023 · 0 comments · Fixed by #14

Comments

@mtrudel
Copy link
Member

mtrudel commented Jun 16, 2023

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).

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 a pull request may close this issue.

1 participant