Default value of check_request

[zoggy]

Since release 2.7 Websocket_lwt.establish_server has a check_request parameter. This is good, except that it has a default value: check_origin_with_host. This is problematic when the server is behind a WS proxy because this control will fail and the resulting exception does not give a lot of information (so it took me some time to find it).

Moreover, since the on_exc parameter has no default handler, the exception Protocol_error "Bad headers" was raised and just shut down the websocker server.

So I would suggest: either

• set no default value for check_request and on_exc or
• keep the same check_origin_with_hostdefault but in this case set a more informative error message and set a default on_exc handler.

[copy]

The default value of on_exc is very important. Otherwise errors would be swallowed, which is something you never want. It's also consistent with lwt.

The default value is of check_request is also important. The alternative of not checking it by default would leave many websocket servers open to cross-origin requests.

The real problem seems to be that we raise when the header checks fail. I don't think we should fail and call on_exc when a client sends bad headers or is failing the check_request test. We're already handling these case gracefully by replying with 403 Forbidden to the client. Instead of failing, we should log the problem.

This is problematic when the server is behind a WS proxy because this control will fail and the resulting exception does not give a lot of information (so it took me some time to find it).

Shouldn't the proxy forward headers? Note that check_origin_with_host doesn't fail if the origin header is missing, only if there is a mismatch. If proxies don't forward the headers for a good reason, we should document this in establish_server.

[vbmithr]

The real problem seems to be that we raise when the header checks fail. I don't think we should fail and call on_exc when a client sends bad headers or is failing the check_request test. We're already handling these case gracefully by replying with 403 Forbidden to the client. Instead of failing, we should log the problem.

Definitely agree. Sorry that you took time debugging @zoggy . I think we should stop raising an exception on client error, this does not make sense I probably coded this a long time ago and never made this part of the code evolve. I'm quite ashamed of this actually. I'll fix this soon, thanks.

[zoggy]

@vbmithr No problem. We can't focus on everything at the same time, then we forget :)

@copy I agree that on_exc is important so it could be mandatory instead of optional. But since this would break backward compatiblity, having a default function which log the exception seems fine to me.

Regarding the check_request defaulting to check_origin_with_host, well this does not protect a lot since the client can put whatever it wants in the origin field but it may be better than nothing and it should be documented. Note that the problem with my proxy was that it changed the host: field in the request from the original server name to localhost:XXX when tunnelling the request to the docker container running the server. It seems that this is the default in apache (https://httpd.apache.org/docs/2.4/mod/mod_proxy.html#proxypreservehost) so others after me will probably get bitten by this.

By now, I provide as check_request a function comparing the origin, if present, to the "public hostname" (i.e. the hostname of the proxy) of my application. So I think it could be useful to have an additional function check_origin_host taking a hostname list and a request and checking that the origin of the request is in the given hostname list. check_origin_with_host could use this function. check_origin_host could be used as check_request parameter when the server can handle websocket requests from different hosts.

[copy]

Regarding the check_request defaulting to check_origin_with_host, well this does not protect a lot since the client can put whatever it wants in the origin field but it may be better than nothing and it should be documented.

The JavaScript API in browsers doesn't let you override the Origin, so our default enforces the same-origin policy. It was inspired by a Go websocket implementation: https://godoc.org/github.com/gorilla/websocket#hdr-Origin_Considerations

check_origin_host could be used as check_request parameter when the server can handle websocket requests from different hosts.

This sounds useful, care to make a PR?

[zoggy]

Indeed the javascript API prevents setting the origin but I can still use Curl of any other library or tool to send any headers and pass the checks.

Here is a patch to add Websocket_lwt.check_origin: https://github.com/vbmithr/ocaml-websocket/pull/86