Correct WebSocket proxying

[ghost]

Subject to careful review. Not sure where to put self._record_activity(), whether self.set_status(400) is enough to send 400 Bad Request back to the client in case the backend does not support the WebSocket (or any other error happens) and should the ws_get be @web.authenticated.

Fixes #35

[ryanlovett]

I'm leary about abandoning the open() call. According to spec clients must open the connection as part of the handshake so if we don't open() the proxied service when the client calls open() on us, some clients might see problems. That being said, this patch worked for you so I need to do more research.

[ghost]

@ryanlovett from what you are pointing to in the spec, I conclude that you misinterpret what WebSocketHandler.open is and when it is called. It is called not when a client opens a connection to a server (before it sends a WS handshake), it is called only after WebSocketHandler responds to Upgrade request!

This is done in WebSocketProtocol13 class, which is instantiated by WebSocketHandler.
From WebSocketHandler.get:

        self.ws_connection = self.get_websocket_protocol()
        if self.ws_connection:
            self.ws_connection.accept_connection()

where get_websocket_protocol is defined as:

    def get_websocket_protocol(self):
        websocket_version = self.request.headers.get("Sec-WebSocket-Version")
        if websocket_version in ("7", "8", "13"):
            return WebSocketProtocol13(
                self, compression_options=self.get_compression_options())
#               ^^^^ passes self to WebSocketProtocol13, which is saved as `handler` in that class

WebSocketProtocol13.accept_connection invokes WebSocketProtocol13._accept_connection. Which answers to Upgrade request and then calls the open callback:

        self.handler.set_status(101)
        self.handler.set_header("Upgrade", "websocket")
        self.handler.set_header("Connection", "Upgrade")
        self.handler.set_header("Sec-WebSocket-Accept", self._challenge_response())
        self.handler.finish()

        self.handler._attach_stream()
        self.stream = self.handler.stream

        self.start_pinging()
        self._run_callback(self.handler.open, *self.handler.open_args,
                           **self.handler.open_kwargs)

[ryanlovett]

Yes, I had been thinking that open() was called when a client connects to the server. Thanks for the additional insight! Let me go over this a bit more.

If this is time-critical for you I can try to escalate -- just let me know.

[ghost]

If this is time-critical for you I can try to escalate -- just let me know.

No problem, if I needed it right now I'd use my fork. Anyway, for what I want to do with this proxy I need more changes than this bugfix.

[adsche]

Just a data point, I was trying this out because of #50 before I found the actual fix, and this pretty much broke nbnovnc (and I assume everything else that inherits LocalProxyHandler/SupervisorHandler and relies on its get to mix ws and http connections in one handler).

I did actually not check how hard that would be to fix, but if you merge this, it should maybe come with a warning about the API change...

What bug is this PR addressing btw?

Cheers!

[ghost]

this pretty much broke nbnovnc

The implementation of ws proxying is broken. If something like nbnovnc works it is either a pure luck or it was written to work with broken ws proxying and thus does not work with correct ws proxying.

and relies on its get to mix ws and http connections in one handler

get still mixes ws and http connections in one handler. It even does it more "directly".

What bug is this PR addressing btw?

The OP message explicitly says what this PR fixes.

[adsche]

this pretty much broke nbnovnc

The implementation of ws proxying is broken. If something like nbnovnc works it is either a pure luck or it was written to work with broken ws proxying

I don't believe this is true. nbnovn is really simple (1 file, <100 LOC), the only relevant overloaded function would be https://github.com/ryanlovett/nbnovnc/blob/master/nbnovnc/handlers.py#L88 and that is not working around anything.

I tried two echo servers, both work fine (after #51 also binary). I can also point nbserverproxy directly to a running websockify instance and it works just fine (noVNC and any other TCP socket, can easily be tested with nc).

and thus does not work with correct ws proxying.

In the same way, I can point my reverse proxy to those endpoints and it works no different than with nbserverproxy.

The OP message explicitly says what this PR fixes.

Yup, sorry about that. Would love to test xpra myself, but a dependency is not packaged for my platform.

Anyway, I'm out of here now. Good luck!

[ghost]

@adsche SuperviseAndProxyHandler overrides the open function, which was used incorrectly, see #38 (comment)
This is an example of how something was written to work with incorrect ws proxying implementation.

I'll add the fix for SuperviseAndProxyHandler. Or just make another PR fixing #1 and close this one.

[manics]

I'll close this since the author of this PR has deleted their account.