Don't follow redirects when path, sock, host or port are overridden

[aaugustin]

(from #544 (comment))

Since #528 websockets follows redirects.

I believe that this feature is incompatible with connection overrides (either with host + port or with sock). If you need to control exactly where you're connecting, I assume you aren't expecting to be redirected somewhere else.

This deserves documentation at a minimum, perhaps a warning or even an exception.

[aaugustin]

Here's what I think is possible:

• path is set (Unix socket connection): only same-origin redirects
• sock is set (already connected socket): if the server responds with connection: keep-alive, only same-origin redirects, else, no redirects — not sure I'll want to get into this level of detail for an edge case, I think it's fine to reject all redirects in that case EDIT: I don't want to get into this level of detail
• host or port is set (network address override): only same-origin redirects
• only uri is set: all redirects

[aaugustin]

Patch should look like:

diff --git a/src/websockets/client.py b/src/websockets/client.py
index 10435c1..a2a9f32 100644
--- a/src/websockets/client.py
+++ b/src/websockets/client.py
@@ -497,6 +497,16 @@ class Connect:
             old_wsuri.host == new_wsuri.host and old_wsuri.port == new_wsuri.port
         )

+        # Redirect isn't compatible with an already connected socket.
+        # (Same-origin redirect would be possible if the server keeps
+        # the connection alive, but let's keep things simple.)
+        if self._create_connection.keywords.get("sock") is not None:
+            raise InvalidHandshake(f"cannot redirect when sock is set")
+
+        # TODO - prevent cross-origin redirects when host and port are
+        # overridden or when path is set. Probably this requires keeping
+        # track of the initial situation in __init__...
+
         # Rewrite the host and port arguments for cross-origin redirects.
         # This preserves connection overrides with the host and port
         # arguments if the redirect points to the same host and port.

Writing this code and associated tests is a slightly boring, so I'll wait to see if someone actually hits this issue. Please add a comment if you do!

[aaugustin]

Well, it looks like demand for this feature is low...

[aaugustin]

Probably the pragmatic solution is to disable all redirects when any of path, sock, host, or port is provided and wait until someone asks for allowing same-origin redirects.

[SLiV9]

I ran into problems with unexpected cross-origin redirects, so I have created a PR that implements this: #1444

My use case is disabling cross-origin redirects for security reasons. I agree with your assessment that if you really care about the host you are connecting to, it makes sense to pass the host as an argument, and that avoids having a separate "disable redirects" parameter. Perhaps this needs to be mentioned in the documentation as well.

Thanks for the extensive issue description, it really simplified writing the PR. Please let me know if this needs anything else to be merged; I'll probably use my fork until then.

[aaugustin]

Supporting keep-alive connections (which is the default in HTTP/1.1 unless the server sends Connection: close) gets complicated quickly.

Probably, we could simply reuse the socket (passing it back with sock=...) in the not-TLS case. In the TLS case, if we keep ssl=..., then asyncio will redo the TLS handshake; if we remove it, then websockets won't realize that it's already a TLS socket and complain.

On top of that, we have to handle cases where host/port are set and unix connections. While it has to be possible to handle systematically all cases, this is a lot of work and complexity for a little to no benefit. No one complained that the current implementation doesn't reuse connections when it could.