Support for WebSocket Protocol header

[joeyhoek]

The problem I was facing was that I'm using Javascript running in a browser. Therefore I need to connect to a WebSocket with the default JS functions and unable to use a 3rd party alternative for NodeJS. I needed to authorize before the WebSocket connection is opened else it leaves you vulnerable to DDoS attacks. This leaves one secure option: passing the token in the sec-websocket-protocol header. I found out there was no support in Facil.io yet.

There are three different types of requests coming in:

1. Has no sec-websocket-protocol header header, so no header will be in the response.
2. Has a single string as value and passes the same value back in the response.
3. Has an array of values as a string separated by colons. Takes the first value and passes it in the response.

Any other way would fail to connect. Normally would there be checks if the WebSocket Server actually supports those protocols. This code just makes it possible that no matter the value you are still able to connect.

[boazsegev]

Hi @joeyhoek ,

Thank you for opening this PR!

I love the general approach and the desire to make things easy for all users using sensible defaults.

However, I suspect that the use of the sec-websocket-protocol header might be specific per application and could be better handled during the on_update callback rather than as part of the automated WebSocket handshake.

After all, different applications might use the sec-websocket-protocol header differently. Some might route different protocol values to different WebSocket callbacks (to match the requested protocol) while others might use this value to manage API versions.

I wonder, do you think that having each application handle and set the sec-websocket-protocol header in the on_upgrade callback wouldn't work? Or do you think this should be handled by the facil.io framework in an opinionated way?

Kindly,
Bo.

P.S.

I needed to authorize before the WebSocket connection is opened else it leaves you vulnerable to DDoS attacks. This leaves one secure option: passing the token in the sec-websocket-protocol header.

AFAIK, it's possible to set a cookie with an authentication token (such as one with a one-time token or an OAuth2 token). This cookie will be available in the HTTP handshake (the on_upgrade callback) and can be used for authentication / authorization before establishing the connection. You might want to use the "SameSite" cookie policy, though it isn't currently supported in the main branch (support will arrive with version 0.8).

Of course, you can also send the token with the sec-websocket-protocol header, but I think this might not be the intended use for the sec-websocket-protocol header.

[boazsegev]

Although I'm not yet sold on the idea that the sec-websocket-protocol header should be handled by the facil.io framework (rather than handled by the app developer in the on_update callback), I thought I'd review the code and see if it might help me make up my mind about the best approach.

Most of the code looks nice 👏🏻, though there are some minor concerns that could effect performance (as well as a debug message I suspect you intended to delete).

[boazsegev]

Avoid strlen for strings with a known length. Length data is returned as part of the fiobj_obj2cstr return value and there's no need to re-compute the string length (it is cached).

[boazsegev]

Is this some debugging line? if required, it is better to use FIO_LOG_DEBUG with a proper message.

[boazsegev]

Although I agree that it's better to avoid an additional string copy, I think this is destructive to the original copy which someone might still want to access later on.

It is better to use tmp_protocol = fiobj_str_new(protocol_str, (tmp_ptr - protocol_str)) instead of destructively adding a NUL char just to recalculate the length (especially since the length is already known by the value of tmp_ptr).

[joeyhoek]

Hi @boazsegev,

I think that every application should be able to set it personally, however I'm unaware of this feature in the on_upgrade function. That's the reason I wrote this as a temporary implementation. The thing with the sec-websocket-protocol header is mostly that it's used for 2 different reasons:

1. Confirmation of used data format within the WebSocket.
2. Only way of passing a token before connection in JS supported by the browser.

Therefore the content is unknown. It is best to be able to set that data per request and like I said I'm unaware we can set that header for response in the on_upgrade function. I thought of this as a temporary fix so all connections are allowed instead of the current none.

As for the written code. I only just read the documentation part of how I could get the length of the string, so I could address the strlen function. Also I noticed after submitting the 2 print statements, I should remove those as well. They were just for debug purposes. Lastly I could also change that.

Just let me know if its possible to set the response in the on_upgrade function and this fix is unnecessary or this isn't supported and this is for the time-being the best alternative.

[boazsegev]

Hi @joeyhoek ,

I think that every application should be able to set it personally, however I'm unaware of this feature in the on_upgrade function.

The exact same code you used in the commit should be possible in the on_upgrade callback. This will allow you to test and set the sec-websocket-protocol header before deciding if you want to allow the connection or not (i.e., in cases where a token is used and you would prefer to deny the request).