Skip to content

Support transport agnostic token passing in channels#6086

Merged
SteffenDE merged 3 commits intomainfrom
sd-auth-token
Feb 19, 2025
Merged

Support transport agnostic token passing in channels#6086
SteffenDE merged 3 commits intomainfrom
sd-auth-token

Conversation

@SteffenDE
Copy link
Contributor

For WebSocket, the Sec-WebSocket-Protocol header is used. For LongPoll, an Authorization header is passed instead.

Fixes #5778.

For WebSocket, the `Sec-WebSocket-Protocol` header is used. For LongPoll,
an `Authorization` header is passed instead.

Fixes #5778.
|> Transport.check_origin(handler, endpoint, opts)
|> Transport.check_subprotocols(opts[:subprotocols])
|> maybe_auth_token_from_header(opts[:auth_token])
|> Transport.check_subprotocols(subprotocols)
Copy link
Member

@josevalim josevalim Feb 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, it feels we should just bring this function to this module and have it deal with both auth_token and subprotocols at once, or move auth_token to check subprotocols. Transport is meant to have transport agnostic functions, and that one is clearly websockets related, so it shouldn't exist in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is part of the public API, so we can't really change it, can we?
https://hexdocs.pm/phoenix/Phoenix.Socket.Transport.html#check_subprotocols/2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we cannot, but maybe we can move the auth token handling there anyway?

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I have dropped a comment about subprotocol handling but it can be handled in a future PR!

@SteffenDE SteffenDE marked this pull request as ready for review February 19, 2025 16:00
@SteffenDE SteffenDE merged commit db8eac8 into main Feb 19, 2025
10 checks passed
@SteffenDE SteffenDE deleted the sd-auth-token branch February 19, 2025 18:00
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.

Current approach to passing token in channels is unsafe

2 participants