actix-ws should support Sec-WebSocket-Protocol (or explain why it doesn't)

[sjoqvist]

Sub-protocols are a part of the WebSocket specification (see RFC 6455):

The |Sec-WebSocket-Protocol| header field is used in the WebSocket
opening handshake.  It is sent from the client to the server and back
from the server to the client to confirm the subprotocol of the
connection.  This enables scripts to both select a subprotocol and be
sure that the server agreed to serve that subprotocol.

They also seem to have been supported by actix-web-actors:
https://github.com/actix/actix-web/blob/568bffeb58db083d63d887bd0f1cceaa0464d1c9/actix-web-actors/src/ws.rs#L425-L448

However, actix-web-actors has been deprecated in favor of actix-ws, which doesn't appear to support sub-protocols. I can't find any references to it, so I don't know if it's intentionally left out, and in that case intended or not intended for later implementation. If it's intentionally left out without the intention of implementing it later, it would save the user's time if this is mentioned in the documentation.

I'd recommend either

1. allowing sub-protocol negotiation, for example using function parameters, or
2. explaining the design decision to not include it.

[asonix]

I see no reason not to support sub protocols in actix-ws. The reasoning is when I first built actix-ws it was more of a "proof you can do websockets in actix-web without actors" and it hasn't changed much since it was adopted in actix-extras.

I think this request along with this other PR actix/actix-web#3496 deserve some extra thought, though. I feel like there should be a way to hook into actix-ws' handshake and encoding steps without needing to open PRs to actix-http and actix-web for every configuration (I may be proposing a new middleware system for websockets)

I would love to hear some thoughts on this from @robjtede and maybe @segfault87

[segfault87]

When it comes to actix/actix-web#3496, it looks like is in a little bit different situation becuase it is an extension, not a subprotocol. Although it's nothing with subprotocols, I'll try to give my thoughts on the concept of "middleware"s.

The problem with current actix-http/ws is it conceals every inner details of the payload, rendering implementing extensions outside of actix-http nearly impossible. In order to support provide APIs for middlewares, lower-level side of WS frames should be exposed to the API surface.

And it raises some questions:

• API design wise, is it okay to provide such functionality? Should we expose the raw payload and give ability to manipulate it in unstructured form? Or in more abstracted form?
• As far as I know this is the only standardized WebSocket extension out there. Generalizing extensions might be a design challenge as there's no clear definition on extensions. Can we provide middleware APIs without making leaky abstractions? And could it be any use cases outside of permessage-deflate that require manipulating WS frames?
• Currently message encoding/decoding is handled by Codec and extending it would break the API compatibility as there are some assumptions we have to break and its API is currently not in good shape. (for example it's cloneable by default, implicit creation could happen everywhere, Encoder and Decoder mixed together, ...) actix-http, actix-web-actors (which is deprecated), awc, actix-ws and possible external API users rely on this. Are we okay with breaking the API compatibility?

Extensions are extensions, and it would be nice if they could be provided outside of the core library. Sadly there's no infrastructure for that and implementing it would bring up another long processes of discussions and API changes. I needed compression feature as soon as possible and that's why I implemented the feature in actix-http. I want actix/actix-web#3496 to keep in current form for now but I'm open to discussion. If there are settled API design around middlewares, I'll happily move to the new API.

[EvilWatermelon]

There is a reason why we need Sec-WebSocket-Protocol. Since the Websocket API does not allow to send additional headers like the needed Authorization header for JWT we need another ways to get the token (see StackOverflow post). Another solution would be using query parameters to get the JWT which is more insecure than anything else. If we are not able to handle this horrible missing feature then we have to switch to different websocket frameworks that allows us to pass additional HTTP headers as a client.

[jokemanfire]

Just change http response can resolve it ?

async fn do_ws(
    token: web::Path<String>,
    req: HttpRequest,
    stream: web::Payload,
    stream_server: web::Data<Arc<StreamsServer>>,
) -> Result<HttpResponse, Error> {
    debug!("recive token {:?}  http header {:?}", token, req);
    let (mut res, session, msg_stream) = actix_ws::handle(&req, stream)?;

    // spawn websocket handler (and don't await it) so that the response is returned immediately
    tokio::task::spawn_local(deal_wsstream(
        (**stream_server).clone(),
        token.to_string(),
        msg_stream,
        session,
    ));
    //change response's sec-websocket-protocol to PROTOCOL
    res.headers_mut().insert(
        http::header::SEC_WEBSOCKET_PROTOCOL,
        http::HeaderValue::from_static(PROTOCOL),
    );
    debug!("Http response {:?}", res);
    Ok(res)
}